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

CII-Best-Practices for Nodejs: Gold level #956

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

Conversation

UlisesGascon
Copy link
Member

@UlisesGascon UlisesGascon commented Apr 16, 2023

Initiative: #953
Related: #955 and #1087

This pull request contains a dump of the current questions and answers for the Node.js project in OpenSSF Best Practices for Gold Level. The purpose is to review the current answers, update and comment on them until we have a final version, and then update the OpenSSF Best Practices site.

tools/ossf_best_practices/gold_criteria.md Outdated Show resolved Hide resolved
tools/ossf_best_practices/gold_criteria.md Outdated Show resolved Hide resolved
tools/ossf_best_practices/gold_criteria.md Outdated Show resolved Hide resolved
tools/ossf_best_practices/gold_criteria.md Outdated Show resolved Hide resolved
tools/ossf_best_practices/gold_criteria.md Outdated Show resolved Hide resolved
tools/ossf_best_practices/gold_criteria.md Outdated Show resolved Hide resolved
tools/ossf_best_practices/gold_criteria.md Outdated Show resolved Hide resolved
tools/ossf_best_practices/gold_criteria.md Outdated Show resolved Hide resolved
tools/ossf_best_practices/gold_criteria.md Outdated Show resolved Hide resolved
tools/ossf_best_practices/gold_criteria.md Outdated Show resolved Hide resolved
@UlisesGascon UlisesGascon marked this pull request as ready for review August 28, 2023 13:20
@mhdawson
Copy link
Member

@UlisesGascon overall a great pass though. One general suggestion is that we should probably include your comments on why met/unmet into what is landed as part of the PR versus just additional comments in the PR review?

@UlisesGascon
Copy link
Member Author

I will work in add more context to the PR as I did in #1163 and #1162. Then we can re-review it again 👍. The discussions won't be lost even if I close them now because I will include the links.

@UlisesGascon UlisesGascon marked this pull request as draft November 26, 2023 18:24
@UlisesGascon UlisesGascon marked this pull request as ready for review November 26, 2023 18:54
@UlisesGascon UlisesGascon requested a review from ljharb November 26, 2023 18:54
@UlisesGascon
Copy link
Member Author

So the PR is back! Ready for review and feedback @nodejs/security . I added links to the documentation and the previous discussions.

Context:
- [CII Best Practices: Quality](https://github.com/coreinfrastructure/best-practices-badge/blob/main/docs/other.md#upgrade-quality-1)

> The project MUST have FLOSS automated test suite(s) that provide at least 90% statement coverage if there is at least one FLOSS tool that can measure this criterion in the selected language.
Copy link
Member

Choose a reason for hiding this comment

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

Check if we met the 90% percentage.

Copy link
Member

Choose a reason for hiding this comment

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

I've no idea if https://app.codecov.io/gh/nodejs/node is able to show statement coverage (it's showing line coverage).
https://github.com/nodejs/node/actions/workflows/coverage-linux.yml?query=branch%3Amain is reporting 95.5% statement coverage in the most recent run for JS code (via c8, see the "Report JS" twisty) but unfortunately no summary/easily readable numbers for the C++ code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created this issue to follow up with the discussion. #1188

- [CII Best Practices: Test Statement Coverage 90%](https://github.com/coreinfrastructure/best-practices-badge/blob/main/docs/other.md#test_statement_coverage90)
- [Team Discussion](https://github.com/nodejs/security-wg/pull/956#discussion_r1307405014)

> The project MUST have FLOSS automated test suite(s) that provide at least 80% branch coverage if there is at least one FLOSS tool that can measure this criterion in the selected language.
Copy link
Member

Choose a reason for hiding this comment

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

We need to check it

Copy link
Member Author

Choose a reason for hiding this comment

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

I created this issue to follow up with the discussion. #1188


## Secured delivery against man-in-the-middle (MITM) attacks

> The project website, repository (if accessible via the web), and download site (if separate) MUST include key hardening headers with nonpermissive values. (URL required)
Copy link
Member

Choose a reason for hiding this comment

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

We need to understand it

Copy link
Member Author

Choose a reason for hiding this comment

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

I created this issue to follow up with the discussion. #1190

@RafaelGSS
Copy link
Member

RafaelGSS commented Mar 14, 2024

@UlisesGascon could you summarize to us what's missing to conclude this initiative/pr? Just #1190?

@github-actions github-actions bot added the stale label Aug 6, 2024
@github-actions github-actions bot closed this Aug 20, 2024
@ljharb
Copy link
Member

ljharb commented Aug 20, 2024

This seems like something that shouldn't ever go stale?

@ljharb

This comment was marked as resolved.

@anonrig anonrig reopened this Dec 3, 2024
@github-actions github-actions bot removed the stale label Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants