refactor: backend-authoritative team scope and config/input hardening

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

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<string>("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<string>(
+      "BACKEND_INTERNAL_URL",
+    );
+    this.backendInternalKey = this.configService.get<string>(
+      "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<TeamScope | null> {
+    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<boolean> {
     const request = context.switchToHttp().getRequest<Request>();
     const authHeader = request.headers.authorization;
 
@@ -49,9 +104,7 @@ export class AuthGuard implements CanActivate {
       (request as unknown as Record<string, unknown>).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<string, unknown>).user = {
           mode: "cloud",
-          prefix,
-          teamPrefix,
-          profileLimit: decoded.profileLimit || 0,
-          teamProfileLimit: decoded.teamProfileLimit || 0,
+          prefix: effectivePrefix,
+          profileLimit: effectiveProfileLimit,
+          sub,
         } satisfies UserContext;
         return true;
       } catch (err) {

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

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() {

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

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<SubscribeEventDto>();
   private s3Ready = false;
   private backendInternalUrl: string | undefined;
   private backendInternalKey: string | undefined;
 
   constructor(private configService: ConfigService) {
-    const endpoint =
-      this.configService.get<string>("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<string>(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<string>("S3_REGION") || "us-east-1";
-    const accessKeyId =
-      this.configService.get<string>("S3_ACCESS_KEY_ID") || "minioadmin";
-    const secretAccessKey =
-      this.configService.get<string>("S3_SECRET_ACCESS_KEY") || "minioadmin";
+    const accessKeyId = requireEnv("S3_ACCESS_KEY_ID");
+    const secretAccessKey = requireEnv("S3_SECRET_ACCESS_KEY");
     const forcePathStyle =
       this.configService.get<string>("S3_FORCE_PATH_STYLE") !== "false";
 
-    this.bucket = this.configService.get<string>("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<ListResponseDto> {
     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<PresignUploadBatchResponseDto> {
+    // 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<PresignDownloadBatchResponseDto> {
+    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<DeletePrefixResponseDto> {
     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<number> {
-    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}`),
       );