Skip to content

fixup: give required presigned headers directly to the client #334

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
cole-h wants to merge 1 commit into
base: main
Choose a base branch
from
+51 −22
Open

fixup: give required presigned headers directly to the client #334

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
3 changes: 3 additions & 0 deletions src/flakehub_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::collections::HashMap;

use color_eyre::eyre::{eyre, Context, Result};
use http::StatusCode;
use reqwest::header::HeaderMap;
Expand All @@ -20,6 +22,7 @@ pub struct Tarball {
#[derive(serde::Deserialize)]
pub(crate) struct StageResult {
pub(crate) s3_upload_url: String,
pub(crate) s3_upload_headers: HashMap<String, String>,

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the target file and nearby definitions.
wc -l src/flakehub_client.rs
cat -n src/flakehub_client.rs | sed -n '1,220p'

# Find all serde-related usage for the type and response path.
rg -n "s3_upload_headers|Decoding release metadata POST response|serde(default)|Deserialize|response\.json|flakehub_client" src

Repository: DeterminateSystems/flakehub-push

Length of output: 6044

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the response handling and the release stage flow.
cat -n src/main.rs | sed -n '130,220p'

# Check whether StageResult is used anywhere else or transformed before deserialization.
rg -n "StageResult|release_stage\(|release_publish\(|Decoding release metadata POST response|s3_upload_headers" src

Repository: DeterminateSystems/flakehub-push

Length of output: 4656

Add a default for StageResult::s3_upload_headers src/flakehub_client.rs/src/main.rs

If a server response ever omits s3_upload_headers, response.json() will fail with a missing-field error and abort the release. Add #[serde(default)] so older responses can still decode cleanly.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/flakehub_client.rs` at line 25, Add the serde default attribute to the
StageResult::s3_upload_headers field so deserialization supplies an empty
HashMap when the server omits it, preserving compatibility with older responses.

pub(crate) uuid: Uuid,
}

Expand Down
12 changes: 8 additions & 4 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{fmt::Display, io::IsTerminal, process::ExitCode};

use clap::Parser;
use color_eyre::eyre::{eyre, Result};
use color_eyre::eyre::{eyre, Context as _, Result};
use error::Error;
use http::StatusCode;
use reqwest::Response;
Expand Down Expand Up @@ -148,7 +148,7 @@ async fn execute() -> Result<std::process::ExitCode> {
let stage_result: StageResult = response
.json()
.await
.map_err(|_| eyre!("Decoding release metadata POST response"))?;
.context("Decoding release metadata POST response")?;

stage_result
}
Expand Down Expand Up @@ -192,8 +192,12 @@ async fn execute() -> Result<std::process::ExitCode> {
}
};

// upload tarball to s3
s3::upload_release_to_s3(stage_result.s3_upload_url, ctx.tarball).await?;
s3::upload_release_to_s3(
stage_result.s3_upload_url,
stage_result.s3_upload_headers,
ctx.tarball,
)
.await?;

// "publish.rs" - publish the release after upload
fhclient.release_publish(stage_result.uuid).await?;
Expand Down
58 changes: 40 additions & 18 deletions src/s3.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,29 @@
use std::collections::HashMap;

use color_eyre::eyre::{eyre, Result, WrapErr};
use http::header::{CONTENT_LENGTH, CONTENT_TYPE};
use http::{HeaderName, HeaderValue};
use reqwest::header::HeaderMap;

use crate::flakehub_client::Tarball;

pub async fn upload_release_to_s3(presigned_s3_url: String, tarball: Tarball) -> Result<()> {
pub async fn upload_release_to_s3(
presigned_s3_url: String,
s3_headers: HashMap<String, String>,
tarball: Tarball,
) -> Result<()> {
let overrides: HashMap<&str, String> = HashMap::from_iter([
(CONTENT_LENGTH.as_str(), tarball.bytes.len().to_string()),
(CONTENT_TYPE.as_str(), "application/gzip".to_string()),
("x-amz-checksum-sha256", tarball.hash_base64),
]);

let headers = build_headers(&s3_headers, &overrides)?;

let client = reqwest::Client::new();
let tarball_put_response = client
.put(presigned_s3_url)
.headers({
let mut header_map = HeaderMap::new();
header_map.insert(
reqwest::header::CONTENT_LENGTH,
reqwest::header::HeaderValue::from_str(&format!("{}", tarball.bytes.len()))
.unwrap(),
);
header_map.insert(
reqwest::header::HeaderName::from_static("x-amz-checksum-sha256"),
reqwest::header::HeaderValue::from_str(&tarball.hash_base64).unwrap(),
);
header_map.insert(
reqwest::header::CONTENT_TYPE,
reqwest::header::HeaderValue::from_str("application/gzip").unwrap(),
);
header_map
})
.headers(headers)
.body(tarball.bytes)
.send()
.await
Expand All @@ -42,3 +42,25 @@ pub async fn upload_release_to_s3(presigned_s3_url: String, tarball: Tarball) ->

Ok(())
}

fn build_headers(
required_headers: &HashMap<String, String>,
overrides: &HashMap<&str, String>,
) -> Result<HeaderMap> {
let mut header_map = HeaderMap::with_capacity(required_headers.len());
for (name, required_value) in required_headers {
let value = overrides
.iter()
.find(|(k, _)| k.eq_ignore_ascii_case(name))
.map(|(_, v)| v)
.unwrap_or(required_value);

let header_name = HeaderName::from_bytes(name.as_bytes())
.wrap_err_with(|| format!("Invalid header name `{name}`"))?;
let header_value = HeaderValue::from_str(value)
.wrap_err_with(|| format!("Invalid header value for `{name}`"))?;
header_map.insert(header_name, header_value);
}

Ok(header_map)
}
Comment on lines +45 to +66

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Required override headers are silently dropped when absent from the caller's s3_headers.

The loop iterates only over required_headers (which is actually bound to the caller-provided s3_headers at the call site — the parameter naming is inverted from the overrides map, which holds the truly required values). For each entry it checks whether a matching override exists and substitutes the value — but it never inserts an override that has no matching key in s3_headers. Since overrides contains Content-Length, Content-Type, and x-amz-checksum-sha256, any of these missing from the presigned-URL response headers will be entirely absent from the final HeaderMap, even though the PUT to S3 depends on them. This contradicts the PR intent that "server-calculated headers continue to override provided values" — they should always be present, not merely substituted when a key collision happens.

Iterate over the union of both maps (or unconditionally insert the overrides after inserting caller headers) so required headers are always present.

Proposed fix 
 fn build_headers(
-    required_headers: &HashMap<String, String>,
+    caller_headers: &HashMap<String, String>,
     overrides: &HashMap<&str, String>,
 ) -> Result<HeaderMap> {
-    let mut header_map = HeaderMap::with_capacity(required_headers.len());
-    for (name, required_value) in required_headers {
-        let value = overrides
-            .iter()
-            .find(|(k, _)| k.eq_ignore_ascii_case(name))
-            .map(|(_, v)| v)
-            .unwrap_or(required_value);
-
+    let mut header_map = HeaderMap::with_capacity(caller_headers.len() + overrides.len());
+    for (name, value) in caller_headers {
+        if overrides.keys().any(|k| k.eq_ignore_ascii_case(name)) {
+            continue; // will be inserted below, with the required value
+        }
         let header_name = HeaderName::from_bytes(name.as_bytes())
             .wrap_err_with(|| format!("Invalid header name `{name}`"))?;
         let header_value = HeaderValue::from_str(value)
             .wrap_err_with(|| format!("Invalid header value for `{name}`"))?;
         header_map.insert(header_name, header_value);
     }
+    for (name, value) in overrides {
+        let header_name = HeaderName::from_bytes(name.as_bytes())
+            .wrap_err_with(|| format!("Invalid header name `{name}`"))?;
+        let header_value = HeaderValue::from_str(value)
+            .wrap_err_with(|| format!("Invalid header value for `{name}`"))?;
+        header_map.insert(header_name, header_value);
+    }

     Ok(header_map)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn build_headers(
required_headers: &HashMap<String, String>,
overrides: &HashMap<&str, String>,
) -> Result<HeaderMap> {
let mut header_map = HeaderMap::with_capacity(required_headers.len());
for (name, required_value) in required_headers {
let value = overrides
.iter()
.find(|(k, _)| k.eq_ignore_ascii_case(name))
.map(|(_, v)| v)
.unwrap_or(required_value);
let header_name = HeaderName::from_bytes(name.as_bytes())
.wrap_err_with(|| format!("Invalid header name `{name}`"))?;
let header_value = HeaderValue::from_str(value)
.wrap_err_with(|| format!("Invalid header value for `{name}`"))?;
header_map.insert(header_name, header_value);
}
Ok(header_map)
}
fn build_headers(
caller_headers: &HashMap<String, String>,
overrides: &HashMap<&str, String>,
) -> Result<HeaderMap> {
let mut header_map = HeaderMap::with_capacity(caller_headers.len() + overrides.len());
for (name, value) in caller_headers {
if overrides.keys().any(|k| k.eq_ignore_ascii_case(name)) {
continue; // will be inserted below, with the required value
}
let header_name = HeaderName::from_bytes(name.as_bytes())
.wrap_err_with(|| format!("Invalid header name `{name}`"))?;
let header_value = HeaderValue::from_str(value)
.wrap_err_with(|| format!("Invalid header value for `{name}`"))?;
header_map.insert(header_name, header_value);
}
for (name, value) in overrides {
let header_name = HeaderName::from_bytes(name.as_bytes())
.wrap_err_with(|| format!("Invalid header name `{name}`"))?;
let header_value = HeaderValue::from_str(value)
.wrap_err_with(|| format!("Invalid header value for `{name}`"))?;
header_map.insert(header_name, header_value);
}
Ok(header_map)
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/s3.rs` around lines 45 - 66, Update build_headers so it inserts every
caller-provided header and then unconditionally inserts all entries from
overrides, ensuring required values such as Content-Length, Content-Type, and
x-amz-checksum-sha256 are present even when absent from the caller map. Preserve
case-insensitive matching and allow override entries to replace caller values
without duplicating headers.

Loading