From fd4f3f018a0feeb6f1abedd93b1c540518529c90 Mon Sep 17 00:00:00 2001 From: Aaron Tagliaboschi Date: Thu, 30 May 2019 23:05:54 +0000 Subject: [PATCH] Collect not sent and not stored cookies in URLRequest Bug: 856777,966576 Change-Id: Ie4500ae83fc0371cf091648519b532cc3b96f7eb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1637033 Reviewed-by: Matt Menke Reviewed-by: Maks Orlovich Commit-Queue: Aaron Tagliaboschi Cr-Commit-Position: refs/heads/master@{#664939} --- net/cookies/canonical_cookie.cc | 20 +- net/cookies/canonical_cookie.h | 27 +- .../embedded_test_server/default_handlers.cc | 24 ++ net/url_request/url_request.cc | 19 + net/url_request/url_request.h | 22 ++ net/url_request/url_request_http_job.cc | 38 +- net/url_request/url_request_http_job.h | 5 +- net/url_request/url_request_unittest.cc | 325 ++++++++++++++++-- 8 files changed, 436 insertions(+), 44 deletions(-) diff --git a/net/cookies/canonical_cookie.cc b/net/cookies/canonical_cookie.cc index 50934f80827a43..4b2fd725631c33 100644 --- a/net/cookies/canonical_cookie.cc +++ b/net/cookies/canonical_cookie.cc @@ -634,9 +634,25 @@ std::string CanonicalCookie::DomainWithoutDot() const { return domain_.substr(1); } -CookieLineWithStatus::CookieLineWithStatus( +CookieAndLineWithStatus::CookieAndLineWithStatus() = default; + +CookieAndLineWithStatus::CookieAndLineWithStatus( + base::Optional cookie, std::string cookie_string, CanonicalCookie::CookieInclusionStatus status) - : cookie_string(std::move(cookie_string)), status(status) {} + : cookie(std::move(cookie)), + cookie_string(std::move(cookie_string)), + status(status) {} + +CookieAndLineWithStatus::CookieAndLineWithStatus( + const CookieAndLineWithStatus&) = default; + +CookieAndLineWithStatus& CookieAndLineWithStatus::operator=( + const CookieAndLineWithStatus& cookie_and_line_with_status) = default; + +CookieAndLineWithStatus::CookieAndLineWithStatus(CookieAndLineWithStatus&&) = + default; + +CookieAndLineWithStatus::~CookieAndLineWithStatus() = default; } // namespace net diff --git a/net/cookies/canonical_cookie.h b/net/cookies/canonical_cookie.h index 560918f4016051..71f61bf8c858b8 100644 --- a/net/cookies/canonical_cookie.h +++ b/net/cookies/canonical_cookie.h @@ -11,6 +11,7 @@ #include #include "base/gtest_prod_util.h" +#include "base/optional.h" #include "base/time/time.h" #include "net/base/net_export.h" #include "net/cookies/cookie_constants.h" @@ -22,8 +23,6 @@ namespace net { class ParsedCookie; -struct CookieWithStatus; - class NET_EXPORT CanonicalCookie { public: CanonicalCookie(); @@ -299,11 +298,25 @@ struct CookieWithStatus { CanonicalCookie::CookieInclusionStatus status; }; -// Just used for the next function to send the cookie string with it's status -struct CookieLineWithStatus { - CookieLineWithStatus(std::string cookie_string, - CanonicalCookie::CookieInclusionStatus status); +// Used to pass excluded cookie information when it's possible that the +// canonical cookie object may not be available. +struct NET_EXPORT CookieAndLineWithStatus { + CookieAndLineWithStatus(); + CookieAndLineWithStatus(base::Optional cookie, + std::string cookie_string, + CanonicalCookie::CookieInclusionStatus status); + CookieAndLineWithStatus( + const CookieAndLineWithStatus& cookie_and_line_with_status); + + CookieAndLineWithStatus& operator=( + const CookieAndLineWithStatus& cookie_and_line_with_status); + + CookieAndLineWithStatus( + CookieAndLineWithStatus&& cookie_and_line_with_status); + ~CookieAndLineWithStatus(); + + base::Optional cookie; std::string cookie_string; CanonicalCookie::CookieInclusionStatus status; }; @@ -312,6 +325,8 @@ typedef std::vector CookieList; typedef std::vector CookieStatusList; +typedef std::vector CookieAndLineStatusList; + } // namespace net #endif // NET_COOKIES_CANONICAL_COOKIE_H_ diff --git a/net/test/embedded_test_server/default_handlers.cc b/net/test/embedded_test_server/default_handlers.cc index f8abd10b3f9be5..58151d3cda0914 100644 --- a/net/test/embedded_test_server/default_handlers.cc +++ b/net/test/embedded_test_server/default_handlers.cc @@ -512,6 +512,7 @@ std::unique_ptr HandleServerRedirect(HttpStatusCode redirect_code, const HttpRequest& request) { GURL request_url = request.GetURL(); std::string dest = UnescapeBinaryURLComponent(request_url.query_piece()); + RequestQuery query = ParseQuery(request_url); std::unique_ptr http_response(new BasicHttpResponse); http_response->set_code(redirect_code); @@ -522,6 +523,25 @@ std::unique_ptr HandleServerRedirect(HttpStatusCode redirect_code, dest.c_str())); return std::move(http_response); } +// /server-redirect-with-cookie?URL +// Returns a server redirect to URL, and sets the cookie server-redirect=true. +std::unique_ptr HandleServerRedirectWithCookie( + HttpStatusCode redirect_code, + const HttpRequest& request) { + GURL request_url = request.GetURL(); + std::string dest = UnescapeBinaryURLComponent(request_url.query_piece()); + RequestQuery query = ParseQuery(request_url); + + std::unique_ptr http_response(new BasicHttpResponse); + http_response->set_code(redirect_code); + http_response->AddCustomHeader("Location", dest); + http_response->AddCustomHeader("Set-Cookie", "server-redirect=true"); + http_response->set_content_type("text/html"); + http_response->set_content(base::StringPrintf( + " Redirecting to %s", + dest.c_str())); + return std::move(http_response); +} // /cross-site?URL // Returns a cross-site redirect to URL. @@ -761,6 +781,10 @@ void RegisterDefaultHandlers(EmbeddedTestServer* server) { server->RegisterDefaultHandler(SERVER_REDIRECT_HANDLER( "/server-redirect-308", &HandleServerRedirect, HTTP_PERMANENT_REDIRECT)); + server->RegisterDefaultHandler(SERVER_REDIRECT_HANDLER( + "/server-redirect-with-cookie", &HandleServerRedirectWithCookie, + HTTP_MOVED_PERMANENTLY)); + server->RegisterDefaultHandler( base::BindRepeating(&HandleCrossSiteRedirect, server)); server->RegisterDefaultHandler( diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc index 1ecf10a0d62386..d11742cbf4400f 100644 --- a/net/url_request/url_request.cc +++ b/net/url_request/url_request.cc @@ -435,6 +435,15 @@ int URLRequest::GetResponseCode() const { return job_->GetResponseCode(); } +void URLRequest::set_not_sent_cookies(CookieStatusList excluded_cookies) { + not_sent_cookies_ = std::move(excluded_cookies); +} + +void URLRequest::set_not_stored_cookies( + CookieAndLineStatusList excluded_cookies) { + not_stored_cookies_ = std::move(excluded_cookies); +} + void URLRequest::SetLoadFlags(int flags) { if ((load_flags_ & LOAD_IGNORE_LIMITS) != (flags & LOAD_IGNORE_LIMITS)) { DCHECK(!job_.get()); @@ -676,6 +685,9 @@ void URLRequest::StartJob(URLRequestJob* job) { response_info_.was_cached = false; + not_sent_cookies_.clear(); + not_stored_cookies_.clear(); + GURL referrer_url(referrer_); if (referrer_url != URLRequestJob::ComputeReferrerForPolicy( referrer_policy_, referrer_url, url())) { @@ -886,6 +898,9 @@ void URLRequest::FollowDeferredRedirect( DCHECK(job_.get()); DCHECK(status_.is_success()); + not_sent_cookies_.clear(); + not_stored_cookies_.clear(); + status_ = URLRequestStatus::FromError(ERR_IO_PENDING); job_->FollowDeferredRedirect(removed_headers, modified_headers); } @@ -894,6 +909,9 @@ void URLRequest::SetAuth(const AuthCredentials& credentials) { DCHECK(job_.get()); DCHECK(job_->NeedsAuth()); + not_sent_cookies_.clear(); + not_stored_cookies_.clear(); + status_ = URLRequestStatus::FromError(ERR_IO_PENDING); job_->SetAuth(credentials); } @@ -1086,6 +1104,7 @@ void URLRequest::NotifyAuthRequiredComplete( void URLRequest::NotifyCertificateRequested( SSLCertRequestInfo* cert_request_info) { status_ = URLRequestStatus(); + OnCallToDelegate(NetLogEventType::URL_REQUEST_DELEGATE_CERTIFICATE_REQUESTED); delegate_->OnCertificateRequested(this, cert_request_info); } diff --git a/net/url_request/url_request.h b/net/url_request/url_request.h index 32966a3ba8f394..f6043de481fa0a 100644 --- a/net/url_request/url_request.h +++ b/net/url_request/url_request.h @@ -578,6 +578,25 @@ class NET_EXPORT URLRequest : public base::SupportsUserData { // the request is redirected. PrivacyMode privacy_mode() { return privacy_mode_; } + void set_not_sent_cookies(CookieStatusList excluded_cookies); + void set_not_stored_cookies(CookieAndLineStatusList excluded_cookies); + + // These lists contain a list of cookies that are associated with the given + // request but are removed or flagged from the request before use, along with + // the reason they were removed or flagged. They are cleared on redirects and + // other request restarts that cause sent cookies to be recomputed / new + // cookies to potentially be received (such as calling SetAuth() to send HTTP + // auth credentials, but not calling ContinueWithCertification() to respond to + // client cert challenges), and only contain the cookies relevant to the most + // recent roundtrip. + + // Populated while the http request is being built. + const CookieStatusList& not_sent_cookies() const { return not_sent_cookies_; } + // Populated after the response headers are received. + const CookieAndLineStatusList& not_stored_cookies() const { + return not_stored_cookies_; + } + // The new flags may change the IGNORE_LIMITS flag only when called // before Start() is called, it must only set the flag, and if set, // the priority of this request must already be MAXIMUM_PRIORITY. @@ -896,6 +915,9 @@ class NET_EXPORT URLRequest : public base::SupportsUserData { // expected values are LOAD_* enums above. PrivacyMode privacy_mode_; + CookieStatusList not_sent_cookies_; + CookieAndLineStatusList not_stored_cookies_; + #if BUILDFLAG(ENABLE_REPORTING) int reporting_upload_depth_; #endif diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc index a87b7808f4a8d7..f7e8b8fc2a7ac6 100644 --- a/net/url_request/url_request_http_job.cc +++ b/net/url_request/url_request_http_job.cc @@ -405,6 +405,7 @@ void URLRequestHttpJob::NotifyBeforeSendHeadersCallback( void URLRequestHttpJob::NotifyHeadersComplete() { DCHECK(!response_info_); DCHECK_EQ(0, num_cookie_lines_left_); + DCHECK(request_->not_stored_cookies().empty()); response_info_ = transaction_->GetResponseInfo(); @@ -421,7 +422,7 @@ void URLRequestHttpJob::NotifyHeadersComplete() { // Clear |cs_status_list_| after any processing in case // SaveCookiesAndNotifyHeadersComplete is called again. - cs_status_list_.clear(); + request_->set_not_stored_cookies(std::move(cs_status_list_)); // The HTTP transaction may be restarted several times for the purposes // of sending authorization information. Each time it restarts, we get @@ -626,6 +627,7 @@ void URLRequestHttpJob::AddCookieHeaderAndStart() { CookieStore* cookie_store = request_->context()->cookie_store(); if (cookie_store && !(request_info_.load_flags & LOAD_DO_NOT_SEND_COOKIES)) { CookieOptions options; + options.set_return_excluded_cookies(); options.set_include_httponly(); options.set_same_site_cookie_context( net::cookie_util::ComputeSameSiteContextForRequest( @@ -643,12 +645,12 @@ void URLRequestHttpJob::AddCookieHeaderAndStart() { void URLRequestHttpJob::SetCookieHeaderAndStart( const CookieList& cookie_list, const CookieStatusList& excluded_list) { + DCHECK(request_->not_sent_cookies().empty()); + CookieStatusList excluded_cookies = excluded_list; + if (!cookie_list.empty()) { - // |excluded_cookies| isn't currently being used, but it will be as part of - // crbug.com/856777 - CookieStatusList excluded_cookies = excluded_list; if (!CanGetCookies(cookie_list)) { - for (auto cookie : cookie_list) { + for (const auto& cookie : cookie_list) { excluded_cookies.push_back( {cookie, CanonicalCookie::CookieInclusionStatus::EXCLUDE_USER_PREFERENCES}); @@ -666,6 +668,8 @@ void URLRequestHttpJob::SetCookieHeaderAndStart( } } + request_->set_not_sent_cookies(std::move(excluded_cookies)); + StartTransaction(); } @@ -702,6 +706,8 @@ void URLRequestHttpJob::SaveCookiesAndNotifyHeadersComplete(int result) { request_->url(), request_->site_for_cookies(), request_->initiator())); + options.set_return_excluded_cookies(); + // Set all cookies, without waiting for them to be set. Any subsequent read // will see the combined result of all cookie operation. const base::StringPiece name("Set-Cookie"); @@ -728,12 +734,14 @@ void URLRequestHttpJob::SaveCookiesAndNotifyHeadersComplete(int result) { &returned_status); if (returned_status != CanonicalCookie::CookieInclusionStatus::INCLUDE) { - OnSetCookieResult(std::move(cookie_string), returned_status); + OnSetCookieResult(base::nullopt, std::move(cookie_string), + returned_status); continue; } if (!CanSetCookie(*cookie, &options)) { OnSetCookieResult( + base::make_optional(*cookie), std::move(cookie_string), CanonicalCookie::CookieInclusionStatus::EXCLUDE_USER_PREFERENCES); continue; @@ -741,8 +749,9 @@ void URLRequestHttpJob::SaveCookiesAndNotifyHeadersComplete(int result) { request_->context()->cookie_store()->SetCookieWithOptionsAsync( request_->url(), cookie_string, options, - base::BindOnce(&URLRequestHttpJob::OnSetCookieResult, - weak_factory_.GetWeakPtr(), cookie_string)); + base::BindOnce( + &URLRequestHttpJob::OnSetCookieResult, weak_factory_.GetWeakPtr(), + base::make_optional(*cookie), cookie_string)); } // Removing the 1 that |num_cookie_lines_left| started with, signifing that // loop has been exited. @@ -753,10 +762,13 @@ void URLRequestHttpJob::SaveCookiesAndNotifyHeadersComplete(int result) { } void URLRequestHttpJob::OnSetCookieResult( + base::Optional cookie, std::string cookie_string, CanonicalCookie::CookieInclusionStatus status) { - if (status != CanonicalCookie::CookieInclusionStatus::INCLUDE) - cs_status_list_.emplace_back(std::move(cookie_string), status); + if (status != CanonicalCookie::CookieInclusionStatus::INCLUDE) { + cs_status_list_.emplace_back(std::move(cookie), std::move(cookie_string), + status); + } num_cookie_lines_left_--; @@ -938,6 +950,12 @@ void URLRequestHttpJob::RestartTransactionWithAuth( // extra_headers, we need to strip them out before adding them again. request_info_.extra_headers.RemoveHeader(HttpRequestHeaders::kCookie); + // TODO(https://crbug.com/968327/): This is weird, as all other clearing is at + // the URLRequest layer. Should this call into URLRequest so it can share + // logic at that layer with SetAuth()? + request_->set_not_sent_cookies({}); + request_->set_not_stored_cookies({}); + AddCookieHeaderAndStart(); } diff --git a/net/url_request/url_request_http_job.h b/net/url_request/url_request_http_job.h index 5da15f70910d34..ff2a8170618f38 100644 --- a/net/url_request/url_request_http_job.h +++ b/net/url_request/url_request_http_job.h @@ -153,10 +153,11 @@ class NET_EXPORT_PRIVATE URLRequestHttpJob : public URLRequestJob { const CookieStatusList& excluded_list); // Another Cookie Monster callback - void OnSetCookieResult(std::string cookie_string, + void OnSetCookieResult(base::Optional cookie, + std::string cookie_string, CanonicalCookie::CookieInclusionStatus status); int num_cookie_lines_left_; - std::vector cs_status_list_; + CookieAndLineStatusList cs_status_list_; // Some servers send the body compressed, but specify the content length as // the uncompressed size. If this is the case, we return true in order diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc index ff3a7faa26537c..9cd73154bc0f5c 100644 --- a/net/url_request/url_request_unittest.cc +++ b/net/url_request/url_request_unittest.cc @@ -77,6 +77,7 @@ #include "net/cert/signed_certificate_timestamp_and_status.h" #include "net/cert/test_root_certs.h" #include "net/cert_net/cert_net_fetcher_impl.h" +#include "net/cookies/canonical_cookie_test_helpers.h" #include "net/cookies/cookie_monster.h" #include "net/cookies/cookie_store_test_helpers.h" #include "net/disk_cache/disk_cache.h" @@ -2578,7 +2579,10 @@ class FilteringTestLayeredNetworkDelegate : public LayeredNetworkDelegate { std::unique_ptr network_delegate) : LayeredNetworkDelegate(std::move((network_delegate))), set_cookie_called_count_(0), - blocked_set_cookie_count_(0) {} + blocked_set_cookie_count_(0), + block_get_cookies_(false), + get_cookie_called_count_(0), + blocked_get_cookie_count_(0) {} ~FilteringTestLayeredNetworkDelegate() override = default; bool OnCanSetCookieInternal(const URLRequest& request, @@ -2610,10 +2614,41 @@ class FilteringTestLayeredNetworkDelegate : public LayeredNetworkDelegate { void ResetBlockedSetCookieCount() { blocked_set_cookie_count_ = 0; } + bool OnCanGetCookiesInternal(const URLRequest& request, + const net::CookieList& cookie, + bool allowed_from_caller) override { + // Filter out cookies if |block_get_cookies_| is set and + // combine with |allowed_from_caller|. + bool allowed = allowed_from_caller && !block_get_cookies_; + + ++get_cookie_called_count_; + + if (!allowed) + ++blocked_get_cookie_count_; + + return allowed; + } + + void set_block_get_cookies() { block_get_cookies_ = true; } + + void unset_block_get_cookies() { block_get_cookies_ = false; } + + int get_cookie_called_count() const { return get_cookie_called_count_; } + + int blocked_get_cookie_count() const { return blocked_get_cookie_count_; } + + void ResetGetCookieCalledCount() { get_cookie_called_count_ = 0; } + + void ResetBlockedGetCookieCount() { blocked_get_cookie_count_ = 0; } + private: std::string cookie_name_filter_; int set_cookie_called_count_; int blocked_set_cookie_count_; + + bool block_get_cookies_; + int get_cookie_called_count_; + int blocked_get_cookie_count_; }; TEST_F(URLRequestTest, DelayedCookieCallbackAsync) { @@ -8499,37 +8534,166 @@ TEST_F(URLRequestTestHTTP, BasicAuthWithCookies) { } } -TEST_F(URLRequestTestHTTP, AuthChallengeWithFilteredCookie) { - ASSERT_TRUE(http_test_server()->Start()); - - GURL url_requiring_auth = http_test_server()->GetURL( - "/auth-basic?set-cookie-if-not-challenged&set-cookie-if-challenged"); +TEST_F(URLRequestTest, CatchFilteredCookies) { + HttpTestServer test_server; + ASSERT_TRUE(test_server.Start()); - FilteringTestLayeredNetworkDelegate filtering_network_delegate( - std::make_unique()); // Must outlive URLRequest. - filtering_network_delegate.SetCookieFilter( - "got_challenged"); // Filter the cookie auth-basic sets + FilteringTestLayeredNetworkDelegate network_delegate( + std::make_unique()); + network_delegate.SetCookieFilter("not_stored_cookie"); + network_delegate.set_block_get_cookies(); TestURLRequestContext context(true); - context.set_network_delegate(&filtering_network_delegate); + context.set_network_delegate(&network_delegate); context.Init(); + // Make sure cookies blocked from being stored are caught. + { + TestDelegate d; + std::unique_ptr req(context.CreateRequest( + test_server.GetURL("/set-cookie?not_stored_cookie=true"), + DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS)); + req->Start(); + d.RunUntilComplete(); - TestDelegate delegate; + ASSERT_EQ(1u, req->not_stored_cookies().size()); + EXPECT_EQ("not_stored_cookie", + req->not_stored_cookies().front().cookie->Name()); + EXPECT_EQ( + net::CanonicalCookie::CookieInclusionStatus::EXCLUDE_USER_PREFERENCES, + req->not_stored_cookies().front().status); + } + // Set a cookie to be blocked later + { + TestDelegate d; + std::unique_ptr req(context.CreateRequest( + test_server.GetURL("/set-cookie?not_sent_cookies=true"), + DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS)); + req->Start(); + d.RunUntilComplete(); + } + { + TestDelegate d; + // Make sure cookies blocked from being sent are caught. + std::unique_ptr req(context.CreateRequest( + test_server.GetURL("/echoheader?Cookie"), DEFAULT_PRIORITY, &d, + TRAFFIC_ANNOTATION_FOR_TESTS)); + req->Start(); + d.RunUntilComplete(); - delegate.set_credentials(AuthCredentials(kUser, kSecret)); + EXPECT_TRUE(d.data_received().find("not_sent_cookies=true") == + std::string::npos); - std::unique_ptr request( - context.CreateRequest(url_requiring_auth, DEFAULT_PRIORITY, &delegate, - TRAFFIC_ANNOTATION_FOR_TESTS)); - request->Start(); + ASSERT_EQ(1u, req->not_sent_cookies().size()); + EXPECT_EQ("not_sent_cookies", + req->not_sent_cookies().front().cookie.Name()); + EXPECT_EQ( + net::CanonicalCookie::CookieInclusionStatus::EXCLUDE_USER_PREFERENCES, + req->not_sent_cookies().front().status); + } +} - delegate.RunUntilComplete(); - EXPECT_THAT(delegate.request_status(), IsOk()); +TEST_F(URLRequestTestHTTP, AuthChallengeWithFilteredCookies) { + ASSERT_TRUE(http_test_server()->Start()); + + GURL url_requiring_auth = + http_test_server()->GetURL("/auth-basic?set-cookie-if-challenged"); + GURL url_requiring_auth_wo_cookies = + http_test_server()->GetURL("/auth-basic"); + // Check not_stored_cookies is populated first round trip, and cleared on the + // second. + { + FilteringTestLayeredNetworkDelegate filtering_network_delegate( + std::make_unique()); + filtering_network_delegate.SetCookieFilter("got_challenged"); + TestURLRequestContext context(true); + context.set_network_delegate(&filtering_network_delegate); + context.Init(); + + TestDelegate delegate; + + std::unique_ptr request( + context.CreateRequest(url_requiring_auth, DEFAULT_PRIORITY, &delegate, + TRAFFIC_ANNOTATION_FOR_TESTS)); + request->Start(); - // Make sure the cookie was actually filtered. - EXPECT_EQ(std::string::npos, - delegate.data_received().find("Cookie: got_challenged=true")); - // Make sure it was blocked twice. - EXPECT_EQ(2, filtering_network_delegate.blocked_set_cookie_count()); + delegate.RunUntilAuthRequired(); + // Make sure it was blocked once. + EXPECT_EQ(1, filtering_network_delegate.blocked_set_cookie_count()); + + // The number of cookies blocked from the most recent round trip. + ASSERT_EQ(1u, request->not_stored_cookies().size()); + + // Now check the second round trip + request->SetAuth(AuthCredentials(kUser, kSecret)); + delegate.RunUntilComplete(); + EXPECT_THAT(delegate.request_status(), IsOk()); + + // There are DCHECKs in URLRequestHttpJob that would fail if + // not_sent_cookies and not_stored_cookies were not cleared properly. + + // Make sure the cookie was actually filtered and not sent. + EXPECT_EQ(std::string::npos, + delegate.data_received().find("Cookie: got_challenged=true")); + + // The number of cookies blocked from the most recent round trip. + ASSERT_EQ(0u, request->not_stored_cookies().size()); + } + + // Check not_sent_cookies on first round trip (and cleared for the second). + { + FilteringTestLayeredNetworkDelegate filtering_network_delegate( + std::make_unique()); + filtering_network_delegate.set_block_get_cookies(); + TestURLRequestContext context(true); + context.set_network_delegate(&filtering_network_delegate); + + std::unique_ptr cm = + std::make_unique(nullptr, nullptr); + cm->SetCookieWithOptionsAsync(url_requiring_auth_wo_cookies, + "another_cookie=true", CookieOptions(), + CookieStore::SetCookiesCallback()); + context.set_cookie_store(cm.get()); + context.Init(); + + TestDelegate delegate; + + std::unique_ptr request( + context.CreateRequest(url_requiring_auth_wo_cookies, DEFAULT_PRIORITY, + &delegate, TRAFFIC_ANNOTATION_FOR_TESTS)); + request->Start(); + + delegate.RunUntilAuthRequired(); + + ASSERT_EQ(1u, request->not_sent_cookies().size()); + EXPECT_EQ("another_cookie", + request->not_sent_cookies().front().cookie.Name()); + EXPECT_EQ("true", request->not_sent_cookies().front().cookie.Value()); + + // Check not_sent_cookies on second roundtrip. + request->set_not_sent_cookies({}); + cm->DeleteAllAsync(CookieStore::DeleteCallback()); + cm->SetCookieWithOptionsAsync(url_requiring_auth_wo_cookies, + "one_more_cookie=true", CookieOptions(), + CookieStore::SetCookiesCallback()); + + request->SetAuth(AuthCredentials(kUser, kSecret)); + delegate.RunUntilComplete(); + EXPECT_THAT(delegate.request_status(), IsOk()); + + // There are DCHECKs in URLRequestHttpJob that would fail if + // not_sent_cookies and not_stored_cookies we not cleared properly. + + // Make sure the cookie was actually filtered. + EXPECT_EQ(std::string::npos, + delegate.data_received().find("Cookie: one_more_cookie=true")); + // got_challenged was set after the first request and blocked on the second, + // so it should only have been blocked this time + EXPECT_EQ(2, filtering_network_delegate.blocked_get_cookie_count()); + + // // The number of cookies blocked from the most recent round trip. + ASSERT_EQ(1u, request->not_sent_cookies().size()); + EXPECT_EQ("one_more_cookie", + request->not_sent_cookies().front().cookie.Name()); + } } // Tests that load timing works as expected with auth and the cache. @@ -8798,6 +8962,119 @@ TEST_F(URLRequestTestHTTP, Redirect302PreserveReferenceFragment) { EXPECT_EQ(expected_url, r->url()); } +TEST_F(URLRequestTestHTTP, RedirectWithFilteredCookies) { + ASSERT_TRUE(http_test_server()->Start()); + + // FilteringTestLayeredNetworkDelegate filters by name, so the names of the + // two cookies have to be the same. The values have been set to different + // strings (the value of the server-redirect cookies is "true" and set-cookie + // is "other") to differentiate between the two round trips. + GURL redirect_to( + http_test_server()->GetURL("/set-cookie?server-redirect=other")); + + GURL original_url(http_test_server()->GetURL("/server-redirect-with-cookie?" + + redirect_to.spec())); + + GURL original_url_wo_cookie( + http_test_server()->GetURL("/server-redirect?" + redirect_to.spec())); + // Check not_stored_cookies on first round trip. + { + FilteringTestLayeredNetworkDelegate filtering_network_delegate( + std::make_unique()); // Must outlive URLRequest. + filtering_network_delegate.SetCookieFilter( + "server-redirect"); // Filter the cookie server-redirect sets. + TestURLRequestContext context(true); + context.set_network_delegate(&filtering_network_delegate); + context.Init(); + + TestDelegate delegate; + std::unique_ptr request( + context.CreateRequest(original_url, DEFAULT_PRIORITY, &delegate, + TRAFFIC_ANNOTATION_FOR_TESTS)); + + request->Start(); + delegate.RunUntilRedirect(); + + // Make sure it was blocked once. + EXPECT_EQ(1, filtering_network_delegate.blocked_set_cookie_count()); + + // The number of cookies blocked from the most recent round trip. + ASSERT_EQ(1u, request->not_stored_cookies().size()); + EXPECT_EQ("server-redirect", + request->not_stored_cookies().front().cookie->Name()); + EXPECT_EQ("true", request->not_stored_cookies().front().cookie->Value()); + + // Check not_stored_cookies on second round trip (and clearing from the + // first). + request->FollowDeferredRedirect(base::nullopt, base::nullopt); + delegate.RunUntilComplete(); + EXPECT_THAT(delegate.request_status(), IsOk()); + + // There are DCHECKs in URLRequestHttpJob that would fail if + // not_sent_cookies and not_stored_cookies we not cleared properly. + + // Make sure it was blocked twice. + EXPECT_EQ(2, filtering_network_delegate.blocked_set_cookie_count()); + + // The number of cookies blocked from the most recent round trip. + ASSERT_EQ(1u, request->not_stored_cookies().size()); + EXPECT_EQ("server-redirect", + request->not_stored_cookies().front().cookie->Name()); + EXPECT_EQ("other", request->not_stored_cookies().front().cookie->Value()); + } + + // Check not_sent_cookies on first round trip. + { + FilteringTestLayeredNetworkDelegate filtering_network_delegate( + std::make_unique()); + filtering_network_delegate.set_block_get_cookies(); + TestURLRequestContext context(true); + context.set_network_delegate(&filtering_network_delegate); + std::unique_ptr cm = + std::make_unique(nullptr, nullptr); + cm->SetCookieWithOptionsAsync(original_url, "another_cookie=true", + CookieOptions(), + CookieStore::SetCookiesCallback()); + context.set_cookie_store(cm.get()); + context.Init(); + + TestDelegate delegate; + std::unique_ptr request( + context.CreateRequest(original_url_wo_cookie, DEFAULT_PRIORITY, + &delegate, TRAFFIC_ANNOTATION_FOR_TESTS)); + + request->Start(); + + delegate.RunUntilRedirect(); + + ASSERT_EQ(1u, request->not_sent_cookies().size()); + EXPECT_EQ("another_cookie", + request->not_sent_cookies().front().cookie.Name()); + + // Check not_sent_cookies on second round trip + request->set_not_sent_cookies({}); + cm->DeleteAllAsync(CookieStore::DeleteCallback()); + cm->SetCookieWithOptionsAsync(original_url_wo_cookie, + "one_more_cookie=true", CookieOptions(), + CookieStore::SetCookiesCallback()); + + request->FollowDeferredRedirect(base::nullopt, base::nullopt); + delegate.RunUntilComplete(); + EXPECT_THAT(delegate.request_status(), IsOk()); + + // There are DCHECKs in URLRequestHttpJob that would fail if + // not_sent_cookies and not_stored_cookies we not cleared properly. + + EXPECT_EQ(2, filtering_network_delegate.blocked_get_cookie_count()); + + // The number of cookies blocked from the most recent round trip. + ASSERT_EQ(1u, request->not_sent_cookies().size()); + EXPECT_EQ("one_more_cookie", + request->not_sent_cookies().front().cookie.Name()); + EXPECT_EQ("true", request->not_sent_cookies().front().cookie.Value()); + } +} + TEST_F(URLRequestTestHTTP, RedirectPreserveFirstPartyURL) { ASSERT_TRUE(http_test_server()->Start());