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 <mrobinson@igalia.com>
This commit is contained in:
Martin Robinson 2024-10-31 17:21:27 +01:00 committed by GitHub
parent 851b125d4b
commit f5fd560ef8
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 42 additions and 8 deletions

View file

@ -125,7 +125,7 @@ impl Default for HttpState {
}
/// Step 13 of <https://fetch.spec.whatwg.org/#concept-fetch>.
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<CookieStorage>,
@ -476,7 +476,7 @@ enum BodySink {
}
impl BodySink {
pub fn transmit_bytes(&self, bytes: Vec<u8>) {
fn transmit_bytes(&self, bytes: Vec<u8>) {
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 {
}
/// <https://fetch.spec.whatwg.org/#redirect-status>
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 requests origin, serialized.
serialize_origin(origin)
}
/// Step 3 of <https://fetch.spec.whatwg.org/#serializing-a-request-origin>.
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)
},
}
}
/// <https://fetch.spec.whatwg.org/#append-a-request-origin-header>
pub fn append_a_request_origin_header(request: &mut Request) {
fn append_a_request_origin_header(request: &mut Request) {
// Step 1. Assert: requests origin is not "client".
let Origin::Origin(request_origin) = &request.origin else {
panic!("origin cannot be \"client\" at this point in time");

View file

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