From 13507e0716a6a81eaf5400d040a860c3ad376a60 Mon Sep 17 00:00:00 2001 From: Claude Code Date: Thu, 30 Jul 2026 19:47:50 +0000 Subject: [PATCH 1/3] fix(validate): count characters, accept every major version, and keep citations inside the workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects found in review, each with a regression test that fails without the corresponding fix. stack: majorVersionRe was `^v[2-9]\d*$`, which rejects v10 through v19 — the leading digit class starts at 2, so only the first digit was ever allowed to be 1-free. The doc comment directly above it claimed v11 as an example, so the code contradicted its own documentation. A module ending /v11 kept the suffix as its base name and was reported undocumented against a stack.md that documented it. skill: the spec's limits are quoted in characters and were compared against len(), which is bytes. Every non-ASCII name, description, or compatibility field was held to a shorter budget than the standard grants — a false positive on a valid skill, which the existing ASCII test could not see. Counted in runes now, via one helper so the three sites cannot drift apart. codewiki: a citation target went into filepath.Join unchecked, so ../.. was cleaned into a real path outside root and then opened. The finding a traversal produced was nothing at all — the outside file resolved, its lines counted in range, and the validator stayed silent — so this was invisible rather than noisy. Same containment rule the workspace already applies to argv, at the one place a path arrives from a document instead. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013XH8WYThtTSGJxS7tXRvfp --- internal/validate/codewiki.go | 28 ++++++++++- internal/validate/knowledge_test.go | 72 +++++++++++++++++++++++++++++ internal/validate/skill.go | 19 +++++--- internal/validate/skill_test.go | 32 +++++++++++++ internal/validate/stack.go | 6 ++- 5 files changed, 149 insertions(+), 8 deletions(-) diff --git a/internal/validate/codewiki.go b/internal/validate/codewiki.go index b105bc6..0bdc880 100644 --- a/internal/validate/codewiki.go +++ b/internal/validate/codewiki.go @@ -106,7 +106,17 @@ func checkCitation(set *finding.Set, root, file string, line int, m []string) { return } - resolved := filepath.Join(root, filepath.FromSlash(target)) + // A citation names a path inside the checkout, and nothing else. Without this the + // target goes straight into filepath.Join, where a leading ../.. is cleaned into a + // real path outside root and then opened — a Markdown file in the workspace would + // be choosing what scc reads off the machine. Same rule as workspace.SafeName, at + // the one place a path arrives from a document rather than from argv. + resolved, ok := resolveInRoot(root, target) + if !ok { + set.Addf(file, line, "codewiki.citation-invalid", + "%s escapes the workspace; a citation names a path inside the checkout", target) + return + } if !isFile(resolved) { set.Addf(file, line, "codewiki.citation-unresolved", "%s does not exist", target) return @@ -124,6 +134,22 @@ func checkCitation(set *finding.Set, root, file string, line int, m []string) { } } +// resolveInRoot joins target onto root and reports whether the result stayed inside it. +// An absolute target is rejected outright; a relative one is rejected once Clean has +// walked it above root. +func resolveInRoot(root, target string) (string, bool) { + t := filepath.FromSlash(target) + if filepath.IsAbs(t) || strings.HasPrefix(target, "/") { + return "", false + } + resolved := filepath.Join(root, t) + rel, err := filepath.Rel(root, resolved) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", false + } + return resolved, true +} + // checkCodewikiHeadings holds slugs to being unique and derived from their headings. A // duplicated heading gives two sections one anchor, and a link to it lands on whichever // came first. diff --git a/internal/validate/knowledge_test.go b/internal/validate/knowledge_test.go index c672d47..f073f8f 100644 --- a/internal/validate/knowledge_test.go +++ b/internal/validate/knowledge_test.go @@ -384,3 +384,75 @@ func TestEverythingMergesAndCounts(t *testing.T) { } } } + +// A citation is a path inside the checkout. A traversing target must be refused before +// it reaches os.ReadFile, or a Markdown page decides what scc opens on the machine. +func TestCodewikiCitationsCannotEscapeTheWorkspace(t *testing.T) { + root := t.TempDir() + // A real file one level above the workspace, which the traversal below would reach. + write(t, filepath.Join(root, "..", "outside-the-workspace.txt"), "secret\nsecret\n") + defer os.Remove(filepath.Join(root, "..", "outside-the-workspace.txt")) + + for _, target := range []string{ + "../outside-the-workspace.txt", + "docs/../../outside-the-workspace.txt", + } { + t.Run(target, func(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "..", "outside-the-workspace.txt"), "secret\nsecret\n") + defer os.Remove(filepath.Join(dir, "..", "outside-the-workspace.txt")) + write(t, filepath.Join(paths.Codewiki(dir), "app.md"), + "# App\n\n## Section\n\n["+target+":1-2]()\n") + + got := runValidator(t, Codewiki, dir) + if !contains(got, "codewiki.citation-invalid") { + t.Errorf("rules = %v, want codewiki.citation-invalid for %q", got, target) + } + // The give-away that the read happened anyway: a resolved citation is + // range-checked, so out-of-range means scc opened the outside file. + if contains(got, "codewiki.citation-out-of-range") { + t.Errorf("rules = %v: %q was read despite escaping the workspace", got, target) + } + }) + } +} + +// An absolute citation escapes just as surely, and filepath.Join would silently graft +// it onto root on Windows while honoring it on Unix. +func TestCodewikiCitationsCannotBeAbsolute(t *testing.T) { + root := t.TempDir() + write(t, filepath.Join(paths.Codewiki(root), "app.md"), + "# App\n\n## Section\n\n[/etc/passwd:1-2]()\n") + + got := runValidator(t, Codewiki, root) + if !contains(got, "codewiki.citation-invalid") { + t.Errorf("rules = %v, want codewiki.citation-invalid", got) + } +} + +// baseName drops a Go major-version suffix so the last real path segment is what gets +// compared against stack.md. Every major from 2 up is a valid suffix, and the two-digit +// ones are the easy range to lose: a leading-digit character class that starts at 2 +// silently rejects v10 through v19. +func TestBaseNameDropsEveryMajorVersionSuffix(t *testing.T) { + for _, tc := range []struct{ module, want string }{ + {"github.com/go-chi/chi/v5", "chi"}, + {"github.com/go-chi/chi/v2", "chi"}, + {"github.com/go-chi/chi/v9", "chi"}, + {"github.com/go-chi/chi/v10", "chi"}, + {"github.com/go-chi/chi/v11", "chi"}, + {"github.com/go-chi/chi/v19", "chi"}, + {"github.com/go-chi/chi/v20", "chi"}, + {"github.com/go-chi/chi/v100", "chi"}, + // v1 is never written as a path suffix, and v0 is not a major version. Neither + // is a segment that merely starts with v. + {"github.com/go-chi/chi/v1", "v1"}, + {"github.com/go-chi/chi/v0", "v0"}, + {"github.com/spf13/viper", "viper"}, + {"gopkg.in/yaml.v3", "yaml.v3"}, + } { + if got := baseName(tc.module); got != tc.want { + t.Errorf("baseName(%q) = %q, want %q", tc.module, got, tc.want) + } + } +} diff --git a/internal/validate/skill.go b/internal/validate/skill.go index 3a8cbcd..57f0a54 100644 --- a/internal/validate/skill.go +++ b/internal/validate/skill.go @@ -7,6 +7,7 @@ import ( "regexp" "sort" "strings" + "unicode/utf8" "github.com/protonspy/spec-claude-code/internal/finding" "github.com/protonspy/spec-claude-code/internal/mdscan" @@ -25,6 +26,12 @@ const ( skillBodyMaxLines = 500 // "Keep your main SKILL.md under 500 lines" ) +// The spec counts characters, so scc counts runes. len() on a string is bytes, which +// agrees with the spec only for ASCII and reports a shorter budget than the standard +// grants for anything else — an accented description would be failed at 1024 bytes +// while still inside its 1024 characters. +func charLen(s string) int { return utf8.RuneCountInString(s) } + // skillNameRe is the name charset: lowercase alphanumerics and hyphens, no leading // or trailing hyphen, no consecutive hyphens. var skillNameRe = regexp.MustCompile(`^[a-z0-9]+(-[a-z0-9]+)*$`) @@ -95,9 +102,9 @@ func skill(set *finding.Set, root, dir, dirName string) error { checkSkillName(set, file, fm, dirName) checkSkillDescription(set, file, fm) - if v, ok := fm.Get("compatibility"); ok && len(v) > skillCompatibilityMax { + if v, ok := fm.Get("compatibility"); ok && charLen(v) > skillCompatibilityMax { set.Addf(file, 1, "skill.compatibility-too-long", - "`compatibility` is %d characters; the spec allows %d", len(v), skillCompatibilityMax) + "`compatibility` is %d characters; the spec allows %d", charLen(v), skillCompatibilityMax) } // The body budget is a SHOULD in the spec, and it is reported as one: the agent @@ -119,9 +126,9 @@ func checkSkillName(set *finding.Set, file string, fm mdscan.Frontmatter, dirNam set.Addf(file, 1, "skill.missing-name", "`name` is required and must be 1-%d characters", skillNameMax) return } - if len(name) > skillNameMax { + if charLen(name) > skillNameMax { set.Addf(file, 1, "skill.name-too-long", - "`name` is %d characters; the spec allows 1-%d", len(name), skillNameMax) + "`name` is %d characters; the spec allows 1-%d", charLen(name), skillNameMax) } if !skillNameRe.MatchString(name) { set.Addf(file, 1, "skill.name-invalid", @@ -142,9 +149,9 @@ func checkSkillDescription(set *finding.Set, file string, fm mdscan.Frontmatter) "`description` is required: say what the skill does and when to use it") return } - if len(desc) > skillDescriptionMax { + if charLen(desc) > skillDescriptionMax { set.Addf(file, 1, "skill.description-too-long", - "`description` is %d characters; the spec allows 1-%d", len(desc), skillDescriptionMax) + "`description` is %d characters; the spec allows 1-%d", charLen(desc), skillDescriptionMax) } } diff --git a/internal/validate/skill_test.go b/internal/validate/skill_test.go index 56e3c9f..2a154b6 100644 --- a/internal/validate/skill_test.go +++ b/internal/validate/skill_test.go @@ -294,3 +294,35 @@ func TestFindingsAreOrderedAcrossSkills(t *testing.T) { t.Errorf("first finding is %s, want the alpha skill", got[0].File) } } + +// The spec's limits are in characters. Counting bytes agrees only for ASCII, and would +// fail a description that is inside its 1024 characters but past 1024 bytes — the +// false positive on a valid skill that TestLengthLimitsComeFromTheSpec guards for ASCII. +func TestLengthLimitsCountCharactersNotBytes(t *testing.T) { + // Two bytes per rune, so a description exactly at the limit is twice over in bytes. + desc := strings.Repeat("é", skillDescriptionMax) + if len(desc) <= skillDescriptionMax { + t.Fatalf("test premise broken: %d bytes is not over the %d limit", len(desc), skillDescriptionMax) + } + root := t.TempDir() + writeSkill(t, root, "accented", "---\nname: accented\ndescription: "+desc+"\n---\n\nBody.\n") + if hasRule(skills(t, root), "skill.description-too-long") { + t.Errorf("a description of exactly %d characters was reported as too long", skillDescriptionMax) + } + + // And one character over is still caught, so the fix did not simply disable the check. + root = t.TempDir() + writeSkill(t, root, "accented", "---\nname: accented\ndescription: "+desc+"é\n---\n\nBody.\n") + if !hasRule(skills(t, root), "skill.description-too-long") { + t.Error("a description one character over the limit was accepted") + } +} + +func TestCompatibilityLimitCountsCharacters(t *testing.T) { + root := t.TempDir() + writeSkill(t, root, "picky", "---\nname: picky\ndescription: A skill. Use when testing.\ncompatibility: "+ + strings.Repeat("é", skillCompatibilityMax)+"\n---\n\nBody.\n") + if hasRule(skills(t, root), "skill.compatibility-too-long") { + t.Errorf("a compatibility field of exactly %d characters was reported as too long", skillCompatibilityMax) + } +} diff --git a/internal/validate/stack.go b/internal/validate/stack.go index 06fa92f..71a05d8 100644 --- a/internal/validate/stack.go +++ b/internal/validate/stack.go @@ -179,4 +179,8 @@ func baseName(module string) string { } // majorVersionRe matches a Go module's major-version path suffix: v2, v5, v11. -var majorVersionRe = regexp.MustCompile(`^v[2-9]\d*$`) +// +// Every integer from 2 up, which is two alternatives rather than one character class: +// a bare 2-9, or a multi-digit number whose leading digit may be 1 (v10, v11). A single +// `v[2-9]\d*` looks equivalent and is not — it rejects the whole v10-v19 range. +var majorVersionRe = regexp.MustCompile(`^v([2-9]|[1-9]\d+)$`) From 61fd6e41b990db64297feb55021ac8aa4ca6a2fc Mon Sep 17 00:00:00 2001 From: Claude Code Date: Thu, 30 Jul 2026 19:57:08 +0000 Subject: [PATCH 2/3] fix(validate): resolve symlinks before accepting a codewiki citation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lexical containment check landed in the previous commit is necessary and not sufficient. A symlink that sits inside the workspace and points out of it never spells "..", so filepath.Clean has nothing to collapse and the check passes it through — then isFile and os.ReadFile follow the link and the kernel performs the escape the path itself never described. It failed the same silent way the original traversal did: the linked file resolved, its lines counted in range, and the validator reported nothing at all. A regression test asserts both halves, that citation-invalid fires and that citation-out-of-range does not, since the latter would prove the read happened. The root is resolved alongside the target rather than compared raw. A workspace legitimately lives under a symlink — macOS puts every t.TempDir() behind /private/var — and matching a fully resolved target against an unresolved root would call every citation in such a workspace an escape. The second test pins that direction so the containment check cannot be tightened into a false positive on a real workspace. A target that does not resolve is left to the existing unresolved finding: nothing was followed, so nothing escaped, and naming a typo an escape would be the less useful of the two answers. Found in review of #2. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013XH8WYThtTSGJxS7tXRvfp --- internal/validate/codewiki.go | 37 ++++++++++++++++++++++-- internal/validate/knowledge_test.go | 45 +++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/internal/validate/codewiki.go b/internal/validate/codewiki.go index 0bdc880..65aa9c8 100644 --- a/internal/validate/codewiki.go +++ b/internal/validate/codewiki.go @@ -137,19 +137,52 @@ func checkCitation(set *finding.Set, root, file string, line int, m []string) { // resolveInRoot joins target onto root and reports whether the result stayed inside it. // An absolute target is rejected outright; a relative one is rejected once Clean has // walked it above root. +// +// Lexical containment is necessary and not sufficient. Both isFile and os.ReadFile +// follow symlinks, so a link that sits inside the workspace and points out of it walks +// past a Clean-based check untouched — the path never spells "..", the kernel does the +// escaping. The second check therefore compares the paths with every link already +// resolved. func resolveInRoot(root, target string) (string, bool) { t := filepath.FromSlash(target) if filepath.IsAbs(t) || strings.HasPrefix(target, "/") { return "", false } resolved := filepath.Join(root, t) - rel, err := filepath.Rel(root, resolved) - if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + if !within(root, resolved) { + return "", false + } + + // The root is resolved too, not just the target: a workspace legitimately sits under + // a symlink (macOS puts /var behind /private/var), and comparing a fully resolved + // target against an unresolved root would call every citation in such a workspace an + // escape. + realRoot, err := filepath.EvalSymlinks(root) + if err != nil { + return "", false + } + realResolved, err := filepath.EvalSymlinks(resolved) + if err != nil { + // The target does not exist, so nothing was followed and nothing escaped. The + // lexical result stands and the caller reports it as unresolved, which is the + // more useful finding than calling a typo an escape. + return resolved, true + } + if !within(realRoot, realResolved) { return "", false } return resolved, true } +// within reports whether path is root or sits beneath it, comparing lexically. +func within(root, path string) bool { + rel, err := filepath.Rel(root, path) + if err != nil { + return false + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + // checkCodewikiHeadings holds slugs to being unique and derived from their headings. A // duplicated heading gives two sections one anchor, and a link to it lands on whichever // came first. diff --git a/internal/validate/knowledge_test.go b/internal/validate/knowledge_test.go index f073f8f..5403258 100644 --- a/internal/validate/knowledge_test.go +++ b/internal/validate/knowledge_test.go @@ -456,3 +456,48 @@ func TestBaseNameDropsEveryMajorVersionSuffix(t *testing.T) { } } } + +// Lexical containment is not enough. A symlink inside the workspace pointing out of it +// spells no "..", so a Clean-based check passes it through — and isFile and os.ReadFile +// both follow it. The escape is silent in exactly the way the traversal case was. +func TestCodewikiCitationsCannotEscapeThroughASymlink(t *testing.T) { + root := t.TempDir() + outside := t.TempDir() + + secret := filepath.Join(outside, "secret.txt") + write(t, secret, "secret\nsecret\n") + link := filepath.Join(root, "link.txt") + if err := os.Symlink(secret, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) // unprivileged Windows + } + + write(t, filepath.Join(paths.Codewiki(root), "app.md"), + "# App\n\n## Section\n\n[link.txt:1-2]()\n") + + got := runValidator(t, Codewiki, root) + if !contains(got, "codewiki.citation-invalid") { + t.Errorf("rules = %v, want codewiki.citation-invalid: the link left the workspace", got) + } + if contains(got, "codewiki.citation-out-of-range") { + t.Errorf("rules = %v: the outside file was read through the symlink", got) + } +} + +// The counterpart, and the reason the root is resolved rather than compared raw: a +// workspace that itself sits under a symlink still validates its own files. macOS does +// this to every t.TempDir() by putting /var behind /private/var. +func TestCodewikiValidatesAWorkspaceBehindASymlink(t *testing.T) { + real := t.TempDir() + write(t, filepath.Join(real, "main.go"), "one\ntwo\n") + write(t, filepath.Join(paths.Codewiki(real), "app.md"), + "# App\n\n## Section\n\n[main.go:1-2]()\n") + + link := filepath.Join(t.TempDir(), "workspace") + if err := os.Symlink(real, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + if got := runValidator(t, Codewiki, link); len(got) != 0 { + t.Errorf("rules = %v, want none: a citation inside a symlinked workspace is not an escape", got) + } +} From e51adf2337116c954288583c93487891661dd3db Mon Sep 17 00:00:00 2001 From: Claude Code Date: Thu, 30 Jul 2026 20:03:45 +0000 Subject: [PATCH 3/3] test(validate): cite past the fixture so the no-read assertion can fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both escape tests asserted that codewiki.citation-out-of-range did not fire, as the evidence that the outside file was never opened. The fixture has two lines and the citation asked for 1-2, which is inside it — so that finding would have stayed silent whether the read happened or not, and the assertion proved nothing. The commits that added them claimed the tests pinned both halves; only the citation-invalid half was actually pinned. Citing 1-3 puts the range past the end of the fixture, so a regression that follows the traversal or the symlink now reports out-of-range and the assertion fails. Confirmed in both directions: against the pre-fix code the traversal case reports [codewiki.citation-out-of-range] where it previously reported [], which is positive evidence of the read rather than the absence of a finding. Found in review of #2. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013XH8WYThtTSGJxS7tXRvfp --- internal/validate/knowledge_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/validate/knowledge_test.go b/internal/validate/knowledge_test.go index 5403258..f245f5e 100644 --- a/internal/validate/knowledge_test.go +++ b/internal/validate/knowledge_test.go @@ -401,8 +401,10 @@ func TestCodewikiCitationsCannotEscapeTheWorkspace(t *testing.T) { dir := t.TempDir() write(t, filepath.Join(dir, "..", "outside-the-workspace.txt"), "secret\nsecret\n") defer os.Remove(filepath.Join(dir, "..", "outside-the-workspace.txt")) + // Past the end of the two-line fixture, so the out-of-range assertion + // below can actually distinguish a blocked citation from a read one. write(t, filepath.Join(paths.Codewiki(dir), "app.md"), - "# App\n\n## Section\n\n["+target+":1-2]()\n") + "# App\n\n## Section\n\n["+target+":1-3]()\n") got := runValidator(t, Codewiki, dir) if !contains(got, "codewiki.citation-invalid") { @@ -471,8 +473,11 @@ func TestCodewikiCitationsCannotEscapeThroughASymlink(t *testing.T) { t.Skipf("symlinks unavailable: %v", err) // unprivileged Windows } + // The cited range runs past the end of the two-line fixture on purpose. Citing 1-2 + // would sit inside the file, so out-of-range would stay silent whether or not the + // read happened and the second assertion below would prove nothing. write(t, filepath.Join(paths.Codewiki(root), "app.md"), - "# App\n\n## Section\n\n[link.txt:1-2]()\n") + "# App\n\n## Section\n\n[link.txt:1-3]()\n") got := runValidator(t, Codewiki, root) if !contains(got, "codewiki.citation-invalid") {