Skip to content

fix(zip): never write through a symlink when extracting - #43910

Merged
Yury Semikhatsky (yury-s) merged 3 commits into
microsoft:mainfrom
yury-s:fix-extract-zip-symlink
Nov 5, 2026
Merged

Yury Semikhatsky (yury-s) merged 3 commits into
microsoft:mainfrom
yury-s:fix-extract-zip-symlink

Conversation

@yury-s

@yury-s Yury Semikhatsky (yury-s) commented Nov 4, 2026 •

Copy link
Copy Markdown
Member

Summary

  • A zip could contain a symlink entry followed by a regular file with the same name; the parent-directory check passed for both and the write followed the symlink out of the extraction directory.
  • Open extracted files with the wx flag. O_EXCL fails when the destination exists, including a symlink, so the kernel enforces the check.
  • The blob reporter added the same attachment file to the zip once per attachment; add each entry once.

A zip archive could contain a symlink entry followed by a regular file
with the same name. The parent directory check passed for both, and
the file write followed the symlink out of the extraction directory.

Symlink entries are now rejected unless the caller passes
allowSymlinks. Only browser archives need them (macOS framework
bundles); report zips opened with show-report never do. With
allowSymlinks, the extractor still refuses to write through an
existing symlink.
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

…or symlinks

O_EXCL fails when the destination exists, including a symlink, so the
kernel enforces the check. Symlink entries are allowed again; the
browser archives on macOS need them.
@yury-s Yury Semikhatsky (yury-s) changed the title fix(zip): reject symlink entries and never write through a symlink fix(zip): never write through a symlink when extracting Nov 5, 2026
@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

🔴 The four test-runner failures are caused by this PR

Hi, I'm the Playwright bot. I checked the latest merged report, the PR diff, and the refreshed CI test history.

Details

Caused by this PR

[playwright-test] › reporter-blob.spec.ts:1951 › merge reports should preserve attachments fails on Windows, macOS, Ubuntu Node, and Ubuntu Bun with EEXIST: file already exists, open '…/tmp/resources/<hash>.html' (report, Ubuntu failure). These are four instances of the same regression.

The new flags: 'wx' rejects existing regular files as well as symlinks. This test attaches the same HTML file repeatedly. BlobReporter gives those attachments the same resource path and writes duplicate ZIP entries. The test's extractReport() calls the changed extractor, so the second entry for that path now fails. Report merging itself succeeds; the subsequent extraction fails.

The same test passed on all four platforms in the previous PR run. The refreshed database covers October 8–November 5 and has no EEXIST failures for this test outside this PR. Its two historical failures elsewhere were browser-launch/UI timeouts, not this error.

A possible fix is to write each resource path only once in BlobReporter, while preserving all attachment references and keeping the exclusive-write safeguard.

Triaged by the Playwright bot - agent run

@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

🟢 The latest MCP failure is a pre-existing flake

Hi, I'm the Playwright bot. I checked the latest merged report, the failed job, the PR diff, and the refreshed CI test history.

Details

The latest MCP report has one failure, which is not likely caused by this PR. This verdict covers MCP only; the separate verdict identifying the test-runner ZIP extraction regression still applies.

Pre-existing flake

[firefox] › mcp/sse.spec.ts:196 › sse transport browser lifecycle (persistent) fails on Ubuntu because localStorage.a returns undefined instead of "42" after reconnecting (failed job).

The refreshed test-results database records the same test, browser, platform, and error on main, before this PR: commit ac65100a5ffe1fba641c288d920ed87fd3e2f860, run 41081090942, October 30. Excluding #43910, this Ubuntu Firefox test failed in 8 runs and passed in 727 during October 8–November 4.

The PR changes ZIP extraction to reject existing destination files. It does not change SSE sessions, persistent browser profiles, or local storage, and this failure is a storage assertion, not an extraction error.

Triaged by the Playwright bot - agent run

Attachments that share a path and content type map to the same zip
entry. The reporter added the entry once per attachment, and the
extractor now refuses to overwrite an existing file.

Drop the extract-zip unit tests.
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Test results for "tests 1"

23586 passed, 704 skipped


Merge workflow run.

@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Test results for "MCP"

5194 passed, 616 skipped


Merge workflow run.

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.

2 participants