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

separating security messages #278

Open
jsmith173 opened this issue Mar 26, 2024 · 10 comments
Open

separating security messages #278

jsmith173 opened this issue Mar 26, 2024 · 10 comments

Comments

@jsmith173
Copy link

Is it possible to seperate security messages?
It seems RestrictedPython uses existing exception types to report a security error.

I have found in compile.py

if result.errors:
raise SyntaxError(result.errors)`

When I replace SyntaxError to my new exception type it will solve the problem? Or how could I do that?

@icemac
Copy link
Member

icemac commented Mar 28, 2024

We could create a custom subclass of the existing exception classes, because a SyntaxError should stay one to prevent problems with existing code expecting a Python SyntaxError.

@icemac
Copy link
Member

icemac commented Mar 28, 2024

A PR is welcome.

@d-maurer
Copy link
Contributor

d-maurer commented Mar 28, 2024 via email

@jsmith173
Copy link
Author

When I handle 'SyntaxError' in the app how could I check, it is a real syntax error or it is a security exception? Also sometimes security error messages come from 'exec'

@d-maurer
Copy link
Contributor

d-maurer commented Mar 28, 2024 via email

@jsmith173
Copy link
Author

compile_restricted collects the real syntax errors so even if I have a new exception I will have real syntax errors in that.

@d-maurer
Copy link
Contributor

d-maurer commented Mar 28, 2024 via email

@ScrambledRK
Copy link

ScrambledRK commented Jun 10, 2024

Thank you d-maurer for the insight and ideas.

Quite clever, but they all smell like a workaround to me.
Maybe I'm just too stiff and conservative.

I don't think its a good idea to rebind or rewrite parts of a library that is so clearly related to security. I do not want to open any more opportunities for mistakes and risks than I already do using the library in the first place. Relying on the number of elements args contains seems bold to me. I rather not rely on something that only "appears" to be so. Dunno, I rather not differentiate then at all. At the cost of worse user feedback.

In my case I'd like to ...
a) inform the user that what they are trying to do is usually okay, but not in my environment
b) use it in tests to exclude accidental test passes due to actual syntax errors in the test code

Since I am just prototyping and experimenting for now, my need for this feature is very low.
But the "accepting PR" is noted x)

@loechel
Copy link
Member

loechel commented Jun 10, 2024

@ScrambleRK and @d-maurer when I see it correctly the SyntaxError in https://github.com/zopefoundation/RestrictedPython/blob/master/src/RestrictedPython/compile.py#L211 is the only one that is raised by RestrictedPython itself. As it just lists all collected errors I wonder if SyntaxError is the correct Error or Exception for this use case.

As RestrictedPython is a restricted sub set of Python, everything that raises a real SyntaxError is really a Syntax Error found by the AST Module or the internal interpreter. All Restrictions should be collected in another way and should be announced in a different way.

If we change this one line of code and introduce a new Exception / Error Class that would be a breaking change and needs a new major version.
@icemac any comments?

@icemac
Copy link
Member

icemac commented Jun 11, 2024

@loechel Changing the SyntaxError we raise to a subclass of SyntaxError would make the problem for the consumers of RestictedPython a bit less problematic as at least the isinstance tests still work. But I'd still consider this a major change.

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

No branches or pull requests

5 participants