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

Escape SQL username and password parameters before substituting them into a URL #7089

Merged
merged 1 commit into from
Jul 8, 2019

Conversation

mgritter
Copy link
Contributor

@mgritter mgritter commented Jul 8, 2019

Customer reported an issue where they could not use "password{0}" as a password in a Postgres connection.

@mgritter mgritter requested review from catsby and jasonodonnell July 8, 2019 22:46
@jefferai jefferai merged commit 7564d84 into master Jul 8, 2019
@jefferai jefferai deleted the escape_sql_parameters branch July 8, 2019 23:03
// Connection() uses sql.Open uses lib/pq uses net/url.Parse
u, err := url.Parse(sql.ConnectionURL)
if err != nil {
t.Errorf("URL parse error on %q %q: %+v", tc.Username, tc.Password, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change these to t.Fatalf() to fail fast?

@calvn
Copy link
Contributor

calvn commented Jul 8, 2019

Whoops didn't refresh after Jeff merged..

{"postgres", "pass/word"},
{"postgres", "p@ssword"},
{"postgres", "pass\"word\""},
// Much to my surprise, CREATE USER "{{password}}" PASSWORD 'foo' worked.
Copy link
Contributor

Choose a reason for hiding this comment

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

PG treats anything quoted as a literal and literals can be used for roles and database names.

Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

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

LGTM 👍

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.

4 participants