refactor(s3): move image upload from frontend to backend#164
Closed
OhYee wants to merge 15 commits into
Closed
Conversation
Optional S3-backed image storage for Excalidraw drawings. When
S3_BUCKET is configured, image dataURLs are uploaded to S3 instead of
being base64-encoded into the drawing's files JSON in SQLite.
Why: a single uncompressed image is several MB; embedding many of
them in SQLite bloats the DB, slows collaborative sync, and makes
backups / exports unwieldy.
Module + routes (backend):
- backend/src/s3.ts: typed S3Config, presigned URL generation, supports
AWS S3, MinIO, Cloudflare R2, Alibaba OSS via S3_ENDPOINT,
S3_FORCE_PATH_STYLE, S3_PUBLIC_URL.
- POST /files/upload-url: presigned PUT URL for browser direct uploads.
- GET /files/config: surfaces { s3Enabled } so the frontend can decide
whether to attempt direct uploads.
- GET /files/:fileId: 302 redirect to a presigned GET URL for private
buckets (otherwise the dataURL stored in the drawing points at
S3_PUBLIC_URL).
- Prisma migration adds S3File(id, userId, s3Key, mimeType, createdAt)
so the redirect route can resolve fileId -> S3 key.
Frontend integration (Editor.tsx):
- Monkey-patch addFiles() to detect new image files, upload them in
the background, and resolve S3 URLs into the persisted scene before
save (saves with base64 are awaited so we never write base64 once
S3 is configured).
- Excalidraw 0.18+ paste flow bypasses addFiles, so onChange also
detects new base64 entries and kicks the same upload pipeline.
- compressExcalidrawFiles still runs first; S3 stores compressed bytes.
Configuration (all optional, S3 disabled when unset):
S3_BUCKET, S3_REGION (default us-east-1), S3_ENDPOINT, S3_PUBLIC_URL,
S3_FORCE_PATH_STYLE, S3_KEY_PREFIX (default "excalidash"),
AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY.
Object key layout: {prefix}/{userId}/{drawingId}/{fileId}.{ext}, with
Cache-Control: public, max-age=31536000, immutable since fileIds are
content hashes.
When S3 is enabled there is no silent base64 fallback: upload failures
surface to the user instead of pretending the image was persisted.
Change-Id: Ie8fc2ad52036c3921f5eb932f2b35f1ce3d31576
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reject file ids that don't match the safe identifier shape (word chars
plus dash, 1-200 long) in two layers:
- security.ts sanitizeDrawingData drops malformed keys before persistence,
so the drawing.files JSON cannot store entries that would later be
uploaded under attacker-controlled paths.
- fileProcessing.processFilesForS3 also drops them defensively, since
this utility is reachable from import paths that bypass the schema.
Without this, a request body of
files: { "../../etc/passwd": { dataURL: "data:image/png;base64,..." } }
would write to S3 outside the per-user prefix and create a corresponding
S3File row.
Adds a regression test in fileProcessing.test.ts.
Change-Id: I4c27df54c3370977a20631a3cb6c08476d1546b9
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: OhYee <oyohyee@oyohyee.com>
…uessability Excalidraw fileIds are SHA-1 hashes of the file bytes — anyone in possession of the original image can compute the id, so the previous "fileId is unguessable" comment was incorrect and any authenticated user could fetch images uploaded by anyone else. Now the route authorises by drawing access: - the uploader (S3File.userId === req.user.id) can always read; OR - the caller has access to a drawing that references this fileId, either one they own or one explicitly shared with them via DrawingPermission. A single substring query on the JSON columns is enough because fileIds are >= 16 random chars; the wrapping quotes prevent prefix collisions (`abc1` won't match `abc12`). Also adds a 401 fallback in case requireAuth is ever removed. Change-Id: I4c9c0640e39d8d6a791009db289dc3a29e6658df Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: OhYee <oyohyee@oyohyee.com>
S3_FORCE_PATH_STYLE was parsed in config.ts and surfaced on the typed S3Config but the property was never threaded through to the initS3() call site. MinIO/Alibaba OSS users who set the env var saw no effect — the S3 client always picked the default addressing style. Change-Id: If16d6d8326612d4fb48501a54fea35c2b1e62d1b Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: OhYee <oyohyee@oyohyee.com>
GET /files/:fileId previously required a logged-in user via requireAuth, which broke private-bucket deployments whenever a drawing was shared with an active link-share: the anonymous viewer could open the drawing scene but every <img src="/api/files/..."> returned 401. Replace requireAuth with optionalAuth and decide access by drawing membership instead: - The uploader (S3File.userId === caller) always has access. - Authenticated callers also pass if they own a drawing referencing the fileId, or were granted access via DrawingPermission. - Anyone (anonymous included) passes if a drawing referencing the fileId has an active DrawingLinkShare row (revokedAt IS NULL and expiresAt either NULL or in the future). - Otherwise return 404 so the response doesn't leak whether a fileId exists for someone who doesn't have access. Change-Id: Id8e6f427d9bd392a0adbefd931a1b219c7b35513 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Schema/route/sanitiser portion of the per-drawing storage isolation fix. (The fileProcessing/duplicate/storage portions land in the backend-direct and storage-management PRs that build on this one.) Why: S3File previously used the global Excalidraw fileId as primary key. Excalidraw fileIds are content hashes, so two drawings — possibly two different users — uploading the same image silently overwrote each other's row. /files/:fileId then couldn't disambiguate which (drawing, owner) the caller meant. Changes here: - Prisma schema + migration: composite primary key @@id([drawingId, fileId]) plus @@index([drawingId]). - s3.ts: shared FILE_KEY_PREFIX constant + buildS3Key() and drawingS3Prefix() helpers so upload/list/cleanup paths cannot drift onto different prefixes (also kills the duplicated env-var read previously sitting in fileProcessing/storage/dashboard). - routes/files.ts: route signature is /files/:drawingId/:fileId; the lookup now uses { drawingId_fileId } and the access decision is drawing-scoped (owner / explicit grantee / active link-share). - security.ts: sanitiser accepts the new dataURL shape /api/files/:drawingId/:fileId. Change-Id: Ie8bcb007b167a6eb7c3167cb7a33661468b8a11d Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Dockerfile production stage runs `npx tsc` to emit dist/. The same tsc invocation was also compiling the __tests__/ tree, which made the build fail whenever a test mock used a partial type (e.g. a Zod schema mock with only safeParse). vitest has its own type-checking pass, so the production build has no business compiling test files. Exclude *.test.ts, *.integration.ts, and the __tests__/ folder. The production tsc now succeeds even when test files have type shortcomings; vitest still catches real test errors at run time. Change-Id: Ica2d5f91c05131b3d9b6674df61e34fcbda60884 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Prior versions of this branch shipped the new S3File composite primary key by editing the original `20260408060000_add_s3_files` migration in place. That works for fresh deployments but is invisible to any deployment that already applied the original `id`-only PK migration — Prisma decides what to apply by migration name, so the new shape never reaches the database and runtime upserts crash with "The column `drawingId` does not exist in the current database." Restore the original migration to its pre-bundle shape and add a follow-up migration that drops + recreates S3File with the new shape. Both fresh and upgrade paths converge on the composite PK. Existing S3 objects are untouched; public-bucket deployments are unaffected. Private-bucket deployments will see /api/files/:drawingId/:fileId 404 for legacy rows until those drawings are re-saved (the save flow upserts on every base64 dataURL); operators with valuable legacy rows can rebuild manually before deploying. Change-Id: I9c06c7a46247050521a1d5013ec37eeebeb86247 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the presigned-URL upload flow with backend-direct uploads. The frontend now sends the entire drawing payload (base64 dataURLs intact) to the standard create/update/import routes; the backend uploads any new dataURLs to S3 and rewrites them to S3 URLs before persisting. Why move it: - Single auth boundary: avoid distributing presigned URLs that bypass app-level access checks. - Coverage of all input paths: paste, drag-and-drop, import all flow through the same processFilesForS3 utility — no need for the Editor.tsx onChange detection that Excalidraw 0.18 routinely missed. - Auditability: S3File rows are written by the backend in the same request that creates/updates the drawing. Changes: - Add backend/src/fileProcessing.ts with processFilesForS3(files, userId, drawingId, prisma): scans the files record for base64 dataURLs, uploads them, upserts S3File rows, returns rewritten files (base64 -> public S3 URL or /api/files/:fileId redirect). - Extend backend/src/s3.ts with uploadBuffer() and listS3Objects(). - Wire processFilesForS3 into POST /drawings, PUT /drawings/:id, and both /import/* routes. - Remove POST /files/upload-url (no longer used by any client). - Strip frontend S3 upload code: addFiles monkey-patch, the pendingS3UploadsRef, and the save-time await loop are all gone. Change-Id: I4ff81d050e08d3eb7344712256785f6dc3465a61 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reject file ids that don't match the safe identifier shape (word chars
plus dash, 1-200 long) in two layers:
- security.ts sanitizeDrawingData drops malformed keys before persistence,
so the drawing.files JSON cannot store entries that would later be
uploaded under attacker-controlled paths.
- fileProcessing.processFilesForS3 also drops them defensively, since
this utility is reachable from import paths that bypass the schema.
Without this, a request body of
files: { "../../etc/passwd": { dataURL: "data:image/png;base64,..." } }
would write to S3 outside the per-user prefix and create a corresponding
S3File row.
Adds a regression test in fileProcessing.test.ts.
Change-Id: I4c27df54c3370977a20631a3cb6c08476d1546b9
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: OhYee <oyohyee@oyohyee.com>
PUT /drawings/:id (scene update path): - Version conflict detection now happens BEFORE processFilesForS3, not after the updateMany. Previously, on a stale-version save the upload had already PUT every base64 file to S3 and we returned 409 — the objects became permanent orphans with no DB row. The new check uses a cheap findFirst before the upload; the existing version-filtered updateMany still catches the residual race window. - Non-owner editors (link-share + edit, share-with-user) can no longer introduce new fileIds. Reading the existing files JSON gives us the set of allowed ids; any unknown id in the payload returns 403. Owners retain full control. This prevents a shared editor from filling the owner's S3 bucket via 50MB uploads. DELETE /drawings/:id: - Best-effort cleanup of S3 objects and S3File rows under the per- drawing prefix. Failures log and continue; the drawing row is already gone, so the cleanup is opportunistic. Without this, deleting a drawing leaked every uploaded image permanently. Change-Id: Ia684b408c1eb0c474c04bf8c04eb339f9a497e47 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: OhYee <oyohyee@oyohyee.com>
Two import flows (excalidash backup + legacy SQLite) both pre-uploaded files to S3 using the imported drawing id, then on a foreign-owner id collision wrote the row under a fresh uuidv4() — leaving every uploaded object as a permanent S3 orphan keyed under an id that no DB row knew about. Now both routes pre-resolve the final drawing id before uploading by peeking at any pre-existing drawing of the same id; if it belongs to another user, we generate the UUID up front and feed it to both the upload and the in-transaction create. The transaction still re-checks authoritatively, but always writes under the same finalId. Also replaces the per-drawing serial await with bounded Promise.all batches of 8. A 5000-drawing import previously serialised every S3 PUT inside the request, easily hitting reverse-proxy timeouts; the bound keeps memory and socket pressure flat without saturating the bucket. Change-Id: I7ae77f642f097c295c71771d59ecea0d045661d3 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: OhYee <oyohyee@oyohyee.com>
Backend-upload portion of the per-drawing storage isolation fix that started in feat/s3-image-upload. Pairs with the schema/route/sanitiser changes in that earlier commit so the upload pipeline is internally consistent end-to-end. Changes here: - fileProcessing.ts: S3File upsert is now keyed by composite (drawingId, fileId); S3 key built via the shared buildS3Key() helper; access URL emitted as /api/files/:drawingId/:fileId. - routes/dashboard/drawings.ts: DELETE handler purges S3File rows by drawingId instead of by s3Key prefix — collision-free with sibling drawings, even when they share the same fileId. - fileProcessing.test.ts: mock now exposes buildS3Key, and the upsert expectations match the composite-key shape. Change-Id: I5259cb2c50752d2ff5e1f567f6c9022594e12bc0 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…leted POST /drawings/:id/duplicate previously copied elements/appState/files verbatim, including the dataURLs that point at the *original* drawing's S3 prefix (`/api/files/origId/fileId` or `https://.../excalidash/owner/origId/fileId.ext`). The duplicate's images therefore pointed at storage owned by the original. Deleting the original then ran the new prefix-scoped S3 cleanup and silently broke every image in the duplicate. Now the duplicate handler does a server-side CopyObject for every S3File row of the original under the new drawingId path, writes a fresh (newDrawingId, fileId) S3File row, and rewrites the dataURLs in the new drawing's files JSON to match the new drawing id. Each drawing — original and duplicate — owns its own storage from the moment of duplication, so DELETE /drawings/:origId can no longer strand the duplicate. Adds a bucket-internal copyS3Object() helper to s3.ts (uses S3 CopyObject; no download/re-upload). Change-Id: If57d006fce8964fe04e702680e96866a3ec3b6f0 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…S3 upload Symptom: every save produced a megabyte-scale `Drawing.preview` column. The SVG had `<image href="data:image/png;base64,...">` for each image element instead of the S3 URL. Root cause: the frontend generates the preview SVG from the canvas state at save time, before the round-trip uploads files to S3. The SVG embeds whatever dataURL the file currently has, which at save time is still base64. processFilesForS3 only rewrites the dataURL inside `Drawing.files`, never the parallel preview SVG, so the inlined base64 stays in the preview forever. Add a rewritePreviewForS3 helper in fileProcessing.ts that diffs the original-vs-processed files map and applies the resulting URL substitutions to the preview string. Wire it into the POST and PUT handlers so the preview saved alongside the upload reflects the same S3 URLs as Drawing.files. Best-effort string substitution: it works because the same dataURL string is character-identical in both `files[fileId].dataURL` and the preview SVG's href. If frontend encoding ever diverges, the worst case is the preview is left as-is — never crashes. Change-Id: I427edabcccc1453bf0f69f5dde33c9648cfcb370 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors ExcaliDash’s S3 image storage flow by moving image upload responsibility from the frontend to the backend save/import handlers, aiming to unify coverage across paste/drag/drop/import paths and improve operational auditability/cleanup.
Changes:
- Added backend-side file processing (
processFilesForS3) to detect base64 dataURLs, upload to S3, upsertS3Filerecords, and rewriteDrawing.files(plus preview SVG rewriting). - Introduced backend S3 helpers and new
/files/config+/files/:drawingId/:fileIddownload redirect route; integrated S3 cleanup/copy behavior into drawing delete/duplicate flows. - Updated import routes to pre-resolve drawing IDs and batch S3 uploads with bounded concurrency.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/pages/Editor.tsx | Updates editor save/sync behavior; adds initial file-id tracking (currently unused) and adjusts file sync bookkeeping. |
| frontend/src/api/index.ts | Adds a cached /files/config probe helper (isS3Enabled). |
| backend/tsconfig.json | Excludes test files from backend TS compilation inputs. |
| backend/src/security.ts | Tightens sanitization to drop unsafe file IDs and allow S3/CDN URLs + /api/files/:drawingId/:fileId paths. |
| backend/src/s3.ts | Adds backend S3 client helpers (upload buffer, list, copy) and URL helpers. |
| backend/src/fileProcessing.ts | Implements backend-side base64 scanning, S3 upload, S3File upsert, and preview SVG rewriting. |
| backend/src/routes/files.ts | Adds S3-related routes: /files/config and /files/:drawingId/:fileId redirect for private-bucket mode. |
| backend/src/routes/dashboard/types.ts | Extends dashboard route deps to include processFilesForS3. |
| backend/src/routes/dashboard/drawings.ts | Integrates backend uploads into create/update, adds delete-time cleanup, and duplicate-time S3 copy + URL rewrite. |
| backend/src/routes/importExport/excalidashImportRoutes.ts | Pre-resolves final drawing IDs and processes files through backend upload path with bounded concurrency. |
| backend/src/routes/importExport/legacySqliteImportRoutes.ts | Same as above for legacy SQLite imports; writes processed files into DB. |
| backend/src/index.ts | Initializes S3 from env config, registers file routes, and wires processFilesForS3 into dashboard routes. |
| backend/src/config.ts | Adds backend config parsing for S3-related env vars. |
| backend/src/tests/fileProcessing.test.ts | Adds unit tests for decodeDataURL and processFilesForS3. |
| backend/prisma/schema.prisma | Defines S3File with composite primary key (drawingId, fileId). |
| backend/prisma/migrations/20260408060000_add_s3_files/migration.sql | Initial migration creating the original single-column PK S3File table. |
| backend/prisma/migrations/20260506130000_s3file_composite_pk/migration.sql | Migration switching S3File to composite PK by dropping/recreating the table. |
| backend/package.json | Adds AWS SDK v3 S3 client and presigner dependencies. |
| backend/.env.example | Documents S3 configuration (currently still describes browser-direct presigned PUT flow). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+268
to
+269
| /** File IDs present at initial drawing load — skip S3 re-upload for these. */ | ||
| const initialFileIdsRef = useRef<Set<string>>(new Set()); |
Comment on lines
+70
to
+75
| const uploadTasks = Object.entries(files).map(async ([fileId, file]) => { | ||
| if (!VALID_FILE_ID.test(fileId)) { | ||
| // Reject path-traversal candidates rather than silently uploading | ||
| // them under a forged S3 key. Drop from output so the bad entry | ||
| // never reaches the database either. | ||
| console.warn(`[s3] Skipping file with invalid id: ${JSON.stringify(fileId)}`); |
Comment on lines
+748
to
+798
|
|
||
| for (const src of sourceFiles) { | ||
| // Replace `/{originalId}/` with `/{newId}/` exactly once at | ||
| // the per-drawing folder boundary in the s3Key. | ||
| const destKey = src.s3Key.replace( | ||
| `/${original.id}/`, | ||
| `/${newDrawing.id}/`, | ||
| ); | ||
|
|
||
| try { | ||
| await copyS3Object(src.s3Key, destKey, src.mimeType); | ||
| } catch (err) { | ||
| console.error( | ||
| `[drawings/duplicate] Failed to copy ${src.s3Key} -> ${destKey}`, | ||
| err, | ||
| ); | ||
| continue; | ||
| } | ||
|
|
||
| await prisma.s3File.create({ | ||
| data: { | ||
| drawingId: newDrawing.id, | ||
| fileId: src.fileId, | ||
| userId: req.user.id, | ||
| s3Key: destKey, | ||
| mimeType: src.mimeType, | ||
| }, | ||
| }); | ||
|
|
||
| // Rewrite dataURL so private-bucket redirects and public CDN | ||
| // links point at the new object instead of the original's. | ||
| const file = filesObj[src.fileId]; | ||
| if (file && typeof file.dataURL === "string") { | ||
| const next = file.dataURL | ||
| .replace( | ||
| `/api/files/${original.id}/`, | ||
| `/api/files/${newDrawing.id}/`, | ||
| ) | ||
| .replace(`/${original.id}/`, `/${newDrawing.id}/`); | ||
| if (next !== file.dataURL) { | ||
| filesObj[src.fileId] = { ...file, dataURL: next }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const serialised = JSON.stringify(filesObj); | ||
| await prisma.drawing.update({ | ||
| where: { id: newDrawing.id }, | ||
| data: { files: serialised }, | ||
| }); | ||
| duplicatedFilesJson = serialised; |
Comment on lines
438
to
444
| const existing = await tx.drawing.findUnique({ where: { id: prepared.id } }); | ||
| // Reuse the id we committed to before the S3 upload. If the | ||
| // pre-upload check disagrees with the in-transaction state | ||
| // (race), we still write under finalId so the file path | ||
| // matches what was uploaded. | ||
| const finalId = finalDrawingIdMap.get(i) ?? prepared.id; | ||
| if (!existing) { |
Comment on lines
386
to
390
| const existing = d.importedId ? await tx.drawing.findUnique({ where: { id: d.importedId } }) : null; | ||
| // Always use the id we committed to before the S3 upload, so | ||
| // the row's S3 keys point to objects that actually exist. | ||
| const finalId = finalDrawingIdMap.get(i) ?? d.importedId ?? uuidv4(); | ||
|
|
Comment on lines
+539
to
+548
| } else if (/^https:\/\//i.test(value)) { | ||
| // S3 / CDN public URL — validate format, no HTML injection risk. | ||
| const hasSuspiciousContent = suspiciousPatterns.some( | ||
| (pattern) => pattern.test(value) | ||
| ); | ||
| if (hasSuspiciousContent || value.length > 2048) { | ||
| file[key] = ""; | ||
| } else { | ||
| file[key] = value; | ||
| } |
Comment on lines
+48
to
+52
| # When S3_BUCKET is set, pasted/dropped images are uploaded directly from the | ||
| # browser to S3, keeping ExcaliDash's server bandwidth and SQLite size small. | ||
| # Compatible with AWS S3, Cloudflare R2, MinIO, Alibaba OSS, and any service | ||
| # that supports presigned PUT URLs. | ||
| # |
Comment on lines
+10
to
+19
| -- This migration drops the existing S3File rows and recreates the | ||
| -- table with the new shape. Public-bucket deployments are unaffected | ||
| -- (image dataURLs already encode the bucket URL directly). Private- | ||
| -- bucket deployments will see /api/files/:drawingId/:fileId 404 for | ||
| -- pre-existing images until each affected drawing is re-saved (the | ||
| -- save flow upserts a fresh row per (drawingId, fileId) only when it | ||
| -- detects new base64 dataURLs, so legacy rows must be rebuilt by hand | ||
| -- if needed). S3 objects themselves are untouched. | ||
|
|
||
| DROP TABLE IF EXISTS "S3File"; |
Comment on lines
+49
to
+52
| // GET /files/:fileId | ||
| // Issues a presigned GET URL and redirects the browser to S3. | ||
| // Used only in private-bucket deployments where S3_PUBLIC_URL is not | ||
| // set and the dataURL stored in the drawing is "/api/files/:fileId". |
Contributor
Author
|
Superseded — folded into #163 to avoid the add-then-delete churn between the frontend signed-URL flow and the backend-direct refactor. The combined PR has the same final state with cleaner per-commit logic. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #145. Depends on #163, please review/merge that first.
Why
The earlier PR shipped a frontend-direct-upload flow (presigned URLs). Three operational issues led us to move uploading to the backend:
addFiles()hook misses Excalidraw 0.18+'s internal paste path. Server-side processing on save catches every code path uniformly.S3Filerows are written by the backend in the same handler as the drawing row; failure modes are surfaceable.What
backend/src/fileProcessing.tsexposesprocessFilesForS3(files, userId, drawingId, prisma)— scans the file record for base64 dataURLs, uploads them, upserts a row keyed(drawingId, fileId), returns a rewritten file record.s3.tsaddsuploadBuffer(),listS3Objects(),copyS3Object().POST /drawings,PUT /drawings/:id, and both/import/*routes.POST /files/upload-url(no longer used by any client).addFilesmonkey-patch,pendingS3UploadsRef, the save-time await loop).Notable safety fixes (review feedback, separate commits)
fix(s3): validate fileId before using as S3 key segment— defence-in-depth on the upload path (sanitiser layer is in feat(s3): add S3 image upload support for Excalidraw drawings #163).fix(drawings): version pre-check, non-owner gate, DELETE-time S3 cleanupfix(import): pre-resolve drawing id, bound parallel S3 uploads— on imported-id collision the old code uploaded underprepared.idthen wrote the row underuuidv4(), leaving every uploaded object as a permanent orphan. Pre-resolves the final id before upload. Also batches parallel uploads (concurrency 8) so thousands-of-drawings imports don't hit reverse-proxy timeouts.fix(drawings): copy S3 storage on duplicate so the original can be deleted— Duplicate route now does a server-sideCopyObjectCommandfor eachS3Filerow of the original under the new drawingId path. Without this, deleting the original would silently break every image in the duplicate's scene (since the duplicate references the original's prefix).fix(drawings): rewrite preview SVG so it doesn't inline base64 after S3 upload— the frontend generates the preview SVG before the round-trip uploads files; the SVG embeds the base64 dataURL as the<image href>. After this fix, the backend rewrites the preview string with the sameoriginalDataURL → s3URLsubstitutions it applied toDrawing.files, so previews don't carry the inlined base64 forever.Risk
POST /files/upload-urlremoved. Old browser tabs holding a stale frontend will hit 404 on paste, butprocessFilesForS3still accepts base64 in the save payload, so the save itself succeeds — user-visible damage is one console error per paste until refresh.Verification
npm run build,npm test(172 / 172 pass)