Skip to content

Review 5530

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 1 revision

#5530 — docs(vega): document that specs are programs, with the untrusted-spec wiring

bhamodi · open, approve posted · reviewed at 59e8dc73977 · view on GitHub

Verdict: approve — the loop merges it

Problem

An app team renders a chart from a spec that came from a user, a stored document or a model. Nothing in the props table, the spec JSDoc or the README tells them the spec's expression strings are compiled with the Function constructor, or that its data URLs are fetched with the page's credentials — so they ship it, and whoever wrote the spec gets script execution and credentialed reads inside their page.

This is the honest "prevents a defect nobody has hit yet" case rather than "we don't expose X": the reachable state is a <VegaChart spec={fromUser}> on any page, and the outcome is arbitrary evaluation.

Solution

(1 decision, in 3 places · 65 added lines, 12 of them prettier reflow of an import/export block, 0 runtime lines)

The decision: state that a spec is executable, put the boundary on the caller, and give the configuration that moves it.

  • a. spec JSDoc — the trust boundary (types.ts:135).
  • b. ParseOptions.ast JSDoc — ast is the safe-evaluation switch, not a tooling flag (types.ts:71-76).
  • c. README "Untrusted specs" — the worked wiring plus the CSP note (README.md:159).

One decision, not three. (a) and (b) both point the reader at "see 'Untrusted specs' in the README", so shipping either without (c) leaves a dangling reference and a half-stated boundary; they must land together. Rendering a chart definition is closer to running someone's code than to displaying their data, and the docs now say that out loud: only render definitions you wrote or read, and for anything from outside, here are the settings that switch the chart engine into a mode where those programs are interpreted instead of compiled and where it refuses to fetch anything. Nothing had to be built, because the wrapper already hands both settings straight through.

All of it traces to the problem the body states — "Neither the README nor the spec JSDoc said any of this." The 12 non-comment lines are the repo's own prettier rather than a style imposed: main's types.ts fails prettier --check and the head passes it, so this is drift repaired on a file the author was already in.

Where the decision is incomplete, and it is the review's one substantive note. (c) prescribes loader({mode: 'file'}) and calls the configuration "required, not optional" — but does not say how it fails. Measured: a spec that names a URL under that loader produces an empty chart and nothing else — runAsync resolves, the console is silent, and <VegaChart>'s own onError never fires, because the rejection is swallowed inside the dataflow rather than thrown. One clause in the snippet's comment closes it.

The documentation lives with the thing it documents. The two seams the recipe rides on both survive: parseOptions → vega.parse (VegaChart.tsx:130) and viewOptions → new View (VegaChart.tsx:133), where new View(runtime, {hover: true, ...viewOptions, container}) spreads viewOptions ahead of container, so expr and loader reach the View. The Vega-Lite branch adds a third — compile() at VegaChart.tsx:126, which runs before parse and is a code path the recipe has to survive too. All three were verified by running them.

The docs are written for a caller, cleanly: every added sentence answers what this component does to what I hand it and what I must configure to be safe — no rubric, no readiness gate, no promotion criteria, no reviewer instruction, no maintainer tool. The README's pre-existing "Publishing" and "Graduating" sections are ours-not-theirs, but they are on main and not this diff's to carry.

Impact

Nobody's chart renders differently. This reaches exactly one person: the next builder who opens the README or hovers spec in an editor, who now learns that the spec is executable before they wire a user-supplied one rather than after.

Newly exposed by merging: nothing. There is no half-fixed sibling — the package has one component, and it is private: true and canary-only, so there is no installed consumer at all.

API

No API change. No prop, no export, no type, no default, no accepted value moves. ParseOptions.ast keeps its exact type (ast?: boolean); only the sentence above it changed.

Theme targets

n/a — docs only. The style grep over the 65 added lines returns 0. No new theme targets.

Ossification

Nothing ossifies, and nothing internal is added either — no new helper, no new constant.

Breaking

First: there is no consumer who can be broken. packages/vega/package.json is private: true with astryx.canaryOnly: true — canary dist-tag only, never a stable release. The PR's changeset-exempt claim is correct.

  • API — no. Nothing's type or signature changed.
  • Visual — no. Zero lines of the diff reach a rendered element; the two files are a markdown README and a types file whose only non-comment change is import formatting.
  • Theme — no. No target, token or override is touched.
  • Behaviour — no. All six states walked and none is reachable: the diff adds no conditional, no default, no early return, no state setter.

Performance & resources

Effects +0 / ~0 / -0. The added lines contain no useEffect, useLayoutEffect or useState. No listener, no observer, no layout read, no dependency, no bundle delta — the package's dependency list is untouched.

The mode the docs recommend costs something, and the README says so itself in the snippet's own comment: "Slower, and a small subset of expressions is unsupported." That is the doc's disclosure, not a measurement — it was not measured. It is a cost the caller opts into and the diff imposes on nobody, so a number would not move the verdict.

Visual evidence

The diff renders nothing, so the frames are not of the diff — they are of the recipe the diff documents, which is the only claim in this PR that can be wrong. The exact snippet from README.md:173-190 was built and driven in real Chromium behind a CSP header that omits 'unsafe-eval', on both code paths the docs claim to cover. Four frames, all opened with the read tool: A (blank page), B (chart renders), H (Vega-Lite chart renders with axes and sort), C (blank container, no error). They are published alongside the posted comment, on the fork only.

run spec kind wiring CSP result
A Vega parse(spec), default View no 'unsafe-eval' EvalError: Evaluating a string as JavaScript violates the following Content Security Policy directive — nothing renders
B Vega {ast:true} + {expr: expressionInterpreter, loader: loader({mode:'file'})} no 'unsafe-eval' renders; 3 marks; fills crimson, crimson, steelblue
C Vega as B, spec names https://example.com/pilfer.json permissive 0 offsite requests attempted; blank chart, no error
D Vega default loader, same spec permissive vega fetches https://example.com/pilfer.json
G Vega-Lite compile() then default parse no 'unsafe-eval' same EvalError, nothing renders — so compile() itself is eval-free and parse is the wall
H Vega-Lite compile() then B's wiring no 'unsafe-eval' renders; axes, descending sort, steelblue, crimson, crimson

B's and H's fills are the load-bearing detail. In B, datum.b > hl with b = 55, 43, 28 and hl = 40 gives crimson, crimson, steelblue; in H the Vega-Lite condition: {test: "datum.b > 40"} gives the same three colours through the compiled spec. So the interpreter is evaluating the expressions rather than silently skipping them — and the collect sort and the nested datum.n.deep access both survive expressionInterpreter having no codegen of its own, because vega-util falls back to getter/the default comparator when ctx.expr.codegen is undefined and vega-runtime sets this.expr = expr || expressionCodegen, so supplying expr replaces the object whole.

G and H are the rows gate 1 sent the run back for. The first pass drove native Vega only, and Vega-Lite is the package's headline example (README.md:35-56) on a different code path. They pass — but "the evidence is a list of things that passed" was true of a table that did not contain them.

Harness check, and it caught a wrong number. The first run reported A as passing under the CSP. It was the instrument: the probe called into the page through page.evaluate, and CDP-originated evaluation is exempt from the CSP eval check — Function('return 41+1') returned 42 inside a document whose CSP forbids it. Rewritten so the page drives itself and the probe only reads the result, A throws EvalError as it should. Every number above is from the corrected harness.

A11y & i18n

Nothing here is touched. aria-|role=|useTranslator|t('@astryx over the 65 added lines → 0.

  • Automated: pr-a11y skipped on this head, correctly — no component changed. .github/a11y-baseline.json is untouched, so no violation is being bought silence for.
  • Manual: not reachable — the diff adds no element, no role, no state, no focusable, no animation. Checked by reading the diff's 65 lines; there is no rendered output to drive.
  • Strings: no user-visible or AT-facing string is added. README prose and JSDoc are not shipped strings.
  • Direction: no CSS, logical or physical. pr-rtl skipped, correctly.

Judgement

approve and merge. Documentation of existing behaviour — no capability, no surface, no design change — so nothing here needs a human ruling. Nothing in any slot describes someone who is stuck, cannot reach something, hears nothing or loses data. The two notes do not compound either: one is a stale table row, the other a missing sentence, and neither makes the other worse.

1. The parseOptions table still calls `ast` a tooling flag
   → a builder scanning the props table decides `ast` is for introspection and
     never reaches the section that says it is the safe-evaluation switch — the
     one place they would look is the one place still saying the old thing
                                        · README.md:134
     | `ast` | `boolean` | Retain expression AST in the runtime (useful for tooling) |

2. loader({mode: 'file'}) fails silently
   → someone follows the configuration the doc calls "required, not optional",
     their spec names a URL, and they get an empty chart: runAsync resolves,
     nothing reaches the console, and onError never fires — measured, run C
                                        · README.md:187
         loader: loader({mode: 'file'}),

Neither blocks, and both are one line. Cindy's standing ruling covers the verdict — docs are good to merge as long as they are correct — and this one is correct on both code paths, because both were run.

CI: 18 checks on this head, every required context green (build, lint, test, docsite-test); pr-a11y, pr-rtl and pr-visual skipped because no component changed. review-required is the only pending status and clears on the approval. Vercel is red on this PR and on all six other open PRs checked (#5533, #5532, #5531, #5529, #5528, #5527) — repo-wide, not theirs. The needs:code-review label is triage rather than a gate: it fired on a src/ path whose only change is a comment block, and PRs carrying it have merged before (#4636, #3681, #3672).

Three things found and not spent on the author: the README's "mode: 'file' with no baseURL rejects everything" clause is inert, because vega-loader's loader.js:168 sets loadFile from options.mode === 'file' unconditionally, so a baseURL changes nothing — correct as written for a browser, and the Node path where it would grant filesystem reads is unreachable since <VegaChart> only builds the View inside an effect; the README's other examples pass inline object literals for parseOptions/viewOptions, which the component's own JSDoc warns against (VegaChart.tsx:41) because they are effect deps at VegaChart.tsx:167, and the new snippet inherits a habit that is pre-existing on main (README.md:74); and of the 60 open PRs, this is the only one touching packages/vega/.

Not verified: the interpreter's runtime cost relative to the default, which was not measured; and whether vega-interpreter's "small subset of expressions is unsupported" caveat bites any spec Astryx consumers actually write — signals, event streams, a formula, a comparator sort, nested field access and a Vega-Lite condition test all worked, but that is a sample rather than the subset.

The review, as posted

Thanks — this is worth documenting and the wiring checks out: I ran the snippet and it renders under a CSP with no 'unsafe-eval'. Two nits.

Full review

[Reviewed by Robohands]

Inlines:

  • packages/vega/README.md:134 — Still says ast is for tooling. Probably wants to point at the new section.
  • packages/vega/README.md:187 — Hmm, this rejects silently — no onError, just an empty chart. Worth a clause.

Rounds

One review, five gate passes. The posted comment has not changed a word since round 1; every round after it graded the hand-off.

  • Gate 1 — failed on seven counts, one of them the gate. The evidence drove native Vega specs only, while the README's first sentence is "A Vega/Vega-Lite spec is a program", the package's headline example is Vega-Lite, and <VegaChart> runs Vega-Lite through compile() before parse() — a code path none of the runs touched. If Vega-Lite's compile reached the Function constructor, the documented recipe would fail for the majority use case under the very CSP the PR recommends. Also: a visual claim in the posted comment whose frames nobody was told to publish; a banked reproduction pointer that resolved to nothing; the public-docs audience question assumed rather than answered; the commit type never tested; and an unmeasured cost stated in the reviewer's own voice instead of attributed to the README.
  • Gate 2 — four more. The substantive note was filed in the slot that noticed it rather than the slot that owns it — the wiring is the solution's, not the screenshots'. "3 doc edits" invited a decision-count smell the PR does not have. An inline asserted an empty chart with a frame sitting unlisted on disk. And the head re-verification was recorded as a hash rather than as an act.
  • Gate 3 — two anchors a few lines off, both introduced by the rewrite gate 2 asked for, which is the usual way a fixed draft breaks something else. Repointed.
  • Gate 4 — the frames were banked where the next person could not rely on them, on a shared machine that gets cleaned, while the run's own durable artifact directory sat beside the gate files. Moved.
  • Gate 5 — clean. Every slot filled, both spec kinds and both loader modes enumerated rather than sampled, every anchor opened at head with its text pasted, the probe banked with a README recording the trap that produced the wrong number, and no unmeasured cost stated as fact.

What changed before posting

Posted as drafted, with a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5530) line appended before the attribution.

Clone this wiki locally