safe-dict: close the gate's known hole — R5, the READ mode

[mdheller]

Stacked on #43 (guard/safe-dict-spread-gate), which is itself stacked on #34. Merge order: #34 → #43 → this.

The hole

#43 shipped R1–R4 with an explicit scope statement, and the first item on it was the READ mode:

· 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 CLEAN both before and after that fix — ownValue() there is still only a convention.

Measured, on the real files rather than a paraphrase:

gremlin.ts PRE-ownValue  (origin/main):     0 violation(s)
gremlin.ts POST-ownValue (this branch):     0 violation(s)

Zero both sides. The other half of the same js/remote-property-injection alert had nothing holding it.

R5

A computed-key READ off an object that carries Object.prototype.

It is origin-directed, not "any computed read". The four guarded surfaces hold 57 computed element accesses, and almost all of them are array indexing (this.tokens[this.pos], row[i], refs[i+1]) or reads off dictionaries R3 already guarantees are null-prototype (binding[term.name], g[v], rr[v]). A blanket rule would be 57 false positives and one switched-off gate. Two base forms, both with known provenance:

• (a) <expr>.properties[k] — the graph's own bag. GraphNode.properties / GraphEdge.properties are Record<string, PropertyValue> (types.ts), materialized by store.addNode() from a caller-supplied object literal, so they carry Object.prototype, and their keys arrive in query text: values(k), has(k, v), order(k). This is the gremlin.ts pattern verbatim.
• (b) an identifier whose construction R1 already tracks as plain-prototype ({}, Object.assign({}, …), Object.fromEntries(…), toPlainRow(…)), read with a computed key. Same scope machinery as R1, so aliases and casts launder a read no more than they launder a write.

Literal keys stay trusted, as in R1. Assignment targets stay R1's — a new isAssignmentTarget() covers =, compound assignment, ++/-- and delete, and a new exclusive fixture flag asserts the two rules never both report one node (a line appearing twice under two diagnoses is a real defect: whichever is read first is the one acted on).

The fix is ownValue(bag, key) — a CALL, not an element access, so corrected code stops matching the rule rather than needing an exemption from it. No allowlist, no magic comment; same posture as #43.

R4 grows to match. R5 keys on a member name, so a rename would neuter it the way a type rename would neuter R3. The gate now fails loudly if properties stops being declared on GraphNode/GraphEdge.

Red proof

Against the real files at origin/main (pre-#34) and on this branch (post):

surface	PRE-#34 R5	POST R5
gremlin.ts	4 — has(), values(), both arms of order()	0
sparql.ts	1 — next[term.name] at line 419	0
cypher.ts	0	0
patternMatcher.ts	0	0

R5  ts/src/gremlin.ts:32:59  `n.properties` is a `.properties` bag — built from an object
                             literal, so it carries Object.prototype — and takes a
                             computed-key READ `n.properties[key]`. …
R5  ts/src/gremlin.ts:58:68  `t.properties` …
R5  ts/src/gremlin.ts:80:43  `a.properties` …
R5  ts/src/gremlin.ts:81:43  `b.properties` …

Those are exactly the four call sites 0.4.47 routed through ownValue().

Teeth proven both ways, per the gate's own doctrine:

• emptying PROTO_BEARING_BAGS → self-test goes red (prefix-gremlin-values: MUST be flagged (R5) and was NOT), with the real files still clean
• renaming GraphNode.properties → R4 goes red with the instruction to update PROTO_BEARING_BAGS/BAG_HOMES

What R5 still does NOT catch

To #43's standard, since that scope statement is what made this hole findable:

• The bag set is one member name, .properties. It is the only untrusted-keyed prototype-bearing bag in this engine's type surface, and R4 fails loudly if it is renamed — but a new such bag under a different name is uncovered until it is added here. R4 catches a rename, not an addition. atom.values[…] is deliberately excluded: it is an Atom's Value map, read with source-fixed constants (LABELS_KEY), not with names off the wire.
• A computed read off a bag reached through a Record<string, …> PARAMETER. Provenance is unknown at that point — the caller may well have passed a null-prototype dictionary — and guessing in either direction is worse than the stated gap. R3 is the compensating control: a parameter typed as a tracked dictionary is guaranteed null-prototype at every construction site, so reading it computed is safe.
• A bag aliased through a local: const p = node.properties; p[key] is form (b) with an origin R1 does not record, because node.properties is not one of the tracked constructors. The direct node.properties[key] — the shape the alert was about, and the shape fluent traversal code naturally writes — is caught.
• Unchanged from safe-dict: make the "never a spread" NB a gate that can go red #43: the in mode ('__proto__' in binding), cross-function laundering, files outside the guarded set, runtime prototype re-attachment, and someone editing the rules themselves.

Verification

npm test 433/433, npm run typecheck clean, npm run build produces no ts/dist drift (the safe-dict.ts change is comment-only). Gate summary now reads:

✓ safe-dict gate — 4 guarded surface(s), 14,886 AST nodes, rules R1–R5 clean;
  self-test: 17 must-fail / 12 must-pass fixtures all behaved.

R5 rides the same enforcement path #43 chose — ts/src/safe-dict-gate.test.ts under npm test, i.e. the required build-and-verify-dist check — so no workflow edit and no conflict with the lanes editing ts-ci.yml.

Lane note

No source overlap with #40 / #42 / #44 (ts/src/vendor-graph.ts). git merge-tree against all three conflicts only in the four rebuilt ts/dist binaries — whoever merges second runs npm run build && git add ts/dist.

[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 a new “R5” safe-dict gate rule to detect computed-key reads from prototype-bearing objects (notably node.properties[key]), closing a previously unguarded half of the js/remote-property-injection risk.

Changes:

• Introduces R5 logic to flag computed-key READs from known prototype-bearing origins while avoiding array-indexing / null-prototype dict false positives.
• Extends R4 “coverage” checks to ensure the graph bag member name (properties) can’t silently drift.
• Adds fixtures and new tests proving R5 is red on pre-fix Gremlin patterns and green on the shipped ownValue()-based fix.

Reviewed changes

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

File	Description
ts/src/safe-dict.ts	Updates module-level documentation to include R5 and clarify remaining known holes.
ts/src/safe-dict-gate.test.ts	Adds explicit R5 regression tests and expands fixture expectations to include R5.
scripts/check-safe-dict.mjs	Implements R5 detection, adds assignment-target discrimination, and strengthens R4 coverage checks for .properties.

---

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

[mdheller]

Rejecting — R5 already flags optional-chained reads; there is no bypass. In the TypeScript AST an optional element access a?.[k] still has SyntaxKind.ElementAccessExpression (with a questionDotToken), so ts.isElementAccessExpression(node) returns true for it. Verified against the real scanSource:

node.properties[key]    -> [R5]
node.properties?.[key]  -> [R5]
node?.properties[key]   -> [R5]
ownValue(node.properties, key) -> []   (the fix, correctly clean)

isElementAccessChain is a narrowing of the same node kind, not a separate one this misses. The genuinely uncaught read shape is the member-access alias (const p = node.properties; p[key]), which the script header already discloses under "what this does NOT catch".

[mdheller]

Fair hardening, low urgency — and it fails safe if it ever triggers. The \n} terminator matches today because the interfaces R4 covers close at column 0 in types.ts; an indented closing brace would make decl null and the coverage assertion go RED (a false failure that blocks CI), never a false pass, so it cannot silently switch the gate off. \n\s*} is the right tolerance and I'll take it; the AST route is heavier than this check needs. Noting rather than blocking.

[mdheller]

Correct that it is worth making explicit; the current code is not actually wrong, though. delete o[k] parses with the ElementAccessExpression as the direct operand of the DeleteExpression, so node.parent being a DeleteExpression does imply this node is the target. delete o[k].x deletes a property of o[k], where o[k]'s parent is the PropertyAccessExpression, not the delete — so it does not misfire either. Adding p.expression === node is a cheap guard against a future AST-shape change and I'm fine taking it; it changes no current classification.