Skip to content

Conversation

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Oct 27, 2025

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. npm Issues and PRs related to the npm client dependency or the npm registry. v24.x Issues that can be reproduced on v24.x or PRs targeting the v24.x-staging branch. labels Oct 27, 2025
@github-actions
Copy link
Contributor

Fast-track has been requested by @nodejs-github-bot. Please 👍 to approve.

@ChALkeR ChALkeR marked this pull request as ready for review October 27, 2025 13:25
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Shouldn't we apply this diff also to

const buf = Buffer.allocUnsafe(stat.size);
import_node_fs.default.readSync(fd, buf, 0, stat.size, 0);
p.end(buf);

@ChALkeR
Copy link
Member Author

ChALkeR commented Oct 27, 2025

@aduh95 this PR targets 24.x branch.
The problematic code in corepack did not get into 24.x

@aduh95 aduh95 changed the title deps: patch npm/tar to not return uninitialized mem [v24.x] deps: patch npm/tar to not return uninitialized mem Oct 27, 2025
@aduh95
Copy link
Contributor

aduh95 commented Oct 27, 2025

If main also has the same problematic code, why not land the fix there instead of v24.x-staging?

@ChALkeR
Copy link
Member Author

ChALkeR commented Oct 27, 2025

@aduh95 is there a reason to bypass the regular procedure in main?
Depends on when would isaacs/node-tar#446 be landed, if soon we could just update it as a regular dep.
24.x has the whole #60423 issue and is scheduled for LTS now

@ChALkeR
Copy link
Member Author

ChALkeR commented Oct 27, 2025

@aduh95 also pls fell free to retarget this to the correct branch / force-push to my branch, I'm out of context of the staging branches

@aduh95
Copy link
Contributor

aduh95 commented Oct 27, 2025

is there a reason to bypass the regular procedure in main?

The regular procedure is for changes to first land upstream, then it can be cherry-picked to main, then to the vXX.x-staging branch of Current release, then be backported to LTS further after being in Current for two weeks. IIUC you're suggesting that we follow that procedure to main and v25.x, and this PR is a shortcut for v24.x only, correct?
In that case, do we want to take the shortcut? I mean, if v24.x is wrongfully zero-filling the buffer, the code is fine for the time being, no?

@ChALkeR
Copy link
Member Author

ChALkeR commented Oct 28, 2025

do we want to take the shortcut?

Only if we we want to merge the non zero-filling behavior to 24 asap.
This PR was made to unblock that specifically

It's not needed otherwise

@ChALkeR
Copy link
Member Author

ChALkeR commented Oct 28, 2025

See #60012 (comment)

I also rechecked the usage (so it was now checked by 2 people), npm does not use the vulnerable codepath
This can be closed, patching npm>tar is a non-blocker

@ChALkeR ChALkeR closed this Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. npm Issues and PRs related to the npm client dependency or the npm registry. v24.x Issues that can be reproduced on v24.x or PRs targeting the v24.x-staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants