diff --git a/moli-fetch/src/blocking/collectors.rs b/moli-fetch/src/blocking/collectors.rs index 843152aa62..ff3c0054fb 100644 --- a/moli-fetch/src/blocking/collectors.rs +++ b/moli-fetch/src/blocking/collectors.rs @@ -235,6 +235,7 @@ pub struct RawStreamingResponseCollector { current_url: Option, current_cookie_context: Option, status: u16, + follow_redirects: bool, max_response_size: Option, response_too_large: bool, response_bytes_received: usize, @@ -633,6 +634,7 @@ impl RawStreamingResponseCollector { current_url: None, current_cookie_context: None, status: 0, + follow_redirects: true, status_text: None, max_response_size: None, response_too_large: false, @@ -658,6 +660,10 @@ impl RawStreamingResponseCollector { } } + pub fn set_follow_redirects(&mut self, follow_redirects: bool) { + self.follow_redirects = follow_redirects; + } + pub fn begin_request( &mut self, max_response_size: Option, @@ -823,7 +829,14 @@ impl RawStreamingResponseCollector { } self.maybe_create_cache_body_writer(); self.maybe_emit_start(); - if response_body_ends_at_headers(self.status) && self.started { + let discard_redirect_body = !self.follow_redirects + && next_redirect_url_from_parts(¤t_url, self.status, &self.headers, 0) + .ok() + .flatten() + .is_some(); + if (response_body_ends_at_headers(self.status) || discard_redirect_body) && self.started { + // Manual raw redirects expose their head and an empty body. The + // browser redirect owner can proceed without waiting for the peer. self.finish_streaming_body(); self.header_terminated = true; return false; @@ -857,10 +870,11 @@ impl RawStreamingResponseCollector { let Some(current_url) = self.current_url.clone() else { return; }; - if next_redirect_url_from_parts(¤t_url, self.status, &self.headers, 0) - .ok() - .flatten() - .is_some() + if self.follow_redirects + && next_redirect_url_from_parts(¤t_url, self.status, &self.headers, 0) + .ok() + .flatten() + .is_some() { return; } diff --git a/moli-fetch/src/blocking/mod.rs b/moli-fetch/src/blocking/mod.rs index 3264b170a4..ff059ec742 100644 --- a/moli-fetch/src/blocking/mod.rs +++ b/moli-fetch/src/blocking/mod.rs @@ -214,7 +214,7 @@ fn outgoing_request_header_bytes_for_url( } if !header_present(&outgoing, "referer") - && let Some(referer) = referrer_header_value_for_request(request, request_url) + && let Some(referer) = request.referrer_header_value(request_url) { outgoing.push(("Referer".to_owned(), referer.into_bytes())); } @@ -535,28 +535,6 @@ fn request_sec_fetch_site(request: &Request, request_url: &Url) -> String { } } -fn referrer_header_value_for_request(request: &Request, request_url: &Url) -> Option { - if !request.infers_referrer_from_initiator() { - return None; - } - let referrer_url = request.cookie_context.initiator_url.as_ref()?; - let (referrer_policy, document_referrer_policy) = request - .subresource_request_metadata() - .map(|metadata| { - ( - metadata.referrer_policy.as_deref(), - metadata.document_referrer_policy.as_deref(), - ) - }) - .unwrap_or((None, None)); - crate::referrer_header_value( - referrer_url, - request_url, - referrer_policy, - document_referrer_policy, - ) -} - pub(crate) fn store_response_cookies( cookie_store: &SharedBrowserCookieStore, response_url: &Url, diff --git a/moli-fetch/src/cancellation.rs b/moli-fetch/src/cancellation.rs index 8978590d26..1cd41e959e 100644 --- a/moli-fetch/src/cancellation.rs +++ b/moli-fetch/src/cancellation.rs @@ -3,11 +3,19 @@ use std::sync::{ atomic::{AtomicBool, Ordering}, }; +use parking_lot::Mutex; + +#[derive(Debug, Default)] +struct ResponseProgress { + declared_body_complete: AtomicBool, + terminal: AtomicBool, +} + #[derive(Debug, Default)] struct FetchLifecycleState { cancel_requested: AtomicBool, - declared_body_complete: AtomicBool, - terminal: AtomicBool, + parent: Option>, + response_progress: Mutex>, } #[derive(Debug, Clone, Default)] @@ -25,31 +33,67 @@ impl FetchCancelHandle { } pub fn is_cancelled(&self) -> bool { - self.state.cancel_requested.load(Ordering::SeqCst) + let mut state = self.state.as_ref(); + loop { + if state.cancel_requested.load(Ordering::SeqCst) { + return true; + } + match state.parent.as_deref() { + Some(parent) => state = parent, + None => return false, + } + } + } + + /// Creates cancellation authority for one preflight or redirect hop. + /// Parent cancellation propagates to this child; cancelling the child does + /// not cancel its parent or siblings. Completion facts remain independent. + pub fn child_for_subrequest(&self) -> Self { + Self { + state: Arc::new(FetchLifecycleState { + parent: Some(self.state.clone()), + ..FetchLifecycleState::default() + }), + } + } + + /// Selects the accepted final response's transport progress. This shares + /// already recorded and future completion facts, without retaining the + /// child's cancellation state (which itself retains the parent). + pub fn adopt_response_progress(&self, response: &Self) { + let progress = response.state.response_progress.lock().clone(); + *self.state.response_progress.lock() = progress; } /// Returns whether transport facts already determine this response's /// terminal result, so a later consumer cancellation must not replace it. pub fn response_completion_is_committed(&self) -> bool { - self.state.declared_body_complete.load(Ordering::Acquire) - || self.state.terminal.load(Ordering::Acquire) + let progress = self.state.response_progress.lock(); + progress.declared_body_complete.load(Ordering::Acquire) + || progress.terminal.load(Ordering::Acquire) } - pub(crate) fn reset_response_progress(&self) { - self.state - .declared_body_complete - .store(false, Ordering::Release); - self.state.terminal.store(false, Ordering::Release); + /// Starts a new logical transfer without clearing cancellation. Detaching + /// the previous progress also prevents an old hop's late completion from + /// committing this new transfer. + pub fn reset_response_progress(&self) { + *self.state.response_progress.lock() = Arc::default(); } pub(crate) fn mark_declared_response_body_complete(&self) { self.state + .response_progress + .lock() .declared_body_complete .store(true, Ordering::Release); } pub(crate) fn mark_response_terminal(&self) { - self.state.terminal.store(true, Ordering::Release); + self.state + .response_progress + .lock() + .terminal + .store(true, Ordering::Release); } } @@ -72,4 +116,52 @@ mod tests { handle.mark_response_terminal(); assert!(observer.response_completion_is_committed()); } + + #[test] + fn subrequest_cancellation_is_isolated_and_inherits_parent_abort() { + let parent = FetchCancelHandle::new(); + let first = parent.child_for_subrequest(); + let second = parent.child_for_subrequest(); + first.cancel(); + assert!(first.is_cancelled()); + assert!(!parent.is_cancelled()); + assert!(!second.is_cancelled()); + parent.cancel(); + assert!(second.is_cancelled()); + assert!(second.child_for_subrequest().is_cancelled()); + } + + #[test] + fn only_selected_response_progress_commits_the_parent() { + for complete_before_selection in [false, true] { + for declared_body in [false, true] { + let parent = FetchCancelHandle::new(); + let discarded = parent.child_for_subrequest(); + discarded.mark_response_terminal(); + assert!(!parent.response_completion_is_committed()); + let response = parent.child_for_subrequest(); + let complete = || { + if declared_body { + response.mark_declared_response_body_complete(); + } else { + response.mark_response_terminal(); + } + }; + if complete_before_selection { + complete(); + } + parent.adopt_response_progress(&response); + assert_eq!( + parent.response_completion_is_committed(), + complete_before_selection + ); + complete(); + assert!(parent.response_completion_is_committed()); + parent.reset_response_progress(); + response.mark_response_terminal(); + assert!(!parent.response_completion_is_committed()); + assert!(!parent.is_cancelled()); + } + } + } } diff --git a/moli-fetch/src/cors.rs b/moli-fetch/src/cors.rs index 07d41630e1..1728cf89cb 100644 --- a/moli-fetch/src/cors.rs +++ b/moli-fetch/src/cors.rs @@ -14,7 +14,8 @@ pub fn validate_cors_response_for_origin( response_headers: &[(String, String)], credentials_mode: RequestCredentialsMode, ) -> Result<(), String> { - let Some(allow_origin) = combined_cors_response_header_value(response_headers, "access-control-allow-origin") + let Some(allow_origin) = + combined_cors_response_header_value(response_headers, "access-control-allow-origin") else { return Err(format!( "CORS check failed: no Access-Control-Allow-Origin for {origin}" @@ -36,8 +37,10 @@ pub fn validate_cors_response_for_origin( } if credentials_mode == RequestCredentialsMode::Include { - let allow_credentials = - combined_cors_response_header_value(response_headers, "access-control-allow-credentials"); + let allow_credentials = combined_cors_response_header_value( + response_headers, + "access-control-allow-credentials", + ); if allow_credentials .as_deref() .is_none_or(|value| value.trim() != "true") diff --git a/moli-fetch/src/request.rs b/moli-fetch/src/request.rs index 05e423a157..b814f35e7a 100644 --- a/moli-fetch/src/request.rs +++ b/moli-fetch/src/request.rs @@ -41,6 +41,7 @@ pub struct Request { browser_navigation_kind: BrowserNavigationRequestKind, infer_referrer_from_initiator: bool, context: RequestContext, + referrer_url: Option, pub use_page_network_policy: bool, pub follow_redirects: bool, pub request_mode: RequestMode, @@ -441,6 +442,7 @@ impl Request { browser_navigation_kind: BrowserNavigationRequestKind::Navigate, infer_referrer_from_initiator: true, context: RequestContext::Http, + referrer_url: None, use_page_network_policy: false, follow_redirects: true, request_mode: RequestMode::Navigate, @@ -475,6 +477,7 @@ impl Request { browser_navigation_kind: BrowserNavigationRequestKind::Navigate, infer_referrer_from_initiator: true, context: RequestContext::Http, + referrer_url: None, use_page_network_policy: false, follow_redirects: true, request_mode: RequestMode::Navigate, @@ -542,6 +545,7 @@ impl Request { browser_navigation_kind: BrowserNavigationRequestKind::Navigate, infer_referrer_from_initiator: true, context: RequestContext::Http, + referrer_url: None, use_page_network_policy: false, follow_redirects: true, request_mode: RequestMode::Cors, @@ -710,6 +714,39 @@ impl Request { self.infer_referrer_from_initiator } + /// Computes the inferred Referer header without changing the initiator + /// used for cookie-site and request-origin decisions. + pub fn referrer_header_value(&self, request_url: &Url) -> Option { + if !self.infers_referrer_from_initiator() { + return None; + } + let referrer_url = self + .referrer_url + .as_ref() + .or(self.cookie_context.initiator_url.as_ref())?; + let (policy, document_policy) = self + .subresource_request_metadata() + .map(|metadata| { + ( + metadata.referrer_policy.as_deref(), + metadata.document_referrer_policy.as_deref(), + ) + }) + .unwrap_or((None, None)); + crate::referrer_header_value(referrer_url, request_url, policy, document_policy) + } + + /// Retains the referrer already selected for this hop. A later, more + /// permissive policy cannot restore an omitted referrer or its stripped path. + pub fn update_referrer_for_redirect(&mut self, response_url: &Url) { + self.referrer_url = self + .referrer_header_value(response_url) + .and_then(|value| Url::parse(&value).ok()); + if self.referrer_url.is_none() { + self.infer_referrer_from_initiator = false; + } + } + pub fn with_page_network_policy(mut self) -> Self { self.use_page_network_policy = true; self @@ -834,6 +871,11 @@ impl Request { &self.redirect_chain } + /// Updates observations attached to already followed redirects. + pub fn redirect_chain_mut(&mut self) -> &mut [RedirectInfo] { + &mut self.redirect_chain + } + /// Records a followed redirect. The caller controls method and current URL /// updates so this also supports transports that retain the original URL. pub fn record_redirect(&mut self, redirect: RedirectInfo) { diff --git a/moli-fetch/src/runtime.rs b/moli-fetch/src/runtime.rs index 15f9f7c579..a6dfbcdede 100644 --- a/moli-fetch/src/runtime.rs +++ b/moli-fetch/src/runtime.rs @@ -1331,6 +1331,7 @@ impl RuntimeOwner { .get_mut() .raw_streaming_mut() .expect("raw streaming request should use raw streaming collector"); + collector.set_follow_redirects(job.request.follow_redirects); collector.begin_request_with_cache_plan( self.config.http_max_response_size(), job.current_url.clone(), diff --git a/moli-renderer-v8/src/network/backend/memory_cache.rs b/moli-renderer-v8/src/network/backend/memory_cache.rs index 04b96dec62..db552c4302 100644 --- a/moli-renderer-v8/src/network/backend/memory_cache.rs +++ b/moli-renderer-v8/src/network/backend/memory_cache.rs @@ -62,6 +62,7 @@ pub(in crate::network) struct RawSubresourceCacheKey { page_cache_partition_id: u64, url: String, resource_type: &'static str, + follow_redirects: bool, credentials_mode: String, request_origin: Option, request_mode: String, @@ -635,6 +636,7 @@ pub(in crate::network) fn raw_subresource_memory_cache_key( page_cache_partition_id: 0, url: request.url.as_str().to_owned(), resource_type: raw_subresource_cache_resource_type_key(request.resource_type), + follow_redirects: request.follow_redirects, credentials_mode: request.credentials_mode.as_ref().to_owned(), request_origin: request .request_origin() @@ -668,7 +670,10 @@ fn raw_subresource_request_is_memory_cacheable(request: &Request) -> bool { && request.method.eq_ignore_ascii_case("GET") && request.body.is_none() && request.auth().is_none() - && request.follow_redirects + // The browser's CORS loop follows redirects itself. Its individual + // transport requests can still reuse cached responses with an empty + // URL list, in a separate bucket from automatically followed fetches. + && request.redirect_mode == moli_fetch::RequestRedirectMode::Follow && raw_subresource_memory_cacheable_headers(request) } @@ -870,6 +875,47 @@ mod tests { load } + #[test] + fn per_hop_cache_keeps_transport_redirects_and_client_origins_separate() { + let initiator = Url::parse("https://cache.test/page").unwrap(); + let automatic = Request::get("https://cache.test/resource") + .unwrap() + .with_resource_type(RequestResourceType::Raw) + .with_browser_request_metadata(BrowserRequestMetadata::Fetch) + .with_request_origin((&initiator).into()) + .with_initiator_url(&initiator); + let per_hop = automatic.clone().with_follow_redirects(false); + let key = raw_subresource_memory_cache_key(&per_hop).expect("cacheable first CORS hop"); + let mut cache = SharedMemoryResourceCache::with_limits(usize::MAX, usize::MAX); + cache.insert_raw_subresource( + key.clone(), + raw_response(per_hop.url.as_str(), b"cached"), + u64::MAX, + ); + assert!(cache.lookup_raw_subresource(&key).is_some()); + for isolated in [ + automatic, + per_hop + .clone() + .with_request_origin(moli_url::WebOrigin::Opaque), + ] { + let isolated_key = raw_subresource_memory_cache_key(&isolated).unwrap(); + assert!( + cache.lookup_raw_subresource(&isolated_key).is_none(), + "a cached hop cannot replace an automatic redirect or another client's response" + ); + } + for mode in [ + moli_fetch::RequestRedirectMode::Manual, + moli_fetch::RequestRedirectMode::Error, + ] { + assert!( + raw_subresource_memory_cache_key(&per_hop.clone().with_redirect_mode(mode)) + .is_none() + ); + } + } + #[test] fn cancelling_one_shared_script_consumer_preserves_its_sibling() { let load = ScriptTextLoad::pending(); diff --git a/moli-renderer-v8/src/network_host.rs b/moli-renderer-v8/src/network_host.rs index 05dfa3549a..6a83507d5a 100644 --- a/moli-renderer-v8/src/network_host.rs +++ b/moli-renderer-v8/src/network_host.rs @@ -31,7 +31,7 @@ pub(in crate::network_host) use self::blob_url_entry::BLOB_URL_ENTRY_SLOT; pub(crate) use self::blob_url_entry::{CapturedBlobUrl, blob_url_entry, set_blob_url_entry}; pub(crate) use self::async_fetch::{ - browser_request_needs_manual_preflight_redirects, collect_image_response_into_parkable, + collect_image_response_into_parkable, fetch_browser_subresource_raw_stream_with_preflight_headers_and_network_metadata, fetch_browser_subresource_with_preflight_and_network_metadata, fetch_browser_subresource_with_preflight_headers, diff --git a/moli-renderer-v8/src/network_host/async_fetch.rs b/moli-renderer-v8/src/network_host/async_fetch.rs index cd139ff787..7e295a3e14 100644 --- a/moli-renderer-v8/src/network_host/async_fetch.rs +++ b/moli-renderer-v8/src/network_host/async_fetch.rs @@ -1,12 +1,16 @@ use super::*; use moli_fetch::{ - BrowserRequestMetadata, FetchCancelHandle, NetworkFetchResult, RedirectInfo, - RequestCredentialsMode, RequestMode, RequestRedirectMode, ResponseHead, StreamingRawResponse, - is_cors_safelisted_method, + BrowserRequestMetadata, FetchCancelHandle, NetworkFetchResult, NetworkObservationJournal, + RedirectInfo, RequestCredentialsMode, RequestMode, RequestRedirectMode, ResponseHead, + StreamingRawResponse, }; const MAX_MANUAL_CORS_REDIRECTS: usize = 20; +#[cfg(test)] +#[path = "async_fetch/redirect_tests.rs"] +mod redirect_tests; + /// Script fetches need CORS authorization before following every redirect, /// even though their browser-generated GET requests do not require preflight. pub(crate) async fn fetch_cors_script_text( @@ -14,7 +18,7 @@ pub(crate) async fn fetch_cors_script_text( request: Request, cancel_handle: FetchCancelHandle, ) -> Result { - let observed = fetch_browser_subresource_raw_stream_with_manual_preflight_redirects( + let observed = fetch_browser_subresource_raw_stream_with_cors_redirect_checks( loader, request, Some(cancel_handle), @@ -95,8 +99,8 @@ async fn fetch_browser_subresource_with_preflight_headers_and_observer( preflight_request_headers: Vec<(String, String)>, preflight_observer: Option<&CorsPreflightNetworkObserver>, ) -> Result, String> { - if browser_request_needs_manual_preflight_redirects(&request, &preflight_request_headers) { - return fetch_browser_subresource_with_manual_preflight_redirects( + if browser_request_needs_cors_redirect_checks(&request) { + return fetch_browser_subresource_with_cors_redirect_checks( loader, request, cancel_handle, @@ -116,10 +120,7 @@ async fn fetch_browser_subresource_with_preflight_headers_and_observer( fetch_once_with_network_metadata(&loader, request, cancel_handle).await } -pub(crate) fn browser_request_needs_manual_preflight_redirects( - request: &Request, - preflight_request_headers: &[(String, String)], -) -> bool { +fn browser_request_needs_cors_redirect_checks(request: &Request) -> bool { matches!( request.browser_request_metadata(), Some( @@ -127,14 +128,12 @@ pub(crate) fn browser_request_needs_manual_preflight_redirects( | BrowserRequestMetadata::EventSource | BrowserRequestMetadata::JsonModule | BrowserRequestMetadata::Manifest + | BrowserRequestMetadata::Script | BrowserRequestMetadata::StyleModule | BrowserRequestMetadata::Xhr, ) ) && request.request_mode == RequestMode::Cors - && request.cookie_context.initiator_url.is_some() - && (request.use_cors_preflight() - || !is_cors_safelisted_method(&request.method) - || !moli_fetch::cors_unsafe_request_header_names(preflight_request_headers).is_empty()) + && request.request_origin().is_some() } #[derive(Clone, Debug, Eq, PartialEq)] @@ -146,12 +145,14 @@ enum ManualCorsRedirectTransition { struct ManualCorsRedirectState { request: Request, + original_url: url::Url, preflight_request_headers: Vec<(String, String)>, } impl ManualCorsRedirectState { fn new(request: Request, preflight_request_headers: Vec<(String, String)>) -> Self { Self { + original_url: request.url.clone(), request, preflight_request_headers, } @@ -175,6 +176,11 @@ impl ManualCorsRedirectState { cancel_handle: Option, preflight_observer: Option<&CorsPreflightNetworkObserver>, ) -> Result<(), String> { + if self.request.request_mode == RequestMode::SameOrigin + && self.request.has_cross_origin_url(&self.request.url) + { + return Err("same-origin request mode blocked a cross-origin URL".to_owned()); + } run_cors_preflight_if_needed( loader, self.request(), @@ -189,18 +195,31 @@ impl ManualCorsRedirectState { &mut self, head: ResponseHead, network_extra_info_available: bool, + request_extra_info: Option<&moli_fetch::NetworkRequestExtraInfo>, ) -> Result { - // Keep redirect-mode error precedence aligned with Fetch: an error-mode - // redirect is rejected before the redirect response is CORS-checked. - if self.request.redirect_mode == RequestRedirectMode::Error { - if next_redirect_url(&head.final_url, head.status, &head.headers, 0)?.is_some() { - return Err(redirect_mode_error_message(&head.final_url)); - } - validate_actual_cors_response_head(&self.request, &head)?; + // Redirect request records describe the next hop's cookie decision, + // just as they do when the transport follows redirects internally. + if let Some(previous) = self.request.redirect_chain_mut().last_mut() + && previous.source == moli_fetch::RedirectSource::Network + { + previous.request_cookie_report = head.request_cookie_report.clone(); + previous.request_extra_info = request_extra_info.cloned(); + } + validate_actual_cors_response_head(&self.request, &head)?; + if !is_redirect_status(head.status) { return Ok(ManualCorsRedirectTransition::FinalResponse); } - - validate_actual_cors_response_head(&self.request, &head)?; + // Fetch selects manual/error handling from the status before parsing + // Location. A missing or invalid Location must not change those modes. + match self.request.redirect_mode { + RequestRedirectMode::Error => return Err(redirect_mode_error_message(&head.final_url)), + RequestRedirectMode::Manual => { + return Ok(ManualCorsRedirectTransition::ManualResponse { + response_url: head.final_url, + }); + } + RequestRedirectMode::Follow => {} + } let Some(next_url) = next_redirect_url( &head.final_url, head.status, @@ -211,25 +230,17 @@ impl ManualCorsRedirectState { return Ok(ManualCorsRedirectTransition::FinalResponse); }; - if self.request.redirect_mode == RequestRedirectMode::Manual { - return Ok(ManualCorsRedirectTransition::ManualResponse { - response_url: head.final_url, - }); - } - - match self.request.redirect_mode { - RequestRedirectMode::Follow => {} - RequestRedirectMode::Error | RequestRedirectMode::Manual => { - unreachable!("redirect modes were handled before the follow transition") - } - } if !matches!(next_url.scheme(), "http" | "https") { return Err(format!("CORS redirect requires an HTTP(S) URL: {next_url}")); } if self.request.request_mode == RequestMode::Cors - && (!next_url.username().is_empty() || next_url.password().is_some()) + && (!next_url.username().is_empty() + || next_url + .password() + .is_some_and(|password| !password.is_empty())) && self.request.request_origin().is_some_and(|origin| { - !origin.same_origin_url(&head.final_url) || !origin.same_origin_url(&next_url) + self.request.has_cross_origin_url(&head.final_url) + || !origin.same_origin_url(&next_url) }) { return Err("CORS redirect URL must not include credentials".to_owned()); @@ -246,6 +257,7 @@ impl ManualCorsRedirectState { self.request.set_auth(None); } } + self.request.update_referrer_for_redirect(&head.final_url); if let Some(policy) = crate::referrer_policy::response_referrer_policy_from_headers(&head.headers) { @@ -261,6 +273,19 @@ impl ManualCorsRedirectState { .with_subresource_request_metadata(metadata); } let redirect_status = head.status; + self.request.cookie_context = moli_cookie_jar::advance_cookie_request_context( + self.request.cookie_context.clone(), + &self.original_url, + &next_url, + ); + let response_extra_info = request_extra_info.cloned().map(|request_extra_info| { + moli_fetch::NetworkResponseExtraInfo { + request_extra_info, + status: redirect_status, + headers: head.headers.clone(), + cookie_set_reports: head.cookie_set_reports.clone(), + } + }); // Keep the original client origin for Fetch Metadata. Recording the hop // makes Request origin serialization and CORS validation apply redirect taint. self.request.record_redirect(RedirectInfo { @@ -271,7 +296,7 @@ impl ManualCorsRedirectState { headers: head.headers, network_extra_info_available, request_extra_info: None, - response_extra_info: None, + response_extra_info, redirect_has_extra_info: network_extra_info_available, request_cookie_report: head.request_cookie_report, cookie_set_reports: head.cookie_set_reports, @@ -296,41 +321,84 @@ impl ManualCorsRedirectState { } } -async fn fetch_browser_subresource_with_manual_preflight_redirects( +async fn fetch_browser_subresource_with_cors_redirect_checks( loader: ResourceRequestClient, request: Request, cancel_handle: Option, preflight_request_headers: Vec<(String, String)>, preflight_observer: Option<&CorsPreflightNetworkObserver>, ) -> Result, String> { + if !request.auth_requires_buffered_transport() { + let redirect_mode = request.redirect_mode; + let observed = fetch_browser_subresource_raw_stream_with_cors_redirect_checks( + &loader, + request, + cancel_handle, + preflight_request_headers, + preflight_observer, + ) + .await?; + let (response, journal) = observed.into_parts_with_observation_journal(); + let response = if redirect_mode == RequestRedirectMode::Manual + && is_redirect_status(response.status) + { + // The opaque redirect has no exposed body. Dropping its private + // transfer must not delay delivery until an unused body ends. + Response::from_head_and_text_body(response.head(), String::new()) + .with_network_request_extra_info(response.network_request_extra_info().cloned()) + } else { + response + .into_lossy_materialized_text_response() + .await + .map_err(format_network_error)? + }; + return Ok(NetworkFetchResult::with_observation_journal( + response, journal, + )); + } + // Challenge-response auth still uses libcurl's buffered collector, which + // handles intermediate 401/407 responses before exposing this hop. let mut redirects = ManualCorsRedirectState::new(request, preflight_request_headers); + let cancel_handle = cancel_handle.unwrap_or_default(); + cancel_handle.reset_response_progress(); + let mut journal = NetworkObservationJournal::default(); loop { redirects - .run_current_hop_preflight(&loader, cancel_handle.clone(), preflight_observer) + .run_current_hop_preflight(&loader, Some(cancel_handle.clone()), preflight_observer) .await?; - let mut observed = fetch_once_with_network_metadata_unvalidated( + let hop_cancel = cancel_handle.child_for_subrequest(); + let observed = fetch_once_with_network_metadata_unvalidated( &loader, redirects.hop_request(), - cancel_handle.clone(), + Some(hop_cancel.clone()), ) .await?; let network_extra_info_available = observed.request_observation().is_some(); - match redirects.advance(observed.response().head(), network_extra_info_available)? { - ManualCorsRedirectTransition::FinalResponse => { + let (mut response, hop_journal) = observed.into_parts_with_observation_journal(); + journal.append(hop_journal); + match redirects.advance( + response.head(), + network_extra_info_available, + response.network_request_extra_info(), + )? { + ManualCorsRedirectTransition::FinalResponse + | ManualCorsRedirectTransition::ManualResponse { .. } => { let redirect_chain = redirects.into_redirect_chain(); - observed.response_mut().redirected = !redirect_chain.is_empty(); - observed.response_mut().redirect_chain = redirect_chain; - return Ok(observed); + response.redirected = !redirect_chain.is_empty(); + response.redirect_chain = redirect_chain; + cancel_handle.adopt_response_progress(&hop_cancel); + return Ok(NetworkFetchResult::with_observation_journal( + response, journal, + )); } - ManualCorsRedirectTransition::ManualResponse { .. } => return Ok(observed), ManualCorsRedirectTransition::FollowedRedirect => {} } } } -async fn fetch_browser_subresource_raw_stream_with_manual_preflight_redirects( +async fn fetch_browser_subresource_raw_stream_with_cors_redirect_checks( loader: &ResourceRequestClient, request: Request, cancel_handle: Option, @@ -338,45 +406,48 @@ async fn fetch_browser_subresource_raw_stream_with_manual_preflight_redirects( preflight_observer: Option<&CorsPreflightNetworkObserver>, ) -> Result, String> { let mut redirects = ManualCorsRedirectState::new(request, preflight_request_headers); + let cancel_handle = cancel_handle.unwrap_or_default(); + cancel_handle.reset_response_progress(); + let mut journal = NetworkObservationJournal::default(); loop { redirects - .run_current_hop_preflight(loader, cancel_handle.clone(), preflight_observer) + .run_current_hop_preflight(loader, Some(cancel_handle.clone()), preflight_observer) .await?; - let mut observed = loader + let hop_cancel = cancel_handle.child_for_subrequest(); + let observed = loader .fetch_raw_stream_with_cancel_and_network_metadata( redirects.hop_request(), - cancel_handle.clone().unwrap_or_default(), + hop_cancel.clone(), ) .await .map_err(format_network_error)?; let network_extra_info_available = observed.request_observation().is_some(); - let head = observed.response().head(); - match redirects.advance(head, network_extra_info_available)? { - ManualCorsRedirectTransition::FinalResponse => { + let (mut response, hop_journal) = observed.into_parts_with_observation_journal(); + journal.append(hop_journal); + match redirects.advance( + response.head(), + network_extra_info_available, + response.network_request_extra_info(), + )? { + ManualCorsRedirectTransition::FinalResponse + | ManualCorsRedirectTransition::ManualResponse { .. } => { let redirect_chain = redirects.into_redirect_chain(); - observed.response_mut().redirected = !redirect_chain.is_empty(); - observed.response_mut().redirect_chain = redirect_chain; - return Ok(observed); - } - ManualCorsRedirectTransition::ManualResponse { response_url } => { - return Err(format!( - "manual redirect unexpectedly entered follow-mode streaming from {}", - response_url + response.redirected = !redirect_chain.is_empty(); + response.redirect_chain = redirect_chain; + cancel_handle.adopt_response_progress(&hop_cancel); + return Ok(NetworkFetchResult::with_observation_journal( + response, journal, )); } ManualCorsRedirectTransition::FollowedRedirect => {} } - // Redirect bodies are not exposed to Fetch/XHR. Finish this hop before - // reusing the logical request's cancel handle for the redirected hop; - // the final non-redirect response remains headers-first and streaming. - observed - .response_mut() - .finish() - .await - .map_err(format_network_error)?; + // The redirect body is unused. Cancel this hop without cancelling the + // logical request or waiting for a body that might never finish. + hop_cancel.cancel(); + drop(response); } } @@ -393,7 +464,7 @@ fn next_redirect_url( headers: &[(String, String)], redirect_count: usize, ) -> Result, String> { - if !matches!(status, 301 | 302 | 303 | 307 | 308) { + if !is_redirect_status(status) { return Ok(None); } let Some(location) = headers @@ -443,6 +514,16 @@ async fn fetch_browser_subresource_raw_stream_with_preflight_headers_and_observe preflight_request_headers: Vec<(String, String)>, preflight_observer: Option<&CorsPreflightNetworkObserver>, ) -> Result, String> { + if browser_request_needs_cors_redirect_checks(&request) { + return fetch_browser_subresource_raw_stream_with_cors_redirect_checks( + loader, + request, + cancel_handle, + preflight_request_headers, + preflight_observer, + ) + .await; + } // Borrow the loader so its fetch runtime stays alive until the caller drains // and finishes the returned StreamingRawResponse. run_cors_preflight_if_needed( @@ -600,8 +681,7 @@ pub(crate) fn spawn_async_subresource_fetch( let preflight_observer = CorsPreflightNetworkObserver::new(completion_tx.clone(), network_context); let auth_requires_buffered_transport = request.auth_requires_buffered_transport(); - let requires_manual_preflight_redirects = - browser_request_needs_manual_preflight_redirects(&request, &preflight_request_headers); + let requires_cors_redirect_checks = browser_request_needs_cors_redirect_checks(&request); let can_stream_subresource_body = matches!( request.browser_request_metadata(), Some( @@ -624,7 +704,7 @@ pub(crate) fn spawn_async_subresource_fetch( redirect_mode = ?request.redirect_mode, follow_redirects = request.follow_redirects, auth_requires_buffered_transport, - requires_manual_preflight_redirects, + requires_cors_redirect_checks, can_stream_subresource_body, can_collect_image_body, stage = "async_subresource_transport_selected", @@ -802,27 +882,14 @@ async fn fetch_browser_subresource_streaming_with_preflight_headers( request_body: Option, ) -> Result<(), String> { let body_source_id = new_network_body_source_id(); - let requires_manual_preflight_redirects = - browser_request_needs_manual_preflight_redirects(&request, &preflight_request_headers); - let observed = if requires_manual_preflight_redirects { - fetch_browser_subresource_raw_stream_with_manual_preflight_redirects( - &loader, - request, - cancel_handle, - preflight_request_headers, - preflight_observer, - ) - .await? - } else { - fetch_browser_subresource_raw_stream_with_preflight_headers_and_observer( - &loader, - request, - cancel_handle, - preflight_request_headers, - preflight_observer, - ) - .await? - }; + let observed = fetch_browser_subresource_raw_stream_with_preflight_headers_and_observer( + &loader, + request, + cancel_handle, + preflight_request_headers, + preflight_observer, + ) + .await?; let (mut response, request_observation) = observed.into_parts(); let head = response.head(); let _ = completion_tx.send_async_subresource_event( @@ -905,35 +972,30 @@ async fn fetch_response_head_once( request: Request, cancel_handle: Option, ) -> Result { + let cancel_handle = cancel_handle.unwrap_or_default().child_for_subrequest(); // Challenge-response auth retries are completed inside libcurl on the // buffered path. Preemptive Basic auth can still use the streaming head // path because credentials are already represented as request headers. if request.auth_requires_buffered_transport() { - return fetch_once(loader, request, cancel_handle) + return fetch_once(loader, request, Some(cancel_handle)) .await .map(|response| response.head()); } - let cancel_handle = cancel_handle.unwrap_or_default(); - let mut response = loader + let response = loader .fetch_raw_stream_with_cancel(request, cancel_handle) .await .map_err(format_network_error)?; - let head = response.head(); - response.finish().await.map_err(format_network_error)?; - Ok(head) + // Preflight authorization depends only on the response head. Drop this + // private transfer instead of draining an unneeded, possibly endless body. + Ok(response.head()) } fn validate_redirect_mode_response( response: Response, redirect_mode: RequestRedirectMode, ) -> Result { - validate_redirect_mode_parts( - &response.final_url, - response.status, - &response.headers, - redirect_mode, - )?; + validate_redirect_mode_parts(&response.final_url, response.status, redirect_mode)?; Ok(response) } @@ -941,19 +1003,18 @@ fn validate_redirect_mode_response_head( head: &ResponseHead, redirect_mode: RequestRedirectMode, ) -> Result<(), String> { - validate_redirect_mode_parts(&head.final_url, head.status, &head.headers, redirect_mode) + validate_redirect_mode_parts(&head.final_url, head.status, redirect_mode) } fn validate_redirect_mode_parts( final_url: &url::Url, status: u16, - headers: &[(String, String)], redirect_mode: RequestRedirectMode, ) -> Result<(), String> { if redirect_mode != RequestRedirectMode::Error { return Ok(()); } - if next_redirect_url(final_url, status, headers, 0)?.is_some() { + if is_redirect_status(status) { return Err(redirect_mode_error_message(final_url)); } Ok(()) @@ -963,6 +1024,10 @@ fn redirect_mode_error_message(final_url: &url::Url) -> String { format!("redirect mode error blocked redirect from {final_url}") } +fn is_redirect_status(status: u16) -> bool { + matches!(status, 301 | 302 | 303 | 307 | 308) +} + #[cfg(test)] mod tests { use super::*; @@ -1032,6 +1097,7 @@ mod tests { negotiated_http_version: None, }, true, + None, ) .map_err(anyhow::Error::msg)?; @@ -1731,10 +1797,7 @@ mod tests { .with_initiator_url(&document_url) .with_request_origin(moli_url::WebOrigin::from_url(&document_url)) .with_browser_request_metadata(BrowserRequestMetadata::Xhr); - assert!(browser_request_needs_manual_preflight_redirects( - &request, - &request_headers, - )); + assert!(browser_request_needs_cors_redirect_checks(&request)); spawn_async_subresource_fetch( crate::network::RendererResourceTaskRunner::from_current_tokio()?, diff --git a/moli-renderer-v8/src/network_host/async_fetch/redirect_tests.rs b/moli-renderer-v8/src/network_host/async_fetch/redirect_tests.rs new file mode 100644 index 0000000000..0ac72a5e9c --- /dev/null +++ b/moli-renderer-v8/src/network_host/async_fetch/redirect_tests.rs @@ -0,0 +1,389 @@ +use super::*; +use anyhow::Result; +use std::time::Duration; +use tokio::{ + io::{AsyncReadExt, AsyncWriteExt}, + net::TcpListener, +}; + +async fn request_head(socket: &mut tokio::net::TcpStream) -> Result { + let mut bytes = Vec::new(); + let mut byte = [0; 1]; + while !bytes.ends_with(b"\r\n\r\n") { + anyhow::ensure!( + bytes.len() < 8192 && socket.read(&mut byte).await? == 1, + "incomplete request head" + ); + bytes.push(byte[0]); + } + Ok(String::from_utf8(bytes)?) +} + +#[test] +fn cors_redirect_referrer_never_recovers_stripped_information() -> Result<()> { + let source = url::Url::parse("https://origin.test/private?token=secret#fragment")?; + for (initial_policy, first_referrer) in [ + ("no-referrer", None), + ("origin", Some("https://origin.test/")), + ( + "unsafe-url", + Some("https://origin.test/private?token=secret"), + ), + ] { + let request = Request::new("GET", "https://origin.test/start", None, Vec::new())? + .with_request_origin((&source).into()) + .with_initiator_url(&source) + .with_credentials_mode(RequestCredentialsMode::SameOrigin) + .with_browser_request_metadata(BrowserRequestMetadata::Fetch) + .with_subresource_request_metadata(moli_fetch::SubresourceRequestMetadata { + referrer_policy: Some(initial_policy.to_owned()), + ..Default::default() + }); + let mut redirects = ManualCorsRedirectState::new(request, Vec::new()); + for (index, (location, policy)) in [ + ("https://remote.test/second", "unsafe-url"), + ("https://another.test/third", "origin"), + ("https://origin.test/final", "unsafe-url"), + ] + .into_iter() + .enumerate() + { + let head = ResponseHead { + final_url: redirects.request().url.clone(), + status: 302, + status_text: None, + headers: vec![ + ("Location".to_owned(), location.to_owned()), + ("Referrer-Policy".to_owned(), policy.to_owned()), + ("Access-Control-Allow-Origin".to_owned(), "*".to_owned()), + ], + request_cookie_report: None, + cookie_set_reports: Vec::new(), + redirected: false, + redirect_chain: Vec::new(), + from_cache: false, + negotiated_http_version: None, + }; + assert_eq!( + redirects.advance(head, false, None), + Ok(ManualCorsRedirectTransition::FollowedRedirect) + ); + let request = redirects.request(); + let expected = if index == 0 { + first_referrer + } else { + first_referrer.map(|_| "https://origin.test/") + }; + assert_eq!( + request.referrer_header_value(&request.url).as_deref(), + expected, + "initial policy={initial_policy}, hop={index}" + ); + assert_eq!(request.cookie_context.initiator_url.as_ref(), Some(&source)); + } + } + Ok(()) +} + +#[test] +fn cors_redirect_modes_precede_location_parsing() -> Result<()> { + let url = url::Url::parse("http://origin.test/start")?; + for status in [301, 302, 303, 307, 308] { + for location in [ + None, + Some("http://[invalid"), + Some("data:text/plain,redirect"), + Some("http://user:password@remote.test/"), + ] { + for mode in [RequestRedirectMode::Manual, RequestRedirectMode::Error] { + let request = Request::new("GET", url.as_str(), None, Vec::new())? + .with_request_origin((&url).into()) + .with_initiator_url(&url) + .with_browser_request_metadata(BrowserRequestMetadata::Fetch) + .with_redirect_mode(mode); + let mut redirects = ManualCorsRedirectState::new(request, Vec::new()); + let head = ResponseHead { + final_url: url.clone(), + status, + status_text: None, + headers: location + .map(|value| vec![("Location".to_owned(), value.to_owned())]) + .unwrap_or_default(), + request_cookie_report: None, + cookie_set_reports: Vec::new(), + redirected: false, + redirect_chain: Vec::new(), + from_cache: false, + negotiated_http_version: None, + }; + let result = redirects.advance(head, false, None); + if mode == RequestRedirectMode::Manual { + assert_eq!( + result, + Ok(ManualCorsRedirectTransition::ManualResponse { + response_url: url.clone() + }), + "status={status}, location={location:?}" + ); + } else { + assert_eq!( + result, + Err(redirect_mode_error_message(&url)), + "status={status}, location={location:?}" + ); + } + } + } + } + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn cors_redirect_discards_intermediate_bodies_and_tracks_final_completion() -> Result<()> { + let listener = TcpListener::bind("127.0.0.1:0").await?; + let address = listener.local_addr()?; + let (tail_tx, tail_rx) = tokio::sync::oneshot::channel(); + let (retired_tx, retired_rx) = tokio::sync::oneshot::channel(); + let server = tokio::spawn(async move { + let mut discarded = Vec::new(); + let mut final_socket = None; + for (method, path, origin) in [ + ("OPTIONS", "/start", "http://origin.test"), + ("PUT", "/start", "http://origin.test"), + ("OPTIONS", "/final", "null"), + ("PUT", "/final", "null"), + ] { + let (mut socket, _) = listener.accept().await?; + let head = request_head(&mut socket).await?; + assert!( + head.starts_with(&format!("{method} {path} HTTP/1.1")), + "{head}" + ); + assert!( + head.to_ascii_lowercase() + .contains(&format!("\r\norigin: {origin}\r\n")), + "{head}" + ); + let final_response = method == "PUT" && path == "/final"; + let mut response = format!( + "HTTP/1.1 {} Response\r\nAccess-Control-Allow-Origin: {origin}\r\nAccess-Control-Allow-Methods: PUT\r\nCache-Control: no-store\r\nContent-Length: {}\r\nConnection: close\r\n", + if method == "PUT" && path == "/start" { + 307 + } else { + 200 + }, + if final_response { 5 } else { 100 } + ); + if method == "PUT" && path == "/start" { + response.push_str(&format!( + "Location: http://localhost:{}/final\r\n", + address.port() + )); + } + response.push_str("\r\n"); + socket.write_all(response.as_bytes()).await?; + if final_response { + socket.write_all(b"he").await?; + final_socket = Some(socket); + } else { + // Deliberately never send the declared preflight/redirect body. + discarded.push(socket); + } + } + for mut socket in discarded { + assert_eq!( + tokio::time::timeout(Duration::from_secs(3), socket.read(&mut [0; 1])).await??, + 0, + "discarded hop must release its connection" + ); + } + let _ = retired_tx.send(()); + tail_rx.await?; + final_socket.unwrap().write_all(b"llo").await?; + Ok::<_, anyhow::Error>(()) + }); + let mut config = moli_fetch::FetchConfig::default(); + config.set_http_no_proxy(Some("*".to_owned())); + let owner = ResourceRequestClient::new(&config)?; + let loader = owner.handle(); + let cancel = FetchCancelHandle::new(); + let initiator = url::Url::parse("http://origin.test/page")?; + let request = Request::new("PUT", &format!("http://{address}/start"), None, Vec::new())? + .with_request_origin((&initiator).into()) + .with_credentials_mode(RequestCredentialsMode::SameOrigin) + .with_initiator_url(&initiator) + .with_browser_request_metadata(BrowserRequestMetadata::Fetch); + let observed = tokio::time::timeout( + Duration::from_secs(3), + fetch_browser_subresource_raw_stream_with_preflight_headers_and_network_metadata( + &loader, + request, + Some(cancel.clone()), + Vec::new(), + ), + ) + .await + .expect("final headers must not wait for discarded response bodies") + .map_err(anyhow::Error::msg)?; + let mut response = observed.into_response(); + assert_eq!(response.status, 200); + assert_eq!(response.redirect_chain.len(), 1); + tokio::time::timeout(Duration::from_secs(3), retired_rx).await??; + assert!( + !cancel.is_cancelled(), + "retiring a hop must not cancel the logical request" + ); + assert!( + !cancel.response_completion_is_committed(), + "discarded hops must not commit the unfinished final response" + ); + tail_tx.send(()).unwrap(); + let mut body = Vec::new(); + while let Some(chunk) = response.next_chunk().await { + body.extend(chunk); + } + assert_eq!(body, b"hello"); + assert!( + cancel.response_completion_is_committed(), + "final transport progress must reach the logical handle" + ); + cancel.cancel(); + response.finish().await?; + server.await??; + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn cors_redirect_parent_abort_reaches_the_current_hop() -> Result<()> { + let listener = TcpListener::bind("127.0.0.1:0").await?; + let address = listener.local_addr()?; + let (started_tx, started_rx) = tokio::sync::oneshot::channel(); + let server = tokio::spawn(async move { + let (mut redirect, _) = listener.accept().await?; + assert!( + request_head(&mut redirect) + .await? + .starts_with("GET /start HTTP/1.1") + ); + redirect.write_all(b"HTTP/1.1 302 Found\r\nAccess-Control-Allow-Origin: *\r\nLocation: /final\r\nContent-Length: 100\r\nConnection: close\r\n\r\n").await?; + let (mut final_socket, _) = listener.accept().await?; + assert!( + request_head(&mut final_socket) + .await? + .starts_with("GET /final HTTP/1.1") + ); + let _ = started_tx.send(()); + assert_eq!( + tokio::time::timeout(Duration::from_secs(3), final_socket.read(&mut [0; 1])).await??, + 0, + "parent abort must close the active transport" + ); + Ok::<_, anyhow::Error>(()) + }); + let mut config = moli_fetch::FetchConfig::default(); + config.set_http_no_proxy(Some("*".to_owned())); + let owner = ResourceRequestClient::new(&config)?; + let loader = owner.handle(); + let cancel = FetchCancelHandle::new(); + let task_cancel = cancel.clone(); + let initiator = url::Url::parse("http://origin.test/page")?; + let request = Request::new("GET", &format!("http://{address}/start"), None, Vec::new())? + .with_request_origin((&initiator).into()) + .with_credentials_mode(RequestCredentialsMode::SameOrigin) + .with_initiator_url(&initiator) + .with_browser_request_metadata(BrowserRequestMetadata::Xhr); + let fetch = tokio::spawn(async move { + fetch_browser_subresource_raw_stream_with_preflight_headers_and_network_metadata( + &loader, + request, + Some(task_cancel), + Vec::new(), + ) + .await + }); + tokio::time::timeout(Duration::from_secs(3), started_rx).await??; + assert!(!cancel.response_completion_is_committed()); + cancel.cancel(); + assert!( + tokio::time::timeout(Duration::from_secs(3), fetch) + .await?? + .is_err() + ); + server.await??; + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn cors_redirect_rejects_preflight_redirects_and_opaque_origin_denials_before_contact() +-> Result<()> { + for preflight in [false, true] { + let listener = TcpListener::bind("127.0.0.1:0").await?; + let address = listener.local_addr()?; + let (stop_tx, mut stop_rx) = tokio::sync::oneshot::channel(); + let server = tokio::spawn(async move { + let mut requests = Vec::new(); + loop { + let mut socket = tokio::select! { + accepted = listener.accept() => accepted?.0, + _ = &mut stop_rx => break, + }; + let head = request_head(&mut socket).await?; + let line = head.lines().next().unwrap(); + let first = line.contains(" /start "); + let response = format!( + "HTTP/1.1 {} Response\r\nLocation: /forbidden\r\nAccess-Control-Allow-Origin: {}\r\nAccess-Control-Allow-Methods: PUT\r\nContent-Length: 2\r\nConnection: close\r\nCache-Control: no-store\r\n\r\nok", + if first { 302 } else { 200 }, + if first && !preflight { + "http://wrong.test" + } else { + "*" + } + ); + requests.push(line.to_owned()); + socket.write_all(response.as_bytes()).await?; + } + Ok::<_, anyhow::Error>(requests) + }); + let mut config = moli_fetch::FetchConfig::default(); + config.set_http_no_proxy(Some("*".to_owned())); + let owner = ResourceRequestClient::new(&config)?; + let request = Request::new( + if preflight { "PUT" } else { "GET" }, + &format!("http://{address}/start"), + None, + Vec::new(), + )? + .with_credentials_mode(RequestCredentialsMode::SameOrigin) + .with_browser_request_metadata(BrowserRequestMetadata::Fetch); + let request = if preflight { + let initiator = url::Url::parse("http://origin.test/page")?; + request + .with_request_origin((&initiator).into()) + .with_initiator_url(&initiator) + } else { + // An explicit opaque origin can exist without an initiator URL. + request.with_request_origin(moli_url::WebOrigin::Opaque) + }; + let result = tokio::time::timeout( + Duration::from_secs(3), + fetch_browser_subresource_raw_stream_with_preflight_headers_and_network_metadata( + &owner.handle(), + request, + None, + Vec::new(), + ), + ) + .await?; + let _ = stop_tx.send(()); + let requests = server.await??; + assert!(result.is_err(), "preflight={preflight}"); + assert_eq!( + requests, + [format!( + "{} /start HTTP/1.1", + if preflight { "OPTIONS" } else { "GET" } + )] + ); + } + Ok(()) +} diff --git a/moli-renderer-v8/src/network_host/response/cors.rs b/moli-renderer-v8/src/network_host/response/cors.rs index b0f66a7b5d..d128eb6159 100644 --- a/moli-renderer-v8/src/network_host/response/cors.rs +++ b/moli-renderer-v8/src/network_host/response/cors.rs @@ -716,8 +716,12 @@ mod tests { ) })); assert_eq!( - validate_cors_response_for_origin(&origin, &response_url, &headers, mode) - .is_ok(), + validate_cors_response_chain( + &origin, + &header_response(response_url.clone(), headers.clone()), + mode + ) + .is_ok(), allowed, "origin={matching}, mode={mode:?}, values={values:?}" ); @@ -764,12 +768,22 @@ mod tests { RequestCredentialsMode::Include, ] { assert_eq!( - validate_cors_response_for_origin(&WebOrigin::from_url(&document_url), &response_url, &headers, mode).is_ok(), + validate_cors_response_chain( + &document_url, + &header_response(response_url.clone(), headers.clone()), + mode + ) + .is_ok(), mode != RequestCredentialsMode::Include || values == ["true"], "mode={mode:?}, values={values:?}" ); assert!( - validate_cors_response_for_origin(&WebOrigin::from_url(&document_url), &document_url, &headers, mode).is_ok(), + validate_cors_response_chain( + &document_url, + &header_response(document_url.clone(), headers.clone()), + mode + ) + .is_ok(), "same-origin responses do not require CORS permission" ); } diff --git a/moli-renderer-v8/src/runtime/page_vm/tests/mod.rs b/moli-renderer-v8/src/runtime/page_vm/tests/mod.rs index 6df8df5f00..1b321fce4b 100644 --- a/moli-renderer-v8/src/runtime/page_vm/tests/mod.rs +++ b/moli-renderer-v8/src/runtime/page_vm/tests/mod.rs @@ -14775,23 +14775,22 @@ async fn spawn_connection_drop_http_server(path: &'static str) -> (String, JoinH } async fn spawn_redirect_loop_http_server(path: &'static str) -> (String, JoinHandle<()>) { - // Keep each hop CORS-eligible so the test reaches the redirect limit. - const REDIRECT_LOOP_REQUESTS: usize = 11; + const REDIRECT_LOOP_REQUESTS: usize = 21; let listener = TcpListener::bind("127.0.0.1:0") .await .expect("bind redirect-loop http server"); let addr = listener.local_addr().expect("redirect-loop server addr"); let server = tokio::spawn(async move { for _ in 0..REDIRECT_LOOP_REQUESTS { - let (mut stream, _) = listener - .accept() + let (mut stream, _) = tokio::time::timeout(Duration::from_secs(5), listener.accept()) .await + .expect("redirect loop should reach its limit without an earlier rejection") .expect("accept redirect-loop request"); read_http_request_head(&mut stream) .await .expect("read redirect-loop request"); let response = format!( - "HTTP/1.1 302 Found\r\nAccess-Control-Allow-Origin: *\r\nLocation: {path}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" + "HTTP/1.1 302 Found\r\nLocation: {path}\r\nAccess-Control-Allow-Origin: *\r\nCache-Control: no-store\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" ); stream .write_all(response.as_bytes()) diff --git a/moli-renderer-v8/src/script_vm/tests/dom_xhr/cors_redirect.rs b/moli-renderer-v8/src/script_vm/tests/dom_xhr/cors_redirect.rs new file mode 100644 index 0000000000..8c5c94d66b --- /dev/null +++ b/moli-renderer-v8/src/script_vm/tests/dom_xhr/cors_redirect.rs @@ -0,0 +1,324 @@ +use super::*; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; + +struct RedirectCase { + label: String, + sites: Vec, + credentials: Option<&'static str>, + denied_hop: Option, + authorization: bool, + allowed: bool, +} + +fn redirect_cases() -> Vec { + let mut cases = Vec::new(); + for sites in [vec![0, 0], vec![0, 1], vec![1, 0], vec![1, 1], vec![1, 2]] { + for credentials in [ + None, + Some(":"), + Some("user:"), + Some(":password"), + Some("user:password"), + ] { + cases.push(RedirectCase { + label: format!("{sites:?}/{credentials:?}"), + allowed: sites == [0, 0] || matches!(credentials, None | Some(":")), + sites: sites.clone(), + credentials, + denied_hop: None, + authorization: false, + }); + } + } + for (sites, denied_hop) in [(vec![1, 0], 0), (vec![0, 1, 0], 1), (vec![1, 2], 1)] { + cases.push(RedirectCase { + label: format!("unauthorized hop {denied_hop} in {sites:?}"), + sites, + denied_hop: Some(denied_hop), + credentials: None, + authorization: false, + allowed: false, + }); + } + for sites in [vec![0, 0], vec![0, 1, 0], vec![1, 2]] { + cases.push(RedirectCase { + label: format!("authorization across {sites:?}"), + sites, + denied_hop: None, + credentials: None, + authorization: true, + allowed: true, + }); + } + cases +} + +async fn check_cors_redirects(worker: bool, api: &str) { + let cases = std::sync::Arc::new(redirect_cases()); + let mut listeners = Vec::new(); + let mut origins = Vec::new(); + for _ in 0..3 { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + origins.push(format!("http://{}", listener.local_addr().unwrap())); + listeners.push(listener); + } + let server_cases = cases.clone(); + let server_origins = origins.clone(); + let (stop_tx, mut stop_rx) = tokio::sync::oneshot::channel::<()>(); + let server = tokio::spawn(async move { + let mut observed = Vec::new(); + loop { + let (site, mut socket) = tokio::select! { + accepted = listeners[0].accept() => (0, accepted.unwrap().0), + accepted = listeners[1].accept() => (1, accepted.unwrap().0), + accepted = listeners[2].accept() => (2, accepted.unwrap().0), + _ = &mut stop_rx => break, + }; + let mut head = Vec::new(); + let mut byte = [0; 1]; + while !head.ends_with(b"\r\n\r\n") { + assert!(head.len() < 8192); + assert_eq!(socket.read(&mut byte).await.unwrap(), 1); + head.push(byte[0]); + } + let head = String::from_utf8(head).unwrap(); + let mut line = head.lines().next().unwrap().split_whitespace(); + let method = line.next().unwrap(); + let path = line.next().unwrap(); + let parts = path + .trim_start_matches('/') + .split('/') + .map(|part| part.parse::().unwrap()) + .collect::>(); + let [status, index, hop] = parts[..] else { + panic!("unexpected path {path}") + }; + let case = &server_cases[index]; + assert_eq!(case.sites[hop], site); + let header = |name: &str| { + head.lines() + .filter_map(|line| line.split_once(':')) + .find(|(field, _)| field.eq_ignore_ascii_case(name)) + .map(|(_, value)| value.trim().to_owned()) + }; + let origin = header("origin"); + let authorization = header("authorization"); + observed.push(( + status, + index, + hop, + method.to_owned(), + origin.clone(), + authorization, + )); + let mut response = format!( + "HTTP/1.1 {} Response\r\nContent-Type: text/plain\r\nContent-Length: 2\r\nConnection: close\r\nCache-Control: no-store\r\n", + if method != "OPTIONS" && hop + 1 < case.sites.len() { + status + } else { + 200 + } + ); + // Even forbidden destinations opt in. A rejected redirect must not + // be explained by a later response's missing permission headers. + if method == "OPTIONS" || case.denied_hop != Some(hop) { + response.push_str(&format!( + "Access-Control-Allow-Origin: {}\r\nAccess-Control-Allow-Credentials: true\r\n", + origin.as_deref().unwrap_or(&server_origins[0]) + )); + } + if method == "OPTIONS" { + response.push_str("Access-Control-Allow-Methods: GET\r\nAccess-Control-Allow-Headers: authorization\r\nAccess-Control-Max-Age: 0\r\n"); + } else if hop + 1 < case.sites.len() { + let mut destination = url::Url::parse(&format!( + "{}/{status}/{index}/{}", + server_origins[case.sites[hop + 1]], + hop + 1 + )) + .unwrap(); + if hop + 2 == case.sites.len() + && let Some(credentials) = case.credentials + { + let (username, password) = credentials.split_once(':').unwrap(); + destination.set_username(username).unwrap(); + destination.set_password(Some(password)).unwrap(); + } + response.push_str(&format!("Location: {destination}\r\n")); + } + response.push_str("\r\nok"); + socket.write_all(response.as_bytes()).await.unwrap(); + } + observed + }); + let mut config = moli_fetch::FetchConfig::default(); + config.set_http_no_proxy(Some("*".to_owned())); + let loader = ResourceRequestClient::new(&config).unwrap(); + let mut vm = + new_page_task_executor_test_vm_with_loader(&format!("{}/page", origins[0]), &loader); + let inputs = cases.iter().map(|case| serde_json::json!({"label": case.label, "site": case.sites[0], "authorization": case.authorization})).collect::>(); + let probe = format!( + r#" + (async () => {{ + const cases = {}, origins = {}, api = {api:?}, results = []; + for (const status of [301, 302, 303, 307, 308]) {{ + for (const [index, item] of cases.entries()) {{ + const url = origins[item.site] + '/' + status + '/' + index + '/0'; + let allowed = false; + if (api === 'fetch') {{ + let response; + try {{ + response = await fetch(url, {{headers: item.authorization ? {{Authorization: 'Bearer author'}} : {{}}}}); + }} catch (error) {{ if (!(error instanceof TypeError)) throw error; }} + if (response) {{ + if (response.status !== 200 || !response.redirected || await response.text() !== 'ok') throw new Error('Unexpected redirect response'); + allowed = true; + }} + }} else {{ + const xhr = new XMLHttpRequest(); + const done = new Promise(resolve => xhr.onloadend = resolve); + xhr.open('GET', url, api !== 'sync-xhr'); + if (item.authorization) xhr.setRequestHeader('Authorization', 'Bearer author'); + try {{ + xhr.send(); + if (api !== 'sync-xhr') await done; + allowed = xhr.status === 200 && xhr.responseText === 'ok'; + }} catch (error) {{ if (api !== 'sync-xhr' || error.name !== 'NetworkError') throw error; }} + if (!allowed && (xhr.status !== 0 || xhr.responseText !== '' || xhr.getAllResponseHeaders() !== '')) throw new Error('Rejected XHR exposed response'); + }} + results.push([status, item.label, allowed]); + }} + }} + return JSON.stringify(results); + }})() + "#, + serde_json::to_string(&inputs).unwrap(), + serde_json::to_string(&origins).unwrap() + ); + let script = if worker { + let worker_script = format!( + "Promise.resolve().then(() => {probe}).then(value => {{ postMessage(value); close(); }}, error => {{ postMessage(String(error.stack || error)); close(); }});" + ); + format!( + r#" + globalThis.corsRedirectResult = 'pending'; + const worker = new Worker(URL.createObjectURL(new Blob([{}], {{type: 'text/javascript'}}))); + worker.onmessage = event => {{ corsRedirectResult = event.data; }}; + worker.onerror = event => {{ corsRedirectResult = event.message; event.preventDefault(); }}; + "#, + serde_json::to_string(&worker_script).unwrap() + ) + } else { + format!( + "globalThis.corsRedirectResult = 'pending'; Promise.resolve().then(() => {probe}).then(value => {{ corsRedirectResult = value; }}, error => {{ corsRedirectResult = String(error.stack || error); }});" + ) + }; + vm.eval(&script).unwrap(); + tokio::time::timeout(std::time::Duration::from_secs(20), async { + while vm.eval("corsRedirectResult === 'pending'").unwrap() == "true" { + wait_for_one_selected_page_task_executor_test_turn(&mut vm, &loader) + .await + .unwrap(); + } + }) + .await + .expect("CORS redirect matrix should finish"); + stop_tx.send(()).unwrap(); + let observed = server.await.unwrap(); + let expected = [301, 302, 303, 307, 308] + .into_iter() + .flat_map(|status| { + cases + .iter() + .map(move |case| (status, &case.label, case.allowed)) + }) + .collect::>(); + assert_eq!( + vm.eval("corsRedirectResult").unwrap(), + serde_json::to_string(&expected).unwrap(), + "worker={worker}, api={api}" + ); + let mut expected_requests = Vec::new(); + for status in [301, 302, 303, 307, 308] { + for (index, case) in cases.iter().enumerate() { + let hops = if case.allowed { + case.sites.len() + } else if let Some(hop) = case.denied_hop { + hop + 1 + } else { + case.sites.len() - 1 + }; + let mut tainted = false; + let mut authorization = case.authorization; + for hop in 0..hops { + let site = case.sites[hop]; + if hop > 0 && case.sites[hop - 1] != site { + tainted |= case.sites[hop - 1] != 0; + authorization = false; + } + if authorization && (site != 0 || tainted) { + expected_requests.push((status, index, hop, "OPTIONS".to_owned())); + } + expected_requests.push((status, index, hop, "GET".to_owned())); + for record in observed + .iter() + .filter(|record| record.0 == status && record.1 == index && record.2 == hop) + { + if site != 0 || tainted { + assert_eq!( + record.4.as_deref(), + Some(if tainted { "null" } else { origins[0].as_str() }), + "request Origin for {} at hop {hop}", + case.label + ); + } + if case.authorization { + assert_eq!( + record.5.as_deref(), + (authorization && record.3 != "OPTIONS").then_some("Bearer author"), + "Authorization for {} at hop {hop}", + case.label + ); + } + } + } + } + } + assert_eq!( + observed + .into_iter() + .map(|(status, index, hop, method, _, _)| (status, index, hop, method)) + .collect::>(), + expected_requests, + "forbidden redirect destinations must not receive a request: worker={worker}, api={api}" + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn cors_redirect_policy_reaches_window_fetch() { + check_cors_redirects(false, "fetch").await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn cors_redirect_policy_reaches_worker_fetch() { + check_cors_redirects(true, "fetch").await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn cors_redirect_policy_reaches_window_xhr() { + check_cors_redirects(false, "xhr").await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn cors_redirect_policy_reaches_worker_xhr() { + check_cors_redirects(true, "xhr").await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn cors_redirect_policy_reaches_window_sync_xhr() { + check_cors_redirects(false, "sync-xhr").await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn cors_redirect_policy_reaches_worker_sync_xhr() { + check_cors_redirects(true, "sync-xhr").await; +} diff --git a/moli-renderer-v8/src/script_vm/tests/dom_xhr/mod.rs b/moli-renderer-v8/src/script_vm/tests/dom_xhr/mod.rs index 69675dab55..5ea90000f5 100644 --- a/moli-renderer-v8/src/script_vm/tests/dom_xhr/mod.rs +++ b/moli-renderer-v8/src/script_vm/tests/dom_xhr/mod.rs @@ -2,6 +2,7 @@ use super::*; mod computed_style; mod cors_preflight; +mod cors_redirect; mod cors_response; mod cssom; mod dom; diff --git a/moli-renderer-v8/src/worker/global_scope/fetch.rs b/moli-renderer-v8/src/worker/global_scope/fetch.rs index 5dde586904..297738c08f 100644 --- a/moli-renderer-v8/src/worker/global_scope/fetch.rs +++ b/moli-renderer-v8/src/worker/global_scope/fetch.rs @@ -214,13 +214,7 @@ pub(in crate::worker) fn spawn_worker_fetch_network( if let Some(auth) = auth { request = request.with_auth(auth.into()); } - if request.auth_requires_buffered_transport() - || !request.follow_redirects - || browser_request_needs_manual_preflight_redirects( - &request, - &cors_preflight_request_headers, - ) - { + if request.auth_requires_buffered_transport() || !request.follow_redirects { match fetch_browser_subresource_with_preflight_headers_and_network_metadata( loader.clone(), request, @@ -568,13 +562,7 @@ pub(in crate::worker) fn spawn_worker_xhr_network( }); let (result, network_request_headers) = match request { - Ok(request) - if request.auth_requires_buffered_transport() - || browser_request_needs_manual_preflight_redirects( - &request, - &cors_preflight_request_headers, - ) => - { + Ok(request) if request.auth_requires_buffered_transport() => { match fetch_browser_subresource_with_preflight_headers_and_network_metadata( loader.clone(), request, diff --git a/moli-renderer-v8/src/worker/global_scope/mod.rs b/moli-renderer-v8/src/worker/global_scope/mod.rs index 8c3505065b..66e83af7c1 100644 --- a/moli-renderer-v8/src/worker/global_scope/mod.rs +++ b/moli-renderer-v8/src/worker/global_scope/mod.rs @@ -72,7 +72,6 @@ use crate::network_host::{ XHR_SEND_FLAG_SLOT, XHR_TIMEOUT_SLOT, XHR_TIMEOUT_START_MS_SLOT, XHR_TIMEOUT_TIMER_SLOT, XHR_URL_SLOT, XHR_WITH_CREDENTIALS_SLOT, append_default_body_content_type, apply_xhr_failure, apply_xhr_response, apply_xhr_response_body_source, apply_xhr_timeout, apply_xhr_upload_event, - browser_request_needs_manual_preflight_redirects, build_fetch_response_object_from_body_source_for_request_mode, build_fetch_response_object_from_stream_for_request_mode, build_fetch_response_object_from_subresource_body_for_request_mode, diff --git a/moli-renderer-v8/src/worker/thread/tests/mod.rs b/moli-renderer-v8/src/worker/thread/tests/mod.rs index aedb3e7286..b0aba78518 100644 --- a/moli-renderer-v8/src/worker/thread/tests/mod.rs +++ b/moli-renderer-v8/src/worker/thread/tests/mod.rs @@ -1011,23 +1011,22 @@ async fn spawn_connection_drop_http_server(path: &'static str) -> (String, JoinH } async fn spawn_redirect_loop_http_server(path: &'static str) -> (String, JoinHandle<()>) { - // Keep each hop CORS-eligible so the test reaches the redirect limit. - const REDIRECT_LOOP_REQUESTS: usize = 11; + const REDIRECT_LOOP_REQUESTS: usize = 21; let listener = TcpListener::bind("127.0.0.1:0") .await .expect("bind worker redirect-loop http server"); let addr = listener.local_addr().expect("worker redirect-loop addr"); let server = tokio::spawn(async move { for _ in 0..REDIRECT_LOOP_REQUESTS { - let (mut stream, _) = listener - .accept() + let (mut stream, _) = tokio::time::timeout(Duration::from_secs(5), listener.accept()) .await + .expect("redirect loop should reach its limit without an earlier rejection") .expect("accept worker redirect-loop request"); read_http_request_head(&mut stream) .await .expect("read worker redirect-loop request"); let response = format!( - "HTTP/1.1 302 Found\r\nAccess-Control-Allow-Origin: *\r\nLocation: {path}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" + "HTTP/1.1 302 Found\r\nLocation: {path}\r\nAccess-Control-Allow-Origin: *\r\nCache-Control: no-store\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" ); stream .write_all(response.as_bytes())