From 4ff801738c4965d0062c31a2f8cd90e471c71aaa Mon Sep 17 00:00:00 2001 From: yusufipk Date: Tue, 18 Aug 2026 10:35:08 +0300 Subject: [PATCH 1/3] fix(uploads): count a Bunny upload from the moment it is admitted A Bunny init asked the quota whether it could store zero bytes, which is a question with only one answer. Nothing an upload was about to consume was visible to the next request, so every init inside the same window read the same total and every one of them passed, and an upload that could never fit was only refused after it had been sent. The client now declares the size up front. It is checked against the account's remaining room before Bunny is asked for anything, and held as a reservation the next init has to see. The declaration is a claim rather than proof, so it is signed into the upload token: the same token already binds the video id, which is what makes the reservation safe to release on a caller's say-so, since releasing it costs them the video it belongs to. The declared size is then written onto the version or asset row and the reservation is dropped in the same transaction, because Bunny reports no size at all for a video until it has finished encoding it. On a half hour of footage that is most of an hour during which the upload did not appear on the uploader's own storage page and did not count against the next upload. Per-video accounting now takes the larger of what Bunny reports and what was declared, so the estimate stands in until the real figure arrives and Bunny's wins once it does. Two smaller things came out of the same reading. The asset route's in-transaction fallback compared against the plan limit, so a caller quoting a reservation that no longer existed was measured against 200 GiB even on a trial worth three. And the guest branch reserves without being able to release early, because a guest grant is bound to our video id and the caller's network context rather than to the Bunny video, which would let the reservation be dropped while the upload it stands for carried on. --- .../videos/[videoId]/versions/route.ts | 29 ++- .../[projectId]/videos/bunny-init/route.ts | 62 +++++- app/api/projects/[projectId]/videos/route.ts | 31 ++- .../[videoId]/assets/bunny-init/route.ts | 62 +++++- app/api/videos/[videoId]/assets/route.ts | 25 ++- components/video-page/assets-pane.tsx | 5 +- .../video-page/hooks/use-version-actions.ts | 2 +- lib/admin-stats.ts | 27 ++- lib/bunny-upload-token.ts | 109 +++++++++-- lib/client/project-video-upload.ts | 4 +- lib/storage-quota.ts | 2 +- lib/upload-size.ts | 41 ++++ tests/api/bunny-upload-reservation.test.ts | 179 ++++++++++++++++++ .../hooks/use-version-actions.test.ts | 13 +- tests/unit/lib/upload-size.test.ts | 60 ++++++ 15 files changed, 606 insertions(+), 45 deletions(-) create mode 100644 lib/upload-size.ts create mode 100644 tests/api/bunny-upload-reservation.test.ts create mode 100644 tests/unit/lib/upload-size.test.ts diff --git a/app/api/projects/[projectId]/videos/[videoId]/versions/route.ts b/app/api/projects/[projectId]/videos/[videoId]/versions/route.ts index 771efb4..a3ab33c 100644 --- a/app/api/projects/[projectId]/videos/[videoId]/versions/route.ts +++ b/app/api/projects/[projectId]/videos/[videoId]/versions/route.ts @@ -5,7 +5,7 @@ import { validateUrl, validateOptionalUrlOrAppPath } from '@/lib/validation'; import { rateLimit } from '@/lib/rate-limit'; import { notifyProjectOwner } from '@/lib/notifications'; import { apiErrors, successResponse, withCacheControl } from '@/lib/api-response'; -import { verifyBunnyUploadToken } from '@/lib/bunny-upload-token'; +import { readBunnyUploadGrant } from '@/lib/bunny-upload-token'; import { finalizeR2VideoUpload } from '@/lib/r2-video-finalize'; import { logError } from '@/lib/logger'; @@ -65,7 +65,10 @@ export async function POST(request: NextRequest, { params }: RouteParams) { const video = await db.video.findFirst({ where: { id: videoId, projectId }, include: { - project: true, + // The workspace owner comes along because they are the account the + // upload is billed to, and the Bunny reservation released below is held + // against them rather than against whoever is adding the version. + project: { include: { workspace: { select: { ownerId: true } } } }, versions: { orderBy: { versionNumber: 'desc' }, take: 1 }, }, }); @@ -135,6 +138,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { const normalizedUploadToken = typeof uploadToken === 'string' ? uploadToken.trim() : ''; let versionSizeBytes = BigInt(0); + let bunnyReservation: string | null = null; let persistedProviderVideoId = normalizedProviderVideoId; let finalizedR2Session: { sessionId: string; @@ -148,14 +152,23 @@ export async function POST(request: NextRequest, { params }: RouteParams) { return apiErrors.badRequest('Bunny uploads must include providerVideoId and uploadToken'); } - const isValidUploadToken = verifyBunnyUploadToken(normalizedUploadToken, { + const grant = readBunnyUploadGrant(normalizedUploadToken, { userId: session.user.id, projectId, videoId: normalizedProviderVideoId, }); - if (!isValidUploadToken) { + if (!grant) { return apiErrors.forbidden('Invalid Bunny upload token'); } + + // The size the upload was admitted on, written down here so the account is + // charged for it from this moment. Bunny reports nothing at all until it + // has finished encoding, which for a half-hour video is the better part of + // an hour, and until this row existed those bytes were simply invisible: + // the uploader's own storage page read zero and the next upload was + // measured against a total that ignored the one before it. + versionSizeBytes = grant.declaredSizeBytes ?? BigInt(0); + bunnyReservation = grant.reservationId; } else if (normalizedProviderId === 'r2') { const normalizedObjectKey = typeof objectKey === 'string' ? objectKey.trim() : ''; if (!normalizedObjectKey || !normalizedUploadToken) { @@ -226,6 +239,14 @@ export async function POST(request: NextRequest, { params }: RouteParams) { } } + // Handed over in the same transaction that records the size, so the bytes + // are never counted twice and never counted zero times. + if (bunnyReservation) { + await tx.uploadReservation.deleteMany({ + where: { id: bunnyReservation, billedUserId: video.project.workspace.ownerId }, + }); + } + return tx.videoVersion.create({ data: { versionNumber: nextVersionNumber, diff --git a/app/api/projects/[projectId]/videos/bunny-init/route.ts b/app/api/projects/[projectId]/videos/bunny-init/route.ts index 2eec2c9..895a3b9 100644 --- a/app/api/projects/[projectId]/videos/bunny-init/route.ts +++ b/app/api/projects/[projectId]/videos/bunny-init/route.ts @@ -5,13 +5,28 @@ import { apiErrors, successResponse, withCacheControl } from '@/lib/api-response import { rateLimit } from '@/lib/rate-limit'; import crypto from 'crypto'; import { cleanupBunnyStreamVideos } from '@/lib/bunny-stream-cleanup'; -import { createBunnyUploadToken, verifyBunnyUploadToken } from '@/lib/bunny-upload-token'; -import { isBunnyUploadsEnabled } from '@/lib/feature-flags'; +import { + createBunnyUploadToken, + readBunnyUploadGrant, + verifyBunnyUploadToken, +} from '@/lib/bunny-upload-token'; +import { getMaxVideoUploadBytes, isBunnyUploadsEnabled } from '@/lib/feature-flags'; import { logError } from '@/lib/logger'; -import { enforceStorageQuota } from '@/lib/storage-quota'; +import { + enforceStorageQuota, + releaseStorageReservation, + reserveStorageQuota, +} from '@/lib/storage-quota'; +import { parseDeclaredUploadSize } from '@/lib/upload-size'; type RouteParams = { params: Promise<{ projectId: string }> }; +// Long enough to outlive a slow upload and Bunny's own reporting delay, matching +// what the R2 video path already reserves for. The reservation is what makes +// concurrent uploads visible to each other, so it has to stay until the bytes it +// stands for are counted, not until the upload finishes. +const BUNNY_RESERVATION_TTL_MS = 2 * 60 * 60 * 1000; + async function getProjectWithEditAccess(projectId: string, userId: string) { const project = await db.project.findUnique({ where: { id: projectId }, @@ -64,14 +79,37 @@ export async function POST(request: NextRequest, { params }: RouteParams) { return apiErrors.badRequest('Bunny direct uploads are disabled by this host'); } - const quotaError = await enforceStorageQuota(project.workspace.ownerId, BigInt(0)); + // The size the client says it is about to upload. It is a claim, not proof, + // and the bytes never pass through us to be checked: they go straight to + // Bunny, whose own reporting is what eventually settles the account. What + // the claim buys is the two things asking for zero bytes could not. An + // upload that plainly does not fit is refused before it starts instead of + // halfway through, and the reservation written below makes concurrent + // uploads visible to each other, where previously every request in the same + // two-minute window read the same stale total and every one of them passed. + const declaredSize = parseDeclaredUploadSize(body?.sizeBytes, getMaxVideoUploadBytes()); + if ('error' in declaredSize) { + return apiErrors.badRequest(declaredSize.error); + } + + const billedUserId = project.workspace.ownerId; + const quotaError = await enforceStorageQuota(billedUserId, declaredSize.sizeBytes); if (quotaError) return quotaError; + const reserveResult = await reserveStorageQuota( + billedUserId, + declaredSize.sizeBytes, + BUNNY_RESERVATION_TTL_MS + ); + if ('error' in reserveResult) return reserveResult.error; + const { reservationId } = reserveResult; + const apiKey = process.env.BUNNY_STREAM_API_KEY; const libraryId = process.env.BUNNY_STREAM_LIBRARY_ID || process.env.NEXT_PUBLIC_BUNNY_STREAM_LIBRARY_ID; if (!apiKey || !libraryId) { + await releaseStorageReservation(reservationId, billedUserId); return apiErrors.internalError('Bunny Stream is not configured correctly'); } @@ -87,6 +125,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { }); if (!bunnyRes.ok) { + await releaseStorageReservation(reservationId, billedUserId); logError('Failed to create Bunny Stream video', await bunnyRes.text()); return apiErrors.internalError('Failed to initialize video upload with provider'); } @@ -94,6 +133,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { const bunnyVideo = await bunnyRes.json(); const videoId = bunnyVideo.guid; if (typeof videoId !== 'string' || videoId.length === 0) { + await releaseStorageReservation(reservationId, billedUserId); return apiErrors.internalError('Upload provider did not return a valid video identifier'); } @@ -109,6 +149,8 @@ export async function POST(request: NextRequest, { params }: RouteParams) { userId: session.user.id, projectId, videoId, + reservationId, + declaredSizeBytes: declaredSize.sizeBytes, }, 3600 ); @@ -164,6 +206,18 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { return apiErrors.forbidden('Invalid Bunny upload token'); } + // Giving the quota back here rather than waiting for the reservation to + // lapse: an abandoned upload that keeps holding gigabytes for two hours is + // most of a trial's whole allowance, and the account has nothing to show for + // it. Safe to do on a caller's say-so only because the reservation id rides + // inside the signed token next to this video id, so releasing it costs the + // caller the video it belongs to. + await releaseStorageReservation( + readBunnyUploadGrant(uploadToken, { userId: session.user.id, projectId, videoId }) + ?.reservationId ?? null, + project.workspace.ownerId + ); + await cleanupBunnyStreamVideos([{ providerId: 'bunny', videoId }]); const response = successResponse({ message: 'Pending upload cleaned up' }); diff --git a/app/api/projects/[projectId]/videos/route.ts b/app/api/projects/[projectId]/videos/route.ts index cad3a12..5954779 100644 --- a/app/api/projects/[projectId]/videos/route.ts +++ b/app/api/projects/[projectId]/videos/route.ts @@ -5,7 +5,7 @@ import { validateUrl, validateOptionalUrlOrAppPath } from '@/lib/validation'; import { rateLimit } from '@/lib/rate-limit'; import { notifyProjectOwner } from '@/lib/notifications'; import { apiErrors, successResponse, withCacheControl } from '@/lib/api-response'; -import { verifyBunnyUploadToken } from '@/lib/bunny-upload-token'; +import { readBunnyUploadGrant } from '@/lib/bunny-upload-token'; import { finalizeR2VideoUpload } from '@/lib/r2-video-finalize'; import { logError } from '@/lib/logger'; import { eventKey, recordEvent } from '@/lib/analytics/record'; @@ -77,7 +77,15 @@ export async function POST(request: NextRequest, { params }: RouteParams) { // Check project access (must be owner, project admin, or workspace admin) const project = await db.project.findUnique({ where: { id: projectId }, - select: { id: true, name: true, ownerId: true, workspaceId: true, visibility: true }, + select: { + id: true, + name: true, + ownerId: true, + workspaceId: true, + visibility: true, + // The billed account, which is who the Bunny reservation is held against. + workspace: { select: { ownerId: true } }, + }, }); if (!project) { @@ -135,6 +143,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { const normalizedUploadToken = typeof uploadToken === 'string' ? uploadToken.trim() : ''; let versionSizeBytes = BigInt(0); + let bunnyReservation: string | null = null; let finalizedR2Session: { sessionId: string; reservationId: string | null; @@ -147,14 +156,20 @@ export async function POST(request: NextRequest, { params }: RouteParams) { return apiErrors.badRequest('Bunny uploads must include videoId and uploadToken'); } - const isValidUploadToken = verifyBunnyUploadToken(normalizedUploadToken, { + const grant = readBunnyUploadGrant(normalizedUploadToken, { userId: session.user.id, projectId, videoId: normalizedVideoId, }); - if (!isValidUploadToken) { + if (!grant) { return apiErrors.forbidden('Invalid Bunny upload token'); } + + // See the versions route: Bunny reports no size at all until it has + // finished encoding, so the size the upload was admitted on is what the + // account is charged until a real figure arrives. + versionSizeBytes = grant.declaredSizeBytes ?? BigInt(0); + bunnyReservation = grant.reservationId; } else if (normalizedProviderId === 'r2') { const normalizedObjectKey = typeof objectKey === 'string' ? objectKey.trim() : ''; if (!normalizedObjectKey || !normalizedUploadToken) { @@ -227,6 +242,14 @@ export async function POST(request: NextRequest, { params }: RouteParams) { } } + // Released in the transaction that records the size, so the bytes are never + // counted twice and never counted zero times. + if (bunnyReservation) { + await tx.uploadReservation.deleteMany({ + where: { id: bunnyReservation, billedUserId: project.workspace.ownerId }, + }); + } + return tx.video.create({ data: { title: title.trim(), diff --git a/app/api/videos/[videoId]/assets/bunny-init/route.ts b/app/api/videos/[videoId]/assets/bunny-init/route.ts index 0e25e68..b666e28 100644 --- a/app/api/videos/[videoId]/assets/bunny-init/route.ts +++ b/app/api/videos/[videoId]/assets/bunny-init/route.ts @@ -2,7 +2,11 @@ import crypto from 'crypto'; import { NextRequest } from 'next/server'; import { apiErrors, successResponse, withCacheControl } from '@/lib/api-response'; import { rateLimit } from '@/lib/rate-limit'; -import { createBunnyUploadToken, verifyBunnyUploadToken } from '@/lib/bunny-upload-token'; +import { + createBunnyUploadToken, + readBunnyUploadGrant, + verifyBunnyUploadToken, +} from '@/lib/bunny-upload-token'; import { cleanupBunnyStreamVideos } from '@/lib/bunny-stream-cleanup'; import { createGuestUploadToken, @@ -10,14 +14,23 @@ import { enforceGuestUploadQuota, verifyGuestUploadToken, } from '@/lib/guest-upload-token'; -import { isBunnyUploadsEnabled } from '@/lib/feature-flags'; +import { getMaxVideoUploadBytes, isBunnyUploadsEnabled } from '@/lib/feature-flags'; import { getShareSessionFromRequest } from '@/lib/share-session'; import { getVideoAssetAccessContext, SAFE_BUNNY_VIDEO_ID } from '@/lib/video-assets'; import { logError } from '@/lib/logger'; -import { enforceStorageQuota } from '@/lib/storage-quota'; +import { + enforceStorageQuota, + releaseStorageReservation, + reserveStorageQuota, +} from '@/lib/storage-quota'; +import { parseDeclaredUploadSize } from '@/lib/upload-size'; type RouteParams = { params: Promise<{ videoId: string }> }; +// Matches the project video path: long enough to outlive a slow upload and +// Bunny's own reporting delay. +const BUNNY_RESERVATION_TTL_MS = 2 * 60 * 60 * 1000; + // POST /api/videos/[videoId]/assets/bunny-init export async function POST(request: NextRequest, { params }: RouteParams) { try { @@ -37,8 +50,16 @@ export async function POST(request: NextRequest, { params }: RouteParams) { return apiErrors.badRequest('Direct uploads are disabled by this host'); } + // See the project video route for why the client's declared size is asked for + // and what it is worth: it buys an honest refusal before the upload starts, + // and a reservation that concurrent uploads can see. + const declaredSize = parseDeclaredUploadSize(body?.sizeBytes, getMaxVideoUploadBytes()); + if ('error' in declaredSize) { + return apiErrors.badRequest(declaredSize.error); + } + const billedUserId = context.video.project.workspace.ownerId; - const quotaError = await enforceStorageQuota(billedUserId, BigInt(0)); + const quotaError = await enforceStorageQuota(billedUserId, declaredSize.sizeBytes); if (quotaError) return quotaError; const shareSession = getShareSessionFromRequest(request, context.video.id); @@ -52,10 +73,19 @@ export async function POST(request: NextRequest, { params }: RouteParams) { if (quotaError) return quotaError; } + const reserveResult = await reserveStorageQuota( + billedUserId, + declaredSize.sizeBytes, + BUNNY_RESERVATION_TTL_MS + ); + if ('error' in reserveResult) return reserveResult.error; + const { reservationId } = reserveResult; + const apiKey = process.env.BUNNY_STREAM_API_KEY; const libraryId = process.env.BUNNY_STREAM_LIBRARY_ID || process.env.NEXT_PUBLIC_BUNNY_STREAM_LIBRARY_ID; if (!apiKey || !libraryId) { + await releaseStorageReservation(reservationId, billedUserId); return apiErrors.internalError('Bunny Stream is not configured correctly'); } @@ -70,6 +100,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { }); if (!bunnyRes.ok) { + await releaseStorageReservation(reservationId, billedUserId); logError('Failed to create Bunny Stream video asset', await bunnyRes.text()); return apiErrors.internalError('Failed to initialize Bunny upload'); } @@ -77,6 +108,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { const bunnyVideo = await bunnyRes.json(); const bunnyVideoId = typeof bunnyVideo?.guid === 'string' ? bunnyVideo.guid.trim() : ''; if (!bunnyVideoId || !SAFE_BUNNY_VIDEO_ID.test(bunnyVideoId)) { + await releaseStorageReservation(reservationId, billedUserId); return apiErrors.internalError('Upload provider did not return a valid video identifier'); } @@ -92,15 +124,24 @@ export async function POST(request: NextRequest, { params }: RouteParams) { userId: context.viewerUserId, projectId: context.video.projectId, videoId: bunnyVideoId, + reservationId, }, 3600 ); } else { const expectedContext = deriveGuestUploadContext(request, shareSession?.token ?? null); if (!expectedContext) { + await releaseStorageReservation(reservationId, billedUserId); return apiErrors.forbidden('Missing trusted client IP header'); } + // No reservation id in the guest grant, so a guest cancelling waits out the + // two hours instead of getting the quota back at once. The guest token is + // bound to our own video id and the caller's network context, not to the + // Bunny video being uploaded, so a released-on-request reservation could be + // dropped while the upload it stands for carried on. Guests are capped at + // four of these per quarter hour, which bounds what the wait can cost. + uploadToken = createGuestUploadToken( { projectId: context.video.projectId, @@ -171,6 +212,19 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { } } + if (context.viewerUserId) { + // Safe on the caller's say-so because the reservation id is signed into the + // same token as this Bunny video id: releasing it costs them the video. + await releaseStorageReservation( + readBunnyUploadGrant(uploadToken, { + userId: context.viewerUserId, + projectId: context.video.projectId, + videoId: bunnyVideoId, + })?.reservationId ?? null, + context.video.project.workspace.ownerId + ); + } + await cleanupBunnyStreamVideos([{ providerId: 'bunny', videoId: bunnyVideoId }]); const response = successResponse({ message: 'Pending upload cleaned up' }); return withCacheControl(response, 'private, no-store'); diff --git a/app/api/videos/[videoId]/assets/route.ts b/app/api/videos/[videoId]/assets/route.ts index 0ee35cc..e6b5b44 100644 --- a/app/api/videos/[videoId]/assets/route.ts +++ b/app/api/videos/[videoId]/assets/route.ts @@ -6,7 +6,7 @@ import { apiErrors, successResponse, withCacheControl } from '@/lib/api-response import { rateLimit } from '@/lib/rate-limit'; import { db } from '@/lib/db'; import { r2Client, R2_BUCKET_NAME } from '@/lib/r2'; -import { verifyBunnyUploadToken } from '@/lib/bunny-upload-token'; +import { readBunnyUploadGrant } from '@/lib/bunny-upload-token'; import { deriveGuestUploadContext, verifyGuestUploadToken } from '@/lib/guest-upload-token'; import { ensureGuestIdentityFromRequest, setGuestIdentityCookie } from '@/lib/guest-identity'; import { getShareSessionFromRequest } from '@/lib/share-session'; @@ -32,7 +32,7 @@ import { enforceStorageQuota, reserveStorageQuota, releaseStorageReservation, - PLAN_STORAGE_LIMIT_BYTES, + getStorageLimitForUser, } from '@/lib/storage-quota'; import { getCachedUserBunnyStorage } from '@/lib/admin-stats'; import { isStripeFeatureEnabled } from '@/lib/feature-flags'; @@ -494,14 +494,21 @@ export async function POST(request: NextRequest, { params }: RouteParams) { } if (context.viewerUserId) { - const isValidUploadToken = verifyBunnyUploadToken(uploadToken, { + const grant = readBunnyUploadGrant(uploadToken, { userId: context.viewerUserId, projectId: context.video.projectId, videoId: providerVideoId, }); - if (!isValidUploadToken) { + if (!grant) { return apiErrors.forbidden('Invalid Bunny upload token'); } + + // Charged from now on the size the upload was admitted on: Bunny reports + // nothing until it has finished encoding, and an asset that reads as zero + // bytes for an hour is an hour of uploads measured against a total that + // does not include it. + assetSizeBytes = grant.declaredSizeBytes ?? BigInt(0); + reservationId = grant.reservationId; } else { const shareSession = getShareSessionFromRequest(request, context.video.id); const expectedContext = deriveGuestUploadContext(request, shareSession?.token ?? null); @@ -529,7 +536,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { } kind = 'VIDEO'; - const quotaError = await enforceStorageQuota(billedUserId, BigInt(0)); + const quotaError = await enforceStorageQuota(billedUserId, assetSizeBytes); if (quotaError) return quotaError; } @@ -545,6 +552,12 @@ export async function POST(request: NextRequest, { params }: RouteParams) { ? await getCachedUserBunnyStorage() : null; + // The ceiling this account is actually held to, read for the fallback below. + // It used to compare against the plan limit, which is 200 GiB whoever is + // asking: a caller who quoted a reservation id that no longer existed was + // measured against the paid ceiling even on a trial worth 3 GiB. + const storageLimitBytes = await getStorageLimitForUser(billedUserId); + // Create the VideoAsset and atomically consume the upload reservation (if any) // so the spot is never double-counted. const created = await db.$transaction(async (tx) => { @@ -585,7 +598,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { (r2Row?.total ?? BigInt(0)) + (resRow?.total ?? BigInt(0)) + BigInt(bunnyData[billedUserId] ?? 0); - if (isStripeFeatureEnabled() && totalUsed + assetSizeBytes >= PLAN_STORAGE_LIMIT_BYTES) { + if (isStripeFeatureEnabled() && totalUsed + assetSizeBytes >= storageLimitBytes) { throw new QuotaExceededInTxError(); } } diff --git a/components/video-page/assets-pane.tsx b/components/video-page/assets-pane.tsx index a2acab3..c2b50c0 100644 --- a/components/video-page/assets-pane.tsx +++ b/components/video-page/assets-pane.tsx @@ -509,7 +509,10 @@ export const AssetsPane = memo(function AssetsPane({ const initRes = await fetch(`/api/videos/${videoId}/assets/bunny-init`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ title: bunnyTitle.trim() || file.name.replace(/\.[^/.]+$/, '') }), + body: JSON.stringify({ + title: bunnyTitle.trim() || file.name.replace(/\.[^/.]+$/, ''), + sizeBytes: file.size.toString(), + }), }); const initPayload = (await initRes.json().catch(() => null)) as { data?: { diff --git a/components/video-page/hooks/use-version-actions.ts b/components/video-page/hooks/use-version-actions.ts index ae8de97..f24ee1c 100644 --- a/components/video-page/hooks/use-version-actions.ts +++ b/components/video-page/hooks/use-version-actions.ts @@ -110,7 +110,7 @@ export function useVersionActions({ const initRes = await fetch(`/api/projects/${projectId}/videos/bunny-init`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ title }), + body: JSON.stringify({ title, sizeBytes: file.size.toString() }), }); if (!initRes.ok) throw new Error('Failed to initialize upload'); diff --git a/lib/admin-stats.ts b/lib/admin-stats.ts index 897ee51..39955f0 100644 --- a/lib/admin-stats.ts +++ b/lib/admin-stats.ts @@ -233,6 +233,8 @@ export const getCachedUserBunnyStorage = unstable_cache( where: { providerId: 'bunny' }, select: { videoId: true, + // What the uploader declared, used as a floor below. + sizeBytes: true, video: { select: { project: { @@ -255,10 +257,28 @@ export const getCachedUserBunnyStorage = unstable_cache( select: { providerVideoId: true, billedUserId: true, + sizeBytes: true, }, }), ]); + /** + * What this video costs us, as the larger of the two numbers we have. + * + * Bunny reports nothing for a video until it has finished encoding it, + * which on a half-hour source is most of an hour, and reading that zero + * literally meant an upload was free for as long as it was being + * processed: it did not show on the uploader's storage page and it did not + * count against the next upload's quota check. The size declared when the + * upload was admitted stands in until Bunny has a figure of its own, and + * Bunny's wins once it arrives, because the renditions it makes are the + * real bill and they are larger than the source. + */ + const chargeableSize = (reported: number, declared: bigint | null): number => { + const declaredBytes = declared === null ? 0 : Number(declared); + return reported > declaredBytes ? reported : declaredBytes; + }; + const seenVideoIds = new Set(); for (const version of bunnyVersions) { const ownerId = version.video.project.workspace.ownerId; @@ -266,7 +286,7 @@ export const getCachedUserBunnyStorage = unstable_cache( if (seenVideoIds.has(dedupeKey)) continue; seenVideoIds.add(dedupeKey); - const size = bunnyStats.byVideoId[version.videoId] || 0; + const size = chargeableSize(bunnyStats.byVideoId[version.videoId] || 0, version.sizeBytes); perUserStorage[ownerId] = (perUserStorage[ownerId] || 0) + size; } @@ -277,7 +297,10 @@ export const getCachedUserBunnyStorage = unstable_cache( if (seenVideoIds.has(dedupeKey)) continue; seenVideoIds.add(dedupeKey); - const size = bunnyStats.byVideoId[asset.providerVideoId] || 0; + const size = chargeableSize( + bunnyStats.byVideoId[asset.providerVideoId] || 0, + asset.sizeBytes + ); perUserStorage[billedUserId] = (perUserStorage[billedUserId] || 0) + size; } } catch (err) { diff --git a/lib/bunny-upload-token.ts b/lib/bunny-upload-token.ts index d83f54c..14f1273 100644 --- a/lib/bunny-upload-token.ts +++ b/lib/bunny-upload-token.ts @@ -10,6 +10,27 @@ interface BunnyUploadTokenPayload { vid: string; iat: number; exp: number; + /** + * The storage reservation this upload holds, when it holds one. + * + * Carried inside the signature rather than handed to the client as its own + * field, because a reservation id the caller can name is a reservation the + * caller can drop: it would take two inits and one cancel to release the + * quota of an upload that is still running, which is the exact hole the + * reservation exists to close. Signed alongside `vid`, releasing it means + * presenting the token for that video, which also deletes that video. + */ + rid?: string; + /** + * The size the client declared when it asked for this grant, as a decimal + * string because JSON has no integer wide enough. + * + * Signed for the same reason as the reservation: it is written onto the row + * the upload creates and counted as storage until Bunny reports a figure of + * its own, so a client that could restate it at that point would be declaring + * one size to pass the quota check and another to be billed for. + */ + sz?: string; } interface BunnyUploadTokenSubject { @@ -41,12 +62,17 @@ function isValidPayload(value: unknown): value is BunnyUploadTokenPayload { typeof payload.iat === 'number' && Number.isFinite(payload.iat) && typeof payload.exp === 'number' && - Number.isFinite(payload.exp) + Number.isFinite(payload.exp) && + (payload.rid === undefined || typeof payload.rid === 'string') && + (payload.sz === undefined || typeof payload.sz === 'string') ); } export function createBunnyUploadToken( - subject: BunnyUploadTokenSubject, + subject: BunnyUploadTokenSubject & { + reservationId?: string | null; + declaredSizeBytes?: bigint | null; + }, ttlSeconds = DEFAULT_TOKEN_TTL_SECONDS ): string { const now = Math.floor(Date.now() / 1000); @@ -57,6 +83,8 @@ export function createBunnyUploadToken( vid: subject.videoId, iat: now, exp: now + ttlSeconds, + ...(subject.reservationId ? { rid: subject.reservationId } : {}), + ...(subject.declaredSizeBytes ? { sz: subject.declaredSizeBytes.toString() } : {}), }; const encodedPayload = Buffer.from(JSON.stringify(payload), 'utf8').toString('base64url'); @@ -64,40 +92,91 @@ export function createBunnyUploadToken( return `${encodedPayload}.${signature}`; } -export function verifyBunnyUploadToken(token: string, subject: BunnyUploadTokenSubject): boolean { +/** + * The verified payload, or null when the token is not a genuine grant for this + * subject. Everything `verifyBunnyUploadToken` promises holds here too; it is + * the same check, returning what it read instead of throwing it away. + */ +function readBunnyUploadToken( + token: string, + subject: BunnyUploadTokenSubject +): BunnyUploadTokenPayload | null { // Resolved before the try. A missing signing secret is a configuration fault, and // swallowing that throw made every upload grant look like a forgery instead. const secret = getBunnyUploadTokenSecret(); try { const parts = token.split('.'); - if (parts.length !== 2) return false; + if (parts.length !== 2) return null; const [encodedPayload, providedSignature] = parts; - if (!encodedPayload || !providedSignature) return false; + if (!encodedPayload || !providedSignature) return null; const expectedSignature = signPayload(encodedPayload, secret); const providedBuffer = Buffer.from(providedSignature, 'utf8'); const expectedBuffer = Buffer.from(expectedSignature, 'utf8'); - if (providedBuffer.length !== expectedBuffer.length) return false; - if (!crypto.timingSafeEqual(providedBuffer, expectedBuffer)) return false; + if (providedBuffer.length !== expectedBuffer.length) return null; + if (!crypto.timingSafeEqual(providedBuffer, expectedBuffer)) return null; const payloadJson = Buffer.from(encodedPayload, 'base64url').toString('utf8'); const payloadUnknown: unknown = JSON.parse(payloadJson); - if (!isValidPayload(payloadUnknown)) return false; + if (!isValidPayload(payloadUnknown)) return null; const payload = payloadUnknown; const now = Math.floor(Date.now() / 1000); - if (payload.exp < now) return false; + if (payload.exp < now) return null; - return ( - payload.uid === subject.userId && - payload.pid === subject.projectId && - payload.vid === subject.videoId - ); + if ( + payload.uid !== subject.userId || + payload.pid !== subject.projectId || + payload.vid !== subject.videoId + ) { + return null; + } + + return payload; } catch { - return false; + return null; } } + +export function verifyBunnyUploadToken(token: string, subject: BunnyUploadTokenSubject): boolean { + return readBunnyUploadToken(token, subject) !== null; +} + +export interface BunnyUploadGrant { + /** The storage reservation this upload holds, or null if it holds none. */ + reservationId: string | null; + /** What the client said it was uploading, or null on a grant that predates the claim. */ + declaredSizeBytes: bigint | null; +} + +/** + * What a genuine grant for this subject carries, or null when the token is not + * one. + * + * Null and empty fields mean the same thing to every caller: there is nothing + * here to release and nothing to bill, so a grant issued before either claim + * existed keeps working rather than failing an upload in flight. + */ +export function readBunnyUploadGrant( + token: string, + subject: BunnyUploadTokenSubject +): BunnyUploadGrant | null { + const payload = readBunnyUploadToken(token, subject); + if (!payload) return null; + + let declaredSizeBytes: bigint | null = null; + if (payload.sz) { + try { + const parsed = BigInt(payload.sz); + declaredSizeBytes = parsed > BigInt(0) ? parsed : null; + } catch { + declaredSizeBytes = null; + } + } + + return { reservationId: payload.rid ?? null, declaredSizeBytes }; +} diff --git a/lib/client/project-video-upload.ts b/lib/client/project-video-upload.ts index b335081..bc93ba7 100644 --- a/lib/client/project-video-upload.ts +++ b/lib/client/project-video-upload.ts @@ -154,7 +154,9 @@ export async function uploadProjectVideo( const initResponse = await fetch(`/api/projects/${projectId}/videos/bunny-init`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ title }), + // The server checks this against the quota and holds a reservation for it, + // so an upload that cannot fit is turned away before any of it is sent. + body: JSON.stringify({ title, sizeBytes: file.size.toString() }), }); const initPayload = (await initResponse.json().catch(() => null)) as { diff --git a/lib/storage-quota.ts b/lib/storage-quota.ts index f8e8255..8fca0ac 100644 --- a/lib/storage-quota.ts +++ b/lib/storage-quota.ts @@ -17,7 +17,7 @@ export const PLAN_STORAGE_LIMIT_BYTES = BigInt(200) * BigInt(1024) * BigInt(1024 * directly rather than taking a flag from the caller, so no upload route can * forget to pass it. */ -async function getStorageLimitForUser(userId: string): Promise { +export async function getStorageLimitForUser(userId: string): Promise { const user = await db.user.findUnique({ where: { id: userId }, select: { subscriptionStatus: true, stripeCurrentPeriodEnd: true }, diff --git a/lib/upload-size.ts b/lib/upload-size.ts new file mode 100644 index 0000000..ab5b4b8 --- /dev/null +++ b/lib/upload-size.ts @@ -0,0 +1,41 @@ +// The size a client declares before a direct upload starts. +// +// Shared because the R2 and Bunny paths have to agree on it: both hand the +// number to the storage quota before a single byte moves, so a value one of them +// would accept and the other would not is a hole in whichever is laxer. + +export type DeclaredUploadSize = { sizeBytes: bigint } | { error: string }; + +/** + * Reads a declared upload size, refusing anything that is not a whole positive + * number of bytes within the host's per-file ceiling. + * + * The number is the client's word and is treated as such. Overstating it only + * spends the caller's own quota, and understating it is caught where the bytes + * land: R2 compares the object against the declaration and deletes it on a + * mismatch, and Bunny's own storage reporting replaces the estimate once the + * upload settles. What is not tolerated is an absent or nonsense value, which is + * what asking for zero bytes effectively was. + */ +export function parseDeclaredUploadSize(raw: unknown, maxBytes: bigint): DeclaredUploadSize { + if (typeof raw !== 'number' && typeof raw !== 'string' && typeof raw !== 'bigint') { + return { error: 'sizeBytes must be a positive integer' }; + } + + let sizeBytes: bigint; + try { + sizeBytes = BigInt(raw); + } catch { + return { error: 'sizeBytes must be a positive integer' }; + } + + if (sizeBytes <= BigInt(0)) { + return { error: 'sizeBytes must be a positive integer' }; + } + + if (sizeBytes > maxBytes) { + return { error: 'File exceeds the maximum allowed upload size' }; + } + + return { sizeBytes }; +} diff --git a/tests/api/bunny-upload-reservation.test.ts b/tests/api/bunny-upload-reservation.test.ts new file mode 100644 index 0000000..f0e988a --- /dev/null +++ b/tests/api/bunny-upload-reservation.test.ts @@ -0,0 +1,179 @@ +// The Bunny init routes used to ask the quota about zero bytes, which meant two +// things at once: an upload that could never fit was only discovered after it had +// been sent, and nothing an upload was about to consume was visible to the next +// request. Bunny reports its own storage on a delay and the figure is cached for +// two minutes on top, so every init inside that window read the same stale total +// and every one of them passed. +// +// These tests pin the two halves of the fix: the declared size is checked before +// a byte moves, and it is held as a reservation the next init has to see. + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { db } from '@/lib/db'; +import { + DELETE as cancelProjectBunnyUpload, + POST as initProjectBunnyUpload, +} from '@/app/api/projects/[projectId]/videos/bunny-init/route'; +import { apiRequest, callRoute, readData, readError } from '../helpers/request'; +import { signedInAs } from '../helpers/session'; +import { seedProject } from '../factories'; + +const GIB = BigInt(1024) * BigInt(1024) * BigInt(1024); + +/** + * Bunny answers every call the same way, with a fresh video id each time so two + * inits in one test are distinguishable. The cancel path talks to Bunny too, so + * the stub has to cover it rather than just the creation call. + */ +function stubBunnyApi(): void { + let created = 0; + vi.stubGlobal( + 'fetch', + vi.fn(async () => { + created += 1; + return new Response(JSON.stringify({ guid: `bunnyvideo-${created}-abcdefgh` }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); + }) + ); +} + +beforeEach(() => { + vi.stubEnv('OPENFRAME_ENABLE_STRIPE', 'true'); + vi.stubEnv('OPENFRAME_ENABLE_BUNNY_UPLOADS', 'true'); + vi.stubEnv('OPENFRAME_ENABLE_S3_VIDEO_UPLOADS', 'false'); + vi.stubEnv('BUNNY_STREAM_API_KEY', 'test-bunny-key'); + vi.stubEnv('BUNNY_STREAM_LIBRARY_ID', '999999'); + vi.stubEnv('BUNNY_UPLOAD_TOKEN_SECRET', 'test-bunny-upload-token-secret'); + stubBunnyApi(); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +function initRequest(projectId: string, body: Record) { + return apiRequest(`/api/projects/${projectId}/videos/bunny-init`, { body }); +} + +async function initUpload(projectId: string, sizeBytes: bigint) { + return callRoute( + initProjectBunnyUpload, + initRequest(projectId, { title: 'A clip', sizeBytes: sizeBytes.toString() }), + { projectId } + ); +} + +describe('POST /api/projects/[projectId]/videos/bunny-init', () => { + it('refuses an init that does not say how big the upload is', async () => { + const scenario = await seedProject(); + signedInAs(scenario.owner); + + const response = await callRoute( + initProjectBunnyUpload, + initRequest(scenario.project.id, { title: 'A clip' }), + { projectId: scenario.project.id } + ); + + expect(response.status).toBe(400); + expect(await readError(response)).toContain('sizeBytes'); + expect(await db.uploadReservation.count()).toBe(0); + }); + + it('refuses a size beyond the host per-file ceiling', async () => { + vi.stubEnv('OPENFRAME_MAX_VIDEO_UPLOAD_BYTES', '1024'); + const scenario = await seedProject(); + signedInAs(scenario.owner); + + const response = await initUpload(scenario.project.id, BigInt(2048)); + + expect(response.status).toBe(400); + expect(await readError(response)).toContain('maximum allowed upload size'); + }); + + // The trial ceiling is 3 GiB, so this is refused on the way in rather than + // after four gigabytes have been pushed to Bunny. + it('refuses an upload the remaining quota cannot hold', async () => { + const scenario = await seedProject(); + signedInAs(scenario.owner); + + const response = await initUpload(scenario.project.id, BigInt(4) * GIB); + + expect(response.status).toBe(507); + expect(await db.uploadReservation.count()).toBe(0); + }); + + it('holds the declared size as a reservation for the workspace owner', async () => { + const scenario = await seedProject(); + signedInAs(scenario.owner); + + const response = await initUpload(scenario.project.id, BigInt(2) * GIB); + + expect(response.status).toBe(200); + const reservations = await db.uploadReservation.findMany(); + expect(reservations).toHaveLength(1); + expect(reservations[0].billedUserId).toBe(scenario.owner.id); + expect(reservations[0].sizeBytes).toBe(BigInt(2) * GIB); + }); + + // The regression this whole change exists for. Both of these used to be + // granted, because neither could see what the other was about to upload. + it('refuses a second upload that no longer fits beside the first', async () => { + const scenario = await seedProject(); + signedInAs(scenario.owner); + + const first = await initUpload(scenario.project.id, BigInt(2) * GIB); + const second = await initUpload(scenario.project.id, BigInt(2) * GIB); + + expect(first.status).toBe(200); + expect(second.status).toBe(507); + expect(await db.uploadReservation.count()).toBe(1); + }); +}); + +describe('DELETE /api/projects/[projectId]/videos/bunny-init', () => { + it('gives the quota back when a pending upload is cancelled', async () => { + const scenario = await seedProject(); + signedInAs(scenario.owner); + + const init = await initUpload(scenario.project.id, BigInt(2) * GIB); + const { videoId, uploadToken } = await readData(init); + + const response = await callRoute( + cancelProjectBunnyUpload, + initRequest(scenario.project.id, { videoId, uploadToken }), + { projectId: scenario.project.id } + ); + + expect(response.status).toBe(200); + expect(await db.uploadReservation.count()).toBe(0); + }); + + // Why the reservation id travels inside the signed token instead of being + // handed to the client as a field of its own: a caller who could name a + // reservation could start two uploads, cancel the cheap one while quoting the + // expensive one's reservation, and keep uploading against quota it no longer + // holds. + it('will not let one upload cancel release another upload reservation', async () => { + const scenario = await seedProject(); + signedInAs(scenario.owner); + + const first = await initUpload(scenario.project.id, BigInt(1) * GIB); + const second = await initUpload(scenario.project.id, BigInt(1) * GIB); + const firstData = await readData(first); + const secondData = await readData(second); + + const response = await callRoute( + cancelProjectBunnyUpload, + initRequest(scenario.project.id, { + videoId: secondData.videoId, + uploadToken: firstData.uploadToken, + }), + { projectId: scenario.project.id } + ); + + expect(response.status).toBe(403); + expect(await db.uploadReservation.count()).toBe(2); + }); +}); diff --git a/tests/component/hooks/use-version-actions.test.ts b/tests/component/hooks/use-version-actions.test.ts index f8d79e2..db32a36 100644 --- a/tests/component/hooks/use-version-actions.test.ts +++ b/tests/component/hooks/use-version-actions.test.ts @@ -419,7 +419,13 @@ describe('useVersionActions uploading a file to Bunny', () => { await createFromFile(harness); - expect(bodyOf(callsTo(BUNNY_INIT_URL, 'POST')[0])).toEqual({ title: 'my clip' }); + // The size goes with the title: the server checks it against the quota and + // reserves it before Bunny is asked for anything, so an upload that cannot + // fit is refused here rather than after it has been sent. + expect(bodyOf(callsTo(BUNNY_INIT_URL, 'POST')[0])).toEqual({ + title: 'my clip', + sizeBytes: '10', + }); expect(tusUploads[0].options.endpoint).toBe('https://video.bunnycdn.com/tusupload'); expect(tusUploads[0].options.headers).toEqual({ AuthorizationSignature: 'sig', @@ -436,7 +442,10 @@ describe('useVersionActions uploading a file to Bunny', () => { await createFromFile(harness); - expect(bodyOf(callsTo(BUNNY_INIT_URL, 'POST')[0])).toEqual({ title: 'Client cut' }); + expect(bodyOf(callsTo(BUNNY_INIT_URL, 'POST')[0])).toEqual({ + title: 'Client cut', + sizeBytes: '10', + }); }); it('registers the version against the Bunny embed and CDN thumbnail', async () => { diff --git a/tests/unit/lib/upload-size.test.ts b/tests/unit/lib/upload-size.test.ts new file mode 100644 index 0000000..0b6dd6f --- /dev/null +++ b/tests/unit/lib/upload-size.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from 'vitest'; +import { parseDeclaredUploadSize } from '@/lib/upload-size'; + +const MAX = BigInt(5) * BigInt(1024) * BigInt(1024) * BigInt(1024); + +function size(result: ReturnType): bigint | null { + return 'sizeBytes' in result ? result.sizeBytes : null; +} + +describe('parseDeclaredUploadSize', () => { + it('accepts a size sent as a string, which is how a client sends bytes it cannot hold in a number', () => { + expect(size(parseDeclaredUploadSize('4294967296', MAX))).toBe(BigInt(4294967296)); + }); + + it('accepts a size sent as a number', () => { + expect(size(parseDeclaredUploadSize(1024, MAX))).toBe(BigInt(1024)); + }); + + it('rejects a missing size', () => { + expect(parseDeclaredUploadSize(undefined, MAX)).toEqual({ + error: 'sizeBytes must be a positive integer', + }); + }); + + // Zero was the old behaviour of every Bunny init: it asked the quota whether + // it could store nothing, and the answer was always yes. + it('rejects zero', () => { + expect(parseDeclaredUploadSize(0, MAX)).toEqual({ + error: 'sizeBytes must be a positive integer', + }); + }); + + it('rejects a negative size', () => { + expect(parseDeclaredUploadSize(-1, MAX)).toEqual({ + error: 'sizeBytes must be a positive integer', + }); + }); + + it('rejects a fractional size rather than rounding it', () => { + expect(parseDeclaredUploadSize(1.5, MAX)).toEqual({ + error: 'sizeBytes must be a positive integer', + }); + }); + + it('rejects text that is not a number', () => { + expect(parseDeclaredUploadSize('a lot', MAX)).toEqual({ + error: 'sizeBytes must be a positive integer', + }); + }); + + it('rejects a size over the ceiling', () => { + expect(parseDeclaredUploadSize(MAX + BigInt(1), MAX)).toEqual({ + error: 'File exceeds the maximum allowed upload size', + }); + }); + + it('accepts a size exactly at the ceiling', () => { + expect(size(parseDeclaredUploadSize(MAX, MAX))).toBe(MAX); + }); +}); From 00f1d430b8b991355d33d3ea7a2224db5d8fcd40 Mon Sep 17 00:00:00 2001 From: yusufipk Date: Tue, 18 Aug 2026 11:07:18 +0300 Subject: [PATCH 2/3] fix(uploads): stop a storage hold from being dropped by whoever can name it A reservation id was never a secret and could not have been one. An upload token is base64url(payload) followed by its signature, so a client can read every claim out of its own token, and the two R2 init routes hand their reservation ids to the client outright. The asset route takes a reservation id from the request body and deleted it on the strength of that id and the billed user alone, and every hold an account owns is billed to the same user. So a caller could start a Bunny upload, read the id out of the token they were just given, quote it while attaching a one byte image or even a bare YouTube link, and have the quota handed back while the upload carried on. Repeat and a trial worth three gigabytes uploads as much as it likes for as long as Bunny takes to report a figure of its own. Signing the id rather than handing it over bought nothing, because signing is not hiding. A hold now records what it was opened for and is only ever consumed by that flow, so naming one is no longer enough to drop it. Guests hold against the workspace owner's quota rather than their own and had no way to give it back: the release was gated on being signed in. Declaring a size and walking away cost the guest nothing and cost the owner their whole remaining allowance for two hours. The guest grant now carries the reservation and the declared size, bound to the Bunny video as well as to ours, so cancelling gives the quota back and costs them the upload it stood for. What a guest can hold without cancelling lapses in half an hour rather than two hours. The in-transaction fallback check counted the account's Bunny storage as zero on a Bunny upload, because the figure was only prefetched for R2 providers and that branch was unreachable for Bunny until this PR made it reachable. On an account whose storage is all Bunny that was a check that could not fail. It is prefetched for every provider that can reach the fallback now. --- .../videos/[videoId]/versions/route.ts | 8 +- .../[projectId]/videos/bunny-init/route.ts | 36 +++-- .../[projectId]/videos/r2-complete/route.ts | 8 +- .../[projectId]/videos/r2-init/route.ts | 14 +- app/api/projects/[projectId]/videos/route.ts | 8 +- app/api/upload/audio/route.ts | 36 ++++- app/api/upload/image/route.ts | 30 ++++- .../versions/[versionId]/comments/route.ts | 25 +++- .../[videoId]/assets/bunny-init/route.ts | 112 ++++++++++------ .../videos/[videoId]/assets/r2-init/route.ts | 14 +- app/api/videos/[videoId]/assets/route.ts | 82 +++++++++--- lib/guest-upload-token.ts | 126 +++++++++++++++--- lib/storage-quota.ts | 38 +++++- .../migration.sql | 10 ++ prisma/schema.prisma | 4 + tests/api/bunny-upload-reservation.test.ts | 86 +++++++++++- tests/api/lib-r2-upload-session.test.ts | 2 + tests/api/lib-r2-video-finalize.test.ts | 2 + tests/api/storage-quota.test.ts | 96 ++++++++++--- tests/factories/video.ts | 4 + tests/setup/db-global.ts | 1 + tests/unit/lib/guest-upload-token.test.ts | 109 +++++++++++++++ 22 files changed, 721 insertions(+), 130 deletions(-) create mode 100644 prisma/migrations/20260818120000_add_upload_reservation_purpose/migration.sql create mode 100644 tests/unit/lib/guest-upload-token.test.ts diff --git a/app/api/projects/[projectId]/videos/[videoId]/versions/route.ts b/app/api/projects/[projectId]/videos/[videoId]/versions/route.ts index a3ab33c..0ae9105 100644 --- a/app/api/projects/[projectId]/videos/[videoId]/versions/route.ts +++ b/app/api/projects/[projectId]/videos/[videoId]/versions/route.ts @@ -7,6 +7,7 @@ import { notifyProjectOwner } from '@/lib/notifications'; import { apiErrors, successResponse, withCacheControl } from '@/lib/api-response'; import { readBunnyUploadGrant } from '@/lib/bunny-upload-token'; import { finalizeR2VideoUpload } from '@/lib/r2-video-finalize'; +import { UPLOAD_RESERVATION_PURPOSES } from '@/lib/storage-quota'; import { logError } from '@/lib/logger'; type RouteParams = { params: Promise<{ projectId: string; videoId: string }> }; @@ -234,6 +235,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { where: { id: finalizedR2Session.reservationId, billedUserId: finalizedR2Session.billedUserId, + purpose: UPLOAD_RESERVATION_PURPOSES.R2_VIDEO, }, }); } @@ -243,7 +245,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { // are never counted twice and never counted zero times. if (bunnyReservation) { await tx.uploadReservation.deleteMany({ - where: { id: bunnyReservation, billedUserId: video.project.workspace.ownerId }, + where: { + id: bunnyReservation, + billedUserId: video.project.workspace.ownerId, + purpose: UPLOAD_RESERVATION_PURPOSES.BUNNY, + }, }); } diff --git a/app/api/projects/[projectId]/videos/bunny-init/route.ts b/app/api/projects/[projectId]/videos/bunny-init/route.ts index 895a3b9..5a4231d 100644 --- a/app/api/projects/[projectId]/videos/bunny-init/route.ts +++ b/app/api/projects/[projectId]/videos/bunny-init/route.ts @@ -5,17 +5,14 @@ import { apiErrors, successResponse, withCacheControl } from '@/lib/api-response import { rateLimit } from '@/lib/rate-limit'; import crypto from 'crypto'; import { cleanupBunnyStreamVideos } from '@/lib/bunny-stream-cleanup'; -import { - createBunnyUploadToken, - readBunnyUploadGrant, - verifyBunnyUploadToken, -} from '@/lib/bunny-upload-token'; +import { createBunnyUploadToken, readBunnyUploadGrant } from '@/lib/bunny-upload-token'; import { getMaxVideoUploadBytes, isBunnyUploadsEnabled } from '@/lib/feature-flags'; import { logError } from '@/lib/logger'; import { enforceStorageQuota, releaseStorageReservation, reserveStorageQuota, + UPLOAD_RESERVATION_PURPOSES, } from '@/lib/storage-quota'; import { parseDeclaredUploadSize } from '@/lib/upload-size'; @@ -99,6 +96,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { const reserveResult = await reserveStorageQuota( billedUserId, declaredSize.sizeBytes, + UPLOAD_RESERVATION_PURPOSES.BUNNY, BUNNY_RESERVATION_TTL_MS ); if ('error' in reserveResult) return reserveResult.error; @@ -109,7 +107,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { process.env.BUNNY_STREAM_LIBRARY_ID || process.env.NEXT_PUBLIC_BUNNY_STREAM_LIBRARY_ID; if (!apiKey || !libraryId) { - await releaseStorageReservation(reservationId, billedUserId); + await releaseStorageReservation( + reservationId, + billedUserId, + UPLOAD_RESERVATION_PURPOSES.BUNNY + ); return apiErrors.internalError('Bunny Stream is not configured correctly'); } @@ -125,7 +127,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { }); if (!bunnyRes.ok) { - await releaseStorageReservation(reservationId, billedUserId); + await releaseStorageReservation( + reservationId, + billedUserId, + UPLOAD_RESERVATION_PURPOSES.BUNNY + ); logError('Failed to create Bunny Stream video', await bunnyRes.text()); return apiErrors.internalError('Failed to initialize video upload with provider'); } @@ -133,7 +139,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { const bunnyVideo = await bunnyRes.json(); const videoId = bunnyVideo.guid; if (typeof videoId !== 'string' || videoId.length === 0) { - await releaseStorageReservation(reservationId, billedUserId); + await releaseStorageReservation( + reservationId, + billedUserId, + UPLOAD_RESERVATION_PURPOSES.BUNNY + ); return apiErrors.internalError('Upload provider did not return a valid video identifier'); } @@ -197,12 +207,12 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { return apiErrors.badRequest('videoId and uploadToken are required'); } - const isValidUploadToken = verifyBunnyUploadToken(uploadToken, { + const grant = readBunnyUploadGrant(uploadToken, { userId: session.user.id, projectId, videoId, }); - if (!isValidUploadToken) { + if (!grant) { return apiErrors.forbidden('Invalid Bunny upload token'); } @@ -213,9 +223,9 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { // inside the signed token next to this video id, so releasing it costs the // caller the video it belongs to. await releaseStorageReservation( - readBunnyUploadGrant(uploadToken, { userId: session.user.id, projectId, videoId }) - ?.reservationId ?? null, - project.workspace.ownerId + grant.reservationId, + project.workspace.ownerId, + UPLOAD_RESERVATION_PURPOSES.BUNNY ); await cleanupBunnyStreamVideos([{ providerId: 'bunny', videoId }]); diff --git a/app/api/projects/[projectId]/videos/r2-complete/route.ts b/app/api/projects/[projectId]/videos/r2-complete/route.ts index d9d416a..648a071 100644 --- a/app/api/projects/[projectId]/videos/r2-complete/route.ts +++ b/app/api/projects/[projectId]/videos/r2-complete/route.ts @@ -7,7 +7,7 @@ import { parseR2UploadToken, verifyR2UploadToken } from '@/lib/r2-upload-token'; import { abortMultipartVideoUpload, completeMultipartVideoUpload } from '@/lib/r2'; import { isS3VideoUploadsEnabled } from '@/lib/feature-flags'; import { objectKeyToVideoProxyPath } from '@/lib/video-upload-validation'; -import { releaseStorageReservation } from '@/lib/storage-quota'; +import { releaseStorageReservation, UPLOAD_RESERVATION_PURPOSES } from '@/lib/storage-quota'; import { logError } from '@/lib/logger'; type RouteParams = { params: Promise<{ projectId: string }> }; @@ -155,7 +155,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { where: { id: uploadSession.id, status: 'INITIATED' }, data: { status: 'CANCELLED', consumedAt: new Date() }, }); - await releaseStorageReservation(uploadSession.reservationId, uploadSession.billedUserId); + await releaseStorageReservation( + uploadSession.reservationId, + uploadSession.billedUserId, + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); return apiErrors.internalError('Failed to complete multipart upload'); } diff --git a/app/api/projects/[projectId]/videos/r2-init/route.ts b/app/api/projects/[projectId]/videos/r2-init/route.ts index 1adaf78..88bc7af 100644 --- a/app/api/projects/[projectId]/videos/r2-init/route.ts +++ b/app/api/projects/[projectId]/videos/r2-init/route.ts @@ -35,6 +35,7 @@ import { enforceStorageQuota, releaseStorageReservation, reserveStorageQuota, + UPLOAD_RESERVATION_PURPOSES, } from '@/lib/storage-quota'; import { createR2UploadSession } from '@/lib/r2-upload-session'; @@ -129,6 +130,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { const reserveResult = await reserveStorageQuota( project.workspace.ownerId, sizeBytes + THUMBNAIL_RESERVE_BYTES, + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO, VIDEO_RESERVATION_TTL_MS ); if ('error' in reserveResult) return reserveResult.error; @@ -188,7 +190,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { ]); } } catch (error) { - await releaseStorageReservation(reserveResult.reservationId, project.workspace.ownerId); + await releaseStorageReservation( + reserveResult.reservationId, + project.workspace.ownerId, + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); logError('Failed to create presigned video upload URL:', error); return apiErrors.internalError('Failed to initialize video upload'); } @@ -340,7 +346,11 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { logError('Failed to delete pending R2 video object:', error); } - await releaseStorageReservation(uploadSession.reservationId, uploadSession.billedUserId); + await releaseStorageReservation( + uploadSession.reservationId, + uploadSession.billedUserId, + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); const response = successResponse({ message: 'Pending upload cleaned up' }); return withCacheControl(response, 'private, no-store'); diff --git a/app/api/projects/[projectId]/videos/route.ts b/app/api/projects/[projectId]/videos/route.ts index 5954779..92b468c 100644 --- a/app/api/projects/[projectId]/videos/route.ts +++ b/app/api/projects/[projectId]/videos/route.ts @@ -7,6 +7,7 @@ import { notifyProjectOwner } from '@/lib/notifications'; import { apiErrors, successResponse, withCacheControl } from '@/lib/api-response'; import { readBunnyUploadGrant } from '@/lib/bunny-upload-token'; import { finalizeR2VideoUpload } from '@/lib/r2-video-finalize'; +import { UPLOAD_RESERVATION_PURPOSES } from '@/lib/storage-quota'; import { logError } from '@/lib/logger'; import { eventKey, recordEvent } from '@/lib/analytics/record'; @@ -237,6 +238,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { where: { id: finalizedR2Session.reservationId, billedUserId: finalizedR2Session.billedUserId, + purpose: UPLOAD_RESERVATION_PURPOSES.R2_VIDEO, }, }); } @@ -246,7 +248,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { // counted twice and never counted zero times. if (bunnyReservation) { await tx.uploadReservation.deleteMany({ - where: { id: bunnyReservation, billedUserId: project.workspace.ownerId }, + where: { + id: bunnyReservation, + billedUserId: project.workspace.ownerId, + purpose: UPLOAD_RESERVATION_PURPOSES.BUNNY, + }, }); } diff --git a/app/api/upload/audio/route.ts b/app/api/upload/audio/route.ts index 2e3fcf6..f2e6c59 100644 --- a/app/api/upload/audio/route.ts +++ b/app/api/upload/audio/route.ts @@ -13,7 +13,11 @@ import { enforceGuestUploadQuota, verifyGuestUploadToken, } from '@/lib/guest-upload-token'; -import { reserveStorageQuota, releaseStorageReservation } from '@/lib/storage-quota'; +import { + reserveStorageQuota, + releaseStorageReservation, + UPLOAD_RESERVATION_PURPOSES, +} from '@/lib/storage-quota'; import { logError } from '@/lib/logger'; const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB @@ -205,7 +209,11 @@ export async function POST(request: NextRequest) { // All paths use the advisory-locked reservation so concurrent uploads always // see each other's in-flight sizes, eliminating the TOCTOU race. const workspaceOwnerId = video.project.workspace.ownerId; - const reserveResult = await reserveStorageQuota(workspaceOwnerId, BigInt(file.size)); + const reserveResult = await reserveStorageQuota( + workspaceOwnerId, + BigInt(file.size), + UPLOAD_RESERVATION_PURPOSES.AUDIO + ); if ('error' in reserveResult) return reserveResult.error; const reservationId = reserveResult.reservationId; @@ -214,7 +222,11 @@ export async function POST(request: NextRequest) { const strippedType = rawContentType.split(';')[0].trim().toLowerCase(); const contentType = MIME_ALIASES[strippedType] ?? strippedType; if (!ALLOWED_TYPES.has(contentType)) { - await releaseStorageReservation(reservationId); + await releaseStorageReservation( + reservationId, + workspaceOwnerId, + UPLOAD_RESERVATION_PURPOSES.AUDIO + ); return apiErrors.badRequest(`Unsupported audio format: ${rawContentType}`); } @@ -231,12 +243,20 @@ export async function POST(request: NextRequest) { // Validate file content against magic bytes — rejects HTML/scripts masquerading as audio if (isHtmlContent(buffer)) { - await releaseStorageReservation(reservationId); + await releaseStorageReservation( + reservationId, + workspaceOwnerId, + UPLOAD_RESERVATION_PURPOSES.AUDIO + ); return apiErrors.badRequest('File content does not match an audio format'); } const hasValidMagicBytes = hasValidAudioMagicBytes(buffer.slice(0, 16), contentType); if (!hasValidMagicBytes) { - await releaseStorageReservation(reservationId); + await releaseStorageReservation( + reservationId, + workspaceOwnerId, + UPLOAD_RESERVATION_PURPOSES.AUDIO + ); return apiErrors.badRequest('File content does not match the declared audio format'); } @@ -251,7 +271,11 @@ export async function POST(request: NextRequest) { }) ); } catch (uploadError) { - await releaseStorageReservation(reservationId); + await releaseStorageReservation( + reservationId, + workspaceOwnerId, + UPLOAD_RESERVATION_PURPOSES.AUDIO + ); throw uploadError; } diff --git a/app/api/upload/image/route.ts b/app/api/upload/image/route.ts index e9dc912..8a46d6d 100644 --- a/app/api/upload/image/route.ts +++ b/app/api/upload/image/route.ts @@ -20,7 +20,11 @@ import { verifyGuestUploadToken, } from '@/lib/guest-upload-token'; import { logError } from '@/lib/logger'; -import { reserveStorageQuota, releaseStorageReservation } from '@/lib/storage-quota'; +import { + reserveStorageQuota, + releaseStorageReservation, + UPLOAD_RESERVATION_PURPOSES, +} from '@/lib/storage-quota'; const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB const MAX_MULTIPART_BODY_SIZE = MAX_FILE_SIZE + 512 * 1024; // file + multipart overhead @@ -137,14 +141,22 @@ export async function POST(request: NextRequest) { // All paths use the advisory-locked reservation so concurrent uploads always // see each other's in-flight sizes, eliminating the TOCTOU race. const workspaceOwnerId = video.project.workspace.ownerId; - const reserveResult = await reserveStorageQuota(workspaceOwnerId, BigInt(file.size)); + const reserveResult = await reserveStorageQuota( + workspaceOwnerId, + BigInt(file.size), + UPLOAD_RESERVATION_PURPOSES.IMAGE + ); if ('error' in reserveResult) return reserveResult.error; const reservationId = reserveResult.reservationId; // Check content type const normalizedMime = normalizeImageMime(file.type); if (normalizedMime && !isAllowedImageType(normalizedMime)) { - await releaseStorageReservation(reservationId); + await releaseStorageReservation( + reservationId, + workspaceOwnerId, + UPLOAD_RESERVATION_PURPOSES.IMAGE + ); return apiErrors.badRequest(`Unsupported image format: ${file.type}`); } @@ -153,7 +165,11 @@ export async function POST(request: NextRequest) { const buffer = Buffer.from(arrayBuffer); const detectedMime = detectImageMime(buffer); if (!detectedMime) { - await releaseStorageReservation(reservationId); + await releaseStorageReservation( + reservationId, + workspaceOwnerId, + UPLOAD_RESERVATION_PURPOSES.IMAGE + ); return apiErrors.badRequest('Uploaded file content does not match an allowed image type'); } @@ -173,7 +189,11 @@ export async function POST(request: NextRequest) { }) ); } catch (uploadError) { - await releaseStorageReservation(reservationId); + await releaseStorageReservation( + reservationId, + workspaceOwnerId, + UPLOAD_RESERVATION_PURPOSES.IMAGE + ); throw uploadError; } diff --git a/app/api/versions/[versionId]/comments/route.ts b/app/api/versions/[versionId]/comments/route.ts index b236f1b..5e81a92 100644 --- a/app/api/versions/[versionId]/comments/route.ts +++ b/app/api/versions/[versionId]/comments/route.ts @@ -21,7 +21,11 @@ import { } from '@/lib/video-assets'; import { validateAnnotationStrokes } from '@/lib/validation'; import { logError } from '@/lib/logger'; -import { reserveStorageQuota, releaseStorageReservation } from '@/lib/storage-quota'; +import { + reserveStorageQuota, + releaseStorageReservation, + UPLOAD_RESERVATION_PURPOSES, +} from '@/lib/storage-quota'; import { isValidEmailAddress, normalizeEmail } from '@/lib/email-validation'; type RouteParams = { params: Promise<{ versionId: string }> }; @@ -202,6 +206,9 @@ export async function GET(request: NextRequest, { params }: RouteParams) { // POST /api/versions/[versionId]/comments export async function POST(request: NextRequest, { params }: RouteParams) { let attachmentReservationId: string | null = null; + // Carried out of the try so the catch below can scope the release to the + // account the hold was opened against. + let attachmentBilledUserId: string | null = null; try { const limited = await rateLimit(request, 'comment'); if (limited) return limited; @@ -416,10 +423,12 @@ export async function POST(request: NextRequest, { params }: RouteParams) { if (totalAttachmentBytes > BigInt(0)) { const reserveResult = await reserveStorageQuota( project.workspace.ownerId, - totalAttachmentBytes + totalAttachmentBytes, + UPLOAD_RESERVATION_PURPOSES.ATTACHMENT ); if ('error' in reserveResult) return reserveResult.error; attachmentReservationId = reserveResult.reservationId; + attachmentBilledUserId = project.workspace.ownerId; } // Use a transaction to create both the comment and any asset rows atomically. @@ -427,7 +436,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { const result = await db.$transaction(async (tx) => { if (attachmentReservationId) { await tx.uploadReservation.deleteMany({ - where: { id: attachmentReservationId, billedUserId: project.workspace.ownerId }, + where: { + id: attachmentReservationId, + billedUserId: project.workspace.ownerId, + purpose: UPLOAD_RESERVATION_PURPOSES.ATTACHMENT, + }, }); } const comment = await tx.comment.create({ @@ -586,7 +599,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { } return withCacheControl(response, 'private, no-store'); } catch (error) { - await releaseStorageReservation(attachmentReservationId); + await releaseStorageReservation( + attachmentReservationId, + attachmentBilledUserId, + UPLOAD_RESERVATION_PURPOSES.ATTACHMENT + ); logError('Error creating comment:', error); return apiErrors.internalError('Failed to create comment'); } diff --git a/app/api/videos/[videoId]/assets/bunny-init/route.ts b/app/api/videos/[videoId]/assets/bunny-init/route.ts index b666e28..da10aab 100644 --- a/app/api/videos/[videoId]/assets/bunny-init/route.ts +++ b/app/api/videos/[videoId]/assets/bunny-init/route.ts @@ -5,14 +5,15 @@ import { rateLimit } from '@/lib/rate-limit'; import { createBunnyUploadToken, readBunnyUploadGrant, - verifyBunnyUploadToken, + type BunnyUploadGrant, } from '@/lib/bunny-upload-token'; import { cleanupBunnyStreamVideos } from '@/lib/bunny-stream-cleanup'; import { createGuestUploadToken, deriveGuestUploadContext, enforceGuestUploadQuota, - verifyGuestUploadToken, + readGuestUploadGrant, + type GuestUploadGrant, } from '@/lib/guest-upload-token'; import { getMaxVideoUploadBytes, isBunnyUploadsEnabled } from '@/lib/feature-flags'; import { getShareSessionFromRequest } from '@/lib/share-session'; @@ -22,6 +23,7 @@ import { enforceStorageQuota, releaseStorageReservation, reserveStorageQuota, + UPLOAD_RESERVATION_PURPOSES, } from '@/lib/storage-quota'; import { parseDeclaredUploadSize } from '@/lib/upload-size'; @@ -31,6 +33,20 @@ type RouteParams = { params: Promise<{ videoId: string }> }; // Bunny's own reporting delay. const BUNNY_RESERVATION_TTL_MS = 2 * 60 * 60 * 1000; +/** + * A guest's hold lapses sooner than a member's. + * + * A guest is whoever opened the share link, and the hold is written against the + * workspace owner's quota rather than their own. Declaring a size and then + * walking away costs the guest nothing and costs the owner their whole remaining + * allowance, which on a trial is the entire account. Half an hour is the same + * window the R2 attachment paths already accept, and it bounds what a guest who + * never uploads can take away. A guest whose upload outruns it loses only the + * concurrency guard for the tail of the transfer; the bytes are still recorded + * from the signed size when the asset is created. + */ +const GUEST_BUNNY_RESERVATION_TTL_MS = 30 * 60 * 1000; + // POST /api/videos/[videoId]/assets/bunny-init export async function POST(request: NextRequest, { params }: RouteParams) { try { @@ -76,7 +92,8 @@ export async function POST(request: NextRequest, { params }: RouteParams) { const reserveResult = await reserveStorageQuota( billedUserId, declaredSize.sizeBytes, - BUNNY_RESERVATION_TTL_MS + UPLOAD_RESERVATION_PURPOSES.BUNNY, + context.viewerUserId ? BUNNY_RESERVATION_TTL_MS : GUEST_BUNNY_RESERVATION_TTL_MS ); if ('error' in reserveResult) return reserveResult.error; const { reservationId } = reserveResult; @@ -85,7 +102,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { const libraryId = process.env.BUNNY_STREAM_LIBRARY_ID || process.env.NEXT_PUBLIC_BUNNY_STREAM_LIBRARY_ID; if (!apiKey || !libraryId) { - await releaseStorageReservation(reservationId, billedUserId); + await releaseStorageReservation( + reservationId, + billedUserId, + UPLOAD_RESERVATION_PURPOSES.BUNNY + ); return apiErrors.internalError('Bunny Stream is not configured correctly'); } @@ -100,7 +121,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { }); if (!bunnyRes.ok) { - await releaseStorageReservation(reservationId, billedUserId); + await releaseStorageReservation( + reservationId, + billedUserId, + UPLOAD_RESERVATION_PURPOSES.BUNNY + ); logError('Failed to create Bunny Stream video asset', await bunnyRes.text()); return apiErrors.internalError('Failed to initialize Bunny upload'); } @@ -108,7 +133,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { const bunnyVideo = await bunnyRes.json(); const bunnyVideoId = typeof bunnyVideo?.guid === 'string' ? bunnyVideo.guid.trim() : ''; if (!bunnyVideoId || !SAFE_BUNNY_VIDEO_ID.test(bunnyVideoId)) { - await releaseStorageReservation(reservationId, billedUserId); + await releaseStorageReservation( + reservationId, + billedUserId, + UPLOAD_RESERVATION_PURPOSES.BUNNY + ); return apiErrors.internalError('Upload provider did not return a valid video identifier'); } @@ -125,29 +154,35 @@ export async function POST(request: NextRequest, { params }: RouteParams) { projectId: context.video.projectId, videoId: bunnyVideoId, reservationId, + declaredSizeBytes: declaredSize.sizeBytes, }, 3600 ); } else { const expectedContext = deriveGuestUploadContext(request, shareSession?.token ?? null); if (!expectedContext) { - await releaseStorageReservation(reservationId, billedUserId); + await releaseStorageReservation( + reservationId, + billedUserId, + UPLOAD_RESERVATION_PURPOSES.BUNNY + ); return apiErrors.forbidden('Missing trusted client IP header'); } - // No reservation id in the guest grant, so a guest cancelling waits out the - // two hours instead of getting the quota back at once. The guest token is - // bound to our own video id and the caller's network context, not to the - // Bunny video being uploaded, so a released-on-request reservation could be - // dropped while the upload it stands for carried on. Guests are capped at - // four of these per quarter hour, which bounds what the wait can cost. - + // The guest grant carries the same three claims the signed-in one does, + // and is bound to the Bunny video as well as to ours. That binding is what + // makes releasing safe on the guest's say-so: presenting this token to + // cancel deletes the upload it stands for, so it cannot be used to drop the + // hold while the transfer carries on. uploadToken = createGuestUploadToken( { projectId: context.video.projectId, videoId: context.video.id, intent: 'bunny', context: expectedContext, + providerVideoId: bunnyVideoId, + reservationId, + declaredSizeBytes: declaredSize.sizeBytes, }, 3600 ); @@ -185,15 +220,18 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { return apiErrors.badRequest('videoId and uploadToken are required'); } + // Both grants are read rather than merely checked, because both carry the + // reservation this upload holds. Releasing on the caller's say-so is safe + // only because the id is signed next to this Bunny video id: presenting the + // token costs them the video, which is deleted immediately below. + let grant: BunnyUploadGrant | GuestUploadGrant | null = null; + if (context.viewerUserId) { - const isValidUploadToken = verifyBunnyUploadToken(uploadToken, { + grant = readBunnyUploadGrant(uploadToken, { userId: context.viewerUserId, projectId: context.video.projectId, videoId: bunnyVideoId, }); - if (!isValidUploadToken) { - return apiErrors.forbidden('Invalid Bunny upload token'); - } } else { const shareSession = getShareSessionFromRequest(request, context.video.id); const expectedContext = deriveGuestUploadContext(request, shareSession?.token ?? null); @@ -201,30 +239,28 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { return apiErrors.forbidden('Missing trusted client IP header'); } - const isValidUploadToken = verifyGuestUploadToken(uploadToken, { - projectId: context.video.projectId, - videoId: context.video.id, - intent: 'bunny', - context: expectedContext, - }); - if (!isValidUploadToken) { - return apiErrors.forbidden('Invalid Bunny upload token'); - } - } - - if (context.viewerUserId) { - // Safe on the caller's say-so because the reservation id is signed into the - // same token as this Bunny video id: releasing it costs them the video. - await releaseStorageReservation( - readBunnyUploadGrant(uploadToken, { - userId: context.viewerUserId, + grant = readGuestUploadGrant( + uploadToken, + { projectId: context.video.projectId, - videoId: bunnyVideoId, - })?.reservationId ?? null, - context.video.project.workspace.ownerId + videoId: context.video.id, + intent: 'bunny', + context: expectedContext, + }, + bunnyVideoId ); } + if (!grant) { + return apiErrors.forbidden('Invalid Bunny upload token'); + } + + await releaseStorageReservation( + grant.reservationId, + context.video.project.workspace.ownerId, + UPLOAD_RESERVATION_PURPOSES.BUNNY + ); + await cleanupBunnyStreamVideos([{ providerId: 'bunny', videoId: bunnyVideoId }]); const response = successResponse({ message: 'Pending upload cleaned up' }); return withCacheControl(response, 'private, no-store'); diff --git a/app/api/videos/[videoId]/assets/r2-init/route.ts b/app/api/videos/[videoId]/assets/r2-init/route.ts index 1f2ff05..899bac2 100644 --- a/app/api/videos/[videoId]/assets/r2-init/route.ts +++ b/app/api/videos/[videoId]/assets/r2-init/route.ts @@ -26,6 +26,7 @@ import { enforceStorageQuota, releaseStorageReservation, reserveStorageQuota, + UPLOAD_RESERVATION_PURPOSES, } from '@/lib/storage-quota'; import { createR2UploadSession } from '@/lib/r2-upload-session'; import { getVideoAssetAccessContext } from '@/lib/video-assets'; @@ -97,6 +98,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { const reserveResult = await reserveStorageQuota( billedUserId, sizeBytes + THUMBNAIL_RESERVE_BYTES, + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO, VIDEO_RESERVATION_TTL_MS ); if ('error' in reserveResult) return reserveResult.error; @@ -117,7 +119,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { createPresignedImagePutUrl(thumbnailObjectKey, 'image/jpeg'), ]); } catch (error) { - await releaseStorageReservation(reserveResult.reservationId, billedUserId); + await releaseStorageReservation( + reserveResult.reservationId, + billedUserId, + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); logError('Failed to create presigned asset video upload URL:', error); return apiErrors.internalError('Failed to initialize video upload'); } @@ -261,7 +267,11 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { logError('Failed to delete pending R2 asset video object:', error); } - await releaseStorageReservation(uploadSession.reservationId, uploadSession.billedUserId); + await releaseStorageReservation( + uploadSession.reservationId, + uploadSession.billedUserId, + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); const response = successResponse({ message: 'Pending upload cleaned up' }); return withCacheControl(response, 'private, no-store'); diff --git a/app/api/videos/[videoId]/assets/route.ts b/app/api/videos/[videoId]/assets/route.ts index e6b5b44..4573b54 100644 --- a/app/api/videos/[videoId]/assets/route.ts +++ b/app/api/videos/[videoId]/assets/route.ts @@ -7,7 +7,7 @@ import { rateLimit } from '@/lib/rate-limit'; import { db } from '@/lib/db'; import { r2Client, R2_BUCKET_NAME } from '@/lib/r2'; import { readBunnyUploadGrant } from '@/lib/bunny-upload-token'; -import { deriveGuestUploadContext, verifyGuestUploadToken } from '@/lib/guest-upload-token'; +import { deriveGuestUploadContext, readGuestUploadGrant } from '@/lib/guest-upload-token'; import { ensureGuestIdentityFromRequest, setGuestIdentityCookie } from '@/lib/guest-identity'; import { getShareSessionFromRequest } from '@/lib/share-session'; import { validateUrl, validateOptionalUrl } from '@/lib/validation'; @@ -33,6 +33,8 @@ import { reserveStorageQuota, releaseStorageReservation, getStorageLimitForUser, + UPLOAD_RESERVATION_PURPOSES, + type UploadReservationPurpose, } from '@/lib/storage-quota'; import { getCachedUserBunnyStorage } from '@/lib/admin-stats'; import { isStripeFeatureEnabled } from '@/lib/feature-flags'; @@ -298,6 +300,13 @@ export async function GET(request: NextRequest, { params }: RouteParams) { // POST /api/videos/[videoId]/assets export async function POST(request: NextRequest, { params }: RouteParams) { let reservationId: string | null = null; + // What the reservation above was opened for, and who it is billed to. A hold is + // only ever consumed by the flow that opened it: the id below can arrive in the + // request body, and every hold an account owns is billed to the same user, so + // the id alone would let an image being attached release a video upload that + // was still in flight. + let reservationPurpose: UploadReservationPurpose | null = null; + let reservationBilledUserId: string | null = null; let finalizedR2AssetSession: { sessionId: string; reservationId: string | null; @@ -342,6 +351,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { let assetSizeBytes = BigInt(0); const billedUserId = context.video.project.workspace.ownerId; + reservationBilledUserId = billedUserId; if (provider === VideoAssetProvider.R2_IMAGE) { sourceUrl = typeof body?.sourceUrl === 'string' ? body.sourceUrl.trim() : ''; @@ -359,8 +369,13 @@ export async function POST(request: NextRequest, { params }: RouteParams) { // the client already supplied a reservationId (new upload flow) the // existing reservation is consumed in the transaction below. For the // backward-compat path (no reservationId) we create one here. + reservationPurpose = UPLOAD_RESERVATION_PURPOSES.IMAGE; if (!reservationId) { - const reserveResult = await reserveStorageQuota(billedUserId, assetSizeBytes); + const reserveResult = await reserveStorageQuota( + billedUserId, + assetSizeBytes, + UPLOAD_RESERVATION_PURPOSES.IMAGE + ); if ('error' in reserveResult) return reserveResult.error; reservationId = reserveResult.reservationId; } @@ -383,8 +398,13 @@ export async function POST(request: NextRequest, { params }: RouteParams) { assetSizeBytes = audioCheck.sizeBytes; // Same reservation logic as R2_IMAGE above + reservationPurpose = UPLOAD_RESERVATION_PURPOSES.AUDIO; if (!reservationId) { - const reserveResult = await reserveStorageQuota(billedUserId, assetSizeBytes); + const reserveResult = await reserveStorageQuota( + billedUserId, + assetSizeBytes, + UPLOAD_RESERVATION_PURPOSES.AUDIO + ); if ('error' in reserveResult) return reserveResult.error; reservationId = reserveResult.reservationId; } @@ -450,6 +470,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { assetSizeBytes = finalizeResult.sizeBytes; reservationId = finalizeResult.reservationId; + reservationPurpose = UPLOAD_RESERVATION_PURPOSES.R2_VIDEO; if (!thumbnailUrl) { thumbnailUrl = finalizeResult.thumbnailProxyUrl; } @@ -509,6 +530,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { // does not include it. assetSizeBytes = grant.declaredSizeBytes ?? BigInt(0); reservationId = grant.reservationId; + reservationPurpose = UPLOAD_RESERVATION_PURPOSES.BUNNY; } else { const shareSession = getShareSessionFromRequest(request, context.video.id); const expectedContext = deriveGuestUploadContext(request, shareSession?.token ?? null); @@ -516,15 +538,28 @@ export async function POST(request: NextRequest, { params }: RouteParams) { return apiErrors.forbidden('Missing trusted client IP header'); } - const isValidGuestUploadToken = verifyGuestUploadToken(uploadToken, { - projectId: context.video.projectId, - videoId: context.video.id, - intent: 'bunny', - context: expectedContext, - }); - if (!isValidGuestUploadToken) { + // Read rather than merely verified, for the same reason as above: a guest + // upload that reads as zero bytes until Bunny finishes encoding is an hour + // of the owner's quota spent on nothing. The grant is bound to this Bunny + // video, so the size and the hold it names belong to this upload and no + // other. + const guestGrant = readGuestUploadGrant( + uploadToken, + { + projectId: context.video.projectId, + videoId: context.video.id, + intent: 'bunny', + context: expectedContext, + }, + providerVideoId + ); + if (!guestGrant) { return apiErrors.forbidden('Invalid Bunny upload token'); } + + assetSizeBytes = guestGrant.declaredSizeBytes ?? BigInt(0); + reservationId = guestGrant.reservationId; + reservationPurpose = UPLOAD_RESERVATION_PURPOSES.BUNNY; } displayName = sanitizeAssetDisplayName(requestedDisplayName, `Bunny ${providerVideoId}`); @@ -543,14 +578,12 @@ export async function POST(request: NextRequest, { params }: RouteParams) { // Pre-fetch Bunny storage BEFORE entering the transaction to avoid making an // HTTP call while holding a DB connection open (connection-pool exhaustion // risk under adversarial load). Mirrors the discipline in reserveStorageQuota. - // Only needed for R2 providers where the invalid-reservation fallback quota - // check requires Bunny usage data. + // Needed by every provider that can reach the invalid-reservation fallback + // quota check below, Bunny included. Leaving Bunny out read its own storage as + // zero, and on an account whose storage is all Bunny that made the fallback a + // check that could not fail. const preFetchedBunnyData = - provider === VideoAssetProvider.R2_IMAGE || - provider === VideoAssetProvider.R2_AUDIO || - provider === VideoAssetProvider.R2_VIDEO - ? await getCachedUserBunnyStorage() - : null; + provider === VideoAssetProvider.YOUTUBE ? null : await getCachedUserBunnyStorage(); // The ceiling this account is actually held to, read for the fallback below. // It used to compare against the plan limit, which is 200 GiB whoever is @@ -561,7 +594,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { // Create the VideoAsset and atomically consume the upload reservation (if any) // so the spot is never double-counted. const created = await db.$transaction(async (tx) => { - if (reservationId) { + if (reservationId && reservationPurpose) { // Acquire the per-user advisory lock unconditionally so both the happy path // (valid reservation) and the fallback path (fake/expired reservation ID) are // serialised — eliminating the TOCTOU race in the deleted.count === 0 branch. @@ -575,7 +608,12 @@ export async function POST(request: NextRequest, { params }: RouteParams) { // we fall back to a standard (non-locked) quota check so the bypass attempt // is caught rather than silently allowed. const deleted = await tx.uploadReservation.deleteMany({ - where: { id: reservationId, billedUserId, expiresAt: { gt: new Date() } }, + where: { + id: reservationId, + billedUserId, + purpose: reservationPurpose, + expiresAt: { gt: new Date() }, + }, }); if (deleted.count === 0) { // Reservation didn't exist — enforce quota the normal way inside the tx. @@ -676,7 +714,11 @@ export async function POST(request: NextRequest, { params }: RouteParams) { if (error instanceof QuotaExceededInTxError) { return apiErrors.storageExceeded() as NextResponse; } - await releaseStorageReservation(reservationId); + await releaseStorageReservation( + reservationId, + reservationBilledUserId, + reservationPurpose ?? undefined + ); logError('Error creating video asset:', error); return apiErrors.internalError('Failed to create asset'); } diff --git a/lib/guest-upload-token.ts b/lib/guest-upload-token.ts index 0ba9f55..b1336b0 100644 --- a/lib/guest-upload-token.ts +++ b/lib/guest-upload-token.ts @@ -20,6 +20,25 @@ interface GuestUploadTokenPayload { exp: number; intent: GuestUploadIntent; ctx: string; + /** + * The provider's own id for the video this grant was issued against. + * + * `vid` is our video, the one the asset will hang off. That is not enough to + * hand a guest the right to release a storage hold: the hold stands for one + * particular upload, and a grant that names only our video would let a guest + * drop it while the upload it stands for carried on. Binding the provider's id + * makes releasing cost the guest the upload itself, which is the same bargain + * the signed-in path already makes. + */ + bvid?: string; + /** The storage reservation this upload holds, when it holds one. */ + rid?: string; + /** + * The size the client declared when it asked for this grant, as a decimal + * string because JSON has no integer wide enough. Signed so a guest cannot + * declare one size to pass the quota check and another to be billed for. + */ + sz?: string; } interface GuestUploadTokenSubject { @@ -29,6 +48,20 @@ interface GuestUploadTokenSubject { context: string; } +interface GuestUploadTokenClaims { + /** The provider video id to bind this grant to. */ + providerVideoId?: string | null; + reservationId?: string | null; + declaredSizeBytes?: bigint | null; +} + +export interface GuestUploadGrant { + /** The storage reservation this upload holds, or null if it holds none. */ + reservationId: string | null; + /** What the client said it was uploading, or null on a grant that predates the claim. */ + declaredSizeBytes: bigint | null; +} + const TRUSTED_IP_PATTERN = /^[\da-fA-F.:]+$/; function getGuestUploadTokenSecret(): string { @@ -77,7 +110,10 @@ function isValidPayload(value: unknown): value is GuestUploadTokenPayload { typeof payload.exp === 'number' && Number.isFinite(payload.exp) && (payload.intent === 'audio' || payload.intent === 'image' || payload.intent === 'bunny') && - typeof payload.ctx === 'string' + typeof payload.ctx === 'string' && + (payload.bvid === undefined || typeof payload.bvid === 'string') && + (payload.rid === undefined || typeof payload.rid === 'string') && + (payload.sz === undefined || typeof payload.sz === 'string') ); } @@ -95,7 +131,7 @@ export function deriveGuestUploadContext( } export function createGuestUploadToken( - subject: GuestUploadTokenSubject, + subject: GuestUploadTokenSubject & GuestUploadTokenClaims, ttlSeconds = DEFAULT_GUEST_UPLOAD_TOKEN_TTL_SECONDS ): string { const now = Math.floor(Date.now() / 1000); @@ -107,6 +143,9 @@ export function createGuestUploadToken( exp: now + ttlSeconds, intent: subject.intent, ctx: subject.context, + ...(subject.providerVideoId ? { bvid: subject.providerVideoId } : {}), + ...(subject.reservationId ? { rid: subject.reservationId } : {}), + ...(subject.declaredSizeBytes ? { sz: subject.declaredSizeBytes.toString() } : {}), }; const encodedPayload = Buffer.from(JSON.stringify(payload), 'utf8').toString('base64url'); @@ -114,39 +153,94 @@ export function createGuestUploadToken( return `${encodedPayload}.${signature}`; } -export function verifyGuestUploadToken(token: string, subject: GuestUploadTokenSubject): boolean { +/** + * The verified payload, or null when the token is not a genuine grant for this + * subject. + * + * `providerVideoId` is checked only when the grant carries one, so a token + * issued before that claim existed keeps working rather than failing an upload + * in flight. A grant that does carry one and does not match is a forgery as far + * as this is concerned. + */ +function readGuestUploadToken( + token: string, + subject: GuestUploadTokenSubject, + providerVideoId?: string | null +): GuestUploadTokenPayload | null { try { const parts = token.split('.'); - if (parts.length !== 2) return false; + if (parts.length !== 2) return null; const [encodedPayload, providedSignature] = parts; - if (!encodedPayload || !providedSignature) return false; + if (!encodedPayload || !providedSignature) return null; const expectedSignature = signPayload(encodedPayload); const providedBuffer = Buffer.from(providedSignature, 'utf8'); const expectedBuffer = Buffer.from(expectedSignature, 'utf8'); - if (providedBuffer.length !== expectedBuffer.length) return false; - if (!timingSafeEqual(providedBuffer, expectedBuffer)) return false; + if (providedBuffer.length !== expectedBuffer.length) return null; + if (!timingSafeEqual(providedBuffer, expectedBuffer)) return null; const payloadRaw = Buffer.from(encodedPayload, 'base64url').toString('utf8'); const payloadUnknown: unknown = JSON.parse(payloadRaw); - if (!isValidPayload(payloadUnknown)) return false; + if (!isValidPayload(payloadUnknown)) return null; const payload = payloadUnknown; const now = Math.floor(Date.now() / 1000); - if (payload.exp < now) return false; + if (payload.exp < now) return null; - return ( - payload.pid === subject.projectId && - payload.vid === subject.videoId && - payload.intent === subject.intent && - payload.ctx === subject.context - ); + if ( + payload.pid !== subject.projectId || + payload.vid !== subject.videoId || + payload.intent !== subject.intent || + payload.ctx !== subject.context + ) { + return null; + } + + if (payload.bvid !== undefined && payload.bvid !== providerVideoId) return null; + + return payload; } catch { - return false; + return null; } } +export function verifyGuestUploadToken( + token: string, + subject: GuestUploadTokenSubject, + providerVideoId?: string | null +): boolean { + return readGuestUploadToken(token, subject, providerVideoId) !== null; +} + +/** + * What a genuine guest grant for this subject carries, or null when the token is + * not one. + * + * Null and empty fields mean the same thing to every caller: there is nothing + * here to release and nothing to bill. + */ +export function readGuestUploadGrant( + token: string, + subject: GuestUploadTokenSubject, + providerVideoId?: string | null +): GuestUploadGrant | null { + const payload = readGuestUploadToken(token, subject, providerVideoId); + if (!payload) return null; + + let declaredSizeBytes: bigint | null = null; + if (payload.sz) { + try { + const parsed = BigInt(payload.sz); + declaredSizeBytes = parsed > BigInt(0) ? parsed : null; + } catch { + declaredSizeBytes = null; + } + } + + return { reservationId: payload.rid ?? null, declaredSizeBytes }; +} + export async function enforceGuestUploadQuota( request: Request, videoId: string, diff --git a/lib/storage-quota.ts b/lib/storage-quota.ts index 8fca0ac..c01d82b 100644 --- a/lib/storage-quota.ts +++ b/lib/storage-quota.ts @@ -29,6 +29,33 @@ export async function getStorageLimitForUser(userId: string): Promise { // TTL for upload reservations: 30 minutes is enough for R2 image/audio uploads const RESERVATION_TTL_MS = 30 * 60 * 1000; +/** + * What a hold was opened for. + * + * A reservation is only ever consumed by the flow that opened it, and the + * finalize routes match on this as well as on the id. Without it, naming a + * reservation would be enough to drop it: the asset route takes a reservation id + * from the request body, and every hold an account owns is billed to the same + * user, so an image being attached could quietly release a video upload that was + * still in flight. The ids are not secret. Two of them are handed to the client + * outright, and the rest ride inside signed-but-readable token payloads. + */ +export const UPLOAD_RESERVATION_PURPOSES = { + /** A comment attachment or standalone image going to R2. */ + IMAGE: 'IMAGE', + /** A voice note going to R2. */ + AUDIO: 'AUDIO', + /** Image and voice attachments weighed together when a comment is posted. */ + ATTACHMENT: 'ATTACHMENT', + /** A presigned direct upload to our own S3-compatible storage. */ + R2_VIDEO: 'R2_VIDEO', + /** A direct upload to Bunny, where the bytes never pass through us. */ + BUNNY: 'BUNNY', +} as const; + +export type UploadReservationPurpose = + (typeof UPLOAD_RESERVATION_PURPOSES)[keyof typeof UPLOAD_RESERVATION_PURPOSES]; + // Sentinel error thrown inside a Prisma transaction to signal quota exceeded class QuotaExceededError extends Error {} @@ -137,6 +164,7 @@ export async function enforceStorageQuota( export async function reserveStorageQuota( userId: string, incomingSizeBytes: bigint, + purpose: UploadReservationPurpose, reservationTtlMs: number = RESERVATION_TTL_MS ): Promise<{ reservationId: string | null } | { error: NextResponse }> { if (!isStripeFeatureEnabled()) { @@ -199,7 +227,7 @@ export async function reserveStorageQuota( } const reservation = await tx.uploadReservation.create({ - data: { billedUserId: userId, sizeBytes: incomingSizeBytes, expiresAt }, + data: { billedUserId: userId, sizeBytes: incomingSizeBytes, expiresAt, purpose }, select: { id: true }, }); @@ -218,16 +246,22 @@ export async function reserveStorageQuota( /** * Deletes an upload reservation created by `reserveStorageQuota`. * Safe to call with `null` (no-op) for flows where billing is disabled. + * + * Pass the purpose wherever the caller knows it. A release that names only an id + * will delete a hold opened for something else, which is the same hole the + * purpose column exists to close. */ export async function releaseStorageReservation( reservationId: string | null, - billedUserId?: string | null + billedUserId?: string | null, + purpose?: UploadReservationPurpose ): Promise { if (!reservationId) return; await db.uploadReservation.deleteMany({ where: { id: reservationId, ...(billedUserId ? { billedUserId } : {}), + ...(purpose ? { purpose } : {}), }, }); } diff --git a/prisma/migrations/20260818120000_add_upload_reservation_purpose/migration.sql b/prisma/migrations/20260818120000_add_upload_reservation_purpose/migration.sql new file mode 100644 index 0000000..04f11a3 --- /dev/null +++ b/prisma/migrations/20260818120000_add_upload_reservation_purpose/migration.sql @@ -0,0 +1,10 @@ +-- AlterTable: record what each hold was opened for, so a reservation can only be +-- consumed by the flow that opened it. Without this a caller who could name a +-- reservation id could drop any of their own holds through whichever finalize +-- route was cheapest, which defeats the point of holding one at all. +-- +-- Rows that predate this column get 'LEGACY', which matches no finalize route. +-- Those uploads fall through to the in-transaction quota check instead, and the +-- rows lapse on their own TTL within the hour. +ALTER TABLE "upload_reservations" ADD COLUMN "purpose" TEXT NOT NULL DEFAULT 'LEGACY'; +ALTER TABLE "upload_reservations" ALTER COLUMN "purpose" DROP DEFAULT; diff --git a/prisma/schema.prisma b/prisma/schema.prisma index b1ccefb..2c4a869 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -703,6 +703,10 @@ model UploadReservation { sizeBytes BigInt expiresAt DateTime createdAt DateTime @default(now()) + /// What this hold was opened for. A reservation can only be consumed by the + /// flow that opened it, so naming one is not enough to drop it. See + /// UPLOAD_RESERVATION_PURPOSES in lib/storage-quota.ts. + purpose String @@index([billedUserId, expiresAt]) @@map("upload_reservations") diff --git a/tests/api/bunny-upload-reservation.test.ts b/tests/api/bunny-upload-reservation.test.ts index f0e988a..6d36bc8 100644 --- a/tests/api/bunny-upload-reservation.test.ts +++ b/tests/api/bunny-upload-reservation.test.ts @@ -14,9 +14,11 @@ import { DELETE as cancelProjectBunnyUpload, POST as initProjectBunnyUpload, } from '@/app/api/projects/[projectId]/videos/bunny-init/route'; +import { POST as createAsset } from '@/app/api/videos/[videoId]/assets/route'; +import { UPLOAD_RESERVATION_PURPOSES } from '@/lib/storage-quota'; import { apiRequest, callRoute, readData, readError } from '../helpers/request'; import { signedInAs } from '../helpers/session'; -import { seedProject } from '../factories'; +import { createVideo, seedProject } from '../factories'; const GIB = BigInt(1024) * BigInt(1024) * BigInt(1024); @@ -177,3 +179,85 @@ describe('DELETE /api/projects/[projectId]/videos/bunny-init', () => { expect(await db.uploadReservation.count()).toBe(2); }); }); + +// The reservation id is not a secret and was never going to be one. The upload +// token is `base64url(payload).signature`, so the client can read every claim in +// it, and the two R2 upload routes hand their reservation ids to the client +// outright. What keeps a hold from being dropped by whoever can name it is that +// a reservation records what it was opened for, and every finalize route matches +// on that as well as on the id. +describe('a Bunny hold cannot be consumed by another flow', () => { + /** The claims the client can read out of an upload token without our help. */ + function claimsOf(uploadToken: string): Record { + return JSON.parse(Buffer.from(uploadToken.split('.')[0], 'base64url').toString('utf8')); + } + + it('puts the reservation id somewhere the client can read it', async () => { + const scenario = await seedProject(); + signedInAs(scenario.owner); + + const init = await initUpload(scenario.project.id, BigInt(1) * GIB); + const { uploadToken } = await readData(init); + + const reservation = (await db.uploadReservation.findMany())[0]; + expect(claimsOf(uploadToken).rid).toBe(reservation.id); + expect(reservation.purpose).toBe(UPLOAD_RESERVATION_PURPOSES.BUNNY); + }); + + // The attack the purpose column closes. Creating a YouTube asset costs nothing + // and consumes no storage, so quoting a Bunny reservation on one was a way to + // hand back the quota of an upload that was still running and then start + // another. Repeat and a trial worth three gigabytes uploads as much as it + // likes for as long as Bunny takes to report. + it('ignores a Bunny reservation id quoted on a YouTube asset create', async () => { + const scenario = await seedProject(); + const video = await createVideo({ projectId: scenario.project.id }); + signedInAs(scenario.owner); + + const init = await initUpload(scenario.project.id, BigInt(2) * GIB); + const { uploadToken } = await readData(init); + const reservationId = claimsOf(uploadToken).rid as string; + + const response = await callRoute( + createAsset, + apiRequest(`/api/videos/${video.id}/assets`, { + body: { + provider: 'YOUTUBE', + sourceUrl: 'https://www.youtube.com/watch?v=dQw4w9WgXcQ', + reservationId, + }, + }), + { videoId: video.id } + ); + + expect(response.status).toBe(201); + const reservations = await db.uploadReservation.findMany(); + expect(reservations).toHaveLength(1); + expect(reservations[0].id).toBe(reservationId); + }); + + // And with the hold still standing, the next init has to see it. + it('still refuses the next upload after the quoted release attempt', async () => { + const scenario = await seedProject(); + const video = await createVideo({ projectId: scenario.project.id }); + signedInAs(scenario.owner); + + const init = await initUpload(scenario.project.id, BigInt(2) * GIB); + const { uploadToken } = await readData(init); + + await callRoute( + createAsset, + apiRequest(`/api/videos/${video.id}/assets`, { + body: { + provider: 'YOUTUBE', + sourceUrl: 'https://www.youtube.com/watch?v=dQw4w9WgXcQ', + reservationId: claimsOf(uploadToken).rid, + }, + }), + { videoId: video.id } + ); + + const second = await initUpload(scenario.project.id, BigInt(2) * GIB); + expect(second.status).toBe(507); + }); +}); diff --git a/tests/api/lib-r2-upload-session.test.ts b/tests/api/lib-r2-upload-session.test.ts index 758f3d6..e049315 100644 --- a/tests/api/lib-r2-upload-session.test.ts +++ b/tests/api/lib-r2-upload-session.test.ts @@ -11,6 +11,7 @@ import { describe, expect, it } from 'vitest'; import { randomUUID } from 'crypto'; import { db } from '@/lib/db'; +import { UPLOAD_RESERVATION_PURPOSES } from '@/lib/storage-quota'; import { cancelR2UploadSession, createR2UploadSession } from '@/lib/r2-upload-session'; import { seedProject } from '../factories'; @@ -101,6 +102,7 @@ describe('createR2UploadSession', () => { data: { billedUserId: scenario.owner.id, sizeBytes: BigInt(4096), + purpose: UPLOAD_RESERVATION_PURPOSES.R2_VIDEO, expiresAt: new Date(Date.now() + HOUR_MS), }, }); diff --git a/tests/api/lib-r2-video-finalize.test.ts b/tests/api/lib-r2-video-finalize.test.ts index c6151ba..5115f55 100644 --- a/tests/api/lib-r2-video-finalize.test.ts +++ b/tests/api/lib-r2-video-finalize.test.ts @@ -16,6 +16,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { randomUUID } from 'crypto'; import { db } from '@/lib/db'; +import { UPLOAD_RESERVATION_PURPOSES } from '@/lib/storage-quota'; import { deleteR2Object, deleteVideoObject, headVideoObject, readVideoObjectBytes } from '@/lib/r2'; import { createR2UploadToken } from '@/lib/r2-upload-token'; import { createR2UploadSession } from '@/lib/r2-upload-session'; @@ -515,6 +516,7 @@ describe('finalizeR2VideoUpload success', () => { data: { billedUserId: scenario.owner.id, sizeBytes: BigInt(4096), + purpose: UPLOAD_RESERVATION_PURPOSES.R2_VIDEO, expiresAt: new Date(Date.now() + 30 * 60 * 1000), }, }); diff --git a/tests/api/storage-quota.test.ts b/tests/api/storage-quota.test.ts index d65357f..916f24d 100644 --- a/tests/api/storage-quota.test.ts +++ b/tests/api/storage-quota.test.ts @@ -15,6 +15,7 @@ import { db } from '@/lib/db'; import { getCachedUserBunnyStorage } from '@/lib/admin-stats'; import { PLAN_STORAGE_LIMIT_BYTES, + UPLOAD_RESERVATION_PURPOSES, enforceStorageQuota, getUserStorageInfo, getUserTotalStorageBytes, @@ -83,7 +84,11 @@ describe('the trial ceiling', () => { const user = await createUser(); await createUploadReservation({ billedUserId: user.id, sizeBytes: BigInt(2) * GIB }); - const result = await reserveStorageQuota(user.id, BigInt(2) * GIB); + const result = await reserveStorageQuota( + user.id, + BigInt(2) * GIB, + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); expect('error' in result).toBe(true); expect((result as { error: Response }).error.status).toBe(507); @@ -92,7 +97,11 @@ describe('the trial ceiling', () => { it('still lets a trial account upload inside its own ceiling', async () => { const user = await createUser(); - const result = await reserveStorageQuota(user.id, BigInt(1) * GIB); + const result = await reserveStorageQuota( + user.id, + BigInt(1) * GIB, + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); expect('reservationId' in result).toBe(true); }); @@ -277,7 +286,11 @@ describe('reserveStorageQuota', () => { it('writes a reservation row billed to the user with the requested size', async () => { const user = await createSubscribedUser(); - const result = await reserveStorageQuota(user.id, BigInt(4096)); + const result = await reserveStorageQuota( + user.id, + BigInt(4096), + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); expect('reservationId' in result).toBe(true); const reservation = await db.uploadReservation.findFirstOrThrow(); @@ -291,7 +304,11 @@ describe('reserveStorageQuota', () => { vi.stubEnv('OPENFRAME_ENABLE_STRIPE', 'false'); const user = await createSubscribedUser(); - const result = await reserveStorageQuota(user.id, BigInt(4096)); + const result = await reserveStorageQuota( + user.id, + BigInt(4096), + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); expect(result).toEqual({ reservationId: null }); expect(await db.uploadReservation.count()).toBe(0); @@ -304,7 +321,11 @@ describe('reserveStorageQuota', () => { sizeBytes: PLAN_STORAGE_LIMIT_BYTES - BigInt(1024), }); - const result = await reserveStorageQuota(user.id, BigInt(2048)); + const result = await reserveStorageQuota( + user.id, + BigInt(2048), + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); expect('error' in result).toBe(true); expect('error' in result && result.error.status).toBe(507); @@ -321,7 +342,11 @@ describe('reserveStorageQuota', () => { sizeBytes: PLAN_STORAGE_LIMIT_BYTES - BigInt(100), }); - const result = await reserveStorageQuota(scenario.owner.id, BigInt(200)); + const result = await reserveStorageQuota( + scenario.owner.id, + BigInt(200), + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); expect('error' in result).toBe(true); expect(await db.uploadReservation.count()).toBe(0); @@ -331,7 +356,11 @@ describe('reserveStorageQuota', () => { const user = await createSubscribedUser(); bunnyStorage({ [user.id]: Number(PLAN_STORAGE_LIMIT_BYTES - BigInt(1024)) }); - const result = await reserveStorageQuota(user.id, BigInt(2048)); + const result = await reserveStorageQuota( + user.id, + BigInt(2048), + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); expect('error' in result).toBe(true); expect(await db.uploadReservation.count()).toBe(0); @@ -345,7 +374,11 @@ describe('reserveStorageQuota', () => { expiresInMs: -60_000, }); - const result = await reserveStorageQuota(user.id, BigInt(2048)); + const result = await reserveStorageQuota( + user.id, + BigInt(2048), + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); expect('reservationId' in result).toBe(true); }); @@ -358,7 +391,11 @@ describe('reserveStorageQuota', () => { sizeBytes: PLAN_STORAGE_LIMIT_BYTES - BigInt(1024), }); - const result = await reserveStorageQuota(light.id, BigInt(10) * GIB); + const result = await reserveStorageQuota( + light.id, + BigInt(10) * GIB, + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); expect('reservationId' in result).toBe(true); }); @@ -375,8 +412,8 @@ describe('reserveStorageQuota', () => { expect(headroom(used)).toBe(BigInt(30) * GIB); const [first, second] = await Promise.all([ - reserveStorageQuota(user.id, request), - reserveStorageQuota(user.id, request), + reserveStorageQuota(user.id, request, UPLOAD_RESERVATION_PURPOSES.R2_VIDEO), + reserveStorageQuota(user.id, request, UPLOAD_RESERVATION_PURPOSES.R2_VIDEO), ]); const granted = [first, second].filter((result) => 'reservationId' in result); @@ -401,8 +438,8 @@ describe('reserveStorageQuota', () => { const request = BigInt(20) * GIB; const results = await Promise.all([ - reserveStorageQuota(user.id, request), - reserveStorageQuota(user.id, request), + reserveStorageQuota(user.id, request, UPLOAD_RESERVATION_PURPOSES.R2_VIDEO), + reserveStorageQuota(user.id, request, UPLOAD_RESERVATION_PURPOSES.R2_VIDEO), ]); expect(results.every((result) => 'reservationId' in result)).toBe(true); @@ -417,7 +454,9 @@ describe('reserveStorageQuota', () => { const request = BigInt(20) * GIB; const results = await Promise.all( - Array.from({ length: 5 }, () => reserveStorageQuota(user.id, request)) + Array.from({ length: 5 }, () => + reserveStorageQuota(user.id, request, UPLOAD_RESERVATION_PURPOSES.R2_VIDEO) + ) ); const granted = results.filter((result) => 'reservationId' in result); @@ -438,8 +477,8 @@ describe('reserveStorageQuota', () => { const request = BigInt(150) * GIB; const results = await Promise.all([ - reserveStorageQuota(first.id, request), - reserveStorageQuota(second.id, request), + reserveStorageQuota(first.id, request, UPLOAD_RESERVATION_PURPOSES.R2_VIDEO), + reserveStorageQuota(second.id, request, UPLOAD_RESERVATION_PURPOSES.R2_VIDEO), ]); expect(results.every((result) => 'reservationId' in result)).toBe(true); @@ -450,7 +489,11 @@ describe('reserveStorageQuota', () => { describe('releaseStorageReservation', () => { it('deletes the reservation and frees the headroom', async () => { const user = await createSubscribedUser(); - const result = await reserveStorageQuota(user.id, BigInt(10) * GIB); + const result = await reserveStorageQuota( + user.id, + BigInt(10) * GIB, + UPLOAD_RESERVATION_PURPOSES.R2_VIDEO + ); const reservationId = 'reservationId' in result ? result.reservationId : null; expect(reservationId).toBeTruthy(); expect(await getUserTotalStorageBytes(user.id)).toBe(BigInt(10) * GIB); @@ -484,6 +527,25 @@ describe('releaseStorageReservation', () => { expect(await db.uploadReservation.count()).toBe(1); }); + + // The same scoping in the other direction. Every hold an account owns is + // billed to the same user, so `billedUserId` alone does not separate them: an + // image being attached would release a Bunny upload that was still in flight + // if it could name it, and the ids are readable by the client. + it('refuses to delete a reservation opened for a different flow', async () => { + const owner = await createSubscribedUser(); + const reservation = await createUploadReservation({ + billedUserId: owner.id, + sizeBytes: BigInt(4096), + purpose: UPLOAD_RESERVATION_PURPOSES.BUNNY, + }); + + await releaseStorageReservation(reservation.id, owner.id, UPLOAD_RESERVATION_PURPOSES.IMAGE); + expect(await db.uploadReservation.count()).toBe(1); + + await releaseStorageReservation(reservation.id, owner.id, UPLOAD_RESERVATION_PURPOSES.BUNNY); + expect(await db.uploadReservation.count()).toBe(0); + }); }); describe('GET /api/settings/storage', () => { diff --git a/tests/factories/video.ts b/tests/factories/video.ts index c05ea04..3437ac7 100644 --- a/tests/factories/video.ts +++ b/tests/factories/video.ts @@ -7,6 +7,7 @@ import { type VideoVersion, } from '@prisma/client'; import { db } from '@/lib/db'; +import { UPLOAD_RESERVATION_PURPOSES, type UploadReservationPurpose } from '@/lib/storage-quota'; import { nextSeq } from './seq'; export interface CreateVideoInput { @@ -105,6 +106,8 @@ export interface CreateUploadReservationInput { sizeBytes: bigint; /** Milliseconds from now. Negative values produce an already-expired row. */ expiresInMs?: number; + /** Which flow the hold belongs to. Only that flow can consume it. */ + purpose?: UploadReservationPurpose; } export async function createUploadReservation( @@ -115,6 +118,7 @@ export async function createUploadReservation( billedUserId: input.billedUserId, sizeBytes: input.sizeBytes, expiresAt: new Date(Date.now() + (input.expiresInMs ?? 30 * 60 * 1000)), + purpose: input.purpose ?? UPLOAD_RESERVATION_PURPOSES.R2_VIDEO, }, }); } diff --git a/tests/setup/db-global.ts b/tests/setup/db-global.ts index ce68aed..f70eff3 100644 --- a/tests/setup/db-global.ts +++ b/tests/setup/db-global.ts @@ -67,6 +67,7 @@ const REVIEWED_MIGRATIONS = [ '20260614160000_add_project_allow_downloads', '20260627140000_add_video_upload_multipart_id', '20260801120000_add_acquisition_analytics', + '20260818120000_add_upload_reservation_purpose', ]; /** Objects POST_PUSH_SQL must have produced. Verified after it runs. */ diff --git a/tests/unit/lib/guest-upload-token.test.ts b/tests/unit/lib/guest-upload-token.test.ts new file mode 100644 index 0000000..04ee2a4 --- /dev/null +++ b/tests/unit/lib/guest-upload-token.test.ts @@ -0,0 +1,109 @@ +// The grant a share-link visitor gets for a direct upload. +// +// A guest's upload is billed to the workspace owner, not to the guest, so this +// token is the only thing tying what they declared and what they hold to the +// upload it was issued for. Two claims matter here beyond the existing subject +// binding: the provider's own video id, which is what makes releasing the hold +// on the guest's say-so safe, and the declared size, which is what the asset is +// charged until Bunny reports a figure of its own. + +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { + createGuestUploadToken, + readGuestUploadGrant, + verifyGuestUploadToken, +} from '@/lib/guest-upload-token'; + +const SUBJECT = { + projectId: 'project-1', + videoId: 'video-1', + intent: 'bunny' as const, + context: '203.0.113.7:public', +}; + +const BUNNY_VIDEO_ID = 'bunnyvideo-1-abcdefgh'; + +beforeEach(() => { + vi.stubEnv('GUEST_UPLOAD_TOKEN_SECRET', 'test-guest-upload-token-secret'); +}); + +describe('readGuestUploadGrant', () => { + it('carries back the reservation and the declared size it was signed with', () => { + const token = createGuestUploadToken({ + ...SUBJECT, + providerVideoId: BUNNY_VIDEO_ID, + reservationId: 'reservation-1', + declaredSizeBytes: BigInt(4096), + }); + + expect(readGuestUploadGrant(token, SUBJECT, BUNNY_VIDEO_ID)).toEqual({ + reservationId: 'reservation-1', + declaredSizeBytes: BigInt(4096), + }); + }); + + // The binding that makes a guest release safe: presenting this token to cancel + // deletes the upload it stands for, so it cannot be used to drop the hold of + // an upload that is still running. + it('refuses a grant presented against a different provider video', () => { + const token = createGuestUploadToken({ + ...SUBJECT, + providerVideoId: BUNNY_VIDEO_ID, + reservationId: 'reservation-1', + }); + + expect(readGuestUploadGrant(token, SUBJECT, 'bunnyvideo-2-abcdefgh')).toBeNull(); + expect(readGuestUploadGrant(token, SUBJECT, null)).toBeNull(); + expect(verifyGuestUploadToken(token, SUBJECT, 'bunnyvideo-2-abcdefgh')).toBe(false); + }); + + it('still refuses a grant for another subject, bound video or not', () => { + const token = createGuestUploadToken({ + ...SUBJECT, + providerVideoId: BUNNY_VIDEO_ID, + reservationId: 'reservation-1', + }); + + expect( + readGuestUploadGrant(token, { ...SUBJECT, videoId: 'video-2' }, BUNNY_VIDEO_ID) + ).toBeNull(); + expect(readGuestUploadGrant(token, { ...SUBJECT, intent: 'image' }, BUNNY_VIDEO_ID)).toBeNull(); + expect( + readGuestUploadGrant(token, { ...SUBJECT, context: '198.51.100.9:public' }, BUNNY_VIDEO_ID) + ).toBeNull(); + }); + + // The image and audio grants carry none of this, and a grant issued before the + // claims existed keeps working rather than failing an upload in flight. + it('reads a grant with no claims as holding nothing', () => { + const token = createGuestUploadToken({ ...SUBJECT, intent: 'image' }); + const subject = { ...SUBJECT, intent: 'image' as const }; + + expect(readGuestUploadGrant(token, subject)).toEqual({ + reservationId: null, + declaredSizeBytes: null, + }); + expect(verifyGuestUploadToken(token, subject, BUNNY_VIDEO_ID)).toBe(true); + }); + + it('refuses a forged signature', () => { + const token = createGuestUploadToken({ + ...SUBJECT, + providerVideoId: BUNNY_VIDEO_ID, + reservationId: 'reservation-1', + }); + const [payload] = token.split('.'); + + expect(readGuestUploadGrant(`${payload}.forged`, SUBJECT, BUNNY_VIDEO_ID)).toBeNull(); + }); + + it('reads a non-positive declared size as nothing declared', () => { + const token = createGuestUploadToken({ + ...SUBJECT, + providerVideoId: BUNNY_VIDEO_ID, + declaredSizeBytes: BigInt(-1), + }); + + expect(readGuestUploadGrant(token, SUBJECT, BUNNY_VIDEO_ID)?.declaredSizeBytes).toBeNull(); + }); +}); From e78911e8505d6687faf75061763b304d611b4917 Mon Sep 17 00:00:00 2001 From: yusufipk Date: Tue, 18 Aug 2026 11:07:25 +0300 Subject: [PATCH 3/3] fix(comments): say the account is full instead of blaming the comment The attachment goes up before the comment does, so a full account fails on the image and never reaches the comment at all. Both that failure and a rejected comment came out as "Failed to add comment", which tells the uploader to try again, and trying again is the one thing that cannot work when there is no room left. Both now read out what the server said. A network fault, which has no message anybody wants to see, still falls back to the old line. --- .../video-page/hooks/use-comment-actions.ts | 25 +++++-- .../hooks/use-comment-actions.test.ts | 68 ++++++++++++++++++- 2 files changed, 88 insertions(+), 5 deletions(-) diff --git a/components/video-page/hooks/use-comment-actions.ts b/components/video-page/hooks/use-comment-actions.ts index 02cc6cd..02ce4e4 100644 --- a/components/video-page/hooks/use-comment-actions.ts +++ b/components/video-page/hooks/use-comment-actions.ts @@ -61,6 +61,17 @@ function getAudioUploadFilename(blob: Blob): string { return 'recording.webm'; } +/** + * A step of the submit that failed with something worth reading out. + * + * The attachment goes up before the comment does, so a full account fails on the + * image and never reaches the comment at all. Reporting that as "failed to add + * comment" tells the uploader to try again, which is the one thing that cannot + * work. Carried as its own error type so a network fault, which has no message + * anybody wants to see, still falls back to the generic line. + */ +class CommentSubmitError extends Error {} + export function useCommentActions({ videoId, setVideo, @@ -262,7 +273,12 @@ export function useCommentActions({ body: imageFormData, }); - if (!imageRes.ok) throw new Error('Failed to upload image'); + if (!imageRes.ok) { + const imagePayload = (await imageRes.json().catch(() => null)) as { + error?: string; + } | null; + throw new CommentSubmitError(imagePayload?.error || 'Failed to upload image'); + } const imageDataResponse = await imageRes.json(); imageData = { url: imageDataResponse.data.url }; } @@ -320,9 +336,10 @@ export function useCommentActions({ ), }; }); - toast.error('Failed to add comment'); + const payload = (await res.json().catch(() => null)) as { error?: string } | null; + toast.error(payload?.error || 'Failed to add comment'); } - } catch { + } catch (error) { setVideo((prev) => { if (!prev) return prev; return { @@ -334,7 +351,7 @@ export function useCommentActions({ ), }; }); - toast.error('Failed to add comment'); + toast.error(error instanceof CommentSubmitError ? error.message : 'Failed to add comment'); } finally { setIsSubmittingComment(false); setIsUploadingImage(false); diff --git a/tests/component/hooks/use-comment-actions.test.ts b/tests/component/hooks/use-comment-actions.test.ts index e875b20..c3affab 100644 --- a/tests/component/hooks/use-comment-actions.test.ts +++ b/tests/component/hooks/use-comment-actions.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { useState } from 'react'; +import { useState, type ChangeEvent } from 'react'; import { act, renderHook, type RenderHookResult } from '@testing-library/react'; import { useCommentActions } from '@/components/video-page/hooks/use-comment-actions'; import type { Comment, CommentTag, VideoData } from '@/components/video-page/types'; @@ -293,6 +293,72 @@ describe('useCommentActions adding a comment', () => { expect(harness.result.current.actions.isSubmittingComment).toBe(false); }); + // The attachment goes up before the comment does, so a full account fails on + // the image and never reaches the comment at all. Reporting that as a comment + // that would not post told the uploader to try again, which is the one thing + // that cannot work. + it('reads out the storage error the attachment upload came back with', async () => { + fetchMock.mockImplementation((url: string) => { + if (url === '/api/upload/image') { + return Promise.resolve({ + ok: false, + status: 507, + json: () => + Promise.resolve({ + error: 'Storage limit exceeded. Please delete some files to free up space.', + }), + }); + } + return Promise.resolve(ok({ data: serverComment })); + }); + const harness = renderActions(); + + // A one-pixel PNG header is enough: the client only sniffs the magic bytes. + const png = new File( + [new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a])], + 'n.png', + { + type: 'image/png', + } + ); + await act(async () => { + await harness.result.current.actions.handleImageSelect({ + target: { files: [png] }, + } as unknown as ChangeEvent); + }); + + act(() => harness.result.current.actions.setCommentText('Colour is off')); + await act(async () => { + await harness.result.current.actions.handleAddComment(); + }); + + expect(toastError).toHaveBeenCalledWith( + 'Storage limit exceeded. Please delete some files to free up space.' + ); + expect(commentIds(harness)).toEqual(['c1', 'c2']); + }); + + it('reads out the storage error the comment itself came back with', async () => { + fetchMock.mockResolvedValue({ + ok: false, + status: 507, + json: () => + Promise.resolve({ + error: 'Storage limit exceeded. Please delete some files to free up space.', + }), + }); + const harness = renderActions(); + + act(() => harness.result.current.actions.setCommentText('Colour is off')); + await act(async () => { + await harness.result.current.actions.handleAddComment(); + }); + + expect(toastError).toHaveBeenCalledWith( + 'Storage limit exceeded. Please delete some files to free up space.' + ); + }); + it('rolls the comment back out of the list when the request throws', async () => { fetchMock.mockRejectedValue(new Error('offline')); const harness = renderActions();