Skip to content

Resolve Windows shell paths in the user environment - #868

Open
Kirtikumar Anandrao Ramchandani (KirtiRamchandani) wants to merge 2 commits into
PowerShell:latestw_allfrom
KirtiRamchandani:fix-windows-configured-shell-path-expansion
Open

Resolve Windows shell paths in the user environment#868
Kirtikumar Anandrao Ramchandani (KirtiRamchandani) wants to merge 2 commits into
PowerShell:latestw_allfrom
KirtiRamchandani:fix-windows-configured-shell-path-expansion

Conversation

@KirtiRamchandani

Summary

  • expand configured Windows shell paths in the target user context before validation/execution
  • require resolved DefaultShell values to be absolute so bare names like pwsh.exe are not looked up through PATH
  • apply the same %...% resolution to external subsystem executable paths while preserving existing relative built-in subsystem behavior
  • add Win32 compatibility tests for environment expansion, relative-name rejection, and unresolved-variable rejection

Validation

  • git diff --cached --check
  • MSBuild.exe contrib/win32/openssh/config.vcxproj /p:Configuration=Debug /p:Platform=x64 /p:WindowsTargetPlatformVersion=10.0.26100.0 /p:SpectreMitigation=false
  • MSBuild.exe contrib/win32/openssh/win32iocompat.vcxproj /p:Configuration=Debug /p:Platform=x64 /p:WindowsTargetPlatformVersion=10.0.26100.0 /p:SpectreMitigation=false

unittest-win32compat.vcxproj and sshd.vcxproj could not be completed locally because this checkout does not have the repo vcpkg/libressl outputs installed. The focused Win32 compatibility library target that contains the new resolver compiles successfully.

Fixes PowerShell/Win32-OpenSSH#2449

Copilot AI lite review requested due to automatic review settings August 8, 2026 04:52
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens Windows-specific execution by resolving %...%-based shell/subsystem paths in the target user’s environment before validation/execution, and by requiring resolved shell paths to be absolute to avoid PATH-based lookups.

Changes:

  • Add resolve_configured_user_path() (Win32) to expand %...% for a given user token and enforce “absolute path” requirements where needed.
  • Resolve/validate user shell paths (DefaultShell-derived) in the user context and reject non-absolute results on Windows.
  • Expand %...% for configured external subsystem executable paths and add Win32compat unit tests covering expansion and rejection cases.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
session.c Resolves %...% in external subsystem program paths (Windows) before stat/exec.
auth.c Resolves shell path in the target user context and enforces absolute shell paths on Windows.
contrib/win32/win32compat/misc.c Adds the user-context environment expansion helper resolve_configured_user_path().
contrib/win32/win32compat/misc_internal.h Exposes resolve_configured_user_path() in the internal Win32compat header.
regress/unittests/win32compat/miscellaneous_tests.c Adds unit tests for user-path resolution behavior and error cases.
Suppressed comments (1)

session.c:2035

  • The failure log at the end of this function always reports "subsystem not found", but the loop can also exit with a matching subsystem name where the configured path expansion failed (e.g., resolve_configured_user_path() returned NULL). This can mislead debugging and operations on Windows.
	if (!success)
		logit("subsystem request for %.100s by user %s failed, "
		    "subsystem not found", s->subsys, s->pw->pw_name);

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread contrib/win32/win32compat/misc.c Outdated
Comment on lines +1481 to +1484
if (wcschr(expanded_path, L'%') != NULL) {
errno = EINVAL;
goto cleanup;
}
Comment on lines +427 to +436
char *out;

TEST_START("configured path expansion");
SetEnvironmentVariableA("OPENSSH_TEST_ROOT", "C:\\OpenSSHTest");
out = resolve_configured_user_path("%OPENSSH_TEST_ROOT%\\pwsh.exe",
NULL, 1);
ASSERT_STRING_EQ(out, "C:\\OpenSSHTest\\pwsh.exe");
free(out);
SetEnvironmentVariableA("OPENSSH_TEST_ROOT", NULL);
TEST_DONE();
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.

DefaultShell and app exec alias support

2 participants