Skip to content

Fix exception check#131078

Open
alinpahontu2912 wants to merge 1 commit into
dotnet:release/8.0-stagingfrom
alinpahontu2912:tar_test-8
Open

Fix exception check#131078
alinpahontu2912 wants to merge 1 commit into
dotnet:release/8.0-stagingfrom
alinpahontu2912:tar_test-8

Conversation

@alinpahontu2912

Copy link
Copy Markdown
Member

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.

@azure-pipelines

Copy link
Copy Markdown
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.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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) or IOException (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.

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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
    }
  ]
}

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@alinpahontu2912

Copy link
Copy Markdown
Member Author

/ba-g test failures are unrelated to my change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants