Skip to content

fix(storage): separate query string from path in SigV4 signing#72

Merged
designcode merged 1 commit intomainfrom
fix/sigv4-signing
Mar 11, 2026
Merged

fix(storage): separate query string from path in SigV4 signing#72
designcode merged 1 commit intomainfrom
fix/sigv4-signing

Conversation

@designcode
Copy link
Copy Markdown
Collaborator

@designcode designcode commented Mar 11, 2026

Note

Medium Risk
Touches SigV4 signing used for credential-based authentication; mistakes could break request auth for any endpoints that use query params. Scope is small and covered by updated integration expectations around rename behavior.

Overview
Fixes AWS SigV4 signing in shared/http-client.ts by signing url.pathname as the request path and passing query parameters via a dedicated query object (instead of including url.search in the path), and exports generateSignatureHeaders for reuse/testing.

Updates storage updateObject integration tests to expect renames (and rename+access updates) to succeed, and adds cleanup steps to rename objects back so subsequent tests remain stable.

Written by Cursor Bugbot for commit 32e3f18. This will update automatically on new commits. Configure here.

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 11, 2026

Greptile Summary

This PR fixes a SigV4 signing bug where query-string parameters were being concatenated onto the path field of the @smithy/signature-v4 request object (url.pathname + url.search) instead of being passed in the library's separate query field. Because the library builds the canonical query string exclusively from query, the old approach caused the query parameters to land in the canonical URI component rather than the canonical query string component, producing a signature that the server could not verify.

Key changes:

  • generateSignatureHeaders now extracts url.searchParams into a Record<string, string> and spreads it into the query field, setting path to url.pathname only — this is the correct structure.
  • generateSignatureHeaders is exported so it can be covered by unit tests.
  • A new test file shared/http-client.test.ts is added, but the tests do not reliably guard the fix: the first two tests compare two URL variants whose url.search strings are identical after URLSearchParams normalisation, so they would have passed with the old broken code; the third test only checks that expected header keys are present, not that the path is free of the query string.

Confidence Score: 4/5

  • The production fix is correct and low-risk; the tests are too weak to provide meaningful regression coverage but do not introduce any bugs.
  • The code change itself is minimal, well-targeted, and aligns with the @smithy/signature-v4 API contract. The only concern is that the accompanying tests would not catch a future regression of the same bug, reducing long-term confidence without blocking a merge.
  • shared/http-client.test.ts — tests need stronger assertions to actually verify the path/query separation

Important Files Changed

Filename Overview
shared/http-client.ts Correctly separates URL query parameters into a dedicated query field when constructing the SigV4 request object, instead of appending url.search directly to path. The fix aligns with how @smithy/signature-v4 computes the canonical query string; no issues found.
shared/http-client.test.ts New test file for generateSignatureHeaders, but the tests do not actually guard against the regression they claim to cover — both URL variants normalise to the same search string so the first two tests would have passed with the old broken code, and the third test only checks that expected header keys are present rather than verifying path/query separation.

Last reviewed commit: a4270ae

@designcode designcode merged commit ad341ea into main Mar 11, 2026
2 checks passed
@designcode designcode deleted the fix/sigv4-signing branch March 11, 2026 16:16
@github-actions
Copy link
Copy Markdown

🎉 This PR is included in version 2.15.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Copy Markdown

🎉 This PR is included in version 1.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants