fix(api): harden generated extractor validation for deterministic JSON by abimaelmartell · Pull Request #4288 · firecrawl/firecrawl

abimaelmartell · 2026-08-11T18:14:31Z

What

Tightens static validation of the generated JavaScript extractor scripts used by deterministic-JSON mode, and validates cached scripts on read.

Reject additional disallowed references — constructors/prototypes, code-eval globals (Function/eval), ambient objects (process/globalThis), and Node module internals — across the syntactic forms that reach them: member access, computed-string access, destructuring, and with statements.
Validate cached extractor scripts on read: a script cached before these rules is regenerated rather than reused. The check is guarded so a validator error falls back to regeneration.
Add unit tests covering the rejected forms, the accepted normal-extractor path, and a documented limitation (computed access via a variable is out of scope for static checking).

Why

Defense-in-depth around the extractor execution path. Generated DOM-extraction code has no legitimate reason to reference these names, so rejecting them — and re-checking cached scripts — keeps disallowed references from being generated or persisted. This is a validation/robustness layer, not an isolation boundary.

Testing

vitest run src/lib/deterministicJson/pipeline/validate.test.ts — 18/18.
tsc --noEmit — clean, project-wide.
knip --cache, prettier — clean.

Summary by cubic

Hardened validation for generated deterministic-JSON extractor code and re-checked cached scripts before running to block disallowed references. Cached scripts that fail validation or make the validator throw are regenerated.

Bug Fixes
- Reject disallowed names — constructors/prototypes, code-eval (Function/eval), ambient (globalThis/require/Reflect/Proxy), Node internals, and network (fetch/XMLHttpRequest) — across member, computed-string (including computed destructuring), declaration object-pattern destructuring, and with; only free global references are flagged (shadowed names are allowed).
- Validate cached extractor code on read; if validation fails or throws, regenerate instead of running it.
- Restrict destructuring checks to declaration-time, non-rest object patterns to avoid false positives; assignment and for-of destructuring are out of scope, and value object literals remain allowed.
- Add unit tests and document limits for variable-based computed access, reflection via string arguments, and assignment/for-of destructuring.

^{Written for commit 7051aff. Summary will update on new commits.}

Tighten static validation of generated deterministic-JSON extractor scripts: - Reject additional disallowed references (constructors/prototypes, code-eval globals, and Node module internals) in member, computed-string, and destructuring access, plus `with` statements. - Validate cached extractor scripts on read, so a script cached before the rules tightened is regenerated instead of reused (guarded against validator throws). - Add unit tests for the validator. This is a defensive validation layer over the extractor sandbox, not an isolation boundary; indirected access (e.g. computed via a variable) is intentionally out of scope.

cubic-dev-ai

All reported issues were addressed across 3 files

_{Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic}

- Catch computed-string destructuring (`const { ["name"]: x } = obj`) in the disallowed-property check, matching the member and `obj["name"]` forms. - Add `fetch` to the disallowed-property set so `window.fetch` / `window["fetch"]` are handled like the other network/global names. - Only flag free references to disallowed globals: skip identifiers in name/declaration positions (declarations, parameters, member/method names, destructuring targets) so a benign extractor that shadows one isn't rejected. Tests added for each.

cubic-dev-ai

All reported issues were addressed across 2 files (changes from recent commits).

_{Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic}

Array-binding elements (`const [x] = arr`) and rest bindings (`const { ...x } = obj`) introduce a local by position/remainder, not a named property read, so a local named after a disallowed word was wrongly rejected. Limit the check to non-rest object-binding-pattern elements. Test added.

cubic-dev-ai

0 issues found across 2 files (changes from recent commits).

_{Re-trigger cubic}

abimaelmartell · 2026-08-11T18:53:35Z

@cubic-dev-ai

cubic-dev-ai · 2026-08-11T18:53:40Z

@cubic-dev-ai

@abimaelmartell I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai

All reported issues were addressed across 3 files

_{Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic}

Dismissed because Cubic found issues in a newer review.

…mbient names - Catch assignment destructuring (`({ constructor: C } = obj)`) reads, while keeping value object literals (`return { constructor: v }`) allowed. - Add ambient names (`globalThis`/`require`/`Reflect`/`Proxy`) to the disallowed-property set so member/computed/destructuring forms match the bare-global list. `process` stays out — it is a common data-field name and is not reachable as a member in the sandbox. - Document the remaining static-analysis limits (reflection via a string argument, computed access via a variable) as out of scope in the comment. Tests added.

cubic-dev-ai

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/api/src/lib/deterministicJson/pipeline/validate.ts">

<violation number="1" location="apps/api/src/lib/deterministicJson/pipeline/validate.ts:249">
P2: Valid `for...of` and `for...in` assignment-pattern destructuring still bypasses the forbidden-property check, for example `for ({ constructor: C } of docs) {}`. Treat a `ForOfStatement`/`ForInStatement` initializer as a destructuring target as well, so these property reads receive the same validation as `({ constructor: C } = obj)`.</violation>
</file>

_{Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic}

Back out the assignment-destructuring property check added earlier: resolving assignment and `for..of` patterns reliably needs analysis this static pass does not do, and the attempt produced a false positive on default-value object literals (`({ foo = { constructor: C } } = obj)`). Keep the robust declaration-destructuring check and document assignment destructuring alongside the other indirection forms (variable-keyed access, reflection helpers) as out of scope. Tests updated.

cubic-dev-ai

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/api/src/lib/deterministicJson/pipeline/validate.ts">

<violation number="1" location="apps/api/src/lib/deterministicJson/pipeline/validate.ts:155">
P3: The limitation comment says `for..of` destructuring is out of scope, but declaration-form `for (const { constructor } of items)` is still rejected by the binding-pattern check; only assignment-form `for ({ constructor } of items)` is skipped. Narrow the wording to assignment destructuring, including assignment-form `for..of`, so the documentation matches the validator and test coverage.</violation>
</file>

_{Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic}

cubic-dev-ai · 2026-08-11T19:58:23Z

+// (`const { name } = x`), plus `with`. Names reached only through indirection are
+// deliberately out of scope, since resolving them reliably needs analysis this
+// pass doesn't do: a variable-keyed access (`x[k]`), a reflection helper, or an
+// assignment / `for..of` destructuring pattern.


P3: The limitation comment says for..of destructuring is out of scope, but declaration-form for (const { constructor } of items) is still rejected by the binding-pattern check; only assignment-form for ({ constructor } of items) is skipped. Narrow the wording to assignment destructuring, including assignment-form for..of, so the documentation matches the validator and test coverage.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/lib/deterministicJson/pipeline/validate.ts, line 155: <comment>The limitation comment says `for..of` destructuring is out of scope, but declaration-form `for (const { constructor } of items)` is still rejected by the binding-pattern check; only assignment-form `for ({ constructor } of items)` is skipped. Narrow the wording to assignment destructuring, including assignment-form `for..of`, so the documentation matches the validator and test coverage.</comment> <file context> @@ -145,15 +145,14 @@ function validateTopLevelShape( +// (`const { name } = x`), plus `with`. Names reached only through indirection are +// deliberately out of scope, since resolving them reliably needs analysis this +// pass doesn't do: a variable-keyed access (`x[k]`), a reflection helper, or an +// assignment / `for..of` destructuring pattern. // // Globals the sandbox doesn't provide, plus code-eval primitives (`Function`/ </file context>

Suggested change

// assignment / `for..of` destructuring pattern.

// assignment destructuring (including assignment-form `for..of`).