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

Same-origin data-URL flag only for fetch and XHR? #381

Closed
youennf opened this issue Sep 6, 2016 · 24 comments
Closed

Same-origin data-URL flag only for fetch and XHR? #381

youennf opened this issue Sep 6, 2016 · 24 comments

Comments

@youennf
Copy link
Collaborator

youennf commented Sep 6, 2016

AFAIK, this flag is only used by XHR and fetch.
It does not seem to be used by the HTML specification at all.
Is it on purpose or just a lack of synchronization?

Doing a quick test, crossorigin-attribute-enabled scripts seem to load fine with data URLs in Chrome and Firefox.

@annevk
Copy link
Member

annevk commented Sep 6, 2016

That's interesting. I would have thought <script crossorigin> to fail. @mikewest, where does Chrome consider data URLs same-origin?

@youennf
Copy link
Collaborator Author

youennf commented Sep 6, 2016

<script> window.onerror = function(message, source, lineno, colno, error) { console.log(message + "\nReceived error from \"" + source + "\". Error location is " + lineno + ":" + colno + "."); } var script = document.createElement("script"); script.crossOrigin = ""; script.src = "data:text/script, throw 'PASS: my error is rich!'"; document.body.appendChild(script); </script>

Firefox, Chrome and WebKit behave differently.

@youennf
Copy link
Collaborator Author

youennf commented Sep 6, 2016

<script>
window.onerror = function(message, source, lineno, colno, error)
{
    console.log(message +  "\nReceived error from \"" + source + "\". Error location is " + lineno + ":" + colno + ".");
}

var script = document.createElement("script");
script.crossOrigin = "";
script.src = "data:text/script,  throw 'PASS: my error is rich!'";
document.body.appendChild(script);
</script>

@mikewest
Copy link
Member

mikewest commented Sep 7, 2016

@mikewest, where does Chrome consider data URLs same-origin?

The snippet @youennf posted does not enrich the error (though it does load, probably because we're shortcutting through CORS for data: for exciting historical reasons). That is, we don't consider it same-origin, but we're not performing the same checks on it that we would for a network request.

More generally, I know that we allow extraction of pixels from data: from <canvas>, and I wouldn't be surprised if there were one or two other exceptions we've made over the years, but, ideally, Chrome doesn't consider data: URLs same-origin (At this point, I think Firefox is the only browser that does consider them same-origin consistently. Perhaps we should instead align the spec with Blink/WebKit/EdgeHTML? :) ).

@youennf
Copy link
Collaborator Author

youennf commented Sep 7, 2016

Firefox does not seem to fully treat data URLs scripts as same-origin: the error is only partially enriched. Or maybe there is a bug preventing to enrich the error.
WebKit does not authorize to load data URLs in cross origin mode for scripts at the moment, but it could be changed.

Another example is ShapeOutside CSS property which must be "potentially CORS-enabled fetch"ed in "anonymous" mode. In WebKit, we recently set the same-origin data-url flag for this one.

I don't know the historical reasons of the same-origin data URL flag.
It seems to me data URL resources and inlined resources have a lot in common.
Since the origin of a data-url resource is well defined, shouldn't we simplify the fetch spec by removing the same origin data url flag? Or making it set by default? That would allow "potentially CORS-enabled fetch" to better match what browsers are doing.

@annevk
Copy link
Member

annevk commented Sep 7, 2016

@mikewest the specification is largely aligned with Chrome already. That is why it has the flag to make data URLs same-origin in a limited number of contexts. The problem here is that Chrome is not consistent. E.g., not failing the load for <script crossorigin> is really weird behavior as the contract for opting into CORS is that either you get same-origin or failure. So it doesn't really help to hand-wave about the exceptions, we need to know them or Chrome needs to remove them.

@mikewest
Copy link
Member

mikewest commented Sep 7, 2016

the specification is largely aligned with Chrome already.

I was being flippant. By "the spec", I was talking more about HTML "origin of a document" than Fetch.

The problem here is that Chrome is not consistent.

Yup. I said "ideally" because I agree with you that Chrome's current behavior is weird in some places. I suspect that data: images in <canvas> is the only exception that's probably baked in for web compatibility purposes. I don't have much data to back that up, of course.

@tyoshino might know more about Chrome's CORS exceptions for data: in general, as he's been working in that code more recently.

@annevk
Copy link
Member

annevk commented Sep 7, 2016

I was talking more about HTML "origin of a document" than Fetch.

Is there an issue on that? I'm not sure what this is referring to.

@tyoshino
Copy link
Member

tyoshino commented Sep 7, 2016

I enabled data URL support for XHR in Chrome by a bit tricky way (changed WebURLLoaderImpl to virtually issue "Access-Control-Allow-Origin: *" to blink) 2 years ago (http://crbug.com/308768). It's not making data URLs handled as same-origin, but as cross-origin resource but with the CORS header to allow access.

<script> being allowed might be side-effect of the fix.

At the layer of checking same/cross-origin-ness, there could be some special treatment for data scheme, but I'm not so much familiar with.

@youennf
Copy link
Collaborator Author

youennf commented Sep 7, 2016

This solution might cause issues with credentials then, not particularly important but still.
Also, it should make the data URL resource CORS-SameOrigin.

If there is no good reason for preventing data URLs to be same-origin, let's make them same-origin.

@mikewest
Copy link
Member

mikewest commented Sep 7, 2016

@annevk: Filed whatwg/html#1753 for discussion.

@annevk
Copy link
Member

annevk commented Sep 7, 2016

@youennf Blink is opposed to making data URLs same-origin. https://bugzilla.mozilla.org/show_bug.cgi?id=255107 has some of their rationale. However, Blink does treat data URLs as same-origin in certain contexts (when not the result of a redirect): XMLHttpRequest, fetch(), and I guess <img> (not yet specified). Apparently the way that is implemented is suboptimal, but hopefully they can fix that.

I think my takeaway from this issue is that we should use this flag for <img>. Not sure if there's an issue on that already. @zcorpan?

@youennf
Copy link
Collaborator Author

youennf commented Sep 7, 2016

Thanks for the pointer, I'll check that. It would be good to set that flag for <img>. It was very recently done so in WebKit.

@zcorpan
Copy link
Member

zcorpan commented Sep 8, 2016

It was added in whatwg/html#499 ... but img-environment-changes also needs it. :-( (Yes, we should factor out the common bits.)

@youennf
Copy link
Collaborator Author

youennf commented Sep 8, 2016

Great, does it cover also https://drafts.csswg.org/css-shapes/#propdef-shape-outside?

@annevk
Copy link
Member

annevk commented Sep 8, 2016

@youennf no, it would not cover that necessarily. CSS doesn't define how it fetches resources, so it's unclear what code paths CSS would (re)use.

@youennf
Copy link
Collaborator Author

youennf commented Sep 8, 2016

This spec says:
User agents must use the potentially CORS-enabled fetch method defined by the [HTML5] specification for all URLs in a shape-outside value. When fetching, user agents must use "Anonymous" mode, set the referrer source to the stylesheet’s URL and set the origin to the URL of the containing document.

In WebKit, the SameOrigin data URL flag is also set.

@annevk
Copy link
Member

annevk commented Sep 8, 2016

@youennf I guess they need to update to integrate with Fetch then.

@bzbarsky
Copy link

Blink is opposed to making data URLs same-origin. https://bugzilla.mozilla.org/show_bug.cgi?id=255107 has some of their rationale.

All of that only applies to navigation and maybe worker loads, not any other fetches, afaict.

@annevk
Copy link
Member

annevk commented Sep 14, 2016

Yeah, given whatwg/html#1243 (comment) onwards perhaps we should remove this flag and just special case navigation and workers.

@youennf
Copy link
Collaborator Author

youennf commented Sep 14, 2016

Sounds like a good plan to me.

annevk added a commit that referenced this issue Sep 14, 2016
HTML gives data URLs a unique origin when navigating to them to prevent
a class of XSS attacks.

Since browsers already largely allow data URLs in all other contexts
this commit aligns with that, opting them into being same-origin
elsewhere.

Workers however are still prevented. It would create problems for
shared workers and potentially also for dedicated workers.

Fixes #381.
@annevk
Copy link
Member

annevk commented Sep 14, 2016

Created a PR, review appreciated.

annevk added a commit to whatwg/html that referenced this issue Sep 14, 2016
The change to Fetch discussed in
whatwg/fetch#381 made it obsolete.

Closes #1243, closes #1778, and closes #1779 as these are all treated
as same-origin now per the change to Fetch.
@annevk
Copy link
Member

annevk commented Sep 15, 2016

Note that in whatwg/html#1243 (comment) @bzbarsky proposes a plan different from what I expected. Namely to allow data URLs for (I assume just dedicated) workers, but make them create a worker in an opaque origin.

@annevk
Copy link
Member

annevk commented Sep 15, 2016

So I won't be landing this right now until the details of that alternative plan are sorted, which I guess will take a while given all the timezone issues.

annevk added a commit to whatwg/xhr that referenced this issue Sep 15, 2016
Fire the final progress event before switching the state to DONE in
“handle response-end-of-body”. And no longer fire events named progress
in “request error steps”.

Fixes #72.

(This commit also removes the same-origin data-URL flag that is about
to be removed in Fetch per whatwg/fetch#381.
That change has no impact on implementations.)
annevk added a commit to whatwg/xhr that referenced this issue Sep 15, 2016
Fire the final progress event before switching the state to DONE in
“handle response-end-of-body”. And no longer fire events named progress
in “request error steps”.

Fixes #72.

(This commit also removes the same-origin data-URL flag that is about
to be removed in Fetch per whatwg/fetch#381.
That change has no impact on implementations.)
annevk added a commit that referenced this issue Sep 15, 2016
By-and-large browsers treat data URLs as same-origin, though there 
are some inconsistencies. This change will treat all data URLs, 
regardless of origin, as same-origin from the perspective of Fetch.

HTML already assigns a unique opague origin to documents created from
a data URL and the plan of record is to do so for dedicated workers
too.

HTML will likely also forbid shared workers to be created from data 
URLs.

See whatwg/html#1782 for the proposed changes 
to HTML. (This has not landed yet, if that PR is tweaked further the 
note added here might need some tweaks.)

Service workers already prevent anything but HTTP(S) URLs from 
creating them.

Fixes #381.
annevk added a commit to whatwg/html that referenced this issue Sep 30, 2016
The change to Fetch discussed in
whatwg/fetch#381 made it obsolete.

Closes #1243, closes #1778, and closes #1779 as these are all treated
as same-origin now per the change to Fetch.
annevk added a commit to whatwg/html that referenced this issue Oct 7, 2016
The change to Fetch discussed in
whatwg/fetch#381 made it obsolete.

Closes #1243, closes #1778, and closes #1779 as these are all treated
as same-origin now per the change to Fetch.
annevk added a commit to whatwg/html that referenced this issue Oct 10, 2016
The change to Fetch discussed in
whatwg/fetch#381 made it obsolete.

Closes #1778, and closes #1779 as these are all treated as
same-origin now per the change to Fetch.
domenic pushed a commit to whatwg/html that referenced this issue Oct 10, 2016
The change to Fetch discussed in
whatwg/fetch#381 made it obsolete.

Closes #1778, and closes #1779 as these are all treated as
same-origin now per the change to Fetch.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
The change to Fetch discussed in
whatwg/fetch#381 made it obsolete.

Closes whatwg#1778, and closes whatwg#1779 as these are all treated as
same-origin now per the change to Fetch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants