From 9ccb505f452f79352b5e39d44fdb898172620ba8 Mon Sep 17 00:00:00 2001 From: Elliot Scribner Date: Mon, 10 Aug 2026 13:53:34 -0400 Subject: [PATCH 1/8] fix(query): prevent N1QL injection via unescaped string literals `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) --- __test__/query-injection.spec.ts | 38 ++++++++++++++++++++++++++++++++ src/query/helpers/builders.ts | 10 ++++++++- 2 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 __test__/query-injection.spec.ts diff --git a/__test__/query-injection.spec.ts b/__test__/query-injection.spec.ts new file mode 100644 index 000000000..ff59b499f --- /dev/null +++ b/__test__/query-injection.spec.ts @@ -0,0 +1,38 @@ +import { Query } from '../src'; + +describe('Test N1QL injection hardening', () => { + test('A string value containing a double quote cannot terminate the literal', async () => { + const injected = 'x" OR 1=1 OR name="'; + const query = new Query({}, 'travel-sample').select('*').where({ name: injected }).build(); + + // The value must survive as a single escaped literal, not as extra N1QL syntax. + expect(query).toStrictEqual(`SELECT * FROM \`travel-sample\` WHERE name=${JSON.stringify(injected)}`); + expect(query).not.toContain('OR 1=1 OR name=""'); + }); + + test('Backslashes in a value are preserved instead of being stripped', async () => { + const windowsPath = 'C:\\Users\\bob'; + const query = new Query({}, 'travel-sample').select('*').where({ path: windowsPath }).build(); + + expect(query).toStrictEqual(`SELECT * FROM \`travel-sample\` WHERE \`path\`=${JSON.stringify(windowsPath)}`); + }); + + test('A quote inside a USE KEYS value cannot introduce another key', async () => { + const injected = 'a" , "b'; + const query = new Query({}, 'travel-sample').select('*').useKeys([injected]).build(); + + expect(query).toStrictEqual(`SELECT * FROM \`travel-sample\` USE KEYS ${JSON.stringify([injected])}`); + }); + + test('A quote inside a comparison value cannot terminate the literal', async () => { + const injected = '%57%" OR "1"="1'; + const query = new Query({}, 'travel-sample') + .select('*') + .where({ address: { $like: injected } }) + .build(); + + expect(query).toStrictEqual( + `SELECT * FROM \`travel-sample\` WHERE address LIKE ${JSON.stringify(injected)}`, + ); + }); +}); diff --git a/src/query/helpers/builders.ts b/src/query/helpers/builders.ts index c1c4c02c9..5fffac6be 100644 --- a/src/query/helpers/builders.ts +++ b/src/query/helpers/builders.ts @@ -467,8 +467,16 @@ const _buildCollectionInWithinOperator = ( return `${searchExpr} ${CollectionDeepSearchOperatorDict[operator]} ${target}`; }; +/** + * Render a value as a N1QL literal. + * + * N1QL string literals accept the same escape sequences as JSON, so the output of + * `JSON.stringify` is already a valid literal. Stripping the backslashes it emits + * would let a value containing `"` terminate the literal and inject arbitrary N1QL. + * @ignore + * */ const stringifyValues = (value: unknown) => { - return JSON.stringify(value).replace(/\\/gi, ''); + return JSON.stringify(value); }; /** From 763a880fe44493333ba08068c845a5c8271c974a Mon Sep 17 00:00:00 2001 From: Elliot Scribner Date: Mon, 10 Aug 2026 13:55:43 -0400 Subject: [PATCH 2/8] fix(query): validate sort direction before interpolating it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- __test__/query-injection.spec.ts | 29 +++++++++++++++++++++++++---- src/query/helpers/builders.ts | 30 +++++++++++++++++++++++++++--- 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/__test__/query-injection.spec.ts b/__test__/query-injection.spec.ts index ff59b499f..4c4955fbb 100644 --- a/__test__/query-injection.spec.ts +++ b/__test__/query-injection.spec.ts @@ -1,4 +1,5 @@ -import { Query } from '../src'; +import { buildIndexExpr, IIndexOnParams, Query } from '../src'; +import { BuildQueryError } from '../src/exceptions/ottoman-errors'; describe('Test N1QL injection hardening', () => { test('A string value containing a double quote cannot terminate the literal', async () => { @@ -31,8 +32,28 @@ describe('Test N1QL injection hardening', () => { .where({ address: { $like: injected } }) .build(); - expect(query).toStrictEqual( - `SELECT * FROM \`travel-sample\` WHERE address LIKE ${JSON.stringify(injected)}`, - ); + expect(query).toStrictEqual(`SELECT * FROM \`travel-sample\` WHERE address LIKE ${JSON.stringify(injected)}`); + }); + + test('An ORDER BY direction that is not ASC or DESC is rejected', async () => { + const build = () => + new Query({}, 'travel-sample') + .select('*') + .orderBy({ name: 'ASC, (SELECT 1)' as any }) + .build(); + + expect(build).toThrow(BuildQueryError); + }); + + test('An index ON sort direction that is not ASC or DESC is rejected', async () => { + const on = [{ name: 'callsign', sort: 'ASC"] , ["injected' }] as unknown as IIndexOnParams[]; + + expect(() => buildIndexExpr('travel-sample', 'CREATE', 'idx_test', on)).toThrow(BuildQueryError); + }); + + test('Valid sort directions are still accepted and normalised', async () => { + const query = new Query({}, 'travel-sample').select('*').orderBy({ name: 'DESC' }).build(); + + expect(query).toStrictEqual('SELECT * FROM `travel-sample` ORDER BY name DESC'); }); }); diff --git a/src/query/helpers/builders.ts b/src/query/helpers/builders.ts index 5fffac6be..680bfed02 100644 --- a/src/query/helpers/builders.ts +++ b/src/query/helpers/builders.ts @@ -91,7 +91,7 @@ export const selectBuilder = ( havingExpr, )}${_buildOrderByExpr(orderBy)}${_buildLimitExpr(limit)}${_buildOffsetExpr(offset)}`; } catch (exception) { - if (exception instanceof WhereClauseException) { + if (exception instanceof WhereClauseException || exception instanceof BuildQueryError) { throw exception; } throw new SelectClauseException(); @@ -190,11 +190,35 @@ const _buildLetExpr = (letExpr?: LetExprType, clause = 'LET') => { const _buildOrderByExpr = (orderExpr: Record | undefined) => { return !!orderExpr ? ` ORDER BY ${Object.keys(orderExpr) - .map((value: string) => `${value.includes('[') ? value : escapeReservedWords(value)} ${orderExpr[value]}`) + .map( + (value: string) => + `${value.includes('[') ? value : escapeReservedWords(value)} ${_buildSortDirection(orderExpr[value])}`, + ) .join(',')}` : ''; }; +/** + * Sort directions accepted by the ORDER BY and index ON clauses. + * @ignore + * */ +const SORT_DIRECTIONS = ['ASC', 'DESC']; + +/** + * A sort direction is interpolated into the statement as bare N1QL, so it has to be + * checked against the allowed keywords rather than quoted. + * @ignore + * */ +const _buildSortDirection = (sort: SortType): string => { + const direction = String(sort).toUpperCase(); + if (!SORT_DIRECTIONS.includes(direction)) { + throw new BuildQueryError( + `The sort direction '${sort}' is not valid, use one of '${SORT_DIRECTIONS.join(`' | '`)}'`, + ); + } + return direction; +}; + /** * @ignore * */ @@ -576,7 +600,7 @@ const buildOnExpr = (on: IIndexOnParams[]) => { * */ const buildOnSortExpr = (onExpr?: IIndexOnParams) => { if (onExpr && onExpr.hasOwnProperty('sort')) { - return `["${onExpr.sort}"]`; + return `["${_buildSortDirection(onExpr.sort as SortType)}"]`; } return ''; }; From d32f1337cdd2af03049ca4d9f799f077e4c46336 Mon Sep 17 00:00:00 2001 From: Elliot Scribner Date: Mon, 10 Aug 2026 13:57:32 -0400 Subject: [PATCH 3/8] fix(query): escape identifiers and validate the statement in buildIndexExpr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- __test__/query-injection.spec.ts | 31 +++++++++++++++++++++++++++++- src/query/helpers/builders.ts | 33 +++++++++++++++++++++++++++----- src/query/utils.ts | 11 +++++++++++ 3 files changed, 69 insertions(+), 6 deletions(-) diff --git a/__test__/query-injection.spec.ts b/__test__/query-injection.spec.ts index 4c4955fbb..5862c5712 100644 --- a/__test__/query-injection.spec.ts +++ b/__test__/query-injection.spec.ts @@ -1,5 +1,5 @@ import { buildIndexExpr, IIndexOnParams, Query } from '../src'; -import { BuildQueryError } from '../src/exceptions/ottoman-errors'; +import { BuildIndexQueryError, BuildQueryError } from '../src/exceptions/ottoman-errors'; describe('Test N1QL injection hardening', () => { test('A string value containing a double quote cannot terminate the literal', async () => { @@ -56,4 +56,33 @@ describe('Test N1QL injection hardening', () => { expect(query).toStrictEqual('SELECT * FROM `travel-sample` ORDER BY name DESC'); }); + + test('A backtick in an index name cannot close the identifier quoting', async () => { + const index = buildIndexExpr('travel-sample', 'DROP', 'idx` ; DROP INDEX `other'); + + expect(index).toStrictEqual('DROP INDEX `travel-sample`.`idx`` ; DROP INDEX ``other`'); + }); + + test('A backtick in the collection name cannot close the identifier quoting', async () => { + const index = buildIndexExpr('bucket` ; SELECT 1 FROM `x', 'DROP', 'idx_test'); + + expect(index).toStrictEqual('DROP INDEX `bucket`` ; SELECT 1 FROM ``x`.`idx_test`'); + }); + + test('An unsupported index statement is rejected', async () => { + expect(() => buildIndexExpr('travel-sample', 'DROP INDEX x; CREATE' as any, 'idx_test')).toThrow( + BuildIndexQueryError, + ); + }); + + test('Valid index expressions are unchanged', async () => { + const on: IIndexOnParams[] = [{ name: 'callsign', sort: 'ASC' }]; + + expect(buildIndexExpr('travel-sample', 'CREATE', 'idx_test', on)).toStrictEqual( + 'CREATE INDEX `idx_test` ON `travel-sample`(callsign["ASC"]) ', + ); + expect(buildIndexExpr('travel-sample', 'DROP', 'idx_test')).toStrictEqual( + 'DROP INDEX `travel-sample`.`idx_test`', + ); + }); }); diff --git a/src/query/helpers/builders.ts b/src/query/helpers/builders.ts index 680bfed02..b9a30aca9 100644 --- a/src/query/helpers/builders.ts +++ b/src/query/helpers/builders.ts @@ -1,4 +1,4 @@ -import { BuildQueryError } from '../../exceptions/ottoman-errors'; +import { BuildIndexQueryError, BuildQueryError } from '../../exceptions/ottoman-errors'; import { CollectionInWithinExceptions, QueryGroupByParamsException, @@ -24,7 +24,7 @@ import { LogicalWhereExpr, SortType, } from '../interface/query.types'; -import { escapeFromClause, escapeReservedWords } from '../utils'; +import { escapeFromClause, escapeIdentifier, escapeReservedWords } from '../utils'; import { AggDict, CollectionDeepSearchOperatorDict, @@ -575,15 +575,38 @@ export const buildIndexExpr = ( usingGSI?: boolean, withExpr?: IIndexWithParams, ): string => { - if (['BUILD', 'CREATE', 'CREATE PRIMARY'].includes(type) && on) { - return `${type} INDEX \`${name}\` ON \`${collection}\`(${buildOnExpr(on)})${buildWhereExpr(where)} ${ + const _type = _buildIndexType(type); + const _name = escapeIdentifier(name); + const _collection = escapeIdentifier(collection); + if (['BUILD', 'CREATE', 'CREATE PRIMARY'].includes(_type) && on) { + return `${_type} INDEX ${_name} ON ${_collection}(${buildOnExpr(on)})${buildWhereExpr(where)} ${ usingGSI ? 'USING GSI' : '' } ${buildWithExpr(withExpr)}`; } else { - return `${type} INDEX \`${collection}\`.\`${name}\`${usingGSI ? ' USING GSI' : ''}`; + return `${_type} INDEX ${_collection}.${_name}${usingGSI ? ' USING GSI' : ''}`; } }; +/** + * Index statements supported by the builder. + * @ignore + * */ +const INDEX_TYPES: IndexType[] = ['CREATE', 'BUILD', 'DROP', 'CREATE PRIMARY']; + +/** + * The statement keyword is interpolated as bare N1QL, so it is checked against the + * supported statements rather than quoted. + * @ignore + * */ +const _buildIndexType = (type: IndexType): IndexType => { + if (!INDEX_TYPES.includes(type)) { + throw new BuildIndexQueryError( + `The index type '${type}' is not valid, use one of '${INDEX_TYPES.join(`' | '`)}'`, + ); + } + return type; +}; + /** * @ignore * */ diff --git a/src/query/utils.ts b/src/query/utils.ts index bbbb09246..7eeafec3e 100644 --- a/src/query/utils.ts +++ b/src/query/utils.ts @@ -14,6 +14,17 @@ export const escapeFromClause = (str: string) => { return `${result.join('.')}${rest.length > 0 ? ` ${rest.map((item) => item.replace(/`/g, '')).join(' ')}` : ''}`; }; +/** + * Wrap a single identifier in backticks. + * + * N1QL escapes a backtick inside a quoted identifier by doubling it, so doubling + * here keeps the identifier intact while making it impossible for the value to + * close the quoting and continue the statement. + * */ +export const escapeIdentifier = (identifier: string) => { + return `\`${String(identifier).replace(/`/g, '``')}\``; +}; + /** * Convert select expression into an Array of selection keys * */ From 86417e726ae15f47e68a85955cca33e0bbc57b55 Mon Sep 17 00:00:00 2001 From: Elliot Scribner Date: Mon, 10 Aug 2026 13:58:46 -0400 Subject: [PATCH 4/8] fix(query): strip result-modifier keywords with a valid regex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- __test__/query-injection.spec.ts | 4 +--- __test__/query-utils.spec.ts | 9 +++++++++ src/query/helpers/builders.ts | 4 +--- src/query/utils.ts | 12 ++++++++---- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/__test__/query-injection.spec.ts b/__test__/query-injection.spec.ts index 5862c5712..5eef80155 100644 --- a/__test__/query-injection.spec.ts +++ b/__test__/query-injection.spec.ts @@ -81,8 +81,6 @@ describe('Test N1QL injection hardening', () => { expect(buildIndexExpr('travel-sample', 'CREATE', 'idx_test', on)).toStrictEqual( 'CREATE INDEX `idx_test` ON `travel-sample`(callsign["ASC"]) ', ); - expect(buildIndexExpr('travel-sample', 'DROP', 'idx_test')).toStrictEqual( - 'DROP INDEX `travel-sample`.`idx_test`', - ); + expect(buildIndexExpr('travel-sample', 'DROP', 'idx_test')).toStrictEqual('DROP INDEX `travel-sample`.`idx_test`'); }); }); diff --git a/__test__/query-utils.spec.ts b/__test__/query-utils.spec.ts index 7bded6d06..0fc9fa52a 100644 --- a/__test__/query-utils.spec.ts +++ b/__test__/query-utils.spec.ts @@ -18,6 +18,15 @@ describe('Test Query Builder Utils', () => { expect(result).toStrictEqual(['address']); }); + test('Result modifier keywords are stripped when there is no alias', async () => { + expect(parseStringSelectExpr('DISTINCT a, b as c')).toStrictEqual(['a', 'c']); + expect(parseStringSelectExpr('RAW type, address')).toStrictEqual(['type', 'address']); + }); + + test('A field whose name contains a keyword is not mangled', async () => { + expect(parseStringSelectExpr('allowed, values as v')).toStrictEqual(['allowed', 'v']); + }); + test('Test get Projections fields with an empty select', () => { const result = getProjectionFields('travel-sample', ''); expect(result.fields).toStrictEqual([]); diff --git a/src/query/helpers/builders.ts b/src/query/helpers/builders.ts index b9a30aca9..3c097d85d 100644 --- a/src/query/helpers/builders.ts +++ b/src/query/helpers/builders.ts @@ -600,9 +600,7 @@ const INDEX_TYPES: IndexType[] = ['CREATE', 'BUILD', 'DROP', 'CREATE PRIMARY']; * */ const _buildIndexType = (type: IndexType): IndexType => { if (!INDEX_TYPES.includes(type)) { - throw new BuildIndexQueryError( - `The index type '${type}' is not valid, use one of '${INDEX_TYPES.join(`' | '`)}'`, - ); + throw new BuildIndexQueryError(`The index type '${type}' is not valid, use one of '${INDEX_TYPES.join(`' | '`)}'`); } return type; }; diff --git a/src/query/utils.ts b/src/query/utils.ts index 7eeafec3e..63fa74049 100644 --- a/src/query/utils.ts +++ b/src/query/utils.ts @@ -2,6 +2,13 @@ import { n1qlReservedWords } from './helpers'; const replaceList = ['ALL', 'DISTINCT', 'RAW', 'ELEMENT', 'VALUE']; +/** + * Matches the result-modifier keywords as whole words, so a field whose name merely + * contains one of them (`allowed`, `values`) is left alone. + * @ignore + * */ +const replaceListExpr = new RegExp(`\\b(${replaceList.join('|')})\\b`, 'g'); + export const escapeFromClause = (str: string) => { const trimStr = str.trim(); const [collection, ...rest] = trimStr.split(' '); @@ -32,10 +39,7 @@ export const parseStringSelectExpr = (expr: string): string[] => { if (expr.indexOf(',') === -1 && expr.indexOf(' as ') === -1) { return [expr]; } - let resultExpr = expr.replace(/[()]/g, ''); - replaceList.forEach((value: string) => { - resultExpr = resultExpr.replace(new RegExp(`/[${value}]/`, 'g'), ''); - }); + const resultExpr = expr.replace(/[()]/g, '').replace(replaceListExpr, ''); return resultExpr.split(',').map((v: string) => { return extractAsValue(v); }); From 2842be4d2c0ab29234548e8dd40e4a91683fe881 Mon Sep 17 00:00:00 2001 From: Elliot Scribner Date: Mon, 10 Aug 2026 14:01:07 -0400 Subject: [PATCH 5/8] fix(indexes): escape identifiers and the model name in N1QL index DDL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- __test__/query-utils.spec.ts | 13 ++++++++- src/model/index/n1ql/ensure-n1ql-indexes.ts | 30 ++++++++++++--------- src/query/utils.ts | 10 +++++++ 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/__test__/query-utils.spec.ts b/__test__/query-utils.spec.ts index 0fc9fa52a..ad4339de6 100644 --- a/__test__/query-utils.spec.ts +++ b/__test__/query-utils.spec.ts @@ -1,5 +1,5 @@ import { parseStringSelectExpr, getProjectionFields, Query, escapeReservedWords } from '../src'; -import { escapeFromClause } from '../src/query/utils'; +import { escapeFieldPath, escapeFromClause, escapeIdentifier } from '../src/query/utils'; describe('Test Query Builder Utils', () => { test('Test the conversion of select expression into an Array of selection keys', async () => { @@ -27,6 +27,17 @@ describe('Test Query Builder Utils', () => { expect(parseStringSelectExpr('allowed, values as v')).toStrictEqual(['allowed', 'v']); }); + test('escapeIdentifier quotes an identifier and doubles embedded backticks', async () => { + expect(escapeIdentifier('travel-sample')).toStrictEqual('`travel-sample`'); + expect(escapeIdentifier('idx` ; DROP INDEX `other')).toStrictEqual('`idx`` ; DROP INDEX ``other`'); + }); + + test('escapeFieldPath quotes each segment so a nested path stays a path', async () => { + expect(escapeFieldPath('_type')).toStrictEqual('`_type`'); + expect(escapeFieldPath('metadata.doc_type')).toStrictEqual('`metadata`.`doc_type`'); + expect(escapeFieldPath('a`.`b')).toStrictEqual('`a```.```b`'); + }); + test('Test get Projections fields with an empty select', () => { const result = getProjectionFields('travel-sample', ''); expect(result.fields).toStrictEqual([]); diff --git a/src/model/index/n1ql/ensure-n1ql-indexes.ts b/src/model/index/n1ql/ensure-n1ql-indexes.ts index 02f0876fb..829eeb184 100644 --- a/src/model/index/n1ql/ensure-n1ql-indexes.ts +++ b/src/model/index/n1ql/ensure-n1ql-indexes.ts @@ -4,6 +4,7 @@ import { ModelMetadata } from '../../interfaces/model-metadata.interface'; import { isDebugMode } from '../../../utils/is-debug-mode'; import { Ottoman } from '../../../ottoman/ottoman'; import { DEFAULT_COLLECTION } from '../../../utils/constants'; +import { escapeFieldPath, escapeIdentifier } from '../../../query/utils'; import { IndexExistsError } from 'couchbase'; /** @@ -29,10 +30,7 @@ export const ensureN1qlIndexes = async (ottoman: Ottoman, n1qlIndexes) => { ? `Ottoman${scopeName}${modelName}` : `Ottoman${scopeName}${scapedModelKey}`; if (!existingIndexesNames.includes(name)) { - const on = - collectionName !== DEFAULT_COLLECTION - ? `\`${bucketName}\`.\`${scopeName}\`.\`${collectionName}\`` - : `\`${bucketName}\``; + const on = buildKeyspace(bucketName, scopeName, collectionName); try { if (!indexesToBuild[on]) { indexesToBuild[on] = []; @@ -61,10 +59,7 @@ export const ensureN1qlIndexes = async (ottoman: Ottoman, n1qlIndexes) => { const Model = ottoman.getModel(index.modelName); const metadata = getModelMetadata(Model); const { scopeName, collectionName } = metadata; - const on = - collectionName !== DEFAULT_COLLECTION - ? `\`${bucketName}\`.\`${scopeName}\`.\`${collectionName}\`` - : `\`${bucketName}\``; + const on = buildKeyspace(bucketName, scopeName, collectionName); yield cluster .query(queryBuildIndexDefered(indexNameSanitized, fieldNames, metadata, on)) .then(() => { @@ -129,21 +124,30 @@ export const ensureN1qlIndexes = async (ottoman: Ottoman, n1qlIndexes) => { return Promise.resolve(true); }; +// Build the escaped keyspace an index is created on. +const buildKeyspace = (bucketName: string, scopeName: string, collectionName: string): string => { + return collectionName !== DEFAULT_COLLECTION + ? [bucketName, scopeName, collectionName].map(escapeIdentifier).join('.') + : escapeIdentifier(bucketName); +}; + // Create the ottoman type index, needed to make model lookups fast. const queryForIndexOttomanType = (ottomanType: string, on: string, collectionKey: string): string => { - return `CREATE INDEX \`${ottomanType}\` ON ${on}(\`${collectionKey}\`) USING GSI WITH {"defer_build": true}`; + return `CREATE INDEX ${escapeIdentifier(ottomanType)} ON ${on}(${escapeFieldPath( + collectionKey, + )}) USING GSI WITH {"defer_build": true}`; }; // Map createIndex across all individual n1ql model indexes. // concurrency: 1 is important to avoid overwhelming the server. const queryBuildIndexDefered = (indexName, fields, metadata: ModelMetadata, on: string) => { const { modelKey, modelName } = metadata; - return `CREATE INDEX \`${indexName}\` ON ${on}(${fields.join( - ',', - )}) WHERE ${modelKey}="${modelName}" USING GSI WITH {"defer_build": true}`; + return `CREATE INDEX ${escapeIdentifier(indexName)} ON ${on}(${fields.join(',')}) WHERE ${escapeFieldPath( + modelKey, + )}=${JSON.stringify(modelName)} USING GSI WITH {"defer_build": true}`; }; // All indexes were built deferred, so now kick off actual build. const queryBuildIndexes = (on, indexesName: string[]) => { - return `BUILD INDEX ON ${on}(${indexesName.map((idx) => `\`${idx}\``).join(',')}) USING GSI`; + return `BUILD INDEX ON ${on}(${indexesName.map((idx) => escapeIdentifier(idx)).join(',')}) USING GSI`; }; diff --git a/src/query/utils.ts b/src/query/utils.ts index 63fa74049..ce925ddbe 100644 --- a/src/query/utils.ts +++ b/src/query/utils.ts @@ -32,6 +32,16 @@ export const escapeIdentifier = (identifier: string) => { return `\`${String(identifier).replace(/`/g, '``')}\``; }; +/** + * Quote every segment of a dotted field path. + * + * Each segment is escaped on its own so that `metadata.doc_type` stays a path to a + * nested field rather than collapsing into a single identifier containing a dot. + * */ +export const escapeFieldPath = (path: string) => { + return String(path).split('.').map(escapeIdentifier).join('.'); +}; + /** * Convert select expression into an Array of selection keys * */ From 96881085bf6e4c6c44dfb42b603e79d0d9195c2b Mon Sep 17 00:00:00 2001 From: Elliot Scribner Date: Mon, 10 Aug 2026 14:05:21 -0400 Subject: [PATCH 6/8] fix(query): reject WHERE field names that are not field paths 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) --- __test__/query-injection.spec.ts | 43 ++++++++++++++++++++++++++++++ src/query/helpers/builders.ts | 14 ++++++---- src/query/utils.ts | 45 ++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 5 deletions(-) diff --git a/__test__/query-injection.spec.ts b/__test__/query-injection.spec.ts index 5eef80155..ce26f72a6 100644 --- a/__test__/query-injection.spec.ts +++ b/__test__/query-injection.spec.ts @@ -1,5 +1,6 @@ import { buildIndexExpr, IIndexOnParams, Query } from '../src'; import { BuildIndexQueryError, BuildQueryError } from '../src/exceptions/ottoman-errors'; +import { WhereClauseException } from '../src/query/exceptions'; describe('Test N1QL injection hardening', () => { test('A string value containing a double quote cannot terminate the literal', async () => { @@ -57,6 +58,48 @@ describe('Test N1QL injection hardening', () => { expect(query).toStrictEqual('SELECT * FROM `travel-sample` ORDER BY name DESC'); }); + test('A WHERE key that is not a field path is rejected', async () => { + const build = () => + new Query({}, 'travel-sample') + .select('*') + .where({ 'a) OR 1=1 --': 'v' }) + .build(); + + expect(build).toThrow(WhereClauseException); + }); + + test('A WHERE key that is not a field path is rejected for comparison operators too', async () => { + const build = () => + new Query({}, 'travel-sample') + .select('*') + .where({ 'a) OR 1=1 --': { $gt: 1 } }) + .build(); + + expect(build).toThrow(WhereClauseException); + }); + + test('Legitimate field paths are still accepted', async () => { + const query = new Query({}, 'travel-sample') + .select('*') + .where({ 'address.city': 'Paris', 'numbers[0]': 3, 'travel-sample.callsign': { $like: '%AF%' } }) + .build(); + + expect(query).toStrictEqual( + 'SELECT * FROM `travel-sample` WHERE address.city="Paris" AND numbers[0]=3 AND `travel-sample`.callsign LIKE "%AF%"', + ); + }); + + test('A quote in a collection-operator literal cannot terminate it', async () => { + const query = new Query({}, 'travel-sample t') + .select('*') + .where({ ['"a" OR 1=1 OR "b"']: { $within: { $field: 't' } } }) + .build(); + + expect(query).toStrictEqual( + `SELECT * FROM \`travel-sample\` t WHERE ${JSON.stringify('a" OR 1=1 OR "b')} WITHIN t`, + ); + }); + test('A backtick in an index name cannot close the identifier quoting', async () => { const index = buildIndexExpr('travel-sample', 'DROP', 'idx` ; DROP INDEX `other'); diff --git a/src/query/helpers/builders.ts b/src/query/helpers/builders.ts index 3c097d85d..d95f1fa70 100644 --- a/src/query/helpers/builders.ts +++ b/src/query/helpers/builders.ts @@ -24,7 +24,7 @@ import { LogicalWhereExpr, SortType, } from '../interface/query.types'; -import { escapeFromClause, escapeIdentifier, escapeReservedWords } from '../utils'; +import { escapeFieldName, escapeFromClause, escapeIdentifier, escapeReservedWords, escapeSearchExpr } from '../utils'; import { AggDict, CollectionDeepSearchOperatorDict, @@ -369,19 +369,19 @@ const _buildFieldClauseExpr = (field: BuildFieldClauseExprType, ignoreCase = fal const fieldExpr = field[value]?.['$field']; if (fieldExpr && typeof fieldExpr === 'string') { - return `${escapeReservedWords(value)}=${fieldExpr}`; + return `${escapeFieldName(value)}=${fieldExpr}`; } if (!value.includes('$')) { if (typeof field[value] === 'string') { - const comparator = escapeReservedWords(value); + const comparator = escapeFieldName(value); const toCompare = stringifyValues(field[value]); return ignoreCase ? applyIgnoreCase(ignoreCase, comparator, '=', toCompare, true) : `${comparator}=${toCompare}`; } if (typeof field[value] === 'number' || typeof field[value] === 'boolean' || Array.isArray(field[value])) { - return `${escapeReservedWords(value)}=${stringifyValues(field[value])}`; + return `${escapeFieldName(value)}=${stringifyValues(field[value])}`; } } throw new QueryOperatorNotFoundException(value); @@ -417,7 +417,11 @@ const _buildComparisonClauseExpr = (fieldName: string, comparison: ComparisonWhe .map((key: string) => { const value = comparison[key]; if (value != null) { - const field = escapeReservedWords(fieldName); + // A collection operator can search for a literal, so its left-hand side is not + // necessarily a field name. + const field = CollectionDeepSearchOperatorDict.hasOwnProperty(key) + ? escapeSearchExpr(fieldName) + : escapeFieldName(fieldName); if (ComparisonEmptyOperatorDict.hasOwnProperty(key)) { return `${field} ${ComparisonEmptyOperatorDict[key]}`; } diff --git a/src/query/utils.ts b/src/query/utils.ts index ce925ddbe..4841b805b 100644 --- a/src/query/utils.ts +++ b/src/query/utils.ts @@ -1,3 +1,4 @@ +import { WhereClauseException } from './exceptions'; import { n1qlReservedWords } from './helpers'; const replaceList = ['ALL', 'DISTINCT', 'RAW', 'ELEMENT', 'VALUE']; @@ -66,6 +67,50 @@ const extractAsValue = (expr: string): string => { return expr.trim(); }; +/** + * A single path segment: an optionally backtick-quoted name, followed by any number of + * array subscripts (`[0]`, `[-1]`, `[*]`). + * @ignore + * */ +const FIELD_SEGMENT = '`?[A-Za-z_$][A-Za-z0-9_$-]*`?(?:\\[(?:\\*|-?\\d+)\\])*'; + +/** + * A field path is one or more of those segments joined by dots. + * @ignore + * */ +const FIELD_PATH_EXPR = new RegExp(`^${FIELD_SEGMENT}(?:\\.${FIELD_SEGMENT})*$`); + +/** + * Escape a name used as a document field in a WHERE clause. + * + * `escapeReservedWords` only adds backticks for reserved words and for names holding a + * dash or subscript; every other name is emitted verbatim. That is fine for the clauses + * whose operands are deliberately N1QL expressions (GROUP BY, `$field`), but a WHERE key + * is a document field, and these keys routinely come from request data. A key such as + * `'a) OR 1=1 --'` would otherwise be interpolated as syntax, so a name that is not a + * field path is rejected instead of escaped. + * */ +export const escapeFieldName = (field: string) => { + if (!FIELD_PATH_EXPR.test(field)) { + throw new WhereClauseException( + `The field name '${field}' is not a valid field path. Expected a document field, optionally dotted and subscripted, e.g. 'address.city' or 'numbers[-1]'.`, + ); + } + return escapeReservedWords(field); +}; + +/** + * Escape the left-hand side of a collection `IN`/`WITHIN` operator. + * + * Unlike an ordinary comparison, this side may be a literal being searched for rather + * than a field — `"CORSAIR" WITHIN t` is valid. A quoted literal is re-escaped so that a + * quote inside it cannot terminate it; anything else has to be a field path. + * */ +export const escapeSearchExpr = (expr: string) => { + const literal = /^"([\s\S]*)"$/.exec(expr); + return literal ? JSON.stringify(literal[1]) : escapeFieldName(expr); +}; + /** * @ignore */ From cb10bceadb77eb19f155648b5160b697505f42cb Mon Sep 17 00:00:00 2001 From: Elliot Scribner Date: Mon, 10 Aug 2026 14:06:36 -0400 Subject: [PATCH 7/8] fix(query): stop quoting the dash of a negative array subscript `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) --- __test__/query-injection.spec.ts | 6 +----- __test__/query-utils.spec.ts | 9 +++++++++ src/query/utils.ts | 5 ++++- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/__test__/query-injection.spec.ts b/__test__/query-injection.spec.ts index ce26f72a6..dfcdbe143 100644 --- a/__test__/query-injection.spec.ts +++ b/__test__/query-injection.spec.ts @@ -59,11 +59,7 @@ describe('Test N1QL injection hardening', () => { }); test('A WHERE key that is not a field path is rejected', async () => { - const build = () => - new Query({}, 'travel-sample') - .select('*') - .where({ 'a) OR 1=1 --': 'v' }) - .build(); + const build = () => new Query({}, 'travel-sample').select('*').where({ 'a) OR 1=1 --': 'v' }).build(); expect(build).toThrow(WhereClauseException); }); diff --git a/__test__/query-utils.spec.ts b/__test__/query-utils.spec.ts index ad4339de6..3fc9337e5 100644 --- a/__test__/query-utils.spec.ts +++ b/__test__/query-utils.spec.ts @@ -77,6 +77,15 @@ describe('Test Query Builder Utils', () => { expect(expr3).toStrictEqual('`travel-sample`'); }); + test('A negative array subscript is not quoted as a dashed identifier', async () => { + expect(escapeReservedWords('numbers[-1]')).toStrictEqual('numbers[-1]'); + expect(escapeReservedWords('a.numbers[-2].b')).toStrictEqual('a.numbers[-2].b'); + }); + + test('A dashed name is quoted regardless of case', async () => { + expect(escapeReservedWords('Travel-Sample')).toStrictEqual('`Travel-Sample`'); + }); + test('escape fromClause', () => { const escaped = escapeFromClause('travel-sample'); expect(escaped).toBe('`travel-sample`'); diff --git a/src/query/utils.ts b/src/query/utils.ts index 4841b805b..a8ef5d487 100644 --- a/src/query/utils.ts +++ b/src/query/utils.ts @@ -129,7 +129,10 @@ export const escapeReservedWords = (field: string) => { return value; }) .join('.'); - expr = expr.replace(/([a-z0-9]*\-[a-z0-9]*)/g, '`$&`'); + // Quote names containing a dash. The leading run is required so that the dash of a + // negative array subscript — which has `[` in front of it, not a name — is not treated + // as a dashed identifier and quoted into `numbers[`-1`]`. + expr = expr.replace(/([a-zA-Z0-9]+-[a-zA-Z0-9]*)/g, '`$&`'); return expr; } return field; From a28870e5393b9c922b245dbcbf17670a31e46ea6 Mon Sep 17 00:00:00 2001 From: Elliot Scribner Date: Mon, 10 Aug 2026 14:41:49 -0400 Subject: [PATCH 8/8] fix(query): remove quadratic backtracking from the dash-quoting pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- __test__/query-utils.spec.ts | 12 ++++++++++++ src/query/utils.ts | 15 +++++++++++---- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/__test__/query-utils.spec.ts b/__test__/query-utils.spec.ts index 3fc9337e5..7aeae0c94 100644 --- a/__test__/query-utils.spec.ts +++ b/__test__/query-utils.spec.ts @@ -86,6 +86,18 @@ describe('Test Query Builder Utils', () => { expect(escapeReservedWords('Travel-Sample')).toStrictEqual('`Travel-Sample`'); }); + test('A dash-free name of any length is escaped in linear time', async () => { + // The dash-quoting pass used to be quadratic: a long name with no dash made the + // engine backtrack from every start position. At this size that took minutes. + const field = `${'0'.repeat(200000)}.x`; + + expect(escapeReservedWords(field)).toStrictEqual(field); + }); + + test('A name with several dashes is quoted as one identifier', async () => { + expect(escapeReservedWords('a-b-c')).toStrictEqual('`a-b-c`'); + }); + test('escape fromClause', () => { const escaped = escapeFromClause('travel-sample'); expect(escaped).toBe('`travel-sample`'); diff --git a/src/query/utils.ts b/src/query/utils.ts index a8ef5d487..a6f3eca25 100644 --- a/src/query/utils.ts +++ b/src/query/utils.ts @@ -129,10 +129,17 @@ export const escapeReservedWords = (field: string) => { return value; }) .join('.'); - // Quote names containing a dash. The leading run is required so that the dash of a - // negative array subscript — which has `[` in front of it, not a name — is not treated - // as a dashed identifier and quoted into `numbers[`-1`]`. - expr = expr.replace(/([a-zA-Z0-9]+-[a-zA-Z0-9]*)/g, '`$&`'); + // Quote names containing a dash. + // + // The run is matched with a single character class so the match cannot backtrack. + // Requiring a dash *after* an alphanumeric run — `[a-zA-Z0-9]+-` — reads more directly + // but is quadratic: for every start position in a long dash-free name the engine + // matches to the end and then backtracks the whole way looking for the dash. + // + // Whether the run is a dashed name is then decided with plain string checks. A run + // starting with the dash is a negative array subscript, where the preceding character + // is `[` rather than part of a name, and must not be quoted into `numbers[`-1`]`. + expr = expr.replace(/[a-zA-Z0-9-]+/g, (run) => (run.includes('-') && !run.startsWith('-') ? `\`${run}\`` : run)); return expr; } return field;