-
Notifications
You must be signed in to change notification settings - Fork 27
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
Refactor SandboxDirective #258
base: main
Are you sure you want to change the base?
Refactor SandboxDirective #258
Conversation
I have also added following public methods: |
if (token.startsWith("'")) { | ||
errors.add(Policy.Severity.Error, "Unrecognized sandbox keyword " + token + " - note that sandbox keywords do not have \"'\"s", index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the error message is going to state double quote single quote shouldn't the condition also check double quote?
if (token.startsWith("'") || token.startsWith("\"")) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example of this error message would be:
Unrecognized sandbox keyword 'strict-dynamic' note that sandbox keywords do not have "'"s
I have left this part intact, but I think the reason of mentioning just single quote is the fact, that Content Security Policy allows them in some situations. For keywords in Source List must have single quotes, they are handled in SourceExpressionDirective.
I guess we could include "
in the condition and also change message to shouldn't be surrounded with quotes
. That would be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just trying to make it match ….have \"'\"s
which seems to indicate either single or double. But, maybe I’m not following the pairs/escaping properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I have added it to PR.
@NiedziolkaMichal because HtmlUnit needs this i'm currently thinking about a fork - do you like to support that? |
@rbri I am not invested in this project anymore. You can use my changes as you please. |
This is simple refactor to SandboxDirective class, shrinking its size and introducing enum "Value" containing all keywords. This change is useful to me, because I can unhardcore sandbox keywords from HTML Validator.