Skip to content

Commit 7535ba1

Browse files
committed
fix: address Copilot review comments on yamlutil PR
- Fix broken doc link: docs/security/adversarial-markdown.md → docs/security/2026-04-05-adversarial-markdown.md - convention.go: revert validateConventionScalar to plain yaml.Unmarshal; node parsing never expands aliases so UnmarshalNodeSafe's alias scan was redundant - requiredstructure: remove double alias check in front-matter path; keep explicit RejectYAMLAliases + yaml.Unmarshal to preserve distinct error messages with a single decode pass - plan/121: correct UnmarshalNodeSafe signature (value not pointer), document validateConventionScalar exception and front-matter error-message split, fix test-migration note https://claude.ai/code/session_01WnJMsRhRBvtMHQX1seKaQW
1 parent 27647ba commit 7535ba1

4 files changed

Lines changed: 20 additions & 14 deletions

File tree

internal/config/convention.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"gopkg.in/yaml.v3"
88

99
"github.com/jeduden/mdsmith/internal/rules/markdownflavor"
10-
"github.com/jeduden/mdsmith/internal/yamlutil"
1110
)
1211

1312
// applyConvention reads the top-level Convention selector from the
@@ -98,10 +97,11 @@ func copyConventionPreset(p map[string]RuleCfg) map[string]RuleCfg {
9897
// clean type error. Inspecting the raw node tag is the only way to
9998
// catch the type mismatch before that coercion happens.
10099
func validateConventionScalar(data []byte) error {
101-
node, err := yamlutil.UnmarshalNodeSafe(data)
102-
if err != nil {
103-
// A YAML parse error is reported by Load's yaml.Unmarshal
104-
// call already; do not double-report.
100+
// yaml.Unmarshal into yaml.Node does not expand aliases, so this
101+
// is safe without an alias-rejection pre-check. Errors are swallowed
102+
// because Load's subsequent UnmarshalSafe call will surface them.
103+
var node yaml.Node
104+
if err := yaml.Unmarshal(data, &node); err != nil {
105105
return nil
106106
}
107107
if node.Kind != yaml.DocumentNode || len(node.Content) == 0 {

internal/rules/requiredstructure/rule.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
rulesettings "github.com/jeduden/mdsmith/internal/rules/settings"
2020
"github.com/jeduden/mdsmith/internal/yamlutil"
2121
"github.com/yuin/goldmark/ast"
22+
"gopkg.in/yaml.v3"
2223
)
2324

2425
func init() {
@@ -1089,7 +1090,7 @@ func readDocFrontMatterRaw(f *lint.File) (map[string]any, []lint.Diagnostic) {
10891090
fmt.Sprintf("front matter: %v", err))}
10901091
}
10911092
var raw map[string]any
1092-
if err := yamlutil.UnmarshalSafe(yamlBytes, &raw); err != nil {
1093+
if err := yaml.Unmarshal(yamlBytes, &raw); err != nil {
10931094
return nil, []lint.Diagnostic{makeDiag(f.Path, 1,
10941095
fmt.Sprintf("front matter: invalid YAML: %v", err))}
10951096
}

internal/yamlutil/yamlutil.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
// - [Marshal] — thin wrapper around yaml.Marshal for consistency; safe for
1414
// output marshaling where data originates from trusted Go values.
1515
//
16-
// See docs/security/adversarial-markdown.md for threat model context.
16+
// See docs/security/2026-04-05-adversarial-markdown.md for threat model context.
1717
package yamlutil
1818

1919
import (

plan/121_yaml-handling-review.md

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ File: `internal/corpus/config.go`
8282

8383
- `UnmarshalSafe(data []byte, v any) error` — combines
8484
alias rejection + unmarshal
85-
- `UnmarshalNodeSafe(data []byte) (*yaml.Node, error)`
85+
- `UnmarshalNodeSafe(data []byte) (yaml.Node, error)`
8686
for node-based parsing
8787
- `Marshal(v any) ([]byte, error)` — thin wrapper for
8888
consistency
@@ -92,13 +92,16 @@ File: `internal/corpus/config.go`
9292
2. Update all 13+ `yaml.Unmarshal` call sites to use
9393
`yamlutil.UnmarshalSafe`:
9494

95-
- `internal/config/load.go` (3 sites: load, topLevelKeySet,
96-
validateConventionScalar)
95+
- `internal/config/load.go` (2 sites: load, topLevelKeySet;
96+
validateConventionScalar uses plain yaml.Unmarshal since
97+
node parsing does not expand aliases)
9798
- `internal/lint/frontmatter.go` (1 site)
9899
- `internal/archetype/gensection/parse.go` (1 site)
99100
- `internal/rules/catalog/rule.go` (1 site)
100-
- `internal/rules/requiredstructure/rule.go` (4 sites:
101-
require, include, schema, allow-empty-section)
101+
- `internal/rules/requiredstructure/rule.go` (3 sites via
102+
UnmarshalSafe: require, include, schema; front-matter
103+
read uses explicit RejectYAMLAliases + yaml.Unmarshal
104+
to preserve distinct error messages per error type)
102105
- `internal/corpus/config.go` (2 sites)
103106
- `cmd/mdsmith/main.go` (check if front-matter parsing
104107
needs update)
@@ -121,8 +124,10 @@ File: `internal/corpus/config.go`
121124

122125
6. Update tests:
123126

124-
- Move `yamlsafe_test.go` to `yamlutil_test.go`
125-
- Add tests for new wrapper functions
127+
- Add `internal/yamlutil/yamlutil_test.go` with tests for
128+
new wrapper functions and alias rejection
129+
- Repurpose `internal/lint/yamlsafe_test.go` to verify
130+
alias rejection through the front-matter parsing path
126131
- Ensure coverage of error paths
127132

128133
7. Run full test suite: `go test ./...`

0 commit comments

Comments
 (0)