safe-dict: make the "never a spread" NB a gate that can go red

[mdheller]

The rule, and the fact that it was already broken

ts/src/safe-dict.ts (this PR's base, #34) closes CodeQL js/remote-property-injection across the query surfaces and ends its docstring with a rule:

NB: object spread ({ ...dict }) and object literals re-attach Object.prototype. Use cloneDict / mergeDicts, never a spread, to carry one of these forward.

That rule was a comment. In the same PR it was already violated: patternMatcher.ts had its interior converted to null-prototype groundings and its output row left as

const row: Record<string, string> = {}
for (const v of variables) { const atom = g[v] ? as.getAtom(g[v]) : undefined; row[v] = atom ? (atom.name ?? atom.type) : '' }

keyed by pattern variable names that arrive from query text. row['__proto__'] = v is an ordinary assignment: it walks the prototype chain, finds Object.prototype's inherited __proto__ setter, and the write is swallowed — the column stays listed in variables and is absent from every row. tsc accepted it. The whole suite passed. It was caught in 454a2c8 by a human reading the diff, which is not a control.

This PR is the control.

Which option, and why

Option 2 — a narrow AST check (scripts/check-safe-dict.mjs), wired into npm test.

Not option 1 (ESLint no-restricted-syntax), for two reasons, the second decisive:

1. The repo has no ESLint config and no eslint in devDependencies (verified). Introducing eslint + typescript-eslint + a parser project + a new CI lane for a single rule is disproportionate, and it opens "so what's our style baseline?" as a side effect of a security fix.
2. no-restricted-syntax is a selector over syntax. It cannot know that a value is a Grounding; it can only match the annotation text. The actual regression was annotated Record<string, string> — not Binding, Grounding, RRow or SafeDict. A rule keyed on those four names would have shipped green against the very bug that motivated this task. That is the failure mode being guarded against, so the gate must key on the shape, not the type name.

Not option 3 alone — a reflective HOSTILE_KEYS test is good and #34 already has one for the pattern-matcher row, but it only covers paths a test happens to drive and says nothing about a newly added surface. Both layers now fire (see below).

Option 2 also matches an existing, proven pattern in this repo: scripts/check-kko-provenance.mjs + npm run check:kko + a CI job. It uses typescript, already a devDependency — no new dependencies.

What it enforces

Guarded set = sparql.ts, cypher.ts, gremlin.ts, patternMatcher.ts, plus any file that imports safe-dict — so a new query surface is covered the moment it adopts the convention, not when someone remembers to edit a list.

	rule
R1	A computed-key assignment onto a normal-prototype object: o = {} / Object.assign({}, …) / Object.fromEntries(…) / toPlainRow(…) followed by o[expr] = v. Literal-key writes (o['a'] = 1) are fine. Syntactic — it reads how the value was constructed, not how it was declared.
R2	Any object spread. Array spread [...xs] and call spread f(...xs) untouched.
R3	An object literal in a Binding / Grounding / RRow / SafeDict position — declaration, parameter default, as cast, satisfies, annotated return.
R4	Coverage: the canonical surfaces still exist, the tracked type names still resolve to declarations, the guarded set is non-empty.

Red-then-green evidence

1. Red against the pre-fix patternMatcher.ts. Restoring the exact construction from the task:

$ node scripts/check-safe-dict.mjs
✗ safe-dict gate: 1 violation(s) — ts/src/safe-dict.ts's "never a spread, never a plain literal" rule is not being kept.

  R1  ts/src/patternMatcher.ts:88:7  `row` is an object literal — it has Object.prototype — and takes a
      computed-key write `row[…] = …`. If that key can be "__proto__" the assignment hits the inherited
      SETTER and is SWALLOWED: the column is declared and absent from every row.

EXIT=1

…while npm run typecheck stays green (exit 0) on the same tree. The compiler cannot see this; the gate can.

2. Red through the actual CI command. npm test is what ts-ci.yml's Test step runs:

$ npm test          # pre-fix patternMatcher.ts restored
✖ GATE: the shipped patternMatcher output row is ACCEPTED
✖ GATE: the guarded set still covers every canonical query surface, and the live files are clean
✖ SECURITY: a pattern variable named __proto__ appears as an own column in the result row   ← #34's own test
ℹ tests 430   ℹ pass 427   ℹ fail 3
npm test exit: 1

3. Green on the shipped code.

$ npm run check:safe-dict
✓ safe-dict gate — 4 guarded surface(s) [cypher.ts, gremlin.ts, patternMatcher.ts, sparql.ts],
  14,886 AST nodes, rules R1–R4 clean; self-test: 13 must-fail / 8 must-pass fixtures all behaved.
$ npm test        # 430 tests, 430 pass, 0 fail
$ npm run typecheck   # clean
$ npm run build       # no ts/dist drift

4. It independently reproduces #34's whole remediation. Scanning origin/main's pre-hardening files:

file	violations
sparql.ts	18 (R1 ×6, R2 ×7, R3 ×5)
cypher.ts	9 (R1 ×6, R2 ×2, R3 ×1)
patternMatcher.ts	4 (R1 ×1, R2 ×1, R3 ×2)
gremlin.ts	0 — see below

5. The gate's own teeth are tested. FIXTURES is a table of 13 sources that must be flagged and 8 that must stay clean; runSelfTest() runs before every scan and ts/src/safe-dict-gate.test.ts drives the same table from npm test. Gutting R1 (const origin = null) produces:

✗ safe-dict gate SELF-TEST failed — the rule engine no longer detects what it claims to.
  A checker that cannot go red is not a check. Fix the rules, do not relax the fixtures.
    · prefix-patternMatcher-row: MUST be flagged (R1) and was NOT — the verbatim regression …
    · prefix-row-untyped / prefix-row-cast-launder / objectassign-launder / toplainrow-then-write
EXIT=1

…with the real files clean. That is the specific defect this estate keeps hitting — a checker that prints its own success line while validating nothing — and it is closed by construction.

Evasion: what was tried, and what still works

Each of these is a must-fail fixture, so it stays closed:

• drop the annotation — const row = {}; row[k] = v → R1. (R1 never reads the type.)
• as cast on the initializer — const row = {} as Record<string,string> → R1.
• as Binding on the literal — → R3 (a cast does not change the prototype).
• Object.assign({}, src) — cloneDict minus the null prototype → R1.
• toPlainRow(...) then write to the result — correct materialization, but the result carries Object.prototype, so a later computed write is unsafe again → R1.
• an intermediate alias — const alias = row; alias[k] = v → R1 follows identifier aliases.
• deferred seed — let row; row = {}; row[k] = v → R1.
• rename the type — R2 does not look at types at all.

What it does not catch

Listed at the top of the script rather than implied away:

• The READ mode. props[key] off a normal-prototype object returns inherited functions (values("constructor") really does hand back the Object constructor). That was gremlin.ts's half of the CodeQL alert, and this gate scores gremlin.ts 0 both before and after that fix (table above). ownValue() there is still only a convention. This is the largest honest hole.
• The in mode. '__proto__' in binding on a prototype-bearing object is not detected.
• Laundering across a function boundary. R1 is intra-procedural: pass {} to a helper that performs the computed write and neither end is flagged alone. In practice such a helper's parameter is typed as a tracked dictionary and R3 fires at the call site — but a Record<string, …> parameter slips through.
• Files outside the guarded set — covered the moment they import safe-dict, and not before.
• Runtime re-attachment — Object.setPrototypeOf, structuredClone, JSON.parse(JSON.stringify(d)).
• Someone editing the checker. There is deliberately no magic-comment escape hatch, so relaxing the rule has to appear as a change to the rule, in the diff, under review.

Coordination

• Stacked on engine 0.4.47: close two live CodeQL alerts on main — remote property injection + log forgery #34 (fix/codeql-injection-alerts), because safe-dict.ts does not exist on main. Merge engine 0.4.47: close two live CodeQL alerts on main — remote property injection + log forgery #34 first; this retargets to main cleanly.
• ts-ci.yml is untouched. engine 0.4.47: close two live CodeQL alerts on main — remote property injection + log forgery #34 and fix: widen the ts-ci PR filter to match its own push filter; consume contractViolations #40 are both live in that file. ts-ci.yml already runs npm test, and npm test globs ts/src/*.test.ts, so ts/src/safe-dict-gate.test.ts puts the gate on the required build-and-verify-dist check with zero workflow conflict. package.json gains one additive check:safe-dict line; nlq: close the declared-unenforced gap inside the declared-unenforced guard (format) #37 and fix: widen the ts-ci PR filter to match its own push filter; consume contractViolations #40 do not touch that file.
• ts/dist unchanged — safe-dict.ts is not re-exported from index.ts, and the change is a docstring; npm run build produces no drift.
• Noticed, not fixed here: on engine 0.4.47: close two live CodeQL alerts on main — remote property injection + log forgery #34's branch package-lock.json still says 0.4.46 while package.json says 0.4.47. npm install re-syncs it. Left alone so it does not ride in on an unrelated PR — worth a line in engine 0.4.47: close two live CodeQL alerts on main — remote property injection + log forgery #34.

safe-dict.ts's NB now names what enforces it, and what does not.

[mdheller]

Coordination note for #40 — a real dependency, not a nit.

ts-ci.yml's PR filter is currently ^(package(-lock)?\.json|ts/|\.github/workflows/ts-ci\.yml). It does not include scripts/. This PR is fine (it touches package.json and ts/src/**), but the checker itself lives at scripts/check-safe-dict.mjs, so a future PR that changes only the gate would skip the Test step and never run the gate on the change to the gate — the same shape #36 described for ts-ci.yml itself.

#40 widens that filter to match the push filter, which already covers scripts/**. So #40 closes this. Recording it here so the dependency is explicit rather than discovered later: if #40 is dropped or narrowed, this gate needs scripts/ added to the filter itself.

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds an enforced “safe-dict” security gate to prevent prototype-related hazards (object spread, plain object literals, and computed-key writes onto prototype-bearing objects) in query-surface code, and wires it into npm test.

Changes:

• Add an AST-based checker (scripts/check-safe-dict.mjs) with self-test fixtures and guarded-file discovery.
• Add a Node test (ts/src/safe-dict-gate.test.ts) that asserts the checker goes red on known-bad patterns and green on current surfaces.
• Document the enforcement mechanism in ts/src/safe-dict.ts and add npm run check:safe-dict.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.

File	Description
ts/src/safe-dict.ts	Expands the docstring NB to explain the new enforcement gate and its scope/limitations.
ts/src/safe-dict-gate.test.ts	Adds test-suite enforcement for the gate (red/green + coverage assertions).
scripts/check-safe-dict.mjs	Implements the AST gate (rules R1–R4), guarded-file discovery, and self-test fixtures.
package.json	Adds check:safe-dict script to run the gate.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.