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

Page macro: CSP header directives - duplicate source info #11185

Merged
merged 4 commits into from
Feb 1, 2022

Conversation

hamishwillee
Copy link
Collaborator

Part of #3196

The CSP headers all (more or less) included the directive information for sources using the page macro. This directly includes the source in each of the relevant pages.

Chose to duplicate rather than link to this material.

@hamishwillee hamishwillee requested a review from a team as a code owner December 14, 2021 01:19
@hamishwillee hamishwillee requested review from teoli2003 and removed request for a team December 14, 2021 01:19
@github-actions github-actions bot added the Content:HTTP HTTP docs label Dec 14, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 14, 2021

Preview URLs

Flaws

None! 🎉

External URLs

URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy
Title: Content-Security-Policy
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/worker-src
Title: CSP: worker-src
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/prefetch-src
Title: CSP: prefetch-src
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src-attr
Title: CSP: script-src-attr
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src-elem
Title: CSP: style-src-elem
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/media-src
Title: CSP: media-src
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/base-uri
Title: CSP: base-uri
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src-elem
Title: CSP: script-src-elem
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/font-src
Title: CSP: font-src
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src-attr
Title: CSP: style-src-attr
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/navigate-to
Title: CSP: navigate-to
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/manifest-src
Title: CSP: manifest-src
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/img-src
Title: CSP: img-src
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/Sources
Title: CSP source values
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/form-action
Title: CSP: form-action
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src
Title: CSP: script-src
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/connect-src
Title: CSP: connect-src
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/object-src
Title: CSP: object-src
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/default-src
Title: CSP: default-src
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/child-src
Title: CSP: child-src
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-src
Title: CSP: frame-src
on GitHub

No new external URLs


URL: /en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src
Title: CSP: style-src
on GitHub

No new external URLs

(this comment was updated 2022-02-01 04:11:40.664809)

@wbamberg
Copy link
Collaborator

wbamberg commented Jan 8, 2022

Even though I feel like this is my fault, and even though I still don't like the {{page}} macro for ad hoc inclusion of content, this PR makes me sad. This is duplication across quite a few pages of quite complex intricate content.

I think it would be better in a case like this to have a single page that documents <source> that we can link to from the other pages. An analogy is in the CSS docs where we have <color> and link to it from all the places that include a color in their definition, like background-color. I don't know if a thing like this would already have a home in the HTTP docs organization though.

But that's just my opinion really. I'd be happy to hear other views.

@hamishwillee
Copy link
Collaborator Author

@wbamberg Yes, it "hurt" to do this.

As per your original justifications, if the sources can ever be different then the duplication makes sense.

If the sources will always the same then you could duplicate in each place, or have a single source and link, or have a single source and include (using the macro).

Page inclusion is better for the reader because they don't have to jump around for the information

I think this could be a single page "CSP Sources" or "CSP Directive Sources". You might argue that having just one page helps readers understand that there really is only one set of possible sources so they only need to read it once. But that's just a convenient justification - my main concern is that this is quite detailed and will be hard to maintain in multiple places. So either link or include, but duplication is risky.

@wbamberg
Copy link
Collaborator

@wbamberg Yes, it "hurt" to do this.

As per your original justifications, if the sources can ever be different then the duplication makes sense.

But they are always the same here, aren't they?

If the sources will always the same then you could duplicate in each place, or have a single source and link, or have a single source and include (using the macro).

Page inclusion is better for the reader because they don't have to jump around for the information

I think this could be a single page "CSP Sources" or "CSP Directive Sources".

I think this would be better.

You might argue that having just one page helps readers understand that there really is only one set of possible sources so they only need to read it once. But that's just a convenient justification - my main concern is that this is quite detailed and will be hard to maintain in multiple places. So either link or include, but duplication is risky.

Yes, agreed.

I think that if we did want to keep some kind of page inclusion, it might be acceptable if we had a more controlled way to do it. For instance maybe we could have /fragments alongside /files, and let {{page}} only reference content under there. That at least makes it explicit that the fragments are fragments, so people editing them know not to break pages that include them. We might also want to restrict the things that they could do, such as not letting them include KS macros, so we could avoid the build dependency problem.

But I'm not sure it's worth it.

@hamishwillee
Copy link
Collaborator Author

I'm happy to try redoing this as a linked page, but not until I get back. So let's park this "for now".

Re other alternatives, I have used it via fragments before on other systems. This is similarly error prone - yes it tells you that more care needs to be taken, but you still need to do lots of checking, and the one time you don't is the one time it breaks.

What might help is if a macro inclusion automatically marked all other including instances as "touched" so that they would all appear in the version to be reviewed.

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Jan 31, 2022

@wbamberg Howdy! OK, this is my attempt at a single page with cross linking. It removes a lot of additional duplication (previously some things uses page macro inclusion and some just duplicated). I am pretty sure that anything that specified a source had exactly the same text, except for obvious errors (and base-uri, which has a caveat that has been retained).

The new main doc is https://pr11185.content.dev.mdn.mozit.cloud/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/Sources

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.

Hi Hamish! I think this is great. I had a few little comments.


{{page("Web/HTTP/Headers/Content-Security-Policy/default-src", "Sources")}}
Note however that some of the values don't make sense for `base-uri`, such as the keywords `'unsafe-inline'` and `'strict-dynamic'`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know it already said this, but as a reader I don't like docs that just say "some of... such as". It leave me thinking, which other ones are there? I don't think it should block this PR, but maybe a follow-up issue to list everything that does not apply?

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 This text comes from the original version. Yes I agree, but I don't know the exceptions, and it is unlikely to be high priority. I'll add an issue once this merges so "someone" can find out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cheers. Thanks for the help. Tracking issue in #12574

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.

Nice, thank you Hamish!

@wbamberg wbamberg merged commit 7cbcf01 into mdn:main Feb 1, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Content:HTTP HTTP docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants