Skip to content

Commit 9a622a8

Browse files
Copilotintel352
Copilot
and
intel352
authored
fix(openapi): harden body validation, determinism, and router wiring (#155)
* Initial plan * fix(openapi): address review feedback from thread 3844286430 - Add configurable max body size limit (default 1 MiB) via http.MaxBytesReader to prevent DoS from arbitrarily large request bodies - Use validateJSONValue() for request body validation to handle non-object root schemas (primitives, arrays) that were previously silently skipped - Only register /openapi.yaml endpoint when source spec is YAML; JSON sources already served via /openapi.json - Sort supportedContentTypes() output for deterministic error messages - Remove /api/v1 from plugin schema DefaultConfig to match factory (empty) default - Add server→router mapping in wireOpenAPIRoutes for consistent router discovery when openapi module depends on http.server instead of http.router directly - Tests: add TestOpenAPIModule_JSONSourceNoYAMLEndpoint and TestOpenAPIModule_MaxBodySize Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
1 parent 55e173d commit 9a622a8

3 files changed

Lines changed: 147 additions & 32 deletions

File tree

‎module/openapi.go‎

Lines changed: 46 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"bytes"
55
"context"
66
"encoding/json"
7+
"errors"
78
"fmt"
89
"html"
910
"io"
@@ -12,6 +13,7 @@ import (
1213
"net/http"
1314
"os"
1415
"regexp"
16+
"sort"
1517
"strconv"
1618
"strings"
1719

@@ -33,13 +35,18 @@ type OpenAPISwaggerUIConfig struct {
3335

3436
// OpenAPIConfig holds the full configuration for an OpenAPI module.
3537
type OpenAPIConfig struct {
36-
SpecFile string `yaml:"spec_file" json:"spec_file"`
37-
BasePath string `yaml:"base_path" json:"base_path"`
38-
Validation OpenAPIValidationConfig `yaml:"validation" json:"validation"`
39-
SwaggerUI OpenAPISwaggerUIConfig `yaml:"swagger_ui" json:"swagger_ui"`
40-
RouterName string `yaml:"router" json:"router"` // optional: explicit router to attach to
38+
SpecFile string `yaml:"spec_file" json:"spec_file"`
39+
BasePath string `yaml:"base_path" json:"base_path"`
40+
Validation OpenAPIValidationConfig `yaml:"validation" json:"validation"`
41+
SwaggerUI OpenAPISwaggerUIConfig `yaml:"swagger_ui" json:"swagger_ui"`
42+
RouterName string `yaml:"router" json:"router"` // optional: explicit router to attach to
43+
MaxBodyBytes int64 `yaml:"max_body_bytes" json:"max_body_bytes"` // max request body size (bytes); 0 = use default
4144
}
4245

46+
// defaultMaxBodyBytes is the default request body size limit (1 MiB) applied
47+
// when Validation.Request is enabled and MaxBodyBytes is not explicitly set.
48+
const defaultMaxBodyBytes int64 = 1 << 20 // 1 MiB
49+
4350
// ---- Minimal OpenAPI v3 structs (parsed from YAML/JSON) ----
4451

4552
// openAPISpec is a minimal representation of an OpenAPI 3.x specification.
@@ -221,7 +228,7 @@ func (m *OpenAPIModule) RegisterRoutes(router HTTPRouter) {
221228
}
222229
}
223230

224-
// Serve raw spec at /openapi.json and /openapi.yaml
231+
// Serve raw spec at /openapi.json and (when source is YAML) /openapi.yaml
225232
if len(m.specBytes) > 0 {
226233
// Cache the JSON representation once per registration.
227234
if m.specJSON == nil {
@@ -233,21 +240,21 @@ func (m *OpenAPIModule) RegisterRoutes(router HTTPRouter) {
233240
}
234241

235242
specPathJSON := basePath + "/openapi.json"
236-
specPathYAML := basePath + "/openapi.yaml"
237243

238244
// JSON endpoint: serve re-serialised spec as JSON.
239245
router.AddRoute(http.MethodGet, specPathJSON, &openAPISpecHandler{specJSON: m.specJSON})
240246

241-
// YAML endpoint: serve the original spec bytes with a content-type that
242-
// matches the source format. JSON source files are served as application/json;
243-
// YAML source files are served as application/yaml.
244-
rawContentType := "application/yaml"
245-
if trimmed := strings.TrimSpace(string(m.specBytes)); len(trimmed) > 0 && (trimmed[0] == '{' || trimmed[0] == '[') {
246-
rawContentType = "application/json"
247+
// YAML endpoint: only register /openapi.yaml when the source spec is YAML.
248+
// When the source is JSON, clients can use /openapi.json instead.
249+
trimmed := strings.TrimSpace(string(m.specBytes))
250+
isJSONSource := len(trimmed) > 0 && (trimmed[0] == '{' || trimmed[0] == '[')
251+
if !isJSONSource {
252+
specPathYAML := basePath + "/openapi.yaml"
253+
router.AddRoute(http.MethodGet, specPathYAML, &openAPIRawSpecHandler{specBytes: m.specBytes, contentType: "application/yaml"})
254+
m.logger.Debug("OpenAPI spec endpoint registered", "module", m.name, "paths", []string{specPathJSON, specPathYAML})
255+
} else {
256+
m.logger.Debug("OpenAPI spec endpoint registered", "module", m.name, "paths", []string{specPathJSON})
247257
}
248-
router.AddRoute(http.MethodGet, specPathYAML, &openAPIRawSpecHandler{specBytes: m.specBytes, contentType: rawContentType})
249-
250-
m.logger.Debug("OpenAPI spec endpoint registered", "module", m.name, "paths", []string{specPathJSON, specPathYAML})
251258
}
252259

253260
// Serve Swagger UI
@@ -365,16 +372,29 @@ func (h *openAPIRouteHandler) validate(r *http.Request) []string {
365372
ct, supportedContentTypes(h.op.RequestBody.Content)))
366373
}
367374

375+
// Enforce a max body size to prevent DoS via arbitrarily large payloads.
376+
// The limit is configurable via OpenAPIConfig.MaxBodyBytes; default is 1 MiB.
377+
maxBytes := h.module.cfg.MaxBodyBytes
378+
if maxBytes <= 0 {
379+
maxBytes = defaultMaxBodyBytes
380+
}
381+
r.Body = http.MaxBytesReader(nil, r.Body, maxBytes)
382+
368383
// Read the body once so we can both check for presence (when required)
369384
// and validate against the schema. Restore it afterwards for downstream handlers.
370385
bodyBytes, readErr := io.ReadAll(r.Body)
371386
if readErr != nil {
372-
h.module.logger.Error("failed to read request body for validation",
373-
"module", h.module.name,
374-
"path", h.specPath,
375-
"error", readErr,
376-
)
377-
errs = append(errs, "failed to read request body")
387+
var maxBytesErr *http.MaxBytesError
388+
if errors.As(readErr, &maxBytesErr) {
389+
errs = append(errs, fmt.Sprintf("request body exceeds maximum allowed size of %d bytes", maxBytes))
390+
} else {
391+
h.module.logger.Error("failed to read request body for validation",
392+
"module", h.module.name,
393+
"path", h.specPath,
394+
"error", readErr,
395+
)
396+
errs = append(errs, "failed to read request body")
397+
}
378398
} else {
379399
// Always restore body for downstream handlers using the original byte slice
380400
// to avoid a bytes→string→bytes conversion that could corrupt non-UTF-8 payloads.
@@ -386,7 +406,7 @@ func (h *openAPIRouteHandler) validate(r *http.Request) []string {
386406
var bodyData any
387407
if jsonErr := json.Unmarshal(bodyBytes, &bodyData); jsonErr != nil {
388408
errs = append(errs, fmt.Sprintf("request body contains invalid JSON: %v", jsonErr))
389-
} else if bodyErrs := validateJSONBody(bodyData, mediaType.Schema); len(bodyErrs) > 0 {
409+
} else if bodyErrs := validateJSONValue(bodyData, "body", mediaType.Schema); len(bodyErrs) > 0 {
390410
errs = append(errs, bodyErrs...)
391411
}
392412
}
@@ -728,12 +748,13 @@ func htmlEscape(s string) string {
728748
return html.EscapeString(s)
729749
}
730750

731-
// supportedContentTypes returns a comma-joined list of content types defined
732-
// in the requestBody.content map, used in validation error messages.
751+
// supportedContentTypes returns a sorted, comma-joined list of content types
752+
// defined in the requestBody.content map, used in validation error messages.
733753
func supportedContentTypes(content map[string]openAPIMediaType) string {
734754
types := make([]string, 0, len(content))
735755
for ct := range content {
736756
types = append(types, ct)
737757
}
758+
sort.Strings(types)
738759
return strings.Join(types, ", ")
739760
}

‎module/openapi_test.go‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,35 @@ func TestOpenAPIModule_ParseJSON(t *testing.T) {
163163
}
164164
}
165165

166+
func TestOpenAPIModule_JSONSourceNoYAMLEndpoint(t *testing.T) {
167+
specPath := writeTempSpec(t, ".json", petstoreJSON)
168+
169+
mod := NewOpenAPIModule("json-api", OpenAPIConfig{
170+
SpecFile: specPath,
171+
BasePath: "/api",
172+
})
173+
if err := mod.Init(nil); err != nil {
174+
t.Fatalf("Init: %v", err)
175+
}
176+
177+
router := &testRouter{}
178+
mod.RegisterRoutes(router)
179+
180+
paths := make(map[string]bool)
181+
for _, rt := range router.routes {
182+
paths[rt.method+":"+rt.path] = true
183+
}
184+
185+
// JSON source spec: /openapi.json should be registered
186+
if !paths["GET:/api/openapi.json"] {
187+
t.Error("expected GET:/api/openapi.json to be registered for JSON source")
188+
}
189+
// /openapi.yaml should NOT be registered for a JSON source spec
190+
if paths["GET:/api/openapi.yaml"] {
191+
t.Error("expected GET:/api/openapi.yaml NOT to be registered for JSON source")
192+
}
193+
}
194+
166195
func TestOpenAPIModule_MissingSpecFile(t *testing.T) {
167196
mod := NewOpenAPIModule("bad", OpenAPIConfig{})
168197
if err := mod.Init(nil); err == nil {
@@ -429,6 +458,41 @@ func TestOpenAPIModule_RequestValidation_Body(t *testing.T) {
429458
})
430459
}
431460

461+
func TestOpenAPIModule_MaxBodySize(t *testing.T) {
462+
specPath := writeTempSpec(t, ".yaml", petstoreYAML)
463+
464+
mod := NewOpenAPIModule("petstore", OpenAPIConfig{
465+
SpecFile: specPath,
466+
BasePath: "/api/v1",
467+
Validation: OpenAPIValidationConfig{Request: true},
468+
MaxBodyBytes: 10, // very small limit to trigger the check
469+
})
470+
if err := mod.Init(nil); err != nil {
471+
t.Fatalf("Init: %v", err)
472+
}
473+
474+
router := &testRouter{}
475+
mod.RegisterRoutes(router)
476+
477+
h := router.findHandler("POST", "/api/v1/pets")
478+
if h == nil {
479+
t.Fatal("POST /api/v1/pets handler not found")
480+
}
481+
482+
body := `{"name": "Fluffy", "tag": "cat"}` // 33 bytes, exceeds limit of 10
483+
w := httptest.NewRecorder()
484+
r := httptest.NewRequest(http.MethodPost, "/api/v1/pets", bytes.NewBufferString(body))
485+
r.Header.Set("Content-Type", "application/json")
486+
h.Handle(w, r)
487+
488+
if w.Code != http.StatusBadRequest {
489+
t.Errorf("expected 400 for oversized body, got %d: %s", w.Code, w.Body.String())
490+
}
491+
if !strings.Contains(w.Body.String(), "exceeds maximum") {
492+
t.Errorf("expected error message about size limit, got: %s", w.Body.String())
493+
}
494+
}
495+
432496
func TestOpenAPIModule_NoValidation(t *testing.T) {
433497
specPath := writeTempSpec(t, ".yaml", petstoreYAML)
434498

‎plugins/openapi/plugin.go‎

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,13 @@ func (p *Plugin) ModuleFactories() map[string]plugin.ModuleFactory {
7777
if v, ok := cfg["router"].(string); ok {
7878
oacfg.RouterName = v
7979
}
80+
if v, ok := cfg["max_body_bytes"].(int); ok && v > 0 {
81+
oacfg.MaxBodyBytes = int64(v)
82+
} else if v, ok := cfg["max_body_bytes"].(int64); ok && v > 0 {
83+
oacfg.MaxBodyBytes = v
84+
} else if v, ok := cfg["max_body_bytes"].(float64); ok && v > 0 {
85+
oacfg.MaxBodyBytes = int64(v)
86+
}
8087

8188
if valCfg, ok := cfg["validation"].(map[string]any); ok {
8289
if v, ok2 := valCfg["request"].(bool); ok2 {
@@ -151,9 +158,16 @@ func (p *Plugin) ModuleSchemas() []*schema.ModuleSchema {
151158
DefaultValue: map[string]any{"enabled": false, "path": "/docs"},
152159
Group: "swagger_ui",
153160
},
161+
{
162+
Key: "max_body_bytes",
163+
Label: "Max Body Size",
164+
Type: schema.FieldTypeNumber,
165+
Description: "Maximum allowed request body size in bytes when validation is enabled (default: 1048576 = 1 MiB)",
166+
Placeholder: "1048576",
167+
Group: "validation",
168+
},
154169
},
155170
DefaultConfig: map[string]any{
156-
"base_path": "/api/v1",
157171
"validation": map[string]any{"request": true, "response": false},
158172
"swagger_ui": map[string]any{"enabled": false, "path": "/docs"},
159173
},
@@ -178,12 +192,16 @@ func (p *Plugin) WiringHooks() []plugin.WiringHook {
178192
func wireOpenAPIRoutes(app modular.Application, cfg *config.WorkflowConfig) error {
179193
// Build name→router lookup from config dependsOn
180194
routerNames := make(map[string]bool)
181-
openAPIDeps := make(map[string][]string) // openapi module name → dependsOn
195+
serverToRouter := make(map[string]string) // http.server name → router name
196+
openAPIDeps := make(map[string][]string) // openapi module name → dependsOn
182197
for _, modCfg := range cfg.Modules {
183-
if modCfg.Type == "http.router" {
198+
switch modCfg.Type {
199+
case "http.router":
184200
routerNames[modCfg.Name] = true
185-
}
186-
if modCfg.Type == "openapi" {
201+
for _, dep := range modCfg.DependsOn {
202+
serverToRouter[dep] = modCfg.Name
203+
}
204+
case "openapi":
187205
openAPIDeps[modCfg.Name] = modCfg.DependsOn
188206
}
189207
}
@@ -214,7 +232,7 @@ func wireOpenAPIRoutes(app modular.Application, cfg *config.WorkflowConfig) erro
214232
targetRouter = routers[rName]
215233
}
216234

217-
// 2) dependsOn router reference
235+
// 2) dependsOn: direct router reference
218236
if targetRouter == nil {
219237
for _, dep := range openAPIDeps[oaMod.Name()] {
220238
if routerNames[dep] {
@@ -226,7 +244,19 @@ func wireOpenAPIRoutes(app modular.Application, cfg *config.WorkflowConfig) erro
226244
}
227245
}
228246

229-
// 3) Fall back to first available router
247+
// 3) dependsOn: server reference → follow server→router mapping
248+
if targetRouter == nil {
249+
for _, dep := range openAPIDeps[oaMod.Name()] {
250+
if rName, ok := serverToRouter[dep]; ok {
251+
if router, found := routers[rName]; found {
252+
targetRouter = router
253+
break
254+
}
255+
}
256+
}
257+
}
258+
259+
// 4) Fall back to first available router
230260
if targetRouter == nil {
231261
targetRouter = firstRouter
232262
}

0 commit comments

Comments
 (0)