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

PRI-106 - fix: sql injection by parsing to number #52

Merged
merged 2 commits into from
Sep 5, 2024
Merged

Conversation

kgmyatthu
Copy link
Contributor

No description provided.

Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ No key issues to review

Copy link
Contributor

@arhamj arhamj left a comment

Choose a reason for hiding this comment

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

@kgmyatthu can we also make the query parameterised using either a named param or a ? unnamed param?

@kgmyatthu
Copy link
Contributor Author

kgmyatthu commented Sep 3, 2024

@kgmyatthu can we also make the query parameterised using either a named param or a ? unnamed param?

I remember trying this with the sql package that's currently in use. It didn't work. I'll revisit them again.
UPDATE: Ok we had a internal discussion. Parameterized queries will come later with separate sweep operation. Right now we agreed to just put targeted fixes at the input level.

arhamj
arhamj previously approved these changes Sep 4, 2024
fix: broken regex enclosed in double quotes
afostr
afostr previously approved these changes Sep 4, 2024
Copy link
Contributor

@afostr afostr left a comment

Choose a reason for hiding this comment

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

looks good thanks

@mhanson-github mhanson-github merged commit 6263243 into dev Sep 5, 2024
7 checks passed
@mhanson-github mhanson-github deleted the PRI-106 branch September 15, 2024 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants