Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Al2Klimov
left a comment
Al2Klimov
left a comment
There was a problem hiding this comment.
Tested together with Icinga/icingaweb2#5477 (review).
This is not allowed in the specification but it allows for more organized configuration files.
- Only add ploicy if we need to - Use `/` instead of `|` as regex delimiter - Return array instead of Generator
policies can never have a space since policies itself are space delimited.
src/Common/Csp.php
Outdated
| } | ||
| $parts = explode(' ', $directive, 2); | ||
| if (count($parts) < 2) { | ||
| continue; |
There was a problem hiding this comment.
Consider throwing InvalidArgumentException in this method whenever you encounter invalid syntax. The already called add() already throws InvalidArgumentException. You decide.
There was a problem hiding this comment.
Yes. This allows us to write better exception messages.
src/Common/Csp.php
Outdated
| || ! str_starts_with($policy, "'") && str_ends_with($policy, "'") | ||
| ) { | ||
| throw new InvalidArgumentException( | ||
| "Quoted policy must be fully surrounded by single quotes. policy: $policy", |
There was a problem hiding this comment.
| "Quoted policy must be fully surrounded by single quotes. policy: $policy", | |
| "Quoted policy must be fully surrounded by single quotes. Policy: $policy", |
Same below.
src/Common/Csp.php
Outdated
| $parsedUrl = parse_url($url); | ||
|
|
||
| if (! isset($parsedUrl['host'])) { | ||
| throw new InvalidArgumentException("URL must specify a host. url: $url"); |
There was a problem hiding this comment.
If it's an invalid argument, why it's allowed above? ('none', *)
| $requestUri = ServerRequest::getUriFromGlobals(); | ||
| if ( | ||
| ($scheme === null || $requestUri->getScheme() === $scheme) | ||
| && $requestUri->getHost() === $parsedUrl['host'] |
There was a problem hiding this comment.
Are you aware that && has a higher precedence than ||?
|
|
||
| $scheme = $parsedUrl['scheme'] ?? null; | ||
| if (in_array("'self'", $policies)) { | ||
| $requestUri = ServerRequest::getUriFromGlobals(); |
There was a problem hiding this comment.
Please tell me evaluateUrl() isn't actually needed at all. 🙈
Because, due to reverse proxies in front of IW2, $_SERVER is so unreliable that https://github.com/Icinga/icinga-sso-web/pull/3 JS had to resolve the absolute URL on the client side...
There was a problem hiding this comment.
It was implemented to support checking for user generated things navigation items and dashlets.
The idea was to give the users feedback on iframes that do not load or during the creation of new entries, specifically when using custom CSP.
src/Common/Csp.php
Outdated
| return; | ||
| } | ||
|
|
||
| // scheme and scheme://* |
There was a problem hiding this comment.
| // scheme and scheme://* | |
| // scheme: and scheme://* |
src/Common/Csp.php
Outdated
| continue; | ||
| } | ||
|
|
||
| if ($scheme !== null && ($policy === $scheme . ':' || $policy === $scheme . '://')) { |
There was a problem hiding this comment.
| if ($scheme !== null && ($policy === $scheme . ':' || $policy === $scheme . '://')) { | |
| if ($scheme !== null && ($policy === $scheme . ':' || $policy === $scheme . '://*')) { |
(If I'm not mistaken.)
| return true; | ||
| } | ||
|
|
||
| // Note: https://*.example.com means https://example.com and https://sub.example.com |
There was a problem hiding this comment.
Then I'd explicitly check for the "."
There was a problem hiding this comment.
It is not possible for anything else to be there.
validatePolicy already prohibits this.
2 participants
Add a Csp class which represents the Content-Security-Policy header.
This new class can be used to manage the content security policy.
This class enforces
default-src 'self'as a baseline for a secure web application.The first nonce added to any directive is selected as THE nonce for the CSP.
Setting a nonce is not supported since a nonce could appear in any number of different directives.