Skip to content

fix: harden git exec resolution in SourceLink metadata #53

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ANcpLua wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: harden git exec resolution in SourceLink metadata #53

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions src/Build/Enforcement/DeterminismAndSourceLink.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,20 @@
<PropertyGroup>
<_StderrRedirect Condition="'$(OS)' == 'Windows_NT'"> 2&gt;nul</_StderrRedirect>
<_StderrRedirect Condition="'$(OS)' != 'Windows_NT'"> 2&gt;/dev/null</_StderrRedirect>

<!-- Resolve git to a trusted absolute path so repo-local binary planting cannot hijack Exec. -->
<_GitExe Condition="'$(_GitExe)' == '' and '$(GitExe)' != '' and $([System.IO.Path]::IsPathRooted('$(GitExe)'))">$(GitExe)</_GitExe>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 22, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In MSBuild property functions on the supported SDK/Visual Studio versions, does System.IO.Path.IsPathRooted("C:git.exe")return true, and isSystem.IO.Path.IsPathFullyQualified available for enforcing a truly absolute path?

💡 Result:

  • Yes. On Windows, System.IO.Path.IsPathRooted("C:git.exe") returns true because IsPathRooted treats “drive letter + colon” as “rooted” (even though it’s still drive-relative, not truly absolute). The .NET docs for IsPathFullyQualified show the same behavior with "C:Documents": Rooted = True, Fully qualified = False. [1], [2]

  • System.IO.Path.IsPathFullyQualified(...) exists on modern .NET / .NET Standard (not .NET Framework) (the API is documented as coming from netstandard.dll / System.Runtime and is intended to distinguish "C:a" vs "C:\\a"). [1]
    MSBuild property functions can call public static .NET methods, but only those available in the runtime hosting MSBuild. [3]

  • Practical upshot for “supported SDK/VS versions”:

    • If you build with the .NET SDK MSBuild (for example via dotnet msbuild / dotnet build), you can use Path.IsPathFullyQualified to enforce a truly absolute path. [4]
    • If you build with MSBuild running on .NET Framework (common for Visual Studio’s in-proc/toolset MSBuild), Path.IsPathFullyQualified is not available, so calling it in a property function will fail at evaluation time (you’ll need an alternate check, e.g., pattern-testing for ^[A-Za-z]:[\\/]|^\\\\). [1], [3]

Sources: [1] [2] [3] [4]

Require $(GitExe) to be fully qualified and to exist before trusting it.

Path.IsPathRooted accepts drive-relative Windows paths like C:git.exe (rooted, not fully qualified), which can resolve relative to the current directory and weakens this hardening. The check also short-circuits fallback probes even when the file doesn't exist, silently disabling git metadata on a stale override.

Use Path.IsPathFullyQualified on .NET SDK MSBuild (e.g., dotnet build), or implement an equivalent check for .NET Framework MSBuild. Always pair with Exists validation.

Suggested change 
-    <_GitExe Condition="'$(_GitExe)' == '' and '$(GitExe)' != '' and $([System.IO.Path]::IsPathRooted('$(GitExe)'))">$(GitExe)</_GitExe>
+    <_GitExe Condition="'$(_GitExe)' == '' and '$(GitExe)' != '' and $([System.IO.Path]::IsPathFullyQualified('$(GitExe)')) and Exists('$(GitExe)')">$(GitExe)</_GitExe>

For .NET Framework MSBuild, replace IsPathFullyQualified with a pattern check or equivalent validation ensuring the path is truly absolute (e.g., ^[A-Za-z]:\\|^\\\\).

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/Build/Enforcement/DeterminismAndSourceLink.props` at line 25, The
condition that sets _GitExe from $(GitExe) should require a truly
fully-qualified path and verify the file exists; replace the Path.IsPathRooted
check with Path.IsPathFullyQualified when available (or a regex check such as
^[A-Za-z]:\\|^\\\\ for .NET Framework) and add an Exists check so the assignment
only occurs if the path is fully qualified and File.Exists('$(GitExe)') is true;
update the conditional that references _GitExe and GitExe to use these stricter
validations to avoid accepting drive-relative paths like C:git.exe or stale
overrides.

<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' != 'Windows_NT' and Exists('/usr/bin/git')">/usr/bin/git</_GitExe>
<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' != 'Windows_NT' and Exists('/usr/local/bin/git')">/usr/local/bin/git</_GitExe>
Comment on lines +25 to +27
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 22, 2026

Choose a reason for hiding this comment

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

P2 Badge Preserve PATH-based git resolution

_GitExe is now only set from a short list of hard-coded absolute paths or an absolute GitExe override. For consumers whose Git is only discoverable through PATH—for example Homebrew/Nix installs on Unix or Scoop/portable installs on Windows—_GitExe stays empty, the rev-parse --show-toplevel call at line 35 is skipped, and this target then flips _ANcpLuaInGitRepo to false. The build still succeeds, but SourceLink and repository metadata are silently dropped for projects that are actually inside a git repo, which is a regression from the previous plain git ... invocations.

Useful? React with 👍 / 👎.

<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(ProgramW6432)\Git\cmd\git.exe')">$(ProgramW6432)\Git\cmd\git.exe</_GitExe>
<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(ProgramFiles)\Git\cmd\git.exe')">$(ProgramFiles)\Git\cmd\git.exe</_GitExe>
<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(ProgramFiles)\Git\bin\git.exe')">$(ProgramFiles)\Git\bin\git.exe</_GitExe>
<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(LocalAppData)\Programs\Git\cmd\git.exe')">$(LocalAppData)\Programs\Git\cmd\git.exe</_GitExe>
Comment on lines +28 to +31
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

On Windows, properties like $(ProgramW6432) can be empty (e.g., 32-bit environments). In that case Exists('$(ProgramW6432)\Git\cmd\git.exe') becomes Exists('\Git\cmd\git.exe'), which can unexpectedly match a drive-root path and undermines the intent of selecting a well-known install location. Add guards like '$(ProgramW6432)' != '' (and similarly for other base dirs) before the Exists(...) checks; also consider adding $(ProgramFiles(x86))\Git\cmd\git.exe as another common Git-for-Windows install location.

Suggested change
<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(ProgramW6432)\Git\cmd\git.exe')">$(ProgramW6432)\Git\cmd\git.exe</_GitExe>
<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(ProgramFiles)\Git\cmd\git.exe')">$(ProgramFiles)\Git\cmd\git.exe</_GitExe>
<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(ProgramFiles)\Git\bin\git.exe')">$(ProgramFiles)\Git\bin\git.exe</_GitExe>
<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(LocalAppData)\Programs\Git\cmd\git.exe')">$(LocalAppData)\Programs\Git\cmd\git.exe</_GitExe>
<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and '$(ProgramW6432)' != '' and Exists('$(ProgramW6432)\Git\cmd\git.exe')">$(ProgramW6432)\Git\cmd\git.exe</_GitExe>
<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and '$(ProgramFiles(x86))' != '' and Exists('$(ProgramFiles(x86))\Git\cmd\git.exe')">$(ProgramFiles(x86))\Git\cmd\git.exe</_GitExe>
<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and '$(ProgramFiles)' != '' and Exists('$(ProgramFiles)\Git\cmd\git.exe')">$(ProgramFiles)\Git\cmd\git.exe</_GitExe>
<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and '$(ProgramFiles)' != '' and Exists('$(ProgramFiles)\Git\bin\git.exe')">$(ProgramFiles)\Git\bin\git.exe</_GitExe>
<_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and '$(LocalAppData)' != '' and Exists('$(LocalAppData)\Programs\Git\cmd\git.exe')">$(LocalAppData)\Programs\Git\cmd\git.exe</_GitExe>

Copilot uses AI. Check for mistakes.
</PropertyGroup>

<Target Name="_ANcpLuaComputeGitMetadata" BeforeTargets="InitializeSourceControlInformation;GenerateNuspec">
<Exec Command="git -C &quot;$(MSBuildProjectDirectory)&quot; rev-parse --show-toplevel$(_StderrRedirect)"
<Exec Command="&quot;$(_GitExe)&quot; -C &quot;$(MSBuildProjectDirectory)&quot; rev-parse --show-toplevel$(_StderrRedirect)"
Condition="'$(_GitExe)' != ''"
ConsoleToMSBuild="true"
IgnoreExitCode="true"
StandardOutputImportance="Low"
Expand All @@ -44,7 +54,7 @@
<NoWarn>$(NoWarn);NETSDK1018</NoWarn>
</PropertyGroup>

<Exec Command="git -C &quot;$(_GitRoot)&quot; config --get remote.origin.url$(_StderrRedirect)"
<Exec Command="&quot;$(_GitExe)&quot; -C &quot;$(_GitRoot)&quot; config --get remote.origin.url$(_StderrRedirect)"
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 22, 2026

Choose a reason for hiding this comment

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

medium

For consistency with the first Exec task and to improve robustness, it's advisable to add a condition here to ensure this task only runs if _GitExe has been successfully resolved. Without the condition, if _GitExe is empty, MSBuild will attempt to execute an empty command, which will fail. Although IgnoreExitCode="true" prevents a build failure, explicitly skipping the task is a cleaner approach.

    <Exec Command="&quot;$(_GitExe)&quot; -C &quot;$(_GitRoot)&quot; config --get remote.origin.url$(_StderrRedirect)"
          Condition="'$(_GitExe)' != ''"

ConsoleToMSBuild="true"
IgnoreExitCode="true"
StandardOutputImportance="Low"
Expand All @@ -53,7 +63,7 @@
<Output TaskParameter="ConsoleOutput" PropertyName="_GitOriginUrlRaw"/>
</Exec>

<Exec Command="git -C &quot;$(_GitRoot)&quot; symbolic-ref --quiet HEAD$(_StderrRedirect)"
<Exec Command="&quot;$(_GitExe)&quot; -C &quot;$(_GitRoot)&quot; symbolic-ref --quiet HEAD$(_StderrRedirect)"
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 22, 2026

Choose a reason for hiding this comment

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

medium

Similar to my other comments, adding a condition to this Exec task will make the logic more robust by preventing an attempt to run an empty command if a trusted git executable isn't found.

    <Exec Command="&quot;$(_GitExe)&quot; -C &quot;$(_GitRoot)&quot; symbolic-ref --quiet HEAD$(_StderrRedirect)"
          Condition="'$(_GitExe)' != ''"

ConsoleToMSBuild="true"
IgnoreExitCode="true"
StandardOutputImportance="Low"
Expand All @@ -62,7 +72,7 @@
<Output TaskParameter="ConsoleOutput" PropertyName="_GitBranchRefRaw"/>
</Exec>

<Exec Command="git -C &quot;$(_GitRoot)&quot; rev-parse --abbrev-ref HEAD$(_StderrRedirect)"
<Exec Command="&quot;$(_GitExe)&quot; -C &quot;$(_GitRoot)&quot; rev-parse --abbrev-ref HEAD$(_StderrRedirect)"
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 22, 2026

Choose a reason for hiding this comment

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

medium

To maintain consistency and prevent errors from attempting to run an empty command, please add a condition to this Exec task to ensure it only runs when _GitExe is defined.

    <Exec Command="&quot;$(_GitExe)&quot; -C &quot;$(_GitRoot)&quot; rev-parse --abbrev-ref HEAD$(_StderrRedirect)"
          Condition="'$(_GitExe)' != ''"

ConsoleToMSBuild="true"
IgnoreExitCode="true"
StandardOutputImportance="Low"
Expand All @@ -71,7 +81,7 @@
<Output TaskParameter="ConsoleOutput" PropertyName="_GitBranchNameRaw"/>
</Exec>

<Exec Command="git -C &quot;$(_GitRoot)&quot; rev-parse HEAD$(_StderrRedirect)"
<Exec Command="&quot;$(_GitExe)&quot; -C &quot;$(_GitRoot)&quot; rev-parse HEAD$(_StderrRedirect)"
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 22, 2026

Choose a reason for hiding this comment

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

medium

As with the other git commands in this target, this Exec task should also be conditional on _GitExe being set. This will prevent it from trying to execute an empty command if git is not found in one of the trusted locations.

    <Exec Command="&quot;$(_GitExe)&quot; -C &quot;$(_GitRoot)&quot; rev-parse HEAD$(_StderrRedirect)"
          Condition="'$(_GitExe)' != ''"

ConsoleToMSBuild="true"
IgnoreExitCode="true"
StandardOutputImportance="Low"
Expand Down
Loading