MM-69725: Add adversarial tests and fixes - #6
Merged
Merged
Conversation
* Additional tests for problematic PDFs * MM-69725: Address fixture test review feedback Avoid counting retained plaintext in allocation probes, preserve depth-limit errors for assertions, and align the fixture tooling defaults and failures.
* Propagate ctx through lexer reload and CMap parsing The buffer.reload() method is called for every chunk read from a stream, but previously had no way to respect context cancellation. A malicious PDF with a huge ToUnicode CMap or a giant literal string token could hold the goroutine indefinitely even after the caller cancelled. Changes: - Add ctx field to buffer; reload() checks it before each read so long tokens and stream decodes stay cancellable. - Thread ctx through Interpret → buffer so PostScript streams (CMap, content) honor cancellation at the lexer level. - Add internal Font.encoder(ctx) / getEncoder(ctx) / charmapEncoding(ctx) and readCmap(ctx, …) so ToUnicode CMap parsing uses the caller's ctx instead of context.Background(). - GetPlainText and Content now call encoder(ctx) so the full extraction path is cancellable end-to-end. * Cap Predictor Columns to prevent large up-front alloc applyFilter allocates a ~2×Columns-byte buffer before any content is read. A malicious PDF with a huge Columns value (e.g. 2^31-1) would cause a multi-GB allocation before cancellation or EOF. Add maxPredictorColumns = 1<<20 (1M columns, well above any real image scanline or xref stream) and panic with a clear error if the value is out of range. Also update adversarial_pdfs/README.md to reflect the "gap under test" framing and add a probe command for manual testing. * Cap literal string tokens and fix AcroForm alloc threshold String token cap: readLiteralString and readHexString accumulate the entire token into a growing slice with no upper bound. A malicious PDF can embed a huge (...) or <...> string — either raw on disk or compressed via FlateDecode — forcing an unbounded allocation before any cancel poll fires. Add maxStringBytes = 128 MiB and panic with a clear error if either accumulator exceeds it. AcroForm test threshold: The hardcoded 8 MiB limit in TestAdversarial_AcroFormStaysCheap was calibrated for the medium-scale fixture (~2 MB, 10 K fields). At large scale (50 K fields, ~9.4 MB), xref-table parsing allocates proportional to object count, so the threshold must scale with the fixture. Use len(data) + 2 MiB instead. * Fix nested_content depth at small scale depth=200 is below maxObjectDepth=1000, so the depth-limit test never triggered at small scale. The fixture is a few KB regardless of depth, so use 1200 (same as medium/large) across all scales. * Convert Python script into Go * Remove development tools and documents * Add CI step to run adversarial tests * Run CI on push to master, and all on PRs
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPDF extraction now propagates cancellation through parsing and font decoding, limits oversized strings and predictor allocations, reports nesting-limit errors consistently, and adds generated adversarial fixtures with tagged tests and CI execution. ChangesAdversarial extraction safeguards
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GetPlainText
participant FontEncoder
participant Interpret
participant Buffer
GetPlainText->>FontEncoder: decode ToUnicode CMap with context
FontEncoder->>Interpret: Interpret(context)
Interpret->>Buffer: assign context
Buffer-->>Interpret: return cancellation error
Interpret-->>GetPlainText: propagate recovered error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adversarial_extract_test.go`:
- Line 33: Update the missing-fixtures failure message in the adversarial
fixture test to direct users to run the Go generator via `go run
./tools/gen_adversarial_pdfs ...`, replacing the nonexistent Python script
reference while preserving the existing directory and error details.
- Around line 63-72: Update the timeout test flow around GetPlainText and the
affected setup at lines 100–121 so cancellation is triggered only after an
instrumented reader confirms parsing has begun and content has been consumed.
Measure cancellation latency and allocations from that confirmation point, and
replace the fixed 5ms wall-clock assertion with a deterministic check that
avoids CI timing flakiness while still verifying cancellation behavior.
In `@lex.go`:
- Around line 41-44: Update readLiteralString so the maxStringBytes limit is
checked after every append to tmp, including escaped characters and
nested-parenthesis content, rather than only in the default branch. Preserve the
existing limit error behavior and ensure all append paths cannot grow tmp beyond
the cap.
In `@page.go`:
- Line 417: Make cancellation cooperative throughout page text processing: in
page.go lines 417-417, add ctx.Err() checks inside the endcodespacerange,
endbfchar, and endbfrange mapping loops and propagate cancellation as an error;
in page.go lines 611-611, update Page.GetPlainText to use the context-aware
decode path; in page.go lines 1010-1010, make Page.Content use that same
context-aware decoding path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1f744cc-beac-4967-bd1e-e9a1022ab9af
📒 Files selected for processing (8)
.github/workflows/ci.ymladversarial_extract_test.golex.gopage.gops.goread.gotools/adversarial_pdfs/.gitignoretools/gen_adversarial_pdfs/main.go
maxStringBytes was only checked in readLiteralString's default branch. Escaped characters, octal escapes, and nested-parenthesis bytes all append to tmp through other branches, so a payload built from those (e.g. repeated \n escapes) could still grow the token past the cap before ever tripping the check. Move the check to run once per loop iteration, after the switch, so every append path is bounded the same way.
Interpret only polls ctx between tokens, but readCmap's
end{codespacerange,bf{char,range}} handlers each loop n times within a
single token callback, where n comes straight from an
attacker-controlled int64 on the stack. A malicious ToUnicode CMap can
set n arbitrarily high, turning one callback into an unbounded busy loop
that no cancellation can interrupt. Check ctx.Err() on each iteration
and bail out via the existing ok=false path so these loops stay
cooperative with a canceled context.
Fix the outdated mention to the Python script. The four cancel-cheaply tests used a fixed 200us context timeout that, measured against unrestricted runs taking 10-340ms, was expiring before GetPlainText ever reached the fixture's adversarial content. The tests were passing by canceling before parsing started, not by exercising cancellation mid-parse. Replace the fixed timeout with contentStreamTrigger, an io.ReaderAt wrapper that cancels only after it observes a run of consecutive, sequentially-offset reads: the signature of the lexer actually streaming through a stream's bytes, as opposed to the scattered small reads used to resolve the xref table, page tree, and fonts. Cancellation latency and allocations are then measured from that confirmed point forward, giving a deterministic signal instead of one that races an arbitrary deadline against unrelated setup cost.
|
/update-branch |
|
Error trying to update the PR. |
jgheithcock
approved these changes
Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Get both #4 and #5 into
master.Ticket Link
https://mattermost.atlassian.net/browse/MM-69725