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 safelisted cors header subfeature #9456

Merged
merged 4 commits into from
Mar 19, 2021

Conversation

hamishwillee
Copy link
Collaborator

Fixes #9300

@ddbeck As we discussed in #9300 this is update adds subfeature for "CORS response header safelist" for two response headers as a test (on completion this will cover all the headers

The Content-Length update is the interesting one, which adds the browser versions at which the header joined the list. The linked item covers the versioning discussion except for Safari. I have marked that as false because it is still in the technology preview: https://developer.apple.com/safari/technology-preview/release-notes/

The Cache-Control is what all the other ones will look like. Essentially it just copies the version info from the parent - my assumption being that the header and the safelist came at the same time. This is reasonable, because most of these headers just say true for version anyway.

If you're OK with the approach/text I will roll out to the other headers in the list.

Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Apart from a couple very minor things in line comments, I think this looks reasonable in terms of data and structure. But since there's something novel in the structure of this…

@Elchi3 would you mind giving this a glance? It's not a huge thing, but this is going to lead to several new features with a common naming scheme and I'd like another owner perspective.

http/headers/cache-control.json Outdated Show resolved Hide resolved
http/headers/content-length.json Outdated Show resolved Hide resolved
@Elchi3
Copy link
Member

Elchi3 commented Mar 15, 2021

LGTM! I'm not sure it is needed to add this to all headers that had this from the beginning, though.
How does this enhance the Cache-Control compat table? The docs already mention whether or not Cache-Control is a CORS safe listed header. Why would the compat table need to repeat that and just show the same basic support data?

@hamishwillee
Copy link
Collaborator Author

@Elchi3

LGTM! I'm not sure it is needed to add this to all headers that had this from the beginning, though.

Then how would you know that a header was compatible or not from the data? i.e. if Cache-Control does not have the feature that might indicate that it isn't compatible at all.

How does this enhance the Cache-Control compat table? The docs already mention whether or not Cache-Control is a CORS safe listed header. Why would the compat table need to repeat that and just show the same basic support data?

It doesn't. Where this helps is for Content-Length and any other future header that gets added to the list. If this goes in though, you could in theory decide to autogenerate the info in the docs from the compatibility table.

It is completely reasonable to argue that this feature is only affecting one header and we might as well just have a note in the docs. But for me this seems like the purpose of the compatibility table. Up to you guys.

hamishwillee and others added 2 commits March 16, 2021 11:48
Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
@Elchi3
Copy link
Member

Elchi3 commented Mar 16, 2021

Then how would you know that a header was compatible or not from the data? i.e. if Cache-Control does not have the feature that might indicate that it isn't compatible at all.

In my view, the compat table communicates what doesn't work. It is not there to communicate everything that works.
The Cache-Control compat table doesn't even list all directives that work either. These are implicitly included in the basic support for Cache-Control (first row in the table). My assumption is that the reader can assume that things work if they are not listed in the table.

It doesn't. Where this helps is for Content-Length and any other future header that gets added to the list.

Right, when support is added after a feature initially ships, there is a usually a sub feature in BCD: So, safe listed CORS header makes a lot of sense for Content-Length. Completely agree on this case.

@ddbeck
Copy link
Collaborator

ddbeck commented Mar 16, 2021

Thanks for the discussion here, @hamishwillee and @Elchi3. This has been clarifying, I think. After reading it, I think this PR can be reduced to the cors_response_safelist feature in Content-Length and then we don't need to add any additional features (until a new safelisted header comes along). Does that make sense for everyone?

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Mar 19, 2021

I think this PR can be reduced to the cors_response_safelist feature in Content-Length and then we don't need to add any additional features (until a new safelisted header comes along). Does that make sense for everyone?

This will work - anyone coming to Content-Length will recognise the behaviour of this header has some non standard behaviour.
I get confused because I expect to be able to go to any doc and see that a feature is supported or not.

Anyway, thanks guys. Changed as suggested.

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Thanks @hamishwillee 👍

@Elchi3 Elchi3 merged commit 37abb38 into mdn:master Mar 19, 2021
@hamishwillee hamishwillee deleted the http_cors_safelist branch March 20, 2021 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:http 🚠 Compat data for HTTP features. https://developer.mozilla.org/docs/Web/HTTP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP > Content Length : Added to safelisted response header in specific versions
3 participants