Detect and reject symbolic links in the Unix cache-file write path#6115
Merged
Conversation
Before writing or touching the cache file on non-Windows platforms, check whether the path is a symbolic link using File.GetAttributes, which maps to lstat(2) on Unix and returns FileAttributes.ReparsePoint for the symlink itself without following it. If a symlink is detected, throw InvalidOperationException with a clear message before entering the TryProcessFile retry loop. This ensures the exception propagates to the caller rather than being silently swallowed by the loop's catch-all handler. Applies to both CreateAndWriteToFile (PosixCreate/creat path) and TouchFile (File.Create path). Works on netstandard2.0 and net8.0 — no conditional compilation needed since File.GetAttributes is available on all target frameworks. Tests: two new facts in FileIOWithRetriesTests covering CreateAndWriteToFile and TouchFile, each creating a real symlink via File.CreateSymbolicLink (net6+) / ln fallback, asserting the InvalidOperationException and message text. Tests are marked [DoNotRunOnWindows] and are skipped on the net48 TFM at runtime. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 00c4f24d-188f-4bd9-926c-a6d0620eb053
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the non-Windows cache-file write/touch path by detecting symbolic links at the cache path and rejecting them early, preventing writes from being redirected through symlinks without any diagnostic.
Changes:
- Added a non-Windows pre-check in
FileIOWithRetriesto detect symlinks and throwInvalidOperationExceptionbefore entering the retry loop. - Documented the new symlink constraint on the Unix permissioned-write helper.
- Added unit tests intended to validate the new behavior on non-Windows platforms.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/client/Microsoft.Identity.Client.Extensions.Msal/FileIOWithRetries.cs | Adds symlink detection and rejects cache paths that are symlinks on Unix before write/touch operations. |
| src/client/Microsoft.Identity.Client.Extensions.Msal/Accessors/FileWithPermissions.cs | Documents the symlink pre-check expectation for Unix permissioned writes. |
| tests/Microsoft.Identity.Test.Unit/CacheExtension/FileIOWithRetriesTests.cs | Adds regression tests to ensure symlinked cache paths are rejected on non-Windows. |
…ling symlink test - Drop [TestMethod] from the two new [DoNotRunOnWindows] tests; the attribute already derives from TestMethodAttribute so combining both was redundant and could interfere with platform-skip discovery. - Fix <remarks> on WriteToNewFileWithOwnerRWPermissionsUnix to only credit CreateAndWriteToFile (setChmod600: true) as the enforcing caller; TouchFile never reaches that method. - Add TouchFile_DanglingSymlinkAtCachePath_ThrowsInvalidOperationException: a dangling symlink (target does not exist) must also be rejected. Pins that File.GetAttributes returns ReparsePoint for broken links rather than throwing FileNotFoundException, which would silently bypass the check via the catch block. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 00c4f24d-188f-4bd9-926c-a6d0620eb053
Replace the PosixCreate (creat) P/Invoke with PosixOpen (open) using O_CREAT|O_WRONLY|O_TRUNC|O_NOFOLLOW. This closes the TOCTOU window between the lstat-based pre-check in FileIOWithRetries and the actual write: the kernel now atomically rejects a symlink placed in that gap, returning ELOOP. Flag values are platform-conditional (Linux/macOS differ for O_CREAT, O_TRUNC, and O_NOFOLLOW). ELOOP is handled in the fileDescriptor == -1 branch and surfaced as InvalidOperationException with the same clear message as the pre-check. The two layers are complementary: - Pre-check (lstat): catches the non-adversarial misconfiguration case with a diagnostic message, before the retry loop is entered. - O_NOFOLLOW (kernel): closes the race window for the adversarial case silently and atomically. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 00c4f24d-188f-4bd9-926c-a6d0620eb053
bgavrilMS
approved these changes
Jul 13, 2026
iNinja
enabled auto-merge (squash)
July 13, 2026 15:12
gladjohn
approved these changes
Jul 13, 2026
1 task
This was referenced Jul 16, 2026
Open
Merged
This was referenced Jul 16, 2026
Closed
fix: Bump Microsoft.Identity.Client from 4.86.0 to 4.86.1
Halceyon/open-telemetry-trace-listener#239
Merged
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On non-Windows platforms,
FileIOWithRetries.CreateAndWriteToFileandTouchFilecould operate on a cache file path that is a symbolic link, writing through to whatever the link points to rather than the intended cache file location. The caller receives no diagnostic -- the write succeeds silently from its perspective.This change adds a pre-check before the write path that detects a symbolic link at the given path and throws
InvalidOperationExceptionwith a clear message if one is found.Changes
FileIOWithRetries.cs: addThrowIfCacheFileIsSymlink-- usesFile.GetAttributes, which maps tolstat(2)on Unix and therefore inspects the link itself rather than its target.FileNotFoundExceptionis caught to handle the normal first-write case where the file does not yet exist. The check runs beforeTryProcessFileso the exception propagates to the caller rather than being swallowed by the retry loop. Applied to bothCreateAndWriteToFileandTouchFile. Guarded by!IsWindowsPlatform()-- no-op on Windows.FileWithPermissions.cs:<remarks>onWriteToNewFileWithOwnerRWPermissionsUnixnoting that the caller enforces the symlink pre-check.FileIOWithRetriesTests.cs: two new tests -- one forCreateAndWriteToFile, one forTouchFile-- each creating a symlink at the target path and assertingInvalidOperationException. Marked[DoNotRunOnWindows]; useFile.CreateSymbolicLinkon net6+ with alnfallback for net48 (which only runs on Windows and is skipped).