Skip to content

If password was encoded, it would double encode. Switched to only enc… #6

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.

Sign up for GitHub

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

Merged
caleb-mabry merged 6 commits into from
Jan 7, 2026
Merged

If password was encoded, it would double encode. Switched to only enc… #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d75e331
If password was encoded, it would double encode. Switched to only enc…
caleb-mabry Jan 7, 2026
97cea5c
Replaced import path
caleb-mabry Jan 7, 2026
ad8cff7
Linting fix
caleb-mabry Jan 7, 2026
74aeb21
Add additional test cases
caleb-mabry Jan 7, 2026
e9976a5
Resolve formatting
caleb-mabry Jan 7, 2026
08c84ae
Lint fix
caleb-mabry Jan 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/postgres_mcp/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@
from .index.llm_opt import LLMOptimizerTool
from .index.presentation import TextPresentation
from .moldes.model import AccessMode
from .resource import register_resource_templates
from .resource import format_error_response
from .resource import format_text_response
from .resource import register_resource_templates
from .sql import SafeSqlDriver
from .sql import check_hypopg_installation_status
from .sql import obfuscate_password
Expand Down
5 changes: 4 additions & 1 deletion src/postgres_mcp/utils/url.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from urllib.parse import quote
from urllib.parse import unquote


def fix_connection_url(url: str) -> str:
Expand All @@ -10,7 +11,9 @@ def fix_connection_url(url: str) -> str:
user_pass = url[scheme_end:at_pos]
if ":" in user_pass:
username, password = user_pass.split(":", 1)
encoded_password = quote(password, safe="")
# If password is already encoded, decode it.
plain_password = unquote(password)
encoded_password = quote(plain_password, safe="")
Comment on lines +15 to +16

Copilot AI Jan 7, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
return url[:scheme_end] + username + ":" + encoded_password + url[at_pos:]

Copilot AI Jan 7, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
except Exception as e:
print(e)
Comment on lines 18 to 19

Copilot AI Jan 7, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Expand Down
23 changes: 23 additions & 0 deletions tests/unit/utils/test_url.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import pytest

from postgres_mcp.utils.url import fix_connection_url


@pytest.mark.parametrize(
"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"),

Copilot AI Jan 7, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
("postgresql://user:pass%word@localhost:5432/db", "postgresql://user:pass%25word@localhost:5432/db"),
("postgresql://user:?pass%25wo%%rd@localhost:5432/db", "postgresql://user:%3Fpass%25wo%25%25rd@localhost:5432/db"),
],
)
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
Comment on lines +15 to +17

Copilot AI Jan 7, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.


def test_fix_connection_url_no_mutation():
"""Ensure a standard safe URL is not changed."""
url = "postgresql://readonly:securepassword123@db.example.com:5432/postgres"
assert fix_connection_url(url) == url