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

Add Escaping for Identifiers and Literals in Migrations #538

Merged

Conversation

grahamhoyes
Copy link
Contributor

@grahamhoyes grahamhoyes commented Oct 27, 2023

Fixes #537

Adds escaping for identifiers (table names, users, etc) using pq.QuoteIdentifier, and for literals using pq.QuoteLiteral. This allows usernames with characters like - and @, passwords with ', and resolves a SQL injection vulnerability.

Updates github.com/lib/pq to the latest version. It needed to be upgraded to at least v1.2.0, which added the QuoteLiteral function.

Verification done thus far:

  • Ran homer image targeting a database with a regular username/password
  • Ran homer targeting a GCP Cloud SQL database with IAM authentication (which has - and @ in the username)
  • Ran the initialization commands in the readme
  • Loaded the image onto a dev environment with a real setup and poked around in it

@CLAassistant
Copy link

CLAassistant commented Oct 27, 2023

CLA assistant check
All committers have signed the CLA.

@grahamhoyes grahamhoyes marked this pull request as ready for review October 27, 2023 18:20
@grahamhoyes grahamhoyes changed the title Add Escaping for Identifiers and Literals in Migrations (Fix #537) Add Escaping for Identifiers and Literals in Migrations Oct 27, 2023
@grahamhoyes
Copy link
Contributor Author

Got this running in a few environments, everything seems to be working as expected. @lmangani if there's any other testing you'd like to see let me know, otherwise this should be good for you to review.

@lmangani lmangani requested a review from adubovikov October 31, 2023 15:20
@lmangani lmangani added the enhancement New feature or request label Oct 31, 2023
@lmangani
Copy link
Member

Thanks @grahamhoyes the change is under review and we'll have an update soon! We appreciate your contribution!

@lmangani
Copy link
Member

lmangani commented Nov 2, 2023

@adubovikov please review whenever possible 👍

@adubovikov adubovikov merged commit 8361b3b into sipcapture:master Nov 2, 2023
2 checks passed
@grahamhoyes
Copy link
Contributor Author

Apologies if there's a proper process I'm missing, but would it be possible to make a release that contains this @adubovikov? 1.4.59 was made just before this was merged.

Thanks!

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
None yet
Development

Successfully merging this pull request may close these issues.

SQL identifiers and string literals aren't escaped in migration.go
4 participants