Close the smaller gaps from the security review #379

New Issue

Closed

opened 2026-09-16 15:47:44 +00:00 by jcoffey-dev · 0 comments

jcoffey-dev commented

2026-09-16 15:47:44 +00:00

Owner

Summary

These are the lower-severity items from the security review. Each one needs an existing session, a shared device or a crafted attachment.

App passwords need the account password.
- POST /api/account/app-passwords now takes current and has its own rate-limit budget.
- The password is compared with what the session holds: the password, or the password plus $code when a code was given at sign-in. Stalwart is only asked when turning on 2FA moved the session onto an app password (a $app$ secret), so a wrong guess never counts toward Stalwart's auto-ban, which counts failures against the proxy's shared address.
- Before this, a session left open on someone else's machine was enough to mint a credential that outlives a sign-out and a password change.
- The settings form gets a "Current password" field.
Checked sessions can read the account's own registry objects but not write them. The browser never sends these methods, since all credential changes go through /api/account. Without this rule, x:AppPassword/set through /api/jmap would get around the password check. x:ApiKey is removed from that list entirely.
No disk cache on devices not marked as the user's own. Attachments (/api/blob) and proxied images get no-store for sessions not marked as the user's own device. Signing out wipes what the app stores, but not the browser's HTTP cache.
winmail.dat contents. Extracted files keep their type only if the server would show that type inline (openableInTab); anything else becomes application/octet-stream. The type comes from the sender's MAPI data and never passed the server's check, so text/html opened from its blob: URL rendered as a page on this origin.
Bidi controls are stripped from sender display names (displayName, formatAddress), attachment names in the reading pane and share sheet, winmail.dat names, the file preview's title and download name, and the server's Content-Disposition. Before this, Invoice_‮fdp.exe displayed as "Invoice_exe.pdf".
Signing out closes the composer.
- logout() first sends any messages still inside their undo window.
- When the session becomes anonymous, the compose store drops its drafts, autosave timers and pending sends.
- Before this, the next person to sign in on a shared machine, without a reload in between, found the previous user's draft open. A pending send's timer would also have fired under the new session.
Sessions are grouped by account. They are keyed by the server and the lower-cased username that Stalwart itself reports. "Sign out other sessions", the session list and password or 2FA changes now also reach sessions opened as alice or [email protected]. Sessions saved before this change fall back to the lower-cased username.

Left out:

The share target: it only opens a draft, which the user still has to send. Guarding it properly needs a confirmation screen, with new strings and a design decision.
The version in /api/health: the sign-in page already shows it, and the deploy script reads that response.

Not verified against a real server: the $app$ prefix is taken from the mock. If real Stalwart uses a different prefix, only 2FA sessions are affected, and 2FA isn't shipped.

Related issues

None.

Translations

Adds none. The new field's label is "Current password", which all nine catalogs already have. npm run i18n:check reports the same counts as main (1,609 of 1,625 translated in each), and finds no unwrapped strings.

Testing

npm test -w server passes (242 tests). New or changed tests cover:
- a missing or wrong password being refused, with nothing created;
- x:AppPassword/set through /api/jmap getting a 403 for a checked session;
- gets passing the admin check while sets and x:ApiKey are refused;
- sessions grouped across spellings but not across servers;
- a download from a session not marked as the user's own getting no-store and a filename without the override.
npm test -w web passes (1,318 tests). The new tests cover bidi stripping in names and in winmail.dat names, and the composer clearing on sign-out and sending pending messages first. 5 of them fail against main.
npm run typecheck and npm run build are clean.
Checked in Chrome against npm run dev:mock: the app-password row shows the new field beside the name, and Create stays disabled until both are filled. I didn't submit the form in the browser; the server tests cover wrong, missing and correct passwords.

Merged 2026-09-16 as coffey-labs/ihasmail@a9302075e7

_{Rebuilt from: git history, session transcript.}

## Summary These are the lower-severity items from the security review. Each one needs an existing session, a shared device or a crafted attachment. - **App passwords need the account password.** - `POST /api/account/app-passwords` now takes `current` and has its own rate-limit budget. - The password is compared with what the session holds: the password, or the password plus `$code` when a code was given at sign-in. Stalwart is only asked when turning on 2FA moved the session onto an app password (a `$app$` secret), so a wrong guess never counts toward Stalwart's auto-ban, which counts failures against the proxy's shared address. - Before this, a session left open on someone else's machine was enough to mint a credential that outlives a sign-out and a password change. - The settings form gets a "Current password" field. - **Checked sessions can read the account's own registry objects but not write them.** The browser never sends these methods, since all credential changes go through `/api/account`. Without this rule, `x:AppPassword/set` through `/api/jmap` would get around the password check. `x:ApiKey` is removed from that list entirely. - **No disk cache on devices not marked as the user's own.** Attachments (`/api/blob`) and proxied images get `no-store` for sessions not marked as the user's own device. Signing out wipes what the app stores, but not the browser's HTTP cache. - **`winmail.dat` contents.** Extracted files keep their type only if the server would show that type inline (`openableInTab`); anything else becomes `application/octet-stream`. The type comes from the sender's MAPI data and never passed the server's check, so `text/html` opened from its `blob:` URL rendered as a page on this origin. - **Bidi controls are stripped** from sender display names (`displayName`, `formatAddress`), attachment names in the reading pane and share sheet, `winmail.dat` names, the file preview's title and download name, and the server's `Content-Disposition`. Before this, `Invoice_‮fdp.exe` displayed as "Invoice_exe.pdf". - **Signing out closes the composer.** - `logout()` first sends any messages still inside their undo window. - When the session becomes anonymous, the compose store drops its drafts, autosave timers and pending sends. - Before this, the next person to sign in on a shared machine, without a reload in between, found the previous user's draft open. A pending send's timer would also have fired under the new session. - **Sessions are grouped by account.** They are keyed by the server and the lower-cased username that Stalwart itself reports. "Sign out other sessions", the session list and password or 2FA changes now also reach sessions opened as `alice` or `[email protected]`. Sessions saved before this change fall back to the lower-cased username. **Left out:** - **The share target:** it only opens a draft, which the user still has to send. Guarding it properly needs a confirmation screen, with new strings and a design decision. - **The version in `/api/health`:** the sign-in page already shows it, and the deploy script reads that response. **Not verified against a real server:** the `$app$` prefix is taken from the mock. If real Stalwart uses a different prefix, only 2FA sessions are affected, and 2FA isn't shipped. ## Related issues None. ## Translations Adds none. The new field's label is "Current password", which all nine catalogs already have. `npm run i18n:check` reports the same counts as `main` (1,609 of 1,625 translated in each), and finds no unwrapped strings. ## Testing - `npm test -w server` passes (242 tests). New or changed tests cover: - a missing or wrong password being refused, with nothing created; - `x:AppPassword/set` through `/api/jmap` getting a 403 for a checked session; - gets passing the admin check while sets and `x:ApiKey` are refused; - sessions grouped across spellings but not across servers; - a download from a session not marked as the user's own getting `no-store` and a filename without the override. - `npm test -w web` passes (1,318 tests). The new tests cover bidi stripping in names and in `winmail.dat` names, and the composer clearing on sign-out and sending pending messages first. **5 of them fail against `main`.** - `npm run typecheck` and `npm run build` are clean. - Checked in Chrome against `npm run dev:mock`: the app-password row shows the new field beside the name, and **Create** stays disabled until both are filled. I didn't submit the form in the browser; the server tests cover wrong, missing and correct passwords. **Merged** 2026-09-16 as coffey-labs/ihasmail@a9302075e7ca <sub>Rebuilt from: git history, session transcript.</sub>

This repo is archived. You cannot comment on issues.

1 Participants

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: coffey-labs/ihasmail-github-archive#379