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

Protection against SQL injection #67

Open
thevahidal opened this issue Nov 6, 2022 · 7 comments · Fixed by #130
Open

Protection against SQL injection #67

thevahidal opened this issue Nov 6, 2022 · 7 comments · Fixed by #130
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@thevahidal
Copy link
Owner

thevahidal commented Nov 6, 2022

  • Use parameterized query / prepared statement
  • Specify an environment variable to restrict transaction endpoint [default (or production) disabled]
  • Ideally, move all queries to the services directory
@thevahidal thevahidal self-assigned this Nov 6, 2022
@thevahidal thevahidal added this to the hn-feedback milestone Nov 6, 2022
@IanMayo
Copy link
Collaborator

IanMayo commented Jul 18, 2023

Hello @thevahidal - is Soul still secure against SQL injection?

Does the use of db.prepare protect us against SQL injection attacks?

image

Aah, I'm no expert at this, but it looks like better-sqlite3 would rather have the query elements passed outside the prepare statement. I believe this practice is called binding statements. Our context is a little more complex than the below example (since we have to handle lots of possible attributes), but I guess we just build up an array of items to go into prepare, and an array of items to go into all.

image

@thevahidal thevahidal reopened this Jul 20, 2023
@thevahidal
Copy link
Owner Author

It's a very good question @IanMayo,
I'm no expert at this too,
Though here's an issue in better-sqlite about the same matter: WiseLibs/better-sqlite3#720
Let me know if you think of a way to test it

@IanMayo
Copy link
Collaborator

IanMayo commented Jul 20, 2023

I'm pretty sure that Soul currently fails this test:

I understand that if someone dumbly crafts a Statement without parameters for .run() and its friends, they're asking for a SQL injection attack

Note: the Soul infrastructure tightly parses incoming URLs and prevents the above issue happening.

But, in terms of layers of an onion - it seems like we should apply security at all levels where we have a chance. And providing the query params through binding statements is one extra security layer we can apply.

@thevahidal
Copy link
Owner Author

You're definitely right, we should start getting rid of the risky parts. I'll create a new project plan so we can prioritize our goals.

@IanMayo
Copy link
Collaborator

IanMayo commented Jul 20, 2023

I'm sure @AbegaM and I can handle the switch to binding statements. We can get a draft PR started, for your feedback.

@thevahidal
Copy link
Owner Author

Thanks man, you guys are the best.

@thevahidal
Copy link
Owner Author

I would like to improve protection against SQL injection more by using sqlstring. I'll reopen this issue for this matter.

@thevahidal thevahidal reopened this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging a pull request may close this issue.

3 participants