URL encode username and password values for DB connections#7486
URL encode username and password values for DB connections#7486
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
vcruces
left a comment
vcruces
left a comment
There was a problem hiding this comment.
Great catch! I added a comment recommending a defensive check, just to be safe
| database = f"/{config.database}" if config.database else "" | ||
| url = f"redshift+psycopg2://{config.user}:{config.password}@{local_host}{port}{database}" | ||
| user = quote(str(config.user), safe="") | ||
| password = quote(str(config.password), safe="") |
There was a problem hiding this comment.
What do you think about adding a check to ensure the credential exists before applying str?
If config.password is None, str(None) converts this to the literal string "None", resulting in URLs like: redshift+psycopg2://user:None@...
This differs from the MySQL/Postgres implementation, which conditionally includes credentials only when they exist
| config = self.secrets_schema(**self.configuration.secrets or {}) | ||
|
|
||
| url_encoded_password = quote_plus(config.password) | ||
| user = quote(str(config.user), safe="") |
There was a problem hiding this comment.
I noticed this changed from quote_plus to quote. I don’t see an issue with using quote here, if anything, it may even be preferable, but I’m wondering whether the original use of quote_plus was intentional 🤔
Not raising this as a change request, just something to keep in mind in case we see any related behavior changes down the line
2 participants
Ticket ENG-2808
Description Of Changes
This should resolve an issue wherein passwords with special characters are properly URL encoded, precluding SQLAlchemy parsing issues.
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works