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

SQL identifiers and string literals aren't escaped in migration.go #537

Closed
grahamhoyes opened this issue Oct 26, 2023 · 3 comments · Fixed by #538
Closed

SQL identifiers and string literals aren't escaped in migration.go #537

grahamhoyes opened this issue Oct 26, 2023 · 3 comments · Fixed by #538

Comments

@grahamhoyes
Copy link
Contributor

I'm working on setting up a Homer install backed by a Google Cloud SQL Postgres database which uses IAM authentication. When doing so, the database user's username is of the form [email protected].

This causes issues with most of the code in migrations/migration.go due to a lack of escaping/quoting for identifiers with just using fmt.Sprintf - the - and @s are a syntax error:

postgres=# CREATE DATABASE homer_data OWNER [email protected];
ERROR:  syntax error at or near "-"
LINE 1: CREATE DATABASE homer_data OWNER my-service-account@my-proje...
                                           ^
postgres=# CREATE DATABASE homer_data OWNER [email protected];
ERROR:  syntax error at or near "@"
LINE 1: CREATE DATABASE homer_data OWNER [email protected];
                                                ^

For this to work properly, the username needs to be double-quoted:

CREATE DATABASE homer_data OWNER "[email protected]";

A similar issue exists when creating a new user due to the password being unescaped - any 's in the password will cause a syntax error at best. Worst case, this code is also vulnerable to SQL injection if there is anywhere that untrusted/unsanitized user inputs are accepted.

I am unfamiliar with the go landscape and Homer's database layer - if parameter binding is possible, that would be the ideal solution. If not, here is an example implementation of proper quoting for identifiers (users, column names, database names, etc) and here for literal strings (like the password). Note that these are just examples I'm familiar with, be aware of that repo's BSD 3-Clause license.

Relevant logs from homer, with fake names:

CONNECT to DB ROOT STRING: [host=localhost [email protected] dbname=postgres sslmode=disable port=5432 password=ignored_by_cloud_sql_proxy]
 
HOMER - create db [homer_config] with [[email protected]]

(/homer-app/migration/migration.go:85)
[2023-10-26 17:36:56]  pq: syntax error at or near "-"

(/homer-app/migration/migration.go:85)
[2023-10-26 17:36:56]  [2.58ms]  CREATE DATABASE homer_config OWNER [email protected]
@github-actions
Copy link

Your report is appreciated. Please star this repository to motivate its developers! ⭐

@lmangani
Copy link
Member

lmangani commented Oct 26, 2023

Thanks for the report @grahamhoyes
Feel free to submit a PR with any desired changes to achieve compatibility with your target.

@grahamhoyes
Copy link
Contributor Author

Some details on a possible fix, I am not familiar with go or gorm so there may be inaccuracies:

  • I've only seen parameter binding with ?, %s, :name, etc work for values, not identifiers. That seems to be backed up by this stackoverflow post. So binding with ? could (and probably should) be used for the password when creating the user, but not elsewhere
  • The issue https://github.com/golang/go/issues/18478 (intentionally not linked to avoid a spammy backlink) has been open for many years on providing escaping in database/sql, which I believe gorm is built on. Seems unlikely that will go anywhere.
  • The postgres docs on syntax are here. The important part, which is what the above examples from pg8000 do:

    Quoted identifiers can contain any character, except the character with code zero. (To include a double quote, write two double quotes.)

  • github.com/lib/pq has a QuoteIdentifier function here. That's probably exactly what we want. It doesn't cover the \0-containing edge case, but should be good enough.

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 a pull request may close this issue.

2 participants