Copilot commented on code in PR #3090: URL: https://github.com/apache/dubbo-go/pull/3090#discussion_r2564901322
########## protocol/triple/triple_protocol/cors.go: ########## @@ -0,0 +1,288 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package triple_protocol + +import ( + "net/http" + "net/url" + "sort" + "strconv" + "strings" +) + +import ( + "github.com/dubbogo/gost/log/logger" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/global" +) + +// corsConfig is an internal CORS configuration struct that doesn't depend on global package. +type corsConfig struct { + allowOrigins []string + allowMethods []string + allowHeaders []string + exposeHeaders []string + allowCredentials bool + maxAge int + hasWildcard bool +} + +const defaultPreflightMaxAge = 86400 + +// convertCorsConfig converts a *global.CorsConfig to an internal *corsConfig. +func convertCorsConfig(cfg *global.CorsConfig) *corsConfig { + if cfg == nil { + return nil + } + return &corsConfig{ + allowOrigins: append([]string(nil), cfg.AllowOrigins...), + allowMethods: append([]string(nil), cfg.AllowMethods...), + allowHeaders: append([]string(nil), cfg.AllowHeaders...), + exposeHeaders: append([]string(nil), cfg.ExposeHeaders...), + allowCredentials: cfg.AllowCredentials, + maxAge: cfg.MaxAge, + } +} + +func buildCorsConfig(cfg *corsConfig, handlers []protocolHandler) *corsConfig { + if cfg == nil { + return nil + } + if len(cfg.allowOrigins) == 0 { + // Empty origins means CORS is disabled. + return nil + } + + built := &corsConfig{ + allowOrigins: append([]string(nil), cfg.allowOrigins...), + allowHeaders: append([]string(nil), cfg.allowHeaders...), + exposeHeaders: append([]string(nil), cfg.exposeHeaders...), + allowCredentials: cfg.allowCredentials, + maxAge: cfg.maxAge, + hasWildcard: hasWildcardOrigin(cfg.allowOrigins), + } + + built.allowMethods = normalizeCorsMethods(cfg.allowMethods, handlers) + + if built.maxAge <= 0 { + built.maxAge = defaultPreflightMaxAge + } + + return built +} + +// normalizeCorsMethods normalizes and deduplicates CORS methods, adding defaults if needed. +func normalizeCorsMethods(configuredMethods []string, handlers []protocolHandler) []string { + methodSet := make(map[string]struct{}) + + // Add configured methods (normalized to uppercase) + for _, m := range configuredMethods { + if m == "" { + continue + } + methodSet[strings.ToUpper(m)] = struct{}{} + } + + // If no methods configured, try to infer from handlers + if len(methodSet) == 0 { + for _, hdl := range handlers { + for m := range hdl.Methods() { + methodSet[strings.ToUpper(m)] = struct{}{} + } + } + } + + // If still no methods, use defaults + if len(methodSet) == 0 { + for _, m := range []string{http.MethodGet, http.MethodPost, http.MethodPut, http.MethodDelete} { + methodSet[m] = struct{}{} + } + } + + // OPTIONS is always included for preflight requests + methodSet[http.MethodOptions] = struct{}{} + + // Convert set to sorted slice + methods := make([]string, 0, len(methodSet)) + for m := range methodSet { + methods = append(methods, m) + } + sort.Strings(methods) + return methods +} + +// matchOrigin checks if the request origin matches any allowed pattern. +// Supports: +// - exact match +// - "*" wildcard +// - subdomain wildcard: "*.example.com" or "https://*.example.com"; +func matchOrigin(origin string, allowedOrigins []string) bool { + if origin == "" || len(allowedOrigins) == 0 { + return false + } + + originURL, err := url.Parse(origin) + if err != nil { + return false + } + originHost := originURL.Hostname() + originScheme := originURL.Scheme + + for _, allowed := range allowedOrigins { + switch allowed { + case "": + continue + case "*": + return true + case origin: + return true + } + + allowedURL, err := url.Parse(allowed) + if err == nil && allowedURL.Hostname() != "" { + if allowedURL.Scheme != "" && allowedURL.Scheme != originScheme { + continue + } + allowedHost := allowedURL.Hostname() + if strings.HasPrefix(allowedHost, "*.") { + if subdomainOf(originHost, strings.TrimPrefix(allowedHost, "*.")) { + return true + } + continue + } + if allowedHost == originHost { + return true + } + continue + } + + if strings.HasPrefix(allowed, "*.") { + if subdomainOf(originHost, strings.TrimPrefix(allowed, "*.")) { + return true + } + } else if allowed == originHost { + return true + } + } + + return false +} + +func subdomainOf(originHost, base string) bool { + if base == "" || originHost == "" { + return false + } + return originHost != base && strings.HasSuffix(originHost, "."+base) Review Comment: The `subdomainOf` function logic is incorrect. It should return true when `originHost` equals `base` OR is a subdomain, but currently it explicitly excludes the exact match case (`originHost != base`). For example, if `base` is \"example.com\" and the allowed origin is \"*.example.com\", a request from \"example.com\" itself should be allowed by the wildcard pattern, but this logic would reject it. Consider whether the exact match exclusion is intentional or if the logic should be `strings.HasSuffix(originHost, \".\"+base) || originHost == base`. ```suggestion return originHost == base || strings.HasSuffix(originHost, "."+base) ``` ########## protocol/triple/triple_protocol/cors.go: ########## @@ -0,0 +1,288 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package triple_protocol + +import ( + "net/http" + "net/url" + "sort" + "strconv" + "strings" +) + +import ( + "github.com/dubbogo/gost/log/logger" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/global" +) + +// corsConfig is an internal CORS configuration struct that doesn't depend on global package. +type corsConfig struct { + allowOrigins []string + allowMethods []string + allowHeaders []string + exposeHeaders []string + allowCredentials bool + maxAge int + hasWildcard bool +} + +const defaultPreflightMaxAge = 86400 + +// convertCorsConfig converts a *global.CorsConfig to an internal *corsConfig. +func convertCorsConfig(cfg *global.CorsConfig) *corsConfig { + if cfg == nil { + return nil + } + return &corsConfig{ + allowOrigins: append([]string(nil), cfg.AllowOrigins...), + allowMethods: append([]string(nil), cfg.AllowMethods...), + allowHeaders: append([]string(nil), cfg.AllowHeaders...), + exposeHeaders: append([]string(nil), cfg.ExposeHeaders...), + allowCredentials: cfg.AllowCredentials, + maxAge: cfg.MaxAge, + } +} + +func buildCorsConfig(cfg *corsConfig, handlers []protocolHandler) *corsConfig { + if cfg == nil { + return nil + } + if len(cfg.allowOrigins) == 0 { + // Empty origins means CORS is disabled. + return nil + } + + built := &corsConfig{ + allowOrigins: append([]string(nil), cfg.allowOrigins...), + allowHeaders: append([]string(nil), cfg.allowHeaders...), + exposeHeaders: append([]string(nil), cfg.exposeHeaders...), + allowCredentials: cfg.allowCredentials, + maxAge: cfg.maxAge, + hasWildcard: hasWildcardOrigin(cfg.allowOrigins), + } + + built.allowMethods = normalizeCorsMethods(cfg.allowMethods, handlers) + + if built.maxAge <= 0 { + built.maxAge = defaultPreflightMaxAge + } + + return built +} + +// normalizeCorsMethods normalizes and deduplicates CORS methods, adding defaults if needed. +func normalizeCorsMethods(configuredMethods []string, handlers []protocolHandler) []string { + methodSet := make(map[string]struct{}) + + // Add configured methods (normalized to uppercase) + for _, m := range configuredMethods { + if m == "" { + continue + } + methodSet[strings.ToUpper(m)] = struct{}{} + } + + // If no methods configured, try to infer from handlers + if len(methodSet) == 0 { + for _, hdl := range handlers { + for m := range hdl.Methods() { + methodSet[strings.ToUpper(m)] = struct{}{} + } + } + } + + // If still no methods, use defaults + if len(methodSet) == 0 { + for _, m := range []string{http.MethodGet, http.MethodPost, http.MethodPut, http.MethodDelete} { + methodSet[m] = struct{}{} + } + } + + // OPTIONS is always included for preflight requests + methodSet[http.MethodOptions] = struct{}{} + + // Convert set to sorted slice + methods := make([]string, 0, len(methodSet)) + for m := range methodSet { + methods = append(methods, m) + } + sort.Strings(methods) + return methods +} + +// matchOrigin checks if the request origin matches any allowed pattern. +// Supports: +// - exact match +// - "*" wildcard +// - subdomain wildcard: "*.example.com" or "https://*.example.com"; +func matchOrigin(origin string, allowedOrigins []string) bool { + if origin == "" || len(allowedOrigins) == 0 { + return false + } + + originURL, err := url.Parse(origin) + if err != nil { + return false + } + originHost := originURL.Hostname() + originScheme := originURL.Scheme + + for _, allowed := range allowedOrigins { + switch allowed { + case "": + continue + case "*": + return true + case origin: + return true + } + + allowedURL, err := url.Parse(allowed) + if err == nil && allowedURL.Hostname() != "" { + if allowedURL.Scheme != "" && allowedURL.Scheme != originScheme { + continue + } + allowedHost := allowedURL.Hostname() + if strings.HasPrefix(allowedHost, "*.") { + if subdomainOf(originHost, strings.TrimPrefix(allowedHost, "*.")) { + return true + } + continue + } + if allowedHost == originHost { + return true + } + continue + } + + if strings.HasPrefix(allowed, "*.") { + if subdomainOf(originHost, strings.TrimPrefix(allowed, "*.")) { + return true + } + } else if allowed == originHost { + return true + } + } + + return false +} + +func subdomainOf(originHost, base string) bool { + if base == "" || originHost == "" { + return false + } + return originHost != base && strings.HasSuffix(originHost, "."+base) +} + +func handlePreflight(w http.ResponseWriter, r *http.Request, cors *corsConfig) bool { Review Comment: The `handlePreflight` function lacks test coverage. This function handles CORS preflight requests which are critical for security. Add tests covering: valid preflight requests, forbidden origins, disallowed methods, missing Origin header, and proper response header setting. ########## protocol/triple/triple_protocol/handler.go: ########## @@ -35,8 +35,9 @@ type Handler struct { // key is group/version implementations map[string]StreamingHandlerFunc protocolHandlers []protocolHandler - allowMethod string // Allow header - acceptPost string // Accept-Post header + allowMethod string // Allow header + acceptPost string // Accept-Post header + cors *corsConfig // CORS configuration Review Comment: [nitpick] Inconsistent formatting: the alignment of field types was changed. The original code had aligned field types (both `string` fields lined up), but now `*corsConfig` breaks that alignment. Consider maintaining the existing alignment pattern for consistency. ```suggestion allowMethod string // Allow header acceptPost string // Accept-Post header cors *corsConfig // CORS configuration ``` ########## protocol/triple/triple_protocol/handler.go: ########## @@ -462,3 +475,19 @@ func (c *handlerConfig) newProtocolHandlers(streamType StreamType) []protocolHan func getIdentifier(group, version string) string { return group + "/" + version } + +// handleCORS processes CORS requests. Returns true if the request was handled and processing should stop. +func (h *Handler) handleCORS(w http.ResponseWriter, r *http.Request) bool { + if r.Method == http.MethodOptions && r.Header.Get("Access-Control-Request-Method") != "" { + return handlePreflight(w, r, h.cors) + } + + if origin := r.Header.Get("Origin"); origin != "" { + if !allowOrigin(origin, h.cors) { + w.WriteHeader(http.StatusForbidden) + return true + } + addCORSHeaders(w, r, h.cors) + } Review Comment: [nitpick] For actual (non-preflight) CORS requests with forbidden origins, the handler returns `StatusForbidden` but doesn't write an error message body. While the status code is correct, returning an empty response body may make debugging difficult for API consumers. Consider writing a descriptive error message in the response body to improve the developer experience. ########## protocol/triple/triple_protocol/option.go: ########## @@ -22,6 +22,10 @@ import ( "time" ) +import ( Review Comment: Duplicate import statement. The package already has an import section at lines 18-23. This second import block should be merged with the existing one to maintain consistent import organization. ```suggestion ``` ########## protocol/triple/triple_protocol/cors.go: ########## @@ -0,0 +1,288 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package triple_protocol + +import ( + "net/http" + "net/url" + "sort" + "strconv" + "strings" +) + +import ( + "github.com/dubbogo/gost/log/logger" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/global" +) + +// corsConfig is an internal CORS configuration struct that doesn't depend on global package. +type corsConfig struct { + allowOrigins []string + allowMethods []string + allowHeaders []string + exposeHeaders []string + allowCredentials bool + maxAge int + hasWildcard bool +} + +const defaultPreflightMaxAge = 86400 + +// convertCorsConfig converts a *global.CorsConfig to an internal *corsConfig. +func convertCorsConfig(cfg *global.CorsConfig) *corsConfig { + if cfg == nil { + return nil + } + return &corsConfig{ + allowOrigins: append([]string(nil), cfg.AllowOrigins...), + allowMethods: append([]string(nil), cfg.AllowMethods...), + allowHeaders: append([]string(nil), cfg.AllowHeaders...), + exposeHeaders: append([]string(nil), cfg.ExposeHeaders...), + allowCredentials: cfg.AllowCredentials, + maxAge: cfg.MaxAge, + } +} + +func buildCorsConfig(cfg *corsConfig, handlers []protocolHandler) *corsConfig { + if cfg == nil { + return nil + } + if len(cfg.allowOrigins) == 0 { + // Empty origins means CORS is disabled. + return nil + } + + built := &corsConfig{ + allowOrigins: append([]string(nil), cfg.allowOrigins...), + allowHeaders: append([]string(nil), cfg.allowHeaders...), + exposeHeaders: append([]string(nil), cfg.exposeHeaders...), + allowCredentials: cfg.allowCredentials, + maxAge: cfg.maxAge, + hasWildcard: hasWildcardOrigin(cfg.allowOrigins), + } + + built.allowMethods = normalizeCorsMethods(cfg.allowMethods, handlers) + + if built.maxAge <= 0 { + built.maxAge = defaultPreflightMaxAge + } + + return built +} + +// normalizeCorsMethods normalizes and deduplicates CORS methods, adding defaults if needed. +func normalizeCorsMethods(configuredMethods []string, handlers []protocolHandler) []string { + methodSet := make(map[string]struct{}) + + // Add configured methods (normalized to uppercase) + for _, m := range configuredMethods { + if m == "" { + continue + } + methodSet[strings.ToUpper(m)] = struct{}{} + } + + // If no methods configured, try to infer from handlers + if len(methodSet) == 0 { + for _, hdl := range handlers { + for m := range hdl.Methods() { + methodSet[strings.ToUpper(m)] = struct{}{} + } + } + } + + // If still no methods, use defaults + if len(methodSet) == 0 { + for _, m := range []string{http.MethodGet, http.MethodPost, http.MethodPut, http.MethodDelete} { + methodSet[m] = struct{}{} + } + } + + // OPTIONS is always included for preflight requests + methodSet[http.MethodOptions] = struct{}{} + + // Convert set to sorted slice + methods := make([]string, 0, len(methodSet)) + for m := range methodSet { + methods = append(methods, m) + } + sort.Strings(methods) + return methods +} + +// matchOrigin checks if the request origin matches any allowed pattern. +// Supports: +// - exact match +// - "*" wildcard +// - subdomain wildcard: "*.example.com" or "https://*.example.com"; +func matchOrigin(origin string, allowedOrigins []string) bool { + if origin == "" || len(allowedOrigins) == 0 { + return false + } + + originURL, err := url.Parse(origin) + if err != nil { + return false + } + originHost := originURL.Hostname() + originScheme := originURL.Scheme + + for _, allowed := range allowedOrigins { + switch allowed { + case "": + continue + case "*": + return true + case origin: + return true + } + + allowedURL, err := url.Parse(allowed) + if err == nil && allowedURL.Hostname() != "" { + if allowedURL.Scheme != "" && allowedURL.Scheme != originScheme { + continue + } + allowedHost := allowedURL.Hostname() + if strings.HasPrefix(allowedHost, "*.") { + if subdomainOf(originHost, strings.TrimPrefix(allowedHost, "*.")) { + return true + } + continue + } Review Comment: The subdomain matching logic is duplicated at lines 164-169 and 176-179. This creates a maintenance burden if the logic needs to be updated. Consider extracting this into a helper function or refactoring to handle both cases in one place. ########## protocol/triple/triple_protocol/cors.go: ########## @@ -0,0 +1,288 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package triple_protocol + +import ( + "net/http" + "net/url" + "sort" + "strconv" + "strings" +) + +import ( + "github.com/dubbogo/gost/log/logger" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/global" +) + +// corsConfig is an internal CORS configuration struct that doesn't depend on global package. +type corsConfig struct { + allowOrigins []string + allowMethods []string + allowHeaders []string + exposeHeaders []string + allowCredentials bool + maxAge int + hasWildcard bool +} + +const defaultPreflightMaxAge = 86400 + +// convertCorsConfig converts a *global.CorsConfig to an internal *corsConfig. +func convertCorsConfig(cfg *global.CorsConfig) *corsConfig { + if cfg == nil { + return nil + } + return &corsConfig{ + allowOrigins: append([]string(nil), cfg.AllowOrigins...), + allowMethods: append([]string(nil), cfg.AllowMethods...), + allowHeaders: append([]string(nil), cfg.AllowHeaders...), + exposeHeaders: append([]string(nil), cfg.ExposeHeaders...), + allowCredentials: cfg.AllowCredentials, + maxAge: cfg.MaxAge, + } +} + +func buildCorsConfig(cfg *corsConfig, handlers []protocolHandler) *corsConfig { + if cfg == nil { + return nil + } + if len(cfg.allowOrigins) == 0 { + // Empty origins means CORS is disabled. + return nil + } Review Comment: The `buildCorsConfig` function lacks test coverage. This is a critical function that determines whether CORS is enabled and normalizes configuration. The logic at lines 68-71 (disabling CORS when origins are empty) and the method normalization at line 82 should be tested. Add unit tests covering: empty origins, nil config, config with wildcard origins, and method inference from handlers. ########## protocol/triple/triple_protocol/handler.go: ########## @@ -462,3 +475,19 @@ func (c *handlerConfig) newProtocolHandlers(streamType StreamType) []protocolHan func getIdentifier(group, version string) string { return group + "/" + version } + +// handleCORS processes CORS requests. Returns true if the request was handled and processing should stop. +func (h *Handler) handleCORS(w http.ResponseWriter, r *http.Request) bool { Review Comment: The `handleCORS` method lacks test coverage. This is the entry point for CORS handling in the request pipeline. Add integration tests covering: OPTIONS preflight requests, actual requests with Origin header, requests without Origin header, and forbidden origins. ########## protocol/triple/triple_protocol/cors.go: ########## @@ -0,0 +1,288 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package triple_protocol + +import ( + "net/http" + "net/url" + "sort" + "strconv" + "strings" +) + +import ( + "github.com/dubbogo/gost/log/logger" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/global" +) + +// corsConfig is an internal CORS configuration struct that doesn't depend on global package. +type corsConfig struct { + allowOrigins []string + allowMethods []string + allowHeaders []string + exposeHeaders []string + allowCredentials bool + maxAge int + hasWildcard bool +} + +const defaultPreflightMaxAge = 86400 + +// convertCorsConfig converts a *global.CorsConfig to an internal *corsConfig. +func convertCorsConfig(cfg *global.CorsConfig) *corsConfig { + if cfg == nil { + return nil + } + return &corsConfig{ + allowOrigins: append([]string(nil), cfg.AllowOrigins...), + allowMethods: append([]string(nil), cfg.AllowMethods...), + allowHeaders: append([]string(nil), cfg.AllowHeaders...), + exposeHeaders: append([]string(nil), cfg.ExposeHeaders...), + allowCredentials: cfg.AllowCredentials, + maxAge: cfg.MaxAge, + } +} + +func buildCorsConfig(cfg *corsConfig, handlers []protocolHandler) *corsConfig { + if cfg == nil { + return nil + } + if len(cfg.allowOrigins) == 0 { + // Empty origins means CORS is disabled. + return nil + } + + built := &corsConfig{ + allowOrigins: append([]string(nil), cfg.allowOrigins...), + allowHeaders: append([]string(nil), cfg.allowHeaders...), + exposeHeaders: append([]string(nil), cfg.exposeHeaders...), + allowCredentials: cfg.allowCredentials, + maxAge: cfg.maxAge, + hasWildcard: hasWildcardOrigin(cfg.allowOrigins), + } + + built.allowMethods = normalizeCorsMethods(cfg.allowMethods, handlers) + + if built.maxAge <= 0 { + built.maxAge = defaultPreflightMaxAge + } + + return built +} + +// normalizeCorsMethods normalizes and deduplicates CORS methods, adding defaults if needed. +func normalizeCorsMethods(configuredMethods []string, handlers []protocolHandler) []string { + methodSet := make(map[string]struct{}) + + // Add configured methods (normalized to uppercase) + for _, m := range configuredMethods { + if m == "" { + continue + } + methodSet[strings.ToUpper(m)] = struct{}{} + } + + // If no methods configured, try to infer from handlers + if len(methodSet) == 0 { + for _, hdl := range handlers { + for m := range hdl.Methods() { + methodSet[strings.ToUpper(m)] = struct{}{} + } + } + } + + // If still no methods, use defaults + if len(methodSet) == 0 { + for _, m := range []string{http.MethodGet, http.MethodPost, http.MethodPut, http.MethodDelete} { + methodSet[m] = struct{}{} + } + } + + // OPTIONS is always included for preflight requests + methodSet[http.MethodOptions] = struct{}{} + + // Convert set to sorted slice + methods := make([]string, 0, len(methodSet)) + for m := range methodSet { + methods = append(methods, m) + } + sort.Strings(methods) + return methods +} + +// matchOrigin checks if the request origin matches any allowed pattern. +// Supports: +// - exact match +// - "*" wildcard +// - subdomain wildcard: "*.example.com" or "https://*.example.com"; +func matchOrigin(origin string, allowedOrigins []string) bool { Review Comment: The `matchOrigin` function lacks test coverage. This function implements critical security logic for origin validation including exact matching, wildcard matching, and subdomain patterns. Add comprehensive unit tests covering: exact matches, wildcard \"*\", subdomain wildcards (\"*.example.com\"), scheme-specific wildcards (\"https://*.example.com\";), invalid origins, and edge cases like empty strings. ########## protocol/triple/options_cors.go: ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package triple + +import ( + "errors" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/global" +) + +// CORSOption configures a single aspect of CORS. +type CORSOption func(*global.CorsConfig) + +var ( + errNilCORSConfig = errors.New("cors config is nil") + errWildcardOriginWithCookies = errors.New("allowCredentials cannot be true when allow-origins contains \"*\"") + errNegativeMaxAge = errors.New("max-age must be non-negative") +) + +// CORSAllowOrigins sets allowed origins for CORS requests. +func CORSAllowOrigins(origins ...string) CORSOption { + return func(c *global.CorsConfig) { + c.AllowOrigins = append([]string(nil), origins...) + } +} + +// CORSAllowMethods sets allowed HTTP methods for CORS requests. +func CORSAllowMethods(methods ...string) CORSOption { + return func(c *global.CorsConfig) { + c.AllowMethods = append([]string(nil), methods...) + } +} + +// CORSAllowHeaders sets allowed request headers for CORS requests. +func CORSAllowHeaders(headers ...string) CORSOption { + return func(c *global.CorsConfig) { + c.AllowHeaders = append([]string(nil), headers...) + } +} + +// CORSExposeHeaders sets headers exposed to the browser. +func CORSExposeHeaders(headers ...string) CORSOption { + return func(c *global.CorsConfig) { + c.ExposeHeaders = append([]string(nil), headers...) + } +} + +// CORSAllowCredentials toggles whether credentials are allowed. +func CORSAllowCredentials(allow bool) CORSOption { + return func(c *global.CorsConfig) { + c.AllowCredentials = allow + } +} + +// CORSMaxAge sets the max age for preflight cache. +func CORSMaxAge(maxAge int) CORSOption { + return func(c *global.CorsConfig) { + c.MaxAge = maxAge + } +} + +// validateCorsConfig validates CORS configuration for unsafe combinations. +func validateCorsConfig(cors *global.CorsConfig) error { Review Comment: The `validateCorsConfig` function lacks test coverage. This function implements important validation logic to prevent insecure configurations. Add unit tests covering: nil config, wildcard origin with credentials enabled, negative max age, and valid configurations. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
