diff --git a/src/server/routes/auth-utils.ts b/src/server/routes/auth-utils.ts new file mode 100644 index 0000000..80dd39b --- /dev/null +++ b/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 +): 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; +} diff --git a/src/server/routes/connection.ts b/src/server/routes/connection.ts index 39e5f06..80e2e6c 100644 --- a/src/server/routes/connection.ts +++ b/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", { - httpStatus: 403, - }) + new AnonymousError( + 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 -// cookie so the web UI is reachable without going through GitHub OAuth. -// Token may come from `Authorization: Bearer …`, `?token=…`, or JSON body. -router.all( +// Accept an API token and establish a session cookie so the web UI is +// reachable without going through GitHub OAuth. Keep credentials out of URLs, +// which are routinely retained in access logs and browser history. +router.post( "/login-token", async function (req: express.Request, res: express.Response) { - const fromHeader = (() => { - 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); + const token = getLoginToken(req); 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) { diff --git a/src/server/routes/route-utils.ts b/src/server/routes/route-utils.ts index c21552a..7da9f60 100644 --- a/src/server/routes/route-utils.ts +++ b/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; - return (repo.model.coauthors || []).some((c) => c.username === user.username); + const githubId = user.model.externalIDs?.github; + 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", { - httpStatus: 403, - }); + throw new AnonymousError( + model.status === "banned" ? "user_banned" : "not_connected", + { + httpStatus: model.status === "banned" ? 403 : 401, + } + ); } return new User(model); } diff --git a/src/server/routes/token-auth.ts b/src/server/routes/token-auth.ts index 29a5520..313d0ae 100644 --- a/src/server/routes/token-auth.ts +++ b/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. diff --git a/src/server/routes/user.ts b/src/server/routes/user.ts index c66ab9d..3bdc75f 100644 --- a/src/server/routes/user.ts +++ b/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. diff --git a/test/auth-utils.test.js b/test/auth-utils.test.js new file mode 100644 index 0000000..1acabca --- /dev/null +++ b/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); + }); +}); diff --git a/test/route-utils-auth.test.js b/test/route-utils-auth.test.js index 0994246..316bc3b 100644 --- a/test/route-utils-auth.test.js +++ b/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" }] });