fix(researcher): give source ingestion a work budget (WALM-683) by nikola0x0 · Pull Request #985 · MystenLabs/MemWal

nikola0x0 · 2026-09-22T10:15:17Z

Part of WALM-683 — deliberately not Closes. One acceptance criterion stays open after this merges: "concurrent and repeated requests cannot exceed the configured budget" is only half met, since per-user concurrency is not implemented. See Not covered, below. Triage of the 22 Sep static security review (WALM-679); MW-04 confirmed against source.

What was wrong

POST /api/research/process-source authenticated the caller and then did unbounded work. The PDF branch checked that a filename ended in .pdf and never checked a size; the URL branch buffered whatever Jina returned; the shared pdf-url path buffered the whole download. processSource then ran metadata generation, chunked the full text, and embedded every chunk — batchEmbed caps a batch at 100 entries but not the number of batches.

No rate limit applied. lib/ratelimit.ts was already wired to the chat and auth routes and simply never reached this one. maxDuration = 120 bounds one request, not how many a caller starts or what each one spends.

The fix

New lib/rag/ingest/limits.ts. The caps sit in processSource rather than in the route, so the chat path inherits them too — a route-only budget would have left the larger caller unbudgeted.

20MB per source, enforced before buffering. readCappedBytes treats Content-Length as a claim: it rejects early when overstated, but the running total is what stops the read, so a missing or understated header cannot get more through. The reader is cancelled on rejection rather than left transferring.
1M extracted characters, applied to every branch at one point.
600 chunks per source, bounding embedding spend directly, since chunk count varies with document structure rather than byte count.
5 sources per chat message.
Per-IP rate limiting on the route, reusing the existing limiter.

The multipart value is now checked instead of cast: formData.get("file") as File accepted a plain text field, which reached .name as undefined. Content is checked too — %PDF- magic bytes, not the filename suffix.

All five thresholds read from env (RESEARCH_MAX_*) so they can be tuned without a deploy. Defaults are generous for real documents; a 250-page text PDF sits well inside them.

After review (Henry, Harry)

Upload budget enforced where the bytes arrive (0c28cfbb). Next drains every proxied request body to EOF, even when the proxy returns early, and truncates it at 10MB. So this route is out of the proxy matcher and reads its own raw stream under the cap. Auth is still getSession() (jwtVerify + user lookup), same as /api/auth/*.
Page-by-page extraction, 500-page limit, stops at the character budget (0c28cfbb).
Own rate-limit bucket (ingest-rate-limit, 20/hour/IP), so uploads don't spend chat quota. Attachments are kept over scraped URLs, and dropped sources are reported (0c28cfbb).
Decompression-bomb guard (bfb4489d). Before pdf.js runs, every compressed stream is inflated with the output counted and discarded, and the file is refused past the budget. Anything the guard can't bound is refused: Flate behind another filter, LZW, indirect or unknown filters, encrypted files. It's hardened against a fake endstream inside the bomb, #xx-escaped filter names, and the lenient parsing pdf.js accepts.

❓ Needs the team to confirm

The guard's limits are a judgement call, so they ship with defaults and can be changed through env without a deploy:

Setting	Default	Env
Decompressed size per stream	64MB	`RESEARCH_PDF_MAX_INFLATED_BYTES_PER_STREAM`
Decompressed size per file	256MB	`RESEARCH_PDF_MAX_INFLATED_BYTES_TOTAL`
Streams per file	20,000	`RESEARCH_PDF_MAX_STREAMS`
Encrypted PDFs	refused	—

For scale: text content is kilobytes to a few MB per stream. What these limits would refuse are PDFs with very large lossless images (a 4000×3000 RGB image is ~36MB decompressed). Refusing encrypted PDFs also refuses the "protected" ones that open without a password, which some publishers ship. That's a product call, because the guard can't measure what it can't decrypt.

Not covered

Per-user concurrency. Repeated and parallel requests are bounded per IP and each request's own work is capped, but a true concurrency budget needs shared state beyond the counter this limiter keeps. Flagging it rather than letting the ticket look fully closed.

Verification

11 new tests: streaming cap with no Content-Length, understated Content-Length, cancellation on rejection, non-File form value, oversized upload, magic-byte check, character cap. Researcher suite 55 pass, build clean.

`POST /api/research/process-source` authenticated the caller and then did unbounded work. The PDF branch checked that a *filename* ended in .pdf and never checked a size; the URL branch buffered whatever Jina returned; the shared pdf-url path buffered the whole download. `processSource` then ran metadata generation, chunked the full text, and embedded every chunk — `batchEmbed` caps a batch at 100 entries but not the number of batches. No rate limit applied: `lib/ratelimit.ts` was already wired to the chat and auth routes and simply never reached this one. `maxDuration = 120` bounds one request, not how many a caller starts or what each one spends. New `lib/rag/ingest/limits.ts` holds the budget, and the caps sit in `processSource` rather than in the route so the chat path inherits them too — a route-only budget would have left the larger caller unbudgeted: - 20MB per source, enforced before buffering. `readCappedBytes` treats Content-Length as a claim: it rejects early when overstated, but the running total is what stops the read, so a missing or understated header cannot get more through. The reader is cancelled on rejection rather than left transferring. - 1M extracted characters, applied to every branch at one point. - 600 chunks per source, bounding embedding spend directly, since chunk count varies with document structure rather than with byte count. - 5 sources per chat message. - Per-IP rate limiting on the route, reusing the existing limiter. The multipart value is now checked instead of cast: `formData.get("file") as File` accepted a plain text field, which reached `.name` as undefined. Content is checked too — `%PDF-` magic bytes, not the filename suffix. All five thresholds read from env (RESEARCH_MAX_*) so they can be tuned without a deploy. Defaults are generous for real documents; a 250-page text PDF sits well inside them. Not covered here: per-user concurrency. Repeated and parallel requests are bounded per IP and each request's own work is capped, but a true concurrency budget needs shared state beyond the counter this limiter keeps. Called out rather than implied. Verified: 11 new tests (streaming cap with no Content-Length, understated Content-Length, cancellation on rejection, non-File form value, oversized upload, magic-byte check, character cap). Researcher suite 55 pass, build clean.

ducnmm

Summary

readCappedBytes does not trust Content-Length: a missing or understated header is stopped by the running total, and the reader is cancelled. Jina and the pdf-url branch use it. processSource then caps extracted text at 1M characters and refuses embedding past 600 chunks, for both chat and POST /api/research/process-source.

Those caps run too late. The upload size check runs after the body has already been read, and proxy.ts drains that body before the route and before the new IP limit. PDF parsing still inflates the file before the character cap.

Issue counts by severity

bugs: 2
suggestions: 1
nits: 1

…(WALM-683) Review follow-up on #985. Henry's review was right that the caps ran too late; confirmed against Next 16.3.5's own source. The upload cap never saw an intact oversized body. The route called `request.formData()`, which buffers everything, and only then read `File.size`. Worse, `proxy.ts` matches `/api/:path*`, and for every request the proxy sees Next clones the body and drains it to EOF — `next-server.js` awaits `body.finalize()` in a `finally`, so even a proxy that answered 413 would still read the whole upload — then hands the route a copy truncated at `proxyClientMaxBodySize` (10MB, unset here). A 12MB PDF inside the 20MB budget arrived cut short and failed the magic-byte check instead. The fix the review suggested — abort from the proxy — is not expressible: the proxy gets no handle on the socket. So this route leaves the proxy matcher, and reads its own raw stream under the cap with `readCappedRequestBody`, which cancels (and so closes) the upload the moment the running total passes the budget. Multipart and JSON are parsed only from those capped bytes; the JSON branch gets a 16KB cap of its own. Auth is unchanged in strength: the route's `getSession()` runs the same `jwtVerify` plus a user lookup, which is how `/api/auth/*` already works outside the proxy. The rate limiter now runs before a single body byte is read. Extraction decoded every page before the character cap could apply. `extractText(..., { mergePages: true })` pulled all pages in parallel and joined the string, so the 1M-character cap bounded what was kept, not what was decoded. Extraction now goes page by page through `getDocumentProxy`, refuses more than 500 pages before opening any, stops as soon as the character budget is spent, and destroys the document. Also: - Ingestion has its own rate-limit bucket (`ingest-rate-limit`, 20/hour per IP, RESEARCH_MAX_INGESTS_PER_HOUR). Sharing the chat key meant each upload spent one of ten hourly chat messages. (Harry) - Chat source selection keeps attachments ahead of URLs scraped from prose, and every source not started is reported on the stream as `data-source-error` rather than only logged. URLs were collected first, so five cited links silently dropped an uploaded PDF. - Bodies are released before throwing on `!response.ok` in both the Jina and pdf-url paths, so an error response with an endless body no longer holds the socket. Still open, deliberately: a single-page Flate bomb. Page-by-page reading bounds decoding across pages, but one page's content stream still inflates inside pdf.js with no output ceiling. That needs either a hard memory limit or process isolation around extraction, which is a design decision rather than a review fix. Verified: 8 new tests, including one that builds a 30-page PDF and runs it through pdf.js to show decoding stops early, a stream test showing an oversized upload is cancelled after a few chunks rather than read to the end, and a matcher test through Next's own `unstable_doesMiddlewareMatch` showing only this route leaves the proxy. Researcher suite 63 pass, build clean.

ducnmm

Summary

The follow-up reads the upload under a byte cap before multipart or JSON parsing, extracts PDFs page by page, and puts ingestion on its own Redis key. Attachments are kept ahead of scraped URLs, and non-OK download bodies are cancelled.

Two holes remain. Percent-encoded spellings of /api/research/process-source still match the proxy, so Next drains those bodies before the new cap. A chat source dropped for the cap emits data-source-error only, and the activity UI never shows it.

Issue counts by severity

bugs: 2
suggestions: 1
nits: 1

Also

[nit] apps/researcher/lib/rag/ingest/limits.ts:69 — describeLimit floors to whole megabytes, so the 16KB JSON cap is reported as "larger than the 0MB limit". Format a limit under 1MB in KB.

ducnmm · 2026-09-23T05:41:05Z

    "/",
    "/chat/:id",
-    "/api/:path*",
+    "/api/((?!research/process-source(?:/|$)).*)",


[bug] The matcher exclusion is only the literal path. Next tests the matcher against pathname before decoding, then decodeURIComponents the path onto this route. unstable_doesMiddlewareMatch on this config returns true for /api/research%2Fprocess-source, /api/research/%70rocess-source, and /%61pi/research/process-source (the same lookahead is repeated on line 67). Once the matcher hits, Next clones the body and reads it to EOF before proxy() runs, which is the drain this commit moved the route out of the matcher to avoid. The new test only asserts the literal path and a trailing slash.

A 20MB-legal PDF sent to the encoded URL is still truncated at the default proxyClientMaxBodySize (10MB), and a larger body is still read to completion before readCappedRequestBody can cancel.

Suggestion: The negative lookahead has to reject every single-encoding of that path, in both matchers: each character as the literal or % plus both hex cases, and each / also as %2F / %2f. decodeURIComponent runs once, so double-encoding does not reach the route. Add those URLs to the unstable_doesMiddlewareMatch test and assert they do not match. Returning early from proxy() cannot fix a request the matcher already accepted.

Confirmed and fixed in d5c6a05. Through unstable_doesMiddlewareMatch, all the spellings you listed matched, plus %2D and a trailing %2F. Both lookaheads now take each character as itself or %XX in either hex case, / as %2F/%2f, and the terminator allows %2F too. The test generates every single-encoding of the path (40), both fully-encoded forms and the %2F variants, and asserts none match; it also checks that neighbours like /api/research/%70rocess-sourcex still do. The pattern is spelled out as a literal because Next analyses matchers statically. The rule is in a comment above it.

ducnmm · 2026-09-23T05:41:05Z

+            const { kept, dropped } = selectSourcesWithinBudget(sources);
+            for (const source of dropped) {
+              dataStream.write({
+                type: "data-source-error",


[bug] Dropped sources are written as data-source-error only. useResearchActivity creates a source step only from a processing event, then looks up an error with the same label. Nothing else reads these events. A kept source emits data-source-processing before its error, so failures on that path still show. A source past the cap never gets a processing event, the handler stores an orphan error, and the activity panel does not render it. The comment above this loop says a dropped PDF would otherwise never show up. It still does not.

Suggestion: Write data-source-processing for the same label before this error, or render an error event that has no matching processing event. Keep the label as the url or file name, which is what the error payload already uses.

Fixed in d5c6a05. Dropped sources now emit data-source-processing then data-source-error with the same label (url or file name), via droppedSourceEvents. I checked useResearchActivity and it builds the step exactly as you describe, so the orphan error is gone.

ducnmm · 2026-09-23T05:41:05Z

+          ? String((item as { str: unknown }).str)
+          : ""
+      )
+      .join(" ");


[suggestion] collectPageText replaces unpdf getPageText. That helper keeps items with str, appends a newline when hasEOL is set, and joins with "". This joins every item with a space, maps marked-content items to "" (another space), and drops hasEOL. Kerning splits that used to concatenate are now separated, and line breaks are lost, so the string that is capped and embedded is not the string extractText produced. The new pdf.js test uses one text run per page, so it does not see the join.

Suggestion: Use the same item rule as getPageText: item.str != null, append \n when hasEOL is set, join with "".

Right, fixed in d5c6a05. One nuance: mergePages then collapses all whitespace (replace(/\s+/g, " ")), so the old string didn't keep line breaks either. hasEOL still matters, since without it foo+bar across a line becomes foobar. collectPageText now reproduces extractText(..., { mergePages: true }) exactly, still page by page. Rather than re-derive the rule, the test pins parity against extractText itself. The PDF has kerned runs, a font change mid-word, whitespace runs, an empty page and a page break. Worth noting: this pdf.js version already merges kerned TJ runs and adjacent Tjs into one item, so the realistic split is a font change (/F2 mid-word). My previous join gave Sepa rate there.

…s them (WALM-683) Review follow-up on #985 — the part the previous commit left open. One PDF page can hold a Flate stream that decompresses ~1030:1, so a 1MB upload expands to a gigabyte. pdf.js inflates a stream in full, doubling its buffer with no ceiling, before returning any text, so the byte, page and character caps never see it and the replica goes down with every request on it. JavaScript cannot put a memory ceiling around pdf.js, so the new guard measures instead. Before pdf.js sees the file, every compressed stream is inflated with the output counted and discarded, and the file is refused if any stream, or all of them together, would expand past the budget. Measuring a 512MB bomb against the 64MB cap takes ~60ms and a few MB. Anything the guard cannot bound is refused, because anything it lets through unmeasured is something pdf.js will inflate: - A stream is measured from its first byte to wherever the deflate data itself ends, not to `/Length` or `endstream`. A bomb can plant a fake `endstream` inside a stored block of its own data; a boundary-trusting scan would have measured a few harmless bytes. - Flate and RunLength (the expanding filters) are accepted only as a stream's first filter. Behind ASCII85, the raw bytes are not zlib, the scan would measure nothing, and pdf.js would still inflate the result. - LZW, Crypt, indirect `/Filter` references, and unknown filter names are refused. Filter names are `#xx`-decoded first: `/Flat#65Decode` is `/FlateDecode` to pdf.js. - Stream detection is as lenient as pdf.js's own parser — comments before the keyword, junk after it, CR-only line endings — so there is no stream pdf.js reads that the scan misses. - Encrypted PDFs are refused: their bytes are ciphertext, while pdf.js decrypts with an empty password and then inflates. Budgets default to 64MB per stream, 256MB per file and 20,000 streams, all env-tunable (RESEARCH_PDF_MAX_INFLATED_BYTES_PER_STREAM, RESEARCH_PDF_MAX_INFLATED_BYTES_TOTAL, RESEARCH_PDF_MAX_STREAMS). The numbers, and refusing encrypted PDFs, are for the team to confirm. Verified: 17 tests. A real Flate-compressed PDF still passes and extracts through pdf.js; the 256MB bomb is refused at the default budget in well under 500ms; the fake-endstream bomb is refused while the test proves the prefix a trusting scan would see inflates to a few bytes; plus each refused case and each lenient-parsing case. The tests never hand pdf.js a bomb. Researcher suite 80 pass, build clean.

… output unchanged (WALM-683) Review round 2 on #985 (Henry). Encoded spellings of the upload route still went through the proxy. The matcher exclusion named only the literal path, while Next tests matchers against the raw pathname and percent-decodes it only when routing — so `/api/research/%70rocess-source`, `/%61pi/...` and `.../research%2F...` all reach this route after the proxy has cloned and drained the body, which is the drain the route left the proxy to avoid. Both lookaheads now reject every single-encoding: each character as itself or `%XX` in either hex case, `/` as `%2F`/`%2f`, and a trailing `%2F`. Decoding happens once, so a double-encoded path never routes here. Matchers must be static literals, so the pattern is spelled out, with the rule in a comment. Dropped chat sources were invisible. The activity panel builds a source's step from `data-source-processing` and only then attaches a matching `data-source-error`; a lone error was stored and never rendered. Dropped sources now emit both, in that order (`droppedSourceEvents`). Page-by-page extraction changed the text that gets embedded. It joined items with " ", so a word pdf.js returns as two items — which a font change mid-word does — became "Sepa rate", and runs of whitespace survived. `collectPageText` now reproduces `extractText(..., { mergePages: true })` exactly: items with `str`, "\n" on `hasEOL`, joined with "", pages joined with "\n", whitespace collapsed — computed page by page so the character budget still stops early. Nit: a limit under 1MB is reported in KB; the 16KB JSON cap read "0MB". Verified: the matcher test covers all 40 single-encoded spellings, both fully-encoded forms and the %2F variants through Next's own `unstable_doesMiddlewareMatch`, and checks neighbouring routes still match. A parity test compares against unpdf's `extractText` on a PDF with kerned runs, a font change mid-word, whitespace runs, an empty page and a page break; it fails on the previous commit's join. Researcher suite passes, build clean.

nikola0x0 · 2026-09-23T06:16:18Z

Round 2 nit (describeLimit) fixed in d5c6a05: limits under 1MB are reported in KB, so the JSON cap reads "16KB limit". Also since your round-2 review: bfb4489 adds the decompression-bomb guard for the single-page Flate case, which is still waiting on your first look. Its limits are in the PR description for the team to confirm.

ducnmm

Summary

d5c6a051 closes the previous round: the proxy matcher now rejects single-encoded spellings of the upload path, dropped chat sources emit processing then error, page text matches extractText, and sub-megabyte limits are reported in KB.

The new decompression guard still lets streams through that pdf.js inflates. Confirmed against this guard and unpdf's pdf.js: /F and #xx filter keys, a decoy /Filter in a string or comment, RunLength wrapped around Flate, a zlib header with CINFO 8–15, and a Predictor Columns row buffer. extract.ts only skips pdf.js when the guard throws.

The 64MB / 256MB / 20,000 defaults and refusing encrypted PDFs are left as the product call in the PR body.

Issue counts by severity

bugs: 4
suggestions: 0
nits: 0

ducnmm · 2026-09-23T06:28:09Z

+    const objAt = text.lastIndexOf("obj", match.index);
+    const dict = text.slice(objAt === -1 ? 0 : objAt, match.index + 2);
+
+    const filter = /\/Filter\s*(\[[^\]]*\]|\/[^\s/<>[\]()]+|\d+\s+\d+\s+R)/.exec(


[bug] Filter identity is a regex for a literal /Filter on the raw bytes between the last obj and >>. pdf.js does not do that. Parser.filter uses dict.get("F", "Filter"), so /F wins over /Filter, and Lexer.getName decodes #xx before the key is stored. Tokens inside strings and comments count for the regex and not for pdf.js.

With a 64KB cap, all of these were allowed, and extractText returned the Flate payload: /F /FlateDecode, /#46ilter /FlateDecode, /#46 /FlateDecode, /Filter /DCTDecode /F /FlateDecode (the guard keeps the passive DCT and skips), and a /Filter /DCTDecode decoy inside a literal string or a % comment placed before the real /Filter /FlateDecode.

The same spelling gap is on the encryption check at line 237 (/\/Encrypt\b/, no name decode). A trailer /#45ncrypt 5 0 R pointing at a Standard encrypt dict is allowed; pdf.js throws PasswordException. Literal /Encrypt is still refused.

Suggestion: Resolve names the way pdf.js does before deciding: #xx-decode every name, treat /F as the filter key with get("F","Filter") precedence, and do not take /Filter or /Encrypt from inside strings or comments. If the dictionary cannot be parsed that far, refuse.

Fixed in 753c7bd. Not by patching each case, though: the regex is gone. The guard now tokenises like pdf.js's Lexer (strings, comments, #xx in names), takes dict.get("F", "Filter") and ("DP", "DecodeParms") precedence, keeps a repeated key's last value as Dict.set does, and treats /#45ncrypt as /Encrypt. Each case you listed is a test: /F, /#46ilter, /#46, /Filter /DCTDecode /F /FlateDecode, decoys in a string and in a comment, and /#45ncrypt 5 0 R in the trailer. One thing I found while reading pdf.js rather than in your review: fetchCompressed parses object streams with allowStreams: true and accepts any stream with /First and /N, so a stream compressed inside another was invisible to a raw scan. Those are decoded now and refused if they hold a stream.

ducnmm · 2026-09-23T06:28:09Z

+    const remaining = limits.total - total;
+    const budget = Math.min(limits.perStream, remaining);
+
+    const size = FLATE.has(filters[0])


[bug] An expanding filter is refused only when the first one is not at index 0. pdf.js applies the array in order, so a later Flate or RunLength still runs. /Filter [/RunLengthDecode /FlateDecode] only hits runLengthSize on the raw bytes. RunLength-literal wrapping of deflate(200KB of zeros) is 224 bytes; the guard allows it under a 64KB cap, and the same zlib with /Filter /FlateDecode alone is refused. pdf.js still opens the page. /Filter [/FlateDecode /RunLengthDecode] measures only the flate stage (22 bytes) of a RunLength program that expands to 102400 bytes.

Suggestion: Refuse a stream with more than one expanding filter, or measure the output after every stage against the same caps. A single leading Flate or a single leading RunLength can stay the only accepts.

[bug] DecodeParms is never read. For /Filter /FlateDecode plus a Predictor, pdf.js uses PredictorStream and ensureBuffers Columns * Colors * BitsPerComponent with no ceiling. That size is not the zlib output. A 12-byte flate stream with /DecodeParms << /Predictor 15 /Columns 32000000 >> was allowed under a 1KB cap, and getTextContent grew RSS by about 31MB. Indirect or non-integer Columns / DecodeParms are also invisible here and still resolved by pdf.js.

Suggestion: When Predictor is greater than 1, compute the row size and refuse if that row (or Rows times the row) exceeds the per-stream cap. If those values are missing, indirect, or not integers, refuse.

Fixed in 753c7bd, both. At most one expanding stage now (Flate or RunLength), and it must follow only ASCIIHex/ASCII85 stages, which are decoded the way pdf.js decodes them. That's needed anyway: real TeX Live files use [/ASCIIHexDecode /FlateDecode]. [/RunLengthDecode /FlateDecode] and [/FlateDecode /RunLengthDecode] are refused. ASCII85 after an expanding stage is refused too, since z is 4:1. Predictor: rowBytes = ceil(Columns × Colors × BPC / 8), with BPC before BitsPerComponent and pdf.js's || defaults, is checked against the per-stream cap whatever the data holds. Indirect or non-integer Predictor/Columns/Colors/BPC is refused, and so is a DecodeParms that is itself a reference, in both the name and array forms. Your Columns 32000000 case is a test.

ducnmm · 2026-09-23T06:28:09Z

+    inflate.on("end", () => finish(false));
+    // A corrupt or non-zlib stream stops where pdf.js's own inflater would stop
+    // too; what was produced before the error is what it could have produced.
+    inflate.on("error", () => finish(false));


[bug] An inflate error resolves { exceeded: false } with whatever was produced, and the caller at line 281 keeps only .size. pdf.js FlateStream checks method 8, FCHECK, and the FDICT bit, then inflates with its own decoder; it does not reject CINFO above 7. Node zlib does (invalid window size) and emits no output, so the guard records 0 and allows the stream.

A stored-block body whose zlib header is CMF 0x88 or 0xf8 (CINFO 8 and 15, FCHECK valid, FDICT clear) and whose payload is the text GUARDPROBE was allowed, and extractText returned that text. The new tests never use a non-default window, so a 0-byte error looks like a harmless stream.

Suggestion: Refuse when inflate errors before a clean end. A 0-byte error result is unmeasured, not proof the stream is small.

Fixed in 753c7bd by removing zlib from measuring altogether. Your CINFO case was one symptom; the other showed up on real files. pdf.js's inflater doesn't bounds-check back-references (e[o]=e[o-l] reads zero and carries on), so zlib's 'distance too far back' refused europecv examples that pdf.js reads fine. Measuring is now a port of pdf.js's FlateStream.readBlock that counts instead of storing. It uses the same header bits (method, FCHECK, FDICT, not CINFO), the same tolerance, and the same stopping points, and counts a stored block at its claimed length as pdf.js allocates it. So nothing is 'unmeasured' any more: the count is what pdf.js would allocate before finishing or throwing. It matches zlib byte for byte on valid data (stored, fixed and dynamic blocks), and there are tests for each quirk, including a hand-built back-reference that zlib rejects.

ducnmm · 2026-09-23T09:23:24Z

Confirming the product call on this guard: keep the defaults — 64MB per stream, 256MB per file, 20,000 streams — and keep refusing encrypted PDFs, including an empty password.

The decompression-guard bugs on d5c6a051 still stand (/F and #xx filter keys, RunLength wrapped around Flate, zlib CINFO that Node does not count, Predictor DecodeParms).

…suring it (WALM-683) Review round 3 on #985 (Henry). The decompression guard read the file with a regex over raw bytes and measured with Node's zlib. Both disagreed with pdf.js, and each disagreement was a way past the guard: `/F` and `#xx` filter keys, a decoy `/Filter` in a string or comment, RunLength wrapped around Flate, a zlib CINFO Node rejects and pdf.js ignores, and predictor `DecodeParms` never read. Rather than patch each, the guard now mirrors pdf.js (unpdf's bundled build, read from source) and refuses only what it cannot read the way pdf.js does. Reading the file: - A tokeniser matching pdf.js's Lexer: literal/hex strings, `%` comments, `#xx` names. `/F` beats `/Filter`, `/DP` beats `/DecodeParms`; a repeated key keeps its last value (Dict.set); a non-name key is skipped; `N G obj endobj` reads as empty. - Stream data ends where pdf.js's does (makeStream): at start+/Length if `endstream` follows, else at its endstream search, typos included. An indirect /Length resolves against top-level integer objects. - Every `N G obj` header and `>> stream` in the raw bytes is accounted for. pdf.js can reach an object by xref offset or recovery scan, so a header the walk did not pass (pdfTeX's `% 168 0 obj`, an embedded file in a stored block, anything after %%EOF) is parsed on its own and its stream measured; a stream keyword outside any object is refused. - pdf.js parses object streams with allowStreams: true and takes any stream with /First and /N as one — so a stream compressed inside another was invisible. Found while reading pdf.js, not in the review. Such streams are decoded and refused if they contain a stream. Measuring: - Flate is counted with a port of pdf.js's own FlateStream, not zlib: same header bits (method, FCHECK, FDICT, not CINFO), same tolerance (back-references are not bounds-checked; stored blocks claim their full length), same stopping points. Output matches zlib byte for byte on valid data; on invalid data it counts what pdf.js allocates. The fixed Huffman tables are generated and checked against pdf.js's. - Leading ASCIIHex/ASCII85 stages are decoded as pdf.js does, then one Flate or RunLength stage; a second expanding stage, or one behind an image codec, is refused. ASCII85's `z` counts (4:1). - A predictor row over the per-stream budget is refused; indirect or non-integer predictor parameters are refused. Hardening found along the way: - The raw scans were regular expressions that backtracked exponentially on runs of whitespace, NUL and `%`, which binary stream data is full of: one real 395KB file hung the guard indefinitely. They are now one right-to-left pass, linear in the file size. Verified against 10,995 real PDFs (TeX Live's documentation: pdfTeX, LuaTeX, Ghostscript, Acrobat, dvips; no personal files): 10,976 pass, slowest 17ms. The 19 refused are LZW (11), oversized (2) and encrypted (1) by policy, plus 4 with image streams inside object streams and 1 with indirect filter parameters, conservatively. Five real fixtures (Quartz, Chromium, Ghostscript, pdfTeX with and without object streams) join 55 guard tests covering each reported bypass, the object-stream case, the pdf.js inflater quirks, and a pathological-input timing test. Researcher suite 121 pass, build clean.

nikola0x0 · 2026-09-23T15:36:00Z

Round 3 is done in 753c7bd. The per-thread replies cover the four bugs; this is the bigger picture.

What changed: the guard now reads each stream the way pdf.js does (tokeniser, key precedence, stream extent including pdf.js's /Length fallback and its typo-tolerant endstream search, object streams) and measures with a port of pdf.js's own inflater. It refuses only what it can't read the way pdf.js reads it.

Also fixed: the raw scans were regexes that backtracked exponentially on runs of whitespace, NUL and %. One real 395KB file hung the guard indefinitely. They're now a single linear pass.

Checked against 10,995 real PDFs from TeX Live's documentation (pdfTeX, LuaTeX, Ghostscript, Acrobat, dvips): 10,976 pass, slowest 17ms. The 19 refused are LZW (11), oversized (2) and encrypted (1) by the confirmed policy, plus 5 conservative: 4 with image streams inside object streams, which the spec forbids and pdf.js tolerates, and 1 with indirect filter parameters. Five real fixtures (Quartz, Chromium, Ghostscript, and pdfTeX with and without object streams) are now in the suite.

ducnmm

Summary

The rewrite closes the four holes from d5c6a051. /F and #xx keys are decoded, a decoy /Filter in a string or comment is ignored, a second Flate or RunLength is refused, CINFO 8–15 is counted by the pdf.js inflater port, and a Predictor row over the cap is refused. Re-running those shapes against this guard throws each time.

One new hole: an indirect /Length is taken from a raw top-level integer, not from the xref object pdf.js fetches. A short orphan integer plus a planted endstream slices the stream there, and the stray-header pass never resumes into the rest. Under a 1000-byte cap the guard returned, and unpdf's pdf.js still decoded 20,011 glyphs from that stream.

The 64MB / 256MB / 20,000 defaults and refusing encrypted PDFs stay as confirmed.

Issue counts by severity

bugs: 1
suggestions: 0
nits: 0

ducnmm · 2026-09-23T16:31:40Z

+  if (lengthValue?.kind === "number" && lengthValue.integer) {
+    length = lengthValue.value;
+  } else if (lengthValue?.kind === "ref") {
+    const resolved = integers.get(`${lengthValue.num} ${lengthValue.gen}`);


[bug] An indirect /Length is resolved by integerObjects, which keeps a top-level N G obj <integer> when every such object agrees and never reads the xref. pdf.js fetches the reference (XRef.fetch, including a type-2 entry inside an object stream). Those bytes are not a top-level N G obj, so the scan never sees the real length. A single orphan 8 0 obj of a short integer is therefore the whole map, and it is not marked ambiguous.

streamRegion then accepts that length when endstream sits at dataStart + length (line 483) and slices the stream there. The inflater never reaches a later deflate block. The sequential walk would resume at that planted keyword and refuse the trailing bytes, but this stream is not on the walk: it is inside another stream's data, after %%EOF, or behind a comment the walk skips. The stray-header pass parses it, records the stream, and drops resumeAt (lines 682–687). The object stream that holds the real length has /First and /N and no inner >> stream, so it is not refused.

Reproduced with unpdf's pdf.js: under a 1000-byte cap the guard returned, the slice inflates to 10 bytes, and getOperatorList on that page shows 20,011 glyphs from the same stream. A larger trailing block past the 64MB default is never read. A direct /Length, and an indirect one whose top-level integers disagree, still measure the deflate stream.

Suggestion: Do not take /Length from the raw integer map unless that object is the one pdf.js would fetch, including a type-2 entry. Leaving dataEnd unset and inflating from dataStart through the end of the deflate data — the path already used when the length is unresolved — closes the hole. Parsing the xref and resolving that one reference is the closer mirror, and a length that cannot be resolved should still be refused rather than trusted.

Fixed in ec4938e, your first suggestion: an indirect /Length is now unknown, so measuring runs until the compressed data itself ends. A planted endstream can't shorten that. The raw integer map is gone. A direct /Length is unchanged, since pdf.js uses the same value. I also checked every other place the guard touches a reference (filters, filter parameters, predictor values, parameter-array items): all of them already refuse rather than resolve, so /Length was the only value taken on trust. Your shape (decoy 8 0 obj 10, endstream at +10, the stream reachable only after %%EOF) is a test that fails on 753c7bd. On the 10,995 TeX Live PDFs the result is unchanged, 10,976 pass, including the files with indirect lengths.

Review round 4 on #985 (Henry). An indirect `/Length` was resolved from top-level `N G obj <integer>` objects in the raw bytes. pdf.js resolves it through the xref instead — possibly to an object inside an object stream, which the raw bytes never show as a top-level integer. So a decoy `8 0 obj 10 endobj`, with `endstream` planted 10 bytes into the data, cut the stream there: the guard measured a three-byte stored block and never reached the compressed bomb behind it. Henry reproduced pdf.js decoding 20,011 glyphs from a stream the guard had passed under a 1000-byte cap. An indirect length is now unknown, and measuring runs until the compressed data itself ends — the path already used for lengths that could not be resolved, which a planted `endstream` cannot shorten. The raw integer map is gone. A direct `/Length` is unchanged: pdf.js uses the same value, so the region is exactly pdf.js's. Every other value pdf.js would resolve through the xref — filters, filter parameters, predictor values — was already refused rather than guessed. Verified: a test builds Henry's shape (decoy integer, planted endstream, stream reachable only by xref offset) and fails on the previous commit. The same 10,995 real PDFs give the same result as before — 10,976 pass, including files with indirect lengths. Researcher suite passes, build clean.

harrymove-ctrl

Approved. Follow-up commit mirrors pdf.js decompression metrics directly instead of zlib heuristic. CI checks pass.

ducnmm requested changes Sep 22, 2026

View reviewed changes

Comment thread apps/researcher/app/api/research/process-source/route.ts Outdated

Comment thread apps/researcher/lib/rag/ingest/extract.ts Outdated

Comment thread apps/researcher/app/(chat)/api/chat/route.ts Outdated

Comment thread apps/researcher/lib/rag/ingest/index.ts

nikola0x0 requested a review from ducnmm September 23, 2026 05:16

ducnmm requested changes Sep 23, 2026

View reviewed changes

nikola0x0 added 2 commits September 23, 2026 13:05

nikola0x0 requested a review from ducnmm September 23, 2026 06:16

ducnmm requested changes Sep 23, 2026

View reviewed changes

nikola0x0 requested a review from ducnmm September 23, 2026 15:36

ducnmm requested changes Sep 23, 2026

View reviewed changes

nikola0x0 requested review from ducnmm and harrymove-ctrl September 24, 2026 02:05

harrymove-ctrl approved these changes Sep 24, 2026

View reviewed changes

Conversation

nikola0x0 commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was wrong

The fix

After review (Henry, Harry)

❓ Needs the team to confirm

Not covered

Verification

Uh oh!

ducnmm left a comment

Choose a reason for hiding this comment

Summary

Issue counts by severity

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ducnmm left a comment

Choose a reason for hiding this comment

Summary

Issue counts by severity

Also

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikola0x0 commented Sep 23, 2026

Uh oh!

ducnmm left a comment

Choose a reason for hiding this comment

Summary

Issue counts by severity

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ducnmm commented Sep 23, 2026

Uh oh!

nikola0x0 commented Sep 23, 2026

Uh oh!

ducnmm left a comment

Choose a reason for hiding this comment

Summary

Issue counts by severity

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harrymove-ctrl left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

nikola0x0 commented Sep 22, 2026 •

edited

Loading