Fix/n1ql injection hardening - #820
Open
ejscribner wants to merge 8 commits into
Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
`stringifyValues` ran `JSON.stringify(value).replace(/\\/gi, '')`, discarding
the escaping that `JSON.stringify` had just produced. A value containing a
double quote therefore terminated the N1QL string literal and the remainder
was parsed as query syntax:
Model.find({ name: 'x" OR 1=1 OR name="' })
-> WHERE name="x" OR 1=1 OR name=""
The same helper renders WHERE comparisons, USE KEYS and index WITH nodes, so
every one of those clauses was affected. Stripping backslashes also silently
corrupted legitimate values: 'C:\Users\bob' was stored as 'C:Usersbob'.
N1QL string literals accept JSON escape sequences, so `JSON.stringify` output
is already a valid literal and needs no post-processing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ORDER BY` and the index `ON` clause interpolated the caller-supplied sort
direction straight into the statement. `SortType` is `'ASC' | 'DESC'`, but that
is a compile-time guarantee only — a direction arriving from an HTTP query
string reached the builder unchecked:
.orderBy({ name: 'ASC, (SELECT 1)' })
-> ORDER BY name ASC, (SELECT 1)
A direction is bare N1QL rather than a literal, so it cannot be quoted; it is
now checked against the allowed keywords and normalised to upper case.
`selectBuilder` also rethrows `BuildQueryError` rather than remapping it, so
the rejection surfaces as the real cause instead of the generic
`SelectClauseException`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…exExpr
`buildIndexExpr` is exported from the package root, so the index name, the
collection name and the statement keyword all arrive from the caller. All three
were interpolated raw. A backtick in either identifier closed the quoting that
was meant to contain it:
buildIndexExpr('travel-sample', 'DROP', 'idx` ; DROP INDEX `other')
-> DROP INDEX `travel-sample`.`idx` ; DROP INDEX `other`
`Query.index()` validates the name before it gets here, but nothing protects
callers using the exported builder directly.
Identifiers are now quoted with embedded backticks doubled, which is how N1QL
escapes them — lossless, and output for a normal identifier is unchanged. The
statement keyword cannot be quoted, so it is checked against the supported set.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`parseStringSelectExpr` built its keyword filter as
`new RegExp('/[DISTINCT]/', 'g')`. The delimiters were part of the pattern and
the keyword became a character class, so it looked for a literal `/`, one letter
from the keyword, then another `/` — which never matches a select expression.
The keywords were therefore never removed:
parseStringSelectExpr('DISTINCT a, b as c') -> ['DISTINCT a', 'c']
The existing test did not catch it because every field in the fixture has an
` as ` alias, and the alias extraction discards the mangled prefix.
The returned names feed the projection and cast paths, so a name that keeps its
keyword prefix no longer matches the corresponding key in the result rows.
One word-boundary regex now removes the keywords, leaving field names that
merely contain one (`allowed`, `values`) untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The index DDL in `ensureN1qlIndexes` interpolated every name it was given straight into the statement. The model name went into a double-quoted literal with no escaping, and the bucket, scope, collection, index and model-key names were wrapped in backticks that a backtick in the value would close. The upstream sanitiser only rewrites `\`, `$`, `[*]` and `::`, so a backtick passes through untouched. Names are now escaped with `escapeIdentifier`, and the model name is rendered with `JSON.stringify`. The keyspace is built once by `buildKeyspace` instead of being assembled twice by hand. This also corrects a nested `modelKey`. The ottoman-type index quoted `metadata.doc_type` whole, producing an index on a single field whose name contains a dot rather than on the nested field — while the deferred-build query for the same model used it as a path. Both now escape per segment, so the two agree. For a flat key such as the default `_type` the output is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WHERE keys were passed through `escapeReservedWords`, which only adds backticks
for reserved words and for names containing a dash or subscript. Every other
name was emitted verbatim, so a key could carry N1QL syntax:
Model.find({ 'a) OR 1=1 --': 'v' })
-> WHERE a) OR 1=1 `-``-`="v"
Filters are frequently built straight from request data, which makes the keys
attacker-controlled as often as the values.
`escapeReservedWords` cannot be tightened in place: GROUP BY and `$field` pass
deliberate N1QL expressions such as `COUNT(amount)` through the same helper. A
WHERE key is a document field, so it now goes through `escapeFieldName`, which
accepts a dotted, optionally subscripted path and rejects anything else.
The left-hand side of a collection `IN`/`WITHIN` is exempt: it may be a literal
being searched for, as in `"CORSAIR" WITHIN t`. Those are re-escaped through
`escapeSearchExpr` so a quote inside the literal cannot terminate it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`escapeReservedWords` quotes names containing a dash with
`/([a-z0-9]*\-[a-z0-9]*)/g`. Both runs around the dash are optional, so the
pattern also matched the dash of a negative array subscript, where the character
before it is `[` rather than part of a name:
escapeReservedWords('numbers[-1]') -> numbers[`-1`]
That quotes the index as an identifier and produces a broken WHERE clause. The
ORDER BY builder never hit it because it skips escaping for any name containing
`[`, so only filters on a negative subscript were affected.
Requiring a leading run fixes it, and the character class now covers upper case:
a name such as `Travel-Sample` was previously left unquoted, which is not a
valid dashed identifier in N1QL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CodeQL flagged the dash-quoting replacement in `escapeReservedWords` as a
polynomial regular expression on uncontrolled data, and it is right.
`[a-zA-Z0-9]+-[a-zA-Z0-9]*` ends its mandatory part with the dash, so for a long
name that contains no dash the engine matches the alphanumeric run to the end,
backtracks the whole way looking for a dash, then repeats that from the next
start position. Reaching the branch needs only a dot somewhere in the name:
'0'.repeat(10000) + '.x' -> 68 ms
'0'.repeat(20000) + '.x' -> 245 ms
'0'.repeat(40000) + '.x' -> 974 ms
Field names reach this helper from filter keys, which are as caller-controlled
as the values.
The structure predates this branch — `/([a-z0-9]*\-[a-z0-9]*)/g` on master is
the same shape and times identically — the line only entered the diff when the
subscript fix touched it.
The run is now matched with one character class, which cannot backtrack, and
whether it is a dashed name is decided with string checks. The 40000-character
case goes from 974 ms to 0.15 ms. A name with several dashes is now quoted as a
single identifier (`a-b-c` rather than `` `a-b`-c ``), which is what the
surrounding code intends.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ejscribner
force-pushed
the
fix/n1ql-injection-hardening
branch
from
August 11, 2026 16:37
0156afe to
a28870e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.