diff --git a/docs/DESIGN-57-yaml-persona.md b/docs/DESIGN-57-yaml-persona.md index deafae9..628e0a6 100644 --- a/docs/DESIGN-57-yaml-persona.md +++ b/docs/DESIGN-57-yaml-persona.md @@ -34,21 +34,36 @@ func parsePersona(data []byte, source string) (*Persona, error) { ### YAML Parsing with Depth Protection ```go -func parseYAML(data []byte, source string) (*Persona, error) { - var p Persona - // gopkg.in/yaml.v3 does NOT have built-in depth limiting. - // Use explicit depth check via yaml.Node API. - if err := yaml.Unmarshal(data, &p); err != nil { - return nil, fmt.Errorf("parse persona %s: %w", source, err) +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 := validatePersona(&p, source); err != nil { - return nil, err + if err := checkYAMLDepth(&node, 0, maxDepth); err != nil { + return err } - return &p, nil + return node.Decode(out) +} + +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 } ``` -The `gopkg.in/yaml.v3` library does not have built-in depth protection, so we implement explicit depth checking by first decoding into a `yaml.Node`, walking the tree to verify depth, then decoding into the target struct. +The `gopkg.in/yaml.v3` library does not have built-in depth protection, so we implement explicit depth checking by first decoding into a `yaml.Node`, walking the tree to verify depth (including alias resolution), then decoding into the target struct. ## State/Data Model diff --git a/review/persona.go b/review/persona.go index bd61c18..2dc7a05 100644 --- a/review/persona.go +++ b/review/persona.go @@ -52,6 +52,9 @@ func LoadPersona(path string) (*Persona, error) { if err != nil { return nil, fmt.Errorf("read persona file %s: %w", path, err) } + if !info.Mode().IsRegular() { + return nil, fmt.Errorf("persona file %s is not a regular file", path) + } if info.Size() > MaxPersonaFileSize { return nil, fmt.Errorf("persona file %s exceeds maximum size (%d bytes)", path, MaxPersonaFileSize) } @@ -59,6 +62,11 @@ func LoadPersona(path string) (*Persona, error) { if err != nil { return nil, fmt.Errorf("read persona file %s: %w", path, err) } + // Re-check size after read to defend against TOCTOU races where file + // grows between stat and read (e.g., appending process, replaced file). + if len(data) > MaxPersonaFileSize { + return nil, fmt.Errorf("persona file %s exceeds maximum size (%d bytes)", path, MaxPersonaFileSize) + } return parsePersona(data, path) } @@ -144,7 +152,10 @@ func parsePersona(data []byte, source string) (*Persona, error) { // unmarshalYAMLWithDepthLimit unmarshals YAML data with explicit depth limiting. // This protects against stack exhaustion from deeply nested structures. -func unmarshalYAMLWithDepthLimit(data []byte, out interface{}, maxDepth int) error { +// Note: Multi-document YAML files are accepted but only the first document is +// parsed; additional documents are silently ignored. This is acceptable for +// persona files where multi-document support is not a use case. +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 { @@ -159,10 +170,16 @@ func unmarshalYAMLWithDepthLimit(data []byte, out interface{}, maxDepth int) err } // checkYAMLDepth recursively checks that YAML nodes don't exceed the depth limit. +// Handles alias nodes by following the Alias pointer to check the target's depth. 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: follow the alias to its anchor target. + // The alias itself doesn't add depth, but we must check the target. + 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