Skip to content

Commit 80a8d2d

Browse files
committed
feat: add S3 cleanup on image change/remove
- Fix S3KeySchema to match actual key format (folder/uuid-filename.ext) - Add removeByUrl command for URL-based S3 deletion - Delete old S3 files when images are changed or removed - Add extractS3KeyFromUrl helper function - Update tests for new key format
1 parent f4c0bd5 commit 80a8d2d

File tree

5 files changed

+164
-30
lines changed

5 files changed

+164
-30
lines changed

docs/knowledges/image-upload.md

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,38 @@ const MAX_BASE64_SIZE = Math.ceil(10 * 1024 * 1024 * 1.37);
4242
- Base64 adds ~37% overhead
4343
- 10MB file → ~13.7MB base64
4444

45+
## S3 Key Format
46+
47+
Keys follow the format: `{folder}/{uuid}-{filename}.{ext}`
48+
49+
Example: `articles/a1b2c3d4-e5f6-7890-abcd-ef1234567890-cover.webp`
50+
51+
Allowed folders: `images`, `uploads`, `covers`, `avatars`, `articles`, `members`, `projects`
52+
53+
## S3 Cleanup
54+
55+
When images are changed or removed, the old S3 file is automatically deleted:
56+
57+
- **Change**: Old image deleted after new upload succeeds
58+
- **Remove**: Image deleted immediately when "Remove" button clicked
59+
- **External URLs**: Non-S3 URLs (different host) are ignored safely
60+
61+
The `removeByUrl` command in `storage.remote.ts` handles URL-to-key conversion server-side.
62+
63+
## User Input Methods
64+
65+
The `ImageUpload` component supports:
66+
67+
1. **Click**: Click to open file picker
68+
2. **Drag & Drop**: Drag image files onto the component
69+
3. **Paste (Ctrl+V)**: Paste from clipboard anywhere on the page
70+
71+
Paste is handled globally via `svelte:window onpaste` and skips INPUT/TEXTAREA elements to avoid conflicts.
72+
4573
## Design Decisions
4674

4775
- **Never reject user uploads**: Compress instead of refusing
4876
- **Quality over size**: Try high quality first, only reduce if needed
4977
- **GIF exception**: GIFs skip compression (animation would be lost)
78+
- **Clean up S3**: Delete old files on change/remove to avoid orphans
79+
- **Fire-and-forget cleanup**: S3 deletion errors don't block the UI

src/lib/components/image-upload.svelte

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
<script lang="ts">
22
import { AlertCircle, Loader2, RotateCw, Upload, X } from "lucide-svelte";
3-
import { upload } from "$lib/data/private/storage.remote";
3+
import { removeByUrl, upload } from "$lib/data/private/storage.remote";
44
import {
55
type AllowedFolder,
66
isAcceptedImageType,
@@ -55,6 +55,9 @@
5555
// Compress image before upload
5656
const processedFile = await compressImage(file);
5757
58+
// Store old URL to delete after successful upload
59+
const oldUrl = value;
60+
5861
// Immediate preview
5962
previewUrl = URL.createObjectURL(processedFile);
6063
isUploading = true;
@@ -72,6 +75,11 @@
7275
});
7376
value = result.url;
7477
lastFailedFile = null; // Clear on success
78+
79+
// Delete old S3 image after successful upload
80+
if (oldUrl) {
81+
removeByUrl(oldUrl).catch(console.error);
82+
}
7583
} catch {
7684
error = "Upload failed. Please try again.";
7785
previewUrl = null;
@@ -128,6 +136,10 @@
128136
}
129137
130138
function clearImage() {
139+
// Delete S3 image if it exists
140+
if (value) {
141+
removeByUrl(value).catch(console.error);
142+
}
131143
value = "";
132144
previewUrl = null;
133145
error = null;

src/lib/data/private/storage.remote.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
import * as v from "valibot";
22
import { command } from "$app/server";
3+
import { env } from "$lib/env/env.server";
34
import { requireUtCodeMember } from "$lib/server/database/auth.server";
45
import { compressImage } from "$lib/server/database/image.server";
56
import { deleteFile, uploadBuffer } from "$lib/server/database/storage.server";
67
import { ACCEPTED_IMAGE_TYPES, ALLOWED_FOLDERS } from "$lib/shared/logic/image";
7-
import { S3KeySchema } from "$lib/shared/logic/storage";
8+
import { extractS3KeyFromUrl, S3KeySchema } from "$lib/shared/logic/storage";
89

910
/** Allowed folder paths for uploads */
1011
const FolderSchema = v.optional(v.picklist([...ALLOWED_FOLDERS]));
@@ -39,3 +40,21 @@ export const remove = command(S3KeySchema, async (key) => {
3940
await requireUtCodeMember();
4041
await deleteFile(key);
4142
});
43+
44+
/**
45+
* Delete an image by its URL
46+
* Only works for S3-hosted images (checks against S3_PUBLIC_URL)
47+
* Returns true if deleted, false if URL was not an S3 image
48+
*/
49+
export const removeByUrl = command(v.string(), async (url) => {
50+
await requireUtCodeMember();
51+
52+
const key = extractS3KeyFromUrl(url, env.S3_PUBLIC_URL);
53+
if (!key) {
54+
// Not an S3 URL or invalid key - nothing to delete
55+
return false;
56+
}
57+
58+
await deleteFile(key);
59+
return true;
60+
});
Lines changed: 75 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,39 @@
11
import { describe, expect, test } from "bun:test";
22
import * as v from "valibot";
3-
import { getExtensionFromKey, S3_KEY_PATTERN, S3KeySchema, validateS3Key } from "./storage";
3+
import {
4+
extractS3KeyFromUrl,
5+
getExtensionFromKey,
6+
S3_KEY_PATTERN,
7+
S3KeySchema,
8+
validateS3Key,
9+
} from "./storage";
410

511
describe("storage", () => {
612
describe("validateS3Key", () => {
7-
test("accepts valid UUID key", () => {
8-
expect(validateS3Key("a1b2c3d4-e5f6/a1b2c3d4-e5f6.jpg")).toBe(true);
13+
test("accepts valid key with folder/uuid-filename format", () => {
14+
expect(validateS3Key("articles/a1b2c3d4-e5f6-7890-abcd-ef1234567890-cover.jpg")).toBe(true);
915
});
1016

1117
test("accepts valid key with full UUID format", () => {
12-
expect(
13-
validateS3Key(
14-
"550e8400-e29b-41d4-a716-446655440000/550e8400-e29b-41d4-a716-446655440000.png",
15-
),
16-
).toBe(true);
18+
expect(validateS3Key("projects/550e8400-e29b-41d4-a716-446655440000-thumbnail.png")).toBe(
19+
true,
20+
);
1721
});
1822

19-
test("accepts various file extensions", () => {
20-
expect(validateS3Key("abc123/def456.jpg")).toBe(true);
21-
expect(validateS3Key("abc123/def456.png")).toBe(true);
22-
expect(validateS3Key("abc123/def456.webp")).toBe(true);
23-
expect(validateS3Key("abc123/def456.gif")).toBe(true);
23+
test("accepts various allowed folders", () => {
24+
expect(validateS3Key("images/abc-123-file.jpg")).toBe(true);
25+
expect(validateS3Key("uploads/abc-123-file.png")).toBe(true);
26+
expect(validateS3Key("covers/abc-123-file.webp")).toBe(true);
27+
expect(validateS3Key("avatars/abc-123-file.gif")).toBe(true);
28+
expect(validateS3Key("members/abc-123-file.jpg")).toBe(true);
2429
});
2530

2631
test("rejects path traversal attempt", () => {
2732
expect(validateS3Key("../../../etc/passwd")).toBe(false);
2833
});
2934

3035
test("rejects key without extension", () => {
31-
expect(validateS3Key("a1b2c3d4-e5f6/a1b2c3d4-e5f6")).toBe(false);
36+
expect(validateS3Key("articles/a1b2c3d4-cover")).toBe(false);
3237
});
3338

3439
test("rejects key without slash", () => {
@@ -43,8 +48,12 @@ describe("storage", () => {
4348
expect(validateS3Key("test/../admin/secret.jpg")).toBe(false);
4449
});
4550

46-
test("rejects uppercase letters", () => {
47-
expect(validateS3Key("ABC123/DEF456.jpg")).toBe(false);
51+
test("rejects uppercase folder", () => {
52+
expect(validateS3Key("ARTICLES/abc-123.jpg")).toBe(false);
53+
});
54+
55+
test("rejects invalid folder", () => {
56+
expect(validateS3Key("invalid/abc-123-file.jpg")).toBe(false);
4857
});
4958

5059
test("rejects empty string", () => {
@@ -54,19 +63,19 @@ describe("storage", () => {
5463

5564
describe("getExtensionFromKey", () => {
5665
test("extracts jpg extension", () => {
57-
expect(getExtensionFromKey("abc123/def456.jpg")).toBe("jpg");
66+
expect(getExtensionFromKey("articles/abc-123-file.jpg")).toBe("jpg");
5867
});
5968

6069
test("extracts png extension", () => {
61-
expect(getExtensionFromKey("abc123/def456.png")).toBe("png");
70+
expect(getExtensionFromKey("projects/abc-123-file.png")).toBe("png");
6271
});
6372

6473
test("extracts webp extension", () => {
65-
expect(getExtensionFromKey("abc123/def456.webp")).toBe("webp");
74+
expect(getExtensionFromKey("images/abc-123-file.webp")).toBe("webp");
6675
});
6776

6877
test("returns null for key without extension", () => {
69-
expect(getExtensionFromKey("abc123/def456")).toBe(null);
78+
expect(getExtensionFromKey("articles/abc-123-file")).toBe(null);
7079
});
7180

7281
test("returns null for empty string", () => {
@@ -76,7 +85,7 @@ describe("storage", () => {
7685

7786
describe("S3KeySchema (Valibot)", () => {
7887
test("passes valid key", () => {
79-
const result = v.safeParse(S3KeySchema, "a1b2c3d4-e5f6/a1b2c3d4-e5f6.jpg");
88+
const result = v.safeParse(S3KeySchema, "articles/a1b2c3d4-cover.jpg");
8089
expect(result.success).toBe(true);
8190
});
8291

@@ -89,15 +98,58 @@ describe("storage", () => {
8998
});
9099

91100
test("fails path without extension", () => {
92-
const result = v.safeParse(S3KeySchema, "abc123/def456");
101+
const result = v.safeParse(S3KeySchema, "articles/abc-123-file");
93102
expect(result.success).toBe(false);
94103
});
104+
105+
test("fails with invalid folder", () => {
106+
const result = v.safeParse(S3KeySchema, "invalid/abc-123-file.jpg");
107+
expect(result.success).toBe(false);
108+
if (!result.success) {
109+
expect(result.issues[0].message).toBe("Invalid folder");
110+
}
111+
});
95112
});
96113

97114
describe("S3_KEY_PATTERN", () => {
98115
test("is exported and usable", () => {
99116
expect(S3_KEY_PATTERN).toBeInstanceOf(RegExp);
100-
expect(S3_KEY_PATTERN.test("abc123/def456.jpg")).toBe(true);
117+
expect(S3_KEY_PATTERN.test("articles/abc-123-file.jpg")).toBe(true);
118+
});
119+
});
120+
121+
describe("extractS3KeyFromUrl", () => {
122+
const baseUrl = "http://localhost:9000/dev";
123+
124+
test("extracts key from valid S3 URL", () => {
125+
expect(extractS3KeyFromUrl(`${baseUrl}/articles/a1b2c3d4-cover.webp`, baseUrl)).toBe(
126+
"articles/a1b2c3d4-cover.webp",
127+
);
128+
});
129+
130+
test("returns null for external URL", () => {
131+
expect(extractS3KeyFromUrl("https://example.com/image.jpg", baseUrl)).toBe(null);
132+
});
133+
134+
test("returns null for URL with different base", () => {
135+
expect(extractS3KeyFromUrl("http://other.host/articles/a1b2c3d4-cover.webp", baseUrl)).toBe(
136+
null,
137+
);
138+
});
139+
140+
test("returns null for invalid key format", () => {
141+
expect(extractS3KeyFromUrl(`${baseUrl}/invalid/key`, baseUrl)).toBe(null);
142+
});
143+
144+
test("returns null for empty URL", () => {
145+
expect(extractS3KeyFromUrl("", baseUrl)).toBe(null);
146+
});
147+
148+
test("works with production-like URLs", () => {
149+
const prodUrl = "https://s3.example.com/bucket";
150+
expect(extractS3KeyFromUrl(`${prodUrl}/members/a1b2c3d4-avatar.png`, prodUrl)).toBe(
151+
"members/a1b2c3d4-avatar.png",
152+
);
101153
});
102154
});
103155
});

src/lib/shared/logic/storage.ts

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,33 @@
11
import * as v from "valibot";
2+
import { ALLOWED_FOLDERS } from "./image";
23

34
/**
4-
* Regex pattern for valid S3 keys: {uuid}/{uuid}.{ext} format
5+
* Regex pattern for valid S3 keys: {folder}/{uuid}-{filename}.{ext} format
6+
* Example: articles/a1b2c3d4-e5f6-7890-abcd-ef1234567890-cover.webp
57
* This prevents path traversal attacks and ensures consistent key structure
68
*/
7-
export const S3_KEY_PATTERN = /^[a-f0-9-]+\/[a-f0-9-]+\.[a-zA-Z0-9]+$/;
9+
export const S3_KEY_PATTERN = /^[a-z]+\/[a-f0-9-]+-[^/]+\.[a-zA-Z0-9]+$/;
810

911
/**
1012
* Valibot schema for S3 key validation
11-
* Ensures keys follow the {uuid}/{uuid}.{ext} format
13+
* Ensures keys follow the {folder}/{uuid}-{filename}.{ext} format
1214
*/
13-
export const S3KeySchema = v.pipe(v.string(), v.regex(S3_KEY_PATTERN, "Invalid S3 key format"));
15+
export const S3KeySchema = v.pipe(
16+
v.string(),
17+
v.regex(S3_KEY_PATTERN, "Invalid S3 key format"),
18+
v.check((key) => {
19+
const folder = key.split("/")[0];
20+
return ALLOWED_FOLDERS.some((f) => f === folder);
21+
}, "Invalid folder"),
22+
);
1423

1524
/**
1625
* Validates an S3 key string
1726
* @param key - The S3 key to validate
1827
* @returns true if valid, false otherwise
1928
*/
2029
export function validateS3Key(key: string): boolean {
21-
return S3_KEY_PATTERN.test(key);
30+
return S3_KEY_PATTERN.test(key) && ALLOWED_FOLDERS.some((f) => f === key.split("/")[0]);
2231
}
2332

2433
/**
@@ -30,3 +39,15 @@ export function getExtensionFromKey(key: string): string | null {
3039
const match = key.match(/\.([a-zA-Z0-9]+)$/);
3140
return match?.[1] ?? null;
3241
}
42+
43+
/**
44+
* Extracts S3 key from a full S3 URL
45+
* @param url - The full S3 URL (e.g., http://localhost:9000/dev/articles/uuid-file.webp)
46+
* @param baseUrl - The S3 public URL base (e.g., http://localhost:9000/dev)
47+
* @returns The S3 key or null if invalid
48+
*/
49+
export function extractS3KeyFromUrl(url: string, baseUrl: string): string | null {
50+
if (!url.startsWith(baseUrl)) return null;
51+
const key = url.slice(baseUrl.length + 1); // +1 for the trailing slash
52+
return validateS3Key(key) ? key : null;
53+
}

0 commit comments

Comments
 (0)