Escape url parameters in sqlalchemy connection strings #235

mdesmet · 2022-09-19T16:12:09Z

Currently it is not possible to create a Trino url in sqlalchemy urls containing characters like &, ' ' or @. The values should be properly encoded and decoded.

This PR provides a helper method that properly encodes the passed values and ensures that values are properly decoded before passing down as dbapi.connect parameters.

>>> URL(
...                         user="user@test.org/my_role",
...                         password="pass /*&",
...                         host="localhost",
...                         session_properties={"query_max_run_time": "1d"},
...                         http_headers={"trino": 1},
...                         extra_credential=[
...                             ("user1@test.org/my_role", "user2@test.org/my_role"),
...                             ("user3@test.org/my_role", "user36@test.org/my_role")],
...                         experimental_python_types=True,
...                         client_tags=["1 @& /\"", "sql"],
...                     )
'trino://user%40test.org%2Fmy_role:pass %2F*&@localhost:8080?source=trino-sqlalchemy&session_properties=%7B%22query_max_run_time%22%3A+%221d%22%7D&http_headers=%7B%22trino%22%3A+1%7D&extra_credential=%5B%28%27user1%40test.org%2Fmy_role%27%2C+%27user2%40test.org%2Fmy_role%27%29%2C+%28%27user3%40test.org%2Fmy_role%27%2C+%27user36%40test.org%2Fmy_role%27%29%5D&client_tags=%5B%221+%40%26+%2F%5C%22%22%2C+%22sql%22%5D&experimental_python_types=true'

Release notes

( ) This is not user-visible and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

* Support sqlalchemy url generation through `trino.sqlalchemy.URL` helper method

hovaesco · 2022-09-22T09:48:16Z

tests/unit/sqlalchemy/test_dialect.py

@@ -19,17 +20,29 @@ def setup(self):
        "url, expected_args, expected_kwargs",
        [
            (
-                make_url("trino://user@localhost"),
+                make_url(trino_url(


It's worth to keep existing tests and create new ones with URL method next to them.

Maybe we can simply assert that the string being generated is equal to the above string?

Sounds good.

hovaesco · 2022-09-22T09:58:39Z

trino/sqlalchemy/util.py

+    key: Optional[str] = None,
+) -> str:
+    """
+    Composes a SQLAlchemy connect string from the given database connection


Suggested change

Composes a SQLAlchemy connect string from the given database connection

Composes a SQLAlchemy connection string from the given database connection

hovaesco · 2022-09-22T10:05:15Z

trino/sqlalchemy/util.py

+from sqlalchemy.engine.url import _rfc_1738_quote  # noqa
+
+
+def _url(


Some parameters are missing like verify or redirect_handler

OAuth is not yet supported in the url. I think we can leave that for another PR. I have added verify

Sure, just please create a follow-up issue for it after merging, Kerberos doesn't seem supported as well, right?

Correct I supported only the fields that are currently parsed in the sqlalchemy connection string (and added verify).

hovaesco · 2022-09-26T08:54:33Z

tests/unit/sqlalchemy/test_dialect.py

+                    port=8080,
+                    catalog="system",
+                    user="user@test.org/my_role",
+                    auth=BasicAuthentication("user@test.org/my_role", "pass /*&"),


Add test for other supported auth methods: JWT and Certificate.

Added both tests.

hovaesco · 2022-09-26T08:55:28Z

trino/sqlalchemy/util.py

+from sqlalchemy.engine.url import _rfc_1738_quote  # noqa
+
+
+def _url(


Sure, just please create a follow-up issue for it after merging, Kerberos doesn't seem supported as well, right?

mdesmet · 2022-09-26T11:54:12Z

@hovaesco: PTAL

mdesmet · 2022-09-27T11:27:31Z

@hashhar, @ebyhr : Please have a look. This is important for Galaxy users to be able to use sqlalchemy.

README.md

trino/sqlalchemy/dialect.py

mdesmet · 2022-10-03T17:23:59Z

@hashhar : PTAL

hashhar · 2022-10-04T11:29:37Z

trino/sqlalchemy/dialect.py


        if "extra_credential" in url.query:
-            kwargs["extra_credential"] = literal_eval(url.query["extra_credential"])
+            kwargs["extra_credential"] = literal_eval(unquote_plus(url.query["extra_credential"]))


unsafe to do literal_eval on user input. Might not be a concern for the client itself but any tool using the client then becomes impacted.

Note that literal_eval only takes into account literals. So it is not actually evaluating arbitrary code only structural and literal types.

def literal_eval(node_or_string): """ Safely evaluate an expression node or a string containing a Python expression. The string or node provided may only consist of the following Python literal structures: strings, bytes, numbers, tuples, lists, dicts, sets, booleans, and None. """

It's safe from executing arbitrary code but I can still crash the Python VM with my input. Try "1 + 100"*1000000 as input.

My point is there are alternative ways to serialize this and we should use them instead of doing the easy thing.

hashhar · 2022-10-04T11:30:20Z

trino/sqlalchemy/util.py

+        trino_url += f"&http_headers={quote_plus(json.dumps(http_headers))}"
+
+    if extra_credential is not None:
+        trino_url += f"&extra_credential={quote_plus(repr(extra_credential))}"


using repr for serialization seems like a shortcut.

json.dumps converts tuples into array. that's why we can't use it there. I have added a comment to explain the reasoning.

I mean we can write our own value class over this list of tuple, serialize the value class, deserialize value class and provide a method on value class to convert back into list of tuple.

mdesmet · 2022-10-05T07:43:05Z

@hashhar : PTAL. I have not added another class as it is a simple one liner using the typical json.loads recipe but looping through it and instantiating a tuple based on the array.

kwargs["extra_credential"] = [
    tuple(extra_credential) for extra_credential in json.loads(unquote_plus(url.query["extra_credential"]))
]

cla-bot bot added the cla-signed label Sep 19, 2022

mdesmet requested a review from hovaesco September 19, 2022 16:12

mdesmet force-pushed the bug/sqlalchemy-urlencode branch 2 times, most recently from b934f2d to 3ac3aab Compare September 20, 2022 06:38

hovaesco reviewed Sep 22, 2022

View reviewed changes

mdesmet force-pushed the bug/sqlalchemy-urlencode branch from 3ac3aab to ff83437 Compare September 23, 2022 11:32

mdesmet requested a review from hovaesco September 23, 2022 12:51

hovaesco reviewed Sep 26, 2022

View reviewed changes

mdesmet force-pushed the bug/sqlalchemy-urlencode branch 2 times, most recently from c81c1db to 54d8603 Compare September 26, 2022 11:51

hovaesco approved these changes Sep 27, 2022

View reviewed changes

mdesmet requested review from ebyhr and hashhar September 27, 2022 11:27

hashhar reviewed Oct 3, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

hashhar reviewed Oct 3, 2022

View reviewed changes

trino/sqlalchemy/dialect.py Show resolved Hide resolved

mdesmet force-pushed the bug/sqlalchemy-urlencode branch from 54d8603 to 90878ef Compare October 3, 2022 17:21

hashhar approved these changes Oct 4, 2022

View reviewed changes

hashhar reviewed Oct 4, 2022

View reviewed changes

mdesmet force-pushed the bug/sqlalchemy-urlencode branch from 90878ef to 8f48c28 Compare October 4, 2022 12:02

Escape url parameters in sqlalchemy connection strings

9f2ac1b

mdesmet force-pushed the bug/sqlalchemy-urlencode branch from 8f48c28 to 9f2ac1b Compare October 4, 2022 21:44

hashhar approved these changes Oct 5, 2022

View reviewed changes

hashhar merged commit cbda49c into trinodb:master Oct 5, 2022

hashhar mentioned this pull request Oct 5, 2022

Add 0.317.0 release notes #238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Escape url parameters in sqlalchemy connection strings #235

Escape url parameters in sqlalchemy connection strings #235

mdesmet commented Sep 19, 2022

hovaesco Sep 22, 2022

mdesmet Sep 22, 2022

hovaesco Sep 22, 2022

hovaesco Sep 22, 2022

hovaesco Sep 22, 2022

mdesmet Sep 23, 2022

hovaesco Sep 26, 2022

mdesmet Sep 26, 2022

hovaesco Sep 26, 2022

mdesmet Sep 26, 2022

hovaesco Sep 26, 2022

mdesmet commented Sep 26, 2022

mdesmet commented Sep 27, 2022

mdesmet commented Oct 3, 2022

hashhar Oct 4, 2022

mdesmet Oct 4, 2022

hashhar Oct 4, 2022

hashhar Oct 4, 2022

mdesmet Oct 4, 2022 •

edited

Loading

hashhar Oct 4, 2022

mdesmet commented Oct 5, 2022

	Composes a SQLAlchemy connect string from the given database connection
	Composes a SQLAlchemy connection string from the given database connection

		from sqlalchemy.engine.url import _rfc_1738_quote # noqa


		def _url(

Escape url parameters in sqlalchemy connection strings #235

Escape url parameters in sqlalchemy connection strings #235

Conversation

mdesmet commented Sep 19, 2022

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdesmet commented Sep 26, 2022

mdesmet commented Sep 27, 2022

mdesmet commented Oct 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdesmet Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdesmet commented Oct 5, 2022

mdesmet Oct 4, 2022 •

edited

Loading