Fix/n1ql injection hardening

__test__/query-injection.spec.ts

@@ -0,0 +1,125 @@
+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 () => {
+    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)}`);
+  });
+
+  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');
+  });
+
+  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');
+
+    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`');
+  });
+});

__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 () => {
@@ -18,6 +18,26 @@ 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('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([]);
@@ -57,6 +77,27 @@ 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('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`');

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`;
 };