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

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@socioprophet/hellgraph",
-  "version": "0.4.46",
+  "version": "0.4.47",
   "description": "HellGraph \u2014 TypeScript OpenCog-compatible AtomSpace metagraph engine (PLN, ECAN, pattern matcher, SPARQL/Gremlin, SHACL, Atomese, StorageNode federation). Polyglot sibling of the Rust hellgraph crate.",
   "license": "UNLICENSED",
   "type": "commonjs",

ts/src/cypher.ts

@@ -1,6 +1,7 @@
 import { createHash } from 'node:crypto'
 import { AtomSpace, nodeHandle, linkHandle, type Atom, type Handle } from './atomspace'
 import { findMatches, V, N, L, type Pattern, type PatternTerm, type Grounding } from './patternMatcher'
+import { cloneDict, emptyDict, ownValue, toPlainRow } from './safe-dict.js'
 
 /**
  * Cypher Facade v0.1 — a human/agent-friendly READ query surface over the AtomSpace.
@@ -354,8 +355,10 @@ class Parser {
   }
 
   private resolveVal(tok: string): string {
-    if (tok === '$') return this.params[this.next()] ?? ''
-    if (tok?.startsWith('$')) return this.params[tok.slice(1)] ?? ''
+    // The parameter NAME is query-derived and `params` is caller-supplied, so an own-property
+    // read only: `$constructor` must be an unsupplied parameter, not Object.prototype.constructor.
+    if (tok === '$') return ownValue(this.params, this.next()) ?? ''
+    if (tok?.startsWith('$')) return ownValue(this.params, tok.slice(1)) ?? ''
     return unquote(tok)
   }
 }
@@ -565,7 +568,9 @@ export function runCypher(as: AtomSpace, query: string, params: Record<string, s
 
   // Build one output row from a complete grounding. Returns false when enough rows exist.
   const emit = (g: Grounding): boolean => {
-    const rr: RRow = {}
+    // Keyed by Cypher variable names and `var.prop` paths, all query-derived — null-prototype
+    // so a variable named `__proto__` is stored rather than swallowed (see safe-dict.ts).
+    const rr: RRow = emptyDict<string | number>()
     for (const [v, h] of Object.entries(g)) {
       if (v.startsWith('_')) continue
       const atom = as.getAtom(h)
@@ -622,7 +627,7 @@ export function runCypher(as: AtomSpace, query: string, params: Record<string, s
     for (const [irVar, e] of byVar) {
       const pin = e.pins.size === 1 ? e.pins.values().next().value as Handle : undefined
       if (e.pins.size > 1 || (pin !== undefined && !as.getAtom(pin))) { groundings = []; break }
-      if (pin !== undefined) groundings = groundings.map((g) => ({ ...g, [irVar]: pin }))
+      if (pin !== undefined) groundings = groundings.map((g) => { const n = cloneDict(g); n[irVar] = pin; return n })
       else if (!clauseBound.has(irVar)) { scanVars.push({ irVar, labels: e.labels }); continue }
       for (const label of e.labels) groundings = groundings.filter((g) => atomMatchesLabel(as, as.getAtom(g[irVar]!), label))
     }
@@ -638,7 +643,9 @@ export function runCypher(as: AtomSpace, query: string, params: Record<string, s
           throw new Error(`Cypher: node scan exceeded maxScan ${maxScan} candidates — narrow the pattern with a ` +
             `label, a form pin or an edge, or raise maxScan (Sentinel bounded-traversal policy)`)
         }
-        if (!walk(i + 1, { ...g, [scanVars[i]!.irVar]: h })) return false
+        const next = cloneDict(g)
+        next[scanVars[i]!.irVar] = h
+        if (!walk(i + 1, next)) return false
       }
       return true
     }
@@ -690,11 +697,15 @@ export function runCypher(as: AtomSpace, query: string, params: Record<string, s
         .map((c) => ({ col: c, ref: { var: c } }))
     : ast.ret.map((c) => ({ col: c.prop ? `${c.var}.${c.prop}` : c.var, ref: c }))
   const outCols = outRefs.map((c) => c.col)
-  let outRows = rows.map((r) => {
-    const o: Record<string, string> = {}
-    for (const { col, ref } of outRefs) { const v = refValue(r, ref); o[col] = v === undefined ? '' : String(v) }
-    return o
-  })
+  // Column names are query-derived (RETURN aliases). Materialize via toPlainRow so a hostile
+  // name lands as an own data property instead of hitting the inherited __proto__ setter and
+  // leaving a DECLARED column missing from every row.
+  let outRows = rows.map((r) =>
+    toPlainRow(outRefs.map(({ col, ref }) => {
+      const v = refValue(r, ref)
+      return [col, v === undefined ? '' : String(v)] as const
+    })),
+  )
 
   outRows = outRows.slice(0, Math.min(ast.limit ?? maxRows, maxRows))
   opts.onEvidence?.({ queryHash, space: as.id, mode: opts.mode ?? 'operational', columns: outCols, rowCount: outRows.length, evaluatedAtSeq: as.logicalClock, useSpace: ast.useSpace })

ts/src/gremlin.ts

@@ -1,5 +1,12 @@
 import type { HellGraphStore } from './store'
 import type { GraphNode, GraphEdge, GremlinResult, PropertyValue } from './types'
+import { ownValue } from './safe-dict.js'
+
+/** Read a property whose KEY came from the traversal text. Own properties only: without this,
+ *  `values("constructor")` / `values("toString")` hand the caller inherited Object.prototype
+ *  FUNCTIONS as if they were stored graph data, and `has("constructor", …)` compares against
+ *  one (js/remote-property-injection — see safe-dict.ts). */
+const prop = (e: GraphNode | GraphEdge, key: string): PropertyValue | undefined => ownValue(e.properties, key)
 
 /**
  * A Gremlin/TinkerPop-style traversal engine over HellGraph's property graph.
@@ -29,7 +36,7 @@ export class GraphTraversal {
   }
 
   has(key: string, value: PropertyValue): GraphTraversal {
-    return this.derive(this.nodes().filter((n) => looseEq(n.properties[key], value)))
+    return this.derive(this.nodes().filter((n) => looseEq(prop(n, key), value)))
   }
 
   out(label?: string): GraphTraversal {
@@ -55,7 +62,7 @@ export class GraphTraversal {
   // ─── Terminal-ish steps ──────────────────────────────────────────────────
 
   values(key: string): GraphTraversal {
-    const out = this.current.map((t) => (isNode(t) || isEdge(t)) ? t.properties[key] : t).filter((v) => v !== undefined)
+    const out = this.current.map((t) => (isNode(t) || isEdge(t)) ? prop(t, key) : t).filter((v) => v !== undefined)
     return this.derive(out as Traverser[])
   }
 
@@ -77,8 +84,8 @@ export class GraphTraversal {
 
   order(key: string, desc = false): GraphTraversal {
     const sorted = [...this.current].sort((a, b) => {
-      const av = isNode(a) || isEdge(a) ? a.properties[key] : a
-      const bv = isNode(b) || isEdge(b) ? b.properties[key] : b
+      const av = isNode(a) || isEdge(a) ? prop(a, key) : a
+      const bv = isNode(b) || isEdge(b) ? prop(b, key) : b
       const an = Number(av), bn = Number(bv)
       const cmp = !Number.isNaN(an) && !Number.isNaN(bn) ? an - bn : String(av ?? '').localeCompare(String(bv ?? ''))
       return desc ? -cmp : cmp

ts/src/index.ts

@@ -54,6 +54,7 @@ export * from './metta-eval'
 export * from './vendor-cache'
 export * from './metrics'
 export * from './rate-limit'
+export * from './log-safe'
 export * from './discourse'
 export * from './graph-analytics'
 export * from './attribute-profile'

ts/src/log-safe.ts

@@ -0,0 +1,119 @@
+/**
+ * Log-boundary sanitizer.
+ *
+ * Every log line this estate writes is potential evidence: receipts, audit records and
+ * operator forensics are all read back as fact. A log line an attacker can *shape* is
+ * therefore worse here than in a typical application — it does not merely add noise, it
+ * lets an attacker author entries a later reader attributes to the system.
+ *
+ * A log record is line-oriented, so the whole attack reduces to "get a line terminator into
+ * a field". Stripping CR/LF alone is NOT enough. The previous guard in super-peer.ts did
+ * exactly that — `.replace(/[\r\n\t]+/g, ' ')` — and still let all of these through:
+ *
+ *   U+000B VT, U+000C FF   line breaks to many log viewers and pagers
+ *   U+0085 NEL             a C1 line break; a real terminator to Unicode-aware readers
+ *   U+2028 LS, U+2029 PS   line/paragraph separators; break lines in JS-based tooling
+ *   U+0000 NUL             truncates the record in C-string consumers
+ *   U+001B ESC             ANSI CSI: `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
+ *
+ * Breaking the line is not the only way to forge one. A record is only evidence once a HUMAN
+ * reads it, so anything that changes what the reader sees is in scope too — `\p{Cf}` format
+ * characters do exactly that without touching a single line terminator:
+ *
+ *   U+202E RLO             the Trojan Source class. `admitted=` + RLO + `resu` renders as
+ *   U+202A-U+202D          `admitted=user` while the bytes say something else, so an auditor
+ *   U+2066-U+2069          reading the log and a tool grepping it disagree about its content
+ *   U+200B ZWSP, U+FEFF    invisible: splits `admitted` so a search for it never matches
+ *   U+200E LRM, U+200F RLM reorder the remainder of the field
+ *   U+00AD SOFT HYPHEN     invisible token split, same effect as ZWSP
+ *
+ * The rule is therefore a category allow-list, not a deny-list of specific characters:
+ * `\p{Cc}` (every C0 control, DEL, every C1 control — so NUL, ESC, VT, FF, NEL are all in),
+ * `\p{Zl}` / `\p{Zp}` (U+2028 / U+2029), and `\p{Cf}` (every bidi control, every zero-width,
+ * the BOM) are rendered as a VISIBLE escape. Visible rather than deleted, so a reader can tell
+ * something was neutralized instead of silently being handed a laundered line.
+ *
+ * A category allow-list is the point: it is chosen so that a character nobody thought of is
+ * covered by the CLASS it belongs to. `\p{Cf}` was the category originally missed — the tests
+ * did not catch it because their assertion was written from this same set, so it could not
+ * fail for anything this set omitted. See the note above `UNSAFE_IN_A_LOG_FIELD` in
+ * security-hardening7.test.ts: that regex is deliberately derived from what a READER can be
+ * made to misrender, never from what this function happens to strip.
+ */
+
+/**
+ * Render one control character as a visible, unambiguous escape.
+ *
+ * `codePointAt`, not `charCodeAt`: \p{Cf} reaches beyond 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. `charCodeAt(0)` would report only the HIGH SURROGATE, printing `\ud80d` for U+13430.
+ * The character is neutralized either way — the replace consumes the whole match — but the
+ * escape is the forensic record of what was neutralized, and naming the wrong code point
+ * defeats the reason these are made visible instead of deleted.
+ */
+function escapeControl(ch: string): string {
+  const code = ch.codePointAt(0) ?? 0
+  const hex = code.toString(16)
+  if (code <= 0xff) return '\\x' + hex.padStart(2, '0')
+  return code <= 0xffff ? '\\u' + hex.padStart(4, '0') : '\\u{' + hex + '}'
+}
+
+/** Hard cap so one request cannot flood the log (and so a record stays greppable). */
+export const LOG_FIELD_MAX = 500
+
+/**
+ * Coerce any value to a string WITHOUT being able to throw.
+ *
+ * `String(value)` is not total, and both failing cases are reachable from this module's callers:
+ *
+ *   - `String(Object.create(null))` throws `TypeError: Cannot convert object to primitive value`
+ *     — it has no inherited `toString`/`valueOf`. `safe-dict.ts` hands out exactly such
+ *     dictionaries throughout the engine, so one reaching a log boundary is not hypothetical.
+ *   - a value whose own `toString` throws.
+ *
+ * Either would throw from INSIDE `super-peer`'s catch block, replacing the 500 response with an
+ * unhandled error — the sanitizer would become the outage. A log boundary must never be a
+ * secondary failure path, and the guarantee below says "any input whatsoever".
+ *
+ * `Object.prototype.toString.call` is used for the fallback tag because it reads the internal
+ * class directly and never invokes user code; the outer guard covers exotic proxies.
+ */
+function coerceToString(value: unknown): string {
+  if (typeof value === 'string') return value
+  try {
+    return String(value)
+  } catch {
+    try {
+      return `[unstringifiable ${Object.prototype.toString.call(value)}]`
+    } catch {
+      return '[unstringifiable]'
+    }
+  }
+}
+
+/**
+ * Render an untrusted value as a single-line, unforgeable log field.
+ *
+ * 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
+ *   - it returns; it never throws, whatever it is handed (see `coerceToString`)
+ */
+export function sanitizeLogValue(value: unknown, max: number = LOG_FIELD_MAX): string {
+  const raw = coerceToString(value)
+  const escaped = raw
+    // CR and LF first, and individually. Besides being the obvious vector, this exact shape
+    // — a global replace of a constant string — is the barrier CodeQL's js/log-injection
+    // recognises. A quantified character class such as /[\r\n\t]+/g is not recognised, which
+    // is why the previous guard kept alerting even though it did remove newlines.
+    .replace(/\r/g, '\\r')
+    .replace(/\n/g, '\\n')
+    // Everything else that can break, truncate or rewrite a line — including \p{Cf}, which
+    // rewrites what the READER sees (bidi override, zero-width) without touching a terminator.
+    .replace(/[\p{Cc}\p{Zl}\p{Zp}\p{Cf}]/gu, escapeControl)
+  return escaped.length > max ? escaped.slice(0, max) + '[truncated]' : escaped
+}

ts/src/patternMatcher.ts

@@ -1,4 +1,5 @@
 import { AtomSpace, nodeHandle, type Atom, type Handle } from './atomspace'
+import { cloneDict, emptyDict, toPlainRow } from './safe-dict.js'
 
 /**
  * Pattern Matcher — native hypergraph query over the AtomSpace.
@@ -46,7 +47,10 @@ export const L = (type: string, ...outgoing: PatternTerm[]): Extract<PatternTerm
 // ─── Matcher ───────────────────────────────────────────────────────────────────
 
 export function findMatches(as: AtomSpace, pattern: Pattern): MatchResult {
-  let groundings: Grounding[] = [{}]
+  // Null-prototype: pattern variable names reach here from Cypher query text, and on a plain
+  // object `'__proto__' in binding` below is spuriously true — unifyTerm would then compare the
+  // candidate handle against Object.prototype, fail, and drop every match (see safe-dict.ts).
+  let groundings: Grounding[] = [emptyDict<Handle>()]
 
   for (const clause of pattern.clauses) {
     const next: Grounding[] = []
@@ -71,14 +75,17 @@ export function findMatches(as: AtomSpace, pattern: Pattern): MatchResult {
   })
 
   const variables = pattern.select ?? collectVars(pattern.clauses)
-  const results = groundings.map((g) => {
-    const row: Record<string, string> = {}
-    for (const v of variables) {
+  // Output rows are keyed by pattern variable names, which arrive from Cypher query text — so this
+  // is the same untrusted-key surface as the groundings above, at the boundary rather than inside
+  // it. Built through toPlainRow (CreateDataProperty) instead of `row[v] = …` on a plain object:
+  // assigning `__proto__` hits Object.prototype's inherited SETTER, the write is swallowed, and the
+  // variable stays listed in `variables` while being absent from every row — a declared column that
+  // silently is not there, which is the WRITE mode described in safe-dict.ts.
+  const results = groundings.map((g) =>
+    toPlainRow(variables.map((v) => {
       const atom = g[v] ? as.getAtom(g[v]) : undefined
-      row[v] = atom ? (atom.name ?? atom.type) : ''
-    }
-    return row
-  })
+      return [v, atom ? (atom.name ?? atom.type) : ''] as const
+    })))
 
   return { variables, results, groundings, evaluatedAtSeq: as.logicalClock }
 }
@@ -103,7 +110,9 @@ function unifyTerm(as: AtomSpace, term: PatternTerm, handle: Handle, binding: Gr
         const atom = as.getAtom(handle)
         if (!atom || !as.types.isA(atom.type, term.type)) return null
       }
-      return { ...binding, [term.name]: handle }
+      const bound = cloneDict(binding)
+      bound[term.name] = handle
+      return bound
     }
     case 'node':
       return nodeHandle(term.type, term.name) === handle ? binding : null

ts/src/safe-dict.ts

@@ -0,0 +1,68 @@
+/**
+ * Null-prototype dictionaries for untrusted keys.
+ *
+ * Every query surface in this engine keys a plain object BY NAMES THAT CAME OFF THE WIRE:
+ * SPARQL solution bindings by variable name, Cypher rows by variable / RETURN alias, Cypher
+ * `$params` by parameter name, Gremlin property lookups by property key, pattern-matcher
+ * groundings by pattern variable. On an ordinary `{}` every such name that collides with a
+ * member of Object.prototype is a live wire (js/remote-property-injection), in three distinct
+ * ways — all three are real, and the middle one is the easiest to miss:
+ *
+ *   WRITE   `row['__proto__'] = v` hits the inherited accessor instead of defining a property.
+ *           The column is DECLARED in the response and then absent from every row: a
+ *           silently-wrong result, not an error.
+ *
+ *   READ    `props['constructor']` / `props['toString']` return inherited FUNCTIONS. Those
+ *           escape into query results as if they were data (`g.V().values("constructor")`
+ *           really does hand back the Object constructor), and feed comparisons that then
+ *           match rows no user ever stored.
+ *
+ *   `in`    `'__proto__' in binding` is true on an object that bound nothing. Unification
+ *           then compares the candidate against Object.prototype, fails, and the query
+ *           silently returns ZERO rows.
+ *
+ * A null-prototype object has no inherited members, so a variable named ?__proto__ or
+ * ?constructor is simply an ordinary variable — which is what every one of these query
+ * languages says it is. That is why this is the fix rather than rejecting such names:
+ * rejecting them would trade one silently-wrong answer for a different one.
+ *
+ * NB: object spread (`{ ...dict }`) and object literals re-attach Object.prototype. Use
+ * cloneDict / mergeDicts, never a spread, to carry one of these forward.
+ */
+
+/** 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>
+}
+
+/** Copy `src`'s own entries into a fresh null-prototype dictionary. */
+export function cloneDict<V>(src: SafeDict<V>): SafeDict<V> {
+  return Object.assign(Object.create(null), src) as SafeDict<V>
+}
+
+/** Merge two dictionaries into a fresh null-prototype one; `b` wins collisions. */
+export function mergeDicts<V>(a: SafeDict<V>, b: SafeDict<V>): SafeDict<V> {
+  return Object.assign(Object.create(null), a, b) as SafeDict<V>
+}
+
+/**
+ * Read a key that came from untrusted input, from an object that may have a normal prototype.
+ * Returns undefined for anything not stored as an OWN property, so `constructor`, `toString`
+ * and friends read as "absent" rather than as inherited functions.
+ */
+export function ownValue<V>(src: Record<string, V> | undefined, key: string): V | undefined {
+  if (!src) return undefined
+  return Object.prototype.hasOwnProperty.call(src, key) ? src[key] : undefined
+}
+
+/**
+ * Materialize an output row for consumers: a NORMAL-prototype object, so callers get a plain
+ * JSON-shaped value, but built with Object.fromEntries — which uses CreateDataProperty, so even
+ * a `__proto__` key lands as a real own data property instead of invoking the inherited setter.
+ */
+export function toPlainRow<V>(entries: Iterable<readonly [string, V]>): Record<string, V> {
+  return Object.fromEntries(entries) as Record<string, V>
+}