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

Add CORP headers to media repo #1197

Merged
merged 6 commits into from
Aug 9, 2022
Merged

Add CORP headers to media repo #1197

merged 6 commits into from
Aug 9, 2022

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Aug 2, 2022

@turt2live turt2live marked this pull request as ready for review August 2, 2022 23:31
@turt2live turt2live requested a review from a team as a code owner August 2, 2022 23:31
@@ -19,6 +19,12 @@ When serving content, the server SHOULD provide a
`Content-Security-Policy` header. The recommended policy is
`sandbox; default-src 'none'; script-src 'none'; plugin-types application/pdf; style-src 'unsafe-inline'; object-src 'self';`.

{{% added-in v="1.4" %}}

The server SHOULD additionally provide `Cross-Origin-Resource-Policy: cross-origin`
Copy link
Member

Choose a reason for hiding this comment

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

I thought we weren't doing RFC2119 keywords?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since when? We've always used SHOULD and MUST, just not MAY. It's in the API standards section...

Copy link
Member

Choose a reason for hiding this comment

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

🤷 fair enough. I thought at some point we'd decided not to use it as it's not familiar for most readers.

It's in the API standards section...

I don't see this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I swear it was there. Somewhere we describe that we use MUST and SHOULD (which have the same meaning as must and should), but not MAY and such.

If we don't have this written down somewhere, my whole life has been a lie 😭

Copy link
Member

Choose a reason for hiding this comment

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

I'd search github for "MUST", but that isn't going to go well :-S

Copy link
Member Author

Choose a reason for hiding this comment

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

even searching the repo for "MUST" (case sensitive) yields a ton of results, so I'm inclined to say that even though it's not written down, we follow the rule:

176 matches over 53 files (tall image)

image

content/client-server-api/modules/content_repo.md Outdated Show resolved Hide resolved
content/client-server-api/modules/content_repo.md Outdated Show resolved Hide resolved
Even though our content doesn't need 2 paragraphs, it's good to have the capability to render it in the future.
Comment on lines +270 to +274
// Make pairs of "added-in" and content inline to each other. We do pairs so authors can
// describe two paragraphs with added-in prefixes within a single box, reducing DOM
// complexity. Each paragraph is expected to be prefixed with an added-in, however.
//
// XXX: We assume the added-in and text will be rendered as paragraph elements.
Copy link
Member Author

Choose a reason for hiding this comment

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

as mentioned in the commit: we don't need the complexity of 2+ paragraphs yet, but it's good behaviour to have for when we need it in the future.

@turt2live turt2live requested a review from richvdh August 9, 2022 01:36
@turt2live turt2live merged commit afc0e6a into main Aug 9, 2022
@turt2live turt2live deleted the travis/corp-headers branch August 9, 2022 16:44
@richvdh
Copy link
Member

richvdh commented Aug 10, 2022

can/should we simplify this now that #1204 is (hopefully) fixed?

@turt2live
Copy link
Member Author

#1207

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants