Skip to content

feat: add image support (upload, serve, manage) - #4

Merged
chapati23 merged 2 commits into
mainfrom
feat/image-support
Mar 1, 2026
Merged

feat: add image support (upload, serve, manage)#4
chapati23 merged 2 commits into
mainfrom
feat/image-support

Conversation

@gisk0

@gisk0 gisk0 commented Mar 1, 2026

Copy link
Copy Markdown
Owner

Summary

Adds full image support to the obs.gisk0.dev worker:

New API Endpoints

  • PUT /api/image — Upload images (base64 JSON body)
  • GET /s/<slug>/img/<filename> — Serve images publicly with immutable caching
  • DELETE /api/image/<slug>/<filename> — Delete individual images
  • GET /api/images/<slug> — List all images for a note

Architecture

  • Images stored in the existing PUBLISHED_NOTES KV namespace with img:<slug>/<filename> keys
  • Image metadata tracked via img-meta:<slug> keys for efficient listing
  • No new KV namespaces or Terraform changes needed

Features

  • Markdown image rewriting: ![alt](img/file.jpg) auto-resolves to /s/<slug>/img/file.jpg at render time
  • Cascade delete: unpublishing a note deletes all associated images
  • 10MB max image size, allowlisted content types (jpeg, png, gif, webp, svg)
  • Immutable cache headers on served images (max-age=31536000)

Helper Scripts (in workspace)

  • scripts/upload-image.sh <slug> <filepath> — standalone image upload
  • scripts/publish-note.sh — updated to scan for img/ references and auto-upload

Tested

- PUT /api/image: upload images as base64 JSON, stored in KV with img: prefix
- GET /s/<slug>/img/<filename>: serve images with correct Content-Type and immutable cache
- DELETE /api/image/<slug>/<filename>: delete individual images
- GET /api/images/<slug>: list images for a note
- Markdown image rewriting: img/<file> references auto-resolve to /s/<slug>/img/<file>
- Cascade delete: unpublishing a note also deletes all associated images
- Image metadata tracked in KV (img-meta:<slug>) for efficient listing
- 10MB max image size, allowlisted content types (jpeg, png, gif, webp, svg)

@gisk0 gisk0 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: #4 — feat: add image support (upload, serve, manage)

Verdict: Approve with suggestions

Summary: Clean, well-structured addition of image support using the existing KV namespace. Auth, input validation, and cascade delete are all handled correctly. One security concern with SVG serving, and a couple of robustness items worth addressing.


🔴 Blockers (must fix before merge)

  • [src/index.ts:531] SVG XSS via image/svg+xml — SVGs can contain <script> tags and event handlers. Serving user-uploaded SVGs with Content-Type: image/svg+xml from the same origin enables XSS attacks. Either (a) remove image/svg+xml from ALLOWED_CONTENT_TYPES, (b) serve SVGs with Content-Disposition: attachment, or (c) serve images from a separate cookieless domain. Option (a) is simplest since this is for Obsidian note images where SVG is rarely needed.

🟡 Warnings (should fix, not blocking)

  • [src/index.ts:569-580] Race condition on image metadatagetImageMetas → modify → putImageMetas is a read-modify-write without atomicity. Concurrent uploads to the same slug can lose metadata entries. Low risk for single-user, but worth noting. KV doesn't offer CAS — if this ever becomes multi-user, consider a different data model.

  • [src/index.ts:714] rewriteImageUrls regex is narrow — Only matches ![alt](img/...). Won't match ![alt](./img/...) which Obsidian also generates. Consider: /!\[([^\]]*)\]\(\.?\/?\/?img\/([^)]+)\)/g

  • [src/index.ts:555] No note-existence check on image upload — Images can be uploaded to slugs that don't have a published note, creating orphans. Minor, since cascade delete only fires on unpublish — but orphaned images from typo'd slugs will persist in KV indefinitely.

💡 Suggestions (optional but worth considering)

  • [src/index.ts:598-600] handleServeImage — add Content-Length header — Browsers and CDNs benefit from knowing the response size upfront. Easy: "Content-Length": String(result.value.byteLength).

  • [src/index.ts:625-626] handleDeleteImage — no 404 on missing image — Delete returns { ok: true } even if the image didn't exist. Fine for idempotency, but a 204 vs 200 distinction (or a found: true/false field) could be useful for scripting.

  • JSON body for image upload — Base64 encoding inflates payload ~33%. For a 10MB limit that means the JSON body can be ~13.3MB. Not a problem now but if you ever need larger images, consider multipart/form-data. Current approach is fine for the use case.

✅ Good Stuff

  • Auth token check on all management endpoints — consistent security posture
  • FILENAME_RE validation prevents path traversal via filenames
  • Cascade delete on unpublish is the right call — no orphaned data
  • Immutable cache headers on served images — excellent for performance
  • Clean separation of helpers, route handlers, and routing logic
  • Image metadata tracking via separate KV key is smart — avoids KV list operations

Test Coverage: No automated tests in this PR (none in repo). Manual testing documented in PR description — acceptable for a personal tool, but worth adding integration tests as the API surface grows.

Security: SVG XSS needs addressing (see blocker). Everything else is solid.

Ready to merge? After SVG blocker is resolved.

- Remove SVG from allowed content types (XSS risk)
- Add race condition comment on image metadata read-modify-write
- Fix image URL regex to match ./img/ relative paths
- Verify note exists before accepting image uploads (prevents orphans)
- Add Content-Length header on served images
- Return 404 on delete when image doesn't exist
- Add comment about base64 payload size limitation
@chapati23
chapati23 merged commit c0af092 into main Mar 1, 2026
3 checks passed
@chapati23
chapati23 deleted the feat/image-support branch March 1, 2026 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants