Enforce validation of GitHub tokens using types (#1520)

Currently, we have robust GitHub token validation logic. However, at a
type level, usage of this logic is unenforced, so many places in the
codebase don't use validation; most crucially, the `Common.githubToken`
method doesn't, which means that the CLI doesn't validate GitHub tokens.

Instead, `Common.githubToken` currently provides a deceptive signature:

`function githubToken(): string | null`

One might reasonably think that the presence of a string means that
there is a GitHub token, and that you can test `if (token != null)`.
However, a command line user can easily provide an empty string:

`SOURCECRED_GITHUB_TOKEN=null node bin/sourcecred.js load ...`

In this case, the user was trying to unset the GitHub token, but this
actually provides a string-y GitHub token, so at a type level, it looks
like a GitHub token is present.

No more! This commit adds `opaque type GitHubToken: string = string` in
the `github/token.js` module. Since the type is opaque, it only has one
legal constructor: the `validateToken` method in `github/token.js`. The
functions that actually use the token have been updated to require this
type. Therefore, we now enforce at the type level that every usage of a
GitHub token needs to be validated, ensuring that we no longer confuse
empty strings for valid GitHub tokens.

Note that making GitHub token an opaque subtype of string
(`GithubToken: string`) is important because it means that consumers can
still pass or store the token as a string; however, no fresh ones can be
constructed except by the validator.

Test plan: Implementation-wise, this is a simple refactor; `yarn test`
passes.

src/api/load.js

@@ -6,6 +6,7 @@ import path from "path";
 import {TaskReporter} from "../util/taskReporter";
 import {Graph} from "../core/graph";
 import {loadGraph} from "../plugins/github/loadGraph";
+import {type GithubToken} from "../plugins/github/token";
 import {TimelineCred} from "../analysis/timeline/timelineCred";
 import {defaultParams, partialParams} from "../analysis/timeline/params";
 import {type TimelineCredParameters} from "../analysis/timeline/params";
@@ -22,7 +23,7 @@ export type LoadOptions = {|
   +params: ?$Shape<TimelineCredParameters>,
   +plugins: $ReadOnlyArray<PluginDeclaration>,
   +sourcecredDirectory: string,
-  +githubToken: string | null,
+  +githubToken: ?GithubToken,
 |};
 
 /**

src/api/load.test.js

@@ -5,6 +5,7 @@ import tmp from "tmp";
 import path from "path";
 import fs from "fs-extra";
 
+import {validateToken} from "../plugins/github/token";
 import type {Options as LoadGraphOptions} from "../plugins/github/loadGraph";
 import type {Options as LoadDiscourseOptions} from "../plugins/discourse/loadDiscourse";
 import {nodeContractions} from "../plugins/identity/nodeContractions";
@@ -44,6 +45,7 @@ const timelineCredCompute: JestMockFn = (require("../analysis/timeline/timelineC
   .TimelineCred.compute: any);
 
 describe("api/load", () => {
+  const exampleGithubToken = validateToken("0".repeat(40));
   const fakeTimelineCred = deepFreeze({
     toJSON: () => ({is: "fake-timeline-cred"}),
   });
@@ -65,7 +67,6 @@ describe("api/load", () => {
     discourseServer: {serverUrl: discourseServerUrl},
   });
   deepFreeze(project);
-  const githubToken = "EXAMPLE_TOKEN";
   const weights = defaultWeights();
   // Tweaks the weights so that we can ensure we aren't overriding with default weights
   weights.nodeManualWeights.set(NodeAddress.empty, 33);
@@ -77,7 +78,7 @@ describe("api/load", () => {
     const taskReporter = new TestTaskReporter();
     const options: LoadOptions = {
       sourcecredDirectory,
-      githubToken,
+      githubToken: exampleGithubToken,
       params,
       plugins,
       project,
@@ -98,7 +99,7 @@ describe("api/load", () => {
     const cacheDirectory = path.join(sourcecredDirectory, "cache");
     const expectedLoadGraphOptions: LoadGraphOptions = {
       repoIds: project.repoIds,
-      token: githubToken,
+      token: exampleGithubToken,
       cacheDirectory,
     };
     expect(loadGraph).toHaveBeenCalledWith(

src/cli/common.js

@@ -6,8 +6,7 @@ import path from "path";
 import deepFreeze from "deep-freeze";
 import fs from "fs-extra";
 import {type Weights, fromJSON as weightsFromJSON} from "../analysis/weights";
-
-import * as NullUtil from "../util/null";
+import {validateToken, type GithubToken} from "../plugins/github/token";
 
 export type PluginName = "git" | "github";
 
@@ -22,8 +21,12 @@ export function sourcecredDirectory(): string {
   return env != null ? env : defaultSourcecredDirectory();
 }
 
-export function githubToken(): string | null {
-  return NullUtil.orElse(process.env.SOURCECRED_GITHUB_TOKEN, null);
+export function githubToken(): ?GithubToken {
+  const envToken = process.env.SOURCECRED_GITHUB_TOKEN;
+  if (envToken == null || !envToken.length) {
+    return null;
+  }
+  return validateToken(envToken);
 }
 
 export async function loadWeights(path: string): Promise<Weights> {

src/cli/common.test.js

@@ -5,6 +5,7 @@ import tmp from "tmp";
 import fs from "fs-extra";
 import {defaultWeights, toJSON as weightsToJSON} from "../analysis/weights";
 import {NodeAddress} from "../core/graph";
+import {validateToken} from "../plugins/github/token";
 
 import {
   defaultPlugins,
@@ -15,6 +16,7 @@ import {
 } from "./common";
 
 describe("cli/common", () => {
+  const exampleGithubToken = validateToken("0".repeat(40));
   beforeEach(() => {
     jest
       .spyOn(require("os"), "tmpdir")
@@ -51,8 +53,8 @@ describe("cli/common", () => {
 
   describe("githubToken", () => {
     it("uses the environment variable when available", () => {
-      process.env.SOURCECRED_GITHUB_TOKEN = "010101";
-      expect(githubToken()).toEqual("010101");
+      process.env.SOURCECRED_GITHUB_TOKEN = exampleGithubToken;
+      expect(githubToken()).toEqual(exampleGithubToken);
     });
     it("returns `null` if the environment variable is not set", () => {
       delete process.env.SOURCECRED_GITHUB_TOKEN;

src/cli/genProject.js

@@ -16,6 +16,7 @@ import {
 } from "../core/project";
 import {type RepoId} from "../plugins/github/repoId";
 import {specToProject} from "../plugins/github/specToProject";
+import {type GithubToken} from "../plugins/github/token";
 import * as NullUtil from "../util/null";
 
 function usage(print: (string) => void): void {
@@ -131,7 +132,7 @@ export async function createProject(opts: {|
   +projectId: string,
   +githubSpecs: $ReadOnlyArray<string>,
   +discourseUrl: string | null,
-  +githubToken: string | null,
+  +githubToken: ?GithubToken,
 |}): Promise<Project> {
   const {projectId, githubSpecs, discourseUrl, githubToken} = opts;
   let repoIds: RepoId[] = [];

src/cli/load.test.js

@@ -14,12 +14,14 @@ import {defaultParams, partialParams} from "../analysis/timeline/params";
 import {declaration as githubDeclaration} from "../plugins/github/declaration";
 import {createProject} from "../core/project";
 import {makeRepoId, stringToRepoId} from "../plugins/github/repoId";
+import {validateToken} from "../plugins/github/token";
 
 jest.mock("../api/load", () => ({load: jest.fn()}));
 type JestMockFn = $Call<typeof jest.fn>;
 const load: JestMockFn = (require("../api/load").load: any);
 
 describe("cli/load", () => {
+  const exampleGithubToken = validateToken("0".repeat(40));
   beforeEach(() => {
     jest.clearAllMocks();
     // Tests should call `newSourcecredDirectory` directly when they
@@ -28,11 +30,10 @@ describe("cli/load", () => {
     newSourcecredDirectory();
   });
 
-  const fakeGithubToken = "....".replace(/./g, "0123456789");
   function newSourcecredDirectory() {
     const dirname = tmp.dirSync().name;
     process.env.SOURCECRED_DIRECTORY = dirname;
-    process.env.SOURCECRED_GITHUB_TOKEN = fakeGithubToken;
+    process.env.SOURCECRED_GITHUB_TOKEN = exampleGithubToken;
     return dirname;
   }
 
@@ -78,7 +79,7 @@ describe("cli/load", () => {
         params: defaultParams(),
         plugins: [githubDeclaration],
         sourcecredDirectory: Common.sourcecredDirectory(),
-        githubToken: fakeGithubToken,
+        githubToken: exampleGithubToken,
       };
       expect(await invocation).toEqual({
         exitCode: 0,
@@ -101,7 +102,7 @@ describe("cli/load", () => {
         params: defaultParams(),
         plugins: [githubDeclaration],
         sourcecredDirectory: Common.sourcecredDirectory(),
-        githubToken: fakeGithubToken,
+        githubToken: exampleGithubToken,
       });
       expect(await invocation).toEqual({
         exitCode: 0,
@@ -137,7 +138,7 @@ describe("cli/load", () => {
         params: partialParams({weights}),
         plugins: [githubDeclaration],
         sourcecredDirectory: Common.sourcecredDirectory(),
-        githubToken: fakeGithubToken,
+        githubToken: exampleGithubToken,
       };
       expect(await invocation).toEqual({
         exitCode: 0,

src/plugins/github/bin/fetchAndPrintGithubOrg.js

@@ -15,6 +15,7 @@
 import stringify from "json-stable-stringify";
 
 import {fetchGithubOrg} from "../fetchGithubOrg";
+import {validateToken} from "../token";
 
 function parseArgs() {
   const argv = process.argv.slice(2);
@@ -27,11 +28,12 @@ function parseArgs() {
   if (argv.length < 2) {
     fail();
   }
-  const [organization, githubToken, ...rest] = argv;
+  const [organization, unvalidatedGithubToken, ...rest] = argv;
   let pageSize: ?number;
   if (rest.length === 1) {
     pageSize = Number(rest[0]);
   }
+  const githubToken = validateToken(unvalidatedGithubToken);
   const result = {organization, githubToken, pageSize};
   if (rest.length > 1) {
     fail();

src/plugins/github/fetchGithubOrg.js

@@ -3,6 +3,7 @@
 import {type RepoId, makeRepoId} from "./repoId";
 import * as Queries from "../../graphql/queries";
 import {postQuery} from "./fetchGithubRepo";
+import {type GithubToken} from "./token";
 
 export type Organization = {|
   +repos: $ReadOnlyArray<RepoId>,
@@ -19,7 +20,7 @@ const DEFAULT_PAGE_SIZE = 100;
  */
 export async function fetchGithubOrg(
   org: string,
-  token: string,
+  token: GithubToken,
   // Regular clients should leave pageSize at the default 50.
   // Exposed for testing purposes.
   pageSize: ?number

src/plugins/github/loadGraph.js

@@ -6,10 +6,11 @@ import fetchGithubRepo from "./fetchGithubRepo";
 import {RelationalView} from "./relationalView";
 import {type RepoId, repoIdToString} from "./repoId";
 import {Graph} from "../../core/graph";
+import {type GithubToken} from "./token";
 
 export type Options = {|
   +repoIds: $ReadOnlyArray<RepoId>,
-  +token: string,
+  +token: GithubToken,
   +cacheDirectory: string,
 |};
 

src/plugins/github/specToProject.js

@@ -2,6 +2,7 @@
 
 import {type Project, createProject} from "../../core/project";
 import {stringToRepoId, githubOwnerPattern, githubRepoPattern} from "./repoId";
+import {type GithubToken} from "./token";
 import {fetchGithubOrg} from "./fetchGithubOrg";
 
 /**
@@ -22,7 +23,7 @@ import {fetchGithubOrg} from "./fetchGithubOrg";
  */
 export async function specToProject(
   spec: string,
-  token: string
+  token: GithubToken
 ): Promise<Project> {
   const repoSpecMatcher = new RegExp(
     `^${githubOwnerPattern}/${githubRepoPattern}$`

src/plugins/github/specToProject.test.js

@@ -3,12 +3,14 @@
 import {specToProject} from "./specToProject";
 import {stringToRepoId} from "./repoId";
 import {type Project, createProject} from "../../core/project";
+import {validateToken} from "./token";
 jest.mock("./fetchGithubOrg", () => ({fetchGithubOrg: jest.fn()}));
 type JestMockFn = $Call<typeof jest.fn>;
 const fetchGithubOrg: JestMockFn = (require("./fetchGithubOrg")
   .fetchGithubOrg: any);
 
 describe("plugins/github/specToProject", () => {
+  const exampleGithubToken = validateToken("0".repeat(40));
   beforeEach(() => {
     jest.clearAllMocks();
   });
@@ -18,18 +20,20 @@ describe("plugins/github/specToProject", () => {
       id: spec,
       repoIds: [stringToRepoId(spec)],
     });
-    const actual = await specToProject(spec, "FAKE_TOKEN");
+    const actual = await specToProject(spec, exampleGithubToken);
     expect(expected).toEqual(actual);
     expect(fetchGithubOrg).not.toHaveBeenCalled();
   });
   it("works for an owner", async () => {
     const repos = [stringToRepoId("foo/bar"), stringToRepoId("foo/zod")];
     const spec = "@foo";
-    const token = "FAKE_TOKEN";
     const fakeOrg = {name: "foo", repos};
     fetchGithubOrg.mockResolvedValueOnce(fakeOrg);
-    const actual = await specToProject(spec, token);
-    expect(fetchGithubOrg).toHaveBeenCalledWith(fakeOrg.name, token);
+    const actual = await specToProject(spec, exampleGithubToken);
+    expect(fetchGithubOrg).toHaveBeenCalledWith(
+      fakeOrg.name,
+      exampleGithubToken
+    );
     const expected: Project = createProject({
       id: spec,
       repoIds: repos,
@@ -49,7 +53,7 @@ describe("plugins/github/specToProject", () => {
     for (const b of bad) {
       it(`fails for "${b}"`, () => {
         expect.assertions(2);
-        const fail = specToProject(b, "FAKE_TOKEN");
+        const fail = specToProject(b, exampleGithubToken);
         return (
           expect(fail)
             .rejects.toThrow(`invalid spec: ${b}`)

src/plugins/github/token.js

@@ -1,5 +1,7 @@
 // @flow
 
+export opaque type GithubToken: string = string;
+
 /**
  * Validates a token against know formatting.
  * Throws an error if it appears invalid.
@@ -10,10 +12,10 @@
  * Installation access token
  * https://developer.github.com/v3/apps/#create-a-new-installation-token
  */
-export function validateToken(token: string) {
+export function validateToken(token: string): GithubToken {
   const personalAccessTokenRE = /^[A-Fa-f0-9]{40}$/;
   if (personalAccessTokenRE.test(token)) {
-    return;
+    return token;
   }
 
   // We're currently being lenient with installation tokens, since we're not completely
@@ -37,7 +39,7 @@ export function validateToken(token: string) {
       );
     }
 
-    return;
+    return token;
   }
 
   throw new Error(

src/plugins/github/token.test.js

@@ -51,10 +51,10 @@ describe("plugins/github/token", () => {
       const token = "1bfb713d900c4962586ec615260b3902438b1d3c";
 
       // When
-      validateToken(token);
+      const validated = validateToken(token);
 
       // Then
-      // Shouldn't throw.
+      expect(token).toEqual(validated);
     });
 
     it("should accept an installation access token format", () => {
@@ -62,10 +62,10 @@ describe("plugins/github/token", () => {
       const token = "v1.1bfb713d900c49621bfb713d900c49621bfb713d";
 
       // When
-      validateToken(token);
+      const validated = validateToken(token);
 
       // Then
-      // Shouldn't throw.
+      expect(token).toEqual(validated);
     });
 
     it("should warn when installation access token has an unexpected version", () => {
@@ -73,7 +73,7 @@ describe("plugins/github/token", () => {
       const token = "v5.1bfb713d900c49621bfb713d900c49621bfb713d";
 
       // When
-      validateToken(token);
+      const validated = validateToken(token);
 
       // Then
       expect(console.warn).toHaveBeenCalledWith(
@@ -81,6 +81,7 @@ describe("plugins/github/token", () => {
       );
       expect(console.warn).toHaveBeenCalledTimes(1);
       spyWarn().mockReset();
+      expect(token).toEqual(validated);
     });
 
     it("should warn when installation access token has an unexpected length", () => {
@@ -88,7 +89,7 @@ describe("plugins/github/token", () => {
       const token = "v1.1bfb713d900c49621bfb713d900c4962";
 
       // When
-      validateToken(token);
+      const validated = validateToken(token);
 
       // Then
       expect(console.warn).toHaveBeenCalledWith(
@@ -96,6 +97,7 @@ describe("plugins/github/token", () => {
       );
       expect(console.warn).toHaveBeenCalledTimes(1);
       spyWarn().mockReset();
+      expect(token).toEqual(validated);
     });
   });
 });