From e0e43a19e300afc695b8c6fa76f7fb463a656a06 Mon Sep 17 00:00:00 2001 From: unknown Date: Sun, 6 Sep 2026 16:00:11 +0300 Subject: [PATCH 1/3] fix: close three account-takeover paths found in review **Mass assignment.** Fastify's default AJV strips unknown properties only from schemas that declare additionalProperties: false, and a plain TypeBox object declares nothing - so an unknown key survives validation and reaches the handler. Three handlers spread ...request.body over an identity taken from the session, and the body won: POST /devices could register a phone against another account and receive every one of that account's notifications, and PATCH /articles/:id could rewrite anybody's article, because UpdateArticleUseCase proves ownership by asking whether the supplied userId owns it. AJV now runs with removeAdditional: "all", and the three handlers name their fields instead of spreading over them. **The reuse alarm revoked nothing.** RefreshUseCase runs inside a transaction, and resolveRetry called revokeAllByUserId and then threw - so Prisma rolled the revocation back with the error that reported it. A stolen token that survived the grace window ejected the victim and kept working. The transaction now reports a compromise and the revocation happens outside it. **OAuth state was not bound to a browser.** The state proved that somebody had started a flow, not that this browser had. An attacker could start one with their own account and have a victim's browser finish it, signing the victim into the attacker's account. The state is now also set as a signed, httpOnly cookie and must match before the callback is honoured. Also from the review: rotation no longer alarms on a second consecutive retry (the chain is repointed, the window is not extended); an in-flight idempotency claim expires in two minutes rather than a day, so a crashed request does not block its key; a Play notification that fails after being recorded releases the record so Pub/Sub can redeliver; STRICT rate limits key on the edge address rather than a client-writable X-Forwarded-For; three schedulers destroy their task on close; deletion survives a failed cancellation; a redirect target that already carries a query string is appended to correctly; the Play secret is compared in constant time; the purchase endpoint answers the badge question through the shared helper; and OAUTH_NATIVE_REDIRECT_ALLOWLIST gets the sync: false a conflict resolution had dropped. --- .github/workflows/ci.yml | 4 + render.yaml | 1 + src/app.ts | 16 ++ .../domain/entities/refresh-token.entity.ts | 21 ++ .../repositories/billing-event.repository.ts | 11 + .../use-cases/auth/refresh/refresh.usecase.ts | 221 +++++++++++------- .../register-play-purchase.usecase.ts | 6 +- .../oauth/oauth-state/oauth-state.usecase.ts | 16 +- .../soft-delete/soft-delete-user.usecase.ts | 18 +- src/http/controllers/article.controller.ts | 11 +- src/http/controllers/device.controller.ts | 9 +- src/http/controllers/oauth.controller.ts | 103 +++++++- .../controllers/play-billing.controller.ts | 34 ++- .../plugins/custom/device-purge.plugin.ts | 4 +- .../plugins/custom/report-purge.plugin.ts | 4 +- .../custom/subscription-reconcile.plugin.ts | 4 +- src/http/plugins/di/use-cases.di.ts | 8 +- .../plugins/idempotency/idempotency.plugin.ts | 21 +- src/http/plugins/rate-limit.plugin.ts | 45 +++- .../billing/play/play-notification.service.ts | 28 ++- .../subscription-reconcile.scheduler.ts | 11 +- .../jobs/device/device-purge.scheduler.ts | 11 +- .../jobs/report/report-purge.scheduler.ts | 11 +- .../prisma-billing-event.repository.ts | 9 + tests/e2e/auth/refresh.test.ts | 45 ++-- tests/e2e/oauth/redirect.test.ts | 68 ++++-- tests/e2e/security/mass-assignment.test.ts | 122 ++++++++++ .../use-cases/auth/refresh.usecase.test.ts | 29 +++ .../user/soft-delete-user.usecase.test.ts | 20 ++ tests/unit/http/rate-limit-key.test.ts | 19 +- 30 files changed, 758 insertions(+), 172 deletions(-) create mode 100644 tests/e2e/security/mass-assignment.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 71345ff2..3ff61c73 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,6 +31,10 @@ env: # The app target the OAuth e2e cases start a flow for. Any value works as # long as the tests use the same one; it is never dialled. OAUTH_NATIVE_REDIRECT_ALLOWLIST: tdn://oauth-success + # The rotation grace window is covered by unit tests, which can move a + # clock. Zero here makes the e2e exercise the thing the window sits on top + # of: presenting a retired token revokes every session the user has. + REFRESH_ROTATION_GRACE_SECONDS: 0 DISABLE_RATE_LIMIT: true HUSKY: "0" diff --git a/render.yaml b/render.yaml index 34f13b50..9195939d 100644 --- a/render.yaml +++ b/render.yaml @@ -177,6 +177,7 @@ projects: - key: OAUTH_REDIRECT_ALLOWLIST sync: false - key: OAUTH_NATIVE_REDIRECT_ALLOWLIST + sync: false # Push notifications. PUSH_ENABLED is the switch: with it false - the # default - devices still register and nothing is delivered, so the # feature can ship before there is an Expo project behind it. diff --git a/src/app.ts b/src/app.ts index 4a7c2d34..de0a8af3 100644 --- a/src/app.ts +++ b/src/app.ts @@ -69,6 +69,22 @@ export class App { this.server = Fastify({ allowErrorHandlerOverride: true, + ajv: { + customOptions: { + // Fastify's default is `true`, which only strips unknown + // properties from schemas that say + // `additionalProperties: false` - and a plain TypeBox + // object says nothing, so unknown keys survive validation + // and reach the handler. Anywhere a handler spreads + // `...request.body` over an identity taken from the + // session, that is an account takeover: the body wins. + // + // "all" drops every property the schema did not declare, + // which removes the whole class rather than the three + // places it currently bites. + removeAdditional: "all", + }, + }, logger: isTest ? false : isDevelopment diff --git a/src/core/domain/entities/refresh-token.entity.ts b/src/core/domain/entities/refresh-token.entity.ts index 9b09e8e7..6a8685f6 100644 --- a/src/core/domain/entities/refresh-token.entity.ts +++ b/src/core/domain/entities/refresh-token.entity.ts @@ -162,6 +162,27 @@ export class RefreshToken { return Date.now() - revokedAt.getTime() <= seconds * 1000; } + /** + * Points an already-retired token at a newer successor. + * + * Used when a retry is served: the token the client keeps presenting was + * retired once, and the chain behind it has moved on since. Without this + * it would still point at a successor *we* retired on its behalf, and the + * next retry would read that as a stolen token and revoke every session + * the user has - for a client whose only fault was losing two responses in + * a row. + * + * `revokedAt` is deliberately left alone: the grace window is anchored to + * the first rotation, so repeated retries cannot slide it forward + * indefinitely. + * + * @param replacedById - The successor now standing in its place + */ + public repoint(replacedById: string): void { + this.props.replacedById = replacedById; + this.props.updatedAt = new Date(); + } + /** * Revoke the refresh token * diff --git a/src/core/ports/repositories/billing-event.repository.ts b/src/core/ports/repositories/billing-event.repository.ts index e97820be..731ca477 100644 --- a/src/core/ports/repositories/billing-event.repository.ts +++ b/src/core/ports/repositories/billing-event.repository.ts @@ -36,4 +36,15 @@ export interface IBillingEventRepository { * @returns True when this caller recorded it, false when it was a repeat. */ recordIfNew(event: BillingEventRecord): Promise; + + /** + * Removes a record, so the delivery it stood for can be retried. + * + * The record is written before the work - that is what makes it a claim - + * so work that then fails has to give it back. Without this a provider's + * redelivery would be dismissed as a duplicate and the event lost. + * + * @param id - The provider's identifier for the delivery. + */ + forget(id: string): Promise; } diff --git a/src/core/use-cases/auth/refresh/refresh.usecase.ts b/src/core/use-cases/auth/refresh/refresh.usecase.ts index e926f52a..97be2763 100644 --- a/src/core/use-cases/auth/refresh/refresh.usecase.ts +++ b/src/core/use-cases/auth/refresh/refresh.usecase.ts @@ -9,6 +9,17 @@ import type { TransactionPort } from "@core/ports/services/transaction.port"; import type { RefreshInput } from "./refresh.input"; import type { RefreshOutput } from "./refresh.output"; +/** + * What the transaction concluded. + * + * A compromise is reported rather than raised, because the containment it + * calls for cannot happen inside a transaction that is about to be rolled + * back by the very error that reports it. + */ +type RefreshOutcome = + | { kind: "issued"; tokens: RefreshOutput } + | { kind: "compromised"; userId: string }; + /** * Use case for refreshing authentication tokens. * @@ -21,12 +32,15 @@ export class RefreshUseCase { * * @param transactionService - Service for managing database transactions * @param authTokenService - Service for token operations + * @param refreshTokenRepository - Used outside the transaction, to revoke + * a compromised chain; see the note on {@link execute} * @param refreshRotationGraceSeconds - How long after a rotation a reuse is * still treated as a retry rather than as a stolen token */ constructor( private readonly transactionService: TransactionPort, private readonly authTokenService: AuthTokenPort, + private readonly refreshTokenRepository: IRefreshTokenRepository, private readonly refreshRotationGraceSeconds: number, ) {} @@ -44,80 +58,122 @@ export class RefreshUseCase { * and generates new access and refresh tokens within a database transaction. */ async execute(input: RefreshInput): Promise { - return await this.transactionService.runInTransaction(async (ctx) => { - const incomingTokenHash = this.authTokenService.hashRefreshSecret( - input.token, - ); - const currentToken = - await ctx.refreshTokenRepository.findByTokenHash( - incomingTokenHash, + const outcome = await this.transactionService.runInTransaction( + async (ctx): Promise => { + const incomingTokenHash = + this.authTokenService.hashRefreshSecret(input.token); + const currentToken = + await ctx.refreshTokenRepository.findByTokenHash( + incomingTokenHash, + ); + + if (!currentToken) { + throw new UnauthorizedError("Session not found"); + } + + // A retired token is normally the alarm. It is a retry only when + // it was retired moments ago and its successor is untouched. + let tokenToRetire = currentToken; + + if (currentToken.isRevoked) { + const successor = await this.resolveRetry( + ctx.refreshTokenRepository, + currentToken, + ); + + // Reported, not acted on. Revoking here and then throwing + // would roll the revocation back with the transaction: the + // client would be told every session was killed while the + // stolen one kept working. The caller does it outside. + if (!successor) + return { + kind: "compromised" as const, + userId: currentToken.userId, + }; + + tokenToRetire = successor; + } + + if (tokenToRetire.isExpired()) { + throw new UnauthorizedError("Session expired"); + } + + const user = await ctx.userRepository.findById( + tokenToRetire.userId, ); + if (user?.isBanned()) { + throw new AccountBannedError(); + } + + if (!user || user.isDeleted()) { + throw new UnauthorizedError("User account unavailable"); + } + + const payload: UserPayload = { + id: user.id, + username: user.username, + }; + + const { + accessToken, + expiresAt, + refreshToken, + refreshTokenExpiresAt, + } = this.authTokenService.generate(payload); + + const refreshTokenHash = + this.authTokenService.hashRefreshSecret(refreshToken); + + const issued = await ctx.refreshTokenRepository.create({ + tokenHash: refreshTokenHash, + userId: user.id, + deviceIp: input.deviceIp, + userAgent: input.userAgent, + expiresAt: new Date(refreshTokenExpiresAt * 1000), + }); + + // Retired after the successor exists, so the chain always points + // somewhere: a retry that arrives between these two writes finds + // the token still live and is served by the ordinary path. + tokenToRetire.revoke(issued.id); + await ctx.refreshTokenRepository.update(tokenToRetire); + + // A retry was served, so the token the client is still holding now + // points at a successor this call has just retired. Left as it + // was, the *next* retry would find that successor invalid and read + // it as a stolen token - revoking every session the user has, + // because the client lost two responses in a row rather than one. + // The window itself does not move: `repoint` leaves `revokedAt` + // where the first rotation put it. + if (tokenToRetire !== currentToken) { + currentToken.repoint(issued.id); + await ctx.refreshTokenRepository.update(currentToken); + } + + return { + kind: "issued" as const, + tokens: { + accessToken, + expiresAt, + refreshToken, + refreshTokenExpiresAt, + user: payload, + }, + }; + }, + ); + + if (outcome.kind === "compromised") { + // Outside the transaction, on its own connection, so it survives + // the error that follows it. + await this.refreshTokenRepository.revokeAllByUserId(outcome.userId); - if (!currentToken) { - throw new UnauthorizedError("Session not found"); - } - - // A retired token is normally the alarm. It is a retry only when - // it was retired moments ago and its successor is untouched. - const tokenToRetire = currentToken.isRevoked - ? await this.resolveRetry( - ctx.refreshTokenRepository, - currentToken, - ) - : currentToken; - - if (tokenToRetire.isExpired()) { - throw new UnauthorizedError("Session expired"); - } - - const user = await ctx.userRepository.findById( - tokenToRetire.userId, + throw new UnauthorizedError( + "Security alert: Session compromised. All sessions revoked.", ); - if (user?.isBanned()) { - throw new AccountBannedError(); - } - - if (!user || user.isDeleted()) { - throw new UnauthorizedError("User account unavailable"); - } - - const payload: UserPayload = { - id: user.id, - username: user.username, - }; - - const { - accessToken, - expiresAt, - refreshToken, - refreshTokenExpiresAt, - } = this.authTokenService.generate(payload); - - const refreshTokenHash = - this.authTokenService.hashRefreshSecret(refreshToken); - - const issued = await ctx.refreshTokenRepository.create({ - tokenHash: refreshTokenHash, - userId: user.id, - deviceIp: input.deviceIp, - userAgent: input.userAgent, - expiresAt: new Date(refreshTokenExpiresAt * 1000), - }); - - // Retired after the successor exists, so the chain always points - // somewhere: a retry that arrives between these two writes finds - // the token still live and is served by the ordinary path. - tokenToRetire.revoke(issued.id); - await ctx.refreshTokenRepository.update(tokenToRetire); - - return { - accessToken, - expiresAt, - refreshToken, - refreshTokenExpiresAt, - user: payload, - }; - }); + } + + return outcome.tokens; } /** @@ -145,17 +201,20 @@ export class RefreshUseCase { * presents a token retired outside the window, which trips it and ejects * both sessions. * + * Answers rather than throwing, and revokes nothing itself. The whole + * caller runs inside a transaction, so a revocation followed by an error + * is a revocation that never happened - the client would be told every + * session was killed while the stolen one kept working. + * * @param repository - Repository used to follow the rotation chain * @param retired - The retired token that was presented - * @returns The successor, which the caller retires in its place - * - * @throws UnauthorizedError - Always, when this is a genuine reuse; every - * session the user has is revoked first + * @returns The successor to retire in its place, or null when this is a + * genuine reuse and the chain has to be destroyed */ private async resolveRetry( repository: IRefreshTokenRepository, retired: RefreshToken, - ): Promise { + ): Promise { const successor = retired.replacedById ? await repository.findById(retired.replacedById) : null; @@ -165,14 +224,6 @@ export class RefreshUseCase { successor !== null && successor.isValid(); - if (!isRetry) { - await repository.revokeAllByUserId(retired.userId); - - throw new UnauthorizedError( - "Security alert: Session compromised. All sessions revoked.", - ); - } - - return successor; + return isRetry ? successor : null; } } diff --git a/src/core/use-cases/billing/register-play-purchase/register-play-purchase.usecase.ts b/src/core/use-cases/billing/register-play-purchase/register-play-purchase.usecase.ts index 89f070e0..33032512 100644 --- a/src/core/use-cases/billing/register-play-purchase/register-play-purchase.usecase.ts +++ b/src/core/use-cases/billing/register-play-purchase/register-play-purchase.usecase.ts @@ -2,6 +2,7 @@ import { BillingProvider, SubscriptionStatus } from "@core/domain/enums"; import type { BillingPort } from "@core/ports/services/billing.port"; import type { LoggerPort } from "@core/ports/services/logger.port"; import type { SyncSubscriptionUseCase } from "../sync-subscription"; +import { isVerified } from "@core/use-cases/shared/verification/is-verified"; /** * Input DTO for the RegisterPlayPurchaseUseCase. @@ -83,6 +84,9 @@ export class RegisterPlayPurchaseUseCase { }, }); - return { isVerified: result.verifiedUntil !== null }; + // Through the shared helper, not `!== null`. An expiry in the past is + // not a badge, and this is the one place that could otherwise answer + // the badge question differently from every profile and post. + return { isVerified: isVerified(result.verifiedUntil) }; } } diff --git a/src/core/use-cases/oauth/oauth-state/oauth-state.usecase.ts b/src/core/use-cases/oauth/oauth-state/oauth-state.usecase.ts index bf407a01..28e29885 100644 --- a/src/core/use-cases/oauth/oauth-state/oauth-state.usecase.ts +++ b/src/core/use-cases/oauth/oauth-state/oauth-state.usecase.ts @@ -61,14 +61,20 @@ export class BeginOAuthUseCase { * * @param provider - Which provider to start with * @param requestedRedirect - Where the caller wants to be returned to - * @returns The authorization URL to redirect to + * @returns The authorization URL, and the state to bind to this browser * * @throws BadRequestError - When the requested redirect is not allow-listed + * + * @remarks + * The state is returned as well as stored because holding it in the cache + * only proves that *somebody* started a flow here. Proving that the + * browser finishing one is the browser that started it takes a second + * copy, in a cookie, and that is the caller's job. */ async execute( provider: OAuthProvider, requestedRedirect?: string, - ): Promise<{ authorizationUrl: string }> { + ): Promise<{ authorizationUrl: string; state: string }> { const target = resolveRedirectTarget( requestedRedirect, this.oauthRedirectConfig, @@ -94,7 +100,7 @@ export class BeginOAuthUseCase { ? this.githubAuthService.getAuthorizationUrl(state) : this.googleAuthService.getAuthorizationUrl(state); - return { authorizationUrl }; + return { authorizationUrl, state }; } } @@ -119,6 +125,10 @@ export class ConsumeOAuthStateUseCase { * Single use: the value is deleted before it is trusted, so a callback URL * that is replayed - or handed to somebody else - finds nothing. * + * The caller must have already checked that the browser presenting this + * callback is the one that started the flow. A state that exists in the + * cache proves only that some browser did. + * * A callback with no usable state is not treated as fatal. It is answered * on the default web target with an error, because the alternative is a * blank page: this runs in a browser being redirected back from a provider, diff --git a/src/core/use-cases/user/soft-delete/soft-delete-user.usecase.ts b/src/core/use-cases/user/soft-delete/soft-delete-user.usecase.ts index b6b15afe..860bfe8d 100644 --- a/src/core/use-cases/user/soft-delete/soft-delete-user.usecase.ts +++ b/src/core/use-cases/user/soft-delete/soft-delete-user.usecase.ts @@ -4,6 +4,7 @@ import type { EmailPort } from "@core/ports/services/email.port"; import type { PasswordPort } from "@core/ports/services/password.port"; import type { IUserRepository } from "@core/ports/repositories/user.repository"; import type { RevokeSubscriptionUseCase } from "@core/use-cases/billing/revoke-subscription"; +import type { LoggerPort } from "@core/ports/services/logger.port"; import type { SoftDeleteUserUseCaseInput } from "./soft-delete-user-usecase.input"; /** @@ -21,12 +22,14 @@ export class SoftDeleteUserUseCase { * @param passwordService - Service for password verification * @param emailService - Service for sending emails * @param revokeSubscriptionUseCase - Stops the account being charged + * @param logger - Records a cancellation that could not be completed */ constructor( private readonly userRepository: IUserRepository, private readonly passwordService: PasswordPort, private readonly emailService: EmailPort, private readonly revokeSubscriptionUseCase: RevokeSubscriptionUseCase, + private readonly logger: LoggerPort, ) {} /** @@ -64,9 +67,18 @@ export class SoftDeleteUserUseCase { // the provider immediately rather than at the end of the period, // because the account is going away either way. // - // The nightly reconcile retries anything the provider refused, which - // is why a failure here does not have to stop the deletion. - await this.revokeSubscriptionUseCase.execute(input.id); + // Caught, though. The account is already soft-deleted by this point, + // so letting this throw would report a 500 for a deletion that in fact + // happened, and skip the confirmation email as well. The nightly + // reconcile retries anything left behind. + try { + await this.revokeSubscriptionUseCase.execute(input.id); + } catch (error: unknown) { + this.logger.error( + { err: error, userId: input.id }, + "Failed to cancel the subscription of a deleted account", + ); + } await this.emailService.sendDeleteUserEmail({ to: user.email, diff --git a/src/http/controllers/article.controller.ts b/src/http/controllers/article.controller.ts index ebc35703..15e3a765 100644 --- a/src/http/controllers/article.controller.ts +++ b/src/http/controllers/article.controller.ts @@ -74,9 +74,12 @@ export class ArticleController { ): Promise { const authorId = request.user.id; + // The identity goes last. Spreading a request body over it lets the + // body name its own author, and a validator that happens to strip + // unknown keys is not a guarantee this code should lean on. const article = await this.createArticleUseCase.execute({ - authorId, ...request.body, + authorId, }); return reply.status(201).send({ @@ -105,10 +108,14 @@ export class ArticleController { ): Promise { const userId = request.user.id; + // Identity last, for the reason given in `create` - and here it is + // sharper: `UpdateArticleUseCase` proves ownership by asking whether + // `userId` owns the article, so a body that supplies its own `userId` + // does not fail that check, it satisfies it. const article = await this.updateArticleUseCase.execute({ + ...request.body, articleId: request.params.id, userId, - ...request.body, }); return reply.status(200).send({ diff --git a/src/http/controllers/device.controller.ts b/src/http/controllers/device.controller.ts index fda62729..6f066a92 100644 --- a/src/http/controllers/device.controller.ts +++ b/src/http/controllers/device.controller.ts @@ -31,9 +31,16 @@ export class DeviceController { request: FastifyRequest<{ Body: RegisterDeviceBody }>, reply: FastifyReply, ): Promise { + // Fields named one by one rather than spread. The account comes from + // the session and must not be something a request body can set, and + // an ordering that depends on the validator stripping unknown keys is + // one refactor away from not being true. await this.registerDeviceUseCase.execute({ currentUserId: request.user!.id, - ...request.body, + token: request.body.token, + platform: request.body.platform, + appVersion: request.body.appVersion, + locale: request.body.locale, }); reply.status(200).send({ diff --git a/src/http/controllers/oauth.controller.ts b/src/http/controllers/oauth.controller.ts index ccb00941..6a8e12e2 100644 --- a/src/http/controllers/oauth.controller.ts +++ b/src/http/controllers/oauth.controller.ts @@ -18,6 +18,40 @@ import type { OAuthStartQuery } from "@typings/schemas/oauth/oauth-start.schema" /** What a provider hands back on the callback. */ type CallbackQuery = { code?: string; error?: string; state?: string }; +/** + * Cookie holding the state of the flow this browser started. + * + * The other half of the state check. What is kept in the cache proves that + * somebody started a flow on this deployment; this proves that the browser + * presenting the callback is that somebody. Without it an attacker can start a + * flow with their own account, hand the resulting callback URL to a victim, + * and have the victim's browser finish it - leaving them signed in as the + * attacker, typing into an account somebody else can read. + */ +const STATE_COOKIE = "oauthState"; + +/** + * Appends a query parameter, whether or not the target already has some. + * + * An allow-listed target may perfectly reasonably carry its own query string - + * `https://app.example/cb?tenant=x` - and gluing `?code=…` onto that produces + * an address no client can parse the code out of. + * + * @param url - The target + * @param key - Parameter name + * @param value - Parameter value, already encoded + * @returns The target with the parameter appended + */ +function appendParam(url: string, key: string, value: string): string { + return `${url}${url.includes("?") ? "&" : "?"}${key}=${value}`; +} + +/** Scoped to the OAuth routes; nothing else has any use for it. */ +const STATE_COOKIE_PATH = "/api/v1/oauth"; + +/** Matches the cache entry the state lives in. */ +const STATE_COOKIE_MAX_AGE_SECONDS = 600; + export class OAuthController extends BaseAuthController { constructor( private readonly githubAuthService: GithubAuthPort, @@ -135,10 +169,23 @@ export class OAuthController extends BaseAuthController { request: FastifyRequest<{ Querystring: OAuthStartQuery }>, reply: FastifyReply, ): Promise { - const { authorizationUrl } = await this.beginOAuthUseCase.execute( - provider, - request.query.redirect, - ); + const { authorizationUrl, state } = + await this.beginOAuthUseCase.execute( + provider, + request.query.redirect, + ); + + // `lax` rather than `strict`: the callback arrives as a top-level + // navigation from the provider, which `strict` would strip the cookie + // from - and then every legitimate flow would fail the check. + reply.setCookie(STATE_COOKIE, state, { + path: STATE_COOKIE_PATH, + httpOnly: true, + secure: this.isProduction, + sameSite: "lax", + maxAge: STATE_COOKIE_MAX_AGE_SECONDS, + signed: true, + }); reply.redirect(authorizationUrl); } @@ -162,7 +209,19 @@ export class OAuthController extends BaseAuthController { ): Promise { const { code, error, state } = request.query; - const target = await this.consumeOAuthStateUseCase.execute(state); + const expectedState = this.readStateCookie(request); + + reply.clearCookie(STATE_COOKIE, { path: STATE_COOKIE_PATH }); + + // Checked before the cache is even consulted. A state that exists + // there proves a flow was started; only the cookie proves it was + // started by the browser standing here. + const bound = + Boolean(state) && Boolean(expectedState) && state === expectedState; + + const target = bound + ? await this.consumeOAuthStateUseCase.execute(state) + : null; // A callback with no usable state is a callback that cannot be tied to // a flow anybody started here: a replay, an expired attempt, or a link @@ -196,12 +255,24 @@ export class OAuthController extends BaseAuthController { })); reply.redirect( - `${target.successUrl}?code=${encodeURIComponent(exchangeCode)}`, + appendParam( + target.successUrl, + "code", + encodeURIComponent(exchangeCode), + ), ); } catch (err: unknown) { if (err instanceof AccountPendingDeletionError) { return reply.redirect( - `${target.successUrl}?error=account_pending_deletion&recoveryToken=${encodeURIComponent(err.recoveryToken)}`, + appendParam( + appendParam( + target.successUrl, + "error", + "account_pending_deletion", + ), + "recoveryToken", + encodeURIComponent(err.recoveryToken), + ), ); } @@ -209,6 +280,22 @@ export class OAuthController extends BaseAuthController { } } + /** + * Reads the state this browser was given when it started a flow. + * + * @param request - The callback request + * @returns The state, or null when the cookie is absent or unsigned + */ + private readStateCookie(request: FastifyRequest): string | null { + const raw = request.cookies[STATE_COOKIE]; + + if (!raw) return null; + + const unsigned = request.unsignCookie(raw); + + return unsigned.valid && unsigned.value ? unsigned.value : null; + } + /** * Reports a failure on the target the flow was started for. * @@ -222,7 +309,7 @@ export class OAuthController extends BaseAuthController { reason: string, ): void { reply.redirect( - `${target.errorUrl}?error=${encodeURIComponent(reason)}`, + appendParam(target.errorUrl, "error", encodeURIComponent(reason)), ); } } diff --git a/src/http/controllers/play-billing.controller.ts b/src/http/controllers/play-billing.controller.ts index bf04e354..603fe12e 100644 --- a/src/http/controllers/play-billing.controller.ts +++ b/src/http/controllers/play-billing.controller.ts @@ -1,5 +1,6 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; 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 { @@ -7,6 +8,37 @@ import type { RegisterPlayPurchaseBody, } from "@typings/schemas/billing/play.schema"; +/** + * Compares a presented secret with the configured one in constant time. + * + * The difference is small here - an attacker would be timing a query string + * over the internet - but the comparison is cheap to get right and the + * endpoint it guards writes billing state. + * + * Note the remaining weakness this does not address: the secret travels in the + * query string, so it is written into access logs by this service and by + * anything in front of it. Pub/Sub push cannot send custom headers, so the + * proper fix is the OIDC token Google can attach instead, which arrives with + * the rest of the Play integration. + * + * @param presented - What the caller sent, if anything + * @param expected - The configured secret + * @returns True when they match + */ +function matchesSecret( + presented: string | undefined, + expected: string, +): boolean { + if (!presented) return false; + + const a = Buffer.from(presented); + const b = Buffer.from(expected); + + // `timingSafeEqual` throws on a length mismatch, which would itself leak + // the length; the check is done first and the comparison still runs. + return a.length === b.length && timingSafeEqual(a, b); +} + /** * Controller for the Google Play billing endpoints. */ @@ -73,7 +105,7 @@ export class PlayBillingController { // 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 || request.query.token !== expected) { + if (!expected || !matchesSecret(request.query.token, expected)) { throw new UnauthorizedError(); } diff --git a/src/http/plugins/custom/device-purge.plugin.ts b/src/http/plugins/custom/device-purge.plugin.ts index 61055019..0718ba18 100644 --- a/src/http/plugins/custom/device-purge.plugin.ts +++ b/src/http/plugins/custom/device-purge.plugin.ts @@ -22,8 +22,8 @@ function devicePurgePlugin(fastify: FastifyInstance): void { ); }); - fastify.addHook("onClose", () => { - devicePurgeScheduler.stop(); + fastify.addHook("onClose", async () => { + await devicePurgeScheduler.stop(); fastify.log.info( { diff --git a/src/http/plugins/custom/report-purge.plugin.ts b/src/http/plugins/custom/report-purge.plugin.ts index 95d44925..506f8862 100644 --- a/src/http/plugins/custom/report-purge.plugin.ts +++ b/src/http/plugins/custom/report-purge.plugin.ts @@ -22,8 +22,8 @@ function reportPurgePlugin(fastify: FastifyInstance): void { ); }); - fastify.addHook("onClose", () => { - reportPurgeScheduler.stop(); + fastify.addHook("onClose", async () => { + await reportPurgeScheduler.stop(); fastify.log.info( { diff --git a/src/http/plugins/custom/subscription-reconcile.plugin.ts b/src/http/plugins/custom/subscription-reconcile.plugin.ts index 4c0ef588..34d7ebab 100644 --- a/src/http/plugins/custom/subscription-reconcile.plugin.ts +++ b/src/http/plugins/custom/subscription-reconcile.plugin.ts @@ -22,8 +22,8 @@ function subscriptionReconcilePlugin(fastify: FastifyInstance): void { ); }); - fastify.addHook("onClose", () => { - subscriptionReconcileScheduler.stop(); + fastify.addHook("onClose", async () => { + await subscriptionReconcileScheduler.stop(); fastify.log.info( { diff --git a/src/http/plugins/di/use-cases.di.ts b/src/http/plugins/di/use-cases.di.ts index cefcda26..1e34056c 100644 --- a/src/http/plugins/di/use-cases.di.ts +++ b/src/http/plugins/di/use-cases.di.ts @@ -380,10 +380,16 @@ export const useCasesModule = { * Use case for JWT token refresh */ refreshUseCase: asFunction( - (transactionService, authTokenService, config) => + ( + transactionService, + authTokenService, + refreshTokenRepository, + config, + ) => new RefreshUseCase( transactionService, authTokenService, + refreshTokenRepository, config.REFRESH_ROTATION_GRACE_SECONDS, ), ).singleton(), diff --git a/src/http/plugins/idempotency/idempotency.plugin.ts b/src/http/plugins/idempotency/idempotency.plugin.ts index c6cc8c82..11153bbe 100644 --- a/src/http/plugins/idempotency/idempotency.plugin.ts +++ b/src/http/plugins/idempotency/idempotency.plugin.ts @@ -1,6 +1,6 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; import fastifyPlugin from "fastify-plugin"; -import { ConflictError } from "@core/errors"; +import { BadRequestError, ConflictError } from "@core/errors"; import type { CachePort } from "@core/ports/services/cache.port"; import { fingerprintBody, @@ -16,9 +16,20 @@ const REPLAY_HEADER = "idempotent-replay"; /** Longest key a client may send; anything longer is a mistake or an attack. */ const MAX_KEY_LENGTH = 200; -/** How long a claim and its answer are remembered. */ +/** How long a completed answer is remembered. */ const RECORD_TTL_SECONDS = 24 * 60 * 60; +/** + * How long an *unfinished* claim is held. + * + * Short on purpose. The claim is cleared when the response is sent, so the + * only thing that leaves one behind is a process that died mid-request - a + * redeploy, a crash - and a claim that then sat for a day would answer every + * retry with "still in progress" for a write that never happened. Longer than + * any request this guards takes, and far shorter than a working day. + */ +const CLAIM_TTL_SECONDS = 120; + /** * Largest response body kept for replay. * @@ -117,7 +128,9 @@ function idempotencyPlugin(fastify: FastifyInstance): void { if (typeof key !== "string" || key.length === 0 || !userId) return; if (key.length > MAX_KEY_LENGTH) { - throw new ConflictError("Idempotency-Key is too long."); + // A malformed request, not a conflict. A client told 409 reads it + // as "already done" and stops retrying with a corrected key. + throw new BadRequestError("Idempotency-Key is too long."); } const cacheKey = idempotencyCacheKey( @@ -137,7 +150,7 @@ function idempotencyPlugin(fastify: FastifyInstance): void { state: "in-flight", fingerprint, } satisfies IdempotencyRecord), - RECORD_TTL_SECONDS, + CLAIM_TTL_SECONDS, ); } catch (error: unknown) { // Fail open, deliberately. This is a safety net over a write that diff --git a/src/http/plugins/rate-limit.plugin.ts b/src/http/plugins/rate-limit.plugin.ts index 742635f9..3337073b 100644 --- a/src/http/plugins/rate-limit.plugin.ts +++ b/src/http/plugins/rate-limit.plugin.ts @@ -28,19 +28,46 @@ 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, timeWindow: "15 minutes", continueExceeding: true, - // Pinned to the IP, overriding the account key the rest of the API - // uses. This policy guards login and registration, where there is no - // proven account yet - so a caller may attach any valid token of its - // own and would otherwise be handed a fresh bucket per account it - // holds, turning three attempts per quarter hour into three times - // however many accounts it can collect. Keeping registration itself on - // the IP key is what bounds that collection. - keyGenerator: (request: FastifyRequest): string => request.ip, + // Pinned to the caller's address, overriding the account key the rest + // of the API uses. This policy guards login and registration, where + // there is no proven account yet - so a caller may attach any valid + // token of its own and would otherwise be handed a fresh bucket per + // 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), }, SENSITIVE: { max: 5, @@ -109,7 +136,7 @@ export function rateLimitKeyFor( } } - return request.ip; + return untrustedClientIp(request); } function rateLimitPlugin(fastify: FastifyInstance): void { diff --git a/src/infrastructure/external/billing/play/play-notification.service.ts b/src/infrastructure/external/billing/play/play-notification.service.ts index a8f6f8b3..158e75f2 100644 --- a/src/infrastructure/external/billing/play/play-notification.service.ts +++ b/src/infrastructure/external/billing/play/play-notification.service.ts @@ -4,7 +4,10 @@ import type { ISubscriptionRepository } from "@core/ports/repositories/subscript import type { BillingPort } from "@core/ports/services/billing.port"; import type { LoggerPort } from "@core/ports/services/logger.port"; import type { SyncSubscriptionUseCase } from "@core/use-cases/billing/sync-subscription"; -import { parsePlayNotification } from "./play-notification"; +import { + parsePlayNotification, + type PlaySubscriptionNotification, +} from "./play-notification"; /** * What handling a notification came to. @@ -75,6 +78,29 @@ export class PlayNotificationService { if (!isNew) return "duplicate"; + try { + return await this.apply(notification); + } catch (error: unknown) { + // The delivery was recorded before the work, so a failure here + // would make Pub/Sub's redelivery look like a duplicate and drop + // it silently. Releasing the record puts the retry back on the + // table; the exception then reaches the caller as a 5xx, which is + // the answer that makes Google send it again. + await this.billingEventRepository.forget(notification.messageId); + + throw error; + } + } + + /** + * Reads the truth from Google and applies it. + * + * @param notification - The delivery being handled + * @returns What became of it + */ + private async apply( + notification: PlaySubscriptionNotification, + ): Promise { const subscription = await this.subscriptionRepository.findByProviderSubscriptionId( notification.purchaseToken, diff --git a/src/infrastructure/jobs/billing/subscription-reconcile.scheduler.ts b/src/infrastructure/jobs/billing/subscription-reconcile.scheduler.ts index 9d14dcdb..0ccb6fb3 100644 --- a/src/infrastructure/jobs/billing/subscription-reconcile.scheduler.ts +++ b/src/infrastructure/jobs/billing/subscription-reconcile.scheduler.ts @@ -78,9 +78,18 @@ export class SubscriptionReconcileScheduler { /** * Stops the schedule. + * + * Destroys the task rather than only dropping the reference. node-cron + * keeps its own registry, so a schedule that is merely forgotten keeps + * firing after `onClose` - against a Prisma client that has already + * disconnected. */ - stop(): void { + async stop(): Promise { if (!this.task) return; + + const task = this.task; this.task = undefined; + + await task.destroy(); } } diff --git a/src/infrastructure/jobs/device/device-purge.scheduler.ts b/src/infrastructure/jobs/device/device-purge.scheduler.ts index c7e87d45..cb30e0e1 100644 --- a/src/infrastructure/jobs/device/device-purge.scheduler.ts +++ b/src/infrastructure/jobs/device/device-purge.scheduler.ts @@ -63,9 +63,18 @@ export class DevicePurgeScheduler { /** * Stops the schedule. + * + * Destroys the task rather than only dropping the reference. node-cron + * keeps its own registry, so a schedule that is merely forgotten keeps + * firing after `onClose` - against a Prisma client that has already + * disconnected. */ - stop(): void { + async stop(): Promise { if (!this.task) return; + + const task = this.task; this.task = undefined; + + await task.destroy(); } } diff --git a/src/infrastructure/jobs/report/report-purge.scheduler.ts b/src/infrastructure/jobs/report/report-purge.scheduler.ts index 82b6b85b..3be9151d 100644 --- a/src/infrastructure/jobs/report/report-purge.scheduler.ts +++ b/src/infrastructure/jobs/report/report-purge.scheduler.ts @@ -65,9 +65,18 @@ export class ReportPurgeScheduler { /** * Stops the schedule. + * + * Destroys the task rather than only dropping the reference. node-cron + * keeps its own registry, so a schedule that is merely forgotten keeps + * firing after `onClose` - against a Prisma client that has already + * disconnected. */ - stop(): void { + async stop(): Promise { if (!this.task) return; + + const task = this.task; this.task = undefined; + + await task.destroy(); } } diff --git a/src/infrastructure/persistence/repositories/prisma-billing-event.repository.ts b/src/infrastructure/persistence/repositories/prisma-billing-event.repository.ts index e22c0073..efebdb84 100644 --- a/src/infrastructure/persistence/repositories/prisma-billing-event.repository.ts +++ b/src/infrastructure/persistence/repositories/prisma-billing-event.repository.ts @@ -46,4 +46,13 @@ export class PrismaBillingEventRepository implements IBillingEventRepository { throw error; } } + + /** + * Removes a record, so the delivery it stood for can be retried. + * + * @param id - The provider's identifier for the delivery. + */ + async forget(id: string): Promise { + await this.prisma.billingEvent.deleteMany({ where: { id } }); + } } diff --git a/tests/e2e/auth/refresh.test.ts b/tests/e2e/auth/refresh.test.ts index f296e984..f0fec76d 100644 --- a/tests/e2e/auth/refresh.test.ts +++ b/tests/e2e/auth/refresh.test.ts @@ -121,17 +121,15 @@ describe("POST /auth/refresh - Token Refresh Flow", () => { }); /** - * Compromise detection, and the window that keeps it from firing on a - * dropped connection. + * Compromise detection. * - * A rotated token presented again moments later is almost always a client - * that never received the response carrying its replacement - on a mobile - * network that happens routinely - so inside the grace window it is served - * as a retry. Presented a third time it is a genuine reuse: the successor - * from the retry has been consumed, which no honest client could have - * done, and every session is revoked. + * CI runs with `REFRESH_ROTATION_GRACE_SECONDS=0`, so a rotated token + * presented again is a reuse rather than a retry - the window itself is + * covered by unit tests, which can move a clock, and this covers the thing + * that actually protects an account: presenting a retired token kills + * every session the user has, including ones opened elsewhere. */ - it("should serve a retry inside the grace window and alarm on a real reuse", async () => { + it("should revoke every session when a retired token is reused", async () => { const ts3 = Date.now(); const compromiseUser = { email: `compromise_${ts3}@example.com`, @@ -164,19 +162,17 @@ describe("POST /auth/refresh - Token Refresh Flow", () => { expect(firstRefresh.statusCode).toBe(200); - // The client never saw that response and tries again with the token - // it still holds. Inside the window this is a retry, not a theft. - const retry = await request({ + // A second session, opened before the alarm. It must not survive it. + const secondSession = await request({ method: "POST", - url: "/auth/refresh", - headers: { cookie: originalCookie }, + url: "/auth/login", + payload: { + identifier: compromiseUser.email, + password: compromiseUser.password, + }, }); + const siblingCookie = extractRefreshTokenCookie(secondSession); - expect(retry.statusCode).toBe(200); - - // A third attempt with the same token cannot be a lost response: the - // successor the retry issued has already been retired, so somebody is - // replaying a token they should not hold. const compromiseResponse = await request({ method: "POST", url: "/auth/refresh", @@ -193,6 +189,17 @@ describe("POST /auth/refresh - Token Refresh Flow", () => { expect(body.detail).toBe( "Security alert: Session compromised. All sessions revoked.", ); + + // The message is not the point - the revocation is. A revocation rolled + // back with its own transaction leaves this session alive while + // telling its owner it was killed. + const sibling = await request({ + method: "POST", + url: "/auth/refresh", + headers: { cookie: siblingCookie }, + }); + + expect(sibling.statusCode).toBe(401); }); /** diff --git a/tests/e2e/oauth/redirect.test.ts b/tests/e2e/oauth/redirect.test.ts index 61828f48..d2ebd8b0 100644 --- a/tests/e2e/oauth/redirect.test.ts +++ b/tests/e2e/oauth/redirect.test.ts @@ -33,7 +33,11 @@ describe("OAuth Redirect Endpoints", () => { ? (new URL(location).searchParams.get("state") ?? "") : ""; - return { statusCode: response.statusCode, location: location ?? "", state }; + return { + statusCode: response.statusCode, + location: location ?? "", + state, + }; }; describe("starting a flow", () => { @@ -88,12 +92,13 @@ describe("OAuth Redirect Endpoints", () => { describe("finishing a flow", () => { it("should report a provider error on the target the flow started for", async () => { - const { state } = await startFlow("github"); + const { state, cookie } = await startFlow("github"); - const response = await request({ - method: "GET", - url: `/oauth/github/callback?error=access_denied&state=${state}`, - }); + const response = await callback( + "github", + `error=access_denied&state=${state}`, + cookie, + ); expect(response.statusCode).toBe(302); expect(response.headers.location).toBe( @@ -102,12 +107,9 @@ describe("OAuth Redirect Endpoints", () => { }); it("should report a missing code on the target the flow started for", async () => { - const { state } = await startFlow("google"); + const { state, cookie } = await startFlow("google"); - const response = await request({ - method: "GET", - url: `/oauth/google/callback?state=${state}`, - }); + const response = await callback("google", `state=${state}`, cookie); expect(response.statusCode).toBe(302); expect(response.headers.location).toBe( @@ -116,15 +118,33 @@ describe("OAuth Redirect Endpoints", () => { }); it("should send an app flow's failure to the app", async () => { - const { state } = await startFlow("github", NATIVE_TARGET); + const { state, cookie } = await startFlow("github", NATIVE_TARGET); + + const response = await callback( + "github", + `error=access_denied&state=${state}`, + cookie, + ); + + expect(response.headers.location).toBe( + `${NATIVE_TARGET}?error=github_access_denied`, + ); + }); + + it("should complete nothing when the browser did not start the flow", async () => { + // The state exists in the cache, so it passes the old check. What + // it does not have is the cookie the starting browser was given - + // which is the whole point: an attacker can start a flow with + // their own account and hand the callback URL to a victim. + const { state } = await startFlow("github"); const response = await request({ method: "GET", - url: `/oauth/github/callback?error=access_denied&state=${state}`, + url: `/oauth/github/callback?code=whatever&state=${state}`, }); expect(response.headers.location).toBe( - `${NATIVE_TARGET}?error=github_access_denied`, + `${FRONTEND_URL}/login?error=invalid_state`, ); }); @@ -154,16 +174,18 @@ describe("OAuth Redirect Endpoints", () => { }); it("should spend a state exactly once", async () => { - const { state } = await startFlow("github"); + const { state, cookie } = await startFlow("github"); - const first = await request({ - method: "GET", - url: `/oauth/github/callback?error=access_denied&state=${state}`, - }); - const replay = await request({ - method: "GET", - url: `/oauth/github/callback?error=access_denied&state=${state}`, - }); + const first = await callback( + "github", + `error=access_denied&state=${state}`, + cookie, + ); + const replay = await callback( + "github", + `error=access_denied&state=${state}`, + cookie, + ); expect(first.headers.location).toBe( `${FRONTEND_URL}/login?error=github_access_denied`, diff --git a/tests/e2e/security/mass-assignment.test.ts b/tests/e2e/security/mass-assignment.test.ts new file mode 100644 index 00000000..a739fda6 --- /dev/null +++ b/tests/e2e/security/mass-assignment.test.ts @@ -0,0 +1,122 @@ +import { authRequest, parseBody, request } from "../setup"; +import { beforeAll, describe, expect, it } from "vitest"; + +/** + * E2E tests for the identity a request body must never be able to set. + * + * Several handlers spread `...request.body` alongside an identity taken from + * the session. That is only safe while the validator refuses to pass unknown + * properties through - Fastify's default does *not*, because it only strips + * them from schemas that say `additionalProperties: false`, and a plain + * TypeBox object says nothing. The app now runs AJV with + * `removeAdditional: "all"`, and these are the tests that would fail if that + * were ever loosened. + */ +describe("Identity cannot be supplied by the request body", () => { + const ts = Date.now(); + const attacker = { + email: `ma-a-${ts}@test.com`, + password: "password123", + username: `maa${ts}`, + }; + const victim = { + email: `ma-v-${ts}@test.com`, + password: "password123", + username: `mav${ts}`, + }; + + let attackerToken = ""; + let attackerId = ""; + let victimId = ""; + + const registerAndLogin = async (u: { + email: string; + password: string; + username: string; + }): Promise<{ id: string; token: string }> => { + const registered = await request({ + method: "POST", + url: "/auth/register", + payload: u, + }); + const id = parseBody<{ data: { id: string } }>(registered).data.id; + + const loggedIn = await request({ + method: "POST", + url: "/auth/login", + payload: { identifier: u.email, password: u.password }, + }); + + return { + id, + token: parseBody<{ data: { accessToken: string } }>(loggedIn).data + .accessToken, + }; + }; + + beforeAll(async () => { + const a = await registerAndLogin(attacker); + const v = await registerAndLogin(victim); + + attackerToken = a.token; + attackerId = a.id; + victimId = v.id; + }); + + it("should file an article under the caller, not the id in the body", async () => { + // Left unchecked this writes a draft owned by the victim, which the + // same trick can then publish under their byline. + const response = await authRequest(attackerToken, { + method: "POST", + url: "/articles", + payload: { + title: `Mass assignment ${ts}`, + content: "#".repeat(1) + " body long enough to pass validation", + authorId: victimId, + }, + }); + + expect(response.statusCode).toBe(201); + + const author = parseBody<{ data: { author: { id: string } } }>(response) + .data.author; + + expect(author.id).toBe(attackerId); + expect(author.id).not.toBe(victimId); + }); + + it("should refuse an edit to somebody else's article, however the body is dressed", async () => { + // The nastiest variant: UpdateArticleUseCase proves ownership by + // asking whether `userId` owns the article, so a body carrying the + // victim's `userId` does not fail that check - it satisfies it. + const victimSession = await request({ + method: "POST", + url: "/auth/login", + payload: { identifier: victim.email, password: victim.password }, + }); + const victimToken = parseBody<{ data: { accessToken: string } }>( + victimSession, + ).data.accessToken; + + const created = await authRequest(victimToken, { + method: "POST", + url: "/articles", + payload: { + title: `Victim article ${ts}`, + content: "# a body long enough to pass validation", + }, + }); + const articleId = parseBody<{ data: { id: string } }>(created).data.id; + + const hijack = await authRequest(attackerToken, { + method: "PATCH", + url: `/articles/${articleId}`, + payload: { + title: "Rewritten by somebody else", + userId: victimId, + }, + }); + + expect([403, 404]).toContain(hijack.statusCode); + }); +}); diff --git a/tests/unit/core/use-cases/auth/refresh.usecase.test.ts b/tests/unit/core/use-cases/auth/refresh.usecase.test.ts index 214d49f4..83976481 100644 --- a/tests/unit/core/use-cases/auth/refresh.usecase.test.ts +++ b/tests/unit/core/use-cases/auth/refresh.usecase.test.ts @@ -56,6 +56,10 @@ describe("RefreshUseCase", () => { useCase = new RefreshUseCase( transactionSvc as TransactionPort, authTokenSvc as AuthTokenPort, + // The same double stands in for the repository outside the + // transaction; what matters is that the revocation is issued + // there rather than inside it. + refreshTokenRepo as IRefreshTokenRepository, GRACE_SECONDS, ); }); @@ -255,6 +259,31 @@ describe("RefreshUseCase", () => { expect(refreshTokenRepo.update).toHaveBeenCalledWith(successor); }); + it("should serve a second consecutive retry rather than raise the alarm", async () => { + // Two responses lost in a row. The client is still holding the + // token it started with, and the successor it points at was + // retired by the *first* retry - by us, not by anybody who + // received it. Reading that as theft signs an honest user out of + // every device they own. + const { retired } = arrangeRetry(5); + + await useCase.execute(input); + + expect(retired.replacedById).toBe("token-3"); + expect(refreshTokenRepo.revokeAllByUserId).not.toHaveBeenCalled(); + }); + + it("should not move the window forward when it serves a retry", async () => { + // Otherwise a stolen token could be kept alive indefinitely by + // presenting it every twenty-nine seconds. + const { retired } = arrangeRetry(5); + const revokedAtBefore = retired.revokedAt; + + await useCase.execute(input); + + expect(retired.revokedAt).toEqual(revokedAtBefore); + }); + it("should raise the alarm for a reuse outside the window", async () => { arrangeRetry(GRACE_SECONDS + 5); diff --git a/tests/unit/core/use-cases/user/soft-delete-user.usecase.test.ts b/tests/unit/core/use-cases/user/soft-delete-user.usecase.test.ts index 7663daab..0421e15f 100644 --- a/tests/unit/core/use-cases/user/soft-delete-user.usecase.test.ts +++ b/tests/unit/core/use-cases/user/soft-delete-user.usecase.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { LoggerPort } from "@core/ports/services/logger.port"; import type { RevokeSubscriptionUseCase } from "@core/use-cases/billing/revoke-subscription"; import { SoftDeleteUserUseCase } from "@core/use-cases/user/soft-delete"; import type { IUserRepository } from "@core/ports/repositories/user.repository"; @@ -32,6 +33,7 @@ describe("SoftDeleteUserUseCase", () => { passwordService as PasswordPort, emailService as EmailPort, revokeSubscriptionUseCase as RevokeSubscriptionUseCase, + { error: vi.fn(), warn: vi.fn() } as unknown as LoggerPort, ); }); @@ -141,4 +143,22 @@ describe("SoftDeleteUserUseCase", () => { "user-1", ); }); + + it("should complete the deletion even if the cancellation fails", async () => { + // The account is already soft-deleted by then; throwing here would + // report a 500 for a deletion that happened, and skip the email. + vi.mocked(userRepository.findById).mockResolvedValue( + buildUser({ password: "hashed" }), + ); + vi.mocked(passwordService.verify).mockResolvedValue(true); + vi.mocked(revokeSubscriptionUseCase.execute).mockRejectedValue( + new Error("billing down"), + ); + + await expect( + useCase.execute({ id: "user-1", password: "correct" }), + ).resolves.toBeUndefined(); + + expect(emailService.sendDeleteUserEmail).toHaveBeenCalled(); + }); }); diff --git a/tests/unit/http/rate-limit-key.test.ts b/tests/unit/http/rate-limit-key.test.ts index f5de40f0..c882f275 100644 --- a/tests/unit/http/rate-limit-key.test.ts +++ b/tests/unit/http/rate-limit-key.test.ts @@ -20,9 +20,12 @@ function fastifyWith(valid: Record): FastifyInstance { } as unknown as FastifyInstance; } -function requestWith(authorization?: string): FastifyRequest { +function requestWith( + authorization?: string, + headers: Record = {}, +): FastifyRequest { return { - headers: authorization ? { authorization } : {}, + headers: authorization ? { authorization, ...headers } : headers, ip: "203.0.113.7", } as unknown as FastifyRequest; } @@ -70,6 +73,18 @@ describe("rateLimitKeyFor", () => { }); describe("RateLimitPolicies.STRICT", () => { + it("should prefer the edge address over the proxied one", () => { + // `request.ip` is the left-hand end of X-Forwarded-For, which the + // client writes. Keying brute-force protection on it hands the caller + // a fresh bucket per request; the edge header is set by Cloudflare and + // cannot be supplied from outside. + const key = RateLimitPolicies.STRICT.keyGenerator( + requestWith(undefined, { "cf-connecting-ip": "198.51.100.9" }), + ); + + expect(key).toBe("198.51.100.9"); + }); + it("should key on the IP regardless of any token attached", () => { // Login and registration run under this policy. If a caller could // pick its bucket by attaching a token it already holds, three From 806f61389030187f5e3355b0fd28af49033c598f Mon Sep 17 00:00:00 2001 From: unknown Date: Sun, 6 Sep 2026 16:08:19 +0300 Subject: [PATCH 2/3] test: repair the e2e cases the review fixes changed The state cookie means a callback must now carry it, the article body field is `body` not `content`, and the rotation retry case cannot hold with the grace window switched off in CI - that path is unit-tested, where a clock can be moved. Co-Authored-By: Claude Opus 5 --- tests/e2e/auth/native-session.test.ts | 27 ---------------------- tests/e2e/oauth/redirect.test.ts | 14 +++++++++++ tests/e2e/security/mass-assignment.test.ts | 4 ++-- 3 files changed, 16 insertions(+), 29 deletions(-) diff --git a/tests/e2e/auth/native-session.test.ts b/tests/e2e/auth/native-session.test.ts index 465a0c77..5fa6e6da 100644 --- a/tests/e2e/auth/native-session.test.ts +++ b/tests/e2e/auth/native-session.test.ts @@ -127,33 +127,6 @@ describe("Native session delivery", () => { expect(response.statusCode).toBe(401); }); - - it("should serve a retry that repeats the previous token", async () => { - const session = parseBody<{ data: SessionData }>( - await login("native"), - ).data; - - const first = await request({ - method: "POST", - url: "/auth/refresh", - payload: { refreshToken: session.refreshToken }, - }); - expect(first.statusCode).toBe(200); - - // The client never saw that response and tries again with the - // token it still holds. Inside the grace window this is a retry, - // not a stolen token, and must not sign every device out. - const retry = await request({ - method: "POST", - url: "/auth/refresh", - payload: { refreshToken: session.refreshToken }, - }); - - expect(retry.statusCode).toBe(200); - expect( - parseBody<{ data: SessionData }>(retry).data.refreshToken, - ).toBeTruthy(); - }); }); describe("POST /auth/logout", () => { diff --git a/tests/e2e/oauth/redirect.test.ts b/tests/e2e/oauth/redirect.test.ts index d2ebd8b0..134fef76 100644 --- a/tests/e2e/oauth/redirect.test.ts +++ b/tests/e2e/oauth/redirect.test.ts @@ -40,6 +40,20 @@ describe("OAuth Redirect Endpoints", () => { }; }; + /** + * Hits a callback the way the browser that started the flow would. + */ + const callback = ( + provider: string, + query: string, + cookie: string, + ): ReturnType => + request({ + method: "GET", + url: `/oauth/${provider}/callback?${query}`, + cookies: { oauthState: cookie }, + }); + describe("starting a flow", () => { it("should redirect to GitHub with a state parameter", async () => { const { statusCode, location, state } = await startFlow("github"); diff --git a/tests/e2e/security/mass-assignment.test.ts b/tests/e2e/security/mass-assignment.test.ts index a739fda6..2594fcba 100644 --- a/tests/e2e/security/mass-assignment.test.ts +++ b/tests/e2e/security/mass-assignment.test.ts @@ -71,7 +71,7 @@ describe("Identity cannot be supplied by the request body", () => { url: "/articles", payload: { title: `Mass assignment ${ts}`, - content: "#".repeat(1) + " body long enough to pass validation", + body: "# A body long enough to pass validation.", authorId: victimId, }, }); @@ -103,7 +103,7 @@ describe("Identity cannot be supplied by the request body", () => { url: "/articles", payload: { title: `Victim article ${ts}`, - content: "# a body long enough to pass validation", + body: "# A body long enough to pass validation.", }, }); const articleId = parseBody<{ data: { id: string } }>(created).data.id; From a2655602c2ed03c75a486391ffe57630ba52f110 Mon Sep 17 00:00:00 2001 From: unknown Date: Sun, 6 Sep 2026 16:09:24 +0300 Subject: [PATCH 3/3] test: carry the OAuth state cookie through the e2e helper Co-Authored-By: Claude Opus 5 --- tests/e2e/oauth/redirect.test.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/e2e/oauth/redirect.test.ts b/tests/e2e/oauth/redirect.test.ts index 134fef76..6ab95072 100644 --- a/tests/e2e/oauth/redirect.test.ts +++ b/tests/e2e/oauth/redirect.test.ts @@ -22,7 +22,12 @@ describe("OAuth Redirect Endpoints", () => { const startFlow = async ( provider: "github" | "google", redirect?: string, - ): Promise<{ statusCode: number; location: string; state: string }> => { + ): Promise<{ + statusCode: number; + location: string; + state: string; + cookie: string; + }> => { const response = await request({ method: "GET", url: `/oauth/${provider}${redirect ? `?redirect=${encodeURIComponent(redirect)}` : ""}`, @@ -33,10 +38,16 @@ describe("OAuth Redirect Endpoints", () => { ? (new URL(location).searchParams.get("state") ?? "") : ""; + // The flow is bound to the browser that started it, so a callback has + // to carry this back the way a real browser would. + const cookie = + response.cookies.find((c) => c.name === "oauthState")?.value ?? ""; + return { statusCode: response.statusCode, location: location ?? "", state, + cookie, }; };