fixup: give required presigned headers directly to the client#334
Open
cole-h wants to merge 1 commit into
Open
fixup: give required presigned headers directly to the client#334cole-h wants to merge 1 commit into
cole-h wants to merge 1 commit into
Conversation
We still override ones with what we calculated (so if something went wrong, the upload will fail because of that), but there could be some additional headers that are unknowable to the client that we just have to send along verbatim.
📝 WalkthroughWalkthroughS3 upload headers are now returned in stage metadata, passed through the release flow, merged with required tarball headers, validated, and used for the upload request. ChangesS3 header upload flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseFlow
participant FlakeHub
participant upload_release_to_s3
participant S3
ReleaseFlow->>FlakeHub: Request release-stage metadata
FlakeHub-->>ReleaseFlow: Return upload URL and headers
ReleaseFlow->>upload_release_to_s3: Pass URL, headers, and tarball
upload_release_to_s3->>S3: PUT tarball with validated headers
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
colemickens
approved these changes
Jul 10, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/flakehub_client.rs`:
- 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.
In `@src/s3.rs`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c466f06-a10d-4437-a872-6667748eccda
📒 Files selected for processing (3)
src/flakehub_client.rssrc/main.rssrc/s3.rs
| #[derive(serde::Deserialize)] | ||
| pub(crate) struct StageResult { | ||
| pub(crate) s3_upload_url: String, | ||
| pub(crate) s3_upload_headers: HashMap<String, String>, |
There was a problem hiding this comment.
🗄️ 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" srcRepository: 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" srcRepository: 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.
Comment on lines
+45
to
+66
|
|
||
| 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) | ||
| } |
There was a problem hiding this comment.
🎯 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We still override ones with what we calculated (so if something went wrong, the upload will fail because of that), but there could be some additional headers that are unknowable to the client that we just have to send along verbatim.
Summary by CodeRabbit
New Features
Bug Fixes