Skip to content

engine 0.4.47: close two live CodeQL alerts on main — remote property injection + log forgery - #34

Open
mdheller wants to merge 6 commits into
mainfrom
fix/codeql-injection-alerts
Open

engine 0.4.47: close two live CodeQL alerts on main — remote property injection + log forgery#34
mdheller wants to merge 6 commits into
mainfrom
fix/codeql-injection-alerts

Conversation

@mdheller

@mdheller mdheller commented Jul 29, 2026

Copy link
Copy Markdown
Member

Closes the two CodeQL alerts that are open against main right now (verified at dbe854f, the tip this branch is cut from). Neither was introduced by an open PR — alert 34 was created 2026-07-17, alert 28 on 2026-07-04.

This also unblocks #33: its aggregate CodeQL check fails only because the rebuilt ts/dist re-includes sparql.ts, so this pre-existing alert is attributed to "code changed by this pull request". #33 needs a rebase onto this once it lands.


1. HIGH — js/remote-property-injection, ts/src/sparql.ts:628

Reachable and exploitable, authenticated. The SPARQL tokenizer accepts /\?[A-Za-z0-9_]+/, and _ is in that class — so ?__proto__ and ?constructor are expressible verbatim in a query body. The query string is fully attacker-controlled at POST /api/graph/sparql (prophet-platform apps/hellgraph-service, scope graph:read) and at super-peer's own POST /query (scope query). No unauthenticated path when a verifier is configured; the deploy manifest does wire one via envFrom: secretRef.

What is not true: this is not remote code execution, and it does not pollute the global Object.prototype — I probed for that specifically and it never happens. The impact is per-object clobbering plus a silently-wrong query surface.

A plain-object Binding reaches Object.prototype three ways, all three verified against main:

effect on main
WRITE out['__proto__'] = v hits the inherited setter instead of defining a property — SELECT (COUNT(?s) AS ?__proto__) declares the variable and then returns {}
READ sol['constructor'] returns an inherited function, which escapes into results as a binding value
in '__proto__' in next true on a binding that bound nothing → unify() compares against Object.prototype, fails, and the query returns zero rows

Fix. Intermediate bindings become null-prototype dictionaries (new ts/src/safe-dict.ts); output rows are materialized with Object.fromEntries, which uses CreateDataProperty, so a hostile name lands as an own data property and consumers still get a normal-prototype JSON object.

Hostile names are deliberately not rejected. SPARQL says any name is a legal variable; rejecting ?constructor would trade one silently-wrong answer for a different one. A null prototype makes it an ordinary variable, which is what it is. No suppression comment anywhere.

The projection site already had a partial fix (build via Map) — it guarded the write but not the read, so SELECT ?constructor still returned the Object constructor. Both halves are closed.

Siblings — the same pattern, also fixed

Checked every dynamic-key site in ts/src/. Three siblings shared it:

  • gremlin.tshas() / values() / order() read n.properties[key] with a traversal-supplied key. g.V().values("constructor") genuinely returned the Object constructor as if it were stored graph data. Now own-property reads only.
  • cypher.ts — row and grounding construction, plus $param lookup: an unsupplied $constructor resolved to Object.prototype.constructor and stringified into the comparison.
  • patternMatcher.ts — the identical in bug in unifyTerm; this is the root cause of Cypher dropping matches for such a variable.

Also noted, deliberately not changed: ROUTE_SCOPE[`${method} ${pathname}`] in super-peer.ts is the same shape but not reachable — the key always contains a space and a /, so it can never equal a prototype member. And a Cypher variable named __proto__ projects as '' because Cypher reserves a leading _ for internal IR variables (_leading behaves identically) — a pre-existing naming convention, unrelated to this alert.


2. MEDIUM — js/log-injection, ts/src/super-peer.ts:318

Reachable and exploitable, post-authentication. The existing guard stripped only /[\r\n\t]+/, and CodeQL kept flagging it — correctly. A SPARQL parse error embeds the offending token verbatim (expected 'BY', got '<token>'), and a quoted literal may contain anything, so POST /query puts attacker bytes into console.error. Verified end-to-end against main: 7 of 9 separator vectors survive the old guard.

VT U+000B   FF U+000C   NEL U+0085   LS U+2028   PS U+2029   NUL U+0000   ANSI CSI

The ANSI one is the sharp edge — ESC[2K ESC[1G erases the real line and rewrites it, so an operator tailing the log sees the forged text and never sees what it replaced. Receipts and audit records in this estate are read back as evidence, so a forgeable line is worse here than in a typical app.

Fix. New ts/src/log-safe.tssanitizeLogValue() escapes \p{Cc} + \p{Zl} + \p{Zp} (every C0/C1 control, DEL, and both Unicode separators) to a visible \xNN form, so a reader can tell something was neutralized rather than being handed a laundered line, and bounds the field length. Applied at both log boundaries: the request-error path and joinSwarm.

Why the old guard did not satisfy CodeQL: the recognised sanitizer shape is a global replace of a constant string. A quantified class like /[\r\n\t]+/g is not one, so \r and \n are now replaced individually — which is also just clearer.


Regression tests — before / after

ts/src/security-hardening7.test.ts, 12 cases. Run against origin/main sources with the pre-fix guard reinstated, then against this branch:

vs origin/main vs this branch
9 cases fail pass
3 cases pass pass

The 3 that pass both ways are invariant guards, not regression witnesses, and are labelled as such: global Object.prototype is never polluted (true before and after), field length is bounded, and ordinary log text is left unmangled.

Representative failures against main:

✖ a hostile variable name still unifies
    ?__proto__ must match the same rows as any ordinary variable name
    actual: 0,  expected: 6
✖ an aggregate aliased to ?__proto__ lands in the row
    COUNT(...) AS ?__proto__ must produce a real own property — actual: false
✖ an unbound hostile variable reads as null
    actual: {},  expected: null          # Object.prototype itself
✖ Gremlin values()/has() read own properties only
    no node stores "__proto__" — actual: 2, expected: 0
✖ no control character can forge a second log line
✖ an attacker-shaped SPARQL parse error logs as exactly one line

Gates

  • Suite 401/401 (389 on main + 12 new), 0 failures
  • npm run typecheck clean
  • npm run build run before commit; npm run check:dist exit 0
  • Branch cut from origin/main @ dbe854f; still the tip, no rebase needed

Version 0.4.47, following the 95a86b9 precedent of taking the next free number when another is in flight — #33 holds 0.4.46.

Please do not auto-merge.

@mdheller
mdheller force-pushed the fix/codeql-injection-alerts branch 2 times, most recently from 64585c3 to 11546fd Compare July 30, 2026 01:37
@mdheller

Copy link
Copy Markdown
Member Author

Review pass — verified the fix works, then found one gap in it.

Verified the alerts actually clear. Zero open alerts on refs/pull/34/merge (gh api "repos/.../code-scanning/alerts?ref=refs/pull/34/merge&state=open"0). Alerts 34 (js/remote-property-injection, sparql.ts:628) and 28 (js/log-injection, super-peer.ts:318) both resolve on this ref; they stay open against refs/heads/main until merge.

The prototype work holds up. Checked the claims rather than the comments: Object.fromEntries really does land __proto__ as an own data property; no residual { ...binding } spread survives in sparql.ts; cypher.ts routes untrusted keys through ownValue/cloneDict/toPlainRow. The tests assert the right thing — that a hostile variable name still matches rows rather than merely failing safe, which is what stops the fix from trading one silently-wrong answer for another.

Confirmed the log tests are not vacuous. SECURITY: an attacker-shaped SPARQL parse error logs as exactly one line has a continue when the query parses cleanly, which would silently skip its assertions. Probed it: all vectors throw, and the parse error does embed the raw control characters verbatim, so the end-to-end shape of alert 28 is real.

Gap found: sanitizeLogValue did not cover \p{Cf}. \p{Cc}/\p{Zl}/\p{Zp} correctly neutralized NUL, ESC/ANSI CSI, VT, FF, NEL, LS, PS, DEL, CRLF — but U+202E RLO, U+202A-U+202E, the U+2066-U+2069 isolates, U+200B ZWSP, U+200C/D, U+200E/F, U+00AD, U+061C and U+FEFF all passed through unescaped. Trojan Source: no terminator, so nothing breaks — the bytes and what a human reads diverge. In an estate where log records are read back as evidence that is inside the stated "cannot rewrite a log line" guarantee.

The reason it survived is the more important half. The test's general bar was const LINE_BREAKING = /[\p{Cc}\p{Zl}\p{Zp}]/u — character-for-character the class the implementation strips. It could not fail for anything the implementation omitted. Replaced with UNSAFE_IN_A_LOG_FIELD, an explicit range enumeration derived from what a reader/terminal/parser can be made to misrender, each range commented with the behaviour that puts it there, deliberately not expressed in the implementation's own categories.

Teeth proven both directions: widened tests against the pre-fix implementation fail 2/12, first failure RLO U+202E (Trojan Source): a line-breaking/control character survived sanitization — and the end-to-end SPARQL test fails too, so the vectors reach the real log boundary. With \p{Cf} added: 12/12, full suite 422/422.

Rebased onto post-#35 main, so this head carries the kko-provenance job.

@mdheller

Copy link
Copy Markdown
Member Author

@copilot review

@mdheller
mdheller requested a review from Copilot July 30, 2026 01:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Closes two open CodeQL alerts on main by hardening query-result dictionaries against prototype key collisions (remote property injection) and sanitizing untrusted log fields to prevent log forgery.

Changes:

  • Introduces null-prototype “safe dict” utilities and applies them across SPARQL/Cypher/Gremlin/pattern matching binding/row construction.
  • Adds a log-boundary sanitizer and applies it to super-peer error logging and swarm-join warnings.
  • Adds a new security regression test suite covering both alert classes; bumps package version to 0.4.47.

Reviewed changes

Copilot reviewed 11 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
ts/src/superpeer-service.ts Sanitizes untrusted joinSwarm error text before logging.
ts/src/super-peer.ts Replaces ad-hoc newline stripping with centralized log-field sanitization.
ts/src/sparql.ts Switches bindings to null-prototype dictionaries and materializes output rows safely.
ts/src/security-hardening7.test.ts Adds regression/security tests for prototype injection and log-field sanitization.
ts/src/safe-dict.ts Adds null-prototype dict helpers + own-property read + safe row materialization.
ts/src/patternMatcher.ts Uses safe dicts for groundings to prevent prototype-key unification issues.
ts/src/log-safe.ts Adds sanitizeLogValue with control/bidi escaping and length bounds.
ts/src/index.ts Re-exports the new log sanitizer as part of the package API.
ts/src/gremlin.ts Ensures traversal-supplied property keys read own-properties only.
ts/src/cypher.ts Uses own-property param lookup and safe row construction for query-derived keys.
package.json Bumps version to 0.4.47.

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

Comment thread ts/src/log-safe.ts Outdated
Comment on lines +69 to +76
* Guarantees, for any input whatsoever:
* - the result contains no character that can terminate or truncate a log line, and none that
* can rewrite one — whether by driving the terminal (ANSI CSI) or by reordering/hiding what
* a human reads (bidi overrides, zero-width characters)
* - the result is at most `max` characters, plus an explicit truncation marker
*/
export function sanitizeLogValue(value: unknown, max: number = LOG_FIELD_MAX): string {
const raw = typeof value === 'string' ? value : String(value)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepted — real, and sharper than it looks. Fixed in 78ca02a.

Verified both failure modes rather than assuming:

String({ toString() { throw } })   -> Error: boom
String(Object.create(null))        -> TypeError: Cannot convert object to primitive value

The null-prototype case is live because of this PR: safe-dict.ts hands out Object.create(null) dictionaries throughout the engine now, so one reaching a log boundary is not hypothetical. And the call site is the aggravating factor — super-peer.ts calls this from inside its catch block, so a throw would abandon the 500 response and make the sanitizer itself the outage. Exactly the "logging becomes a secondary failure path" you named.

Fix: coerceToString wraps String() in try/catch and falls back to `[unstringifiable ${Object.prototype.toString.call(value)}]` — that reads the internal class directly and never invokes user code, with an outer guard for exotic proxies. The guarantee list now states explicitly that the function returns rather than throws.

The test asserts the precondition first (that String() really does throw for each of the three values) so it cannot silently stop exercising anything, then asserts a safe single-line field comes back. Confirmed it fails against the previous implementation with Error: boom.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepted — and already fixed on this branch in a01b5c7, before this review round.

coerceToString now wraps the coercion, with a second guard behind it:

  • String(Object.create(null)) throws, and safe-dict.ts (added by this same PR) hands out exactly those dictionaries across the engine, so this was reachable rather than hypothetical;
  • a value whose own toString throws.

Both matter specifically because super-peer calls the sanitizer from inside a catch block — a throw here would have swallowed the 500 response and made the log boundary the outage.

The fallback uses Object.prototype.toString.call, which reads the internal class without invoking user code. security-hardening7.test.ts asserts it with a precondition that String() really does throw for each value, so the test cannot quietly stop exercising anything.

Comment on lines +7 to +16
process.env['HELLGRAPH_STORE_DIR'] = mkdtempSync(join(tmpdir(), 'hg-sec7-'))

import { getHellGraph } from './store.js'
import { getAtomSpace } from './atomspace.js'
import { runSparql } from './sparql.js'
import { runGremlin } from './gremlin.js'
import { runCypher } from './cypher.js'
import { sanitizeLogValue, LOG_FIELD_MAX } from './log-safe.js'

const g = getHellGraph()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rejecting — the stated risk is not reachable under this test runner, and the suggested remedy would break the setup. Checked rather than assumed.

1. Cross-file leakage is impossible here. npm test is node --test ts/src/*.test.ts, and Node's runner executes each test file in its own child process. Probed it directly with two files, one setting an env var and the other reading it:

[a] pid=60076  ISO_PROBE=set-by-a
[b] pid=60077  ISO_PROBE=(unset)
[b] -> cross-file env leakage: NO

Different PIDs, and the variable is unset in the second file. So "leak state across the test suite depending on import order" cannot happen — there is no shared process to leak through.

2. The remedy would break it. The assignment is deliberately placed above import { getHellGraph } from './store.js'. The package is "type": "commonjs", so imports transpile to require() in source order and the env var is set before the store module loads. A before() hook runs after module evaluation — too late. Moving it there would silently stop configuring the store dir.

3. It is the established pattern. ts/src/sparql11.test.ts does the same thing for the same reason; this PR did not introduce the convention.

You are right about one thing and it is worth recording: getHellGraph() is singleton-cached — on globalThis.__hellgraph_store__ (store.ts:313). That is a genuine order-dependence hazard in general. It does not bite here because the two module-level nodes (n:1, n:2) are only ever read by these tests — every case runs queries, none mutates the store — so the tests are order-independent in fact.

Leaving as-is.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rejected — the leak this describes cannot occur here, for three independent reasons.

  1. node:test runs each test file in its own process. Verified directly rather than assumed: two files in one node --test invocation report different PIDs. There is no shared module registry for state to leak across, so import order between files is not a factor.
  2. The env assignment must precede the import. HELLGRAPH_STORE_DIR is read in the AtomSpace constructor (atomspace.ts:261), and getHellGraph() runs at module scope. A before() hook runs after module evaluation, so moving it there would mean the store is already built against the wrong directory — the suggestion would break the isolation it is trying to add.
  3. The directory is unique per process. mkdtempSync mints a fresh temp dir per run, so even concurrent runs cannot collide.

This is also the established pattern in this repo — sparql11.test.ts:12 does the same thing for the same reason.

Not restoring process.env afterwards is therefore a no-op: the process exits at the end of the file. Happy to revisit if the runner ever moves to in-process file execution, which is the one assumption this rests on.

mdheller added a commit that referenced this pull request Jul 30, 2026
Accepted from Copilot's review. `String(value)` is not total, and this PR is
what makes the sharp case live.

  String(Object.create(null))  -> TypeError: Cannot convert object to primitive
  String({toString(){throw}})  -> propagates

Verified both. The null-prototype case matters here specifically because
safe-dict.ts, added by this same PR, hands out `Object.create(null)`
dictionaries throughout the engine — so one reaching a log boundary is not
hypothetical. And super-peer calls sanitizeLogValue from INSIDE its catch
block, so a throw there would abandon the 500 response and make the sanitizer
the outage. The documented guarantee is "for any input whatsoever", which has
to include inputs that refuse to stringify.

Added `coerceToString`: String() under try/catch, falling back to
`[unstringifiable <internal class>]` via Object.prototype.toString.call, which
reads the internal class without invoking user code. Outer guard covers exotic
proxies. Guarantee list now states explicitly that it returns rather than
throws.

Test asserts the precondition first (String() really does throw for each of the
three values) so it cannot quietly stop exercising anything, then asserts the
sanitizer returns a safe single-line field for each. Verified it fails against
the previous implementation with "Error: boom".

Copilot AI commented Jul 30, 2026

Copy link
Copy Markdown

@copilot review

Reviewed. The changes hold up:

log-safe.ts — implementation

[\p{Cc}\p{Zl}\p{Zp}\p{Cf}] with the u flag covers all 17 characters in UNSAFE_IN_A_LOG_FIELD. Verified each code point against the pattern directly — every entry resolves to \p{Cf} as expected (\u00AD, \u061C, \u200B–\u200F, \u202A–\u202E, \u2066–\u2069, \uFEFF). Astral \p{Cf} members (U+13430, U+1BCA0) are matched as a whole code point by the u flag, and codePointAt names them correctly in the escape record (\u{13430}, not \ud80d).

security-hardening7.test.ts — test coverage

UNSAFE_IN_A_LOG_FIELD is derived from reader/terminal behaviour, not from the implementation's own categories. Confirmed the tests are not vacuous: stripping \p{Cf} from the implementation causes 5 test vectors to fail (RLO U+202E, LRO U+202D, ZWSP U+200B, SOFT HYPHEN U+00AD, BOM U+FEFF).

Test run: 14/14 pass.

@mdheller

Copy link
Copy Markdown
Member Author

Status — review complete, NOT merged, waiting on CI

Leaving this for Michael rather than merging: the checks on the current head 78ca02a are pending, not green. They have been queued ~35 min behind org-wide Actions contention (hellgraph had 12 runs queued with 0 in-progress while noetica held the runner pool). Nothing has failed — fail=0 — but pending is not green, so it does not meet the merge bar.

Copilot review — 2 comments, both answered

Copilot had never run on this repo; @copilot review as a comment does not register it. The mechanism that works is POST /pulls/<N>/requested_reviewers with copilot-pull-request-reviewer[bot]. Note its review ran in degraded mode"Copilot couldn't run its full agentic review because it didn't start before the timeout", and it covered 11 of 15 changed files. So this is a partial review, not a clean bill.

  • log-safe.tsString(value) can throw. ACCEPTED, fixed in 78ca02a. Genuinely sharp, and live because of this PR: String(Object.create(null)) throws TypeError: Cannot convert object to primitive value, and safe-dict.ts hands out exactly those dictionaries now. super-peer calls the sanitizer from inside its catch block, so the throw would abandon the 500 response. Fixed with a total coerceToString; test asserts the precondition first and was verified to fail against the old code.
  • security-hardening7.test.ts — module-level side effects. REJECTED, with grounds on the thread: node --test runs each file in its own process (proved with a two-file probe — different PIDs, env unset in the second), so the stated cross-file leak cannot occur; and the suggested before() hook would run after module load and silently break the setup.

Verified independently

  • Both target alerts clear: 0 open CodeQL alerts on refs/pull/34/merge.
  • Prototype fix is real — Object.fromEntries lands __proto__ as own data; no residual { ...binding } spread in sparql.ts.
  • Log tests are not vacuous — all 20 SPARQL vectors throw and reach their assertions.
  • Added beyond the original PR: \p{Cf} coverage (Trojan Source — U+202E and friends passed through unescaped), a LINE_BREAKING bar no longer derived from the implementation's own category set, and astral-plane escaping via codePointAt. Each proven to go red before green.
  • Local: 424/424 tests, typecheck clean, ts/dist in sync.

One caveat on what green will mean

main only gained the Test step when #36 merged (97c14e0), after this branch's run was queued. Worth confirming the settled run actually shows Test -> success rather than no Test step at all — if it does not, re-run the checks so the suite genuinely executes against this diff before merging.

Rebased onto post-#35 main; head carries the kko-provenance job.

@mdheller
mdheller requested a review from Copilot July 30, 2026 03:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (5)

ts/src/superpeer-service.ts:84

  • (e as Error).message will be undefined when a non-Error is thrown/rejected, which loses useful diagnostic content and can make multiple distinct failures log identically. Consider mirroring super-peer’s handling here by passing the caught value itself (or using an instanceof Error guard) into sanitizeLogValue, so non-Error throws still produce meaningful output.
      // Same log boundary as super-peer's request handler: this message originates outside the
      // process (swarm/DHT peers), so it is untrusted text and must not be able to forge a line.
      console.warn(`[superpeer] joinSwarm skipped: ${sanitizeLogValue((e as Error).message)}`)

ts/src/safe-dict.ts:39

  • The SafeDict<V> = Record<string, V> type implies Object.prototype methods/properties are available, but emptyDict() returns a null-prototype object where e.g. dict.toString / dict.hasOwnProperty are undefined. This is a real footgun for future callers (TypeScript will allow those accesses, runtime will throw). Consider introducing a branded type (so SafeDict is not freely interchangeable with Record<string, V>), and/or tightening APIs so only toPlainRow() crosses the boundary into normal-prototype objects.
/** A dictionary whose keys are untrusted. Structurally a Record; semantically null-prototype. */
export type SafeDict<V> = Record<string, V>

/** A fresh dictionary with no inherited members. */
export function emptyDict<V>(): SafeDict<V> {
  return Object.create(null) as SafeDict<V>
}

ts/src/security-hardening7.test.ts:7

  • This test file mutates process.env at module load time and never restores it. In runners that execute test files in parallel (or where other tests rely on a different store directory), this can create order-dependent/flaky failures. Consider setting/restoring the env var in a setup/teardown hook (or using a per-test configuration injection) and cleaning up the temp directory afterward.
process.env['HELLGRAPH_STORE_DIR'] = mkdtempSync(join(tmpdir(), 'hg-sec7-'))

ts/src/security-hardening7.test.ts:18

  • These tests share a module-global graph instance and also call getHellGraph() again mid-file to mutate state, which makes the suite sensitive to execution order and can be fragile if the underlying store is a singleton. Prefer creating an isolated graph/store per test (or resetting between tests) so each assertion runs against a known fixture and can’t be affected by prior mutations.
const g = getHellGraph()
g.addNode('n:1', ['Person'], { name: 'Ada', age: 30 })
g.addNode('n:2', ['Person'], { name: 'Alan', age: 40 })

ts/src/security-hardening7.test.ts:138

  • These tests share a module-global graph instance and also call getHellGraph() again mid-file to mutate state, which makes the suite sensitive to execution order and can be fragile if the underlying store is a singleton. Prefer creating an isolated graph/store per test (or resetting between tests) so each assertion runs against a known fixture and can’t be affected by prior mutations.
  getHellGraph().addNode('n:bait', ['Person'], { name: bait })

mdheller added 5 commits July 29, 2026 23:40
…ion + log forgery

Both alerts were open against origin/main @ dbe854f, not introduced by any PR.

HIGH js/remote-property-injection (alert 34, ts/src/sparql.ts:628)
  A Binding is a plain `{}` keyed by SPARQL variable names, and the tokenizer accepts
  /\?[A-Za-z0-9_]+/ — so `?__proto__` and `?constructor` are expressible verbatim in a
  query body POSTed to /api/graph/sparql. That reaches Object.prototype three ways:
    WRITE  `out['__proto__'] = v` hits the inherited setter instead of defining a
           property, so a variable the response DECLARES is absent from every row.
    READ   `sol['constructor']` returns an inherited FUNCTION, which escapes into
           results as if it were a binding value.
    `in`   `'__proto__' in next` is true on a binding that bound nothing, so unify()
           compares against Object.prototype, fails, and the query returns ZERO rows.
  Fix: intermediate bindings are null-prototype dictionaries (new ts/src/safe-dict.ts),
  and output rows are materialized with Object.fromEntries — CreateDataProperty, so a
  hostile name lands as an own data property. Names are NOT rejected: SPARQL says any
  name is a legal variable, so rejecting would trade one silently-wrong answer for
  another. The prior partial fix at the projection site guarded the write but not the
  read; both are closed now.

  Same pattern found and fixed in the siblings rather than left half-done:
    gremlin.ts       has()/values()/order() read n.properties[key] with a traversal-
                     supplied key — g.V().values("constructor") really did hand back
                     the Object constructor. Own-property reads only.
    cypher.ts        row + grounding construction, and `$param` lookup: an unsupplied
                     $constructor resolved to Object.prototype.constructor.
    patternMatcher.ts groundings had the identical `in` bug, which is what made a
                     Cypher variable named __proto__ drop matches.

MEDIUM js/log-injection (alert 28, ts/src/super-peer.ts:318)
  The existing guard stripped only /[\r\n\t]+/. Verified still forgeable end-to-end: a
  SPARQL parse error embeds the offending token verbatim, so POST /query with a hostile
  literal put VT, FF, NEL (U+0085), LS/PS (U+2028/9), NUL and ANSI CSI straight into
  console.error — ESC[2K ESC[1G erases the real line and rewrites it, so an operator
  tailing the log sees only the forged text.
  Fix: new ts/src/log-safe.ts sanitizeLogValue() escapes \p{Cc} + \p{Zl} + \p{Zp} to a
  VISIBLE \xNN form (a reader can see something was neutralized) and bounds the field.
  Applied at both log boundaries: the request-error path and joinSwarm.

Tests: ts/src/security-hardening7.test.ts, 12 cases. 9 of 12 fail against origin/main
and all 12 pass here; the other 3 are invariant guards (global prototype never polluted,
field bounded, ordinary text unmangled) that hold in both directions by design.

Suite 401/401 (389 on main + 12), typecheck clean, dist rebuilt, check:dist 0.
Version 0.4.47: 0.4.46 is being prepared concurrently on #33 (vendor graph).
…lementation

Two findings, and the second is why the first survived review.

1. `sanitizeLogValue` escaped \p{Cc}, \p{Zl} and \p{Zp} — correct for NUL, ESC,
   VT, FF, NEL, LS, PS, DEL and CRLF, all re-verified. It did NOT cover \p{Cf}.
   U+202E RIGHT-TO-LEFT OVERRIDE, the rest of U+202A-U+202E, the U+2066-U+2069
   isolates, U+200B ZWSP, U+200C/D, U+200E/F, U+00AD, U+061C and U+FEFF all
   passed through unescaped.

   These are the Trojan Source class. They contain no terminator, so nothing
   about the line BREAKS — they change what a human reads. `admitted=` + RLO +
   `resu` renders as `admitted=user` while the bytes say otherwise, which means
   an auditor reading a receipt and a tool grepping it disagree about its
   content. The function's stated guarantee is that nothing can "rewrite a log
   line", and in an estate where log records are read back as evidence that
   covers the reader, not just the parser. Added \p{Cf} to the escaped class.

2. The test's general assertion was

       const LINE_BREAKING = /[\p{Cc}\p{Zl}\p{Zp}]/u

   character-for-character the class the implementation strips. It therefore
   could not fail for anything the implementation omitted — it was decoration
   that looked like coverage, and it is precisely why (1) got through. Replaced
   with UNSAFE_IN_A_LOG_FIELD: an explicit range enumeration derived from what
   a reader, terminal or line-oriented parser can be made to misrender, with
   each range commented for the behaviour that puts it there. It is deliberately
   NOT expressed in the implementation's categories, so if the implementation
   later drops a class this list still names, the tests go red.

   Added eleven reader-forgery vectors to FORGERY_VECTORS.

Teeth proven both ways:
  - widened tests vs the PRE-FIX implementation: 2 of 12 FAIL, first failure
    "RLO U+202E (Trojan Source): a line-breaking/control character survived
    sanitization". The end-to-end SPARQL-parse-error test fails too, so the
    vectors reach the real log boundary, not just the unit.
  - with \p{Cf} added: 12/12 pass, full suite 422/422.
  - re-checked that the parse-error test is not vacuous: all 20 vectors throw
    and reach their assertions; none hit the `continue`.
…rogate

Follow-on from the \p{Cf} widening. \p{Cf} extends past the BMP (U+13430-U+1343F
Egyptian format controls, U+1BCA0-U+1BCA3) and the u-flagged class matches those
as a whole code point, so `charCodeAt(0)` reported only the HIGH SURROGATE:
U+13430 printed as `\ud80d`.

Not a security hole — the replace consumes the whole match, so the character is
neutralized either way, verified. But these are escaped rather than deleted
specifically so a reader can see WHAT was neutralized, and naming a code point
that was never in the input defeats that. `codePointAt` + `\u{...}` above U+FFFF.

Test added; verified it fails against the previous implementation with
"U+13430 must be escaped as its own code point, got \"benign\\ud80dtail\"".
Accepted from Copilot's review. `String(value)` is not total, and this PR is
what makes the sharp case live.

  String(Object.create(null))  -> TypeError: Cannot convert object to primitive
  String({toString(){throw}})  -> propagates

Verified both. The null-prototype case matters here specifically because
safe-dict.ts, added by this same PR, hands out `Object.create(null)`
dictionaries throughout the engine — so one reaching a log boundary is not
hypothetical. And super-peer calls sanitizeLogValue from INSIDE its catch
block, so a throw there would abandon the 500 response and make the sanitizer
the outage. The documented guarantee is "for any input whatsoever", which has
to include inputs that refuse to stringify.

Added `coerceToString`: String() under try/catch, falling back to
`[unstringifiable <internal class>]` via Object.prototype.toString.call, which
reads the internal class without invoking user code. Outer guard covers exotic
proxies. Guarantee list now states explicitly that it returns rather than
throws.

Test asserts the precondition first (String() really does throw for each of the
three values) so it cannot quietly stop exercising anything, then asserts the
sanitizer returns a safe single-line field for each. Verified it fails against
the previous implementation with "Error: boom".
Copilot review on #34 (low-confidence channel). `(e as Error).message` reads `.message`
off whatever was thrown; a rejection need not be an Error, so a thrown string or a
plain object logged `undefined` and every non-Error swarm failure produced the same
contentless line.

This PR is what makes handing the raw value over the SAFE choice: `sanitizeLogValue`
now coerces total via `coerceToString`, so it cannot throw on a null-prototype object
or a hostile `toString`. super-peer's request handler already passes the value this
way two files over — this is the same shape, applied consistently.
@mdheller
mdheller force-pushed the fix/codeql-injection-alerts branch from 78ca02a to e0dbccf Compare July 30, 2026 03:41
@mdheller

Copy link
Copy Markdown
Member Author

Review disposition — including the suppressed channel

Copilot's review carries the degraded-mode banner ("couldn't run its full agentic review … make sure your repository has a runner available") and reports 11 of 15 changed files reviewed. Treating this as full coverage would be a mistake, so the diff was also reviewed independently; findings below.

Five further findings were in the review body's <details>Comments suppressed due to low confidence</details> block, which neither issues/34/comments nor pulls/34/comments returns. One of them was real.

Fixed

  • superpeer-service.ts:84(e as Error).message (suppressed channel). Correct, and an inconsistency inside this PR: super-peer.ts passes err instanceof Error ? err.message : err, while this site cast blindly. A rejection need not be an Error, so a thrown string logged undefined and every non-Error swarm failure produced the same contentless line. Fixed in e0dbccf; the point of coerceToString is precisely that handing over the raw value is now the safe option. Regression test added.

Rejected

  • Test isolation (security-hardening7.test.ts, one visible + three suppressed). Answered on-thread: node:test runs each file in its own process (verified — distinct PIDs), the env assignment must precede the import because atomspace.ts:261 reads it in the constructor, and mkdtempSync gives a unique dir per process. Established pattern in sparql11.test.ts.

Deferred, with reason

  • safe-dict.ts:39SafeDict<V> = Record<string, V> is a type-level footgun (suppressed channel). Legitimate: TypeScript permits dict.toString() on the alias while the runtime value is null-prototype, so it typechecks and throws. But branding the type changes every consumer (sparql, cypher, gremlin, patternMatcher), which is a wide refactor of type ergonomics with no security effect — the runtime behaviour is already correct and intentional. Worth doing; not inside a PR whose job is closing two CodeQL alerts.

Independent findings

  • The astral-plane assertion at security-hardening7.test.ts:296 (!LINE_BREAKING.test(out)) is vacuous on its own — UNSAFE_IN_A_LOG_FIELD enumerates BMP ranges only, so it cannot match U+13430 whatever the implementation does. The next assertion (the \u{13430} escape must be present) is the one carrying the weight, so the test is sound; flagging it only because a reader could mistake the first line for coverage.

Verified rather than assumed

  • UNSAFE_IN_A_LOG_FIELD is an explicit code-point enumeration derived from what a reader can be made to misrender — not the implementation's own \p{Cc}\p{Zl}\p{Zp}\p{Cf} categories. That distinction is the whole point, and it holds.
  • Proved the tests go red against pre-fix code, both ways: dropping \p{Cf} from the implementation fails 3 tests; restoring the original .replace(/[\r\n\t]+/g, ' ') guard fails 5. A guard that only goes green proves nothing.

Found reviewing this PR's own diff — Copilot reported 11 of 15 changed files, and
this was in one of the four it did not reach.

`findMatches` was converted to null-prototype groundings, and `unifyTerm` to
`cloneDict`, but the row handed back to callers was still built as

    const row: Record<string, string> = {}
    for (const v of variables) row[v] = ...

keyed by the same query-derived pattern variable names. `row['__proto__'] = v` hits
Object.prototype's inherited SETTER, so the write is swallowed: the variable stays
listed in `variables` and is absent from every row. Reproduced against the pre-fix
code — `declaredInVariables=true ownProperty=false`, and the value reads back as
Object.prototype rather than the atom label.

That is the WRITE mode safe-dict.ts documents, at the boundary of the file whose
interior this PR fixed. The interior was converted and the exit was not.

Now built through `toPlainRow`, which uses CreateDataProperty, so a hostile name
lands as a real own data property — the same treatment cypher.ts already gives its
output rows. Test asserts every HOSTILE_KEYS name is an own data property of the
row and reads back as a string; it is red against the old construction.
@mdheller

Copy link
Copy Markdown
Member Author

Independent finding — a site this PR missed in a file it edited

Copilot reported 11 of 15 changed files. Reviewing the four it did not reach turned up one real defect, now fixed in 454a2c8.

ts/src/patternMatcher.ts had its interior converted — findMatches starts from emptyDict, unifyTerm uses cloneDict — but the row handed back to callers was still:

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

keyed by the same query-derived pattern variable names. Reproduced against the pre-fix code:

normal      : declaredInVariables=true ownProperty=true  typeof=string
__proto__   : declaredInVariables=true ownProperty=false typeof=object  <-- DECLARED COLUMN, MISSING FROM ROW

row['__proto__'] = v hits Object.prototype's inherited setter, the write is swallowed, and the variable stays listed in variables while being absent from every row — reading back as Object.prototype instead of the atom label. That is verbatim the WRITE mode safe-dict.ts describes in its own docstring, and MatchResult.results is public API.

The interior of the file was fixed and the exit was not. Now built through toPlainRow (CreateDataProperty), the same treatment cypher.ts already gives its output rows. The new test is red against the old construction and green against the new one.

A structural note, not a blocker

safe-dict.ts states the rule as a convention — "object spread { ...dict } and object literals re-attach Object.prototype. Use cloneDict / mergeDicts, never a spread." Nothing enforces it. This finding is what that gap looks like in practice: the rule was followed in three places in a file and missed in a fourth, and no gate noticed. A lint rule (no-restricted-syntax on spreads of Binding/Grounding/RRow, or a narrow AST check in the existing test suite) would make the convention checkable. Worth a follow-up rather than expanding this PR.

I swept sparql.ts, cypher.ts, gremlin.ts and patternMatcher.ts for remaining spreads and bare {} initialisers on untrusted-key dictionaries; patternMatcher.ts:79 was the only remaining one.

Gates

npm test 426 pass / 0 fail · npm run typecheck clean · npm run check:kko green · ts/dist rebuilt and committed.

@mdheller

Copy link
Copy Markdown
Member Author

Not merged — CI never ran, and the previous run never ran the tests

Held deliberately. Two separate reasons, both about the suite not having executed against this diff.

1. The earlier green run did not include the Test step

Test is a step inside build-and-verify-dist, not its own check context, so gh pr checks showing that context green says nothing about whether the suite ran. Reading the job steps directly:

job build-and-verify-dist  steps: … | Typecheck=success | Rebuild dist=success | …

No Test between them. main only gained that step with #36, merged at 02:21:46Z — and this PR's run was created at 02:20:24Z, 82 seconds earlier. The same was true of #37 (run 01:51:30Z). Both branches were one commit behind and neither had ever executed the 401-test suite; the green checks were typecheck-and-build only.

Both branches are now rebased onto main, so the Test step is present and will run.

2. The new runs are queued behind an exhausted runner pool

self-hosted runners (org): 0
hellgraph runs queued:    29
hellgraph runs in progress: 0
oldest queued run:        03:13:38Z  (42 minutes and counting)
last run to actually execute: 03:23:42Z

Nothing has started since 03:23. Pending is not green, so this is not being merged on the strength of local results.

What did pass, locally

npm test · npm run typecheck · npm run check:kko · npm run build (dist committed, no drift). Full counts are in the commit messages. That is evidence the change is sound — it is not evidence the required checks passed, and only the latter is a merge criterion.

To merge

Required contexts on main are exactly build-and-verify-dist and CodeQL (strict: false, no required reviews). kko-provenance, docs-integrity and rust report but block nothing. Once runners free up, confirm Test → success inside build-and-verify-dist — not just the context's own conclusion — and then merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants