Add an |includeReportOnlyPolicies| boolean argument to Does sink type require trusted types? #518
Conversation
spec/index.bs
Outdated
| 1. For each |policy| in |global|'s <a>CSP list</a>: | ||
| 1. If |policy|'s <a>directive set</a> does not contain a <a>directive</a> | ||
| whose [=directive/name=] is `"require-trusted-types-for"`, skip to the next |policy|. | ||
| 1. Let |directive| be the |policy|'s |directive set|'s [=directive=] whose name | ||
| is `"require-trusted-types-for"` | ||
| 1. If |directive|'s [=directive/value=] does not contain a <a>trusted-types-sink-group</a> which is a match | ||
| for |sinkGroup|, skip to the next |policy|. | ||
| 1. Set |result| to `true`. | ||
| 1. Let |enforced| be true if |policy|'s [=policy/disposition=] is `"enforce"`, and false otherwise. | ||
| 1. Set |result| to true if |enforced| is true, and set to |includeReportOnly| otherwise. |
There was a problem hiding this comment.
If I read this correctly, the name includeReportOnly seems wrong, since the "enforce" policies are always forcing the result to true ; while the "report" policies only forces the result to true if includeReportOnly is false.
Also the "set to |includeReportOnly|" wording sounds a bit ambiguous to me. At least as a non-native speaker I initally misunderstood this meant setting the result variable to the value of includeReportOnly, which does not make sense.
So I guess what you want is rename the parameter includeEnforceOnly and rewrite the line to something like:
- If
includeEnforceOnlyis false or |policy|'s [=policy/disposition=] is"enforce"then set |result| totrue.
The boolean values of the caller would still need to be changed to includeEnforceOnly=false if we want to keep including all the policies.
Finally, even before your change it seems the result can only be set to true and never goes back to false ; so probably you can either just exit the "for each" loop immediately when it's set to true. Alternatively, remove the "result" variable and use "return true" or "return false" statements.
There was a problem hiding this comment.
So this method right now returns true if there's a require-trusted-types-for CSP header. Regardless of whether it's report only or not. Because currently it's used to go down the default policy codepath etc.
However, with the new CSP script-src keyword, we don't want that. We want this to only return true if it is enforced.
So this is updated with a new |includeReportOnly| boolean, which is set to true in the current usage, and set to false in the CSP PR.
So this then returns true if the policy is enforced, and additionally returns true if the policy is report only and |includeReportOnly| is true.
I'll reword it a bit to hopefully make it clearer, and add the early return when the result is true.
There was a problem hiding this comment.
I've realised the ambiguity regarding the variable name too. Good point, I've changed it to includeReportOnlyPolicies to hopefully make it clearer what is meant.
lukewarlow
force-pushed
the
does-sink-type-require-include-report-only
branch
from
2adae0d to
7359179
Compare
… require trusted types?
lukewarlow
force-pushed
the
does-sink-type-require-include-report-only
branch
from
7359179 to
80a006f
Compare
lukewarlow
changed the title
This is needed by w3c/webappsec-csp#665
Preview | Diff