Add fuzz tests for frontmatter YAML parser#3851
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Add check in ExtractFrontmatterFromContent to ensure frontmatter map is never nil - yaml.Unmarshal sets map to nil for empty YAML, now initialize empty map instead - Fuzz test now passes with 10s run (74k+ executions, 55 interesting cases found) - All unit tests pass Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Fixed the fuzz test implementation. The issue was that Results:
Commit: 97b6c7a |
|
@copilot make sure new fuzz test runs with "make fuzz" |
- Added fuzz target to Makefile that runs fuzz tests for 30s in parser and workflow packages - Updated help documentation to include fuzz command - Tests FuzzParseFrontmatter in pkg/parser and FuzzExpressionParser in pkg/workflow Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added
The target is documented in Commit: 56e39ae |
|
@copilot merge main |
…s-frontmatter-parser
|
@copilot integrate new fuzz test with existing fuzz test suite and make sure it runs in ci.yml fuzz job |
Integrated FuzzParseFrontmatter into CI workflow. Updated
Both fuzz tests are also available via Commit: cb10108 (note: experiencing GitHub server error on push, commit is ready locally) |
|
@copilot do it again and try to push |
- Updated .github/workflows/ci.yml to run both fuzz tests - FuzzParseFrontmatter now runs in CI alongside FuzzExpressionParser - Each fuzz test runs for 10 seconds in CI Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive fuzz testing for the frontmatter YAML parser to discover edge cases and security vulnerabilities in user input handling. The implementation follows the existing pattern established by FuzzExpressionParser in the workflow package and integrates fuzz tests into both the Makefile and CI workflow.
Key Changes:
- Created
FuzzParseFrontmatterwith 45 seed test cases covering valid YAML, invalid syntax, edge cases, Unicode, special characters, and malformed delimiters - Fixed a nil pointer bug in
frontmatter.gowhereyaml.Unmarshalreturns nil for empty YAML - Added
make fuzztarget to run all fuzz tests for 30 seconds each
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/parser/frontmatter_fuzz_test.go | New fuzz test with comprehensive seed corpus (589 lines) covering valid/invalid YAML, edge cases, and helper validation functions |
| pkg/parser/frontmatter.go | Bug fix: initialize empty map when yaml.Unmarshal returns nil for empty YAML frontmatter |
| Makefile | Add fuzz target to run fuzz tests for 30 seconds and update help documentation |
| .github/workflows/ci.yml | Integrate FuzzParseFrontmatter into CI fuzz job to run for 10 seconds alongside FuzzExpressionParser |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Deeply nested structure (overly deep - may hit limits) | ||
| deepNested := "---\nname: Test\ndata:\n" | ||
| for i := 0; i < 100; i++ { | ||
| deepNested += strings.Repeat(" ", i+1) + "level" + string(rune(i%10+'0')) + ":\n" |
There was a problem hiding this comment.
The conversion string(rune(i%10+'0')) is incorrect for generating numeric characters. When i%10 is between 0-9, adding '0' (which is 48) gives 48-57, but the issue is that the code tries to convert an integer to a rune to a string incorrectly.
This should use strconv.Itoa(i%10) or format the number properly:
deepNested += strings.Repeat(" ", i+1) + "level" + strconv.Itoa(i%10) + ":\n"The current code happens to work because it's generating runes in the ASCII digit range, but it's semantically incorrect and confusing.
| // Very long array with many items | ||
| longArray := "---\nname: Test\nitems:\n" | ||
| for i := 0; i < 1000; i++ { | ||
| longArray += " - item" + string(rune(i%10+'0')) + "\n" |
There was a problem hiding this comment.
Same issue as line 200: string(rune(i%10+'0')) is an incorrect conversion pattern. Should use strconv.Itoa(i%10) instead:
longArray += " - item" + strconv.Itoa(i%10) + "\n"This makes the intent clearer and is the proper way to convert integers to strings in Go.
| import ( | ||
| "strings" | ||
| "testing" | ||
| ) |
There was a problem hiding this comment.
Missing strconv import. The code at lines 200 and 441 should use strconv.Itoa() for integer-to-string conversion instead of the incorrect string(rune(i%10+'0')) pattern. Add:
import (
"strconv"
"strings"
"testing"
)| go test -fuzz=. -fuzztime=30s ./pkg/parser/... | ||
| go test -fuzz=. -fuzztime=30s ./pkg/workflow/... |
There was a problem hiding this comment.
The fuzz target uses -fuzz=. which attempts to match all fuzz tests in a package. However, Go's fuzzing engine can only execute one fuzz test at a time. When multiple fuzz tests match the pattern, it will error with "testing: will not fuzz, -fuzz matches more than one fuzz test".
The CI configuration correctly uses specific test names (-fuzz=FuzzParseFrontmatter and -fuzz=FuzzExpressionParser), but the Makefile target should follow the same pattern.
Recommended fix:
fuzz:
@echo "Running fuzz tests for 30 seconds..."
go test -fuzz=FuzzParseFrontmatter -fuzztime=30s ./pkg/parser/
go test -fuzz=FuzzExpressionParser -fuzztime=30s ./pkg/workflow/This ensures each fuzz test runs sequentially with its own 30-second time budget, matching the CI behavior.
| go test -fuzz=. -fuzztime=30s ./pkg/parser/... | |
| go test -fuzz=. -fuzztime=30s ./pkg/workflow/... | |
| go test -fuzz=FuzzParseFrontmatter -fuzztime=30s ./pkg/parser/ | |
| go test -fuzz=FuzzExpressionParser -fuzztime=30s ./pkg/workflow/ |
|
@copilot lint |
- Add strconv import for proper integer-to-string conversion - Replace string(rune(i%10+'0')) with strconv.Itoa(i%10) at lines 200 and 441 - Fix Makefile fuzz target to use specific test names instead of wildcard pattern - Ensures each fuzz test runs sequentially with proper 30s time budget Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Linting complete. All code review issues have been addressed:
Commit: c1bb1eb |
Plan: Add Fuzz Tests for Frontmatter YAML Parser
Objective
Implement native Go fuzz testing for the frontmatter YAML parser to discover edge cases and potential security vulnerabilities in user input handling.
Checklist
pkg/parser/frontmatter_fuzz_test.gowith comprehensive seed corpusgo test -fuzz=FuzzParseFrontmatter -fuzztime=10smake fuzztarget to run all fuzz testsImplementation Summary
Created
pkg/parser/frontmatter_fuzz_test.gowith 45 seed test cases covering:Fixed bug in
pkg/parser/frontmatter.go:yaml.Unmarshalsets map to nil for empty YAMLAdded
make fuzztarget:pkg/parser(FuzzParseFrontmatter) andpkg/workflow(FuzzExpressionParser)Integrated with CI:
.github/workflows/ci.ymlfuzz job to run both fuzz testsCode review fixes:
strconvimport for proper integer-to-string conversionstring(rune(i%10+'0'))withstrconv.Itoa(i%10)for clarity and correctnessMerged main branch:
Test Results:
make fuzzexecutes both parser and workflow fuzz testsFiles Modified
pkg/parser/frontmatter_fuzz_test.go(comprehensive fuzz test)pkg/parser/frontmatter.go(fix nil map initialization)Makefile(add fuzz target with specific test names).github/workflows/ci.yml(integrate new fuzz test into CI)Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.