Bug 24683 – XHR in Workers doesn't set the referrer correctly.

• Component: XML
• Status: RESOLVED FIXED

[David Levin]

It currently sends the url document as the referrer.

[David Levin]

Created attachment 28734 [details]
Proposed fix.

[Alexey Proskuryakov]

Does this fix Origin for cross-origin requests, as well?

[David Levin]

> Does this fix Origin for cross-origin requests, as well?
Yes, this line in WorkerThreadableLoader::WorkerThreadableLoader: FrameLoader::addHTTPOriginIfNeeded()

But I haven't added a test for that yet.  Perhaps I should remove this line and just get in this fix w/o it.  Then, add this line with the appropriate test.

[Alexey Proskuryakov]

Comment on attachment 28734 [details]
Proposed fix.

+        * http/tests/xmlhttprequest/workers/resources/print-referrer.cgi: Added.

There is no need to copy this file - the one from xmlhttprequest/resources can be used instead.

+Test for bug 11611: REGRESSION: No http referer header sent on XMLHttpRequest.

That's a lie!

    // FIXME: the created loader has no knowledge of the origin of the worker doing the load request.

How is this FIXME from in WorkerThreadableLoader::MainThreadBridge::mainThreadCreateLoader() affected by this change? It certainly doesn't look good that we're setting request fields hoping that SubresourceLoader won't override them, and that it won't add anything improper. E.g., the setResponseContentDispositionEncodingFallbackArray() call it makes via FrameLoader::addExtraFieldsToRequest() is incorrect, but harmless - it's awfully fragile to assume that this function will never change.

This is not subject of this bug, but I'd like to understand what the right cookie policy URL is for XHRs made from workers.

+    // FIXME: Combine this code into a shared function with similar code from FrameLoader.

It would be nice to move this code to a separate function at least.

[Sam Weinig]

Comment on attachment 28734 [details]
Proposed fix.

r- based on Alexey's comments just to get this bug moving.

[Alexey Proskuryakov]

David, you've been doing some related work today, would you like to finish this one?

[David Levin]

(In reply to comment #6)
> David, you've been doing some related work today, would you like to finish this one?

I'll try to get to it this week (but if not this week, at least soon).

[David Levin]

(In reply to comment #6)
> David, you've been doing some related work today, would you like to finish this one?

I've been looking at this and the feedback from before. I've been trying to consider how to make this less fragile and here's what I came up with:

It looks like FrameLoader::outgoingOrigin and outgoingReferrer are the two fields that may cause problems. One simple solution would be to pass them a url and if that url is not empty, use it as an override. This url could be in the ResourceRequest (m_originatingURL or m_requesteeURL).

The worker would be the only place that sets the URL (and I'd add the logic to copy it cross thread of course).

How does that sound?

[Alexey Proskuryakov]

Adding things to ResourceRequest sounds undesirable. Im not sure if we pass these requests over Mac API boundary as NSURLRequest, but even if we don't, we probably should start doing that one day.

Logically, this also doesn't sound great. ResourceRequest encapsulates a network layer view of the request, and network layer shouldn't know that there is some page or worker somewhere originating the request. It's a higher level task to prepare correct Referer and Origin headers.

[David Levin]

(In reply to comment #9)
> Adding things to ResourceRequest sounds undesirable. Im not sure if we pass these requests over Mac API boundary as NSURLRequest, but even if we don't, we probably should start doing that one day.
> 
> Logically, this also doesn't sound great. ResourceRequest encapsulates a network layer view of the request, and network layer shouldn't know that there is some page or worker somewhere originating the request. It's a higher level task to prepare correct Referer and Origin headers.

Ok, so instead of ResourceRequest, we can just the url to the SubresourceLoader::create api, plumb it through to there (and the rest remains the same).

[David Levin]

Created attachment 82980 [details]
Patch

[WebKit Review Bot]

Attachment 82980 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/loader/WorkerThreadableLoader.h:49:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/loader/SubresourceLoader.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

fyi, Adam lest you think that Alexey will review this since he looked at it last time. (In irc) He suggested that you are likely the best reviewer for it.

No rush at all.

[Adam Barth]

I looked at it enough to know that it will require a more careful look.  Will look soon.

[Adam Barth]

Comment on attachment 82980 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82980&action=review

I'm not in love with this patch.  It's not really clear when it's safe to pass an empty originating URL.

I've wanted to add a mechanism like this for a long time.  Another place we need something like this so that data URLs can inherit the origin of their requestor.  Given that we can't extend the ResourceRequest structure (due to sadness in the Mac port), the approach I had looked at before was to create a structure that contained both the resource request and the origin information.  At some level, that's just a syntactic change from what you've done in this patch (which just passes the two next to each other), but that seems like a better road forward.

Concretely, if you don't want to introduce the larger structure (which will likely require some refactoring), it might be worth looking at whether we can avoid using an empty KURL as an originating URL.  For example, if you disentangle createOutgoingReferrer from FrameLoader, then you can rename originatingURL to outgoingReferrer and do the URL tweaking on the worker thread.

I'm going to mark this R- for now, but if you think I'm way off base, please feel free to re-nominate it.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:72
> -DocumentThreadableLoader::DocumentThreadableLoader(Document* document, ThreadableLoaderClient* client, BlockingBehavior blockingBehavior, const ResourceRequest& request, const ThreadableLoaderOptions& options)
> +DocumentThreadableLoader::DocumentThreadableLoader(Document* document, ThreadableLoaderClient* client, BlockingBehavior blockingBehavior, const ResourceRequest& request, const ThreadableLoaderOptions& options, const KURL& originatingURL)

What is an originatingURL ?  That's like the referrer but without the fragment (and friends) removed?

> Source/WebCore/loader/FrameLoader.cpp:644
> -void FrameLoader::setOutgoingReferrer(const KURL& url)
> +KURL FrameLoader::createOutgoingReferrer(const KURL& url)
>  {
>      KURL outgoingReferrer(url);
>      outgoingReferrer.setUser(String());
>      outgoingReferrer.setPass(String());
>      outgoingReferrer.removeFragmentIdentifier();
> -    m_outgoingReferrer = outgoingReferrer.string();
> +    return outgoingReferrer;
> +}

Should this be a method on KURL?  Maybe a free function somewhere?  It seems unrelated to FrameLoader now.

[David Levin]

(In reply to comment #15)
> (From update of attachment 82980 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82980&action=review
> 
> I'm not in love with this patch.  It's not really clear when it's safe to pass an empty originating URL.

Fair enough.

> the approach I had looked at before was to create a structure that contained both the resource request and the origin information.  At some level, that's just a syntactic change from what you've done in this patch (which just passes the two next to each other), but that seems like a better road forward.

I'll work something up and run it by you.

> > Source/WebCore/loader/FrameLoader.cpp:644
> > +KURL FrameLoader::createOutgoingReferrer(const KURL& url)
> Should this be a method on KURL?  Maybe a free function somewhere?  It seems unrelated to FrameLoader now.

Sounds good. I wanted it to be somewhere else but couldn't think of a good place (and loader_util.h was a cop-out that I didn't want to do).

[Adam Barth]

WebKit is generally ok with free functions in header files that also contain classes.  (Although personally, I think they're hard to find the implementation of.)

[David Levin]

(In reply to comment #15)
>   the approach I had looked at before was to create a structure that contained both the resource request and the origin information.  At some level, that's just a syntactic change from what you've done in this patch (which just passes the two next to each other), but that seems like a better road forward.

What is the benefit of this larger structure?

I'm not opposed to the work but I don't understand why. I see only downsides: more complicated and more importantly two pieces of information which can be out of sync.

>   you can rename originatingURL to outgoingReferrer and do the URL tweaking on the worker thread.

So I can change the parameter to be "KURL* outgoingReferrerOverride". One can pass in 0 if no override is desired. If one passes in an override, it will apply to the referrer and origin.

[David Levin]

Created attachment 86377 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 86377 [details]
Patch

Clearing flags on attachment: 86377

Committed r81639: <http://trac.webkit.org/changeset/81639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.