fix(renderer): don't feed binary bodies to the HTML extractor - #485
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
6d85437 to
666f055
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
thanks for this, and for the writeup. the diagnosis is right: the escalation point is a good catch too. burning a camoufox session on a i have the branch checked out and am running it through the full pipeline now, not just the unit boundary. i will come back with findings either way. |
`http_only.rs` decides how to handle a response body from the declared
content type alone:
let is_pdf = content_type.as_deref() == Some("application/pdf");
let (html, raw_bytes) = if is_pdf { ... } else { decode_html_bytes(...) };
so *anything* that isn't literally `application/pdf` is assumed to be
text. Two consequences, both silent:
1. A `.docx`/`.xlsx`/`.pptx` (or any ZIP container) is UTF-8-lossy'd and
run through the HTML extractor. The caller gets `success: true` with
`markdown` beginning `PK\u{3}\u{4}...[Content_Types].xml`, 1 MB of
binary noise that is indistinguishable from a real scrape. An LLM
consumer will happily summarise it.
2. A real PDF served as `application/octet-stream` (common for S3 and
for `Content-Disposition: attachment` endpoints) misses the PDF
branch entirely and comes back as ~2 MB of `%PDF-1.5 ... /FlateDecode
... stream` source text instead of the parsed document.
This adds two cheap checks in the same place:
* `%PDF-` magic sniff. If the bytes are a PDF but the header disagrees,
relabel `content_type` so the existing PDF path, and the
`content_type == "application/pdf"` gate in `crw-crawl/src/single.rs`,
engage as they already do for correctly-labelled PDFs.
* NUL byte in the first 1 KB (git's binary heuristic) => the body is not
text. Skipped when the header declares a wide charset (utf-16/32,
ucs-*), whose HTML legitimately contains NULs.
Binary bodies now raise a new `CrwError::UnsupportedContentType`, mapped
to HTTP 422. It needs to be its own variant rather than `HttpError`
because `classify_renderer_error` maps `HttpError` to
`FailoverErrorKind::NetworkError`, which escalates: measured on a 1.3 MB
`.docx`, that climbs lightpanda -> chrome -> camoufox and burns 23.06s
and a Camoufox session before failing with the generic "Near-empty
content (61 bytes)". No browser turns a `.docx` into a page, so both
escalation gates in `lib.rs` now exclude the variant. Same document
after: 0.005s, HTTP 422, and an error naming the actual content type.
Measured against a local origin serving a real `.docx` and arXiv
1706.03762 mislabelled as `application/octet-stream`:
| target | before | after |
|------------------------|-------------------------------|--------------------------|
| PDF as octet-stream | success, 1,969,949 ch of raw | success, 40,701 ch of |
| | `%PDF-1.5 ... /FlateDecode` | article text, 0.286s |
| .docx | success, 1,019,506 ch of ZIP | 422 in 0.005s, no ladder |
Tests: 3 for `looks_binary` (ZIP flagged, HTML/empty passed, declared
wide charset respected) and one asserting the server maps the new
variant to 422 rather than falling through to the `_ => 500` arm.
The charset opt-out in `looks_binary` was a substring test on the header label while `decode_html_bytes` resolves it with `encoding_rs::Encoding::for_label`. The two sets differ: `unicode`, `csunicode`, `unicodefeff` and `unicodefffe` all map to UTF-16 and none of them matched. Measured on the same UTF-16 bytes, only the label differing: `charset=utf-16le` returned 200 with the page text, `charset=unicode` returned 422. Classic IIS emits `unicode`. The opt-out now asks `for_label` directly so the two cannot drift. `utf-32` leaves the list because WHATWG has no such encoding and `for_label` rejects it. The NUL test also ran on bodies the origin correctly declared as HTML. A stray NUL in HTML is not a reason to refuse a page, since the HTML5 tokenizer maps it to U+FFFD and browsers render it, and refusing would hand any origin a one-byte way to stop the renderer ladder at no cost to its human visitors. It is now skipped for a declared HTML-ish type, via the existing `is_html_like_content_type`. An absent or empty `Content-Type` is not a declaration and stays in scope, which is the case the byte sniff exists for. Also: * `is_pdf` is computed after the `%PDF-` relabel, so a sniffed PDF and a declared one are one branch rather than a disjunction repeated at each use, and `rendered_with` no longer contradicts the relabelled type. * the refusal is logged. It is the only path that returns a hard error without climbing the ladder, so a class of pages landing there by mistake has to be visible in production. * `crw scrape` no longer escalates on it. The CLI runs its own phase-2 escalation outside `FallbackRenderer`, so it still spawned LightPanda and Chrome for a .docx and printed "trying JS renderer" first. It returns `CmdError` rather than exiting directly, so teardown keeps owning the single exit path. Tests: two that count JS-renderer invocations to prove the ladder is not climbed, and a wiremock file driving `FallbackRenderer::fetch` against a real origin for the octet-stream relabel, the refusal, a body with no declared type, and a UTF-16 page. Deleting the call site, the relabel, either escalation guard, the charset opt-out or the HTML carve-out each fails at least one of them; the previous unit tests passed through all of it. Docs: the new code joins the two error tables, and the PDF page no longer says parsing needs an `application/pdf` response.
666f055 to
39c37ce
Compare
|
ran this through the full pipeline and it holds up. i reproduced both halves against a local origin first, on unmodified main as a baseline, same binary, same origin:
the octet-stream output matches the correctly-labelled control exactly, so the relabel really does hand the body to the same parser rather than to a second path. and reusing the i pushed a top-up to your branch, rebased onto current main. what it changes and why: the charset opt-out had to come from the decoder, not from a substring test. so a page that scrapes correctly today became a hard failure. the opt-out now asks the NUL test needed to stay off declared-HTML bodies. an html page with a stray NUL in its first 1 KB renders fine in a real browser, the HTML5 tokenizer maps NUL to U+FFFD. more importantly, because the escalation gates exclude the variant, a single
tests. the three smaller: one thing i should mention plainly: i reworded three characters in your commit message body. the repo has a hard no-em-dash rule and there were three in there; everything else, including authorship, is untouched. last piece, since merging here reaches production within the hour: i evaluated the guard predicate against the gate dataset itself,
|
|
merged in ab5e93e (yours) and 4f741d5 (the top-up), rebased onto main, no merge commit. worth saying since it is not obvious from outside: opencore is the engine and a merge to main reaches production on its own, so this is already on its way to the live api rather than waiting for a release. i will confirm it there. thanks again. two things about this report that made it easy to act on: you traced the actual branch that made the decision rather than describing the symptom, and you attached a before and after with real byte counts, which meant i could reproduce your exact claim on a baseline build instead of taking it on trust. the octet-stream case in particular is the kind of thing that hides forever, since it returns a confident 200 and megabytes of text that look like content until you read them. if you are looking for another one: |
|
wrote it up as #487, with the code path, the crawl-side helpers to reuse, and the one part that needs care (the job-completion gate keys off |
http_only.rsdecides how to handle a response body from the declared content type alone:so anything that isn't literally
application/pdfis assumed to be text. Two consequences, both silent:.docx/.xlsx/.pptx(or any ZIP container) is UTF-8-lossy'd and run through the HTML extractor. The caller getssuccess: truewithmarkdownbeginningPK\u{3}\u{4}...[Content_Types].xml— ~1 MB of binary noise indistinguishable from a real scrape. An LLM consumer downstream will happily summarise it.application/octet-stream(common for S3 and forContent-Disposition: attachmentendpoints) misses the PDF branch entirely and comes back as ~2 MB of%PDF-1.5 ... /FlateDecode ... streamsource text instead of the parsed document.Change
Two cheap checks in the same place:
%PDF-magic sniff. If the bytes are a PDF but the header disagrees, relabelcontent_typeso the existing PDF path — and thecontent_type == "application/pdf"gate incrw-crawl/src/single.rs— engages exactly as it already does for correctly-labelled PDFs.utf-16/utf-32/ucs-*), whose HTML legitimately contains NULs.Binary bodies raise a new
CrwError::UnsupportedContentType, mapped to HTTP 422.It needs to be its own variant rather than
HttpError:classify_renderer_errormapsHttpErrortoFailoverErrorKind::NetworkError, which escalates. Measured on a 1.3 MB.docx, that climbslightpanda → chrome → camoufoxand burns 23.06s and a Camoufox session before failing anyway with the genericNear-empty content (61 bytes). No browser turns a.docxinto a page, so both escalation gates inlib.rsexclude the variant. Same document after: 0.005s, 422, error naming the actual content type.crw-server'sIntoResponseneeded an explicit arm too — the_ => INTERNAL_SERVER_ERRORfallback would have reported a.docxas a server bug.Measured
Local origin serving a real
.docxand arXiv 1706.03762 mislabelled asapplication/octet-stream:octet-streamsuccess: true, 1,969,949 chars of raw%PDF-1.5 ... /FlateDecodesuccess: true, 40,701 chars of article text,content_type: application/pdf, 15 pages, 0.286s.docxsuccess: true, 1,019,506 chars of ZIP bytesUnsupported content type: application/vnd.openxmlformats-officedocument.wordprocessingml.document (1311881 bytes), no ladder climbThe PDF output is byte-identical to what crw already produces for the same paper served with a correct content type.
Tests
Three for
looks_binary(ZIP flagged, HTML and empty bodies passed, declared wide charset respected) and one asserting the server maps the new variant to 422 rather than falling through to_ => 500.cargo test -p crw-core -p crw-server --libgreen (495 / 274).crw-renderer --lib715 pass; the 4 failures there (connection_failure_catches_connect_timeout,is_retriable_error_false_for_connect_timeout,direct_blackhole_then_{refusing,hanging}_proxy_is_target_unreachable) reproduce identically on unmodifiedmainon this host — Windows refuses blackhole IPs instead of hanging — so they are environmental, not from this change.