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(plugin-auth-backend): resolve CVE-2021-39171 #7095

Closed
wants to merge 1 commit into from

Conversation

ellisio
Copy link
Contributor

@ellisio ellisio commented Sep 7, 2021

Hey, I just made a Pull Request!

passport-saml@^2.0.0 installs a version that flags code scans with CVE-2021-39171. This bumps the package to 3.1.0, which includes a resolution for this CVE.

Due to the 2.0.0 -> 3.1.2 upgrade, the auth.saml.cert config parameter is now required.

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@ellisio ellisio requested a review from a team as a code owner September 7, 2021 22:23
@ellisio

This comment has been minimized.

@changeset-bot
Copy link

changeset-bot bot commented Sep 7, 2021

🦋 Changeset detected

Latest commit: 21ce17d

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

This PR includes changesets to release 3 packages
Name Type
@backstage/plugin-auth-backend Minor
@backstage/plugin-config-schema Patch
example-backend 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

@ellisio ellisio force-pushed the feat/CVE-2021-39171 branch 2 times, most recently from 2d22340 to bb715e5 Compare September 7, 2021 23:00
Signed-off-by: Andrew Ellis <awellis89@gmail.com>
cert: config.getOptionalString('cert'),
privateCert: config.getOptionalString('privateKey'),
cert: config.getString('cert'),
privateKey: config.getOptionalString('privateKey'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

privateCert was deprecated, and removed, here.

@freben
Copy link
Member

freben commented Sep 8, 2021

Hi! Thanks for looking at this. Haven't reviewed yet, but see #7015 which addresses the same. Due to the stability level of this package and the breaking change that this implies, it's awaiting merge a little longer.

@benjdlambert
Copy link
Member

@freben I'm wondering if because it's a CVE do we not just merge the fix and get it out sooner rather than later and just accept that we broke the stability index because of a security issue? Not really sure what other projects do here tbf.

@ellisio
Copy link
Contributor Author

ellisio commented Sep 8, 2021

@freben your change is identical. I don't know why I couldn't find it before I did this work ha. Yours has a better change log message too, so I'll close this out in favor of yours.

🍻

@ellisio ellisio closed this Sep 8, 2021
@ellisio ellisio deleted the feat/CVE-2021-39171 branch September 8, 2021 07:11
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

3 participants