w3c / webappsec-csp

WebAppSec Content Security Policy
https://w3c.github.io/webappsec-csp/
Other
207 stars 78 forks source link

Problem with SecurityPolicyViolationEvent constructor and optional init dict #631

Closed evilpie closed 7 months ago

evilpie commented 9 months ago

See https://github.com/whatwg/webidl/issues/1378 for a description of this issue.

TLDR: Having an optional eventInitDict parameter in the SecurityPolicyViolationEvent constructor doesn't really make sense, considering that dictonary SecurityPolicyViolationEventInit has required properties.

After adjusting Gecko to follow the spec to the letter, new SecurityPolicyViolationEvent("securitypolicyviolation") fails, which currently works on all browsers and is tested by https://wpt.fyi/results/content-security-policy/securitypolicyviolation/idlharness.window.html.

This worked in Gecko before, because we didn't make any of the properties required and seemingly in Chrome because it has a second non-standard constructor with just one argument.

Either all browsers should align with spec or the spec needs to be updated to match.

mikewest commented 9 months ago

I don't have much of an opinion here, and I expect the constructor's usage is low enough to allow ~any approach. What outcome would you prefer?

annevk commented 9 months ago

I'd suggest we drop required. Throwing less exceptions when it doesn't matter is pretty much always better. And it seems implementations already have a way of dealing with it when it's not supplied which we could reuse in the specification.

mikewest commented 9 months ago

I'm fine with dropping it. I think that means we should simply define default values for those required items. Chromium ends up with empty strings for the USVString and DOMString items, enforce for the disposition, and 0 for the unsigned short. Is that acceptable to y'all?

annevk commented 9 months ago

Seems fine to me.

annevk commented 9 months ago

And createEvent() does not work for this class so you can indeed rely on defaulting I think.

mikewest commented 9 months ago

@SaeidEid wanted to grab this as well, and it should be a very straightforward change to the spec and test.

evilpie commented 9 months ago

I'm fine with dropping it. I think that means we should simply define default values for those required items. Chromium ends up with empty strings for the USVString and DOMString items, enforce for the disposition, and 0 for the unsigned short. Is that acceptable to y'all?

Sounds good to me. This largely matches Firefox's WebIDL. We do use report instead of enforce, but I am not concerned about that.

SaeidEid commented 7 months ago

I create an issue for this in chromium: https://issues.chromium.org/issues/325291983

antosart commented 7 months ago

Just wanted to notice that Gecko seems to set report as default value for the disposition, see https://wpt.fyi/results/content-security-policy/securitypolicyviolation/constructor-required-fields.html?diff&filter=ADC&run_id=5069213999300608&run_id=5069891194847232.