The theme sanitizer was a short denylist. It missed protocol-relative
`url(//host)`, schemes and keywords spelled with CSS escapes
(`url(\68ttps:...)`, `@\69mport`), `image-set("https://...")` and
`@font-face` sources, so a theme could load remote resources from the
app document. `validateThemeSelectors` was never called, so theme CSS
could also target `input[type=password]` or restyle real UI. The admin
routes and the zip validator only sanitized when the safety check
failed, and CSS returned by the `onThemeBeforeApply` plugin hook was
injected as-is.
- every `url()` except a `#fragment` and every `image-set()` is removed
with its arguments; a banned construct spelled with escapes drops the
whole stylesheet
- theme CSS keeps only `:root` and `.dark` rules (no nested rules),
plus `@font-face`, `@keyframes` and `@media`/`@supports` wrappers
- sanitizing is unconditional everywhere, including plugin-transformed
CSS; compiled token values may not contain escapes or `url()`
The Stalwart admin check only bailed while `allowCustomJmapEndpoint` was
on. A context cookie minted while it was on keeps naming the
user-chosen server, and the envelope has no expiry, so after the admin
turned the switch off that server could still answer the admin probe
and mint a Bulwark admin session. The check now also requires the
cookie's server to be an admin-configured one.
The Stalwart admin check probed `x:Account/query`. Stalwart's default
tenant-admin role carries `sysAccount*`, so an administrator of a single
tenant passed and, in the default `auto` mode, was signed into the
Bulwark dashboard, which configures the whole instance for every
tenant. The `auto|password|off` switch added for #870 left the default
at `auto`, so the advisory never held on a default install.
The probe is now `x:Tenant/query`, which needs `sysTenantQuery`: by
default only the server-wide superuser holds it.
GetFile answered on the webmail origin with the Content-Type stored in
the WOPI token, which for a mail attachment comes straight from the
launch request and for a Files node is a user-settable property. No
CSP, nosniff or disposition was added, since `/api/*` skips the proxy.
A user could mint a token for their own blob typed `text/html` and have
a victim open the GetFile URL, running script in the webmail origin.
GetFile now always sends `application/octet-stream` with
`Content-Disposition: attachment`, `nosniff` and a sandboxing CSP, and
refuses browser loads (any `Sec-Fetch-Dest` other than `empty`). The
editor fetches it server-to-server, so it is unaffected.
The origin gate in the proxy only covered `/api/auth/*`. The JMAP
passthrough, `/api/settings`, `/api/wopi/launch`,
`/api/plugin-approval-status`, `/api/fetch-ical`, `/api/translate`,
`/api/caldav/discover`, `/api/calendar-agenda` and `/api/webdav` all act
with the session cookie and had no origin check. A page on a same-site
sibling origin receives SameSite=Lax cookies, so it could run arbitrary
JMAP as the signed-in user, including sharing mailboxes to a third
account.
The proxy now gates every `/api/` route except the WOPI file endpoints
the office editor calls server-to-server, and each of these handlers
checks again. The proxy also decides on the decoded path, so an escaped
spelling such as `/api/%61uth/session` no longer skips the gate.
An impersonation handoff sealed `<target>%<master>` and the master
password into the session cookie, and `PUT /api/auth/session` hands that
cookie's contents to the browser on every restore. Anyone who received
a handoff could read a credential that signs in as every mailbox and
into Stalwart administration, with no expiry and no audit trail.
The impersonate route now uses the master credential server-side, once,
to create an app password on the target mailbox that expires after
8 hours. Only that app password reaches the cookies. Signing out
revokes it through a sealed grant cookie, and PUT refuses any session
that still holds the configured master password, so cookies minted
before this change are dropped instead of returned.
A PatchObject key is a JSON Pointer (RFC 8620 section 5.3), so a slash
inside the member name it points at has to be escaped as `~1`. Keyword
patches were built as `keywords/${keyword}` verbatim, which is fine for
every flat keyword but wrong for a nested tag: `$label:work/clients`
became the pointer `keywords/$label:work/clients`, asking for the
`clients` member of the `$label:work` keyword rather than for the keyword
itself. The patch is not a boolean at that path, so the server rejects or
ignores it and the tag never lands.
Route the keyword patch sites - setKeyword, removeKeyword, migrateKeyword
and the category-tab batch patch - through `keywordPointer`, which escapes
per RFC 6901 the way the calendar store already does for participant ids.
Flat keywords are untouched, so `$seen`/`$flagged` and friends keep the
exact pointers they had. The dev JMAP mock reads the escaping back, or
nested tags would behave differently against the mock than against a real
server.
migrateKeyword also took any answer as success: a method-level error on
the query read as "no message carries this keyword", one on the write was
ignored outright, and a per-message refusal was invisible - so a rename
that moved nothing still reported a clean migration. It now answers with
{ migrated, refused }, because the work is batched and a failure partway
through cannot undo the batches already applied. A whole batch the server
refuses stops the rest, which it would refuse too; a single message it
refuses does not, the mail behind it being unaffected. A query error
throws, nothing having been written yet, and so does a run that migrated
nothing at all - the case where the caller must not rename a tag over
messages that all kept the old keyword.
The walk over tagged mail was not paginating safely either. It stopped at
the first page shorter than the limit it asked for, but RFC 8620 section
5.5 lets a server clamp that limit, so a clamping server left most of the
mail unmigrated. Positions are also only comparable within one queryState,
which the walk ignored: mail arriving mid-walk shifts every later page. It
now restarts when the state changes under it, and gives up - before any
write, so the caller can simply repeat - once a mailbox has moved under it
three times, rather than mixing ids from states that no longer agree. A
page carrying no id it has not already seen ends the walk, which keeps it
bounded against a server that answers every position with the same page.
contactUsing(), calendarUsing(), and fileUsing() checked the base
"principals" capability but then declared the separate
"principals:owner" sub-capability (RFC 9670) in the request's "using"
array. RFC 8620 section 3.3 requires a server to reject the whole
request if it declares a capability the server does not advertise, so
a server that supports base principals without the owner
sub-capability rejects every one of these requests with
unknownCapability.
Fixed to check the specific sub-capability being declared instead of
assuming it from the broader one. The calendar agenda route had the
same mismatch and is fixed the same way. Note that RFC 9670 section
1.5.2 places principals:owner in accountCapabilities rather than the
session capabilities, so on compliant servers the sub-capability is
simply never declared, which is the intent. Nothing is lost by
omitting it.
The file share test asserted the old behavior by expecting the owner
capability to be declared against a server that advertises only base
principals. It now asserts the capability is omitted in that case and
declared only when the server advertises it. The same pair of
assertions is added for the contacts, calendars, and agenda route
request builders, so each site this commit touches is covered against
regression.
If you have a large number of tags, getTagCounts would not be able to get the
unread counts because it did not respect maxCallsInRequest, even though the
value was actually read out, it was just ignored. There are also other places
where the limits were not respected.
Batching is now generalized in a helper that also takes maxObjectsInSet, which
also was ignored, into account and is applied to all functions.
In addition, the dev mock now also advertises and enforces the limits, so these
issues can get picked up during development.
Possible closes#699
Possibly closes#399
Linus Rath
Linus Rath
Paulhenry Saux
KazNIISA IT
zisisvakras
Eduard Polonskyi
Mathy Vanvoorden