From 7458944d3a25b3f68576d629045c6e8b24408d1c Mon Sep 17 00:00:00 2001 From: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com> Date: Sun, 18 Jan 2026 03:45:23 -0500 Subject: [PATCH] fix: prevent OAuth state replay attacks and fix preview script types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Use atomic consumeOAuthState() to delete+return state in one operation, preventing race conditions where parallel requests could both succeed - Make redirectUrl and encryptionKey readonly - Make cleanupExpiredStates private (internal implementation) - Fix eventTypes in preview script mock data (string → array) - Bump prettier to 3.8.0 Co-Authored-By: Claude Opus 4.5 --- README.md | 2 +- bun.lock | 4 ++-- package.json | 2 +- scripts/preview-oauth-pages.ts | 6 ++--- src/services/github-oauth-service.ts | 33 +++++++++++++--------------- 5 files changed, 22 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 34d7356..3760cab 100644 --- a/README.md +++ b/README.md @@ -248,7 +248,7 @@ The bot supports two delivery modes: ### Event Organization - [x] Thread-based event grouping - Group related events (PR + commits + CI) in threads -- [ ] **Dynamic PR anchor updates** - Update anchor message when PR metadata changes (title, state, labels, assignees, reviews, CI) +- [x] **Dynamic PR anchor updates** - Update anchor message when PR metadata changes (title, state, labels, assignees, reviews, CI) - [ ] **Mentions support** - Convert GitHub @mentions to Towns @mentions when users are linked - [ ] **Label filters** - Filter subscriptions by PR/issue labels (`--labels bug,security`) - [ ] Event summaries - Digest multiple events into single update message diff --git a/bun.lock b/bun.lock index 9809231..86e17c4 100644 --- a/bun.lock +++ b/bun.lock @@ -34,7 +34,7 @@ "eslint-plugin-import-x": "^4.16.1", "eslint-plugin-prettier": "^5.5.5", "eslint-plugin-tsdoc": "^0.5.0", - "prettier": "^3.7.4", + "prettier": "^3.8.0", "prettier-plugin-ember-template-tag": "^2.1.2", "typescript": "~5.8.3", "typescript-eslint": "^8.53.0", @@ -1268,7 +1268,7 @@ "prelude-ls": ["prelude-ls@1.2.1", "", {}, "sha512-vkcDPrRZo1QZLbn5RLGPpg/WmIQ65qoWWhcGKf/b5eplkkarX0m9z8ppCat4mlOqUsWpyNuYgO3VRyrYHSzX5g=="], - "prettier": ["prettier@3.7.4", "", { "bin": { "prettier": "bin/prettier.cjs" } }, "sha512-v6UNi1+3hSlVvv8fSaoUbggEM5VErKmmpGA7Pl3HF8V6uKY7rvClBOJlH6yNwQtfTueNkGVpOv/mtWL9L4bgRA=="], + "prettier": ["prettier@3.8.0", "", { "bin": { "prettier": "bin/prettier.cjs" } }, "sha512-yEPsovQfpxYfgWNhCfECjG5AQaO+K3dp6XERmOepyPDVqcJm+bjyCVO3pmU+nAPe0N5dDvekfGezt/EIiRe1TA=="], "prettier-linter-helpers": ["prettier-linter-helpers@1.0.1", "", { "dependencies": { "fast-diff": "^1.1.2" } }, "sha512-SxToR7P8Y2lWmv/kTzVLC1t/GDI2WGjMwNhLLE9qtH8Q13C+aEmuRlzDst4Up4s0Wc8sF2M+J57iB3cMLqftfg=="], diff --git a/package.json b/package.json index 0bfa73b..d3c02d3 100644 --- a/package.json +++ b/package.json @@ -50,7 +50,7 @@ "eslint-plugin-import-x": "^4.16.1", "eslint-plugin-prettier": "^5.5.5", "eslint-plugin-tsdoc": "^0.5.0", - "prettier": "^3.7.4", + "prettier": "^3.8.0", "prettier-plugin-ember-template-tag": "^2.1.2", "typescript": "~5.8.3", "typescript-eslint": "^8.53.0" diff --git a/scripts/preview-oauth-pages.ts b/scripts/preview-oauth-pages.ts index ce2763a..782b924 100644 --- a/scripts/preview-oauth-pages.ts +++ b/scripts/preview-oauth-pages.ts @@ -25,13 +25,13 @@ const mockResults: Record = { success: true, deliveryMode: "webhook", repoFullName: "HereNotThere/bot-github", - eventTypes: "pr,issues,commits,releases", + eventTypes: ["pr", "issues", "commits", "releases"], }, "polling-success": { success: true, deliveryMode: "polling", repoFullName: "HereNotThere/bot-github", - eventTypes: "pr,issues,commits", + eventTypes: ["pr", "issues", "commits"], installUrl: "https://github.com/apps/towns-github-bot-test/installations/new/permissions?target_id=98539902", }, @@ -41,7 +41,7 @@ const mockResults: Record = { installUrl: "https://github.com/apps/towns-github-bot-test/installations/new/permissions?target_id=98539902", repoFullName: "HereNotThere/bot-github", - eventTypes: "pr,issues,commits", + eventTypes: ["pr", "issues", "commits"], error: "Private repository requires GitHub App installation", }, "subscription-error": { diff --git a/src/services/github-oauth-service.ts b/src/services/github-oauth-service.ts index 3cdddd0..689c444 100644 --- a/src/services/github-oauth-service.ts +++ b/src/services/github-oauth-service.ts @@ -67,8 +67,8 @@ export enum TokenStatus { */ export class GitHubOAuthService { private githubApp: GitHubApp; - private redirectUrl: string; - private encryptionKey: Buffer; + private readonly redirectUrl: string; + private readonly encryptionKey: Buffer; /** In-flight refresh promises to prevent concurrent refresh race conditions */ private refreshPromises = new Map>(); @@ -150,21 +150,15 @@ export class GitHubOAuthService { code: string, state: string ): Promise { - // Validate state and get stored data - const [stateData] = await db - .select() - .from(oauthStates) - .where(eq(oauthStates.state, state)) - .limit(1); + // Atomically consume state to prevent race conditions and replay attacks + const stateData = await this.consumeOAuthState(state); if (!stateData) { throw new Error("Invalid or expired state parameter"); } - // Check expiration + // Check expiration (state already deleted, just need to reject) if (new Date() > stateData.expiresAt) { - // Clean up expired state - await this.deleteOAuthState(state); throw new Error("OAuth state expired"); } @@ -222,9 +216,6 @@ export class GitHubOAuthService { set: tokenFields, }); - // Clean up used state - await this.deleteOAuthState(state); - // Validate redirect data from database const actionResult = RedirectActionSchema.safeParse( stateData.redirectAction @@ -390,12 +381,18 @@ export class GitHubOAuthService { } /** - * Delete OAuth state record + * Atomically consume OAuth state record (delete and return) + * Prevents race conditions where two requests could both succeed * * @param state - OAuth state token + * @returns The deleted state record, or undefined if not found */ - private async deleteOAuthState(state: string): Promise { - await db.delete(oauthStates).where(eq(oauthStates.state, state)); + private async consumeOAuthState(state: string) { + const [deleted] = await db + .delete(oauthStates) + .where(eq(oauthStates.state, state)) + .returning(); + return deleted; } /** @@ -534,7 +531,7 @@ export class GitHubOAuthService { * * @returns Number of expired states deleted */ - async cleanupExpiredStates(): Promise { + private async cleanupExpiredStates(): Promise { const now = new Date(); try {