fix(preset): allow symlinked extends targets outside the config directory#346
Conversation
intent(preset): support overlay-style configurations where a relative extends reference is a symlink to a file managed in a separate repository, without forcing users to fall back to absolute `~/` paths decision(preset): validate the reference text only — collapse `.` and `..` lexically and check that the result stays inside the resolution root, without canonicalizing symlinks rejected(preset): keep canonicalize-based check and add a config flag to opt out — extends already accepts unrestricted absolute paths, so a filesystem-level sandbox on relative refs is incoherent rather than conservative constraint(preset): cycle detection (`./runok.yml` vs `runok.yml` must hash to the same key) still needs symlink resolution, so canonicalize_best_effort is retained for normalize_reference_key only
…se notes intent(preset): keep doc comments and release notes focused on observable behavior — threat-model rationale for the validation change belongs in the PR body, not in the codebase
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #346 +/- ##
==========================================
- Coverage 89.07% 89.06% -0.02%
==========================================
Files 53 53
Lines 12657 12661 +4
==========================================
+ Hits 11274 11276 +2
- Misses 1383 1385 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request updates the extends: path resolution to use lexical normalization instead of filesystem canonicalization, allowing symlinks to point outside the base directory while still preventing .. traversal. Documentation and tests have been updated to reflect this change. A security concern was raised regarding the validate_within function, which may incorrectly permit path traversal if the root path is empty; a specific code improvement was suggested to address this vulnerability.
intent(preset): record the caller-side invariant so future readers do not introduce an empty-root call site, which would silently bypass the `..` traversal check via `Path::starts_with`'s empty-prefix semantics
intent(preset): keep docs focused on observable behavior (relative reference + symlink target outside the config directory) without framing it through a specific dotfiles-overlay workflow
Purpose
extendsreference (./foo.yml, ...) whose symlink target points outside the directory of the config file fails withpath traversal detected: './foo.yml' escapes the base directory.~/path is accepted, so rejecting only the relative form has no consistent justification.Reproduction steps
Approach
~/extendsreferences to a purely lexical evaluation of the reference string...segments escape the resolution root; do not follow the resolved path's symlinks...is accepted regardless of where its symlink target points...-based escapes such as../../etc/passwdor~/../../etc/passwdcontinue to be rejected.Design decisions
Validation threat model
..-based escapes by lexically evaluating the reference stringextends:already accepts absolute paths without restriction, so a filesystem sandbox around symlink targets carries no meaning in the threat model (anyone who can authorextends:can also write any absolute path). A..-segment check is enough as a typo-detection safety guard.extends:resolves to stays inside the root, even through symlinks.extends:already accepting absolute paths. The flag only adds cognitive overhead for users.