fix: confine database restore paths to the data directory - #14
Draft
P3RF3CTION wants to merge 1 commit into
Draft
Conversation
Snyk Code flags /database/restore for path traversal, and it is right: backupPath and targetPath come straight from the request body and become fs read and write targets with no confinement. The route is admin-only, but that is not a boundary against traversal - an admin session is exactly what an attacker who lands one turns into arbitrary file read/write on the host, and restore reads a "backup" and writes a "target" wherever the strings point. Backups only ever live under DATA_DIR, so both paths are now resolved against it and rejected if they escape - via .., an absolute path, a shared-prefix sibling (/app/data-evil vs /app/data), or a null byte. New helper resolveWithinDir with tests covering each escape. The other nine Snyk Code path-traversal hits in this scan are not real: seven are ssh2 sftp.createReadStream/createWriteStream, which write to the remote managed host by design, not the Termix filesystem; the other two are the upload path in database.ts, whose filename already runs through path.basename in the multer storage layer. The scanner cannot tell an SFTP stream or a sanitized name from a raw fs path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UN1o1GH1wgG2KM4SJHiMs5
Owner
Author
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
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.
Overview
Follow-up to the security-hardening PR, addressing the Snyk Code (SAST) path-traversal findings. Of the ten flagged, one is real and is fixed here; the other nine are false positives the scanner cannot classify, explained below so they can be dismissed with a reason.
The real one:
/database/restorebackupPathandtargetPatharrive straight from the request body and becomefsread/write targets — a "backup" is read and a "target" is written wherever the strings point, with no confinement. The route isrequireAdmin, but admin-only is not a boundary against traversal: an admin session is exactly what an attacker who lands one wants to turn into arbitrary file read/write on the host.Backups only ever live under
DATA_DIR, so both paths are now resolved against it and rejected if they escape — via.., an absolute path, a shared-prefix sibling (/app/data-evilvs/app/data), or a null byte. A new helperresolveWithinDirdoes the containment check, with tests covering each escape.Behavior: no change for any legitimate restore, since real backups sit under
DATA_DIR. No frontend calls this endpoint; it is an admin API.The nine false positives
Documented here so they can be marked Ignore in Snyk with a rationale rather than left dangling:
fleet-routes.ts,content-routes.ts,download-routes.ts) aresftp.createReadStream/sftp.createWriteStreamfrom ssh2. They read and write on the remote managed host over SFTP — that is the feature — not on the Termix filesystem. Snyk cannot tell an SFTP stream from a localfspath.database.ts, the upload path) operate onreq.file.pathfrom multer, whose filename already runs throughpath.basenamein the disk-storage layer (added in the previous security PR). The taint tracker does not see that sanitizer.Verification
New tests:
resolveWithinDiragainst traversal, absolute paths, shared-prefix siblings, null bytes and non-string input.🤖 Generated with Claude Code
https://claude.ai/code/session_01UN1o1GH1wgG2KM4SJHiMs5
Generated by Claude Code