security: prevent http install path escape#10245
Conversation
|
Need an answer fast? Review this PR in Change Stack to ask focused questions about the PR or a changed range. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughHttpBackend centralizes deterministic, filesystem-safe install-directory naming (sanitizes inputs, uses cache-key for ChangesInstall Symlink Naming
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes GHSA-f94h-j2qg-fxw3 by sanitizing HTTP backend version strings before using them as install-directory names, preventing repository-controlled version values from escaping
Confidence Score: 4/5The path-traversal fix is sound and well-tested; the only rough edge is an untested inconsistency in lookup_install_path for latest. The core sanitization logic is correct: tv_pathname() eliminates / and :, sanitize_install_version_name then catches backslash, ., and .., and the hash suffix prevents identity collisions. The one gap is that lookup_install_path for latest without tv.install_path set returns installs_path/latest rather than installs_path/{content_version}, and there is no test covering this branch. src/backend/http.rs — specifically the lookup_install_path function and its behaviour for the latest version when tv.install_path has not yet been populated. Important Files Changed
Reviews (5): Last reviewed commit: "security: align http install lookup path..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/backend/http.rs`:
- Around line 509-515: install_version_name currently returns an empty string
for implicit versions which causes create_version_alias_symlink (and the other
alias logic around install_version_name) to resolve to the installs root and
potentially remove it; change install_version_name (and the same logic used at
the other location noted) to never return "" — either reject empty tv.version by
returning a Result and bubble an error, or map empty/implicit versions to a safe
deterministic alias (e.g. a prefixed name derived from cache_key like
"_implicit_<short-cache>") instead of "" — update callers such as
create_version_alias_symlink to handle the new non-empty alias (or the Result)
so we never join an empty string onto installs_path.
- Around line 509-515: install_version_name currently trusts tv.tv_pathname()
for non-"latest" versions but tv_pathname only normalizes ':' and '/' so Windows
backslashes (e.g. "..\\..\\escape" or "\\\\server\\share") can still cause path
traversal when later joined into installs_path; update install_version_name (and
any joins around installs_path usage) to normalize and reject or escape
backslashes (treat '\' like '/' for normalization or strictly sanitize any path
separator characters) before returning the install name, ensure tv.tv_pathname()
is updated or wrap its result to replace/backslash-normalize and validate there
are no parent-segment escapes or UNC paths, and add a regression test that
passes a Windows-style version string (e.g. "..\\..\\escape" and
"\\\\server\\share") to install_version_name and to the install flow to confirm
traversal/UNC are blocked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/backend/http.rs`:
- Around line 509-526: The install-name mapping
(install_version_name/content_version_name/sanitize_install_version_name) is
lossy and can collide for inputs like "a/b", "a:b", "a\\b", and "a-b"; update
sanitize_install_version_name (used by install_version_name and
content_version_name logic) so the produced install directory is injective:
compute a deterministic short hash (e.g., first 7 hex chars of SHA-256) of the
original raw version/request string (use tv.tv_pathname() or the raw version as
available) and append it to the sanitized name whenever sanitization changes the
input (or always append for simplicity) so different raw inputs never map to the
same directory; update install_version_name to use this new behavior and add a
regression test that installs two colliding-looking versions (e.g., "a/b" and
"a-b" or "a:b") and asserts they produce distinct install directories and do not
clobber each other.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Hyperfine Performance
|
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.0 x -- echo |
19.1 ± 0.9 | 17.3 | 22.3 | 1.00 |
mise x -- echo |
20.1 ± 2.0 | 17.6 | 50.3 | 1.05 ± 0.12 |
mise env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.0 env |
18.7 ± 0.9 | 16.7 | 23.0 | 1.00 |
mise env |
19.1 ± 1.0 | 17.2 | 25.2 | 1.02 ± 0.07 |
mise hook-env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.0 hook-env |
19.3 ± 1.0 | 17.5 | 24.6 | 1.00 |
mise hook-env |
19.8 ± 0.9 | 18.0 | 24.1 | 1.02 ± 0.07 |
mise ls
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.0 ls |
15.6 ± 0.7 | 14.0 | 19.1 | 1.00 |
mise ls |
16.1 ± 0.8 | 14.5 | 20.0 | 1.03 ± 0.07 |
xtasks/test/perf
| Command | mise-2026.6.0 | mise | Variance |
|---|---|---|---|
| install (cached) | 136ms | 136ms | +0% |
| ls (cached) | 61ms | 62ms | -1% |
| bin-paths (cached) | 63ms | 64ms | -1% |
| task-ls (cached) | 125ms | 130ms | -3% |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 39e8496. Configure here.
|
Addressed the hashed install path lookup mismatch in 80ae49f: HTTP now uses the same normalized install path for installed checks and bin discovery, and empty HTTP versions use a stable Verified with:
This comment was generated by an AI coding assistant. |
1 participant
Summary
ToolVersion::tv_pathname()latestand empty HTTP versionsSecurity
Fixes GHSA-f94h-j2qg-fxw3 by preventing repository-controlled HTTP backend version strings from escaping the mise installs directory via absolute paths.
Tests
cargo test backend::http::testscargo fmt --checkNote
High Risk
Changes install directory layout and lookup for HTTP tools (security-critical path handling); existing installs keyed by raw version strings may not be found until reinstalled.
Overview
Security fix for the HTTP backend: install symlink directory names no longer use raw version strings, so malicious or path-like versions cannot escape
installs_path(GHSA-f94h-j2qg-fxw3).Non-
latestversions are named viaToolVersion::tv_pathname()with extra rules (./.., backslashes, hash suffix when sanitization changes the string).lateststill uses a short content-derived cache key; empty versions map to_implicit. Alias symlinks underlatestare created only for explicit"latest", not empty versions.Install and lookup paths are centralized (
install_path_for/lookup_install_path); installs recordtv.install_path,is_version_installedandlist_bin_pathsresolve the same sanitized paths (including runtime-symlink checks where applicable). Unit tests cover absolute paths,.., Windows separators/UNC, distinct version collisions, andlatest/empty behavior.Reviewed by Cursor Bugbot for commit 80ae49f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit