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

Adds bound parameters for enhanced security #31

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

fgheorghe
Copy link

  • Introduces bound parameter support by adding an optional second parameter to method query(), named 'queryParams' of type array, or .queryParams optional 'query' object key of the same type.
  • Updates documentation to reflect the availability of bound parameters

Examples:
await athenaExpress.query('SELECT * FROM movies WHERE movie_title = ?', ['Spider-Man']);
await athenaExpress
.query({ sql: 'SELECT * FROM movies WHERE movie_title = ?', queryParams: ['Spider-Man']});

@coveralls
Copy link

coveralls commented Jan 10, 2020

Pull Request Test Coverage Report for Build 100

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 91: 0.0%
Covered Lines: 3
Relevant Lines: 3

💛 - Coveralls

@fgheorghe fgheorghe mentioned this pull request Jan 10, 2020
@mdesousa
Copy link

mdesousa commented Dec 4, 2020

Hi, I was wondering about the status of this PR? Support for bound parameters is an important security feature... thanks!

@fgheorghe
Copy link
Author

Hi @mdesousa - waiting for it to be merged as indeed it is useful!

@jhkueh
Copy link

jhkueh commented Nov 18, 2021

Thanks for your wonderful work in creating this library, @ghdna.
Is there anything stopping this PR from being merged?
This is a critical security feature.

@ghdna
Copy link
Owner

ghdna commented Nov 19, 2021

ok, let me look into this one next.

@fgheorghe fgheorghe marked this pull request as ready for review December 13, 2021 14:04
@fgheorghe
Copy link
Author

fgheorghe commented Dec 13, 2021

sorry - it's been a while, i can look into updating the code and resolve merge conflicts at some point soon to make it easier for @ghdna

@fgheorghe
Copy link
Author

PR updated to bring it in line with master @ghdna
Please review

@lsharples1
Copy link

Wondering the status on this? Came across this PR 3 years later as I was looking for this feature. Still relevant, please merge if possible!

@pierre-marieb
Copy link

What is the PR status? Athena supports parameterized queries already so it would be really nice to have that integrated in athena-express.

@fgheorghe
Copy link
Author

I suspect this project is abandoned, and considering it doesn't properly escape parameters I'd say it's not secure enough to use. One option is to use code from my PR:
https://github.com/fgheorghe/athena-express/tree/master

@pierre-marieb
Copy link

That's too bad... we went the direction of integrating sqlstring the same way that you did but it would have been nice to have that PR officially merged.

But if it's no longer maintained we might have to go back to using the regular AWS SDK I guess

@ghdna
Copy link
Owner

ghdna commented Aug 23, 2023

Its maintained. The PR has conflicts. Once they are resolved, I can merge it

@fgheorghe
Copy link
Author

Conflicts have been resolved a year ago, and since then new conflicts have been introduced. I can not re-issue a fix as I don't have time for it.

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.

7 participants