If password was encoded, it would double encode. Switched to only enc…#6
If password was encoded, it would double encode. Switched to only enc…#6caleb-mabry merged 6 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix a double-encoding issue where passwords that were already URL-encoded would be encoded again. The solution unconditionally decodes passwords using unquote() before encoding them with quote().
Key Changes:
- Added
unquoteimport to decode passwords before encoding - Modified the password encoding logic to decode first, then encode
- Added comprehensive test cases covering both plain and pre-encoded passwords
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/postgres_mcp/utils/url.py | Modified password encoding logic to unquote before quoting to prevent double-encoding |
| tests/unit/utils/test_url.py | Added new test file with parametrized tests for encoding behavior and URL mutation checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| plain_password = unquote(password) | ||
| encoded_password = quote(plain_password, safe="") |
There was a problem hiding this comment.
The unconditional unquote-then-quote approach can cause issues with passwords that legitimately contain percent-encoded sequences. For example, if a password is literally "pass%3Fword" (containing the literal characters %, 3, F), unquoting it would convert it to "pass?word", then quote it back to "pass%3Fword", which would be incorrect.
This approach assumes that any percent-encoding in the password is meant to be decoded, but percent signs could be part of the actual password. The current implementation cannot distinguish between a password that was already URL-encoded versus a password that literally contains percent-encoded sequences as part of its plaintext value.
| # If password is already encoded, decode it. | ||
| plain_password = unquote(password) | ||
| encoded_password = quote(plain_password, safe="") | ||
| return url[:scheme_end] + username + ":" + encoded_password + url[at_pos:] |
There was a problem hiding this comment.
The username could also contain special characters that need encoding, but the current implementation only encodes the password. If the username contains characters like '@', ':', '/', '?', or other reserved characters, they should also be properly encoded to avoid parsing issues.
| "input_url, expected_output", | ||
| [ | ||
| ("postgresql://user:pass?word@localhost:5432/db", "postgresql://user:pass%3Fword@localhost:5432/db"), | ||
| ("postgresql://user:pass%3Fword@localhost:5432/db", "postgresql://user:pass%3Fword@localhost:5432/db"), |
There was a problem hiding this comment.
This test case doesn't actually verify the intended behavior. A password that literally contains the characters "pass%3Fword" (with a literal percent sign) would need different handling than a password "pass?word" that was already encoded. The current implementation cannot distinguish between these two cases, and this test doesn't address that ambiguity.
| except Exception as e: | ||
| print(e) |
There was a problem hiding this comment.
Using a bare print statement to log exceptions in production code is not a best practice. Consider using proper logging (e.g., Python's logging module) to record errors with appropriate severity levels. Additionally, swallowing all exceptions without re-raising or handling them specifically may hide important errors from callers.
| def test_fix_connection_url_encoding(input_url: str, expected_output: str) -> None: | ||
| """Verifies that passwords are encoded once and only once.""" | ||
| assert fix_connection_url(input_url) == expected_output |
There was a problem hiding this comment.
Missing test coverage for edge cases such as: passwords with multiple special characters (e.g., "pass?word&123"), passwords with spaces, passwords containing '@' symbol, empty passwords, URLs without passwords, and URLs with only username but no password. These scenarios should be tested to ensure the function handles them correctly.
If the password passed in was already encoded, it would double encode the password.
Working
pytest