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

Un-page-ify and adapt text of CSP script-src[-*]? #5732

Merged
merged 1 commit into from
Jun 14, 2021

Conversation

teoli2003
Copy link
Contributor

Fix #5286

I removed the page macros in the three cases and replace it with an adapted text removing the mention of inline styles, irrelevant in these cases.

@teoli2003 teoli2003 requested a review from a team as a code owner June 6, 2021 14:17
@teoli2003 teoli2003 requested review from mirunacurtean and removed request for a team June 6, 2021 14:17
@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2021

Preview URLs

Flaws

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

  • broken_links:
    • Can't resolve /en-US/docs/Web/Guide/Events/Event_handlers

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

  • macros:
    • /en-US/docs/Web/API/Window/execScript does not exist
  • broken_links:
    • Can't resolve /en-US/docs/Web/Guide/Events/Event_handlers
    • No need for the pathname in anchor links if it's the same page
    • No need for the pathname in anchor links if it's the same page
    • No need for the pathname in anchor links if it's the same page

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

  • broken_links:
    • Can't resolve /en-US/docs/Web/Guide/Events/Event_handlers

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/script-src
Title: CSP: script-src
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

(this comment was updated 2021-06-07 02:50:05.006445)

@teoli2003
Copy link
Contributor Author

This will help a bit #3196

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

@teoli2003 Thanks so much for getting on board with this - the Page macro clean up is a bit of a pain :-(.

I recently used a similar approach here, duplicating the text for some common directives.

@wbamberg made the point that since all of the text I was duplicating was the same, if you read one, you've read them all. Further, people reading don't know if there is a subtle difference between the text in each case so they are forced to re-read in each case. Lastly, if we change one we have to change all. So he felt this would be better done by linking to the single definition.

While this is of course arguable, I think that it does make sense for these really large blocks of text, so perhaps instead link to
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/default-src ?

@sideshowbarker
Copy link
Member

Further, people reading don't know if there is a subtle difference between the text in each case so they are forced to re-read in each case

In some cases there actually are subtle differences between the text, intentionally. And for developers, it’s more helpful to have the specific relevant information they need in the article they’re already looking at — rather than being forced to go look at different page to get the information they need.

While this is of course arguable, I think that it does make sense for these really large blocks of text, so perhaps instead link to
developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/default-src ?

The problem with that is, default-src is very a different directive than script-src. So if we did that, we’d now be directing developers to go look for the info they need in page about a completely different directive.

There are issues such as #2460 that come when we try to have just a boilerplate description for — in that case, some particulars about host-src that are relevant when the directive is frame-src. In #3899, I attempted to address that issue by adding some content to the central description in the default-src page. And the review comment I got back for that is indicative of the kind of confusion that doing that makes us end up with.

So I’m not sure what the ideal solution is, but I think we have some evidence that trying to centralize the information on the default-src page is probably not the ideal solution.

And so, short of trying to find an ideal solution, the solution we actually have at hand is the approach taken in this PR — which as far as I can see actually will lead to less confusion for developers in practice, not more.

@hamishwillee
Copy link
Collaborator

hamishwillee commented Jun 7, 2021

... And for developers, it’s more helpful to have the specific relevant information they need in the article they’re already looking at — rather than being forced to go look at different page to get the information they need.

@sideshowbarker

That was my argument for duplication in #5240, and why I believe it is arguable. However I was convinced for that case because the docs really are exactly the same and always will be. The "difference" that exists is the default value, and someone cleverly pulled that out of the text.

Further, something to remember here is that these fixes are replacing usage of the {{page}} macro, so

  • while there could be subtle differences in the text, we know that there are not in this case, or at least there haven't been before. If we duplicate we are now telling people that there could be differences when there are not.
  • it doesn't get more boilerplate than a page macro :-)

I raised for this case based on the fact that script-src was previously doing an import of default-src. I hadn't considered that there might be different semantics for default_scr`. IF we think this is a serious issue, it might also be possible to duplicate in one script-src case and link from the others.

I don't want to be the one making this call - I really just wanted you to understand that I had reasons for this :-).

Perhaps @wbamberg has an opinion.

@sideshowbarker
Copy link
Member

sideshowbarker commented Jun 7, 2021

(Sorry, I pushed the wrong button somehow — I definitely didn’t intend to close this.)

@sideshowbarker
Copy link
Member

... And for developers, it’s more helpful to have the specific relevant information they need in the article they’re already looking at — rather than being forced to go look at different page to get the information they need.

That was my argument for duplication in #5240, and why I believe it is arguable.

Yeah, I agree with that argument.

However I was convinced for that case because the docs really are exactly the same and always will be.

There’s actually no reason in this case why they always will be. Depending on which directives they’re used with, there in fact seem to be different things that can be said — and should be said — about the values. It seems like that’s illustrated by the multiple issues we have about this.

The "difference" that exists is the default value, and someone cleverly pulled that out of the text.

Further, something to remember here is that these fixes are replacing usage of the {{page}} macro, so

  • while there could be subtle differences in the text, we know that there are not in this case, or at least there haven't been before.

The reason there haven’t been differences before is that having the information in one place has actually prevented us from introducing any differences. See #2460 and see #3899.

If we duplicate we are now telling people that there could be differences when there are not.

As outlined above, there do actually seem to be differences.

I don't want to be the one making this call

Me neither. I really don’t know what’s ideal here. I think no matter what we end up with, it’s gonna be less than ideal

I really just wanted you to understand that I had reasons for this :-).

Yeah, I did understood that :) There seem to be reasonable arguments both for and against the possible choices that could be made here.

@wbamberg
Copy link
Collaborator

wbamberg commented Jun 8, 2021

I think it's a matter of judgement and depends on the case. If the text is going to differ, even in subtle ways, we have to duplicate it. And in a case where it's just contingent that the things are the same, in a way that we could easily imagine them diverging, then we should duplicate. But if it seems to be referring to fundamentally the same thing, it's better IMO to describe the thing in one place and link to it, and this helps to communicate that this is one thing used in multiple places in the API - learn it once and use it everywhere.

@hamishwillee
Copy link
Collaborator

Well, I have no idea whether this case differs in some subtle way, or may eventually differ. My thinking is that the decision might be deferred - i.e. we create one instance for this, then link to it. Later if we find a difference we duplicate the text and change as needed for the differing case.

I'm not convinced that the page macro is not better for the cases where we know the text is the same.

@sideshowbarker
Copy link
Member

Well, I have no idea whether this case differs in some subtle way, or may eventually differ.

In this case, the content does have a very clear specific difference, which the issue description states:

adapted text removing the mention of inline styles, irrelevant in these cases

…which is exactly what the #5286 OP was asking for.

In other words, this PR successfully fixes an actual specific problem somebody found and is asking us to fix.

So it seems clear the #5286 OP at least would be be satisfied with this fix.

Of course it could be reasonably argued that a better fix would be to entirely remove all the contents of the Sources sections of these articles, and replace the contents of those sections with a link saying, “See the Sources section of the article on the default-src article”. But I don’t know whether the #5286 OP would consider that to be better.

In general, it seems like as readers, we prefer having as much specific, useful information at point of use in an article as we can get — rather than being required to go look at another article that has more-general information that’s not necessarily tuned to a specific context.

And I think if that general principle is applied here, it suggests that what we’d want as readers of the script-src-attr and script-src-elem articles is for them to each have a Sources section that‘s tuned specifically to the context of CSP sources being used with the script-src-attr and script-src-elem — which is exactly what this PR does.

And while I can imagine an argument being made that if we make this kind of change for the script-src-attr and script-src-elem articles, we should also make it for all the other articles for CSP directives that are also page()ing in the Sources section of the default-src article in the same way.

I’d agree that ideally and given unlimited time, that’s what we should do. But it seems worth mentioning that as far as the actual issue at hand, #5286, changing the rest of the articles would do no more than this PR as-is already does.

So I think another general principle that might apply here is that when we have a fix ready in hand that satisfactorily resolves a specific issue from someone in the community, it’s better to go ahead and land that fix and get that person’s problem resolved — and we can file another general issue ourselves for dealing with the other cases later.

And in fact that doesn’t even preclude us from later deciding that, yes, in fact the best fix is to replace the content of those Sources section with links to the default-src Sources section.

But since we don’t seem to yet have agreement about that actually being the best thing to do, in the meantime there’s really nothing preventing use from landing this PR and getting #5286 satisfactorily resolved — until such time as we might decide we do want to re-work it for all the articles.

@teoli2003
Copy link
Contributor Author

teoli2003 commented Jun 8, 2021

After some thoughts:

  • Yes, as pointed out above, this PR original intent was not to remove the {{page}} macro, but to introduce a subtle difference for these 3 pages (a difference that was noticed and reported).
  • I think it is worth having a different Sources section for each cases where it is different, even slightly.
  • I also think that from the point of view of the user, it is better to have the Sources info directly in the page, rather elsewhere, even more if in a more generic text.
  • I understand that having the same content in 3 pages (in this case) is not ideal, as if we want to modify it in the future, we have to remember to update it in three pages.

What about I keep this text in CSP/script-scr, but use the page macro in the two CSP/script-src-* pages where it is indentical?

@sideshowbarker
Copy link
Member

What about I keep this text in CSP/script-scr, but use the page macro in the two CSP/script-src-* pages where it is indentical?

That to me seems somewhat like the worst of both worlds…

@hamishwillee
Copy link
Collaborator

hamishwillee commented Jun 14, 2021

The only strong opinions I have are unhelpful for this case:

  • duplicating a lot of identical text feels pointless. I'd rather use a macro.
  • I prefer docs in place where I am using them than links to elsewhere. I appreciate that people might not want to read the same thing multiple times, but if you're looking at docs for a page, you don't really want to to be sent off elsewhere either.
  • I would quite like to know if a wall of text is the same as something I already know. Maybe a note for the cases where things are the same might help in some cases.
  • The fact that these all differ can be known from the issue, but is not obvious on quick scan of the source, and would be even less obvious reading each rendered section. So to find the subtle differences people will have to read this all.

Anyway, ultimately I suspect we're going to end up copying everything. People will ignore the subtleties that you just injected and use whatever pre-existing knowledge they have - only discovering it in an "ah-ha" moment when things break. Entropy will mean we end up with lots of unintentional near duplicates.

The fact is, some problems have no tidy solutions.

Finally, I think we all agree that we can't block this case on a philosophical debate. I'll merge. Thanks for your patience.

@hamishwillee hamishwillee merged commit 576ba26 into mdn:main Jun 14, 2021
@teoli2003
Copy link
Contributor Author

Anyway, ultimately I suspect we're going to end up copying everything. People will ignore the subtleties that you just injected and use whatever pre-existing knowledge they have - only discovering it in an "ah-ha" moment when things break. Entropy will mean we end up with lots of unintentional near duplicates.

Thanks @hamishwillee for the merge. I think this point is really well said: the solution may be to be able to put forward the distinction. I will keep thinking about it.

@hamishwillee
Copy link
Collaborator

Absolutely. Somehow highlighting distinctions would be great. I think challenging to do "generically" but we will get better.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with "CSP: script-src": (short summary here please)
4 participants