From f5fd560ef8cc1ae9c67641808956cefbacfa3169 Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Thu, 31 Oct 2024 17:21:27 +0100 Subject: [PATCH] net: Ensure that origin serialization is consistent (#34081) A recent refactoring (#33531) made a change that resulted in the `Origin` header including the port even when the default port for a scheme was used. This made the serialization different from that used for `rust-url`'s `Origin::ascii_serialization()`, breaking CORS on some sites. This change makes it so that the serialization is consistent again. This change also fixes the visiblity on a few methods in `http_loader.rs` since visibility needs to be adjusted for testing anyway. Signed-off-by: Martin Robinson --- components/net/http_loader.rs | 26 +++++++++++++++++++------- components/net/tests/http_loader.rs | 24 +++++++++++++++++++++++- 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 53dba14a0ce..2ebf2bdc738 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -125,7 +125,7 @@ impl Default for HttpState { } /// Step 13 of . -pub fn set_default_accept(request: &mut Request) { +pub(crate) fn set_default_accept(request: &mut Request) { if request.headers.contains_key(header::ACCEPT) { return; } @@ -330,7 +330,7 @@ pub fn determine_requests_referrer( } } -pub fn set_request_cookies( +fn set_request_cookies( url: &ServoUrl, headers: &mut HeaderMap, cookie_jar: &RwLock, @@ -476,7 +476,7 @@ enum BodySink { } impl BodySink { - pub fn transmit_bytes(&self, bytes: Vec) { + fn transmit_bytes(&self, bytes: Vec) { match self { BodySink::Chunked(ref sender) => { let sender = sender.clone(); @@ -490,7 +490,7 @@ impl BodySink { } } - pub fn close(&self) { + fn close(&self) { match self { BodySink::Chunked(_) => { /* no need to close sender */ }, BodySink::Buffered(ref sender) => { @@ -2264,7 +2264,7 @@ fn is_no_store_cache(headers: &HeaderMap) -> bool { } /// -pub fn is_redirect_status(status: StatusCode) -> bool { +fn is_redirect_status(status: StatusCode) -> bool { matches!( status, StatusCode::MOVED_PERMANENTLY | @@ -2320,18 +2320,30 @@ fn serialize_request_origin(request: &Request) -> headers::Origin { } // Step 3. Return request’s origin, serialized. + serialize_origin(origin) +} + +/// Step 3 of . +pub fn serialize_origin(origin: &ImmutableOrigin) -> headers::Origin { match origin { ImmutableOrigin::Opaque(_) => headers::Origin::NULL, ImmutableOrigin::Tuple(scheme, host, port) => { + // Note: This must be kept in sync with `Origin::ascii_serialization()`, which does not + // use the port number when a default port is used. + let port = match (scheme.as_ref(), port) { + ("http" | "ws", 80) | ("https" | "wss", 443) | ("ftp", 21) => None, + _ => Some(*port), + }; + // TODO: Ensure that hyper/servo don't disagree about valid origin headers - headers::Origin::try_from_parts(scheme, &host.to_string(), *port) + headers::Origin::try_from_parts(scheme, &host.to_string(), port) .unwrap_or(headers::Origin::NULL) }, } } /// -pub fn append_a_request_origin_header(request: &mut Request) { +fn append_a_request_origin_header(request: &mut Request) { // Step 1. Assert: request’s origin is not "client". let Origin::Origin(request_origin) = &request.origin else { panic!("origin cannot be \"client\" at this point in time"); diff --git a/components/net/tests/http_loader.rs b/components/net/tests/http_loader.rs index d19febcbd7e..68c8868428a 100644 --- a/components/net/tests/http_loader.rs +++ b/components/net/tests/http_loader.rs @@ -33,7 +33,7 @@ use ipc_channel::router::ROUTER; use net::cookie::ServoCookie; use net::cookie_storage::CookieStorage; use net::fetch::methods::{self}; -use net::http_loader::determine_requests_referrer; +use net::http_loader::{determine_requests_referrer, serialize_origin}; use net::resource_thread::AuthCacheEntry; use net::test::{replace_host_table, DECODER_BUFFER_SIZE}; use net_traits::http_status::HttpStatus; @@ -45,6 +45,7 @@ use net_traits::response::{Response, ResponseBody}; use net_traits::{CookieSource, FetchTaskTarget, NetworkError, ReferrerPolicy}; use servo_url::{ImmutableOrigin, ServoUrl}; use tokio_test::block_on; +use url::Url; use crate::{fetch, fetch_with_context, make_server, new_fetch_context}; @@ -1457,3 +1458,24 @@ fn test_fetch_compressed_response_update_count() { (DATA_DECOMPRESSED_LEN + DECODER_BUFFER_SIZE - 1) / DECODER_BUFFER_SIZE; assert_eq!(response_update_count, EXPECTED_UPDATE_COUNT); } + +#[test] +fn test_origin_serialization_compatability() { + let ensure_serialiations_match = |url_string| { + let url = Url::parse(url_string).unwrap(); + let origin = ImmutableOrigin::new(url.origin()); + let serialized = format!("{}", serialize_origin(&origin)); + assert_eq!(serialized, origin.ascii_serialization()); + }; + + ensure_serialiations_match("https://example.com"); + ensure_serialiations_match("https://example.com:443"); + + ensure_serialiations_match("http://example.com"); + ensure_serialiations_match("http://example.com:80"); + + ensure_serialiations_match("https://example.com:1234"); + ensure_serialiations_match("http://example.com:1234"); + + ensure_serialiations_match("data:,dataurltexta"); +}