feat(minimald)!: rebase sftp paths at / to support accessing /home - #1164
Conversation
📝 WalkthroughWalkthroughMinimald now exposes separate workspace and home roots through SFTP. It validates canonical containment, protects synthetic and export roots, and uses typed paths. VCS detection verifies the resolved Git work-tree. Absolute paths support component-aware prefix checks. ChangesDual-root SFTP namespace
VCS root validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SFTPClient
participant SFTPSession
participant Filesystem
SFTPClient->>SFTPSession: Send client-visible path
SFTPSession->>Filesystem: Resolve and canonicalize target
Filesystem-->>SFTPSession: Return contained daemon path
SFTPSession->>Filesystem: Execute SFTP operation
Filesystem-->>SFTPSession: Return metadata or operation result
SFTPSession-->>SFTPClient: Return client-visible response
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Reviewed with the escape from the #1162 campaign in mind (the
Two things, one worth acting on: 1. No
Same consideration for the 2. Observation, not an objection — the export surface roughly doubles.
Context that may be useful: this bug class has now recurred six times in the tree as "one containment rule, two implementations, only one hardened" — most recently in #1162, where switching tar extraction to a per-entry loop closed the symlink hole and opened a hardlink one ( Two related items from the same audit are still open and independent of this PR: |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/minimald/src/sftp.rs (1)
404-416: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winClose the symlink race on
openwithO_NOFOLLOW.
containedproves containment at check time. For a leaf that does not exist yet, the proof covers only the parent chain. A concurrent writer in the same session — the sandbox process shares these directories — can create a symlink at that leaf after the check and before thisopen.opts.openthen follows it and writes outside the export.
crates/minimald/src/env.rscreate_artifact(line 1294) already setscustom_flags(libc::O_NOFOLLOW)for this exact reason. Apply the same flag here. Note thatO_NOFOLLOWmakesopenfail withELOOPon a symlinked final component, so a client that legitimately opens a symlink inside an export loses that ability; decide whether SFTP needs to support it.🔒 Proposed change
let host = self.resolve(&filename).await?.real()?; + use std::os::unix::fs::OpenOptionsExt; let mut opts = OpenOptions::new(); opts.read(pflags.contains(OpenFlags::READ)) .write(pflags.contains(OpenFlags::WRITE)) .append(pflags.contains(OpenFlags::APPEND)) - .truncate(pflags.contains(OpenFlags::TRUNCATE)); + .truncate(pflags.contains(OpenFlags::TRUNCATE)) + // The containment proof is about the path at check time; refuse + // to follow a link planted at the leaf since then. + .custom_flags(libc::O_NOFOLLOW);🤖 Prompt for 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. In `@crates/minimald/src/sftp.rs` around lines 404 - 416, Update the OpenOptions construction in the SFTP open flow before opts.open to set the platform-specific custom O_NOFOLLOW flag, matching env.rs create_artifact. Preserve the existing read/write/create flag handling so final-component symlinks fail with the underlying ELOOP error rather than being followed.
🤖 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/minimald/src/session_delta.rs`:
- Around line 172-179: Update vcs_at_risk and its filesystem checks to avoid
synchronous Path::exists() and Path::canonicalize() on Tokio workers. Execute
both probes through spawn_blocking or Tokio filesystem APIs, and wrap each
corresponding await with WALK_TIMEOUT while preserving the existing
repository-path comparison behavior.
In `@crates/minimald/src/sftp.rs`:
- Around line 554-556: Update the setstat permission-update path around resolve
and perm_from_mode to use resolve_below_export before calling real(), preventing
clients from changing permissions on export roots while preserving permission
updates for paths below them.
- Around line 168-175: Update SftpSession::new to canonicalize the
tree/workspace and home roots after directory creation, then reject them when
either canonical path equals or contains the other. Return an error before
constructing the session or exposing exports, while preserving normal
initialization for disjoint roots; use the existing SftpSession fields and
exports method for locating the relevant paths.
---
Outside diff comments:
In `@crates/minimald/src/sftp.rs`:
- Around line 404-416: Update the OpenOptions construction in the SFTP open flow
before opts.open to set the platform-specific custom O_NOFOLLOW flag, matching
env.rs create_artifact. Preserve the existing read/write/create flag handling so
final-component symlinks fail with the underlying ELOOP error rather than being
followed.
🪄 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: 2cf0e423-6bc0-43b0-ac76-622ec6db22e0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/minimald/Cargo.tomlcrates/minimald/src/env.rscrates/minimald/src/session_delta.rscrates/minimald/src/sftp.rscrates/paths/src/lib.rs
💤 Files with no reviewable changes (1)
- crates/minimald/Cargo.toml
| // A `.git` entry is only a marker. When it is not a real repository, git | ||
| // does not fail: it keeps walking up and answers for the nearest enclosing | ||
| // repo, which would report an unrelated tree's uncommitted files as this | ||
| // session's work at risk. So the work tree git found has to be this one. | ||
| let top = run_git(root, &["rev-parse", "--show-toplevel"]).await?; | ||
| if !is_same_dir(Path::new(top.trim()), root) { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- session_delta.rs outline ---'
ast-grep outline crates/minimald/src/session_delta.rs --view expanded
printf '%s\n' '--- target implementation and tests ---'
sed -n '1,240p' crates/minimald/src/session_delta.rs
printf '%s\n' '--- is_same_dir usages ---'
rg -n -C 3 'is_same_dir|vcs_at_risk|WALK_TIMEOUT|canonicalize' crates/minimald/src crates/minimald/tests 2>/dev/null || true
printf '%s\n' '--- tokio dependency/features ---'
rg -n -C 3 'tokio' Cargo.toml crates/*/Cargo.tomlRepository: gominimal/minimal
Length of output: 45700
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- assess callers and runtime context ---'
rg -n -C 5 'assess\(|session_delta::|DeltaSource::arm|changed_files\(' crates/minimald/src crates/minimald/tests 2>/dev/null || true
printf '%s\n' '--- synchronous filesystem calls in async session_delta functions ---'
python3 - <<'PY'
from pathlib import Path
p = Path("crates/minimald/src/session_delta.rs")
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 140 <= i <= 225:
print(f"{i:4}: {line}")
PY
printf '%s\n' '--- async filesystem handling precedent ---'
sed -n '320,345p' crates/minimald/src/sftp.rs
sed -n '1275,1310p' crates/minimald/src/env.rs
printf '%s\n' '--- just recipes relevant to minimald ---'
rg -n -C 2 'test-cross|test.*minimald|minimald' Justfile justfile* 2>/dev/null || trueRepository: gominimal/minimal
Length of output: 25030
🌐 Web query:
Tokio official documentation spawn_blocking cancellation timeout blocking operations
💡 Result:
In Tokio, tasks spawned using spawn_blocking cannot be cancelled or aborted once they have started running because they are synchronous and do not cooperate with the asynchronous task system [1][2][3]. Key points regarding cancellation and timeouts with spawn_blocking: 1. Cancellation: Calling abort on a JoinHandle for a spawn_blocking task has no effect if the task is already running [1][4]. The abort method only works if the task has not yet started and is still waiting in the thread pool queue [1][3]. 2. Timeouts: Wrapping a spawn_blocking call in a tokio::time::timeout future will cause the timeout to fire if the task takes too long, but this only stops the async task from waiting for the result [5]. The blocking operation itself will continue to run to completion in its background thread, regardless of the timeout [1][2]. 3. Recommended Pattern for Cancellation: Since Tokio cannot force a blocking task to stop, you must implement cooperative cancellation yourself [6]. This is typically done by passing a cancellation signal into the blocking closure, such as an AtomicBool or a oneshot::channel [6][7]. The blocking code should periodically check this signal and return early if it is set [8][7]. If your application requires prompt cancellation or strict timing, consider whether the task can be refactored into truly asynchronous code, or use a dedicated thread pool (like rayon for CPU-bound tasks) where you can manage the lifecycle of the threads more directly [4][9].
Citations:
- 1: https://docs.rs/tokio/latest/tokio/task/fn.spawn_blocking.html
- 2: https://github.com/tokio-rs/tokio/blob/306ed1c3/tokio/src/task/blocking.rs
- 3: https://docs.rs/tokio/latest/tokio/task/
- 4: https://docs.rs/tokio/1.48.0/tokio/task/fn.spawn_blocking.html
- 5: https://users.rust-lang.org/t/need-help-understanding-tokio-timeout-mechanics/131861
- 6: https://stackoverflow.com/questions/73528236/how-to-terminate-a-blocking-tokio-task
- 7: https://stackoverflow.com/questions/59977693/how-can-i-stop-running-synchronous-code-when-the-future-wrapping-it-is-dropped
- 8: Aborting a Task with Spawn_Blocking tokio-rs/tokio#6570
- 9: Is it safe to use block_on at the top level of a spawn_blocking call? tokio-rs/tokio#3717
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
src = Path("crates/minimald/src/session_delta.rs").read_text()
m = re.search(r'async fn vcs_at_risk\(.*?(?=\n///|\nfn |\nasync fn )', src, re.S)
assert m, "vcs_at_risk not found"
body = m.group(0)
checks = {
"vcs_at_risk is async": body.startswith("async fn vcs_at_risk"),
"root marker uses synchronous exists": ".exists()" in body,
"canonicalization occurs in async call path": "is_same_dir(Path::new(top.trim()), root)" in body,
}
for name, ok in checks.items():
print(f"{name}: {'yes' if ok else 'no'}")
assert all(checks.values())
helper = re.search(r'fn is_same_dir\(.*?(?=\n///|\nfn |\nasync fn )', src, re.S)
assert helper, "is_same_dir not found"
helper_body = helper.group(0)
print("is_same_dir uses std canonicalize:", "canonicalize()" in helper_body)
print("is_same_dir uses spawn_blocking:", "spawn_blocking" in helper_body)
print("is_same_dir uses timeout:", "timeout(" in helper_body)
assert "canonicalize()" in helper_body
assert "spawn_blocking" not in helper_body
assert "timeout(" not in helper_body
PY
printf '%s\n' '--- Tokio runtime declarations ---'
rg -n -C 3 '#\[tokio::main|Builder::new_|new_current_thread|new_multi_thread|flavor\s*=' crates/minimald/src crates/minimald/tests 2>/dev/null || trueRepository: gominimal/minimal
Length of output: 44876
Move filesystem probes off the Tokio runtime.
vcs_at_risk calls synchronous Path::exists() and Path::canonicalize(). A slow filesystem can block a Tokio worker. Run both probes with spawn_blocking or Tokio filesystem APIs, and bound their awaits with WALK_TIMEOUT.
🤖 Prompt for 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.
In `@crates/minimald/src/session_delta.rs` around lines 172 - 179, Update
vcs_at_risk and its filesystem checks to avoid synchronous Path::exists() and
Path::canonicalize() on Tokio workers. Execute both probes through
spawn_blocking or Tokio filesystem APIs, and wrap each corresponding await with
WALK_TIMEOUT while preserving the existing repository-path comparison behavior.
Source: Learnings
| let host = self.resolve(&path).await?.real()?; | ||
| if let Some(perms) = perm_from_mode(attrs.permissions) { | ||
| fs::set_permissions(host, perms).await?; | ||
| fs::set_permissions(host.as_str(), perms).await?; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
setstat still lets a client chmod an export root.
rmdir, remove, and rename use resolve_below_export, so the export roots are protected. setstat uses resolve(...).real(), so chmod 000 /workbench succeeds and strips access to the session's working tree for the sandbox process. The directory survives, but the session's layout becomes unusable.
If the export roots are part of the session layout the client must not change, use resolve_below_export here too.
🛡️ Proposed change
- let host = self.resolve(&path).await?.real()?;
+ // The export roots are the session's layout: permissions on them are
+ // not the client's to change, for the same reason `rmdir` refuses.
+ let host = self.resolve_below_export(&path).await?;
if let Some(perms) = perm_from_mode(attrs.permissions) {
fs::set_permissions(host.as_str(), perms).await?;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let host = self.resolve(&path).await?.real()?; | |
| if let Some(perms) = perm_from_mode(attrs.permissions) { | |
| fs::set_permissions(host, perms).await?; | |
| fs::set_permissions(host.as_str(), perms).await?; | |
| // The export roots are the session's layout: permissions on them are | |
| // not the client's to change, for the same reason `rmdir` refuses. | |
| let host = self.resolve_below_export(&path).await?; | |
| if let Some(perms) = perm_from_mode(attrs.permissions) { | |
| fs::set_permissions(host.as_str(), perms).await?; |
🤖 Prompt for 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.
In `@crates/minimald/src/sftp.rs` around lines 554 - 556, Update the setstat
permission-update path around resolve and perm_from_mode to use
resolve_below_export before calling real(), preventing clients from changing
permissions on export roots while preserving permission updates for paths below
them.
0e924a1 to
52b6979
Compare
|
Re-reviewed after the update. The
That's the argument that makes The The remaining handlers check out for the reasons that make them uninteresting rather than because of new code, which is worth stating so it's a deliberate conclusion:
One residual, lower severity than the open, and possibly a deliberate accept: Nothing blocking from me. LGTM. |
Rebases paths over sftp as rooted at the same tree as in the corresponding session, so that
/homeand/workbenchare both accessible. Previously,/mapped to/workbenchwhich was limiting.Summary by CodeRabbit
Note
Rebase SFTP paths at
/to expose both/workbenchand/homeexports/) containing two exports:/workbench(session working tree) and/home(user home directory), replacing the previous single-root layout.sftp.rsto normalize client paths textually, map them to the correct export, and enforce containment via a symlink-safecontainedwalk.resolve_below_exportto reject destructive operations (remove, rename, rmdir) targeting the export roots themselves./minimal.tomlto/workbench/minimal.tomlto match the new layout./(e.g./foo) must now use/workbench/foo; symlinked ancestors or dangling symlink leaves inside exports are refused withPermissionDenied.Changes since #1164 opened
russh_sftp::server::Handler::setstatimplementation forSftpSessionto enforce minimum owner permissions by performingfstaton the file handle to determine if target is a directory, then calling a new helper function to apply a permission floor of owner read for files or owner read-execute for directories before applying permissions to the filesystem [da91cab]keep_owner_accessfunction that acceptsstd::fs::Permissionsand a directory boolean flag, returning modified permissions with an enforced minimum of owner read-execute for directories or owner read for files while preserving all other permission bits [da91cab]sftp_setstat_leaves_the_owner_able_to_reach_what_it_chmoddedintegration test that creates a session, writes test files and directories, exercisessetstatwith various permission modes including 0o000, 0o644, 0o751, and 0o444, and validates that resulting permissions are floored to 0o400 for files and 0o500 for directories when restrictive modes are requested while preserving requested group and other bits [da91cab]Macroscope summarized 52b6979.