AlexStocks commented on code in PR #948: URL: https://github.com/apache/dubbo-go-pixiu/pull/948#discussion_r3308066019
########## pkg/filter/http/openapi/openapi.go: ########## @@ -0,0 +1,215 @@ +/* + * 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 openapi + +import ( + "net/http" + "os" + "path/filepath" + "strings" +) + +import ( + "github.com/pb33f/libopenapi" + openapiValidator "github.com/pb33f/libopenapi-validator" + validatorConfig "github.com/pb33f/libopenapi-validator/config" + validatorErrors "github.com/pb33f/libopenapi-validator/errors" + validatorPaths "github.com/pb33f/libopenapi-validator/paths" + "github.com/pb33f/libopenapi/datamodel" + v3 "github.com/pb33f/libopenapi/datamodel/high/v3" + + "github.com/pkg/errors" +) + +import ( + "github.com/apache/dubbo-go-pixiu/pkg/common/constant" + "github.com/apache/dubbo-go-pixiu/pkg/common/extension/filter" + contexthttp "github.com/apache/dubbo-go-pixiu/pkg/context/http" + "github.com/apache/dubbo-go-pixiu/pkg/logger" +) + +const ( + // Kind is the kind of OpenAPI validation filter. + Kind = constant.HTTPOpenAPIFilter +) + +func init() { + filter.RegisterHttpFilter(&Plugin{}) +} + +type ( + Plugin struct { + } + + FilterFactory struct { + cfg *Config + validator openapiValidator.Validator + model *v3.Document + } + + Filter struct { + validator openapiValidator.Validator + model *v3.Document + } + + Config struct { + Path string `yaml:"path" json:"path,omitempty"` + } +) + +func (p *Plugin) Kind() string { + return Kind +} + +func (p *Plugin) CreateFilterFactory() (filter.HttpFilterFactory, error) { + return &FilterFactory{cfg: &Config{}}, nil +} + +func (factory *FilterFactory) Config() any { + return factory.cfg +} + +func (factory *FilterFactory) Apply() error { + path := strings.TrimSpace(factory.cfg.Path) + if path == "" { + return errors.New("openapi path is required") + } + factory.cfg.Path = path + + validator, model, err := loadValidatorFromFile(path) + if err != nil { + return err + } + factory.validator = validator + factory.model = model + return nil +} + +func (factory *FilterFactory) PrepareFilterChain(ctx *contexthttp.HttpContext, chain filter.FilterChain) error { + f := &Filter{ + validator: factory.validator, + model: factory.model, + } + chain.AppendDecodeFilters(f) + return nil +} + +func (f *Filter) Decode(ctx *contexthttp.HttpContext) filter.FilterStatus { + if f.validator == nil || f.model == nil { + return filter.Continue + } + + req := ctx.Request + pathItem, foundPath, ok := f.findRequestOperation(req) + if !ok { + return filter.Continue + } + + if valid, validationErrs := f.validator.ValidateHttpRequestSyncWithPathItem(req, pathItem, foundPath); !valid { + errResp := contexthttp.BadRequest.WithError(errors.New(formatValidationErrors(validationErrs))) + ctx.SendLocalReply(errResp.Status, errResp.ToJSON()) + logger.Debug(errResp.Error()) + return filter.Stop + } + return filter.Continue +} + +func (f *Filter) findRequestOperation(req *http.Request) (*v3.PathItem, string, bool) { + pathItem, validationErrs, foundPath := validatorPaths.FindPath(req, f.model, nil) + if len(validationErrs) > 0 || pathItem == nil { + return nil, "", false + } + if !hasRequestOperation(req, pathItem) { + return nil, "", false + } + return pathItem, foundPath, true +} + +func hasRequestOperation(req *http.Request, pathItem *v3.PathItem) bool { + switch req.Method { + case http.MethodGet: + return pathItem.Get != nil + case http.MethodPost: + return pathItem.Post != nil + case http.MethodPut: + return pathItem.Put != nil + case http.MethodDelete: + return pathItem.Delete != nil + case http.MethodOptions: + return pathItem.Options != nil + case http.MethodHead: + return pathItem.Head != nil || pathItem.Get != nil + case http.MethodPatch: + return pathItem.Patch != nil + case http.MethodTrace: + return pathItem.Trace != nil + default: + operations := pathItem.GetOperations() + return operations != nil && operations.GetOrZero(strings.ToLower(req.Method)) != nil + } +} + +func loadValidatorFromFile(path string) (openapiValidator.Validator, *v3.Document, error) { + spec, err := os.ReadFile(path) + if err != nil { + return nil, nil, err + } + + doc, err := libopenapi.NewDocumentWithConfiguration(spec, &datamodel.DocumentConfiguration{ + BasePath: filepath.Dir(path), + }) + if err != nil { + return nil, nil, err + } + + model, err := doc.BuildV3Model() + if err != nil { Review Comment: [P0] `doc.BuildV3Model()` 的错误被静默丢弃。 `BuildV3Model()` 可能同时返回非 nil error 和部分 model。当前代码直接丢弃了 error： ```go model, err := doc.BuildV3Model() // err 被忽略 ``` spec 本身的问题（无效 `$ref`、schema 循环引用等）会被静默忽略，validator 基于残缺模型运行，可能导致本该拒绝的请求被放行，或本该放行的请求被误拦。 修复： ```go model, err := doc.BuildV3Model() if err != nil { return nil, nil, fmt.Errorf("build openapi model: %w", err) } ``` 如果确实需要容忍部分错误，至少应该 `logger.Warn` 记录 model build 的 warnings。 ########## pkg/filter/http/openapi/openapi.go: ########## @@ -0,0 +1,215 @@ +/* + * 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 openapi + +import ( + "net/http" + "os" + "path/filepath" + "strings" +) + +import ( + "github.com/pb33f/libopenapi" + openapiValidator "github.com/pb33f/libopenapi-validator" + validatorConfig "github.com/pb33f/libopenapi-validator/config" + validatorErrors "github.com/pb33f/libopenapi-validator/errors" + validatorPaths "github.com/pb33f/libopenapi-validator/paths" + "github.com/pb33f/libopenapi/datamodel" + v3 "github.com/pb33f/libopenapi/datamodel/high/v3" + + "github.com/pkg/errors" +) + +import ( + "github.com/apache/dubbo-go-pixiu/pkg/common/constant" + "github.com/apache/dubbo-go-pixiu/pkg/common/extension/filter" + contexthttp "github.com/apache/dubbo-go-pixiu/pkg/context/http" + "github.com/apache/dubbo-go-pixiu/pkg/logger" +) + +const ( + // Kind is the kind of OpenAPI validation filter. + Kind = constant.HTTPOpenAPIFilter +) + +func init() { + filter.RegisterHttpFilter(&Plugin{}) +} + +type ( + Plugin struct { + } + + FilterFactory struct { + cfg *Config + validator openapiValidator.Validator + model *v3.Document + } + + Filter struct { + validator openapiValidator.Validator + model *v3.Document + } + + Config struct { + Path string `yaml:"path" json:"path,omitempty"` + } +) + +func (p *Plugin) Kind() string { + return Kind +} + +func (p *Plugin) CreateFilterFactory() (filter.HttpFilterFactory, error) { + return &FilterFactory{cfg: &Config{}}, nil +} + +func (factory *FilterFactory) Config() any { + return factory.cfg +} + +func (factory *FilterFactory) Apply() error { + path := strings.TrimSpace(factory.cfg.Path) + if path == "" { + return errors.New("openapi path is required") + } + factory.cfg.Path = path + + validator, model, err := loadValidatorFromFile(path) + if err != nil { + return err + } + factory.validator = validator + factory.model = model + return nil +} + +func (factory *FilterFactory) PrepareFilterChain(ctx *contexthttp.HttpContext, chain filter.FilterChain) error { + f := &Filter{ + validator: factory.validator, + model: factory.model, + } + chain.AppendDecodeFilters(f) + return nil +} + +func (f *Filter) Decode(ctx *contexthttp.HttpContext) filter.FilterStatus { + if f.validator == nil || f.model == nil { + return filter.Continue + } + + req := ctx.Request + pathItem, foundPath, ok := f.findRequestOperation(req) + if !ok { + return filter.Continue + } + + if valid, validationErrs := f.validator.ValidateHttpRequestSyncWithPathItem(req, pathItem, foundPath); !valid { + errResp := contexthttp.BadRequest.WithError(errors.New(formatValidationErrors(validationErrs))) + ctx.SendLocalReply(errResp.Status, errResp.ToJSON()) + logger.Debug(errResp.Error()) + return filter.Stop + } + return filter.Continue +} + +func (f *Filter) findRequestOperation(req *http.Request) (*v3.PathItem, string, bool) { + pathItem, validationErrs, foundPath := validatorPaths.FindPath(req, f.model, nil) + if len(validationErrs) > 0 || pathItem == nil { Review Comment: [P1] `findRequestOperation` 把路径查找错误和「未声明」混为一谈。 `validatorPaths.FindPath` 返回的 `validationErrs` 可能包含多种含义：路径确实不存在、路径模板匹配但存在歧义、内部错误。当前全部当成「未声明」处理并放行，调试时完全无法区分「真的没声明」和「查找过程出错」。 建议至少在 `validationErrs` 非空时 `logger.Debug` 记录一下错误内容，方便排查。 ########## go.mod: ########## @@ -73,6 +73,8 @@ require ( v.marlon.life/toolkit v0.0.0-20211025131614-e4a91730b4ab ) +replace github.com/go-openapi/testify/v2 => github.com/go-openapi/testify/v2 v2.0.2 Review Comment: [P1] `replace` directive 缺少注释说明原因。 ```go replace github.com/go-openapi/testify/v2 => github.com/go-openapi/testify/v2 v2.0.2 ``` 为什么需要 replace？是上游版本兼容问题还是临时 workaround？按照 Go module 惯例，每个 replace 都应该附带注释说明理由和预期移除条件。 另外请确认 `go-openapi/testify/v2 v2.0.2` 是否已被 libopenapi v0.36.1 的传递依赖正确引入——如果是依赖冲突导致的 replace，建议在 PR description 中说明冲突链。 ########## pkg/filter/http/openapi/openapi.go: ########## @@ -0,0 +1,215 @@ +/* + * 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 openapi + +import ( + "net/http" + "os" + "path/filepath" + "strings" +) + +import ( + "github.com/pb33f/libopenapi" + openapiValidator "github.com/pb33f/libopenapi-validator" + validatorConfig "github.com/pb33f/libopenapi-validator/config" + validatorErrors "github.com/pb33f/libopenapi-validator/errors" + validatorPaths "github.com/pb33f/libopenapi-validator/paths" + "github.com/pb33f/libopenapi/datamodel" + v3 "github.com/pb33f/libopenapi/datamodel/high/v3" + + "github.com/pkg/errors" +) + +import ( + "github.com/apache/dubbo-go-pixiu/pkg/common/constant" + "github.com/apache/dubbo-go-pixiu/pkg/common/extension/filter" + contexthttp "github.com/apache/dubbo-go-pixiu/pkg/context/http" + "github.com/apache/dubbo-go-pixiu/pkg/logger" +) + +const ( + // Kind is the kind of OpenAPI validation filter. + Kind = constant.HTTPOpenAPIFilter +) + +func init() { + filter.RegisterHttpFilter(&Plugin{}) +} + +type ( + Plugin struct { + } + + FilterFactory struct { + cfg *Config + validator openapiValidator.Validator + model *v3.Document + } + + Filter struct { + validator openapiValidator.Validator + model *v3.Document + } + + Config struct { + Path string `yaml:"path" json:"path,omitempty"` + } +) + +func (p *Plugin) Kind() string { + return Kind +} + +func (p *Plugin) CreateFilterFactory() (filter.HttpFilterFactory, error) { + return &FilterFactory{cfg: &Config{}}, nil +} + +func (factory *FilterFactory) Config() any { + return factory.cfg +} + +func (factory *FilterFactory) Apply() error { + path := strings.TrimSpace(factory.cfg.Path) + if path == "" { + return errors.New("openapi path is required") + } + factory.cfg.Path = path + + validator, model, err := loadValidatorFromFile(path) + if err != nil { + return err + } + factory.validator = validator + factory.model = model + return nil +} + +func (factory *FilterFactory) PrepareFilterChain(ctx *contexthttp.HttpContext, chain filter.FilterChain) error { + f := &Filter{ + validator: factory.validator, + model: factory.model, + } + chain.AppendDecodeFilters(f) + return nil +} + +func (f *Filter) Decode(ctx *contexthttp.HttpContext) filter.FilterStatus { + if f.validator == nil || f.model == nil { + return filter.Continue + } + + req := ctx.Request + pathItem, foundPath, ok := f.findRequestOperation(req) + if !ok { + return filter.Continue + } + + if valid, validationErrs := f.validator.ValidateHttpRequestSyncWithPathItem(req, pathItem, foundPath); !valid { + errResp := contexthttp.BadRequest.WithError(errors.New(formatValidationErrors(validationErrs))) + ctx.SendLocalReply(errResp.Status, errResp.ToJSON()) + logger.Debug(errResp.Error()) + return filter.Stop + } + return filter.Continue +} + +func (f *Filter) findRequestOperation(req *http.Request) (*v3.PathItem, string, bool) { + pathItem, validationErrs, foundPath := validatorPaths.FindPath(req, f.model, nil) + if len(validationErrs) > 0 || pathItem == nil { + return nil, "", false + } + if !hasRequestOperation(req, pathItem) { + return nil, "", false + } + return pathItem, foundPath, true +} + +func hasRequestOperation(req *http.Request, pathItem *v3.PathItem) bool { + switch req.Method { + case http.MethodGet: + return pathItem.Get != nil + case http.MethodPost: + return pathItem.Post != nil + case http.MethodPut: + return pathItem.Put != nil + case http.MethodDelete: + return pathItem.Delete != nil + case http.MethodOptions: + return pathItem.Options != nil + case http.MethodHead: + return pathItem.Head != nil || pathItem.Get != nil + case http.MethodPatch: + return pathItem.Patch != nil + case http.MethodTrace: + return pathItem.Trace != nil + default: + operations := pathItem.GetOperations() + return operations != nil && operations.GetOrZero(strings.ToLower(req.Method)) != nil + } +} + +func loadValidatorFromFile(path string) (openapiValidator.Validator, *v3.Document, error) { + spec, err := os.ReadFile(path) Review Comment: [P2] `os.ReadFile(path)` 对路径没有做任何校验。 如果 `path` 来自远程配置（如 Nacos/Apollo），攻击者可以注入任意文件路径读取服务器上的敏感文件内容（虽然读取后只用于 OpenAPI 解析，不会直接返回给用户，但 spec 内容可能通过 error message 泄露）。 建议： 1. 如果 path 来自用户配置，至少校验不是绝对路径或不在敏感目录下 2. 考虑限制 `BasePath` 不能是 `/` 或 `/etc` 等敏感路径 3. 如果只支持相对路径，在 `Apply()` 阶段做 `filepath.IsAbs()` 检查 当前场景如果 path 只来自本地 YAML 配置文件，风险较低，但作为防御性编程应该加上。 -- 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]
