From 411adaac6a8ffe799661239c560f7bc408b5d12e Mon Sep 17 00:00:00 2001 From: Bharat Date: Thu, 29 Oct 2026 16:56:26 +0545 Subject: [PATCH 1/7] feat: validate request payloads before sending (PLAT-1314) --- README.md | 11 ++ cmd/twoctl/cli/extra_test.go | 9 +- cmd/twoctl/cli/operations.go | 2 +- cmd/twoctl/cli/paginate.go | 4 + cmd/twoctl/cli/validation.go | 177 ++++++++++++++++++++++++++++ cmd/twoctl/cli/validation_test.go | 190 ++++++++++++++++++++++++++++++ 6 files changed, 390 insertions(+), 3 deletions(-) create mode 100644 cmd/twoctl/cli/validation.go create mode 100644 cmd/twoctl/cli/validation_test.go diff --git a/README.md b/README.md index c30e81b..596f26e 100644 --- a/README.md +++ b/README.md @@ -187,6 +187,17 @@ Top-level non-resource commands: - `-o table|json|yaml` (default `auto`: table on TTY, JSON when piped). - Errors are emitted as JSON on stderr with shape `{error, message, status, response, request_id}`. Stdout stays clean for piping. +### Request validation + +JSON payloads supplied with `--data`, `--file`, or `--file -` are checked +against the embedded OpenAPI request schema before the API call (including +`--all` and `--dry-run`). Invalid JSON, missing required fields, wrong types, +and enum/constraint mismatches return usage exit code 2 without sending. +Unknown fields in structured objects produce warnings on stderr but are +sent unchanged to tolerate schema drift. Explicitly open and typed maps +retain their schema semantics. Payload values are not printed in validation +errors or warnings. Use `--describe` to inspect the expected schema. + ### Exit codes | Code | Meaning | diff --git a/cmd/twoctl/cli/extra_test.go b/cmd/twoctl/cli/extra_test.go index 5319829..78c2386 100644 --- a/cmd/twoctl/cli/extra_test.go +++ b/cmd/twoctl/cli/extra_test.go @@ -95,8 +95,13 @@ func TestRunOperationWithRequestBody(t *testing.T) { })) defer srv.Close() - stdout, _, err := runRoot(t, "--url", srv.URL, "--api-key", "secret_test_x", - "order", "create", "--data", `{"foo":"bar"}`) + flagURL, flagAPIKey = srv.URL, "secret_test_x" + cmd := buildOperationCommand("test", "/test", "POST", payloadTestOperation(t, `{"type":"object","properties":{"foo":{"type":"string"}}}`)) + var output strings.Builder + cmd.SetOut(&output) + cmd.SetArgs([]string{"--data", `{"foo":"bar"}`}) + err := cmd.Execute() + stdout := output.String() if err != nil { t.Fatal(err) } diff --git a/cmd/twoctl/cli/operations.go b/cmd/twoctl/cli/operations.go index 2386000..2057652 100644 --- a/cmd/twoctl/cli/operations.go +++ b/cmd/twoctl/cli/operations.go @@ -349,7 +349,7 @@ func runOperation(c *cobra.Command, method, path string, op *openapi3.Operation, if err != nil { return err } - body, err := readRequestBody(b, headers) + body, err := readValidatedRequestBody(c.ErrOrStderr(), op, b, headers) if err != nil { return err } diff --git a/cmd/twoctl/cli/paginate.go b/cmd/twoctl/cli/paginate.go index 098291e..23e5dd7 100644 --- a/cmd/twoctl/cli/paginate.go +++ b/cmd/twoctl/cli/paginate.go @@ -77,6 +77,10 @@ func runPaginated(c *cobra.Command, method, path string, op *openapi3.Operation, } } + if err := validatePayload(c.ErrOrStderr(), op, bodyBytes); err != nil { + return err + } + combined, err = fetchAllPages(c, client, resolved.BaseURL, method, path, op, b, bodyBytes, cursorPtr) if err != nil { return err diff --git a/cmd/twoctl/cli/validation.go b/cmd/twoctl/cli/validation.go new file mode 100644 index 0000000..2594bc5 --- /dev/null +++ b/cmd/twoctl/cli/validation.go @@ -0,0 +1,177 @@ +package cli + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "sort" + "strings" + + "github.com/getkin/kin-openapi/openapi3" +) + +// validatePayload checks a private schema copy: embedded schemas are shared by +// all commands and must retain their original describe output. +func validatePayload(w io.Writer, op *openapi3.Operation, raw []byte) error { + if len(raw) == 0 { + return nil + } + var value any + if err := json.Unmarshal(raw, &value); err != nil { + return payloadError("request body must contain one valid JSON value") + } + if op.RequestBody == nil || op.RequestBody.Value == nil { + return nil + } + mt := op.RequestBody.Value.Content.Get("application/json") + if mt == nil || mt.Schema == nil || mt.Schema.Value == nil { + return nil + } + schema := permissiveSchema(mt.Schema.Value, map[*openapi3.Schema]*openapi3.Schema{}) + if err := schema.VisitJSON(value, openapi3.VisitAsRequest()); err != nil { + // Do not render SchemaError.Error(): it includes the submitted payload. + var schemaErr *openapi3.SchemaError + if errors.As(err, &schemaErr) { + return payloadError(fmt.Sprintf("request body violates %s at %q; use --describe to inspect the schema", + schemaErr.SchemaField, "/"+strings.Join(schemaErr.JSONPointer(), "/"))) + } + return payloadError("request body does not match the operation schema; use --describe") + } + warnings := map[string]bool{} + unknownPayloadFields(schema, value, "", warnings) + paths := make([]string, 0, len(warnings)) + for path := range warnings { + paths = append(paths, path) + } + sort.Strings(paths) + for _, path := range paths { + fmt.Fprintf(w, "warning: unknown request property at %q; sending unchanged (the API schema may have drifted)\n", path) + } + return nil +} + +func payloadError(message string) error { + return &apiError{code: "invalid_request_body", message: message, exit: ExitUsage} +} + +func permissiveSchema(s *openapi3.Schema, seen map[*openapi3.Schema]*openapi3.Schema) *openapi3.Schema { + if s == nil { + return nil + } + if copy := seen[s]; copy != nil { + return copy + } + copy := *s + seen[s] = © + clone := func(ref *openapi3.SchemaRef) *openapi3.SchemaRef { + if ref == nil { + return nil + } + r := *ref + r.Value = permissiveSchema(ref.Value, seen) + return &r + } + copy.Properties = openapi3.Schemas{} + for key, ref := range s.Properties { + copy.Properties[key] = clone(ref) + } + copy.Items = clone(s.Items) + copy.AdditionalProperties.Schema = clone(s.AdditionalProperties.Schema) + if s.AdditionalProperties.Has != nil && !*s.AdditionalProperties.Has { + // nil allows additional fields, while preserving explicitly open maps. + copy.AdditionalProperties.Has = nil + } + copy.AllOf = cloneSchemaRefs(s.AllOf, clone) + copy.AnyOf = cloneSchemaRefs(s.AnyOf, clone) + copy.OneOf = cloneSchemaRefs(s.OneOf, clone) + // Negation is a constraint, not an object receiving extra properties. + // Retain it unchanged so relaxing a forbidden shape cannot reject more input. + return © +} + +func cloneSchemaRefs(refs openapi3.SchemaRefs, clone func(*openapi3.SchemaRef) *openapi3.SchemaRef) openapi3.SchemaRefs { + out := make(openapi3.SchemaRefs, len(refs)) + for i, ref := range refs { + out[i] = clone(ref) + } + return out +} + +// payloadSchemas combines applicable branches before checking property names. +// A property declared by one allOf branch is not unknown to its siblings. +func payloadSchemas(s *openapi3.Schema, value any, seen map[*openapi3.Schema]bool) []*openapi3.Schema { + if s == nil || seen[s] { + return nil + } + seen[s] = true + out := []*openapi3.Schema{s} + refs := append(append(append(openapi3.SchemaRefs{}, s.AllOf...), s.AnyOf...), s.OneOf...) + for _, ref := range refs { + if ref != nil && ref.Value != nil && ref.Value.VisitJSON(value, openapi3.VisitAsRequest()) == nil { + out = append(out, payloadSchemas(ref.Value, value, seen)...) + } + } + return out +} + +func unknownPayloadFields(s *openapi3.Schema, value any, path string, warnings map[string]bool) { + schemas := payloadSchemas(s, value, map[*openapi3.Schema]bool{}) + switch v := value.(type) { + case map[string]any: + unknownObjectFields(schemas, v, path, warnings) + case []any: + for _, schema := range schemas { + if schema.Items == nil || schema.Items.Value == nil { + continue + } + for i, item := range v { + unknownPayloadFields(schema.Items.Value, item, fmt.Sprintf("%s/%d", path, i), warnings) + } + } + } +} + +func unknownObjectFields(schemas []*openapi3.Schema, value map[string]any, path string, warnings map[string]bool) { + for key, item := range value { + known, declared, open := false, false, false + childPath := path + "/" + strings.ReplaceAll(strings.ReplaceAll(key, "~", "~0"), "/", "~1") + for _, schema := range schemas { + declared = declared || len(schema.Properties) > 0 + if ref := schema.Properties[key]; ref != nil && ref.Value != nil { + known = true + unknownPayloadFields(ref.Value, item, childPath, warnings) + } + if ref := schema.AdditionalProperties.Schema; ref != nil && ref.Value != nil { + open = true + if schema.Properties[key] == nil { + unknownPayloadFields(ref.Value, item, childPath, warnings) + } + } + open = open || (schema.AdditionalProperties.Has != nil && *schema.AdditionalProperties.Has) + } + if declared && !known && !open { + warnings[childPath] = true + } + } +} + +func readValidatedRequestBody(w io.Writer, op *openapi3.Operation, b *opBindings, headers http.Header) (io.Reader, error) { + if b.bodyFile == nil || b.bodyData == nil { + return nil, nil + } + raw, err := readBody(*b.bodyFile, *b.bodyData) + if err != nil { + return nil, err + } + if err := validatePayload(w, op, raw); err != nil { + return nil, err + } + if len(raw) == 0 { + return nil, nil + } + headers.Set("Content-Type", "application/json") + return bytes.NewReader(raw), nil +} diff --git a/cmd/twoctl/cli/validation_test.go b/cmd/twoctl/cli/validation_test.go new file mode 100644 index 0000000..dd2b567 --- /dev/null +++ b/cmd/twoctl/cli/validation_test.go @@ -0,0 +1,190 @@ +package cli + +import ( + "bytes" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/getkin/kin-openapi/openapi3" +) + +func payloadTestOperation(t *testing.T, schema string) *openapi3.Operation { + t.Helper() + doc, err := openapi3.NewLoader().LoadFromData([]byte(`{"openapi":"3.0.3","info":{"title":"test","version":"1"},"paths":{"/test":{"post":{"requestBody":{"content":{"application/json":{"schema":` + schema + `}}},"responses":{"200":{"description":"ok"}}}}}}`)) + if err != nil { + t.Fatal(err) + } + return doc.Paths.Value("/test").Post +} + +const payloadTestSchema = `{"type":"object","required":["amount","currency"],"additionalProperties":false,"properties":{"amount":{"type":"integer","minimum":1},"currency":{"type":"string","enum":["GBP"]},"lines":{"type":"array","items":{"type":"object","required":["name"],"additionalProperties":false,"properties":{"name":{"type":"string"}}}}}` + +func TestPayloadValidation(t *testing.T) { + op := payloadTestOperation(t, payloadTestSchema) + for _, tc := range []struct { + name, body, warning string + invalid bool + }{ + {"valid", `{"amount":1,"currency":"GBP"}`, "", false}, + {"unknown", `{"amount":1,"currency":"GBP","extra":"secret"}`, "/extra", false}, + {"nested unknown", `{"amount":1,"currency":"GBP","lines":[{"name":"ok","extra":"secret"}]}`, "/lines/0/extra", false}, + {"missing", `{"ammount":1,"currency":"GBP"}`, "", true}, + {"enum", `{"amount":1,"currency":"secret"}`, "", true}, + {"type", `{"amount":"secret","currency":"GBP"}`, "", true}, + {"minimum", `{"amount":0,"currency":"GBP"}`, "", true}, + {"nested required", `{"amount":1,"currency":"GBP","lines":[{}]}`, "", true}, + {"malformed", `{"secret":`, "", true}, + {"multiple values", `{} {}`, "", true}, + {"null", `null`, "", true}, + } { + t.Run(tc.name, func(t *testing.T) { + var warnings bytes.Buffer + err := validatePayload(&warnings, op, []byte(tc.body)) + if (err != nil) != tc.invalid { + t.Fatalf("error = %v", err) + } + if err != nil { + ae, ok := err.(*apiError) + if !ok || ae.exit != ExitUsage || ae.code != "invalid_request_body" { + t.Fatalf("unexpected error: %v", err) + } + if strings.Contains(err.Error(), "secret") { + t.Fatal("payload leaked in error") + } + } + if tc.warning != "" && !strings.Contains(warnings.String(), tc.warning) { + t.Fatalf("missing warning: %s", warnings.String()) + } + if tc.warning == "" && warnings.Len() != 0 { + t.Fatalf("unexpected warning: %s", warnings.String()) + } + if strings.Contains(warnings.String(), "secret") { + t.Fatal("payload value leaked in warning") + } + }) + } + schema := op.RequestBody.Value.Content.Get("application/json").Schema.Value + if schema.AdditionalProperties.Has == nil || *schema.AdditionalProperties.Has { + t.Fatal("shared schema was changed") + } +} + +func TestPayloadCompositionsAndMaps(t *testing.T) { + for _, tc := range []struct { + name, schema, body string + warn, invalid bool + }{ + {"allOf", `{"allOf":[{"type":"object","properties":{"a":{"type":"string"}}},{"type":"object","properties":{"b":{"type":"integer"}}}]}`, `{"a":"x","b":1}`, false, false}, + {"oneOf", `{"oneOf":[{"type":"object","required":["kind"],"properties":{"kind":{"enum":["a"]}}},{"type":"object","required":["kind"],"properties":{"kind":{"enum":["b"]}}}]}`, `{"kind":"b","extra":1}`, true, false}, + {"anyOf", `{"anyOf":[{"type":"string"},{"type":"object","properties":{"a":{"type":"string"}}}]}`, `{"a":"x","extra":1}`, true, false}, + {"open map", `{"type":"object","additionalProperties":true,"properties":{"a":{"type":"string"}}}`, `{"extra":1}`, false, false}, + {"typed map", `{"type":"object","additionalProperties":{"type":"integer"}}`, `{"a":1}`, false, false}, + {"invalid map", `{"type":"object","additionalProperties":{"type":"integer"}}`, `{"a":"x"}`, false, true}, + {"free form", `{"type":"object"}`, `{"extra":1}`, false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + var warnings bytes.Buffer + err := validatePayload(&warnings, payloadTestOperation(t, tc.schema), []byte(tc.body)) + if (err != nil) != tc.invalid || (warnings.Len() != 0) != tc.warn { + t.Fatalf("error=%v warnings=%s", err, warnings.String()) + } + }) + } +} + +func TestPayloadValidationBeforeHTTP(t *testing.T) { + for _, source := range []string{"data", "file", "stdin"} { + for _, invalid := range []bool{false, true} { + t.Run(source+map[bool]string{true: "-invalid", false: "-valid"}[invalid], func(t *testing.T) { + resetGlobals(t) + body := `{"amount":1,"currency":"GBP","extra":"kept"}` + if invalid { + body = `{"amount":1,"currency":"wrong"}` + } + calls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + raw, _ := io.ReadAll(r.Body) + if string(raw) != body { + t.Errorf("body changed: %s", raw) + } + _, _ = w.Write([]byte(`{"ok":true}`)) + })) + defer srv.Close() + flagURL, flagAPIKey = srv.URL, "secret_test_mock" + cmd := buildOperationCommand("test", "/test", "POST", payloadTestOperation(t, payloadTestSchema)) + args := []string{"--data", body} + if source != "data" { + file := filepath.Join(t.TempDir(), "payload.json") + if err := os.WriteFile(file, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + args = []string{"--file", file} + if source == "stdin" { + f, err := os.Open(file) + if err != nil { + t.Fatal(err) + } + defer f.Close() + old := os.Stdin + os.Stdin = f + defer func() { os.Stdin = old }() + args = []string{"--file", "-"} + } + } + var stdout, stderr bytes.Buffer + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.SetArgs(args) + err := cmd.Execute() + if invalid { + if err == nil || calls != 0 || stdout.Len() != 0 { + t.Fatalf("invalid payload sent: calls=%d err=%v output=%s", calls, err, stdout.String()) + } + } else if err != nil || calls != 1 || !strings.Contains(stderr.String(), "/extra") || strings.Contains(stdout.String(), "warning") { + t.Fatalf("calls=%d err=%v stdout=%s stderr=%s", calls, err, stdout.String(), stderr.String()) + } + }) + } + } +} + +func TestPayloadValidationPagination(t *testing.T) { + resetGlobals(t) + op := payloadTestOperation(t, payloadTestSchema) + op.Parameters = openapi3.Parameters{{Value: &openapi3.Parameter{Name: "page_cursor", In: "query", Schema: &openapi3.SchemaRef{Value: openapi3.NewStringSchema()}}}} + calls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + _, _ = w.Write([]byte(`{"items":[]}`)) + })) + defer srv.Close() + flagURL, flagAPIKey = srv.URL, "secret_test_mock" + cmd := buildOperationCommand("test", "/test", "POST", op) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmd.SetArgs([]string{"--all", "--data", `{"amount":1}`}) + if err := cmd.Execute(); err == nil || calls != 0 { + t.Fatalf("invalid paginated body sent: calls=%d err=%v", calls, err) + } +} + +func TestPermissiveSchemaCycle(t *testing.T) { + s := openapi3.NewObjectSchema() + s.Properties = openapi3.Schemas{"child": {Value: s}} + copy := permissiveSchema(s, map[*openapi3.Schema]*openapi3.Schema{}) + if copy == s || copy.Properties["child"].Value != copy { + t.Fatal("schema cycle was not preserved in private copy") + } + if err := validatePayload(io.Discard, &openapi3.Operation{}, nil); err != nil { + t.Fatal(err) + } + if err := validatePayload(io.Discard, &openapi3.Operation{}, []byte(`{}`)); err != nil { + t.Fatal(err) + } +} From 068ee57f97d69f30796c8bd1f034febc6e826a8d Mon Sep 17 00:00:00 2001 From: Bharat Date: Thu, 29 Oct 2026 16:58:00 +0545 Subject: [PATCH 2/7] test: correct payload schema fixture and typo case --- cmd/twoctl/cli/validation_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/twoctl/cli/validation_test.go b/cmd/twoctl/cli/validation_test.go index dd2b567..ab48fca 100644 --- a/cmd/twoctl/cli/validation_test.go +++ b/cmd/twoctl/cli/validation_test.go @@ -22,7 +22,7 @@ func payloadTestOperation(t *testing.T, schema string) *openapi3.Operation { return doc.Paths.Value("/test").Post } -const payloadTestSchema = `{"type":"object","required":["amount","currency"],"additionalProperties":false,"properties":{"amount":{"type":"integer","minimum":1},"currency":{"type":"string","enum":["GBP"]},"lines":{"type":"array","items":{"type":"object","required":["name"],"additionalProperties":false,"properties":{"name":{"type":"string"}}}}}` +const payloadTestSchema = `{"type":"object","required":["amount","currency"],"additionalProperties":false,"properties":{"amount":{"type":"integer","minimum":1},"currency":{"type":"string","enum":["GBP"]},"lines":{"type":"array","items":{"type":"object","required":["name"],"additionalProperties":false,"properties":{"name":{"type":"string"}}}}}}` func TestPayloadValidation(t *testing.T) { op := payloadTestOperation(t, payloadTestSchema) @@ -33,7 +33,7 @@ func TestPayloadValidation(t *testing.T) { {"valid", `{"amount":1,"currency":"GBP"}`, "", false}, {"unknown", `{"amount":1,"currency":"GBP","extra":"secret"}`, "/extra", false}, {"nested unknown", `{"amount":1,"currency":"GBP","lines":[{"name":"ok","extra":"secret"}]}`, "/lines/0/extra", false}, - {"missing", `{"ammount":1,"currency":"GBP"}`, "", true}, + {"missing", `{"amount_typo":1,"currency":"GBP"}`, "", true}, {"enum", `{"amount":1,"currency":"secret"}`, "", true}, {"type", `{"amount":"secret","currency":"GBP"}`, "", true}, {"minimum", `{"amount":0,"currency":"GBP"}`, "", true}, From 209cce76ec954d4f9ba510a86d492528ac9f9ac8 Mon Sep 17 00:00:00 2001 From: Bharat Date: Thu, 29 Oct 2026 16:59:09 +0545 Subject: [PATCH 3/7] test: match root error output policy and simplify source fixtures --- cmd/twoctl/cli/validation_test.go | 46 ++++++++++++++++++------------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/cmd/twoctl/cli/validation_test.go b/cmd/twoctl/cli/validation_test.go index ab48fca..ab8a1d2 100644 --- a/cmd/twoctl/cli/validation_test.go +++ b/cmd/twoctl/cli/validation_test.go @@ -118,25 +118,8 @@ func TestPayloadValidationBeforeHTTP(t *testing.T) { defer srv.Close() flagURL, flagAPIKey = srv.URL, "secret_test_mock" cmd := buildOperationCommand("test", "/test", "POST", payloadTestOperation(t, payloadTestSchema)) - args := []string{"--data", body} - if source != "data" { - file := filepath.Join(t.TempDir(), "payload.json") - if err := os.WriteFile(file, []byte(body), 0o600); err != nil { - t.Fatal(err) - } - args = []string{"--file", file} - if source == "stdin" { - f, err := os.Open(file) - if err != nil { - t.Fatal(err) - } - defer f.Close() - old := os.Stdin - os.Stdin = f - defer func() { os.Stdin = old }() - args = []string{"--file", "-"} - } - } + args := payloadSourceArgs(t, source, body) + cmd.SilenceUsage, cmd.SilenceErrors = true var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -188,3 +171,28 @@ func TestPermissiveSchemaCycle(t *testing.T) { t.Fatal(err) } } + +func payloadSourceArgs(t *testing.T, source, body string) []string { + t.Helper() + if source == "data" { + return []string{"--data", body} + } + file := filepath.Join(t.TempDir(), "payload.json") + if err := os.WriteFile(file, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + if source == "file" { + return []string{"--file", file} + } + f, err := os.Open(file) + if err != nil { + t.Fatal(err) + } + old := os.Stdin + os.Stdin = f + t.Cleanup(func() { + os.Stdin = old + f.Close() + }) + return []string{"--file", "-"} +} From 774e61a49b73fc75412399c3665feb553680b434 Mon Sep 17 00:00:00 2001 From: Bharat Date: Thu, 29 Oct 2026 17:00:27 +0545 Subject: [PATCH 4/7] fix: reject empty required bodies and cover dry-run validation --- cmd/twoctl/cli/validation.go | 3 +++ cmd/twoctl/cli/validation_test.go | 22 +++++++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/cmd/twoctl/cli/validation.go b/cmd/twoctl/cli/validation.go index 2594bc5..8137f75 100644 --- a/cmd/twoctl/cli/validation.go +++ b/cmd/twoctl/cli/validation.go @@ -17,6 +17,9 @@ import ( // all commands and must retain their original describe output. func validatePayload(w io.Writer, op *openapi3.Operation, raw []byte) error { if len(raw) == 0 { + if op.RequestBody != nil && op.RequestBody.Value != nil && op.RequestBody.Value.Required { + return payloadError("request body is required") + } return nil } var value any diff --git a/cmd/twoctl/cli/validation_test.go b/cmd/twoctl/cli/validation_test.go index ab8a1d2..0155d81 100644 --- a/cmd/twoctl/cli/validation_test.go +++ b/cmd/twoctl/cli/validation_test.go @@ -119,7 +119,7 @@ func TestPayloadValidationBeforeHTTP(t *testing.T) { flagURL, flagAPIKey = srv.URL, "secret_test_mock" cmd := buildOperationCommand("test", "/test", "POST", payloadTestOperation(t, payloadTestSchema)) args := payloadSourceArgs(t, source, body) - cmd.SilenceUsage, cmd.SilenceErrors = true + cmd.SilenceUsage, cmd.SilenceErrors = true, true var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -196,3 +196,23 @@ func payloadSourceArgs(t *testing.T, source, body string) []string { }) return []string{"--file", "-"} } + +func TestRequiredEmptyPayload(t *testing.T) { + op := payloadTestOperation(t, payloadTestSchema) + op.RequestBody.Value.Required = true + if err := validatePayload(io.Discard, op, nil); err == nil { + t.Fatal("required empty body accepted") + } +} + +func TestPayloadDryRunValidation(t *testing.T) { + resetGlobals(t) + cmd := buildOperationCommand("test", "/test", "POST", payloadTestOperation(t, payloadTestSchema)) + cmd.SilenceUsage, cmd.SilenceErrors = true, true + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmd.SetArgs([]string{"--dry-run", "--data", `{"amount":1}`}) + if err := cmd.Execute(); err == nil { + t.Fatal("invalid dry-run body accepted") + } +} From a26a0a928052db9c14a005977de962c13fce337f Mon Sep 17 00:00:00 2001 From: Bharat Date: Thu, 29 Oct 2026 17:02:30 +0545 Subject: [PATCH 5/7] fix: preserve closed oneOf discrimination during permissive validation --- cmd/twoctl/cli/validation.go | 77 ++++++++++++++++++++++++++++++- cmd/twoctl/cli/validation_test.go | 21 +++++++++ 2 files changed, 97 insertions(+), 1 deletion(-) diff --git a/cmd/twoctl/cli/validation.go b/cmd/twoctl/cli/validation.go index 8137f75..4b72595 100644 --- a/cmd/twoctl/cli/validation.go +++ b/cmd/twoctl/cli/validation.go @@ -34,7 +34,7 @@ func validatePayload(w io.Writer, op *openapi3.Operation, raw []byte) error { return nil } schema := permissiveSchema(mt.Schema.Value, map[*openapi3.Schema]*openapi3.Schema{}) - if err := schema.VisitJSON(value, openapi3.VisitAsRequest()); err != nil { + if err := validatePermissiveSchema(mt.Schema.Value, schema, value); err != nil { // Do not render SchemaError.Error(): it includes the submitted payload. var schemaErr *openapi3.SchemaError if errors.As(err, &schemaErr) { @@ -178,3 +178,78 @@ func readValidatedRequestBody(w io.Writer, op *openapi3.Operation, b *opBindings headers.Set("Content-Type", "application/json") return bytes.NewReader(raw), nil } + +func validatePermissiveSchema(original, relaxed *openapi3.Schema, value any) error { + // Relaxing closed oneOf branches can make them overlap. Keep an original + // match, or retry without fields unknown to every branch, before treating + // that newly introduced ambiguity as a genuine schema violation. + err := relaxed.VisitJSON(value, openapi3.VisitAsRequest()) + if !errors.Is(err, openapi3.ErrOneOfConflict) { + return err + } + if original.VisitJSON(value, openapi3.VisitAsRequest()) == nil { + return nil + } + return original.VisitJSON(pruneUnknownPayload(original, value), openapi3.VisitAsRequest()) +} + +func allPayloadSchemas(s *openapi3.Schema, seen map[*openapi3.Schema]bool) []*openapi3.Schema { + if s == nil || seen[s] { + return nil + } + seen[s] = true + out := []*openapi3.Schema{s} + refs := append(append(append(openapi3.SchemaRefs{}, s.AllOf...), s.AnyOf...), s.OneOf...) + for _, ref := range refs { + if ref != nil { + out = append(out, allPayloadSchemas(ref.Value, seen)...) + } + } + return out +} + +func pruneUnknownPayload(s *openapi3.Schema, value any) any { + schemas := allPayloadSchemas(s, map[*openapi3.Schema]bool{}) + switch v := value.(type) { + case map[string]any: + return pruneUnknownObject(schemas, v) + case []any: + out := append([]any(nil), v...) + for _, schema := range schemas { + if schema.Items != nil && schema.Items.Value != nil { + for i, item := range out { + out[i] = pruneUnknownPayload(schema.Items.Value, item) + } + } + } + return out + default: + return value + } +} + +func pruneUnknownObject(schemas []*openapi3.Schema, value map[string]any) map[string]any { + props := openapi3.Schemas{} + open, declared := false, false + for _, schema := range schemas { + open = open || schema.AdditionalProperties.Schema != nil || + (schema.AdditionalProperties.Has != nil && *schema.AdditionalProperties.Has) + declared = declared || len(schema.Properties) > 0 || + (schema.AdditionalProperties.Has != nil && !*schema.AdditionalProperties.Has) + for key, ref := range schema.Properties { + if props[key] == nil { + props[key] = &openapi3.SchemaRef{Value: &openapi3.Schema{}} + } + props[key].Value.AllOf = append(props[key].Value.AllOf, ref) + } + } + out := map[string]any{} + for key, item := range value { + if ref := props[key]; ref != nil && ref.Value != nil { + out[key] = pruneUnknownPayload(ref.Value, item) + } else if open || !declared { + out[key] = item + } + } + return out +} diff --git a/cmd/twoctl/cli/validation_test.go b/cmd/twoctl/cli/validation_test.go index 0155d81..268e0da 100644 --- a/cmd/twoctl/cli/validation_test.go +++ b/cmd/twoctl/cli/validation_test.go @@ -216,3 +216,24 @@ func TestPayloadDryRunValidation(t *testing.T) { t.Fatal("invalid dry-run body accepted") } } + +func TestPayloadClosedOneOf(t *testing.T) { + op := payloadTestOperation(t, `{"oneOf":[{"type":"object","additionalProperties":false,"properties":{"a":{"type":"string"}}},{"type":"object","additionalProperties":false,"properties":{"b":{"type":"string"}}}]}`) + for _, body := range []string{`{"a":"x"}`, `{"a":"x","extra":1}`} { + if err := validatePayload(io.Discard, op, []byte(body)); err != nil { + t.Fatalf("valid branch rejected for %s: %v", body, err) + } + } + for _, body := range []string{`{}`, `{"a":1}`, `{"a":"x","b":"x"}`} { + if err := validatePayload(io.Discard, op, []byte(body)); err == nil { + t.Fatalf("invalid oneOf accepted for %s", body) + } + } +} + +func TestPayloadOneOfWithUnrelatedUnknown(t *testing.T) { + op := payloadTestOperation(t, `{"type":"object","properties":{"company":{"oneOf":[{"type":"object","additionalProperties":false,"properties":{"a":{"type":"string"}}},{"type":"object","additionalProperties":false,"properties":{"b":{"type":"string"}}}]},"other":{"type":"object","additionalProperties":false,"properties":{"x":{"type":"string"}}}}}`) + if err := validatePayload(io.Discard, op, []byte(`{"company":{"a":"x"},"other":{"extra":1}}`)); err != nil { + t.Fatal(err) + } +} From 38a4167b55c4a67b9ff839f464706a4391942012 Mon Sep 17 00:00:00 2001 From: Bharat Date: Thu, 29 Oct 2026 17:04:58 +0545 Subject: [PATCH 6/7] fix: preserve original schema constraints with a private payload projection --- cmd/twoctl/cli/validation.go | 153 ++++++++---------------------- cmd/twoctl/cli/validation_test.go | 30 +++++- 2 files changed, 68 insertions(+), 115 deletions(-) diff --git a/cmd/twoctl/cli/validation.go b/cmd/twoctl/cli/validation.go index 4b72595..81e847a 100644 --- a/cmd/twoctl/cli/validation.go +++ b/cmd/twoctl/cli/validation.go @@ -13,8 +13,8 @@ import ( "github.com/getkin/kin-openapi/openapi3" ) -// validatePayload checks a private schema copy: embedded schemas are shared by -// all commands and must retain their original describe output. +// validatePayload checks a private payload projection. Shared embedded schemas +// and the original bytes sent to the API are never modified. func validatePayload(w io.Writer, op *openapi3.Operation, raw []byte) error { if len(raw) == 0 { if op.RequestBody != nil && op.RequestBody.Value != nil && op.RequestBody.Value.Required { @@ -33,8 +33,8 @@ func validatePayload(w io.Writer, op *openapi3.Operation, raw []byte) error { if mt == nil || mt.Schema == nil || mt.Schema.Value == nil { return nil } - schema := permissiveSchema(mt.Schema.Value, map[*openapi3.Schema]*openapi3.Schema{}) - if err := validatePermissiveSchema(mt.Schema.Value, schema, value); err != nil { + schema := mt.Schema.Value + if err := schema.VisitJSON(pruneUnknownPayload(schema, value), openapi3.VisitAsRequest()); err != nil { // Do not render SchemaError.Error(): it includes the submitted payload. var schemaErr *openapi3.SchemaError if errors.As(err, &schemaErr) { @@ -60,68 +60,8 @@ func payloadError(message string) error { return &apiError{code: "invalid_request_body", message: message, exit: ExitUsage} } -func permissiveSchema(s *openapi3.Schema, seen map[*openapi3.Schema]*openapi3.Schema) *openapi3.Schema { - if s == nil { - return nil - } - if copy := seen[s]; copy != nil { - return copy - } - copy := *s - seen[s] = © - clone := func(ref *openapi3.SchemaRef) *openapi3.SchemaRef { - if ref == nil { - return nil - } - r := *ref - r.Value = permissiveSchema(ref.Value, seen) - return &r - } - copy.Properties = openapi3.Schemas{} - for key, ref := range s.Properties { - copy.Properties[key] = clone(ref) - } - copy.Items = clone(s.Items) - copy.AdditionalProperties.Schema = clone(s.AdditionalProperties.Schema) - if s.AdditionalProperties.Has != nil && !*s.AdditionalProperties.Has { - // nil allows additional fields, while preserving explicitly open maps. - copy.AdditionalProperties.Has = nil - } - copy.AllOf = cloneSchemaRefs(s.AllOf, clone) - copy.AnyOf = cloneSchemaRefs(s.AnyOf, clone) - copy.OneOf = cloneSchemaRefs(s.OneOf, clone) - // Negation is a constraint, not an object receiving extra properties. - // Retain it unchanged so relaxing a forbidden shape cannot reject more input. - return © -} - -func cloneSchemaRefs(refs openapi3.SchemaRefs, clone func(*openapi3.SchemaRef) *openapi3.SchemaRef) openapi3.SchemaRefs { - out := make(openapi3.SchemaRefs, len(refs)) - for i, ref := range refs { - out[i] = clone(ref) - } - return out -} - -// payloadSchemas combines applicable branches before checking property names. -// A property declared by one allOf branch is not unknown to its siblings. -func payloadSchemas(s *openapi3.Schema, value any, seen map[*openapi3.Schema]bool) []*openapi3.Schema { - if s == nil || seen[s] { - return nil - } - seen[s] = true - out := []*openapi3.Schema{s} - refs := append(append(append(openapi3.SchemaRefs{}, s.AllOf...), s.AnyOf...), s.OneOf...) - for _, ref := range refs { - if ref != nil && ref.Value != nil && ref.Value.VisitJSON(value, openapi3.VisitAsRequest()) == nil { - out = append(out, payloadSchemas(ref.Value, value, seen)...) - } - } - return out -} - func unknownPayloadFields(s *openapi3.Schema, value any, path string, warnings map[string]bool) { - schemas := payloadSchemas(s, value, map[*openapi3.Schema]bool{}) + schemas := allPayloadSchemas(s, map[*openapi3.Schema]bool{}) switch v := value.(type) { case map[string]any: unknownObjectFields(schemas, v, path, warnings) @@ -138,24 +78,14 @@ func unknownPayloadFields(s *openapi3.Schema, value any, path string, warnings m } func unknownObjectFields(schemas []*openapi3.Schema, value map[string]any, path string, warnings map[string]bool) { + shape := payloadObjectShape(schemas) for key, item := range value { - known, declared, open := false, false, false childPath := path + "/" + strings.ReplaceAll(strings.ReplaceAll(key, "~", "~0"), "/", "~1") - for _, schema := range schemas { - declared = declared || len(schema.Properties) > 0 - if ref := schema.Properties[key]; ref != nil && ref.Value != nil { - known = true - unknownPayloadFields(ref.Value, item, childPath, warnings) - } - if ref := schema.AdditionalProperties.Schema; ref != nil && ref.Value != nil { - open = true - if schema.Properties[key] == nil { - unknownPayloadFields(ref.Value, item, childPath, warnings) - } - } - open = open || (schema.AdditionalProperties.Has != nil && *schema.AdditionalProperties.Has) - } - if declared && !known && !open { + if ref := shape.props[key]; ref != nil { + unknownPayloadFields(ref.Value, item, childPath, warnings) + } else if shape.open { + unknownPayloadFields(shape.extra, item, childPath, warnings) + } else if shape.declared { warnings[childPath] = true } } @@ -179,20 +109,6 @@ func readValidatedRequestBody(w io.Writer, op *openapi3.Operation, b *opBindings return bytes.NewReader(raw), nil } -func validatePermissiveSchema(original, relaxed *openapi3.Schema, value any) error { - // Relaxing closed oneOf branches can make them overlap. Keep an original - // match, or retry without fields unknown to every branch, before treating - // that newly introduced ambiguity as a genuine schema violation. - err := relaxed.VisitJSON(value, openapi3.VisitAsRequest()) - if !errors.Is(err, openapi3.ErrOneOfConflict) { - return err - } - if original.VisitJSON(value, openapi3.VisitAsRequest()) == nil { - return nil - } - return original.VisitJSON(pruneUnknownPayload(original, value), openapi3.VisitAsRequest()) -} - func allPayloadSchemas(s *openapi3.Schema, seen map[*openapi3.Schema]bool) []*openapi3.Schema { if s == nil || seen[s] { return nil @@ -214,41 +130,56 @@ func pruneUnknownPayload(s *openapi3.Schema, value any) any { case map[string]any: return pruneUnknownObject(schemas, v) case []any: - out := append([]any(nil), v...) + items := &openapi3.Schema{} for _, schema := range schemas { - if schema.Items != nil && schema.Items.Value != nil { - for i, item := range out { - out[i] = pruneUnknownPayload(schema.Items.Value, item) - } + if schema.Items != nil { + items.AllOf = append(items.AllOf, schema.Items) } } + out := make([]any, len(v)) + for i, item := range v { + out[i] = pruneUnknownPayload(items, item) + } return out default: return value } } -func pruneUnknownObject(schemas []*openapi3.Schema, value map[string]any) map[string]any { - props := openapi3.Schemas{} - open, declared := false, false +type payloadShape struct { + props openapi3.Schemas + extra *openapi3.Schema + open, declared bool +} + +func payloadObjectShape(schemas []*openapi3.Schema) payloadShape { + shape := payloadShape{props: openapi3.Schemas{}, extra: &openapi3.Schema{}} for _, schema := range schemas { - open = open || schema.AdditionalProperties.Schema != nil || + if schema.AdditionalProperties.Schema != nil { + shape.extra.AllOf = append(shape.extra.AllOf, schema.AdditionalProperties.Schema) + } + shape.open = shape.open || schema.AdditionalProperties.Schema != nil || (schema.AdditionalProperties.Has != nil && *schema.AdditionalProperties.Has) - declared = declared || len(schema.Properties) > 0 || + shape.declared = shape.declared || len(schema.Properties) > 0 || (schema.AdditionalProperties.Has != nil && !*schema.AdditionalProperties.Has) for key, ref := range schema.Properties { - if props[key] == nil { - props[key] = &openapi3.SchemaRef{Value: &openapi3.Schema{}} + if shape.props[key] == nil { + shape.props[key] = &openapi3.SchemaRef{Value: &openapi3.Schema{}} } - props[key].Value.AllOf = append(props[key].Value.AllOf, ref) + shape.props[key].Value.AllOf = append(shape.props[key].Value.AllOf, ref) } } + return shape +} + +func pruneUnknownObject(schemas []*openapi3.Schema, value map[string]any) map[string]any { + shape := payloadObjectShape(schemas) out := map[string]any{} for key, item := range value { - if ref := props[key]; ref != nil && ref.Value != nil { + if ref := shape.props[key]; ref != nil { out[key] = pruneUnknownPayload(ref.Value, item) - } else if open || !declared { - out[key] = item + } else if shape.open || !shape.declared { + out[key] = pruneUnknownPayload(shape.extra, item) } } return out diff --git a/cmd/twoctl/cli/validation_test.go b/cmd/twoctl/cli/validation_test.go index 268e0da..8cba3d7 100644 --- a/cmd/twoctl/cli/validation_test.go +++ b/cmd/twoctl/cli/validation_test.go @@ -157,12 +157,16 @@ func TestPayloadValidationPagination(t *testing.T) { } } -func TestPermissiveSchemaCycle(t *testing.T) { +func TestPayloadProjectionCycle(t *testing.T) { s := openapi3.NewObjectSchema() s.Properties = openapi3.Schemas{"child": {Value: s}} - copy := permissiveSchema(s, map[*openapi3.Schema]*openapi3.Schema{}) - if copy == s || copy.Properties["child"].Value != copy { - t.Fatal("schema cycle was not preserved in private copy") + original := map[string]any{"child": map[string]any{"extra": "kept"}} + projected := pruneUnknownPayload(s, original).(map[string]any) + if len(projected["child"].(map[string]any)) != 0 { + t.Fatal("nested unknown field was not removed from validation projection") + } + if original["child"].(map[string]any)["extra"] != "kept" { + t.Fatal("original payload changed") } if err := validatePayload(io.Discard, &openapi3.Operation{}, nil); err != nil { t.Fatal(err) @@ -237,3 +241,21 @@ func TestPayloadOneOfWithUnrelatedUnknown(t *testing.T) { t.Fatal(err) } } + +func TestPayloadArrayComposition(t *testing.T) { + op := payloadTestOperation(t, `{"allOf":[{"type":"array","items":{"type":"object","properties":{"a":{"type":"string"}}}},{"type":"array","items":{"type":"object","properties":{"b":{"type":"string"}},"required":["b"]}}]}`) + if err := validatePayload(io.Discard, op, []byte(`[{"a":"x","b":"y"}]`)); err != nil { + t.Fatal(err) + } +} + +func TestPayloadTypedMapNestedUnknown(t *testing.T) { + op := payloadTestOperation(t, `{"type":"object","additionalProperties":{"type":"object","additionalProperties":false,"required":["a"],"properties":{"a":{"type":"string"}}}}`) + var warnings bytes.Buffer + if err := validatePayload(&warnings, op, []byte(`{"entry":{"a":"x","extra":1}}`)); err != nil { + t.Fatal(err) + } + if !strings.Contains(warnings.String(), "/entry/extra") { + t.Fatal("missing typed-map child warning") + } +} From 2b7647cf5acea82380c7d0e7eeea7dfd7653c8ef Mon Sep 17 00:00:00 2001 From: Bharat Date: Thu, 29 Oct 2026 17:05:58 +0545 Subject: [PATCH 7/7] fix: use composition item union for unknown-field warnings --- cmd/twoctl/cli/validation.go | 27 ++++++++++++++------------- cmd/twoctl/cli/validation_test.go | 6 +++++- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/cmd/twoctl/cli/validation.go b/cmd/twoctl/cli/validation.go index 81e847a..92af35f 100644 --- a/cmd/twoctl/cli/validation.go +++ b/cmd/twoctl/cli/validation.go @@ -66,13 +66,9 @@ func unknownPayloadFields(s *openapi3.Schema, value any, path string, warnings m case map[string]any: unknownObjectFields(schemas, v, path, warnings) case []any: - for _, schema := range schemas { - if schema.Items == nil || schema.Items.Value == nil { - continue - } - for i, item := range v { - unknownPayloadFields(schema.Items.Value, item, fmt.Sprintf("%s/%d", path, i), warnings) - } + items := payloadItemSchema(schemas) + for i, item := range v { + unknownPayloadFields(items, item, fmt.Sprintf("%s/%d", path, i), warnings) } } } @@ -130,12 +126,7 @@ func pruneUnknownPayload(s *openapi3.Schema, value any) any { case map[string]any: return pruneUnknownObject(schemas, v) case []any: - items := &openapi3.Schema{} - for _, schema := range schemas { - if schema.Items != nil { - items.AllOf = append(items.AllOf, schema.Items) - } - } + items := payloadItemSchema(schemas) out := make([]any, len(v)) for i, item := range v { out[i] = pruneUnknownPayload(items, item) @@ -184,3 +175,13 @@ func pruneUnknownObject(schemas []*openapi3.Schema, value map[string]any) map[st } return out } + +func payloadItemSchema(schemas []*openapi3.Schema) *openapi3.Schema { + items := &openapi3.Schema{} + for _, schema := range schemas { + if schema.Items != nil { + items.AllOf = append(items.AllOf, schema.Items) + } + } + return items +} diff --git a/cmd/twoctl/cli/validation_test.go b/cmd/twoctl/cli/validation_test.go index 8cba3d7..00f7216 100644 --- a/cmd/twoctl/cli/validation_test.go +++ b/cmd/twoctl/cli/validation_test.go @@ -244,9 +244,13 @@ func TestPayloadOneOfWithUnrelatedUnknown(t *testing.T) { func TestPayloadArrayComposition(t *testing.T) { op := payloadTestOperation(t, `{"allOf":[{"type":"array","items":{"type":"object","properties":{"a":{"type":"string"}}}},{"type":"array","items":{"type":"object","properties":{"b":{"type":"string"}},"required":["b"]}}]}`) - if err := validatePayload(io.Discard, op, []byte(`[{"a":"x","b":"y"}]`)); err != nil { + var warnings bytes.Buffer + if err := validatePayload(&warnings, op, []byte(`[{"a":"x","b":"y"}]`)); err != nil { t.Fatal(err) } + if warnings.Len() != 0 { + t.Fatalf("known composition properties warned: %s", warnings.String()) + } } func TestPayloadTypedMapNestedUnknown(t *testing.T) {