Do not double-decode username, password and query values in SQLAlchemy URL#616
Do not double-decode username, password and query values in SQLAlchemy URL#616arpitjain099 wants to merge 1 commit into
Conversation
| # SQLAlchemy's make_url already percent-decodes the userinfo, so we use the | ||
| # username and password as-is. Running them through unquote_plus again would | ||
| # corrupt a literal '+' into a space (form-encoding only applies to the query | ||
| # string, not to the userinfo component). |
There was a problem hiding this comment.
is the comment actually true? form-encoding applies to whole URL AFAIK. Can you add test for the other calls to unquote_plus here?
There was a problem hiding this comment.
hmmmm, also does this mean a literal %2B in the password for example would corrupt the URL? Can you test?
There was a problem hiding this comment.
You are right, that comment was misleading and I have rewritten it. I checked what make_url actually hands over, and it is the same on 1.3.24, 1.4.54 and 2.0.51:
- userinfo (username/password) is decoded with unquote, so %2B becomes + but a literal + stays a +.
- query values are decoded with parse_qsl, which does apply form-encoding, so there + already means space.
- url.database (catalog/schema) is the one part SQLAlchemy leaves url-encoded.
So form-decoding does happen on the query, which means the other unquote_plus calls had the same double-decode bug: an access_token or client_tag containing a + was getting corrupted too. I removed unquote_plus from the query params as well (that also matches how roles was already handled) and kept it only on catalog/schema. New test is test_trino_connection_query_params_preserve_plus, plus jwt/cert cases for those specific call sites.
There was a problem hiding this comment.
Yes, confirmed. With the old code, pass%2Bword got corrupted: SQLAlchemy decodes %2B to +, then unquote_plus turned that + into a space, so it ended up as "pass word" and auth failed. The user%2Bname:pass%2Bword case in the parameterized test asserts it now stays "pass+word". The same was happening on the query side (for example access_token=...%2B...), which is why I also dropped unquote_plus from the query params.
| assert cparams['auth']._password == password | ||
|
|
||
|
|
||
| def test_trino_connection_basic_auth_with_plus_in_credentials(): |
There was a problem hiding this comment.
maybe parameterize the test to also test the value with literal + present?
There was a problem hiding this comment.
Done. test_trino_connection_basic_auth_with_plus_in_credentials is now parameterized over user+name:pass+word (literal +), user%2Bname:pass%2Bword (encoded +), and user%20name:pass%20word (encoded space). The first two come out as +, the last as a space.
…y URL SQLAlchemy's make_url already decodes the userinfo (username and password) and the query values before the dialect sees them, so create_connect_args was decoding them a second time with unquote_plus. The second decode follows the form-encoding rule where '+' means space, which corrupts any value that contains a literal '+'. For example a password given as pass%2Bword arrives from SQLAlchemy as pass+word and was then turned into pass word, failing authentication with a 401. Use the username, password and query values as-is, matching how the roles parameter was already handled. The catalog and schema keep unquote_plus because they come from url.database, which SQLAlchemy leaves url-encoded. Added parameterized tests covering a literal '+' and a percent-encoded '+' (%2B) in the userinfo, plus round-trip tests for the query parameters (source, access_token, cert/key, session_properties, http_headers, extra_credential, client_tags, roles) through the trino URL builder. Closes trinodb#611. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
47656f1 to
c85cf51
Compare
arpitjain099
changed the title
2 participants
Description
When connecting through SQLAlchemy, a value that contains a literal
+(for example a password) was being corrupted into a space, which then failed authentication with a 401. The cause is a double decode: SQLAlchemy'smake_url()already decodes the URL before the dialect sees it, andcreate_connect_argsthen ran the values throughunquote_plusa second time.unquote_plusfollows the form-encoding rule where+means space, sopass+wordbecamepass word.Looking at what
make_url()actually hands over (same on SQLAlchemy 1.3.24, 1.4.54 and 2.0.51):unquote, so%2Bbecomes+but a literal+stays+.parse_qsl, which does apply form-encoding, so both+and%2Bare already resolved.url.database(catalog and schema) is the one component SQLAlchemy leaves url-encoded.So the second
unquote_pluswas wrong on both the userinfo and the query parameters. A password given aspass%2Bwordarrives from SQLAlchemy aspass+wordand was then turned intopass word; the same happened to values likeaccess_tokenorclient_tagsthat contained a+. The fix uses the username, password and query values as-is (matching how therolesparameter was already handled) and keepsunquote_plusonly on the catalog and schema, which come fromurl.database.Added parameterized tests for a literal
+and a percent-encoded+(%2B) in the userinfo, plus round-trip tests for the query parameters through the trino URL builder. The fulltests/unit/sqlalchemy/test_dialect.pysuite passes (24 tests). Closes #611.Non-technical explanation
Passwords, tokens and other connection values that contain a plus sign now work when you connect using a SQLAlchemy URL, instead of failing or being silently changed.
Release notes
( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, with the following suggested text: