From 78803ab2899c25116f9cc7d8ce617d24c198e8cb Mon Sep 17 00:00:00 2001 From: zhom <2717306+zhom@users.noreply.github.com> Date: Tue, 7 Jul 2026 22:45:07 +0400 Subject: [PATCH] refactor: backend-authoritative team scope and config/input hardening --- donut-sync/.env.example | 12 +- donut-sync/src/auth/auth.guard.ts | 112 ++++++++++++--- donut-sync/src/auth/user-context.interface.ts | 11 +- donut-sync/src/main.ts | 18 ++- donut-sync/src/sync/internal.controller.ts | 18 ++- donut-sync/src/sync/sync.service.ts | 135 ++++++++---------- 6 files changed, 196 insertions(+), 110 deletions(-) diff --git a/donut-sync/.env.example b/donut-sync/.env.example index 1976b7e..4d199db 100644 --- a/donut-sync/.env.example +++ b/donut-sync/.env.example @@ -1,9 +1,15 @@ -SYNC_TOKEN=secret-sync-token +# REQUIRED: a long, random shared secret used to authenticate sync clients. +# Generate one, e.g.: openssl rand -hex 32 +# The server refuses to start with this placeholder or a value shorter than 24 chars. +SYNC_TOKEN=CHANGE_ME_generate_a_long_random_secret PORT=12342 + +# REQUIRED S3 / S3-compatible (e.g. MinIO) connection. No defaults are assumed — +# the server fails to start if endpoint / access key / secret key is missing. S3_ENDPOINT=http://localhost:8987 S3_REGION=us-east-1 -S3_ACCESS_KEY_ID=minioadmin -S3_SECRET_ACCESS_KEY=minioadmin +S3_ACCESS_KEY_ID=CHANGE_ME +S3_SECRET_ACCESS_KEY=CHANGE_ME S3_BUCKET=donut-sync S3_FORCE_PATH_STYLE=true diff --git a/donut-sync/src/auth/auth.guard.ts b/donut-sync/src/auth/auth.guard.ts index 2b76548..a41304d 100644 --- a/donut-sync/src/auth/auth.guard.ts +++ b/donut-sync/src/auth/auth.guard.ts @@ -18,10 +18,22 @@ function safeEqual(a: string, b: string): boolean { return ab.length === bb.length && timingSafeEqual(ab, bb); } +type TeamScope = { ownerId: string; teamId: string; teamProfileLimit: number }; + @Injectable() export class AuthGuard implements CanActivate { private readonly logger = new Logger(AuthGuard.name); private jwtPublicKey: string | null = null; + private readonly backendInternalUrl: string | undefined; + private readonly backendInternalKey: string | undefined; + + // Short-lived cache of the per-user team scope so membership revocation takes + // effect quickly (within TTL) without a backend round-trip on every request. + private readonly teamScopeCache = new Map< + string, + { value: TeamScope | null; expires: number } + >(); + private static readonly TEAM_SCOPE_TTL_MS = 30_000; constructor(private configService: ConfigService) { const publicKey = this.configService.get("SYNC_JWT_PUBLIC_KEY"); @@ -29,9 +41,52 @@ export class AuthGuard implements CanActivate { this.jwtPublicKey = publicKey.replace(/\\n/g, "\n"); this.logger.log("JWT public key configured — cloud auth enabled"); } + this.backendInternalUrl = this.configService.get( + "BACKEND_INTERNAL_URL", + ); + this.backendInternalKey = this.configService.get( + "BACKEND_INTERNAL_KEY", + ); } - canActivate(context: ExecutionContext): boolean { + /** + * Resolve a cloud user's team scope via the backend (the ONLY authority for + * team membership). Cached briefly. Throws on backend error so the caller can + * fail closed (fall back to the user's own namespace, never a team one). + */ + private async resolveTeamScope(sub: string): Promise { + if (!this.backendInternalUrl || !this.backendInternalKey) return null; + + const now = Date.now(); + const cached = this.teamScopeCache.get(sub); + if (cached && cached.expires > now) return cached.value; + + const resp = await fetch( + `${this.backendInternalUrl}/api/auth/internal/team-scope`, + { + method: "POST", + headers: { + "Content-Type": "application/json", + "x-internal-key": this.backendInternalKey, + }, + body: JSON.stringify({ userId: sub }), + }, + ); + if (!resp.ok) { + throw new Error(`team-scope resolver returned ${resp.status}`); + } + const value = (await resp.json()) as TeamScope | null; + + // Bound the cache; a coarse clear is fine since entries are cheap to rebuild. + if (this.teamScopeCache.size > 10_000) this.teamScopeCache.clear(); + this.teamScopeCache.set(sub, { + value: value ?? null, + expires: now + AuthGuard.TEAM_SCOPE_TTL_MS, + }); + return value ?? null; + } + + async canActivate(context: ExecutionContext): Promise { const request = context.switchToHttp().getRequest(); const authHeader = request.headers.authorization; @@ -49,9 +104,7 @@ export class AuthGuard implements CanActivate { (request as unknown as Record).user = { mode: "self-hosted", prefix: "", - teamPrefix: null, profileLimit: 0, - teamProfileLimit: 0, } satisfies UserContext; return true; } @@ -63,31 +116,46 @@ export class AuthGuard implements CanActivate { algorithms: ["RS256"], }) as jwt.JwtPayload; - // Validate the scope claims' SHAPE before trusting them as S3 key - // prefixes. An empty/over-broad prefix would make validateKeyAccess - // (`key.startsWith(prefix)`) authorize the entire bucket, so a signer - // bug or permissive claim must not silently widen scope. - const prefix = decoded.prefix || `users/${decoded.sub}/`; - if (typeof prefix !== "string" || !/^users\/[^/]+\/$/.test(prefix)) { + const sub = typeof decoded.sub === "string" ? decoded.sub : ""; + // Validate the prefix claim SHAPE before trusting it as an S3 key + // prefix. An empty/over-broad prefix would make validateKeyAccess + // (`key.startsWith(prefix)`) authorize the entire bucket. + const ownPrefix = decoded.prefix || `users/${sub}/`; + if ( + typeof ownPrefix !== "string" || + !/^users\/[^/]+\/$/.test(ownPrefix) + ) { throw new Error(`Invalid prefix claim: ${String(decoded.prefix)}`); } - const teamPrefix = - decoded.teamPrefix === undefined || decoded.teamPrefix === null - ? null - : decoded.teamPrefix; - if ( - teamPrefix !== null && - !/^teams\/[^/]+\/$/.test(String(teamPrefix)) - ) { - throw new Error(`Invalid teamPrefix claim: ${String(teamPrefix)}`); + + // Resolve the EFFECTIVE namespace: a team member's requests are scoped + // to the shared team owner namespace. The JWT carries no team data — the + // backend is the sole authority. On any resolver error we fail CLOSED: + // fall back to the user's own namespace, never widening to a team one. + let effectivePrefix = ownPrefix; + let effectiveProfileLimit = + typeof decoded.profileLimit === "number" ? decoded.profileLimit : 0; + try { + const scope = sub ? await this.resolveTeamScope(sub) : null; + if (scope && /^[^/]+$/.test(scope.ownerId)) { + effectivePrefix = `users/${scope.ownerId}/`; + if (scope.teamProfileLimit > 0) { + effectiveProfileLimit = scope.teamProfileLimit; + } + } + } catch (err) { + this.logger.warn( + `Team scope resolution failed for ${sub}; using own namespace: ${ + err instanceof Error ? err.message : err + }`, + ); } (request as unknown as Record).user = { mode: "cloud", - prefix, - teamPrefix, - profileLimit: decoded.profileLimit || 0, - teamProfileLimit: decoded.teamProfileLimit || 0, + prefix: effectivePrefix, + profileLimit: effectiveProfileLimit, + sub, } satisfies UserContext; return true; } catch (err) { diff --git a/donut-sync/src/auth/user-context.interface.ts b/donut-sync/src/auth/user-context.interface.ts index ec93d6a..38402d9 100644 --- a/donut-sync/src/auth/user-context.interface.ts +++ b/donut-sync/src/auth/user-context.interface.ts @@ -1,7 +1,10 @@ export interface UserContext { mode: "self-hosted" | "cloud"; - prefix: string; // '' for self-hosted, 'users/{id}/' for cloud - teamPrefix: string | null; // 'teams/{id}/' or null - profileLimit: number; // 0 for unlimited (self-hosted) - teamProfileLimit: number; // 0 for unlimited or non-team users + // The EFFECTIVE namespace for this request: '' for self-hosted, and for cloud + // either the user's own 'users/{sub}/' or, for a team member, the shared team + // owner's 'users/{ownerId}/' — resolved server-side by the AuthGuard from the + // backend (never carried in the JWT). All key scoping uses this directly. + prefix: string; + profileLimit: number; // 0 for unlimited (self-hosted); effective (team) limit for team members + sub?: string; // the authenticated user id (cloud only) } diff --git a/donut-sync/src/main.ts b/donut-sync/src/main.ts index d9cf8e1..ff1b53b 100644 --- a/donut-sync/src/main.ts +++ b/donut-sync/src/main.ts @@ -2,11 +2,27 @@ import { NestFactory } from "@nestjs/core"; import type { NestExpressApplication } from "@nestjs/platform-express"; import { AppModule } from "./app.module.js"; +const INSECURE_DEFAULT_TOKENS = new Set([ + "secret-sync-token", + "CHANGE_ME_generate_a_long_random_secret", + "CHANGE_ME", +]); + function validateEnv() { - if (!process.env.SYNC_TOKEN && !process.env.SYNC_JWT_PUBLIC_KEY) { + const token = process.env.SYNC_TOKEN; + if (!token && !process.env.SYNC_JWT_PUBLIC_KEY) { console.error("Either SYNC_TOKEN or SYNC_JWT_PUBLIC_KEY must be set"); process.exit(1); } + // A static SYNC_TOKEN is the only credential on a self-hosted server that is + // typically exposed on 0.0.0.0, so reject the shipped placeholders and any + // token short enough to brute-force. + if (token && (INSECURE_DEFAULT_TOKENS.has(token) || token.length < 24)) { + console.error( + "SYNC_TOKEN is a known default or too short. Set a long, random secret, e.g. `openssl rand -hex 32`.", + ); + process.exit(1); + } } async function bootstrap() { diff --git a/donut-sync/src/sync/internal.controller.ts b/donut-sync/src/sync/internal.controller.ts index df5a558..f34f4f7 100644 --- a/donut-sync/src/sync/internal.controller.ts +++ b/donut-sync/src/sync/internal.controller.ts @@ -1,5 +1,6 @@ import { timingSafeEqual } from "node:crypto"; import { + BadRequestException, Body, Controller, Headers, @@ -38,9 +39,18 @@ export class InternalController { throw new UnauthorizedException("Invalid internal key"); } - return this.syncService.cleanupExcessProfiles( - body.userId, - body.maxProfiles, - ); + // The userId is interpolated into a destructive S3 delete prefix + // (users/{userId}/profiles/), so constrain it to a plain id — no empty + // value, no slashes/dots that could widen or redirect the prefix. + const userId = body?.userId; + if (typeof userId !== "string" || !/^[A-Za-z0-9_-]{1,128}$/.test(userId)) { + throw new BadRequestException("Invalid userId"); + } + const maxProfiles = body?.maxProfiles; + if (!Number.isInteger(maxProfiles) || maxProfiles < 0) { + throw new BadRequestException("Invalid maxProfiles"); + } + + return this.syncService.cleanupExcessProfiles(userId, maxProfiles); } } diff --git a/donut-sync/src/sync/sync.service.ts b/donut-sync/src/sync/sync.service.ts index f0677b0..bb2ec1e 100644 --- a/donut-sync/src/sync/sync.service.ts +++ b/donut-sync/src/sync/sync.service.ts @@ -13,6 +13,7 @@ import { } from "@aws-sdk/client-s3"; import { getSignedUrl } from "@aws-sdk/s3-request-presigner"; import { + BadRequestException, ForbiddenException, Injectable, Logger, @@ -82,23 +83,34 @@ export class SyncService implements OnModuleInit { private readonly logger = new Logger(SyncService.name); private s3Client: S3Client; private bucket: string; + // Upper bound on presign batch array length (DoS guard). + private static readonly MAX_BATCH_ITEMS = 1000; + private changeSubject = new Subject(); private s3Ready = false; private backendInternalUrl: string | undefined; private backendInternalKey: string | undefined; constructor(private configService: ConfigService) { - const endpoint = - this.configService.get("S3_ENDPOINT") || "http://localhost:8987"; + // Fail fast instead of silently falling back to insecure local dev defaults + // (localhost / minioadmin) — a misconfigured server must not start pointed + // at an unintended or public-default S3 backend. + const requireEnv = (name: string): string => { + const value = this.configService.get(name); + if (!value) { + throw new Error(`Required environment variable ${name} is not set`); + } + return value; + }; + + const endpoint = requireEnv("S3_ENDPOINT"); const region = this.configService.get("S3_REGION") || "us-east-1"; - const accessKeyId = - this.configService.get("S3_ACCESS_KEY_ID") || "minioadmin"; - const secretAccessKey = - this.configService.get("S3_SECRET_ACCESS_KEY") || "minioadmin"; + const accessKeyId = requireEnv("S3_ACCESS_KEY_ID"); + const secretAccessKey = requireEnv("S3_SECRET_ACCESS_KEY"); const forcePathStyle = this.configService.get("S3_FORCE_PATH_STYLE") !== "false"; - this.bucket = this.configService.get("S3_BUCKET") || "donut-sync"; + this.bucket = requireEnv("S3_BUCKET"); this.s3Client = new S3Client({ endpoint, @@ -181,7 +193,6 @@ export class SyncService implements OnModuleInit { */ private scopeKey(ctx: UserContext, key: string): string { if (ctx.mode === "self-hosted") return key; - if (ctx.teamPrefix && key.startsWith(ctx.teamPrefix)) return key; return `${ctx.prefix}${key}`; } @@ -192,9 +203,7 @@ export class SyncService implements OnModuleInit { */ private scopesFor(ctx: UserContext): string[] { if (ctx.mode === "self-hosted") return [""]; - const out = [ctx.prefix]; - if (ctx.teamPrefix) out.push(ctx.teamPrefix); - return out; + return [ctx.prefix]; } /** @@ -243,9 +252,6 @@ export class SyncService implements OnModuleInit { */ private scopeForKey(ctx: UserContext, scopedKey: string): string | null { if (ctx.mode === "self-hosted") return ""; - if (ctx.teamPrefix && scopedKey.startsWith(ctx.teamPrefix)) { - return ctx.teamPrefix; - } if (scopedKey.startsWith(ctx.prefix)) return ctx.prefix; return null; } @@ -258,7 +264,6 @@ export class SyncService implements OnModuleInit { if (ctx.mode === "self-hosted") return; if (key.startsWith(ctx.prefix)) return; - if (ctx.teamPrefix && key.startsWith(ctx.teamPrefix)) return; throw new ForbiddenException("Access denied to this key"); } @@ -422,6 +427,9 @@ export class SyncService implements OnModuleInit { async list(dto: ListRequestDto, ctx?: UserContext): Promise { const prefix = ctx ? this.scopeKey(ctx, dto.prefix) : dto.prefix; + // Enforce scope on the read side too, so a crafted absolute prefix can't + // enumerate another tenant's objects. + if (ctx) this.validateKeyAccess(ctx, prefix); const response = await this.s3Client.send( new ListObjectsV2Command({ @@ -433,15 +441,12 @@ export class SyncService implements OnModuleInit { ); const userPrefix = ctx?.prefix || ""; - const teamPrefix = ctx?.teamPrefix || ""; const objects = (response.Contents || []) // Don't leak donut-sync's internal manifest object to clients. .filter((obj) => !(obj.Key || "").endsWith(MANIFEST_KEY)) .map((obj) => { let key = obj.Key || ""; - if (teamPrefix && key.startsWith(teamPrefix)) { - key = key.substring(teamPrefix.length); - } else if (userPrefix && key.startsWith(userPrefix)) { + if (userPrefix && key.startsWith(userPrefix)) { key = key.substring(userPrefix.length); } return { @@ -462,6 +467,16 @@ export class SyncService implements OnModuleInit { dto: PresignUploadBatchRequestDto, ctx: UserContext, ): Promise { + // Cap batch size: each item triggers a signing operation, so an unbounded + // array is a CPU/memory amplification vector for an authenticated caller. + if ( + !Array.isArray(dto.items) || + dto.items.length > SyncService.MAX_BATCH_ITEMS + ) { + throw new BadRequestException( + `items must be an array of at most ${SyncService.MAX_BATCH_ITEMS} entries`, + ); + } // Check profile limit for cloud users if (ctx.mode === "cloud" && ctx.profileLimit > 0) { await this.checkProfileLimit(ctx); @@ -520,6 +535,14 @@ export class SyncService implements OnModuleInit { dto: PresignDownloadBatchRequestDto, ctx: UserContext, ): Promise { + if ( + !Array.isArray(dto.keys) || + dto.keys.length > SyncService.MAX_BATCH_ITEMS + ) { + throw new BadRequestException( + `keys must be an array of at most ${SyncService.MAX_BATCH_ITEMS} entries`, + ); + } const expiresIn = clampExpiresIn(dto.expiresIn); const expiresAt = new Date(Date.now() + expiresIn * 1000); @@ -551,6 +574,15 @@ export class SyncService implements OnModuleInit { ctx: UserContext, ): Promise { const prefix = this.scopeKey(ctx, dto.prefix); + // Bulk delete is the highest-blast-radius op, yet it was the only mutating + // path that skipped this check — so a client passing an absolute prefix + // (one already starting with its own/team scope, which scopeKey returns + // verbatim) could wipe an entire shared namespace. Enforce scope, and + // refuse an empty scoped prefix (which would match the whole scope). + this.validateKeyAccess(ctx, prefix); + if (ctx.mode === "cloud" && prefix.length === 0) { + throw new ForbiddenException("Refusing to delete an empty prefix"); + } let deletedCount = 0; let tombstoneCreated = false; let continuationToken: string | undefined; @@ -593,6 +625,7 @@ export class SyncService implements OnModuleInit { // Create tombstone if requested if (dto.tombstoneKey && deletedCount > 0) { const scopedTombstoneKey = this.scopeKey(ctx, dto.tombstoneKey); + this.validateKeyAccess(ctx, scopedTombstoneKey); const tombstoneData = JSON.stringify({ prefix: dto.prefix, deleted_at: dto.deletedAt || new Date().toISOString(), @@ -929,22 +962,9 @@ export class SyncService implements OnModuleInit { ); count += userResult.CommonPrefixes?.length || 0; - if (ctx.teamPrefix && ctx.teamProfileLimit && ctx.teamProfileLimit > 0) { - const teamResult = await this.s3Client.send( - new ListObjectsV2Command({ - Bucket: this.bucket, - Prefix: `${ctx.teamPrefix}profiles/`, - Delimiter: "/", - }), - ); - const teamCount = teamResult.CommonPrefixes?.length || 0; - if (teamCount >= ctx.teamProfileLimit) { - throw new ForbiddenException( - `Team profile limit reached (${ctx.teamProfileLimit}). Ask the team owner to upgrade.`, - ); - } - } - + // ctx.prefix is already the effective namespace (the team owner's, for a + // team member) and ctx.profileLimit the effective (team) limit, so this + // single check covers both personal and team accounts. if (count >= ctx.profileLimit) { throw new ForbiddenException( `Profile limit reached (${ctx.profileLimit}). Upgrade your plan for more profiles.`, @@ -985,37 +1005,10 @@ export class SyncService implements OnModuleInit { return match ? match[1] : null; } - private async countTeamProfiles(ctx: UserContext): Promise { - if (!ctx.teamPrefix) return 0; - const profilePrefix = `${ctx.teamPrefix}profiles/`; - let count = 0; - let continuationToken: string | undefined; - - do { - const result = await this.s3Client.send( - new ListObjectsV2Command({ - Bucket: this.bucket, - Prefix: profilePrefix, - Delimiter: "/", - MaxKeys: 1000, - ContinuationToken: continuationToken, - }), - ); - count += result.CommonPrefixes?.length || 0; - continuationToken = result.NextContinuationToken; - } while (continuationToken); - - return count; - } - - private extractTeamId(ctx: UserContext): string | null { - if (!ctx.teamPrefix) return null; - const match = ctx.teamPrefix.match(/^teams\/([^/]+)\/$/); - return match ? match[1] : null; - } - /** - * Fire-and-forget: count profiles and report to backend. + * Fire-and-forget: count profiles and report to backend. The count is for the + * effective namespace (the team owner's, for a team member), reported against + * that namespace's user id — i.e. the team account for teams. */ private reportProfileUsageAsync(ctx: UserContext): void { if (!this.backendInternalUrl || !this.backendInternalKey) return; @@ -1024,17 +1017,7 @@ export class SyncService implements OnModuleInit { if (!userId) return; this.countProfiles(ctx) - .then(async (count) => { - await this.reportProfileUsage(userId, count); - - if (ctx.teamPrefix) { - const teamCount = await this.countTeamProfiles(ctx); - const teamId = this.extractTeamId(ctx); - if (teamId) { - await this.reportProfileUsage(teamId, teamCount); - } - } - }) + .then((count) => this.reportProfileUsage(userId, count)) .catch((err) => this.logger.warn(`Failed to report profile usage: ${err.message}`), );