Libpq connection string escaping #1285
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix handlings of libpq connection properties to properly escape single quotes and backslashes. Previously the values were surrounded in single quotes which handled whitespace within the property value, but internal single quotes and backslashes would cause invalid connection strings to be generated.
Update the expect host output in the connection parameter test to expect it to be surrounded by single quotes.
charmander
approved these changes
May 10, 2017
|
This is fantastic @sehrope - thank you! Would you consider this to be a breaking change or a bug fix? If I'm understanding you correctly the old, broken way would never even work so it's unlikely anyone was relying on that behavior. |
|
I'd consider it a bug fix. I can't imagine anyone relying on the old broken functionality. |
|
Okay awesome! thank you @sehrope - will merge it in shortly! |
|
published |
|
@brianc Anytime! And thanks more generally for the awesomeness that is this driver. Been using it happily for years and it's always a pleasure to contribute to it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While silly, the following are perfectly legal identifiers for a username and database:
The existing libpq config code blindly surrounds the passed in parameters using single quotes (ex:
foo => 'foo'). That breaks down when there are backslashes or single quotes in the values.This PR fixes handling of single quotes and backslashes in connection config values used to generate libpq connection strings by escaping any occurrences of those characters.
For most property names there is no change in the valid case (i.e. no funky chars) as they were already quoted. This just corrects handling when they have funky chars.
The two exceptions are
host, andhostaddrwhich previously weren't escaped at all. There's no valid value for either of those that could contain a single quote or even whitespace so it was fine for the valid case. But that also means that you have a config injection if you pass an invalid value likesomehost foo=bar. It's not really possible right now as there's also a DNS lookup which presumably would fail for such a host, but if it were ever removed (which may be a good idea as hostaddr is an optional field anyway and having it unspecified has some value as well) it'd be broken.Either way, the new code also escapes/quotes them as well and I've updated a couple tests to take that into account.