From 6be6fa9adac1dd2b00c8ba2f87d22749495bc52c Mon Sep 17 00:00:00 2001 From: "mmenke@chromium.org" Date: Wed, 6 Aug 2014 23:44:56 +0000 Subject: [PATCH] Add fake headers to URLRequestRedirectJobs. Plugin logic expects redirects to have headers, and gets very confused when they're missing. BUG=345160 Review URL: https://codereview.chromium.org/422233006 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@287901 0039d316-1c4b-4281-b951-d872f2087c98 --- .../api_test/webrequest/test_blocking.js | 4 +- .../api_test/webrequest/test_declarative1.js | 12 +-- net/base/net_log_event_type_list.h | 8 ++ net/url_request/url_request_redirect_job.cc | 77 ++++++++++++++----- net/url_request/url_request_redirect_job.h | 21 +++-- net/url_request/url_request_unittest.cc | 45 +++++++++++ 6 files changed, 133 insertions(+), 34 deletions(-) diff --git a/chrome/test/data/extensions/api_test/webrequest/test_blocking.js b/chrome/test/data/extensions/api_test/webrequest/test_blocking.js index f60f55dead03..f3aee70d04ca 100644 --- a/chrome/test/data/extensions/api_test/webrequest/test_blocking.js +++ b/chrome/test/data/extensions/api_test/webrequest/test_blocking.js @@ -280,9 +280,9 @@ runTests([ details: { url: getURL("complexLoad/a.html"), redirectUrl: getURL("simpleLoad/a.html"), - statusLine: "", - statusCode: -1, fromCache: false, + statusLine: "HTTP/1.1 307 Internal Redirect", + statusCode: 307, } }, { label: "onBeforeRequest-2", diff --git a/chrome/test/data/extensions/api_test/webrequest/test_declarative1.js b/chrome/test/data/extensions/api_test/webrequest/test_declarative1.js index bdd46aa7a41d..591f329c4253 100644 --- a/chrome/test/data/extensions/api_test/webrequest/test_declarative1.js +++ b/chrome/test/data/extensions/api_test/webrequest/test_declarative1.js @@ -310,9 +310,9 @@ runTests([ details: { url: getURLHttpComplex(), redirectUrl: getURLHttpSimple(), - statusLine: "", - statusCode: -1, fromCache: false, + statusLine: "HTTP/1.1 307 Internal Redirect", + statusCode: 307, } }, { label: "onBeforeRequest-b", @@ -371,8 +371,8 @@ runTests([ "AAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJ" + "ggg==", fromCache: false, - statusCode: -1, - statusLine: "", + statusLine: "HTTP/1.1 307 Internal Redirect", + statusCode: 307, type: "image", } }, @@ -385,8 +385,8 @@ runTests([ "extensions/api_test/webrequest/declarative/frame.html"), redirectUrl: "data:text/html,", fromCache: false, - statusCode: -1, - statusLine: "", + statusLine: "HTTP/1.1 307 Internal Redirect", + statusCode: 307, type: "sub_frame", } }, diff --git a/net/base/net_log_event_type_list.h b/net/base/net_log_event_type_list.h index 64336bccdee8..0626683a5914 100644 --- a/net/base/net_log_event_type_list.h +++ b/net/base/net_log_event_type_list.h @@ -798,6 +798,14 @@ EVENT_TYPE(URL_REQUEST_REDIRECT_JOB) // "reason": , // } +EVENT_TYPE(URL_REQUEST_FAKE_RESPONSE_HEADERS_CREATED) +// This event is logged when a URLRequestRedirectJob creates the fake response +// headers for a request, prior to returning them. +// The following parameters are attached: +// { +// "headers": , +// } + // ------------------------------------------------------------------------ // HttpCache // ------------------------------------------------------------------------ diff --git a/net/url_request/url_request_redirect_job.cc b/net/url_request/url_request_redirect_job.cc index 818d4c397ec5..15ebdcdf1c09 100644 --- a/net/url_request/url_request_redirect_job.cc +++ b/net/url_request/url_request_redirect_job.cc @@ -4,12 +4,18 @@ #include "net/url_request/url_request_redirect_job.h" +#include + #include "base/bind.h" #include "base/compiler_specific.h" #include "base/logging.h" #include "base/message_loop/message_loop.h" +#include "base/strings/stringprintf.h" #include "net/base/load_timing_info.h" +#include "net/base/net_errors.h" #include "net/base/net_log.h" +#include "net/http/http_response_headers.h" +#include "net/http/http_util.h" #include "net/url_request/url_request.h" namespace net { @@ -17,16 +23,36 @@ namespace net { URLRequestRedirectJob::URLRequestRedirectJob(URLRequest* request, NetworkDelegate* network_delegate, const GURL& redirect_destination, - StatusCode http_status_code, + ResponseCode response_code, const std::string& redirect_reason) : URLRequestJob(request, network_delegate), redirect_destination_(redirect_destination), - http_status_code_(http_status_code), + response_code_(response_code), redirect_reason_(redirect_reason), weak_factory_(this) { DCHECK(!redirect_reason_.empty()); } +void URLRequestRedirectJob::GetResponseInfo(HttpResponseInfo* info) { + // Should only be called after the URLRequest has been notified there's header + // information. + DCHECK(fake_headers_); + + // This assumes |info| is a freshly constructed HttpResponseInfo. + info->headers = fake_headers_; + info->request_time = response_time_; + info->response_time = response_time_; +} + +void URLRequestRedirectJob::GetLoadTimingInfo( + LoadTimingInfo* load_timing_info) const { + // Set send_start and send_end to receive_headers_end_ to be consistent + // with network cache behavior. + load_timing_info->send_start = receive_headers_end_; + load_timing_info->send_end = receive_headers_end_; + load_timing_info->receive_headers_end = receive_headers_end_; +} + void URLRequestRedirectJob::Start() { request()->net_log().AddEvent( NetLog::TYPE_URL_REQUEST_REDIRECT_JOB, @@ -37,33 +63,48 @@ void URLRequestRedirectJob::Start() { weak_factory_.GetWeakPtr())); } -bool URLRequestRedirectJob::IsRedirectResponse(GURL* location, - int* http_status_code) { - *location = redirect_destination_; - *http_status_code = http_status_code_; - return true; -} - bool URLRequestRedirectJob::CopyFragmentOnRedirect(const GURL& location) const { // The instantiators have full control over the desired redirection target, // including the reference fragment part of the URL. return false; } +int URLRequestRedirectJob::GetResponseCode() const { + // Should only be called after the URLRequest has been notified there's header + // information. + DCHECK(fake_headers_); + return response_code_; +} + URLRequestRedirectJob::~URLRequestRedirectJob() {} void URLRequestRedirectJob::StartAsync() { receive_headers_end_ = base::TimeTicks::Now(); - NotifyHeadersComplete(); -} + response_time_ = base::Time::Now(); -void URLRequestRedirectJob::GetLoadTimingInfo( - LoadTimingInfo* load_timing_info) const { - // Set send_start and send_end to receive_headers_end_ to keep consistent - // with network cache behavior. - load_timing_info->send_start = receive_headers_end_; - load_timing_info->send_end = receive_headers_end_; - load_timing_info->receive_headers_end = receive_headers_end_; + std::string header_string = + base::StringPrintf("HTTP/1.1 %i Internal Redirect\n" + "Location: %s\n" + "Non-Authoritative-Reason: %s", + response_code_, + redirect_destination_.spec().c_str(), + redirect_reason_.c_str()); + fake_headers_ = new HttpResponseHeaders( + HttpUtil::AssembleRawHeaders(header_string.c_str(), + header_string.length())); + DCHECK(fake_headers_->IsRedirect(NULL)); + + request()->net_log().AddEvent( + NetLog::TYPE_URL_REQUEST_FAKE_RESPONSE_HEADERS_CREATED, + base::Bind( + &HttpResponseHeaders::NetLogCallback, + base::Unretained(fake_headers_.get()))); + + // TODO(mmenke): Consider calling the NetworkDelegate with the headers here. + // There's some weirdness about how to handle the case in which the delegate + // tries to modify the redirect location, in terms of how IsSafeRedirect + // should behave, and whether the fragment should be copied. + URLRequestJob::NotifyHeadersComplete(); } } // namespace net diff --git a/net/url_request/url_request_redirect_job.h b/net/url_request/url_request_redirect_job.h index 580fec2d1096..18c4e10ed20e 100644 --- a/net/url_request/url_request_redirect_job.h +++ b/net/url_request/url_request_redirect_job.h @@ -8,7 +8,9 @@ #include #include "base/memory/weak_ptr.h" +#include "base/time/time.h" #include "net/base/net_export.h" +#include "net/http/http_response_info.h" #include "net/url_request/url_request_job.h" class GURL; @@ -24,7 +26,7 @@ class NET_EXPORT URLRequestRedirectJob : public URLRequestJob { // valid, but unused so far. Both 302 and 307 are temporary redirects, with // the difference being that 302 converts POSTs to GETs and removes upload // data. - enum StatusCode { + enum ResponseCode { REDIRECT_302_FOUND = 302, REDIRECT_307_TEMPORARY_REDIRECT = 307, }; @@ -34,16 +36,16 @@ class NET_EXPORT URLRequestRedirectJob : public URLRequestJob { URLRequestRedirectJob(URLRequest* request, NetworkDelegate* network_delegate, const GURL& redirect_destination, - StatusCode http_status_code, + ResponseCode response_code, const std::string& redirect_reason); - virtual void Start() OVERRIDE; - virtual bool IsRedirectResponse(GURL* location, - int* http_status_code) OVERRIDE; - virtual bool CopyFragmentOnRedirect(const GURL& location) const OVERRIDE; - + // URLRequestJob implementation: + virtual void GetResponseInfo(HttpResponseInfo* info) OVERRIDE; virtual void GetLoadTimingInfo( LoadTimingInfo* load_timing_info) const OVERRIDE; + virtual void Start() OVERRIDE; + virtual bool CopyFragmentOnRedirect(const GURL& location) const OVERRIDE; + virtual int GetResponseCode() const OVERRIDE; private: virtual ~URLRequestRedirectJob(); @@ -51,10 +53,13 @@ class NET_EXPORT URLRequestRedirectJob : public URLRequestJob { void StartAsync(); const GURL redirect_destination_; - const int http_status_code_; + const ResponseCode response_code_; base::TimeTicks receive_headers_end_; + base::Time response_time_; std::string redirect_reason_; + scoped_refptr fake_headers_; + base::WeakPtrFactory weak_factory_; }; diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc index 791904eed9b5..a66924ef862f 100644 --- a/net/url_request/url_request_unittest.cc +++ b/net/url_request/url_request_unittest.cc @@ -2949,9 +2949,23 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequest) { GURL original_url(test_server_.GetURL("empty.html")); URLRequest r(original_url, DEFAULT_PRIORITY, &d, &context); + // Quit after hitting the redirect, so can check the headers. + d.set_quit_on_redirect(true); r.Start(); base::RunLoop().Run(); + // Check headers from URLRequestJob. + EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status()); + EXPECT_EQ(307, r.GetResponseCode()); + EXPECT_EQ(307, r.response_headers()->response_code()); + std::string location; + ASSERT_TRUE(r.response_headers()->EnumerateHeader(NULL, "Location", + &location)); + EXPECT_EQ(redirect_url, GURL(location)); + + // Let the request finish. + r.FollowDeferredRedirect(); + base::RunLoop().Run(); EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status()); EXPECT_TRUE(r.proxy_server().Equals(test_server_.host_port_pair())); EXPECT_EQ( @@ -2988,9 +3002,24 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequestSynchronously) { GURL original_url(test_server_.GetURL("empty.html")); URLRequest r(original_url, DEFAULT_PRIORITY, &d, &context); + // Quit after hitting the redirect, so can check the headers. + d.set_quit_on_redirect(true); r.Start(); base::RunLoop().Run(); + // Check headers from URLRequestJob. + EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status()); + EXPECT_EQ(307, r.GetResponseCode()); + EXPECT_EQ(307, r.response_headers()->response_code()); + std::string location; + ASSERT_TRUE(r.response_headers()->EnumerateHeader(NULL, "Location", + &location)); + EXPECT_EQ(redirect_url, GURL(location)); + + // Let the request finish. + r.FollowDeferredRedirect(); + base::RunLoop().Run(); + EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status()); EXPECT_TRUE(r.proxy_server().Equals(test_server_.host_port_pair())); EXPECT_EQ( @@ -3034,9 +3063,25 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequestPost) { headers.SetHeader(HttpRequestHeaders::kContentLength, base::UintToString(arraysize(kData) - 1)); r.SetExtraRequestHeaders(headers); + + // Quit after hitting the redirect, so can check the headers. + d.set_quit_on_redirect(true); r.Start(); base::RunLoop().Run(); + // Check headers from URLRequestJob. + EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status()); + EXPECT_EQ(307, r.GetResponseCode()); + EXPECT_EQ(307, r.response_headers()->response_code()); + std::string location; + ASSERT_TRUE(r.response_headers()->EnumerateHeader(NULL, "Location", + &location)); + EXPECT_EQ(redirect_url, GURL(location)); + + // Let the request finish. + r.FollowDeferredRedirect(); + base::RunLoop().Run(); + EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status()); EXPECT_EQ(0, r.status().error()); EXPECT_EQ(redirect_url, r.url());