Securitypolicyviolationevent sidebar #35384
Conversation
hamishwillee
requested review from
wbamberg and
bsmth
and removed request for
a team
github-actions
bot
added
Content:WebAPI
| @@ -64,6 +62,15 @@ The Reporting API spec also defines a Generate Test Report [WebDriver](/en-US/do | |||
| - {{domxref("ReportingObserver")}} | |||
| - : An object that can be used to collect and access reports as they are generated. | |||
|
|
|||
| ### Related interfaces | |||
There was a problem hiding this comment.
Note "related". These are not explicitly extension to this API. Separated in this doc so that I can justifiably include the event.
| @@ -34,4 +32,4 @@ document.addEventListener("securitypolicyviolation", (e) => { | |||
|
|
|||
| ## See also | |||
|
|
|||
| - [Content Security Policy (CSP)](/en-US/docs/Web/HTTP/CSP) | |||
There was a problem hiding this comment.
Note, I moved this string up into the first descriptive sentence. If you don't have context you need to know what a policy is straight away.
|
|
||
| > [!NOTE] | ||
| > It is recommended to add the handler for this event to a top level object (i.e. {{domxref("Window")}} or {{domxref("Document")}}). While the property exists in HTML elements, you can't assign a handler to the property until the elements have been loaded, by which time this event will already have fired. | ||
| > You should add the handler for this event to a top level object (i.e. {{domxref("Window")}} or {{domxref("Document")}}). | ||
| > While the property exists in HTML elements, you can't assign a handler to the property until the elements have been loaded, by which time this event will already have fired. |
There was a problem hiding this comment.
I'm not sure this is true for dynamically constructed elements, such as document.createElement("img"). Not what this PR changed though.
There was a problem hiding this comment.
So this is saying if your static HTML element would trigger a violation, perhaps because it loads a restricted img, by the time you get to add your handler the event will have already fired? Makes sense, though I guess if you declare the handler inline that might be assigned and ready before the violation triggers. That could be browser dependent.
You're saying that you can add a dynamically constructed element with a handler, but the the policy violation isn't triggered until here is a global context - when you add it to the document. So that should work.
Would this fix it "enough"? I don't want to mess with this too much.
| > While the property exists in HTML elements, you can't assign a handler to the property until the elements have been loaded, by which time this event will already have fired. | |
| > This is because you can't define a handler for elements in your HTML file until they are fully loaded, by which time the event will already have fired. |
There was a problem hiding this comment.
Worth pointing out that the example doesn't demonstrate listening for this event on elements; maybe should be a follow-up fix?
There was a problem hiding this comment.
Fixed by 9d74140 , though perhaps I should omit the statement about dynamic construction?
| @@ -34,4 +32,4 @@ document.addEventListener("securitypolicyviolation", (e) => { | |||
|
|
|||
| ## See also | |||
|
|
|||
| - [Content Security Policy (CSP)](/en-US/docs/Web/HTTP/CSP) | |||
| - [`CSPViolationReportBody.lineNumber`](/en-US/docs/Web/API/CSPViolationReportBody#cspviolationreportbody.linenumber) | |||
There was a problem hiding this comment.
Suggestion: link to the property page, even if it doesn't exist, so we don't need to update it in the future. (Same below)
There was a problem hiding this comment.
Thanks, but I don't think so. A link that works now is worth far more than one in future that might.
There was a problem hiding this comment.
Sure, I've added a check to https://jc-verse.github.io/mdn-graph/warnings (Replace DT link with real target) so this is in our backlog.
There was a problem hiding this comment.
That would be an excellent improvement. I wonder if the linter could do this automagically.
|
FWIW—I have very little knowledge of this, but from what I know, my worry for this move is that reporting API spec itself is WIP and our current documentation doesn't even reflect the latest spec? #29292 So maybe this could give off the wrong signal on the availability of CSP APIs? |
AFAIK this is no longer WIP. The "legacy version 0" was Chrome only, as is the Report-To header. However the new spec is being adopted by FF as well - I picked this up because I am documenting this stuff in #35279 Upshot, I'm not concerned about this. I am more concerned about whether this is discoverable and appropriate. My reasoning for putting it here is that there is prior-art, in that CSPViolationReportBody is part of the CSP "API" too, and that is here. With proper cross linking, which I have done, I think this is "better" than what exists now. The only better alternative would require (long requested) infrastructure fixes to allow the sidebar to properly cross link items across different parts of the web platform to make them related. If that happens then I'd consider having a CSP API doc. I'm not 100% certain though, because I like |
|
Okay, if you think |
I think it is where it "most belongs" given the limitations of the infrastructure. Can you approve this? |
|
Thank you! |
SecurityPolicyViolationEventdocs have HTTP sidebar, even though its a Web APIEvent. However it is defined in the HTTP CSP spec rather than an API.I wanted to have it in its own API group, but given there is no way to show related APIs properly, decided against.
In the end I have made it as part of the Reporting_API.
The reporting API already includes the only other API entity in the CSP spec - CSPViolationReportBody. Even though that fits better here than the event does, it is not a completely unexpected fit - the content is related, and is already documented as being associated with CSP.
It isn't perfect, but IMO reasonable compromise.
FYI @chrisdavidmills @wbamberg
Fixes #35320
Part of work around #35279