Skip to content

fix(trino): sanitize bracketed userinfo and URL-decode credentials - #2472

Merged
goldmedal merged 1 commit into
Canner:mainfrom
Bartok9:fix/trino-url-brackets
Jul 20, 2026
Merged

fix(trino): sanitize bracketed userinfo and URL-decode credentials#2472
goldmedal merged 1 commit into
Canner:mainfrom
Bartok9:fix/trino-url-brackets

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Escape raw [ / ] in Trino URL userinfo before urlparse so bracketed passwords no longer raise ValueError on IPv6 netloc checks.
  • Keep real IPv6 host parsing ([::1]) intact and continue URL-decoding credentials/catalog/schema with unquote (not unquote_plus) so literal + is preserved.
  • Move the pure Docker-free URL-parser regression tests into tests/unit/test_trino_parser.py so the test-unit CI job covers them (CodeRabbit nitpick).

Motivation

Follows / completes #2435.

Passwords containing raw brackets in trino:// connection URLs crash urllib.parse.urlparse because it treats any bare [ / ] in netloc as IPv6 markers. This is a small, MySQL-style sanitizer limited to the userinfo segment.

Verification

  • Local pure-helper checks on _parse_trino_url:
    • trino://user:p[a]ss@host:8080/catalog/schema → password p[a]ss
    • trino://user:p[a]ss@[::1]:8080/catalog/schema → host ::1, password p[a]ss
    • percent-encoded creds still decode
    • literal + preserved
  • Diff is 2 files only vs main.
  • Earlier CI reds on fix(trino): sanitize bracketed userinfo and URL-decode credentials #2455 were Cargo.lock --locked maturin failures across unit/postgres/mysql/ui/memory (infra/stale, shared with other open PRs), not trino-logic failures.

Notes

#2455 closed after a force-push mishap; this is a clean single-commit recreate on current main with the CodeRabbit nitpick applied.

Summary by CodeRabbit

  • Bug Fixes
    • Improved Trino connection URL parsing when credentials include literal square brackets ([/]), ensuring they no longer break parsing and are retained correctly.
    • Correctly preserves literal + characters in credentials and decodes %-escapes as expected.
    • Maintains proper parsing of IPv6 hosts in bracket notation ([::1]) even when the password contains bracket characters.

@github-actions github-actions Bot added python Pull requests that update Python code core labels Jul 10, 2026
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@Bartok9, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 57 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a37ef224-4888-4aac-9402-fcaf4e5f1ffe

📥 Commits

Reviewing files that changed from the base of the PR and between 5a16599 and db604db.

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

Walkthrough

Trino connection URL parsing now sanitizes brackets in userinfo before parsing, preserves literal plus signs during decoding, and adds tests for bracketed passwords with regular and IPv6 hosts.

Changes

Trino URL parsing

Layer / File(s) Summary
Userinfo sanitization and parsing coverage
core/wren/src/wren/connector/trino.py, core/wren/tests/unit/test_trino_parser.py
Raw brackets are escaped only within URL credentials before parsing, decoded with unquote, and tested with regular and IPv6 hosts.

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

Possibly related PRs

Suggested reviewers: goldmedal

Poem

I’m a bunny with brackets tucked tight,
Parsing passwords just right.
IPv6 hops through the host’s little door,
While plus signs stay as they were before.
unquote makes the credentials bright!

🚥 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 describes the main Trino URL parsing fix and credential decoding behavior.
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.

@goldmedal

Copy link
Copy Markdown
Collaborator

The shared CI failure here (maturin failedcannot update the lock file core/wren-core-py/Cargo.lock because --locked was passed) came from the Rust core 0.2.0 bump in #2467 leaving core/wren-core-py/Cargo.lock out of sync. That's now fixed on main by #2478 — please rebase onto latest main to pick up the lockfile fix and turn CI green. 🙏

@Bartok9
Bartok9 force-pushed the fix/trino-url-brackets branch from d7ee523 to 5a16599 Compare July 13, 2026 02:32
@Bartok9

Bartok9 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Done — rebased onto latest main to pick up the core/wren-core-py/Cargo.lock fix from #2478. CI should go green now. Thanks @goldmedal! 🙏

Raw '[' / ']' in trino:// credentials raise ValueError from urlparse's
IPv6 netloc check. Escape only the userinfo segment (preserving real
IPv6 hosts), then unquote username/password/catalog/schema so both the
escaped brackets and ordinary percent-encoded reserved characters land
as the intended strings. unquote (not unquote_plus) keeps literal '+'.

Move the pure URL-parser regressions into tests/unit/test_trino_parser.py
so the Docker-free test-unit CI job covers them.
@Bartok9
Bartok9 force-pushed the fix/trino-url-brackets branch from 5a16599 to db604db Compare July 13, 2026 12:11

@goldmedal goldmedal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @Bartok9 👍

@goldmedal
goldmedal merged commit 769e222 into Canner:main Jul 20, 2026
9 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