Implement symlink default-deny security policy (plan 84) - #162
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #162 +/- ##
==========================================
+ Coverage 88.03% 89.42% +1.39%
==========================================
Files 110 111 +1
Lines 14106 11898 -2208
==========================================
- Hits 12418 10640 -1778
+ Misses 1228 798 -430
Partials 460 460 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR implements plan 84 by inverting symlink handling to a secure default: symlinks are skipped during file discovery unless explicitly opted in via --follow-symlinks or follow-symlinks: true in config. It also updates config/CLI interfaces, adds deprecation handling for legacy settings, and expands e2e coverage around symlink security behavior.
Changes:
- Replace pattern-based
no-follow-symlinkswith opt-infollow-symlinksacross config, discovery, and resolution options. - Update CLI flags to
--follow-symlinkswhile silently accepting legacy--no-follow-symlinks; emit config deprecation warnings for legacy YAML keys. - Add/adjust unit + e2e tests and update docs/plans to reflect the new default-deny behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| plan/84_symlink-default-deny.md | Marks plan 84 complete; documents implemented tasks/criteria. |
| PLAN.md | Updates plan index status for plan 84. |
| internal/lint/files.go | Switches symlink behavior to default-deny for walks/globs via FollowSymlinks. |
| internal/lint/files_test.go | Updates resolution tests for default-deny + opt-in behavior (walk + glob). |
| internal/lint/lint_coverage_test.go | Removes obsolete glob/symlink helper tests; updates walkDir call. |
| internal/discovery/discovery.go | Adds FollowSymlinks option and skips symlinks by default in walker. |
| internal/discovery/discovery_coverage_test.go | Updates discovery coverage tests for default-deny + opt-in. |
| internal/config/config.go | Replaces config field with follow-symlinks; adds deprecation plumbing fields. |
| internal/config/load.go | Detects legacy config key and records deprecation warning. |
| internal/config/merge.go | Merges new config fields and carries deprecation messages forward. |
| cmd/mdsmith/main.go | Adds --follow-symlinks, silently accepts legacy flag, prints deprecations, wires resolve/discovery options. |
| cmd/mdsmith/metrics.go | Updates metrics rank to use --follow-symlinks and accept legacy flag. |
| cmd/mdsmith/e2e_symlink_default_deny_test.go | Adds e2e coverage for default-deny, opt-in, and deprecation behavior. |
| cmd/mdsmith/e2e_coverage_test.go | Updates legacy-flag/config e2e tests for new semantics. |
| docs/reference/cli.md | Documents new flag/key behavior and secure default. |
| docs/security/2026-04-05-adversarial-markdown.md | Notes plan 84 status update for symlink default inversion. |
There was a problem hiding this comment.
Pull request overview
Implements plan 84’s “symlink default-deny” posture for mdsmith’s file discovery/resolution so check, fix, and metrics rank skip symlinks unless explicitly opted in via CLI/config. This hardens runs against malicious symlinks redirecting operations outside the project.
Changes:
- Inverts symlink handling to “skip by default” across directory walks, glob expansion, and explicit non-glob path args; adds
--follow-symlinksopt-in. - Replaces pattern-based
no-follow-symlinkswithfollow-symlinks: trueconfig flag; adds deprecation warnings for the legacy config key. - Updates docs and expands e2e/unit coverage for the new behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| plan/84_symlink-default-deny.md | Marks plan 84 complete; records implemented tasks/acceptance criteria. |
| PLAN.md | Marks plan 84 as completed in the plan index. |
| internal/lint/files.go | Implements default-deny symlink behavior in argument resolution, glob expansion, and directory walking. |
| internal/lint/files_test.go | Updates/extends unit tests for default-deny and opt-in behavior (including glob case). |
| internal/lint/lint_coverage_test.go | Removes coverage for deleted helpers; updates walkDir call signature. |
| internal/discovery/discovery.go | Switches discovery options from pattern-based skipping to a FollowSymlinks boolean. |
| internal/discovery/discovery_coverage_test.go | Updates coverage tests to reflect new default-deny behavior and opt-in switch. |
| internal/config/config.go | Replaces no-follow-symlinks setting with follow-symlinks, adds legacy field + deprecation plumbing. |
| internal/config/load.go | Adds detection of legacy config key and records deprecation warning. |
| internal/config/merge.go | Propagates new config fields (FollowSymlinks, legacy/deprecations) through merge/copy. |
| cmd/mdsmith/main.go | Replaces CLI flag with --follow-symlinks, registers legacy flag, prints config deprecations. |
| cmd/mdsmith/metrics.go | Updates metrics rank to use --follow-symlinks and registers the legacy flag. |
| cmd/mdsmith/e2e_symlink_default_deny_test.go | Adds e2e tests for core security behavior + opt-in + fix semantics. |
| cmd/mdsmith/e2e_coverage_test.go | Updates existing e2e coverage tests for legacy flag/key compatibility under new defaults. |
| docs/security/2026-04-05-adversarial-markdown.md | Updates security write-up to reflect plan 84’s resolved status and new defaults. |
| docs/reference/cli.md | Documents --follow-symlinks, secure default behavior, and legacy compatibility notes. |
Comments suppressed due to low confidence (1)
internal/lint/files.go:145
resolveGlobonly skips matches where the final path element is a symlink (viaos.Lstat(m)), butfilepath.Globcan traverse into a symlinked directory component and return non-symlink files underneath it (e.g. patternlinked/*.mdwherelinkedis a symlink to an external dir). In that caseLstat("linked/evil.md")won’t reportModeSymlink, so the external file can still be processed even whenFollowSymlinksis false. To enforce default-deny for glob expansion, also reject matches that have any symlink ancestor (or perform globbing via a walk that never follows symlink dirs) whenFollowSymlinksis false.
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)
}
for _, m := range matches {
// Default-deny symlinks unless the caller opts in.
if !opts.FollowSymlinks {
if linfo, lerr := os.Lstat(m); lerr == nil &&
linfo.Mode()&os.ModeSymlink != 0 {
continue
}
}
info, err := os.Stat(m)
if err != nil {
continue
}
if info.IsDir() {
if err := addDirFiles(m, opts, addFile); err != nil {
return err
}
} else if isMarkdown(m) {
addFile(m)
}
}
There was a problem hiding this comment.
Pull request overview
Implements plan 84’s security hardening by making symlinks default-deny during file discovery/resolution, with an explicit opt-in via CLI/config, plus deprecation handling for legacy settings.
Changes:
- Inverts symlink handling to skip by default across directory walks, glob expansion, and explicit path args; adds
FollowSymlinksplumbing end-to-end. - Replaces legacy pattern-based config/flags with
follow-symlinks(config) /--follow-symlinks(CLI), while silently accepting--no-follow-symlinksand warning on legacy config key usage. - Adds/updates unit + e2e coverage and updates docs/plans to reflect the new security posture.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| plan/84_symlink-default-deny.md | Marks plan 84 complete; updates tasks/acceptance criteria. |
| internal/lint/lint_coverage_test.go | Removes coverage tests tied to the deleted pattern-matching helpers; updates walkDir signature usage. |
| internal/lint/files_test.go | Updates tests to new default-deny + opt-in behavior for ResolveFilesWithOpts. |
| internal/lint/files.go | Core implementation: introduces FollowSymlinks and default-deny enforcement in arg resolution, glob expansion, and walks. |
| internal/discovery/discovery_coverage_test.go | Updates discovery tests for default-deny and opt-in behavior. |
| internal/discovery/discovery.go | Replaces pattern-based no-follow with boolean FollowSymlinks during discovery. |
| internal/config/merge.go | Merges/copies new FollowSymlinks and deprecation-related fields. |
| internal/config/load.go | Detects deprecated top-level key usage and records deprecation messages for CLI emission. |
| internal/config/config.go | Defines new config schema (follow-symlinks) and tracks deprecations. |
| docs/security/2026-04-05-adversarial-markdown.md | Updates security write-up to reflect plan 84 mitigation status. |
| docs/reference/cli.md | Documents --follow-symlinks default behavior and legacy compatibility semantics. |
| cmd/mdsmith/metrics.go | Updates metrics rank flags/options to use --follow-symlinks while accepting legacy --no-follow-symlinks. |
| cmd/mdsmith/main.go | Updates check/fix CLI flags and resolution plumbing; prints config deprecation warnings. |
| cmd/mdsmith/e2e_symlink_default_deny_test.go | Adds e2e tests covering default-deny, opt-in, legacy behavior, and fix safety expectations. |
| cmd/mdsmith/e2e_coverage_test.go | Updates existing e2e coverage tests for legacy-flag acceptance and legacy-config deprecation warnings. |
| PLAN.md | Marks plan 84 as completed in the plan index. |
There was a problem hiding this comment.
Pull request overview
Implements plan 84 by inverting symlink-handling to a secure default-deny across file discovery and resolution, with explicit opt-in via CLI/config and compatibility shims for the legacy flag/key.
Changes:
- Default-deny symlink handling in
internal/lintresolution (directory walks, glob expansion, and explicit path args) with opt-inFollowSymlinks. - Update discovery/config/CLI plumbing to replace
no-follow-symlinkspatterns with afollow-symlinksboolean, plus deprecation warnings and legacy flag parsing. - Add/adjust unit + e2e coverage and update user-facing docs and plan status.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plan/84_symlink-default-deny.md | Marks plan 84 complete; records implemented tasks/acceptance. |
| PLAN.md | Updates plan 84 status in the plan catalog. |
| internal/lint/files.go | Core implementation: skip symlinks by default in arg/glob/walk resolution; adds FollowSymlinks. |
| internal/lint/files_test.go | Updates/extends ResolveFiles tests for new default-deny + opt-in behavior. |
| internal/lint/lint_coverage_test.go | Removes coverage tests for deleted glob-matching helpers; updates walkDir call signature. |
| internal/discovery/discovery.go | Updates discovery walker to default-skip symlinks unless opted in. |
| internal/discovery/discovery_coverage_test.go | Removes tests for deleted pattern-matching helpers; adds coverage for new follow/skip behavior. |
| internal/config/config.go | Replaces NoFollowSymlinks with FollowSymlinks; adds legacy field + Deprecations. |
| internal/config/load.go | Adds top-level key probing and records deprecation warnings for legacy config key. |
| internal/config/merge.go | Merges new config fields (FollowSymlinks, legacy + deprecations). |
| cmd/mdsmith/main.go | Replaces CLI flag with --follow-symlinks; keeps legacy flag; threads symlink policy via walkCLI; prints deprecations. |
| cmd/mdsmith/metrics.go | Updates metrics rank flags/options to use follow-symlinks + legacy flag parsing. |
| cmd/mdsmith/e2e_symlink_default_deny_test.go | Adds new e2e suite validating default-deny, opt-in, legacy behavior, and fix semantics. |
| cmd/mdsmith/e2e_coverage_test.go | Updates existing e2e coverage cases for legacy flag/key + new defaults. |
| docs/reference/cli.md | Documents --follow-symlinks, secure default, legacy behavior, and deprecation warning. |
| docs/security/2026-04-05-adversarial-markdown.md | Updates security note to reflect the resolved symlink-default-deny finding. |
There was a problem hiding this comment.
Pull request overview
Implements plan 84’s “symlink default-deny” policy so mdsmith skips symlinks by default during discovery and file resolution, with explicit opt-in via CLI/config to follow symlinked files.
Changes:
- Invert symlink handling default to “skip”, introducing
FollowSymlinksacross config, discovery, and resolution paths. - Replace CLI flag
--no-follow-symlinkswith--follow-symlinks, while keeping the old flag/config key for compatibility plus deprecation messaging. - Add/adjust unit + e2e coverage for the new behavior across
check,fix, andmetrics rank.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| plan/84_symlink-default-deny.md | Marks plan 84 complete and records implemented tasks/acceptance criteria. |
| PLAN.md | Updates plan index to reflect plan 84 completion. |
| internal/lint/files.go | Implements default-deny symlink handling in explicit args, globs, and directory walks via FollowSymlinks. |
| internal/lint/files_test.go | Updates/introduces resolution tests for default-deny and opt-in behavior. |
| internal/lint/lint_coverage_test.go | Removes now-obsolete glob/symlink skip unit tests and updates walkDir signature usage. |
| internal/discovery/discovery.go | Switches discovery options to FollowSymlinks and applies symlink skipping in walker. |
| internal/discovery/discovery_coverage_test.go | Updates discovery walker coverage tests to match the new symlink semantics. |
| internal/config/config.go | Replaces no-follow-symlinks patterns with follow-symlinks boolean; adds deprecation plumbing fields. |
| internal/config/load.go | Detects deprecated config key presence and records deprecation warnings. |
| internal/config/merge.go | Merges FollowSymlinks and carries deprecation info through merged config. |
| cmd/mdsmith/main.go | Adds --follow-symlinks, registers legacy flag, threads unified walk options, and prints deprecation warnings. |
| cmd/mdsmith/metrics.go | Updates metrics rank to use --follow-symlinks and accept legacy flag. |
| cmd/mdsmith/e2e_symlink_default_deny_test.go | Adds e2e scenarios for default-deny, opt-in, legacy behavior, and fix safety. |
| cmd/mdsmith/e2e_coverage_test.go | Updates existing e2e coverage tests to align with the new defaults and legacy acceptance. |
| docs/reference/cli.md | Updates CLI reference for the new flag and documents default-deny behavior and deprecations. |
| docs/security/2026-04-05-adversarial-markdown.md | Updates security doc status for the symlink issue to reflect plan 84 changes. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Implements plan 84 by flipping symlink handling to a secure default: symlinks are skipped during discovery/walks unless explicitly opted-in via CLI/config, with backward-compatible parsing for legacy flags/config keys.
Changes:
- Replace pattern-based
NoFollowSymlinkswith opt-inFollowSymlinksacross config, resolution, and discovery. - Update CLI flags to
--follow-symlinkswhile silently accepting legacy--no-follow-symlinks; emit deprecation warning for legacy config key. - Add/adjust unit + e2e tests to cover default-deny behavior, opt-in behavior, and legacy deprecation.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| plan/84_symlink-default-deny.md | Marks plan 84 complete and documents implementation details. |
| internal/lint/lint_coverage_test.go | Removes tests for deleted glob helpers; updates walkDir signature usage. |
| internal/lint/files_test.go | Updates resolver tests for new default-deny + opt-in behavior. |
| internal/lint/files.go | Implements default-deny symlink behavior in arg/gob/walk resolution. |
| internal/discovery/discovery_coverage_test.go | Updates discovery tests to default-deny/opt-in symlink semantics. |
| internal/discovery/discovery.go | Removes pattern-based no-follow logic; adds FollowSymlinks flag to walker. |
| internal/config/merge.go | Merges new FollowSymlinks + deprecation metadata into final config. |
| internal/config/load.go | Adds top-level key probing and records deprecation warnings for legacy key. |
| internal/config/config.go | Replaces NoFollowSymlinks with FollowSymlinks; adds legacy capture + warnings. |
| docs/security/2026-04-05-adversarial-markdown.md | Updates security writeup to reflect plan 84 mitigations. |
| docs/reference/cli.md | Documents new --follow-symlinks and legacy behavior. |
| cmd/mdsmith/metrics.go | Updates metrics rank to use new follow/legacy flag plumbing. |
| cmd/mdsmith/main.go | Threads new symlink options via walkCLI; prints config deprecations. |
| cmd/mdsmith/e2e_symlink_default_deny_test.go | Adds comprehensive e2e coverage for symlink default-deny policy. |
| cmd/mdsmith/e2e_coverage_test.go | Updates existing e2e tests for new flags and deprecation behavior. |
| PLAN.md | Marks plan 84 as completed. |
There was a problem hiding this comment.
Pull request overview
Implements plan 84’s symlink default-deny policy across file discovery, glob resolution, and directory walks, requiring an explicit opt-in (--follow-symlinks / follow-symlinks: true) to follow symlinked files.
Changes:
- Inverts symlink behavior to default-deny across lint file resolution and discovery, with explicit opt-in support.
- Replaces pattern-based
no-follow-symlinksconfig/flags with a booleanfollow-symlinks, while accepting legacy inputs and emitting config deprecation warnings. - Adds/updates unit + e2e coverage (including fix-side behavior and legacy compatibility paths).
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| plan/84_symlink-default-deny.md | Marks plan 84 complete and records implemented tasks/acceptance criteria. |
| PLAN.md | Updates plan index status for plan 84 to ✅. |
| internal/lint/files.go | Implements default-deny symlink handling for explicit args, glob expansion, and directory walks; adds ancestor-symlink-dir rejection. |
| internal/lint/files_test.go | Updates/extends ResolveFiles tests to validate new FollowSymlinks behavior and default-deny. |
| internal/lint/lint_coverage_test.go | Removes coverage tests for removed glob-matching helpers; updates walkDir signature usage. |
| internal/discovery/discovery.go | Updates discovery walker to default-deny symlink entries unless opted in. |
| internal/discovery/discovery_coverage_test.go | Updates discovery coverage tests for new FollowSymlinks behavior and removes removed-helper tests. |
| internal/config/config.go | Replaces NoFollowSymlinks with FollowSymlinks; adds LegacyNoFollowSymlinks and Deprecations fields. |
| internal/config/load.go | Detects deprecated no-follow-symlinks key presence and records deprecation warnings. |
| internal/config/merge.go | Merges/copies FollowSymlinks + deprecation-related fields. |
| cmd/mdsmith/main.go | Replaces CLI surface with --follow-symlinks, threads precedence via walkCLI, and prints config deprecation warnings. |
| cmd/mdsmith/metrics.go | Updates metrics rank to use --follow-symlinks + legacy flag parsing and precedence. |
| cmd/mdsmith/e2e_test.go | Adds skipIfSymlinkUnsupported helper for symlink-dependent e2e tests. |
| cmd/mdsmith/e2e_symlink_default_deny_test.go | Adds comprehensive e2e coverage for default-deny, opt-in, legacy config warning, and fix-side behavior. |
| cmd/mdsmith/e2e_coverage_test.go | Updates existing e2e coverage tests for legacy-flag acceptance and legacy-config deprecation warnings. |
| docs/reference/cli.md | Documents --follow-symlinks and the new default-deny behavior + legacy compatibility notes. |
| docs/security/2026-04-05-adversarial-markdown.md | Updates threat model doc to reflect plan 84’s default-deny behavior and related protections. |
Invert the symlink-follow default to protect `check` and `fix` from a malicious symlink pointing outside the project. - config.Config: FollowSymlinks bool replaces the pattern-based NoFollowSymlinks slice; LegacyNoFollowSymlinks retains the old key so it still parses, and Deprecations surfaces a warning. - lint.ResolveOpts, walkDir, resolveGlob: FollowSymlinks bool controls both directory walks and glob expansion; the zero value skips all symlinks. - discovery.Options/walker: same shape; the `matchesPath` glob helper is no longer needed. - cmd/mdsmith: new --follow-symlinks flag on check/fix/metrics; the old --no-follow-symlinks flag is registered as a silent deprecation via a shared helper. loadConfig prints any config-level deprecation warnings once per invocation. - Tests: red->green for the new behavior across both unit and e2e suites; legacy-flag/config tests retained and renamed to document the silent-acceptance path.
- docs/reference/cli.md: document --follow-symlinks opt-in and the deprecation status of --no-follow-symlinks. - docs/security/2026-04-05-adversarial-markdown.md: annotate the symlink finding with the plan 84 mitigation. - plan 84 AC checked off; task list notes the concrete landing. - PLAN.md catalog regenerated.
- internal/lint/files.go: resolveArg now Lstats non-glob paths and skips symlinks unless FollowSymlinks is set. Without this, `mdsmith check ./evil.md` bypassed the default-deny policy and still processed the symlink target. - cmd/mdsmith/e2e_symlink_default_deny_test.go: new test TestE2E_Symlink_DefaultDeny_ExplicitFileArg exercises both the default-deny and --follow-symlinks opt-in on an explicit path. - docs/reference/cli.md: note covers explicit file arguments, not just walks and globs. - docs/security/2026-04-05-adversarial-markdown.md: reword the original-finding paragraph so it no longer contradicts the plan 84 status line below. - internal/config/config.go: clarify that the `omitempty` tag on LegacyNoFollowSymlinks keeps round-tripped configs from re-emitting the deprecated key unless the user supplied it.
- internal/config/load.go: parse YAML only twice instead of four times by extracting topLevelKeySet(data). The previous code called yamlHasKey twice, each time re-running RejectYAMLAliases and yaml.Unmarshal over the same bytes. yamlHasKey now wraps topLevelKeySet for the existing test. - cmd/mdsmith: the deprecated --no-follow-symlinks flag is no longer silent — it forces FollowSymlinks off even when `follow-symlinks: true` is set in config, giving users a way to run securely without editing the config. Precedence (highest wins): --no-follow-symlinks, then --follow-symlinks, then the config key. - cmd/mdsmith: bundle the three walk-related CLI flags into a walkCLI struct so helpers thread one value instead of growing a fourth bool parameter. - cmd/mdsmith/e2e_symlink_default_deny_test.go: new test TestE2E_Symlink_LegacyFlag_OverridesFollowConfig pins the force-deny semantics against a config that enables follow.
Copilot's third-round comments all converge on one fact: filepath.Walk
is Lstat-based and never descends into a symlink root. That makes a
few places misleading:
- resolveArg / resolveGlob: if FollowSymlinks is true and an explicit
arg or glob match is a symlink-to-dir, os.Stat classifies it as a
directory but walkDir silently walks nothing. Now the symlink-to-
directory case is skipped unconditionally, and the doc comment on
ResolveOpts.FollowSymlinks spells out that the flag applies to
file symlinks only.
- walkDir / discovery.walker: the `if info.IsDir() { return SkipDir }`
branch inside the symlink guard was dead because Walk's Lstat-based
info never reports IsDir==true for a symlink. Dropped and commented.
- discovery Options.FollowSymlinks: doc comment now notes that
symlinked directories are never descended into even when the flag
is on, matching production behavior.
- discovery coverage test: the synthetic fakeFileInfo-with-isDir=true
pair asserted the now-dead SkipDir branch and gave false confidence.
Replaced with a real os.Symlink fixture + os.Lstat info. The
unused fakeFileInfo helper is removed.
- new e2e test TestE2E_Symlink_DirSymlinkAlwaysSkipped pins the
symlink-to-directory semantics under --follow-symlinks.
hasSymlinkAncestorWithCwd called filepath.Abs(path), which calls os.Getwd internally for relative paths and ignores the cwd argument. The precompute-cwd optimisation in resolveGlob was a no-op: every match still incurred the per-call syscall, and behavior depended on the process cwd rather than the caller- supplied cwd. Added absWithCwd: for absolute paths, just clean; for relative paths, join with the supplied cwd; only fall back to filepath.Abs when cwd is empty (Getwd error upstream). TestHasSymlinkAncestorWithCwd_HonorsCwdArg pins the new contract: resolve against the supplied cwd, not the process cwd, by passing a cwd argument that differs from the test's working directory.
There was a problem hiding this comment.
Pull request overview
This PR implements plan 84’s “default-deny” symlink policy across mdsmith’s file resolution and discovery paths, adding an opt-in follow-symlinks configuration and a tri-state --follow-symlinks CLI flag to allow controlled symlink handling.
Changes:
- Invert symlink behavior to “skip by default” across explicit args, glob expansion, directory walks, and discovery; opt-in only for symlinks that resolve to regular files.
- Replace legacy
no-follow-symlinksconfig behavior withfollow-symlinks(legacy key still parses and emits a deprecation warning); remove--no-follow-symlinksflag. - Add extensive unit + e2e test coverage, including Windows/sandbox gating via a shared symlink capability probe.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| plan/84_symlink-default-deny.md | Marks plan complete; documents final migration decisions (flag removal, defaults). |
| PLAN.md | Updates plan 84 status to ✅ in the catalog. |
| internal/testutil/symlink.go | Adds shared SkipIfSymlinkUnsupported helper for symlink-capability-gated tests. |
| internal/lint/files.go | Core implementation: default-deny symlink logic in resolveArg/resolveGlob/walkDir + ancestor-symlink detection. |
| internal/lint/files_test.go | Adds unit tests for FollowSymlinks behavior and ancestor-symlink detection + caching. |
| internal/lint/files_unix_test.go | Adds non-regular file (FIFO) coverage and symlink-to-non-regular edge cases (non-Windows). |
| internal/lint/lint_coverage_test.go | Removes coverage tests for deleted glob matcher helpers; updates walkDir call signature. |
| internal/discovery/discovery.go | Applies default-deny symlink behavior in discovery walker; opts in via Options.FollowSymlinks. |
| internal/discovery/discovery_coverage_test.go | Updates discovery coverage tests to match new symlink defaults + opt-in behavior. |
| internal/config/config.go | Replaces NoFollowSymlinks with FollowSymlinks + legacy parsing field + Deprecations storage. |
| internal/config/load.go | Adds top-level key probing for deprecated keys + emits deprecation warnings via cfg.Deprecations. |
| internal/config/merge.go | Threads FollowSymlinks + legacy/deprecation fields through merged config. |
| cmd/mdsmith/main.go | Adds tri-state --follow-symlinks plumbing and deprecation warning printing. |
| cmd/mdsmith/main_unit_test.go | Updates resolveOpts tests for new FollowSymlinks/tri-state behavior; adds printDeprecations tests. |
| cmd/mdsmith/metrics.go | Threads tri-state --follow-symlinks into metrics rank file resolution. |
| cmd/mdsmith/e2e_test.go | Exposes a local skipIfSymlinkUnsupported wrapper for e2e tests. |
| cmd/mdsmith/e2e_symlink_default_deny_test.go | Adds comprehensive e2e coverage for default-deny, opt-in, override semantics, and fix behavior. |
| cmd/mdsmith/e2e_coverage_test.go | Removes old --no-follow-symlinks e2e coverage; adds legacy-config deprecation coverage. |
| docs/reference/cli.md | Documents new flag/config semantics and tri-state behavior; removes old flag mention. |
| docs/security/2026-04-05-adversarial-markdown.md | Updates security write-up to reflect plan 84 mitigation details and status. |
Real bypass: `linked/../dirty.md` where `linked` is a symlinked directory. filepath.Clean collapses `linked/..` lexically, erasing the symlink component from the ancestor scan. The kernel, however, still traverses `linked` when resolving the leaf — so resolveArg / resolveGlob would see a regular file and happily process an external target even with FollowSymlinks=false. Fix: containsDotDotAfterName flags any path containing `..` after a non-`..` segment, and hasSymlinkAncestorWithCwd returns true (skip) for such inputs. Leading `../` — a legitimate way to name paths above cwd — is unaffected because there is no named segment before it for `..` to mask. Tests: - TestContainsDotDotAfterName tables the helper. - TestHasSymlinkAncestorWithCwd_RejectsDotDotAfterName pins the wiring. - TestE2E_Symlink_DotDotAfterSymlinkedDir is the end-to-end guard: `mdsmith check linked/../dirty.md` against a symlinked ancestor does not process the external target.
There was a problem hiding this comment.
Pull request overview
Implements plan 84’s security hardening by switching file discovery/resolution to a default-deny symlink policy across lint resolution (ResolveFiles*), config discovery (discovery.Discover), and CLI plumbing, with opt-in via follow-symlinks: true (config) or --follow-symlinks (CLI tri-state override).
Changes:
- Replace pattern-based
no-follow-symlinksopt-out with booleanFollowSymlinksopt-in across config, lint resolution, and discovery. - Add symlink-ancestor rejection for glob matches and explicit args to prevent “symlinked dir component” bypasses.
- Add shared symlink test gating helper and expand unit + e2e coverage for default-deny, opt-in, legacy config deprecation, and write-side safety.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| plan/84_symlink-default-deny.md | Marks plan 84 complete; documents flag/key migration and acceptance criteria. |
| internal/testutil/symlink.go | Adds shared SkipIfSymlinkUnsupported helper to gate symlink tests on host capabilities. |
| internal/lint/lint_coverage_test.go | Updates coverage tests to match new walkDir signature and removes obsolete glob/symlink-pattern tests. |
| internal/lint/files_unix_test.go | Adds Unix-only tests to ensure FIFOs (and symlink-to-FIFO) are never enqueued, even under opt-in. |
| internal/lint/files_test.go | Adds/updates unit tests for default-deny, opt-in, glob behavior, and symlink-ancestor helpers. |
| internal/lint/files.go | Implements default-deny symlink policy, symlink-ancestor detection, and non-regular-file filtering across resolve paths. |
| internal/discovery/discovery_coverage_test.go | Updates discovery coverage tests for default-deny and opt-in behavior using real Lstat semantics. |
| internal/discovery/discovery.go | Switches discovery walker to FollowSymlinks opt-in and enforces “symlinks to regular files only”. |
| internal/config/merge.go | Threads FollowSymlinks, legacy key capture, and deprecation warnings through merged config. |
| internal/config/load.go | Detects deprecated no-follow-symlinks key presence and records deprecation warnings. |
| internal/config/config.go | Replaces NoFollowSymlinks with FollowSymlinks; adds legacy field + Deprecations. |
| docs/security/2026-04-05-adversarial-markdown.md | Updates security doc to reflect plan 84 mitigation and current behavior. |
| docs/reference/cli.md | Updates CLI reference to new --follow-symlinks behavior and tri-state semantics. |
| cmd/mdsmith/metrics.go | Threads tri-state follow-symlinks into metrics rank file resolution. |
| cmd/mdsmith/main_unit_test.go | Updates resolveOpts tests for tri-state follow-symlinks and adds printDeprecations tests. |
| cmd/mdsmith/main.go | Adds walkCLI bundle, tri-state --follow-symlinks, and prints config deprecations once per run. |
| cmd/mdsmith/e2e_test.go | Adds local alias wrapper for shared symlink test gating helper. |
| cmd/mdsmith/e2e_symlink_default_deny_test.go | Adds e2e coverage for default-deny, opt-in, ancestor/glob bypasses, legacy config warning, and fix behavior. |
| cmd/mdsmith/e2e_coverage_test.go | Removes obsolete --no-follow-symlinks tests and adds legacy config deprecation coverage. |
| PLAN.md | Marks plan 84 as complete in the plan index. |
The previous guard rejected any path with '..' after a named segment — catching `linked/../dirty.md` but also sweeping up legitimate `a/b/../c.md` into the silent-skip bucket. Copilot flagged the UX regression. Replace both containsDotDotAfterName and the upward-chain ancestorChainHasSymlink with a single component walk that preserves the kernel's resolution semantics: walk each segment of the ORIGINAL path left-to-right; `..` pops `current` up physically; each named segment is Lstat'd against the accumulated path. The cache now keys on those accumulated paths so sibling globs under a shared ancestor still share work. Outcome: - `linked/../dirty.md` with symlinked `linked` → probes `linked`, detects symlink, rejects. Correct. - `a/b/../c.md` with no symlinks → probes `a` and `a/b`, finds nothing, passes through. No more false reject. - `../project/foo.md` (escape-to-sibling) → walker starts at cwd, `..` pops out, `project` probed. Stop boundary still keeps probes inside cwd or a .git tree. Unit tests updated to the new semantic: - TestHasSymlinkAncestor_DotDotAfterSymlinkedDir — real symlink + `..` → flagged. - TestHasSymlinkAncestor_DotDotAfterRealDir — no symlinks + `..` → NOT flagged. - TestHasSymlinkAncestorCache_SkipsRepeatLstat — shared ancestor cached across sibling paths. Removed TestAncestorChainHasSymlink_CacheShortcircuits and TestContainsDotDotAfterName + TestHasSymlinkAncestorWithCwd_RejectsDotDotAfterName since the helpers they tested are gone.
Round 20's component walk added a few branches the existing tests didn't exercise: - absWithCwd Getwd fallback (empty cwd). - hasSymlinkAncestorWithCwd Getwd fallback for `current` when cwd is empty and path is relative. - isDescendantOf `p == base` branch (used by the walker to stop at stop boundary). - Cross-sibling cache hit under a shared symlinked ancestor. Added unit tests that pin each branch.
There was a problem hiding this comment.
Pull request overview
Implements plan 84’s security hardening by default-denying symlink traversal across mdsmith’s file resolution and discovery paths, with an explicit opt-in via --follow-symlinks / follow-symlinks: true, plus deprecation handling for the legacy config key.
Changes:
- Default-deny symlinks across explicit args, glob expansion, directory walks, and discovery; opt-in only follows symlinks that resolve to regular files.
- Replace legacy
no-follow-symlinksbehavior withFollowSymlinks(config + CLI tri-state), emit deprecation warnings, and remove the old--no-follow-symlinksflag. - Add/expand unit + e2e coverage for symlink edge cases (ancestor traversal, broken links, FIFOs, fix behavior).
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plan/84_symlink-default-deny.md | Marks plan complete; updates tasks/acceptance criteria and clarifies CLI flag removal. |
| internal/testutil/symlink.go | Adds a shared test helper to skip symlink tests on unsupported hosts. |
| internal/lint/lint_coverage_test.go | Updates coverage tests to match new walkDir signature and removes obsolete symlink/glob helper tests. |
| internal/lint/files_unix_test.go | Adds UNIX-only tests ensuring non-regular files (FIFOs) are never enqueued, even under symlink opt-in. |
| internal/lint/files_test.go | Adds extensive tests for default-deny, opt-in behavior, and symlink-ancestor detection/caching. |
| internal/lint/files.go | Implements default-deny + opt-in symlink handling and adds hasSymlinkAncestor* logic for ancestor traversal blocking. |
| internal/discovery/discovery_coverage_test.go | Updates discovery tests to reflect default-deny and opt-in follow semantics. |
| internal/discovery/discovery.go | Switches discovery options to FollowSymlinks and applies Lstat-based symlink filtering. |
| internal/config/merge.go | Threads FollowSymlinks, legacy key capture, and deprecation warnings through config merges/copies. |
| internal/config/load.go | Detects deprecated no-follow-symlinks key presence and records deprecation warnings. |
| internal/config/config.go | Replaces NoFollowSymlinks with FollowSymlinks and adds legacy field + deprecations field. |
| docs/security/2026-04-05-adversarial-markdown.md | Updates security write-up to reflect plan 84’s resolved symlink behavior. |
| docs/reference/cli.md | Documents default-deny behavior and tri-state --follow-symlinks semantics; updates metrics rank flags list. |
| cmd/mdsmith/metrics.go | Adds tri-state --follow-symlinks support to metrics rank via shared walk options. |
| cmd/mdsmith/main_unit_test.go | Updates resolve options tests for new follow-symlinks tri-state + deprecation printing. |
| cmd/mdsmith/main.go | Implements tri-state CLI plumbing, threads FollowSymlinks into lint/discovery, and prints deprecations once per run. |
| cmd/mdsmith/e2e_test.go | Adds a local helper aliasing the shared symlink-skip probe. |
| cmd/mdsmith/e2e_symlink_default_deny_test.go | Adds comprehensive e2e coverage for default-deny, opt-in, ancestor traversal, legacy key warning, and fix behavior. |
| cmd/mdsmith/e2e_coverage_test.go | Removes obsolete --no-follow-symlinks tests and adds legacy-config deprecation coverage. |
| PLAN.md | Marks plan 84 complete in the plan index. |
Two cross-platform concerns from Copilot: 1. hasSymlinkAncestorWithCwd initialised `current` at `string(filepath.Separator)` for absolute paths and then filepath.Join'd each segment of the raw path. On Windows, absolute paths carry a volume/UNC prefix (`C:\...`, `\\host\share\...`). Starting at `/` and Joining `C:\foo` segments produces invalid intermediate paths, which would miss symlink ancestors. Now use filepath.VolumeName to seed `current` with the volume root and strip the prefix before splitting segments. 2. isDescendantOf used a raw `strings.HasPrefix` check against base+separator. On Windows, separator normalisation and case differences between cwd and the reconstructed `current` could incorrectly classify a path inside the boundary as outside. Switched to filepath.Rel + a "!..prefix" check, which handles separator normalisation. Case-insensitivity on Windows still requires the caller to pass consistently- cased paths (which they do, since `current` is built from `cwd` + Join'd segments).
There was a problem hiding this comment.
Pull request overview
Implements plan 84’s “symlink default-deny” policy across mdsmith’s file resolution and discovery paths, reducing the risk of processing or overwriting files outside the intended project via malicious symlink redirects.
Changes:
- Invert symlink behavior to default-deny across explicit args, directory walks, glob expansion, and config-driven discovery; opt-in via
follow-symlinks: trueor--follow-symlinks(tri-state override). - Replace legacy
no-follow-symlinksbehavior with a deprecated config key warning and remove the legacy--no-follow-symlinksflag. - Add extensive unit + e2e coverage and shared symlink-capability gating for portability.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| plan/84_symlink-default-deny.md | Marks plan complete and documents final migration decisions (including removal of --no-follow-symlinks). |
| PLAN.md | Updates plan 84 status to ✅ in the plan index. |
| internal/testutil/symlink.go | Adds a shared test helper to skip symlink tests on hosts without symlink support. |
| internal/lint/files.go | Implements default-deny symlink handling in resolveArg/resolveGlob/walkDir and adds symlink-ancestor detection/memoization. |
| internal/lint/files_test.go | Adds focused unit tests for follow/deny behavior, symlink-ancestor scanning, and caching. |
| internal/lint/files_unix_test.go | Adds non-Windows coverage for skipping FIFOs and symlinks-to-non-regular targets. |
| internal/lint/lint_coverage_test.go | Updates coverage tests to new walkDir signature and removes obsolete pattern-based symlink tests. |
| internal/discovery/discovery.go | Updates discovery walker to default-deny symlinks and opt-in only for symlinks to regular files. |
| internal/discovery/discovery_coverage_test.go | Updates/extends coverage tests for new FollowSymlinks behavior and adds symlink support gating. |
| internal/config/config.go | Replaces NoFollowSymlinks with FollowSymlinks and adds legacy + deprecation tracking fields. |
| internal/config/load.go | Emits deprecation warnings when legacy no-follow-symlinks key is present; refactors key-presence detection. |
| internal/config/merge.go | Threads FollowSymlinks + legacy/deprecation fields through config merge/copy logic. |
| cmd/mdsmith/main.go | Adds tri-state --follow-symlinks plumbing via walkCLI, applies to check/fix/discovery, and prints config deprecations once per run. |
| cmd/mdsmith/main_unit_test.go | Updates resolveOpts tests for new tri-state follow behavior; adds printDeprecations tests. |
| cmd/mdsmith/metrics.go | Updates metrics rank to use tri-state --follow-symlinks and shared walkCLI threading. |
| cmd/mdsmith/e2e_test.go | Wires the shared symlink capability skip helper for e2e tests. |
| cmd/mdsmith/e2e_symlink_default_deny_test.go | Adds comprehensive e2e coverage for default-deny, opt-in, ancestor traversal rejection, legacy flag removal, and fix/write-side behavior. |
| cmd/mdsmith/e2e_coverage_test.go | Updates e2e coverage for legacy config deprecation; removes old --no-follow-symlinks coverage. |
| docs/reference/cli.md | Updates CLI docs to describe --follow-symlinks tri-state semantics and the new secure default. |
| docs/security/2026-04-05-adversarial-markdown.md | Updates the security writeup to reflect plan 84’s resolved symlink behavior and current mitigations. |
Added focused unit tests for the plan-84-specific branches that neither unit nor e2e paths exercised after the round-20 refactor: internal/lint: - Test*_SymlinkedAncestorPath_Skipped covers the hasSymlinkAncestor guard in resolveArg. - Test*_SymlinkFileNoFollow_Skipped covers the isSymlink && !FollowSymlinks early return in resolveArg. - Test*_BrokenSymlink_SilentlySkipped covers the symlink-with- Stat-err path. - Test*_NonexistentNonSymlink_Errors covers the non-symlink Stat-err branch (missing arg surfaces as an error). - TestAbsWithCwd_AbsolutePath_Clean covers the absolute-path branch of absWithCwd. - TestIsDescendantOf extended with "ancestor-of-base" and "self" branches. - TestHasSymlinkAncestor_NoSymlinkButCached covers the cache-false `continue` branch in the component walk. - TestHasSymlinkAncestorWithCwd_AbsolutePath_UnderCwd exercises the VolumeName-aware absolute-path init. internal/discovery: - discovery_unix_test.go (new, !windows): TestDiscover_SkipsFifoEntries and TestDiscover_SymlinkToFifo_SkippedUnderOptIn cover the FIFO and symlink-to-FIFO skip branches in the walker. internal/config: - TestTopLevelKeySet_InvalidYAML + _NotAMapping cover the parse error branches. - TestLoad_LegacyNoFollowSymlinksEmitsDeprecation covers the deprecation-accumulation branch in Load.
There was a problem hiding this comment.
Pull request overview
Implements plan 84’s “default-deny” symlink security posture across mdsmith’s file resolution and discovery paths, so check/fix won’t follow symlinks (or traverse symlinked directory ancestors) unless explicitly opted in.
Changes:
- Switched symlink handling to default-deny across explicit args, glob expansion, directory walks, and discovery; added ancestor-symlink detection for path/glob bypasses.
- Replaced legacy pattern-based config (
no-follow-symlinks) withfollow-symlinks: trueopt-in + CLI tri-state--follow-symlinksoverride behavior; removed--no-follow-symlinks. - Added/updated unit and E2E coverage for symlink default-deny, opt-in behavior, legacy config deprecation warnings, and non-regular file skipping.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| plan/84_symlink-default-deny.md | Marks plan 84 complete; updates tasks/acceptance criteria to match implemented behavior. |
| internal/testutil/symlink.go | Adds shared test helper to skip symlink-dependent tests on unsupported hosts. |
| internal/lint/lint_coverage_test.go | Updates coverage tests to reflect new walkDir signature and removes tests for deleted helpers. |
| internal/lint/files_unix_test.go | Adds Unix-only tests ensuring non-regular files (FIFOs) and symlink-to-FIFO are skipped. |
| internal/lint/files_test.go | Adds extensive unit tests for FollowSymlinks default/opt-in and symlink-ancestor detection helpers. |
| internal/lint/files.go | Implements default-deny symlink policy, symlink-ancestor scanning, and non-regular file skipping in resolveArg/resolveGlob/walkDir. |
| internal/discovery/discovery_unix_test.go | Adds Unix-only discovery tests for FIFO and symlink-to-FIFO behavior. |
| internal/discovery/discovery_coverage_test.go | Updates discovery coverage tests for default-deny + FollowSymlinks opt-in and removes legacy pattern-matching tests. |
| internal/discovery/discovery.go | Threads FollowSymlinks option into discovery walker; default-deny symlink behavior aligned with lint resolution. |
| internal/config/merge.go | Updates merge/copy logic to include FollowSymlinks, legacy symlink key capture, and deprecation warnings list. |
| internal/config/load.go | Adds top-level key set parsing to detect deprecated no-follow-symlinks and emit a deprecation warning. |
| internal/config/config_test.go | Adds tests for topLevelKeySet and legacy no-follow-symlinks deprecation emission. |
| internal/config/config.go | Replaces NoFollowSymlinks with FollowSymlinks; adds LegacyNoFollowSymlinks + Deprecations fields for migration messaging. |
| docs/security/2026-04-05-adversarial-markdown.md | Updates security assessment to document plan 84 resolution and current behavior/locations. |
| docs/reference/cli.md | Documents --follow-symlinks tri-state semantics and default-deny behavior; removes old flag mention. |
| cmd/mdsmith/metrics.go | Adds --follow-symlinks tri-state plumbing to metrics rank. |
| cmd/mdsmith/main_unit_test.go | Updates resolveOpts tests for new FollowSymlinks config + CLI override and adds printDeprecations tests. |
| cmd/mdsmith/main.go | Adds tri-state --follow-symlinks to check/fix; threads walk options via walkCLI; prints config deprecations once per run. |
| cmd/mdsmith/e2e_test.go | Imports shared symlink-skip helper via a local alias for E2E tests. |
| cmd/mdsmith/e2e_symlink_default_deny_test.go | Adds comprehensive E2E test suite for default-deny, opt-in, ancestor traversal blocking, legacy config warning, and fix-side behavior. |
| cmd/mdsmith/e2e_coverage_test.go | Removes old --no-follow-symlinks E2E coverage and adds E2E assertion for legacy config deprecation warning. |
| PLAN.md | Marks plan 84 as complete in the plan index. |
|
🟢 Merge Queue — picked up This PR is in the queue and will be batched with other Next: No action needed — you'll get another comment when CI starts on the batch. View merge queue run. |
|
🔵 Merge Queue — CI running Merged into batch branch Next: No action needed — you'll be notified when CI completes. |
|
✅ Merge Queue — merged This PR landed on Next: Done — nothing more to do here. |
Summary
Implements plan 84: symlinks are now skipped by default during file discovery, glob expansion, and explicit path resolution. Users opt in with
--follow-symlinks(CLI) orfollow-symlinks: true(config) — protectingcheckandfixfrom a malicious symlink redirecting them at files outside the project.Key Changes
Default-deny across every entry point:
internal/lint/walkDir,internal/discovery/walker.visit).resolveGlob) — including matches whose ancestor chain crosses a symlinked directory (linked/*.md).resolveArg), including absolute paths inside cwd, paths via.., and paths into other.git-rooted projects.--follow-symlinks. The flag opts in to symlinked files only —filepath.Walkis Lstat-based and can't recurse a symlink root.Configuration:
NoFollowSymlinks []string(pattern-based) withFollowSymlinks bool(opt-in) inconfig.Config.lint.ResolveOptsanddiscovery.Options.no-follow-symlinks:config key still parses and emits a deprecation warning on stderr.CLI:
--follow-symlinksflag oncheck,fix, andmetrics rank. Tri-state: omitted falls back to config;--follow-symlinks=trueforces opt-in;--follow-symlinks=falseforces deny — even over a config that has opted in. The latter is the secure-one-off-run knob.--no-follow-symlinksflag has been removed outright (the polarity is redundant under the new default; passing it now produces a parse error).Performance / correctness:
hasSymlinkAncestorwalks each path's ancestor chain anchored at cwd or, if the path is outside cwd, the nearest.gitproject root. System paths above the boundary are never probed — so/tmpon macOS and other system-level symlinks don't misclassify benign paths.hasSymlinkAncestorCached) so large globs don't repeat Lstat calls on shared ancestors.Test coverage (
cmd/mdsmith/e2e_symlink_default_deny_test.go):linked/...), absolute paths inside cwd, abs paths into a sibling.gitproject, and..-relative paths..md.follow-symlinks: trueconfig opt-in.--follow-symlinks=falseoverrides config opt-in.no-follow-symlinks:config deprecation warning.--no-follow-symlinksflag now errors as unknown.fixwrite-side TOCTOU behavior (symlink replaced via atomic rename, target untouched).skipIfSymlinkUnsupportedhelper for Windows / sandboxed CI.Acceptance Criteria Met
--follow-symlinksflag enables symlink following;--follow-symlinks=falseforces denyfollow-symlinks: truein config enables symlink followingno-follow-symlinksconfig emits deprecation warning; old--no-follow-symlinksflag is removedcheckandfixrespect the settinginternal/corpussigning failures tracked by plan 90)https://claude.ai/code/session_0131atyQR7nTyFzzbQ9i8kAG