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

Add server-side parameter handling for embeds #1014

Merged
merged 1 commit into from
May 4, 2016

Conversation

whummer
Copy link
Contributor

@whummer whummer commented Apr 27, 2016

Hey @arikfr,

This is an updated pull request fulfilling #929, taking a slightly different approach than our previous PR (#995). Instead of retrieving the query template and filling in the parameter on the client side, this code receives the parameters and renders the query template on the server side (via pystache.render) and then returns the result.

It seems that this is a much simpler approach which integrates nicely with the existing embed.html. It is potentially also safer because the SQL query is not exposed to the client. SQL injection is still a potential threat, though, therefore we keep a warning message in the settings.py and disable this feature by default.

In general, it seems that other parts of the framework are also affected by potential SQL injection (e.g.,QueryResultListResource::post uses plain parameters). As part of a future work item, maybe we could use a proper SQL templating engine to prevent SQL injection altogether. However, for our current use cases this solution is sufficient (it is used mostly internally).

Please let us know what you think. Thanks

@arikfr
Copy link
Member

arikfr commented Apr 28, 2016

I don't want to introduce synchronous execution of queries with current implementation, as it will lead to some long running queries choking the frontend making the system unresponsive. I do want to introduce it in the future when we make the API server async (either by utilizing gevent or some other way).

Adding non standard query execution way makes this pull request less simpler than the other pull request. While #995 requires some more work (I will comment there soon on the results of my testing), it is the way to go, as it uses existing components.

QueryResultListResource::post allows plain parameters because it verifies you have permission to execute a query first. If you have a permission to run any query there is no risk of SQL injection (you can just run this query directly). Regardless we should consider moving to safe templating option, it just that we need to take into account that not all the data sources we support are SQL based...

if query_parameters:
query_text = pystache.render(query_text, parameter_values)

data, error = data_source.query_runner.run_query(query_text)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run_query might raise an exception, so better wrap it with a try/except.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. changed in the updated PR

@whummer
Copy link
Contributor Author

whummer commented May 3, 2016

@arikfr Thanks for the update on this PR. Please see my changes corresponding to your comments.

@arikfr arikfr merged commit 6d495d2 into getredash:master May 4, 2016
@arikfr
Copy link
Member

arikfr commented May 4, 2016

Merged. 🎉

dairyo pushed a commit to KiiCorp/redash that referenced this pull request Mar 1, 2019
Add server-side parameter handling for embeds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants