adding cors support #6987

chrislusf · 2025-07-15T04:47:00Z

What problem are we solving?

CORS was hard coded.

How are we solving the problem?

Support Get/Put/Delete BucketCORS

How is the PR tested?

intergration tests.

Checks

I have added unit tests if possible.
I will add related wiki document changes and link to this PR after merging.

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 introduces comprehensive Cross-Origin Resource Sharing (CORS) support to the S3 API. It transitions from hardcoded CORS behavior to a flexible, S3-compatible configuration system, allowing users to define and manage CORS rules per bucket. The changes include new API endpoints, a dedicated CORS logic module, persistent storage for configurations, and an integrated middleware for automatic header handling, all backed by a thorough suite of integration tests.

Highlights

S3-Compatible CORS API: Implemented full S3-compatible API endpoints for managing bucket CORS configurations, including GetBucketCors, PutBucketCors, and DeleteBucketCors.
Dedicated CORS Logic Module: Introduced a new weed/s3api/cors package containing the core logic for parsing, validating, evaluating, and applying CORS rules. This module handles preflight (OPTIONS) requests and actual cross-origin requests.
Persistent CORS Configuration: CORS configurations are now persisted as metadata within the filer's .s3metadata files, ensuring that rules are retained across server restarts and are dynamically loaded for each bucket.
CORS Middleware Integration: A new HTTP middleware (weed/s3api/cors/middleware.go) has been integrated into the S3 API server. This middleware automatically intercepts and processes CORS requests, applying the appropriate Access-Control-* headers based on the bucket's configured rules.
Comprehensive Integration Tests: Added a new test/s3/cors directory with extensive integration tests. These tests cover various CORS scenarios, including configuration management (CRUD), origin/header/method matching, wildcard support, rule limits, caching behavior, and error handling, ensuring robust and compliant CORS functionality.

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 adds comprehensive support for S3 CORS, including the necessary API handlers, core logic, and a robust set of integration tests. The code is well-structured, but I've identified a critical issue in the preflight request handling that affects S3 compliance, along with some opportunities to improve performance, error handling, and maintainability.

gemini-code-assist · 2025-07-15T04:49:28Z

weed/s3api/cors/cors.go

+func EvaluateRequest(config *CORSConfiguration, corsReq *CORSRequest) (*CORSResponse, error) {
+	if config == nil || corsReq == nil {
+		return nil, fmt.Errorf("config and corsReq cannot be nil")
+	}
+
+	if corsReq.Origin == "" {
+		return nil, fmt.Errorf("origin header is required for CORS requests")
+	}
+
+	// Find the first matching rule
+	for _, rule := range config.CORSRules {
+		if matchesRule(&rule, corsReq) {
+			return buildResponse(&rule, corsReq), nil
+		}
+	}
+
+	return nil, fmt.Errorf("no matching CORS rule found")
+}


The current CORS preflight logic is not fully compliant with the S3 specification and has an inconsistency with its own unit tests.

According to the S3 documentation, a preflight OPTIONS request should receive a 403 Forbidden response if the requested method or headers are not allowed by a matching CORS rule. This implementation currently returns a 200 OK even for invalid methods/headers, which can lead to unexpected client-side behavior.

gemini-code-assist · 2025-07-15T04:49:28Z

weed/s3api/s3api_bucket_config.go

+	if corsConfig, err := s3a.loadCORSFromMetadata(bucket); err == nil {
+		config.CORS = corsConfig
+	}


Errors from loadCORSFromMetadata are silently ignored here. If loadCORSFromMetadata fails due to issues like malformed JSON in the .s3metadata file, the error is suppressed. This will cause the server to proceed with a missing or stale CORS configuration without any indication of the underlying problem, potentially leading to hard-to-debug CORS issues. The error should be checked and logged.

Suggested change

if corsConfig, err := s3a.loadCORSFromMetadata(bucket); err == nil {

config.CORS = corsConfig

}

// Load CORS configuration from .s3metadata

corsConfig, err := s3a.loadCORSFromMetadata(bucket)

if err != nil {

glog.V(3).Infof("loading CORS for bucket %s: %v", bucket, err)

}

config.CORS = corsConfig

gemini-code-assist · 2025-07-15T04:49:28Z

test/s3/cors/Makefile

+lint:
+	@echo "Running linter..."
+	@golint . || echo "golint not available, skipping..."


The lint target uses golint, which is deprecated and has been archived since 2021. For better static analysis and long-term maintainability, it's recommended to migrate to a modern, supported linter like staticcheck or golangci-lint.

gemini-code-assist · 2025-07-15T04:49:28Z

weed/s3api/cors/cors.go

+func matchesWildcard(pattern, origin string) bool {
+	// Convert wildcard pattern to regex
+	escapedPattern := regexp.QuoteMeta(pattern)
+	regexPattern := strings.Replace(escapedPattern, "\\*", ".*", -1)
+	regex, err := regexp.Compile("^" + regexPattern + "$")
+	if err != nil {
+		return false
+	}
+	return regex.MatchString(origin)
+}


This function compiles a regular expression on every call, which can be inefficient. Since the S3 CORS spec only allows a single * wildcard in origins and headers, you can achieve the same result with much more performant string manipulation functions like strings.SplitN, strings.HasPrefix, and strings.HasSuffix.

func matchesWildcard(pattern, origin string) bool { // S3-style wildcard matching is simpler than full regex. if !strings.Contains(pattern, "*") { return pattern == origin } parts := strings.SplitN(pattern, "*", 2) return strings.HasPrefix(origin, parts[0]) && strings.HasSuffix(origin, parts[1]) }

gemini-code-assist · 2025-07-15T04:49:29Z

weed/s3api/s3api_bucket_cors_handlers.go

+	// Store CORS configuration and update cache
+	if err := s3a.updateCORSConfiguration(bucket, &config); err != s3err.ErrNone {
+		glog.Errorf("Failed to update CORS configuration: %v", err)
+		s3err.WriteErrorResponse(w, r, s3err.ErrInternalError)
+		return
+	}
+
+	// Also update the .s3metadata file directly
+	storage := s3a.getCORSStorage()
+	if err := storage.Store(bucket, &config); err != nil {
+		glog.Errorf("Failed to store CORS configuration in metadata: %v", err)
+		s3err.WriteErrorResponse(w, r, s3err.ErrInternalError)
+		return
+	}


The logic for persisting the CORS configuration is split between two different mechanisms, which is confusing and could be error-prone. The handler first calls s3a.updateCORSConfiguration, which updates the in-memory cache, and then immediately calls storage.Store to persist the configuration to the .s3metadata file.

For better maintainability and clarity, the persistence logic should be consolidated. Ideally, s3a.updateCORSConfiguration should be responsible for both updating the cache and persisting the change to the filer, making the handler's logic simpler and less redundant.

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>

Copilot

Pull Request Overview

Adds support for Cross-Origin Resource Sharing (CORS) in the S3 API by introducing service-level static headers, bucket-specific middleware, metadata storage, and handler implementations, along with comprehensive integration tests.

Enable static CORS headers on service-level OPTIONS requests
Integrate bucket-specific CORS middleware and handlers
Persist bucket CORS configuration in .s3metadata and update in-memory cache
Add end-to-end integration tests covering configuration CRUD, preflight, actual requests, and edge cases

Reviewed Changes

Copilot reviewed 14 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
weed/s3api/s3err/error_handler.go	Updated `setCommonHeaders` to conditionally inject static CORS headers
weed/s3api/s3api_server.go	Refactored router setup: added `handleCORSOriginValidation`, global/bucket OPTIONS handlers, and CORS middleware
weed/s3api/s3api_object_handlers.go	Added `captureCORSHeaders`/`restoreCORSHeaders` to preserve middleware headers during proxy pass-through
weed/s3api/s3api_bucket_skip_handlers.go	Removed placeholder CORS handlers
weed/s3api/s3api_bucket_cors_handlers.go	Implemented real Get/Put/Delete bucket CORS configuration handlers
weed/s3api/s3api_bucket_config.go	Extended `BucketConfig` with CORS and implemented load/update/remove logic
weed/s3api/cors/middleware.go	Introduced CORS middleware for request evaluation and header injection
weed/s3api/cors/cors.go	Core CORS types, parsing, validation, matching, and header application logic
test/s3/cors/...	Added integration tests for CORS across various scenarios

Comments suppressed due to low confidence (1)

weed/s3api/s3err/error_handler.go:92

[nitpick] Indentation is inconsistent here; align this line with the surrounding header-set calls to match project style.

			w.Header().Set("Access-Control-Allow-Origin", "*")

Copilot · 2025-07-15T06:47:21Z

weed/s3api/s3api_server.go

+	w.Header().Set("Access-Control-Allow-Origin", origin)
+	w.Header().Set("Access-Control-Expose-Headers", "*")
+	w.Header().Set("Access-Control-Allow-Methods", "*")
+	w.Header().Set("Access-Control-Allow-Headers", "*")


The handler is setting methods, headers, and expose headers but never sets Access-Control-Allow-Origin. You should mirror the request's Origin (e.g. w.Header().Set("Access-Control-Allow-Origin", origin)) so browsers accept the preflight.

weed/s3api/s3api_server.go

Copilot · 2025-07-15T06:47:22Z

weed/s3api/s3api_bucket_config.go

@@ -118,6 +123,19 @@ func (s3a *S3ApiServer) getBucketConfig(bucket string) (*BucketConfig, s3err.Err
 		}
 	}

+	// Load CORS configuration from .s3metadata
+	if corsConfig, err := s3a.loadCORSFromMetadata(bucket); err != nil {


The code checks err == filer_pb.ErrNotFound but loadCORSFromMetadata never returns that sentinel; consider returning a typed 'not found' error so callers can cleanly distinguish missing metadata from parse failures.

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

adding cors support

5ba551c

chrislusf requested a review from Copilot July 15, 2025 04:47

gemini-code-assist bot reviewed Jul 15, 2025

View reviewed changes