From f71b70f1bb498313bc8a821f21b21bcd3505089a Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Tue, 11 Jun 2024 14:45:18 -0400 Subject: [PATCH 1/2] feat: fix middleware race condition when checking auth validity --- src/server/git/router.ts | 4 ++-- src/utils/auth.ts | 17 ++++++++++++----- src/utils/trpc-middleware.ts | 20 ++++++++++++++------ src/utils/trpc-server.ts | 10 +++++++--- 4 files changed, 35 insertions(+), 16 deletions(-) diff --git a/src/server/git/router.ts b/src/server/git/router.ts index 284a46a..609cd1f 100644 --- a/src/server/git/router.ts +++ b/src/server/git/router.ts @@ -1,9 +1,9 @@ -import { procedure, router } from '../../utils/trpc-server' +import { gitProcedure, router } from '../../utils/trpc-server' import { syncReposHandler } from './controller' import { SyncReposSchema } from './schema' const gitRouter = router({ - syncRepos: procedure + syncRepos: gitProcedure .input(SyncReposSchema) .mutation(({ input }) => syncReposHandler({ input })), }) diff --git a/src/utils/auth.ts b/src/utils/auth.ts index 9531fa7..cfb8184 100644 --- a/src/utils/auth.ts +++ b/src/utils/auth.ts @@ -43,12 +43,19 @@ export const checkGitHubAppInstallationAuth = async ( const octokit = personalOctokit(accessToken) - const data = await octokit.rest.repos.get({ - owner: mirrorOrgOwner, - repo: mirrorRepo, - }) + const data = await octokit.rest.repos + .get({ + owner: mirrorOrgOwner, + repo: mirrorRepo, + }) + .catch((error) => { + middlewareLogger.error('Error checking github app installation auth', { + error, + }) + return null + }) - if (!data.data) { + if (!data?.data) { middlewareLogger.error('App does not have access to mirror repo') throw new TRPCError({ code: 'UNAUTHORIZED' }) } diff --git a/src/utils/trpc-middleware.ts b/src/utils/trpc-middleware.ts index 837f9c8..7f42a7d 100644 --- a/src/utils/trpc-middleware.ts +++ b/src/utils/trpc-middleware.ts @@ -1,12 +1,13 @@ +import { TRPCError } from '@trpc/server' import { checkGitHubAppInstallationAuth, checkGitHubAuth } from './auth' import { Middleware } from './trpc-server' -export const verifyAuth: Middleware = async (opts) => { - const { ctx, rawInput, path } = opts +export const verifyGitHubAppAuth: Middleware = async (opts) => { + const { ctx, rawInput } = opts - if (path === 'git.syncRepos') { - // Check app authentication - checkGitHubAppInstallationAuth( + // Check app authentication + try { + await checkGitHubAppInstallationAuth( (rawInput as Record)?.accessToken, (rawInput as Record)?.mirrorOwner, (rawInput as Record)?.mirrorName, @@ -15,10 +16,17 @@ export const verifyAuth: Middleware = async (opts) => { return opts.next({ ctx, }) + } catch (error) { + console.error('Error checking github app installation auth', error) + throw new TRPCError({ code: 'UNAUTHORIZED' }) } +} + +export const verifyAuth: Middleware = async (opts) => { + const { ctx, rawInput } = opts // Verify valid github session - checkGitHubAuth( + await checkGitHubAuth( ctx.session?.user?.accessToken, (rawInput as Record)?.orgId, // Fetch orgId if there is one ) diff --git a/src/utils/trpc-server.ts b/src/utils/trpc-server.ts index cea7e3e..4d25e02 100644 --- a/src/utils/trpc-server.ts +++ b/src/utils/trpc-server.ts @@ -1,9 +1,9 @@ import { initTRPC } from '@trpc/server' import { CreateNextContextOptions } from '@trpc/server/adapters/next' import { getServerSession } from 'next-auth' -import { nextAuthOptions } from '../app/api/auth/lib/nextauth-options' -import { verifyAuth } from '../utils/trpc-middleware' import SuperJSON from 'superjson' +import { nextAuthOptions } from '../app/api/auth/lib/nextauth-options' +import { verifyAuth, verifyGitHubAppAuth } from '../utils/trpc-middleware' export const createContext = async (opts: CreateNextContextOptions) => { const session = await getServerSession(opts.req, opts.res, nextAuthOptions) @@ -23,5 +23,9 @@ export const t = initTRPC.context().create({ // Base router and procedure helpers export const router = t.router +const publicProcedure = t.procedure export type Middleware = Parameters<(typeof t.procedure)['use']>[0] -export const procedure = t.procedure.use(verifyAuth) +// Used for general user access token verification +export const procedure = publicProcedure.use(verifyAuth) +// Used for GitHub App authentication verification (non-user events like 'push') +export const gitProcedure = publicProcedure.use(verifyGitHubAppAuth) From 7165caa55fd3fb60094206d3c46b38ccd37afc82 Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Tue, 11 Jun 2024 14:48:16 -0400 Subject: [PATCH 2/2] fix: remove unneeded try/catch --- src/utils/trpc-middleware.ts | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/utils/trpc-middleware.ts b/src/utils/trpc-middleware.ts index 7f42a7d..f27659b 100644 --- a/src/utils/trpc-middleware.ts +++ b/src/utils/trpc-middleware.ts @@ -1,4 +1,3 @@ -import { TRPCError } from '@trpc/server' import { checkGitHubAppInstallationAuth, checkGitHubAuth } from './auth' import { Middleware } from './trpc-server' @@ -6,20 +5,15 @@ export const verifyGitHubAppAuth: Middleware = async (opts) => { const { ctx, rawInput } = opts // Check app authentication - try { - await checkGitHubAppInstallationAuth( - (rawInput as Record)?.accessToken, - (rawInput as Record)?.mirrorOwner, - (rawInput as Record)?.mirrorName, - ) + await checkGitHubAppInstallationAuth( + (rawInput as Record)?.accessToken, + (rawInput as Record)?.mirrorOwner, + (rawInput as Record)?.mirrorName, + ) - return opts.next({ - ctx, - }) - } catch (error) { - console.error('Error checking github app installation auth', error) - throw new TRPCError({ code: 'UNAUTHORIZED' }) - } + return opts.next({ + ctx, + }) } export const verifyAuth: Middleware = async (opts) => {