Skip to content

Prevent cli from sending git credentials#1988

Merged
JakeDawkins merged 2 commits intomasterfrom
jake/fix-git-credentials
Jun 8, 2020
Merged

Prevent cli from sending git credentials#1988
JakeDawkins merged 2 commits intomasterfrom
jake/fix-git-credentials

Conversation

@JakeDawkins
Copy link
Copy Markdown
Contributor

@JakeDawkins JakeDawkins commented Jun 8, 2020

This PR aims to fix a security issue where the CLI would send some users' git credentials to Apollo Graph Manager (AGM).

It's important to note that although this security risk was identified, we do not believe that any personal data was ever leaked through this. This issue was identified internally and has been fully dealt with on the backend before this change.

Once the changed in this PR is released, we recommend all users upgrade to the newest version of the CLI to prevent sending any unnecessary sensitive information.

Issue description

Apollo Graph Manager attempts to link back to a PR or a repository on GitHub or BitBucket from the Graph Manager's history page. This url comes from the CLI, who reports the git remote url. This was coming from running the equivalent of git ls-remote --get-url with the git-rev-sync package.

We realized that this git remote can in certain circumstances (mostly BitBucket setups using a gitlab-ci-token user) contain a git user and/or password.

Prior to realizing this, we weren't taking any effort on the CLI or backend services to sanitize these urls properly, and we were unintentionally storing this information.

The fix for the CLI

This PR aims to sanitize all git urls on the CLI before being sent to the backend. We now replace git usernames/passwords that are found with REDACTED prior to reporting. In addition to that, any URLs with sources that are unsupported by Graph Manager (i.e. not GitHub or BitBucket) are just removed and not reported. These changes have been accompanied by a test suite.

The fix in Apollo Graph Manager

Before releasing this PR publicly, we have already changed the Graph Manager servers to perform the same sanitization on its input, and we have (conservatively) removed all git URLs from our databases and database backups that were sent to us before this server change was deployed. This means that users on older versions of the CLI may still send us git credentials but we will not save them persistently.

TODO:

  • Update CHANGELOG.md* with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.

@JakeDawkins
Copy link
Copy Markdown
Contributor Author

This PR went through internal discussion and reviews, which is why none of that shows up in this PR :)

@JakeDawkins JakeDawkins merged commit 622db6f into master Jun 8, 2020
@JakeDawkins JakeDawkins deleted the jake/fix-git-credentials branch June 8, 2020 14:02
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.

1 participant