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

Whitelisting more builtin primitives #1435

Merged
merged 1 commit into from
Nov 28, 2016

Conversation

mattrobenolt
Copy link
Contributor

This is also a slight refactor to make it easier to expand on this list
of builtins we allow.

Also, doesn't mutate the global safe_builtins dict every time from
RestrictedPython, instead, opts for copying the dict first.

@@ -38,6 +38,11 @@ def __call__(self):


class Python(BaseQueryRunner):
safe_builtins = (
'sorted', 'reversed', 'min', 'max',
'sum', 'set',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be explicit, sum and set are the additions here.

@mattrobenolt
Copy link
Contributor Author

It seems these test failures are unrelated since other PRs are failing for the same reason.

@arikfr
Copy link
Member

arikfr commented Nov 28, 2016

Yes, the failures are for a different reason. I'll sort it out. I assume you tested these changes locally?

@mattrobenolt
Copy link
Contributor Author

I assume you tested these changes locally?

You would be assuming incorrectly. :( I haven't set up re:dash locally to run against. I can do that later. We run it in a docker container ourselves for production.

@mattrobenolt
Copy link
Contributor Author

I'm building docker container locally and testing this out now. In theory, this should work just fine. :)

@arikfr
Copy link
Member

arikfr commented Nov 28, 2016

👍 it's quite easy to run the query runner alone, as it's just Python code.

This is also a slight refactor to make it easier to expand on this list
of builtins we allow.

Also, doesn't mutate the global `safe_builtins` dict every time from
`RestrictedPython`, instead, opts for copying the dict first.
@mattrobenolt
Copy link
Contributor Author

Alright, couldn't get docker container running, but just got it running through python enough to get into the CLI.

I did:

In [1]: from redash.query_runner.python import Python
In [2]: Python({}).run_query('print(sorted, reversed, min, max, sum, set)', None)
Out[2]:
('{"rows": [], "log": ["[2016-11-28T19:38:07.050052] (<built-in function sorted>, <type \'reversed\'>, <built-in function min>, <built-in function max>, <built-in function sum>, <type \'set\'>)"], "columns": []}',
 None)

as a test, so it appears that everything is patched in correctly.

fwiw, when building from master in docker, I get the following JavaScript error when trying to go to the /new/query page:

image

@arikfr arikfr merged commit 1961770 into getredash:master Nov 28, 2016
@arikfr
Copy link
Member

arikfr commented Nov 28, 2016

How do you "build" the frontend code in your Docker image?

Note that I wouldn't recommend using master for "production" usage at the moment. I merged earlier today big refactor of the frontend code, so there are some issues still.

@mattrobenolt
Copy link
Contributor Author

I just did docker build --rm -t redash . and ran this which should compile from whatever is in my local directory, so I guess it'd just be this: https://github.com/getredash/redash/blob/master/Dockerfile#L32

@mattrobenolt mattrobenolt deleted the safe-builtins branch November 28, 2016 21:16
@arikfr
Copy link
Member

arikfr commented Nov 28, 2016

Strange, that I would expect to work. I'll give it a look tomorrow.

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.

None yet

2 participants