diff --git a/PLAN.md b/PLAN.md index 21ce66ca3..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/cmd/mdsmith/e2e_coverage_test.go b/cmd/mdsmith/e2e_coverage_test.go index 4e2c1b6b7..f4f60548d 100644 --- a/cmd/mdsmith/e2e_coverage_test.go +++ b/cmd/mdsmith/e2e_coverage_test.go @@ -136,55 +136,6 @@ func TestE2E_Fix_Discovered_BadConfig_ExitsTwo(t *testing.T) { "expected error in stderr, got: %s", stderr) } -// ============================================================= -// 9. resolveOpts — --no-follow-symlinks flag -// ============================================================= - -func TestE2E_Check_NoFollowSymlinks(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. - _, _, 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)") - - // With --no-follow-symlinks, symlinked dir is skipped; only real/ found. - _, 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) -} - -func TestE2E_Fix_NoFollowSymlinks(t *testing.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"))) - - // --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, - "expected exit 0 after fix, got %d; stderr: %s", exitCode, stderr) -} - // ============================================================= // 10. rootDirFromConfig — empty cfgPath falls back to cwd // ============================================================= @@ -486,31 +437,36 @@ 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) { + skipIfSymlinkUnsupported(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) } // ============================================================= @@ -685,21 +641,6 @@ func TestE2E_Init_HelpFlag(t *testing.T) { "expected init usage, got: %s", stderr) } -// ============================================================= -// Additional: metrics rank with --no-follow-symlinks -// ============================================================= - -func TestE2E_MetricsRank_NoFollowSymlinks(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 new file mode 100644 index 000000000..dd7485476 --- /dev/null +++ b/cmd/mdsmith/e2e_symlink_default_deny_test.go @@ -0,0 +1,481 @@ +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) { + 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") + 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_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) { + 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") + + 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) { + 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") + + 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) { + 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("# 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_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) { + 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 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_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) { + skipIfSymlinkUnsupported(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_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) { + 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"))) + + // 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) { + 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"))) + + _, _, 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_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_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_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_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. +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, 2, exitCode, + "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. +func TestE2E_Symlink_LegacyNoFollowConfig_Deprecation(t *testing.T) { + skipIfSymlinkUnsupported(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` 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) { + 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") + + externalFile := filepath.Join(external, "dirty.md") + const dirtyContent = "# Dirty\n\ntrailing \n" + require.NoError(t, os.WriteFile(externalFile, + []byte(dirtyContent), 0o644)) + linked := filepath.Join(project, "linked.md") + require.NoError(t, os.Symlink(externalFile, linked)) + + // 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 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", ".") + 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.Equal(t, dirtyContent, string(extAfter), + "fix must never rewrite the external symlink target directly") +} diff --git a/cmd/mdsmith/e2e_test.go b/cmd/mdsmith/e2e_test.go index 4556ebeb9..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,6 +202,14 @@ func writeFixture(t *testing.T, dir, name, content string) string { return path } +// 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() + testutil.SkipIfSymlinkUnsupported(t) +} + 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/cmd/mdsmith/main.go b/cmd/mdsmith/main.go index 8e55adcbd..2db76c717 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,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(&noFollowSymlinks, "no-follow-symlinks", false, "Skip symbolic links when walking directories") + 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() { @@ -192,6 +194,11 @@ func runCheck(args []string) int { verbose = false } + walk := walkCLI{ + noGitignore: noGitignore, + followSymlinks: followSymlinksOverride(fs, followSymlinks), + } + allArgs := fs.Args() // Check for explicit stdin argument "-". @@ -202,31 +209,25 @@ func runCheck(args []string) int { } if len(fileArgs) > 0 { - return checkFiles( - fileArgs, configPath, format, noColor, quiet, verbose, - noGitignore, noFollowSymlinks, 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, noFollowSymlinks, maxInputSize, - ) + return checkDiscovered(configPath, format, noColor, quiet, verbose, walk, maxInputSize) } // runFix implements the "fix" subcommand: auto-fix lint issues in place. 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,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(&noFollowSymlinks, "no-follow-symlinks", false, "Skip symbolic links when walking directories") + 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() { @@ -257,6 +260,11 @@ func runFix(args []string) int { verbose = false } + walk := walkCLI{ + noGitignore: noGitignore, + followSymlinks: followSymlinksOverride(fs, followSymlinks), + } + allArgs := fs.Args() // Check for explicit stdin argument "-". @@ -268,17 +276,11 @@ func runFix(args []string) int { } if len(fileArgs) > 0 { - return fixFiles( - fileArgs, configPath, format, noColor, quiet, verbose, - noGitignore, noFollowSymlinks, 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, noFollowSymlinks, maxInputSize, - ) + return fixDiscovered(configPath, format, noColor, quiet, verbose, walk, maxInputSize) } // runQuery implements the "query" subcommand: select files by CUE @@ -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 bool, walk walkCLI, maxInputSize string, ) int { cfg, cfgPath, logger, files, maxBytes, code := loadAndResolve( - fileArgs, configPath, verbose, noGitignore, noFollowSymlinks, maxInputSize, + fileArgs, configPath, verbose, walk, 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 bool, walk walkCLI, maxInputSize string, ) int { cfg, cfgPath, logger, files, maxBytes, code := loadAndResolve( - fileArgs, configPath, verbose, noGitignore, noFollowSymlinks, maxInputSize, + fileArgs, configPath, verbose, walk, 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 bool, walk walkCLI, 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, walk) 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 bool, walk walkCLI, ) (*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: !walk.noGitignore, + FollowSymlinks: resolveOpts(cfg, walk).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 bool, walk walkCLI, maxInputSize string, ) int { - cfg, cfgPath, logger, files, code := discoverFiles(configPath, verbose, noGitignore, noFollowSymlinks) + cfg, cfgPath, logger, files, code := discoverFiles(configPath, verbose, walk) 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 bool, walk walkCLI, maxInputSize string, ) int { - cfg, cfgPath, logger, files, code := discoverFiles(configPath, verbose, noGitignore, noFollowSymlinks) + cfg, cfgPath, logger, files, code := discoverFiles(configPath, verbose, walk) if code >= 0 { return code } @@ -883,21 +885,46 @@ func fixDiscovered( return 0 } +// walkCLI bundles the CLI flags that affect how files are +// discovered and resolved, so helpers can thread one value +// 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 +} + // 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 { - useGitignore := !noGitignore - opts := lint.ResolveOpts{ - UseGitignore: &useGitignore, - NoFollowSymlinks: cfg.NoFollowSymlinks, - } - if noFollowSymlinks { - opts.NoFollowSymlinks = []string{"**"} - } - return opts +// The `--follow-symlinks` CLI flag overrides `follow-symlinks:` +// from config when explicitly set; otherwise the config value +// stands. +func resolveOpts(cfg *config.Config, walk walkCLI) lint.ResolveOpts { + useGitignore := !walk.noGitignore + follow := cfg.FollowSymlinks + if walk.followSymlinks != nil { + follow = *walk.followSymlinks + } + return lint.ResolveOpts{ + UseGitignore: &useGitignore, + FollowSymlinks: follow, + } +} + +// 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 nil } // resolveMaxInputBytes returns the effective max-input-size in bytes. @@ -959,7 +986,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 +1011,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/main_unit_test.go b/cmd/mdsmith/main_unit_test.go index 4d0df7056..36da13273 100644 --- a/cmd/mdsmith/main_unit_test.go +++ b/cmd/mdsmith/main_unit_test.go @@ -168,35 +168,38 @@ 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) + yes := true + opts := resolveOpts(cfg, walkCLI{followSymlinks: &yes}) + 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_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 --- @@ -626,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/cmd/mdsmith/metrics.go b/cmd/mdsmith/metrics.go index 3b89a37f9..100a9fd9a 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 { @@ -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,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(&opts.noFollowSymlinks, "no-follow-symlinks", false, "Skip symbolic links when walking directories") + 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)") @@ -144,6 +147,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 { @@ -243,7 +247,10 @@ func resolveRankSelection( } func resolveRankFiles(cfg *config.Config, opts metricsRankOptions, fileArgs []string) ([]string, error) { - resolveOptions := resolveOpts(cfg, opts.noGitignore, opts.noFollowSymlinks) + resolveOptions := resolveOpts(cfg, walkCLI{ + noGitignore: opts.noGitignore, + followSymlinks: opts.followSymlinks, + }) return lint.ResolveFilesWithOpts(fileArgs, resolveOptions) } diff --git a/docs/reference/cli.md b/docs/reference/cli.md index e99d51d47..c042328f5 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -34,16 +34,37 @@ 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` | config | Follow symlinks; tri-state — see below | +| `--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. 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. 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. ## Other Subcommand Flags @@ -55,7 +76,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..4578bf501 100644 --- a/docs/security/2026-04-05-adversarial-markdown.md +++ b/docs/security/2026-04-05-adversarial-markdown.md @@ -108,12 +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 - -Symlink following is the default behavior. The `--no-follow-symlinks` flag and config exist but are opt-in. +**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. 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/internal/config/config.go b/internal/config/config.go index 7f408ac8c..ff041a2c7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -24,15 +24,23 @@ 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. 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 // the user config (not just inherited from defaults). This is @@ -46,6 +54,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/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/config/load.go b/internal/config/load.go index 216bc9a00..efb8e12da 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -34,34 +34,51 @@ 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 keys["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 } -// 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 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..71b135f34 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,15 @@ 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 including symlinks that resolve + // to regular files. The zero value skips all symlinks, which + // is the secure default. + // + // 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 } // Discover walks BaseDir and returns files matching any of the configured @@ -55,11 +61,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 +89,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,11 +109,21 @@ func (w *walker) visit(path string, info os.FileInfo, walkErr error) error { } rel = filepath.ToSlash(rel) - if w.isNoFollow(path, info) { - if info.IsDir() { - return filepath.SkipDir + // 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 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. 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 } - return nil } if w.isGitignored(path, info) { @@ -120,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) @@ -127,17 +151,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 +184,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..aa6d10ab4 100644 --- a/internal/discovery/discovery_coverage_test.go +++ b/internal/discovery/discovery_coverage_test.go @@ -4,77 +4,12 @@ import ( "os" "path/filepath" "testing" - "time" + "github.com/jeduden/mdsmith/internal/testutil" "github.com/stretchr/testify/assert" "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,26 +22,60 @@ func TestVisit_WalkError(t *testing.T) { assert.ErrorIs(t, err, os.ErrPermission) } -func TestVisit_SkipsDirWhenNoFollow(t *testing.T) { +// 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) { + skipIfSymlinkUnsupported(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"}, - noFollow: []string{"linked"}, seen: make(map[string]bool), } + visitErr := w.visit(linked, info, nil) + assert.NoError(t, visitErr) + assert.Empty(t, w.result, "symlink must be skipped") +} + +// 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) { + skipIfSymlinkUnsupported(t) + dir := t.TempDir() + + 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)) - // 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) + info, err := os.Lstat(linked) + require.NoError(t, err) + + w := &walker{ + absBase: dir, + patterns: []string{"**/*.md"}, + followSymlinks: true, + seen: make(map[string]bool), + } + 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) { @@ -180,7 +149,10 @@ 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) { + skipIfSymlinkUnsupported(t) dir := t.TempDir() writeFile(t, dir, "real.md", "# Real\n") @@ -193,11 +165,14 @@ 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) { + skipIfSymlinkUnsupported(t) dir := t.TempDir() writeFile(t, dir, "real.md", "# Real\n") @@ -205,31 +180,17 @@ 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. -type fakeFileInfo struct { - name string - mode os.FileMode - isDir bool +// skipIfSymlinkUnsupported forwards to the shared testutil helper. +func skipIfSymlinkUnsupported(t *testing.T) { + t.Helper() + testutil.SkipIfSymlinkUnsupported(t) } - -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/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.go b/internal/lint/files.go index d6f0875bc..a7acae496 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,16 @@ 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 that + // resolve to regular files. The zero value skips all symlinks, + // which is the secure default. + // + // 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 } // DefaultResolveOpts returns options with defaults applied. @@ -108,45 +97,303 @@ 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 + // 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) + } + isSymlink := linfo.Mode()&os.ModeSymlink != 0 + if isSymlink && !opts.FollowSymlinks { + return nil + } + 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) } + // 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 + } + if info.IsDir() { 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 } +// hasSymlinkAncestor reports whether any ancestor directory of +// 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. +// +// 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)) +} + +// 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 { + 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. +// +// 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 { + abs := absWithCwd(path, cwd) + if abs == "" { + return false + } + stop := ancestorStopBoundary(abs, cwd) + if stop == "" { + return false + } + + // 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) { + vol := filepath.VolumeName(path) + current = vol + string(filepath.Separator) + rest = strings.TrimPrefix(path, vol) + } else if current == "" { + wd, err := os.Getwd() + if err != nil { + return false + } + current = wd + } + + // 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(rest), "/") + 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: + 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. 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 { + rel, err := filepath.Rel(base, p) + if err != nil { + return false + } + if rel == "." { + return false + } + return !strings.HasPrefix(rel, "..") +} + +// 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 +// 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). 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)) { + return cwd + } + } + return gitProjectRoot(filepath.Dir(abs)) +} + +// 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 + } +} + // 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) 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 { - // 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 - } - } + // 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 hasSymlinkAncestorWithCwd(m, cwd, ancestorCache) { + 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 are followed only when they resolve to a regular + // file; see resolveArg for rationale. + if isSymlink && !info.Mode().IsRegular() { + continue + } if info.IsDir() { 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) } } @@ -155,7 +402,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 +412,12 @@ 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. 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 { matcher = NewGitignoreMatcher(dir) @@ -192,11 +429,22 @@ func walkDir(dir string, useGitignore bool, noFollowSymlinks []string) ([]string return err } - if isSkippedSymlink(info, path, noFollowSymlinks) { - if info.IsDir() { - return filepath.SkipDir + // 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 info.Mode()&os.ModeSymlink != 0 { + if !followSymlinks { + return nil + } + // In opt-in mode, follow the link only if it resolves to + // 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 } - return nil } if matcher != nil && isGitignored(matcher, path, info.IsDir()) { @@ -206,7 +454,17 @@ func walkDir(dir string, useGitignore bool, noFollowSymlinks []string) ([]string 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 bf686df3f..526975d26 100644 --- a/internal/lint/files_test.go +++ b/internal/lint/files_test.go @@ -4,8 +4,10 @@ import ( "os" "path/filepath" "sort" + "strings" "testing" + "github.com/jeduden/mdsmith/internal/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -275,60 +277,425 @@ 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 both symlinked files +// and symlinked directories encountered during the walk. +func TestResolveFilesWithOpts_SkipsSymlinksByDefault(t *testing.T) { + skipIfSymlinkUnsupported(t) dir := t.TempDir() 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)) - // Without no-follow-symlinks: all files should be found. - files, err := ResolveFilesWithOpts([]string{dir}, DefaultResolveOpts()) - require.NoError(t, err) - require.Len(t, files, 3) // real.md, link.md, target/doc.md + // 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"))) - // 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) + // 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, "expected 2 files (symlink skipped)") + 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), "link.md should have been skipped (symlink)") + 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") } } -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) { + skipIfSymlinkUnsupported(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)) + + 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)) - 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"))) + 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) { + skipIfSymlinkUnsupported(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) + 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") +} + +// --- 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 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) + assert.False(t, hasSymlinkAncestor( + filepath.Join(outside, "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") +} + +// 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") +} + +// 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") +} + +// 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. +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) + 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_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. 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") +} + +// 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() + 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 +// `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() + testutil.SkipIfSymlinkUnsupported(t) } 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) +} 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) { diff --git a/internal/testutil/symlink.go b/internal/testutil/symlink.go new file mode 100644 index 000000000..26b2bdab5 --- /dev/null +++ b/internal/testutil/symlink.go @@ -0,0 +1,41 @@ +// 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. +// +// 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() + + 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(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) + } +} diff --git a/plan/84_symlink-default-deny.md b/plan/84_symlink-default-deny.md index aacf7d218..9e808a243 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. @@ -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 @@ -83,29 +87,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 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 +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