SecurityPolicyViolationEventInit data members naming doesn't seem to match browsers

w3c / webappsec-csp

WebAppSec Content Security Policy

https://w3c.github.io/webappsec-csp/

Other

209 stars 78 forks source link

SecurityPolicyViolationEventInit data members naming doesn't seem to match browsers #498

Open cdumez opened 3 years ago

cdumez commented 3 years ago

SecurityPolicyViolationEventInit data members naming doesn't seem to match browsers. In particular:

WebKit & Blink use documentURI & blockedURI while the specification uses documentURL & blockedURL
WebKit & Blink use lineNumber & columnNumber while the specification uses lineno & colno
WebKit & Blink have violatedDirective as a required data member but it is not present in the spec

The SecurityPolicyViolationEvent interface itself is fine because it has attributes with both names so it is backward compatible. However, the SecurityPolicyViolationEventInit seems to be an issue.

In my point of view (I work on WebKit), it seems risky compatibility-size for browser engines to change the naming of our SecurityPolicyViolationEventInit dictionary data members. It is also not really feature-detectable on JS side as far as I can tell.

cdumez commented 3 years ago

WebKit bug where we got closer to the specification but did not rename the dictionary members out of fear of breaking compatibility: https://bugs.webkit.org/show_bug.cgi?id=226215

darinadler commented 3 years ago

I think you also mentioned that WPT currently has tests that match the browsers, not the specification.

gsnedders commented 3 years ago

c.f. https://github.com/w3c/webappsec-csp/pull/353

cdumez commented 3 years ago

I think you also mentioned that WPT currently has tests that match the browsers, not the specification.

That is correct:

content-security-policy/securitypolicyviolation/constructor-required-fields.html Uses documentURI / blockedURI / lineNumber / columnNumber for dictionary members Also expects violatedDirective as a mandatory parameter
interfaces/CSP.idl matches the spec naming for dictionary members but those don't cause failures WPT test failures currently. In particular, content-security-policy/securitypolicyviolation/idlharness.window.html passes with old or new dictionary member naming.

cdumez commented 3 years ago

cc @mikewest @annevk

cdumez commented 3 years ago

I have just verified that Gecko is also still using the documentURI / blockedURI / lineNumber / columnNumber naming. So it seems that none of the 3 major browser engines match the specification, even though the specification changed the naming years ago.

mikewest commented 3 years ago

That's unfortunate, and likely my fault (at least for Blink).

At this point, I agree that shifting the spec to match browsers is the right thing to do. @antosart might have time to put up a PR?

antosart commented 3 years ago

I will prepare a PR.

cdumez commented 3 years ago

Does this mean also dropping the documentURL / blockedURL attributes on SecurityPolicyViolationEvent interface? It seems no browser has adopted these either. While they don't hurt compatibility, they don't bring much and won't match the properties on the dictionary anyway.

annevk commented 3 years ago

Yeah, we should revert all of #353 for now and then only reapply if there's more agreement on shipping (and writing test updates).