Skip to content

feat: custom wallpaper background with brightness adjustment - #624

Closed
renheqiang wants to merge 11 commits into
nesquena:masterfrom
renheqiang:feat/wallpaper
Closed

renheqiang wants to merge 11 commits into
nesquena:masterfrom
renheqiang:feat/wallpaper

Conversation

@renheqiang

Copy link
Copy Markdown
Contributor

Summary

  • Upload a custom background image (JPEG / PNG / WebP, ≤ 10MB) from Settings → Theme → Wallpaper. Image fills the entire viewport behind all UI.
  • Brightness slider (10–150%) with live CSS-variable preview; persisted in settings.json via the existing /api/settings POST flow.
  • Thumbnail preview in the settings panel so users see what's currently set, with a Remove button to revert to the theme color.
  • Strict magic-byte validation — rejects .svg / HTML / mis-tagged containers regardless of client Content-Type. WebP requires both RIFF prefix AND WEBP at offset 8 (catches mis-tagged AVI/WAV).
  • Centered modal alert (not a corner toast) when uploading > 10MB, so the failure is unmissable.
  • Single-slot file replacement: uploading a new image deletes the old one (via glob + unlink) before writing.
  • Bonus: closes a pre-existing race in save_settings by adding _SETTINGS_WRITE_LOCK (covered by a dedicated concurrent-write test).

Architecture notes

  • Image stored at ~/.hermes/webui/wallpaper-<sha1[:8]>.{ext}; filename hash doubles as cache buster (Cache-Control: public, max-age=31536000, immutable).
  • Frontend uses a dedicated <div id="wallpaper"> at position:fixed; z-index:-1 with filter: brightness(var(--wallpaper-brightness)).
  • CSS change: moves background:var(--bg) from body to html, sets body{background:transparent}. Theme color is the bottom-most fallback layer; semi-transparent chrome (.main, .topbar, .composer-wrap) composites the wallpaper through.
  • Backend: 4 routes (POST /api/wallpaper, POST /api/wallpaper/delete, GET /api/wallpaper, GET /api/wallpaper/info) in 1 new module api/wallpaper.py.
  • POST uses raw bytes with file's MIME as Content-Type — simpler than multipart for a single-file upload, no coupling to existing parse_multipart helper.

Test plan

  • 16 new pytest cases in tests/test_wallpaper.py: 7 unit (storage module + magic-byte) + 6 HTTP (endpoints + cache headers + error responses) + 3 settings (brightness range validator + concurrent write race)
  • Full webui suite: 1337 passed, 0 new regressions (2 pre-existing test_model_resolver failures unrelated)
  • Playwright end-to-end: 21/21 — boot with no wallpaper, upload + apply, brightness CSS variable + clamping, cache headers, magic-byte rejection, remove, settings UI presence
  • Manual checklist added to TESTING.md under "Wallpaper" (14 items)
  • Smoke-tested in browser: upload JPEG/PNG/WebP, drag brightness slider, refresh persistence, magic-byte rejection, theme switching with wallpaper present, thumbnail preview shows current file

Notes for reviewer

  • 9 focused commits, each independently reviewable. The body{background}html{background} swap is its own commit (7a22ab5) for easy bisect if any visual regression appears.
  • _SETTINGS_WRITE_LOCK (20abf8d) closes a load-merge-write race that affected ALL settings POSTs, not just wallpaper. Marked as bonus fix in the commit message.
  • New i18n keys added to all 5 supported locales (en, zh, es, de, zh-Hant). Verified by existing locale-parity tests.

🤖 Generated with Claude Code

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

This is a well-scoped, well-documented PR. The feature is genuinely useful and the implementation is thorough. A few questions and observations before merge:

Architecture ✅

The layered approach (html{background}body{background:transparent}#wallpaper fixed layer → semi-transparent chrome composites through) is the right design. Moving background from body to html is a meaningful change — making it a standalone commit (7a22ab5) for easy bisect is thoughtful. The SHA1-based filename as cache buster is clean.

Magic-byte validation ✅

The strict rejection approach (RIFF+WEBP offset check, explicit rejection of SVG/HTML) is the right security posture for a server that serves back user-uploaded files. The mis-tagged AVI/WAV edge case catch is a nice detail.

_SETTINGS_WRITE_LOCK bonus fix ✅

This is a real race that affects all settings POSTs. Good catch. The concurrent-write test coverage is important.

Specific questions / items to verify

1. Serving the wallpaper file — what's the Content-Type on GET /api/wallpaper?

The stored filename preserves the original extension (wallpaper-<hash>.jpg / .png / .webp). When serving the file back, is the Content-Type response header set explicitly from the extension, or does Python infer it? Browsers are tolerant but explicitly setting it (image/jpeg, image/png, image/webp) is cleaner and avoids any edge-case where mimetypes.guess_type returns None.

2. POST /api/wallpaper/delete — authorization check

The description mentions a /api/wallpaper/delete route. Is this endpoint protected by the same auth middleware as the other API routes? A rogue request that can delete the wallpaper is low-risk but worth confirming.

3. Brightness clamp at the validator level

The description says the brightness slider is 10–150%. Is this range validated server-side on POST /api/settings, or only client-side? If a request with wallpaper_brightness: 999.0 comes in, does the float-range validator catch it? (The test for "brightness range validator" suggests yes — just confirming that test exercises the server validator, not a JS-side clamp.)

4. #wallpaper z-index and modal overlap

The #wallpaper div is at z-index:-1. Any element that creates a new stacking context with its own z-index will render above the wallpaper regardless (e.g., the topbar, modals). That's intended — just want to confirm there's no case where a themed element with z-index:0 and background:transparent accidentally lets the wallpaper bleed through into a panel or modal overlay. The semi-transparent chrome design accounts for this, but worth a quick sanity check in the Playwright smoke test.

5. Large file upload UX — progress indicator

10MB is not instantaneous on a slow connection. Does the upload flow show any loading state between the user selecting a file and the preview appearing? If the endpoint blocks (synchronous write), the UI might appear frozen. Even a simple spinner or disabled button during upload would be a nice polish item (not a blocker).

6. Cache-Control: immutable and theme switching

The wallpaper URL includes a hash (wallpaper-<sha1[:8]>.ext) and the response is Cache-Control: public, max-age=31536000, immutable. When the user replaces the wallpaper, the new filename has a different hash, so browsers will fetch the new URL. But what happens to the old cached URL in the browser? This is fine functionally (old URL is just orphaned), but confirming that the frontend correctly updates the <img src> on the settings panel thumbnail and the #wallpaper background-image CSS var to the new URL after upload.

Summary

This is solid work. Item 3 (server-side brightness clamp) and item 6 (frontend URL update on re-upload) are the most important to verify before merge. Items 1, 2, 4, 5 are nice-to-have or minor polish items.

Happy to approve once items 3 and 6 are confirmed.

…nsive

When _settingsDirty is true, clicking the X button shows the unsaved-
changes bar instead of closing the modal. Previously the bar was
prepended into .settings-main with no scroll/visibility hint, so users
who had scrolled past the top of the panel saw no reaction at all.

Newly observed because the wallpaper batch added a brightness slider
whose input event marks dirty — easier to land in this state.

Fix:
  - position:sticky;top:0;z-index:5 keeps the bar visible regardless
    of scroll position inside the settings body
  - scrollIntoView + brief flash/scale animation on (re)show so even
    a repeat click on X gives clear feedback
  - stronger background + backdrop-filter for visual weight
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

The new commit 55ae71db (sticky unsaved-changes bar) is a clean, focused fix — position:sticky;top:0;z-index:5 plus a brief scale animation is exactly the right approach for this. Good catch that the wallpaper brightness slider made the dirty state easier to land in.

On the earlier open questions from my first review — following up on items 3 and 6 which I flagged as most important:

Item 3 (server-side brightness clamp): If the dedicated concurrent-write test confirms the validator rejects out-of-range values at the server layer (not just client-side), that's sufficient. Just confirming the test isn't only exercising a JS clamp.

Item 6 (frontend URL update on re-upload): After uploading a new wallpaper (new hash → new filename), does the frontend update both the <img src> in the thumbnail preview and the background-image CSS on the #wallpaper div to the new URL? The Cache-Control: immutable header means the browser will cache indefinitely at the old URL — so if the frontend accidentally reuses the old URL string for the new image, users would see a stale wallpaper until a hard refresh. Worth a quick Playwright assertion that the URL changes after re-upload.

Once those two are confirmed, this is ready to merge. The overall implementation quality is high.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Good feature concept and solid test coverage (16 pytest + 21 E2E). Three blockers to fix before merge:

Required:

  1. Security — path traversal via /api/settingswallpaper_file is in _SETTINGS_ALLOWED_KEYS, so POST /api/settings {"wallpaper_file": "../../../etc/passwd"} sets it, and GET /api/wallpaper will attempt to serve that path. The magic-byte check limits real damage (only valid image files pass), but the vector is real and /api/wallpaper/info leaks the traversal path back in the JSON response. Fix: either exclude wallpaper_file from _SETTINGS_ALLOWED_KEYS (it has no business being user-settable directly — only save_wallpaper() should update it), or add strict filename validation: ^wallpaper-[0-9a-f]{8}\.(jpg|png|webp)$. Please add a regression test.

  2. Unresolved merge conflict in static/boot.js line 676 — the file contains committed <<<<<<</=======/>>>>>>> markers which cause a syntax error (node --check fails). The conflict is between await bootCommands() (upstream) and applyWallpaper() (this PR). Both are needed — keep both calls.

  3. Conflict with PR Replace color scheme system with light/dark theme + accent skins #627 (color scheme overhaul) — this PR sets body{background:transparent} (necessary for the wallpaper layer to show through), while Replace color scheme system with light/dark theme + accent skins #627 sets body{background:var(--bg)} on the same declaration. The index.html settings panel is also structurally incompatible between the two PRs. These need to be coordinated — either land in a defined order with an explicit rebase plan, or the authors collaborate on a joint resolution. The _SETTINGS_WRITE_LOCK race condition fix from this PR must be preserved in whichever version merges.

Strengths worth noting: magic-byte validation is solid (WebP double-check is correct), 10MB cap enforced at both client and server, caching with immutable headers + hash-based filenames is the right approach, test coverage is thorough.

The first reviewer flagged that wallpaper_file is in _SETTINGS_ALLOWED_KEYS
but had no validator. An authenticated user could POST /api/settings with
{"wallpaper_file": "/etc/passwd"} or {"wallpaper_file": "../../foo.jpg"}
and the value would persist. GET /api/wallpaper would then attempt to
serve it; the magic-byte sniff in read_wallpaper blocks /etc/passwd-style
non-image files, but ANY image file readable by the webui process anywhere
on disk (e.g. another user's avatar, screenshots in /tmp) would leak.

Fix: add an anchored regex validator in save_settings that requires the
value to either be None (clears the wallpaper) or match the exact format
save_wallpaper() writes: wallpaper-<8 hex>.{jpg,png,webp}.  Same pattern
already used for language code validation.

Added two regression tests covering:
  - 12 invalid inputs (paths, traversal, wrong extensions, types, etc.)
    are silently rejected without overwriting the prior value
  - All three valid extensions round-trip correctly
  - None correctly clears the field

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@nesquena

Copy link
Copy Markdown
Owner

Independent End-to-End Review — PR #624

Independent review picking up the path-traversal blocker from the prior reviews. Pushed a fix.

TL;DR

One blocker fixed, two remaining for maintainer/author. Pushed a wallpaper_file validator (commit d1c764f) that closes the path-traversal blocker. The other two open items (CSS theme-skin coordination with #627, #629 polish follow-ups) are tracked but not blockers I can resolve from this branch.

Path traversal blocker — FIXED ✅

The first reviewer correctly identified that wallpaper_file is in _SETTINGS_ALLOWED_KEYS with no validator. A POST /api/settings {"wallpaper_file": "/etc/passwd"} (or "../../etc/passwd") would persist, and GET /api/wallpaper would attempt to serve the path.

The magic-byte _sniff() in read_wallpaper() blocks non-image files (returns FileNotFoundError on /etc/passwd), so the most obvious disclosure path is mitigated. But this leaves a real residual risk:

Any image file readable by the webui process, anywhere on disk, can be exfiltrated by setting wallpaper_file to its path and then GETing /api/wallpaper. Examples:

  • Another user's avatar/photo on the same machine
  • Screenshots in /tmp
  • Cached images in ~/.cache
  • Container layer images, etc.

Fix pushed in d1c764f: added a regex validator _SETTINGS_WALLPAPER_FILE_RE = ^wallpaper-[0-9a-f]{8}\.(jpg|png|webp)$ matching the exact format save_wallpaper() writes (hashlib.sha1(raw_body).hexdigest()[:8] + extension). The validator follows the same pattern as the existing language field check — silently ignores invalid input rather than raising.

Regression tests added (12 invalid inputs verified rejected):

  • /etc/passwd (absolute path)
  • ../../etc/passwd, ../wallpaper-deadbeef.jpg (path traversal)
  • wallpaper-deadbeef.svg, wallpaper-deadbeef.jpg.gz (wrong extension)
  • wallpaper-DEADBEEF.jpg (wrong case)
  • wallpaper-12345.jpg, wallpaper-deadbeefcafe.jpg (wrong hex length)
  • myown.jpg (not a wallpaper-* prefix)
  • Empty string, integer, dict (wrong types)

Plus a positive test confirming all three valid extensions round-trip.

Other items from the prior review thread

Item from review-2 ("merge sequence with #627") — not addressed by me. PR #627 introduces the new theme/skin axis system; this PR's body { background: var(--bg) } and html { background: ... } patterns coexist with #627's changes but require careful coordination on merge. Whoever merges should:

#629 polish follow-ups — already tracked as a separate issue, not blocking.

Tests ✅

  • 1349 passed, 42 skipped, 0 failed (full suite in isolated worktree, after my fix)
  • All 18 tests in test_wallpaper.py pass, including 2 new regression tests for the validator

Security audit ✅ (after fix)

  • Path traversal: ✅ now closed by the regex anchor
  • Magic-byte sniff: ✅ already in place as defense-in-depth
  • File-exfiltration via _purge_old_files: ✅ globbing only wallpaper-* in STATE_DIR (cannot delete arbitrary files)
  • Brightness range: ✅ float-range validator [0.1, 1.5] already in place
  • Upload size cap: ✅ 10MB limit on POST /api/wallpaper
  • Allowed image types: ✅ JPEG/PNG/WebP magic-byte check
  • No new auth bypass, no new endpoints with user-controlled file paths

Summary

Aspect Status
Tests ✅ 1349 passed, 0 failed (+2 regression tests)
Security: path traversal blocker ✅ Fixed in d1c764f
Security: magic-byte sniff (DiD) ✅ Already present
#627 merge coordination ⏳ Maintainer call (whichever lands first sets the pattern)
#629 polish follow-ups ⏳ Tracked separately, not blocking
CHANGELOG / version bump ⏳ Add at merge time (depends on #627 ordering)

Path traversal blocker is closed. Remaining open items are coordination-with-other-PRs concerns rather than issues with this PR's content.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Quick status: the path-traversal blocker was fixed by the prior reviewer (commit d1c764f). Two items remain before merge: (1) CSS coordination with the skin/theme system (#627/#629) — the wallpaper brightness CSS may conflict with the CSS variable approach used in the theme system; and (2) the wallpaper should degrade gracefully when the stored file is deleted or moved. If you'd like to continue, a rebase onto current master (v0.50.92) is the first step. Happy to assist with the rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants