Repository navigation
Conversation
|
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:
📝 WalkthroughWalkthroughRemote task resolution now enforces defining-config trust, preserves metadata provenance, gates remote tool installation, and isolates uncached HTTP and Git artifacts. CLI paths, fixtures, and end-to-end tests cover trusted, untrusted, mutable, and nested remote task flows. ChangesRemote task trust
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TaskCommand
participant TaskFetcher
participant TrustCheck
participant RemoteSource
participant ToolInstaller
TaskCommand->>TaskFetcher: Resolve remote task metadata
TaskFetcher->>TrustCheck: Validate defining config
TrustCheck-->>TaskFetcher: Trust result
TaskFetcher->>RemoteSource: Fetch and parse remote metadata
RemoteSource-->>TaskFetcher: Task body and tool metadata
TaskFetcher-->>TaskCommand: Return resolved task
TaskCommand->>ToolInstaller: Process remote tool requirements
ToolInstaller->>TrustCheck: Validate metadata provenance
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR enforces trust for every side-effectful operation a remote task can trigger: Tera template rendering,
Confidence Score: 5/5All trust-enforcement paths are correctly gated; the RAII snapshot mechanism is sound; the retry logic for remote aliases is well-scoped. No blocking issues were found. The core security invariants — safe-mode SSRF prevention, passive-discovery trust gating, per-fetch snapshot isolation — are each verified by dedicated e2e or unit tests. The logic in Files Needing Attention: No files require special attention beyond the three minor observations noted in the comments. Important Files Changed
Reviews (15): Last reviewed commit: "fix(task): address remote metadata revie..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/task/task_file_providers/remote_task_git.rs (1)
166-166: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPrevent concurrent non-cached clone corruption.
tmp_destinationis derived solely fromcache_key. If multiple processes perform non-cached fetches for the same repository concurrently, they will all race to use and delete the exact sametmp_destinationdirectory, corrupting the clone and failing the task.Since
destinationis already unique for non-cached fetches (and correctly deterministic for cached ones), derivingtmp_destinationfromdestination's filename guarantees uniqueness across concurrent non-cached requests and prevents the race condition.🔒️ Proposed fix to isolate concurrent clone paths
- let tmp_destination = self.storage_path.join(format!("{}.clone-tmp", &cache_key)); + let tmp_destination = self.storage_path.join(format!( + "{}.clone-tmp", + destination.file_name().unwrap().to_string_lossy() + ));🤖 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 `@src/task/task_file_providers/remote_task_git.rs` at line 166, Update the temporary clone path construction near tmp_destination to derive its filename from the already-unique destination path rather than solely from cache_key. Preserve deterministic temporary paths for cached fetches while ensuring concurrent non-cached requests receive distinct clone directories.
🤖 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/task/task_file_providers/remote_task_http.rs`:
- Around line 76-77: Update the download flow in the method containing
self.download_file so it writes to a temporary path rather than the final
destination, then renames the completed temporary file to destination only after
a successful download. Ensure download failures or interruptions do not leave
destination or stale temporary files that can be treated as valid cache entries.
---
Outside diff comments:
In `@src/task/task_file_providers/remote_task_git.rs`:
- Line 166: Update the temporary clone path construction near tmp_destination to
derive its filename from the already-unique destination path rather than solely
from cache_key. Preserve deterministic temporary paths for cached fetches while
ensuring concurrent non-cached requests receive distinct clone directories.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 505b9bfa-a87b-4a27-8bd3-73dd9759d6ba
📒 Files selected for processing (12)
e2e/helpers/scripts/git_http_backend_server.pye2e/helpers/scripts/http_test_server.pye2e/tasks/test_task_remote_git_includese2e/tasks/test_task_remote_metadata_trustsrc/cli/tasks/info.rssrc/cli/tasks/ls.rssrc/cli/tasks/validate.rssrc/task/mod.rssrc/task/task_fetcher.rssrc/task/task_file_providers/remote_task_git.rssrc/task/task_file_providers/remote_task_http.rssrc/task/task_tool_installer.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/task/mod.rs
- src/task/task_tool_installer.rs
- src/task/task_fetcher.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/task/task_file_providers/remote_task_http.rs (1)
122-124: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse atomic download for un-cached paths to prevent cache corruption and concurrent read errors.
When cache mode is disabled,
get_local_pathstill overwrites the shareddestinationdirectly (the exact same path used by cached runs). If an un-cached download fails or is interrupted, a partial file is left on disk. A subsequent run with cache enabled will mistakenly treat this partial file as a valid cache hit, reintroducing the cache corruption issue. Furthermore, direct overwriting can cause crashes or access-denied errors for any concurrent readers.Use
download_file_atomicallyhere to safely replace the file and automatically clean up on failure.🛠️ Proposed fix
let _lock = LockFile::new(&destination).lock()?; - self.download_file(file, &destination).await?; + self.download_file_atomically(file, &destination).await?; Ok(destination)🤖 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 `@src/task/task_file_providers/remote_task_http.rs` around lines 122 - 124, Replace the direct self.download_file call in get_local_path with download_file_atomically, preserving the existing destination and lock flow. Ensure failed or interrupted uncached downloads clean up temporary data and readers never observe a partially written file.
🧹 Nitpick comments (2)
src/task/task_file_providers/remote_task_http.rs (2)
159-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to reflect its actual assertions.
This test was updated to call
get_local_artifactand verify unique artifact generation, but the test name still refers toget_local_path. Consider renaming it to match the tested behavior.📝 Proposed fix
#[tokio::test] - async fn test_http_remote_task_get_local_path_without_cache() { + async fn test_http_remote_task_get_local_artifact_without_cache() {🤖 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 `@src/task/task_file_providers/remote_task_http.rs` around lines 159 - 160, Rename the test function test_http_remote_task_get_local_path_without_cache to reflect that it calls get_local_artifact and verifies unique artifact generation, while leaving its assertions and behavior unchanged.
86-87: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueRemove the
remove_filecall.download_filealready stages through a temp file and persists to the target path, so deleting the reserved path first only adds a TOCTOU window.🤖 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 `@src/task/task_file_providers/remote_task_http.rs` around lines 86 - 87, Remove the file::remove_file call after temp_file.keep() in the download_file flow, while retaining the destination extraction and subsequent processing. Rely on download_file’s existing temp-file staging and persistence without deleting the reserved target path.
🤖 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.
Outside diff comments:
In `@src/task/task_file_providers/remote_task_http.rs`:
- Around line 122-124: Replace the direct self.download_file call in
get_local_path with download_file_atomically, preserving the existing
destination and lock flow. Ensure failed or interrupted uncached downloads clean
up temporary data and readers never observe a partially written file.
---
Nitpick comments:
In `@src/task/task_file_providers/remote_task_http.rs`:
- Around line 159-160: Rename the test function
test_http_remote_task_get_local_path_without_cache to reflect that it calls
get_local_artifact and verifies unique artifact generation, while leaving its
assertions and behavior unchanged.
- Around line 86-87: Remove the file::remove_file call after temp_file.keep() in
the download_file flow, while retaining the destination extraction and
subsequent processing. Rely on download_file’s existing temp-file staging and
persistence without deleting the reserved target path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d9f82fda-540c-4cc6-894f-58fa141860fa
📒 Files selected for processing (1)
src/task/task_file_providers/remote_task_http.rs
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 2 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 3 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
e56be0f to
0437438
Compare
75f2ae5 to
b47964d
Compare
|
This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically. This is warning day 2 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
b47964d to
7078147
Compare
|
This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically. This is warning day 6 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR has had merge conflicts for more than 7 days, so it is being closed automatically. Feel free to reopen or create a new PR if you'd like to continue working on this. This comment was generated by an automated workflow. |
…4416) Listing the tasks of a directory you have not trusted could run a command from a remote task file. A `mise.toml` that holds only `[tasks]` loads without trust, so this is enough: ```toml # mise.toml in a freshly cloned, untrusted repository [tasks.build] file = "https://example.com/build.sh" ``` ```bash #!/usr/bin/env bash #MISE description="{{ exec(command='touch /tmp/PWNED') }}Build" ``` `mise tasks ls`, `mise tasks info build` and `mise tasks validate` downloaded `build.sh` and rendered its `#MISE` headers, so `exec()` ran with no trust prompt. The same template written in `mise.toml` itself, or in a local task file such as `mise-tasks/build.sh`, is already refused until the config is trusted. A remote task file now follows the rule for local task includes. When its `#MISE` headers contain template syntax, the config that names it must be trusted before they render. In an untrusted directory, mise asks to trust it, or fails with `Config files in … are not trusted` when it cannot ask, and nothing renders. Only the headers are checked, because only they render: braces elsewhere in the script, such as `docker ps --format '{{.Names}}'`, need no trust. They are read the same way when a `git::` file is named `*.toml`. A remote file with no template in its headers still lists without trust, and a trusted config renders as before. `mise run` keeps trusting the config it runs, as it does for local tasks. Safe mode still waives trust, since `exec()` and `read_file()` are disabled there. #12202 (and #11113 before it) by @risu729 reported this and proposed a wider rework of remote task metadata, which pr-closer closed. This is a narrower fix built on main's existing trust check for task includes. They are credited as co-author. ## Validation - New `e2e/tasks/test_task_remote_http_trust`, with the remote files served from a local HTTP server: - In an untrusted directory, `mise tasks ls`, `mise tasks info` and `mise tasks validate` each fail with `not trusted`, and the `exec()` marker file is not created. - A remote file whose headers have no template still lists without trust, with `{{.Names}}` in its script body. - With the config trusted, a templated remote file renders. - Unit test `test_header_checks_ignore_the_file_name`: an escaped template in the header of a file named `evil.toml` needs trust (read as a TOML document, the header is a comment), and a template in the script body does not. - On Linux, in a `rust:1` container, the test passes with this branch's build. Run against the released 2026.10.8, the same test fails with `remote task template executed by untrusted 'mise tasks ls'`. - `cargo clippy --workspace --all-features --all-targets -- -D warnings` passes. *AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5-5; version: unavailable.* 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Security** - Remote task files that require trust are checked before loading. Untrusted files return a trust error when listed, inspected, or validated, without creating a trust marker. - Trust checks apply to remote task files with decoded templates or secret declarations in their `#MISE` headers. - **Bug Fixes** - Remote task files with plain headers remain available without requiring trust. - Templated remote task descriptions render when the referencing configuration is trusted. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Taku Kodama <79110363+risu729@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Work order
Prerequisite #11222 is merged. This branch is rebased directly onto #12000's head commit; GitHub still displays
mainas the base because the stack branches live in a fork.Summary
file,source, modules, and Python venvs), or remote tool metadata can execute, resolve, activate, or install anything#MISEheaders consistently across run, bare dispatch, info, dependency, dynamic, and listing pathsRoot cause
#11111 began parsing remote task headers during discovery. That made remote templates, tools, environment directives, aliases, and visibility active metadata, but the remote task path did not consistently retain or enforce the trust of the local config that selected it. Passive discovery could also perform HTTP or Git work before trust, including in safe mode.
Remote Git paths also needed a shared containment check after checkout canonicalization; parser-only traversal checks do not prevent an intermediate symlink from escaping the artifact root.
Broader cache publication, locking, cleanup policy, retention, and cache-key hardening remain intentionally separate in #11203.
Validation
mise run lint-fixcargo check --lockedmise run test:e2e e2e/tasks/test_task_remote_metadata_trustmise run test:e2e e2e/tasks/test_task_remote_git_includesFollow-up to #11111.
AI-assisted — Tool: Codex; model: unavailable/unavailable; version: unavailable.