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

feat: add CVE-ID to commit-output #167

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Conversation

RafaelGSS
Copy link
Member

This PR changes the commit output to include CVE-ID in the output (part of automation of security releases - fixes: https://github.com/nodejs-private/security-release/issues/38)

Output:

➜  node (v18.20.4) ../changelog-maker/changelog-maker.js --group --markdown --filter-release --start-ref v18.20.3 --end-ref v18.20.4                                                                                                                        ✱
* \[[`85abedf1ff`](https://github.com/nodejs/node/commit/85abedf1ff)] - **(CVE-2024-22020)** **lib,esm**: handle bypass network-import via data: (RafaelGSS) [nodejs-private/node-private#522](https://github.com/nodejs-private/node-private/pull/522)
* \[[`eccd63b865`](https://github.com/nodejs/node/commit/eccd63b865)] - **(CVE-2024-36138)** **src**: handle permissive extension on cmd check (RafaelGSS) [nodejs-private/node-private#596](https://github.com/nodejs-private/node-private/pull/596)

➜  node (v18.20.4) ../changelog-maker/changelog-maker.js --group --plaintext --filter-release --start-ref v18.20.3 --end-ref v18.20.4                                                                                                                       ✱
lib,esm:
 * CVE-2024-22020 - handle bypass network-import via data: - https://github.com/nodejs-private/node-private/pull/522
src:
 * CVE-2024-36138 - handle permissive extension on cmd check - https://github.com/nodejs-private/node-private/pull/596
 
➜  node (v18.20.4) ../changelog-maker/changelog-maker.js --group --simple --filter-release --start-ref v18.20.3 --end-ref v18.20.4
* [85abedf1ff] - lib,esm: handle bypass network-import via data: (RafaelGSS) https://github.com/nodejs-private/node-private/pull/522
* [eccd63b865] - src: handle permissive extension on cmd check (RafaelGSS) https://github.com/nodejs-private/node-private/pull/596
 
➜  node (v18.20.4) ../changelog-maker/changelog-maker.js --group --messageonly --filter-release --start-ref v18.20.3 --end-ref v18.20.4
lib,esm:
  * CVE-2024-22020 - handle bypass network-import via data:
src:
  * CVE-2024-36138 - handle permissive extension on cmd check

I haven't added support to --simple as we won't use it in the automation. We can add it later if needed.

@RafaelGSS

This comment was marked as resolved.

@RafaelGSS
Copy link
Member Author

RafaelGSS commented Nov 12, 2024

I don't know why this is failing. I'm getting success locally:

➜  changelog-maker (add-cve-id-to-commitoutput) npm run test:ci                                                             ✱

> [email protected] test:ci
> npm run test


> [email protected] test
> npm run lint && npm run unit


> [email protected] lint
> standard


> [email protected] unit
> tap --allow-incomplete-coverage

 PASS  test.js 13 OK 9.473s

--------------------------|---------|----------|---------|---------|---------------------------
File                      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
--------------------------|---------|----------|---------|---------|---------------------------
All files                 |   84.02 |    75.29 |   96.29 |   84.02 |
 changelog-maker.js       |   76.66 |       50 |      80 |   76.66 | 41-43,45-53,86-99,118-119
 collect-commit-labels.js |    87.5 |    82.35 |     100 |    87.5 | 23-25,35-36,45-46
 commit-to-output.js      |   95.52 |    76.47 |     100 |   95.52 | 43-45,50,103-104
 find-matching-prs.js     |   56.94 |       75 |     100 |   56.94 | 41-71
 groups.js                |      78 |       75 |     100 |      78 | 40-50
 process-commits.js       |   91.95 |       80 |     100 |   91.95 | 12,14,28-29,52,81-82
 reverts.js               |   84.61 |       75 |     100 |   84.61 | 11-12
--------------------------|---------|----------|---------|---------|---------------------------


  🌈 TEST COMPLETE 🌈


Asserts:  13 pass  0 fail  13 of 13 complete
Suites:    1 pass  0 fail    1 of 1 complete

# { total: 13, pass: 13 }
# time=9570.561ms

It might be due to some credentials. Can someone test it too? cc: @nodejs/releasers

nodejs-github-bot pushed a commit to nodejs/node that referenced this pull request Nov 14, 2024
PR-URL: #55830
Refs: nodejs/changelog-maker#167
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
commit-to-output.js Outdated Show resolved Hide resolved
commit-to-output.js Outdated Show resolved Hide resolved
aduh95 pushed a commit to nodejs/node that referenced this pull request Nov 16, 2024
PR-URL: #55830
Refs: nodejs/changelog-maker#167
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@RafaelGSS
Copy link
Member Author

@rvagg PTAL

@RafaelGSS
Copy link
Member Author

I still can't reproduce the errors on my local environment

@richardlau
Copy link
Member

I still can't reproduce the errors on my local environment

The workflow checks out with fetch-depth 0 so there's no local commit history:

@RafaelGSS
Copy link
Member Author

Oh, so I think merging it should be fine?

@richardlau
Copy link
Member

Oh, so I think merging it should be fine?

No? It will break the workflow.

@rvagg
Copy link
Member

rvagg commented Nov 20, 2024

remove the fetch-depth in the workflow and give it another go, it's not exactly an expensive repo to fetch, I guess it's there as a copy-pasta from another repo where it made sense

@aduh95
Copy link
Contributor

aduh95 commented Nov 20, 2024

The workflow checks out with fetch-depth 0 so there's no local commit history:

fetch-depth: 0 means "all history", not none, see https://github.com/actions/checkout/blob/cbb722410c2e876e24abbe8de2cc27693e501dcb/README.md?plain=1#L7

@rvagg
Copy link
Member

rvagg commented Nov 21, 2024

Screenshot 2024-11-21 at 2 46 05 pm

I think this might be because checkout is adding a merge commit, which maybe we don't want here: https://github.com/actions/checkout/blob/61b9e3751b92087fd0b06925ba6dd6314e06f089/README.md#L194-L201

Could you try adding

  with:
    ref: ${{ github.event.pull_request.head.sha }}

to the checkout in the action and see if that helps?

tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
PR-URL: nodejs#55830
Refs: nodejs/changelog-maker#167
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@aduh95
Copy link
Contributor

aduh95 commented Nov 21, 2024

Could you try adding

  with:
    ref: ${{ github.event.pull_request.head.sha }}

to the checkout in the action and see if that helps?

FWIW you can also do fetch-depth: 2 and HEAD^2 gives you the head commit – fetching directly the head commit would also work, but that means you can get PR far behind, which can lead to tricky failures hard to understand (e.g. the npm install will install older deps)

@RafaelGSS
Copy link
Member Author

Oh nevermind, I found the error. I made a change on commit-stream and I thought it was part of this code. I'll fix it

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Nov 26, 2024
PR-URL: nodejs#55830
Refs: nodejs/changelog-maker#167
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
ruyadorno pushed a commit to nodejs/node that referenced this pull request Nov 27, 2024
PR-URL: #55830
Refs: nodejs/changelog-maker#167
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
ruyadorno pushed a commit to nodejs/node that referenced this pull request Nov 27, 2024
PR-URL: #55830
Refs: nodejs/changelog-maker#167
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
ruyadorno pushed a commit to nodejs/node that referenced this pull request Nov 27, 2024
PR-URL: #55830
Refs: nodejs/changelog-maker#167
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@RafaelGSS RafaelGSS force-pushed the add-cve-id-to-commitoutput branch 2 times, most recently from af314d0 to 3297a29 Compare November 27, 2024 21:49
@RafaelGSS
Copy link
Member Author

Finally! PTAL @nodejs/releasers @rvagg

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

nice

@RafaelGSS
Copy link
Member Author

I was about to merge this, but Create merge commit is disabled on this repository, I think that's a mistake? Otherwise, my test would fail on the main since it relies on commit sha. I'll enable the merge button and press it in a few hours.

@ruyadorno
Copy link
Member

I was about to merge this, but Create merge commit is disabled on this repository, I think that's a mistake? Otherwise, my test would fail on the main since it relies on commit sha. I'll enable the merge button and press it in a few hours.

I've seen many teams choosing to disable it for personal preference in the past. My recommendation is that you enable, land this PR and disable it afterwards. (unless you plan to continue relying on it in the long term)

@RafaelGSS RafaelGSS merged commit f839143 into main Nov 28, 2024
18 checks passed
@RafaelGSS RafaelGSS deleted the add-cve-id-to-commitoutput branch November 28, 2024 14:58
@rvagg
Copy link
Member

rvagg commented Nov 28, 2024

I really would rather not have merge commits; if you must rely on a sha then push to main to merge it yourself and avoid the squash/rebase/merge commit hoopla GitHub forces.

@RafaelGSS
Copy link
Member Author

Alright, I'll do it next time!

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.

5 participants