feat(sessions): var expansion and path canonicalization - #389
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR implements ChangesPatch source and policy pattern expansion via environment variables and tilde
macOS CI coverage for sessions crate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/sessions/src/composable.rs`:
- Around line 1395-1406: The policy is expanded after enumerate_patch_files
causing filesystem walks on unresolved/invalid patterns; call
policy.expand_with(resolved_vars) first and use that expanded policy when
constructing any checks before calling expand_patch_sources and
enumerate_patch_files (i.e., move let mut expanded =
policy.expand_with(resolved_vars)? above
expand_patch_sources/enumerate_patch_files), so validation happens before
PatchWalk and directory traversal and any returned errors surface
deterministically.
- Around line 977-982: resolve_patches/expand_patch_sources now only consult
resolved_vars (via resolve_patches, expand_source) which removes access to the
host env and breaks ~/ and $HOME expansion; fix by restoring ambient-home
lookup: either seed the session vars with HOME, XDG_CONFIG_HOME, XDG_CACHE_HOME
etc. when Composer::new() builds initial vars, or modify
resolve_patches/expand_source to fall back to std::env when a name is not found
in &vars (keep resolve_vars precedence). Update code paths referencing
resolve_patches, expand_patch_sources, and expand_source to use the chosen
approach so patch expansion again supports ~ and $VAR without requiring users to
predeclare them.
In `@crates/sessions/src/expansion.rs`:
- Around line 104-121: The escaped substitution values produced by
escape_glob_metas cause FileSet::walk_root() to stop early on inserted `[` and
widen the root; fix by having the walk-root logic use the original unescaped
variable value (or compute the literal prefix before escaping) when scanning for
glob metacharacters instead of the escaped output. Concretely, when expanding
vars in parse_var_ref/where escape_glob_metas is called, preserve or pass the
raw/decoded value into FileSet::walk_root() (or derive the concrete literal
prefix from the unescaped bytes) so FileSet::walk_root() no longer treats
escaped `[`/`*`/`?` as metacharacters; update FileSet::walk_root() and places
that call it to accept/consume the unescaped prefix (or an explicit
literal-prefix) rather than the escaped string.
- Around line 110-123: The current $-handling branch should only perform
expansion for strict variable names and treat other forms (malformed names,
lowercase, `${...}` non-strict, and escaped `\$`) as literals instead of
erroring; update the b == b'$' branch so it first detects an escape (`\$`) and
emits a literal `$` (consuming the backslash and dollar), otherwise attempt
parse_var_ref but only proceed with lookup and expansion when the parsed name
matches the StrictVarName rule (or use an existing is_strict check); if
parse_var_ref fails or the name is non-strict, push the raw `$` (and leave
subsequent characters intact) instead of returning ExpandError::UndefinedVar,
and continue using escape_glob_metas on expanded values when lookup succeeds
(functions to touch: parse_var_ref, lookup, ExpandError::UndefinedVar,
StrictVarName, escape_glob_metas).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32f95551-1a43-4435-8b80-4b72003c231d
📒 Files selected for processing (6)
crates/sessions/src/composable.rscrates/sessions/src/expansion.rscrates/sessions/src/lib.rscrates/sessions/src/loadout.rscrates/sessions/src/patches.rscrates/sessions/src/policy.rs
There was a problem hiding this comment.
This patch is breaking local macos builds, this is my fault as the ci-macos.yml path filter I added omits crates/sessions/**, so the workflow never runs for this PR. Even when it runs, the macOS jobs only test minvmd (cargo test -p minvmd, clippy -p minvmd at ci-macos.yml:360-367). Adding sessions to the paths would trigger the workflow but still never execute a sessions test.
Two edits to .github/workflows/ci-macos.yml are needed
- Add sessions to both path filters (push and pull_request):
paths:
- "crates/minvmd/**"
- "crates/minimal2/**"
- "crates/sessions/**" # platform-sensitive: canonicalization, symlinks
...
Run the sessions tests in the existing clippy+test job (the one at line ~336 on the self-hosted runner) — one line, reuses the job's build cache rather than adding a new job to the bottlenecked single runner:
- run: cargo clippy -p minvmd --all-targets -- -D warnings
- run: cargo test -p minvmd
- run: cargo test -p sessions
| /// Glob metacharacters (`**`, `*`, `?`, `[...]`, `{...}`) are | ||
| /// path components in their own right and pass through unchanged — | ||
| /// only literal `.` and `..` components are touched. | ||
| fn normalize_path(s: &str) -> Result<String, ExpandError> { |
There was a problem hiding this comment.
Path traversal is blanket illegal?
Seems like a safe starting point, but i wonder if we want to loosen this in the future. Handling path traversal is a mess tho (absolutize() isnt good enough if the user can do symlinks - canonicalize is best but requires files to exist + eats IOPS).
There was a problem hiding this comment.
I'm just having a hard time seeing a good reason to allow .., but I may be missing something and there may be workflows that would benefit from it. I don't think it would be that hard to support in the future if we need to though.
There was a problem hiding this comment.
A note on symlinks: We do need to support symlinks because home-manager (nix) uses them extensively and I want to support home-manager generated files for admittedly selfish reasons. That said to mitigate security concerns around them:
- Policies apply to both the link and the file it's linking to. If one is ignored or denied it treats them as both being such and if one isn't allowed (outside of user loadouts which bypass the user allowlist for ergonomic reasons) neither are allowed
- There will be a user config option to turn off resolving symlinks entirely
There was a problem hiding this comment.
If we dont support .., i dont think its possible to use symlinks to admit sensitive files in a way that wasnt possible already without this primitive. Because if someone can create symlinks on the host, they can already access files on the host.
|
|
||
| /// Parse a `$VAR` or `${VAR}` reference starting at `bytes[at]` (which | ||
| /// is the `$`). Returns `(name, bytes_consumed_including_dollar)`. | ||
| fn parse_var_ref(bytes: &[u8], at: usize) -> Result<(&str, usize), ExpandError> { |
There was a problem hiding this comment.
Might be worth adding a comment here that escaping variable declarations (i.e. $$var) must be handled earlier
twitchyliquid64
left a comment
There was a problem hiding this comment.
Approved tho note Norries comment above about fixing CI
52c5184 to
d263152
Compare
Fixed |
c123616 to
12c39d8
Compare
Summary
$VARexpansion behind oneexpand_sourcecall,driven by resolved session vars
HomeLookup/HomeResolutionFailure/expand_homemachineryBehavior changes
~/xor$HOME/xfor home-relative).Relative →
ExpandError::NotAbsolute...rejected everywhere (raw or substituted).unescaped-home regression for
~/).follow_symlinks: truechecks both link + canonical target; denyon either wins.
PatchPolicysetters are infallible; validation moves toexpand_withat resolve time.Test plan
cargo test -p sessions(174 lib + 5 doc)cargo clippy -p sessions --all-targets -- -D warningsResolves #384
Summary by CodeRabbit
New Features
~and$VAR/${VAR}using session-resolved values (with clearer errors when expansion fails).allow/deny/ignoreprecedence and more accurate matching when symlinks are involved.Bug Fixes/Improvements
..from expanded inputs..components).Tests / CI