Skip to content

No certify assets streaming issue #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Gwojda wants to merge 1 commit into
base: main
Choose a base branch
from
Open

No certify assets streaming issue #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 19 additions & 24 deletions packages/ic-http-gateway/src/protocol/handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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());
}
}
}

Expand All @@ -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());
}
}
Expand All @@ -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());
}
Expand All @@ -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);
}
}
Expand Down
99 changes: 99 additions & 0 deletions packages/ic-http-gateway/tests/range_request_stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
Loading