Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reporting API - tidy up CSPViolationReportBody #35602

Merged
merged 25 commits into from
Sep 21, 2024

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Aug 27, 2024

This adds missing property docs for CSPViolationReportBody, fixes up cross links to here from SecurityPolicyViolationEvent, and fixes the compat statements on the REporting API top page.

It should all be pretty correct, albeit a bit repetative, because all the examples are the same, and the properties in the event and body are also almost identical.

Related docs work can be tracked in #35279

Fixes #29292

@hamishwillee hamishwillee requested a review from a team as a code owner August 27, 2024 04:39
@hamishwillee hamishwillee requested review from wbamberg and removed request for a team August 27, 2024 04:39
@github-actions github-actions bot added Content:WebAPI Web API docs size/l [PR only] 501-1000 LoC changed labels Aug 27, 2024
Copy link
Contributor

github-actions bot commented Aug 27, 2024

Copy link
Contributor

github-actions bot commented Sep 2, 2024

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main comment here is to add more useful and concrete examples, showing how we can use this reported information to figure out what went wrong.

```js
const observer = new ReportingObserver(
(reports, observer) => {
console.log(`Disposition: ${reports[0].body.disposition}`); // For example: "enforce"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better not to have lines >80 characters or they get hard-wrapped on MDN.

Copy link
Collaborator Author

@hamishwillee hamishwillee Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. FWIW wrap seems to be about 96 chars on desktop,.

Edit: pushed comment to following line.

files/en-us/web/api/cspviolationreportbody/sample/index.md Outdated Show resolved Hide resolved
Comment on lines 11 to 15
The **`toJSON()`** method of the {{domxref("CSPViolationReportBody")}} interface is a _serializer_, which returns a JSON representation of the `CSPViolationReportBody` object.

This is used by the reporting API when creating a serialized version of a violation report to send to a reporting endpoint.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth saying somewhere that AFAIK the most common usage for toJSON is to support JSON.stringify()? Maybe also worth having an example that uses JSON.stringify()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wbamberg I was going to say no, since you never call this directly. But then you rarely call toJSON directly and that doesn't stop us documenting what it is for. I've modified.

I do have a "confirmation". The docs refer to toJSON as a "serializer", and there is a strong implication in many docs that it returns a string - which is a serialized version of the object. If you ask the popular APIs they go further and say that the JSON object returned "is a string, but you could return something else if you wanted".

My understanding is that this is a little bit wrong. The toJSON() method does not actually (necessarily) do any serialization and it probably won't return a string (though it can).
What it does is return a "JSON-compatible" representation of the object - one that doesn't have anything that can't be converted into a serialized string of JSON data, such as functions, or data types unsupported by JSON.

So it would be more accurate to refer to returned objects as a "JSON-compatible representation of the object that can be serialized using JSON.stringify()" than as a "serializer"?

I'm not sure it matters too much for this case, but it would be good to be clear generally so that the AIs don't confuse people that this must be a string coming out of toJSON()

files/en-us/web/api/reporting_api/index.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added size/xl [PR only] >1000 LoC changed and removed size/l [PR only] 501-1000 LoC changed labels Sep 16, 2024
@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Sep 16, 2024

@wbamberg This is ready for another look. I'm here for (my) Friday and then away for a week, so if you don't get to it by then, there is no urgency.

I have added examples as you suggested. They are a little boring and I wish I could make them live examples :-(. But they have been useful as some of the previous explanation was wrong.

I was lazy and had copied some of it from https://developer.mozilla.org/en-US/docs/Web/API/SecurityPolicyViolationEvent so that will need to be updated. But that will have to be a post process.

I was a bit rushed in the end and have not self-subedited. It should be mostly OK. If you want to wait, I'd still appreciate a scan to check that the example approach makes sense. I followed the live example structure just for convenience, and you might hate it.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all your work on this! I had some pretty minor comments. The examples are really helpful IMO - it's hard to explain what e.g. "caused a violation" but the examples make it very clear.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thank you Hamish!

@wbamberg wbamberg merged commit 51b1250 into mdn:main Sep 21, 2024
8 checks passed
@hamishwillee hamishwillee deleted the cspviolation_tidy branch September 29, 2024 23:10
@hamishwillee
Copy link
Collaborator Author

Thanks for all your patience with reviews!

fiji-flo pushed a commit that referenced this pull request Oct 2, 2024
* CSPViolationReportBody docs

* SecurityPolicyViolationEvent - fix crosslinks to CSPViolationReportBody

* Minor fixes to Report API top level

* Code review typo fixes

Co-authored-by: wbamberg <[email protected]>

* Fix code line lengths

* Apply suggestions from code review

* sample - aggregate the conditions.

* use: that violated the [Content Security Policy (CSP)](/en-US/docs/Web/HTTP/CSP).

* sourceFile example

* Make file, line, column the same(ish)

* OriginalPolicy/sourceFile example

* toJSON - add JSON.stringify()

* Add examples

* Add example for disposition

* Apply suggestions from code review - the easy ones

Co-authored-by: wbamberg <[email protected]>

* Apply suggestions from code review

Co-authored-by: wbamberg <[email protected]>

* Apply suggestions from code review

* blockedURL fix

* Apply suggestions from code review - folder from inline to test

* Fix up line numbers etc

* Update files/en-us/web/api/cspviolationreportbody/columnnumber/index.md

* Update files/en-us/web/api/cspviolationreportbody/linenumber/index.md

* Update files/en-us/web/api/cspviolationreportbody/sourcefile/index.md

* Update files/en-us/web/api/cspviolationreportbody/linenumber/index.md

* Update files/en-us/web/api/cspviolationreportbody/columnnumber/index.md

---------

Co-authored-by: wbamberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/xl [PR only] >1000 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reporting API page describes v0 API and should mention the v1 API
2 participants