From d5061735b4ecaff668f41f1ea336d9e25c607cc9 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 28 Oct 2026 02:27:25 -0700 Subject: [PATCH 1/3] fix(github): /status reports PAT liveness, not just presence GET /api/github/status answered configured: true whenever GITHUB_PAT was set, even while every proxied call got 401. Through the 2026-08-04 outage that sent operators hunting for a fault in their own seat. checkPatLiveness asks GitHub (GET /rate_limit, not rate-limited) and reports accepted, rejected or unreachable. Only a 401 counts as rejected. Anything else is live: null, so a flaky upstream never reads as a dead credential. isPatConfigured stays I/O-free because the proxy gates call it on hot paths. A dead PAT is a successful diagnosis: 200 with credentialLive false, not a mapped 502. Written by Forge, a Commonly agent. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: forge Agent-Model: claude/claude-haiku-5-5/default --- .../unit/routes/github.status.test.js | 71 +++++++++++++++++++ .../routes/github.upstreamErrorRoutes.test.js | 5 +- .../githubAppService.patLiveness.test.js | 67 +++++++++++++++++ backend/routes/github.ts | 21 ++++-- backend/services/githubAppService.ts | 36 ++++++++++ 5 files changed, 193 insertions(+), 7 deletions(-) create mode 100644 backend/__tests__/unit/routes/github.status.test.js create mode 100644 backend/__tests__/unit/services/githubAppService.patLiveness.test.js diff --git a/backend/__tests__/unit/routes/github.status.test.js b/backend/__tests__/unit/routes/github.status.test.js new file mode 100644 index 0000000000..5993d80973 --- /dev/null +++ b/backend/__tests__/unit/routes/github.status.test.js @@ -0,0 +1,71 @@ +// GET /api/github/status answers "is the GitHub credential working?", not "is +// one set?". A dead PAT is a successful diagnosis: 200 with credentialLive +// false, never a 5xx that looks like our fault. + +jest.mock('../../../middleware/agentRuntimeAuth', () => jest.fn((req, res, next) => next())); + +jest.mock('../../../middleware/auth', () => jest.fn((req, res, next) => { + req.user = { _id: 'admin-1', role: 'admin' }; + req.userId = 'admin-1'; + next(); +})); + +jest.mock('../../../services/githubAppService', () => ({ + isConfigured: jest.fn(), + isPatConfigured: jest.fn(), + checkPatLiveness: jest.fn(), + generateAppJWT: jest.fn(), +})); + +const express = require('express'); +const request = require('supertest'); +// eslint-disable-next-line import/no-unresolved, import/extensions +const GitHubAppService = require('../../../services/githubAppService'); +// eslint-disable-next-line import/no-unresolved, import/extensions +const router = require('../../../routes/github'); + +const app = express(); +app.use(express.json()); +app.use('/api/github', router); + +describe('GET /api/github/status', () => { + beforeEach(() => { + jest.clearAllMocks(); + GitHubAppService.isConfigured.mockReturnValue(false); + GitHubAppService.isPatConfigured.mockReturnValue(true); + }); + + it('reports a PAT the upstream accepts as live', async () => { + GitHubAppService.checkPatLiveness.mockResolvedValue({ live: true, status: 'accepted', upstreamStatus: 200 }); + const res = await request(app).get('/api/github/status'); + expect(res.status).toBe(200); + expect(res.body).toEqual({ + mode: 'pat', configured: true, credentialLive: true, credentialStatus: 'accepted', upstreamStatus: 200, + }); + }); + + it('reports a set but refused PAT as dead, with a 200 and no 5xx', async () => { + GitHubAppService.checkPatLiveness.mockResolvedValue({ live: false, status: 'rejected', upstreamStatus: 401 }); + const res = await request(app).get('/api/github/status'); + expect(res.status).toBe(200); + expect(res.body.configured).toBe(true); + expect(res.body.credentialLive).toBe(false); + expect(res.body.credentialStatus).toBe('rejected'); + expect(res.body.upstreamStatus).toBe(401); + }); + + it('reports unknown, not dead, when GitHub cannot be reached', async () => { + GitHubAppService.checkPatLiveness.mockResolvedValue({ live: null, status: 'unreachable', upstreamStatus: 503 }); + const res = await request(app).get('/api/github/status'); + expect(res.status).toBe(200); + expect(res.body.credentialLive).toBeNull(); + expect(res.body.credentialStatus).toBe('unreachable'); + }); + + it('keeps the mode none answer when nothing is configured, with no probe', async () => { + GitHubAppService.isPatConfigured.mockReturnValue(false); + const res = await request(app).get('/api/github/status'); + expect(res.body).toEqual({ mode: 'none', configured: false, message: 'Set GitHub App env vars' }); + expect(GitHubAppService.checkPatLiveness).not.toHaveBeenCalled(); + }); +}); diff --git a/backend/__tests__/unit/routes/github.upstreamErrorRoutes.test.js b/backend/__tests__/unit/routes/github.upstreamErrorRoutes.test.js index 55e7034e9a..90682a631a 100644 --- a/backend/__tests__/unit/routes/github.upstreamErrorRoutes.test.js +++ b/backend/__tests__/unit/routes/github.upstreamErrorRoutes.test.js @@ -9,8 +9,9 @@ * reaches the upstream service. The source-level check below counts every * handler that calls GitHubAppService, not merely every mapper call, so an * unmapped proxy route cannot make both sides of the assertion move together. - * `/status` carries the single explicit exemption because it only reads local - * configuration and signs locally; it must keep its honest local 500. + * `/status` carries the single explicit exemption because it is a diagnostic: + * it reports a refused credential as data (200, credentialLive: false) rather + * than mapping it to a 502. Its own tests live in github.status.test.js. */ jest.mock('../../../middleware/agentRuntimeAuth', () => jest.fn((req, res, next) => { diff --git a/backend/__tests__/unit/services/githubAppService.patLiveness.test.js b/backend/__tests__/unit/services/githubAppService.patLiveness.test.js new file mode 100644 index 0000000000..0e384f7bae --- /dev/null +++ b/backend/__tests__/unit/services/githubAppService.patLiveness.test.js @@ -0,0 +1,67 @@ +// PAT liveness: presence (GITHUB_PAT is set) is not the same as the credential +// working. checkPatLiveness asks GitHub, and only a 401 counts as a rejection. +// Anything else is unknown (live: null), so a flaky upstream never reads as a +// dead credential. +const axios = require('axios'); +const GitHubAppService = require('../../../services/githubAppService'); + +describe('GitHubAppService.checkPatLiveness', () => { + const savedPat = process.env.GITHUB_PAT; + let get; + + beforeEach(() => { + process.env.GITHUB_PAT = 'ghp_test_token'; + get = jest.spyOn(axios, 'get'); + }); + + afterEach(() => { + if (savedPat === undefined) delete process.env.GITHUB_PAT; else process.env.GITHUB_PAT = savedPat; + jest.restoreAllMocks(); + }); + + it('reports absent without any network call when no PAT is set', async () => { + delete process.env.GITHUB_PAT; + await expect(GitHubAppService.checkPatLiveness()).resolves.toEqual({ live: false, status: 'absent' }); + expect(get).not.toHaveBeenCalled(); + }); + + it('reports accepted when GitHub answers 200 to the rate-limit probe', async () => { + get.mockResolvedValue({ status: 200, data: {} }); + await expect(GitHubAppService.checkPatLiveness()).resolves.toEqual({ + live: true, status: 'accepted', upstreamStatus: 200, + }); + expect(get).toHaveBeenCalledWith( + 'https://api.github.com/rate_limit', + expect.objectContaining({ headers: expect.objectContaining({ Authorization: 'Bearer ghp_test_token' }) }), + ); + }); + + it('reports rejected on a 401, which is the outage signature', async () => { + get.mockRejectedValue({ message: 'Request failed with status code 401', response: { status: 401 } }); + await expect(GitHubAppService.checkPatLiveness()).resolves.toEqual({ + live: false, status: 'rejected', upstreamStatus: 401, + }); + }); + + it('reports unknown, not dead, on a 5xx', async () => { + get.mockRejectedValue({ message: 'Request failed with status code 503', response: { status: 503 } }); + const result = await GitHubAppService.checkPatLiveness(); + expect(result.live).toBeNull(); + expect(result.status).toBe('unreachable'); + expect(result.upstreamStatus).toBe(503); + }); + + it('reports unknown, not dead, on a 403 (never guess the confident direction)', async () => { + get.mockRejectedValue({ message: 'Request failed with status code 403', response: { status: 403 } }); + const result = await GitHubAppService.checkPatLiveness(); + expect(result.live).toBeNull(); + expect(result.status).toBe('unreachable'); + }); + + it('reports unknown on a network failure with no response at all', async () => { + get.mockRejectedValue({ message: 'socket hang up' }); + await expect(GitHubAppService.checkPatLiveness()).resolves.toEqual({ + live: null, status: 'unreachable', upstreamStatus: undefined, detail: 'socket hang up', + }); + }); +}); diff --git a/backend/routes/github.ts b/backend/routes/github.ts index 2332841bb5..6f4d84f010 100644 --- a/backend/routes/github.ts +++ b/backend/routes/github.ts @@ -141,14 +141,25 @@ const router: ReturnType = express.Router(); // per-user App install of its own, not a share of ours. router.get('/status', auth, async (req: AuthReq, res: Res) => { - // @github-upstream-exempt: this route touches no upstream — it reads env and - // signs a JWT locally — so a throw here really is our fault and 500 is the - // honest answer. The route-contract test enforces this exemption against the - // mapper count; do not add the marker to a networked route. + // @github-upstream-exempt: a diagnostic, not a proxy. It probes GitHub for + // the PAT's liveness and reports the outcome as data, so a dead credential is + // a successful diagnosis (200, credentialLive: false), never a 502. Mapping + // it would make "the credential is dead" indistinguishable from "this + // diagnostic is broken". A throw from our own code still gets the honest 500. + // The route-contract test enforces this exemption against the mapper count. try { if (req.user?.role !== 'admin') return res.status(403).json({ message: 'Admin only' }); if (!GitHubAppService.isConfigured()) { - if (GitHubAppService.isPatConfigured()) return res.json({ mode: 'pat', configured: true }); + if (GitHubAppService.isPatConfigured()) { + const liveness = await GitHubAppService.checkPatLiveness(); + return res.json({ + mode: 'pat', + configured: true, + credentialLive: liveness.live, + credentialStatus: liveness.status, + upstreamStatus: liveness.upstreamStatus, + }); + } return res.json({ mode: 'none', configured: false, message: 'Set GitHub App env vars' }); } const appJWT = GitHubAppService.generateAppJWT(); diff --git a/backend/services/githubAppService.ts b/backend/services/githubAppService.ts index 19d81a30cc..8797261c17 100644 --- a/backend/services/githubAppService.ts +++ b/backend/services/githubAppService.ts @@ -6,6 +6,13 @@ export interface GitHubToken { expiresAt: string | null; } +export interface PatLiveness { + live: boolean | null; + status: 'absent' | 'accepted' | 'rejected' | 'unreachable'; + upstreamStatus?: number; + detail?: string; +} + export interface GitHubIssueLabel { name: string; color?: string; @@ -173,6 +180,35 @@ class GitHubAppService { return { token: process.env.GITHUB_PAT as string, expiresAt: null }; } + /** + * Ask GitHub whether the PAT is accepted. Presence is not liveness: a token + * can be set and still refused (revoked, expired), which is what the + * 2026-08-04 outage looked like from inside the pods. + * + * Kept separate from isPatConfigured, which guards hot request paths and + * must stay free of I/O. GET /rate_limit is documented as not counting + * against the rate limit, so this diagnostic can be called freely. + * + * `live: null` means we could not reach a verdict. Only a 401 is a + * rejection; anything else (403, 5xx, network) is reported as unknown rather + * than guessed, since a false "dead" sends an operator to rotate a good key. + */ + static async checkPatLiveness(): Promise { + if (!this.isPatConfigured()) return { live: false, status: 'absent' }; + try { + await axios.get('https://api.github.com/rate_limit', { + headers: { Authorization: `Bearer ${process.env.GITHUB_PAT}`, Accept: 'application/vnd.github+json' }, + timeout: 5000, + }); + return { live: true, status: 'accepted', upstreamStatus: 200 }; + } catch (err) { + const e = err as { message?: string; response?: { status?: number } }; + const upstreamStatus = e.response?.status; + if (upstreamStatus === 401) return { live: false, status: 'rejected', upstreamStatus }; + return { live: null, status: 'unreachable', upstreamStatus, detail: e.message }; + } + } + // ─── Issues API ────────────────────────────────────────────────────────── /** From d1e8bb474128e7891e74a3a7b09640442d5f6f39 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 28 Oct 2026 03:10:37 -0700 Subject: [PATCH 2/3] test(github): stub the PAT liveness probe in the browser-JWT route test The /status route now probes GitHub. The fake PAT in this test reached api.github.com and got a real 401, so CI depended on GitHub being up. Stub checkPatLiveness and assert the new /status shape. Written by Forge, a Commonly agent. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: forge Agent-Model: claude/claude-haiku-5-5/default --- backend/__tests__/service/githubBrowserJwtAuth.test.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/backend/__tests__/service/githubBrowserJwtAuth.test.js b/backend/__tests__/service/githubBrowserJwtAuth.test.js index cee48d07b7..7497edef41 100644 --- a/backend/__tests__/service/githubBrowserJwtAuth.test.js +++ b/backend/__tests__/service/githubBrowserJwtAuth.test.js @@ -44,6 +44,9 @@ describe('GitHub admin route with a browser JWT', () => { it('admits a real admin browser session and denies a non-admin', async () => { jest.spyOn(GitHubAppService, 'isPatConfigured').mockReturnValue(true); + // Stub the liveness probe: the fake PAT must never reach api.github.com. + jest.spyOn(GitHubAppService, 'checkPatLiveness') + .mockResolvedValue({ live: true, status: 'accepted', upstreamStatus: 200 }); const admin = await createUser('admin'); const member = await createUser('user'); @@ -51,7 +54,9 @@ describe('GitHub admin route with a browser JWT', () => { .get('/api/github/status') .set('Authorization', `Bearer ${generateTestToken(String(admin._id))}`); expect(adminResponse.status).toBe(200); - expect(adminResponse.body).toEqual({ mode: 'pat', configured: true }); + expect(adminResponse.body).toEqual({ + mode: 'pat', configured: true, credentialLive: true, credentialStatus: 'accepted', upstreamStatus: 200, + }); const memberResponse = await request(app) .get('/api/github/status') From f0c24f554dadd6e25fc85505d7d8162a045a44d9 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 28 Oct 2026 03:32:50 -0700 Subject: [PATCH 3/3] test(github): pin the admin gate on /status and the probe timeout /status must refuse a non-admin before any upstream probe runs. Nothing guarded that order, so moving the probe above the role check passed every test. Also pin the 5 s probe timeout, so a hung GitHub cannot hold /status. Written by Forge, a Commonly agent. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: forge Agent-Model: claude/claude-haiku-5-5/default --- .../__tests__/unit/routes/github.status.test.js | 16 ++++++++++++++-- .../githubAppService.patLiveness.test.js | 6 ++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/backend/__tests__/unit/routes/github.status.test.js b/backend/__tests__/unit/routes/github.status.test.js index 5993d80973..0859a1bca4 100644 --- a/backend/__tests__/unit/routes/github.status.test.js +++ b/backend/__tests__/unit/routes/github.status.test.js @@ -4,9 +4,11 @@ jest.mock('../../../middleware/agentRuntimeAuth', () => jest.fn((req, res, next) => next())); +// The role is switched per test, so the non-admin path can be exercised. +let mockRole = 'admin'; jest.mock('../../../middleware/auth', () => jest.fn((req, res, next) => { - req.user = { _id: 'admin-1', role: 'admin' }; - req.userId = 'admin-1'; + req.user = { _id: 'user-1', role: mockRole }; + req.userId = 'user-1'; next(); })); @@ -31,6 +33,7 @@ app.use('/api/github', router); describe('GET /api/github/status', () => { beforeEach(() => { jest.clearAllMocks(); + mockRole = 'admin'; GitHubAppService.isConfigured.mockReturnValue(false); GitHubAppService.isPatConfigured.mockReturnValue(true); }); @@ -62,6 +65,15 @@ describe('GET /api/github/status', () => { expect(res.body.credentialStatus).toBe('unreachable'); }); + it('refuses a non-admin before any upstream probe runs', async () => { + mockRole = 'user'; + GitHubAppService.checkPatLiveness.mockResolvedValue({ live: true, status: 'accepted', upstreamStatus: 200 }); + const res = await request(app).get('/api/github/status'); + expect(res.status).toBe(403); + expect(res.body).toEqual({ message: 'Admin only' }); + expect(GitHubAppService.checkPatLiveness).not.toHaveBeenCalled(); + }); + it('keeps the mode none answer when nothing is configured, with no probe', async () => { GitHubAppService.isPatConfigured.mockReturnValue(false); const res = await request(app).get('/api/github/status'); diff --git a/backend/__tests__/unit/services/githubAppService.patLiveness.test.js b/backend/__tests__/unit/services/githubAppService.patLiveness.test.js index 0e384f7bae..2c6682a5c2 100644 --- a/backend/__tests__/unit/services/githubAppService.patLiveness.test.js +++ b/backend/__tests__/unit/services/githubAppService.patLiveness.test.js @@ -36,6 +36,12 @@ describe('GitHubAppService.checkPatLiveness', () => { ); }); + it('bounds the probe with a 5 second timeout so a hung GitHub cannot hold /status', async () => { + get.mockResolvedValue({ status: 200, data: {} }); + await GitHubAppService.checkPatLiveness(); + expect(get.mock.calls[0][1]).toEqual(expect.objectContaining({ timeout: 5000 })); + }); + it('reports rejected on a 401, which is the outage signature', async () => { get.mockRejectedValue({ message: 'Request failed with status code 401', response: { status: 401 } }); await expect(GitHubAppService.checkPatLiveness()).resolves.toEqual({