Refactor and document cookie-list retrieval (#38070)

This refactors some of the cookie retrieval mechanism to be less
repetitive and separates the cookie-list from the cookie-string which
will also be needed for Cookie Store.

Testing: No new behavior, should be covered by existing WPT tests.

Signed-off-by: Sebastian C <sebsebmc@gmail.com>

components/net/cookie.rs

@@ -10,6 +10,7 @@ use std::net::{Ipv4Addr, Ipv6Addr};
 use std::time::SystemTime;
 
 use cookie::Cookie;
+use log::{Level, debug, log_enabled};
 use net_traits::CookieSource;
 use net_traits::pub_domains::is_pub_domain;
 use nom::IResult;
@@ -322,7 +323,21 @@ impl ServoCookie {
 
     /// <http://tools.ietf.org/html/rfc6265#section-5.4> step 1
     pub fn appropriate_for_url(&self, url: &ServoUrl, source: CookieSource) -> bool {
+        if log_enabled!(Level::Debug) {
+            debug!(
+                " === SENT COOKIE : {} {} {:?} {:?}",
+                self.cookie.name(),
+                self.cookie.value(),
+                self.cookie.domain(),
+                self.cookie.path()
+            );
+        }
+
         let domain = url.host_str();
+        // Either: The cookie's host-only-flag is true and the canonicalized host of the
+        // retrieval's URI is identical to the cookie's domain
+        // Or: The cookie's host-only-flag is false and the canonicalized host of the
+        // retrieval's URI domain-matches the cookie's domain
         if self.host_only {
             if self.cookie.domain() != domain {
                 return false;
@@ -333,18 +348,24 @@ impl ServoCookie {
             }
         }
 
+        // The retrieval's URI's path path-matches the cookie's path.
         if let Some(cookie_path) = self.cookie.path() {
             if !ServoCookie::path_match(url.path(), cookie_path) {
                 return false;
             }
         }
 
+        // If the cookie's secure-only-flag is true, then the retrieval's URI must denote a "secure" connection
         if self.cookie.secure().unwrap_or(false) && !url.is_secure_scheme() {
             return false;
         }
+
+        // If the cookie's http-only-flag is true, then exclude the cookie if the retrieval's type is "non-HTTP"
         if self.cookie.http_only().unwrap_or(false) && source == CookieSource::NonHTTP {
             return false;
         }
+        // TODO: Apply same site checks
+        // TOOD: Apply Partitioning checks
 
         true
     }

components/net/cookie_storage.rs

@@ -10,7 +10,9 @@ use std::collections::HashMap;
 use std::collections::hash_map::Entry;
 use std::time::SystemTime;
 
-use log::{debug, info};
+use cookie::Cookie;
+use itertools::Itertools;
+use log::info;
 use net_traits::CookieSource;
 use net_traits::pub_domains::reg_suffix;
 use serde::{Deserialize, Serialize};
@@ -175,41 +177,26 @@ impl CookieStorage {
 
     // http://tools.ietf.org/html/rfc6265#section-5.4
     pub fn cookies_for_url(&mut self, url: &ServoUrl, source: CookieSource) -> Option<String> {
-        let filterer = |c: &&mut ServoCookie| -> bool {
-            debug!(
-                " === SENT COOKIE : {} {} {:?} {:?}",
-                c.cookie.name(),
-                c.cookie.value(),
-                c.cookie.domain(),
-                c.cookie.path()
-            );
-            debug!(
-                " === SENT COOKIE RESULT {}",
-                c.appropriate_for_url(url, source)
-            );
-            // Step 1
-            c.appropriate_for_url(url, source)
-        };
-        // Step 2
-        let domain = reg_host(url.host_str().unwrap_or(""));
-        let cookies = self.cookies_map.entry(domain).or_default();
+        // Let cookie-list be the set of cookies from the cookie store
+        let cookie_list = self.cookies_data_for_url(url, source);
 
-        let mut url_cookies: Vec<&mut ServoCookie> = cookies.iter_mut().filter(filterer).collect();
-        url_cookies.sort_by(|a, b| CookieStorage::cookie_comparator(a, b));
-
-        let reducer = |acc: String, c: &mut &mut ServoCookie| -> String {
-            // Step 3
-            c.touch();
-
-            // Step 4
+        let reducer = |acc: String, cookie: Cookie<'static>| -> String {
+            // Serialize the cookie-list into a cookie-string by processing each cookie in the cookie-list in order:
+            // If the cookies' name is not empty, output the cookie's name followed by the %x3D ("=") character.
+            // If the cookies' value is not empty, output the cookie's value.
+            // If there is an unprocessed cookie in the cookie-list, output the characters %x3B and %x20 ("; ").
+            // Security: the above steps allow for "nameless" cookies which have proved to be a security footgun
+            // especially with the new cookie name prefix proposals
             (match acc.len() {
                 0 => acc,
                 _ => acc + "; ",
-            }) + c.cookie.name() +
+            }) + cookie.name() +
                 "=" +
-                c.cookie.value()
+                cookie.value()
         };
-        let result = url_cookies.iter_mut().fold("".to_owned(), reducer);
+
+        // Serialize the cookie-list into a cookie-string by processing each cookie in the cookie-list in order
+        let result = cookie_list.fold("".to_owned(), reducer);
 
         info!(" === COOKIES SENT: {}", result);
         match result.len() {
@@ -229,7 +216,12 @@ impl CookieStorage {
         cookies
             .iter_mut()
             .filter(move |c| c.appropriate_for_url(url, source))
+            .sorted_by(|a: &&mut ServoCookie, b: &&mut ServoCookie| {
+                // The user agent SHOULD sort the cookie-list
+                CookieStorage::cookie_comparator(a, b)
+            })
             .map(|c| {
+                // Update the last-access-time of each cookie in the cookie-list to the current date and time
                 c.touch();
                 c.cookie.clone()
             })