Skip to content

JS: Add support for knex #6086

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

Merged
merged 2 commits into from
Jun 18, 2021
Merged

JS: Add support for knex #6086

merged 2 commits into from
Jun 18, 2021

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Jun 15, 2021

Adds support for knex, a SQL query builder library.

Evaluations: (internal links)

@asgerf asgerf added JS JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis labels Jun 15, 2021
@asgerf asgerf requested a review from a team as a code owner June 15, 2021 15:20
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One thought on the empty getAQueryArgument.

)
}

override DataFlow::Node getAQueryArgument() { none() }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor usability: I suppose it can be confusing for new users of the library to observe that SQL::SqlString exists and that DatabaseAccess::getAQueryArgument does not. But I can see how it is messy to tie the two together in this model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could say that the receiver of the then call is the query argument (i.e the Knex query object is the query). But we don't actually use the result for anything currently so it's hard to say what would be a useful implementation.

@codeql-ci codeql-ci merged commit 1ffd9c9 into github:main Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants