Skip to content
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

feat(security): add injection prevention #139

Merged
merged 9 commits into from
Sep 12, 2023

Conversation

ffMathy
Copy link
Contributor

@ffMathy ffMathy commented Sep 3, 2023

  • I have added automatic tests where applicable
  • The PR title is suitable as a release note
  • The PR contains a description of what has been changed
  • The description contains manual test instructions

Changes

I have added a new prepared tagged template literal, which helps protect against injection. I'll need confirmation on whether or not this is enough. See #137.

Test

An example of how to use it:

var session = new Session();
const someDynamicFirstNameWithInjectionAttack = "Mathias' or 1 = 1"

session.query(prepared`select foo from Bar where blah='${someDynamicFirstNameWithInjectionAttack}'

//the resulting query is:
//select foo from Bar where blah='Mathias\' or 1 = 1'

@ffMathy ffMathy requested a review from a team as a code owner September 3, 2023 03:42
Copy link
Contributor

@gismya gismya left a comment

Choose a reason for hiding this comment

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

Good idea! Works well to stop the kind of injections you mention in #137 and I tried my best at breaking it and it seems robust. I added a test suite to it.

Since the code is rather short, some of my comments are borderline bikeshedding.

source/session.ts Outdated Show resolved Hide resolved
source/session.ts Outdated Show resolved Hide resolved
source/session.ts Show resolved Hide resolved
source/session.ts Outdated Show resolved Hide resolved
source/session.ts Outdated Show resolved Hide resolved
source/session.ts Outdated Show resolved Hide resolved
@ffMathy
Copy link
Contributor Author

ffMathy commented Sep 7, 2023

Once again great feedback! I applied some changes.

@ffMathy
Copy link
Contributor Author

ffMathy commented Sep 11, 2023

I changed some more stuff based on review. Should be ready now? 🤞

@gismya
Copy link
Contributor

gismya commented Sep 11, 2023

Fix the merge conflicts and it's ready to go

@ffMathy ffMathy reopened this Sep 12, 2023
@ffMathy
Copy link
Contributor Author

ffMathy commented Sep 12, 2023

Alright, should be good to go now!

@gismya gismya merged commit aa4d9da into ftrackhq:main Sep 12, 2023
@ffMathy ffMathy deleted the feature/injection-prevention branch September 12, 2023 20:50
gismya added a commit that referenced this pull request Sep 13, 2023
* feat(security): add injection prevention

* Update session.ts

* Add tests for template tag

* Add the prepared import to the test

* fix: review changes

* chore: rename `prepared` to `expression`

* fix: conflict fixes

* fix: compile errors

---------

Co-authored-by: Lars Johansson <gismya@gmail.com>
@gismya gismya linked an issue Sep 14, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How do we handle injection attacks?
3 participants