From 0070aa7ee7eb069d7f522ff80d2de9cdeeb81f6c Mon Sep 17 00:00:00 2001 From: Roy Paterson Date: Mon, 20 Nov 2023 16:49:05 +0000 Subject: [PATCH 1/4] Increase database test timeout to 10 seconds Setting up the database is slow. --- .../Middleware/ProjectAuthorization.test.ts | 25 +++++++++++-------- .../Tests/Services/ProbeService.test.ts | 11 +++++--- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/CommonServer/Tests/Middleware/ProjectAuthorization.test.ts b/CommonServer/Tests/Middleware/ProjectAuthorization.test.ts index 12adea4e9f..4684732237 100644 --- a/CommonServer/Tests/Middleware/ProjectAuthorization.test.ts +++ b/CommonServer/Tests/Middleware/ProjectAuthorization.test.ts @@ -145,19 +145,22 @@ describe('ProjectMiddleware', () => { let database!: Database; - beforeEach(async () => { - jest.clearAllMocks(); - next = jest.fn(); - database = new Database(); - await database.createAndConnect(); + beforeEach( + async () => { + jest.clearAllMocks(); + next = jest.fn(); + database = new Database(); + await database.createAndConnect(); - if (req.headers === undefined) { - req.headers = {}; - } + if (req.headers === undefined) { + req.headers = {}; + } - req.headers['tenantid'] = mockedObjectId.toString(); - req.headers['apikey'] = mockedObjectId.toString(); - }); + req.headers['tenantid'] = mockedObjectId.toString(); + req.headers['apikey'] = mockedObjectId.toString(); + }, + 10 * 1000 // 10 second timeout because setting up the DB is slow + ); afterEach(async () => { await database.disconnectAndDropDatabase(); diff --git a/CommonServer/Tests/Services/ProbeService.test.ts b/CommonServer/Tests/Services/ProbeService.test.ts index 4fa2d8f94d..60cd7f6ee7 100644 --- a/CommonServer/Tests/Services/ProbeService.test.ts +++ b/CommonServer/Tests/Services/ProbeService.test.ts @@ -12,10 +12,13 @@ import { fail } from 'assert'; describe('probeService', () => { let database!: Database; - beforeEach(async () => { - database = new Database(); - await database.createAndConnect(); - }); + beforeEach( + async () => { + database = new Database(); + await database.createAndConnect(); + }, + 10 * 1000 // 10 second timeout because setting up the DB is slow + ); afterEach(async () => { await database.disconnectAndDropDatabase(); From 59cd7afcc59fbfdc12e7035ec314739c0892f31c Mon Sep 17 00:00:00 2001 From: Roy Paterson Date: Tue, 21 Nov 2023 19:34:44 +0000 Subject: [PATCH 2/4] Add Tests for StatementGenerator.toSetStatement() --- .../StatementGenerator.test.ts | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 CommonServer/Tests/Utils/AnalyticsDatabase/StatementGenerator.test.ts diff --git a/CommonServer/Tests/Utils/AnalyticsDatabase/StatementGenerator.test.ts b/CommonServer/Tests/Utils/AnalyticsDatabase/StatementGenerator.test.ts new file mode 100644 index 0000000000..683b524b03 --- /dev/null +++ b/CommonServer/Tests/Utils/AnalyticsDatabase/StatementGenerator.test.ts @@ -0,0 +1,114 @@ +import '../../TestingUtils/Init'; +import AnalyticsBaseModel from 'Common/AnalyticsModels/BaseModel'; +import AnalyticsTableColumn from 'Common/Types/AnalyticsDatabase/TableColumn'; +import TableColumnType from 'Common/Types/AnalyticsDatabase/TableColumnType'; +import StatementGenerator from '../../../Utils/AnalyticsDatabase/StatementGenerator'; +import { ClickhouseAppInstance } from '../../../Infrastructure/ClickhouseDatabase'; +import ObjectID from 'Common/Types/ObjectID'; + +describe('StatementGenerator', () => { + class TestModel extends AnalyticsBaseModel { + public constructor() { + super({ + tableName: '', + singularName: '', + pluralName: '', + tableColumns: Object.keys(TableColumnType) + .filter((tableColumnType: string) => { + // NestedModel not supported? + return tableColumnType !== 'NestedModel'; + }) + .map((tableColumnType: string) => { + return new AnalyticsTableColumn({ + key: `column_${tableColumnType}`, + title: '', + description: '<description>', + required: tableColumnType === 'ObjectID', + type: TableColumnType[ + tableColumnType as keyof typeof TableColumnType + ], + }); + }), + primaryKeys: ['column_ObjectID'], + }); + } + } + + let generator: StatementGenerator<TestModel>; + beforeEach(async () => { + generator = new StatementGenerator<TestModel>({ + modelType: TestModel, + database: ClickhouseAppInstance, + }); + }); + + describe('toSetStatement', () => { + let model: TestModel; + beforeEach(() => { + model = new TestModel(); + }); + + test('should return the contents of a SET statement', () => { + model.setColumnValue('column_ObjectID', new ObjectID('<value>')); + model.setColumnValue('column_Date', new Date(9876543210)); + model.setColumnValue('column_Number', 123); + model.setColumnValue('column_Text', '<value>'); + model.setColumnValue('column_JSON', { key: '<value>' }); + model.setColumnValue('column_Decimal', 234.56); + model.setColumnValue('column_ArrayNumber', [3, 4, 5]); + model.setColumnValue('column_ArrayText', [ + '<value-1>', + '<value-2>', + ]); + model.setColumnValue('column_LongNumber', '12345678901234567890'); + expect(generator.toSetStatement(model)).toEqual( + "column_ObjectID = '<value>', " + + "column_Date = parseDateTimeBestEffortOrNull('1970-04-25T07:29:03.210Z'), " + + 'column_Number = 123, ' + + "column_Text = '<value>', " + + 'column_JSON = \'{"key":"<value>"}\', ' + + 'column_Decimal = 234.56, ' + + 'column_ArrayNumber = [3, 4, 5], ' + + "column_ArrayText = ['<value-1>', '<value-2>'], " + + "column_LongNumber = 12345678901234567890" + ); + }); + + test('should sanitize column values', () => { + const unsafeString: string = "Robert'; DROP TABLE Students;--"; + model.setColumnValue('column_ObjectID', new ObjectID(unsafeString)); + // model.setColumnValue('column_Date', unsafeString); // throws error + model.setColumnValue('column_Number', unsafeString); + model.setColumnValue('column_Text', unsafeString); + model.setColumnValue('column_JSON', { key: unsafeString }); + model.setColumnValue('column_Decimal', unsafeString); + model.setColumnValue('column_ArrayNumber', [ + ']; DROP TABLE Students;--', + ]); + model.setColumnValue('column_ArrayText', [ + "Robert']; DROP TABLE Students;--", + ]); + model.setColumnValue( + 'column_LongNumber', + '0; DROP TABLE Students;--' + ); + expect(generator.toSetStatement(model)).toEqual( + "column_ObjectID = 'Robert'; DROP TABLE Students;--', " + // TODO unsafe! + "column_Number = NULL, " + + "column_Text = 'Robert'; DROP TABLE Students;--', " + // TODO unsafe! + "column_JSON = '{\"key\":\"Robert'; DROP TABLE Students;--\"}', " + // TODO unsafe! + "column_Decimal = NULL, " + + "column_ArrayNumber = []; DROP TABLE Students;--], " + // TODO unsafe! + "column_ArrayText = ['Robert']; DROP TABLE Students;--'], " + // TODO unsafe! + "column_LongNumber = 0; DROP TABLE Students;--" // TODO unsafe! + ); + }); + + test('should set column to NULL', () => { + model.setColumnValue('column_Text', null); + expect(generator.toSetStatement(model)).toEqual( + 'column_Text = NULL' + ); + }); + }); +}); From fdc7887e48e03cc93e88711d5768e22380b55cbb Mon Sep 17 00:00:00 2001 From: Roy Paterson <roy@roypaterson.com> Date: Wed, 22 Nov 2023 14:44:35 +0000 Subject: [PATCH 3/4] Escape values to prevent SQL injection --- .../StatementGenerator.test.ts | 18 ++++++++-------- .../AnalyticsDatabase/StatementGenerator.ts | 21 ++++++++++++++++--- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/CommonServer/Tests/Utils/AnalyticsDatabase/StatementGenerator.test.ts b/CommonServer/Tests/Utils/AnalyticsDatabase/StatementGenerator.test.ts index 683b524b03..c188a8628c 100644 --- a/CommonServer/Tests/Utils/AnalyticsDatabase/StatementGenerator.test.ts +++ b/CommonServer/Tests/Utils/AnalyticsDatabase/StatementGenerator.test.ts @@ -70,7 +70,7 @@ describe('StatementGenerator', () => { 'column_Decimal = 234.56, ' + 'column_ArrayNumber = [3, 4, 5], ' + "column_ArrayText = ['<value-1>', '<value-2>'], " + - "column_LongNumber = 12345678901234567890" + "column_LongNumber = CAST('12345678901234567890' AS Int128)" ); }); @@ -93,14 +93,14 @@ describe('StatementGenerator', () => { '0; DROP TABLE Students;--' ); expect(generator.toSetStatement(model)).toEqual( - "column_ObjectID = 'Robert'; DROP TABLE Students;--', " + // TODO unsafe! - "column_Number = NULL, " + - "column_Text = 'Robert'; DROP TABLE Students;--', " + // TODO unsafe! - "column_JSON = '{\"key\":\"Robert'; DROP TABLE Students;--\"}', " + // TODO unsafe! - "column_Decimal = NULL, " + - "column_ArrayNumber = []; DROP TABLE Students;--], " + // TODO unsafe! - "column_ArrayText = ['Robert']; DROP TABLE Students;--'], " + // TODO unsafe! - "column_LongNumber = 0; DROP TABLE Students;--" // TODO unsafe! + "column_ObjectID = 'Robert\\'; DROP TABLE Students;--', " + + 'column_Number = NULL, ' + + "column_Text = 'Robert\\'; DROP TABLE Students;--', " + + 'column_JSON = \'{"key":"Robert\\\'; DROP TABLE Students;--"}\', ' + + 'column_Decimal = NULL, ' + + 'column_ArrayNumber = [NULL], ' + + "column_ArrayText = ['Robert\\']; DROP TABLE Students;--'], " + + "column_LongNumber = CAST('0; DROP TABLE Students;--' AS Int128)" ); }); diff --git a/CommonServer/Utils/AnalyticsDatabase/StatementGenerator.ts b/CommonServer/Utils/AnalyticsDatabase/StatementGenerator.ts index ccf2d53e4f..0ca0d7271c 100644 --- a/CommonServer/Utils/AnalyticsDatabase/StatementGenerator.ts +++ b/CommonServer/Utils/AnalyticsDatabase/StatementGenerator.ts @@ -190,6 +190,11 @@ export default class StatementGenerator<TBaseModel extends AnalyticsBaseModel> { return record; } + private escapeStringLiteral(raw: string): string { + // escape String literal based on https://clickhouse.com/docs/en/sql-reference/syntax#string + return `'${raw.replace(/'|\\/g, '\\$&')}'`; + } + private sanitizeValue( value: RecordValue | undefined, column: AnalyticsTableColumn, @@ -215,7 +220,7 @@ export default class StatementGenerator<TBaseModel extends AnalyticsBaseModel> { column.type === TableColumnType.ObjectID || column.type === TableColumnType.Text ) { - value = `'${value?.toString()}'`; + value = this.escapeStringLiteral(value?.toString()); } if (column.type === TableColumnType.Date && value instanceof Date) { @@ -239,6 +244,10 @@ export default class StatementGenerator<TBaseModel extends AnalyticsBaseModel> { if (column.type === TableColumnType.ArrayNumber) { value = `[${(value as Array<number>) .map((v: number) => { + if (v && typeof v !== 'number') { + v = parseFloat(v); + return isNaN(v) ? 'NULL' : v; + } return v; }) .join(', ')}]`; @@ -247,13 +256,19 @@ export default class StatementGenerator<TBaseModel extends AnalyticsBaseModel> { if (column.type === TableColumnType.ArrayText) { value = `[${(value as Array<string>) .map((v: string) => { - return `'${v}'`; + return this.escapeStringLiteral(v); }) .join(', ')}]`; } if (column.type === TableColumnType.JSON) { - value = `'${JSON.stringify(value)}'`; + value = this.escapeStringLiteral(JSON.stringify(value)); + } + + if (column.type === TableColumnType.LongNumber) { + value = `CAST(${this.escapeStringLiteral( + value.toString() + )} AS Int128)`; } return value; From 407abd4146debf33aa720874145bc85e65d33ac4 Mon Sep 17 00:00:00 2001 From: Roy Paterson <roy@roypaterson.com> Date: Fri, 24 Nov 2023 11:58:15 +0000 Subject: [PATCH 4/4] Add crudApiPath to test --- .../Tests/Utils/AnalyticsDatabase/StatementGenerator.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CommonServer/Tests/Utils/AnalyticsDatabase/StatementGenerator.test.ts b/CommonServer/Tests/Utils/AnalyticsDatabase/StatementGenerator.test.ts index c188a8628c..dd15207712 100644 --- a/CommonServer/Tests/Utils/AnalyticsDatabase/StatementGenerator.test.ts +++ b/CommonServer/Tests/Utils/AnalyticsDatabase/StatementGenerator.test.ts @@ -5,6 +5,7 @@ import TableColumnType from 'Common/Types/AnalyticsDatabase/TableColumnType'; import StatementGenerator from '../../../Utils/AnalyticsDatabase/StatementGenerator'; import { ClickhouseAppInstance } from '../../../Infrastructure/ClickhouseDatabase'; import ObjectID from 'Common/Types/ObjectID'; +import Route from 'Common/Types/API/Route'; describe('StatementGenerator', () => { class TestModel extends AnalyticsBaseModel { @@ -29,6 +30,7 @@ describe('StatementGenerator', () => { ], }); }), + crudApiPath: new Route('route'), primaryKeys: ['column_ObjectID'], }); }