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

Remove the CORS flag #960

Merged
merged 2 commits into from Nov 11, 2019
Merged

Remove the CORS flag #960

merged 2 commits into from Nov 11, 2019

Conversation

annevk
Copy link
Member

@annevk annevk commented Nov 8, 2019

As far as I can tell we use "response tainting" for the same thing.

This also fixes an issue whereby we checked origin tainting rather than response tainting in main fetch which results in a minor bug when it comes to opaquing A -> B -> A chains.


Preview | Diff

As far as I can tell we use "response tainting" for the same thing.

This also fixes an issue whereby we checked origin tainting rather than response tainting in main fetch which results in a minor bug when it comes to opaquing A -> B -> A chains.
@annevk
Copy link
Member Author

annevk commented Nov 8, 2019

@JuniorHsu interested in reviewing as well?

<a>same origin</a> with <var>request</var>'s <a for=request>origin</a>, <var>request</var>'s
<a for=request>tainted origin flag</a> is unset, and the <i>CORS flag</i> is unset
<a>same origin</a> with <var>request</var>'s <a for=request>origin</a>, and <var>request</var>'s
<a for=request>response tainting</a> is "<code>basic</code>"
Copy link
Member

Choose a reason for hiding this comment

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

It's likely I'm missing something, but I don't exactly see how this condition changes, could you please explain:

This also fixes an issue whereby we checked origin tainting rather than response tainting in main fetch which results in a minor bug when it comes to opaquing A -> B -> A chains.

From what I can tell, assuming the current URL's origin is same-origin with request's origin, then:

  • Whenever tainted origin flag is unset, response tainting is always "basic" here, and
  • Whenever tainted origin flag is set, response tainting is not "basic"

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 think you're right. A -> B -> A does set the tainted origin flag. I guess that means this whole change is editorial. I still think it's for the better though.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, I think favoring response tainting is clearer. Thanks.

Copy link
Member

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

(But please wait for Yutaka)

@yutakahirano
Copy link
Member

The change looks good, but I have a few questions for the future direction.

At this moment we have two options for "bookkeeping".

  1. As a parameter for fetch algorithms (e.g., CORS preflight flag)
  2. As a field of the concept request (e.g., request's tainted origin flag)

Do we have a general preference to one over the other? What do you think about separating such bookkeeping fields from the concept request? To be honest I'm not very comfortable with these fields as I believe they should be part of loader (or fetcher), not the request.

fetch.bs Outdated Show resolved Hide resolved
@annevk
Copy link
Member Author

annevk commented Nov 11, 2019

@yutakahirano I think putting this state on some other "object" makes a lot of sense. I suspect this is still a reasonable incremental step towards that though. If you have a design in mind, could you describe that in a new issue? And if you think solving that blocks this PR, please say so.

Co-Authored-By: Michael[tm] Smith <mike@w3.org>
@yutakahirano
Copy link
Member

@yutakahirano I think putting this state on some other "object" makes a lot of sense. I suspect this is still a reasonable incremental step towards that though. If you have a design in mind, could you describe that in a new issue? And if you think solving that blocks this PR, please say so.

Thank you. I don't want to block this PR, and I agree that this is a good change.

@annevk annevk merged commit 65138f3 into master Nov 11, 2019
@annevk annevk deleted the annevk/cors-flag-removal branch November 11, 2019 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants