Skip to content

Centralize YAML handling with safe unmarshal wrappers - #225

Merged
jeduden merged 2 commits into
mainfrom
claude/review-yaml-handling-bDeW8
May 3, 2026
Merged

Centralize YAML handling with safe unmarshal wrappers#225
jeduden merged 2 commits into
mainfrom
claude/review-yaml-handling-bDeW8

Conversation

@jeduden

@jeduden jeduden commented May 3, 2026

Copy link
Copy Markdown
Owner

Summary

This PR extracts YAML security and marshaling logic into a new internal/yamlutil package, centralizing all user-content YAML parsing under safe unmarshal wrappers that prevent billion-laughs denial-of-service attacks.

Key Changes

  • New internal/yamlutil package with three main functions:

    • UnmarshalSafe() — unmarshal user content into Go structs/maps with anchor/alias rejection
    • UnmarshalNodeSafe() — unmarshal into raw yaml.Node trees with the same security checks
    • Marshal() — thin wrapper around yaml.Marshal for consistency
    • RejectYAMLAliases() — core security check that decodes YAML into nodes without expanding aliases, safely detecting billion-laughs payloads
  • Migrated RejectYAMLAliases from internal/lint to internal/yamlutil and removed the old implementation

  • Updated all user-content unmarshal sites to use yamlutil.UnmarshalSafe instead of direct yaml.Unmarshal:

    • internal/config/load.go — config file parsing
    • internal/corpus/config.go — corpus config parsing
    • internal/lint/frontmatter.go — front matter parsing
    • internal/rules/requiredstructure/rule.go — directive and schema parsing
    • internal/rules/catalog/rule.go — catalog front matter parsing
    • internal/archetype/gensection/parse.go — generated section YAML parsing
    • cmd/mdsmith/main.go — front matter reading
  • Updated marshal sites to use yamlutil.Marshal for consistency:

    • internal/kindsout/kindsout.go
  • Comprehensive test coverage in internal/yamlutil/yamlutil_test.go with 20+ test cases covering:

    • Clean YAML acceptance
    • Anchor/alias rejection (including billion-laughs chains)
    • Edge cases (quoted strings, comments, block scalars, multi-document YAML)
    • Error handling and empty input
  • Updated test in internal/lint/yamlsafe_test.go to verify anchor/alias rejection through the front matter parsing path

  • Marked plan item 121 complete in plan/121_yaml-handling-review.md

Implementation Details

The security model relies on decoding YAML into yaml.Node without expanding aliases (which prevents the exponential expansion attack), then checking for anchor/alias nodes in the tree. This approach safely detects malicious YAML before the standard yaml.Unmarshal call that would expand aliases. Non-anchor syntax errors are allowed through to be handled by the caller's unmarshal logic.

https://claude.ai/code/session_01WnJMsRhRBvtMHQX1seKaQW

Creates internal/yamlutil with UnmarshalSafe, UnmarshalNodeSafe,
Marshal, and RejectYAMLAliases wrappers. Migrates all 13+ yaml.Unmarshal
call sites to use the safe wrappers, eliminating duplicated
RejectYAMLAliases + yaml.Unmarshal pairs across 7 packages. Removes
internal/lint/yamlsafe.go as RejectYAMLAliases now lives in yamlutil.

https://claude.ai/code/session_01WnJMsRhRBvtMHQX1seKaQW
Copilot AI review requested due to automatic review settings May 3, 2026 12:07
@codecov

codecov Bot commented May 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.58%. Comparing base (2b749c4) to head (7535ba1).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #225      +/-   ##
==========================================
+ Coverage   94.51%   94.58%   +0.07%     
==========================================
  Files         136      136              
  Lines       15850    15839      -11     
==========================================
+ Hits        14981    14982       +1     
+ Misses        530      524       -6     
+ Partials      339      333       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR centralizes YAML parsing/marshaling behind a new internal/yamlutil package so mdsmith can apply one consistent alias-rejection policy anywhere it reads user-controlled YAML. It fits into the codebase as a security-hardening and cleanup pass across config loading, front matter handling, directive parsing, and YAML-emitting command output.

Changes:

  • Added internal/yamlutil with shared safe-unmarshal, node-unmarshal, marshal, and alias-rejection helpers plus dedicated tests.
  • Replaced scattered direct YAML unmarshalling at user-input call sites with yamlutil wrappers across config, front matter, directives, corpus config, and CLI code.
  • Removed the old internal/lint alias-check helper and marked the YAML-handling plan item complete.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
plan/121_yaml-handling-review.md Marks the YAML-centralization plan complete and updates acceptance criteria.
internal/yamlutil/yamlutil_test.go Adds focused tests for alias rejection and wrapper behavior.
internal/yamlutil/yamlutil.go Introduces the new shared YAML safety/marshal helpers.
internal/rules/requiredstructure/rule.go Switches schema/directive/front-matter parsing to the new helpers.
internal/rules/catalog/rule.go Uses the centralized safe unmarshal for catalog front matter.
internal/lint/yamlsafe_test.go Retargets alias-rejection coverage through front-matter parsing.
internal/lint/yamlsafe.go Removes the old alias-rejection implementation after extraction.
internal/lint/frontmatter.go Routes front-matter kinds: parsing through yamlutil.
internal/kindsout/kindsout.go Uses the shared marshal wrapper for YAML output.
internal/corpus/config.go Uses safe unmarshal for corpus config and local overrides.
internal/config/load.go Replaces config parsing/key inspection with centralized YAML helpers.
internal/config/convention.go Uses node-safe parsing for raw convention: validation.
internal/archetype/gensection/parse.go Uses safe unmarshal for generated-section YAML bodies.
cmd/mdsmith/main.go Uses centralized YAML helpers for front matter reading and init output.
PLAN.md Updates the plan catalog entry for plan 121 to completed.

Comment thread internal/yamlutil/yamlutil.go Outdated
Comment thread internal/config/convention.go Outdated
Comment thread internal/rules/requiredstructure/rule.go
Comment thread plan/121_yaml-handling-review.md
- 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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Comment thread internal/yamlutil/yamlutil.go
Comment thread plan/121_yaml-handling-review.md
@jeduden

jeduden commented May 3, 2026

Copy link
Copy Markdown
Owner Author

🔍 Merge Queue — bisecting

A larger batch failed CI. Bisection is isolating the culprit: this run tests up to 2 of 3 candidate PRs on merge-queue/batch-bisect-223-1777815378. View current bisect CI run.

Next: No action needed — you'll be notified when the culprit is isolated or this PR merges.

@jeduden jeduden added queue Add to a PR to enqueue it queue:active Applied automatically when a PR is in an active batch and removed queue Add to a PR to enqueue it labels May 3, 2026
@jeduden

jeduden commented May 3, 2026

Copy link
Copy Markdown
Owner Author

🟢 Merge Queue — picked up

This PR is in the queue and will be batched with other queue-labelled PRs.

Next: No action needed — you'll get another comment when CI starts on the batch. View merge queue run.

@jeduden

jeduden commented May 3, 2026

Copy link
Copy Markdown
Owner Author

🔵 Merge Queue — CI running

Merged into batch branch merge-queue/batch-225-1777815487. View CI run.

Next: No action needed — you'll be notified when CI completes.

@jeduden jeduden removed the queue:active Applied automatically when a PR is in an active batch label May 3, 2026
@jeduden
jeduden merged commit 81cc3f5 into main May 3, 2026
16 checks passed
@jeduden

jeduden commented May 3, 2026

Copy link
Copy Markdown
Owner Author

Merge Queue — merged

This PR landed on main via commit 81cc3f5. CI run that validated the merge.

Next: Done — nothing more to do here.

jeduden pushed a commit that referenced this pull request May 3, 2026
…fix README

After PR #225 merged into main, RejectYAMLAliases moved from internal/lint
to internal/yamlutil and a combined UnmarshalSafe helper was introduced.
Update singleh1/rule.go to import yamlutil and use UnmarshalSafe (which
already rejects YAML aliases internally) instead of the now-removed
lint.RejectYAMLAliases + yaml.Unmarshal pair.

Also add the missing blank import of singleh1 to cmd/mdsmith/main.go so
the rule is registered in the binary.

Update the README Diagnostics table condition for the front-matter conflict
message to accurately describe when it fires: the front matter must contain
the configured field with a non-empty string value (YAML parse errors and
alias rejection both result in no conflict).

https://claude.ai/code/session_019k6vHWPxLjqFjVmtUX6V51
jeduden pushed a commit that referenced this pull request May 3, 2026
…fix README

After PR #225 merged into main, RejectYAMLAliases moved from internal/lint
to internal/yamlutil and a combined UnmarshalSafe helper was introduced.
Update singleh1/rule.go to import yamlutil and use UnmarshalSafe (which
already rejects YAML aliases internally) instead of the now-removed
lint.RejectYAMLAliases + yaml.Unmarshal pair.

Also add the missing blank import of singleh1 to cmd/mdsmith/main.go so
the rule is registered in the binary.

Update the README Diagnostics table condition for the front-matter conflict
message to accurately describe when it fires: the front matter must contain
the configured field with a non-empty string value (YAML parse errors and
alias rejection both result in no conflict).

https://claude.ai/code/session_019k6vHWPxLjqFjVmtUX6V51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants