Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: remove erroneous CVE-2024-27980 revert option #52543

Merged

Conversation

tniessen
Copy link
Member

No security reverts should exist on the main branch.

It seems to me that this was done correctly by @bnoordhuis in https://github.com/nodejs-private/node-private/pull/565 but that commit somehow didn't end up on the main branch in this repository.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Apr 15, 2024
@tniessen tniessen added the security Issues and PRs related to security. label Apr 15, 2024
No security reverts should exist on the main branch.
@tniessen tniessen force-pushed the src-remove-revert-cve-2024-27980 branch from 836c4c0 to fe22cd6 Compare April 15, 2024 18:46
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2024
@nodejs-github-bot

This comment was marked as outdated.

@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 16, 2024
@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS
Copy link
Member

RafaelGSS commented Apr 16, 2024

No security reverts should exist on the main branch.

It seems to me that this was done correctly by @bnoordhuis in nodejs-private/node-private#565 but that commit somehow didn't end up on the main branch in this repository.

That was on purpose. That's why I left https://github.com/nodejs-private/node-private/pull/565 open, otherwise, I would need to avoid this commit somehow in the last sync for Node.js 22 (which is basically a git reset --hard upstream/main). I was planning to include it as soon as v22.x arrises.

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

Note, I would not merge it until v22.0.0 is out.

@RafaelGSS RafaelGSS added the blocked PRs that are blocked by other issues or PRs. label Apr 16, 2024
@tniessen
Copy link
Member Author

@RafaelGSS Should I rebase #52365 then and then later rebase this PR on top of that or should I consider both blocked? I wasn't aware that we can't merge non-v22 commits into main at this point.

@RafaelGSS
Copy link
Member

RafaelGSS commented Apr 16, 2024

@RafaelGSS Should I rebase #52365 then and then later rebase this PR on top of that or should I consider both blocked? I wasn't aware that we can't merge non-v22 commits into main at this point.

Technically, we can... but it makes our process of a semver-major release slightly harder. For instance, you can add a dont-land-on-v22.x label, but it will require some adjustments in our sync that might result in conflicts. At this point, we are not planning any further sync to v22.x (except including V8 commit). I have added the blocked label just in case a sync is necessary (which is unlikely).

I think a rebase is not necessary, #52365 can land without issues.

@richardlau
Copy link
Member

No security reverts should exist on the main branch.
It seems to me that this was done correctly by @bnoordhuis in nodejs-private/node-private#565 but that commit somehow didn't end up on the main branch in this repository.

That was on purpose. That's why I left nodejs-private/node-private#565 open, otherwise, I would need to avoid this commit somehow in the last sync for Node.js 22 (which is basically a git reset --hard upstream/main). I was planning to include it as soon as v22.x arrises.

I don't understand why the revert is included for Node.js 22 -- we haven't had any releases of that yet and we should release Node.js 22 without any security reverts.

@RafaelGSS
Copy link
Member

No security reverts should exist on the main branch.
It seems to me that this was done correctly by @bnoordhuis in nodejs-private/node-private#565 but that commit somehow didn't end up on the main branch in this repository.

That was on purpose. That's why I left nodejs-private/node-private#565 open, otherwise, I would need to avoid this commit somehow in the last sync for Node.js 22 (which is basically a git reset --hard upstream/main). I was planning to include it as soon as v22.x arrises.

I don't understand why the revert is included for Node.js 22 -- we haven't had any releases of that yet and we should release Node.js 22 without any security reverts.

I have included it because a backport specifically to Node.js 22 was created: https://github.com/nodejs-private/node-private/pull/560.

@targos
Copy link
Member

targos commented Apr 16, 2024

I suppose a backport was created because the branch existed.

@RafaelGSS RafaelGSS removed the blocked PRs that are blocked by other issues or PRs. label Apr 16, 2024
@RafaelGSS
Copy link
Member

If you all agree, let's merge it then. I can pull directly into v22.x.

Copy link
Member

@larson-carter larson-carter left a comment

Choose a reason for hiding this comment

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

I agree that these items need to be merged... LGTM

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 18, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 18, 2024
@nodejs-github-bot nodejs-github-bot merged commit 3790d52 into nodejs:main Apr 18, 2024
64 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 3790d52

@richardlau richardlau added the dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. label Apr 18, 2024
@richardlau richardlau added dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v21.x labels Apr 18, 2024
RafaelGSS pushed a commit that referenced this pull request Apr 18, 2024
No security reverts should exist on the main branch.

PR-URL: #52543
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run. security Issues and PRs related to security.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants