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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

patch vulnerabilities #282

Merged
merged 6 commits into from Jun 23, 2023
Merged

patch vulnerabilities #282

merged 6 commits into from Jun 23, 2023

Conversation

mark-omarov
Copy link
Contributor

@mark-omarov mark-omarov commented Apr 12, 2023

Hi 馃憢
The security department didn't like Snyk's report on this action, and I didn't like the idea of making a custom action for the same job, so here we go. We will be using the forked version for now, but you might also want to consider this.

Changes:

  • Moved some dev dependencies from dependencies to devDependencies
  • Forced resolution of vulnerable packages

I've tested these changes on my fork by releasing the changes and going through the PR raised by the action. I might have missed something.

Cheers!

@changeset-bot
Copy link

changeset-bot bot commented Apr 12, 2023

馃 Changeset detected

Latest commit: 3295764

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@changesets/action Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mark-omarov mark-omarov marked this pull request as ready for review April 12, 2023 00:57
@mattlunn
Copy link

mattlunn commented May 2, 2023

Our security team are similarly reluctant to allow us to use changesets in our hosted GitHub Enterprise runners, because of the vulnerabilities found in the Snyk report;

image

Would be great if this PR was merged!

@MrTibbles
Copy link

This PR is also of great interest as we wish to use in our hosted Github enterprise runners but can not due to Snyk report. @Andarist are you able to provide any insights into how the approval process works, i am asking you as you made the most recent commit to the repo! :)

@mark-omarov mark-omarov force-pushed the temp/upstream branch 2 times, most recently from d7cc1fb to 085bff8 Compare May 11, 2023 04:53
@MelSumner
Copy link

馃憢 +1 on this PR as well, this is a blocker for us at work and we'd love to be able to keep using this awesome package. @Andarist is there anything that we can do to help this move forward?

P.S. Thank you to @mark-omarov for doing this work! 馃挭 馃檱

@mark-omarov
Copy link
Contributor Author

There are conflicts needs to be resolved. I'm occupied with other things right now, but I'll try to get to it sometime this weekend.

Thanks everyone.

@mark-omarov
Copy link
Contributor Author

I got a minute to resolve conflicts, did a scan with Dependabot, and added resolutions to patch reported vulnerabilities. As of now, there should be no vulnerabilities.

Sorry to bother @Andarist, but I'd appreciate it if you could take a look.
I tested the latest changes again on the fork, but don't quote me. I might have missed something.

@alex-ju
Copy link

alex-ju commented Jun 13, 2023

hey @Andarist! sorry to keep nudging you on reviewing this, but I would hate for us (and potentially others) to have to fork away from this repo because of security compliance. it would be lovely if you could make some time to review this 馃檹

@mark-omarov
Copy link
Contributor Author

Hi @emmatown , sorry to bother you, in the hope of getting this PR noticed, I figured you might be able to help. If no - apologies for annoyance. 鈽猴笍

package.json Outdated
Comment on lines 58 to 65
"glob-parent": "^5.1.2",
"node-fetch": "^2.6.7",
"minimist": "^1.2.8",
"mixme": "^0.5.2",
"minimatch": "^3.1.1",
"lodash": "^4.17.21",
"trim": "^0.0.3",
"y18n": "^4.0.1"
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a breakdown of what packages depend on some outdated versions of those? Couldn't we update some packages to fix this instead of overriding what is installed using resolutions?

"minimatch": "^3.1.1",
"lodash": "^4.17.21",
"trim": "^0.0.3",
"y18n": "^4.0.1"
Copy link
Member

Choose a reason for hiding this comment

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

This is a dependency of yargs and smartwrap depends on it. It's a bummer because we actually don't depend on any parts of smartwrap that depend on that... the problem is that smartwrap ships both its CLI and a runtime library in a single package.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I also don't quite see that package in the Snyk report.

"mixme": "^0.5.2",
"minimatch": "^3.1.1",
"lodash": "^4.17.21",
"trim": "^0.0.3",
Copy link
Member

Choose a reason for hiding this comment

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

This one comes from remark-parse and to fix it we'd have to update a major of this library. I agree that it's best to do it in a separate PR.

package.json Outdated
"**/@octokit/core": "4.2.0",
"glob-parent": "^5.1.2",
"node-fetch": "^2.6.7",
"minimist": "^1.2.8",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this one is about. I don't see any package to depend on this.

package.json Outdated
"minimist": "^1.2.8",
"mixme": "^0.5.2",
"minimatch": "^3.1.1",
"lodash": "^4.17.21",
Copy link
Member

Choose a reason for hiding this comment

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

after your updates packages don't depend on this at all so this is redundant (likely the same story for minimist)

@Andarist Andarist merged commit eb19e25 into changesets:main Jun 23, 2023
1 check passed
@github-actions github-actions bot mentioned this pull request Jun 23, 2023
@Andarist
Copy link
Member

I tweaked this PR a little bit because I think that some bits were overeager and that some of those "vulnerabilities" went away when some of the packages were updated.

Let's publish this and then recheck with Snyk again.

@mark-omarov
Copy link
Contributor Author

Thanks, @Andarist !
I might have forgotten to rescan after resolving conflicts, some vulnerabilities might have indeed been resolved by then.
I'm gonna ask the security team to rescan next week.

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.

None yet

7 participants