From 22ebf72b6b6af1297b1faab522ea295283586dbf Mon Sep 17 00:00:00 2001 From: Jesse Wierzbinski Date: Wed, 6 Dec 2023 13:34:56 -1000 Subject: [PATCH] Improve OpenID login flow security --- .../migration.sql | 12 +++++++ .../migration.sql | 8 +++++ prisma/schema.prisma | 28 ++++++++------- server/api/oauth/authorize-external/index.ts | 21 +++++++++-- server/api/oauth/callback/[issuer]/index.ts | 36 +++++++++++-------- 5 files changed, 77 insertions(+), 28 deletions(-) create mode 100644 prisma/migrations/20231206231959_add_issuer_and_client_to_flows/migration.sql create mode 100644 prisma/migrations/20231206232131_make_client_id_unique/migration.sql diff --git a/prisma/migrations/20231206231959_add_issuer_and_client_to_flows/migration.sql b/prisma/migrations/20231206231959_add_issuer_and_client_to_flows/migration.sql new file mode 100644 index 00000000..4e0ca8ae --- /dev/null +++ b/prisma/migrations/20231206231959_add_issuer_and_client_to_flows/migration.sql @@ -0,0 +1,12 @@ +/* + Warnings: + + - Added the required column `issuerId` to the `OpenIdLoginFlow` table without a default value. This is not possible if the table is not empty. + +*/ +-- AlterTable +ALTER TABLE "OpenIdLoginFlow" ADD COLUMN "applicationId" UUID, +ADD COLUMN "issuerId" TEXT NOT NULL; + +-- AddForeignKey +ALTER TABLE "OpenIdLoginFlow" ADD CONSTRAINT "OpenIdLoginFlow_applicationId_fkey" FOREIGN KEY ("applicationId") REFERENCES "Application"("id") ON DELETE CASCADE ON UPDATE CASCADE; diff --git a/prisma/migrations/20231206232131_make_client_id_unique/migration.sql b/prisma/migrations/20231206232131_make_client_id_unique/migration.sql new file mode 100644 index 00000000..99632ccb --- /dev/null +++ b/prisma/migrations/20231206232131_make_client_id_unique/migration.sql @@ -0,0 +1,8 @@ +/* + Warnings: + + - A unique constraint covering the columns `[client_id]` on the table `Application` will be added. If there are existing duplicate values, this will fail. + +*/ +-- CreateIndex +CREATE UNIQUE INDEX "Application_client_id_key" ON "Application"("client_id"); diff --git a/prisma/schema.prisma b/prisma/schema.prisma index e5539dfa..77f97f04 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -10,16 +10,17 @@ datasource db { } model Application { - id String @id @default(dbgenerated("uuid_generate_v7()")) @db.Uuid - name String - website String? - vapid_key String? - client_id String - secret String - scopes String - redirect_uris String - statuses Status[] // One to many relation with Status - tokens Token[] // One to many relation with Token + id String @id @default(dbgenerated("uuid_generate_v7()")) @db.Uuid + name String + website String? + vapid_key String? + client_id String @unique + secret String + scopes String + redirect_uris String + statuses Status[] // One to many relation with Status + tokens Token[] // One to many relation with Token + openIdLoginFlows OpenIdLoginFlow[] } model Emoji { @@ -140,8 +141,11 @@ model Token { } model OpenIdLoginFlow { - id String @id @default(dbgenerated("uuid_generate_v7()")) @db.Uuid - codeVerifier String + id String @id @default(dbgenerated("uuid_generate_v7()")) @db.Uuid + codeVerifier String + issuerId String + application Application? @relation(fields: [applicationId], references: [id], onDelete: Cascade) + applicationId String? @db.Uuid } model Attachment { diff --git a/server/api/oauth/authorize-external/index.ts b/server/api/oauth/authorize-external/index.ts index a4562e81..34807566 100644 --- a/server/api/oauth/authorize-external/index.ts +++ b/server/api/oauth/authorize-external/index.ts @@ -5,8 +5,10 @@ import type { MatchedRoute } from "bun"; import { calculatePKCECodeChallenge, discoveryRequest, + generateRandomCodeVerifier, processDiscoveryResponse, } from "oauth4webapi"; +import { client } from "~database/datasource"; export const meta = applyConfig({ allowedMethods: ["GET"], @@ -63,7 +65,22 @@ export default async ( algorithm: "oidc", }).then(res => processDiscoveryResponse(issuerUrl, res)); - const codeVerifier = "tempString"; + const codeVerifier = generateRandomCodeVerifier(); + + // Store into database + + const newFlow = await client.openIdLoginFlow.create({ + data: { + codeVerifier, + application: { + connect: { + client_id: clientId, + }, + }, + issuerId, + }, + }); + const codeChallenge = await calculatePKCECodeChallenge(codeVerifier); return Response.redirect( @@ -72,7 +89,7 @@ export default async ( new URLSearchParams({ client_id: issuer.client_id, redirect_uri: - oauthRedirectUri(issuerId) + `?clientId=${clientId}`, + oauthRedirectUri(issuerId) + `?flow=${newFlow.id}`, response_type: "code", scope: "openid profile email", // PKCE diff --git a/server/api/oauth/callback/[issuer]/index.ts b/server/api/oauth/callback/[issuer]/index.ts index aca0bba2..88280906 100644 --- a/server/api/oauth/callback/[issuer]/index.ts +++ b/server/api/oauth/callback/[issuer]/index.ts @@ -52,8 +52,18 @@ export default async ( // Remove state query parameter from URL currentUrl.searchParams.delete("state"); const issuerParam = matchedRoute.params.issuer; - // This is the Lysand client's client_id, not the external OAuth provider's client_id - const clientId = matchedRoute.query.clientId; + const flow = await client.openIdLoginFlow.findFirst({ + where: { + id: matchedRoute.query.flow, + }, + include: { + application: true, + }, + }); + + if (!flow) { + return redirectToLogin("Invalid flow"); + } const config = getConfig(); @@ -95,8 +105,8 @@ export default async ( client_secret: issuer.client_secret, }, parameters, - oauthRedirectUri(issuerParam) + `?clientId=${clientId}`, - "tempString" + oauthRedirectUri(issuerParam) + `?flow=${flow.id}`, + flow.codeVerifier ); const result = await processAuthorizationCodeOpenIDResponse( @@ -153,24 +163,19 @@ export default async ( return redirectToLogin("No user found with that account"); } - const application = await client.application.findFirst({ - where: { - client_id: clientId, - }, - }); - - if (!application) return redirectToLogin("Invalid client_id"); + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (!flow.application) return redirectToLogin("Invalid client_id"); const code = randomBytes(32).toString("hex"); await client.application.update({ - where: { id: application.id }, + where: { id: flow.application.id }, data: { tokens: { create: { access_token: randomBytes(64).toString("base64url"), code: code, - scope: application.scopes, + scope: flow.application.scopes, token_type: TokenType.BEARER, user: { connect: { @@ -183,5 +188,8 @@ export default async ( }); // Redirect back to application - return Response.redirect(`${application.redirect_uris}?code=${code}`, 302); + return Response.redirect( + `${flow.application.redirect_uris}?code=${code}`, + 302 + ); };