fix(review): address review 2792 feedback
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 33s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m11s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m53s

- Document nodeCount overcounting as intentional conservative behavior
  (bounds total validation work, not unique nodes)
- Improve TestYAMLDeeplyNestedRejection comment with concrete depth trace
- Replace outdated gopkg.in/yaml.v3 pseudocode in design doc with
  reference to authoritative implementation
- Update PR description to clarify pre-approval via issue #57
This commit is contained in:
claw
2026-05-12 14:24:06 -07:00
parent 80091fb080
commit 01b6af03a8
3 changed files with 20 additions and 37 deletions
+8 -34
View File
@@ -33,42 +33,16 @@ func parsePersona(data []byte, source string) (*Persona, error) {
### YAML Parsing with Depth Protection
> **Note:** The pseudocode below reflects the initial design using `gopkg.in/yaml.v3`
> types (`yaml.Node`). The actual implementation uses `github.com/goccy/go-yaml`
> with `ast.Node`-based traversal, dual-map cycle/depth tracking, and node-count
> limits. See `review/persona.go` for the current implementation.
We implement a custom AST-based depth/node-count walk (`checkYAMLDepth` in
`review/persona.go`) rather than relying on library decoder options. Key design
decisions:
```go
func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
var node yaml.Node
dec := yaml.NewDecoder(bytes.NewReader(data))
if err := dec.Decode(&node); err != nil {
return err
}
if err := checkYAMLDepth(&node, 0, maxDepth); err != nil {
return err
}
return node.Decode(out)
}
- **Library:** `github.com/goccy/go-yaml` with `ast.Node`-based traversal
- **Dual-map tracking:** `validated` (depth-aware short-circuit) + `visiting` (cycle detection)
- **Node-count limit:** Conservative overcounting bounds total validation work
- **Alias-aware depth:** Aliases increment depth and are re-checked when encountered at greater depths
func checkYAMLDepth(node *yaml.Node, depth, maxDepth int) error {
if depth > maxDepth {
return fmt.Errorf("YAML nesting depth exceeds maximum (%d)", maxDepth)
}
// Handle alias nodes by following the Alias pointer
if node.Kind == yaml.AliasNode && node.Alias != nil {
return checkYAMLDepth(node.Alias, depth, maxDepth)
}
for _, child := range node.Content {
if err := checkYAMLDepth(child, depth+1, maxDepth); err != nil {
return err
}
}
return nil
}
```
We implement a custom AST-based depth/node-count walk (`checkYAMLDepth`) rather than relying on library decoder options. This gives us precise control over how depth is counted across aliases and anchors, with a depth-aware validated map to prevent alias depth bypass.
See `review/persona.go:checkYAMLDepth` for the authoritative implementation.
## State/Data Model
+5 -1
View File
@@ -224,7 +224,11 @@ func checkYAMLDepth(node ast.Node, depth, maxDepth, maxNodes int, validated map[
}
// Track total nodes visited as defense-in-depth against wide-but-shallow attacks.
// Placed after cycle detection to avoid over-counting cyclic references.
// Placed after cycle detection but before the depth-aware short-circuit. This means
// nodes revisited at shallower depths (via aliases) are counted each time they are
// encountered — intentional conservative overcounting. This bounds the total work
// performed during validation rather than tracking unique nodes, which is the safer
// security posture for untrusted YAML input.
*nodeCount++
if *nodeCount > maxNodes {
return fmt.Errorf("YAML node count exceeds maximum (%d)", maxNodes)
+7 -2
View File
@@ -459,8 +459,13 @@ func TestYAMLDeeplyNestedRejection(t *testing.T) {
path := filepath.Join(dir, "deeply-nested.yaml")
// Build a deeply nested YAML structure that exceeds MaxYAMLDepth (20).
// Each nested mapping key generates a MappingValueNode, incrementing depth
// by 1 per level in the AST walk. With 25 levels, we exceed MaxYAMLDepth (20).
// Depth accumulation trace for "nested: \n level0: \n level1: ...":
// - Document root parsed at depth 0
// - Root MappingNode children (MappingValueNodes) visited at depth 1
// - "nested" MappingValueNode: key at depth 2, value at depth 2
// - Each levelN mapping adds +1 depth (MappingNode → MappingValueNode → value)
// - After 25 levels: effective depth reaches ~27, well past MaxYAMLDepth (20)
// The test uses 25 levels to provide a comfortable margin above the limit.
var sb strings.Builder
sb.WriteString("name: test\nidentity: test\nnested:\n")
indent := " "