Skip to content

Commit

Permalink
Add fake headers to URLRequestRedirectJobs.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
mmenke@chromium.org committed Aug 6, 2014
1 parent ff0ed12 commit 6be6fa9
Show file tree
Hide file tree
Showing 6 changed files with 133 additions and 34 deletions.
4 changes: 2 additions & 2 deletions chrome/test/data/extensions/api_test/webrequest/test_blocking.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
12 changes: 6 additions & 6 deletions chrome/test/data/extensions/api_test/webrequest/test_declarative1.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -371,8 +371,8 @@ runTests([
"AAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJ" +
"ggg==",
fromCache: false,
statusCode: -1,
statusLine: "",
statusLine: "HTTP/1.1 307 Internal Redirect",
statusCode: 307,
type: "image",
}
},
Expand All @@ -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",
}
},
Expand Down
8 changes: 8 additions & 0 deletions net/base/net_log_event_type_list.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -798,6 +798,14 @@ EVENT_TYPE(URL_REQUEST_REDIRECT_JOB)
// "reason": <Reason for the redirect, as a string>,
// }

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": <The list of header:value pairs>,
// }

// ------------------------------------------------------------------------
// HttpCache
// ------------------------------------------------------------------------
Expand Down
77 changes: 59 additions & 18 deletions net/url_request/url_request_redirect_job.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,55 @@

#include "net/url_request/url_request_redirect_job.h"

#include <string>

#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 {

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,
Expand All @@ -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
21 changes: 13 additions & 8 deletions net/url_request/url_request_redirect_job.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
#include <string>

#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;
Expand All @@ -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,
};
Expand All @@ -34,27 +36,30 @@ 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();

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<HttpResponseHeaders> fake_headers_;

base::WeakPtrFactory<URLRequestRedirectJob> weak_factory_;
};

Expand Down
45 changes: 45 additions & 0 deletions net/url_request/url_request_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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());
Expand Down

0 comments on commit 6be6fa9

Please sign in to comment.