fix(@angular/cli): resolve executables strictly from PATH

[alan-agius4]

Update executable invocation logic to resolve system binaries (such as git and which) strictly from the PATH environment variable.

This prevents bare command names passed to execFileSync / execFile from implicitly searching and resolving binaries relative to process.cwd() on Windows.

Fixes #33755

[alan-agius4]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a utility function findExecutableOnPath to safely resolve executables from the PATH environment variable, preventing implicit resolution from the current working directory on Windows. It integrates this utility when executing git and which. The review feedback highlights critical security and correctness improvements: avoiding fallback to bare command names ('git' and 'which') when they are not found on the PATH to prevent command injection risks, and stripping double quotes from PATH directory entries on Windows to correctly handle paths with spaces.

[bilguunbicktivism]

Thanks for turning this around so fast, and for picking up completion.ts alongside it — that was the sibling call site with the same shape.

One small edge case on findExecutableOnPath, since it is the one thing that could let the original behaviour back in.

join(dir, binaryName + ext) preserves a relative PATH entry, so the returned candidate is not guaranteed to be absolute:

> node -e "const {join,isAbsolute}=require('path'); for (const d of ['.','sub','C:\\Program Files\\Git\\cmd']) { const r=join(d,'git.exe'); console.log(JSON.stringify(d).padEnd(28),'->',JSON.stringify(r),' absolute:',isAbsolute(r)); }"
"."                          -> "git.exe"  absolute: false
"sub"                        -> "sub\\git.exe"  absolute: false
"C:\\Program Files\\Git\\cmd" -> "C:\\Program Files\\Git\\cmd\\git.exe"  absolute: true

A . entry collapses to the bare name git.exe. existsSync then resolves it against process.cwd(), and so does execFileSync — which is the original resolution path. The empty-string case is already covered by the if (!rawDir) continue, but . and other relative entries are not.

. on PATH is unusual, but it is the one configuration where the guard would silently no-op rather than fail closed, and the rest of the change is careful to fail closed.

One line covers it:

const dir = rawDir.startsWith('"') && rawDir.endsWith('"') ? rawDir.slice(1, -1) : rawDir;
if (!isAbsolute(dir)) {
  continue;
}

or resolve(dir) if relative entries should still be honoured — though skipping them seems closer to the intent of "strictly from system PATH".

Also agree with your framing on the issue itself: a workspace you have chosen to build in already executes project code through lifecycle scripts, builders and schematics, so this is defense in depth rather than a boundary. Thanks for treating it that way.

[alan-agius4]

Good call! Updated findExecutableOnPath to skip non-absolute PATH entries so relative paths like . won't fall back to process.cwd() resolution.

[clydin]

Other tools don't do this and it seems quite complex with the mix of PATH/PATHEXT and multiple system calls. Are we sure this is a viable path forward? This can also break legitimate use cases like custom git shims or developer/project specific wrappers.

If we were to do this, would it be better to try setting the NoDefaultCurrentDirectoryInExePath environment variable on Windows early in the CLI's lifecycle to avoid this scenario at the system level? Ref: https://learn.microsoft.com/en-us/windows/win32/api/processenv/nf-processenv-needcurrentdirectoryforexepathw

[bilguunbicktivism]

@clydin I think you're right, and it's checkable, so I measured it rather than argue. Windows 10.0.26200, Node v24.17.0, a plant git.exe in the working directory that writes a canary and proxies to the real git.

Setting the variable from inside the running process works — it does not need to be inherited:

  case                         var        canaryFired
  execFileSync, unset          (unset)    true       <- the bug
  execFileSync, "1"            "1"        false
  execFileSync, "" (empty)     ""         false
  execFileSync, "0"            "0"        false
  spawnSync, unset             (unset)    true
  spawnSync, "1"               "1"        false
  shell:true, unset            (unset)    true
  shell:true, "1"              "1"        false

Negative control, same runs against a directory with no plant: canary never fires and the real git answers, so the false rows are the mitigation and not a broken harness.

Three things that follow, and one of them is a foot-gun:

1. process.env.NoDefaultCurrentDirectoryInExePath = '1' set early in the CLI lifecycle is sufficient, and it covers more than the PATH resolver does — it also covers shell: true, which goes through cmd.exe and would not be helped by findExecutableOnPath. One line instead of PATH/PATHEXT logic across every call site.

2. Only definedness matters — the value is ignored. "" and even "0" enable it. So it must be deleted rather than set to a falsy value if anyone ever wants it off, and a check like if (process.env.NoDefault... === '1') would be wrong.

3. It only protects children spawned after the assignment, so "early in the lifecycle" is load-bearing. Anything that spawns during module initialisation, before that line runs, is still on the old behaviour.

On your "custom git shims" concern — that cuts against this approach too, and more bluntly: the env var is process-global, so it disables working-directory resolution for every child the CLI spawns, whereas findExecutableOnPath at least leaves the decision per call site. The trade-off is real either way; the difference is that the env var makes it one visible decision instead of N.

Happy to share the probe scripts if useful.

[clydin]

In that case, I would propose adding something similar to the following near the top of https://github.com/angular/angular-cli/blob/04888ea72136426672cf002327c1d413cbdda71a/packages/angular/cli/bin/ng.js

// Ensure Windows CreateProcess does not search the current directory for bare executable names.
if (!process.env['NG_ALLOW_CURRENT_DIR_EXE']) {
  process.env['NoDefaultCurrentDirectoryInExePath'] = '1';
}

The NG_ALLOW_CURRENT_DIR_EXE "escape hatch" environment variable is debatable. The alternative is for a user to add . to the PATH which could be considered more ideal since it is cross-platform.

As a separate fix, it may be useful to replace the direct execution of the which utility with the https://www.npmjs.com/package/which package. There's no guarantee that the utility will exist cross platform. While the usage appears to handle that case, it seems it could result in false negatives. This would also provide the avenue of local bare executable discovery without maintaining the complexity, if preferred in the future.

[bilguunbicktivism]

Measured both halves of that proposal on Windows 10.0.26200 / Node v24.17.0, same plant harness as before (a git.exe in the working directory that writes a canary and proxies to the real git). I ran your guard verbatim.

The .-on-PATH alternative works, so the escape hatch is not needed:

  mitigation on, "." prepended to PATH        NoDefault=1   plantRan=true
  mitigation on, DIR prepended to PATH        NoDefault=1   plantRan=true

Even with NoDefaultCurrentDirectoryInExePath=1, an explicit . (or the project dir) on PATH restores the old resolution. So a user with a project-local git shim already has a cross-platform way back, exactly as you said.

And the hatch has inverted semantics for the two values a user is most likely to reach for:

  hatch unset      NoDefault=1        plantRan=false    <- protected
  hatch="1"        NoDefault=(unset)  plantRan=true     <- opted out, as intended
  hatch="0"        NoDefault=(unset)  plantRan=true     <- opted out, NOT intended
  hatch="false"    NoDefault=(unset)  plantRan=true     <- opted out, NOT intended
  hatch=""         NoDefault=1        plantRan=false    <- protected

if (!process.env['NG_ALLOW_CURRENT_DIR_EXE']) is a truthiness test, and "0" and "false" are non-empty strings, so someone setting NG_ALLOW_CURRENT_DIR_EXE=0 to mean "no, keep me protected" silently turns the protection off.

Worth noting the two variables in that snippet are inverted in opposite directions: NoDefaultCurrentDirectoryInExePath is definedness-only ("0" and "" both enable it, measured earlier in this thread), while the hatch is truthiness ("0" disables the guard). Two env vars, one patch, opposite conventions.

Given the .-on-PATH route demonstrably works, dropping NG_ALLOW_CURRENT_DIR_EXE entirely looks like the cleaner call — it removes the foot-gun rather than documenting around it.

No opinion on the which package swap, I have not measured that one.

[dgp1130]

This can also break legitimate use cases like custom git shims or developer/project specific wrappers.

+1, whether or not we agree with Windows decision to execute binaries in the CWD, that is the decision Microsoft made and actively preventing it creates compatibility problems, either for node_modules/ in our transitive dependencies or user projects which might rely on the feature. This strikes me as a request to harden Windows rather than Angular.

That said, I can agree defense in depth is generally helpful and I agree this feature is bad for security, so I don't object to mitigating the impact here and will defer to you @alan-agius4, just sharing my own perspective.

[dgp1130]

TIL about PATHEXT.

[dgp1130]

Question: Is there a specific platform which uses ${Path} as distinct from ${PATH}? I've never seen that before.

[dgp1130]

Nit: Prefer ?? to ||.

[dgp1130]

Question: Do we have to deal with escaping? The fact that we need to parse quotes makes me generally uncomfortable that there's more hidden complexity here.