feat(sftp): add the local-path gate for the streaming file tools - #208
Merged
Merged
Conversation
First half of the #186 takeover. The module is deliberately inert: nothing imports it yet, so no tool behaviour changes here. Wiring it to sftp-list/sftp-upload-file/sftp-download-file is the next piece. Three things differ from the version proposed in #186. The path-resolution order is fixed. `resolve(root, input)` ignores `root` when `input` is absolute, so an absolute path spelled the way an operator's shell shows it does not lexically sit under a realpath'd root. Confining that form first therefore refused every legitimate absolute path on any host whose transfer root is behind a symlink — which is every macOS host, where os.tmpdir() is /var/folders/..., a symlink to /private/var/folders/.... Two of #186's own tests were red for this reason and no CI job could see it. The refusal stays lexical rather than being moved after realpath, because that is what stops these tools answering "does this path exist?" for arbitrary locations: a path outside the root is rejected before anything stats it. A test asserts an existing and an absent path outside the root produce byte-identical errors, and fails if the gate is dropped. Windows is refused outright instead of reusing the config-file ACL posture, which waives a read-exposed directory, never consults the `O:` owner, and treats a missing or slow icacls.exe as a pass. A root that cannot be verified has to disable the tools, not enable them; the Windows gate is its own piece. The audit log's directory and the SSH key directories are excludable by name. Both are conventionally 0700 and owner-owned, so the ownership and permission checks accept them — and a transfer root containing either turns a download into "overwrite the record of this transfer" or an upload into "read the private key". A forbidden directory that does not exist yet still counts, so creating it afterwards cannot re-open the hole. Also here, because the fix is invisible without them: - a macos CI job, wired into the required `ci` aggregate. Neither ubuntu-latest nor windows-latest has a symlinked temp directory, so the bug above passes both by construction. Lean on purpose: no docker, so no integration or e2e suite, and no coverage upload. - windows-acl.ts's containment goes through the same shared helper rather than its own `!rel.startsWith('..')`, which rejected a legitimately named `..cache` as an escape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s refusal The whole suite failed on the windows job: every case went through configuredRoot, which refuses win32 outright, so 26 tests asserted POSIX messages against 'not available on Windows'. Running on macOS could not show this — the windows job is what caught it, which is the reason that job blocks a merge. The POSIX cases are now guarded, and the Windows contract has its own block that only runs there. That block is the point: without it, the fail-closed branch would have been the one piece of this module with no coverage at all, on the one platform where it is the entire behaviour. Its second case pins that the refusal comes before any decision about the path, so a caller cannot tell an in-root path from an out-of-root one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #208 +/- ##
==========================================
- Coverage 82.90% 82.41% -0.49%
==========================================
Files 35 37 +2
Lines 2176 2309 +133
Branches 606 640 +34
==========================================
+ Hits 1804 1903 +99
- Misses 257 277 +20
- Partials 115 129 +14
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second piece of the #186 takeover, after #205. The module is inert — nothing imports it yet, so no tool behaviour changes. Wiring it to
sftp-list/sftp-upload-file/sftp-download-fileis the next PR.What differs from #186
1. The path-resolution order (was one of the two blocking test failures)
resolve(root, input)ignoresrootentirely for an absoluteinput, so a path spelled the way an operator's shell shows it is not yet comparable to a realpath'd root. Confining that form first refused every legitimate absolute path on any host whose transfer root sits behind a symlink — which is every macOS host.Fixed by checking the candidate against both spellings of the root (canonical and as-configured) lexically, then canonicalizing the parent before the leaf.
2. The refusal stays lexical, on purpose
The obvious fix — delete the early check and rely on realpath-then-confine — would make these tools answer "does this path exist?" for arbitrary locations, since a resolvable parent and an unresolvable one produce different errors. A test pins it:
It fails if the lexical gate is removed. Verified by removing it.
3. Windows is refused outright
Not reused from the config-file ACL posture, which waives a read-exposed directory, never consults the
O:owner, and treats a missing or slowicacls.exeas a pass. A root that cannot be verified must disable the tools. The Windows gate is its own piece (C).4. The audit log and SSH key directories are excludable
Both are conventionally 0700 and owner-owned, so the ownership/permission checks accept them — and a transfer root containing either turns a download into overwrite the record of this transfer and an upload into read the private key. A forbidden directory that does not exist yet still counts, so creating it later cannot re-open the hole.
Verification
Every claim above was checked by mutating the source and re-running:
confined(root, lexical)27 new unit tests;
npm run test:unitis 867 passing.windows-aclkeeps its 100 (14 win32-only skip locally, they run on thewindowsjob).Also here
macosCI job, wired into the requiredciaggregate — without it the bug in §1 is invisible, since neither ubuntu-latest nor windows-latest has a symlinked temp dir. Lean: no docker, so no integration/e2e, no coverage upload.windows-acl.tscontainment now uses the shared helper instead of its own!rel.startsWith('..'), which rejected a legitimately named..cacheas an escape. That one is a real if narrow fix, so it carries the changeset.Still to come
sftp.tsstreaming, and the ordering answer from Streaming SFTP tools need the audited string resolved before policy without side effects preceding it #207