diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2e85c04..ed9bde4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,6 +26,12 @@ jobs: - run: npm ci - run: npm test + # The filter-to-SQL compiler is an exercise rather than a deliverable, but + # its suite is where the §7 requirement that operands be bound and never + # interpolated is actually checked. A test that does not run enforces + # nothing, so it runs here. + - run: npm run test:experiment + # The 0.6.0 bin shipped as a silent no-op because nothing exercised the packed # artifact. Package managers disagree about how they link a bin — npm and yarn # symlink, pnpm writes a shim, Berry's PnP has no node_modules at all, and diff --git a/CHANGELOG.md b/CHANGELOG.md index beff949..19de636 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,61 @@ minor release may break compatibility, in which case the break is spelled out be ## [Unreleased] +### Changed + +- **Operands MUST be bound, and duplicate member names MUST be rejected.** Two normative + requirements the specification relied on and never stated, argued in + [`decisions/0002`](https://github.com/OpenPredicate/open-predicate/blob/main/decisions/0002-compilation-safety-and-duplicate-members.md). + Both were surfaced by writing the security considerations for the IANA media type registration, + which RFC 6838 §4.6 makes mandatory — the honest answer required stating, in a document that cites + `SPEC.md` as the published specification, that the specification did not address them. + + [SPEC.md §7](./SPEC.md#7-safety-limits) now says that operands are caller-supplied and MUST be + passed to a backend as bound parameters or through its own escaping facility, and MUST NOT be + concatenated into a statement. The premise of this project is that a JSON predicate compiles to a + backend query, so this is the requirement that decides whether an implementation is safe — and it + appeared nowhere in the normative text. The project's position already existed everywhere else: + `SECURITY.md` lists an injection defect in the filter-to-SQL compiler as in scope, and + `experiments/filter-to-sql` has bound every operand from the start. + + The same section adds the asymmetric rule for field paths, which usually cannot be bound as + parameters: a path MUST be resolved against the exposed field set (§3.5) and anything + unrecognised rejected with `unknown-field`, rather than escaped and passed through. Refusing an + unknown path is a whitelist; escaping one is a guess about the backend's quoting rules. + + [SPEC.md §2.3](./SPEC.md#23-the-filter-document) is new and settles duplicate member names. + RFC 8259 §4 says names SHOULD be unique and leaves the outcome undefined otherwise, which is + tolerable for a document and not for a predicate: this design deliberately validates with a JSON + Schema and evaluates somewhere else, so two parsers see the filter. Where they resolve + `{"$gt": 18, "$gt": 65}` differently, the filter that was approved is not the filter that ran and + neither component has malfunctioned. An implementation MUST now reject a duplicate member name + with `malformed-query` rather than prefer an occurrence. The subsection also records that the §7 + limits bound evaluation rather than parsing, so input must be bounded independently — a parser + exhausted by depth has failed before any limit applies. + + **Migration.** Nothing changes for a caller except one case that never had a defined meaning: a + filter containing a duplicate member name was previously accepted according to whichever reading + the implementation's parser happened to take, and is now rejected. For an implementer, both are + no-ops if operands are already bound and the JSON parser already rejects duplicates; otherwise the + first describes a vulnerability rather than introducing one, and the second needs a check at the + read boundary. `JSON.parse` cannot report duplicates, so this binds the parser, not the validator + — which is also why no fixture can express it, as the record explains. + + **The grammar is unchanged.** No operator is added, removed or altered, the schema is + byte-identical and the `$id` still names `v0.4.0`. Both requirements constrain implementations + rather than the grammar, and §9 ties the `$id` to the grammar — the same reasoning that left the + `$id` alone through 0.5.0 and 0.6.0. + +### Added + +- **The injection requirement is checkable.** + `experiments/filter-to-sql/compile.test.mjs` drives five hostile operands, each carrying a unique + sentinel, through every operator family that takes a string operand, in both dialects. For each it + requires one of exactly two outcomes: the operand is refused with a `QueryProblem`, or it is bound + — never interpolated, and never silently dropped, since a dropped predicate widens the result set. + Refusal counts as the stronger answer: `$like` legitimately rejects a lone `\'` as an invalid + escape. Verified by mutation — making the compiler interpolate strings fails all three tests. + ## [0.6.2] — 2026-09-14 **A packaging release, again.** The schema is still byte-identical and the `$id` still names diff --git a/SPEC.md b/SPEC.md index a2006c2..5043677 100644 --- a/SPEC.md +++ b/SPEC.md @@ -100,6 +100,36 @@ Each member of `fields` describes one queryable path: `operators` tells a client what it may write; `type`, `format` and `values` tell it *what to write*. The grammar cannot carry that second half: field names are constrained through `propertyNames`, but every path shares one `Constraint` definition, so per-field operand domains are not expressible in the schema (§6). A filter naming a real field with a value outside that field's domain is therefore well-formed, and matches nothing — the failure surfaces as an empty result set rather than an error (§8). The capability document is the only place the domain can be stated, which is why `values` is RECOMMENDED wherever the domain is closed. +### 2.3 The filter document + +A filter arrives as JSON. Two properties of JSON itself decide what an implementation can safely +promise about it, and both have to be settled before the filter is validated rather than after. + +**Duplicate member names MUST be rejected.** [RFC 8259](https://www.rfc-editor.org/rfc/rfc8259) §4 +says names SHOULD be unique and leaves the outcome undefined when they are not: parsers variously +keep the first, keep the last, or retain both. That is tolerable for a document and not for a +predicate, because a filter is validated by one component and evaluated by another. Given + +```json +{ "age": { "$gt": 18, "$gt": 65 } } +``` + +a validator that keeps the first member approves a filter an evaluator that keeps the last then +executes, and no layer has malfunctioned. An implementation MUST reject an object containing a +duplicate member name with `malformed-query` (§8), and MUST NOT resolve the duplication by +preferring one occurrence. + +This cannot be expressed in the schema. JSON Schema constrains a parsed instance, and by then the +duplication has already collapsed — so the requirement binds the parser, not the validator. An +implementation whose JSON parser cannot report duplicates needs to detect them while reading the +document, or replace the parser. + +**Validation precedes evaluation, and parsing precedes both.** The limits of §7 bound what an +implementation will evaluate; they do not bound what it will parse, and a parser exhausted by +nesting depth has already failed before any of them applies. An implementation MUST therefore bound +the input independently of the grammar — by request body size at least, which §7 RECOMMENDS +anyway — and reject anything it cannot parse within those bounds with `malformed-query`. + ## 3. Data model and field paths ### 3.1 Records @@ -459,6 +489,19 @@ For `$regex`, a linear-time engine (RE2, Rust `regex`, Go `regexp`) is strongly Fields backed by unindexed storage are their own denial-of-service surface. An implementation SHOULD restrict expensive operators to indexed fields through its capability document rather than accepting them and timing out. +**Operands are attacker-controlled and MUST NOT be interpolated into a backend query.** A filter +compiles to a query, and every operand in it came from the caller. An implementation MUST pass +operands to its backend as bound parameters, or otherwise through the backend's own escaping +facility; it MUST NOT build a query by concatenating operand values into a statement. This is the +one safety requirement that no limit in this section mitigates: a bound operand of any length is +inert, and an interpolated one of any length is not. + +Field paths are the harder half, because an identifier usually cannot be bound as a parameter. A +path is caller-supplied too, so an implementation MUST resolve each path against the field set it +exposes (§3.5) and reject anything unrecognised with `unknown-field` (§8) — rather than escaping a +path and passing it through. Refusing an unknown path is a whitelist; escaping one is a guess about +the backend's quoting rules. + `$some` and `$every` are the expensive operators on most backends: each one is a traversal of an array, and a nested quantifier is a traversal per element. They count toward the nesting-depth and clause limits like any other clause, and — unlike the `[*]` path segment they replaced — a server that cannot afford them can decline the whole `collections` profile (§2.1) instead of having to accept them as part of the path grammar. An implementation SHOULD publish a lower `maxDepth` for filters containing quantifiers if its storage makes them disproportionately costly. ## 8. Errors diff --git a/decisions/0002-compilation-safety-and-duplicate-members.md b/decisions/0002-compilation-safety-and-duplicate-members.md new file mode 100644 index 0000000..16b8861 --- /dev/null +++ b/decisions/0002-compilation-safety-and-duplicate-members.md @@ -0,0 +1,140 @@ +# 0002 — Compilation safety, and duplicate member names + +**Status:** proposed · **Affects:** SPEC.md §2.3 (new), §7 · **Grammar version:** unchanged + +## Context + +Writing the IANA registration for `application/vnd.openpredicate+json` surfaced two requirements the +specification relies on and never states. RFC 6838 §4.6 makes a security considerations section +mandatory and requires, among other things, that a registration referencing an existing format +describe the issues specific to using *that* format. Answering it honestly meant writing, in a +document that points at `SPEC.md` as the published specification, that the specification does not +address the thing every implementation has to get right. + +Two gaps, from opposite ends of the pipeline. + +**The specification never mentions injection.** The premise of this project is that a JSON-encoded +predicate compiles to a backend query. Every operand in it is caller-supplied. `SPEC.md` contains no +occurrence of *injection*, *bound*, *parameter* in that sense, or *escape* outside the `$like` +grammar — so the one obligation that decides whether an implementation is safe or catastrophic is +absent from the normative text. The project's position exists everywhere else: `SECURITY.md` lists +"an injection or escaping defect in the filter-to-SQL compiler" as in scope for a security report, +and `experiments/filter-to-sql` binds every operand and has done from the first commit. Only the +specification is silent, which is the one place an independent implementer reads. + +**Duplicate member names are undefined, and this architecture is unusually exposed to that.** RFC +8259 §4 says object member names SHOULD be unique and declines to define the outcome when they are +not. The specification inherits that silence. It matters here more than it would elsewhere, because +the design deliberately separates validation from evaluation: a JSON Schema approves the filter, and +something else executes it. Those are two parsers. Where they resolve `{"$gt": 18, "$gt": 65}` +differently, the filter that was approved is not the filter that ran, and neither component has +malfunctioned. For a predicate that carries tenancy or visibility, that is an authorization bypass +assembled entirely out of conforming parts. + +### What this record does not do + +It does not add, remove or change an operator, and it does not touch the schema. It changes what a +conforming implementation must do with a filter before and after the grammar has its say. + +## Decision 1 — operands are bound, paths are whitelisted + +§7 gains two paragraphs. Operands MUST be passed to the backend as bound parameters or through its +own escaping facility, and MUST NOT be concatenated into a statement. + +The second paragraph is the one worth arguing. Field paths are caller-supplied in exactly the same +way, and an identifier generally cannot be bound as a parameter — so the symmetric rule is +unavailable and the temptation is to escape the path instead. The record rejects that. An +implementation MUST resolve each path against the field set it exposes (§3.5) and reject anything +unrecognised with `unknown-field`. Refusing an unknown path is a whitelist and is decidable from +information the implementation already has; escaping one is a guess about the backend's quoting +rules, and quoting rules are where identifier injection lives. §3.5 and §8 already provide both +halves of the whitelist, so this states a consequence rather than inventing a mechanism. + +Placing this in §7 is a small stretch of that section's title — *Safety limits* — but the section +already carries safety guidance that is not a limit: prefer a linear-time regex engine, restrict +expensive operators to indexed fields, publish a lower `maxDepth` for quantifiers. A separate +section would have been tidier and would have renumbered §8 through §10 and every cross-reference in +the README, the schema descriptions, the site and `decisions/0001`. Not worth it. + +## Decision 2 — duplicate member names are rejected, at the parser + +A new §2.3 requires an implementation to reject an object containing a duplicate member name with +`malformed-query`, and forbids resolving the duplication by preferring an occurrence. + +Rejection rather than a defined precedence rule. Defining "last wins" would make the outcome +predictable and would not fix anything: the two parsers in a conforming deployment are usually not +both ours, so a rule we state cannot make a third-party JSON parser obey it. A filter whose meaning +depends on a behaviour the specification cannot enforce should not be accepted at all. Rejection is +also the only option that fails loudly, which is the stance the rest of the document takes — §2.1 +forbids silently ignoring an unsupported operator, §7 forbids truncating an over-limit filter, and +both for the same reason: a filter that matches more than it says is the worst failure available. + +**This requirement cannot be tested through the schema, and that is the point.** JSON Schema +constrains a parsed instance; by then the duplication has collapsed and `JSON.parse` has already +chosen a winner. So the requirement binds the parser, and §2.3 says so explicitly. The test suite +cannot express it, and no fixture is added — a fixture would have to be a *document*, and +`tests/fixtures/` holds parsed instances. An implementer whose parser cannot report duplicates has +to detect them while reading or change parser, which §2.3 states. + +The same subsection records the adjacent ordering constraint, because it has the same shape and the +same cause: the §7 limits bound evaluation, not parsing, and a parser exhausted by nesting depth has +failed before any limit applies. An implementation must bound its input independently. + +## Migration + +Nothing to migrate for a caller. No filter that was valid becomes invalid, with one exception that +was never well-defined: a filter containing a duplicate member name was previously accepted by +whichever reading the implementation's parser happened to take, and is now rejected with +`malformed-query`. No such filter had a specified meaning, so none can be said to have worked. + +For an implementer, both decisions are conformance requirements that may already hold: + +- If operands are already bound, Decision 1 is a no-op. If they are interpolated, that is a + vulnerability being described rather than introduced. +- If the JSON parser rejects or reports duplicates, Decision 2 is a no-op. Several do not by + default, and the check has to be added at the read boundary. + +## Costs and risks + +- **§7's title is now slightly wrong.** Accepted, with the renumbering argument above. If §7 is ever + split, the title goes with it. +- **§2.3 asks for something some parsers make awkward.** `JSON.parse` offers no duplicate reporting; + detection means a streaming parser, a reviver-based trick, or a pre-scan. This is a real cost + imposed on implementers, and it buys the elimination of a divergence class that is otherwise + undetectable at run time. +- **The registration now cites sections that must not drift.** The IANA submission quotes §2.3 and + §7 by number. A future renumbering would strand it. +- **Neither decision is enforced by the test suite for its own sake.** Decision 1 gains a real + regression test in `experiments/filter-to-sql`; Decision 2 gains none, for the structural reason + above. A normative requirement no test can reach is weaker than one a test can, and that asymmetry + is recorded rather than hidden. + +## Open question + +**Should `malformed-query` be the condition for a duplicate member, or should there be a distinct +one?** Settled: `malformed-query`, whose §8 definition is "the body does not conform to the schema: +unknown operator, wrong operand type, structural error". A duplicate member is a structural error and +needs no new problem type; a new type would also mean a new URI under +`https://openpredicate.tech/problems/`, and five conditions that each mean something distinct are +worth more than six where two overlap. Reconsider if implementers report that clients cannot tell a +duplicate member from a schema violation and that the distinction changes what a client does about +it — the `pointer` of §8 already locates the clause either way. + +## Follow-ups, explicitly out of scope + +- Defining the media type itself in `SPEC.md`. The registration needs a normative §Media type + section with the parameters it declares; that is its own record, and this one only fixes what the + registration's security considerations exposed. +- The `profile` media type parameter, deliberately not registered for now. +- Whether §7's RECOMMENDED limits are the right numbers. Untouched here. +- Guidance on composing a caller's predicate with a server's authorization predicate. The + registration says it must be by conjunction; the specification still does not, because the + enclosing request body is out of scope per §1 and the right home is unclear. + +## Results + +`SPEC.md` gains §2.3 and two paragraphs in §7. The schema is byte-identical and the `$id` still +names `v0.4.0`: both decisions constrain implementations rather than the grammar, and §9 ties the +`$id` to the grammar — the same reasoning that left the `$id` alone through 0.5.0 and 0.6.0. +`experiments/filter-to-sql/compile.test.mjs` gains a test asserting that no operand appears +literally in emitted SQL, in both dialects, which is Decision 1 made checkable. diff --git a/experiments/filter-to-sql/compile.test.mjs b/experiments/filter-to-sql/compile.test.mjs index 671b742..fe74f99 100644 --- a/experiments/filter-to-sql/compile.test.mjs +++ b/experiments/filter-to-sql/compile.test.mjs @@ -3,10 +3,29 @@ import assert from "node:assert/strict"; import { CASES, expectationFor, bindingsDiverge } from "./cases.mjs"; import { DOCUMENT_BINDING, HYBRID_BINDING, RECORDS } from "./dataset.mjs"; -import { openDatabase, search } from "./harness.mjs"; import { compile, parsePath, QueryProblem } from "./compile.mjs"; -const db = openDatabase(); +/** + * The database is a Node 22.5+ builtin, and this suite runs on every version + * package.json claims to support. So the harness is loaded conditionally: where + * `node:sqlite` is missing the executed half of the suite reports as skipped + * rather than failing, and the compiler-level half — including the §7 binding + * rule this suite exists to enforce — still runs everywhere. + */ +let harness = null; +let unavailable = null; +try { + await import("node:sqlite"); + harness = await import("./harness.mjs"); +} catch (error) { + if (error?.code !== "ERR_UNKNOWN_BUILTIN_MODULE") throw error; + unavailable = `node:sqlite is unavailable on ${process.version} (needs Node 22.5+)`; +} + +const search = (...args) => harness.search(...args); +const db = harness?.openDatabase() ?? null; +// `skip: false` runs the test normally, so one options object covers both cases. +const executed = { skip: unavailable ?? false }; const BINDINGS = { hybrid: HYBRID_BINDING, document: DOCUMENT_BINDING }; /** @@ -15,7 +34,7 @@ const BINDINGS = { hybrid: HYBRID_BINDING, document: DOCUMENT_BINDING }; */ for (const testCase of CASES) { for (const [name, binding] of Object.entries(BINDINGS)) { - test(`${testCase.id} ${name}: ${testCase.title}`, () => { + test(`${testCase.id} ${name}: ${testCase.title}`, executed, () => { const expected = expectationFor(testCase, name); const result = search(db, testCase.filter, binding); if (expected.problem) { @@ -36,7 +55,7 @@ for (const testCase of CASES) { * them to differ. Every disagreement is either a compiler bug or a case that * declares itself binding-dependent. */ -test("the two bindings agree except where the spec allows them not to", () => { +test("the two bindings agree except where the spec allows them not to", executed, () => { const divergent = []; for (const testCase of CASES) { const results = Object.entries(BINDINGS).map(([name, binding]) => { @@ -48,7 +67,7 @@ test("the two bindings agree except where the spec allows them not to", () => { assert.deepEqual(divergent, CASES.filter(bindingsDiverge).map((c) => c.id)); }); -test("every fixture record is reachable, and no case matches everything by accident", () => { +test("every fixture record is reachable, and no case matches everything by accident", executed, () => { const all = search(db, { id: { $exists: true } }, DOCUMENT_BINDING); assert.equal(all.ids.length, RECORDS.length); }); @@ -130,3 +149,87 @@ test("both dialects compile the whole corpus", () => { } assert.deepEqual(failures, []); }); + +/** + * SPEC.md §7: operands are caller-supplied and MUST be bound, never + * interpolated. This is the one safety requirement no limit in §7 mitigates — + * a bound operand of any length is inert, an interpolated one is not — so it is + * asserted directly rather than inferred from the result sets above. + * + * Each operand is a hostile payload with a unique alphabetic sentinel appended. + * The sentinel is what the assertions track, for two reasons. It cannot collide + * with SQL the compiler legitimately generates — an earlier version of this test + * probed for "SELECT 1" and tripped over the compiler's own + * `EXISTS (SELECT 1 FROM json_each(...))`. And it contains no LIKE + * metacharacter, so it survives the rewriting the pattern operators properly do: + * $contains escapes a literal `%` to `\%` so it cannot act as a wildcard, while + * $like passes the pattern through because there `%` is a wildcard by design. + * Tracking the sentinel tells "escaped" from "dropped" without asserting any one + * operator's escaping scheme. + */ +const HOSTILE = [ + ["x' OR 1=1 --", "SENTINELONE"], + ['"; DROP TABLE pets; --', "SENTINELTWO"], + ["'||(SELECT name FROM pets LIMIT 1)||'", "SENTINELTHREE"], + ["\\'; SELECT 1; --", "SENTINELFOUR"], + ["%' OR name LIKE '%", "SENTINELFIVE"], +].map(([payload, sentinel]) => ({ value: payload + sentinel, probe: sentinel })); + +for (const [dialectName, binding] of [ + ["sqlite", HYBRID_BINDING], + ["postgres", { ...HYBRID_BINDING, dialect: "postgres" }], +]) { + test(`${dialectName}: hostile operands are bound, never interpolated (§7)`, () => { + for (const { value, probe } of HOSTILE) { + // One filter per operator family that carries a string operand, so an + // operator added later that forgets to bind is caught rather than only + // $eq being covered. + const filters = [ + { name: { $eq: value } }, + { name: { $ne: value } }, + { name: { $in: [value] } }, + { name: { $like: value } }, + { name: { $contains: value } }, + { name: { $startsWith: value } }, + { tags: { $some: { $eq: value } } }, + ]; + + for (const filter of filters) { + const label = JSON.stringify(filter); + + // Two outcomes are safe and no third is: the operand is refused, or it + // is bound. $like legitimately refuses a lone "\\'" as an invalid + // escape (§5.5), and refusing is a stronger answer than binding. + let compiled; + try { + compiled = compile(filter, binding); + } catch (err) { + assert.ok(err instanceof QueryProblem, `expected a QueryProblem for ${label}, got ${err}`); + continue; + } + + const { sql, params } = compiled; + assert.ok(!sql.includes(value), `operand was interpolated into the statement for ${label}:\n ${sql}`); + assert.ok(!sql.includes(probe), `part of the operand reached the statement for ${label}:\n ${sql}`); + + // It must also have been passed along rather than silently dropped: a + // dropped predicate widens the result set, which §2.1 and §7 forbid. + assert.ok( + params.some((v) => typeof v === "string" && v.includes(probe)), + `operand never reached the parameter list for ${label}: ${JSON.stringify(params)}`, + ); + } + } + }); +} + +test("a hostile operand matches nothing rather than changing the query (§7)", executed, () => { + // The end-to-end form of the same claim: executed, against the real dataset. + for (const { value } of HOSTILE) { + const result = search(db, { name: { $eq: value } }, HYBRID_BINDING); + assert.ok(!result.problem, `unexpected ${result.problem?.type} for ${value}`); + assert.deepEqual(result.ids, [], `an injected operand must select no records: ${value}`); + } + // And the table is still there, which a successful injection would have settled. + assert.deepEqual(search(db, { status: "available" }, HYBRID_BINDING).problem, undefined); +});