Repository: apollographql/apollo-tooling
Pull request #1988 (2020-06-08 13:52:06)

Prevent cli from sending git credentials
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. 

Few users were affected by this issue, and if your account was affected by this, you will also receive an email informing you.

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](https://www.npmjs.com/package/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.
<!--
  Thanks for filing a pull request on Apollo Tooling!

  Please look at the following checklist to ensure that your PR
  can be accepted quickly:
-->

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: This PR went through internal discussion and reviews, which is why none of that shows up in this PR :) 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3ff5c21e..e4ae3a0f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,7 +3,7 @@
 ## Upcoming
 
 - `apollo`
-  - <First `apollo` related entry goes here>
+  - Prevent cli from sending some git credentials [#1988](https://github.com/apollographql/apollo-tooling/pull/1988)
 - `apollo-codegen-flow`
   - <First `apollo-codegen-flow` related entry goes here>
 - `apollo-codegen-scala`
diff --git a/package-lock.json b/package-lock.json
index 1f14dce1..21db8088 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -4190,6 +4190,7 @@
         "gaze": "1.1.3",
         "git-parse": "1.0.4",
         "git-rev-sync": "2.0.0",
+        "git-url-parse": "^11.1.2",
         "glob": "7.1.5",
         "graphql": "14.0.2 - 14.2.0 || ^14.3.1",
         "graphql-tag": "2.10.3",
@@ -8883,7 +8884,6 @@
       "version": "4.0.1",
       "resolved": "https://registry.npmjs.org/git-up/-/git-up-4.0.1.tgz",
       "integrity": "sha512-LFTZZrBlrCrGCG07/dm1aCjjpL1z9L3+5aEeI9SBhAqSc+kiA9Or1bgZhQFNppJX6h/f5McrvJt1mQXTFm6Qrw==",
-      "dev": true,
       "requires": {
         "is-ssh": "^1.3.0",
         "parse-url": "^5.0.0"
@@ -8893,7 +8893,6 @@
       "version": "11.1.2",
       "resolved": "https://registry.npmjs.org/git-url-parse/-/git-url-parse-11.1.2.tgz",
       "integrity": "sha512-gZeLVGY8QVKMIkckncX+iCq2/L8PlwncvDFKiWkBn9EtCfYDbliRTTp6qzyQ1VMdITUfq7293zDzfpjdiGASSQ==",
-      "dev": true,
       "requires": {
         "git-up": "^4.0.0"
       }
@@ -9857,7 +9856,6 @@
       "version": "1.3.1",
       "resolved": "https://registry.npmjs.org/is-ssh/-/is-ssh-1.3.1.tgz",
       "integrity": "sha512-0eRIASHZt1E68/ixClI8bp2YK2wmBPVWEismTs6M+M099jKgrzl/3E976zIbImSIob48N2/XGe9y7ZiYdImSlg==",
-      "dev": true,
       "requires": {
         "protocols": "^1.1.0"
       }
@@ -15697,7 +15695,6 @@
       "version": "4.0.1",
       "resolved": "https://registry.npmjs.org/parse-path/-/parse-path-4.0.1.tgz",
       "integrity": "sha512-d7yhga0Oc+PwNXDvQ0Jv1BuWkLVPXcAoQ/WREgd6vNNoKYaW52KI+RdOFjI63wjkmps9yUE8VS4veP+AgpQ/hA==",
-      "dev": true,
       "requires": {
         "is-ssh": "^1.3.0",
         "protocols": "^1.4.0"
@@ -15716,7 +15713,6 @@
       "version": "5.0.1",
       "resolved": "https://registry.npmjs.org/parse-url/-/parse-url-5.0.1.tgz",
       "integrity": "sha512-flNUPP27r3vJpROi0/R3/2efgKkyXqnXwyP1KQ2U0SfFRgdizOdWfvrrvJg1LuOoxs7GQhmxJlq23IpQ/BkByg==",
-      "dev": true,
       "requires": {
         "is-ssh": "^1.3.0",
         "normalize-url": "^3.3.0",
@@ -15727,8 +15723,7 @@
         "normalize-url": {
           "version": "3.3.0",
           "resolved": "https://registry.npmjs.org/normalize-url/-/normalize-url-3.3.0.tgz",
-          "integrity": "sha512-U+JJi7duF1o+u2pynbp2zXDW2/PADgC30f0GsHZtRh+HOcXHnw137TrNlyxxRvWW5fjKd3bcLHPxofWuCjaeZg==",
-          "dev": true
+          "integrity": "sha512-U+JJi7duF1o+u2pynbp2zXDW2/PADgC30f0GsHZtRh+HOcXHnw137TrNlyxxRvWW5fjKd3bcLHPxofWuCjaeZg=="
         }
       }
     },
@@ -16109,8 +16104,7 @@
     "protocols": {
       "version": "1.4.7",
       "resolved": "https://registry.npmjs.org/protocols/-/protocols-1.4.7.tgz",
-      "integrity": "sha512-Fx65lf9/YDn3hUX08XUc0J8rSux36rEsyiv21ZGUC1mOyeM3lTRpZLcrm8aAolzS4itwVfm7TAPyxC2E5zd6xg==",
-      "dev": true
+      "integrity": "sha512-Fx65lf9/YDn3hUX08XUc0J8rSux36rEsyiv21ZGUC1mOyeM3lTRpZLcrm8aAolzS4itwVfm7TAPyxC2E5zd6xg=="
     },
     "protoduck": {
       "version": "5.0.1",
diff --git a/packages/apollo/package.json b/packages/apollo/package.json
index 69cf2a0f..fbd517f6 100644
--- a/packages/apollo/package.json
+++ b/packages/apollo/package.json
@@ -54,6 +54,7 @@
     "gaze": "1.1.3",
     "git-parse": "1.0.4",
     "git-rev-sync": "2.0.0",
+    "git-url-parse": "^11.1.2",
     "glob": "7.1.5",
     "graphql": "14.0.2 - 14.2.0 || ^14.3.1",
     "graphql-tag": "2.10.3",
diff --git a/packages/apollo/src/git.ts b/packages/apollo/src/git.ts
index 0afed927..70a1a512 100644
--- a/packages/apollo/src/git.ts
+++ b/packages/apollo/src/git.ts
@@ -6,6 +6,7 @@ import git from "git-rev-sync";
 import pickBy from "lodash.pickby";
 import identity from "lodash.identity";
 import Command from "@oclif/command";
+import gitUrlParse from "git-url-parse";
 
 const findGitRoot = (start?: string | string[]): string | void => {
   start = start || process.cwd();
@@ -23,6 +24,35 @@ const findGitRoot = (start?: string | string[]): string | void => {
   }
 };
 
+/**
+ * remove any username and password info from the
+ * git remote (`git ls-remote --get-url`)
+ *
+ * This can be made more generic in the future, allowing for more options
+ * for git providers. right now, we only support github & bitbucket. other remotes
+ * serve no purpose currently in graph manager.
+ */
+
+export const sanitizeGitRemote = (remote?: string) => {
+  if (!remote) return null;
+  const info = gitUrlParse(remote);
+
+  // we only support github and bitbucket sources
+  const source = info.source.toLowerCase();
+  if (source !== "github.com" && source !== "bitbucket.com") return null;
+
+  if (info.user !== "" && info.user !== "git") {
+    info.user = "REDACTED";
+  }
+
+  // just to make sure that with an unknown `protocol` that stringify doesn't
+  // just print the old, dirty url
+  // https://github.com/IonicaBizau/git-url-parse/blob/0b362b3e3b91a23ae58355fd2160523f0abde5d9/lib/index.js#L216-L217
+  info.href = null;
+
+  return gitUrlParse.stringify(info);
+};
+
 export interface Commit {
   authorName: string | null;
   authorEmail: string | null;
@@ -71,7 +101,7 @@ export const gitInfo = async (
     // The remoteUrl call can fail and throw an error
     // https://github.com/kurttheviking/git-rev-sync-js#gitremoteurl--string
     try {
-      remoteUrl = git.remoteUrl();
+      remoteUrl = sanitizeGitRemote(git.remoteUrl());
     } catch (e) {
       log(["Unable to retrieve remote url, failed with:", e].join("\n\n"));
     }


Test patch:
diff --git a/packages/apollo/src/__tests__/git.test.ts b/packages/apollo/src/__tests__/git.test.ts
index 01e51515..d67a9b3b 100644
--- a/packages/apollo/src/__tests__/git.test.ts
+++ b/packages/apollo/src/__tests__/git.test.ts
@@ -1,4 +1,4 @@
-import { gitInfo } from "../git";
+import { gitInfo, sanitizeGitRemote } from "../git";
 
 describe("Git integration", () => {
   it("Returns commit, branch, message, committer, and remoteUrl", async () => {
@@ -17,3 +17,78 @@ describe("Git integration", () => {
     expect(info.branch).toBeDefined();
   });
 });
+
+describe("strip usernames/passwords from git remotes", () => {
+  it("returns empty for unknown remotes", () => {
+    let clean = sanitizeGitRemote("https://un@gitlab.com/apollographql/test");
+    expect(clean).toBeNull();
+  });
+  it("removes username from remote with only a username present", () => {
+    let clean = sanitizeGitRemote(
+      "https://un@bitbucket.com/apollographql/test"
+    );
+    expect(clean).toEqual("https://REDACTED@bitbucket.com/apollographql/test");
+  });
+  it("does not mind case", () => {
+    let clean = sanitizeGitRemote("https://un@GITHUB.com/apollographql/test");
+    expect(clean).toEqual("https://REDACTED@GITHUB.com/apollographql/test");
+  });
+  it("strips usernames from ssh urls", () => {
+    let clean = sanitizeGitRemote("ssh://un%401@github.com/apollographql/test");
+    expect(clean).toEqual("REDACTED@github.com:apollographql/test");
+  });
+  it("works properly with (allowed) special characters in username/password", () => {
+    let clean = sanitizeGitRemote(
+      "https://un:p%40ssw%3Ard@github.com/apollographql/test"
+    );
+    expect(clean).toEqual("https://REDACTED@github.com/apollographql/test");
+
+    let bbClean = sanitizeGitRemote(
+      "https://un:p%40ssw%3Ard@bitbucket.com/apollographql/test"
+    );
+    expect(bbClean).toEqual(
+      "https://REDACTED@bitbucket.com/apollographql/test"
+    );
+  });
+  it("works with non-url remotes from github with git user ONLY", () => {
+    let clean = sanitizeGitRemote(
+      "git@github.com:apollographql/apollo-tooling.git"
+    );
+    expect(clean).toEqual("git@github.com:apollographql/apollo-tooling.git");
+
+    let clean2 = sanitizeGitRemote(
+      "bob@github.com:apollographql/apollo-tooling.git"
+    );
+    expect(clean2).toEqual(
+      "REDACTED@github.com:apollographql/apollo-tooling.git"
+    );
+  });
+  it("works with non-url remotes from bitbucket with git user ONLY", () => {
+    let clean = sanitizeGitRemote(
+      "git@bitbucket.com:apollographql/apollo-tooling.git"
+    );
+    expect(clean).toEqual("git@bitbucket.com:apollographql/apollo-tooling.git");
+
+    let clean2 = sanitizeGitRemote(
+      "bob@bitbucket.com:apollographql/apollo-tooling.git"
+    );
+    expect(clean2).toEqual(
+      "REDACTED@bitbucket.com:apollographql/apollo-tooling.git"
+    );
+  });
+  it("does not allow non-url remotes from unrecognized providers (not github)", () => {
+    let clean = sanitizeGitRemote(
+      "git@lab.com:apollographql/apollo-tooling.git"
+    );
+    expect(clean).toBeNull();
+  });
+  // TODO maybe fix this in the future?
+  // git-url-parse right now just uses the dirty `href` if the protocol is unknow
+  // https://github.com/IonicaBizau/git-url-parse/blob/master/lib/index.js#L216-L217
+  it("returns null with unknown protocols", () => {
+    let clean = sanitizeGitRemote(
+      "git+http://un:p%40sswrd@github.com/apollographql/test"
+    );
+    expect(clean).toBeNull();
+  });
+});