add a better Description for PyPI #67
Conversation
| ... """ | ||
| >>> | ||
| >>> locals = {} | ||
| >>> byte_code = compile_restricted(source_code, '<inline>', 'exec') |
There was a problem hiding this comment.
The second and third parameter are optional resp. you use the default arguments, so I'd suggest to remove them here to ease the code.
There was a problem hiding this comment.
yes in RestrictedPython those params are optional, but in the transfer to the builtin compile, there those are required params, so for the base example I would like to pass them in.
README.rst
Outdated
| >>> | ||
| >>> locals = {} | ||
| >>> byte_code = compile_restricted(source_code, '<inline>', 'exec') | ||
| >>> exec(byte_code, safe_builtins, locals) |
There was a problem hiding this comment.
locals is not such a nice name here as it might be confused with the locals built-in.
Even the code renderer of GitHub does this: it renders locals in blue but other variables in black.
| ... os.listdir('/') | ||
| ... """ | ||
| >>> byte_code = compile_restricted(source_code, '<inline>', 'exec') | ||
| >>> # exec(byte_code, safe_builtins, {}) |
There was a problem hiding this comment.
What does happen here? I think Python complains that __import__ is not defined as it is not in safe_builtins.
According to Guards.py it is provided by AccessControl.
There was a problem hiding this comment.
Might it be a good idea to move some of the additional features used in Zope from AccessControl to RestrictedPython to make it easier adoptable, those import checks are one of those thing I would like to see.
There was a problem hiding this comment.
The import checks might be a bit too Zope specific as they involve SecurityInfo objects which are a subclass of Acquisition.Implicit.
setup.py
Outdated
| description='RestrictedPython provides a restricted execution ' | ||
| 'environment for Python, e.g. for running untrusted code.', | ||
| description='RestrictedPython is a defined subset of the Python language' | ||
| ' which allows to provide a program input into a trusted' |
There was a problem hiding this comment.
I think the common case is to put the spaces at the end of the previous line.
| @pytest.mark.parametrize(*e_exec) | ||
| def test_os_import(c_exec, e_exec): | ||
| """Test that import should not work out of the box. | ||
| TODO: Why does this work. |
There was a problem hiding this comment.
It works because RestrictedPython does not forbid import statements.
There was a problem hiding this comment.
see above, might be good to move that feature to RestrictedPython.
tests/test_imports.py
Outdated
| assert result.errors == () | ||
|
|
||
| # with pytest.raises(NameError): | ||
| # e_exec(OS_IMPORT_EXAMPLE) |
There was a problem hiding this comment.
This code does not raise a NameError because you execute against the default globals() which provide __import__. To run against save_builtins you have to provide them as second argument to e_exec.
| >>> source_code = """ | ||
| ... import os | ||
| ... | ||
| ... os.listdir('/') |
There was a problem hiding this comment.
I am not sure if this is the best example and what you want to show here: Code which RestrictedPython forbids? Code which should run through RestrictedPython as it would be harmful otherwise?
There was a problem hiding this comment.
I am also not sure about which negative example we should use, but I think we should show an example that will be restricted.
…ctedPython into better-description
|
@icemac most important part of this pull is the documentation changes, as they are shown on PyPI and that should reflect RestrictedPython as best as possible. Could you please merge this request, even if the test_imports needs more work, and cut a release before saturday (PyCon.WEB) so if anybody looks a RestrictedPython did not get a wrong understanding and thinks of a sandbox. Everything else we could discuss and solve on saturday after your talk or on Sunday. |
and make the test also a standalone pytest test