content/child/web_url_loader_impl.cc - Issue 1017583002: Set Origin header to "null" for cross origin redirects.

Unified Diff: content/child/web_url_loader_impl.cc

Issue 1017583002: Set Origin header to "null" for cross origin redirects. (Closed) Base URL: https://chromium.googlesource.com/chromium/src@master

Patch Set: Addressed nits Created 5 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: content/child/web_url_loader_impl.cc

diff --git a/content/child/web_url_loader_impl.cc b/content/child/web_url_loader_impl.cc

index e2cec70d33531e72ea4eb4d3a7a83f4120a9b233..01fad1ccb2059af11645499b2409b51a1af5a1d1 100644

--- a/content/child/web_url_loader_impl.cc

+++ b/content/child/web_url_loader_impl.cc

@@ -34,11 +34,13 @@

#include "net/base/filename_util.h"

#include "net/base/mime_util.h"

#include "net/base/net_errors.h"

+#include "net/http/http_request_headers.h"

#include "net/http/http_response_headers.h"

#include "net/http/http_util.h"

#include "net/url_request/redirect_info.h"

#include "net/url_request/url_request_data_job.h"

#include "third_party/WebKit/public/platform/WebHTTPLoadInfo.h"

+#include "third_party/WebKit/public/platform/WebString.h"

#include "third_party/WebKit/public/platform/WebURL.h"

#include "third_party/WebKit/public/platform/WebURLError.h"

#include "third_party/WebKit/public/platform/WebURLLoadTiming.h"

@@ -562,6 +564,14 @@ bool WebURLLoaderImpl::Context::OnReceivedRedirect(

if (redirect_info.new_method == old_method)

new_request.setHTTPBody(request_.httpBody());

+ // This is necessary to avoid laundering the Origin header across redirects,

+ // which would break some CSRF protections. See the comment in

+ // URLRequest::Redirect in //net/url_request.cc for more information.

+ WebString origin_header =

+ WebString::fromUTF8(net::HttpRequestHeaders::kOrigin);

+ new_request.setHTTPHeaderField(origin_header,

+ request_.httpHeaderField(origin_header));

davidben 2015/03/24 23:47:38 I think this does the opposite of what the comment

jww 2015/03/27 22:16:15 Hm, okay, I'm happy to remove it. All of the tests

On 2015/03/24 23:47:38, David Benjamin wrote: > I think this does the opposite of what the comment says. The renderer doesn't > really find out about any changes that happen during redirects. That's what I > meant by we end up unfortunately duplicating a lot of stuff, and why there's a > net::RedirectInfo now. This *is* where that all gets applied. > > In fact, this is kind of odd... I wonder if we've been failing to copy over all > the extra headers (including Origin) from request leg to request leg (from > Blink's perspective) all this time... > > To make matters worse, having URLRequest send a copy of the new extra request > headers back down to renderer doesn't quite work either because if code in > between staples more headers, Blink should be blind to them. And I don't even > want to think about what happens if ServiceWorker mucks about with this. > > I *think* the thing to do for now is to remove this code (sorry!) and only put > logic in URLRequest. We have a giant hairball to untangle around redirects and > process boundaries and CORS... Blink apparently has its own version of this > logic here. Which... actually if I'm reading it right, that does something > completely different! > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Except I rather suspect this doesn't do anything. Certainly this never made its > way to the network stack because Blink has no influence on redirects. I also > don't think the Origin header got copied over anyway because this code never > copies over random headers... > > ...which means that probably doing a form submission which then does a > method-preserving redirect elsewhere and then pressing refresh causes all kinds > of problems. Until all these navigation refactors go through and then they might > not! (I suspect long term, we might want to move CORS out of the renderer > process? If we can get rid of all the places where the renderer cares about the > post-redirect request headers, that would resolve a lot of this.)

Hm, okay, I'm happy to remove it. All of the tests that I wrote still pass, so that does make me think it's unnecessary :-)

davidben 2015/03/27 22:46:51 Well, the cases where Blink's version of the reque

On 2015/03/27 22:16:15, jww wrote: > On 2015/03/24 23:47:38, David Benjamin wrote: > > I think this does the opposite of what the comment says. The renderer doesn't > > really find out about any changes that happen during redirects. That's what I > > meant by we end up unfortunately duplicating a lot of stuff, and why there's a > > net::RedirectInfo now. This *is* where that all gets applied. > > > > In fact, this is kind of odd... I wonder if we've been failing to copy over > all > > the extra headers (including Origin) from request leg to request leg (from > > Blink's perspective) all this time... > > > > To make matters worse, having URLRequest send a copy of the new extra request > > headers back down to renderer doesn't quite work either because if code in > > between staples more headers, Blink should be blind to them. And I don't even > > want to think about what happens if ServiceWorker mucks about with this. > > > > I *think* the thing to do for now is to remove this code (sorry!) and only put > > logic in URLRequest. We have a giant hairball to untangle around redirects and > > process boundaries and CORS... Blink apparently has its own version of this > > logic here. Which... actually if I'm reading it right, that does something > > completely different! > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > Except I rather suspect this doesn't do anything. Certainly this never made > its > > way to the network stack because Blink has no influence on redirects. I also > > don't think the Origin header got copied over anyway because this code never > > copies over random headers... > > > > ...which means that probably doing a form submission which then does a > > method-preserving redirect elsewhere and then pressing refresh causes all > kinds > > of problems. Until all these navigation refactors go through and then they > might > > not! (I suspect long term, we might want to move CORS out of the renderer > > process? If we can get rid of all the places where the renderer cares about > the > > post-redirect request headers, that would resolve a lot of this.) > > Hm, okay, I'm happy to remove it. All of the tests that I wrote still pass, so > that does make me think it's unnecessary :-)

Well, the cases where Blink's version of the request headers are important are pretty rare. I'm sure we have zero tests for those. I know of two (reloads and document.referrer), but I'm sure there's more. :-)

// Protect from deletion during call to willSendRequest.

scoped_refptr<Context> protect(this);

« no previous file with comments | « no previous file | net/data/url_request_unittest/redirect301-to-ftp » ('j') | net/url_request/url_request.cc » ('J')