Skip to content

fix(env): prevent config root injection into PATH via _.source#8936

Merged
jdx merged 1 commit intomainfrom
fix/source-path-injection
Apr 6, 2026
Merged

fix(env): prevent config root injection into PATH via _.source#8936
jdx merged 1 commit intomainfrom
fix/source-path-injection

Conversation

@jdx
Copy link
Copy Markdown
Owner

@jdx jdx commented Apr 6, 2026

Summary

  • When a sourced script prepends to PATH (e.g., export PATH="/custom:$PATH"), the strip_suffix + split_paths logic produced an empty path component that resolved to the config root directory being injected into PATH
  • Filter out empty paths from the split to prevent this spurious injection
  • Added e2e test verifying the config root does not appear in PATH

Fixes #8931

Test plan

  • mise run test:e2e test_env_source passes
  • CI passes

🤖 Generated with Claude Code


Note

Medium Risk
Adjusts PATH-diff parsing for _.source scripts, which affects how PATH is constructed from sourced env changes and could impact users relying on edge-case PATH manipulations.

Overview
Fixes an edge case where a _.source-sourced script that prepends to PATH (e.g., export PATH="/x:$PATH") could produce an empty path segment that resolves to the config root and gets injected into PATH.

The PATH patch extraction in src/config/env_directive/source.rs now skips empty path components, and an e2e test (e2e/env/test_env_source) asserts the config root does not appear in the resulting PATH.

Reviewed by Cursor Bugbot for commit 75a115d. Bugbot is set up for automated code reviews on this repo. Configure here.

When a sourced script prepends to PATH (e.g., export PATH="/custom:$PATH"),
the strip_suffix + split_paths logic produced an empty path component that
resolved to the config root directory. Filter out empty paths to prevent this.

Fixes #8931

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 6, 2026

Greptile Summary

This PR fixes a subtle bug where sourcing a script that prepends to PATH (e.g., export PATH="/custom:$PATH") could inject the config root directory spuriously into PATH.

Root cause: In source.rs, after a sourced script modifies PATH, the code strips the original PATH suffix from the new value to find the newly-added prefix. For example, if the new PATH is "/custom:/original" and the original is "/original", strip_suffix correctly returns "/custom:". However, calling env::split_paths("/custom:") on a colon-terminated string (on Unix, : is the path separator) yields ["/custom", ""] — the empty final component resolves to the current working directory, which in mise's context is the config root. This caused the config root to be silently injected into PATH.

Fix: A 3-line guard in src/config/env_directive/source.rs skips empty path components before pushing them into the paths vector.

  • Fixed src/config/env_directive/source.rs: filter empty path components from split_paths result after stripping the original PATH suffix
  • Added regression test in e2e/env/test_env_source: verifies $MISE_CONFIG_DIR does not appear in PATH after sourcing a script that prepends to PATH

Confidence Score: 5/5

This PR is safe to merge — the fix is minimal, correct, and well-targeted with a direct regression test.

The change is exactly 3 lines in a single well-understood code path. The root cause analysis is sound: split_paths on a colon-terminated string yields an empty final component which resolves to CWD (config root). Filtering empty components is the correct and complete fix. A regression test is included using the established e2e test framework patterns.

No files require special attention

Important Files Changed

Filename Overview
src/config/env_directive/source.rs Adds 3-line guard to skip empty path components from split_paths, correctly fixing the config root injection bug with a minimal, targeted change
e2e/env/test_env_source Adds regression test verifying that config root does not appear in PATH after sourcing a script that prepends to PATH using the established assert_not_contains helper

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["_.source script executes"] --> B["EnvDiff captures PATH change"]
    B --> C{"PATH key changed?"}
    C -- No --> D["Process as regular env var"]
    C -- Yes --> E["strip_suffix orig_path from new PATH"]
    E --> F{"strip_suffix succeeds?"}
    F -- No --> G["Skip PATH change"]
    F -- Yes --> H["split_paths on stripped prefix\ne.g. '/custom:' → ['/custom', '']"]
    H --> I{"Path component empty?"}
    I -- "Yes (BUG FIXED)" --> J["skip — no longer injects config root"]
    I -- No --> K["push path to paths vec"]
    K --> L["PATH correctly includes new component"]
Loading

Reviews (1): Last reviewed commit: "fix(env): prevent config root injection ..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses an issue where sourcing a script that prepends to the PATH could inadvertently inject the configuration root. The changes include a fix in src/config/env_directive/source.rs to skip empty path segments and a new E2E test case. Review feedback recommends renaming a shadowed variable to improve code clarity and enhancing the test assertions to verify the presence of the expected path and maintain stylistic consistency.

Comment on lines 39 to 44
for p in env::split_paths(new_path) {
if p.as_os_str().is_empty() {
continue;
}
paths.push((p, source.to_path_buf()));
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The variable p is shadowed here. It is already used in the outer loop (line 33) to represent an EnvDiffOperation. Using the same name for a PathBuf in the inner loop can be confusing and is generally discouraged for maintainability. Renaming it to something more descriptive like path or entry would improve readability.

Suggested change
for p in env::split_paths(new_path) {
if p.as_os_str().is_empty() {
continue;
}
paths.push((p, source.to_path_buf()));
}
for path in env::split_paths(new_path) {
if path.as_os_str().is_empty() {
continue;
}
paths.push((path, source.to_path_buf()));
}

Comment thread e2e/env/test_env_source
Comment on lines +27 to +28
path_output=$(mise env -s bash | grep PATH)
assert_not_contains "$path_output" "$MISE_CONFIG_DIR"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The test verifies that the config root is not present in the PATH, but it should also verify that the expected path (/custom-test-dir) is present. If the mise env command fails or returns an empty PATH, the current test would still pass (a false positive). Additionally, passing the variable $path_output to the assertion might be inconsistent with the existing style (e.g., line 14) where the command string is passed directly to be executed by the assertion helper.

assert_contains "mise env -s bash | grep PATH" "/custom-test-dir"
assert_not_contains "mise env -s bash | grep PATH" "$MISE_CONFIG_DIR"

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 6, 2026

Hyperfine Performance

mise x -- echo

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2026.4.4 x -- echo 21.9 ± 0.4 21.2 24.2 1.00
mise x -- echo 22.4 ± 0.3 21.8 23.7 1.02 ± 0.02

mise env

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2026.4.4 env 21.8 ± 0.8 20.8 27.6 1.00
mise env 22.0 ± 0.4 21.4 23.2 1.01 ± 0.04

mise hook-env

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2026.4.4 hook-env 22.3 ± 0.5 21.4 24.8 1.00
mise hook-env 22.7 ± 0.3 21.9 23.9 1.01 ± 0.03

mise ls

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2026.4.4 ls 19.6 ± 0.4 18.8 20.8 1.00
mise ls 20.0 ± 0.4 19.2 22.3 1.02 ± 0.03

xtasks/test/perf

Command mise-2026.4.4 mise Variance
install (cached) 149ms 149ms +0%
ls (cached) 77ms 77ms +0%
bin-paths (cached) 81ms 82ms -1%
task-ls (cached) 827ms 799ms +3%

@jdx jdx merged commit 9b7ad0d into main Apr 6, 2026
39 checks passed
@jdx jdx deleted the fix/source-path-injection branch April 6, 2026 03:15
mise-en-dev added a commit that referenced this pull request Apr 6, 2026
### 🚀 Features

- **(config)** report env files in config ls and doctor output by
@SamSoldatenko in [#8853](#8853)
- add support for token sources in GitLab and Forgejo by @roele in
[#8868](#8868)

### 🐛 Bug Fixes

- **(aqua)** prevent double .exe extension when Windows override URL
already ends in .exe by @yusei-wy in
[#8863](#8863)
- **(bash)** avoid duplicate trust warning after cd by @timothysparg in
[#8920](#8920)
- **(env)** prevent config root injection into PATH via _.source by @jdx
in [#8936](#8936)
- **(install)** suppress spurious dependency warning when tool is
configured by @jdx in [#8923](#8923)

### 📚 Documentation

- **(node)** add section on pinning npm version by @jdx in
[#8925](#8925)
- add Windows default paths and mise.toml examples alongside CLI
commands by @jdx in [#8926](#8926)
- clarify common sources of confusion from GitHub discussions by @jdx in
[#8927](#8927)
- clarify Python venv mechanisms, JAVA_HOME behavior, and activation
performance by @jdx in [#8928](#8928)
- add FAQ and troubleshooting entries based on common Discord questions
by @jdx in [#8930](#8930)

### New Contributors

- @SamSoldatenko made their first contribution in
[#8853](#8853)
- @yusei-wy made their first contribution in
[#8863](#8863)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant