fix(dts-plugin): do not overwrite on extract when the types folder was just deleted - #5025
fix(dts-plugin): do not overwrite on extract when the types folder was just deleted#5025taufiq-dev wants to merge 2 commits into
Conversation
…s just deleted adm-zip 0.5.9 through 0.6.0 (CVE-2026-76845) follows symlinks at the extraction destination, but only when overwrite is enabled. downloadTypesArchive removes the destination folder right before extracting when deleteTypesFolder is true (the default), so passing overwrite: true there was redundant. Tie overwrite to !deleteTypesFolder so the default path no longer meets the CVE's precondition, while users who keep an existing folder still get refreshed types. Fixes module-federation#5024
🦋 Changeset detectedLatest commit: 5d424ba The changes in this PR will be included in the next version bump. This PR includes changesets to release 48 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac3e066b2f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // and overwrite is redundant. Keeping it off removes the precondition of | ||
| // CVE-2026-76845 (adm-zip follows symlinks at the destination only when | ||
| // overwrite is enabled). | ||
| zip.extractAllTo(destinationPath, !hostOptions.deleteTypesFolder); |
There was a problem hiding this comment.
When deleteTypesFolder is true but rm rejects while leaving the destination usable—for example, an unrelated locked child prevents removal on Windows—this passes overwrite: false, so adm-zip silently skips every archive entry whose path already exists and the function still logs and returns success. Previously those matching declarations were refreshed despite the cleanup failure; now consumers can unknowingly compile against stale types. Propagate the removal failure into the retry/error path rather than extracting under the assumption that the folder was deleted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, agreed that silently stale types is worse than the previous behaviour. Fixed in e93da36: overwrite is now tied to whether the rm actually succeeded (typesFolderRemoved), not to the option alone. So the default happy path extracts with overwrite: false, while deleteTypesFolder: false and a failed cleanup both keep overwrite: true exactly as before. I chose that over aborting on cleanup failure because the previous behaviour on that path was to refresh what it could and succeed, and changing that felt out of scope for a CVE mitigation.
Review feedback: with overwrite tied to deleteTypesFolder alone, a failed rm that leaves the folder usable would make adm-zip silently skip existing entries and still report success, so consumers could compile against stale types. Track whether the removal actually succeeded and only disable overwrite in that case; the failure path keeps the previous behaviour.
e93da36 to
5d424ba
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Description
downloadTypesArchiveremoves the types destination folder right before extracting the remote's@mf-types.zipwhendeleteTypesFolderis enabled (the default), then callszip.extractAllTo(destinationPath, true). In that path every entry is new, sooverwrite: truewas redundant.adm-zip 0.5.9 through 0.6.0 (the version dts-plugin pins) is affected by CVE-2026-76845, symlink following at the extraction destination. Per the CVE text, the write is only reachable when overwrite is enabled, because adm-zip's
existsSynccheck otherwise declines. This PR passesoverwriteonly when something may already exist at the destination, tracked by whether thermactually succeeded (typesFolderRemoved):deleteTypesFolder: true,rmsucceeded): overwrite is off, the CVE precondition is no longer met, and the written output is unchanged since every entry is new.deleteTypesFolder: false: overwrite stays on, so users who keep an existing folder still get refreshed types, exactly as before.deleteTypesFolder: truebutrmfailed (already caught and logged): overwrite stays on, exactly as before, so consumers never get silently stale types. (Updated after review feedback; the first revision tied overwrite to the option alone.)This is a mitigation of the reachable path in this package, not a fix of adm-zip. A fixed adm-zip release (cthackers/adm-zip#575 is open upstream) plus a bump is still the complete fix.
Not in this PR:
packages/native-federation-typescriptandpackages/native-federation-testshave the samermthenextractAllTo(destinationPath, true)sequence in theirarchiveHandler.tsand also depend onadm-zip ^0.6.0. Happy to apply the same change there in this PR or a follow-up if maintainers want it.Related Issue
Fixes #5024
Types of changes
Checklist
No test added: the change is a single argument on the default path with identical output, and there is no existing test asserting the
extractAllToarguments. I can add one if you would like it pinned. Changeset included (@module-federation/dts-pluginpatch).