refactor: cleanup

donut-sync/src/auth/auth.guard.ts

@@ -1,3 +1,4 @@
+import { timingSafeEqual } from "node:crypto";
 import {
   type CanActivate,
   type ExecutionContext,
@@ -10,6 +11,13 @@ import type { Request } from "express";
 import * as jwt from "jsonwebtoken";
 import type { UserContext } from "./user-context.interface.js";
 
+/** Constant-time string compare; false on length mismatch (no early return). */
+function safeEqual(a: string, b: string): boolean {
+  const ab = Buffer.from(a);
+  const bb = Buffer.from(b);
+  return ab.length === bb.length && timingSafeEqual(ab, bb);
+}
+
 @Injectable()
 export class AuthGuard implements CanActivate {
   private readonly logger = new Logger(AuthGuard.name);
@@ -37,7 +45,7 @@ export class AuthGuard implements CanActivate {
 
     // Try SYNC_TOKEN first (self-hosted mode)
     const expectedToken = this.configService.get<string>("SYNC_TOKEN");
-    if (expectedToken && token === expectedToken) {
+    if (expectedToken && safeEqual(token, expectedToken)) {
       (request as unknown as Record<string, unknown>).user = {
         mode: "self-hosted",
         prefix: "",
@@ -55,10 +63,29 @@ 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)) {
+          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)}`);
+        }
+
         (request as unknown as Record<string, unknown>).user = {
           mode: "cloud",
-          prefix: decoded.prefix || `users/${decoded.sub}/`,
-          teamPrefix: decoded.teamPrefix || null,
+          prefix,
+          teamPrefix,
           profileLimit: decoded.profileLimit || 0,
           teamProfileLimit: decoded.teamProfileLimit || 0,
         } satisfies UserContext;

donut-sync/src/sync/internal.controller.ts

@@ -1,3 +1,4 @@
+import { timingSafeEqual } from "node:crypto";
 import {
   Body,
   Controller,
@@ -9,6 +10,13 @@ import {
 import { ConfigService } from "@nestjs/config";
 import { SyncService } from "./sync.service.js";
 
+/** Constant-time string compare; false on length mismatch. */
+function safeEqual(a: string, b: string): boolean {
+  const ab = Buffer.from(a);
+  const bb = Buffer.from(b);
+  return ab.length === bb.length && timingSafeEqual(ab, bb);
+}
+
 @Controller("v1/internal")
 export class InternalController {
   private readonly internalKey: string | undefined;
@@ -26,7 +34,7 @@ export class InternalController {
     @Headers("x-internal-key") key: string,
     @Body() body: { userId: string; maxProfiles: number },
   ) {
-    if (!this.internalKey || key !== this.internalKey) {
+    if (!this.internalKey || !key || !safeEqual(key, this.internalKey)) {
       throw new UnauthorizedException("Invalid internal key");
     }
 

donut-sync/src/sync/sync.service.ts

@@ -54,6 +54,29 @@ import type {
  */
 const MANIFEST_KEY = ".donut-sync-manifest";
 
+/** Max presigned-URL lifetime. The client requests ~1h; never mint a URL that
+ * outlives this, regardless of a (possibly hostile) client-supplied expiresIn. */
+const MAX_PRESIGN_EXPIRES_IN = 3600;
+
+/** Clamp a client-supplied expiresIn to a sane positive range. */
+function clampExpiresIn(requested: number | undefined): number {
+  const v = typeof requested === "number" && requested > 0 ? requested : 3600;
+  return Math.min(v, MAX_PRESIGN_EXPIRES_IN);
+}
+
+/** Only this metadata key is meaningful to sync (LWW conflict resolution).
+ * Whitelisting prevents a client from signing arbitrary x-amz-meta-* values. */
+function sanitizeMetadata(
+  metadata: Record<string, string> | undefined,
+): Record<string, string> | undefined {
+  if (!metadata) return undefined;
+  const out: Record<string, string> = {};
+  if (typeof metadata["updated-at"] === "string") {
+    out["updated-at"] = metadata["updated-at"];
+  }
+  return Object.keys(out).length > 0 ? out : undefined;
+}
+
 @Injectable()
 export class SyncService implements OnModuleInit {
   private readonly logger = new Logger(SyncService.name);
@@ -286,16 +309,19 @@ export class SyncService implements OnModuleInit {
       await this.checkProfileLimit(ctx);
     }
 
-    const expiresIn = dto.expiresIn || 3600;
+    const expiresIn = clampExpiresIn(dto.expiresIn);
     const expiresAt = new Date(Date.now() + expiresIn * 1000);
 
+    // Whitelist metadata to the single key sync relies on, so a client can't
+    // sign arbitrary x-amz-meta-* values into its objects.
+    const metadata = sanitizeMetadata(dto.metadata);
     const command = new PutCmd({
       Bucket: this.bucket,
       Key: key,
       ContentType: dto.contentType || "application/octet-stream",
       // Signed into the presigned URL as `x-amz-meta-*`. The client must send
       // exactly these headers on the PUT, so we echo them in the response.
-      Metadata: dto.metadata,
+      Metadata: metadata,
     });
 
     const url = await getSignedUrl(this.s3Client, command, { expiresIn });
@@ -313,6 +339,9 @@ export class SyncService implements OnModuleInit {
     return {
       url,
       expiresAt: expiresAt.toISOString(),
+      // Echo the metadata we actually signed so the client sends matching
+      // x-amz-meta-* headers on the PUT (S3 rejects unsigned ones).
+      metadata,
     };
   }
 
@@ -323,7 +352,7 @@ export class SyncService implements OnModuleInit {
     const key = this.scopeKey(ctx, dto.key);
     this.validateKeyAccess(ctx, key);
 
-    const expiresIn = dto.expiresIn || 3600;
+    const expiresIn = clampExpiresIn(dto.expiresIn);
     const expiresAt = new Date(Date.now() + expiresIn * 1000);
 
     const command = new GetObjectCommand({
@@ -438,7 +467,7 @@ export class SyncService implements OnModuleInit {
       await this.checkProfileLimit(ctx);
     }
 
-    const expiresIn = dto.expiresIn || 3600;
+    const expiresIn = clampExpiresIn(dto.expiresIn);
     const expiresAt = new Date(Date.now() + expiresIn * 1000);
 
     const items = await Promise.all(
@@ -491,7 +520,7 @@ export class SyncService implements OnModuleInit {
     dto: PresignDownloadBatchRequestDto,
     ctx: UserContext,
   ): Promise<PresignDownloadBatchResponseDto> {
-    const expiresIn = dto.expiresIn || 3600;
+    const expiresIn = clampExpiresIn(dto.expiresIn);
     const expiresAt = new Date(Date.now() + expiresIn * 1000);
 
     const items = await Promise.all(