mirror of
https://github.com/storytold/pdfcraft.git
synced 2026-11-11 16:32:04 +00:00
Michael Higgins
495cba8efc
## What this changes
The nesting guard that keeps hostile scripts from overflowing the stack in boa's parser could be put out of phase, and this replaces it with one that can't be. boa parses by recursive descent with no depth limit of its own (boa_parser 0.22 has none to turn on), and a stack overflow aborts the process, since `catch_unwind` can't catch it.
**The problem.** `refuse()` scanned bytes and guessed where strings begin. A quote inside a regular expression put it out of phase with the real strings. The first version of this PR counted opening brackets inside skipped text, which closed the reported `/'/` reproduction. It still let closing brackets inside *real* strings count as code. `x=` followed by 5,000 lines of `(/"/,"))))",` nests 5,000 deep in 65 KB (under the 256 KiB limit), while that scan never went past depth 1. The same works with `'`, and with template literals, which the scan didn't read at all.
**The fix.** The guard now lives in `crates/js/src/nesting.rs`.
- **It doesn't guess.** Whether a `/` is a division or starts a regular expression depends on where the parser is: boa's parser, not its lexer, sets that. So the scan follows every reading at once. At each `/` that isn't a comment, it splits into a division reading and a regular-expression reading, and counts nesting on each. boa's reading is always one of them, so the deepest count is at least boa's real depth. That makes it a sound bound, not a mitigation for one reproduction.
- **It drops readings boa rejects,** at the byte where boa stops with a syntax error: a string or regular expression running into a line break, or a regular expression whose groups don't match. boa runs `regress::try_parse` on each literal as it lexes it. This keeps ordinary divisions such as `Math.round(a / b * 100) / 100` from leaving stray readings that add up to false depth.
- **It reads the rest of the lexical grammar that changes what's code:** strings with escapes and line continuations, comments, template text and `${}` substitutions (which count as a level), the Annex B `<!--` and `-->` comments (followed both ways), and a leading `#!`.
- **It caps the work.** At most 16 readings are followed at once. Readings in the same place are merged first, keeping the larger counts, and past that the script is refused.
The limits are unchanged: 64 brackets, a run of 64 prefix operators, 4,096 conditionals. Every path that hands source to boa goes through `refuse()`: AcroForm scripts, XFA JavaScript, and `CheckedCodeFromStrings`.
Brackets inside strings and comments no longer count at all. That drops the trade-off in this PR's first version, which refused a script whose strings held more than 64 opening brackets in total.
Fixes #5079
## How you verified it
Locally, only formatting was run. I didn't build or test this commit, and I'm relying on CI for the build, clippy and the tests.
- [x] `cargo fmt --check -p pdfcraft-js` passes.
- [ ] clippy with `-D warnings` on `pdfcraft-js`: not run locally.
- [ ] `cargo test -p pdfcraft-js`: not run locally. A local test build was stopped before it compiled anything, so I have no local results for this commit.
What the tests check:
- `a_quote_in_a_regex_cannot_put_the_guard_out_of_phase`: the 5,000-line bypass above, and the same with `'` and backticks. It also runs a script nested exactly to the limit, closed properly.
- `nesting_guard_reads_templates_regexes_and_comments` and `divisions_do_not_add_up_to_a_refusal`: controls that must run.
- `boa_rejects_what_the_nesting_guard_drops`: the boa behaviour the guard relies on.
- The XFA-open test (`session::a_quote_in_a_regex_cannot_hide_nesting_from_an_initialize_script`) now runs the 5,000-line bypass too.
- `ordinary_nesting_still_runs` again expects 1,000 brackets inside strings and comments to run.
Not yet shown: that these tests fail on the previous head (439f86220). By reading the previous scan, the `(/"/,"))))",` script never goes past depth 1 there, but I haven't run that.
## Anything a reviewer should look at first
- `boa_rejects_what_the_nesting_guard_drops` pins the assumptions behind dropping a reading. If a boa upgrade starts accepting one of those inputs, that test fails, and the guard has to keep that reading.
- Out of scope: recursion that isn't bracket nesting, prefix runs or conditionals, such as long `a=a=a=…` chains or `if(1)if(1)…` statements. The guard didn't count those before this PR either. They're worth a separate issue once someone checks whether any of them overflows the stack within 256 KiB.
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
<!-- coderabbit-ai-generated:v1 -->
## Summary by CodeRabbit
* **Bug Fixes**
* Scripts that exceed supported nesting limits are rejected with a “nested too deeply” error, including excessive bracket nesting, prefix operators, and conditional expressions.
* Nesting checks now account for strings, comments, templates, regular expressions, and other JavaScript syntax, avoiding incorrect results from bracket-like text or ambiguous slash characters.
* After a nesting-related refusal during initialization, the session remains usable.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->