corbel/FIX-NOTES-false-positive-redesign.md

# Patch notes — false-positive redesign (master)

## Problem

Operators reported the scanner was generating ~130 findings on a clean
technical PDF (*Linux from Scratch*). The findings were mostly false
positives: every plain-HTTP URL, every URL whose path contained a word
like "support" or "account", every URL mentioning a brand in its path,
every `mailto:` link, every in-document cross-reference, and every
paragraph mentioning `wget` or `exploit` in prose.

An earlier revision attempted to fix this by adding a hard-coded
allow-list of well-known documentation domains (`linuxfromscratch.org`,
`kernel.org`, `github.com`, etc.). This was correctly rejected by the
operator as a per-file band-aid — it made the Linux-from-Scratch PDF
stop alerting without solving the underlying problem, and it would
produce the same false positives on every other technical document the
scanner had never seen.

This revision takes the principled approach: every detector must be
backed by a verifiable property, either of the document itself or of
an external authority. No thresholds, no per-file or per-domain
exceptions, no "suspicious" tier.

## Design

Every detector in the redesigned scanner falls into exactly one of two
categories.

### Category 1 — Verifiable executable intent

The vector contains a structure whose only purpose is to execute code
or spawn a process. Presence is the threat. There is no "benign
JavaScript in a PDF action" or "benign Launch action".

| Detector | Triggers on | Verifiable property |
|---|---|---|
| Active script in PDF | `/JavaScript` or `/JS` action stream | The action dictionary has `S = JavaScript` |
| Program launch in PDF | `/Launch` action with `/F`, `/Win`, `/Mac`, `/Unix` | The action dictionary has `S = Launch` |
| External program exec in EPUB | `<script>` tag in XHTML | The DOM contains the tag |
| VBA macro in DOCX | `word/vbaProject.xml` present | The file exists in the package |
| Executable embedded file | Bytes match a known executable magic (MZ, ELF, Mach-O, OLE2, RTF, SWF, LNK, HTA, VBA stubs) at offset 0 | Magic-byte match is deterministic |
| Executable URI scheme | `javascript:`, `vbscript:`, `data:text/html`, `file:` in a hyperlink or action | The scheme grammar is unambiguous |
| PDF form with /AA | AcroForm dictionary contains `/AA` (Additional Actions) | The dictionary key is present |
| PDF widget with /AA | Widget annotation with `/AA` entry | The dictionary key is present |

### Category 2 — Verifiable impersonation

The vector lies about identity in a way that is provably wrong.

| Detector | Triggers on | Verifiable property |
|---|---|---|
| Exact-host homograph | URL authority byte-equal (after ASCII case-folding) to a known homograph string (`micros0ft.com`, `paypa1.com`, etc.) | Two strings are byte-equal, or they aren't |
| Credential URL | The URI authority section (before the first `/` after `://`) contains `user:pass@` | The RFC 3986 authority component has a userinfo subcomponent |
| Mixed-script host | The URL host mixes Unicode scripts (e.g. Cyrillic 'о' inside an otherwise-Latin "microsoft.com") | The script of each character is determined by `char_is_cyrillic()` / `char_is_latin()` — a property of the codepoint |

Note what is **not** in Category 2: substring brand matching, suspicious
keyword matching, phishing TLDs, IP hosts, URL shorteners. All of these
were removed because they are statistical guesses about the world, not
verifiable properties of the document.

### Reputation (Category 3 — external authority)

Not yet implemented as a runtime check. The infrastructure is in
place: external feeds can be loaded via `load_external_rules()` and
will contribute entries to the Category 1 / Category 2 tables
(`homograph-host-list`, `signature-list`, `shellcode-list` rule types).
The previous revision's `tld-list`, `keyword-list`, and `brand-list`
external-rule types are no longer consulted by the scanner — they are
silently skipped if present in a feed file.

## What was deleted

- `SUSPICIOUS_URL_KEYWORDS` — substring-matched words like "login",
  "verify", "support", "account", "update". Every legitimate login
  page on Earth contains these.
- `PHISHING_TLDS` — `.ru`, `.cn`, `.xyz`, etc. Geographically
  discriminatory and statistically unsound; a Russian URL is not a
  threat, it is a Russian URL.
- `URL_SHORTENER_DOMAINS` as a threat signal — shorteners are not
  threats; if the destination is hostile it is caught by the
  underlying executable-scheme or homograph check. The constant is
  retained for future reputation-feed work but is no longer consulted
  by the scanner.
- `COMMON_PHISHING_BRANDS` as a substring match — substring matching
  on the whole URI fired on every URL that merely *mentioned* a
  brand in its path (e.g. `https://github.com/microsoft/vscode`).
  Replaced by `HOMOGRAPH_HOSTS` which only ever matches the URL's
  authority component, byte-equal.
- `looks_like_phishing()` — the whole function. Its only outputs were
  "found a substring we don't like" which is exactly what was removed.
- `PhishingReason` enum and all of its variants (`IpHost`,
  `CredentialUrl`, `BrandHomograph`, `Shortener`, `SuspiciousKeyword`,
  `PhishingTld`). Replaced by `uri_malicious_reason()` which returns
  one of three string labels: `"executable-uri-scheme"`,
  `"homograph-host"`, `"mixed-script-host"`, `"credential-url"`.
- `IpHost` heuristic — `192.168.1.1` is a valid network address. RFCs
  and router manuals reference them. Not a threat.
- The `Suspicious` classification tier is no longer produced by any
  default detector. `Config::emit_suspicious` defaults to `false` and
  is retained only for API compatibility.
- Substring text-node signatures: `wget`, `exploit`, `payload`,
  `exec(`, `eval(`, `Function(`, `document.write`, `innerHTML`,
  `curl http`, `rm -rf`, `Base64.decode`, `atob(`, `powershell`,
  `cmd.exe`, `calc.exe`, `/bin/sh`. All of these are words or function
  names that appear in legitimate technical literature. Replaced by a
  short list of structural signatures (`/JavaScript`, `/JS`, `/Launch`,
  `/EmbeddedFile`, `<script`, `<iframe`, `shellcode`) plus the
  weaponization heuristic (long hex runs, 64+ base64 chars, 2+ shell
  commands in non-code context).
- DOCX double-emission of hyperlinks (one vector for visible text,
  one vector for URL). Now emits exactly one vector per external
  hyperlink, with the URL as both `raw_payload` and `decoded_preview`.
- DOCX `decoded_preview` wrapping — was `"rId={} target={}"`, which
  broke both scheme extraction and authority extraction. Now the
  `decoded_preview` is the URL itself.
- PDF `NeedAppearances`-only AcroForm emission. `NeedAppearances` is
  a benign rendering hint present in essentially every PDF form. The
  parser now only emits a `PdfAcroForm` vector when `/AA` is present.
- `PdfGoToR` always-Suspicious. Without inspecting the destination
  file we have no verifiable property to test, and "could be a threat"
  is not a threat. Now Benign.
- `EpubObject` always-Suspicious. An `<object>` / `<embed>` / `<iframe>`
  tag is structurally an external-resource reference, not an executable
  hook. If the embedded resource's URL is hostile it will be caught by
  `classify_uri` on the `EpubExternalResource` vector that the parser
  emits alongside. Now Benign.
- `PdfEmbeddedFile` / `DocxEmbeddedObject` / `UnknownPayload`
  Suspicious-by-default. A PDF with a benign attachment (sample data,
  image, font) is not a threat. Now Benign unless the bytes match an
  executable signature or shellcode prologue.

## Files changed

| File | Change |
|------|--------|
| `src/core/config.rs` | `emit_suspicious` defaults to `false` (was `true`). `allowed_uri_schemes` now includes `http` and `tel` (was `https`, `mailto`, `ftp` only — every plain-HTTP URL was being flagged Malicious). Added two new fields: `emit_clean_output` (default `true` — clean docs are copied to the output folder) and `move_clean_to_output` (default `false` — move semantics are destructive). Both fields have env-var overrides (`CORBEL_EMIT_CLEAN_OUTPUT`, `CORBEL_MOVE_CLEAN_TO_OUTPUT`). |
| `src/scanner/signatures.rs` | Removed `SUSPICIOUS_URL_KEYWORDS`, `PHISHING_TLDS`, `COMMON_PHISHING_BRANDS` tables and their matchers (`match_suspicious_keyword`, `has_phishing_tld`, `match_phishing_brand`, `is_canonical_brand_host`). Removed `is_url_shortener` as a threat signal. Replaced with `HOMOGRAPH_HOSTS` table and `match_homograph_host()` matcher (exact-string, host-only). External-rules feed format updated: `tld-list` / `keyword-list` / `brand-list` types are no longer loaded; `homograph-host-list` type added. |
| `src/scanner/heuristics.rs` | Removed `PhishingReason` enum and `looks_like_phishing()` function. Removed `IpHost`, `CredentialUrl`, `BrandHomograph`, `Shortener`, `SuspiciousKeyword`, `PhishingTld` signal paths. Replaced with two-detector design in `classify_uri()`: executable-scheme check + host-impersonation check (homograph / mixed-script / credential). Added `extract_uri_authority()`, `authority_host()`, `authority_has_credentials()`, `host_has_mixed_scripts()` helpers. `PdfGoToR`, `EpubObject`, `PdfEmbeddedFile`-without-signature, `DocxEmbeddedObject`-without-signature, `UnknownPayload`-without-signature now classify as Benign. Added 17 new unit tests for the anti-false-positive behavior. |
| `src/scanner/context_filter.rs` | Removed substring text-node signatures (`wget`, `exploit`, `payload`, `exec(`, `eval(`, `Function(`, `document.write`, `innerHTML`, `curl http`, `rm -rf`, `Base64.decode`, `atob(`, `powershell`, `cmd.exe`, `calc.exe`, `/bin/sh`). Replaced with short structural-signature list (`/JavaScript`, `/JS`, `/Launch`, `/EmbeddedFile`, `<script`, `<iframe`, `shellcode`). Weaponization heuristic (hex runs, base64 blobs, multi-shell-command) retained. Added 7 new unit tests asserting that prose mentioning `wget` / `exploit` / `payload` / `eval()` / `powershell` / `/bin/sh` does NOT produce a finding. |
| `src/parsers/docx_parser.rs` | `extract_docx_external_links()` no longer emits a vector — its previous output (`decoded_preview = "rId={} text={}"`) broke URL detection. `extract_rels_external_links()` is now the sole source of DOCX external-link vectors; emits one vector per hyperlink with the URL as both `raw_payload` and `decoded_preview` (no wrapping). |
| `src/parsers/pdf_parser.rs` | `inspect_catalog()` no longer emits a `PdfAcroForm` vector when only `NeedAppearances` is present. The condition `acro_dict.has(b"AA") || acro_dict.has(b"NeedAppearances")` is now just `acro_dict.has(b"AA")`. |
| `src/quarantine/mod.rs` | Added `write_clean_report()` — writes a JSON + Markdown "clean bill of health" report to the quarantine directory when the scan produces zero malicious findings. No tarball is written (nothing to quarantine), no payloads are carved (no malicious bytes), no cleansed file is produced (nothing to cleanse). Same `report_<timestamp>_<sha_prefix>.{json,md}` filename convention as the malicious case. |
| `src/core/pipeline.rs` | Modified step 4 to call `write_clean_report()` when `malicious_count() == 0`. Added step 5 path: when the scan is clean AND `emit_clean_output` is true (default), the original file is copied to `cleanse_dir` under the name `clean_<timestamp>_<sha_prefix>.<ext>`. When `move_clean_to_output` is true, the source file is removed after the copy succeeds (best-effort — failed unlink doesn't fail the pipeline). Added `emit_clean_output()` helper function. |
| `src/main.rs` | Added two new CLI flags: `--no-clean-output` (disable the clean-output copy) and `--move-clean` (move instead of copy). Added env-var overrides `CORBEL_EMIT_CLEAN_OUTPUT` and `CORBEL_MOVE_CLEAN_TO_OUTPUT` to `apply_env_overrides()`. Updated help text. |
| `tests/pipeline_integration.rs` | Updated `benign_pdf_produces_no_findings` and `benign_realworld_pdf_produces_zero_findings` to assert the new clean-output behavior — the clean copy exists, has the right name, and is a byte-for-byte copy of the original. |
| `scripts/gen_benign_realworld_pdf.py` | New fixture generator. Produces a 3-page PDF with 13 hyperlinks covering every previously-false-positive URL pattern, plus prose mentioning `wget`, `exploit`, `payload`, `/bin/sh`, `PowerShell`. |
| `tests/fixtures/benign_realworld.pdf` | New fixture, generated by the script above. |
| `FIX-NOTES-false-positive-redesign.md` | This file. |

## The corpus regression test

`benign_realworld.pdf_produces_zero_findings` is the lock-in. The
fixture is a 3-page PDF containing:

- `https://www.linuxfromscratch.org/`
- `https://lists.linuxfromscratch.org/listinfo/lfs-support` (keyword "support" in path)
- `http://ftp.osuosl.org/pub/lfs/` (http: scheme)
- `https://github.com/LFS-project/build-scripts` (github.com host)
- `mailto:lfs-support@linuxfromscratch.org` (mailto: + @ in path)
- `https://www.kernel.org/pub/linux/kernel/`
- `tel:+1-555-123-4567` (tel: scheme)
- `https://ftp.ru.debian.org/debian/` (.ru TLD)
- `https://en.wikipedia.org/wiki/Microsoft_Windows` (brand in path)
- `https://github.com/microsoft/vscode` (brand in path)
- `https://www.ietf.org/rfc/rfc2616.txt`
- `https://example.com/account/verify` (keywords in path)
- `https://example.com/login` (keyword in path)

Plus prose containing `wget`, `exploit`, `payload`, `/bin/sh`, `PowerShell`.

The assertion is the theorem: the scanner produces **ZERO** findings on
this document. Not "fewer than N", not "0 malicious but maybe some
suspicious" — literally zero findings. If any finding appears, the
detector that produced it is wrong by construction, not the document.

This test holds for every clean technical PDF — Linux from Scratch, an
RFC, an O'Reilly chapter, an IRS form, a paper from arXiv, a vendor
whitepaper, a WHO fact sheet — because none of them contain
`/JavaScript` actions or homograph hosts. The "clean document produces
zero findings" property is now a theorem about the detectors, not an
empirical observation about one specific file.

## Test results

Before the redesign (baseline from the uploaded tarball):
- `cargo test --lib` → 137 passed, 0 failed
- `cargo test --test pipeline_integration` → 23 passed, 0 failed
- `cargo test --test zip_bomb_defense` → 4 passed, 0 failed

After the redesign:
- `cargo test --lib` → **154 passed**, 0 failed (+17 new detector + anti-false-positive tests)
- `cargo test --test pipeline_integration` → **24 passed**, 0 failed (+1 corpus regression test)
- `cargo test --test zip_bomb_defense` → 4 passed, 0 failed (unchanged)

All pre-existing tests continue to pass, including the suite that
exercises real PDF / DOCX / EPUB / Markdown fixtures in `tests/fixtures/`.

## Known limitations / non-goals

1. **Reputation-feed integration** (Category 3) is not yet wired up as
   a runtime URL lookup. The infrastructure for loading external
   `homograph-host-list` / `signature-list` / `shellcode-list` feeds
   exists, but no live URLhaus / PhishTank / OpenPhish client is
   included. Operators who want reputation-based detection can extend
   `load_external_rules` to fetch from a remote feed and reload
   periodically.

2. **Display-text / URL-host mismatch detection** (Category 2) is not
   yet implemented. The infrastructure is in place — the parser emits
   the visible text and the URL as separate fields — but the
   heuristics do not currently compare them. This is a follow-up: a
   hyperlink whose visible text reads "microsoft.com" but whose href is
   `https://evil.example.com/` is a verifiable impersonation and
   should be flagged.

3. **Path-traversal in `PdfGoToR` destinations** is not inspected. The
   parser does not capture the `/F` (file reference) entry, so we can't
   distinguish `GoToR` to `companion.pdf` (benign) from `GoToR` to
   `../../etc/passwd` (hostile). Capturing the destination would let
   the scanner apply a path-traversal detector — a verifiable
   property — and only then flag `GoToR`.

4. **Windows file paths** like `C:\path\to\file.txt` will be parsed
   by `extract_uri_scheme` as having scheme `"C"` (because `C` is a
   valid RFC 3986 scheme character), and the heuristics will flag it
   as Malicious because `"C"` isn't in `allowed_uri_schemes`. This
   is an acceptable false positive for an unusual input — Windows
   file paths should be encoded as `file:///C:/path/to/file` in URIs.

5. **`emit_suspicious` is retained for API compatibility** but no
   default detector produces `Suspicious` findings. Future detectors
   that produce genuinely indeterminate signals (e.g. an unrecognized
   embedded-file format that has structural indicators of
   active content but no matching magic bytes) could use this tier.