diff --git a/packages/ic-http-gateway/src/protocol/handler.rs b/packages/ic-http-gateway/src/protocol/handler.rs index 9ea5aa3..13a96bd 100644 --- a/packages/ic-http-gateway/src/protocol/handler.rs +++ b/packages/ic-http-gateway/src/protocol/handler.rs @@ -19,6 +19,17 @@ use ic_utils::{ interfaces::{http_request::HeaderField, HttpRequestCanister}, }; +/// Returns true if the header should be stripped from the response. +/// When the gateway receives a 206 response to a non-range request, it reassembles the full +/// body via streaming and sets the correct Content-Length itself. The canister's original +/// Content-Length (chunk size) and Content-Range must not be forwarded to the client. +fn is_streaming_206_header(name: &str, status_code: StatusCode, is_range_request: bool) -> bool { + !is_range_request + && status_code == 206 + && (name.eq_ignore_ascii_case(http_header::CONTENT_RANGE.as_ref()) + || name.eq_ignore_ascii_case(http_header::CONTENT_LENGTH.as_ref())) +} + fn create_err_response(status_code: StatusCode, msg: &str) -> CanisterResponse { let mut response = Response::new(HttpGatewayResponseBody::Right(Full::from( msg.as_bytes().to_vec(), @@ -275,7 +286,9 @@ pub async fn process_request( // this should only happen for raw domains. None => { for HeaderField(name, value) in &agent_response.headers { - response_builder = response_builder.header(name.as_ref(), value.as_ref()); + if !is_streaming_206_header(name, status_code, is_range_request) { + response_builder = response_builder.header(name.as_ref(), value.as_ref()); + } } } @@ -298,7 +311,9 @@ pub async fn process_request( // headers are also not certified in v1, filter known dangerous headers for HeaderField(name, value) in &agent_response.headers { - if !name.eq_ignore_ascii_case(CACHE_HEADER_NAME) { + if !is_streaming_206_header(name, status_code, is_range_request) + && !name.eq_ignore_ascii_case(CACHE_HEADER_NAME) + { response_builder = response_builder.header(name.as_ref(), value.as_ref()); } } @@ -308,17 +323,7 @@ pub async fn process_request( // assume the developer knows what they're doing and return the response as-is None => { for HeaderField(name, value) in &agent_response.headers { - // If the request is not a range-request, but got range-response, - // do not copy "Content-Range" and "Content-Length" headers, - // as clients obtain the full asset via a streaming response. - if !is_range_request - && status_code == 206 - && (name.eq_ignore_ascii_case(http_header::CONTENT_RANGE.as_ref()) - || name - .eq_ignore_ascii_case(http_header::CONTENT_LENGTH.as_ref())) - { - // skip copying - } else { + if !is_streaming_206_header(name, status_code, is_range_request) { response_builder = response_builder.header(name.as_ref(), value.as_ref()); } @@ -328,17 +333,7 @@ pub async fn process_request( // return only the certified headers Some(certified_http_response) => { for (name, value) in &certified_http_response.headers { - // If the request is not a range-request, but got range-response, - // do not copy "Content-Range" and "Content-Length" headers, - // as clients obtain the full asset via a streaming response. - if !is_range_request - && status_code == 206 - && (name.eq_ignore_ascii_case(http_header::CONTENT_RANGE.as_ref()) - || name - .eq_ignore_ascii_case(http_header::CONTENT_LENGTH.as_ref())) - { - // skip copying - } else { + if !is_streaming_206_header(name, status_code, is_range_request) { response_builder = response_builder.header(name, value); } } diff --git a/packages/ic-http-gateway/tests/range_request_stream.rs b/packages/ic-http-gateway/tests/range_request_stream.rs index 71aa8a4..391a91e 100644 --- a/packages/ic-http-gateway/tests/range_request_stream.rs +++ b/packages/ic-http-gateway/tests/range_request_stream.rs @@ -160,6 +160,105 @@ fn test_long_asset_request_yields_entire_asset(#[case] asset_name: &str) { ); } +/// Regression test: when skip_verification is true (raw domains), the gateway must not +/// forward the canister's chunk-level Content-Length alongside its own total Content-Length. +/// Previously, the raw/skip-verification path copied all canister headers unfiltered, +/// producing duplicate Content-Length values that violate HTTP/2. +#[rstest] +#[case(TWO_CHUNKS_ASSET_NAME)] +#[case(SIX_CHUNKS_ASSET_NAME)] +#[case(TEN_CHUNKS_ASSET_NAME)] +fn test_long_asset_skip_verification_has_single_content_length(#[case] asset_name: &str) { + let rt = tokio::runtime::Runtime::new().unwrap(); + let wasm_bytes = rt.block_on(async { utils::load_custom_assets_wasm().await }); + + let pic = PocketIcBuilder::new() + .with_nns_subnet() + .with_application_subnet() + .build(); + + let canister_id = pic.create_canister(); + pic.add_cycles(canister_id, 2_000_000_000_000_000); + pic.install_canister(canister_id, wasm_bytes, vec![], None); + + let url = pic.auto_progress(); + + let agent = Agent::builder().with_url(url).build().unwrap(); + rt.block_on(async { + agent.fetch_root_key().await.unwrap(); + }); + + let http_gateway = HttpGatewayClient::builder() + .with_agent(agent) + .build() + .unwrap(); + + let response = rt.block_on(async { + let mut req = http_gateway.request(HttpGatewayRequestArgs { + canister_id, + canister_request: Request::builder() + .uri(format!("/{asset_name}")) + .body(Bytes::new()) + .unwrap(), + }); + req.unsafe_set_skip_verification(true); + req.send().await + }); + + let expected_body = long_asset_body(asset_name); + + assert_eq!(response.canister_response.status(), 200); + + // There must be exactly one Content-Length header with the total asset size. + let content_length_values: Vec<&str> = response + .canister_response + .headers() + .get_all("content-length") + .iter() + .map(|v| v.to_str().unwrap()) + .collect(); + assert_eq!( + content_length_values, + vec![expected_body.len().to_string().as_str()], + "expected exactly one content-length header with total size {}, got {:?}", + expected_body.len(), + content_length_values + ); + + // Content-Range must not be present (it's a streaming reassembly, not a range response). + assert!( + !contains_header("content-range", response + .canister_response + .headers() + .iter() + .map(|(k, v)| (k.as_str(), v.to_str().unwrap())) + .collect()), + "content-range header should not be present in reassembled response" + ); + + rt.block_on(async { + let body = response + .canister_response + .into_body() + .collect() + .await + .unwrap() + .to_bytes() + .to_vec(); + + assert_eq!(body, expected_body); + }); + + assert_response_metadata( + response.metadata, + HttpGatewayResponseMetadata { + upgraded_to_update_call: false, + response_verification_version: None, + internal_error: None, + }, + ); +} + #[rstest] #[case(TWO_CHUNKS_ASSET_NAME, 0)] #[case(TWO_CHUNKS_ASSET_NAME, 1)]