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: added platform validation #12

Merged
merged 8 commits into from
Mar 21, 2023

Conversation

UlisesGascon
Copy link
Sponsor Contributor

@UlisesGascon UlisesGascon commented Mar 17, 2023

Main changes

  • Added business logic to ensure that the platform is checked against the affectedEnvironments from the vuln database.
  • Added platform input as optional for Github Action
  • Added os.platform() to the CLI by default

Notes

@RafaelGSS I added some additional cognitive points to the getVulnerabilityList function and it is not easy to test with the current files structure. Should I move getVulnerabilityList and getSystemEnvironment to a utility file so I can include proper Unit Tests?

Context

This PR is related to nodejs/security-wg#912, nodejs/security-wg#914 and close #9

@UlisesGascon UlisesGascon marked this pull request as ready for review March 17, 2023 15:00
action.js Outdated Show resolved Hide resolved
Copy link
Owner

@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.

Also, tests will need to be updated as well

action.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@UlisesGascon
Copy link
Sponsor Contributor Author

I added specific tests in b4f87da

Copy link
Owner

@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. I'll wait for the security-wg PRs to merge before releasing this one

Thank you

Copy link
Owner

@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.

Actually, @UlisesGascon, I think we need to receive an option checkEnvironment in the Github Action and set it to false as default to prevent breaking changes. Most of cases the runner environment that will run the action will not be the same as will run in production, so it can lead to false positives.

Can you do it and document it in the README, please?

@UlisesGascon
Copy link
Sponsor Contributor Author

@RafaelGSS I updated the docs in 60fa37b. The argument platform is optional in the Github Action, and it can be specified by the user. Only when the execution is made by npx as executable the platform is provided by default os.platform().

I believe this is not a breaking change at all, so this can be the v1.3.0.

@UlisesGascon
Copy link
Sponsor Contributor Author

UlisesGascon commented Mar 18, 2023

The tests are failing until nodejs/security-wg#916 got merged as the index is using an old version of the database that used [linux, win, macos] values and not the ones compatible with os.platform() as agreed in nodejs/security-wg#914

@RafaelGSS RafaelGSS merged commit 4817449 into RafaelGSS:main Mar 21, 2023
@UlisesGascon UlisesGascon deleted the feat/platform-logic branch March 21, 2023 17:53
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.

Add support to environment check
2 participants