fix: close account authorization gaps (#755)

src/server/routes/auth-utils.ts

@@ -0,0 +1,22 @@
+import * as express from "express";
+
+export function isDisabledAccount(
+  status: string | undefined
+): status is "banned" | "removed" {
+  return status === "banned" || status === "removed";
+}
+
+export function getLoginToken(
+  req: Pick<express.Request, "headers" | "body">
+): string | null {
+  const authorization = req.headers.authorization;
+  if (typeof authorization === "string") {
+    const match = authorization.match(/^Bearer\s+(.+)$/i);
+    if (match) return match[1].trim();
+  }
+
+  if (req.body && typeof req.body.token === "string") {
+    return req.body.token.trim() || null;
+  }
+  return null;
+}

src/server/routes/connection.ts

@@ -13,6 +13,7 @@ import AnonymousError from "../../core/AnonymousError";
 import AnonymizedPullRequestModel from "../../core/model/anonymizedPullRequests/anonymizedPullRequests.model";
 import { hashToken } from "./token-auth";
 import { createLogger, serializeError } from "../../core/logger";
+import { getLoginToken, isDisabledAccount } from "./auth-utils";
 
 const logger = createLogger("auth");
 
@@ -38,6 +39,15 @@ const verify = async (
     const now = new Date();
     user = await UserModel.findOne({ "externalIDs.github": profile.id });
     if (user) {
+      if (isDisabledAccount(user.status)) {
+        done(
+          new AnonymousError(
+            user.status === "banned" ? "user_banned" : "not_connected",
+            { httpStatus: user.status === "banned" ? 403 : 401 }
+          )
+        );
+        return;
+      }
       await UserModel.updateOne(
         { _id: user._id },
         {
@@ -59,6 +69,15 @@ const verify = async (
       // the isAdmin flag.
       user = await UserModel.findOne({ username: profile.username });
       if (user) {
+        if (isDisabledAccount(user.status)) {
+          done(
+            new AnonymousError(
+              user.status === "banned" ? "user_banned" : "not_connected",
+              { httpStatus: user.status === "banned" ? 403 : 401 }
+            )
+          );
+          return;
+        }
         await UserModel.updateOne(
           { _id: user._id },
           {
@@ -92,11 +111,14 @@ const verify = async (
         await user.save();
       }
     }
-    if (user!.status === "banned") {
+    if (isDisabledAccount(user!.status)) {
       done(
-        new AnonymousError("user_banned", {
+        new AnonymousError(
-          httpStatus: 403,
+          user!.status === "banned" ? "user_banned" : "not_connected",
-        })
+          {
+            httpStatus: user!.status === "banned" ? 403 : 401,
+          }
+        )
       );
       return;
     }
@@ -181,22 +203,13 @@ router.get(
   }
 );
 
-// Dev-friendly login: accept an admin API token and establish a session
+// Accept an API token and establish a session cookie so the web UI is
-// cookie so the web UI is reachable without going through GitHub OAuth.
+// reachable without going through GitHub OAuth. Keep credentials out of URLs,
-// Token may come from `Authorization: Bearer …`, `?token=…`, or JSON body.
+// which are routinely retained in access logs and browser history.
-router.all(
+router.post(
   "/login-token",
   async function (req: express.Request, res: express.Response) {
-    const fromHeader = (() => {
+    const token = getLoginToken(req);
-      const h = req.headers["authorization"];
-      if (typeof h !== "string") return null;
-      const m = h.match(/^Bearer\s+(.+)$/i);
-      return m ? m[1].trim() : null;
-    })();
-    const token =
-      fromHeader ||
-      (typeof req.query.token === "string" ? req.query.token : null) ||
-      (req.body && typeof req.body.token === "string" ? req.body.token : null);
     if (!token) {
       return res.status(400).json({ error: "missing_token" });
     }
@@ -205,7 +218,11 @@ router.all(
         "apiTokens.tokenHash": hashToken(token),
       });
       if (!model) return res.status(401).json({ error: "invalid_token" });
-      if (model.status === "banned") return res.status(403).json({ error: "user_banned" });
+      if (isDisabledAccount(model.status)) {
+        return res.status(model.status === "banned" ? 403 : 401).json({
+          error: model.status === "banned" ? "user_banned" : "not_connected",
+        });
+      }
       const synthUser = {
         username: model.username,
         accessToken: model.accessTokens?.github,
@@ -221,7 +238,6 @@ router.all(
           { _id: model._id, "apiTokens.tokenHash": hashToken(token) },
           { $set: { "apiTokens.$.lastUsedAt": new Date() } }
         ).catch(() => undefined);
-        if (req.method === "GET") return res.redirect("/");
         return res.json({ ok: true, username: model.username });
       });
     } catch (err) {

src/server/routes/route-utils.ts

@@ -7,6 +7,7 @@ import Repository from "../../core/Repository";
 import { HTTPError } from "got";
 import { RepositoryStatus } from "../../core/types";
 import { createLogger, serializeError } from "../../core/logger";
+import { isDisabledAccount } from "./auth-utils";
 
 const logger = createLogger("route");
 
@@ -101,8 +102,13 @@ export function isOwnerOrAdmin(authorizedUsers: string[], user: User) {
 }
 
 export function isCoauthor(repo: Repository, user: User): boolean {
-  if (!user.username) return false;
+  const githubId = user.model.externalIDs?.github;
-  return (repo.model.coauthors || []).some((c) => c.username === user.username);
+  return (repo.model.coauthors || []).some((coauthor) => {
+    if (coauthor.githubId) {
+      return Boolean(githubId && coauthor.githubId === githubId);
+    }
+    return Boolean(user.username && coauthor.username === user.username);
+  });
 }
 
 export function isOwnerCoauthorOrAdmin(repo: Repository, user: User) {
@@ -270,15 +276,18 @@ export async function getUser(req: express.Request) {
   if (!model) {
     notConnected();
   }
-  if (model.status === "banned") {
+  if (isDisabledAccount(model.status)) {
     req.logout((error) => {
       if (error) {
         logger.error("logout failed", serializeError(error));
       }
     });
-    throw new AnonymousError("user_banned", {
+    throw new AnonymousError(
-      httpStatus: 403,
+      model.status === "banned" ? "user_banned" : "not_connected",
-    });
+      {
+        httpStatus: model.status === "banned" ? 403 : 401,
+      }
+    );
   }
   return new User(model);
 }

src/server/routes/token-auth.ts

@@ -2,6 +2,7 @@ import * as express from "express";
 import * as crypto from "crypto";
 import UserModel from "../../core/model/users/users.model";
 import { createLogger, serializeError } from "../../core/logger";
+import { isDisabledAccount } from "./auth-utils";
 
 const logger = createLogger("token-auth");
 
@@ -29,7 +30,7 @@ export async function bearerTokenAuth(
   try {
     const model = await UserModel.findOne({ "apiTokens.tokenHash": tokenHash });
     if (!model) return next();
-    if (model.status === "banned") return next();
+    if (isDisabledAccount(model.status)) return next();
 
     // Mirror the shape produced by passport's verify() in connection.ts
     // so existing getUser()/route code works unchanged.

src/server/routes/user.ts

@@ -5,6 +5,8 @@ import { ensureAuthenticated } from "./connection";
 import { handleError, getUser, isOwnerOrAdmin } from "./route-utils";
 import UserModel from "../../core/model/users/users.model";
 import AnonymizedRepositoryModel from "../../core/model/anonymizedRepositories/anonymizedRepositories.model";
+import AnonymizedPullRequestModel from "../../core/model/anonymizedPullRequests/anonymizedPullRequests.model";
+import AnonymizedGistModel from "../../core/model/anonymizedGists/anonymizedGists.model";
 import User from "../../core/User";
 import FileModel from "../../core/model/files/files.model";
 import { isConnected } from "../database";
@@ -170,6 +172,23 @@ router.delete("/", async (req: express.Request, res: express.Response) => {
       if (gist.status !== "removed") await gist.remove();
     }
 
+    // Removal keeps anonymized records as tombstones, so explicitly erase the
+    // duplicated OAuth credentials stored on each source record.
+    await Promise.all([
+      AnonymizedRepositoryModel.updateMany(
+        { owner: user.model._id },
+        { $unset: { "source.accessToken": "" } }
+      ).exec(),
+      AnonymizedPullRequestModel.updateMany(
+        { owner: user.model._id },
+        { $unset: { "source.accessToken": "" } }
+      ).exec(),
+      AnonymizedGistModel.updateMany(
+        { owner: user.model._id },
+        { $unset: { "source.accessToken": "" } }
+      ).exec(),
+    ]);
+
     // Revoke the OAuth grant so the application no longer appears in the
     // user's GitHub authorized applications. Best-effort: the account is
     // scrubbed even if GitHub rejects the revocation.

test/auth-utils.test.js

@@ -0,0 +1,56 @@
+const { expect } = require("chai");
+require("ts-node/register/transpile-only");
+
+const {
+  getLoginToken,
+  isDisabledAccount,
+} = require("../src/server/routes/auth-utils");
+
+describe("auth-utils.getLoginToken", function () {
+  it("accepts a bearer token", function () {
+    expect(
+      getLoginToken({ headers: { authorization: "Bearer secret" }, body: {} })
+    ).to.equal("secret");
+  });
+
+  it("accepts a token in the request body", function () {
+    expect(getLoginToken({ headers: {}, body: { token: "secret" } })).to.equal(
+      "secret"
+    );
+  });
+
+  it("does not inspect query-string credentials", function () {
+    expect(
+      getLoginToken({ headers: {}, body: {}, query: { token: "leaked" } })
+    ).to.equal(null);
+  });
+
+  it("rejects empty body tokens", function () {
+    expect(getLoginToken({ headers: {}, body: { token: "  " } })).to.equal(
+      null
+    );
+  });
+});
+
+describe("login-token route", function () {
+  it("only accepts POST requests", function () {
+    const { router } = require("../src/server/routes/connection");
+    const layer = router.stack.find(
+      (candidate) => candidate.route?.path === "/login-token"
+    );
+    expect(layer).to.not.equal(undefined);
+    expect(layer.route.methods).to.deep.equal({ post: true });
+  });
+});
+
+describe("auth-utils.isDisabledAccount", function () {
+  it("rejects banned and removed accounts", function () {
+    expect(isDisabledAccount("banned")).to.equal(true);
+    expect(isDisabledAccount("removed")).to.equal(true);
+  });
+
+  it("allows active and legacy accounts", function () {
+    expect(isDisabledAccount("active")).to.equal(false);
+    expect(isDisabledAccount(undefined)).to.equal(false);
+  });
+});

test/route-utils-auth.test.js

@@ -19,7 +19,7 @@ const Repository = require("../src/core/Repository").default;
  * model objects without a live DB.
  */
 
-function makeUser({ id, username, isAdmin = false } = {}) {
+function makeUser({ id, username, githubId, isAdmin = false } = {}) {
   const _id = id || new mongoose.Types.ObjectId();
   return new User(
     new UserModel({
@@ -28,6 +28,7 @@ function makeUser({ id, username, isAdmin = false } = {}) {
       username,
       isAdmin,
       accessTokens: { github: "tok" },
+      externalIDs: githubId ? { github: githubId } : {},
     })
   );
 }
@@ -105,6 +106,30 @@ describe("route-utils.isCoauthor", function () {
     expect(isCoauthor(repo, user)).to.equal(false);
   });
 
+  it("uses the immutable GitHub id when a coauthor id is present", function () {
+    const user = makeUser({ username: "renamed", githubId: "123" });
+    const repo = makeRepo({
+      coauthors: [{ username: "old-name", githubId: "123" }],
+    });
+    expect(isCoauthor(repo, user)).to.equal(true);
+  });
+
+  it("does not grant access from a recycled username", function () {
+    const user = makeUser({ username: "alice", githubId: "456" });
+    const repo = makeRepo({
+      coauthors: [{ username: "alice", githubId: "123" }],
+    });
+    expect(isCoauthor(repo, user)).to.equal(false);
+  });
+
+  it("matches by GitHub id even when the user has no username", function () {
+    const user = makeUser({ username: undefined, githubId: "123" });
+    const repo = makeRepo({
+      coauthors: [{ username: "old-name", githubId: "123" }],
+    });
+    expect(isCoauthor(repo, user)).to.equal(true);
+  });
+
   it("matches case-sensitively (alice !== Alice)", function () {
     const user = makeUser({ username: "alice" });
     const repo = makeRepo({ coauthors: [{ username: "Alice" }] });