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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

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

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.

4 participants