fix(sql): omit connection URL from read_sql error messages and __repr__

[madvart]

Why

A user reported credentials still leaking in daft.read_sql errors on v0.7.11 even after the original redactor in #6902. The URL shape from their stack trace:

trino://<USER>@<HOST>:<PORT>?auth=jwt&access_token=<SECRET>&http_scheme=https

There's no user:password@host here — the secret lives in access_token= as a query parameter. The previous fix only redacted userinfo passwords, so the JWT was emitted in plaintext in the Failed to execute sql: ... from connection: ... error.

Separately, urllib.parse.urlparse silently mis-parses passwords containing # / / / ? (it pushes them into the fragment field), so parsed.password came back None and the old _redact_url returned the URL unchanged for inputs like trino://alice:p#ss@host/db.

I initially tried to plug both holes inside _redact_url (sqlalchemy make_url as the parser, plus query-parameter redaction by sensitive-name substring). Reviewers (rchowell) pushed back: "secrets appear in many places around the connection string and are not limited to query params — we probably shouldn't log it". Agreed. Trying to enumerate every shape a credential can take in a connection URL is a losing game.

What this PR actually does

Stop echoing the connection URL in error messages and in SQLConnection.__repr__. That's it.

• RuntimeError(f\"Failed to execute sql: {sql} from connection: {self.conn}, error: {e}\") → RuntimeError(f\"Failed to execute sql: {sql}, error: {e}\"). The caller knows which connection they passed in; the URL is redundant in the message.
• SQLConnection.__repr__ now returns SQLConnection(dialect='trino', driver='') instead of the URL.

Removed alongside: _redact_url, _SENSITIVE_PARAM_KEYWORDS, _is_sensitive_param, the opportunistic SQLAlchemy make_url import, and the urllib.parse fallback with its @-in-authority heuristic — none of it is needed once the URL itself is not echoed.

After the fix

Failed to execute sql: SELECT * FROM (SELECT * FROM iceberg.namespace.\"table\") AS subquery LIMIT 10,
  error: (trino.exceptions.TrinoConnectionError) failed to execute: HTTPSConnectionPool(...): ...
[SQL: SELECT * FROM (SELECT * FROM iceberg.namespace.\"table\") AS subquery LIMIT 10]

No URL, no userinfo, no query parameters.

Verified end-to-end

Tested locally against the customer's exact URL shape plus two regressions:

Shape	Secret value	Present in error?
Trino JWT (?access_token=<JWT>&auth=jwt)	the JWT	✅ No
Userinfo password (alice:hunter2@...)	hunter2	✅ No
Password with # (alice:p#ss@...)	p#ss	✅ No

Tests

tests/io/test_sql.py:

• test_execute_sql_error_does_not_leak_credentials — parametrized across the three leak shapes; asserts the secret is not in the raised RuntimeError.
• test_repr_does_not_leak_url — asserts repr(conn) contains neither the secret, the host, nor the username.

Related

Follow-up to #6902 / issue #6903. Customer-reported continued leak after v0.7.11.

🤖 Generated with Claude Code

[github-actions]

Rust Dependency Diff

Head: 41ff844dac8b65abc472e3611ba17750b454e3ab vs Base: f7f8c12dcd4428cf598fa64e8db31e7c3efe816c.

✅ OK: Within budget.

• New Crates: 0
• Removed Crates: 0

[greptile-apps]

Greptile Summary

This PR closes credential-leak vectors in daft.read_sql errors that the original fix (#6902) missed. Rather than attempting smart field-by-field redaction, the implementation takes the more robust approach of dropping the connection URL entirely from error messages and __repr__, since secrets can appear in userinfo, query parameters, or driver-specific extras.

• _redact_url is removed; __repr__ now shows only dialect and driver; both _execute_sql_query_with_connectorx and _execute_sql_query_with_sqlalchemy emit Failed to execute sql: {sql}, error: {e} with no URL.
• Tests are restructured around the new contract: test_execute_sql_error_does_not_leak_credentials (parametrized over password, JWT token, and special-char password cases) and test_repr_does_not_leak_url replace the old redaction unit tests.

Confidence Score: 5/5

Safe to merge; the fix correctly eliminates all explicit URL echoing from error messages and repr, and no existing behavior is regressed.

The core change is removing the connection URL from error messages and repr entirely, which is the simplest and most robust mitigation. Both execution paths are covered, and no sensitive data is now added explicitly to any error string.

tests/io/test_sql.py — the Trino JWT parametrize case shares a psycopg2 skip guard it does not actually require

Important Files Changed

Filename	Overview
daft/sql/sql_connection.py	Removes _redact_url entirely and drops the connection URL from all error messages and repr; simpler and more robust than field-specific redaction
tests/io/test_sql.py	Replaces old redaction unit tests with connection-error leak checks; the Trino JWT parametrize case shares a psycopg2 skip guard it doesn't need

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[execute_sql_query called] --> B{_should_use_connectorx?}
    B -- "Yes" --> C[_execute_sql_query_with_connectorx]
    B -- "No" --> D[_execute_sql_query_with_sqlalchemy]
    C -- success --> E[Return pa.Table]
    C -- exception --> F["RuntimeError: Failed to execute sql, error — URL omitted"]
    D -- success --> E
    D -- exception --> F
    style F fill:#fdd,stroke:#900

Loading

_{Reviews (4): Last reviewed commit: "fix(sql): also drop URL from SQLConnecti..." | Re-trigger Greptile}

[madvart]

@greptile-apps re-review

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.05%. Comparing base (f7f8c12) to head (c28ca58).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6933      +/-   ##
==========================================
+ Coverage   75.70%   76.05%   +0.34%     
==========================================
  Files        1138     1138              
  Lines      161543   161511      -32     
==========================================
+ Hits       122304   122835     +531     
+ Misses      39239    38676     -563     

Files with missing lines	Coverage Δ
daft/sql/sql_connection.py	80.00% <100.00%> (-2.46%)	⬇️

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[madvart]

@greptile-apps re-review

[madvart]

@greptile-apps re-review

[rchowell]

Yep this way my thinking, "The caller knows which connection they passed in" -- if they want to debug further then the responsible handling is on them. We aren't going to let the cat out of the bag on their behalf. Nice catch on the repr too

[greptile-apps]

Want your agent to iterate on Greptile's feedback? Try greploops.