feat(auth): proxy OIDC provider icons server-side (#1333)#1342
Merged
Conversation
Strict img-src CSP blocked external OIDC icon hosts on the login page.
Loosening CSP was rejected via the MakerWorld precedent, so icons are
proxied: admin sets icon_url, backend fetches and caches the bytes in a
deferred BLOB column, the SPA renders from a same-origin
/api/v1/auth/oidc/providers/{id}/icon endpoint.
* Streaming fetcher with 1 MB early-exit, MIME whitelist (PNG/JPEG/
WebP/GIF), follow_redirects=False, dedicated SSRF guard stricter
than the Spoolman variant (blocks loopback, RFC-1918, link-local,
cloud-metadata, multicast, numeric-encoded IPs, IPv4-mapped IPv6).
Shared primitives extracted to _url_safety.py and used by both
guards.
* httpx.InvalidURL caught explicitly as a sibling of HTTPError so
admins see 400 instead of 500. Pydantic icon_url validator
delegates to the runtime SSRF guard via lazy import (single
allowlist at both layers).
* DB-layer CheckConstraint enforces the (icon_data, icon_content_type,
icon_etag) all-null-or-all-set invariant. Fresh installs get it on
both dialects via metadata.create_all; stale PostgreSQL installs
via ALTER TABLE ADD CONSTRAINT in run_migrations. Stale SQLite gap
matches the existing default_group_id-FK trade-off.
* GET /icon (public, no auth) serves cached bytes with a strong ETag,
Cache-Control 1h, If-None-Match supporting the W/ weak-validator
prefix and the * wildcard. Disabled providers 404 to avoid leaking
their existence.
* DELETE /icon clears all four columns (icon_url plus the three cached
columns) so the admin form does not show a stale URL after removal.
* PUT with explicit icon_url=null clears the icon record (detected via
model_fields_set since exclude_none would silently drop None).
* POST /icon/refresh re-fetches from the stored URL for admin's
refresh button.
* All fetch and SSRF rejections log WARNING so operators have a
forensic trail beyond the admin's transient toast.
* Settings-backup type-mapping helper (_sqlalchemy_type_to_sqlite_type)
extracted with an explicit BLOB/BYTEA/BINARY branch so the
PG->SQLite-ZIP round trip preserves non-UTF8 icon bytes.
* Frontend: per-provider iconFailed sub-components fall back to
Shield/Globe instead of the browser broken-image glyph; SameOriginUrl
branded type guarantees the proxy URL is same-origin at compile time;
has_icon is required on the OIDCProvider type.
* Four i18n keys added to all eight locales (en, de, fr, it, ja,
pt-BR, zh-CN, zh-TW).
About 100 new tests cover streaming early-exit on the first oversized
chunk, the disabled-provider existence-leak guard, raw-SQL inconsistent
triplet defence, the dialect-conditional migration branch (BLOB vs
BYTEA via patched is_sqlite), CSP regression-guard, and validator
parity with the runtime SSRF check. All 20+ existing Spoolman SSRF
tests remain green after the shared-data refactor.
# Conflicts: # static/assets/index-BEsXo8iL.js # static/assets/index-D5ddq2Sh.js # static/assets/index-oQDjU99K.js # static/index.html
# Conflicts: # static/assets/index-CntkZk2s.js # static/assets/index-oQDjU99K.js # static/assets/index-r5aLap7j.js # static/index.html
Merged
5 tasks
…or OIDC icon proxy Four follow-on fixes on top of the icon-proxy work in PR maziggy#1342: - schemas/auth.py: drop the `= False` default on OIDCProviderResponse.has_icon so Pydantic fails loudly if any code path skips `_build_provider_response` and tries `model_validate(provider)` directly. - models/oidc_provider.py: add a `has_icon` @Property reading the non-deferred `icon_content_type` column. Single source of truth that feeds the required Pydantic field via `from_attributes=True` without touching the deferred BLOB. - routes/mfa.py (update_oidc_provider): fetch the icon BEFORE the setattr loop, not after. If `_fetch_icon_or_400` raises, the ORM object is still untouched, so the in-memory state stays consistent (the DB row is already safe via get_db()'s rollback — this closes the in-memory window too). - routes/mfa.py: redact admin URLs in WARNING log lines via a new `_redact_url_for_log` helper that strips query string and fragment. Admin-supplied icon URLs are usually harmless, but presigned URLs with `X-Amz-Signature=...` or bearer tokens in the query string should not end up in operator log files. - tests/__tests__/pages/LoginPage.test.tsx: new test asserting that two has_icon:true providers maintain independent iconFailed state — one img erroring must not hide the other. Locks in the load-bearing property of the OIDCProviderButton sub-component extraction so a future hoist of useState to the parent loop is caught by CI instead of in production.
maziggy
approved these changes
May 15, 2026
Merged
maziggy
added a commit
that referenced
this pull request
May 16, 2026
**Bambuddy v0.2.4.1** ⚠ **Upgrade Notes — Read Before Updating** Almost everyone is upgrading from 0.2.4. 0.2.4.1 is a patch release: stability and correctness fixes built on the same code base as 0.2.4, no schema breaks, no Docker entrypoint changes, no Vite/proxy quirks. The in-app Apply Update button in Settings → System → Updates resolves to the latest stable tag and works for all users — no flags needed. Make a backup before upgrading via Settings → Backup → Create Backup. Native install with update.sh snapshots the database automatically and rolls back on failure. Docker and fully-manual paths don't. **Docker** docker compose pull docker compose up -d docker-compose.yml doesn't need refreshing — none of the entrypoint, volume, or env-var conventions changed since 0.2.4. **Native install — recommended path** sudo BRANCH=main /opt/bambuddy/install/update.sh **Native install — manual path** sudo systemctl stop bambuddy cd /opt/bambuddy sudo -u bambuddy git fetch origin --tags sudo -u bambuddy git checkout main sudo /opt/bambuddy/venv/bin/pip install -r requirements.txt sudo systemctl start bambuddy **Behaviour changes to know about** - Reprint stats are now per-event, not per-archive. Re-printing a file no longer overwrites the source archive's totals; Quick Stats and the per-archive Print Log gain an orange N prints badge with a per-run breakdown (successful + failed). If you already had a reprint that overwrote stats in 0.2.4, the existing archive row keeps its current numbers — but every new print event from 0.2.4.1 onward writes a separate PrintLogEntry, so totals start adding correctly again. (#1378) - Pending queue items for soft-deleted archives are now auto-cancelled. Soft-deleting an archive (default delete path) removes its files from disk, which makes any pending queue item pointing at it un-dispatchable. From 0.2.4.1 those items get status=cancelled + waiting_reason="Source archive deleted" so you see why the queue item disappeared from pending instead of finding it silently stuck. (#1348 follow-up) - i18n parity check now blocks English-leak in non-English locale files. The build's parity step (npm run build) now fails CI when a non-English locale entry equals the English source unless explicitly allow-listed as a cognate. 2,377 accumulated English fallbacks across 7 locales were translated in this cycle as the underlying cleanup. No user-visible change today — just no more "Advanced" buttons in your German UI from new keys going forward. --- **Highlights** 0.2.4.1 closes correctness gaps that hit power-users running queues, reprints, and Obico fault detection at the same time. The three biggest are: per-event stats aggregation so reprints add to Quick Stats instead of overwriting (#1378), camera stream no longer freezes when Obico polls the same printer (#1348, reported by @SL666), and multi-color archive cost now charges untracked AMS slots at the default rate instead of reporting near-zero (#1344, reported by @nicktags). Around them: AMS slot configuration that survives Reset Slot on A1 Mini BMCU / P1S Standard AMS, MakerWorld URL import, queue/VP-dispatched prints finally getting layer timelapse, plate detection respecting the external camera setting, firmware checks staying alive when bambulab.com Cloudflare-blocks the page, LDAP manual provisioning, and a narrow API-key permission for the Home Assistant dynamic-tariff integration. Plus a deep i18n debt cleanup — 2,377 strings translated across 7 locales — and mechanical CI enforcement so the debt can't accumulate again. --- **New Features** - Manual LDAP user provisioning from the UI (#1298) — Add LDAP users into Bambuddy without waiting for their first login. Pre-create groups, permissions, and inventory ownership before the user even authenticates once. - Build-plate override in the SliceModal (#1337) — Pick which build plate (Cool / Cool SuperTack / Engineering / High Temp / Textured PEI / Smooth PEI) to slice for, independent of the source 3MF's embedded plate. The slice respects the override end-to-end (sliced output is bound to the chosen plate, archive metadata records it, printer card thumbnail matches). - API Keys: narrowly-scoped "Update electricity price" toggle (#1356) — New per-key permission flag exposes a single endpoint POST /api/v1/settings/electricity-price that accepts {"energy_cost_per_kwh": <float>}. Closes the gap where the wiki documented a Home Assistant dynamic-tariff rest_command example that was never deliverable (every key with general SETTINGS_UPDATE is hard-denied for security). The new flag does NOT widen general settings-write access — the broader PATCH /settings route remains denied. Wiki updated; existing keys default off. - Per-archive Print Log view + clickable "N prints" badge (#1378 follow-up) — Every archive card with more than one print event shows an orange N prints badge with a hover-tooltip breakdown (successful vs failed). Click it (or use the context-menu entry) to open a print log dialog showing every print event for that archive — date, status, duration, filament, cost — with failure_reason text under failed runs. Also embedded as a section at the top of the Edit Archive modal so the history is one click away. --- **Improved** - Reset Slot on A1 Mini BMCU / P1S Standard AMS no longer deadlocks Assign Spool (#1322, reported by @RosdasHH) — The empty-detection that gated ams_filament_setting was too cautious; now only short-circuits on state ∈ {9, 10} (firmware's explicit "no spool" codes) so the post-Reset-Slot "spool inserted, state=3, tray_type empty" case fires MQTT and configures the slot. Same Reset Slot click that previously sat in pending state forever now lands cleanly. - Multi-color archive cost now tops up untracked AMS slots at the default rate (#1344, reported by @nicktags) — A 110 g multi-color print with only one of four trays mapped to inventory used to show $0.01 instead of ~$1.10. Untracked slots now charge at the global default filament cost. Fully-tracked prints are unchanged. - Plate-detection calibration captures from the configured external camera (#1359, reported by @Andlar94) — On printers with an external RTSP / go2rtc camera enabled, calibration was previously sourcing the reference frame from the built-in chamber camera while the runtime check used the external one — guaranteeing a "Build plate not empty" false-positive on every print. Both paths now share the same external-camera default, with a backend-side derivation so future callers can't drift again. - Layer timelapse now starts for queue / VP-dispatched prints (#1353, reported by @Andlar94) — The timelapse start_session() call was only on the new-archive code paths. Queue dispatches and VP-dispatched reprints landed on the expected-archive branch and silently lost timelapse. The expected-archive branch now mirrors the same gate. - Firmware update dialog survives Cloudflare-blocked / transient outages on bambulab.com (#1350, reported by @K1ngJony) — Adds honest browser-like Accept / Accept-Language headers alongside the existing Bambuddy/1.0 UA, persists the resolved buildId to disk (so a single bambulab.com 403 doesn't permanently break download-URL resolution for that session), retries once on 404 (Bambu rebuilt the page), and shows an honest error message when the download endpoint truly can't be reached. - Subtype dropdown on the Add/Edit Spool form offers CF and GF (#1345) — Adding a third-party PETG-CF / PLA-CF spool no longer requires typing the variant by hand. - Page-header visual style unified across the app (PR #1272 by @EdwardChamberlain) — Every page now uses the same icon-aligned heading shape. - OIDC provider icons proxied server-side (PR #1342 by @netscout2001) — Icon fetches no longer expose the issuer's URL via browser request logs / DNS. - Auto-print start G-code now fires after the printer reaches RUNNING, not before (#1304) — The first RUNNING transition after Bambuddy boots no longer fires an unrelated print-start; users with custom G-code injection get their snippets at the actual start of each print, not the boot of the daemon. - i18n parity gate now enforces real translations in every locale, not English fallbacks — frontend/scripts/check-i18n-parity.mjs gains a new Check 4 that fails CI when a non-English leaf equals its English source unless explicitly allow-listed as a cognate. The 2,377 accumulated English fallbacks across the 7 non-English locales were translated as the underlying cleanup. Going forward, "English fallbacks per project convention" is not a thing — new keys must be translated in every locale or explicitly added to the per-locale IDENTICAL_TO_EN_ALLOWED cognate list. - Support bundle records more application state — Adds OIDC providers + 2FA / API key / long-lived token counts, library / inventory / queue / maintenance totals, slicer-API CLI versions, GitHub backup status, per-printer Obico flag. Redacts two settings that were previously included in cleartext and fixes a reachability-check architecture bug. Future triage rarely needs a follow-up "can you also send X" round-trip. --- **Fixed** **Stats / Archives / Print Log** - Reprints (including failed and cancelled ones) no longer overwrite the source archive's statistics (#1378, reported by @IndividualGhost1905) — Statistics are now event-based, not file-based. The existing PrintLogEntry table gains six columns (archive_id, cost, energy_kwh, energy_cost, failure_reason, created_by_id); /archives/stats and /metrics sum from it. Each print completion writes a new row with the run's actual filament / time / cost / energy / status. The cost overwrite at usage_tracker.py:633 and energy overwrite at main.py:3625 now both preserve the source archive's first-run values on reprints; the run's actuals are stored on the PrintLogEntry row instead. - Partial prints record accurate run filament (#1378 follow-up) — Failed / cancelled / stopped reprints no longer record the source archive's slicer estimate verbatim. New _compute_run_filament_grams helper prefers sum of tracked spool deltas, then falls back to estimate × progress%, then None — captured in 14 unit tests across every combination of status × inventory-tracked. - Print log no longer 404-storms thumbnails for entries whose archive was deleted or whose print failed before extraction (#1348 follow-up) — Two-part fix: route self-heals on first 404 (NULL the cached path on the entry so subsequent renders skip the request) + eager NULL at archive-delete time so future deletes don't fire the one-time storm. - Soft-deleted archive no longer leaves linked queue items silently stuck in pending forever (#1348 follow-up) — Pending queue items pointing at soft-deleted archives are now cancelled at delete time with waiting_reason="Source archive deleted". Queue API also suppresses the cached archive thumbnail / name / metadata when deleted_at is set, and the queue page's /plates query is gated on a new archive_deleted flag — three 404s per orphaned queue row are now zero. **Camera** - Camera stream no longer freezes every ~30s when Obico fault detection is enabled on the same printer (#1348, reported by @SL666) — Obico's _capture_frame was reusing the fan-out broadcaster's buffered frame when available, but falling through to a competing RTSP socket in race windows where the buffer was momentarily empty (stream startup, mid-reconnect). On X1-class firmware that allows only one camera connection, that second socket kicked the live viewer. New is_stream_active() helper now gates the fresh-socket fallback independently of the buffer state — when a viewer is connected, Obico never opens a competing socket. **AMS / Inventory** - Bare-tray empty-slot signal on P1S / A1 Mini (#1322 follow-up) — Genuinely-empty AMS slots on these printers send {"id": N} only (no state, no tray_type). The AMS parser now promotes this shape to state=9 so the inventory route's state ∈ {9, 10} short-circuit fires and we don't waste an ams_filament_setting publish that firmware would silently drop. - AMS slot configuration lands cleanly for spools with no k-profile — The "configure" call no longer 422s when the spool's filament has no calibration profile entry. Affects a long tail of third-party / generic-PLA spools. - AMS slot configuration lands on firmwares that never report state=11 (#1322 follow-up) — Some older firmwares never report the literal state=11 for loaded; the configure path's gate was too strict. Now treats absence of explicit empty (state ∈ {9, 10}) as loaded. - Spool removal from AMS on X1C firmware that reports power_on_flag=False while idle (#1365, reported by @an3k) — Empty-slot detection now narrows the skip to zero-bits + power_on_flag=False (the shutdown shape from #765) instead of any power_on_flag=False. Spool pulls between prints now register without a manual Reconnect. - AssignSpoolModal sits above the mobile sidebar drawer (#1336) — z-index fix; clicking Assign on mobile no longer opens the modal behind the drawer. - Catalog color's gradient + effect now applied, not just hex (#1340) — Picking a Bambu Lab gradient or sparkle entry from the colour picker now copies all three colour properties. - Storage location persists for internal spools (#1291) — Local-mode inventory now writes the storage_location field on save (was Spoolman-only). **Spoolman** - AMS-HT range allowed in slot-assignment table (#1274) — The ams_id upper bound was hardcoded at 4; AMS-HT extends the range. Now matches the parser's range. - External-spool ams_filament_setting uses global tray_id (#1279) — Was sending the local slot ID for external spools; firmware rejects. - Persist color_name edits without round-tripping the subtype synth fallback (#1319) — Editing the colour name on a Spoolman spool no longer reverts after the next AMS push. - Restore Spoolman spool ID search + Unassign button (#1336) — Two regressions from the Spoolman inventory UI work that landed in 0.2.4. - Resolve -1 in ams_mapping to external spool (#1276) — Bambu's multi-color slicer convention; the queue dispatcher now interprets it correctly. - Per-print 3MF tracking is the only weight writer (#1119) — Removes a competing path that double-wrote weights in Spoolman mode. - External library lookup filtered by Bambu Lab manufacturer (PR #1330 by @ojimpo) — Stops cross-manufacturer matches polluting the Bambu library picker. **Virtual Printer / Slicer** - VP cache preserves AMS / vt_tray / net.info across incremental push_status updates (#1371, reported by @Andlar94) — Slicer no longer needs a printer power-cycle to see AMS info on a queue-mode VP. The bridge's _latest_print_state cache now preserves a small set of sticky keys (ams, vt_tray, ams_extruder_map, mapping, net, ipcam, lights_report) when an incremental push omits them — mirrors what Bambuddy already does for its own internal state.raw_data. - VP emits FINISH after FTP upload so Print-flow slicers un-wedge (#1280) — BambuStudio's Print-flow path waits for FINISH before clearing its upload progress UI. - VP broadcasts archive_created so Archives page refreshes live (#1282) — Slicing through the VP now updates the open Archives page without a manual refresh. - VP queue-mode honours workflow default print options (#1235) — VP-dispatched prints now pick up the user's "Auto-Print start gcode" / "Auto-Off" / etc. defaults consistently. - Slicer bundle import logs the sidecar's reject reason (#1312 follow-up) — Failed .bbscfg imports now show the upstream error in the Bambuddy log so users can diagnose without curling the sidecar. **Scheduler / Dispatch** - Watchdogs no longer falsely treat FINISH → IDLE as "print landed" (#1370, reported by @Martinnygaard) — Queue items dispatched onto a printer that was in FINISH (un-dismissed "Print complete" prompt from a prior job) used to stay stuck at printing forever. The post-dispatch verifier now narrows the "command landed" check to an allow-list of active-print states (PREPARE / SLICING / RUNNING / PAUSE). - First RUNNING after Bambuddy boots no longer fires a phantom print-start (#1304) — Cold-boot of Bambuddy onto a printer that's mid-print no longer creates a stray archive at the boot moment. **Camera** - Plate-detection UI uses the external camera when configured (#1359) — Above under Improved. - Layer timelapse for queue/VP-dispatched prints (#1353) — Above under Improved. - Camera fan-out broadcaster buffered frame shared with Obico + /camera/snapshot (#1271) — Reuse path landed in 0.2.4; this cycle's #1348 fix completes the race-free version. Listed for completeness. **Notifications / Backups** - Discord webhook accepts legacy discordapp.com URLs (#1363, reported by @mrfoureyed) — Discord's Copy Webhook URL button still emits the legacy hostname; validation now accepts either. - Backup tab indicator dot for scheduled backups (PR #1338 by @chanakyan-arivumani) — Visual cue when a backup is queued. **Auth / LDAP / OIDC** - Manually-assigned groups preserved across LDAP logins (#1292) — LDAP user re-login no longer wipes admin-assigned group memberships. - Orphan OIDC / MFA rows cleaned up when user is deleted (PR #1295 by @netscout2001) — Deleting a user now cascades to their OIDC binding + TOTP secret rows. - Password rules shown in user-create form + FE/BE checks aligned (#1303) — Frontend rejected passwords the backend would accept and vice versa; now both apply the same rules and the form shows them. - External-scan STL thumbnails deferred + Path coerced (#1299) — External library scans no longer block on STL thumbnail rendering; mountpoints expressed as strings work alongside Path objects. - MakerWorld settings link points to /profiles (#1300) — The "Open Cloud settings" link from the MakerWorld page now goes to the right tab. **UI / Misc** - Smart-plug live wattage rounded to whole watts on the printer card (#1266, reported by @Carter3DP) — Plugs reporting fractional watts (ESPHome / HA-bridged) no longer overflow the card. - Settings UI rendering fields exposed without requiring SETTINGS_READ (#1293) — Non-admin users with narrower scopes can now load the Settings UI; the rendering-only fields (theme, locale) are no longer gated on admin-tier read. - Bed-jog Z direction inverted on A1 / A1 Mini bed-slingers (#1334) — Up was down on bed-slinger printers; now matches the physical motion. - Usage tracker: skip remain% fallback for trays not used by the print (#1269, reported by @maugsburger) — Swapping spools in unrelated AMS slots mid-print no longer charges the original spool the full estimate. - Soft-deleted archives keep their Quick Stats contribution (#1343) — Was already there for archive-level totals; this release locks it in via the new PrintLogEntry event aggregation (#1378) which references log entries by ON DELETE SET NULL, so the contribution survives even a hard delete. - scan_timelapse picked stale video at false offset (#1278) — Resolved. --- **Security** - urllib3 floor raised to 2.7.0 to clear CVE-2026-44431 and CVE-2026-44432. urllib3 is a transitive dependency (none of Bambuddy's top-level deps require >=2.7.0 yet), so the resolver was silently keeping the vulnerable 2.6.x line. requirements.txt now carries an explicit urllib3>=2.7.0 pin. - Bandit suppression syntax corrected on two verify=False calls in support.py — the two local-sidecar reachability probes used # noqa: S501 (ruff syntax, ignored by bandit) instead of # nosec B501. The probes themselves are unchanged (no payload, no secrets — health-check only) but the local security scan now passes cleanly without false-positive high-severity findings. --- **Contributors** Big thanks to everyone who shipped code or filed reproducible bug reports this cycle: Code: @netscout2001, @EdwardChamberlain, @chanakyan-arivumani, @ojimpo, @maziggy Reproducible bug reports: @IndividualGhost1905, @SL666, @nicktags, @Andlar94, @RosdasHH, @an3k, @K1ngJony, @Martinnygaard, @Fuechslein, @mrfoureyed, @maugsburger, @Carter3DP (See CHANGELOG.md for the full per-fix detail.)
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.
Description
Resolves the strict
img-srcCSP blocking external OIDC provider icons on the login page.Background. The SPA ships with
img-src 'self' data: blob:so the entire admin UI cannot hot-link arbitrary external images. When an admin configures an OIDC provider withicon_url=https://google.com/icon.png, the browser blocks the load and shows a broken-image glyph next to the login button.Two options were considered:
https:toimg-src). Simple, one-line change.icon_url, backend fetches and caches the bytes, the SPA renders from a same-origin endpoint/api/v1/auth/oidc/providers/{id}/icon.This PR takes option 2 because:
backend/app/services/makerworld.py:494already does the same pattern for MakerWorld CDN thumbnails, and the existing comment there explicitly justifies it: "avoids loosening the SPA's img-src CSP and keeps users' IP addresses out of MakerWorld's access logs." Looseningimg-srcfor OIDC icons would contradict the documented architectural decision.img-srcfor the entire SPA (all routes, all states, including authenticated pages). Option 2 keeps the strict CSP and adds a single same-origin endpoint that the SPA already trusts.img-srcwould have no such control.Related Issue
Fixes #1333
Documentation
Pick one:
Type of Change
Changes Made
Backend — icon fetcher and proxy
backend/app/services/oidc_icon.py(new): streaming fetch with 1 MB early-exit cap, MIME whitelist (PNG/JPEG/WebP/GIF),follow_redirects=False, 10 s timeoutbackend/app/api/routes/_oidc_helpers.py(new):assert_safe_public_https_url— stricter than the Spoolman SSRF guard (Spoolman allows loopback/RFC-1918 for same-LAN topology; OIDC must not)backend/app/api/routes/_url_safety.py(new): shared SSRF data (cloud-metadata IP set, numeric-IP regex, IPv4-mapped unwrap helper) consumed by both the Spoolman and OIDC guardsbackend/app/api/routes/mfa.py:GET /oidc/providers/{id}/icon— public (no auth, same rationale as/makerworld/thumbnail), serves cached bytes with strongETag,Cache-Control: public, max-age=3600, supportsIf-None-Matchweak prefixW/and*wildcard, returns 404 for disabled providersDELETE /oidc/providers/{id}/icon— clears all four icon columns (URL plus the three cached columns) so the admin form does not show a stale URL after removalPOST /oidc/providers/{id}/icon/refresh— re-fetches from the stored URL (Refresh button)POST/PUTintegrate the fetcher transactionally: failed fetch aborts with 400, leaving either no half-configured row (create) or the previous cached bytes intact (update)icon_url: nullclears the icon record (detected via Pydanticmodel_fields_set)Backend — model and migration
backend/app/models/oidc_provider.py: addsicon_data(LargeBinary,deferred=True),icon_content_type(String(20)),icon_etag(String(64))CheckConstrainton the model enforces the all-or-nothing triplet invariant (icon_data,icon_content_type,icon_etagare either all null or all set)backend/app/core/database.py: dialect-conditionalALTER TABLE ... ADD COLUMN icon_data BLOB|BYTEAplus PostgreSQL-onlyADD CONSTRAINTfor stale installs. SQLite stale-install gap matches the existingdefault_group_idFK trade-offBackend — supporting changes
backend/app/schemas/auth.py:_validate_icon_urldelegates to the runtime SSRF guard via lazy import — schema and route enforce the same allowlistbackend/app/api/routes/settings.py: extracted_sqlalchemy_type_to_sqlite_typehelper with explicitBLOB/BYTEA/BINARYbranch so the PG → SQLite-ZIP backup round trip preserves non-UTF8 icon bytesbackend/app/api/routes/support.py:has_iconnow derived fromicon_content_type(non-deferred) to avoid an async lazy-load on theicon_dataBLOBFrontend
frontend/src/api/client.ts:has_icon: boolean(required) onOIDCProvider;SameOriginUrlbranded type for the proxy URL helperfrontend/src/pages/LoginPage.tsx: per-providerOIDCProviderButtonsub-component owns a localiconFailedstate; on<img>error the SPA swaps in theShieldfallback instead of showing the broken-image glyph to anonymous usersfrontend/src/components/OIDCProviderSettings.tsx:ProviderIconAvatarsub-component with the sameiconFailedstate for admin preview; Refresh and Remove buttons appear when relevant; clearing the URL field sends explicitnullso the backend clears the cached bytesi18n
refreshIcon,removeIcon,iconRefreshed,iconRemoved,iconFetchFailed)Review findings (all addressed in this PR)
A full multi-agent review was run against the initial implementation. Findings ranked by confidence:
httpx.InvalidURLis a sibling ofhttpx.HTTPError, not a subclass; admin URLs with null bytes returned 500 instead of 400except httpx.InvalidURL→OIDCIconUrlError→ 400is_private / is_loopback / is_link_localwhile the runtime SSRF guard covered numeric-encoded IPs, cloud metadata, multicast and IPv4-mapped IPv6 — schema layer was weakerassert_safe_public_https_urlvia lazy import_fetch_icon_or_400raised 400 with no log line; six months later, "the OIDC icon stopped working" had zero forensic traillogger.warningon both SSRF rejection and fetch failure<img>had noonErrorhandler; admin preview'sonErrorsetdisplay: nonewith no fallbackiconFailedstate, fall back toShield/Globeclient.stream+aiter_byteswith early-exit at first chunk past the capALTER TABLEwas dead code in CI (project tests run on SQLite); a typo inBYTEAemission would slip silentlyis_sqlite()and asserts the emitted SQL stringhas_icon?: boolean(optional) on the frontend type but always populated by the backend_PNG_BYTES+ mock builder duplicated across three test filesbackend/tests/_fixtures/oidc_icon.py(mirrors existingbackend/tests/unit/services/mock_ftp_server.pycross-import pattern)backend/app/api/routes/_url_safety.py; the two top-level assertion functions remain separate because their policies deliberately differIf-None-Matchdid not handle theW/weak-validator prefix or the*wildcard_etag_matcheshelper covers both per RFC 7232Content-Typeheader produced the user-hostile message"unsupported content-type: ''"_resolve_content_typeraises"missing a Content-Type header"icon_content_typedeclared asString(50); longest valid value is"image/jpeg"(10 chars)String(20)CheckConstrainton the model plusALTER TABLE ADD CONSTRAINTfor stale PostgreSQL installsoidcProviderIconUrlreturned a barestring; callers couldn't distinguish same-origin URLs from arbitrary ones at the type levelSameOriginUrlbranded typeIf-None-Match: *, inconsistent-triplet defence — all silently uncoveredAdditional post-review fixes (caught during manual UI testing):
icon_urlfield silently kept the old URL because the frontend sentundefined(omitted byJSON.stringify) and the backend filteredNoneviaexclude_none=True. Fixed both sides: frontend now sends explicitnull, backend distinguishes "cleared" from "absent" viamodel_fields_set.icon_urland only cleared the cached bytes, producing a confusing half-state where the admin form showed a stale URL while the login page rendered the Shield fallback. Now clears all four columns — "Remove" means remove everything; to re-add an icon the admin re-types the URL.Screenshots
Testing
About 100 new tests cover:
httpx.InvalidURLmapping)localhost) are rejected here so the two guards do not silently converge_url_safetyprimitives in isolationALTER TABLEmigration (both BLOB and BYTEA paths via patchedis_sqlite())icon_urlpresent but no cached bytes → refetch on next save), DELETE/POST 404 paths, the disabled-provider existence-leak guardW/weak prefix,*wildcard, and multi-token listsicon_content_typeset,icon_datanull)https:inimg-src— protects the entire proxy pattern from a future "fix" that silently degrades itFrontend:
has_icon: true|false, mixed providers, and<img>error → Shield fallbackhas_icon: true|false, missingicon_url, Refresh/Remove buttons visibility, image error → Globe fallbackManually verified end-to-end against a live PocketID instance with multiple icon URLs.
Checklist
ruff check && ruff format --checkclean)#1333and the failure mode it prevents)Additional Notes
CheckConstraintis intentional. SQLite cannotALTER TABLE ADD CONSTRAINT, so stale SQLite installs rely on the application-layer enforcement (the same trade-off the existingdefault_group_idFKON DELETE SET NULLdocuments — seemodels/oidc_provider.pyarounddefault_group_id). Fresh SQLite installs and all PostgreSQL installs (fresh + stale) get the CHECK constraint.xlink:href, embedded references) that the cheap whitelist would not catch all of them; PNG/JPEG/WebP/GIF cover Google, Microsoft, GitHub, Pocket-ID and the major IdPs. SVG can be added as a follow-up with explicit sanitization if anyone asks.img-srcpolicy is unchanged. The CSP regression-guard test makes that explicit so a future contributor "fixing" a broken icon by relaxing CSP discovers the proxy pattern instead.upstream/devat the time of opening (merge commit on top, no rebase — history preserved).