fix(review): address review 2792 feedback
- 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:
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user