AlexStocks commented on code in PR #948: URL: https://github.com/apache/dubbo-go-pixiu/pull/948#discussion_r3308060049
########## 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] 的错误被静默丢弃。 当 OpenAPI spec 存在部分校验错误时， 会同时返回非 nil error **和** 部分 model。当前代码只取了 model，error 被丢弃： 这意味着 spec 本身的问题（如无效的 、schema 循环引用等）会被静默忽略，validator 基于残缺模型运行，可能导致： 1. 本该拒绝的请求被放行 2. 本该放行的请求被误拦 修复建议： 如果确实需要容忍部分错误，至少应该 记录 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] 把路径查找错误和「未声明」混为一谈。 返回的 可能包含多种含义： - 路径确实不存在 → 应该 skip - 路径模板匹配但存在歧义 → 可能需要 log - 内部错误 → 应该 log 当前全部当成「未声明」处理并放行，在调试时完全无法区分「真的没声明」和「查找过程出错」。 建议至少在 非空时 记录一下错误内容，方便排查。 ########## 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] 对路径没有做任何校验。 如果 来自远程配置（如 Nacos/Apollo），攻击者可以注入任意文件路径读取服务器上的敏感文件内容（虽然读取后只用于 OpenAPI 解析，不会直接返回给用户，但 spec 内容可能通过 error message 泄露）。 建议： 1. 如果 path 来自用户配置，至少校验不是绝对路径或不在敏感目录下 2. 考虑限制 不能是 或 等敏感路径 3. 如果只支持相对路径，在 阶段做 检查 当前场景如果 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]
