Skip to content

fix(simplestorage): sign headers passed through WithS3Options on presigned URLs - #29

Merged
efirs merged 3 commits into
mainfrom
fix/presign-forward-s3-options
Oct 1, 2026
Merged

efirs merged 3 commits into
mainfrom
fix/presign-forward-s3-options

Conversation

@efirs

@efirs efirs commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • PresignURL accepted WithS3Options but never forwarded them to the presigner, so a header added that way (for example X-Tigris-Regions via WithStaticReplicationRegions) was missing from X-Amz-SignedHeaders.
  • The GET, PUT and DELETE presign paths now pass the per-call S3 options into PresignOptions.ClientOptions, so the same header middleware that runs on a direct call runs inside the presigner and the header is signed.

Details

Tigris is starting to reject x-amz-* and x-tigris-* request headers that are not covered by the SigV4 signature (tigrisdata/tigris-os#5587, docs in tigrisdata/tigris-os-docs#550). With the old behaviour a caller could pass WithStaticReplicationRegions to PresignURL, get back a URL that did not sign X-Tigris-Regions, and have the upload rejected once the header was sent. Forwarding the options fixes that without any API change: whoever uses the URL must send the same headers with the same values, which is the SigV4 rule for every signed header.

New unit tests presign offline with static credentials and check X-Amz-SignedHeaders on all three methods, with and without options.

Test plan

  • Code compiles with go build ./...
  • Code formatted with npm run format
  • go test ./... passes, including the new TestPresignURLSignsS3OptionHeaders

Assisted-by: Claude Fable 5.1 via Claude Code


Note

Medium Risk
Changes SigV4 presign behavior for all HTTP methods when S3 options are used; callers relying on the old (broken) unsigned-header behavior could see different signed-header requirements, but the fix aligns with Tigris enforcement.

Overview
PresignURL now forwards per-call WithS3Options into the AWS presigner so Tigris/S3 middleware headers (e.g. X-Tigris-Regions) appear in X-Amz-SignedHeaders. A shared presignOptions helper wires expiry plus ClientOptions.S3Options into PresignOptions.ClientOptions for GET, PUT, and DELETE—fixing uploads that were rejected when callers sent unsigned x-tigris-* / x-amz-* headers.

Adds offline unit tests that assert signed headers with and without options, a docs example for presigned PUT with replication/metadata headers, and fixes bucket tests to defer cleanup()() when setupEnv returns a factory function.

Reviewed by Cursor Bugbot for commit dde5525. Bugbot is set up for automated code reviews on this repo. Configure here.

…igned URLs

PresignURL accepted WithS3Options but never forwarded them to the presigner, so
a header added that way, for example X-Tigris-Regions via
WithStaticReplicationRegions, was missing from X-Amz-SignedHeaders. Tigris is
starting to reject x-amz-* and x-tigris-* headers that are not covered by the
signature, which turned the silently dropped option into a rejected upload.

The GET, PUT and DELETE presign paths now pass the per-call S3 options into
PresignOptions.ClientOptions, so the same header middleware that runs on a
direct call runs inside the presigner and the header lands in the signature.

Assisted-by: Claude Fable 5.1 via Claude Code
Signed-off-by: Yevgeniy Firsov <firsov@tigrisdata.com>
@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Presigned URL generation now includes custom headers in signature.

The PR appears safe to merge; no new actionable issue was identified.

Summary

The PR forwards per-call S3 options to GET, PUT, and DELETE presign operations so their request headers are signed.

  • Adds offline signed-header tests and a PUT example showing the headers a URL consumer must send.
  • Corrects when bucket-test environment setup runs.

Reviews (3) · Last reviewed commit: "test(simplestorage): run setupEnv before..."

Comment thread simplestorage/client.go
…aders

Assisted-by: Claude Fable 5.1 via Claude Code
Signed-off-by: Yevgeniy Firsov <firsov@tigrisdata.com>
setupEnv returns the restore function, so "defer cleanup()" called the setup
at the end of the case and dropped the restore. staticcheck SA9010 now flags
it; "defer cleanup()()" runs the setup first and defers the restore.

Assisted-by: Claude Fable 5.1 via Claude Code
Signed-off-by: Yevgeniy Firsov <firsov@tigrisdata.com>
@efirs
efirs merged commit 796a35a into main Oct 1, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants