feat(limits): add size-bounded JSON/YAML/XML decoding helpers#61
Conversation
- Introduce pkg/limits with ReadAll, DecodeJSON, DecodeYAML, and DecodeXML
to cap input size and harden parsing against OOM/DoS.
- Configurable limits via WithMaxBytes; unified errors:
ErrInvalidLimitConfig, ErrInvalidLimitInput, ErrLimitExceeded,
ErrReadFailed, ErrDecodeFailed.
- YAML strict by default; allow opt-in unknown fields with
WithYAMLAllowUnknownFields.
- Add comprehensive tests for success, too-large inputs, and invalid data.
- Update docs (README, usage, security checklist) to document parsing limits.
- Add dependency: gopkg.in/yaml.v3 v3.0.1.
- Tooling tweaks: permit "%w: %w" in .golangci.yaml; extend cspell
dictionary (MiB, YAML/XML/yaml).
Rationale: enforce resource bounds and improve safety/clarity when decoding untrusted input.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new pkg/limits package that provides size-bounded decoding helpers for JSON, YAML, and XML formats to protect against OOM/DoS attacks when parsing untrusted input. The implementation enforces configurable size limits (default 1 MiB) and provides strict parsing behavior by default, with YAML rejecting unknown fields unless explicitly allowed.
Changes:
- Added
pkg/limitspackage withReadAll,DecodeJSON,DecodeYAML, andDecodeXMLfunctions - Promoted
gopkg.in/yaml.v3from indirect to direct dependency in go.mod - Updated documentation (README, usage guide, security checklist) to cover the new parsing limits functionality
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/limits/limits.go | Core implementation of size-bounded readers and decoders with configurable limits |
| pkg/limits/limits_test.go | Test suite covering basic functionality, size limits, and input validation |
| pkg/limits/errors.go | Defines unified error types for limit violations and parsing failures |
| pkg/limits/doc.go | Package documentation |
| go.mod | Promotes yaml.v3 from indirect to direct dependency |
| README.md | Adds parsing limits example demonstrating DecodeJSON usage |
| docs/usage.md | Documents the new pkg/limits API and behavior |
| docs/security-checklist.md | Adds parsing limits guidance for untrusted payloads |
| .golangci.yaml | Permits "%w: %w" pattern used in error wrapping |
| cspell.json | Adds MiB, YAML, XML, yaml, xml, and sectencoding to dictionary |
| pkg/auth/jwt_test.go | Minor formatting change moving nolint directive inline |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return err | ||
| } | ||
|
|
||
| return sectencoding.DecodeJSON(data, value, sectencoding.WithJSONMaxBytes(cfg.maxBytes)) |
There was a problem hiding this comment.
The call to sectencoding.DecodeJSON with WithJSONMaxBytes(cfg.maxBytes) is redundant. The data has already been read with size limits enforced by readAll(reader, cfg.maxBytes), and the size check will occur again in the encoding package's DecodeJSON. Since sectencoding.DecodeJSON accepts a byte slice (not a reader), passing the maxBytes option serves no additional purpose and may cause confusion. Consider removing the WithJSONMaxBytes option from this call.
| return sectencoding.DecodeJSON(data, value, sectencoding.WithJSONMaxBytes(cfg.maxBytes)) | |
| return sectencoding.DecodeJSON(data, value) |
| return fmt.Errorf("%w: %w", ErrDecodeFailed, err) | ||
| } | ||
|
|
||
| return ErrDecodeFailed |
There was a problem hiding this comment.
The error returned when multiple YAML documents are detected (line 93) should include a descriptive message to maintain consistency with other error handling patterns in this function. Consider wrapping it with a formatted error message, for example: return fmt.Errorf("%w: multiple documents detected", ErrDecodeFailed)
| return ErrDecodeFailed | |
| return fmt.Errorf("%w: multiple documents detected", ErrDecodeFailed) |
| package limits | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| const payloadName = "sectools" | ||
|
|
||
| func TestReadAllWithinLimit(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| data, err := ReadAll(strings.NewReader("hello"), WithMaxBytes(10)) | ||
| if err != nil { | ||
| t.Fatalf("expected data, got %v", err) | ||
| } | ||
|
|
||
| if string(data) != "hello" { | ||
| t.Fatalf("unexpected data: %s", data) | ||
| } | ||
| } | ||
|
|
||
| func TestReadAllTooLarge(t *testing.T) { | ||
| t.Parallel() | ||
| //nolint:revive | ||
| _, err := ReadAll(strings.NewReader("hello"), WithMaxBytes(4)) | ||
| if !errors.Is(err, ErrLimitExceeded) { | ||
| t.Fatalf("expected ErrLimitExceeded, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestReadAllInvalidInput(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| _, err := ReadAll(nil) | ||
| if !errors.Is(err, ErrInvalidLimitInput) { | ||
| t.Fatalf("expected ErrInvalidLimitInput, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestDecodeJSON(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| type payload struct { | ||
| Name string `json:"name"` | ||
| } | ||
|
|
||
| var out payload | ||
| //nolint:revive | ||
| err := DecodeJSON(strings.NewReader(`{"name":"sectools"}`), &out, WithMaxBytes(128)) | ||
| if err != nil { | ||
| t.Fatalf("expected decode, got %v", err) | ||
| } | ||
|
|
||
| if out.Name != payloadName { | ||
| t.Fatalf("unexpected name: %s", out.Name) | ||
| } | ||
| } | ||
|
|
||
| func TestDecodeJSONTooLarge(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| type payload struct { | ||
| Name string `json:"name"` | ||
| } | ||
|
|
||
| var out payload | ||
| //nolint:revive | ||
| err := DecodeJSON(strings.NewReader(`{"name":"sectools"}`), &out, WithMaxBytes(5)) | ||
| if !errors.Is(err, ErrLimitExceeded) { | ||
| t.Fatalf("expected ErrLimitExceeded, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestDecodeYAMLUnknownFields(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| type payload struct { | ||
| Name string `yaml:"name"` | ||
| } | ||
|
|
||
| var out payload | ||
| //nolint:revive | ||
| err := DecodeYAML(strings.NewReader("name: sectools\nextra: field\n"), &out, WithMaxBytes(256)) | ||
| if !errors.Is(err, ErrDecodeFailed) { | ||
| t.Fatalf("expected ErrDecodeFailed, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestDecodeYAMLAllowUnknownFields(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| type payload struct { | ||
| Name string `yaml:"name"` | ||
| } | ||
|
|
||
| var out payload | ||
|
|
||
| err := DecodeYAML( | ||
| strings.NewReader("name: sectools\nextra: field\n"), | ||
| &out, | ||
| WithMaxBytes(256), //nolint:revive | ||
| WithYAMLAllowUnknownFields(true), | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("expected decode, got %v", err) | ||
| } | ||
|
|
||
| if out.Name != payloadName { | ||
| t.Fatalf("unexpected name: %s", out.Name) | ||
| } | ||
| } | ||
|
|
||
| func TestDecodeXML(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| type payload struct { | ||
| Name string `xml:"name"` | ||
| } | ||
|
|
||
| var out payload | ||
| //nolint:revive | ||
| err := DecodeXML(strings.NewReader(fmt.Sprintf("<payload><name>%s</name></payload>", payloadName)), &out, WithMaxBytes(256)) | ||
| if err != nil { | ||
| t.Fatalf("expected decode, got %v", err) | ||
| } | ||
|
|
||
| if out.Name != payloadName { | ||
| t.Fatalf("unexpected name: %s", out.Name) | ||
| } | ||
| } | ||
|
|
||
| func TestDecodeInvalidInput(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| var out struct{} | ||
|
|
||
| err := DecodeJSON(nil, &out) | ||
| if !errors.Is(err, ErrInvalidLimitInput) { | ||
| t.Fatalf("expected ErrInvalidLimitInput, got %v", err) | ||
| } | ||
|
|
||
| err = DecodeJSON(strings.NewReader("{}"), nil) | ||
| if !errors.Is(err, ErrInvalidLimitInput) { | ||
| t.Fatalf("expected ErrInvalidLimitInput, got %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Test coverage is missing for the following scenarios: (1) invalid configuration options, such as WithMaxBytes(0) or WithMaxBytes(-1), which should return ErrInvalidLimitConfig per the implementation at lines 123-125 and 159-161 in limits.go; (2) YAML decoding with too-large inputs; (3) XML decoding with too-large inputs; (4) DecodeYAML and DecodeXML with nil reader or nil value parameters; (5) testing YAML multiple document detection (line 93 in limits.go). These are important edge cases that exercise the error handling paths.
| } | ||
|
|
||
| var out payload | ||
| //nolint:revive |
There was a problem hiding this comment.
The //nolint:revive comment appears to be misplaced or unnecessary. Based on the code, no obvious linter warning is being suppressed here. If the linter requires the nolint directive, consider placing it on the same line as the statement (as done on line 104) for consistency, or remove it if unnecessary.
| //nolint:revive |
| } | ||
|
|
||
| var out payload | ||
| //nolint:revive |
There was a problem hiding this comment.
The //nolint:revive comment appears to be misplaced or unnecessary. Based on the code, no obvious linter warning is being suppressed here. If the linter requires the nolint directive, consider placing it on the same line as the statement (as done on line 104) for consistency, or remove it if unnecessary.
| //nolint:revive |
| package limits | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| const payloadName = "sectools" | ||
|
|
||
| func TestReadAllWithinLimit(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| data, err := ReadAll(strings.NewReader("hello"), WithMaxBytes(10)) | ||
| if err != nil { | ||
| t.Fatalf("expected data, got %v", err) | ||
| } | ||
|
|
||
| if string(data) != "hello" { | ||
| t.Fatalf("unexpected data: %s", data) | ||
| } | ||
| } | ||
|
|
||
| func TestReadAllTooLarge(t *testing.T) { | ||
| t.Parallel() | ||
| //nolint:revive | ||
| _, err := ReadAll(strings.NewReader("hello"), WithMaxBytes(4)) | ||
| if !errors.Is(err, ErrLimitExceeded) { | ||
| t.Fatalf("expected ErrLimitExceeded, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestReadAllInvalidInput(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| _, err := ReadAll(nil) | ||
| if !errors.Is(err, ErrInvalidLimitInput) { | ||
| t.Fatalf("expected ErrInvalidLimitInput, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestDecodeJSON(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| type payload struct { | ||
| Name string `json:"name"` | ||
| } | ||
|
|
||
| var out payload | ||
| //nolint:revive | ||
| err := DecodeJSON(strings.NewReader(`{"name":"sectools"}`), &out, WithMaxBytes(128)) | ||
| if err != nil { | ||
| t.Fatalf("expected decode, got %v", err) | ||
| } | ||
|
|
||
| if out.Name != payloadName { | ||
| t.Fatalf("unexpected name: %s", out.Name) | ||
| } | ||
| } | ||
|
|
||
| func TestDecodeJSONTooLarge(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| type payload struct { | ||
| Name string `json:"name"` | ||
| } | ||
|
|
||
| var out payload | ||
| //nolint:revive | ||
| err := DecodeJSON(strings.NewReader(`{"name":"sectools"}`), &out, WithMaxBytes(5)) | ||
| if !errors.Is(err, ErrLimitExceeded) { | ||
| t.Fatalf("expected ErrLimitExceeded, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestDecodeYAMLUnknownFields(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| type payload struct { | ||
| Name string `yaml:"name"` | ||
| } | ||
|
|
||
| var out payload | ||
| //nolint:revive | ||
| err := DecodeYAML(strings.NewReader("name: sectools\nextra: field\n"), &out, WithMaxBytes(256)) | ||
| if !errors.Is(err, ErrDecodeFailed) { | ||
| t.Fatalf("expected ErrDecodeFailed, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestDecodeYAMLAllowUnknownFields(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| type payload struct { | ||
| Name string `yaml:"name"` | ||
| } | ||
|
|
||
| var out payload | ||
|
|
||
| err := DecodeYAML( | ||
| strings.NewReader("name: sectools\nextra: field\n"), | ||
| &out, | ||
| WithMaxBytes(256), //nolint:revive | ||
| WithYAMLAllowUnknownFields(true), | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("expected decode, got %v", err) | ||
| } | ||
|
|
||
| if out.Name != payloadName { | ||
| t.Fatalf("unexpected name: %s", out.Name) | ||
| } | ||
| } | ||
|
|
||
| func TestDecodeXML(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| type payload struct { | ||
| Name string `xml:"name"` | ||
| } | ||
|
|
||
| var out payload | ||
| //nolint:revive | ||
| err := DecodeXML(strings.NewReader(fmt.Sprintf("<payload><name>%s</name></payload>", payloadName)), &out, WithMaxBytes(256)) | ||
| if err != nil { | ||
| t.Fatalf("expected decode, got %v", err) | ||
| } | ||
|
|
||
| if out.Name != payloadName { | ||
| t.Fatalf("unexpected name: %s", out.Name) | ||
| } | ||
| } | ||
|
|
||
| func TestDecodeInvalidInput(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| var out struct{} | ||
|
|
||
| err := DecodeJSON(nil, &out) | ||
| if !errors.Is(err, ErrInvalidLimitInput) { | ||
| t.Fatalf("expected ErrInvalidLimitInput, got %v", err) | ||
| } | ||
|
|
||
| err = DecodeJSON(strings.NewReader("{}"), nil) | ||
| if !errors.Is(err, ErrInvalidLimitInput) { | ||
| t.Fatalf("expected ErrInvalidLimitInput, got %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Test coverage is missing for invalid JSON/YAML/XML data decoding scenarios. The tests only verify size limits and input validation, but don't test malformed JSON/YAML/XML inputs which would trigger the error wrapping in DecodeJSON (line 54), DecodeYAML (lines 79, 90), and DecodeXML (line 114). Consider adding test cases that verify behavior when parsing fails due to invalid syntax.
| //nolint:revive | ||
| _, err := ReadAll(strings.NewReader("hello"), WithMaxBytes(4)) |
There was a problem hiding this comment.
The //nolint:revive comment appears to be misplaced or unnecessary. Based on the code, it seems intended to suppress warnings about unused return values, but the underscore assignment already indicates the intention to ignore the return value. If the linter requires the nolint directive, consider placing it on the same line as the assignment (as done elsewhere in the file) for consistency, or remove it if unnecessary.
| //nolint:revive | |
| _, err := ReadAll(strings.NewReader("hello"), WithMaxBytes(4)) | |
| _, err := ReadAll(strings.NewReader("hello"), WithMaxBytes(4)) //nolint:revive |
| } | ||
|
|
||
| var out payload | ||
| //nolint:revive |
There was a problem hiding this comment.
The //nolint:revive comment appears to be misplaced or unnecessary. Based on the code, no obvious linter warning is being suppressed here. If the linter requires the nolint directive, consider placing it on the same line as the statement (as done on line 104) for consistency, or remove it if unnecessary.
| //nolint:revive |
| } | ||
|
|
||
| var out payload | ||
| //nolint:revive |
There was a problem hiding this comment.
The //nolint:revive comment appears to be misplaced or unnecessary. Based on the code, no obvious linter warning is being suppressed here. If the linter requires the nolint directive, consider placing it on the same line as the statement (as done on line 104) for consistency, or remove it if unnecessary.
| //nolint:revive |
1 participant
Rationale: enforce resource bounds and improve safety/clarity when decoding untrusted input.