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 <mmenke@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Commit-Queue: Aaron Tagliaboschi <aarontag@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664939}

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<CanonicalCookie> 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

net/cookies/canonical_cookie.h

@@ -11,6 +11,7 @@
 #include <vector>
 
 #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<CanonicalCookie> 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<CanonicalCookie> cookie;
   std::string cookie_string;
   CanonicalCookie::CookieInclusionStatus status;
 };
@@ -312,6 +325,8 @@ typedef std::vector<CanonicalCookie> CookieList;
 
 typedef std::vector<CookieWithStatus> CookieStatusList;
 
+typedef std::vector<CookieAndLineWithStatus> CookieAndLineStatusList;
+
 }  // namespace net
 
 #endif  // NET_COOKIES_CANONICAL_COOKIE_H_

net/test/embedded_test_server/default_handlers.cc

@@ -512,6 +512,7 @@ std::unique_ptr<HttpResponse> 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<BasicHttpResponse> http_response(new BasicHttpResponse);
   http_response->set_code(redirect_code);
@@ -522,6 +523,25 @@ std::unique_ptr<HttpResponse> 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<HttpResponse> 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<BasicHttpResponse> 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(
+      "<html><head></head><body>Redirecting to %s</body></html>",
+      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(

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);
 }

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

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,21 +734,24 @@ 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<CanonicalCookie>(*cookie),
           std::move(cookie_string),
           CanonicalCookie::CookieInclusionStatus::EXCLUDE_USER_PREFERENCES);
       continue;
     }
 
     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<CanonicalCookie>(*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<CanonicalCookie> 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();
 }
 

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<CanonicalCookie> cookie,
+                         std::string cookie_string,
                          CanonicalCookie::CookieInclusionStatus status);
   int num_cookie_lines_left_;
-  std::vector<CookieLineWithStatus> 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