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

fix(security): Bump dep to eliminate ProtobufJS security vulnerability #18917

Closed

Conversation

adamdmharvey
Copy link
Member

@adamdmharvey adamdmharvey commented Aug 2, 2023

Hey, I just made a Pull Request!

This PR includes a manual bump of a transitive dependency which should fix a high security vulnerability reported in the repo.

Interestingly, Dependabot wasn't able to update it, and various combinations of commands in yarn weren't locally either. (for maintainers -> https://github.com/backstage/backstage/security/dependabot/245 )

Related: #18744 (but not a direct fix for)

I ended up just removing the proto3-json-serializer@1.0.0 reference from the Yarn.lock, and a yarn install reinstall found it was missing, and properly installed v1.1.1 (which was within Google GAX's semver compliance). This version has a transitive dependency on the actual vulnerable ProtobufJS package, with the upgrade eliminating ProtobufJS v6 and bumping it to v7, which is the version which patches the CVE. (fixed in proto3-json-serializer v1.0.3: https://github.com/googleapis/proto3-json-serializer-nodejs/releases/tag/v1.0.3 )

# before
google-gax
  --> proto3-json-serializer: ^1.0.0
    v1.0.0 required:
    --> protobufjs: ^6.11.2

# after:
google-gax
  --> proto3-json-serializer: ^1.0.0
    v1.1.0 requires:
    --> protobufjs: ^7.0.0

I ran tests locally against the Backend Auth plugin which included these transitive references for Google usage and all looks good so far. The change itself is actually within semver compliance all through, so should be good!

✔️ Checklist

  • 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)

…rability

Signed-off-by: Adam Harvey <aharvey00@gmail.com>
@adamdmharvey adamdmharvey added dependencies Pull requests that update a dependency file security Pull requests that address a security vulnerability labels Aug 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

Uffizzi Preview deployment-32407 was deleted.

@adamdmharvey
Copy link
Member Author

Wasn't sure how (or if) we needed a changeset for this one; welcome any advice !

@awanlin
Copy link
Collaborator

awanlin commented Aug 3, 2023

Wasn't sure how (or if) we needed a changeset for this one; welcome any advice !

Looking over the ones that get merged by Renovate they do not have a changeset but I wonder if it would be smart to add one anyway just to highlight that it was updated manually? I'm not sure what packages/plugins are impacted by this change though

@adamdmharvey
Copy link
Member Author

adamdmharvey commented Aug 3, 2023

@awanlin Looking over the ones that get merged by Renovate they do not have a changeset but I wonder if it would be smart to add one anyway just to highlight that it was updated manually? I'm not sure what packages/plugins are impacted by this change though

Tracing it back manually, it looks like it fans out to potentially affect these three:

1. @backstage/plugin-kubernetes-backend
2. @backstage/plugin-catalog-backend-module-gcp
3. @backstage/plugin-auth-backend

I could fire a changeset for those as a patch, though technically if you wiped out your yarn.lock, and reinstalled everything, you would have gotten this patch. (there's still a bit of mystery in my mind for why Dependabot and Yarn couldn't figure out how to patch it, TBH) So there's no actual package.json or any such "release" per se in this, since we don't distribute the yarn.lock. (and for integrators, wonder if they have this listed as a vulnerability as well and get hit with it not being able to be patched?)

Welcome any maintainer advice, and/or any thoughts from those who have some ideas re: the non-transitive patching nature!

@adamdmharvey
Copy link
Member Author

Gonna rely on this one instead!

#18928

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file security Pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants