New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a string template tag handler for securely composing queries. #1926

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@mikesamuel
Copy link

mikesamuel commented Jan 23, 2018

This is a rough draft. It is probably not suitable in its current
form.

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 enables

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

and similar idioms.

mikesamuel added some commits Jan 23, 2018

Add a string template tag handler for securely composing queries.
This is a rough draft.  It is probably not suitable in its current
form.

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 enables

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

and similar idioms.
Add TZ=GMT to testing scripts so that the MySQL client does consisten…
…t things when escaping JavaScript Date values
@mikesamuel

This comment has been minimized.

Copy link

mikesamuel commented Jan 24, 2018

It looks like tests pass for node >= 6 and fail for node < 6. This is because of ES6 language features used. Working around those to be ES3.1-compatible shouldn't be a problem if this looks largely good.

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Jan 26, 2018

Hi @mikesamuel makes sense. Yes, ideally if this can work on all Node.js versions, that would be best, because we strive to support as many Node.js versions as possible since this is a very low-level driver module (as compared to a higher level ORM, etc.).

I would, though, say that the actual formatting code should not live in this module, otherwise it cannot be shared with the other MySQL modules like "mysql2" etc. The module https://github.com/mysqljs/sqlstring is dedicated to hosting the code for formatting.

@mikesamuel

This comment has been minimized.

Copy link

mikesamuel commented Jan 26, 2018

Ok. So to move this forward, I could move Template.js and its tests into a separate npm module.
Could that live under the github.com/mysqljs org?

On failing gracefully on older node runtimes, I could define a utility file:

var calledAsTemplateTagQuick = (function () {
  try {
    return require('template-tag-common').calledAsTemplateTagQuick;
  } catch (ignored) {
    // Occurs if an ES6 parser is unavailable which is true for Node <= 6
    // String templates are not available there either, but might mismatch
    // if the calling code is transpiled, but mysql and its deps aren't.
    return function () { return false; };
  }
}());

If we don't attempt to load the interpolation handling code in branches that
don't pass that function, we should never observe that.

Test code would have to do an explicit version test when deciding whether to skip
tests and wrap template tag uses inside new Function("...").

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Jan 26, 2018

Why not into the sqlstring module?

@mikesamuel

This comment has been minimized.

Copy link

mikesamuel commented Jan 26, 2018

Why not into the sqlstring module?

Sorry, didn't read closely enough. Will do.

mikesamuel added a commit to mikesamuel/sqlstring that referenced this pull request Jan 26, 2018

Contextual string tags to prevent SQL injection
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.query`SELECT * FROM T WHERE x = ${x}, y = ${y}, z = ${z}`(callback)

and similar idioms.

This was broken out of mysqljs/mysql#1926
@mikesamuel

This comment has been minimized.

Copy link

mikesamuel commented Jan 26, 2018

@dougwilson I filed mysqljs/sqlstring#29 there. If you want to close this, I can see what happens there and then come up with a new PR here, or if you want to leave this open, I can layer another commit on top once I'm ready to integrate a new version of sqlstring.

mikesamuel added a commit to mikesamuel/sqlstring that referenced this pull request Mar 7, 2018

Contextual string tags to prevent SQL injection
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.query`SELECT * FROM T WHERE x = ${x}, y = ${y}, z = ${z}`(callback)

and similar idioms.

This was broken out of mysqljs/mysql#1926

@dougwilson dougwilson force-pushed the mysqljs:master branch 2 times, most recently from 946727b to 37fbbdd Nov 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment