Feature: SQL Injection Prevention (Flow Simulation) #23

kokokuo · 2022-07-20T08:53:24Z

Description

Support preventing sql injection attack.

For the detailed description and introduction, please read Notion link !

🔔 Notice: The PR is only to simulates the getting data source and re-parsing parameters flow and add re-parse method in the interface, the really logistic should wait data source to implement, then could be able to replace it.

How To Test / Expected Results

For the test result, please see the below test cases that passed the unit test:

Core package

Serve package

Integration testing package

Commit Message

90b3942 - feat(core, serve): add sql injection prevention structure and flow.
- add prepare parameters method in data source interface.
- inject data source in route generator
- refactor template engine for re-wrap data to context object.
- update req tag runner to get bind paramters from context and send to executor for creating builder.
- add identifier in data query builder
- update test cases for data query builder and template engine.
- update route generator, app, server test cases.

- add prepare parameters method in data source interface. - inject data source in route generator - refactor template engine for re-wrap data to context object. - update req tag runner to get bind paramters from context and send to executor for creating builder. - add identifier in data query builder - update test cases for data query builder and template engine. - update route generator, app, server test cases.

kokokuo · 2022-08-16T01:21:30Z

Rebase finished from #25.

oscar60310

Other part LGTM.

Just a kindly reminder: We have a tradeoff when using prepared parameters: we won’t able to take operation on parameters anymore.

select * from users where age > {{ context.params.age + 20 }}

--
age=18
select * from users where age > 38

oscar60310 · 2022-08-17T07:45:27Z

packages/integration-testing/src/mockExtensions/mockDb.ts

+    // handle parameterized query statement
+    let query = statement;
+    for (const identifier of Object.keys(bindParams)) {
+      query = query.replace(identifier, bindParams[identifier]);


Should use global searching instead:

Suggested change

query = query.replace(identifier, bindParams[identifier]);

query = query.replace(new RegExp(identifier, 'g') , bindParams[identifier]);

Thanks for reviewing! it has been fixed.

oscar60310 · 2022-08-17T07:46:09Z

packages/core/test/data-query/builder/where-clause.spec.ts

@@ -75,6 +78,7 @@ describe('Test data query builder > where clause', () => {
      const builder = new DataQueryBuilder({
        statement: 'select * from orders',
        dataSource: stubDataSource,
+        bindParams: sinon.stubInterface<BindParameters>(),


NIT: I'd prefer to create a helper function like createMockBuild to prevent potential changes in the future.

Thanks for suggesting it! it has been refactored.

oscar60310 · 2022-08-17T07:46:39Z

packages/core/test/data-query/builder/having-clause.spec.ts

@@ -304,6 +310,7 @@ describe('Test data query builder > having clause', () => {
      const builder = new DataQueryBuilder({
        statement: 'select * from orders',
        dataSource: stubDataSource,
+        bindParams: sinon.stubInterface<BindParameters>(),


NIT: We used to stub at the arranging phase.

Thanks for suggesting it! it has been refactored, I use the createStub function like above you suggested to handle it.

oscar60310 · 2022-08-17T07:47:05Z

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

+    const binds = (context.ctx?.context || {})['_paramBinds'] || {};
+    const builder = await this.executor.createBuilder(query, binds);


How about adding a testing case to cover your change:

// Arrange const { compiler, loader, builder, executor } = await createTestCompiler(); const { compiledData } = await compiler.compile(` {% req userCount main %} select count(*) as count from user where user.id = '{{ context.params.userId }}'; {% endreq %} `); builder.value.onFirstCall().resolves([{ count: 1 }]); // Action loader.setSource('test', compiledData); await compiler.execute('test', { context: { params: { userId: '@userId' } }, ['_paramBinds']: { '@userId': '000000' }, }); // Assert expect(executor.createBuilder.firstCall.args[0]).toBe( `select count(*) as count from user where user.id = '@userId';` ); expect(executor.createBuilder.firstCall.args[1]).toEqual({ '@userId': '000000', });

Thanks for suggesting and providing the complete test cases ❤️

oscar60310 · 2022-08-17T07:47:46Z

packages/core/src/lib/template-engine/templateEngine.ts

+    return this.compiler.execute(
+      templateName,
+      {
+        context: {
+          ...others,
+          ['params']: prepared?.identifiers || {},
+          ['_paramBinds']: prepared?.binds || {},
+        },
+      },
+      pagination
+    );


Since we don't want users to access _paramBinds via templates, how about moving it out of context properties?

Suggested change

return this.compiler.execute(

templateName,

{

context: {

...others,

['params']: prepared?.identifiers || {},

['_paramBinds']: prepared?.binds || {},

},

},

pagination

);

return this.compiler.execute(

templateName,

{

context: {

...others,

['params']: prepared?.identifiers || {},

},

['_paramBinds']: prepared?.binds || {},

},

pagination

);

Thanks for suggesting, you're right, it will be better to move out of context, it has been fixed!

oscar60310 · 2022-08-17T07:54:42Z

packages/serve/src/lib/route/routeGenerator.ts

@@ -37,11 +38,13 @@ export class RouteGenerator {
    @inject(TYPES.RequestValidator) reqValidator: IRequestValidator,
    @inject(TYPES.PaginationTransformer)
    paginationTransformer: IPaginationTransformer,
+    @inject(CORE_TYPES.DataSource) dataSource: DataSource,


NIT: We have more and more dependencies to inject, I think it is better to add RestfulRoute and GraphQLRoute to container and inject them to RouteGenerator directly.

Thanks for suggesting, I agree with your idea, but it needs some time to refactor ( maybe need to use AutoNamedFactory ), I would like to refactor it with a new feature together :)

… for binding params

…st cases.

kokokuo · 2022-08-17T09:47:22Z

Other part LGTM.

Just a kindly reminder: We have a tradeoff when using prepared parameters: we won’t able to take operation on parameters anymore.
select * from users where age > {{ context.params.age + 20 }}

--
age=18
select * from users where age > 38

Thanks for reminding me~, I think we could solve the case in the future with new design ?

kokokuo · 2022-08-17T09:48:57Z

Hi @oscar60310 besides the last NIT I would like to handle it in the new feature (need some time to refactor and test), others are fixed, thanks so much!

kokokuo changed the base branch from develop to feature/response-format July 20, 2022 08:53

kokokuo changed the title ~~Feature/prevent sql injection~~ Feature: SQL Injection Prevention ( Add method contract and flow Simulation ) Jul 20, 2022

kokokuo changed the title ~~Feature: SQL Injection Prevention ( Add method contract and flow Simulation )~~ Feature: SQL Injection Prevention ( Flow Simulation ) Jul 20, 2022

kokokuo marked this pull request as ready for review July 20, 2022 08:55

kokokuo requested a review from oscar60310 July 20, 2022 08:55

kokokuo changed the title ~~Feature: SQL Injection Prevention ( Flow Simulation )~~ Feature: SQL Injection Prevention (Flow Simulation) Jul 20, 2022

kokokuo changed the title ~~Feature: SQL Injection Prevention (Flow Simulation)~~ [WIP] Feature: SQL Injection Prevention (Flow Simulation) Jul 25, 2022

kokokuo changed the title ~~[WIP] Feature: SQL Injection Prevention (Flow Simulation)~~ Feature: SQL Injection Prevention (Flow Simulation) Jul 25, 2022

kokokuo force-pushed the feature/response-format branch from ebc253e to bbcd1c2 Compare July 28, 2022 04:31

kokokuo force-pushed the feature/prevent-sql-injection branch from 1a337d6 to e2fe9a4 Compare July 28, 2022 07:06

Base automatically changed from feature/response-format to develop August 2, 2022 07:32

kokokuo force-pushed the feature/prevent-sql-injection branch from e2fe9a4 to 90b3942 Compare August 15, 2022 12:31

kokokuo changed the base branch from develop to feature/extension-loader August 15, 2022 12:32

kokokuo mentioned this pull request Aug 17, 2022

Feature: Add Auth middleware and Authenticator. #26

Merged

Base automatically changed from feature/extension-loader to develop August 17, 2022 06:45

kokokuo mentioned this pull request Aug 17, 2022

Feature: Add enforce HTTPS middleware #27

Merged

oscar60310 approved these changes Aug 17, 2022

View reviewed changes

kokokuo added 2 commits August 17, 2022 17:20

fix: move the '_paramBinds' to outer of context and append test cases…

90396be

… for binding params

fix: bind the parameters to query string and refactor data builder te…

8d9a446

…st cases.

oscar60310 merged commit 0b1f30b into develop Aug 18, 2022

oscar60310 deleted the feature/prevent-sql-injection branch August 18, 2022 01:49

oscar60310 mentioned this pull request Aug 31, 2022

Fix: Fix and enhance SQL injection prevention #40

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: SQL Injection Prevention (Flow Simulation) #23

Feature: SQL Injection Prevention (Flow Simulation) #23

kokokuo commented Jul 20, 2022 •

edited

kokokuo commented Aug 16, 2022

oscar60310 left a comment

oscar60310 Aug 17, 2022

kokokuo Aug 17, 2022

oscar60310 Aug 17, 2022

kokokuo Aug 17, 2022

oscar60310 Aug 17, 2022

kokokuo Aug 17, 2022

oscar60310 Aug 17, 2022

kokokuo Aug 17, 2022

oscar60310 Aug 17, 2022

kokokuo Aug 17, 2022

oscar60310 Aug 17, 2022

kokokuo Aug 17, 2022

kokokuo commented Aug 17, 2022

kokokuo commented Aug 17, 2022

	query = query.replace(identifier, bindParams[identifier]);
	query = query.replace(new RegExp(identifier, 'g') , bindParams[identifier]);

		const binds = (context.ctx?.context \|\| {})['_paramBinds'] \|\| {};
		const builder = await this.executor.createBuilder(query, binds);

Feature: SQL Injection Prevention (Flow Simulation) #23

Feature: SQL Injection Prevention (Flow Simulation) #23

Conversation

kokokuo commented Jul 20, 2022 • edited

Description

How To Test / Expected Results

Core package

Serve package

Integration testing package

Commit Message

kokokuo commented Aug 16, 2022

oscar60310 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokokuo commented Aug 17, 2022

kokokuo commented Aug 17, 2022

kokokuo commented Jul 20, 2022 •

edited