Skip to content

Commit 43dfa16

Browse files
author
merge-queue-bot
committed
Merge PR #203: [WIP] Review yaml handling and centralize interactions
2 parents 637205e + 6f5ea8e commit 43dfa16

2 files changed

Lines changed: 147 additions & 0 deletions

File tree

‎PLAN.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ footer: |
3333
| 114 | ✅ | sonnet | [MDS034 message clarity and flavor-vs-rule docs](plan/114_mds034-message-and-flavor-vs-rule-docs.md) |
3434
| 120 | 🔲 | sonnet | [Unify glob matcher and field naming across mdsmith](plan/120_glob-unification.md) |
3535
| 121 | 🔲 | opus | [Expose mdsmith to VS Code via Language Server Protocol](plan/121_vscode-integration.md) |
36+
| 121 | 🔲 | sonnet | [Review and centralize YAML handling](plan/121_yaml-handling-review.md) |
3637
| 122 | 🔲 | sonnet | [VS Code hover help and palette commands](plan/122_vscode-hover-and-palette.md) |
3738
| 124 | 🔲 | sonnet | [No space inside code spans rule](plan/124_no-space-in-code-spans.md) |
3839
| 125 | 🔲 | sonnet | [No space inside link text rule](plan/125_no-space-in-link-text.md) |

‎plan/121_yaml-handling-review.md‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
---
2+
id: 121
3+
title: Review and centralize YAML handling
4+
status: 🔲
5+
summary: Audit all YAML parsing/marshaling, unify under a central package, ensure consistent security checks
6+
model: sonnet
7+
---
8+
# Review and centralize YAML handling
9+
10+
<!-- Plan conventions:
11+
- Work test-driven: write a failing test, make it
12+
pass, commit.
13+
- Plan files must pass `mdsmith check plan/`.
14+
- Use Markdown links for real repo paths in prose.
15+
Bare backticked paths are allowed in commands,
16+
code blocks, and placeholders.
17+
18+
Status values:
19+
- 🔲 not started
20+
- 🔳 in progress
21+
- ✅ completed
22+
- ⛔ superseded (replaced by another plan)
23+
24+
-->
25+
26+
## Goal
27+
28+
Centralize YAML operations under one package. Reduce duplication.
29+
Ensure consistent security checks prevent DoS attacks.
30+
31+
## Context
32+
33+
### Current State
34+
35+
YAML handling is scattered across 9 packages with 13+ unmarshal
36+
sites. Each site must manually call `RejectYAMLAliases()` before
37+
`yaml.Unmarshal()` to prevent billion-laughs attacks. This
38+
pattern is duplicated at every call site.
39+
40+
### Security Layer
41+
42+
File: `internal/lint/yamlsafe.go`
43+
44+
- Defines `RejectYAMLAliases()` for DoS protection
45+
46+
### Config Loading
47+
48+
Files: `internal/config/{load,convention,config}.go`
49+
50+
- Load config files, validate conventions
51+
- Define `RuleCfg` custom marshal/unmarshal
52+
53+
### Front-matter
54+
55+
File: `internal/lint/frontmatter.go`
56+
57+
- Parse front-matter `kinds:` field
58+
59+
### Directive Parsing
60+
61+
Files in `internal/archetype/gensection/`, `internal/rules/`:
62+
63+
- Parse YAML parameters for multiple directive types
64+
- Files: `parse.go`, `catalog/rule.go`,
65+
`requiredstructure/rule.go`
66+
67+
### Output
68+
69+
Files: `internal/kindsout/kindsout.go`, `cmd/mdsmith/main.go`
70+
71+
- Marshal kind bodies and config
72+
73+
### Other
74+
75+
File: `internal/corpus/config.go`
76+
77+
- Load corpus config
78+
79+
## Tasks
80+
81+
1. Create `internal/yamlutil` package with:
82+
83+
- `UnmarshalSafe(data []byte, v any) error` — combines
84+
alias rejection + unmarshal
85+
- `UnmarshalNodeSafe(data []byte) (*yaml.Node, error)` —
86+
for node-based parsing
87+
- `Marshal(v any) ([]byte, error)` — thin wrapper for
88+
consistency
89+
- Move `RejectYAMLAliases` from `internal/lint` to
90+
`internal/yamlutil`
91+
92+
2. Update all 13+ `yaml.Unmarshal` call sites to use
93+
`yamlutil.UnmarshalSafe`:
94+
95+
- `internal/config/load.go` (3 sites: load, topLevelKeySet,
96+
validateConventionScalar)
97+
- `internal/lint/frontmatter.go` (1 site)
98+
- `internal/archetype/gensection/parse.go` (1 site)
99+
- `internal/rules/catalog/rule.go` (1 site)
100+
- `internal/rules/requiredstructure/rule.go` (4 sites:
101+
require, include, schema, allow-empty-section)
102+
- `internal/corpus/config.go` (2 sites)
103+
- `cmd/mdsmith/main.go` (check if front-matter parsing
104+
needs update)
105+
106+
3. Update all `yaml.Marshal` call sites to use
107+
`yamlutil.Marshal`:
108+
109+
- `internal/kindsout/kindsout.go`
110+
- `cmd/mdsmith/main.go`
111+
- `internal/config/config.go` (RuleCfg.MarshalYAML —
112+
keep custom logic, but consider if any standardization
113+
helps)
114+
115+
4. Update import statements across all affected files
116+
5. Add godoc to `internal/yamlutil` package explaining:
117+
118+
- Why alias rejection is mandatory for user content
119+
- When to use `UnmarshalSafe` vs direct `yaml` package
120+
- Link to adversarial-markdown security doc
121+
122+
6. Update tests:
123+
124+
- Move `yamlsafe_test.go` to `yamlutil_test.go`
125+
- Add tests for new wrapper functions
126+
- Ensure coverage of error paths
127+
128+
7. Run full test suite: `go test ./...`
129+
8. Run linter: `go tool golangci-lint run`
130+
131+
## Acceptance Criteria
132+
133+
- [ ] New `internal/yamlutil` package exists with documented
134+
safe-unmarshal wrappers
135+
- [ ] `RejectYAMLAliases` moved from `internal/lint` to
136+
`internal/yamlutil`
137+
- [ ] All user-content unmarshal sites use
138+
`yamlutil.UnmarshalSafe` (no direct `yaml.Unmarshal`
139+
on user data)
140+
- [ ] All marshal sites use `yamlutil.Marshal` or keep
141+
well-documented custom logic in place
142+
- [ ] `internal/yamlutil` has comprehensive godoc
143+
- [ ] All tests pass: `go test ./...`
144+
- [ ] `go tool golangci-lint run` reports no issues
145+
- [ ] No direct calls to `lint.RejectYAMLAliases` outside
146+
`yamlutil` package

0 commit comments

Comments
 (0)