Skip to content
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

Sqlalchemy query #34

Closed

Conversation

therealryanbonham
Copy link
Contributor

Add SQL Query to X-Ray Metadata for SQLAlchemy and Flask-SQLAlchemy

@haotianw465
Copy link
Contributor

Thank you for the contribution and we will get our security team review it as soon as possible. At the meantime could you explain a little bit of why sql['sanitized_query'] = str(arg) is sufficient? It seems like this one doesn't remove any user data.

@therealryanbonham
Copy link
Contributor Author

I just updated the unit test to better/correctly show why this is valid.

The str(arg) only returns a paramaratized query. and will not contain data values.. so

session.query(User).filter(User.password=="mypassword!").first()

produces

SELECT users.id AS users_id, users.name AS users_name, users.fullname AS users_fullname, users.password AS users_password
FROM users
WHERE users.password = ?

@haotianw465
Copy link
Contributor

Thanks for the explanation.

@haotianw465
Copy link
Contributor

haotianw465 commented Mar 23, 2018

Hi, we are in the process of doing security review for this PR. I have one question regarding the SQL expression capture.

I read through http://docs.sqlalchemy.org/en/latest/orm/tutorial.html#querying but it doesn't mention SQLAlchemy intentionally separate the SQL expression and user specified parameters into two args under the hood.

The unit test covered the filter operation but there are multiple ones like text, join and delete etc. If you do str(arg) when those operations are captured could it contain any sensitive information?

Some extra unit test for other operations or official docs mentioning about how SQL expression is generated would be great.

@shouichi
Copy link
Contributor

statement = query.statement
print(statement.compile(someengine))

The above forms will render the SQL statement as it is passed to the Python DBAPI, which includes that bound parameters are not rendered inline. SQLAlchemy normally does not stringify bound parameters, as this is handled appropriately by the Python DBAPI

http://docs.sqlalchemy.org/en/latest/faq/sqlexpressions.html

According to the official doc, it seems like it's safe to str(arg). If you want to be extra care, I can take this over and add more test cases.

Thanks!

@haotianw465
Copy link
Contributor

haotianw465 commented Apr 19, 2018

@shouichi Thank you for point out the official SQLAlchemy doc. I believe this is enough and I will merge it in once all conflicts are resolved.

@haotianw465
Copy link
Contributor

Manually merged due to some conflicts in GA release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants