fix: trim whitespace from bucket location - #2274
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe bucket location response processor now trims whitespace before normalizing empty and legacy-region values. Tests cover whitespace-only input and whitespace-padded regional input. ChangesBucket location normalization
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| {"my-bucket", "", true, APIErrors[0], "us-east-1", nil, true}, | ||
| {"my-bucket", "", false, APIError{}, "us-east-1", nil, true}, | ||
| {"my-bucket", "\n \n", false, APIError{}, "us-east-1", nil, true}, | ||
| {"my-bucket", " \neu-central-1\t", false, APIError{}, "eu-central-1", nil, true}, |
There was a problem hiding this comment.
The description says the trim happens before the EU normalization, but neither new row exercises that ordering. The padded eu-central-1 row trims to a value that never reaches the EU branch, and the plain "EU" row passes with or without trimming, so nothing here would notice if the two steps were ever reordered.
This row covers the interaction directly: it resolves to eu-west-1 with the trim in place, and comes back as " EU " unchanged without it.
| {"my-bucket", " \neu-central-1\t", false, APIError{}, "eu-central-1", nil, true}, | |
| {"my-bucket", " \neu-central-1\t", false, APIError{}, "eu-central-1", nil, true}, | |
| {"my-bucket", " EU ", false, APIError{}, "eu-west-1", nil, true}, |
| } | ||
|
|
||
| location := locationConstraint | ||
| location := strings.TrimSpace(locationConstraint) |
There was a problem hiding this comment.
One branch up, the error-response path returns errResp.Region without the same treatment, and the errResp.Region == "" check guarding it has the identical gap this line just closed below: a whitespace-only region is not "", so it is returned as-is, cached by getBucketLocation, and ends up as the SigV4 region — the same newline-in-the-Authorization-header failure described in #2271.
The same value also reaches that cache and the signer from executeMethod's retry path (api.go:805, 814, 822) without passing through this function at all, so trimming only here would not cover it. Trimming once where errResp.Region is populated covers all three callers, and has the nicer property that a whitespace-only region normalizes to "" and falls through to the x-amz-bucket-region header instead of defaulting to us-east-1. strings is already imported in that file.
In api-error-response.go, immediately before the existing header fallback:
errResp.Region = strings.TrimSpace(errResp.Region)
if errResp.Region == "" {
errResp.Region = resp.Header.Get("x-amz-bucket-region")
}I have not seen a server actually emit a padded <Region> in an error body, so treat this as hardening rather than something you are hitting today — likely its own PR, and entirely your call.
2 participants
Description
Trim whitespace from decoded
LocationConstraintvalues before applying existing empty-region and EU normalization. This prevents whitespace-only responses from being cached and inserted into SigV4Authorizationheaders.Testing
go test -run '^TestProcessBucketLocationResponse$' -count=1 .go test -short -race ./...go test -race -v ./...against TLS-enabled AIStor edgemake lintmake examplesgo build ./...govulncheck ./...with Go 1.26.5Fixes #2271
Summary by CodeRabbit
Bug Fixes
Tests