Skip to content

Commit cc102ab

Browse files
fix(scan,ui): bound memory growth (size cap + LRU caches) (#76)
* fix(scan,ui): bound memory growth (size cap + LRU caches) Two unbounded resources could OOM the process: 1. `parseSkill` called `os.ReadFile` with no size check and stored the raw bytes on `SkillRecord`. A multi-GB SKILL.md — accidental or hostile — would crash the scanner before it could surface a useful error. 2. The preview render cache and the package-level raw-highlight cache were plain maps that grew without bound. A long session across many skills and terminal-resize events would slowly leak rendered output. Both are resource-exhaustion bugs: any SKILL.md visible to the scanner becomes attacker-controlled input from the process's point of view. Add `MaxSkillBytes` (1 MiB) in scanner.go and reject oversize files with a ParseErr — surfaces as `!` in the matrix, no Raw retained. Replace both preview caches with `github.com/hashicorp/golang-lru/v2` bounded at 256 entries. Public API (`Get`, `Set`, `Key`) is unchanged so existing tests still pass by value. Closes #64 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(scan): fuzz YAML frontmatter parser (stacked on #76) (#80) * test(scan): fuzz YAML frontmatter parser; surface type-assertion errors SKILL.md content is third-party — yaml.v3 has no built-in bounds on nesting depth or alias expansion, and our parser previously dropped type-assertion failures on `name` / `description` silently. Both are cheap insurance worth carrying before v1.0. - New `FuzzParseFrontmatter` in `internal/scan/fuzz_test.go` seeded with valid / BOM / CRLF / unclosed / malformed / wrong-type / deeply-nested / self-aliased corpus entries. Each input writes a temp SKILL.md and runs through `parseSkill` under a 2s wall-clock guard; panics, missing records, and budget overruns all fail. - Seed corpus is also exercised by a plain `TestParseSkillSeeds` so these inputs run on every `go test`, not just under `-fuzz`. - Replace silent `rec.Name, _ = v.(string)` (and Description) with explicit handling: non-nil non-string values populate `ParseErr`; nil / missing fields still fall back to dirname (documented). - CI: 30s fuzz job on every PR. Longer runs can be scheduled later. Defers oversize-input handling to PR for #64 (size cap in `parseSkill`); fuzz target skips inputs > 64 KiB so the two efforts don't fight over the same territory. Closes #68 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fuzz: use MaxSkillBytes as the fuzz threshold Now that parseSkill rejects oversize inputs outright via #64, the fuzzer no longer needs its own 64 KiB workaround threshold — anything over MaxSkillBytes would just hit the early ParseErr path. Reuse the same constant so they can't drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 6bc4057 commit cc102ab

8 files changed

Lines changed: 446 additions & 21 deletions

File tree

.github/workflows/ci.yml

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,21 @@ jobs:
9595
version: "2025.1.1"
9696
install-go: false
9797

98+
fuzz:
99+
name: fuzz (parseFrontmatter, 30s)
100+
runs-on: ubuntu-latest
101+
steps:
102+
- uses: actions/checkout@v6
103+
- uses: actions/setup-go@v6
104+
with:
105+
go-version: "1.24"
106+
cache: true
107+
- name: Short fuzz run on YAML frontmatter parser
108+
# Third-party SKILL.md content is untrusted; 30 s per PR is cheap
109+
# insurance against yaml.v3 parser regressions. Longer runs can
110+
# be scheduled nightly later.
111+
run: go test -run=^$ -fuzz=FuzzParseFrontmatter -fuzztime=30s ./internal/scan/...
112+
98113
build-cross:
99114
name: build (cross-compile)
100115
runs-on: ubuntu-latest

go.mod

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ require (
2929
github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f // indirect
3030
github.com/fsnotify/fsnotify v1.10.1 // indirect
3131
github.com/gorilla/css v1.0.1 // indirect
32+
github.com/hashicorp/golang-lru/v2 v2.0.7 // indirect
3233
github.com/lucasb-eyer/go-colorful v1.3.0 // indirect
3334
github.com/mattn/go-isatty v0.0.20 // indirect
3435
github.com/mattn/go-localereader v0.0.1 // indirect

go.sum

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ github.com/fsnotify/fsnotify v1.10.1 h1:b0/UzAf9yR5rhf3RPm9gf3ehBPpf0oZKIjtpKrx5
4646
github.com/fsnotify/fsnotify v1.10.1/go.mod h1:TLheqan6HD6GBK6PrDWyDPBaEV8LspOxvPSjC+bVfgo=
4747
github.com/gorilla/css v1.0.1 h1:ntNaBIghp6JmvWnxbZKANoLyuXTPZ4cAMlo6RyhlbO8=
4848
github.com/gorilla/css v1.0.1/go.mod h1:BvnYkspnSzMmwRK+b8/xgNPLiIuNZr6vbZBTPQ2A3b0=
49+
github.com/hashicorp/golang-lru/v2 v2.0.7 h1:a+bsQ5rvGLjzHuww6tVxozPZFVghXaHOwFs4luLUK2k=
50+
github.com/hashicorp/golang-lru/v2 v2.0.7/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM=
4951
github.com/hexops/gotextdiff v1.0.3 h1:gitA9+qJrrTCsiCl7+kh75nPqQt1cx4ZkudSTLoUqJM=
5052
github.com/hexops/gotextdiff v1.0.3/go.mod h1:pSWU5MAI3yDq+fZBTazCSJysOMbxWL1BSow5/V2vxeg=
5153
github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc=

internal/scan/fuzz_test.go

Lines changed: 200 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,200 @@
1+
package scan
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"strings"
7+
"testing"
8+
"time"
9+
10+
"github.com/heidihowilson/skillscope/internal/harness"
11+
)
12+
13+
// fuzzWallClockBudget is the per-input wall-clock limit. yaml.v3 has no
14+
// built-in nesting / alias bound, so pathological inputs (e.g. billion-laughs
15+
// style alias expansion) could in principle spin. We assert termination
16+
// inside the budget so a regression here is loud, not silent.
17+
const fuzzWallClockBudget = 2 * time.Second
18+
19+
// seedCorpus is a small set of representative SKILL.md frontmatter shapes
20+
// (good and bad). Reused as table-driven entries by TestParseSkillSeeds so
21+
// the fuzz seeds also run under a plain `go test` (no fuzzing args).
22+
var seedCorpus = []struct {
23+
name string
24+
in string
25+
}{
26+
{"valid-simple", "---\nname: foo\ndescription: bar\n---\nbody\n"},
27+
{"valid-crlf", "---\r\nname: foo\r\ndescription: bar\r\n---\r\nbody\r\n"},
28+
{"valid-bom", "\ufeff---\nname: foo\ndescription: bar\n---\nbody\n"},
29+
{"empty", ""},
30+
{"no-frontmatter", "just a markdown body, no fences\n"},
31+
{"unclosed", "---\nname: foo\nbody but no closer\n"},
32+
{"malformed-yaml", "---\nname: [unclosed\n---\nbody\n"},
33+
{"name-not-string", "---\nname: 42\ndescription: ok\n---\nbody\n"},
34+
{"name-is-list", "---\nname: [a, b, c]\ndescription: ok\n---\nbody\n"},
35+
{"name-is-map", "---\nname: {a: 1}\ndescription: ok\n---\nbody\n"},
36+
{"description-not-string", "---\nname: ok\ndescription: 42\n---\nbody\n"},
37+
{"name-null", "---\nname: null\ndescription: ok\n---\nbody\n"},
38+
{"deeply-nested", "---\n" + strings.Repeat("a:\n ", 200) + "x\n---\nbody\n"},
39+
{"alias-self", "---\na: &a\n b: *a\n---\nbody\n"},
40+
{"only-delims", "---\n---\n"},
41+
{"binary-junk", "---\n\x00\x01\x02\x03name: foo\n---\nbody\n"},
42+
}
43+
44+
// TestParseSkillSeeds exercises the fuzz seed corpus under a normal
45+
// `go test` run so the same inputs that guard the parser are also
46+
// regression-tested without fuzzing args.
47+
func TestParseSkillSeeds(t *testing.T) {
48+
for _, tc := range seedCorpus {
49+
tc := tc
50+
t.Run(tc.name, func(t *testing.T) {
51+
runParseSkill(t, []byte(tc.in))
52+
})
53+
}
54+
}
55+
56+
// FuzzParseFrontmatter feeds arbitrary bytes through parseSkill, asserting
57+
// no panic, that a SkillRecord is always returned, and that parsing
58+
// terminates inside fuzzWallClockBudget. SKILL.md content is third-party;
59+
// fuzzing the parser is cheap insurance against yaml.v3 surprises.
60+
func FuzzParseFrontmatter(f *testing.F) {
61+
for _, tc := range seedCorpus {
62+
f.Add([]byte(tc.in))
63+
}
64+
f.Fuzz(func(t *testing.T, data []byte) {
65+
// parseSkill rejects anything over MaxSkillBytes outright; skip
66+
// here so the fuzzer doesn't waste its budget rediscovering that.
67+
if len(data) > MaxSkillBytes {
68+
t.Skip("input over MaxSkillBytes")
69+
}
70+
runParseSkill(t, data)
71+
})
72+
}
73+
74+
// runParseSkill writes data to a temp SKILL.md and invokes parseSkill under
75+
// a wall-clock guard. Any panic, nil-record return, or budget overrun is a
76+
// test failure.
77+
func runParseSkill(t *testing.T, data []byte) {
78+
t.Helper()
79+
80+
dir := t.TempDir()
81+
skillDir := filepath.Join(dir, "fuzz-skill")
82+
if err := os.Mkdir(skillDir, 0o755); err != nil {
83+
t.Fatalf("mkdir: %v", err)
84+
}
85+
path := filepath.Join(skillDir, "SKILL.md")
86+
if err := os.WriteFile(path, data, 0o644); err != nil {
87+
t.Fatalf("write: %v", err)
88+
}
89+
90+
scope := harness.Scope{
91+
Harness: "fuzz",
92+
Kind: harness.User,
93+
Path: dir,
94+
ReadOnly: false,
95+
}
96+
97+
type result struct {
98+
rec SkillRecord
99+
}
100+
done := make(chan result, 1)
101+
panicked := make(chan any, 1)
102+
103+
go func() {
104+
defer func() {
105+
if r := recover(); r != nil {
106+
panicked <- r
107+
}
108+
}()
109+
rec := parseSkill(path, scope, time.Now())
110+
done <- result{rec: rec}
111+
}()
112+
113+
select {
114+
case r := <-done:
115+
// Path/Scope are always set; Name always has a fallback. Any
116+
// malformed-YAML / type-mismatch case should be reflected in
117+
// ParseErr — but we don't assert that here because some
118+
// well-formed-but-weird YAML legitimately parses clean.
119+
if r.rec.Path != path {
120+
t.Errorf("rec.Path = %q, want %q", r.rec.Path, path)
121+
}
122+
if r.rec.Name == "" {
123+
t.Error("rec.Name is empty; dirname fallback should have fired")
124+
}
125+
case p := <-panicked:
126+
t.Fatalf("parseSkill panicked: %v", p)
127+
case <-time.After(fuzzWallClockBudget):
128+
t.Fatalf("parseSkill exceeded %v wall-clock budget", fuzzWallClockBudget)
129+
}
130+
}
131+
132+
// TestParseSkill_NameWrongType locks down the explicit error path added
133+
// alongside the fuzz target: non-string `name` should populate ParseErr,
134+
// not silently drop to the dirname fallback.
135+
func TestParseSkill_NameWrongType(t *testing.T) {
136+
dir := t.TempDir()
137+
skillDir := filepath.Join(dir, "wrongtype")
138+
if err := os.Mkdir(skillDir, 0o755); err != nil {
139+
t.Fatal(err)
140+
}
141+
path := filepath.Join(skillDir, "SKILL.md")
142+
if err := os.WriteFile(path, []byte("---\nname: 42\n---\nbody\n"), 0o644); err != nil {
143+
t.Fatal(err)
144+
}
145+
rec := parseSkill(path, harness.Scope{Path: dir}, time.Now())
146+
if rec.ParseErr == nil {
147+
t.Fatal("expected ParseErr for non-string name, got nil")
148+
}
149+
if !strings.Contains(rec.ParseErr.Error(), "name must be a string") {
150+
t.Errorf("ParseErr = %v, want message about name type", rec.ParseErr)
151+
}
152+
if rec.Name != "wrongtype" {
153+
t.Errorf("rec.Name = %q, want dirname fallback %q", rec.Name, "wrongtype")
154+
}
155+
}
156+
157+
// TestParseSkill_DescriptionWrongType is the description-field analogue.
158+
func TestParseSkill_DescriptionWrongType(t *testing.T) {
159+
dir := t.TempDir()
160+
skillDir := filepath.Join(dir, "descwrong")
161+
if err := os.Mkdir(skillDir, 0o755); err != nil {
162+
t.Fatal(err)
163+
}
164+
path := filepath.Join(skillDir, "SKILL.md")
165+
if err := os.WriteFile(path, []byte("---\nname: ok\ndescription: [a, b]\n---\nbody\n"), 0o644); err != nil {
166+
t.Fatal(err)
167+
}
168+
rec := parseSkill(path, harness.Scope{Path: dir}, time.Now())
169+
if rec.ParseErr == nil {
170+
t.Fatal("expected ParseErr for non-string description, got nil")
171+
}
172+
if !strings.Contains(rec.ParseErr.Error(), "description must be a string") {
173+
t.Errorf("ParseErr = %v, want message about description type", rec.ParseErr)
174+
}
175+
// Name still parses cleanly when description is the bad field.
176+
if rec.Name != "ok" {
177+
t.Errorf("rec.Name = %q, want %q", rec.Name, "ok")
178+
}
179+
}
180+
181+
// TestParseSkill_NameNullFallsBackToDirname documents that an explicit
182+
// `name: null` is treated as missing (dirname fallback), not as a type error.
183+
func TestParseSkill_NameNullFallsBackToDirname(t *testing.T) {
184+
dir := t.TempDir()
185+
skillDir := filepath.Join(dir, "nullname")
186+
if err := os.Mkdir(skillDir, 0o755); err != nil {
187+
t.Fatal(err)
188+
}
189+
path := filepath.Join(skillDir, "SKILL.md")
190+
if err := os.WriteFile(path, []byte("---\nname: null\ndescription: ok\n---\nbody\n"), 0o644); err != nil {
191+
t.Fatal(err)
192+
}
193+
rec := parseSkill(path, harness.Scope{Path: dir}, time.Now())
194+
if rec.ParseErr != nil {
195+
t.Errorf("unexpected ParseErr for null name: %v", rec.ParseErr)
196+
}
197+
if rec.Name != "nullname" {
198+
t.Errorf("rec.Name = %q, want dirname fallback %q", rec.Name, "nullname")
199+
}
200+
}

internal/scan/scanner.go

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,12 @@ import (
1414
"gopkg.in/yaml.v3"
1515
)
1616

17+
// MaxSkillBytes caps the size of a SKILL.md file we'll fully read into
18+
// memory. Real skills are a few KB at most; anything past 1 MiB is either
19+
// a runaway log, an accidentally-committed binary, or a hostile fixture
20+
// trying to OOM the scanner. We record a ParseErr instead of loading it.
21+
const MaxSkillBytes = 1 << 20 // 1 MiB
22+
1723
// SkillRecord is the parsed representation of a single SKILL.md file.
1824
type SkillRecord struct {
1925
Name string
@@ -93,6 +99,15 @@ func parseSkill(path string, scope harness.Scope, mtime time.Time) SkillRecord {
9399
return rec
94100
}
95101

102+
// Reject oversized files before they hit the renderer / cache. We
103+
// blank out Raw so a multi-GB read isn't pinned in memory by the
104+
// returned record.
105+
if len(data) > MaxSkillBytes {
106+
rec.ParseErr = fmt.Errorf("SKILL.md exceeds %d bytes (got %d)", MaxSkillBytes, len(data))
107+
rec.Raw = ""
108+
return rec
109+
}
110+
96111
// Compute content hash.
97112
rec.Hash = sha256.Sum256(data)
98113

@@ -105,15 +120,36 @@ func parseSkill(path string, scope harness.Scope, mtime time.Time) SkillRecord {
105120

106121
if fm != nil {
107122
rec.Frontmatter = fm
108-
if v, ok := fm["name"]; ok {
109-
rec.Name, _ = v.(string)
123+
// Type-asserting frontmatter values: nil means the field is absent
124+
// (fall back to dirname below), but a non-nil non-string value is a
125+
// real bug in the SKILL.md — surface it via ParseErr so the UI can
126+
// flag the row instead of silently dropping the data.
127+
if v, ok := fm["name"]; ok && v != nil {
128+
s, isStr := v.(string)
129+
if !isStr {
130+
if rec.ParseErr == nil {
131+
rec.ParseErr = fmt.Errorf("frontmatter name must be a string, got %T", v)
132+
}
133+
} else {
134+
rec.Name = s
135+
}
110136
}
111-
if v, ok := fm["description"]; ok {
112-
rec.Description, _ = v.(string)
137+
if v, ok := fm["description"]; ok && v != nil {
138+
s, isStr := v.(string)
139+
if !isStr {
140+
if rec.ParseErr == nil {
141+
rec.ParseErr = fmt.Errorf("frontmatter description must be a string, got %T", v)
142+
}
143+
} else {
144+
rec.Description = s
145+
}
113146
}
114147
}
115148

116-
// Fall back to directory name if name field missing.
149+
// Fall back to directory name if the `name` field is missing or empty.
150+
// An explicit-but-wrong-type `name` is handled above (ParseErr set); we
151+
// still want to display *something* in that case, so dirname is a sane
152+
// last resort regardless of how we got here.
117153
if rec.Name == "" {
118154
rec.Name = filepath.Base(filepath.Dir(path))
119155
}

0 commit comments

Comments
 (0)