Skip to content

fix(mssql): sanitize brackets in connection-URL userinfo before parse - #2447

Merged
goldmedal merged 1 commit into
Canner:mainfrom
Bartok9:fix/2-mssql-url-brackets
Jul 13, 2026
Merged

fix(mssql): sanitize brackets in connection-URL userinfo before parse#2447
goldmedal merged 1 commit into
Canner:mainfrom
Bartok9:fix/2-mssql-url-brackets

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • mssql:// connection URLs containing [ or ] in the userinfo (username/password) crashed with ValueError from urlparse's IPv6 netloc check.
  • Bracketed credentials now parse, while legitimate bracketed IPv6 hosts still work.

Motivation

_connect_mssql_from_url called urlparse(connection_url) directly. Python's urlparse treats [/] anywhere in the netloc as IPv6 host delimiters, so a URL like mssql://user:p[a]ss@host:1433/db raises ValueError: 'host' does not appear to be an IPv4 or IPv6 address before any connection is attempted — the credential can never be used.

The MySQL connector already solved this exact problem with _parse_mysql_connection_url, which escapes brackets in the userinfo segment only. This change mirrors that helper for MSSQL (_parse_mssql_connection_url), keeping valid IPv6 hosts (mssql://user:pass@[::1]:1433/db) intact.

Verification

$ uv run pytest tests/unit/test_mssql_connection.py -q
18 passed in 0.90s

Regression test proof (fails without the fix):

$ git stash push -- src/wren/connector/mssql.py
$ uv run pytest tests/unit/test_mssql_connection.py -k "brackets or ipv6" -q
FAILED ...test_mssql_url_with_brackets_in_credentials - ValueError: 'host' does not appear to be an IPv4 or IPv6 address
1 failed, 1 passed

The ipv6 test passes both with and without the fix, confirming no regression for legitimate bracketed IPv6 hosts.

Apache-2.0 path (core/**). One commit, two files.

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of MSSQL connection URLs so credentials containing [ or ] are parsed correctly.
    • Preserved support for valid IPv6 hosts in bracketed form while avoiding confusion with bracket characters in the username/password portion.
  • Tests

    • Added coverage for URLs with bracket characters in credentials.
    • Added coverage to confirm IPv6 host parsing still works as expected.

mssql:// URLs with '[' or ']' in credentials (e.g. mssql://user:p[a]ss@host/db)
raised ValueError from urlparse's IPv6 netloc check, so such connections
could never be opened. Mirror the MySQL connector's userinfo sanitiser
(_parse_mysql_connection_url) so bracketed credentials parse while
legitimate bracketed IPv6 hosts still work.
@github-actions github-actions Bot added python Pull requests that update Python code core labels Jul 8, 2026
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This PR adds a helper function to pre-sanitize MSSQL connection URLs by escaping bracket characters within the userinfo segment before URL parsing, preventing misinterpretation as IPv6 delimiters. Two unit tests validate credential bracket handling and legitimate IPv6 host parsing.

Changes

MSSQL URL Parsing Fix

Layer / File(s) Summary
Userinfo bracket sanitization
core/wren/src/wren/connector/mssql.py, core/wren/tests/unit/test_mssql_connection.py
Adds _parse_mssql_connection_url() to escape [/] characters only in the userinfo segment before calling urlparse, updates _connect_mssql_from_url() to use this helper, and adds tests verifying bracketed credentials are preserved while legitimate bracketed IPv6 hosts still parse correctly.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant ConnectMssqlFromUrl as _connect_mssql_from_url
    participant ParseHelper as _parse_mssql_connection_url
    participant Urlparse as urllib.parse.urlparse
    participant PyodbcConnect as _connect_mssql_pyodbc

    Caller->>ConnectMssqlFromUrl: connection_url
    ConnectMssqlFromUrl->>ParseHelper: raw connection_url
    ParseHelper->>ParseHelper: escape [ ] in userinfo segment
    ParseHelper->>Urlparse: sanitized URL
    Urlparse-->>ParseHelper: parsed components
    ParseHelper-->>ConnectMssqlFromUrl: parsed URL result
    ConnectMssqlFromUrl->>ConnectMssqlFromUrl: validate scheme/host/database
    ConnectMssqlFromUrl->>PyodbcConnect: build connection with unquoted UID/PWD
    PyodbcConnect-->>Caller: connection
Loading

Poem

A bracket in the username, oh what a plight,
Mistaken for IPv6, it gave urlparse a fright!
Now I hop through userinfo, escaping with care, 🐰
Brackets stay put, no IPv6 flare,
Two tests confirm the fix is tight! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main fix: sanitizing brackets in MSSQL connection URL userinfo before parsing.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
core/wren/src/wren/connector/mssql.py (1)

340-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Extract the shared userinfo bracket-sanitization helper.

The MSSQL and MySQL parsers use the same bracket-escaping logic; moving it into a shared URL utility would keep the two implementations from drifting.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/wren/src/wren/connector/mssql.py` around lines 340 - 369, The MSSQL URL
parser `_parse_mssql_connection_url` duplicates the same userinfo
bracket-escaping logic already used by the MySQL parser, so extract that
sanitization into a shared URL utility and have both parsers call it. Keep the
parsing behavior in `_parse_mssql_connection_url` the same, but move the common
`userinfo` sanitization/authority-splitting logic into a reusable helper to
prevent the MSSQL and MySQL implementations from drifting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@core/wren/src/wren/connector/mssql.py`:
- Around line 340-369: The MSSQL URL parser `_parse_mssql_connection_url`
duplicates the same userinfo bracket-escaping logic already used by the MySQL
parser, so extract that sanitization into a shared URL utility and have both
parsers call it. Keep the parsing behavior in `_parse_mssql_connection_url` the
same, but move the common `userinfo` sanitization/authority-splitting logic into
a reusable helper to prevent the MSSQL and MySQL implementations from drifting.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e89ae179-0696-4ee2-ad4f-e466ea57f402

📥 Commits

Reviewing files that changed from the base of the PR and between 7db2651 and 5948933.

📒 Files selected for processing (2)
  • core/wren/src/wren/connector/mssql.py
  • core/wren/tests/unit/test_mssql_connection.py

@goldmedal
goldmedal merged commit 5d94aaa into Canner:main Jul 13, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants