Contextual string tags to prevent SQL injection

[mikesamuel]

https://nodesecroadmap.fyi/chapter-7/query-langs.html describes this approach as part of a larger discussion about library support for safe coding practices.

This is one step in a larger effort to enable

connection.querySELECT * FROM T WHERE x = ${x}, y = ${y}, z = ${z}(callback)

and similar idioms.

This was broken out of https://github.com/mysqljs/mysql/pull/1926

[dougwilson]

Thanks so much! I would live to play around this to review, but not 100% sure about all the different cases around in here. When you get a chance, can you write up documentation for the README so myself (and everyone else) knows this exists and how to make use of it? I'm really confused why there is a SQL parser in here and concerned that there are going to be lots of edge cases with issues and I'm not super interested in maintaining an actual SQL parser as part of this. Why is the parser necessary? I would almost expect this to act just like ? does today, or what is happening here?

[dougwilson]

Running npm test seems to fail on my machine for some reason:

$ npm test

> sqlstring@2.3.0 test C:\Users\dougw\GitHub\nodejs-sqlstring
> node test/run.js

1..2
ok 1 test\unit\test-SqlString.js
  0 fail | 64 pass | 16 ms

not ok 2 test\unit\test-Template.js
  Failed: template tag date

    AssertionError: 'SELECT \'1999-12-31 19:00:00.000\'' == 'SELECT \'2000-01-01 00:00:00.000\''
        at runTagTest (C:\Users\dougw\GitHub\nodejs-sqlstring\test\unit\es6-Template.js:72:12)
        at Object.date (C:\Users\dougw\GitHub\nodejs-sqlstring\test\unit\es6-Template.js:83:5)
        at TestCase.run (C:\Users\dougw\GitHub\nodejs-sqlstring\node_modules\utest\lib\TestCase.js:30:10)
        at Collection._runTestCase (C:\Users\dougw\GitHub\nodejs-sqlstring\node_modules\utest\lib\Collection.js:44:6)
        at Collection.run (C:\Users\dougw\GitHub\nodejs-sqlstring\node_modules\utest\lib\Collection.js:23:10)
        at _combinedTickCallback (internal/process/next_tick.js:73:7)
        at process._tickCallback (internal/process/next_tick.js:104:9)
        at Module.runMain (module.js:606:11)
        at run (bootstrap_node.js:389:7)
        at startup (bootstrap_node.js:149:9)

  1 fail | 24 pass | 46 ms

npm ERR! Test failed.  See above for more details.

[dougwilson]

Ok, so I poked around a bit there. Let me know when you have some docs, because I can't (at least in the small time I have right now) figure out how to specify the timezone setting when using the template strings, for example (or stringifyObject).

[dougwilson]

In the end, would be great to see template tags land because they are very convenient.

[dougwilson]

Is the following the expected output?

> SqlString.sql`SELECT * FROM foo WHERE bin = "${Buffer.from("1f3870be274f6c49b3e31a0c6728957f","hex")}"`.toString()
SELECT * FROM foo WHERE bin = "8p�\'OlI��\Z
                                           g(�"

[dougwilson]

Also not sure if I'm using this right: was just trying to put in a NULL:

> SqlString.sql`SELECT * FROM foo WHERE bin = ${null}`.toString()
SELECT * FROM foo WHERE bin = ?

[dougwilson]

The following seems to throw an error, even though it's valid SQL syntax:

> SqlString.sql`UPDATE scores SET score=score--1`.toString()
Error: Expected delimiter at "UPDATE scores SET score=score--1"

[mikesamuel]

Ok, so I poked around a bit there. Let me know when you have some docs, because I can't (at least in the small time I have right now) figure out how to specify the timezone setting when using the template strings, for example (or stringifyObject).

I neglected to provide a way to thread those through.

TODO(mikesamuel): allow sql(optionsObject) to specify a tag handler that closes over options including timezone and stringifyObject. Fix the test that fails in non GMT contexts. Run tests in two or more TZ=... contexts before submitting PRs.

[dougwilson]

Thanks for your comments! I don't see any replies for the bugs(?) I think I found and would love to hear back on those. Also, not sure how you made that second set of line comments but there is no reply button for them so I cannot reply to them.

[mikesamuel]

I don't see any replies for the bugs(?) I think I found and would love to hear back on those.

Looking at this again. Will address those and others shortly.

Also, not sure how you made that second set of line comments but there is no reply button for them so I cannot reply to them.

Hmm. That's odd. I'll try to leave things as file comments.

[mikesamuel]

The test failures seem to be related to npm run-script of eslint and test-ci.

I'll tackle that in the next commit but probably won't push anything until tomorrow.

[mikesamuel]

The remaining Travis CI failures seem to be in test-{Lexer,Template} because the following test is not working on Node runtimes with version 0.x.x:

var nodeVersion = process.env.npm_config_node_version;
if (/^[0-5][.]/.test(nodeVersion || '')) {

I don't know enough about historical oddities of Node runtimes, so I'll have to figure out how to do a robust Node version test.

I'll see if https://www.npmjs.com/package/check-node-version works on older node tomorrow.

[mikesamuel]

I discovered npx which lets me test with various versions.

$ npm install --no-save npx
$ ./node_modules/.bin/npx node@0.12 test/run.js
1..3
ok 1 test/unit/test-Lexer.js
  Skipping ES6 tests for node_version v0.12.18

ok 2 test/unit/test-SqlString.js
  0 fail | 72 pass | 11 ms

ok 3 test/unit/test-Template.js
  Skipping ES6 tests for node_version v0.12.18
  0 fail | 3 pass | 1 ms

[mikesamuel]

Tests run green now.

I've looked over the coverage report. The main sticky point there is

// lib/Template.js
try {
  module.exports = require('./es6/Template');
} catch (ignored) {
  // ES6 code failed to load.
  ...
}

I added tests for the missing branch but those won't be reflected in the coverage report since it does not, IIUC, union coverage from runs on multiple node versions.

[dougwilson]

Sorry I've been busy these past 2 days. The coverage is definitely a union as reported to the PR status -- I maintain many modules that have separate code paths based on versions. Looking at the missing 2.2% it's lines not covered in any Node.js version run.

[mikesamuel]

The coverage is definitely a union as reported to the PR status

Ah. I see https://coveralls.io/builds/15290075/source?filename=lib%2FTemplate.js Nice!

[mikesamuel]

it's lines not covered in any Node.js version run.

It was unreachable since the lexer patterns will all fall back to matching the empty string. Fixed.

[dougwilson]

It was unreachable since the lexer patterns will all fall back to matching the empty string. Fixed.

No, it was as I said at the time I made comments. I'm not sure why you are assuming I'm making comments on changes you made after I made comments. Or am I missing something here? You're saying all 2.2% of those uncovered lines where from the lexer pattern fallback stuff?

[mikesamuel]

No, it was as I said at the time I made comments. I'm not sure why you are assuming I'm making comments on changes you made after I made comments. Or am I missing something here? You're saying all 2.2% of those uncovered lines where from the lexer pattern fallback stuff?

You're not missing anything, and I'm not saying that.

[dougwilson]

You're not missing anything, and I'm not saying that.

Very sorry, I guess I just misunderstood what you're trying to say 😂 So what are you trying to say?

[mikesamuel]

Very sorry, I guess I just misunderstood what you're trying to say 😂 So what are you trying to say?

I saw your comment just after I'd convinced myself that the last missed branch was unreachable, and assumed I hadn't seen it earlier since it hadn't been there.

I was focused on particular lines in files, and did not intend to make a claim about percentages though that is a reasonable interpretation of what I wrote.

[dougwilson]

P.S. thanks so much for all this work ! I think I'm finally understanding why you're adding the lexer and it seems like the old .format would also benefit from the added context information, but the lexer is written in es6. There doesn't seem to be anything in the lexer itself that requires es6 to function, though. One day it would be sweet to post that back to es5 and add to .format so the es6 template can be sugar and everyone can benefit from the additional context / protection that the lexer is enabling instead of letting those users continue to footgun 😂

I need to get going and I'll be back in a few days to continue to review and learn from the code. There is a lot of code, especially in the lexer that is new to me, at least :) Also something to think about would be if you'd be open to committing to helping maintain this stuff for the next 1 year or not. No big deal if not, but just asking because I don't really need to fully understand it prior to merging, for example, if I know you'll be around to help if issues come up 👍

[mikesamuel]

Also something to think about would be if you'd be open to committing to helping maintain this stuff for the next 1 year or not.

I'm planning on doing a variety of open-source hardening things, so I can be available to walk early adopters through and fix bugs that shake out. I'm unlikely to have reliable availability outside of GMT+5 working hours though.

One day it would be sweet to post that back to es5

It should be fairly straightforward.

Trying to recast format in terms of machinery currently provided by Template though should happen after we have early adopters' experiences to inform us.

I need to get going and ...

Ttyt.

[dougwilson]

Can you make changes to the new dependency template-tag-common so it does not have a fragile installation? It has dependencies with ">=" which means when new major versions of those are published they'll get installed, likely breaking installs of this module that used to work.

[mikesamuel]

Docs at https://github.com/mikesamuel/sqlstring/tree/contextual-template-tags#es6-template-tag-support

[mikesamuel]

I answered your questions on the cache, and did all the remaining TODOs.

If the cache is a concern, I could try benchmarking with and without. If that sounds useful, do you have any preferred way of doing micro-benchmarks in JS?

[mikesamuel]

I rebased around releases 2.3.{0,1} which required a git push -f. That required one manual merge around the new 'triple question marks are ignored' and a test I added in test-SqlString.js

[mikesamuel]

Ping?

[mikesamuel]

Ping?

[mikesamuel]

@dougwilson If this is effectively dead, please let me know and I'll close out the PR.

[dougwilson]

I am sorry I have been bad about providing updates. I have been working on rewriting the syntax to be es5 instead of es6 so it can be exposed to everyone and not restricted to only use in template strings (though that would still be supported). There are also some edge cases I fixed as well. I'm going to push them up (as separate commits -- not altering your existing commits) to the branch in this PR so we can review them to make sure they are correct. I know you didn't want to translate it to es5 from our earlier conversations, so I figured I'd help with that effort since I'm the one of us two who is interested in that support :+1:

[mikesamuel]

Thanks for explaining. I wasn't averse to translating to es5. I was just worried that doing that in a single PR might be too much, but it sounds like you've got it sorted.

[mikesamuel]

Fyi, I needed something like this so I wrote safesql to provide template tags for MySQL and Postgres based in part on this PR.