-
-
Notifications
You must be signed in to change notification settings - Fork 775
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
fix: properly escape postgres password #3424
fix: properly escape postgres password #3424
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to add tests for this? Specifically the password you provided, as well as any other edge cases you can think of. You can parametrize test_pg_connection_url_encode_password
in test_config.py
. Something like:
@pytest.mark.parametrize(
"input, expected_output",
[("input_1", "output_1"), ("input_2", "output_2"), ...],
)
def test_pg_connection_url_encode_password(input: str, expected_output: str, monkeypatch):
...
Additionally, does this properly handle cases where users have already escaped their passwords? (i.e. they have already inserted double % signs). Can we add a test for this as well?
Testing added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, just one more bit of feedback and I think this is good to go
Looks good, though I'd prefer to have @hay-kot look at it before signing off in case I'm missing anything |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a small backwards compatibility change so that users who have manually escaped their passwords shouldn't have to update their config again.
@tba-code let me know if there's any issue with that, otherwise I can merge.
@hay-kot we cannot unquote passwords first as it may break others. this was intentionally added then dropped in a previous commit in this PR |
Breaking some configs is unavoidable for this one. Now that the underlying issue is fixed though, there is no longer a reason for people to try and escape any values in the configs. Would rather break the few that did rather than have an obscure bug down the line. I suggest we make a patch note for this. |
LGTM. Agreed that we're not going to be able to account for 100% of users, so this is good enough. Thankfully while it's a pain to have to fix your password one more time, it's not like we're breaking setups or breaking data, so it's a just-one-more-fix kind of breaking change |
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #3423
Fixes #3418
Special notes for your reviewer:
The string manipipulation to find the password in POSTGRES_URL_OVERRIDE is quite ugly but I didn't know of a better way to handle it. Any suggestions for improvements would be appreciated.
Testing
P@ssword!@#$%%^^&&**()+;'"'<>?{}[]
I was able to launch the backend from both POSTGRES_URL_OVERRIDE and POSTGRES_* variables using the same password.