Image assets (1/3): Signal K plugin secure upload, processing, worker pool, cache (replicates mxtommy/Kip #1080) - #54
Conversation
First slice of user-uploaded image assets (server). ImageStore is an Express-free, unit-tested core that: - detects image type by content magic bytes (png/jpeg/webp/gif/heic/svg), not the client extension/MIME, and rejects anything else; - sanitizes SVG with DOMPurify (strips scripts/handlers/external refs) and stores it as vector, rejecting SVGs that don't survive sanitization; - validates raster decodability + dimensions via sharp with a pixel-budget guard; - stores the original bytes plus a per-image sidecar JSON of metadata (id-addressed UUIDs, no client path), and supports list/getMeta/remove with id validation. Adds the server deps (multer, sharp, heic-convert, isomorphic-dompurify) and runs all kip-plugin test files. On-demand WebP re-encode/resize + cache + worker pool and the HTTP routes build on this next.
Builds the serving layer on top of ImageStore: - image-processing: convert/resize a validated original to WebP, preserving animation (GIF/WebP) and decoding HEIC/HEIF via heic-convert; widths snapped to an allow-list; worker-count helper = n-1 CPUs (clamped to 1). - worker-pool: runs processing in a worker_threads pool (default n-1) so the SK server stays responsive; concurrent requests for the same variant are coalesced into one job. - image-worker: the pool's worker entry (sharp.concurrency(1) per worker). - ImageStore.getServable: serves sanitized SVG as-is, or an on-demand WebP variant that is cached on disk and re-served thereafter; raster is always re-encoded (raw original never served); every response carries nosniff + restrictive CSP + inline headers. Adds cacheStats()/purgeCache() for the settings UI. 22 node:test cases cover snapping, animation preservation, on-demand+cache, SVG passthrough, cache stats/purge, safe headers, and worker-pool sizing/coalescing. Fixes the plugin test script to run every *.test.cjs file.
image-router registers the /images endpoints on the plugin's Express router: - POST /images upload (multer, 10MB, login required, auth checked before parsing) - GET /images list the shared library - GET /images/:id?w= serve a cached WebP variant (raster) or sanitized SVG, with safe headers - DELETE /images/:id remove (login required) - GET/DELETE /images/cache cache size + purge (purge login required) Auth: SK's security middleware is the primary gate (as for the existing displays routes); a defensive isAuthenticatedRequest() also checks the SK-set req.skPrincipal so anonymous users are refused when security is enabled, while a server with no security configured stays permissive. Cache routes are registered before /images/:id so "cache" is not matched as an id; ids are UUID-validated. index.ts lazily builds the ImageStore + worker pool on first route use (the data dir is only known after init) and tears the pool down on stop(). 8 route tests cover auth rejection, listing, serving + headers, id validation/404, cache stats, and not-ready 503. Full plugin suite: 87 green.
…view HIGH — worker pool could permanently wedge all image serving: a worker that died via native crash/OOM/hard-exit (no 'error' event) silently left the pool, which never respawned, so queued jobs (and their HTTP requests) hung forever. The pool now detects deaths via BOTH 'error' and 'exit', rejects the in-flight job, and spawns a replacement, and a per-job timeout rejects+replaces a hung worker. (tests: crash-recovery, timeout-recovery) HIGH — HEIC decode bypassed the pixel budget: heic-convert decodes the full raster before sharp, so a 12KB/49MP HEIC forced ~632MB per serve x 7 widths. HEIC is now transcoded to a canonical WebP ONCE at upload, bounded by a strict 24MP HEIC cap, and served as cheap WebP resizes thereafter (no per-request HEIC decode). LOW — un-serveable "poison" assets: a non-HEVC HEIF / brand-spoofed AVIF detected as HEIC was stored but failed on every GET. The ingest transcode now proves decodability, so such files are rejected up front; the HEIC brand allow-list is narrowed to HEVC brands. (test: AVIF-as-HEIC) LOW — no upload quota let an authenticated user fill the shared data volume over time. Added a configurable image-count + total-bytes cap, checked before any decode work. (tests: both quotas) Full plugin suite: 92 green.
…egexes DOMPurify was only forbidding the legacy xlink:href attribute, so a modern href on <image>/<a>/<feImage> survived sanitization — an external href is a phone-home beacon in the stored SVG bytes, which is supposed to be self-safe (defense in depth beyond the serve-path sandbox CSP). Forbid 'href' as well. Internal references drawings rely on (fill="url(#g)", filter="url(#f)") are not href attributes and are preserved; added a test covering both. Also replace two regexes that contained literal control bytes (a raw BOM and a raw NUL..0x1f range) with \uFEFF / \x00-\x1f escape sequences. Behavior is identical, but the source no longer reads as a binary file to git/grep/editors.
node --test only expands glob patterns on Node 21+. The quoted "kip-plugin/tests/*.test.cjs" was treated as a literal path on the Node 20 CI leg, failing with "Could not find". Drop the quotes so the shell expands the glob, which works across the 20/22/24 matrix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ETfeXXs8R5ciw6nGxGYj2C
Content review: accept-with-fixes — strong ingest/sanitization, but verified DoS + authz gaps to close before mergeReviewed the full implementation ( What's genuinely solid (verified in code):
Findings:
Plus lows: variant cache has no size eviction (unauth GETs can force ~3.5 GB across 500 images × 7 widths on the shared SK data volume), TOCTOU-racy quota under concurrent uploads, orphaned originals never reclaimed on partial write, non-atomic cache/original writes, and a clean-idle-exit zombie worker burning one job into a timeout. Bottom line: the ingest/sanitization path is well-built and I couldn't smuggle a malicious file or stored XSS through it. But before this runs unattended, the two HIGH OOM vectors (unbounded queue + no serve-path backpressure), the open read routes, and the authn-only write gate should be fixed — taking down the Signal K server is the worst failure mode here. (Resolution applied earlier: conflict-free; CI fix — unquoted the plugin-test glob so Node 20's |
… review) Address the DoS and authz findings from the content review: - getServable now coalesces concurrent identical variant generations into one read+decode and caps concurrent DISTINCT generations (ImageStore GenerationLimiter), so an unauthenticated serve flood can no longer multiply full-size originals in memory and OOM the Signal K host. Excess distinct requests queue to a bound, then get a fast 503. - WorkerPool: bound the job queue (was unbounded), and reject queued + in-flight jobs on destroy() instead of leaking their promises. - multer: cap non-file multipart parts (fields:0) — busboy otherwise defaults to an unlimited number of 1 MB text fields buffered in memory. - Gate GET /images (list) and /images/cache: both are fetched by the authenticated HTTP client and expose library metadata / uploader ids. The /images/:id serve route stays open by necessity (<img> can't carry the bearer token); its DoS is now bounded above rather than by auth. - Write files atomically (temp + rename) so a mid-write crash can't serve a half-written original or variant. Not fixed here (needs verification against a security-enabled SK server, not possible in this repo): the write gate checks authentication, not write permission, and fail-opens when SK populates no principal fields. SK's server-api exposes no principal/permission surface to key these off. Tests added: coalescing, concurrency-cap 503, queue bound, destroy rejection, read-route gating, serve-open, 503 mapping. Plugin suite 100/100 on Node 24; 0 fail on Node 20. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ETfeXXs8R5ciw6nGxGYj2C
Fixes pushedAddresses the DoS + reliability findings from the review:
Tests added: coalescing, concurrency-cap 503, queue bound, destroy rejection, read-gating, serve-open, 503 mapping. Plugin suite 100/100 on Node 24, 0 fail on Node 20; CI green. Deliberately not fixed here (flagging so it isn't lost): the write gate still checks authentication, not write permission — a read-only SK account can still upload/delete/purge — and it fail-opens if SK sets no principal fields on plugin routes. |
2 participants
Replicates upstream PR mxtommy/Kip#1080 ("Image assets (1/3): Signal K plugin secure upload, processing, worker pool, cache") by dillan.
Method: commit-by-commit cherry-pick (5 commits, linear history, no merge commits). Each commit's original author was preserved. The package.json and package-lock.json changes auto-merged cleanly with no conflict; SKip identity (
@halos-org/skip, version 4.8.0) was retained while the PR's new dependencies (heic-convert,isomorphic-dompurify,multer,sharp) and thetest:pluginscript change were incorporated.Note:
package-lock.jsonwas auto-merged by git and will need regeneration (npm install) before a clean build, since dependency installation/build was intentionally not run during replication.This is an experimental replica carried in the fork for evaluation. It needs a build + review before merge.