Skip to content
This repository has been archived by the owner on Dec 10, 2022. It is now read-only.

Eliminate potential SQL injection from database queries #30

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

Conversation

Alex-Vol-SV
Copy link

The TODO markers indicating the possibility of SQL injection issues were
used to guide this implementation. Fixed by applying parameterized
queries.

Found a unitest issue that was masked by the use of concatenation in
SQL and fixed the unit tests to match the runtime code execution.

Travis CI updated the base images so the hacky PG 10 install is no
longer needed and on top of that is failing the builds.
The TODO markers indicating the possibility of SQL injection issues were
used to guide this implementation. Fixed by applying parameterized
queries.

Found a unitest issue that was masked by the use of concatenation in
SQL and fixed the unit tests to match the runtime code execution.
@coveralls
Copy link

coveralls commented Dec 23, 2019

Pull Request Test Coverage Report for Build 128

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 72.318%

Totals Coverage Status
Change from base Build 89: 0.3%
Covered Lines: 258
Relevant Lines: 333

💛 - Coveralls

Copy link
Collaborator

@SkUrRiEr SkUrRiEr left a comment

Choose a reason for hiding this comment

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

Changes look good, however there's some details in the tests that should be resolved before this can be merged.

As requested I added an actual DB query in the test. I pretty much
copied the query code from the runtime to use in the test but it gets
the job done.
Copy link
Collaborator

@SkUrRiEr SkUrRiEr left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants