fix: trim whitespace-only Region in error responses before header fallback - #2308
cipherprofessor wants to merge 1 commit into
Conversation
…lback httpRespToErrorResponse() only falls back to the x-amz-bucket-region header when the XML body's <Region> is exactly "". A whitespace-only value (e.g. a single space) is not caught by that check, so it gets used verbatim as the SigV4 signing region -- corrupting the Authorization header on every subsequent request to that bucket with "net/http: invalid header field value". This is the error-response-path sibling of the whitespace-region bug being fixed on the success-response path by minio#2274: reviewing that PR, @allanrogerr pointed out the identical issue exists here too and asked for it to be handled in its own PR, since minio#2274 doesn't touch this code path. This has no issue number of its own -- it was found via that review comment, not filed separately. Scoped to Region only, not RequestID/HostID just above it (populated by the identical "if empty, fall back to header" pattern): a stray space in a diagnostic ID field has no functional consequence, while a corrupted Region breaks every subsequent signed request to the bucket. Added a regression test modeling a whitespace-only <Region> in the XML body confirming it now correctly falls back to the x-amz-bucket-region header. go build, go vet, gofmt, and go test -short -race ./... all clean.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe error response parser trims the XML ChangesRegion normalization
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit trims the region neat Comment |
Problem
httpRespToErrorResponse()only falls back to thex-amz-bucket-regionheader when the XML error body's<Region>is exactly"". A whitespace-only value (e.g. a single space) isn't caught by that check, so it gets used verbatim as the SigV4 signing region — corrupting theAuthorizationheader on every subsequent request to that bucket withnet/http: invalid header field value.This is the error-response-path sibling of the whitespace-region bug already being fixed on the success-response path by #2274. While reviewing that PR, @allanrogerr pointed out the identical issue exists on this path too and asked for it to be handled in its own PR, since #2274 doesn't touch
httpRespToErrorResponse(). This fix has no issue number of its own — it was found via that review comment, not filed as a separate issue.Fix
One line:
errResp.Region = strings.TrimSpace(errResp.Region)immediately before the existing empty-string check, so a whitespace-only region is treated the same as a genuinely empty one and correctly falls back to the header.Scoped to
Regiononly — notRequestID/HostIDjust above it, which are populated by the identical "if empty, fall back to header" pattern. A stray space in a diagnostic ID field has no functional consequence, while a corruptedRegionbreaks every subsequent signed request to the bucket. Happy to extend to those two as well if maintainers would rather have all three handled consistently.Testing
TestHttpRespToErrorResponseWhitespaceRegion, modeling a whitespace-only<Region>in the XML body alongside a realx-amz-bucket-regionheader, confirming the result now falls back to the header value instead of using the whitespace verbatim. Confirmed this test fails against the pre-fix code and passes after the fix (TDD red/green).go build ./...,go vet ./...,gofmt -lall clean.go test -short -race ./...(full repo) green.golangci-lint run --config ./.golangci.ymlcurrently fails locally withunknown linters: 'gomodguard_v2'— verified this is pre-existing and unrelated to this change (identical failure on a clean checkout with this diff stashed out).Summary by CodeRabbit