* fix(video): try metadata extraction and thumbnail generation independently
processUploadedVideo() gated everything behind isValidVideo(), which
rejects the whole video if ffprobe can't read even one of
duration/width/height -- common on some iPhone/Lightroom-exported
MP4s (issue 1370). Callers (photoProcessor.js's processPhoto and
processUploadedPhotos) already catch that throw and fall back to a
static placeholder thumbnail plus a metadata-only retry (codex
review of #845), but that fallback never got a REAL thumbnail even
when generateVideoThumbnail() would have succeeded on its own --
thumbnailing doesn't need valid duration/width/height, it just seeks
and grabs a frame.
processUploadedVideo now tries metadata extraction and thumbnail
generation independently, keeping whichever succeeds instead of
discarding both on a single failed field. The callers' existing
throw handling stays as a backstop.
Also: extractVideoMetadata stored duration as 0 (not null) whenever
ffprobe had no duration field, masking "unknown" as a fake real
zero-second clip and defeating downstream `duration != null` checks
meant to skip an untrustworthy value.
Relates to issue 1370
* fix(video): fall back to the SVG placeholder when thumbnail generation fails
processUploadedVideo could return success with thumbnailKey: null
when only thumbnail generation failed. The gallery grid
(GridGalleryLayout/JustifiedGalleryLayout) falls back to
`photo.thumbnail_url || photo.url` when there's no thumbnail, so
AuthenticatedImage downloaded the full original video and tried to
render it as an <img> -- a broken tile and a potentially huge
fetch just from opening the gallery.
Falls back to the same ffmpeg-free SVG placeholder the callers
already generate for a total processing failure, so a bare
thumbnail-generation failure degrades to that placeholder too,
never to "no thumbnail at all".
Found by codex review.
* fix(video): avoid a SQLite connection deadlock in the placeholder fallback
generateVideoPlaceholder() unconditionally called getThumbnailSettings(),
which queries the database directly (not through any active transaction).
videoProcessor.js's new placeholder fallback can run from inside
processUploadedPhotos' open per-file SQLite transaction (chunked video
upload) -- knex's default SQLite pool has exactly one connection, so
that second, un-transacted query deadlocks against the transaction
holding it, timing out after acquireConnectionTimeout (60s). Reproduced
directly against an isolated SQLite db.
generateVideoPlaceholder now skips the settings lookup entirely when
the caller supplies explicit width/height, and the video fallback
passes the same DEFAULT_THUMBNAIL_WIDTH/HEIGHT the settings lookup
would have fallen back to anyway (now exported for reuse).
Found by codex review.
* fix(video): throw when neither a real thumbnail nor the placeholder can be produced
processUploadedVideo returned success with thumbnailKey: null when
both the real thumbnail AND the SVG placeholder failed -- a total,
systemic failure (storage backend down, disk full), not a quirk of
one file. On stable, which doesn't have the #845 call-site fallback,
this silently completed the video with no thumbnail at all instead
of the retryable 'failed' status a throw here produces. On main,
the pre-existing #845 fallback already absorbed this exact case
(no behavior change there) -- verified against codex's own
git-blame check of the pre-PR stable code before applying this.
Now throws in that case, restoring the pre-existing "let the caller
mark it failed and retryable" behavior for a genuinely unrecoverable
video, while keeping every partial-failure case (the vast majority)
resolving with whatever succeeded.
Found by codex review.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(backup): stop ignoring the configured database-backup destination path
databaseBackupService.getBackupConfig() returns the raw
database_backup_*-prefixed setting keys, but backup() and
startScheduledBackups() destructured unprefixed names off that
object (destinationPath, compress, enabled, schedule,
retentionDays, emailOnSuccess/Failure). None of those keys ever
existed on the config object, so every read silently fell through
to its hardcoded default.
The visible symptom (reported in issue 1365): the inline database
dump that runs before every file backup (default ON) always tried
to create /backup/database, regardless of what an admin configured,
and died with EACCES on the read-only default path — before the
file backup's own (correctly wired) backup_destination_path was
ever reached. The standalone scheduled database-backup runner had
the same bug: config.enabled was always undefined, so it silently
never started regardless of database_backup_enabled.
Also fixes saveManifestToLocal's manifest-directory fallback,
which hardcoded /backup instead of matching the sane
getStoragePath()/backups default used everywhere else for a
missing backup_destination_path.
Relates to issue 1365
* fix(backup): reject a database-backup destination inside a public static mount
Making database_backup_destination_path actually take effect
reopens a GHSA-jw8m-43r2-jqrm-class exfiltration path: that
setting is writable via PUT /api/admin/database-backup/config
under backup.create alone (the built-in admin role has it
without settings.edit or backup.restore), with no path
validation. Before this fix the setting was silently ignored
(the destructuring bug), so pointing it at the public
uploads/logos or fonts mount was harmless; now that it is
honored, it needed the same defense GHSA-jw8m already applies
to the per-request override.
Rejects the setting at both the config write (immediate 400)
and, defensively, at backup() time before mkdir.
Found by codex review.
* fix(backup): close two gaps codex round 2 found in the destination guard
- The public-roots list missed the bundled fallback fonts dir
(backend/assets/fonts, also mounted at /fonts, and nodejs-owned
per the Dockerfile's COPY --chown so it's writable at runtime).
- The comparison was case-sensitive; on a case-insensitive-but-
preserving filesystem (APFS, NTFS, Docker Desktop bind mounts of
either) STORAGE_PATH/UPLOADS/Logos names the same directory as
uploads/logos on disk. Now compares lowercased.
- database_backup_retention_days reached cleanupOldBackups
unvalidated. A value <= 0 pushes the cutoff to today or the
future, deleting every completed backup on the next scheduled
run -- a backup.create holder achieving what backup.delete
gates on the manual /cleanup route. Rejected at config-write
time (400) and defensively inside cleanupOldBackups itself.
- The scheduled-backup cron callback closed over retention_days
from schedule-start time; a retention-only /config update
(which doesn't restart the schedule) ran stale until restart.
Re-reads it on every tick instead.
Found by codex review, round 2.
* fix(backup): resolve symlinks and add the all-in-one frontend dir to the destination guard
Codex round 3 found two more bypasses of the public-root guard,
both specific to the all-in-one image (Dockerfile.aio):
- /app/frontend/dist (FRONTEND_DIR) ships nodejs-owned and is
served unauthenticated as the built SPA -- missing from the
protected-roots list.
- /app/storage is a symlink to /data/storage (the actual
STORAGE_PATH). A destination given as /app/storage/uploads/logos
passed the guard's lexical path.resolve() comparison while
resolving, on disk, to the exact same directory as the protected
STORAGE_PATH/uploads/logos. isUnderPubliclyServableRoot now
resolves symlinks in whatever prefix of each path already
exists (resolveRealish) before comparing, rather than relying on
path.resolve() alone.
Also restores three fs.mkdir spies in the test file that were
never un-spied, which silently leaked a rejected mock into any
later test doing a real fs.mkdir -- exactly what the new symlink
test needed to set up its fixture.
Found by codex review, round 3.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(usage): explain and de-emphasize the pending-packet button lock
An admin whose report delivery is stuck (old schema, network issue,
etc.) saw the v5-upgrade and portal buttons greyed out with no
indication why, or what to do about it — the backend's single-packet-
in-flight guard is correct, but silent. Add an inline note pointing at
"Retry / send if due" when a pending packet is the actual cause.
Also: "Review expanded usage.v5 scope" didn't read as an upgrade
action — renamed to "Upgrade to usage.v5" / "Auf usage.v5 upgraden".
"Open usage portal" is now a primary (green) button in both its
signed-in and pre-participation forms, matching the visual weight of
the other primary actions on this tab instead of blending in as a
secondary outline button.
* fix(usage): scope the pending-packet note to controls it actually gates
The note added in the previous commit rendered whenever pending_action
was truthy, regardless of participation status. Outside `active`
(activation_pending, deletion_pending) the portal renders as a plain
un-gated link and no v5-upgrade section exists at all, so the note
named two controls that either weren't blocked or weren't on screen.
And when active but already on the current schema, it wrongly implied
a v5-upgrade button existed.
Gate on `active` (nothing is actually blocked outside it), and choose
between the existing two-control message and a new portal-only one
based on whether consent_update_available — which the v5-upgrade
section itself is gated on — is true. Four new regression tests cover
each shape: activation_pending, deletion_pending, active+current-schema
(portal-only), and active+outdated-schema (both, the original case).
Found by code review.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
#1359 independently added 211_revocations_without_expiry.js against
the same main baseline. Renumbering this one to 212 keeps the
migrations directory sequentially numbered once both land, regardless
of merge order. No functional change — same up()/down(), same column.
Found testing against WebKit at an iPhone-sized viewport: the fixed
overlay + flex-center wrapper has no height limit, so on a short
screen the title is clipped off the top and the "Maybe later" button
off the bottom, with no way to reach either. Capping the card at
max-h-[90vh] with its own overflow-y-auto keeps the backdrop static
and makes the card scroll internally instead.
An admin who already had PicPeak installed before the opt-in reporting
feature existed never gets asked — the setup wizard only runs once, on
a brand-new instance. Adds a one-time modal, shown on the admin's next
dashboard visit after updating, offering the same choice the wizard
gives a new install.
- New `product_usage_state.prompt_shown` column (migration 211) and
UsageService.markPromptShown(), set on either outcome (enable or
decline) from both this modal and the wizard step, so an
installation is never asked twice regardless of which path it took.
- New POST /admin/usage/prompt-seen endpoint.
- Extracted the wizard's three-point pitch (UsageReportingPitch.tsx)
so the modal and the wizard step share identical copy instead of
drifting apart.
- The modal never shows once participation is already active, and
never shows a second time after either the wizard or the modal has
been through it once.
Depends on #1360 (the setup wizard step this reuses).
Adds a step between the SMTP/site-URL config step and the final
thank-you screen, asking whether to participate in the existing
Product usage & feedback reporting. Kept short — three points on how
it differs from typical telemetry (one-way only, no personal data,
and participants can browse the same shared feature-adoption dataset
other installations report) plus an explicit consent checkbox — and
calls the same enable() endpoint the post-install Settings page uses.
Skipping does nothing; participation can be toggled anytime from
Settings → Product usage.
Harden gallery authentication and authorization, consolidate gallery workflows, and prevent token-bearing URLs from leaking through nginx request error logs.
generatePreviewImage rewrites the output extension to match the encoding
it chose, .jpg or .webp for alpha and multi-frame sources. The tier
lookup in ensurePreviewImageAtWidth and the cleanup list in
previewTierKeys kept the SOURCE extension instead, so for anything but a
lowercase .jpg source the stat never matched: every tier request for a
.png, .JPG, .jpeg, .heic or RAW photo re-ran Sharp, and cleanup never
found the files it left behind, which accumulated for the life of the
install.
Both now derive every key the tier can live under: the .jpg and .webp
candidates, plus the source-extension key last so tiers written before
the rewrite are still found by lookup and by cleanup.
Follow-up to issue 1020, where the mismatch was identified during review.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(images): single-flight lazy rendition generation and keep the old rendition during replacement
The lazy generators in imageProcessor are check-then-generate, and the
check reads the path off the photo row the route already fetched. N
concurrent cold requests for one photo all held a snapshot with the path
still null, all missed, and all ran the same sharp pipeline. Only the
thumbnail tier path had a guard; the canonical thumbnail it falls back
to, heroes, previews and preview tiers had none.
One process-local map now covers every rendition, keyed by photo id and
rendition (`thumbnail:<id>`, `thumbnail:<id>:w<width>`, `hero:<id>`,
`preview:<id>`, `preview:<id>:w<width>`). Concurrent callers share one
promise; the entry is cleared in a finally on success and failure alike
so a rejection cannot poison the key. The tier stat moved inside the
flight so a request arriving as the previous flight clears finds the
written tier instead of missing on a stale probe.
Heroes and previews also deleted the existing object before generating
its replacement, and again in the catch. Both are gone, mirroring what
the thumbnail generator already does: put is the last statement in the
try and replaces atomically on local storage and by key on S3, so the
delete only ever opened a window with no rendition at all, and a source
that failed to read stripped the old rendition with the row still
pointing at it.
No re-read of the photo row inside the flight: the admin regenerate
endpoints force a rebuild by passing a row with the path nulled, and a
re-read would hand back the persisted rendition untouched.
Fixes the single-flight half of issue 1020. The preview cache-key
extension mismatch and any server-wide work queue remain separate.
* fix(images): keep the snapshot validity check outside the single-flight
With the check inside the flight, an admin regeneration (row passed with
the path nulled) could join a viewer's flight for the same photo that was
merely confirming an already-good rendition, and be handed back the very
file it was asked to replace while the endpoint counted a success. Only a
miss enters the flight now; inside it everything is a regeneration.
* fix(images): forced rebuilds run after an in-flight lazy generation instead of adopting it
The admin regenerate endpoints could still join a lazy flight that was
already generating for the same photo. That flight read the thumbnail
settings when it started, so after a settings change it produces exactly
the rendition the regenerate was invoked to replace; adopting it counted
a success while the old size stayed cached.
ensureThumbnail and ensurePreviewImage take `{ force: true }`: skip the
snapshot check and, if a flight is pending, start after it settles. Lazy
misses arriving meanwhile join the forced flight, and an older flight
settling late no longer evicts the newer entry from the map.
* fix(images): key rendition flights by source as well as photo id
replacePhoto keeps the photo id and changes path and filename. Keyed by
id and width alone, a request carrying the replacement row joined a
flight still rendering the file it replaced and was handed the old
image, which the gallery caches for 30 minutes. The tier map this
replaced was keyed by storage key and so already told the two apart.
* test(thumbnails): wait for the regenerate loop's completion line instead of a fixed 150 ms
The loop runs in setImmediate after the response. Under a loaded machine
(fifteen suites in parallel, each booting a migrated SQLite) it took
longer than 150 ms once and the assertions ran against a half-finished
mock call list. Poll the logger spy for the "regeneration complete" line
with a 10 s deadline; the suite also finishes sooner because the wait
ends as soon as the loop does.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
PUT /admin/events/:id spreads the body into the UPDATE. SQLite resolves
quoted identifiers case-insensitively, so `{ "Event_Name": ... }` lands
on event_name there — while every check in the handler (validators, the
field-level permission guards, the deny-set) keys on the exact lowercase
name. The deny-set already case-folded for its own columns; every other
column was reachable through a spelling variant.
Every events column and every input-only key the handler accepts is
lowercase snake_case, so a key with any uppercase in it is not something
a legitimate client sends. Such keys are now removed before anything
looks at the body. Postgres was unaffected (quoted identifiers are
case-sensitive there; a variant produced a 500 instead).
Surfaced by the Codex review of the folder-watcher change, where a
photos.upload guard on external_watch could be walked around this way.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* feat(external-media): watch reference folders and import new files automatically
Managed uploads dropped into storage/events/active are picked up by the
chokidar watcher; external media had no equivalent, so a NAS folder that
keeps growing needed an admin to open the event and press Import every
time. Relates to issue 1187.
- The import pass moves out of the route into
services/externalImportService.js. The watcher and the Import button
now run the identical function; the route only validates and maps
errors to status codes.
- Mutual exclusion is the per-event claim from maintenanceJobState
(`external_import:<id>`, seeded on demand by the new ensure()) instead
of the in-process Set. The Set stopped a double-click in one process;
the claim also stops the watcher on a second replica, or an admin
clicking while the watcher is mid-run elsewhere. The run heartbeats so
a claim from a dead process is taken over.
- services/externalMediaWatcher.js: per-event opt-in via the new
events.external_watch column (migration 208), chokidar with
awaitWriteFinish so a copy in flight is not imported half-written,
debounced full pass per change, a timer sweep every 15 minutes as the
fallback for NFS/SMB mounts that deliver no inotify events, optional
stat-polling via EXTERNAL_MEDIA_WATCH_POLLING. The set of watched
events is re-read every minute, so the toggle works from any replica.
A watcher that just started runs one pass immediately.
- Deletions are ignored on purpose: a file vanishing from a NAS is at
least as likely to be a reorganisation or a dropped mount as an
intentional removal, and acting on it would delete a guest-visible
photo. Rows whose file is gone stay, as they do today.
- Not gated on STORAGE_BACKEND: EXTERNAL_MEDIA_ROOT is always local.
- Quiet system passes stay out of the activity log; runs that imported
something are logged with actor external-media-watcher.
- Frontend: "Watch folder for new files" checkbox under the external
folder picker, status line in view mode, EN/DE strings.
* fix(external-media): close the review gaps in the folder watcher
Codex review of the watcher, round 1. All six findings were real:
- Enabling the watcher, or pointing an enabled one at another folder,
now requires photos.upload — the permission the manual Import already
requires. events.edit alone was a way around it. Only the transition
is checked, so a role without photos.upload can still edit an
already-watched event. The checkbox is disabled for such roles.
- Automatic passes defer files that are still changing: anything
modified inside the stability window, or whose size moves across one
wait of that window, is left for the next pass. chokidar's
awaitWriteFinish only settles the file that fired the event, and the
sweep sees no events at all, so a sibling still being copied could be
inserted half-written and then skipped forever.
- Photos an admin deleted are not brought back by the sweep. The delete
routes record the file in external_import_exclusions (migration 209);
automatic passes skip the list, the manual Import ignores it and
clears it for what it imports.
- The six EXTERNAL_MEDIA_WATCH* variables are forwarded in all three
compose files; they were documented but the backend services use
explicit environment lists, so the kill switch did nothing.
- A pass re-checks is_active / is_archived at run time, not only in the
minutely reconcile.
- The lease is renewed on a timer for the whole run, walk included, and
ownership is checked before the event row is touched.
* fix(external-media): make automatic passes follow the row, not rewrite it
Codex review round 2, four findings, all applied:
- The event update route drops non-canonical spellings of external_watch
and external_path before the permission guard. SQLite resolves column
names case-insensitively, so `External_Watch` reached the column while
the guard only looked at the lowercase key.
- Exclusions are checked per file at insert time, not against a
snapshot taken before the settle wait. A photo deleted during the wait
was present in the snapshot and got re-inserted by the loop.
- An automatic pass no longer writes source_mode / external_path. It
re-reads the row after the walk and the settle wait and stops if the
folder changed or the event went managed; the manual Import is the
only writer. The options are now `automatic` + `settleMs`.
- A pass that deferred files re-arms the debounced import, so a file
copied just before the watcher started is not stranded when the sweep
is disabled.
* fix(external-media): keep exclusions for replaced photos, stop a pass whose event stopped qualifying
Codex review round 3, both findings applied:
- recordExclusions keys on external_relpath alone. A replaced external
photo becomes managed but keeps its relpath on purpose, and deleting
that replacement must not republish the NAS original.
- An automatic pass checks the full watcher predicate (reference mode,
same folder, watch on, active, not archived) before it inserts and on
every heartbeat tick during the loop, and stops as soon as the event
no longer qualifies.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
On the CMS page the editor has no bounded height, so a long document
scrolls the whole admin content area and the toolbar scrolled away with
it. Editing a 16-section privacy policy meant scrolling back to the top
for every heading or list.
The top toolbar block (mode/save row and formatting row) is now
`sticky top-0` from the md breakpoint up, pinned to the admin page's
scroller. The rounded wrapper clips with `overflow-clip` instead of
`overflow-hidden`, because hidden turns the wrapper into a scroll
container and the toolbar would pin to that instead of to the page. Not
below md: there the formatting row wraps to several lines and a
permanently stuck block would eat most of a phone's editing area.
Two things follow from pinning. The link-entry row moves inside the
sticky block: rendered below it, the URL field sat at the toolbar's
original document position, under the pinned toolbar. And ProseMirror's
selection scrolling gets a top threshold and margin sized from the
block's rendered height (ResizeObserver, re-applied through
editor.setOptions), because the formatting row wraps to two rows at
common desktop widths and the link row comes and goes; a constant would
leave the caret behind the toolbar half the time. Below md the offsets
are zero again.
Verified in Chromium against the CMS page with an 18-section document:
scrolled to the last sections, the toolbar stays at the top of the
content area; on main it is gone. A source-level test pins the sticky
block, the wrapper's clip, the link row's placement and the measured
offsets, since jsdom does not lay out.
Relates to issue 1289
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* feat(security): opt-in recoverable gallery passwords
Gallery passwords are bcrypt hashes, so an admin who needs to hand a
password to a client a second time has to reset it, which invalidates
what the client already has. This adds a security setting,
security_gallery_password_recoverable, off by default, that keeps an
AES-256-GCM encrypted copy of each gallery password and client PIN next
to the hash. The key is derived from GALLERY_PASSWORD_ENCRYPTION_KEY or
JWT_SECRET.
While the setting is on:
- create, publish, send-later, edit, reset and the v1 API write the copy
alongside the hash; turning a gallery's password requirement off
clears it
- GET /api/admin/events/:id/password returns the copy to admins with
events.edit and ownership, and writes a gallery_password_viewed
activity entry on every real reveal
- resend-email uses the stored password instead of the "set at creation"
sentinel, so the client receives what already works
Switching the setting off purges every stored copy. Login and hash
verification are untouched; the copy is never read on the gallery side.
The Security tab carries the toggle with a warning that stays visible,
and the event page shows "Show password" with copy buttons only while
the setting is on and the gallery has a secret.
Relates to issue 1271
* fix(security): close the write-versus-switch-off race in the password vault
The recoverable setting is read while an event insert is assembled and the
client-PIN hash awaits after that, so a settings request that switched the
feature off and purged in that gap was overtaken by the insert. Every write
site now re-reads the setting right after its statement and clears its own
row when the setting is off; the settings writer flips the value before it
purges, so either the purge or the re-check catches the row.
* fix(security): resend carries the stored client PIN and link; deterministic tamper test
The creation mail includes the client-access link and PIN; a resend only
sent the gallery password even when a stored PIN was available. The
ciphertext tamper assertion replaced the last two characters with a
constant, which was a no-op roughly once in 4096 runs.
* fix(security): drop the revealed password after Send gallery email
The send-later route can replace the password; the share card keys its
revealed copy on the event query's refetch time, so invalidate the event
after the send like the other password-changing mutations do.
* fix(security): purge leftovers before the setting write when turning recovery on
Switching on wrote the setting first and purged after, so a password write
that read the new "on" in between stored a copy the purge then deleted.
Turning on now purges before the write; turning off keeps purging after it,
which together with the write-site re-check leaves the vault holding
exactly what was written while the setting was on.
* chore(security): drop the duplicate rateLimitService import left by the rebase
* chore(usage): register the password recovery routes in the v5 coverage inventory
The inventory moved from v4 to v5 on main; the entry added by this branch
followed it.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* feat(usage): distinguish real edits and template delivery with v5 consent
* fix(usage): exclude queued test messages and count reorders as edits
- queueEmail carries usageEligible: false into email_data and the queue
processor passes it on, so the dev tools' send-test-email no longer
records email_template_delivery once the worker sends it.
- event-types/reorder and categories/reorder-global compare the persisted
order before and after and record the v5 edit markers only when it
changed, matching the display_order edit already counted on PUT.
- normalized() builds arrays with Array.from so a row array from the sqlite
binding compares equal under Jest's separate realm.
* fix(usage): cover per-gallery category order and workflow test runs
- categories/reorder records category_editing when an event's override
changes; reorder/:eventId records it when an override was actually
removed.
- send_email and the collections handoff pass usageEligible: false for a
workflow test run (engine.testRun sets __test), so a non-dry test send is
not counted as template delivery.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(email): scrub gallery passwords from the sent-mail archive
The email queue kept every gallery password and client PIN in clear
text in email_data and rendered_html after the mail was sent, and the
Messages reading pane handed them back to any admin with the messaging
flag. A password hash in the events table bought nothing while the
plaintext sat next to it.
Once a mail is out, or its retries are exhausted, the processor now
masks secret-looking variables (password, passcode, pin) in email_data
and replaces their values in the rendered body, plain and HTML-escaped.
The reading pane applies the same masking to rows archived before this
change. Pending rows keep the real values so a retry still sends them.
Relates to issue 1271
* fix(email): keep a quoted ">" from cutting an attribute value out of redaction
The tag splitter stopped at the first ">", so a template attribute such as
title="{{gallery_password}} > details" left the password unmasked in the
archived HTML while email_data was already masked. The tokenizer is now
quote-aware; a tag with an unbalanced quote falls through as text and is
scrubbed there.
* fix(email): scrub secrets inside HTML comments in the archived body
A comment such as <!-- PIN: {{client_password}} --> was split off as a tag
and its body, which has no attribute, was never scrubbed. Comments are now
one segment and their content is masked whole.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* feat(settings): expose the API rate limiter in the Security tab
The general per-IP limiter had six settings in app_settings and a
backend route to write them, and no screen. Installs ran on the code
fallback — 300 requests per 15 minutes per IP — with no way to see it,
which is how issue 1287 played out: a 546-photo gallery exhausted the
budget for one viewer and the operator learned about the setting from
a grep of the backend log.
Security tab: a card with the six settings, the validation ranges the
route enforces, a one-line explanation per field, and a note that the
unit is the client IP — an office or household behind one NAT shares
a budget, and behind a proxy TRUST_PROXY has to cover the proxy or
every visitor shares its address. The tab's Save button saves the
limiter through its own route. The limiter values are checked against
the route's ranges before anything is written and the limiter is
written first, so a rejected value cannot leave the password/session
settings half-saved behind a failure toast.
Backend, three things the screen needed:
- The settings read fills the six keys with the code defaults when
they have no row, so the form shows the budget in force rather than
an empty field; the defaults live in one exported constant the
limiter itself reads.
- The write route upserts instead of updating: on a fresh install,
which has no rows, the old UPDATE matched nothing and the route
answered 200 while changing nothing.
- The live limiter instances move into rateLimitService and the
write route rebuilds them. express-rate-limit fixes windowMs when an
instance is built — max and skip re-read the settings per request,
the window does not — so a saved window used to apply only after a
restart. The gates in server.js resolve the instance per request
through the service's getters. A rebuild starts fresh counters,
which on a settings change is acceptable. The limiters get explicit
MemoryStores and a rebuild shuts the superseded ones down, because a
store keeps a cleanup interval alive for as long as it exists and
dropping the reference alone would leak one timer per save.
Tests: the read surfaces defaults and honours the key filter; the
write creates rows on a fresh database, the limiter sees the values
immediately and hands the gates a fresh instance; existing rows are
updated not duplicated; out-of-range values are rejected. The tab
renders the values, edits through the hook state, carries the ranges,
saves with the tab's button, and the pre-write validation accepts the
bounds and rejects outside them and cleared fields.
Relates to issue 1337
* docs(security): point the rate limiter doc at the Security tab and the upsert
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* docs(security): correct the rate limiter defaults and how they are set
SECURITY_LOGGING.md said rate_limit_max_requests defaults to 1000 and
that the settings are configurable via the admin panel. The code
fallback when no app_settings row exists is 300 (19e125d8), a fresh
install has no row, and there is no admin screen: the settings are
written by PUT /api/admin/settings/security/rate-limit, which nothing
in the frontend calls. The reporter of issue 1287 ran on the 300
default without any way to see it.
The table now carries the real defaults, what the auth budget counts,
the exemptions including the gallery-image one from v3.127.0-beta.0,
and the TRUST_PROXY and shared-NAT caveats.
Relates to issue 1287
* docs(security): note the rate-limit route only updates existing rows
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
In Safari the product-usage consent dialog opened as a 302px box on a
714px viewport with the disclosure text squeezed into a 32px strip
between header and footer. The dialog is a native <dialog> laid out as
a flex column with only a max-height, so its own height is indefinite,
and the disclosure region used `flex-1`, which is `flex: 1 1 0%`.
WebKit resolves that 0% basis against the indefinite container height
as zero: the region's hypothetical size is zero, the dialog sizes to
header plus footer, and the max-height never comes into play. Chromium
treats the same basis as `content` and was fine.
An `auto` basis with min-height 0 sizes the region from its content and
lets it shrink to the max-height: measured in iOS Safari, 641px dialog
and a 371px scrolling region, footer on screen, matching Chromium.
Only the <dialog> is affected. The div-based modals that use the same
`flex-1 overflow-y-auto` pattern inside a max-height column were
measured in the same WebKit and size correctly, so they stay as they
are. A test pins the classes with the reasoning, since jsdom cannot
see the layout.
Paul Nothaft