Fix exception check#131078
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-io |
There was a problem hiding this comment.
Pull request overview
Adjusts a Windows-specific test expectation for TarFile.ExtractToDirectory when extracting archives containing chained symlinks that attempt directory traversal. On Windows, symlink creation/handling for this scenario can fail at different points depending on OS build behavior, so the test is updated to accept both observed exception types while still verifying no traversal occurred.
Changes:
- Update the Windows assertion to accept either
UnauthorizedAccessException(failure while resolving a type-mismatched symlink) orIOException(failure when creating a directory where a file-symlink already exists). - Add detailed comments explaining why Windows can surface different exceptions for the same underlying rejection behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "9cc20173003f2e7750c109f9f7850aa7b7799360",
"last_dispatched_base_ref": "release/8.0-staging",
"last_dispatched_base_sha": "2b55485e3039d029405287530e1918a5617ec4a1",
"last_reviewed_commit": "9cc20173003f2e7750c109f9f7850aa7b7799360",
"last_reviewed_base_ref": "release/8.0-staging",
"last_reviewed_base_sha": "2b55485e3039d029405287530e1918a5617ec4a1",
"last_recorded_worker_run_id": "29747435667",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "9cc20173003f2e7750c109f9f7850aa7b7799360",
"review_id": 4735594091
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: The test ExtractToDirectory_RejectsChainedSymlinkDirectoryTraversal_WithNestedFile was flaky on Windows. Because Windows always creates file symlinks (FileInfo.CreateAsSymbolicLink), the entry a/b (whose target . is a directory) becomes a type-mismatched reparse point. Whether the extractor throws UnauthorizedAccessException (while resolving the link during the traversal-safety walk) or IOException (failing to create a directory where the file symlink already exists) depends on the Windows build, so a single expected exception type produced intermittent failures.
Approach: The Windows branch of the test now accepts either exception via Assert.ThrowsAny<Exception> followed by Assert.True(ex is IOException or UnauthorizedAccessException, ...), with an explanatory comment. The security-relevant post-conditions are unchanged: the test still verifies that outside/ and pwned.txt are not created, so the archive is still confirmed to be rejected regardless of the surfaced exception type. This is a test-only change targeting release/8.0-staging.
Summary: The relaxation is well-scoped and correct. Both accepted exception types genuinely represent rejection of the malicious archive, and the escape-prevention assertions that follow provide the real security guarantee. The diagnostic message on the type assertion is helpful. No production code is affected and no test coverage is weakened in a meaningful way. LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 31.8 AIC · ⌖ 14.4 AIC · ⊞ 10K
|
/ba-g test failures are unrelated to my change |
Windows symlinks are always created as files, not directories. Depending on the build, it can throw either during creation of a symlink that represents directory structure or when parsing it.