From bab5208f531668dd6d086c218ad94ac406e4c532 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 09:00:54 +0000 Subject: [PATCH 01/28] plan 84: mark in-progress --- PLAN.md | 2 +- plan/84_symlink-default-deny.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/PLAN.md b/PLAN.md index 21ce66ca3..a93587290 100644 --- a/PLAN.md +++ b/PLAN.md @@ -21,7 +21,7 @@ footer: | | 65 | 🔲 | [Spike WASM-Embedded Weasel Inference](plan/65_spike-wasm-embedded-inference.md) | | 78 | ✅ | [Query subcommand for front-matter filtering](plan/78_query-command.md) | | 83 | ✅ | [Security hardening batch](plan/83_security-hardening-batch.md) | -| 84 | 🔲 | [Symlink default-deny for file discovery](plan/84_symlink-default-deny.md) | +| 84 | 🔳 | [Symlink default-deny for file discovery](plan/84_symlink-default-deny.md) | | 85 | 🔳 | [Increase test coverage to 95% by extracting shared rule helpers](plan/85_coverage-to-95-percent.md) | | 86 | 🔳 | [Markdown flavor validation](plan/86_markdown-flavor-validation.md) | | 89 | 🔲 | [TOC generator directive and MDS035 auto-fix](plan/89_toc-generator-directive.md) | diff --git a/plan/84_symlink-default-deny.md b/plan/84_symlink-default-deny.md index aacf7d218..ada54fe3c 100644 --- a/plan/84_symlink-default-deny.md +++ b/plan/84_symlink-default-deny.md @@ -1,7 +1,7 @@ --- id: 84 title: 'Symlink default-deny for file discovery' -status: "🔲" +status: "🔳" summary: >- Skip symlinks by default during directory walks; add --follow-symlinks opt-in flag. From 75715bd4f0bc850b897af531afb5ab1ea45ab96e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 09:02:21 +0000 Subject: [PATCH 02/28] =?UTF-8?q?plan=2084:=20red=20=E2=80=94=20assert=20s?= =?UTF-8?q?ymlinks=20are=20skipped=20by=20default=20(--follow-symlinks=20o?= =?UTF-8?q?pt-in)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- cmd/mdsmith/e2e_symlink_default_deny_test.go | 145 +++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 cmd/mdsmith/e2e_symlink_default_deny_test.go diff --git a/cmd/mdsmith/e2e_symlink_default_deny_test.go b/cmd/mdsmith/e2e_symlink_default_deny_test.go new file mode 100644 index 000000000..3e74681e8 --- /dev/null +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -0,0 +1,145 @@ +package main_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Plan 84: symlinks are skipped by default across discovery and +// explicit walks; users must opt in with --follow-symlinks (CLI) or +// follow-symlinks: true (config). + +// TestE2E_Symlink_DefaultDeny_ExternalTargetSkipped is the core +// security test: a repo with a symlink pointing to a file outside +// the project must not be walked by default. Running `fix` would +// otherwise overwrite that external file. +func TestE2E_Symlink_DefaultDeny_ExternalTargetSkipped(t *testing.T) { + project := t.TempDir() + external := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "rules:\n no-trailing-spaces: true\n") + writeFixture(t, project, "ok.md", "# Title\n\nClean body.\n") + + // Place a dirty markdown file OUTSIDE the project and symlink + // it in. Without default-deny, `check` would find it. + externalFile := filepath.Join(external, "evil.md") + require.NoError(t, os.WriteFile(externalFile, + []byte("# Evil\n\ntrailing \n"), 0o644)) + require.NoError(t, os.Symlink(externalFile, + filepath.Join(project, "evil.md"))) + + // Default: symlink is skipped, only ok.md is seen, exit 0. + _, stderr, exitCode := runBinaryInDir(t, project, "", + "check", "--no-color", "--no-gitignore", ".") + assert.Equal(t, 0, exitCode, + "expected exit 0 with symlink skipped by default, got %d; stderr: %s", + exitCode, stderr) +} + +// TestE2E_Symlink_FollowSymlinksFlag_OptsIn asserts the new +// --follow-symlinks CLI flag walks symlinked entries. +func TestE2E_Symlink_FollowSymlinksFlag_OptsIn(t *testing.T) { + project := t.TempDir() + external := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "rules:\n no-trailing-spaces: true\n") + + externalFile := filepath.Join(external, "dirty.md") + require.NoError(t, os.WriteFile(externalFile, + []byte("# Dirty\n\ntrailing \n"), 0o644)) + require.NoError(t, os.Symlink(externalFile, + filepath.Join(project, "linked.md"))) + + // Opting in follows the symlink and flags the trailing-space issue. + _, _, exitCode := runBinaryInDir(t, project, "", + "check", "--no-color", "--no-gitignore", "--follow-symlinks", ".") + assert.Equal(t, 1, exitCode, + "expected exit 1 with --follow-symlinks exposing dirty linked file") +} + +// TestE2E_Symlink_FollowSymlinksConfigKey_OptsIn asserts the new +// follow-symlinks: true config key works. +func TestE2E_Symlink_FollowSymlinksConfigKey_OptsIn(t *testing.T) { + project := t.TempDir() + external := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "follow-symlinks: true\nrules:\n no-trailing-spaces: true\n") + + externalFile := filepath.Join(external, "dirty.md") + require.NoError(t, os.WriteFile(externalFile, + []byte("# Dirty\n\ntrailing \n"), 0o644)) + require.NoError(t, os.Symlink(externalFile, + filepath.Join(project, "linked.md"))) + + _, _, exitCode := runBinaryInDir(t, project, "", + "check", "--no-color", "--no-gitignore", ".") + assert.Equal(t, 1, exitCode, + "expected exit 1 with follow-symlinks: true exposing dirty linked file") +} + +// TestE2E_Symlink_LegacyNoFollowConfig_Deprecation verifies that the +// old `no-follow-symlinks:` key still parses and emits a deprecation +// warning on stderr. +func TestE2E_Symlink_LegacyNoFollowConfig_Deprecation(t *testing.T) { + project := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "no-follow-symlinks:\n - \"**\"\nrules:\n no-trailing-spaces: true\n") + writeFixture(t, project, "ok.md", "# Title\n\nClean body.\n") + + _, stderr, exitCode := runBinaryInDir(t, project, "", + "check", "--no-color", "--no-gitignore", ".") + assert.Equal(t, 0, exitCode, + "expected exit 0, got %d; stderr: %s", exitCode, stderr) + assert.Contains(t, stderr, "no-follow-symlinks", + "expected deprecation warning mentioning no-follow-symlinks, got: %s", + stderr) + assert.Contains(t, stderr, "deprecated", + "expected deprecation warning, got: %s", stderr) +} + +// TestE2E_Symlink_FixRespectsFollowSymlinks ensures `fix` also +// honors --follow-symlinks: the dirty external file must NOT be +// rewritten by default, and MUST be rewritten when opted in. +func TestE2E_Symlink_FixRespectsFollowSymlinks(t *testing.T) { + project := t.TempDir() + external := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "rules:\n no-trailing-spaces: true\n") + + externalFile := filepath.Join(external, "dirty.md") + const dirtyContent = "# Dirty\n\ntrailing \n" + require.NoError(t, os.WriteFile(externalFile, + []byte(dirtyContent), 0o644)) + require.NoError(t, os.Symlink(externalFile, + filepath.Join(project, "linked.md"))) + + // Default-deny: fix must not touch the external file. + _, _, _ = runBinaryInDir(t, project, "", + "fix", "--no-color", "--no-gitignore", ".") + got, err := os.ReadFile(externalFile) + require.NoError(t, err) + assert.Equal(t, dirtyContent, string(got), + "fix must not rewrite symlinked external file by default") + + // Opt-in: fix follows the symlink and rewrites the file. + _, _, _ = runBinaryInDir(t, project, "", + "fix", "--no-color", "--no-gitignore", "--follow-symlinks", ".") + got2, err := os.ReadFile(externalFile) + require.NoError(t, err) + assert.NotContains(t, string(got2), " \n", + "fix --follow-symlinks must rewrite symlinked external file") +} From 74aa0afa91a9749e122dc3d321b05607f07ee4fc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 13:27:32 +0000 Subject: [PATCH 03/28] =?UTF-8?q?plan=2084:=20green=20=E2=80=94=20symlinks?= =?UTF-8?q?=20skipped=20by=20default=20across=20walks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Invert the symlink-follow default to protect `check` and `fix` from a malicious symlink pointing outside the project. - config.Config: FollowSymlinks bool replaces the pattern-based NoFollowSymlinks slice; LegacyNoFollowSymlinks retains the old key so it still parses, and Deprecations surfaces a warning. - lint.ResolveOpts, walkDir, resolveGlob: FollowSymlinks bool controls both directory walks and glob expansion; the zero value skips all symlinks. - discovery.Options/walker: same shape; the `matchesPath` glob helper is no longer needed. - cmd/mdsmith: new --follow-symlinks flag on check/fix/metrics; the old --no-follow-symlinks flag is registered as a silent deprecation via a shared helper. loadConfig prints any config-level deprecation warnings once per invocation. - Tests: red->green for the new behavior across both unit and e2e suites; legacy-flag/config tests retained and renamed to document the silent-acceptance path. --- cmd/mdsmith/e2e_coverage_test.go | 56 ++++---- cmd/mdsmith/e2e_symlink_default_deny_test.go | 37 ++++-- cmd/mdsmith/main.go | 115 ++++++++++------- cmd/mdsmith/metrics.go | 23 ++-- internal/config/config.go | 31 +++-- internal/config/load.go | 8 ++ internal/config/merge.go | 48 +++---- internal/discovery/discovery.go | 55 +++----- internal/discovery/discovery_coverage_test.go | 121 ++++++------------ internal/lint/files.go | 57 ++------- internal/lint/files_test.go | 76 +++++++---- internal/lint/lint_coverage_test.go | 52 +------- 12 files changed, 313 insertions(+), 366 deletions(-) diff --git a/cmd/mdsmith/e2e_coverage_test.go b/cmd/mdsmith/e2e_coverage_test.go index 4e2c1b6b7..e974ad3c9 100644 --- a/cmd/mdsmith/e2e_coverage_test.go +++ b/cmd/mdsmith/e2e_coverage_test.go @@ -137,37 +137,39 @@ func TestE2E_Fix_Discovered_BadConfig_ExitsTwo(t *testing.T) { } // ============================================================= -// 9. resolveOpts — --no-follow-symlinks flag +// 9. Deprecated --no-follow-symlinks flag is silently accepted // ============================================================= -func TestE2E_Check_NoFollowSymlinks(t *testing.T) { +// TestE2E_Check_LegacyNoFollowSymlinksFlag asserts the removed +// `--no-follow-symlinks` flag still parses without error (plan 84 +// "accept silently") and that the new secure default leaves the +// real directory reachable. +func TestE2E_Check_LegacyNoFollowSymlinksFlag(t *testing.T) { dir := t.TempDir() - // Use a config with rules enabled and file discovery patterns. require.NoError(t, os.MkdirAll(filepath.Join(dir, ".git"), 0o755)) writeFixture(t, dir, ".mdsmith.yml", "files:\n - \"**/*.md\"\nrules:\n no-trailing-spaces: true\n") - // Create a subdirectory with a dirty file, and a symlink to it. subDir := filepath.Join(dir, "real") require.NoError(t, os.MkdirAll(subDir, 0o755)) writeFixture(t, subDir, "dirty.md", "# Title\n\nHello \n") require.NoError(t, os.Symlink(subDir, filepath.Join(dir, "linked"))) - // Without --no-follow-symlinks, discovery finds dirty.md via symlink. - // Use --no-gitignore to avoid ancestor .gitignore interference. + // Default-deny already skips the symlink; exit 1 from real/dirty.md. _, _, exitWithout := runBinaryInDir(t, dir, "", "check", "--no-color", "--no-gitignore") assert.Equal(t, 1, exitWithout, - "expected exit 1 without --no-follow-symlinks (dirty file found via discovery)") + "expected exit 1 (dirty real/dirty.md found under default-deny)") - // With --no-follow-symlinks, symlinked dir is skipped; only real/ found. + // Same outcome with the deprecated flag present (flag is silently accepted). _, stderr, exitCode := runBinaryInDir(t, dir, "", "check", "--no-color", "--no-gitignore", "--no-follow-symlinks") - // Should still find real/dirty.md (exit 1) but the flag exercises resolveOpts. assert.Equal(t, 1, exitCode, - "expected exit 1 (real/dirty.md still found), got %d; stderr: %s", exitCode, stderr) + "legacy --no-follow-symlinks must still parse; stderr: %s", stderr) } -func TestE2E_Fix_NoFollowSymlinks(t *testing.T) { +// TestE2E_Fix_LegacyNoFollowSymlinksFlag covers the same silent- +// acceptance path for the fix subcommand. +func TestE2E_Fix_LegacyNoFollowSymlinksFlag(t *testing.T) { dir := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(dir, ".git"), 0o755)) writeFixture(t, dir, ".mdsmith.yml", @@ -178,7 +180,6 @@ func TestE2E_Fix_NoFollowSymlinks(t *testing.T) { writeFixture(t, subDir, "fixme.md", "# Title\n\nHello \n") require.NoError(t, os.Symlink(subDir, filepath.Join(dir, "linked"))) - // --no-follow-symlinks exercises resolveOpts; real/fixme.md still found. _, stderr, exitCode := runBinaryInDir(t, dir, "", "fix", "--no-color", "--no-follow-symlinks") assert.Equal(t, 0, exitCode, @@ -486,31 +487,35 @@ func TestE2E_Query_Verbose_MixedResults(t *testing.T) { } // ============================================================= -// 36. check with no-follow-symlinks via discovery +// 36. Legacy no-follow-symlinks config key emits a deprecation // ============================================================= -func TestE2E_Check_Discovered_NoFollowSymlinks(t *testing.T) { +// TestE2E_Check_LegacyNoFollowSymlinksConfig asserts that the old +// `no-follow-symlinks:` config key is still parsed (no hard error) +// and surfaces a deprecation warning. The new default-deny already +// achieves what the legacy key intended, so discovery results match +// what the user expects. +func TestE2E_Check_LegacyNoFollowSymlinksConfig(t *testing.T) { dir := t.TempDir() - // Create config that enables no-follow-symlinks. - writeFixture(t, dir, ".mdsmith.yml", "no-follow-symlinks:\n - \"**\"\nrules:\n no-trailing-spaces: true\n") + writeFixture(t, dir, ".mdsmith.yml", + "no-follow-symlinks:\n - \"**\"\nrules:\n no-trailing-spaces: true\n") - // Create a real directory with a dirty file. subDir := filepath.Join(dir, "real") require.NoError(t, os.MkdirAll(subDir, 0o755)) writeFixture(t, subDir, "dirty.md", "# Title\n\nHello \n") - // Create a symlink to the subdirectory. link := filepath.Join(dir, "linked") require.NoError(t, os.Symlink(subDir, link)) - // The real dir's file should be found, but the symlinked one should not. _, stderr, exitCode := runBinaryInDir(t, dir, "", "check", "--no-color") - // We expect exit 1 (real/dirty.md found) but only once. assert.Equal(t, 1, exitCode, "expected exit 1 (real dirty.md found), got %d; stderr: %s", exitCode, stderr) - // Should not report the symlinked path. assert.NotContains(t, stderr, "linked/", - "expected symlinked dir to be skipped, but found in stderr: %s", stderr) + "symlinked dir must be skipped under default-deny; stderr: %s", stderr) + assert.Contains(t, stderr, "no-follow-symlinks", + "expected deprecation warning, got stderr: %s", stderr) + assert.Contains(t, stderr, "deprecated", + "expected deprecation warning, got stderr: %s", stderr) } // ============================================================= @@ -686,10 +691,13 @@ func TestE2E_Init_HelpFlag(t *testing.T) { } // ============================================================= -// Additional: metrics rank with --no-follow-symlinks +// Additional: metrics rank still accepts deprecated flag // ============================================================= -func TestE2E_MetricsRank_NoFollowSymlinks(t *testing.T) { +// TestE2E_MetricsRank_LegacyNoFollowSymlinksFlag asserts the +// deprecated --no-follow-symlinks flag is silently accepted by the +// metrics rank subcommand after plan 84. +func TestE2E_MetricsRank_LegacyNoFollowSymlinksFlag(t *testing.T) { dir := t.TempDir() writeFixture(t, dir, "a.md", "# Title\n\nSome content here.\n") diff --git a/cmd/mdsmith/e2e_symlink_default_deny_test.go b/cmd/mdsmith/e2e_symlink_default_deny_test.go index 3e74681e8..67c287976 100644 --- a/cmd/mdsmith/e2e_symlink_default_deny_test.go +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -109,9 +109,11 @@ func TestE2E_Symlink_LegacyNoFollowConfig_Deprecation(t *testing.T) { "expected deprecation warning, got: %s", stderr) } -// TestE2E_Symlink_FixRespectsFollowSymlinks ensures `fix` also -// honors --follow-symlinks: the dirty external file must NOT be -// rewritten by default, and MUST be rewritten when opted in. +// TestE2E_Symlink_FixRespectsFollowSymlinks ensures `fix` honors +// --follow-symlinks: the dirty external file is never rewritten +// (atomic rename replaces the symlink itself, not its target — see +// plan 83 section C), and the in-project symlink is only visited +// when the flag is set. func TestE2E_Symlink_FixRespectsFollowSymlinks(t *testing.T) { project := t.TempDir() external := t.TempDir() @@ -124,22 +126,37 @@ func TestE2E_Symlink_FixRespectsFollowSymlinks(t *testing.T) { const dirtyContent = "# Dirty\n\ntrailing \n" require.NoError(t, os.WriteFile(externalFile, []byte(dirtyContent), 0o644)) - require.NoError(t, os.Symlink(externalFile, - filepath.Join(project, "linked.md"))) + linked := filepath.Join(project, "linked.md") + require.NoError(t, os.Symlink(externalFile, linked)) - // Default-deny: fix must not touch the external file. + // Default-deny: fix does not visit the symlink. The link remains + // a symlink and the external file is untouched. _, _, _ = runBinaryInDir(t, project, "", "fix", "--no-color", "--no-gitignore", ".") + lstat, err := os.Lstat(linked) + require.NoError(t, err) + assert.NotZero(t, lstat.Mode()&os.ModeSymlink, + "default-deny must leave the symlink intact") got, err := os.ReadFile(externalFile) require.NoError(t, err) assert.Equal(t, dirtyContent, string(got), "fix must not rewrite symlinked external file by default") - // Opt-in: fix follows the symlink and rewrites the file. + // Opt-in: fix visits the symlink. Atomic rename replaces the + // symlink with a regular file containing the fixed content; the + // external target stays untouched (plan 83 write-side protection). _, _, _ = runBinaryInDir(t, project, "", "fix", "--no-color", "--no-gitignore", "--follow-symlinks", ".") - got2, err := os.ReadFile(externalFile) + lstat2, err := os.Lstat(linked) + require.NoError(t, err) + assert.Zero(t, lstat2.Mode()&os.ModeSymlink, + "fix --follow-symlinks must replace symlink with a regular file") + projectContent, err := os.ReadFile(linked) + require.NoError(t, err) + assert.NotContains(t, string(projectContent), " \n", + "fix --follow-symlinks must rewrite the in-project file") + extAfter, err := os.ReadFile(externalFile) require.NoError(t, err) - assert.NotContains(t, string(got2), " \n", - "fix --follow-symlinks must rewrite symlinked external file") + assert.Equal(t, dirtyContent, string(extAfter), + "fix must never rewrite the external symlink target directly") } diff --git a/cmd/mdsmith/main.go b/cmd/mdsmith/main.go index 8e55adcbd..8cbf060a0 100644 --- a/cmd/mdsmith/main.go +++ b/cmd/mdsmith/main.go @@ -154,14 +154,14 @@ func printVersion() { func runCheck(args []string) int { fs := flag.NewFlagSet("check", flag.ContinueOnError) var ( - configPath string - format string - noColor bool - quiet bool - verbose bool - noGitignore bool - noFollowSymlinks bool - maxInputSize string + configPath string + format string + noColor bool + quiet bool + verbose bool + noGitignore bool + followSymlinks bool + maxInputSize string ) fs.StringVarP(&configPath, "config", "c", "", "Override config file path") @@ -170,7 +170,8 @@ func runCheck(args []string) int { fs.BoolVarP(&quiet, "quiet", "q", false, "Suppress non-error output") fs.BoolVarP(&verbose, "verbose", "v", false, "Show config, files, and rules on stderr") fs.BoolVar(&noGitignore, "no-gitignore", false, "Disable .gitignore filtering when walking directories") - fs.BoolVar(&noFollowSymlinks, "no-follow-symlinks", false, "Skip symbolic links when walking directories") + fs.BoolVar(&followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") + registerLegacyNoFollowSymlinks(fs) fs.StringVar(&maxInputSize, "max-input-size", "", "Maximum file size to process (e.g. 2MB, 500KB, 0=unlimited)") fs.Usage = func() { @@ -204,14 +205,14 @@ func runCheck(args []string) int { if len(fileArgs) > 0 { return checkFiles( fileArgs, configPath, format, noColor, quiet, verbose, - noGitignore, noFollowSymlinks, maxInputSize, + noGitignore, followSymlinks, maxInputSize, ) } // No file args and no stdin: discover files from config. return checkDiscovered( configPath, format, noColor, quiet, verbose, - noGitignore, noFollowSymlinks, maxInputSize, + noGitignore, followSymlinks, maxInputSize, ) } @@ -219,14 +220,14 @@ func runCheck(args []string) int { func runFix(args []string) int { fs := flag.NewFlagSet("fix", flag.ContinueOnError) var ( - configPath string - format string - noColor bool - quiet bool - verbose bool - noGitignore bool - noFollowSymlinks bool - maxInputSize string + configPath string + format string + noColor bool + quiet bool + verbose bool + noGitignore bool + followSymlinks bool + maxInputSize string ) fs.StringVarP(&configPath, "config", "c", "", "Override config file path") @@ -235,7 +236,8 @@ func runFix(args []string) int { fs.BoolVarP(&quiet, "quiet", "q", false, "Suppress non-error output") fs.BoolVarP(&verbose, "verbose", "v", false, "Show config, files, and rules on stderr") fs.BoolVar(&noGitignore, "no-gitignore", false, "Disable .gitignore filtering when walking directories") - fs.BoolVar(&noFollowSymlinks, "no-follow-symlinks", false, "Skip symbolic links when walking directories") + fs.BoolVar(&followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") + registerLegacyNoFollowSymlinks(fs) fs.StringVar(&maxInputSize, "max-input-size", "", "Maximum file size to process (e.g. 2MB, 500KB, 0=unlimited)") fs.Usage = func() { @@ -270,14 +272,14 @@ func runFix(args []string) int { if len(fileArgs) > 0 { return fixFiles( fileArgs, configPath, format, noColor, quiet, verbose, - noGitignore, noFollowSymlinks, maxInputSize, + noGitignore, followSymlinks, maxInputSize, ) } // No file args: discover files from config. return fixDiscovered( configPath, format, noColor, quiet, verbose, - noGitignore, noFollowSymlinks, maxInputSize, + noGitignore, followSymlinks, maxInputSize, ) } @@ -526,11 +528,11 @@ func printRunStats(format string, quiet bool, stats runStats) { // checkFiles lints the given file paths and returns the appropriate exit code. func checkFiles( fileArgs []string, configPath, format string, - noColor, quiet, verbose, noGitignore, noFollowSymlinks bool, + noColor, quiet, verbose, noGitignore, followSymlinks bool, maxInputSize string, ) int { cfg, cfgPath, logger, files, maxBytes, code := loadAndResolve( - fileArgs, configPath, verbose, noGitignore, noFollowSymlinks, maxInputSize, + fileArgs, configPath, verbose, noGitignore, followSymlinks, maxInputSize, ) if code >= 0 { return code @@ -572,11 +574,11 @@ func checkFiles( // fixFiles fixes lint issues in the given file paths. func fixFiles( fileArgs []string, configPath, format string, - noColor, quiet, verbose, noGitignore, noFollowSymlinks bool, + noColor, quiet, verbose, noGitignore, followSymlinks bool, maxInputSize string, ) int { cfg, cfgPath, logger, files, maxBytes, code := loadAndResolve( - fileArgs, configPath, verbose, noGitignore, noFollowSymlinks, maxInputSize, + fileArgs, configPath, verbose, noGitignore, followSymlinks, maxInputSize, ) if code >= 0 { return code @@ -698,7 +700,7 @@ func checkStdin(format string, noColor, quiet, verbose bool, configPath, maxInpu // or -1 on success. func loadAndResolve( fileArgs []string, configPath string, - verbose, noGitignore, noFollowSymlinks bool, + verbose, noGitignore, followSymlinks bool, maxInputSize string, ) (*config.Config, string, *vlog.Logger, []string, int64, int) { logger := &vlog.Logger{Enabled: verbose, W: os.Stderr} @@ -712,7 +714,7 @@ func loadAndResolve( logger.Printf("config: %s", cfgPath) } - opts := resolveOpts(cfg, noGitignore, noFollowSymlinks) + opts := resolveOpts(cfg, noGitignore, followSymlinks) files, err := lint.ResolveFilesWithOpts(fileArgs, opts) if err != nil { fmt.Fprintf(os.Stderr, "mdsmith: %v\n", err) @@ -750,7 +752,7 @@ func splitStdinArg(args []string) (hasStdin bool, fileArgs []string) { // exit code; the caller should return it directly. A negative code means // "continue with the returned values". func discoverFiles( - configPath string, verbose, noGitignore, noFollowSymlinks bool, + configPath string, verbose, noGitignore, followSymlinks bool, ) (*config.Config, string, *vlog.Logger, []string, int) { logger := &vlog.Logger{Enabled: verbose, W: os.Stderr} @@ -767,9 +769,9 @@ func discoverFiles( } files, err := discovery.Discover(discovery.Options{ - Patterns: cfg.Files, - UseGitignore: !noGitignore, - NoFollowSymlinks: resolveOpts(cfg, noGitignore, noFollowSymlinks).NoFollowSymlinks, + Patterns: cfg.Files, + UseGitignore: !noGitignore, + FollowSymlinks: resolveOpts(cfg, noGitignore, followSymlinks).FollowSymlinks, }) if err != nil { fmt.Fprintf(os.Stderr, "mdsmith: discovering files: %v\n", err) @@ -785,10 +787,10 @@ func discoverFiles( // and lints them. Returns the appropriate exit code. func checkDiscovered( configPath, format string, - noColor, quiet, verbose, noGitignore, noFollowSymlinks bool, + noColor, quiet, verbose, noGitignore, followSymlinks bool, maxInputSize string, ) int { - cfg, cfgPath, logger, files, code := discoverFiles(configPath, verbose, noGitignore, noFollowSymlinks) + cfg, cfgPath, logger, files, code := discoverFiles(configPath, verbose, noGitignore, followSymlinks) if code >= 0 { return code } @@ -836,10 +838,10 @@ func checkDiscovered( // and fixes them. Returns the appropriate exit code. func fixDiscovered( configPath, format string, - noColor, quiet, verbose, noGitignore, noFollowSymlinks bool, + noColor, quiet, verbose, noGitignore, followSymlinks bool, maxInputSize string, ) int { - cfg, cfgPath, logger, files, code := discoverFiles(configPath, verbose, noGitignore, noFollowSymlinks) + cfg, cfgPath, logger, files, code := discoverFiles(configPath, verbose, noGitignore, followSymlinks) if code >= 0 { return code } @@ -883,19 +885,27 @@ func fixDiscovered( return 0 } +// registerLegacyNoFollowSymlinks registers the removed +// `--no-follow-symlinks` flag as a silently-accepted deprecation. +// Plan 84 inverted the default; the flag no longer has any effect. +func registerLegacyNoFollowSymlinks(fs *flag.FlagSet) { + _ = fs.Bool("no-follow-symlinks", false, + "Deprecated: symlinks are now skipped by default") +} + // frontMatterEnabled returns whether front matter stripping is enabled. // Defaults to true if not set in config. // resolveOpts builds ResolveOpts from config and CLI flags. // CLI flags override config: --no-gitignore disables gitignore filtering, -// --no-follow-symlinks skips all symlinks (adds "**" to the pattern list). -func resolveOpts(cfg *config.Config, noGitignore, noFollowSymlinks bool) lint.ResolveOpts { +// --follow-symlinks opts in to following symbolic links (default: skip). +func resolveOpts(cfg *config.Config, noGitignore, followSymlinks bool) lint.ResolveOpts { useGitignore := !noGitignore opts := lint.ResolveOpts{ - UseGitignore: &useGitignore, - NoFollowSymlinks: cfg.NoFollowSymlinks, + UseGitignore: &useGitignore, + FollowSymlinks: cfg.FollowSymlinks, } - if noFollowSymlinks { - opts.NoFollowSymlinks = []string{"**"} + if followSymlinks { + opts.FollowSymlinks = true } return opts } @@ -959,7 +969,9 @@ func loadConfigRaw(configPath string) (*config.Config, string, error) { if err != nil { return nil, "", err } - return config.Merge(defaults, loaded), configPath, nil + merged := config.Merge(defaults, loaded) + printDeprecations(merged) + return merged, configPath, nil } // Try to discover a config file. @@ -982,7 +994,22 @@ func loadConfigRaw(configPath string) (*config.Config, string, error) { return nil, "", err } - return config.Merge(defaults, loaded), discovered, nil + merged := config.Merge(defaults, loaded) + printDeprecations(merged) + return merged, discovered, nil +} + +// printDeprecations writes config deprecation warnings to stderr. It is +// safe to call multiple times; the warnings are consumed so the second +// call is a no-op. +func printDeprecations(cfg *config.Config) { + if cfg == nil { + return + } + for _, msg := range cfg.Deprecations { + fmt.Fprintf(os.Stderr, "mdsmith: deprecated: %s\n", msg) + } + cfg.Deprecations = nil } const helpUsageText = `Usage: mdsmith help diff --git a/cmd/mdsmith/metrics.go b/cmd/mdsmith/metrics.go index 3b89a37f9..3fe7aeb0b 100644 --- a/cmd/mdsmith/metrics.go +++ b/cmd/mdsmith/metrics.go @@ -92,15 +92,15 @@ func runMetricsList(args []string) int { } type metricsRankOptions struct { - configPath string - metricsRaw string - byRaw string - orderRaw string - top int - format string - noGitignore bool - noFollowSymlinks bool - maxInputSize string + configPath string + metricsRaw string + byRaw string + orderRaw string + top int + format string + noGitignore bool + followSymlinks bool + maxInputSize string } func runMetricsRank(args []string) int { @@ -123,7 +123,8 @@ func parseMetricsRankOptions(args []string) (metricsRankOptions, []string, error fs.IntVar(&opts.top, "top", 0, "Limit results to top N files (0 = all)") fs.StringVarP(&opts.format, "format", "f", "text", "Output format: text, json") fs.BoolVar(&opts.noGitignore, "no-gitignore", false, "Disable .gitignore filtering when walking directories") - fs.BoolVar(&opts.noFollowSymlinks, "no-follow-symlinks", false, "Skip symbolic links when walking directories") + fs.BoolVar(&opts.followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") + registerLegacyNoFollowSymlinks(fs) fs.StringVar(&opts.maxInputSize, "max-input-size", "", "Maximum file size to process (e.g. 2MB, 500KB, 0=unlimited)") @@ -243,7 +244,7 @@ func resolveRankSelection( } func resolveRankFiles(cfg *config.Config, opts metricsRankOptions, fileArgs []string) ([]string, error) { - resolveOptions := resolveOpts(cfg, opts.noGitignore, opts.noFollowSymlinks) + resolveOptions := resolveOpts(cfg, opts.noGitignore, opts.followSymlinks) return lint.ResolveFilesWithOpts(fileArgs, resolveOptions) } diff --git a/internal/config/config.go b/internal/config/config.go index 7f408ac8c..9aa1d519d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -24,15 +24,22 @@ var DefaultFiles = []string{"**/*.md", "**/*.markdown"} // Config is the top-level configuration. type Config struct { - Rules map[string]RuleCfg `yaml:"rules"` - Ignore []string `yaml:"ignore"` - Overrides []Override `yaml:"overrides"` - FrontMatter *bool `yaml:"front-matter"` - Categories map[string]bool `yaml:"categories"` - Files []string `yaml:"files"` - NoFollowSymlinks []string `yaml:"no-follow-symlinks"` - MaxInputSize string `yaml:"max-input-size"` - Archetypes ArchetypesCfg `yaml:"archetypes"` + Rules map[string]RuleCfg `yaml:"rules"` + Ignore []string `yaml:"ignore"` + Overrides []Override `yaml:"overrides"` + FrontMatter *bool `yaml:"front-matter"` + Categories map[string]bool `yaml:"categories"` + Files []string `yaml:"files"` + FollowSymlinks bool `yaml:"follow-symlinks"` + MaxInputSize string `yaml:"max-input-size"` + Archetypes ArchetypesCfg `yaml:"archetypes"` + + // LegacyNoFollowSymlinks captures the removed `no-follow-symlinks` + // key. Its presence surfaces a deprecation warning via + // Deprecations; its contents are otherwise ignored now that + // symlinks are skipped by default. + // Not serialized to YAML in marshalled output. + LegacyNoFollowSymlinks []string `yaml:"no-follow-symlinks,omitempty"` // ExplicitRules tracks rule names that were explicitly set in // the user config (not just inherited from defaults). This is @@ -46,6 +53,12 @@ type Config struct { // (use defaults) and an explicitly empty list (no files). // Not serialized to YAML. FilesExplicit bool `yaml:"-"` + + // Deprecations lists human-readable warnings about deprecated + // keys found in the loaded config. Callers (cmd/mdsmith) print + // them to stderr. + // Not serialized to YAML. + Deprecations []string `yaml:"-"` } // ArchetypesCfg configures archetype discovery. Roots are directories diff --git a/internal/config/load.go b/internal/config/load.go index 216bc9a00..c8c285fd2 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -37,6 +37,14 @@ func Load(path string) (*Config, error) { // Detect whether the "files" key was explicitly present in the YAML. cfg.FilesExplicit = yamlHasKey(data, "files") + if yamlHasKey(data, "no-follow-symlinks") { + cfg.Deprecations = append(cfg.Deprecations, + "config key `no-follow-symlinks` is deprecated; "+ + "symlinks are now skipped by default — "+ + "use `follow-symlinks: true` to opt in, "+ + "or remove the key") + } + return &cfg, nil } diff --git a/internal/config/merge.go b/internal/config/merge.go index f915491ac..ee9ee86f1 100644 --- a/internal/config/merge.go +++ b/internal/config/merge.go @@ -53,17 +53,19 @@ func Merge(defaults, loaded *Config) *Config { } return &Config{ - Rules: rules, - Ignore: loaded.Ignore, - Overrides: loaded.Overrides, - FrontMatter: fm, - Categories: cats, - Files: files, - FilesExplicit: filesExplicit, - ExplicitRules: explicit, - NoFollowSymlinks: loaded.NoFollowSymlinks, - MaxInputSize: maxInputSize, - Archetypes: archetypes, + Rules: rules, + Ignore: loaded.Ignore, + Overrides: loaded.Overrides, + FrontMatter: fm, + Categories: cats, + Files: files, + FilesExplicit: filesExplicit, + ExplicitRules: explicit, + FollowSymlinks: loaded.FollowSymlinks, + LegacyNoFollowSymlinks: copyStrings(loaded.LegacyNoFollowSymlinks), + Deprecations: copyStrings(loaded.Deprecations), + MaxInputSize: maxInputSize, + Archetypes: archetypes, } } @@ -81,17 +83,19 @@ func copyConfig(cfg *Config) *Config { explicit[k] = v } return &Config{ - Rules: rules, - Ignore: copyStrings(cfg.Ignore), - Overrides: cfg.Overrides, - FrontMatter: cfg.FrontMatter, - Categories: copyCategories(cfg.Categories), - Files: copyStrings(cfg.Files), - NoFollowSymlinks: copyStrings(cfg.NoFollowSymlinks), - MaxInputSize: cfg.MaxInputSize, - ExplicitRules: explicit, - FilesExplicit: cfg.FilesExplicit, - Archetypes: ArchetypesCfg{Roots: copyStrings(cfg.Archetypes.Roots)}, + Rules: rules, + Ignore: copyStrings(cfg.Ignore), + Overrides: cfg.Overrides, + FrontMatter: cfg.FrontMatter, + Categories: copyCategories(cfg.Categories), + Files: copyStrings(cfg.Files), + FollowSymlinks: cfg.FollowSymlinks, + LegacyNoFollowSymlinks: copyStrings(cfg.LegacyNoFollowSymlinks), + Deprecations: copyStrings(cfg.Deprecations), + MaxInputSize: cfg.MaxInputSize, + ExplicitRules: explicit, + FilesExplicit: cfg.FilesExplicit, + Archetypes: ArchetypesCfg{Roots: copyStrings(cfg.Archetypes.Roots)}, } } diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go index e4c497ae1..76be15b17 100644 --- a/internal/discovery/discovery.go +++ b/internal/discovery/discovery.go @@ -7,7 +7,6 @@ import ( "sort" "github.com/bmatcuk/doublestar/v4" - "github.com/gobwas/glob" "github.com/jeduden/mdsmith/internal/lint" ) @@ -23,8 +22,9 @@ type Options struct { // UseGitignore enables filtering by .gitignore rules. UseGitignore bool - // NoFollowSymlinks lists glob patterns for symlinks that should be skipped. - NoFollowSymlinks []string + // FollowSymlinks opts in to following symlinks during the walk. + // The zero value skips all symlinks, which is the secure default. + FollowSymlinks bool } // Discover walks BaseDir and returns files matching any of the configured @@ -55,11 +55,11 @@ func Discover(opts Options) ([]string, error) { } w := &walker{ - absBase: absBase, - patterns: validPatterns, - git: gitMatcher, - noFollow: opts.NoFollowSymlinks, - seen: make(map[string]bool), + absBase: absBase, + patterns: validPatterns, + git: gitMatcher, + followSymlinks: opts.FollowSymlinks, + seen: make(map[string]bool), } if err := filepath.Walk(absBase, w.visit); err != nil { @@ -83,12 +83,12 @@ func validatePatterns(patterns []string) []string { // walker holds state for the directory walk. type walker struct { - absBase string - patterns []string - git *lint.GitignoreMatcher - noFollow []string - seen map[string]bool - result []string + absBase string + patterns []string + git *lint.GitignoreMatcher + followSymlinks bool + seen map[string]bool + result []string } // visit is the filepath.WalkFunc callback. @@ -103,7 +103,7 @@ func (w *walker) visit(path string, info os.FileInfo, walkErr error) error { } rel = filepath.ToSlash(rel) - if w.isNoFollow(path, info) { + if !w.followSymlinks && info.Mode()&os.ModeSymlink != 0 { if info.IsDir() { return filepath.SkipDir } @@ -127,17 +127,6 @@ func (w *walker) visit(path string, info os.FileInfo, walkErr error) error { return nil } -// isNoFollow returns true if the path is a symlink matching no-follow patterns. -func (w *walker) isNoFollow(path string, info os.FileInfo) bool { - if len(w.noFollow) == 0 { - return false - } - if info.Mode()&os.ModeSymlink == 0 { - return false - } - return matchesPath(w.noFollow, path) -} - // isGitignored returns true if the path should be skipped by .gitignore rules. func (w *walker) isGitignored(path string, info os.FileInfo) bool { if w.git == nil { @@ -171,17 +160,3 @@ func (w *walker) addFile(rel, absPath string) { w.result = append(w.result, rel) } } - -func matchesPath(patterns []string, path string) bool { - cleanPath := filepath.Clean(path) - for _, pattern := range patterns { - g, err := glob.Compile(pattern) - if err != nil { - continue - } - if g.Match(path) || g.Match(cleanPath) || g.Match(filepath.Base(path)) { - return true - } - } - return false -} diff --git a/internal/discovery/discovery_coverage_test.go b/internal/discovery/discovery_coverage_test.go index c39e2127a..978c9eb19 100644 --- a/internal/discovery/discovery_coverage_test.go +++ b/internal/discovery/discovery_coverage_test.go @@ -10,71 +10,6 @@ import ( "github.com/stretchr/testify/require" ) -// --- matchesPath tests --- - -func TestMatchesPath_ExactMatch(t *testing.T) { - assert.True(t, matchesPath([]string{"foo.md"}, "foo.md")) -} - -func TestMatchesPath_GlobPattern(t *testing.T) { - assert.True(t, matchesPath([]string{"*.md"}, "readme.md")) -} - -func TestMatchesPath_NoMatch(t *testing.T) { - assert.False(t, matchesPath([]string{"*.txt"}, "readme.md")) -} - -func TestMatchesPath_EmptyPatterns(t *testing.T) { - assert.False(t, matchesPath([]string{}, "readme.md")) -} - -func TestMatchesPath_InvalidPattern(t *testing.T) { - // Invalid glob pattern should be skipped without panic. - assert.False(t, matchesPath([]string{"[invalid"}, "readme.md")) -} - -func TestMatchesPath_MatchesBasename(t *testing.T) { - assert.True(t, matchesPath([]string{"readme.md"}, "/some/path/readme.md")) -} - -func TestMatchesPath_MatchesCleanedPath(t *testing.T) { - assert.True(t, matchesPath([]string{"foo/bar.md"}, "foo//bar.md")) -} - -func TestMatchesPath_MultiplePatterns(t *testing.T) { - assert.True(t, matchesPath([]string{"*.txt", "*.md"}, "readme.md")) -} - -func TestMatchesPath_MultiplePatterns_NoneMatch(t *testing.T) { - assert.False(t, matchesPath([]string{"*.txt", "*.go"}, "readme.md")) -} - -// --- isNoFollow tests --- - -func TestIsNoFollow_NoPatterns(t *testing.T) { - w := &walker{noFollow: nil} - info := fakeFileInfo{name: "link.md", mode: os.ModeSymlink} - assert.False(t, w.isNoFollow("link.md", info)) -} - -func TestIsNoFollow_NotSymlink(t *testing.T) { - w := &walker{noFollow: []string{"*.md"}} - info := fakeFileInfo{name: "file.md", mode: 0} - assert.False(t, w.isNoFollow("file.md", info)) -} - -func TestIsNoFollow_SymlinkMatchesPattern(t *testing.T) { - w := &walker{noFollow: []string{"*.md"}} - info := fakeFileInfo{name: "link.md", mode: os.ModeSymlink} - assert.True(t, w.isNoFollow("link.md", info)) -} - -func TestIsNoFollow_SymlinkNoMatch(t *testing.T) { - w := &walker{noFollow: []string{"*.txt"}} - info := fakeFileInfo{name: "link.md", mode: os.ModeSymlink} - assert.False(t, w.isNoFollow("link.md", info)) -} - // --- visit tests --- func TestVisit_WalkError(t *testing.T) { @@ -87,7 +22,10 @@ func TestVisit_WalkError(t *testing.T) { assert.ErrorIs(t, err, os.ErrPermission) } -func TestVisit_SkipsDirWhenNoFollow(t *testing.T) { +// TestVisit_SkipsSymlinkDirByDefault confirms the walker returns +// SkipDir for a symlinked directory entry when FollowSymlinks is +// false (the secure default), without needing a pattern. +func TestVisit_SkipsSymlinkDirByDefault(t *testing.T) { dir := t.TempDir() absBase := dir @@ -97,18 +35,35 @@ func TestVisit_SkipsDirWhenNoFollow(t *testing.T) { w := &walker{ absBase: absBase, patterns: []string{"**/*.md"}, - noFollow: []string{"linked"}, seen: make(map[string]bool), } - // Synthetic: real filepath.Walk reports ModeSymlink without IsDir for - // symlinks, but we test the branch guard directly to confirm SkipDir - // is returned when the walker sees a symlinked directory entry. info := fakeFileInfo{name: "linked", mode: os.ModeDir | os.ModeSymlink, isDir: true} err := w.visit(filepath.Join(absBase, "linked"), info, nil) assert.Equal(t, filepath.SkipDir, err) } +// TestVisit_FollowsSymlinkDirWhenOptedIn asserts the symlinked +// directory is NOT skipped when the walker has FollowSymlinks=true. +func TestVisit_FollowsSymlinkDirWhenOptedIn(t *testing.T) { + dir := t.TempDir() + absBase := dir + + subDir := filepath.Join(dir, "linked") + require.NoError(t, os.MkdirAll(subDir, 0o755)) + + w := &walker{ + absBase: absBase, + patterns: []string{"**/*.md"}, + followSymlinks: true, + seen: make(map[string]bool), + } + + info := fakeFileInfo{name: "linked", mode: os.ModeDir | os.ModeSymlink, isDir: true} + err := w.visit(filepath.Join(absBase, "linked"), info, nil) + assert.NoError(t, err) +} + func TestVisit_SkipsFileWhenGitignored(t *testing.T) { dir := t.TempDir() writeFile(t, dir, ".gitignore", "ignored.md\n") @@ -180,7 +135,9 @@ func TestDiscover_DefaultBaseDir(t *testing.T) { require.NoError(t, err) } -func TestDiscover_SymlinkToFile(t *testing.T) { +// TestDiscover_SymlinkToFile_SkippedByDefault asserts the secure +// default: a symlinked file is not discovered. +func TestDiscover_SymlinkToFile_SkippedByDefault(t *testing.T) { dir := t.TempDir() writeFile(t, dir, "real.md", "# Real\n") @@ -193,11 +150,13 @@ func TestDiscover_SymlinkToFile(t *testing.T) { BaseDir: dir, }) require.NoError(t, err) - // Both real.md and link.md should be found (separate abs paths). - require.Len(t, files, 2) + require.Len(t, files, 1, "symlink must be skipped by default") + assert.Equal(t, "real.md", filepath.Base(files[0])) } -func TestDiscover_NoFollowSymlinksSkipsSymlinkedFile(t *testing.T) { +// TestDiscover_FollowSymlinks_OptIn asserts that FollowSymlinks=true +// surfaces the symlink as a distinct discovery result. +func TestDiscover_FollowSymlinks_OptIn(t *testing.T) { dir := t.TempDir() writeFile(t, dir, "real.md", "# Real\n") @@ -205,19 +164,13 @@ func TestDiscover_NoFollowSymlinksSkipsSymlinkedFile(t *testing.T) { realPath := filepath.Join(dir, "real.md") require.NoError(t, os.Symlink(realPath, linkPath)) - // filepath.Walk uses os.Lstat, so it reports symlink entries with - // ModeSymlink and does not follow them during traversal. The walker's - // isNoFollow check can therefore match NoFollowSymlinks patterns and - // skip the symlink entry itself. files, err := Discover(Options{ - Patterns: []string{"**/*.md"}, - BaseDir: dir, - NoFollowSymlinks: []string{"link.md"}, + Patterns: []string{"**/*.md"}, + BaseDir: dir, + FollowSymlinks: true, }) require.NoError(t, err) - // NoFollowSymlinks skips the symlinked file, only real.md remains. - assert.Len(t, files, 1, "expected only real.md (link.md skipped)") - assert.Contains(t, files[0], "real.md") + require.Len(t, files, 2, "both real and linked entries are discovered") } // fakeFileInfo implements os.FileInfo for testing. diff --git a/internal/lint/files.go b/internal/lint/files.go index d6f0875bc..368abc001 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -6,8 +6,6 @@ import ( "path/filepath" "sort" "strings" - - "github.com/gobwas/glob" ) // isMarkdown returns true if the file extension is .md or .markdown. @@ -16,21 +14,6 @@ func isMarkdown(path string) bool { return ext == ".md" || ext == ".markdown" } -// matchesGlob returns true if path matches any of the given glob patterns. -func matchesGlob(patterns []string, path string) bool { - cleanPath := filepath.Clean(path) - for _, pattern := range patterns { - g, err := glob.Compile(pattern) - if err != nil { - continue - } - if g.Match(path) || g.Match(cleanPath) || g.Match(filepath.Base(path)) { - return true - } - } - return false -} - // hasGlobChars returns true if the string contains glob meta-characters. func hasGlobChars(s string) bool { return strings.ContainsAny(s, "*?[") @@ -45,10 +28,10 @@ type ResolveOpts struct { // used (see DefaultResolveOpts). UseGitignore *bool - // NoFollowSymlinks is a list of glob patterns. Symbolic links - // whose path matches any pattern are skipped during directory - // walking and glob expansion. - NoFollowSymlinks []string + // FollowSymlinks opts in to following symbolic links during + // directory walks and glob expansion. The zero value skips all + // symlinks, which is the secure default. + FollowSymlinks bool } // DefaultResolveOpts returns options with defaults applied. @@ -129,13 +112,11 @@ func resolveGlob(pattern string, opts ResolveOpts, addFile func(string)) error { return fmt.Errorf("invalid glob pattern %q: %w", pattern, err) } for _, m := range matches { - // Skip symlinks matching no-follow-symlinks patterns. - if len(opts.NoFollowSymlinks) > 0 { - linfo, lerr := os.Lstat(m) - if lerr == nil && linfo.Mode()&os.ModeSymlink != 0 { - if matchesGlob(opts.NoFollowSymlinks, m) { - continue - } + // Default-deny symlinks unless the caller opts in. + if !opts.FollowSymlinks { + if linfo, lerr := os.Lstat(m); lerr == nil && + linfo.Mode()&os.ModeSymlink != 0 { + continue } } info, err := os.Stat(m) @@ -155,7 +136,7 @@ func resolveGlob(pattern string, opts ResolveOpts, addFile func(string)) error { // addDirFiles walks a directory and adds all markdown files found. func addDirFiles(dir string, opts ResolveOpts, addFile func(string)) error { - dirFiles, err := walkDir(dir, opts.useGitignore(), opts.NoFollowSymlinks) + dirFiles, err := walkDir(dir, opts.useGitignore(), opts.FollowSymlinks) if err != nil { return err } @@ -165,22 +146,10 @@ func addDirFiles(dir string, opts ResolveOpts, addFile func(string)) error { return nil } -// isSkippedSymlink reports whether path should be skipped because it is -// a symlink matching one of the no-follow-symlinks patterns. -func isSkippedSymlink(info os.FileInfo, path string, patterns []string) bool { - if len(patterns) == 0 { - return false - } - if info.Mode()&os.ModeSymlink == 0 { - return false - } - return matchesGlob(patterns, path) -} - // walkDir recursively walks a directory and returns all markdown files. // When useGitignore is true, files matched by .gitignore patterns are skipped. -// Symlinks whose path matches a noFollowSymlinks pattern are skipped. -func walkDir(dir string, useGitignore bool, noFollowSymlinks []string) ([]string, error) { +// Symlinks are skipped unless followSymlinks is true. +func walkDir(dir string, useGitignore, followSymlinks bool) ([]string, error) { var matcher *GitignoreMatcher if useGitignore { matcher = NewGitignoreMatcher(dir) @@ -192,7 +161,7 @@ func walkDir(dir string, useGitignore bool, noFollowSymlinks []string) ([]string return err } - if isSkippedSymlink(info, path, noFollowSymlinks) { + if !followSymlinks && info.Mode()&os.ModeSymlink != 0 { if info.IsDir() { return filepath.SkipDir } diff --git a/internal/lint/files_test.go b/internal/lint/files_test.go index bf686df3f..6b0412fa9 100644 --- a/internal/lint/files_test.go +++ b/internal/lint/files_test.go @@ -275,9 +275,12 @@ func TestResolveFilesWithOpts_GitignoreNegation(t *testing.T) { assert.Equal(t, "keep.md", filepath.Base(files[0])) } -// --- NoFollowSymlinks tests --- +// --- FollowSymlinks tests --- -func TestResolveFilesWithOpts_NoFollowSymlinks(t *testing.T) { +// TestResolveFilesWithOpts_SkipsSymlinksByDefault asserts that the +// secure default (FollowSymlinks=false) skips all symlinked files and +// symlinked directories during directory walks. +func TestResolveFilesWithOpts_SkipsSymlinksByDefault(t *testing.T) { dir := t.TempDir() realFile := filepath.Join(dir, "real.md") @@ -291,44 +294,63 @@ func TestResolveFilesWithOpts_NoFollowSymlinks(t *testing.T) { linkFile := filepath.Join(dir, "link.md") require.NoError(t, os.Symlink(targetFile, linkFile)) - // Without no-follow-symlinks: all files should be found. + // Default: only the real files are walked; symlink is skipped. files, err := ResolveFilesWithOpts([]string{dir}, DefaultResolveOpts()) require.NoError(t, err) - require.Len(t, files, 3) // real.md, link.md, target/doc.md - - // With no-follow-symlinks matching all .md: symlink should be skipped. - noGitignore := false - opts := ResolveOpts{ - UseGitignore: &noGitignore, - NoFollowSymlinks: []string{"*.md"}, - } - files, err = ResolveFilesWithOpts([]string{dir}, opts) - require.NoError(t, err) - require.Len(t, files, 2, "expected 2 files (symlink skipped)") + require.Len(t, files, 2) for _, f := range files { - assert.NotEqual(t, "link.md", filepath.Base(f), "link.md should have been skipped (symlink)") + assert.NotEqual(t, "link.md", filepath.Base(f), + "symlinked link.md must be skipped by default") } } -func TestResolveFilesWithOpts_NoFollowSymlinks_PatternSpecific(t *testing.T) { +// TestResolveFilesWithOpts_FollowSymlinks_OptIn asserts that +// FollowSymlinks=true restores the pre-plan-84 behavior of walking +// symlinked entries. +func TestResolveFilesWithOpts_FollowSymlinks_OptIn(t *testing.T) { dir := t.TempDir() - targetA := filepath.Join(dir, "a.md") - targetB := filepath.Join(dir, "b.md") - require.NoError(t, os.WriteFile(targetA, []byte("# A"), 0o644)) - require.NoError(t, os.WriteFile(targetB, []byte("# B"), 0o644)) + realFile := filepath.Join(dir, "real.md") + require.NoError(t, os.WriteFile(realFile, []byte("# Real"), 0o644)) - linkDir := filepath.Join(dir, "links") - require.NoError(t, os.MkdirAll(linkDir, 0o755)) - require.NoError(t, os.Symlink(targetA, filepath.Join(linkDir, "link-a.md"))) - require.NoError(t, os.Symlink(targetB, filepath.Join(linkDir, "link-b.md"))) + subDir := filepath.Join(dir, "target") + require.NoError(t, os.MkdirAll(subDir, 0o755)) + targetFile := filepath.Join(subDir, "doc.md") + require.NoError(t, os.WriteFile(targetFile, []byte("# Target"), 0o644)) + + linkFile := filepath.Join(dir, "link.md") + require.NoError(t, os.Symlink(targetFile, linkFile)) noGitignore := false opts := ResolveOpts{ - UseGitignore: &noGitignore, - NoFollowSymlinks: []string{"**/links/*"}, + UseGitignore: &noGitignore, + FollowSymlinks: true, } files, err := ResolveFilesWithOpts([]string{dir}, opts) require.NoError(t, err) - require.Len(t, files, 2) + require.Len(t, files, 3, + "with FollowSymlinks=true, symlink is walked alongside real files") +} + +// TestResolveFilesWithOpts_Glob_SkipsSymlinksByDefault asserts the +// same default-deny applies when paths come in through a glob +// expansion (resolveGlob path) rather than a directory walk. +func TestResolveFilesWithOpts_Glob_SkipsSymlinksByDefault(t *testing.T) { + dir := t.TempDir() + + realFile := filepath.Join(dir, "real.md") + require.NoError(t, os.WriteFile(realFile, []byte("# Real"), 0o644)) + + target := filepath.Join(dir, "target.md") + require.NoError(t, os.WriteFile(target, []byte("# Target"), 0o644)) + linkFile := filepath.Join(dir, "link.md") + require.NoError(t, os.Symlink(target, linkFile)) + + pattern := filepath.Join(dir, "*.md") + files, err := ResolveFilesWithOpts([]string{pattern}, DefaultResolveOpts()) + require.NoError(t, err) + for _, f := range files { + assert.NotEqual(t, "link.md", filepath.Base(f), + "glob expansion must skip symlinks by default") + } } diff --git a/internal/lint/lint_coverage_test.go b/internal/lint/lint_coverage_test.go index d69fc4407..7020278c5 100644 --- a/internal/lint/lint_coverage_test.go +++ b/internal/lint/lint_coverage_test.go @@ -48,36 +48,6 @@ func TestGetGitignore_Cached(t *testing.T) { assert.Equal(t, 1, called, "GitignoreFunc should be called only once") } -// --- matchesGlob tests --- - -func TestMatchesGlob_ExactMatch(t *testing.T) { - assert.True(t, matchesGlob([]string{"readme.md"}, "readme.md")) -} - -func TestMatchesGlob_WildcardMatch(t *testing.T) { - assert.True(t, matchesGlob([]string{"*.md"}, "readme.md")) -} - -func TestMatchesGlob_NoMatch(t *testing.T) { - assert.False(t, matchesGlob([]string{"*.txt"}, "readme.md")) -} - -func TestMatchesGlob_EmptyPatterns(t *testing.T) { - assert.False(t, matchesGlob([]string{}, "readme.md")) -} - -func TestMatchesGlob_InvalidPattern(t *testing.T) { - assert.False(t, matchesGlob([]string{"[invalid"}, "readme.md")) -} - -func TestMatchesGlob_MatchesBasename(t *testing.T) { - assert.True(t, matchesGlob([]string{"readme.md"}, "/some/path/readme.md")) -} - -func TestMatchesGlob_MatchesCleanedPath(t *testing.T) { - assert.True(t, matchesGlob([]string{"foo/bar.md"}, "foo//bar.md")) -} - // --- useGitignore tests --- func TestUseGitignore_NilPointer(t *testing.T) { @@ -505,7 +475,7 @@ func TestExtractPINameBytes_EmptyAfterPI(t *testing.T) { // --- Additional walk coverage --- func TestWalkDir_NonexistentDir(t *testing.T) { - _, err := walkDir(filepath.Join(t.TempDir(), "no-such-dir"), false, nil) + _, err := walkDir(filepath.Join(t.TempDir(), "no-such-dir"), false, false) assert.Error(t, err) } @@ -529,26 +499,6 @@ func TestIsMarkdown(t *testing.T) { assert.False(t, isMarkdown("file.go")) } -// --- isSkippedSymlink tests --- - -func TestIsSkippedSymlink_NotSymlink(t *testing.T) { - dir := t.TempDir() - f := filepath.Join(dir, "real.md") - require.NoError(t, os.WriteFile(f, []byte("# Test"), 0o644)) - info, err := os.Stat(f) - require.NoError(t, err) - assert.False(t, isSkippedSymlink(info, f, []string{"*.md"})) -} - -func TestIsSkippedSymlink_EmptyPatterns(t *testing.T) { - dir := t.TempDir() - f := filepath.Join(dir, "real.md") - require.NoError(t, os.WriteFile(f, []byte("# Test"), 0o644)) - info, err := os.Stat(f) - require.NoError(t, err) - assert.False(t, isSkippedSymlink(info, f, nil)) -} - // --- matchRule edge cases --- func TestMatchRule_OutsideBase(t *testing.T) { From b79dee05300b5b4452cd9cb17aa5bc218c145d22 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 13:27:41 +0000 Subject: [PATCH 04/28] =?UTF-8?q?plan=2084:=20close=20=E2=80=94=20docs,=20?= =?UTF-8?q?plan=20AC,=20security=20note?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - docs/reference/cli.md: document --follow-symlinks opt-in and the deprecation status of --no-follow-symlinks. - docs/security/2026-04-05-adversarial-markdown.md: annotate the symlink finding with the plan 84 mitigation. - plan 84 AC checked off; task list notes the concrete landing. - PLAN.md catalog regenerated. --- PLAN.md | 2 +- docs/reference/cli.md | 35 +++++++++----- .../2026-04-05-adversarial-markdown.md | 2 + plan/84_symlink-default-deny.md | 48 +++++++++++-------- 4 files changed, 55 insertions(+), 32 deletions(-) diff --git a/PLAN.md b/PLAN.md index a93587290..dfdf9399d 100644 --- a/PLAN.md +++ b/PLAN.md @@ -21,7 +21,7 @@ footer: | | 65 | 🔲 | [Spike WASM-Embedded Weasel Inference](plan/65_spike-wasm-embedded-inference.md) | | 78 | ✅ | [Query subcommand for front-matter filtering](plan/78_query-command.md) | | 83 | ✅ | [Security hardening batch](plan/83_security-hardening-batch.md) | -| 84 | 🔳 | [Symlink default-deny for file discovery](plan/84_symlink-default-deny.md) | +| 84 | ✅ | [Symlink default-deny for file discovery](plan/84_symlink-default-deny.md) | | 85 | 🔳 | [Increase test coverage to 95% by extracting shared rule helpers](plan/85_coverage-to-95-percent.md) | | 86 | 🔳 | [Markdown flavor validation](plan/86_markdown-flavor-validation.md) | | 89 | 🔲 | [TOC generator directive and MDS035 auto-fix](plan/89_toc-generator-directive.md) | diff --git a/docs/reference/cli.md b/docs/reference/cli.md index e99d51d47..2dad7d92a 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -34,16 +34,29 @@ match, exits 0. ## Subcommand Flags (check, fix) -| Flag | Default | Description | -|------------------------|---------|-----------------------------------------| -| `-c`, `--config` | auto | Config path (auto-discovers by default) | -| `-f`, `--format` | `text` | `text` or `json` | -| `--max-input-size` | `2MB` | Max file size (e.g. `2MB`, `0`=none) | -| `--no-color` | false | Plain output | -| `--no-follow-symlinks` | false | Skip symbolic links when walking | -| `--no-gitignore` | false | Skip gitignore | -| `-q`, `--quiet` | false | Quiet mode | -| `-v`, `--verbose` | false | Verbose output | +| Flag | Default | Description | +|---------------------|---------|-----------------------------------------| +| `-c`, `--config` | auto | Config path (auto-discovers by default) | +| `-f`, `--format` | `text` | `text` or `json` | +| `--max-input-size` | `2MB` | Max file size (e.g. `2MB`, `0`=none) | +| `--no-color` | false | Plain output | +| `--follow-symlinks` | false | Follow symlinks (default: skip) | +| `--no-gitignore` | false | Skip gitignore | +| `-q`, `--quiet` | false | Quiet mode | +| `-v`, `--verbose` | false | Verbose output | + +Symlinks are skipped by default. This blocks a malicious +symlink from redirecting `check` or `fix` to files outside +the project. Both directory walks and glob expansion apply +the rule. + +Pass `--follow-symlinks` to opt in on the command line. +Set `follow-symlinks: true` in `.mdsmith.yml` to opt in +from config. + +The old `--no-follow-symlinks` flag parses silently. +The old `no-follow-symlinks:` config key parses too, and +emits a deprecation warning on stderr. ## Other Subcommand Flags @@ -55,7 +68,7 @@ and `--scope` (only `file` is supported; defaults to `file`). `metrics rank` accepts `-c`/`--config`, `-f`/`--format`, -`--no-gitignore`, `--no-follow-symlinks`, +`--no-gitignore`, `--follow-symlinks`, `--max-input-size`, plus `--metrics`, `--by`, `--order`, `--top`. diff --git a/docs/security/2026-04-05-adversarial-markdown.md b/docs/security/2026-04-05-adversarial-markdown.md index 0705d01b9..e20edf016 100644 --- a/docs/security/2026-04-05-adversarial-markdown.md +++ b/docs/security/2026-04-05-adversarial-markdown.md @@ -115,6 +115,8 @@ Escape sequences pass through to the terminal: screen clearing (`\033[2J`), wind Symlink following is the default behavior. The `--no-follow-symlinks` flag and config exist but are opt-in. +**Status (plan 84):** the default is now inverted. Symlinks are skipped by default across directory walks and glob expansion; users opt in with `--follow-symlinks` or `follow-symlinks: true`. The legacy `--no-follow-symlinks` flag and `no-follow-symlinks:` config key are silently accepted for one release, with a deprecation warning emitted for the config key. + **Adversarial file:** ```bash diff --git a/plan/84_symlink-default-deny.md b/plan/84_symlink-default-deny.md index ada54fe3c..740d4f721 100644 --- a/plan/84_symlink-default-deny.md +++ b/plan/84_symlink-default-deny.md @@ -1,7 +1,7 @@ --- id: 84 title: 'Symlink default-deny for file discovery' -status: "🔳" +status: "✅" summary: >- Skip symlinks by default during directory walks; add --follow-symlinks opt-in flag. @@ -83,29 +83,37 @@ Consider migrating from `filepath.Walk` to ## Tasks -1. Replace `NoFollowSymlinks []string` with - `FollowSymlinks bool` in `config.Config` -2. Replace `--no-follow-symlinks` with - `--follow-symlinks` in CLI flag sets -3. Update `ResolveOpts` to use `FollowSymlinks bool` -4. Update `walkDir` to skip symlinks by default -5. Update `resolveGlob` to skip symlinks by default -6. Add deprecation warning for old config key -7. Update tests in `files_test.go` -8. Add integration test: symlink to file outside - project is skipped by default, followed with - `--follow-symlinks` +1. [x] Replaced `NoFollowSymlinks []string` with + `FollowSymlinks bool` in `config.Config`; kept + `LegacyNoFollowSymlinks` for deprecation parsing +2. [x] Replaced `--no-follow-symlinks` with + `--follow-symlinks`; old flag silently accepted +3. [x] Updated `ResolveOpts` to use `FollowSymlinks bool` +4. [x] Updated `walkDir` to skip symlinks by default +5. [x] Updated `resolveGlob` to skip symlinks by default +6. [x] Added deprecation warning for old config key + (emitted by `cmd/mdsmith.loadConfig` once per run) +7. [x] Updated `files_test.go` and `lint_coverage_test.go` +8. [x] Added integration tests in + `cmd/mdsmith/e2e_symlink_default_deny_test.go`: + external-target symlink skipped by default, + followed with `--follow-symlinks`, config-key + opt-in, legacy-config deprecation warning, and + fix TOCTOU behavior (symlink replaced, target + untouched) ## Acceptance Criteria -- [ ] Symlinks are skipped by default in directory +- [x] Symlinks are skipped by default in directory walks -- [ ] `--follow-symlinks` flag enables symlink +- [x] `--follow-symlinks` flag enables symlink following -- [ ] `follow-symlinks: true` in config enables +- [x] `follow-symlinks: true` in config enables symlink following -- [ ] Old `no-follow-symlinks` config emits +- [x] Old `no-follow-symlinks` config emits deprecation warning -- [ ] Both `check` and `fix` respect the setting -- [ ] All tests pass: `go test ./...` -- [ ] `go tool golangci-lint run` reports no issues +- [x] Both `check` and `fix` respect the setting +- [x] All tests pass: `go test ./...` (except + pre-existing `internal/corpus` failures + tracked by plan 90) +- [x] `go tool golangci-lint run` reports no issues From 9564b18c704b2990f346b432ae94f927f8ac26a9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 16:44:50 +0000 Subject: [PATCH 05/28] =?UTF-8?q?plan=2084:=20address=20Copilot=20review?= =?UTF-8?q?=20=E2=80=94=20cover=20explicit-arg=20symlinks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - internal/lint/files.go: resolveArg now Lstats non-glob paths and skips symlinks unless FollowSymlinks is set. Without this, `mdsmith check ./evil.md` bypassed the default-deny policy and still processed the symlink target. - cmd/mdsmith/e2e_symlink_default_deny_test.go: new test TestE2E_Symlink_DefaultDeny_ExplicitFileArg exercises both the default-deny and --follow-symlinks opt-in on an explicit path. - docs/reference/cli.md: note covers explicit file arguments, not just walks and globs. - docs/security/2026-04-05-adversarial-markdown.md: reword the original-finding paragraph so it no longer contradicts the plan 84 status line below. - internal/config/config.go: clarify that the `omitempty` tag on LegacyNoFollowSymlinks keeps round-tripped configs from re-emitting the deprecated key unless the user supplied it. --- cmd/mdsmith/e2e_symlink_default_deny_test.go | 31 +++++++++++++++++++ docs/reference/cli.md | 6 ++-- .../2026-04-05-adversarial-markdown.md | 4 +-- internal/config/config.go | 5 +-- internal/lint/files.go | 12 +++++++ 5 files changed, 52 insertions(+), 6 deletions(-) diff --git a/cmd/mdsmith/e2e_symlink_default_deny_test.go b/cmd/mdsmith/e2e_symlink_default_deny_test.go index 67c287976..570b075a7 100644 --- a/cmd/mdsmith/e2e_symlink_default_deny_test.go +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -42,6 +42,37 @@ func TestE2E_Symlink_DefaultDeny_ExternalTargetSkipped(t *testing.T) { exitCode, stderr) } +// TestE2E_Symlink_DefaultDeny_ExplicitFileArg asserts that the +// secure default also covers explicit, non-glob path arguments. +// `mdsmith check ./evil.md` must not process a symlink target. +func TestE2E_Symlink_DefaultDeny_ExplicitFileArg(t *testing.T) { + project := t.TempDir() + external := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "rules:\n no-trailing-spaces: true\n") + + externalFile := filepath.Join(external, "dirty.md") + require.NoError(t, os.WriteFile(externalFile, + []byte("# Evil\n\ntrailing \n"), 0o644)) + require.NoError(t, os.Symlink(externalFile, + filepath.Join(project, "evil.md"))) + + // Default-deny: explicit symlinked file arg is silently skipped; + // no diagnostics reported, exit 0. + _, _, exitCode := runBinaryInDir(t, project, "", "check", + "--no-color", "--no-gitignore", "evil.md") + assert.Equal(t, 0, exitCode, + "explicit symlinked file arg must be skipped by default") + + // Opt-in: the symlinked file is visited and flagged. + _, _, exitOpt := runBinaryInDir(t, project, "", "check", + "--no-color", "--no-gitignore", "--follow-symlinks", "evil.md") + assert.Equal(t, 1, exitOpt, + "with --follow-symlinks the symlink target is linted") +} + // TestE2E_Symlink_FollowSymlinksFlag_OptsIn asserts the new // --follow-symlinks CLI flag walks symlinked entries. func TestE2E_Symlink_FollowSymlinksFlag_OptsIn(t *testing.T) { diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 2dad7d92a..dbdb2184c 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -47,8 +47,10 @@ match, exits 0. Symlinks are skipped by default. This blocks a malicious symlink from redirecting `check` or `fix` to files outside -the project. Both directory walks and glob expansion apply -the rule. +the project. The rule applies to directory walks, glob +expansion, and explicit file or directory arguments: +`mdsmith check ./linked.md` silently skips a symlink named +on the command line. Pass `--follow-symlinks` to opt in on the command line. Set `follow-symlinks: true` in `.mdsmith.yml` to opt in diff --git a/docs/security/2026-04-05-adversarial-markdown.md b/docs/security/2026-04-05-adversarial-markdown.md index e20edf016..cedba6aa2 100644 --- a/docs/security/2026-04-05-adversarial-markdown.md +++ b/docs/security/2026-04-05-adversarial-markdown.md @@ -113,9 +113,9 @@ Escape sequences pass through to the terminal: screen clearing (`\033[2J`), wind - `internal/lint/files.go:190` — `filepath.Walk` follows file symlinks - `internal/fix/fix.go:84-122` — `os.WriteFile` follows symlinks on write -Symlink following is the default behavior. The `--no-follow-symlinks` flag and config exist but are opt-in. +**Original finding:** symlink following was the default; the `--no-follow-symlinks` flag and config key were opt-in. -**Status (plan 84):** the default is now inverted. Symlinks are skipped by default across directory walks and glob expansion; users opt in with `--follow-symlinks` or `follow-symlinks: true`. The legacy `--no-follow-symlinks` flag and `no-follow-symlinks:` config key are silently accepted for one release, with a deprecation warning emitted for the config key. +**Status (plan 84, resolved):** the default is inverted. Symlinks are skipped by default across directory walks, glob expansion, and explicit non-glob path arguments. Users opt in with `--follow-symlinks` or `follow-symlinks: true`. The legacy `--no-follow-symlinks` flag and `no-follow-symlinks:` config key are silently accepted for one release, with a deprecation warning emitted for the config key. **Adversarial file:** diff --git a/internal/config/config.go b/internal/config/config.go index 9aa1d519d..ff041a2c7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -37,8 +37,9 @@ type Config struct { // LegacyNoFollowSymlinks captures the removed `no-follow-symlinks` // key. Its presence surfaces a deprecation warning via // Deprecations; its contents are otherwise ignored now that - // symlinks are skipped by default. - // Not serialized to YAML in marshalled output. + // symlinks are skipped by default. The `omitempty` tag keeps + // round-tripped configs from re-emitting the deprecated key + // unless a user explicitly supplied it. LegacyNoFollowSymlinks []string `yaml:"no-follow-symlinks,omitempty"` // ExplicitRules tracks rule names that were explicitly set in diff --git a/internal/lint/files.go b/internal/lint/files.go index 368abc001..20e3a530d 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -91,6 +91,18 @@ func resolveArg(arg string, opts ResolveOpts, addFile func(string)) error { return resolveGlob(arg, opts, addFile) } + // Default-deny symlinks even for explicit, non-glob paths. Lstat + // (not Stat) avoids following the link so that an attacker who + // plants `evil.md -> /etc/cron.d/jobs` can't trick a user into + // processing the target with `mdsmith check ./evil.md`. + linfo, lerr := os.Lstat(arg) + if lerr != nil { + return fmt.Errorf("cannot access %q: %w", arg, lerr) + } + if !opts.FollowSymlinks && linfo.Mode()&os.ModeSymlink != 0 { + return nil + } + info, err := os.Stat(arg) if err != nil { return fmt.Errorf("cannot access %q: %w", arg, err) From 4b925d02458a06851942141f3fd7191fa639bbf8 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 17:59:49 +0000 Subject: [PATCH 06/28] plan 84: address second Copilot review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - internal/config/load.go: parse YAML only twice instead of four times by extracting topLevelKeySet(data). The previous code called yamlHasKey twice, each time re-running RejectYAMLAliases and yaml.Unmarshal over the same bytes. yamlHasKey now wraps topLevelKeySet for the existing test. - cmd/mdsmith: the deprecated --no-follow-symlinks flag is no longer silent — it forces FollowSymlinks off even when `follow-symlinks: true` is set in config, giving users a way to run securely without editing the config. Precedence (highest wins): --no-follow-symlinks, then --follow-symlinks, then the config key. - cmd/mdsmith: bundle the three walk-related CLI flags into a walkCLI struct so helpers thread one value instead of growing a fourth bool parameter. - cmd/mdsmith/e2e_symlink_default_deny_test.go: new test TestE2E_Symlink_LegacyFlag_OverridesFollowConfig pins the force-deny semantics against a config that enables follow. --- cmd/mdsmith/e2e_symlink_default_deny_test.go | 33 +++++++ cmd/mdsmith/main.go | 99 ++++++++++++-------- cmd/mdsmith/metrics.go | 10 +- internal/config/load.go | 35 ++++--- 4 files changed, 122 insertions(+), 55 deletions(-) diff --git a/cmd/mdsmith/e2e_symlink_default_deny_test.go b/cmd/mdsmith/e2e_symlink_default_deny_test.go index 570b075a7..dcf971623 100644 --- a/cmd/mdsmith/e2e_symlink_default_deny_test.go +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -118,6 +118,39 @@ func TestE2E_Symlink_FollowSymlinksConfigKey_OptsIn(t *testing.T) { "expected exit 1 with follow-symlinks: true exposing dirty linked file") } +// TestE2E_Symlink_LegacyFlag_OverridesFollowConfig verifies that the +// deprecated --no-follow-symlinks flag still has meaning: it forces +// deny even when `follow-symlinks: true` is set in config. This lets +// users run one-off secure invocations without editing the config. +func TestE2E_Symlink_LegacyFlag_OverridesFollowConfig(t *testing.T) { + project := t.TempDir() + external := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "follow-symlinks: true\nrules:\n no-trailing-spaces: true\n") + + externalFile := filepath.Join(external, "dirty.md") + require.NoError(t, os.WriteFile(externalFile, + []byte("# Evil\n\ntrailing \n"), 0o644)) + require.NoError(t, os.Symlink(externalFile, + filepath.Join(project, "linked.md"))) + + // Baseline: with follow-symlinks:true in config, the linked + // file is walked and the dirty line surfaces. + _, _, withConfig := runBinaryInDir(t, project, "", "check", + "--no-color", "--no-gitignore", ".") + require.Equal(t, 1, withConfig, + "follow-symlinks: true config must expose the linked file") + + // Legacy --no-follow-symlinks overrides the config and forces + // deny for this invocation; no findings. + _, _, withLegacy := runBinaryInDir(t, project, "", "check", + "--no-color", "--no-gitignore", "--no-follow-symlinks", ".") + assert.Equal(t, 0, withLegacy, + "legacy --no-follow-symlinks must force deny over config opt-in") +} + // TestE2E_Symlink_LegacyNoFollowConfig_Deprecation verifies that the // old `no-follow-symlinks:` key still parses and emits a deprecation // warning on stderr. diff --git a/cmd/mdsmith/main.go b/cmd/mdsmith/main.go index 8cbf060a0..35e5e7616 100644 --- a/cmd/mdsmith/main.go +++ b/cmd/mdsmith/main.go @@ -171,7 +171,7 @@ func runCheck(args []string) int { fs.BoolVarP(&verbose, "verbose", "v", false, "Show config, files, and rules on stderr") fs.BoolVar(&noGitignore, "no-gitignore", false, "Disable .gitignore filtering when walking directories") fs.BoolVar(&followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") - registerLegacyNoFollowSymlinks(fs) + legacyNoFollow := registerLegacyNoFollowSymlinks(fs) fs.StringVar(&maxInputSize, "max-input-size", "", "Maximum file size to process (e.g. 2MB, 500KB, 0=unlimited)") fs.Usage = func() { @@ -193,6 +193,12 @@ func runCheck(args []string) int { verbose = false } + walk := walkCLI{ + noGitignore: noGitignore, + followSymlinks: followSymlinks, + legacyNoFollow: *legacyNoFollow, + } + allArgs := fs.Args() // Check for explicit stdin argument "-". @@ -203,17 +209,11 @@ func runCheck(args []string) int { } if len(fileArgs) > 0 { - return checkFiles( - fileArgs, configPath, format, noColor, quiet, verbose, - noGitignore, followSymlinks, maxInputSize, - ) + return checkFiles(fileArgs, configPath, format, noColor, quiet, verbose, walk, maxInputSize) } // No file args and no stdin: discover files from config. - return checkDiscovered( - configPath, format, noColor, quiet, verbose, - noGitignore, followSymlinks, maxInputSize, - ) + return checkDiscovered(configPath, format, noColor, quiet, verbose, walk, maxInputSize) } // runFix implements the "fix" subcommand: auto-fix lint issues in place. @@ -237,7 +237,7 @@ func runFix(args []string) int { fs.BoolVarP(&verbose, "verbose", "v", false, "Show config, files, and rules on stderr") fs.BoolVar(&noGitignore, "no-gitignore", false, "Disable .gitignore filtering when walking directories") fs.BoolVar(&followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") - registerLegacyNoFollowSymlinks(fs) + legacyNoFollow := registerLegacyNoFollowSymlinks(fs) fs.StringVar(&maxInputSize, "max-input-size", "", "Maximum file size to process (e.g. 2MB, 500KB, 0=unlimited)") fs.Usage = func() { @@ -259,6 +259,12 @@ func runFix(args []string) int { verbose = false } + walk := walkCLI{ + noGitignore: noGitignore, + followSymlinks: followSymlinks, + legacyNoFollow: *legacyNoFollow, + } + allArgs := fs.Args() // Check for explicit stdin argument "-". @@ -270,17 +276,11 @@ func runFix(args []string) int { } if len(fileArgs) > 0 { - return fixFiles( - fileArgs, configPath, format, noColor, quiet, verbose, - noGitignore, followSymlinks, maxInputSize, - ) + return fixFiles(fileArgs, configPath, format, noColor, quiet, verbose, walk, maxInputSize) } // No file args: discover files from config. - return fixDiscovered( - configPath, format, noColor, quiet, verbose, - noGitignore, followSymlinks, maxInputSize, - ) + return fixDiscovered(configPath, format, noColor, quiet, verbose, walk, maxInputSize) } // runQuery implements the "query" subcommand: select files by CUE @@ -528,11 +528,11 @@ func printRunStats(format string, quiet bool, stats runStats) { // checkFiles lints the given file paths and returns the appropriate exit code. func checkFiles( fileArgs []string, configPath, format string, - noColor, quiet, verbose, noGitignore, followSymlinks bool, + noColor, quiet, verbose bool, walk walkCLI, maxInputSize string, ) int { cfg, cfgPath, logger, files, maxBytes, code := loadAndResolve( - fileArgs, configPath, verbose, noGitignore, followSymlinks, maxInputSize, + fileArgs, configPath, verbose, walk, maxInputSize, ) if code >= 0 { return code @@ -574,11 +574,11 @@ func checkFiles( // fixFiles fixes lint issues in the given file paths. func fixFiles( fileArgs []string, configPath, format string, - noColor, quiet, verbose, noGitignore, followSymlinks bool, + noColor, quiet, verbose bool, walk walkCLI, maxInputSize string, ) int { cfg, cfgPath, logger, files, maxBytes, code := loadAndResolve( - fileArgs, configPath, verbose, noGitignore, followSymlinks, maxInputSize, + fileArgs, configPath, verbose, walk, maxInputSize, ) if code >= 0 { return code @@ -700,7 +700,7 @@ func checkStdin(format string, noColor, quiet, verbose bool, configPath, maxInpu // or -1 on success. func loadAndResolve( fileArgs []string, configPath string, - verbose, noGitignore, followSymlinks bool, + verbose bool, walk walkCLI, maxInputSize string, ) (*config.Config, string, *vlog.Logger, []string, int64, int) { logger := &vlog.Logger{Enabled: verbose, W: os.Stderr} @@ -714,7 +714,7 @@ func loadAndResolve( logger.Printf("config: %s", cfgPath) } - opts := resolveOpts(cfg, noGitignore, followSymlinks) + opts := resolveOpts(cfg, walk) files, err := lint.ResolveFilesWithOpts(fileArgs, opts) if err != nil { fmt.Fprintf(os.Stderr, "mdsmith: %v\n", err) @@ -752,7 +752,7 @@ func splitStdinArg(args []string) (hasStdin bool, fileArgs []string) { // exit code; the caller should return it directly. A negative code means // "continue with the returned values". func discoverFiles( - configPath string, verbose, noGitignore, followSymlinks bool, + configPath string, verbose bool, walk walkCLI, ) (*config.Config, string, *vlog.Logger, []string, int) { logger := &vlog.Logger{Enabled: verbose, W: os.Stderr} @@ -770,8 +770,8 @@ func discoverFiles( files, err := discovery.Discover(discovery.Options{ Patterns: cfg.Files, - UseGitignore: !noGitignore, - FollowSymlinks: resolveOpts(cfg, noGitignore, followSymlinks).FollowSymlinks, + UseGitignore: !walk.noGitignore, + FollowSymlinks: resolveOpts(cfg, walk).FollowSymlinks, }) if err != nil { fmt.Fprintf(os.Stderr, "mdsmith: discovering files: %v\n", err) @@ -787,10 +787,10 @@ func discoverFiles( // and lints them. Returns the appropriate exit code. func checkDiscovered( configPath, format string, - noColor, quiet, verbose, noGitignore, followSymlinks bool, + noColor, quiet, verbose bool, walk walkCLI, maxInputSize string, ) int { - cfg, cfgPath, logger, files, code := discoverFiles(configPath, verbose, noGitignore, followSymlinks) + cfg, cfgPath, logger, files, code := discoverFiles(configPath, verbose, walk) if code >= 0 { return code } @@ -838,10 +838,10 @@ func checkDiscovered( // and fixes them. Returns the appropriate exit code. func fixDiscovered( configPath, format string, - noColor, quiet, verbose, noGitignore, followSymlinks bool, + noColor, quiet, verbose bool, walk walkCLI, maxInputSize string, ) int { - cfg, cfgPath, logger, files, code := discoverFiles(configPath, verbose, noGitignore, followSymlinks) + cfg, cfgPath, logger, files, code := discoverFiles(configPath, verbose, walk) if code >= 0 { return code } @@ -886,27 +886,46 @@ func fixDiscovered( } // registerLegacyNoFollowSymlinks registers the removed -// `--no-follow-symlinks` flag as a silently-accepted deprecation. -// Plan 84 inverted the default; the flag no longer has any effect. -func registerLegacyNoFollowSymlinks(fs *flag.FlagSet) { - _ = fs.Bool("no-follow-symlinks", false, +// `--no-follow-symlinks` flag as a silently-accepted deprecation +// and returns a pointer to its parsed value. When set, the caller +// should force `FollowSymlinks` off regardless of config / other +// flags, so existing invocations remain a safe way to get the +// secure default without editing the config file. +func registerLegacyNoFollowSymlinks(fs *flag.FlagSet) *bool { + return fs.Bool("no-follow-symlinks", false, "Deprecated: symlinks are now skipped by default") } +// walkCLI bundles the CLI flags that affect how files are +// discovered and resolved, so helpers can thread one value +// instead of three (and the next addition isn't a parameter +// explosion). +type walkCLI struct { + noGitignore bool + followSymlinks bool + legacyNoFollow bool +} + // frontMatterEnabled returns whether front matter stripping is enabled. // Defaults to true if not set in config. // resolveOpts builds ResolveOpts from config and CLI flags. -// CLI flags override config: --no-gitignore disables gitignore filtering, -// --follow-symlinks opts in to following symbolic links (default: skip). -func resolveOpts(cfg *config.Config, noGitignore, followSymlinks bool) lint.ResolveOpts { - useGitignore := !noGitignore +// CLI precedence for symlinks (highest wins): the deprecated +// --no-follow-symlinks flag forces deny, then --follow-symlinks +// opts in, then the `follow-symlinks:` config key. The deprecated +// flag lets users run in secure mode without editing an existing +// config that enables symlink following. +func resolveOpts(cfg *config.Config, walk walkCLI) lint.ResolveOpts { + useGitignore := !walk.noGitignore opts := lint.ResolveOpts{ UseGitignore: &useGitignore, FollowSymlinks: cfg.FollowSymlinks, } - if followSymlinks { + if walk.followSymlinks { opts.FollowSymlinks = true } + if walk.legacyNoFollow { + opts.FollowSymlinks = false + } return opts } diff --git a/cmd/mdsmith/metrics.go b/cmd/mdsmith/metrics.go index 3fe7aeb0b..e5aa3aa9c 100644 --- a/cmd/mdsmith/metrics.go +++ b/cmd/mdsmith/metrics.go @@ -100,6 +100,7 @@ type metricsRankOptions struct { format string noGitignore bool followSymlinks bool + legacyNoFollow bool maxInputSize string } @@ -124,7 +125,7 @@ func parseMetricsRankOptions(args []string) (metricsRankOptions, []string, error fs.StringVarP(&opts.format, "format", "f", "text", "Output format: text, json") fs.BoolVar(&opts.noGitignore, "no-gitignore", false, "Disable .gitignore filtering when walking directories") fs.BoolVar(&opts.followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") - registerLegacyNoFollowSymlinks(fs) + legacyNoFollow := registerLegacyNoFollowSymlinks(fs) fs.StringVar(&opts.maxInputSize, "max-input-size", "", "Maximum file size to process (e.g. 2MB, 500KB, 0=unlimited)") @@ -145,6 +146,7 @@ func parseMetricsRankOptions(args []string) (metricsRankOptions, []string, error if opts.top < 0 { return metricsRankOptions{}, nil, fmt.Errorf("--top must be >= 0") } + opts.legacyNoFollow = *legacyNoFollow fileArgs := fs.Args() if len(fileArgs) == 0 { @@ -244,7 +246,11 @@ func resolveRankSelection( } func resolveRankFiles(cfg *config.Config, opts metricsRankOptions, fileArgs []string) ([]string, error) { - resolveOptions := resolveOpts(cfg, opts.noGitignore, opts.followSymlinks) + resolveOptions := resolveOpts(cfg, walkCLI{ + noGitignore: opts.noGitignore, + followSymlinks: opts.followSymlinks, + legacyNoFollow: opts.legacyNoFollow, + }) return lint.ResolveFilesWithOpts(fileArgs, resolveOptions) } diff --git a/internal/config/load.go b/internal/config/load.go index c8c285fd2..efb8e12da 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -34,10 +34,13 @@ func Load(path string) (*Config, error) { return nil, fmt.Errorf("parsing config file: %w", err) } - // Detect whether the "files" key was explicitly present in the YAML. - cfg.FilesExplicit = yamlHasKey(data, "files") + // Detect top-level key presence with a single additional parse so + // "files" (omitted vs empty) and deprecated keys can be probed + // without re-parsing per key. + keys := topLevelKeySet(data) + cfg.FilesExplicit = keys["files"] - if yamlHasKey(data, "no-follow-symlinks") { + if keys["no-follow-symlinks"] { cfg.Deprecations = append(cfg.Deprecations, "config key `no-follow-symlinks` is deprecated; "+ "symlinks are now skipped by default — "+ @@ -48,28 +51,34 @@ func Load(path string) (*Config, error) { return &cfg, nil } -// yamlHasKey returns true if the top-level YAML mapping contains the given key. -func yamlHasKey(data []byte, key string) bool { +// topLevelKeySet returns the set of top-level YAML mapping keys +// present in data, or an empty set on parse error. It rejects +// anchor/alias usage for the same reason yamlHasKey does. +func topLevelKeySet(data []byte) map[string]bool { if err := lint.RejectYAMLAliases(data); err != nil { - return false + return map[string]bool{} } var node yaml.Node if err := yaml.Unmarshal(data, &node); err != nil { - return false + return map[string]bool{} } if node.Kind != yaml.DocumentNode || len(node.Content) == 0 { - return false + return map[string]bool{} } mapping := node.Content[0] if mapping.Kind != yaml.MappingNode { - return false + return map[string]bool{} } + result := make(map[string]bool, len(mapping.Content)/2) for i := 0; i < len(mapping.Content)-1; i += 2 { - if mapping.Content[i].Value == key { - return true - } + result[mapping.Content[i].Value] = true } - return false + return result +} + +// yamlHasKey returns true if the top-level YAML mapping contains the given key. +func yamlHasKey(data []byte, key string) bool { + return topLevelKeySet(data)[key] } // Discover walks up the directory tree from startDir looking for a From 5066fcf69394fa8c7cf9f0a4cb0117eb080d1972 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 18:14:12 +0000 Subject: [PATCH 07/28] =?UTF-8?q?plan=2084:=20address=20third=20Copilot=20?= =?UTF-8?q?review=20=E2=80=94=20clarify=20symlink-to-dir?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot's third-round comments all converge on one fact: filepath.Walk is Lstat-based and never descends into a symlink root. That makes a few places misleading: - resolveArg / resolveGlob: if FollowSymlinks is true and an explicit arg or glob match is a symlink-to-dir, os.Stat classifies it as a directory but walkDir silently walks nothing. Now the symlink-to- directory case is skipped unconditionally, and the doc comment on ResolveOpts.FollowSymlinks spells out that the flag applies to file symlinks only. - walkDir / discovery.walker: the `if info.IsDir() { return SkipDir }` branch inside the symlink guard was dead because Walk's Lstat-based info never reports IsDir==true for a symlink. Dropped and commented. - discovery Options.FollowSymlinks: doc comment now notes that symlinked directories are never descended into even when the flag is on, matching production behavior. - discovery coverage test: the synthetic fakeFileInfo-with-isDir=true pair asserted the now-dead SkipDir branch and gave false confidence. Replaced with a real os.Symlink fixture + os.Lstat info. The unused fakeFileInfo helper is removed. - new e2e test TestE2E_Symlink_DirSymlinkAlwaysSkipped pins the symlink-to-directory semantics under --follow-symlinks. --- cmd/mdsmith/e2e_symlink_default_deny_test.go | 27 +++++++ internal/discovery/discovery.go | 15 ++-- internal/discovery/discovery_coverage_test.go | 71 +++++++++---------- internal/lint/files.go | 50 +++++++++---- 4 files changed, 107 insertions(+), 56 deletions(-) diff --git a/cmd/mdsmith/e2e_symlink_default_deny_test.go b/cmd/mdsmith/e2e_symlink_default_deny_test.go index dcf971623..5ae13a933 100644 --- a/cmd/mdsmith/e2e_symlink_default_deny_test.go +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -118,6 +118,33 @@ func TestE2E_Symlink_FollowSymlinksConfigKey_OptsIn(t *testing.T) { "expected exit 1 with follow-symlinks: true exposing dirty linked file") } +// TestE2E_Symlink_DirSymlinkAlwaysSkipped asserts that a symlink +// pointing at a directory is skipped even in opt-in mode. Rationale: +// filepath.Walk is Lstat-based and would silently return zero files +// for a symlink root; `--follow-symlinks` applies only to file +// symlinks, as documented on ResolveOpts.FollowSymlinks. +func TestE2E_Symlink_DirSymlinkAlwaysSkipped(t *testing.T) { + project := t.TempDir() + external := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "rules:\n no-trailing-spaces: true\n") + + // External dir with a dirty file. Project contains a symlinked + // directory pointing at it. + require.NoError(t, os.WriteFile(filepath.Join(external, "dirty.md"), + []byte("# Evil\n\ntrailing \n"), 0o644)) + require.NoError(t, os.Symlink(external, filepath.Join(project, "linked"))) + + // Explicit arg + --follow-symlinks: symlinked directory is + // skipped silently; no findings. + _, _, exitOpt := runBinaryInDir(t, project, "", "check", + "--no-color", "--no-gitignore", "--follow-symlinks", "linked") + assert.Equal(t, 0, exitOpt, + "symlinked directory must be skipped even with --follow-symlinks") +} + // TestE2E_Symlink_LegacyFlag_OverridesFollowConfig verifies that the // deprecated --no-follow-symlinks flag still has meaning: it forces // deny even when `follow-symlinks: true` is set in config. This lets diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go index 76be15b17..8cd3dc105 100644 --- a/internal/discovery/discovery.go +++ b/internal/discovery/discovery.go @@ -22,8 +22,13 @@ type Options struct { // UseGitignore enables filtering by .gitignore rules. UseGitignore bool - // FollowSymlinks opts in to following symlinks during the walk. - // The zero value skips all symlinks, which is the secure default. + // FollowSymlinks opts in to reading symlinked file entries during + // the walk. The zero value skips all symlinks, which is the secure + // default. + // + // filepath.Walk is Lstat-based, so a symlinked directory is never + // descended into even when this flag is true — it only controls + // whether symlinked file entries are included in results. FollowSymlinks bool } @@ -103,10 +108,10 @@ func (w *walker) visit(path string, info os.FileInfo, walkErr error) error { } rel = filepath.ToSlash(rel) + // Symlink entries always have Lstat-based info with + // IsDir()==false under filepath.Walk, so returning nil also + // means Walk won't try to descend. if !w.followSymlinks && info.Mode()&os.ModeSymlink != 0 { - if info.IsDir() { - return filepath.SkipDir - } return nil } diff --git a/internal/discovery/discovery_coverage_test.go b/internal/discovery/discovery_coverage_test.go index 978c9eb19..bb7a27809 100644 --- a/internal/discovery/discovery_coverage_test.go +++ b/internal/discovery/discovery_coverage_test.go @@ -4,7 +4,6 @@ import ( "os" "path/filepath" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -22,46 +21,58 @@ func TestVisit_WalkError(t *testing.T) { assert.ErrorIs(t, err, os.ErrPermission) } -// TestVisit_SkipsSymlinkDirByDefault confirms the walker returns -// SkipDir for a symlinked directory entry when FollowSymlinks is -// false (the secure default), without needing a pattern. +// TestVisit_SkipsSymlinkDirByDefault confirms the walker returns nil +// (no descent) for a symlinked directory entry when FollowSymlinks is +// false. filepath.Walk reports symlinks via Lstat, so the entry's +// info has ModeSymlink set but IsDir()==false — the test uses real +// Lstat info rather than a synthetic fakeFileInfo so the assertion +// reflects actual Walk semantics. func TestVisit_SkipsSymlinkDirByDefault(t *testing.T) { dir := t.TempDir() - absBase := dir - subDir := filepath.Join(dir, "linked") - require.NoError(t, os.MkdirAll(subDir, 0o755)) + target := filepath.Join(dir, "real") + require.NoError(t, os.MkdirAll(target, 0o755)) + linked := filepath.Join(dir, "linked") + require.NoError(t, os.Symlink(target, linked)) + + info, err := os.Lstat(linked) + require.NoError(t, err) + require.NotZero(t, info.Mode()&os.ModeSymlink, "fixture must be a symlink") w := &walker{ - absBase: absBase, + absBase: dir, patterns: []string{"**/*.md"}, seen: make(map[string]bool), } - - info := fakeFileInfo{name: "linked", mode: os.ModeDir | os.ModeSymlink, isDir: true} - err := w.visit(filepath.Join(absBase, "linked"), info, nil) - assert.Equal(t, filepath.SkipDir, err) + visitErr := w.visit(linked, info, nil) + assert.NoError(t, visitErr) + assert.Empty(t, w.result, "symlink must be skipped") } -// TestVisit_FollowsSymlinkDirWhenOptedIn asserts the symlinked -// directory is NOT skipped when the walker has FollowSymlinks=true. -func TestVisit_FollowsSymlinkDirWhenOptedIn(t *testing.T) { +// TestVisit_FollowsSymlinkFileWhenOptedIn asserts a symlinked markdown +// file is NOT skipped when FollowSymlinks=true. This is the primary +// opt-in case — symlinked directories are still not recursed into, +// per the Options.FollowSymlinks doc comment. +func TestVisit_FollowsSymlinkFileWhenOptedIn(t *testing.T) { dir := t.TempDir() - absBase := dir - subDir := filepath.Join(dir, "linked") - require.NoError(t, os.MkdirAll(subDir, 0o755)) + target := filepath.Join(dir, "real.md") + require.NoError(t, os.WriteFile(target, []byte("# Real\n"), 0o644)) + linked := filepath.Join(dir, "linked.md") + require.NoError(t, os.Symlink(target, linked)) + + info, err := os.Lstat(linked) + require.NoError(t, err) w := &walker{ - absBase: absBase, + absBase: dir, patterns: []string{"**/*.md"}, followSymlinks: true, seen: make(map[string]bool), } - - info := fakeFileInfo{name: "linked", mode: os.ModeDir | os.ModeSymlink, isDir: true} - err := w.visit(filepath.Join(absBase, "linked"), info, nil) - assert.NoError(t, err) + visitErr := w.visit(linked, info, nil) + assert.NoError(t, visitErr) + assert.Len(t, w.result, 1, "symlinked file must be included under opt-in") } func TestVisit_SkipsFileWhenGitignored(t *testing.T) { @@ -172,17 +183,3 @@ func TestDiscover_FollowSymlinks_OptIn(t *testing.T) { require.NoError(t, err) require.Len(t, files, 2, "both real and linked entries are discovered") } - -// fakeFileInfo implements os.FileInfo for testing. -type fakeFileInfo struct { - name string - mode os.FileMode - isDir bool -} - -func (f fakeFileInfo) Name() string { return f.name } -func (f fakeFileInfo) Size() int64 { return 0 } -func (f fakeFileInfo) Mode() os.FileMode { return f.mode } -func (f fakeFileInfo) ModTime() time.Time { return time.Time{} } -func (f fakeFileInfo) IsDir() bool { return f.isDir } -func (f fakeFileInfo) Sys() any { return nil } diff --git a/internal/lint/files.go b/internal/lint/files.go index 20e3a530d..3018f24cb 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -28,9 +28,15 @@ type ResolveOpts struct { // used (see DefaultResolveOpts). UseGitignore *bool - // FollowSymlinks opts in to following symbolic links during - // directory walks and glob expansion. The zero value skips all - // symlinks, which is the secure default. + // FollowSymlinks opts in to following symbolic links that resolve + // to files. The zero value skips all symlinks, which is the secure + // default. + // + // Symlinked directories are always skipped, regardless of this + // flag. `filepath.Walk` is Lstat-based and does not descend into + // a symlink root, so supporting symlinked-directory traversal + // would require explicit EvalSymlinks resolution plus atomic + // writes against an unknown path — out of scope for plan 84. FollowSymlinks bool } @@ -99,7 +105,8 @@ func resolveArg(arg string, opts ResolveOpts, addFile func(string)) error { if lerr != nil { return fmt.Errorf("cannot access %q: %w", arg, lerr) } - if !opts.FollowSymlinks && linfo.Mode()&os.ModeSymlink != 0 { + isSymlink := linfo.Mode()&os.ModeSymlink != 0 + if isSymlink && !opts.FollowSymlinks { return nil } @@ -108,6 +115,14 @@ func resolveArg(arg string, opts ResolveOpts, addFile func(string)) error { return fmt.Errorf("cannot access %q: %w", arg, err) } + // Symlinks to directories are always skipped. filepath.Walk is + // Lstat-based and cannot recurse into a symlink root, so walking + // a symlinked dir would silently yield no files and confuse + // callers. `--follow-symlinks` applies to file symlinks only. + if isSymlink && info.IsDir() { + return nil + } + if info.IsDir() { return addDirFiles(arg, opts, addFile) } @@ -124,17 +139,22 @@ func resolveGlob(pattern string, opts ResolveOpts, addFile func(string)) error { return fmt.Errorf("invalid glob pattern %q: %w", pattern, err) } for _, m := range matches { - // Default-deny symlinks unless the caller opts in. - if !opts.FollowSymlinks { - if linfo, lerr := os.Lstat(m); lerr == nil && - linfo.Mode()&os.ModeSymlink != 0 { - continue - } + linfo, lerr := os.Lstat(m) + if lerr != nil { + continue + } + isSymlink := linfo.Mode()&os.ModeSymlink != 0 + if isSymlink && !opts.FollowSymlinks { + continue } info, err := os.Stat(m) if err != nil { continue } + // Symlinks to directories are always skipped (see resolveArg). + if isSymlink && info.IsDir() { + continue + } if info.IsDir() { if err := addDirFiles(m, opts, addFile); err != nil { return err @@ -160,7 +180,9 @@ func addDirFiles(dir string, opts ResolveOpts, addFile func(string)) error { // walkDir recursively walks a directory and returns all markdown files. // When useGitignore is true, files matched by .gitignore patterns are skipped. -// Symlinks are skipped unless followSymlinks is true. +// Symlinks are skipped unless followSymlinks is true. filepath.Walk is +// Lstat-based, so symlinked directories encountered during the walk are +// never descended into either way. func walkDir(dir string, useGitignore, followSymlinks bool) ([]string, error) { var matcher *GitignoreMatcher if useGitignore { @@ -173,10 +195,10 @@ func walkDir(dir string, useGitignore, followSymlinks bool) ([]string, error) { return err } + // Symlink entries always have Lstat-based info with + // IsDir()==false under filepath.Walk, so a plain return nil + // here also means Walk won't try to descend. if !followSymlinks && info.Mode()&os.ModeSymlink != 0 { - if info.IsDir() { - return filepath.SkipDir - } return nil } From 3c9dd52ec4a5b32093ab192c6677e49ae4e08c2e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 18:22:50 +0000 Subject: [PATCH 08/28] =?UTF-8?q?plan=2084:=20address=20fourth=20Copilot?= =?UTF-8?q?=20review=20=E2=80=94=20.md-named=20dir=20symlinks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot caught a real edge case in the walk path: with FollowSymlinks=true, a symlink named `evil.md` pointing at a directory has Lstat info with ModeSymlink set and IsDir==false. That entry slipped past the symlink guard, matched `isMarkdown(path)`, and got queued as a markdown file — then failed later on "is a directory" read. Fix: in both walkDir (internal/lint) and the discovery walker, when we encounter a symlink entry in opt-in mode, os.Stat the target; if it's a directory, skip the entry. This keeps the opt-in surface to regular files only, matching the FollowSymlinks doc comments. Added TestE2E_Symlink_DirSymlinkWithMdName to pin the new behavior: a dir-symlink whose name ends in .md is silently dropped under --follow-symlinks; no "is a directory" error leaks to stderr. --- cmd/mdsmith/e2e_symlink_default_deny_test.go | 29 ++++++++++++++++++++ internal/discovery/discovery.go | 13 +++++++-- internal/lint/files.go | 14 ++++++++-- 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/cmd/mdsmith/e2e_symlink_default_deny_test.go b/cmd/mdsmith/e2e_symlink_default_deny_test.go index 5ae13a933..8c7c87f4b 100644 --- a/cmd/mdsmith/e2e_symlink_default_deny_test.go +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -145,6 +145,35 @@ func TestE2E_Symlink_DirSymlinkAlwaysSkipped(t *testing.T) { "symlinked directory must be skipped even with --follow-symlinks") } +// TestE2E_Symlink_DirSymlinkWithMdName asserts that a symlink named +// like a markdown file but pointing at a directory is NOT picked up +// as a markdown file, even in opt-in mode. This guards against a +// subtle failure mode: without the target os.Stat, Lstat-based info +// reports IsDir==false and isMarkdown(path) is true, so the entry +// would be queued and then fail later on "is a directory" read. +func TestE2E_Symlink_DirSymlinkWithMdName(t *testing.T) { + project := t.TempDir() + external := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "files:\n - \"**/*.md\"\nrules:\n no-trailing-spaces: true\n") + writeFixture(t, project, "ok.md", "# OK\n\nClean body.\n") + + // Symlink whose name ends in .md but whose target is a directory. + require.NoError(t, os.Symlink(external, filepath.Join(project, "evil.md"))) + + // Discovery (no explicit file args) with --follow-symlinks: the + // dir-symlink-with-.md-name must be filtered out. + _, stderr, exitCode := runBinaryInDir(t, project, "", + "check", "--no-color", "--no-gitignore", "--follow-symlinks") + assert.Equal(t, 0, exitCode, + "dir-symlink-with-.md-name must not be read; stderr: %s", stderr) + assert.NotContains(t, stderr, "is a directory", + "symlink-to-dir must not leak as a markdown read error; stderr: %s", + stderr) +} + // TestE2E_Symlink_LegacyFlag_OverridesFollowConfig verifies that the // deprecated --no-follow-symlinks flag still has meaning: it forces // deny even when `follow-symlinks: true` is set in config. This lets diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go index 8cd3dc105..40b4fe366 100644 --- a/internal/discovery/discovery.go +++ b/internal/discovery/discovery.go @@ -111,8 +111,17 @@ func (w *walker) visit(path string, info os.FileInfo, walkErr error) error { // Symlink entries always have Lstat-based info with // IsDir()==false under filepath.Walk, so returning nil also // means Walk won't try to descend. - if !w.followSymlinks && info.Mode()&os.ModeSymlink != 0 { - return nil + if info.Mode()&os.ModeSymlink != 0 { + if !w.followSymlinks { + return nil + } + // In opt-in mode, include the entry only if it resolves to + // a regular file. A symlink-to-dir named like "evil.md" + // would otherwise match patterns and land in results — + // the Options doc says symlinked directories are skipped. + if tgt, statErr := os.Stat(path); statErr != nil || tgt.IsDir() { + return nil + } } if w.isGitignored(path, info) { diff --git a/internal/lint/files.go b/internal/lint/files.go index 3018f24cb..9ebd0b3f5 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -198,8 +198,18 @@ func walkDir(dir string, useGitignore, followSymlinks bool) ([]string, error) { // Symlink entries always have Lstat-based info with // IsDir()==false under filepath.Walk, so a plain return nil // here also means Walk won't try to descend. - if !followSymlinks && info.Mode()&os.ModeSymlink != 0 { - return nil + if info.Mode()&os.ModeSymlink != 0 { + if !followSymlinks { + return nil + } + // In opt-in mode, follow the link only if it resolves to + // a regular file. A symlink-to-dir named "evil.md" would + // otherwise be treated as a markdown file and fail on + // read; this also matches the FollowSymlinks doc that + // says symlinked directories are always skipped. + if tgt, statErr := os.Stat(path); statErr != nil || tgt.IsDir() { + return nil + } } if matcher != nil && isGitignored(matcher, path, info.IsDir()) { From 003f5515e36d9fe6e70c3706a81835d16a498029 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 18:33:27 +0000 Subject: [PATCH 09/28] =?UTF-8?q?plan=2084:=20address=20fifth=20Copilot=20?= =?UTF-8?q?review=20=E2=80=94=20symlinked=20ancestors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot's round 5 identified two real bypasses: a user-supplied path `linked/dirty.md` (where `linked` is a symlinked dir) still reached the external target because Lstat on the leaf followed the intermediate symlink during name resolution; and a glob like `linked/*.md` expanded through the symlinked dir because `filepath.Glob` transparently resolves symlinked path components. Fix: add hasSymlinkAncestor(path) in internal/lint/files.go. It Lstats every relative ancestor of the path (but not the fs root) and returns true if any component is a symbolic link. Both resolveArg and resolveGlob now reject matches with a symlinked ancestor, regardless of FollowSymlinks — symlinked directories are always out of scope. Scope note: the helper skips absolute paths and paths escaping via `..`. Probing above the invocation directory would break environments where the temp dir or project root sits behind a system-level symlink (e.g. `/tmp` on macOS). Added two e2e regression tests: - TestE2E_Symlink_DirSymlinkAncestorPath — explicit path `linked/dirty.md` - TestE2E_Symlink_DirSymlinkAncestorGlob — glob `linked/*.md` Also updated docs/security/2026-04-05-adversarial-markdown.md: the "Locations" bullets now describe the current Lstat-based implementation instead of the original filepath.Walk write path. --- cmd/mdsmith/e2e_symlink_default_deny_test.go | 52 +++++++++++++++++++ .../2026-04-05-adversarial-markdown.md | 18 +++++-- internal/lint/files.go | 52 +++++++++++++++++++ 3 files changed, 117 insertions(+), 5 deletions(-) diff --git a/cmd/mdsmith/e2e_symlink_default_deny_test.go b/cmd/mdsmith/e2e_symlink_default_deny_test.go index 8c7c87f4b..840038a9a 100644 --- a/cmd/mdsmith/e2e_symlink_default_deny_test.go +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -174,6 +174,58 @@ func TestE2E_Symlink_DirSymlinkWithMdName(t *testing.T) { stderr) } +// TestE2E_Symlink_DirSymlinkAncestorPath asserts that a path +// traversing a symlinked ancestor directory is rejected in both +// default-deny and opt-in modes. `mdsmith check linked/dirty.md` +// must not reach the external target even though the leaf itself +// is a regular file. +func TestE2E_Symlink_DirSymlinkAncestorPath(t *testing.T) { + project := t.TempDir() + external := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "rules:\n no-trailing-spaces: true\n") + + require.NoError(t, os.WriteFile(filepath.Join(external, "dirty.md"), + []byte("# Evil\n\ntrailing \n"), 0o644)) + require.NoError(t, os.Symlink(external, filepath.Join(project, "linked"))) + + // Default-deny: explicit path through the symlinked directory. + _, _, exitDeny := runBinaryInDir(t, project, "", "check", + "--no-color", "--no-gitignore", "linked/dirty.md") + assert.Equal(t, 0, exitDeny, + "path through a symlinked directory must be skipped by default") + + // Opt-in mode: still skipped. Symlinked directories are always + // out of scope (doc on ResolveOpts.FollowSymlinks). + _, _, exitOpt := runBinaryInDir(t, project, "", "check", + "--no-color", "--no-gitignore", "--follow-symlinks", "linked/dirty.md") + assert.Equal(t, 0, exitOpt, + "path through a symlinked directory must be skipped under opt-in too") +} + +// TestE2E_Symlink_DirSymlinkAncestorGlob asserts the same bypass +// via glob expansion: `linked/*.md` must not reach files under the +// symlinked directory. +func TestE2E_Symlink_DirSymlinkAncestorGlob(t *testing.T) { + project := t.TempDir() + external := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "rules:\n no-trailing-spaces: true\n") + + require.NoError(t, os.WriteFile(filepath.Join(external, "dirty.md"), + []byte("# Evil\n\ntrailing \n"), 0o644)) + require.NoError(t, os.Symlink(external, filepath.Join(project, "linked"))) + + _, _, exitCode := runBinaryInDir(t, project, "", "check", + "--no-color", "--no-gitignore", "linked/*.md") + assert.Equal(t, 0, exitCode, + "glob expanding under a symlinked dir must not reach external files") +} + // TestE2E_Symlink_LegacyFlag_OverridesFollowConfig verifies that the // deprecated --no-follow-symlinks flag still has meaning: it forces // deny even when `follow-symlinks: true` is set in config. This lets diff --git a/docs/security/2026-04-05-adversarial-markdown.md b/docs/security/2026-04-05-adversarial-markdown.md index cedba6aa2..f8343c3df 100644 --- a/docs/security/2026-04-05-adversarial-markdown.md +++ b/docs/security/2026-04-05-adversarial-markdown.md @@ -108,14 +108,22 @@ Escape sequences pass through to the terminal: screen clearing (`\033[2J`), wind ### 4. MEDIUM — Symlinks Followed by Default (Read + Write) -**Locations:** - -- `internal/lint/files.go:190` — `filepath.Walk` follows file symlinks -- `internal/fix/fix.go:84-122` — `os.WriteFile` follows symlinks on write +**Locations (after plan 84):** + +- `internal/lint/files.go` — `resolveArg`, `resolveGlob`, and + `walkDir` Lstat each path entry and skip symbolic links unless + `FollowSymlinks` opts in. `hasSymlinkAncestor` also rejects any + path whose relative ancestors include a symlinked directory, so + `linked/dirty.md` and `linked/*.md` cannot reach external targets. +- `internal/discovery/discovery.go` — the discovery walker applies + the same Lstat-based skip during directory traversal. +- `internal/fix/fix.go` — atomic write via `os.Rename(tmp, path)` + replaces the symlink entry itself rather than following it to the + target (plan 83 write-side protection). **Original finding:** symlink following was the default; the `--no-follow-symlinks` flag and config key were opt-in. -**Status (plan 84, resolved):** the default is inverted. Symlinks are skipped by default across directory walks, glob expansion, and explicit non-glob path arguments. Users opt in with `--follow-symlinks` or `follow-symlinks: true`. The legacy `--no-follow-symlinks` flag and `no-follow-symlinks:` config key are silently accepted for one release, with a deprecation warning emitted for the config key. +**Status (plan 84, resolved):** the default is inverted. Symlinks are skipped by default across directory walks, glob expansion, and explicit non-glob path arguments. Symlinked directories are always skipped — including when a path or glob traverses through one — regardless of `FollowSymlinks`. Users opt in (for file symlinks) with `--follow-symlinks` or `follow-symlinks: true`. The legacy `--no-follow-symlinks` flag and `no-follow-symlinks:` config key are silently accepted for one release; the config key emits a deprecation warning. **Adversarial file:** diff --git a/internal/lint/files.go b/internal/lint/files.go index 9ebd0b3f5..b3daf4007 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -97,6 +97,16 @@ func resolveArg(arg string, opts ResolveOpts, addFile func(string)) error { return resolveGlob(arg, opts, addFile) } + // Reject any path that traverses a symlinked directory. Intermediate + // symlinked components would otherwise let a user-supplied path + // like `linked/dirty.md` reach an external target, since Lstat + // on the leaf follows the intermediate symlink during name + // resolution. Symlinked directories are always skipped regardless + // of FollowSymlinks, per the Option doc. + if hasSymlinkAncestor(arg) { + return nil + } + // Default-deny symlinks even for explicit, non-glob paths. Lstat // (not Stat) avoids following the link so that an attacker who // plants `evil.md -> /etc/cron.d/jobs` can't trick a user into @@ -132,6 +142,40 @@ func resolveArg(arg string, opts ResolveOpts, addFile func(string)) error { return nil } +// hasSymlinkAncestor reports whether any ancestor directory named +// by the relative components of path is a symbolic link. It rejects +// paths like `linked/dirty.md` where `linked` is a symlinked dir — +// the filesystem would resolve the link during `os.Stat(path)` and +// let the external target slip past a leaf-only Lstat check. +// +// Only relative path components are walked. Absolute paths and +// paths that escape via `..` are treated as the user's explicit +// choice: we do not probe above the invocation directory (that +// would misclassify environments where temp dirs or project roots +// live behind a system-wide symlink, e.g. `/tmp` on macOS). +func hasSymlinkAncestor(path string) bool { + clean := filepath.Clean(path) + if filepath.IsAbs(clean) || strings.HasPrefix(clean, "..") { + return false + } + dir := filepath.Dir(clean) + for dir != "." && dir != "/" { + if strings.HasPrefix(dir, "..") { + return false + } + info, err := os.Lstat(dir) + if err == nil && info.Mode()&os.ModeSymlink != 0 { + return true + } + parent := filepath.Dir(dir) + if parent == dir { + return false + } + dir = parent + } + return false +} + // resolveGlob expands a glob pattern and adds matching markdown files. func resolveGlob(pattern string, opts ResolveOpts, addFile func(string)) error { matches, err := filepath.Glob(pattern) @@ -139,6 +183,14 @@ func resolveGlob(pattern string, opts ResolveOpts, addFile func(string)) error { return fmt.Errorf("invalid glob pattern %q: %w", pattern, err) } for _, m := range matches { + // filepath.Glob follows symlinked directory components + // during expansion, so a pattern like `linked/*.md` will + // return paths rooted under a symlinked dir. Reject any + // match whose ancestor chain contains a symlink: symlinked + // directories are always skipped. + if hasSymlinkAncestor(m) { + continue + } linfo, lerr := os.Lstat(m) if lerr != nil { continue From 96bac7d6f9c855a85b0cd97915ee80bb59de7320 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 18:41:14 +0000 Subject: [PATCH 10/28] plan 84: address sixth Copilot review Three substantive concerns and one portability note. 1. Absolute-path bypass: the previous hasSymlinkAncestor returned false for all absolute paths, including those inside cwd. An attacker-plantable symlink at `./linked` and an invocation like `mdsmith check /abs/path/to/project/linked/dirty.md` slipped through. Now the helper converts absolute paths to their cwd-relative form via filepath.Rel. Paths inside cwd are checked against their relative ancestors; paths that escape via `..` are still treated as the user's explicit choice, preserving the macOS `/tmp` workaround. 2. Doc vs implementation: the ResolveOpts.FollowSymlinks doc says "symlinked directories are always skipped"; the new check matches that statement for paths inside cwd. Updated the helper doc to describe the rel-from-cwd scope explicitly. 3. Per-match Lstat multiplier in resolveGlob: a glob expansion with many matches under deep paths re-Lstatted the same ancestor directories over and over. Split the helper into a public hasSymlinkAncestor and a memoized hasSymlinkAncestorCached; resolveGlob now shares one cache across matches so each directory is Lstat'd at most once per expansion. 4. Windows/sandbox portability: a new skipIfSymlinkUnsupported helper in cmd/mdsmith/e2e_test.go probes os.Symlink once per test and skips cleanly when unsupported. Applied to all 12 tests in cmd/mdsmith/e2e_symlink_default_deny_test.go. Added TestE2E_Symlink_DirSymlinkAncestorAbsPath to lock in the absolute-path-inside-cwd behavior. --- cmd/mdsmith/e2e_symlink_default_deny_test.go | 36 +++++++ cmd/mdsmith/e2e_test.go | 17 ++++ internal/lint/files.go | 99 ++++++++++++++------ 3 files changed, 125 insertions(+), 27 deletions(-) diff --git a/cmd/mdsmith/e2e_symlink_default_deny_test.go b/cmd/mdsmith/e2e_symlink_default_deny_test.go index 840038a9a..9d6764e68 100644 --- a/cmd/mdsmith/e2e_symlink_default_deny_test.go +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -18,6 +18,7 @@ import ( // the project must not be walked by default. Running `fix` would // otherwise overwrite that external file. func TestE2E_Symlink_DefaultDeny_ExternalTargetSkipped(t *testing.T) { + skipIfSymlinkUnsupported(t) project := t.TempDir() external := t.TempDir() @@ -46,6 +47,7 @@ func TestE2E_Symlink_DefaultDeny_ExternalTargetSkipped(t *testing.T) { // secure default also covers explicit, non-glob path arguments. // `mdsmith check ./evil.md` must not process a symlink target. func TestE2E_Symlink_DefaultDeny_ExplicitFileArg(t *testing.T) { + skipIfSymlinkUnsupported(t) project := t.TempDir() external := t.TempDir() @@ -76,6 +78,7 @@ func TestE2E_Symlink_DefaultDeny_ExplicitFileArg(t *testing.T) { // TestE2E_Symlink_FollowSymlinksFlag_OptsIn asserts the new // --follow-symlinks CLI flag walks symlinked entries. func TestE2E_Symlink_FollowSymlinksFlag_OptsIn(t *testing.T) { + skipIfSymlinkUnsupported(t) project := t.TempDir() external := t.TempDir() @@ -99,6 +102,7 @@ func TestE2E_Symlink_FollowSymlinksFlag_OptsIn(t *testing.T) { // TestE2E_Symlink_FollowSymlinksConfigKey_OptsIn asserts the new // follow-symlinks: true config key works. func TestE2E_Symlink_FollowSymlinksConfigKey_OptsIn(t *testing.T) { + skipIfSymlinkUnsupported(t) project := t.TempDir() external := t.TempDir() @@ -124,6 +128,7 @@ func TestE2E_Symlink_FollowSymlinksConfigKey_OptsIn(t *testing.T) { // for a symlink root; `--follow-symlinks` applies only to file // symlinks, as documented on ResolveOpts.FollowSymlinks. func TestE2E_Symlink_DirSymlinkAlwaysSkipped(t *testing.T) { + skipIfSymlinkUnsupported(t) project := t.TempDir() external := t.TempDir() @@ -152,6 +157,7 @@ func TestE2E_Symlink_DirSymlinkAlwaysSkipped(t *testing.T) { // reports IsDir==false and isMarkdown(path) is true, so the entry // would be queued and then fail later on "is a directory" read. func TestE2E_Symlink_DirSymlinkWithMdName(t *testing.T) { + skipIfSymlinkUnsupported(t) project := t.TempDir() external := t.TempDir() @@ -180,6 +186,7 @@ func TestE2E_Symlink_DirSymlinkWithMdName(t *testing.T) { // must not reach the external target even though the leaf itself // is a regular file. func TestE2E_Symlink_DirSymlinkAncestorPath(t *testing.T) { + skipIfSymlinkUnsupported(t) project := t.TempDir() external := t.TempDir() @@ -209,6 +216,7 @@ func TestE2E_Symlink_DirSymlinkAncestorPath(t *testing.T) { // via glob expansion: `linked/*.md` must not reach files under the // symlinked directory. func TestE2E_Symlink_DirSymlinkAncestorGlob(t *testing.T) { + skipIfSymlinkUnsupported(t) project := t.TempDir() external := t.TempDir() @@ -226,11 +234,37 @@ func TestE2E_Symlink_DirSymlinkAncestorGlob(t *testing.T) { "glob expanding under a symlinked dir must not reach external files") } +// TestE2E_Symlink_DirSymlinkAncestorAbsPath covers the absolute-path +// variant of the ancestor-bypass test. `mdsmith check` invoked with +// an absolute path that happens to pass through a symlinked directory +// under cwd must still be rejected — the ancestor check converts the +// path to cwd-relative form before probing. +func TestE2E_Symlink_DirSymlinkAncestorAbsPath(t *testing.T) { + skipIfSymlinkUnsupported(t) + project := t.TempDir() + external := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "rules:\n no-trailing-spaces: true\n") + + require.NoError(t, os.WriteFile(filepath.Join(external, "dirty.md"), + []byte("# Evil\n\ntrailing \n"), 0o644)) + require.NoError(t, os.Symlink(external, filepath.Join(project, "linked"))) + + absPath := filepath.Join(project, "linked", "dirty.md") + _, _, exitCode := runBinaryInDir(t, project, "", "check", + "--no-color", "--no-gitignore", absPath) + assert.Equal(t, 0, exitCode, + "absolute path through a symlinked directory must be skipped") +} + // TestE2E_Symlink_LegacyFlag_OverridesFollowConfig verifies that the // deprecated --no-follow-symlinks flag still has meaning: it forces // deny even when `follow-symlinks: true` is set in config. This lets // users run one-off secure invocations without editing the config. func TestE2E_Symlink_LegacyFlag_OverridesFollowConfig(t *testing.T) { + skipIfSymlinkUnsupported(t) project := t.TempDir() external := t.TempDir() @@ -263,6 +297,7 @@ func TestE2E_Symlink_LegacyFlag_OverridesFollowConfig(t *testing.T) { // old `no-follow-symlinks:` key still parses and emits a deprecation // warning on stderr. func TestE2E_Symlink_LegacyNoFollowConfig_Deprecation(t *testing.T) { + skipIfSymlinkUnsupported(t) project := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) @@ -287,6 +322,7 @@ func TestE2E_Symlink_LegacyNoFollowConfig_Deprecation(t *testing.T) { // plan 83 section C), and the in-project symlink is only visited // when the flag is set. func TestE2E_Symlink_FixRespectsFollowSymlinks(t *testing.T) { + skipIfSymlinkUnsupported(t) project := t.TempDir() external := t.TempDir() diff --git a/cmd/mdsmith/e2e_test.go b/cmd/mdsmith/e2e_test.go index 4556ebeb9..61dfd0909 100644 --- a/cmd/mdsmith/e2e_test.go +++ b/cmd/mdsmith/e2e_test.go @@ -201,6 +201,23 @@ func writeFixture(t *testing.T, dir, name, content string) string { return path } +// skipIfSymlinkUnsupported skips the calling test when the host +// cannot create symbolic links. This typically happens on Windows +// without Developer Mode / elevated privileges, and in some +// sandboxed CI environments. +func skipIfSymlinkUnsupported(t *testing.T) { + t.Helper() + probe := t.TempDir() + target := filepath.Join(probe, "t") + link := filepath.Join(probe, "l") + if err := os.WriteFile(target, nil, 0o644); err != nil { + t.Skipf("cannot create probe file: %v", err) + } + if err := os.Symlink(target, link); err != nil { + t.Skipf("symbolic links not supported on this host: %v", err) + } +} + func parseStats(t *testing.T, stderr string) (checked, fixed, failures, unfixed int) { t.Helper() re := regexp.MustCompile(`stats: checked=(\d+) fixed=(\d+) failures=(\d+) unfixed=(\d+)`) diff --git a/internal/lint/files.go b/internal/lint/files.go index b3daf4007..002589034 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -142,38 +142,80 @@ func resolveArg(arg string, opts ResolveOpts, addFile func(string)) error { return nil } -// hasSymlinkAncestor reports whether any ancestor directory named -// by the relative components of path is a symbolic link. It rejects -// paths like `linked/dirty.md` where `linked` is a symlinked dir — -// the filesystem would resolve the link during `os.Stat(path)` and -// let the external target slip past a leaf-only Lstat check. +// hasSymlinkAncestor reports whether any ancestor directory of +// path (up to, but excluding, the invocation directory) is a +// symbolic link. It rejects paths like `linked/dirty.md` where +// `linked` is a symlinked dir — the filesystem would resolve the +// link during `os.Stat(path)` and let the external target slip +// past a leaf-only Lstat check. // -// Only relative path components are walked. Absolute paths and -// paths that escape via `..` are treated as the user's explicit -// choice: we do not probe above the invocation directory (that -// would misclassify environments where temp dirs or project roots -// live behind a system-wide symlink, e.g. `/tmp` on macOS). +// For absolute paths inside the invocation directory, the check +// is performed on the rel-from-cwd form so that `cwd/linked/...` +// is still blocked without misclassifying system-level symlinks +// above cwd (e.g. `/tmp` on macOS). Paths outside the invocation +// directory are treated as the user's explicit choice and not +// probed. func hasSymlinkAncestor(path string) bool { - clean := filepath.Clean(path) - if filepath.IsAbs(clean) || strings.HasPrefix(clean, "..") { + return hasSymlinkAncestorCached(path, make(map[string]bool)) +} + +// hasSymlinkAncestorCached is the memoized form of +// hasSymlinkAncestor. Callers that run the check repeatedly (e.g. +// per glob match) should share a `cache` across invocations to +// avoid repeated `os.Lstat` calls on the same ancestor directories. +func hasSymlinkAncestorCached(path string, cache map[string]bool) bool { + rel, ok := relForAncestorCheck(path) + if !ok { return false } - dir := filepath.Dir(clean) - for dir != "." && dir != "/" { - if strings.HasPrefix(dir, "..") { - return false - } - info, err := os.Lstat(dir) - if err == nil && info.Mode()&os.ModeSymlink != 0 { - return true + return ancestorChainHasSymlink(filepath.Dir(rel), cache) +} + +// relForAncestorCheck normalises path into a cwd-relative form +// suitable for ancestor probing. It returns (rel, false) to signal +// "skip the check" for paths that are outside cwd or that escape +// via `..`. +func relForAncestorCheck(path string) (string, bool) { + clean := filepath.Clean(path) + if filepath.IsAbs(clean) { + cwd, err := os.Getwd() + if err != nil { + return "", false } - parent := filepath.Dir(dir) - if parent == dir { - return false + rel, err := filepath.Rel(cwd, clean) + if err != nil { + return "", false } - dir = parent + clean = rel + } + if clean == "." || clean == ".." || + strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + return "", false + } + return clean, true +} + +// ancestorChainHasSymlink walks upward from dir and reports whether +// any step in the chain is a symbolic link. Results are memoised +// per directory so sibling paths under a shared ancestor do not +// re-Lstat the same entries. +func ancestorChainHasSymlink(dir string, cache map[string]bool) bool { + if dir == "." || dir == "/" || + strings.HasPrefix(dir, ".."+string(filepath.Separator)) { + return false + } + if v, ok := cache[dir]; ok { + return v + } + result := false + if info, err := os.Lstat(dir); err == nil && + info.Mode()&os.ModeSymlink != 0 { + result = true + } else if parent := filepath.Dir(dir); parent != dir { + result = ancestorChainHasSymlink(parent, cache) } - return false + cache[dir] = result + return result } // resolveGlob expands a glob pattern and adds matching markdown files. @@ -182,13 +224,16 @@ func resolveGlob(pattern string, opts ResolveOpts, addFile func(string)) error { if err != nil { return fmt.Errorf("invalid glob pattern %q: %w", pattern, err) } + ancestorCache := make(map[string]bool) for _, m := range matches { // filepath.Glob follows symlinked directory components // during expansion, so a pattern like `linked/*.md` will // return paths rooted under a symlinked dir. Reject any // match whose ancestor chain contains a symlink: symlinked - // directories are always skipped. - if hasSymlinkAncestor(m) { + // directories are always skipped. The cache shares Lstat + // results across matches so large expansions don't repeat + // ancestor syscalls. + if hasSymlinkAncestorCached(m, ancestorCache) { continue } linfo, lerr := os.Lstat(m) From 879c4bc1cdf244d547cf4d3e35f9615bddf2c550 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 18:52:34 +0000 Subject: [PATCH 11/28] =?UTF-8?q?plan=2084:=20address=20seventh=20Copilot?= =?UTF-8?q?=20review=20=E2=80=94=20.git=20project=20root?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Close the last ancestor-check bypass: when the user runs mdsmith from outside the project and passes an absolute path that traverses a symlinked directory inside the target tree, the previous helper gave up because the path was not under cwd. The helper now chooses the stop boundary in this order: 1. cwd, if the path is under it (covers the common case); 2. otherwise the nearest ancestor of the path that contains a `.git` entry (the conventional project root); 3. otherwise "" — the path is outside any known project, so we trust the user rather than probing system paths. `gitProjectRoot` walks upward from the leaf's directory looking for `.git` (file or directory, matching the submodule case). Added TestE2E_Symlink_DirSymlinkAncestorAbsPathFromOutsideCwd: cwd points at a sibling temp dir, the arg is an absolute path into a separate `.git`-rooted project, and the symlinked component is still rejected. --- cmd/mdsmith/e2e_symlink_default_deny_test.go | 29 +++++ internal/lint/files.go | 107 ++++++++++++------- 2 files changed, 95 insertions(+), 41 deletions(-) diff --git a/cmd/mdsmith/e2e_symlink_default_deny_test.go b/cmd/mdsmith/e2e_symlink_default_deny_test.go index 9d6764e68..1559fa221 100644 --- a/cmd/mdsmith/e2e_symlink_default_deny_test.go +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -259,6 +259,35 @@ func TestE2E_Symlink_DirSymlinkAncestorAbsPath(t *testing.T) { "absolute path through a symlinked directory must be skipped") } +// TestE2E_Symlink_DirSymlinkAncestorAbsPathFromOutsideCwd covers the +// cross-cwd variant: the user is running mdsmith from a sibling +// directory and points at an absolute path inside a project they +// own (the target dir contains a .git marker). The ancestor check +// must still anchor at the project's .git boundary so a symlinked +// component inside the target tree is rejected. +func TestE2E_Symlink_DirSymlinkAncestorAbsPathFromOutsideCwd(t *testing.T) { + skipIfSymlinkUnsupported(t) + project := t.TempDir() + external := t.TempDir() + cwd := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "rules:\n no-trailing-spaces: true\n") + + require.NoError(t, os.WriteFile(filepath.Join(external, "dirty.md"), + []byte("# Evil\n\ntrailing \n"), 0o644)) + require.NoError(t, os.Symlink(external, filepath.Join(project, "linked"))) + + absPath := filepath.Join(project, "linked", "dirty.md") + // Note: cwd is a sibling directory, NOT the project root. + _, _, exitCode := runBinaryInDir(t, cwd, "", "check", + "--no-color", "--no-gitignore", absPath) + assert.Equal(t, 0, exitCode, + "abs path through a symlinked dir in a .git-rooted project must be "+ + "skipped even when the invocation cwd is outside the project") +} + // TestE2E_Symlink_LegacyFlag_OverridesFollowConfig verifies that the // deprecated --no-follow-symlinks flag still has meaning: it forces // deny even when `follow-symlinks: true` is set in config. This lets diff --git a/internal/lint/files.go b/internal/lint/files.go index 002589034..5dbb3d77e 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -143,18 +143,23 @@ func resolveArg(arg string, opts ResolveOpts, addFile func(string)) error { } // hasSymlinkAncestor reports whether any ancestor directory of -// path (up to, but excluding, the invocation directory) is a -// symbolic link. It rejects paths like `linked/dirty.md` where -// `linked` is a symlinked dir — the filesystem would resolve the -// link during `os.Stat(path)` and let the external target slip -// past a leaf-only Lstat check. +// path (up to, but excluding, the project boundary) is a symbolic +// link. It rejects paths like `linked/dirty.md` where `linked` is +// a symlinked dir — the filesystem would resolve the link during +// `os.Stat(path)` and let the external target slip past a +// leaf-only Lstat check. // -// For absolute paths inside the invocation directory, the check -// is performed on the rel-from-cwd form so that `cwd/linked/...` -// is still blocked without misclassifying system-level symlinks -// above cwd (e.g. `/tmp` on macOS). Paths outside the invocation -// directory are treated as the user's explicit choice and not -// probed. +// The project boundary is picked in this order: +// +// - cwd, if the path is under it (so `mdsmith check .` from the +// project root catches any symlinked dir inside); +// - otherwise, the nearest ancestor of the path that contains a +// `.git` entry (so an absolute path run from a sibling shell +// is still scanned within the target project); +// - otherwise, "" (trust the user — no scan). +// +// This keeps system-level symlinks above the boundary (e.g. `/tmp` +// on macOS) out of the probe. func hasSymlinkAncestor(path string) bool { return hasSymlinkAncestorCached(path, make(map[string]bool)) } @@ -164,44 +169,61 @@ func hasSymlinkAncestor(path string) bool { // per glob match) should share a `cache` across invocations to // avoid repeated `os.Lstat` calls on the same ancestor directories. func hasSymlinkAncestorCached(path string, cache map[string]bool) bool { - rel, ok := relForAncestorCheck(path) - if !ok { + clean := filepath.Clean(path) + stop := ancestorStopBoundary(clean) + if stop == "" { return false } - return ancestorChainHasSymlink(filepath.Dir(rel), cache) + return ancestorChainHasSymlink(filepath.Dir(clean), stop, cache) } -// relForAncestorCheck normalises path into a cwd-relative form -// suitable for ancestor probing. It returns (rel, false) to signal -// "skip the check" for paths that are outside cwd or that escape -// via `..`. -func relForAncestorCheck(path string) (string, bool) { - clean := filepath.Clean(path) - if filepath.IsAbs(clean) { - cwd, err := os.Getwd() - if err != nil { - return "", false +// ancestorStopBoundary returns the directory at which ancestor +// probing should stop (exclusive), or "" if the path should not be +// scanned. For relative paths the boundary is "." (so we only walk +// the components the user typed). For absolute paths we prefer +// cwd if the path is under it, falling back to the nearest .git +// project root above the leaf. +func ancestorStopBoundary(clean string) string { + if !filepath.IsAbs(clean) { + if clean == "." || clean == ".." || + strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + return "" } - rel, err := filepath.Rel(cwd, clean) - if err != nil { - return "", false + return "." + } + if cwd, err := os.Getwd(); err == nil { + if rel, err := filepath.Rel(cwd, clean); err == nil && + rel != "." && rel != ".." && + !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return cwd } - clean = rel } - if clean == "." || clean == ".." || - strings.HasPrefix(clean, ".."+string(filepath.Separator)) { - return "", false + return gitProjectRoot(filepath.Dir(clean)) +} + +// gitProjectRoot walks upward from start and returns the first +// directory that contains a `.git` entry (file or directory), or +// "" if none is reached before the filesystem root. +func gitProjectRoot(start string) string { + dir := start + for { + if _, err := os.Lstat(filepath.Join(dir, ".git")); err == nil { + return dir + } + parent := filepath.Dir(dir) + if parent == dir { + return "" + } + dir = parent } - return clean, true } -// ancestorChainHasSymlink walks upward from dir and reports whether -// any step in the chain is a symbolic link. Results are memoised -// per directory so sibling paths under a shared ancestor do not -// re-Lstat the same entries. -func ancestorChainHasSymlink(dir string, cache map[string]bool) bool { - if dir == "." || dir == "/" || - strings.HasPrefix(dir, ".."+string(filepath.Separator)) { +// ancestorChainHasSymlink walks upward from dir up to (but not +// including) stop and reports whether any step in the chain is a +// symbolic link. Results are memoised per directory so sibling +// paths under a shared ancestor do not re-Lstat the same entries. +func ancestorChainHasSymlink(dir, stop string, cache map[string]bool) bool { + if dir == stop || dir == "." || dir == "/" { return false } if v, ok := cache[dir]; ok { @@ -211,8 +233,11 @@ func ancestorChainHasSymlink(dir string, cache map[string]bool) bool { if info, err := os.Lstat(dir); err == nil && info.Mode()&os.ModeSymlink != 0 { result = true - } else if parent := filepath.Dir(dir); parent != dir { - result = ancestorChainHasSymlink(parent, cache) + } else { + parent := filepath.Dir(dir) + if parent != dir { + result = ancestorChainHasSymlink(parent, stop, cache) + } } cache[dir] = result return result From fd686cc2dd0c5c11cf459e069df767c315570305 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 19:03:11 +0000 Subject: [PATCH 12/28] plan 84: rebase fixups and round-8 portability skips MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After rebase onto main: - cmd/mdsmith/main_unit_test.go — a new test file landed on main with resolveOpts tests against the old (cfg, bool, bool) signature. Rewritten for walkCLI and the deprecated-flag force-deny precedence. Round 8 Copilot portability sweep: - cmd/mdsmith/e2e_coverage_test.go — apply the existing skipIfSymlinkUnsupported helper to the three legacy-flag / legacy-config symlink tests (Check/Fix flag + Check config). - internal/discovery/discovery_coverage_test.go — add a package-local skipIfSymlinkUnsupported helper and gate all four tests that create real symlinks (visit + discover, deny + opt-in). - internal/lint/files_test.go — same helper and apply to the three ResolveFilesWithOpts symlink tests. --- cmd/mdsmith/e2e_coverage_test.go | 3 ++ cmd/mdsmith/main_unit_test.go | 29 ++++++++++--------- internal/discovery/discovery_coverage_test.go | 20 +++++++++++++ internal/lint/files_test.go | 19 ++++++++++++ 4 files changed, 57 insertions(+), 14 deletions(-) diff --git a/cmd/mdsmith/e2e_coverage_test.go b/cmd/mdsmith/e2e_coverage_test.go index e974ad3c9..fdd85758a 100644 --- a/cmd/mdsmith/e2e_coverage_test.go +++ b/cmd/mdsmith/e2e_coverage_test.go @@ -145,6 +145,7 @@ func TestE2E_Fix_Discovered_BadConfig_ExitsTwo(t *testing.T) { // "accept silently") and that the new secure default leaves the // real directory reachable. func TestE2E_Check_LegacyNoFollowSymlinksFlag(t *testing.T) { + skipIfSymlinkUnsupported(t) dir := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(dir, ".git"), 0o755)) writeFixture(t, dir, ".mdsmith.yml", @@ -170,6 +171,7 @@ func TestE2E_Check_LegacyNoFollowSymlinksFlag(t *testing.T) { // TestE2E_Fix_LegacyNoFollowSymlinksFlag covers the same silent- // acceptance path for the fix subcommand. func TestE2E_Fix_LegacyNoFollowSymlinksFlag(t *testing.T) { + skipIfSymlinkUnsupported(t) dir := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(dir, ".git"), 0o755)) writeFixture(t, dir, ".mdsmith.yml", @@ -496,6 +498,7 @@ func TestE2E_Query_Verbose_MixedResults(t *testing.T) { // achieves what the legacy key intended, so discovery results match // what the user expects. func TestE2E_Check_LegacyNoFollowSymlinksConfig(t *testing.T) { + skipIfSymlinkUnsupported(t) dir := t.TempDir() writeFixture(t, dir, ".mdsmith.yml", "no-follow-symlinks:\n - \"**\"\nrules:\n no-trailing-spaces: true\n") diff --git a/cmd/mdsmith/main_unit_test.go b/cmd/mdsmith/main_unit_test.go index 4d0df7056..c5c3ed247 100644 --- a/cmd/mdsmith/main_unit_test.go +++ b/cmd/mdsmith/main_unit_test.go @@ -168,35 +168,36 @@ func TestResolveMaxInputBytes_InvalidCLI_Error(t *testing.T) { func TestResolveOpts_BothFalse_GitignoreEnabled(t *testing.T) { cfg := &config.Config{} - opts := resolveOpts(cfg, false, false) + opts := resolveOpts(cfg, walkCLI{}) require.NotNil(t, opts.UseGitignore) assert.True(t, *opts.UseGitignore) - assert.Nil(t, opts.NoFollowSymlinks) + assert.False(t, opts.FollowSymlinks) } func TestResolveOpts_NoGitignore_DisablesFilter(t *testing.T) { cfg := &config.Config{} - opts := resolveOpts(cfg, true, false) + opts := resolveOpts(cfg, walkCLI{noGitignore: true}) require.NotNil(t, opts.UseGitignore) assert.False(t, *opts.UseGitignore) } -func TestResolveOpts_NoFollowSymlinks_SetsGlobStar(t *testing.T) { +func TestResolveOpts_FollowSymlinksFlag_OptsIn(t *testing.T) { cfg := &config.Config{} - opts := resolveOpts(cfg, false, true) - assert.Equal(t, []string{"**"}, opts.NoFollowSymlinks) + opts := resolveOpts(cfg, walkCLI{followSymlinks: true}) + assert.True(t, opts.FollowSymlinks) } -func TestResolveOpts_ConfigNoFollowSymlinks_Propagated(t *testing.T) { - cfg := &config.Config{NoFollowSymlinks: []string{"vendor/**"}} - opts := resolveOpts(cfg, false, false) - assert.Equal(t, []string{"vendor/**"}, opts.NoFollowSymlinks) +func TestResolveOpts_ConfigFollowSymlinks_Propagated(t *testing.T) { + cfg := &config.Config{FollowSymlinks: true} + opts := resolveOpts(cfg, walkCLI{}) + assert.True(t, opts.FollowSymlinks) } -func TestResolveOpts_CLISymlinksOverridesConfig(t *testing.T) { - cfg := &config.Config{NoFollowSymlinks: []string{"vendor/**"}} - opts := resolveOpts(cfg, false, true) - assert.Equal(t, []string{"**"}, opts.NoFollowSymlinks) +func TestResolveOpts_LegacyNoFollowForcesDeny(t *testing.T) { + cfg := &config.Config{FollowSymlinks: true} + opts := resolveOpts(cfg, walkCLI{followSymlinks: true, legacyNoFollow: true}) + assert.False(t, opts.FollowSymlinks, + "deprecated --no-follow-symlinks flag must win over both config and --follow-symlinks") } // --- printRunStats --- diff --git a/internal/discovery/discovery_coverage_test.go b/internal/discovery/discovery_coverage_test.go index bb7a27809..04f271f3f 100644 --- a/internal/discovery/discovery_coverage_test.go +++ b/internal/discovery/discovery_coverage_test.go @@ -28,6 +28,7 @@ func TestVisit_WalkError(t *testing.T) { // Lstat info rather than a synthetic fakeFileInfo so the assertion // reflects actual Walk semantics. func TestVisit_SkipsSymlinkDirByDefault(t *testing.T) { + skipIfSymlinkUnsupported(t) dir := t.TempDir() target := filepath.Join(dir, "real") @@ -54,6 +55,7 @@ func TestVisit_SkipsSymlinkDirByDefault(t *testing.T) { // opt-in case — symlinked directories are still not recursed into, // per the Options.FollowSymlinks doc comment. func TestVisit_FollowsSymlinkFileWhenOptedIn(t *testing.T) { + skipIfSymlinkUnsupported(t) dir := t.TempDir() target := filepath.Join(dir, "real.md") @@ -149,6 +151,7 @@ func TestDiscover_DefaultBaseDir(t *testing.T) { // TestDiscover_SymlinkToFile_SkippedByDefault asserts the secure // default: a symlinked file is not discovered. func TestDiscover_SymlinkToFile_SkippedByDefault(t *testing.T) { + skipIfSymlinkUnsupported(t) dir := t.TempDir() writeFile(t, dir, "real.md", "# Real\n") @@ -168,6 +171,7 @@ func TestDiscover_SymlinkToFile_SkippedByDefault(t *testing.T) { // TestDiscover_FollowSymlinks_OptIn asserts that FollowSymlinks=true // surfaces the symlink as a distinct discovery result. func TestDiscover_FollowSymlinks_OptIn(t *testing.T) { + skipIfSymlinkUnsupported(t) dir := t.TempDir() writeFile(t, dir, "real.md", "# Real\n") @@ -183,3 +187,19 @@ func TestDiscover_FollowSymlinks_OptIn(t *testing.T) { require.NoError(t, err) require.Len(t, files, 2, "both real and linked entries are discovered") } + +// skipIfSymlinkUnsupported skips the calling test when the host +// cannot create symbolic links (e.g. Windows without Developer Mode +// or sandboxed CI). +func skipIfSymlinkUnsupported(t *testing.T) { + t.Helper() + probe := t.TempDir() + target := filepath.Join(probe, "t") + link := filepath.Join(probe, "l") + if err := os.WriteFile(target, nil, 0o644); err != nil { + t.Skipf("cannot create probe file: %v", err) + } + if err := os.Symlink(target, link); err != nil { + t.Skipf("symbolic links not supported on this host: %v", err) + } +} diff --git a/internal/lint/files_test.go b/internal/lint/files_test.go index 6b0412fa9..6e5da1551 100644 --- a/internal/lint/files_test.go +++ b/internal/lint/files_test.go @@ -281,6 +281,7 @@ func TestResolveFilesWithOpts_GitignoreNegation(t *testing.T) { // secure default (FollowSymlinks=false) skips all symlinked files and // symlinked directories during directory walks. func TestResolveFilesWithOpts_SkipsSymlinksByDefault(t *testing.T) { + skipIfSymlinkUnsupported(t) dir := t.TempDir() realFile := filepath.Join(dir, "real.md") @@ -308,6 +309,7 @@ func TestResolveFilesWithOpts_SkipsSymlinksByDefault(t *testing.T) { // FollowSymlinks=true restores the pre-plan-84 behavior of walking // symlinked entries. func TestResolveFilesWithOpts_FollowSymlinks_OptIn(t *testing.T) { + skipIfSymlinkUnsupported(t) dir := t.TempDir() realFile := filepath.Join(dir, "real.md") @@ -336,6 +338,7 @@ func TestResolveFilesWithOpts_FollowSymlinks_OptIn(t *testing.T) { // same default-deny applies when paths come in through a glob // expansion (resolveGlob path) rather than a directory walk. func TestResolveFilesWithOpts_Glob_SkipsSymlinksByDefault(t *testing.T) { + skipIfSymlinkUnsupported(t) dir := t.TempDir() realFile := filepath.Join(dir, "real.md") @@ -354,3 +357,19 @@ func TestResolveFilesWithOpts_Glob_SkipsSymlinksByDefault(t *testing.T) { "glob expansion must skip symlinks by default") } } + +// skipIfSymlinkUnsupported skips the calling test when the host +// cannot create symbolic links (e.g. Windows without Developer Mode +// or sandboxed CI). +func skipIfSymlinkUnsupported(t *testing.T) { + t.Helper() + probe := t.TempDir() + target := filepath.Join(probe, "t") + link := filepath.Join(probe, "l") + if err := os.WriteFile(target, nil, 0o644); err != nil { + t.Skipf("cannot create probe file: %v", err) + } + if err := os.Symlink(target, link); err != nil { + t.Skipf("symbolic links not supported on this host: %v", err) + } +} From 1d17e3b1103948707172fda235832201fae503dc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 19:11:26 +0000 Subject: [PATCH 13/28] =?UTF-8?q?plan=2084:=20address=20ninth=20Copilot=20?= =?UTF-8?q?review=20=E2=80=94=20..-relative=20bypass?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hasSymlinkAncestor treated `..` and `../...` relative paths as out of scope, which let `mdsmith check ../project/linked/dirty.md` reach the external target even when `linked` was a symlinked directory. Fix: resolve the arg to an absolute path first via filepath.Abs, then apply the existing cwd / .git project-root boundary logic uniformly. Relative `../...` paths that escape cwd fall back to the .git boundary of the target tree, just like absolute paths do. Added TestE2E_Symlink_DirSymlinkAncestorDotDotPath: cwd is a sibling directory, the arg is `../project/linked/dirty.md`, and the symlinked component is still rejected. --- cmd/mdsmith/e2e_symlink_default_deny_test.go | 32 +++++++++++++++++++ internal/lint/files.go | 33 ++++++++++---------- 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/cmd/mdsmith/e2e_symlink_default_deny_test.go b/cmd/mdsmith/e2e_symlink_default_deny_test.go index 1559fa221..dd50c6f36 100644 --- a/cmd/mdsmith/e2e_symlink_default_deny_test.go +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -288,6 +288,38 @@ func TestE2E_Symlink_DirSymlinkAncestorAbsPathFromOutsideCwd(t *testing.T) { "skipped even when the invocation cwd is outside the project") } +// TestE2E_Symlink_DirSymlinkAncestorDotDotPath covers the +// `..`-relative variant. An invocation like +// `mdsmith check ../project/linked/dirty.md` from a sibling +// directory must still walk the ancestor chain and reject the +// symlinked `linked` component, because we resolve the arg to an +// absolute path and then anchor at the .git boundary. +func TestE2E_Symlink_DirSymlinkAncestorDotDotPath(t *testing.T) { + skipIfSymlinkUnsupported(t) + root := t.TempDir() + project := filepath.Join(root, "project") + external := filepath.Join(root, "external") + cwd := filepath.Join(root, "cwd") + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + require.NoError(t, os.MkdirAll(external, 0o755)) + require.NoError(t, os.MkdirAll(cwd, 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "rules:\n no-trailing-spaces: true\n") + + require.NoError(t, os.WriteFile(filepath.Join(external, "dirty.md"), + []byte("# Evil\n\ntrailing \n"), 0o644)) + require.NoError(t, os.Symlink(external, filepath.Join(project, "linked"))) + + // cwd sits next to `project`; the arg crosses the symlinked + // component via `..`. + _, _, exitCode := runBinaryInDir(t, cwd, "", "check", + "--no-color", "--no-gitignore", + "../project/linked/dirty.md") + assert.Equal(t, 0, exitCode, + "`..`-relative path through symlinked dir must be skipped") +} + // TestE2E_Symlink_LegacyFlag_OverridesFollowConfig verifies that the // deprecated --no-follow-symlinks flag still has meaning: it forces // deny even when `follow-symlinks: true` is set in config. This lets diff --git a/internal/lint/files.go b/internal/lint/files.go index 5dbb3d77e..cb1d986e1 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -169,36 +169,35 @@ func hasSymlinkAncestor(path string) bool { // per glob match) should share a `cache` across invocations to // avoid repeated `os.Lstat` calls on the same ancestor directories. func hasSymlinkAncestorCached(path string, cache map[string]bool) bool { - clean := filepath.Clean(path) - stop := ancestorStopBoundary(clean) + abs, err := filepath.Abs(path) + if err != nil { + return false + } + abs = filepath.Clean(abs) + stop := ancestorStopBoundary(abs) if stop == "" { return false } - return ancestorChainHasSymlink(filepath.Dir(clean), stop, cache) + return ancestorChainHasSymlink(filepath.Dir(abs), stop, cache) } // ancestorStopBoundary returns the directory at which ancestor // probing should stop (exclusive), or "" if the path should not be -// scanned. For relative paths the boundary is "." (so we only walk -// the components the user typed). For absolute paths we prefer -// cwd if the path is under it, falling back to the nearest .git -// project root above the leaf. -func ancestorStopBoundary(clean string) string { - if !filepath.IsAbs(clean) { - if clean == "." || clean == ".." || - strings.HasPrefix(clean, ".."+string(filepath.Separator)) { - return "" - } - return "." - } +// scanned. Both relative and absolute forms are resolved to +// absolute paths before calling this, so the boundary logic is +// uniform: prefer cwd if the path is under it (cheapest and +// matches user intent), otherwise walk upward for the nearest +// `.git` project root (handles absolute paths to sibling projects +// and `../...` relative paths that escape cwd). +func ancestorStopBoundary(abs string) string { if cwd, err := os.Getwd(); err == nil { - if rel, err := filepath.Rel(cwd, clean); err == nil && + if rel, err := filepath.Rel(cwd, abs); err == nil && rel != "." && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { return cwd } } - return gitProjectRoot(filepath.Dir(clean)) + return gitProjectRoot(filepath.Dir(abs)) } // gitProjectRoot walks upward from start and returns the first From 8fe77d47d5da7cd3092bb73a03154d25bef42f49 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 20:48:52 +0000 Subject: [PATCH 14/28] plan 84: remove --no-follow-symlinks CLI flag outright MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per followup discussion, the deprecated flag is gone now rather than lingering for a release. Rationale: - Polarity flipped with plan 84 — `--follow-symlinks` is the opt-in; keeping the negated sibling adds confusion without giving anything the new default doesn't already deliver. - The force-deny trick (overriding `follow-symlinks: true` in config) is an edge case a user can solve by editing config or passing a different config with `--config`. Changes: - cmd/mdsmith: drop registerLegacyNoFollowSymlinks, the walkCLI.legacyNoFollow field, the `metricsRankOptions` counterpart, and the precedence branch in resolveOpts. - Tests: remove the four e2e tests that exercised the flag (two for check/fix, one for metrics rank, one for force-deny over config). Add TestE2E_Symlink_NoFollowFlagRemoved to confirm the flag now surfaces a parse error (exit 2). - Remove the unit test TestResolveOpts_LegacyNoFollowForcesDeny. - Update cli.md, the security doc, and plan 84 to describe removal instead of deprecation. Deprecated `no-follow-symlinks:` config key still parses with a warning. --- cmd/mdsmith/e2e_coverage_test.go | 70 ------------------- cmd/mdsmith/e2e_symlink_default_deny_test.go | 39 +++-------- cmd/mdsmith/main.go | 28 +------- cmd/mdsmith/main_unit_test.go | 7 -- cmd/mdsmith/metrics.go | 4 -- docs/reference/cli.md | 3 +- .../2026-04-05-adversarial-markdown.md | 2 +- plan/84_symlink-default-deny.md | 10 ++- 8 files changed, 20 insertions(+), 143 deletions(-) diff --git a/cmd/mdsmith/e2e_coverage_test.go b/cmd/mdsmith/e2e_coverage_test.go index fdd85758a..f4f60548d 100644 --- a/cmd/mdsmith/e2e_coverage_test.go +++ b/cmd/mdsmith/e2e_coverage_test.go @@ -136,58 +136,6 @@ func TestE2E_Fix_Discovered_BadConfig_ExitsTwo(t *testing.T) { "expected error in stderr, got: %s", stderr) } -// ============================================================= -// 9. Deprecated --no-follow-symlinks flag is silently accepted -// ============================================================= - -// TestE2E_Check_LegacyNoFollowSymlinksFlag asserts the removed -// `--no-follow-symlinks` flag still parses without error (plan 84 -// "accept silently") and that the new secure default leaves the -// real directory reachable. -func TestE2E_Check_LegacyNoFollowSymlinksFlag(t *testing.T) { - skipIfSymlinkUnsupported(t) - dir := t.TempDir() - require.NoError(t, os.MkdirAll(filepath.Join(dir, ".git"), 0o755)) - writeFixture(t, dir, ".mdsmith.yml", - "files:\n - \"**/*.md\"\nrules:\n no-trailing-spaces: true\n") - - subDir := filepath.Join(dir, "real") - require.NoError(t, os.MkdirAll(subDir, 0o755)) - writeFixture(t, subDir, "dirty.md", "# Title\n\nHello \n") - require.NoError(t, os.Symlink(subDir, filepath.Join(dir, "linked"))) - - // Default-deny already skips the symlink; exit 1 from real/dirty.md. - _, _, exitWithout := runBinaryInDir(t, dir, "", "check", "--no-color", "--no-gitignore") - assert.Equal(t, 1, exitWithout, - "expected exit 1 (dirty real/dirty.md found under default-deny)") - - // Same outcome with the deprecated flag present (flag is silently accepted). - _, stderr, exitCode := runBinaryInDir(t, dir, "", - "check", "--no-color", "--no-gitignore", "--no-follow-symlinks") - assert.Equal(t, 1, exitCode, - "legacy --no-follow-symlinks must still parse; stderr: %s", stderr) -} - -// TestE2E_Fix_LegacyNoFollowSymlinksFlag covers the same silent- -// acceptance path for the fix subcommand. -func TestE2E_Fix_LegacyNoFollowSymlinksFlag(t *testing.T) { - skipIfSymlinkUnsupported(t) - dir := t.TempDir() - require.NoError(t, os.MkdirAll(filepath.Join(dir, ".git"), 0o755)) - writeFixture(t, dir, ".mdsmith.yml", - "files:\n - \"**/*.md\"\nrules:\n no-trailing-spaces: true\n") - - subDir := filepath.Join(dir, "real") - require.NoError(t, os.MkdirAll(subDir, 0o755)) - writeFixture(t, subDir, "fixme.md", "# Title\n\nHello \n") - require.NoError(t, os.Symlink(subDir, filepath.Join(dir, "linked"))) - - _, stderr, exitCode := runBinaryInDir(t, dir, "", - "fix", "--no-color", "--no-follow-symlinks") - assert.Equal(t, 0, exitCode, - "expected exit 0 after fix, got %d; stderr: %s", exitCode, stderr) -} - // ============================================================= // 10. rootDirFromConfig — empty cfgPath falls back to cwd // ============================================================= @@ -693,24 +641,6 @@ func TestE2E_Init_HelpFlag(t *testing.T) { "expected init usage, got: %s", stderr) } -// ============================================================= -// Additional: metrics rank still accepts deprecated flag -// ============================================================= - -// TestE2E_MetricsRank_LegacyNoFollowSymlinksFlag asserts the -// deprecated --no-follow-symlinks flag is silently accepted by the -// metrics rank subcommand after plan 84. -func TestE2E_MetricsRank_LegacyNoFollowSymlinksFlag(t *testing.T) { - dir := t.TempDir() - writeFixture(t, dir, "a.md", "# Title\n\nSome content here.\n") - - stdout, stderr, exitCode := runBinaryInDir(t, dir, "", - "metrics", "rank", "--by", "bytes", "--no-follow-symlinks", ".") - require.Equal(t, 0, exitCode, - "expected exit 0, got %d; stderr: %s", exitCode, stderr) - assert.Contains(t, stdout, "a.md", "expected a.md in output") -} - // ============================================================= // Additional: metrics rank with --no-gitignore // ============================================================= diff --git a/cmd/mdsmith/e2e_symlink_default_deny_test.go b/cmd/mdsmith/e2e_symlink_default_deny_test.go index dd50c6f36..b1ddc2750 100644 --- a/cmd/mdsmith/e2e_symlink_default_deny_test.go +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -320,38 +320,15 @@ func TestE2E_Symlink_DirSymlinkAncestorDotDotPath(t *testing.T) { "`..`-relative path through symlinked dir must be skipped") } -// TestE2E_Symlink_LegacyFlag_OverridesFollowConfig verifies that the -// deprecated --no-follow-symlinks flag still has meaning: it forces -// deny even when `follow-symlinks: true` is set in config. This lets -// users run one-off secure invocations without editing the config. -func TestE2E_Symlink_LegacyFlag_OverridesFollowConfig(t *testing.T) { - skipIfSymlinkUnsupported(t) - project := t.TempDir() - external := t.TempDir() - - require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) - writeFixture(t, project, ".mdsmith.yml", - "follow-symlinks: true\nrules:\n no-trailing-spaces: true\n") - - externalFile := filepath.Join(external, "dirty.md") - require.NoError(t, os.WriteFile(externalFile, - []byte("# Evil\n\ntrailing \n"), 0o644)) - require.NoError(t, os.Symlink(externalFile, - filepath.Join(project, "linked.md"))) - - // Baseline: with follow-symlinks:true in config, the linked - // file is walked and the dirty line surfaces. - _, _, withConfig := runBinaryInDir(t, project, "", "check", - "--no-color", "--no-gitignore", ".") - require.Equal(t, 1, withConfig, - "follow-symlinks: true config must expose the linked file") - - // Legacy --no-follow-symlinks overrides the config and forces - // deny for this invocation; no findings. - _, _, withLegacy := runBinaryInDir(t, project, "", "check", +// TestE2E_Symlink_NoFollowFlagRemoved asserts that the deprecated +// `--no-follow-symlinks` CLI flag has been removed and now errors +// out as an unknown flag. +func TestE2E_Symlink_NoFollowFlagRemoved(t *testing.T) { + dir := t.TempDir() + _, _, exitCode := runBinaryInDir(t, dir, "", "check", "--no-color", "--no-gitignore", "--no-follow-symlinks", ".") - assert.Equal(t, 0, withLegacy, - "legacy --no-follow-symlinks must force deny over config opt-in") + assert.Equal(t, 2, exitCode, + "removed --no-follow-symlinks flag must surface as a parse error") } // TestE2E_Symlink_LegacyNoFollowConfig_Deprecation verifies that the diff --git a/cmd/mdsmith/main.go b/cmd/mdsmith/main.go index 35e5e7616..e213fdcb3 100644 --- a/cmd/mdsmith/main.go +++ b/cmd/mdsmith/main.go @@ -171,7 +171,6 @@ func runCheck(args []string) int { fs.BoolVarP(&verbose, "verbose", "v", false, "Show config, files, and rules on stderr") fs.BoolVar(&noGitignore, "no-gitignore", false, "Disable .gitignore filtering when walking directories") fs.BoolVar(&followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") - legacyNoFollow := registerLegacyNoFollowSymlinks(fs) fs.StringVar(&maxInputSize, "max-input-size", "", "Maximum file size to process (e.g. 2MB, 500KB, 0=unlimited)") fs.Usage = func() { @@ -196,7 +195,6 @@ func runCheck(args []string) int { walk := walkCLI{ noGitignore: noGitignore, followSymlinks: followSymlinks, - legacyNoFollow: *legacyNoFollow, } allArgs := fs.Args() @@ -237,7 +235,6 @@ func runFix(args []string) int { fs.BoolVarP(&verbose, "verbose", "v", false, "Show config, files, and rules on stderr") fs.BoolVar(&noGitignore, "no-gitignore", false, "Disable .gitignore filtering when walking directories") fs.BoolVar(&followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") - legacyNoFollow := registerLegacyNoFollowSymlinks(fs) fs.StringVar(&maxInputSize, "max-input-size", "", "Maximum file size to process (e.g. 2MB, 500KB, 0=unlimited)") fs.Usage = func() { @@ -262,7 +259,6 @@ func runFix(args []string) int { walk := walkCLI{ noGitignore: noGitignore, followSymlinks: followSymlinks, - legacyNoFollow: *legacyNoFollow, } allArgs := fs.Args() @@ -885,35 +881,20 @@ func fixDiscovered( return 0 } -// registerLegacyNoFollowSymlinks registers the removed -// `--no-follow-symlinks` flag as a silently-accepted deprecation -// and returns a pointer to its parsed value. When set, the caller -// should force `FollowSymlinks` off regardless of config / other -// flags, so existing invocations remain a safe way to get the -// secure default without editing the config file. -func registerLegacyNoFollowSymlinks(fs *flag.FlagSet) *bool { - return fs.Bool("no-follow-symlinks", false, - "Deprecated: symlinks are now skipped by default") -} - // walkCLI bundles the CLI flags that affect how files are // discovered and resolved, so helpers can thread one value -// instead of three (and the next addition isn't a parameter +// instead of two (and the next addition isn't a parameter // explosion). type walkCLI struct { noGitignore bool followSymlinks bool - legacyNoFollow bool } // frontMatterEnabled returns whether front matter stripping is enabled. // Defaults to true if not set in config. // resolveOpts builds ResolveOpts from config and CLI flags. -// CLI precedence for symlinks (highest wins): the deprecated -// --no-follow-symlinks flag forces deny, then --follow-symlinks -// opts in, then the `follow-symlinks:` config key. The deprecated -// flag lets users run in secure mode without editing an existing -// config that enables symlink following. +// The `--follow-symlinks` CLI flag overrides `follow-symlinks:` +// from config; otherwise the config value stands. func resolveOpts(cfg *config.Config, walk walkCLI) lint.ResolveOpts { useGitignore := !walk.noGitignore opts := lint.ResolveOpts{ @@ -923,9 +904,6 @@ func resolveOpts(cfg *config.Config, walk walkCLI) lint.ResolveOpts { if walk.followSymlinks { opts.FollowSymlinks = true } - if walk.legacyNoFollow { - opts.FollowSymlinks = false - } return opts } diff --git a/cmd/mdsmith/main_unit_test.go b/cmd/mdsmith/main_unit_test.go index c5c3ed247..41869d862 100644 --- a/cmd/mdsmith/main_unit_test.go +++ b/cmd/mdsmith/main_unit_test.go @@ -193,13 +193,6 @@ func TestResolveOpts_ConfigFollowSymlinks_Propagated(t *testing.T) { assert.True(t, opts.FollowSymlinks) } -func TestResolveOpts_LegacyNoFollowForcesDeny(t *testing.T) { - cfg := &config.Config{FollowSymlinks: true} - opts := resolveOpts(cfg, walkCLI{followSymlinks: true, legacyNoFollow: true}) - assert.False(t, opts.FollowSymlinks, - "deprecated --no-follow-symlinks flag must win over both config and --follow-symlinks") -} - // --- printRunStats --- func TestPrintRunStats_NormalOutputContainsAllFields(t *testing.T) { diff --git a/cmd/mdsmith/metrics.go b/cmd/mdsmith/metrics.go index e5aa3aa9c..66bad6644 100644 --- a/cmd/mdsmith/metrics.go +++ b/cmd/mdsmith/metrics.go @@ -100,7 +100,6 @@ type metricsRankOptions struct { format string noGitignore bool followSymlinks bool - legacyNoFollow bool maxInputSize string } @@ -125,7 +124,6 @@ func parseMetricsRankOptions(args []string) (metricsRankOptions, []string, error fs.StringVarP(&opts.format, "format", "f", "text", "Output format: text, json") fs.BoolVar(&opts.noGitignore, "no-gitignore", false, "Disable .gitignore filtering when walking directories") fs.BoolVar(&opts.followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") - legacyNoFollow := registerLegacyNoFollowSymlinks(fs) fs.StringVar(&opts.maxInputSize, "max-input-size", "", "Maximum file size to process (e.g. 2MB, 500KB, 0=unlimited)") @@ -146,7 +144,6 @@ func parseMetricsRankOptions(args []string) (metricsRankOptions, []string, error if opts.top < 0 { return metricsRankOptions{}, nil, fmt.Errorf("--top must be >= 0") } - opts.legacyNoFollow = *legacyNoFollow fileArgs := fs.Args() if len(fileArgs) == 0 { @@ -249,7 +246,6 @@ func resolveRankFiles(cfg *config.Config, opts metricsRankOptions, fileArgs []st resolveOptions := resolveOpts(cfg, walkCLI{ noGitignore: opts.noGitignore, followSymlinks: opts.followSymlinks, - legacyNoFollow: opts.legacyNoFollow, }) return lint.ResolveFilesWithOpts(fileArgs, resolveOptions) } diff --git a/docs/reference/cli.md b/docs/reference/cli.md index dbdb2184c..279e57a85 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -56,8 +56,7 @@ Pass `--follow-symlinks` to opt in on the command line. Set `follow-symlinks: true` in `.mdsmith.yml` to opt in from config. -The old `--no-follow-symlinks` flag parses silently. -The old `no-follow-symlinks:` config key parses too, and +The old `no-follow-symlinks:` config key still parses and emits a deprecation warning on stderr. ## Other Subcommand Flags diff --git a/docs/security/2026-04-05-adversarial-markdown.md b/docs/security/2026-04-05-adversarial-markdown.md index f8343c3df..4578bf501 100644 --- a/docs/security/2026-04-05-adversarial-markdown.md +++ b/docs/security/2026-04-05-adversarial-markdown.md @@ -123,7 +123,7 @@ Escape sequences pass through to the terminal: screen clearing (`\033[2J`), wind **Original finding:** symlink following was the default; the `--no-follow-symlinks` flag and config key were opt-in. -**Status (plan 84, resolved):** the default is inverted. Symlinks are skipped by default across directory walks, glob expansion, and explicit non-glob path arguments. Symlinked directories are always skipped — including when a path or glob traverses through one — regardless of `FollowSymlinks`. Users opt in (for file symlinks) with `--follow-symlinks` or `follow-symlinks: true`. The legacy `--no-follow-symlinks` flag and `no-follow-symlinks:` config key are silently accepted for one release; the config key emits a deprecation warning. +**Status (plan 84, resolved):** the default is inverted. Symlinks are skipped by default across directory walks, glob expansion, and explicit non-glob path arguments. Symlinked directories are always skipped — including when a path or glob traverses through one — regardless of `FollowSymlinks`. Users opt in (for file symlinks) with `--follow-symlinks` or `follow-symlinks: true`. The old `--no-follow-symlinks` flag has been removed; the legacy `no-follow-symlinks:` config key still parses and emits a deprecation warning. **Adversarial file:** diff --git a/plan/84_symlink-default-deny.md b/plan/84_symlink-default-deny.md index 740d4f721..9e808a243 100644 --- a/plan/84_symlink-default-deny.md +++ b/plan/84_symlink-default-deny.md @@ -63,8 +63,12 @@ follow-symlinks: true # opt-in, default false - Deprecate `no-follow-symlinks` config key: if present, emit a warning suggesting migration to the new `follow-symlinks: false` default. -- Deprecate `--no-follow-symlinks` CLI flag: accept - silently (it's now the default). +- Remove `--no-follow-symlinks` CLI flag outright: + the polarity flipped (`--follow-symlinks` is the + new opt-in) and keeping a negated sibling is both + redundant with the new secure default and confusing + next to it. Passing the removed flag errors out on + parse (exit 2), same as any other unknown flag. ### Write-side protection @@ -87,7 +91,7 @@ Consider migrating from `filepath.Walk` to `FollowSymlinks bool` in `config.Config`; kept `LegacyNoFollowSymlinks` for deprecation parsing 2. [x] Replaced `--no-follow-symlinks` with - `--follow-symlinks`; old flag silently accepted + `--follow-symlinks`; old flag removed outright 3. [x] Updated `ResolveOpts` to use `FollowSymlinks bool` 4. [x] Updated `walkDir` to skip symlinks by default 5. [x] Updated `resolveGlob` to skip symlinks by default From 1d31511b9fca1507c496c87502bad83c7160c5e9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Apr 2026 04:18:53 +0000 Subject: [PATCH 15/28] plan 84: tri-state --follow-symlinks + tighten test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real concerns from Copilot's latest round: 1. --follow-symlinks now had no override against a config that sets `follow-symlinks: true`. Without --no-follow-symlinks, users couldn't run a one-off secure invocation. Make the flag tri-state via fs.Changed: walkCLI.followSymlinks is *bool, nil means "fall back to config", non-nil overrides either way. So --follow-symlinks=false now forces deny over config, replacing the deprecated flag's role. 2. TestResolveFilesWithOpts_Glob_SkipsSymlinksByDefault only asserted that no result was named link.md — an empty slice would have passed silently. Tightened to require length 2 and assert real.md and target.md are present. Also added: - TestResolveOpts_ExplicitFalseFlag_OverridesConfigOptIn unit test - TestE2E_Symlink_FollowFalseFlag_OverridesConfigOptIn e2e test --- cmd/mdsmith/e2e_symlink_default_deny_test.go | 36 +++++++++++++++++++ cmd/mdsmith/main.go | 38 ++++++++++++++------ cmd/mdsmith/main_unit_test.go | 11 +++++- cmd/mdsmith/metrics.go | 6 ++-- internal/lint/files_test.go | 9 ++--- 5 files changed, 82 insertions(+), 18 deletions(-) diff --git a/cmd/mdsmith/e2e_symlink_default_deny_test.go b/cmd/mdsmith/e2e_symlink_default_deny_test.go index b1ddc2750..5487f5db4 100644 --- a/cmd/mdsmith/e2e_symlink_default_deny_test.go +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -331,6 +331,42 @@ func TestE2E_Symlink_NoFollowFlagRemoved(t *testing.T) { "removed --no-follow-symlinks flag must surface as a parse error") } +// TestE2E_Symlink_FollowFalseFlag_OverridesConfigOptIn asserts the +// tri-state semantic of `--follow-symlinks`: an explicit +// `--follow-symlinks=false` forces deny even when +// `follow-symlinks: true` is set in the loaded config. This is the +// secure-one-off-run knob that replaces the deprecated +// `--no-follow-symlinks`. +func TestE2E_Symlink_FollowFalseFlag_OverridesConfigOptIn(t *testing.T) { + skipIfSymlinkUnsupported(t) + project := t.TempDir() + external := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "follow-symlinks: true\nrules:\n no-trailing-spaces: true\n") + + externalFile := filepath.Join(external, "dirty.md") + require.NoError(t, os.WriteFile(externalFile, + []byte("# Evil\n\ntrailing \n"), 0o644)) + require.NoError(t, os.Symlink(externalFile, + filepath.Join(project, "linked.md"))) + + // Baseline: with follow-symlinks: true in config, the linked + // file is walked and the dirty line surfaces. + _, _, withConfig := runBinaryInDir(t, project, "", "check", + "--no-color", "--no-gitignore", ".") + require.Equal(t, 1, withConfig, + "follow-symlinks: true config must expose the linked file") + + // Override: --follow-symlinks=false explicitly forces deny for + // this invocation. + _, _, withOverride := runBinaryInDir(t, project, "", "check", + "--no-color", "--no-gitignore", "--follow-symlinks=false", ".") + assert.Equal(t, 0, withOverride, + "--follow-symlinks=false must force deny over a config opt-in") +} + // TestE2E_Symlink_LegacyNoFollowConfig_Deprecation verifies that the // old `no-follow-symlinks:` key still parses and emits a deprecation // warning on stderr. diff --git a/cmd/mdsmith/main.go b/cmd/mdsmith/main.go index e213fdcb3..3b40eb7a4 100644 --- a/cmd/mdsmith/main.go +++ b/cmd/mdsmith/main.go @@ -194,7 +194,7 @@ func runCheck(args []string) int { walk := walkCLI{ noGitignore: noGitignore, - followSymlinks: followSymlinks, + followSymlinks: followSymlinksOverride(fs, followSymlinks), } allArgs := fs.Args() @@ -258,7 +258,7 @@ func runFix(args []string) int { walk := walkCLI{ noGitignore: noGitignore, - followSymlinks: followSymlinks, + followSymlinks: followSymlinksOverride(fs, followSymlinks), } allArgs := fs.Args() @@ -883,28 +883,44 @@ func fixDiscovered( // walkCLI bundles the CLI flags that affect how files are // discovered and resolved, so helpers can thread one value -// instead of two (and the next addition isn't a parameter -// explosion). +// instead of several (and the next addition isn't a parameter +// explosion). followSymlinks is tri-state: nil means "fall back +// to cfg.FollowSymlinks"; non-nil overrides config either way, +// so users can write `--follow-symlinks=false` to force the +// secure default for a one-off run against a config that opts +// in. type walkCLI struct { noGitignore bool - followSymlinks bool + followSymlinks *bool } // frontMatterEnabled returns whether front matter stripping is enabled. // Defaults to true if not set in config. // resolveOpts builds ResolveOpts from config and CLI flags. // The `--follow-symlinks` CLI flag overrides `follow-symlinks:` -// from config; otherwise the config value stands. +// from config when explicitly set; otherwise the config value +// stands. func resolveOpts(cfg *config.Config, walk walkCLI) lint.ResolveOpts { useGitignore := !walk.noGitignore - opts := lint.ResolveOpts{ + follow := cfg.FollowSymlinks + if walk.followSymlinks != nil { + follow = *walk.followSymlinks + } + return lint.ResolveOpts{ UseGitignore: &useGitignore, - FollowSymlinks: cfg.FollowSymlinks, + FollowSymlinks: follow, } - if walk.followSymlinks { - opts.FollowSymlinks = true +} + +// followSymlinksOverride returns a *bool override for the +// `--follow-symlinks` flag if it was explicitly set on the +// command line, or nil to defer to config. +func followSymlinksOverride(fs *flag.FlagSet, value bool) *bool { + if fs.Changed("follow-symlinks") { + v := value + return &v } - return opts + return nil } // resolveMaxInputBytes returns the effective max-input-size in bytes. diff --git a/cmd/mdsmith/main_unit_test.go b/cmd/mdsmith/main_unit_test.go index 41869d862..df671f534 100644 --- a/cmd/mdsmith/main_unit_test.go +++ b/cmd/mdsmith/main_unit_test.go @@ -183,7 +183,8 @@ func TestResolveOpts_NoGitignore_DisablesFilter(t *testing.T) { func TestResolveOpts_FollowSymlinksFlag_OptsIn(t *testing.T) { cfg := &config.Config{} - opts := resolveOpts(cfg, walkCLI{followSymlinks: true}) + yes := true + opts := resolveOpts(cfg, walkCLI{followSymlinks: &yes}) assert.True(t, opts.FollowSymlinks) } @@ -193,6 +194,14 @@ func TestResolveOpts_ConfigFollowSymlinks_Propagated(t *testing.T) { assert.True(t, opts.FollowSymlinks) } +func TestResolveOpts_ExplicitFalseFlag_OverridesConfigOptIn(t *testing.T) { + cfg := &config.Config{FollowSymlinks: true} + no := false + opts := resolveOpts(cfg, walkCLI{followSymlinks: &no}) + assert.False(t, opts.FollowSymlinks, + "--follow-symlinks=false must force deny over a config opt-in") +} + // --- printRunStats --- func TestPrintRunStats_NormalOutputContainsAllFields(t *testing.T) { diff --git a/cmd/mdsmith/metrics.go b/cmd/mdsmith/metrics.go index 66bad6644..d4ff34110 100644 --- a/cmd/mdsmith/metrics.go +++ b/cmd/mdsmith/metrics.go @@ -99,7 +99,7 @@ type metricsRankOptions struct { top int format string noGitignore bool - followSymlinks bool + followSymlinks *bool maxInputSize string } @@ -115,6 +115,7 @@ func runMetricsRank(args []string) int { func parseMetricsRankOptions(args []string) (metricsRankOptions, []string, error) { fs := flag.NewFlagSet("metrics rank", flag.ContinueOnError) var opts metricsRankOptions + var followSymlinks bool fs.StringVarP(&opts.configPath, "config", "c", "", "Override config file path") fs.StringVar(&opts.metricsRaw, "metrics", "", "Comma-separated metrics (defaults to registry defaults)") @@ -123,7 +124,7 @@ func parseMetricsRankOptions(args []string) (metricsRankOptions, []string, error fs.IntVar(&opts.top, "top", 0, "Limit results to top N files (0 = all)") fs.StringVarP(&opts.format, "format", "f", "text", "Output format: text, json") fs.BoolVar(&opts.noGitignore, "no-gitignore", false, "Disable .gitignore filtering when walking directories") - fs.BoolVar(&opts.followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") + fs.BoolVar(&followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") fs.StringVar(&opts.maxInputSize, "max-input-size", "", "Maximum file size to process (e.g. 2MB, 500KB, 0=unlimited)") @@ -144,6 +145,7 @@ func parseMetricsRankOptions(args []string) (metricsRankOptions, []string, error if opts.top < 0 { return metricsRankOptions{}, nil, fmt.Errorf("--top must be >= 0") } + opts.followSymlinks = followSymlinksOverride(fs, followSymlinks) fileArgs := fs.Args() if len(fileArgs) == 0 { diff --git a/internal/lint/files_test.go b/internal/lint/files_test.go index 6e5da1551..ca17b6bb3 100644 --- a/internal/lint/files_test.go +++ b/internal/lint/files_test.go @@ -352,10 +352,11 @@ func TestResolveFilesWithOpts_Glob_SkipsSymlinksByDefault(t *testing.T) { pattern := filepath.Join(dir, "*.md") files, err := ResolveFilesWithOpts([]string{pattern}, DefaultResolveOpts()) require.NoError(t, err) - for _, f := range files { - assert.NotEqual(t, "link.md", filepath.Base(f), - "glob expansion must skip symlinks by default") - } + require.Len(t, files, 2, + "expected only the two real markdown files, got %v", files) + bases := []string{filepath.Base(files[0]), filepath.Base(files[1])} + assert.ElementsMatch(t, []string{"real.md", "target.md"}, bases, + "glob expansion must yield real.md and target.md and skip link.md") } // skipIfSymlinkUnsupported skips the calling test when the host From fbc75a87fa3acaf4602a7f1fe11a5fb5b3fe9dd1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Apr 2026 04:28:30 +0000 Subject: [PATCH 16/28] plan 84: require regular-file target + document tri-state flag Three Copilot points: 1/2. Opt-in symlink handling in both lint and discovery only filtered out directory targets. A symlink to a FIFO, device, or socket would still be queued and could block or fail at read time. Tightened the check: symlinks are followed only when the target `os.Stat` reports a regular file. Applied in resolveArg, resolveGlob, lint.walkDir, and discovery walker. The FollowSymlinks doc comments on ResolveOpts and discovery.Options now state "resolves to a regular file" explicitly. 3. docs/reference/cli.md: the tri-state semantics of `--follow-symlinks` weren't documented. Added a list distinguishing omitted (defer to config), true (opt in for this run), and false (force deny, replacing the removed `--no-follow-symlinks`). --- docs/reference/cli.md | 17 +++++++++---- internal/discovery/discovery.go | 22 +++++++++-------- internal/lint/files.go | 43 ++++++++++++++++++--------------- 3 files changed, 47 insertions(+), 35 deletions(-) diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 279e57a85..cf872ec10 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -50,11 +50,18 @@ symlink from redirecting `check` or `fix` to files outside the project. The rule applies to directory walks, glob expansion, and explicit file or directory arguments: `mdsmith check ./linked.md` silently skips a symlink named -on the command line. - -Pass `--follow-symlinks` to opt in on the command line. -Set `follow-symlinks: true` in `.mdsmith.yml` to opt in -from config. +on the command line. Opt-in follows only symlinks that +resolve to regular files; directory / FIFO / device / socket +targets are always skipped. + +`--follow-symlinks` is tri-state: + +- omitted — fall back to `follow-symlinks:` in + `.mdsmith.yml` (default: skip) +- `--follow-symlinks` or `--follow-symlinks=true` — opt in + for this run +- `--follow-symlinks=false` — force deny for this run, even + when the loaded config has `follow-symlinks: true` The old `no-follow-symlinks:` config key still parses and emits a deprecation warning on stderr. diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go index 40b4fe366..ed18328db 100644 --- a/internal/discovery/discovery.go +++ b/internal/discovery/discovery.go @@ -22,13 +22,14 @@ type Options struct { // UseGitignore enables filtering by .gitignore rules. UseGitignore bool - // FollowSymlinks opts in to reading symlinked file entries during - // the walk. The zero value skips all symlinks, which is the secure - // default. + // FollowSymlinks opts in to including symlinks that resolve + // to regular files. The zero value skips all symlinks, which + // is the secure default. // - // filepath.Walk is Lstat-based, so a symlinked directory is never - // descended into even when this flag is true — it only controls - // whether symlinked file entries are included in results. + // Symlinks resolving to anything other than a regular file + // (directories, FIFOs, devices, sockets) are always skipped. + // filepath.Walk is Lstat-based, so symlinked directories are + // never descended into regardless of this flag. FollowSymlinks bool } @@ -116,10 +117,11 @@ func (w *walker) visit(path string, info os.FileInfo, walkErr error) error { return nil } // In opt-in mode, include the entry only if it resolves to - // a regular file. A symlink-to-dir named like "evil.md" - // would otherwise match patterns and land in results — - // the Options doc says symlinked directories are skipped. - if tgt, statErr := os.Stat(path); statErr != nil || tgt.IsDir() { + // a regular file. Directory targets are skipped (Options + // doc); FIFO/device/socket targets are skipped to avoid + // blocking reads during linting. + if tgt, statErr := os.Stat(path); statErr != nil || + !tgt.Mode().IsRegular() { return nil } } diff --git a/internal/lint/files.go b/internal/lint/files.go index cb1d986e1..b9ef666a6 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -28,15 +28,15 @@ type ResolveOpts struct { // used (see DefaultResolveOpts). UseGitignore *bool - // FollowSymlinks opts in to following symbolic links that resolve - // to files. The zero value skips all symlinks, which is the secure - // default. + // FollowSymlinks opts in to following symbolic links that + // resolve to regular files. The zero value skips all symlinks, + // which is the secure default. // - // Symlinked directories are always skipped, regardless of this - // flag. `filepath.Walk` is Lstat-based and does not descend into - // a symlink root, so supporting symlinked-directory traversal - // would require explicit EvalSymlinks resolution plus atomic - // writes against an unknown path — out of scope for plan 84. + // Symlinks that resolve to anything other than a regular file + // are always skipped, regardless of this flag: directories + // (filepath.Walk is Lstat-based and does not descend a symlink + // root) as well as FIFOs, devices, and sockets (reading them + // during linting could block or fail unexpectedly). FollowSymlinks bool } @@ -125,11 +125,12 @@ func resolveArg(arg string, opts ResolveOpts, addFile func(string)) error { return fmt.Errorf("cannot access %q: %w", arg, err) } - // Symlinks to directories are always skipped. filepath.Walk is - // Lstat-based and cannot recurse into a symlink root, so walking - // a symlinked dir would silently yield no files and confuse - // callers. `--follow-symlinks` applies to file symlinks only. - if isSymlink && info.IsDir() { + // Symlinks are followed only when they resolve to a regular + // file. Directory targets are skipped (filepath.Walk is + // Lstat-based and cannot recurse into a symlink root); device, + // FIFO, and socket targets are skipped to avoid blocking reads + // later. `--follow-symlinks` applies to file symlinks only. + if isSymlink && !info.Mode().IsRegular() { return nil } @@ -272,8 +273,9 @@ func resolveGlob(pattern string, opts ResolveOpts, addFile func(string)) error { if err != nil { continue } - // Symlinks to directories are always skipped (see resolveArg). - if isSymlink && info.IsDir() { + // Symlinks are followed only when they resolve to a regular + // file; see resolveArg for rationale. + if isSymlink && !info.Mode().IsRegular() { continue } if info.IsDir() { @@ -324,11 +326,12 @@ func walkDir(dir string, useGitignore, followSymlinks bool) ([]string, error) { return nil } // In opt-in mode, follow the link only if it resolves to - // a regular file. A symlink-to-dir named "evil.md" would - // otherwise be treated as a markdown file and fail on - // read; this also matches the FollowSymlinks doc that - // says symlinked directories are always skipped. - if tgt, statErr := os.Stat(path); statErr != nil || tgt.IsDir() { + // a regular file. Directory targets would be silently + // treated as an empty walk; FIFO/device/socket targets + // would block or fail later on read. `--follow-symlinks` + // applies to file symlinks only. + if tgt, statErr := os.Stat(path); statErr != nil || + !tgt.Mode().IsRegular() { return nil } } From 3d56f6dee161d8712d97436682ec24283d86b9d4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Apr 2026 04:38:44 +0000 Subject: [PATCH 17/28] plan 84: consistent broken-symlink skip + symlinked-dir in test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Copilot points: 1. resolveArg returned an error when os.Stat on a symlink target failed (broken or inaccessible link), while walkDir, resolveGlob, and the discovery walker silently skipped in the same situation. The FollowSymlinks contract says symlinks are included only when the target resolves to a regular file — unreachable targets fall under that filter. Aligned resolveArg to skip silently for symlinks; non-symlink paths still error. 2. TestResolveFilesWithOpts_SkipsSymlinksByDefault claimed to cover symlinked directories but only created a symlinked file. Added a symlinked-dir fixture (linked-dir -> target/) and an assertion that no result path lives under it. --- internal/lint/files.go | 7 +++++++ internal/lint/files_test.go | 25 +++++++++++++++++++------ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/internal/lint/files.go b/internal/lint/files.go index b9ef666a6..a50aa44a4 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -122,6 +122,13 @@ func resolveArg(arg string, opts ResolveOpts, addFile func(string)) error { info, err := os.Stat(arg) if err != nil { + // Broken or inaccessible symlink targets are silently + // skipped to match walkDir / resolveGlob / discovery + // behavior and the FollowSymlinks contract. For a + // non-symlink, the missing target is a real error. + if isSymlink { + return nil + } return fmt.Errorf("cannot access %q: %w", arg, err) } diff --git a/internal/lint/files_test.go b/internal/lint/files_test.go index ca17b6bb3..9f4fb7d18 100644 --- a/internal/lint/files_test.go +++ b/internal/lint/files_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "sort" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -278,8 +279,8 @@ func TestResolveFilesWithOpts_GitignoreNegation(t *testing.T) { // --- FollowSymlinks tests --- // TestResolveFilesWithOpts_SkipsSymlinksByDefault asserts that the -// secure default (FollowSymlinks=false) skips all symlinked files and -// symlinked directories during directory walks. +// secure default (FollowSymlinks=false) skips both symlinked files +// and symlinked directories encountered during the walk. func TestResolveFilesWithOpts_SkipsSymlinksByDefault(t *testing.T) { skipIfSymlinkUnsupported(t) dir := t.TempDir() @@ -287,21 +288,33 @@ func TestResolveFilesWithOpts_SkipsSymlinksByDefault(t *testing.T) { realFile := filepath.Join(dir, "real.md") require.NoError(t, os.WriteFile(realFile, []byte("# Real"), 0o644)) + // Target for the file-symlink case. subDir := filepath.Join(dir, "target") require.NoError(t, os.MkdirAll(subDir, 0o755)) targetFile := filepath.Join(subDir, "doc.md") require.NoError(t, os.WriteFile(targetFile, []byte("# Target"), 0o644)) + // Symlinked file (link.md -> target/doc.md). linkFile := filepath.Join(dir, "link.md") require.NoError(t, os.Symlink(targetFile, linkFile)) - // Default: only the real files are walked; symlink is skipped. + // Symlinked directory (linked-dir -> target/). A dirty markdown + // file inside the target would surface if the walker descended. + require.NoError(t, os.Symlink(subDir, filepath.Join(dir, "linked-dir"))) + + // Default: only the real files under target/ are walked; both + // symlinked entries are skipped. files, err := ResolveFilesWithOpts([]string{dir}, DefaultResolveOpts()) require.NoError(t, err) - require.Len(t, files, 2) + require.Len(t, files, 2, + "expected only real.md and target/doc.md, got %v", files) for _, f := range files { - assert.NotEqual(t, "link.md", filepath.Base(f), - "symlinked link.md must be skipped by default") + base := filepath.Base(f) + assert.NotEqual(t, "link.md", base, + "symlinked file must be skipped by default") + assert.False(t, + strings.Contains(filepath.ToSlash(f), "/linked-dir/"), + "symlinked directory must not be descended") } } From 2751cf2aaac81ce79baa35b828fcd29ed7fd77cb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Apr 2026 04:50:10 +0000 Subject: [PATCH 18/28] plan 84: address Copilot round 13 + codecov gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review comments plus the codecov/patch gate (87.28% vs 88.03% target). 1. Precompute cwd once per glob expansion. Added hasSymlinkAncestorWithCwd and refactored ancestorStopBoundary to take cwd as a parameter. resolveGlob reads os.Getwd once up front and reuses it for every match; each ancestor dir is still Lstat'd at most once via the shared cache. hasSymlinkAncestorCached keeps the convenience shape for one- shot callers. 2. Extracted skipIfSymlinkUnsupported into internal/testutil. The three callers (internal/lint, internal/discovery, cmd/mdsmith) now forward through a tiny local wrapper so the probing logic lives in exactly one place. 3. docs/reference/cli.md: the `--follow-symlinks` default column said `false`, which was misleading under the tri-state semantic. Changed to `config` with a pointer to the detailed explanation below the table. Plus closure on the codecov/patch gate: added focused unit tests for the plan 84 helpers that the e2e suite didn't fully exercise — - TestHasSymlinkAncestor_RelativeUnderCwd - TestHasSymlinkAncestor_CacheReusedAcrossSiblings - TestHasSymlinkAncestor_AbsPathOutsideCwdWithGitRoot - TestHasSymlinkAncestor_SkipsPathOutsideProjects - TestGitProjectRoot_FindsAncestor / _NoAncestor - TestAncestorChainHasSymlink_CacheShortcircuits - TestPrintDeprecations_NilConfig_NoPanic - TestPrintDeprecations_EmitsEachMessageAndClears Also restored a garbled `skipIfSymlinkUnsupported` fragment in internal/lint/files_test.go that I'd clipped in round 12 while inserting unit tests (the test file still compiled via the cmd/mdsmith copy by accident). --- cmd/mdsmith/e2e_test.go | 18 +-- cmd/mdsmith/main_unit_test.go | 20 ++++ docs/reference/cli.md | 2 +- internal/discovery/discovery_coverage_test.go | 15 +-- internal/lint/files.go | 29 +++-- internal/lint/files_test.go | 113 ++++++++++++++++-- internal/testutil/symlink.go | 26 ++++ 7 files changed, 177 insertions(+), 46 deletions(-) create mode 100644 internal/testutil/symlink.go diff --git a/cmd/mdsmith/e2e_test.go b/cmd/mdsmith/e2e_test.go index 61dfd0909..a166d0d78 100644 --- a/cmd/mdsmith/e2e_test.go +++ b/cmd/mdsmith/e2e_test.go @@ -12,6 +12,7 @@ import ( "strings" "testing" + "github.com/jeduden/mdsmith/internal/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -201,21 +202,12 @@ func writeFixture(t *testing.T, dir, name, content string) string { return path } -// skipIfSymlinkUnsupported skips the calling test when the host -// cannot create symbolic links. This typically happens on Windows -// without Developer Mode / elevated privileges, and in some -// sandboxed CI environments. +// skipIfSymlinkUnsupported is a thin local alias for +// testutil.SkipIfSymlinkUnsupported so existing call sites stay +// readable. The probing logic lives in internal/testutil. func skipIfSymlinkUnsupported(t *testing.T) { t.Helper() - probe := t.TempDir() - target := filepath.Join(probe, "t") - link := filepath.Join(probe, "l") - if err := os.WriteFile(target, nil, 0o644); err != nil { - t.Skipf("cannot create probe file: %v", err) - } - if err := os.Symlink(target, link); err != nil { - t.Skipf("symbolic links not supported on this host: %v", err) - } + testutil.SkipIfSymlinkUnsupported(t) } func parseStats(t *testing.T, stderr string) (checked, fixed, failures, unfixed int) { diff --git a/cmd/mdsmith/main_unit_test.go b/cmd/mdsmith/main_unit_test.go index df671f534..36da13273 100644 --- a/cmd/mdsmith/main_unit_test.go +++ b/cmd/mdsmith/main_unit_test.go @@ -629,3 +629,23 @@ func TestShowRule_UnknownRule_ExitsTwo(t *testing.T) { }) }) } + +// --- printDeprecations --- + +func TestPrintDeprecations_NilConfig_NoPanic(t *testing.T) { + // Guard: nil-safe. + assert.NotPanics(t, func() { printDeprecations(nil) }) +} + +func TestPrintDeprecations_EmitsEachMessageAndClears(t *testing.T) { + cfg := &config.Config{Deprecations: []string{"first", "second"}} + stderr := captureStderr(func() { printDeprecations(cfg) }) + + assert.Contains(t, stderr, "mdsmith: deprecated: first") + assert.Contains(t, stderr, "mdsmith: deprecated: second") + assert.Empty(t, cfg.Deprecations, + "consumed deprecations must be cleared so a second call is a no-op") + + stderr2 := captureStderr(func() { printDeprecations(cfg) }) + assert.Empty(t, stderr2, "second call on the same cfg emits nothing") +} diff --git a/docs/reference/cli.md b/docs/reference/cli.md index cf872ec10..c042328f5 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -40,7 +40,7 @@ match, exits 0. | `-f`, `--format` | `text` | `text` or `json` | | `--max-input-size` | `2MB` | Max file size (e.g. `2MB`, `0`=none) | | `--no-color` | false | Plain output | -| `--follow-symlinks` | false | Follow symlinks (default: skip) | +| `--follow-symlinks` | config | Follow symlinks; tri-state — see below | | `--no-gitignore` | false | Skip gitignore | | `-q`, `--quiet` | false | Quiet mode | | `-v`, `--verbose` | false | Verbose output | diff --git a/internal/discovery/discovery_coverage_test.go b/internal/discovery/discovery_coverage_test.go index 04f271f3f..aa6d10ab4 100644 --- a/internal/discovery/discovery_coverage_test.go +++ b/internal/discovery/discovery_coverage_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + "github.com/jeduden/mdsmith/internal/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -188,18 +189,8 @@ func TestDiscover_FollowSymlinks_OptIn(t *testing.T) { require.Len(t, files, 2, "both real and linked entries are discovered") } -// skipIfSymlinkUnsupported skips the calling test when the host -// cannot create symbolic links (e.g. Windows without Developer Mode -// or sandboxed CI). +// skipIfSymlinkUnsupported forwards to the shared testutil helper. func skipIfSymlinkUnsupported(t *testing.T) { t.Helper() - probe := t.TempDir() - target := filepath.Join(probe, "t") - link := filepath.Join(probe, "l") - if err := os.WriteFile(target, nil, 0o644); err != nil { - t.Skipf("cannot create probe file: %v", err) - } - if err := os.Symlink(target, link); err != nil { - t.Skipf("symbolic links not supported on this host: %v", err) - } + testutil.SkipIfSymlinkUnsupported(t) } diff --git a/internal/lint/files.go b/internal/lint/files.go index a50aa44a4..486d8224d 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -177,12 +177,21 @@ func hasSymlinkAncestor(path string) bool { // per glob match) should share a `cache` across invocations to // avoid repeated `os.Lstat` calls on the same ancestor directories. func hasSymlinkAncestorCached(path string, cache map[string]bool) bool { + cwd, _ := os.Getwd() // "" on error; handled downstream + return hasSymlinkAncestorWithCwd(path, cwd, cache) +} + +// hasSymlinkAncestorWithCwd is like hasSymlinkAncestorCached but +// takes a precomputed cwd. Per-glob-expansion callers should read +// `os.Getwd` once and pass it here for every match to avoid the +// syscall per entry. +func hasSymlinkAncestorWithCwd(path, cwd string, cache map[string]bool) bool { abs, err := filepath.Abs(path) if err != nil { return false } abs = filepath.Clean(abs) - stop := ancestorStopBoundary(abs) + stop := ancestorStopBoundary(abs, cwd) if stop == "" { return false } @@ -196,9 +205,10 @@ func hasSymlinkAncestorCached(path string, cache map[string]bool) bool { // uniform: prefer cwd if the path is under it (cheapest and // matches user intent), otherwise walk upward for the nearest // `.git` project root (handles absolute paths to sibling projects -// and `../...` relative paths that escape cwd). -func ancestorStopBoundary(abs string) string { - if cwd, err := os.Getwd(); err == nil { +// and `../...` relative paths that escape cwd). An empty cwd +// falls through to the .git walk. +func ancestorStopBoundary(abs, cwd string) string { + if cwd != "" { if rel, err := filepath.Rel(cwd, abs); err == nil && rel != "." && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { @@ -256,16 +266,19 @@ func resolveGlob(pattern string, opts ResolveOpts, addFile func(string)) error { if err != nil { return fmt.Errorf("invalid glob pattern %q: %w", pattern, err) } + // Precompute cwd once: it's the dominant input for + // ancestorStopBoundary and doesn't change across matches. The + // per-directory Lstat cache is shared across every match too, + // so each ancestor dir is Lstat'd at most once per expansion. + cwd, _ := os.Getwd() ancestorCache := make(map[string]bool) for _, m := range matches { // filepath.Glob follows symlinked directory components // during expansion, so a pattern like `linked/*.md` will // return paths rooted under a symlinked dir. Reject any // match whose ancestor chain contains a symlink: symlinked - // directories are always skipped. The cache shares Lstat - // results across matches so large expansions don't repeat - // ancestor syscalls. - if hasSymlinkAncestorCached(m, ancestorCache) { + // directories are always skipped. + if hasSymlinkAncestorWithCwd(m, cwd, ancestorCache) { continue } linfo, lerr := os.Lstat(m) diff --git a/internal/lint/files_test.go b/internal/lint/files_test.go index 9f4fb7d18..1ba17c122 100644 --- a/internal/lint/files_test.go +++ b/internal/lint/files_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + "github.com/jeduden/mdsmith/internal/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -372,18 +373,106 @@ func TestResolveFilesWithOpts_Glob_SkipsSymlinksByDefault(t *testing.T) { "glob expansion must yield real.md and target.md and skip link.md") } -// skipIfSymlinkUnsupported skips the calling test when the host -// cannot create symbolic links (e.g. Windows without Developer Mode -// or sandboxed CI). +// --- hasSymlinkAncestor / helpers --- + +// TestHasSymlinkAncestor_RelativeUnderCwd exercises the common case: +// a project-relative path whose ancestor is a symbolic link. +func TestHasSymlinkAncestor_RelativeUnderCwd(t *testing.T) { + skipIfSymlinkUnsupported(t) + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "real"), 0o755)) + require.NoError(t, os.Symlink( + filepath.Join(dir, "real"), filepath.Join(dir, "linked"))) + + t.Chdir(dir) + assert.True(t, hasSymlinkAncestor("linked/foo.md"), + "linked/foo.md must be flagged as crossing a symlinked ancestor") + assert.False(t, hasSymlinkAncestor("real/foo.md"), + "real/foo.md has no symlink ancestors") +} + +// TestHasSymlinkAncestor_CacheReusedAcrossSiblings covers the +// memoization path in hasSymlinkAncestorCached: two sibling paths +// under the same symlinked directory share one Lstat result. +func TestHasSymlinkAncestor_CacheReusedAcrossSiblings(t *testing.T) { + skipIfSymlinkUnsupported(t) + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "real"), 0o755)) + require.NoError(t, os.Symlink( + filepath.Join(dir, "real"), filepath.Join(dir, "linked"))) + t.Chdir(dir) + + cache := make(map[string]bool) + assert.True(t, hasSymlinkAncestorCached("linked/a.md", cache)) + assert.True(t, hasSymlinkAncestorCached("linked/b.md", cache)) + // The shared parent is cached as "true"; both paths return true + // without re-Lstat'ing the ancestor. + assert.Contains(t, cache, filepath.Join(dir, "linked"), + "shared ancestor must be memoised") +} + +// TestHasSymlinkAncestor_AbsPathOutsideCwdWithGitRoot ensures an +// absolute path outside cwd but inside a .git-rooted project still +// gets its ancestor chain scanned. +func TestHasSymlinkAncestor_AbsPathOutsideCwdWithGitRoot(t *testing.T) { + skipIfSymlinkUnsupported(t) + root := t.TempDir() + project := filepath.Join(root, "project") + cwd := filepath.Join(root, "other") + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(project, "real"), 0o755)) + require.NoError(t, os.MkdirAll(cwd, 0o755)) + require.NoError(t, os.Symlink( + filepath.Join(project, "real"), + filepath.Join(project, "linked"))) + + t.Chdir(cwd) + assert.True(t, + hasSymlinkAncestor(filepath.Join(project, "linked", "foo.md")), + "abs path into a .git-rooted project must scan its ancestors") +} + +// TestHasSymlinkAncestor_SkipsPathOutsideProjects confirms that an +// absolute path with no cwd or .git anchor is trusted (no probe). +// This keeps system-level symlinks like /tmp on macOS out of scope. +func TestHasSymlinkAncestor_SkipsPathOutsideProjects(t *testing.T) { + cwd := t.TempDir() + t.Chdir(cwd) + // /etc/some-file isn't under cwd and no .git ancestor exists + // on the path; helper must return false. + assert.False(t, hasSymlinkAncestor("/etc/nonexistent/foo.md")) +} + +// TestGitProjectRoot_FindsAncestor and _None cover the boundary +// helper used for absolute paths outside cwd. +func TestGitProjectRoot_FindsAncestor(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".git"), 0o755)) + sub := filepath.Join(dir, "docs", "guides") + require.NoError(t, os.MkdirAll(sub, 0o755)) + + got := gitProjectRoot(sub) + assert.Equal(t, dir, got, + "gitProjectRoot must walk up to the nearest .git-containing dir") +} + +func TestGitProjectRoot_NoAncestor(t *testing.T) { + dir := t.TempDir() + assert.Empty(t, gitProjectRoot(dir), + "gitProjectRoot must return \"\" when no .git ancestor exists") +} + +// TestAncestorChainHasSymlink_CacheShortcircuits covers the cache +// hit branch in the recursive helper: a second call with the same +// dir skips the Lstat and returns the memoised value. +func TestAncestorChainHasSymlink_CacheShortcircuits(t *testing.T) { + cache := map[string]bool{"/fake/dir": true} + got := ancestorChainHasSymlink("/fake/dir", "/fake", cache) + assert.True(t, got, "cache hit must return stored value") +} + +// skipIfSymlinkUnsupported forwards to the shared testutil helper. func skipIfSymlinkUnsupported(t *testing.T) { t.Helper() - probe := t.TempDir() - target := filepath.Join(probe, "t") - link := filepath.Join(probe, "l") - if err := os.WriteFile(target, nil, 0o644); err != nil { - t.Skipf("cannot create probe file: %v", err) - } - if err := os.Symlink(target, link); err != nil { - t.Skipf("symbolic links not supported on this host: %v", err) - } + testutil.SkipIfSymlinkUnsupported(t) } diff --git a/internal/testutil/symlink.go b/internal/testutil/symlink.go new file mode 100644 index 000000000..1097a9be2 --- /dev/null +++ b/internal/testutil/symlink.go @@ -0,0 +1,26 @@ +// Package testutil holds small helpers shared across test +// binaries. It is intended for use only from *_test.go files. +package testutil + +import ( + "os" + "path/filepath" + "testing" +) + +// SkipIfSymlinkUnsupported skips the calling test when the host +// cannot create symbolic links. This typically happens on Windows +// without Developer Mode or elevated privileges, and in some +// sandboxed CI environments. +func SkipIfSymlinkUnsupported(t *testing.T) { + t.Helper() + probe := t.TempDir() + target := filepath.Join(probe, "t") + link := filepath.Join(probe, "l") + if err := os.WriteFile(target, nil, 0o644); err != nil { + t.Skipf("cannot create probe file: %v", err) + } + if err := os.Symlink(target, link); err != nil { + t.Skipf("symbolic links not supported on this host: %v", err) + } +} From fdd54518279b3d01d94db4350ef6aa74699d027a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Apr 2026 04:55:28 +0000 Subject: [PATCH 19/28] plan 84: help text reflects tri-state --follow-symlinks `--help` said "default: skip" which doesn't match the implementation: when the flag is omitted we defer to the follow-symlinks config value, and `--follow-symlinks=false` forces skip over a config opt-in. Updated the description on `check`, `fix`, and `metrics rank` to spell both behaviors out so CLI help matches the docs. --- cmd/mdsmith/main.go | 8 ++++++-- cmd/mdsmith/metrics.go | 4 +++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/cmd/mdsmith/main.go b/cmd/mdsmith/main.go index 3b40eb7a4..2db76c717 100644 --- a/cmd/mdsmith/main.go +++ b/cmd/mdsmith/main.go @@ -170,7 +170,9 @@ func runCheck(args []string) int { fs.BoolVarP(&quiet, "quiet", "q", false, "Suppress non-error output") fs.BoolVarP(&verbose, "verbose", "v", false, "Show config, files, and rules on stderr") fs.BoolVar(&noGitignore, "no-gitignore", false, "Disable .gitignore filtering when walking directories") - fs.BoolVar(&followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") + fs.BoolVar(&followSymlinks, "follow-symlinks", false, + "Follow symlinks; omitted defers to follow-symlinks config (default skip); "+ + "=false forces skip over any config opt-in") fs.StringVar(&maxInputSize, "max-input-size", "", "Maximum file size to process (e.g. 2MB, 500KB, 0=unlimited)") fs.Usage = func() { @@ -234,7 +236,9 @@ func runFix(args []string) int { fs.BoolVarP(&quiet, "quiet", "q", false, "Suppress non-error output") fs.BoolVarP(&verbose, "verbose", "v", false, "Show config, files, and rules on stderr") fs.BoolVar(&noGitignore, "no-gitignore", false, "Disable .gitignore filtering when walking directories") - fs.BoolVar(&followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") + fs.BoolVar(&followSymlinks, "follow-symlinks", false, + "Follow symlinks; omitted defers to follow-symlinks config (default skip); "+ + "=false forces skip over any config opt-in") fs.StringVar(&maxInputSize, "max-input-size", "", "Maximum file size to process (e.g. 2MB, 500KB, 0=unlimited)") fs.Usage = func() { diff --git a/cmd/mdsmith/metrics.go b/cmd/mdsmith/metrics.go index d4ff34110..100a9fd9a 100644 --- a/cmd/mdsmith/metrics.go +++ b/cmd/mdsmith/metrics.go @@ -124,7 +124,9 @@ func parseMetricsRankOptions(args []string) (metricsRankOptions, []string, error fs.IntVar(&opts.top, "top", 0, "Limit results to top N files (0 = all)") fs.StringVarP(&opts.format, "format", "f", "text", "Output format: text, json") fs.BoolVar(&opts.noGitignore, "no-gitignore", false, "Disable .gitignore filtering when walking directories") - fs.BoolVar(&followSymlinks, "follow-symlinks", false, "Follow symlinks (default: skip)") + fs.BoolVar(&followSymlinks, "follow-symlinks", false, + "Follow symlinks; omitted defers to follow-symlinks config (default skip); "+ + "=false forces skip over any config opt-in") fs.StringVar(&opts.maxInputSize, "max-input-size", "", "Maximum file size to process (e.g. 2MB, 500KB, 0=unlimited)") From 998a7e1dbfb1b6da05330bfd6c13b251c0ed2abd Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Apr 2026 05:05:31 +0000 Subject: [PATCH 20/28] plan 84: reject non-regular entries + portable test Close the FIFO/device/socket gap on the non-symlink path that Copilot flagged across resolveArg, resolveGlob, walkDir, and the discovery walker. A FIFO or socket whose name happens to end in .md would previously slip through the "markdown by extension" check and then block or error on read. All four entry points now require `Mode().IsRegular()` (or a symlink that already passed the target check) before queueing a path. Also: - TestResolveFiles_SkipsNonRegularEntries exercises the three lint-side entry points with a mkfifo'd .md file. Skipped on Windows. - TestHasSymlinkAncestor_SkipsPathOutsideProjects no longer hardcodes `/etc/...`; it uses an unrelated temp dir so the test stays portable and hermetic. --- internal/discovery/discovery.go | 8 ++++++ internal/lint/files.go | 30 +++++++++++++++++++-- internal/lint/files_test.go | 46 ++++++++++++++++++++++++++++++--- 3 files changed, 79 insertions(+), 5 deletions(-) diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go index ed18328db..71b135f34 100644 --- a/internal/discovery/discovery.go +++ b/internal/discovery/discovery.go @@ -136,6 +136,14 @@ func (w *walker) visit(path string, info os.FileInfo, walkErr error) error { if info.IsDir() { return nil } + // Only include regular files and opted-in symlinks whose + // target is regular (already verified in the symlink branch + // above). FIFO, device, and socket entries are skipped to + // avoid blocking reads during linting. + isSymlink := info.Mode()&os.ModeSymlink != 0 + if !isSymlink && !info.Mode().IsRegular() { + return nil + } if w.matchesAny(rel) { w.addFile(rel, path) diff --git a/internal/lint/files.go b/internal/lint/files.go index 486d8224d..a1e652e42 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -145,6 +145,14 @@ func resolveArg(arg string, opts ResolveOpts, addFile func(string)) error { return addDirFiles(arg, opts, addFile) } + // Non-directory, non-symlink path: reject FIFO, device, and + // socket entries even when explicitly named. Reading them via + // the lint pipeline could block or error, and nothing markdown + // ever lives behind them. + if !isSymlink && !info.Mode().IsRegular() { + return nil + } + // Explicitly named files are never filtered by gitignore. addFile(arg) return nil @@ -302,7 +310,15 @@ func resolveGlob(pattern string, opts ResolveOpts, addFile func(string)) error { if err := addDirFiles(m, opts, addFile); err != nil { return err } - } else if isMarkdown(m) { + continue + } + // Skip FIFO, device, and socket entries even when their + // name ends in .md; only regular files (and symlinks to + // regular files, which passed the check above) are linted. + if !isSymlink && !info.Mode().IsRegular() { + continue + } + if isMarkdown(m) { addFile(m) } } @@ -363,7 +379,17 @@ func walkDir(dir string, useGitignore, followSymlinks bool) ([]string, error) { return nil } - if !info.IsDir() && isMarkdown(path) { + if info.IsDir() { + return nil + } + // Only regular files and opted-in symlinks (target regular, + // verified above) are markdown candidates. Skip FIFOs, + // devices, and sockets to avoid blocking reads later. + isSymlink := info.Mode()&os.ModeSymlink != 0 + if !isSymlink && !info.Mode().IsRegular() { + return nil + } + if isMarkdown(path) { files = append(files, path) } return nil diff --git a/internal/lint/files_test.go b/internal/lint/files_test.go index 1ba17c122..3f4659acc 100644 --- a/internal/lint/files_test.go +++ b/internal/lint/files_test.go @@ -3,8 +3,10 @@ package lint import ( "os" "path/filepath" + "runtime" "sort" "strings" + "syscall" "testing" "github.com/jeduden/mdsmith/internal/testutil" @@ -373,6 +375,40 @@ func TestResolveFilesWithOpts_Glob_SkipsSymlinksByDefault(t *testing.T) { "glob expansion must yield real.md and target.md and skip link.md") } +// TestResolveFiles_SkipsNonRegularEntries asserts that FIFOs, and +// by extension other non-regular file types, are never enqueued — +// even when their name has a markdown extension. Reading such +// entries via the lint pipeline could block indefinitely. +func TestResolveFiles_SkipsNonRegularEntries(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("named pipes behave differently on Windows") + } + dir := t.TempDir() + // Real file + FIFO-with-.md-name in the same directory. + require.NoError(t, os.WriteFile( + filepath.Join(dir, "real.md"), []byte("# Real"), 0o644)) + fifo := filepath.Join(dir, "pipe.md") + require.NoError(t, syscall.Mkfifo(fifo, 0o644)) + + // Explicit arg (resolveArg path). + gotExplicit, err := ResolveFiles([]string{fifo}) + require.NoError(t, err) + assert.Empty(t, gotExplicit, + "explicit FIFO arg must not be enqueued") + + // Directory walk (walkDir path). + gotWalk, err := ResolveFiles([]string{dir}) + require.NoError(t, err) + assert.Equal(t, []string{filepath.Join(dir, "real.md")}, gotWalk, + "walkDir must include the regular file and skip the FIFO") + + // Glob expansion (resolveGlob path). + gotGlob, err := ResolveFiles([]string{filepath.Join(dir, "*.md")}) + require.NoError(t, err) + assert.Equal(t, []string{filepath.Join(dir, "real.md")}, gotGlob, + "resolveGlob must skip the FIFO even with a matching name") +} + // --- hasSymlinkAncestor / helpers --- // TestHasSymlinkAncestor_RelativeUnderCwd exercises the common case: @@ -436,11 +472,15 @@ func TestHasSymlinkAncestor_AbsPathOutsideCwdWithGitRoot(t *testing.T) { // absolute path with no cwd or .git anchor is trusted (no probe). // This keeps system-level symlinks like /tmp on macOS out of scope. func TestHasSymlinkAncestor_SkipsPathOutsideProjects(t *testing.T) { + // cwd and target live in unrelated temp dirs; neither has a + // .git ancestor, so hasSymlinkAncestor should find no anchor + // and return false. Using temp dirs keeps the test portable + // across OSes — unlike a hardcoded `/etc/...` path. cwd := t.TempDir() + outside := t.TempDir() t.Chdir(cwd) - // /etc/some-file isn't under cwd and no .git ancestor exists - // on the path; helper must return false. - assert.False(t, hasSymlinkAncestor("/etc/nonexistent/foo.md")) + assert.False(t, hasSymlinkAncestor( + filepath.Join(outside, "nonexistent", "foo.md"))) } // TestGitProjectRoot_FindsAncestor and _None cover the boundary From 1767d2480b012ddd4477ec14b3d0cfca34c70d2a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Apr 2026 05:15:20 +0000 Subject: [PATCH 21/28] plan 84: gate FIFO test on !windows + cover more branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit syscall.Mkfifo isn't available on Windows, so the FIFO test must live behind a build tag — otherwise the package fails to compile there regardless of t.Skip. Moved TestResolveFiles_SkipsNonRegularEntries into a new internal/lint/files_unix_test.go gated with `//go:build !windows` and dropped the now-unused runtime/syscall imports from files_test.go. Added two more Unix-only tests to close the codecov/patch gap on the FIFO-related branches in resolveArg, resolveGlob, and walkDir: - TestResolveFiles_SymlinkToFifo_SkippedUnderOptIn — symlink to a FIFO is skipped across all three entry points even with FollowSymlinks=true. - TestResolveFiles_BrokenSymlink_SilentlySkipped — dangling symlink is silently skipped (not an error) across all three entry points. --- internal/lint/files_test.go | 36 ---------- internal/lint/files_unix_test.go | 113 +++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 36 deletions(-) create mode 100644 internal/lint/files_unix_test.go diff --git a/internal/lint/files_test.go b/internal/lint/files_test.go index 3f4659acc..d8c5bddc2 100644 --- a/internal/lint/files_test.go +++ b/internal/lint/files_test.go @@ -3,10 +3,8 @@ package lint import ( "os" "path/filepath" - "runtime" "sort" "strings" - "syscall" "testing" "github.com/jeduden/mdsmith/internal/testutil" @@ -375,40 +373,6 @@ func TestResolveFilesWithOpts_Glob_SkipsSymlinksByDefault(t *testing.T) { "glob expansion must yield real.md and target.md and skip link.md") } -// TestResolveFiles_SkipsNonRegularEntries asserts that FIFOs, and -// by extension other non-regular file types, are never enqueued — -// even when their name has a markdown extension. Reading such -// entries via the lint pipeline could block indefinitely. -func TestResolveFiles_SkipsNonRegularEntries(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("named pipes behave differently on Windows") - } - dir := t.TempDir() - // Real file + FIFO-with-.md-name in the same directory. - require.NoError(t, os.WriteFile( - filepath.Join(dir, "real.md"), []byte("# Real"), 0o644)) - fifo := filepath.Join(dir, "pipe.md") - require.NoError(t, syscall.Mkfifo(fifo, 0o644)) - - // Explicit arg (resolveArg path). - gotExplicit, err := ResolveFiles([]string{fifo}) - require.NoError(t, err) - assert.Empty(t, gotExplicit, - "explicit FIFO arg must not be enqueued") - - // Directory walk (walkDir path). - gotWalk, err := ResolveFiles([]string{dir}) - require.NoError(t, err) - assert.Equal(t, []string{filepath.Join(dir, "real.md")}, gotWalk, - "walkDir must include the regular file and skip the FIFO") - - // Glob expansion (resolveGlob path). - gotGlob, err := ResolveFiles([]string{filepath.Join(dir, "*.md")}) - require.NoError(t, err) - assert.Equal(t, []string{filepath.Join(dir, "real.md")}, gotGlob, - "resolveGlob must skip the FIFO even with a matching name") -} - // --- hasSymlinkAncestor / helpers --- // TestHasSymlinkAncestor_RelativeUnderCwd exercises the common case: diff --git a/internal/lint/files_unix_test.go b/internal/lint/files_unix_test.go new file mode 100644 index 000000000..48c9f565b --- /dev/null +++ b/internal/lint/files_unix_test.go @@ -0,0 +1,113 @@ +//go:build !windows + +package lint + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestResolveFiles_SkipsNonRegularEntries asserts that FIFOs, and +// by extension other non-regular file types, are never enqueued — +// even when their name has a markdown extension. Reading such +// entries via the lint pipeline could block indefinitely. +// +// The test is gated behind `!windows` because `syscall.Mkfifo` is +// not available on Windows (the syscall package elides it at +// compile time for that platform). +func TestResolveFiles_SkipsNonRegularEntries(t *testing.T) { + dir := t.TempDir() + // Real file + FIFO-with-.md-name in the same directory. + require.NoError(t, os.WriteFile( + filepath.Join(dir, "real.md"), []byte("# Real"), 0o644)) + fifo := filepath.Join(dir, "pipe.md") + require.NoError(t, syscall.Mkfifo(fifo, 0o644)) + + // Explicit arg (resolveArg path). + gotExplicit, err := ResolveFiles([]string{fifo}) + require.NoError(t, err) + assert.Empty(t, gotExplicit, + "explicit FIFO arg must not be enqueued") + + // Directory walk (walkDir path). + gotWalk, err := ResolveFiles([]string{dir}) + require.NoError(t, err) + assert.Equal(t, []string{filepath.Join(dir, "real.md")}, gotWalk, + "walkDir must include the regular file and skip the FIFO") + + // Glob expansion (resolveGlob path). + gotGlob, err := ResolveFiles([]string{filepath.Join(dir, "*.md")}) + require.NoError(t, err) + assert.Equal(t, []string{filepath.Join(dir, "real.md")}, gotGlob, + "resolveGlob must skip the FIFO even with a matching name") +} + +// TestResolveFiles_SymlinkToFifo_SkippedUnderOptIn covers the +// "symlink to non-regular file" branch: even with FollowSymlinks +// enabled, a symlink pointing at a FIFO must not be enqueued. +func TestResolveFiles_SymlinkToFifo_SkippedUnderOptIn(t *testing.T) { + dir := t.TempDir() + fifo := filepath.Join(dir, "pipe") + require.NoError(t, syscall.Mkfifo(fifo, 0o644)) + link := filepath.Join(dir, "link.md") + require.NoError(t, os.Symlink(fifo, link)) + + opts := DefaultResolveOpts() + opts.FollowSymlinks = true + + // Explicit arg via resolveArg. + gotExplicit, err := ResolveFilesWithOpts([]string{link}, opts) + require.NoError(t, err) + assert.Empty(t, gotExplicit, + "symlink to FIFO must be skipped even under --follow-symlinks") + + // Directory walk via walkDir. + gotWalk, err := ResolveFilesWithOpts([]string{dir}, opts) + require.NoError(t, err) + assert.Empty(t, gotWalk, + "walkDir must skip symlinks whose target is a FIFO") + + // Glob expansion via resolveGlob. + gotGlob, err := ResolveFilesWithOpts( + []string{filepath.Join(dir, "*.md")}, opts) + require.NoError(t, err) + assert.Empty(t, gotGlob, + "resolveGlob must skip symlinks whose target is a FIFO") +} + +// TestResolveFiles_BrokenSymlink_SilentlySkipped covers the +// broken-symlink branches: a symlink whose target was removed +// must be silently skipped (not error out) across all entry +// points, matching the FollowSymlinks contract. +func TestResolveFiles_BrokenSymlink_SilentlySkipped(t *testing.T) { + dir := t.TempDir() + // Create a symlink pointing at a path that does not exist. + link := filepath.Join(dir, "dangling.md") + require.NoError(t, os.Symlink( + filepath.Join(dir, "does-not-exist.md"), link)) + + opts := DefaultResolveOpts() + opts.FollowSymlinks = true + + // Explicit arg. + got, err := ResolveFilesWithOpts([]string{link}, opts) + require.NoError(t, err, + "broken symlink arg must not surface an error") + assert.Empty(t, got) + + // Glob expansion. + got, err = ResolveFilesWithOpts( + []string{filepath.Join(dir, "*.md")}, opts) + require.NoError(t, err) + assert.Empty(t, got) + + // Directory walk. + got, err = ResolveFilesWithOpts([]string{dir}, opts) + require.NoError(t, err) + assert.Empty(t, got) +} From 7c65adea70834d479b0446abdc8d4912617034a9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Apr 2026 05:21:58 +0000 Subject: [PATCH 22/28] plan 84: probe both file and directory symlinks in skip helper On Windows, file symlinks and directory symlinks have separate privilege requirements. The current probe only tried a file symlink, so a host that allows file symlinks but blocks directory symlinks would still run the tests that create directory symlinks and fail instead of skipping. Extended SkipIfSymlinkUnsupported to probe both kinds before proceeding. The skip message names which kind failed, so a partial-capability host surfaces the right reason. --- internal/testutil/symlink.go | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/internal/testutil/symlink.go b/internal/testutil/symlink.go index 1097a9be2..26b2bdab5 100644 --- a/internal/testutil/symlink.go +++ b/internal/testutil/symlink.go @@ -12,15 +12,30 @@ import ( // cannot create symbolic links. This typically happens on Windows // without Developer Mode or elevated privileges, and in some // sandboxed CI environments. +// +// Both a file symlink and a directory symlink are probed: on +// Windows these use different code paths and can have different +// privilege requirements, so a test that assumes one works because +// the other does would fail instead of being skipped. Every test +// in the repo that creates a symlink may create either kind, so +// we gate conservatively on the weaker capability. func SkipIfSymlinkUnsupported(t *testing.T) { t.Helper() probe := t.TempDir() - target := filepath.Join(probe, "t") - link := filepath.Join(probe, "l") - if err := os.WriteFile(target, nil, 0o644); err != nil { + + fileTarget := filepath.Join(probe, "f") + if err := os.WriteFile(fileTarget, nil, 0o644); err != nil { t.Skipf("cannot create probe file: %v", err) } - if err := os.Symlink(target, link); err != nil { - t.Skipf("symbolic links not supported on this host: %v", err) + if err := os.Symlink(fileTarget, filepath.Join(probe, "flink")); err != nil { + t.Skipf("file symlinks not supported on this host: %v", err) + } + + dirTarget := filepath.Join(probe, "d") + if err := os.MkdirAll(dirTarget, 0o755); err != nil { + t.Skipf("cannot create probe directory: %v", err) + } + if err := os.Symlink(dirTarget, filepath.Join(probe, "dlink")); err != nil { + t.Skipf("directory symlinks not supported on this host: %v", err) } } From ef6ec17125777fb8504ec412762129f11df25f97 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Apr 2026 05:30:15 +0000 Subject: [PATCH 23/28] plan 84: actually use precomputed cwd for relative paths hasSymlinkAncestorWithCwd called filepath.Abs(path), which calls os.Getwd internally for relative paths and ignores the cwd argument. The precompute-cwd optimisation in resolveGlob was a no-op: every match still incurred the per-call syscall, and behavior depended on the process cwd rather than the caller- supplied cwd. Added absWithCwd: for absolute paths, just clean; for relative paths, join with the supplied cwd; only fall back to filepath.Abs when cwd is empty (Getwd error upstream). TestHasSymlinkAncestorWithCwd_HonorsCwdArg pins the new contract: resolve against the supplied cwd, not the process cwd, by passing a cwd argument that differs from the test's working directory. --- internal/lint/files.go | 24 +++++++++++++++++++++--- internal/lint/files_test.go | 23 +++++++++++++++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/internal/lint/files.go b/internal/lint/files.go index a1e652e42..8326de577 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -194,11 +194,10 @@ func hasSymlinkAncestorCached(path string, cache map[string]bool) bool { // `os.Getwd` once and pass it here for every match to avoid the // syscall per entry. func hasSymlinkAncestorWithCwd(path, cwd string, cache map[string]bool) bool { - abs, err := filepath.Abs(path) - if err != nil { + abs := absWithCwd(path, cwd) + if abs == "" { return false } - abs = filepath.Clean(abs) stop := ancestorStopBoundary(abs, cwd) if stop == "" { return false @@ -206,6 +205,25 @@ func hasSymlinkAncestorWithCwd(path, cwd string, cache map[string]bool) bool { return ancestorChainHasSymlink(filepath.Dir(abs), stop, cache) } +// absWithCwd resolves path to an absolute, cleaned form using the +// caller-supplied cwd to avoid the per-call `os.Getwd` that +// `filepath.Abs` performs for relative paths. When cwd is empty +// (e.g. a Getwd error upstream), it falls back to `filepath.Abs`. +// Returns "" only on the unreachable `filepath.Abs` failure path. +func absWithCwd(path, cwd string) string { + if filepath.IsAbs(path) { + return filepath.Clean(path) + } + if cwd != "" { + return filepath.Clean(filepath.Join(cwd, path)) + } + abs, err := filepath.Abs(path) + if err != nil { + return "" + } + return filepath.Clean(abs) +} + // ancestorStopBoundary returns the directory at which ancestor // probing should stop (exclusive), or "" if the path should not be // scanned. Both relative and absolute forms are resolved to diff --git a/internal/lint/files_test.go b/internal/lint/files_test.go index d8c5bddc2..98ce613ac 100644 --- a/internal/lint/files_test.go +++ b/internal/lint/files_test.go @@ -475,6 +475,29 @@ func TestAncestorChainHasSymlink_CacheShortcircuits(t *testing.T) { assert.True(t, got, "cache hit must return stored value") } +// TestHasSymlinkAncestorWithCwd_HonorsCwdArg confirms that the +// `cwd` parameter — not the process working directory — is what +// gets joined to a relative path. Otherwise the precomputed-cwd +// optimisation in resolveGlob would be a lie and wouldn't avoid +// the per-match `os.Getwd` it claims to. +func TestHasSymlinkAncestorWithCwd_HonorsCwdArg(t *testing.T) { + skipIfSymlinkUnsupported(t) + target := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(target, "real"), 0o755)) + require.NoError(t, os.Symlink( + filepath.Join(target, "real"), filepath.Join(target, "linked"))) + + // Process cwd is NOT `target`. Pass `target` explicitly. + processCwd := t.TempDir() + t.Chdir(processCwd) + + cache := make(map[string]bool) + got := hasSymlinkAncestorWithCwd("linked/foo.md", target, cache) + assert.True(t, got, + "helper must resolve `linked/foo.md` against the supplied cwd, "+ + "not os.Getwd; otherwise the precomputed-cwd optimisation is moot") +} + // skipIfSymlinkUnsupported forwards to the shared testutil helper. func skipIfSymlinkUnsupported(t *testing.T) { t.Helper() From 5ec698ad9da14cb9a14c61ebe7a090dc38cb507d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Apr 2026 05:43:14 +0000 Subject: [PATCH 24/28] plan 84: reject paths with '..' after a named segment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real bypass: `linked/../dirty.md` where `linked` is a symlinked directory. filepath.Clean collapses `linked/..` lexically, erasing the symlink component from the ancestor scan. The kernel, however, still traverses `linked` when resolving the leaf — so resolveArg / resolveGlob would see a regular file and happily process an external target even with FollowSymlinks=false. Fix: containsDotDotAfterName flags any path containing `..` after a non-`..` segment, and hasSymlinkAncestorWithCwd returns true (skip) for such inputs. Leading `../` — a legitimate way to name paths above cwd — is unaffected because there is no named segment before it for `..` to mask. Tests: - TestContainsDotDotAfterName tables the helper. - TestHasSymlinkAncestorWithCwd_RejectsDotDotAfterName pins the wiring. - TestE2E_Symlink_DotDotAfterSymlinkedDir is the end-to-end guard: `mdsmith check linked/../dirty.md` against a symlinked ancestor does not process the external target. --- cmd/mdsmith/e2e_symlink_default_deny_test.go | 37 ++++++++++++++++++++ internal/lint/files.go | 28 +++++++++++++++ internal/lint/files_test.go | 32 +++++++++++++++++ 3 files changed, 97 insertions(+) diff --git a/cmd/mdsmith/e2e_symlink_default_deny_test.go b/cmd/mdsmith/e2e_symlink_default_deny_test.go index 5487f5db4..dd7485476 100644 --- a/cmd/mdsmith/e2e_symlink_default_deny_test.go +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -320,6 +320,43 @@ func TestE2E_Symlink_DirSymlinkAncestorDotDotPath(t *testing.T) { "`..`-relative path through symlinked dir must be skipped") } +// TestE2E_Symlink_DotDotAfterSymlinkedDir blocks the +// lexical-collapse bypass: an arg like `linked/../dirty.md` +// where `linked` is a symlink to an external directory must +// not reach the external target. filepath.Clean would collapse +// the path to `dirty.md`, hiding the symlink ancestor from a +// naive scan, but the kernel still traverses `linked` when +// resolving the leaf. +func TestE2E_Symlink_DotDotAfterSymlinkedDir(t *testing.T) { + skipIfSymlinkUnsupported(t) + project := t.TempDir() + external := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(project, ".git"), 0o755)) + writeFixture(t, project, ".mdsmith.yml", + "rules:\n no-trailing-spaces: true\n") + + // External tree: `/ext/parent/dirty.md` with trailing space. + parent := filepath.Join(external, "parent") + require.NoError(t, os.MkdirAll(parent, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(parent, "dirty.md"), + []byte("# Evil\n\ntrailing \n"), 0o644)) + + // Project symlink: `project/linked -> /ext/parent/child`. The + // traversal `linked/..` then lands back at /ext/parent (not + // inside project), so `linked/../dirty.md` resolves to + // /ext/parent/dirty.md via the kernel. + child := filepath.Join(parent, "child") + require.NoError(t, os.MkdirAll(child, 0o755)) + require.NoError(t, os.Symlink(child, filepath.Join(project, "linked"))) + + _, _, exitCode := runBinaryInDir(t, project, "", + "check", "--no-color", "--no-gitignore", + "linked/../dirty.md") + assert.Equal(t, 0, exitCode, + "`..` after a symlinked component must not defeat default-deny") +} + // TestE2E_Symlink_NoFollowFlagRemoved asserts that the deprecated // `--no-follow-symlinks` CLI flag has been removed and now errors // out as an unknown flag. diff --git a/internal/lint/files.go b/internal/lint/files.go index 8326de577..0f8892420 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -194,6 +194,15 @@ func hasSymlinkAncestorCached(path string, cache map[string]bool) bool { // `os.Getwd` once and pass it here for every match to avoid the // syscall per entry. func hasSymlinkAncestorWithCwd(path, cwd string, cache map[string]bool) bool { + // A segment of ".." after a named segment (e.g. `linked/..`) + // would be collapsed by filepath.Clean — but the kernel still + // traverses the symlink during os.Lstat, which could let an + // external target slip past the scan. Conservatively treat any + // such path as if it had a symlinked ancestor: skip silently, + // same as the legitimate symlinked-ancestor case. + if containsDotDotAfterName(path) { + return true + } abs := absWithCwd(path, cwd) if abs == "" { return false @@ -205,6 +214,25 @@ func hasSymlinkAncestorWithCwd(path, cwd string, cache map[string]bool) bool { return ancestorChainHasSymlink(filepath.Dir(abs), stop, cache) } +// containsDotDotAfterName returns true if any path segment of path +// equals ".." and is preceded by a non-".." named segment. Such +// paths can hide a symlinked directory from a lexical ancestor +// scan: `linked/../foo` cleans to `foo`, erasing `linked`. +func containsDotDotAfterName(path string) bool { + seenName := false + for _, p := range strings.Split(filepath.ToSlash(path), "/") { + switch p { + case "", ".", "..": + if p == ".." && seenName { + return true + } + default: + seenName = true + } + } + return false +} + // absWithCwd resolves path to an absolute, cleaned form using the // caller-supplied cwd to avoid the per-call `os.Getwd` that // `filepath.Abs` performs for relative paths. When cwd is empty diff --git a/internal/lint/files_test.go b/internal/lint/files_test.go index 98ce613ac..1f7826249 100644 --- a/internal/lint/files_test.go +++ b/internal/lint/files_test.go @@ -475,6 +475,38 @@ func TestAncestorChainHasSymlink_CacheShortcircuits(t *testing.T) { assert.True(t, got, "cache hit must return stored value") } +// TestContainsDotDotAfterName tables the guard helper that blocks +// `linked/../foo.md` style inputs from erasing a symlink component +// via filepath.Clean. +func TestContainsDotDotAfterName(t *testing.T) { + cases := []struct { + path string + want bool + }{ + {"linked/../foo.md", true}, + {"a/b/../c.md", true}, + {"../foo.md", false}, // leading .. is fine; no name to mask + {"../../foo", false}, // still leading + {"./foo.md", false}, // leading . only + {"foo.md", false}, // no .. + {"foo/./bar.md", false}, // dot, not dot-dot + } + for _, tc := range cases { + got := containsDotDotAfterName(tc.path) + assert.Equal(t, tc.want, got, "path=%q", tc.path) + } +} + +// TestHasSymlinkAncestorWithCwd_RejectsDotDotAfterName confirms the +// guard is wired into the public helper. +func TestHasSymlinkAncestorWithCwd_RejectsDotDotAfterName(t *testing.T) { + cache := make(map[string]bool) + got := hasSymlinkAncestorWithCwd( + "linked/../dirty.md", t.TempDir(), cache) + assert.True(t, got, + "`linked/../dirty.md` must be rejected even without touching the fs") +} + // TestHasSymlinkAncestorWithCwd_HonorsCwdArg confirms that the // `cwd` parameter — not the process working directory — is what // gets joined to a relative path. Otherwise the precomputed-cwd From 09c8adac29776c0f4f9fde7515d9dfecc6e738c8 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Apr 2026 16:33:23 +0000 Subject: [PATCH 25/28] plan 84: component-walk for symlink-ancestor detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous guard rejected any path with '..' after a named segment — catching `linked/../dirty.md` but also sweeping up legitimate `a/b/../c.md` into the silent-skip bucket. Copilot flagged the UX regression. Replace both containsDotDotAfterName and the upward-chain ancestorChainHasSymlink with a single component walk that preserves the kernel's resolution semantics: walk each segment of the ORIGINAL path left-to-right; `..` pops `current` up physically; each named segment is Lstat'd against the accumulated path. The cache now keys on those accumulated paths so sibling globs under a shared ancestor still share work. Outcome: - `linked/../dirty.md` with symlinked `linked` → probes `linked`, detects symlink, rejects. Correct. - `a/b/../c.md` with no symlinks → probes `a` and `a/b`, finds nothing, passes through. No more false reject. - `../project/foo.md` (escape-to-sibling) → walker starts at cwd, `..` pops out, `project` probed. Stop boundary still keeps probes inside cwd or a .git tree. Unit tests updated to the new semantic: - TestHasSymlinkAncestor_DotDotAfterSymlinkedDir — real symlink + `..` → flagged. - TestHasSymlinkAncestor_DotDotAfterRealDir — no symlinks + `..` → NOT flagged. - TestHasSymlinkAncestorCache_SkipsRepeatLstat — shared ancestor cached across sibling paths. Removed TestAncestorChainHasSymlink_CacheShortcircuits and TestContainsDotDotAfterName + TestHasSymlinkAncestorWithCwd_RejectsDotDotAfterName since the helpers they tested are gone. --- internal/lint/files.go | 112 ++++++++++++++++++++---------------- internal/lint/files_test.go | 78 +++++++++++++------------ 2 files changed, 106 insertions(+), 84 deletions(-) diff --git a/internal/lint/files.go b/internal/lint/files.go index 0f8892420..936c93f5e 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -193,16 +193,14 @@ func hasSymlinkAncestorCached(path string, cache map[string]bool) bool { // takes a precomputed cwd. Per-glob-expansion callers should read // `os.Getwd` once and pass it here for every match to avoid the // syscall per entry. +// +// The path is walked component-by-component (preserving `..` +// segments) so a lexical `filepath.Clean` can't erase a symlinked +// directory from the scan. For `linked/../dirty.md` where `linked` +// is a symlinked dir, the walker probes `linked` (detects the +// symlink) before the `..` pops back; `a/b/../c.md` with no +// symlinks anywhere is allowed. func hasSymlinkAncestorWithCwd(path, cwd string, cache map[string]bool) bool { - // A segment of ".." after a named segment (e.g. `linked/..`) - // would be collapsed by filepath.Clean — but the kernel still - // traverses the symlink during os.Lstat, which could let an - // external target slip past the scan. Conservatively treat any - // such path as if it had a symlinked ancestor: skip silently, - // same as the legitimate symlinked-ancestor case. - if containsDotDotAfterName(path) { - return true - } abs := absWithCwd(path, cwd) if abs == "" { return false @@ -211,28 +209,71 @@ func hasSymlinkAncestorWithCwd(path, cwd string, cache map[string]bool) bool { if stop == "" { return false } - return ancestorChainHasSymlink(filepath.Dir(abs), stop, cache) -} -// containsDotDotAfterName returns true if any path segment of path -// equals ".." and is preceded by a non-".." named segment. Such -// paths can hide a symlinked directory from a lexical ancestor -// scan: `linked/../foo` cleans to `foo`, erasing `linked`. -func containsDotDotAfterName(path string) bool { - seenName := false - for _, p := range strings.Split(filepath.ToSlash(path), "/") { - switch p { - case "", ".", "..": - if p == ".." && seenName { - return true + // Determine the starting "current" directory for the walk — + // root for absolute paths, cwd (or a Getwd fallback) for + // relative paths. + current := cwd + if filepath.IsAbs(path) { + current = string(filepath.Separator) + } else if current == "" { + wd, err := os.Getwd() + if err != nil { + return false + } + current = wd + } + + // Walk each component of the original (uncleaned) path. + // Named segments are probed for symlinkness once we're at or + // below the stop boundary; `..` segments pop `current` up; + // the leaf segment (last) is never probed — we only check + // ancestors. + segs := strings.Split(filepath.ToSlash(path), "/") + if len(segs) > 0 { + segs = segs[:len(segs)-1] + } + for _, seg := range segs { + switch seg { + case "", ".": + // no-op + case "..": + if parent := filepath.Dir(current); parent != current { + current = parent } default: - seenName = true + current = filepath.Join(current, seg) + // Only probe inside the stop boundary (skip + // system-level ancestors like `/tmp` on macOS). + if current == stop || !isDescendantOf(current, stop) { + continue + } + if v, ok := cache[current]; ok { + if v { + return true + } + continue + } + info, err := os.Lstat(current) + isSymlink := err == nil && info.Mode()&os.ModeSymlink != 0 + cache[current] = isSymlink + if isSymlink { + return true + } } } return false } +// isDescendantOf reports whether p is a strict descendant of base. +// Returns false when p equals base. +func isDescendantOf(p, base string) bool { + if p == base { + return false + } + return strings.HasPrefix(p, base+string(filepath.Separator)) +} + // absWithCwd resolves path to an absolute, cleaned form using the // caller-supplied cwd to avoid the per-call `os.Getwd` that // `filepath.Abs` performs for relative paths. When cwd is empty @@ -289,31 +330,6 @@ func gitProjectRoot(start string) string { } } -// ancestorChainHasSymlink walks upward from dir up to (but not -// including) stop and reports whether any step in the chain is a -// symbolic link. Results are memoised per directory so sibling -// paths under a shared ancestor do not re-Lstat the same entries. -func ancestorChainHasSymlink(dir, stop string, cache map[string]bool) bool { - if dir == stop || dir == "." || dir == "/" { - return false - } - if v, ok := cache[dir]; ok { - return v - } - result := false - if info, err := os.Lstat(dir); err == nil && - info.Mode()&os.ModeSymlink != 0 { - result = true - } else { - parent := filepath.Dir(dir) - if parent != dir { - result = ancestorChainHasSymlink(parent, stop, cache) - } - } - cache[dir] = result - return result -} - // resolveGlob expands a glob pattern and adds matching markdown files. func resolveGlob(pattern string, opts ResolveOpts, addFile func(string)) error { matches, err := filepath.Glob(pattern) diff --git a/internal/lint/files_test.go b/internal/lint/files_test.go index 1f7826249..26b0c0ad5 100644 --- a/internal/lint/files_test.go +++ b/internal/lint/files_test.go @@ -466,45 +466,51 @@ func TestGitProjectRoot_NoAncestor(t *testing.T) { "gitProjectRoot must return \"\" when no .git ancestor exists") } -// TestAncestorChainHasSymlink_CacheShortcircuits covers the cache -// hit branch in the recursive helper: a second call with the same -// dir skips the Lstat and returns the memoised value. -func TestAncestorChainHasSymlink_CacheShortcircuits(t *testing.T) { - cache := map[string]bool{"/fake/dir": true} - got := ancestorChainHasSymlink("/fake/dir", "/fake", cache) - assert.True(t, got, "cache hit must return stored value") -} - -// TestContainsDotDotAfterName tables the guard helper that blocks -// `linked/../foo.md` style inputs from erasing a symlink component -// via filepath.Clean. -func TestContainsDotDotAfterName(t *testing.T) { - cases := []struct { - path string - want bool - }{ - {"linked/../foo.md", true}, - {"a/b/../c.md", true}, - {"../foo.md", false}, // leading .. is fine; no name to mask - {"../../foo", false}, // still leading - {"./foo.md", false}, // leading . only - {"foo.md", false}, // no .. - {"foo/./bar.md", false}, // dot, not dot-dot - } - for _, tc := range cases { - got := containsDotDotAfterName(tc.path) - assert.Equal(t, tc.want, got, "path=%q", tc.path) - } +// TestHasSymlinkAncestor_DotDotAfterSymlinkedDir covers the +// physics-preserving component walk: `linked/../dirty.md` where +// `linked` is a real symlink must be rejected (the walker probes +// `linked` before the `..` pops back, catching the symlink). +func TestHasSymlinkAncestor_DotDotAfterSymlinkedDir(t *testing.T) { + skipIfSymlinkUnsupported(t) + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "real"), 0o755)) + require.NoError(t, os.Symlink( + filepath.Join(dir, "real"), filepath.Join(dir, "linked"))) + t.Chdir(dir) + + assert.True(t, hasSymlinkAncestor("linked/../dirty.md"), + "`linked/../dirty.md` with symlinked `linked` must be flagged") } -// TestHasSymlinkAncestorWithCwd_RejectsDotDotAfterName confirms the -// guard is wired into the public helper. -func TestHasSymlinkAncestorWithCwd_RejectsDotDotAfterName(t *testing.T) { +// TestHasSymlinkAncestor_DotDotAfterRealDir confirms the +// component walk does NOT over-reject: `a/b/../c.md` where no +// component is a symlink is allowed through. +func TestHasSymlinkAncestor_DotDotAfterRealDir(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "a", "b"), 0o755)) + t.Chdir(dir) + + assert.False(t, hasSymlinkAncestor("a/b/../c.md"), + "`..` after a non-symlink dir must not trigger rejection") +} + +// TestHasSymlinkAncestorCache_SkipsRepeatLstat confirms that once +// a directory has been Lstat'd, a second hit uses the cached value +// instead of re-probing. +func TestHasSymlinkAncestorCache_SkipsRepeatLstat(t *testing.T) { + skipIfSymlinkUnsupported(t) + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "real"), 0o755)) + require.NoError(t, os.Symlink( + filepath.Join(dir, "real"), filepath.Join(dir, "linked"))) + cache := make(map[string]bool) - got := hasSymlinkAncestorWithCwd( - "linked/../dirty.md", t.TempDir(), cache) - assert.True(t, got, - "`linked/../dirty.md` must be rejected even without touching the fs") + first := hasSymlinkAncestorWithCwd("linked/a.md", dir, cache) + second := hasSymlinkAncestorWithCwd("linked/b.md", dir, cache) + assert.True(t, first) + assert.True(t, second) + assert.True(t, cache[filepath.Join(dir, "linked")], + "shared ancestor must be memoised as a symlink") } // TestHasSymlinkAncestorWithCwd_HonorsCwdArg confirms that the From 14c26f486255da30bafda5e33ee4adf76d37d64c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Apr 2026 16:38:05 +0000 Subject: [PATCH 26/28] plan 84: cover new walker branches for codecov/patch Round 20's component walk added a few branches the existing tests didn't exercise: - absWithCwd Getwd fallback (empty cwd). - hasSymlinkAncestorWithCwd Getwd fallback for `current` when cwd is empty and path is relative. - isDescendantOf `p == base` branch (used by the walker to stop at stop boundary). - Cross-sibling cache hit under a shared symlinked ancestor. Added unit tests that pin each branch. --- internal/lint/files_test.go | 38 +++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/internal/lint/files_test.go b/internal/lint/files_test.go index 26b0c0ad5..eeade2bb6 100644 --- a/internal/lint/files_test.go +++ b/internal/lint/files_test.go @@ -513,6 +513,44 @@ func TestHasSymlinkAncestorCache_SkipsRepeatLstat(t *testing.T) { "shared ancestor must be memoised as a symlink") } +// TestHasSymlinkAncestorWithCwd_EmptyCwdFallsBackToGetwd exercises +// the Getwd fallback for both the stop-boundary and the component +// walk when the caller passes an empty cwd. +func TestHasSymlinkAncestorWithCwd_EmptyCwdFallsBackToGetwd(t *testing.T) { + skipIfSymlinkUnsupported(t) + dir := t.TempDir() + // .git marker so gitProjectRoot can anchor the stop boundary. + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".git"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "real"), 0o755)) + require.NoError(t, os.Symlink( + filepath.Join(dir, "real"), filepath.Join(dir, "linked"))) + t.Chdir(dir) + + cache := make(map[string]bool) + got := hasSymlinkAncestorWithCwd("linked/a.md", "", cache) + assert.True(t, got, + "empty cwd arg must fall back to os.Getwd for resolution") +} + +// TestIsDescendantOf_SelfReturnsFalse pins the `p == base` branch +// (strict-descendant semantics) so `current == stop` in the walker +// correctly skips probing. +func TestIsDescendantOf_SelfReturnsFalse(t *testing.T) { + assert.False(t, isDescendantOf("/foo", "/foo"), + "path equal to base is not a strict descendant") + assert.True(t, isDescendantOf("/foo/bar", "/foo")) + assert.False(t, isDescendantOf("/foo-other", "/foo"), + "must not mistake sibling with shared prefix for descendant") +} + +// TestAbsWithCwd_EmptyCwdUsesGetwd covers the Getwd fallback. +func TestAbsWithCwd_EmptyCwdUsesGetwd(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + got := absWithCwd("foo.md", "") + assert.Equal(t, filepath.Join(dir, "foo.md"), got) +} + // TestHasSymlinkAncestorWithCwd_HonorsCwdArg confirms that the // `cwd` parameter — not the process working directory — is what // gets joined to a relative path. Otherwise the precomputed-cwd From f6ae3cb03759b3b67dc14d2b2291f8a1673aaded Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Apr 2026 16:40:16 +0000 Subject: [PATCH 27/28] plan 84: Windows-safe component walk and boundary check Two cross-platform concerns from Copilot: 1. hasSymlinkAncestorWithCwd initialised `current` at `string(filepath.Separator)` for absolute paths and then filepath.Join'd each segment of the raw path. On Windows, absolute paths carry a volume/UNC prefix (`C:\...`, `\\host\share\...`). Starting at `/` and Joining `C:\foo` segments produces invalid intermediate paths, which would miss symlink ancestors. Now use filepath.VolumeName to seed `current` with the volume root and strip the prefix before splitting segments. 2. isDescendantOf used a raw `strings.HasPrefix` check against base+separator. On Windows, separator normalisation and case differences between cwd and the reconstructed `current` could incorrectly classify a path inside the boundary as outside. Switched to filepath.Rel + a "!..prefix" check, which handles separator normalisation. Case-insensitivity on Windows still requires the caller to pass consistently- cased paths (which they do, since `current` is built from `cwd` + Join'd segments). --- internal/lint/files.go | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/internal/lint/files.go b/internal/lint/files.go index 936c93f5e..a7acae496 100644 --- a/internal/lint/files.go +++ b/internal/lint/files.go @@ -210,12 +210,17 @@ func hasSymlinkAncestorWithCwd(path, cwd string, cache map[string]bool) bool { return false } - // Determine the starting "current" directory for the walk — - // root for absolute paths, cwd (or a Getwd fallback) for - // relative paths. + // Determine the starting "current" directory for the walk and + // the path suffix to walk from it. For absolute paths we have + // to respect the Windows volume/UNC prefix (e.g. `C:\`, + // `\\host\share\`); starting at `/` would produce invalid + // intermediate paths like `/foo` for an arg of `C:\foo`. current := cwd + rest := path if filepath.IsAbs(path) { - current = string(filepath.Separator) + vol := filepath.VolumeName(path) + current = vol + string(filepath.Separator) + rest = strings.TrimPrefix(path, vol) } else if current == "" { wd, err := os.Getwd() if err != nil { @@ -224,12 +229,12 @@ func hasSymlinkAncestorWithCwd(path, cwd string, cache map[string]bool) bool { current = wd } - // Walk each component of the original (uncleaned) path. + // Walk each component of the original (uncleaned) suffix. // Named segments are probed for symlinkness once we're at or // below the stop boundary; `..` segments pop `current` up; // the leaf segment (last) is never probed — we only check // ancestors. - segs := strings.Split(filepath.ToSlash(path), "/") + segs := strings.Split(filepath.ToSlash(rest), "/") if len(segs) > 0 { segs = segs[:len(segs)-1] } @@ -266,12 +271,20 @@ func hasSymlinkAncestorWithCwd(path, cwd string, cache map[string]bool) bool { } // isDescendantOf reports whether p is a strict descendant of base. -// Returns false when p equals base. +// Returns false when p equals base. Uses filepath.Rel so paths +// that only differ in separator normalisation still compare +// correctly on Windows; it does not do case folding, so callers +// should pass consistently-cased paths (both from the same cwd / +// Lstat family). func isDescendantOf(p, base string) bool { - if p == base { + rel, err := filepath.Rel(base, p) + if err != nil { + return false + } + if rel == "." { return false } - return strings.HasPrefix(p, base+string(filepath.Separator)) + return !strings.HasPrefix(rel, "..") } // absWithCwd resolves path to an absolute, cleaned form using the From 9bce83bd3519c880fc9405796a3562eab471b750 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Apr 2026 17:06:53 +0000 Subject: [PATCH 28/28] plan 84: cover more plan-84 branches for codecov/patch Added focused unit tests for the plan-84-specific branches that neither unit nor e2e paths exercised after the round-20 refactor: internal/lint: - Test*_SymlinkedAncestorPath_Skipped covers the hasSymlinkAncestor guard in resolveArg. - Test*_SymlinkFileNoFollow_Skipped covers the isSymlink && !FollowSymlinks early return in resolveArg. - Test*_BrokenSymlink_SilentlySkipped covers the symlink-with- Stat-err path. - Test*_NonexistentNonSymlink_Errors covers the non-symlink Stat-err branch (missing arg surfaces as an error). - TestAbsWithCwd_AbsolutePath_Clean covers the absolute-path branch of absWithCwd. - TestIsDescendantOf extended with "ancestor-of-base" and "self" branches. - TestHasSymlinkAncestor_NoSymlinkButCached covers the cache-false `continue` branch in the component walk. - TestHasSymlinkAncestorWithCwd_AbsolutePath_UnderCwd exercises the VolumeName-aware absolute-path init. internal/discovery: - discovery_unix_test.go (new, !windows): TestDiscover_SkipsFifoEntries and TestDiscover_SymlinkToFifo_SkippedUnderOptIn cover the FIFO and symlink-to-FIFO skip branches in the walker. internal/config: - TestTopLevelKeySet_InvalidYAML + _NotAMapping cover the parse error branches. - TestLoad_LegacyNoFollowSymlinksEmitsDeprecation covers the deprecation-accumulation branch in Load. --- internal/config/config_test.go | 32 ++++++ internal/discovery/discovery_unix_test.go | 58 ++++++++++ internal/lint/files_test.go | 132 +++++++++++++++++++++- 3 files changed, 216 insertions(+), 6 deletions(-) create mode 100644 internal/discovery/discovery_unix_test.go diff --git a/internal/config/config_test.go b/internal/config/config_test.go index dba48c434..22ef38040 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1301,6 +1301,38 @@ func TestYamlHasKeyRejectsAnchor(t *testing.T) { assert.False(t, yamlHasKey(yml, "base")) } +// TestTopLevelKeySet_InvalidYAML covers the yaml.Unmarshal error +// branch of topLevelKeySet: a syntactically bad YAML payload +// returns an empty set (not a panic) so callers can degrade +// gracefully. +func TestTopLevelKeySet_InvalidYAML(t *testing.T) { + assert.Empty(t, topLevelKeySet([]byte("{not: valid: yaml:"))) +} + +// TestTopLevelKeySet_NotAMapping covers the kind-check branch: +// a top-level scalar (e.g. a bare string) yields an empty set +// because there are no keys to list. +func TestTopLevelKeySet_NotAMapping(t *testing.T) { + assert.Empty(t, topLevelKeySet([]byte("bare-string-value\n"))) +} + +// TestLoad_LegacyNoFollowSymlinksEmitsDeprecation exercises the +// deprecation-warning branch of Load: the legacy config key +// `no-follow-symlinks` is parsed and a warning is appended to +// cfg.Deprecations for the CLI to print. +func TestLoad_LegacyNoFollowSymlinksEmitsDeprecation(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, ".mdsmith.yml") + require.NoError(t, os.WriteFile(cfgPath, + []byte("no-follow-symlinks:\n - \"**\"\n"), 0o644)) + + cfg, err := Load(cfgPath) + require.NoError(t, err) + require.NotEmpty(t, cfg.Deprecations, + "legacy no-follow-symlinks key must emit a deprecation") + assert.Contains(t, cfg.Deprecations[0], "no-follow-symlinks") +} + func TestMergeMaxInputSize_FromLoaded(t *testing.T) { defaults := &Config{ Rules: map[string]RuleCfg{"a": {Enabled: true}}, diff --git a/internal/discovery/discovery_unix_test.go b/internal/discovery/discovery_unix_test.go new file mode 100644 index 000000000..2282803db --- /dev/null +++ b/internal/discovery/discovery_unix_test.go @@ -0,0 +1,58 @@ +//go:build !windows + +package discovery + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/jeduden/mdsmith/internal/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestDiscover_SkipsFifoEntries covers the non-symlink +// non-regular skip branch in the discovery walker: a FIFO with +// a markdown-matching name must not be added to results. +func TestDiscover_SkipsFifoEntries(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile( + filepath.Join(dir, "real.md"), []byte("# Real"), 0o644)) + require.NoError(t, syscall.Mkfifo( + filepath.Join(dir, "pipe.md"), 0o644)) + + files, err := Discover(Options{ + Patterns: []string{"**/*.md"}, + BaseDir: dir, + }) + require.NoError(t, err) + require.Len(t, files, 1) + assert.Equal(t, "real.md", filepath.Base(files[0])) +} + +// TestDiscover_SymlinkToFifo_SkippedUnderOptIn covers the +// symlink-to-non-regular skip branch: even in opt-in mode, a +// symlink whose target is a FIFO must not be included. +func TestDiscover_SymlinkToFifo_SkippedUnderOptIn(t *testing.T) { + testutil.SkipIfSymlinkUnsupported(t) + dir := t.TempDir() + + require.NoError(t, os.WriteFile( + filepath.Join(dir, "real.md"), []byte("# Real"), 0o644)) + + fifo := filepath.Join(dir, "pipe") + require.NoError(t, syscall.Mkfifo(fifo, 0o644)) + require.NoError(t, os.Symlink(fifo, filepath.Join(dir, "link.md"))) + + files, err := Discover(Options{ + Patterns: []string{"**/*.md"}, + BaseDir: dir, + FollowSymlinks: true, + }) + require.NoError(t, err) + require.Len(t, files, 1, + "symlink to FIFO must not be enqueued even under FollowSymlinks") + assert.Equal(t, "real.md", filepath.Base(files[0])) +} diff --git a/internal/lint/files_test.go b/internal/lint/files_test.go index eeade2bb6..526975d26 100644 --- a/internal/lint/files_test.go +++ b/internal/lint/files_test.go @@ -494,6 +494,106 @@ func TestHasSymlinkAncestor_DotDotAfterRealDir(t *testing.T) { "`..` after a non-symlink dir must not trigger rejection") } +// TestAbsWithCwd_EmptyCwdUsesGetwd covers the Getwd fallback. +func TestAbsWithCwd_EmptyCwdUsesGetwd(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + got := absWithCwd("foo.md", "") + assert.Equal(t, filepath.Join(dir, "foo.md"), got) +} + +// TestAbsWithCwd_AbsolutePath_Clean covers the absolute-path +// branch: no Getwd lookup, just cleaned form. +func TestAbsWithCwd_AbsolutePath_Clean(t *testing.T) { + got := absWithCwd("/foo/./bar/../baz.md", "/some/cwd") + assert.Equal(t, filepath.Clean("/foo/baz.md"), got) +} + +// TestResolveArg_SymlinkedAncestorPath_Skipped covers the +// `hasSymlinkAncestor(arg)` branch in resolveArg via the public +// ResolveFilesWithOpts API. +func TestResolveArg_SymlinkedAncestorPath_Skipped(t *testing.T) { + skipIfSymlinkUnsupported(t) + dir := t.TempDir() + real := filepath.Join(dir, "real") + require.NoError(t, os.MkdirAll(real, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(real, "foo.md"), []byte("# Real"), 0o644)) + require.NoError(t, os.Symlink(real, filepath.Join(dir, "linked"))) + t.Chdir(dir) + + got, err := ResolveFiles([]string{"linked/foo.md"}) + require.NoError(t, err) + assert.Empty(t, got, "path through symlinked ancestor must be skipped") +} + +// TestResolveArg_SymlinkFileNoFollow_Skipped covers the +// `isSymlink && !FollowSymlinks` branch in resolveArg. +func TestResolveArg_SymlinkFileNoFollow_Skipped(t *testing.T) { + skipIfSymlinkUnsupported(t) + dir := t.TempDir() + real := filepath.Join(dir, "real.md") + require.NoError(t, os.WriteFile(real, []byte("# Real"), 0o644)) + link := filepath.Join(dir, "link.md") + require.NoError(t, os.Symlink(real, link)) + + got, err := ResolveFilesWithOpts([]string{link}, DefaultResolveOpts()) + require.NoError(t, err) + assert.Empty(t, got, "symlink file must be skipped under default-deny") +} + +// TestResolveArg_BrokenSymlink_SilentlySkipped covers the +// `isSymlink && Stat-err` branch in resolveArg. +func TestResolveArg_BrokenSymlink_SilentlySkipped(t *testing.T) { + skipIfSymlinkUnsupported(t) + dir := t.TempDir() + link := filepath.Join(dir, "dangling.md") + require.NoError(t, os.Symlink( + filepath.Join(dir, "does-not-exist.md"), link)) + + opts := DefaultResolveOpts() + opts.FollowSymlinks = true + got, err := ResolveFilesWithOpts([]string{link}, opts) + require.NoError(t, err, + "broken-symlink arg must not error") + assert.Empty(t, got) +} + +// TestResolveArg_NonexistentNonSymlink_Errors covers the +// `!isSymlink && Stat-err` branch — a typo'd regular path +// surfaces as an error so users see their mistake. +func TestResolveArg_NonexistentNonSymlink_Errors(t *testing.T) { + _, err := ResolveFiles([]string{ + filepath.Join(t.TempDir(), "nonexistent.md")}) + require.Error(t, err, + "missing non-symlink path must surface as an error") +} + +// TestIsDescendantOf_NotADescendant covers the negative path: +// `filepath.Rel` returns a `..`-prefixed string when p is outside +// base, and isDescendantOf reports false. +func TestIsDescendantOf_NotADescendant(t *testing.T) { + assert.False(t, isDescendantOf("/a/b", "/c/d"), + "unrelated tree must not be a descendant") +} + +// TestHasSymlinkAncestorWithCwd_AbsolutePath_UnderCwd exercises +// the absolute-path branch that initialises `current` with the +// volume root. The walk must reach the symlinked component. +func TestHasSymlinkAncestorWithCwd_AbsolutePath_UnderCwd(t *testing.T) { + skipIfSymlinkUnsupported(t) + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "real"), 0o755)) + require.NoError(t, os.Symlink( + filepath.Join(dir, "real"), filepath.Join(dir, "linked"))) + + cache := make(map[string]bool) + abs := filepath.Join(dir, "linked", "foo.md") + got := hasSymlinkAncestorWithCwd(abs, dir, cache) + assert.True(t, got, + "absolute path through symlinked component must be flagged") +} + // TestHasSymlinkAncestorCache_SkipsRepeatLstat confirms that once // a directory has been Lstat'd, a second hit uses the cached value // instead of re-probing. @@ -534,21 +634,41 @@ func TestHasSymlinkAncestorWithCwd_EmptyCwdFallsBackToGetwd(t *testing.T) { // TestIsDescendantOf_SelfReturnsFalse pins the `p == base` branch // (strict-descendant semantics) so `current == stop` in the walker -// correctly skips probing. +// correctly skips probing. filepath.Rel returns "." when the two +// paths are identical. func TestIsDescendantOf_SelfReturnsFalse(t *testing.T) { assert.False(t, isDescendantOf("/foo", "/foo"), "path equal to base is not a strict descendant") assert.True(t, isDescendantOf("/foo/bar", "/foo")) assert.False(t, isDescendantOf("/foo-other", "/foo"), "must not mistake sibling with shared prefix for descendant") + assert.False(t, isDescendantOf("/", "/foo"), + "ancestor of base is not a descendant") } -// TestAbsWithCwd_EmptyCwdUsesGetwd covers the Getwd fallback. -func TestAbsWithCwd_EmptyCwdUsesGetwd(t *testing.T) { +// TestHasSymlinkAncestor_NoSymlinkButCached covers the cache-hit +// `continue` branch: a second walk over paths that share a +// non-symlink ancestor reuses the cached `false`, skipping the +// Lstat. +func TestHasSymlinkAncestor_NoSymlinkButCached(t *testing.T) { dir := t.TempDir() - t.Chdir(dir) - got := absWithCwd("foo.md", "") - assert.Equal(t, filepath.Join(dir, "foo.md"), got) + sub := filepath.Join(dir, "sub", "deep") + require.NoError(t, os.MkdirAll(sub, 0o755)) + + cache := make(map[string]bool) + assert.False(t, + hasSymlinkAncestorWithCwd("sub/deep/a.md", dir, cache)) + assert.False(t, + hasSymlinkAncestorWithCwd("sub/deep/b.md", dir, cache)) + // The non-symlink ancestor is memoised as `false`. + for _, d := range []string{ + filepath.Join(dir, "sub"), + filepath.Join(dir, "sub", "deep"), + } { + v, ok := cache[d] + assert.True(t, ok, "%s must be cached", d) + assert.False(t, v, "%s must be cached as non-symlink", d) + } } // TestHasSymlinkAncestorWithCwd_HonorsCwdArg confirms that the