Fix: Fix and enhance SQL injection prevention

labs/playground1/playground-exts/mockDb.js

@@ -29,17 +29,7 @@ class MockDataSource extends DataSource {
     // handle parameterized query statement
     let query = statement;
 
-    const parameters =
-      // {$1: 'v1', $3: 'v3', $2: 'v2' }
-      _.chain(bindParams)
-        // [[$1, 'v1'], [$3, 'v3'], [$2, 'v2']]
-        .toPairs()
-        // [[$1, 'v1'], [$2, 'v2'], [$3, 'v3']]
-        .orderBy(([name,]) => Number(name.slice(1)))
-        // ['v1,'v2','v3']
-        .map(([_, value]) => value)
-        .value()
-
+    const parameters = Array.from(bindParams.values());
 
     return new Promise((resolve, reject) => {
       logger.info(query, parameters);
@@ -72,19 +62,7 @@ class MockDataSource extends DataSource {
   }
 
   async prepare(params) {
-    const identifiers = {};
-    const binds = {};
-    let index = 1;
-    for (const key of Object.keys(params)) {
-      const identifier = `$${index}`;
-      identifiers[key] = identifier;
-      binds[identifier] = params[key];
-      index += 1;
-    }
-    return {
-      identifiers,
-      binds,
-    };
+    return `$${params.parameterIndex}`
   }
 }
 

labs/playground1/sqls/artist/artist.yaml

@@ -5,3 +5,5 @@ request:
     description: constituent id
     validators:
       - required
+exampleParameter:
+  id: "1"

labs/playground1/sqls/artist/works.yaml

@@ -5,3 +5,5 @@ request:
     description: constituent id
     validators:
       - required
+exampleParameter:
+  id: '1'

packages/build/src/lib/schema-parser/middleware/responseSampler.ts

@@ -36,8 +36,7 @@ export class ResponseSampler extends SchemaParserMiddleware {
         offset: 0,
       }
     );
-    // TODO: I haven't known the response of queryBuilder.value(), assume that there is a "columns" property that indicates the columns' name and type here.
-    const columns: { name: string; type: string }[] = response.columns;
+    const columns = response.getColumns();
     const responseColumns = this.normalizeResponseColumns(columns);
     schema.response = this.mergeResponse(
       schema.response || [],

packages/build/test/schema-parser/middleware/responseSampler.spec.ts

@@ -1,6 +1,7 @@
 import { RawAPISchema } from '@vulcan-sql/build/schema-parser';
 import { ResponseSampler } from '@vulcan-sql/build/schema-parser/middleware/responseSampler';
 import { FieldDataType, TemplateEngine } from '@vulcan-sql/core';
+import { Stream } from 'stream';
 import * as sinon from 'ts-sinon';
 
 it('Should create response definition when example parameter is provided', async () => {
@@ -14,10 +15,11 @@ it('Should create response definition when example parameter is provided', async
   };
   const stubTemplateEngine = sinon.stubInterface<TemplateEngine>();
   stubTemplateEngine.execute.resolves({
-    columns: [
+    getColumns: () => [
       { name: 'id', type: 'string' },
       { name: 'age', type: 'number' },
     ],
+    getData: () => new Stream(),
   });
   const responseSampler = new ResponseSampler(stubTemplateEngine);
   // Act
@@ -38,10 +40,11 @@ it('Should create response definition when example parameter is a empty object',
   };
   const stubTemplateEngine = sinon.stubInterface<TemplateEngine>();
   stubTemplateEngine.execute.resolves({
-    columns: [
+    getColumns: () => [
       { name: 'id', type: 'string' },
       { name: 'age', type: 'number' },
     ],
+    getData: () => new Stream(),
   });
   const responseSampler = new ResponseSampler(stubTemplateEngine);
   // Act
@@ -61,10 +64,11 @@ it('Should not create response definition when example parameter is not provided
   };
   const stubTemplateEngine = sinon.stubInterface<TemplateEngine>();
   stubTemplateEngine.execute.resolves({
-    columns: [
+    getColumns: () => [
       { name: 'id', type: 'string' },
       { name: 'age', type: 'number' },
     ],
+    getData: () => new Stream(),
   });
   const responseSampler = new ResponseSampler(stubTemplateEngine);
   // Act
@@ -88,11 +92,12 @@ it('Should append response definition when there are some existed definitions',
   };
   const stubTemplateEngine = sinon.stubInterface<TemplateEngine>();
   stubTemplateEngine.execute.resolves({
-    columns: [
+    getColumns: () => [
       { name: 'id', type: 'string' },
       { name: 'age', type: 'number' },
       { name: 'name', type: 'boolean' },
     ],
+    getData: () => new Stream(),
   });
   const responseSampler = new ResponseSampler(stubTemplateEngine);
   // Act

packages/core/src/lib/data-query/executor.ts

@@ -1,4 +1,9 @@
-import { BindParameters, DataSource } from '@vulcan-sql/core/models';
+import {
+  BindParameters,
+  DataSource,
+  PrepareParameterFunc,
+  RequestParameter,
+} from '@vulcan-sql/core/models';
 import { inject, injectable } from 'inversify';
 import { TYPES } from '@vulcan-sql/core/types';
 import { DataQueryBuilder, IDataQueryBuilder } from './builder';
@@ -8,6 +13,7 @@ export interface IExecutor {
     query: string,
     bindParams: BindParameters
   ): Promise<IDataQueryBuilder>;
+  prepare: PrepareParameterFunc;
 }
 
 @injectable()
@@ -16,6 +22,11 @@ export class QueryExecutor implements IExecutor {
   constructor(@inject(TYPES.DataSource) dataSource: DataSource) {
     this.dataSource = dataSource;
   }
+
+  public async prepare(request: RequestParameter) {
+    return this.dataSource.prepare(request);
+  }
+
   /**
    * create data query builder
    * @returns

packages/core/src/lib/data-source/pg.ts

@@ -1,11 +1,9 @@
 import { Stream } from 'stream';
 import {
-  BindParameters,
   DataResult,
   DataSource,
   ExecuteOptions,
-  IdentifierParameters,
-  RequestParameters,
+  RequestParameter,
   VulcanExtensionId,
   VulcanInternalExtension,
 } from '../../models/extensions';
@@ -24,19 +22,8 @@ export class PGDataSource extends DataSource {
       },
     };
   }
-  public async prepare(params: RequestParameters) {
-    const identifiers = {} as IdentifierParameters;
-    const binds = {} as BindParameters;
-    let index = 1;
-    for (const key of Object.keys(params)) {
-      const identifier = `$${index}`;
-      identifiers[key] = identifier;
-      binds[identifier] = params[key];
-      index += 1;
-    }
-    return {
-      identifiers,
-      binds,
-    };
+
+  public async prepare({ parameterIndex }: RequestParameter) {
+    return `$${parameterIndex}`;
   }
 }

packages/core/src/lib/template-engine/built-in-extensions/query-builder/constants.ts

@@ -3,3 +3,6 @@ export const METADATA_NAME = 'builder.vulcan.com';
 export const EXECUTE_COMMAND_NAME = 'value';
 export const EXECUTE_FILTER_NAME = 'execute';
 export const REFERENCE_SEARCH_MAX_DEPTH = 100;
+export const SANITIZER_NAME = 'sanitize';
+export const PARAMETERIZER_VAR_NAME = 'parameterizer';
+export const RAW_FILTER_NAME = 'raw';

packages/core/src/lib/template-engine/built-in-extensions/query-builder/executorRunner.ts

@@ -1,12 +1,18 @@
 import { IDataQueryBuilder } from '@vulcan-sql/core/data-query';
-import { FilterRunner, VulcanInternalExtension } from '@vulcan-sql/core/models';
+import {
+  FilterRunner,
+  FilterRunnerTransformOptions,
+  VulcanInternalExtension,
+} from '@vulcan-sql/core/models';
 import { EXECUTE_FILTER_NAME } from './constants';
 
 @VulcanInternalExtension()
 export class ExecutorRunner extends FilterRunner {
   public filterName = EXECUTE_FILTER_NAME;
 
-  public async transform({ value }: { value: any; args: any[] }): Promise<any> {
+  public async transform({
+    value,
+  }: FilterRunnerTransformOptions): Promise<any> {
     const builder: IDataQueryBuilder = value;
     return builder.value();
   }

packages/core/src/lib/template-engine/built-in-extensions/query-builder/index.ts

@@ -2,5 +2,18 @@ import { ReqTagBuilder } from './reqTagBuilder';
 import { ReqTagRunner } from './reqTagRunner';
 import { ExecutorRunner } from './executorRunner';
 import { ExecutorBuilder } from './executorBuilder';
+import { SanitizerBuilder } from './sanitizerBuilder';
+import { SanitizerRunner } from './sanitizerRunner';
+import { RawBuilder } from './rawBuilder';
+import { RawRunner } from './rawRunner';
 
-export default [ReqTagBuilder, ReqTagRunner, ExecutorRunner, ExecutorBuilder];
+export default [
+  ReqTagBuilder,
+  ReqTagRunner,
+  ExecutorRunner,
+  ExecutorBuilder,
+  SanitizerBuilder,
+  SanitizerRunner,
+  RawBuilder,
+  RawRunner,
+];

packages/core/src/lib/template-engine/built-in-extensions/query-builder/parameterizer.ts

@@ -0,0 +1,40 @@
+import { PrepareParameterFunc } from '@vulcan-sql/core/models';
+
+export class Parameterizer {
+  private parameterIndex = 1;
+  private sealed = false;
+  // We MUST not use pure object here because we care about the order of the keys.
+  // https://stackoverflow.com/questions/5525795/does-javascript-guarantee-object-property-order
+  // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#description
+  private idToValueMapping = new Map<string, any>();
+  private valueToIdMapping = new Map<any, string>();
+  private prepare: PrepareParameterFunc;
+
+  constructor(prepare: PrepareParameterFunc) {
+    this.prepare = prepare;
+  }
+
+  public async generateIdentifier(value: any): Promise<string> {
+    if (this.sealed)
+      throw new Error(
+        `This parameterizer has been sealed, we might use the parameterizer from a wrong request scope.`
+      );
+    if (this.valueToIdMapping.has(value))
+      return this.valueToIdMapping.get(value)!;
+    const id = await this.prepare({
+      parameterIndex: this.parameterIndex++,
+      value,
+    });
+    this.idToValueMapping.set(id, value);
+    this.valueToIdMapping.set(value, id);
+    return id;
+  }
+
+  public seal() {
+    this.sealed = true;
+  }
+
+  public getBinding() {
+    return this.idToValueMapping;
+  }
+}

packages/core/src/lib/template-engine/built-in-extensions/query-builder/rawBuilder.ts

@@ -0,0 +1,10 @@
+import {
+  FilterBuilder,
+  VulcanInternalExtension,
+} from '@vulcan-sql/core/models';
+import { RAW_FILTER_NAME } from './constants';
+
+@VulcanInternalExtension()
+export class RawBuilder extends FilterBuilder {
+  public filterName = RAW_FILTER_NAME;
+}

packages/core/src/lib/template-engine/built-in-extensions/query-builder/rawRunner.ts

@@ -0,0 +1,18 @@
+import {
+  FilterRunner,
+  FilterRunnerTransformOptions,
+  VulcanInternalExtension,
+} from '@vulcan-sql/core/models';
+import { RAW_FILTER_NAME } from './constants';
+
+@VulcanInternalExtension()
+export class RawRunner extends FilterRunner {
+  public filterName = RAW_FILTER_NAME;
+
+  public async transform({
+    value,
+  }: FilterRunnerTransformOptions): Promise<any> {
+    // Do nothing, this filer is only a place holder to block sanitizer
+    return value;
+  }
+}

packages/core/src/lib/template-engine/built-in-extensions/query-builder/reqTagRunner.ts

@@ -6,7 +6,8 @@ import {
   TagRunnerOptions,
   VulcanInternalExtension,
 } from '@vulcan-sql/core/models';
-import { FINIAL_BUILDER_NAME } from './constants';
+import { FINIAL_BUILDER_NAME, PARAMETERIZER_VAR_NAME } from './constants';
+import { Parameterizer } from './parameterizer';
 
 @VulcanInternalExtension()
 export class ReqTagRunner extends TagRunner {
@@ -23,17 +24,27 @@ export class ReqTagRunner extends TagRunner {
   }
 
   public async run({ context, args, contentArgs }: TagRunnerOptions) {
-    const name = args[0];
+    const name = String(args[0]);
+
+    const parameterizer = new Parameterizer(
+      this.executor.prepare.bind(this.executor)
+    );
+    // parameterizer from parent, we should set it back after rendered our context.
+    const parentParameterizer = context.lookup(PARAMETERIZER_VAR_NAME);
+    context.setVariable(PARAMETERIZER_VAR_NAME, parameterizer);
     let query = '';
     for (let index = 0; index < contentArgs.length; index++) {
       query += await contentArgs[index]();
     }
+    // Seal current parameterizer to avoid incorrect usage.
+    parameterizer.seal();
+    context.setVariable(PARAMETERIZER_VAR_NAME, parentParameterizer);
     query = query
       .split(/\r?\n/)
       .filter((line) => line.trim().length > 0)
       .join('\n');
     // Get bind real parameters and pass to data query builder for data source used.
-    const binds = (context.ctx || {})['_paramBinds'] || {};
+    const binds = parameterizer.getBinding();
     const builder = await this.executor.createBuilder(query, binds);
     context.setVariable(name, builder);