-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Escape url parameters in sqlalchemy connection strings #235
Conversation
b934f2d
to
3ac3aab
Compare
@@ -19,17 +20,29 @@ def setup(self): | |||
"url, expected_args, expected_kwargs", | |||
[ | |||
( | |||
make_url("trino://user@localhost"), | |||
make_url(trino_url( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth to keep existing tests and create new ones with URL method next to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can simply assert that the string being generated is equal to the above string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
trino/sqlalchemy/util.py
Outdated
key: Optional[str] = None, | ||
) -> str: | ||
""" | ||
Composes a SQLAlchemy connect string from the given database connection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some parameters are missing like verify
or redirect_handler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OAuth is not yet supported in the url. I think we can leave that for another PR. I have added verify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, just please create a follow-up issue for it after merging, Kerberos doesn't seem supported as well, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct I supported only the fields that are currently parsed in the sqlalchemy connection string (and added verify
).
3ac3aab
to
ff83437
Compare
port=8080, | ||
catalog="system", | ||
user="user@test.org/my_role", | ||
auth=BasicAuthentication("user@test.org/my_role", "pass /*&"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add test for other supported auth methods: JWT and Certificate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added both tests.
from sqlalchemy.engine.url import _rfc_1738_quote # noqa | ||
|
||
|
||
def _url( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, just please create a follow-up issue for it after merging, Kerberos doesn't seem supported as well, right?
c81c1db
to
54d8603
Compare
@hovaesco: PTAL |
54d8603
to
90878ef
Compare
@hashhar : PTAL |
trino/sqlalchemy/dialect.py
Outdated
|
||
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"])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
trino/sqlalchemy/util.py
Outdated
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))}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using repr
for serialization seems like a shortcut.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json.dumps
converts tuples into array. that's why we can't use it there. I have added a comment to explain the reasoning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
90878ef
to
8f48c28
Compare
8f48c28
to
9f2ac1b
Compare
@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.
|
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.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: