fix: close three account-takeover paths found in review

.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"
 

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.

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

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
      *

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<boolean>;
+
+    /**
+     * 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<void>;
 }

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<RefreshOutput> {
-        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<RefreshOutcome> => {
+                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<RefreshToken> {
+    ): Promise<RefreshToken | null> {
         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;
     }
 }

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