rewrite, simplify, avoid unused functions #6989

chrislusf · 2025-07-15T14:58:05Z

No description provided.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Service-level responses need both Access-Control-Allow-Methods and Access-Control-Allow-Headers. After setting Access-Control-Allow-Origin and Access-Control-Expose-Headers, also set Access-Control-Allow-Methods: * and Access-Control-Allow-Headers: * so service endpoints satisfy CORS preflight requirements.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

gemini-code-assist

Summary of Changes

Hello @chrislusf, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the S3 API by implementing full Cross-Origin Resource Sharing (CORS) capabilities, providing developers with granular control over cross-origin access to their data. Alongside this major feature addition, it introduces a robust, dedicated test suite for CORS and includes important refactorings to the existing AWS Signature authentication mechanisms, improving code clarity and efficiency.

Highlights

CORS Functionality: Introduced comprehensive Cross-Origin Resource Sharing (CORS) support for the S3 API, allowing bucket owners to define rules for cross-origin access to their resources. This includes API endpoints for PutBucketCors, GetBucketCors, and DeleteBucketCors.
CORS Test Suite: Added a new, dedicated Go module (test/s3/cors) with extensive integration tests for CORS. This suite covers configuration management, preflight and actual request handling, origin/header/method matching, wildcard support, rule limits, caching, and error handling.
Signature Authentication Refactoring: Refactored and simplified the AWS Signature Version 2 and Version 4 authentication logic. This involved removing several unused helper functions, streamlining the string-to-sign generation, and optimizing the signature calculation process by removing the sync.Pool based hash pool for V4 signatures.
CORS Middleware Integration: Integrated a new CORS middleware into the S3 API server to automatically handle CORS preflight (OPTIONS) requests and inject appropriate Access-Control-* headers into responses for both bucket-specific and service-level requests.
Robust Header Management: Enhanced the passThroughResponse mechanism to correctly capture and restore CORS headers set by the middleware, preventing them from being overwritten by proxy responses during object operations.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces CORS support to the S3 API, including configuration management, request evaluation, and header application. The changes also include refactoring and optimization of signature verification processes.

gemini-code-assist · 2025-07-15T15:01:34Z

weed/s3api/s3api_object_handlers.go

+	// Capture existing CORS headers that may have been set by middleware
+	capturedCORSHeaders := captureCORSHeaders(w, corsHeaders)
+
+	// Copy headers from proxy response
 	for k, v := range proxyResponse.Header {
 		w.Header()[k] = v
 	}
+
+	// Restore CORS headers that were set by middleware
+	restoreCORSHeaders(w, capturedCORSHeaders)


It is important to preserve CORS headers set by middleware during proxy responses. The code should capture and restore these headers to ensure CORS settings are correctly applied.

Suggested change

// Capture existing CORS headers that may have been set by middleware

capturedCORSHeaders := captureCORSHeaders(w, corsHeaders)

// Copy headers from proxy response

for k, v := range proxyResponse.Header {

w.Header()[k] = v

}

// Restore CORS headers that were set by middleware

restoreCORSHeaders(w, capturedCORSHeaders)

// Capture existing CORS headers that may have been set by middleware

capturedCORSHeaders := captureCORSHeaders(w, corsHeaders)

// Copy headers from proxy response

for k, v := range proxyResponse.Header {

w.Header()[k] = v

}

// Restore CORS headers that were set by middleware

restoreCORSHeaders(w, capturedCORSHeaders)

gemini-code-assist · 2025-07-15T15:01:34Z

weed/s3api/auth_signature_v4.go

+		// X-Amz-Content-Sha256 header value is optional for presigned requests.
 		defaultSha256Cksum = unsignedPayload
-		v, ok = r.URL.Query()["X-Amz-Content-Sha256"]
-		if !ok {
-			v, ok = r.Header["X-Amz-Content-Sha256"]
-		}
 	} else {
-		// X-Amz-Content-Sha256, if not set in signed requests, checksum
-		// will default to sha256([]byte("")).
+		// X-Amz-Content-Sha256 header value is required for all non-presigned requests.
 		defaultSha256Cksum = emptySHA256
-		v, ok = r.Header["X-Amz-Content-Sha256"]
 	}

-	// We found 'X-Amz-Content-Sha256' return the captured value.
-	if ok {
+	// If the client sends a SHA256 checksum of the object in this header, use it.
+	if v, ok = r.Header["X-Amz-Content-Sha256"]; ok {
 		return v[0]
 	}


It is better to retrieve the header value directly instead of using a local variable. This reduces the number of variables and makes the code more concise.

Suggested change

// X-Amz-Content-Sha256 header value is optional for presigned requests.

defaultSha256Cksum = unsignedPayload

v, ok = r.URL.Query()["X-Amz-Content-Sha256"]

if !ok {

v, ok = r.Header["X-Amz-Content-Sha256"]

}

} else {

// X-Amz-Content-Sha256, if not set in signed requests, checksum

// will default to sha256([]byte("")).

// X-Amz-Content-Sha256 header value is required for all non-presigned requests.

defaultSha256Cksum = emptySHA256

v, ok = r.Header["X-Amz-Content-Sha256"]

}

// We found 'X-Amz-Content-Sha256' return the captured value.

if ok {

// If the client sends a SHA256 checksum of the object in this header, use it.

if v, ok = r.Header["X-Amz-Content-Sha256"]; ok {

return v[0]

}

if v, ok = r.Header["X-Amz-Content-Sha256"]; ok {

return v[0]

}

gemini-code-assist · 2025-07-15T15:01:35Z

weed/s3api/auth_signature_v4.go

+	for _, header := range signedHeaders {
+		headerKey := http.CanonicalHeaderKey(header)
+		if header == "host" {
+			extractedSignedHeaders.Set("host", r.Host)


It is better to retrieve the header value directly instead of using a local variable. This reduces the number of variables and makes the code more concise.

if header == "host" { extractedSignedHeaders[header] = []string{r.Host} continue }

Copilot

Pull Request Overview

This PR removes unused S3 helper functions and refactors the V4 and V2 signature code to use shared signing utilities, simplifies the chunked reader, and updates tests accordingly.

Unified signature logic by introducing getSigningKey and getSignature, removing the old hash pool mechanism.
Simplified chunked reader by replacing custom CRLF helpers and consolidating chunk signature computation.
Overhauled auth_signature_v4.go, stripping legacy streaming and presign logic and adding streamlined implementations with updated imports.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
weed/s3api/chunked_reader_v4.go	Replaced pooled signature calls with `getSigningKey`/`getSignature`; removed CRLF helpers and unified signature computation.
weed/s3api/auto_signature_v4_test.go	Removed old presign helper; added new `preSignV4` implementation and adjusted imports.
weed/s3api/auth_signature_v4.go	Massive refactor of V4 auth: removed legacy code, consolidated signing, updated header extraction, and added new helper functions.
weed/s3api/auth_signature_v2.go	Simplified V2 auth paths, removed unneeded URL parsing, and aligned signature calls with shared utilities.

Comments suppressed due to low confidence (3)

weed/s3api/chunked_reader_v4.go:121

Deriving the signing key on each chunk read may be expensive; consider caching the signing key per reader instance to avoid repeated HMAC computations for every chunk.

	signingKey := getSigningKey(cred.SecretKey, signV4Values.Credential.scope.date.Format(yyyymmdd), region, "s3")

weed/s3api/auto_signature_v4_test.go:211

[nitpick] The new preSignV4 helper is not covered by existing tests; consider adding unit tests to verify presigned URL generation and parameter handling.

func preSignV4(iam *IdentityAccessManagement, req *http.Request, accessKey, secretKey string, expires int64) error {

weed/s3api/auth_signature_v4.go:65

getContentSha256Cksum does not read 'X-Amz-Content-Sha256' from query parameters for presigned requests, potentially ignoring a provided payload checksum; consider also checking URL query parameters for this header when presigned.

	if isRequestPresignedSignatureV4(r) {

weed/s3api/auth_signature_v4.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

This reverts commit fce2d4e.

chrislusf and others added 25 commits July 14, 2025 21:46

adding cors support

5ba551c

address some comments

7c22a23

optimize matchesWildcard

f3ba114

address comments

4630eea

fix for tests

c64c96d

address comments

902a5f6

address comments

b6fbedd

address comments

d218428

path building

c61ae43

refactor

0a68aae

Update weed/s3api/s3api_bucket_config.go

212dab3

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update weed/s3api/s3api_bucket_config.go

4d4463a

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update weed/s3api/s3api_object_handlers.go

456c3ce

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update weed/s3api/s3api_object_handlers.go

43bd62d

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix

27ec13c

refactor

68d9245

Update weed/s3api/s3api_bucket_config.go

5e8bbf9

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update weed/s3api/s3api_object_handlers.go

aab2df4

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update weed/s3api/s3api_server.go

76eafea

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

simplify

925a183

add cors tests

d7bae20

fix tests

a9f1dd2

fix tests

71aface

remove unused functions

28ca4cf

chrislusf requested a review from Copilot July 15, 2025 14:58

gemini-code-assist bot reviewed Jul 15, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

gemini-code-assist bot reviewed Jul 15, 2025

View reviewed changes

fix tests

21aaddf

chrislusf requested a review from Copilot July 15, 2025 15:10

This comment was marked as outdated.

Sign in to view

chrislusf added 2 commits July 15, 2025 08:20

simplify

0778298

address comments

21ae7ae

chrislusf requested a review from Copilot July 15, 2025 15:35

chrislusf and others added 2 commits July 15, 2025 08:35

Merge branch 'master' into add-bucket-cors

0cff72d

fix

28de5bb

This comment was marked as outdated.

Sign in to view

chrislusf requested a review from Copilot July 15, 2025 15:37

Copilot AI reviewed Jul 15, 2025

View reviewed changes

weed/s3api/auth_signature_v4.go Outdated Show resolved Hide resolved

chrislusf and others added 4 commits July 15, 2025 08:39

Update weed/s3api/auth_signature_v4.go

273faff

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Apply suggestion from @Copilot

fce2d4e

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

rename variable

8e08571

Revert "Apply suggestion from @Copilot"

6d0f66b

This reverts commit fce2d4e.

chrislusf changed the title ~~Remove unused functions~~ rewrite, avoid unused functions Jul 15, 2025

chrislusf changed the title ~~rewrite, avoid unused functions~~ rewrite, simplify, avoid unused functions Jul 15, 2025

chrislusf merged commit 74f4e9b into master Jul 15, 2025
17 checks passed

chrislusf deleted the add-bucket-cors branch July 15, 2025 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

rewrite, simplify, avoid unused functions #6989

rewrite, simplify, avoid unused functions #6989

Uh oh!

chrislusf commented Jul 15, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

This comment was marked as outdated.

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Jul 15, 2025

Uh oh!

gemini-code-assist bot Jul 15, 2025

Uh oh!

gemini-code-assist bot Jul 15, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rewrite, simplify, avoid unused functions #6989

rewrite, simplify, avoid unused functions #6989

Uh oh!

Conversation

chrislusf commented Jul 15, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

This comment was marked as outdated.

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!