Move semicolon/newline handling to validation and raise errors #422
Conversation
…oleans and loosely defined strints
| ensure_array_of_strings!(directive, sandbox_token_expression) | ||
| valid = sandbox_token_expression.compact.all? do |v| | ||
| v.is_a?(String) && v.start_with?("allow-") | ||
| v.is_a?(String) && v.start_with?("allow-") && v !~ ESCAPE_SEQUENCE |
There was a problem hiding this comment.
It would probably be cleaner to move this check to ensure_array_of_strings.
But the belt-and-suspenders approach of also raising an error on output is also probably a good idea.
…rs to check if one snuck by
|
@oreoshake RE #418: we were notified that adding characters like |
|
Ugh, ya. That probably deserves another advisory. |
|
If someone else is willing to push this to completion, I’d be happy to review and release. I don’t see myself getting to this anytime soon. |
|
@oreoshake Happy to get this over the finish line. I'll pick up another issue first to go through the codebase and then I'll come back to this one (until someone beats me to it). |
|
The validation is all over the place, and adding more will work but I’ve been wondering if a real programmer could use the actual grammar from the spec or something close enough. |
|
@oreoshake I'll start working on this now. Will read the spec (send positive thoughts!) and see how we can improve the validation. I'll look through your code first, but I might publish a separate branch to explore alternative approaches. |
A followup to #418, instead of safely escaping bad values reject them at time of use. This applies to static configurations at boot time and dynamic configurations as well. All policies flow through the validation.
In a breaking release, we can start raising errors when encountering these misconfigurations and/or malicious attempts.
In adding additional validation, I tightened up the assertions a bit so we don't have tests passing when they shouldn't.