From dfe885a9217963b508b8e2be37b3e4e6d77093b9 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Wed, 16 Sep 2026 08:47:23 -0700 Subject: [PATCH] Close the smaller gaps from the security review Ask for the account password before minting an app password, and keep sessions the proxy checks from writing the account's own registry objects, so a session left open on someone else's machine cannot take a credential away from it. The password is compared with what the session holds; Stalwart is asked only when 2FA moved the session onto an app password. Serve attachments and proxied images with no-store on a device that is not the person's own. Give files from a winmail.dat only the types the server would show inline. Strip direction controls from sender and attachment names and from saved filenames. On signing out, send what is inside its undo window, then close every composer, so the next person to sign in does not find the last one's draft. Group sessions by the account Stalwart names and its server, so "sign out other sessions" also reaches a session opened as a bare or differently cased username. --- server/src/account.test.ts | 36 ++++++++- server/src/adminGate.test.ts | 14 +++- server/src/adminGate.ts | 13 +++- server/src/app.ts | 75 ++++++++++++++++--- server/src/imageproxy.ts | 3 +- server/src/sessions.test.ts | 16 +++- server/src/sessions.ts | 40 ++++++++-- web/src/lib/__tests__/address.test.ts | 13 +++- web/src/lib/__tests__/tnef.test.ts | 6 ++ web/src/lib/address.ts | 11 ++- web/src/lib/text/text.ts | 9 +++ web/src/lib/tnef.ts | 3 +- .../store/__tests__/compose-sign-out.test.ts | 53 +++++++++++++ web/src/store/compose.ts | 41 +++++++++- web/src/store/session.ts | 8 ++ web/src/ui/filepreview.tsx | 5 +- web/src/views/mail/MessageView.tsx | 23 ++++-- web/src/views/settings/SecuritySettings.tsx | 11 ++- 18 files changed, 334 insertions(+), 46 deletions(-) create mode 100644 web/src/store/__tests__/compose-sign-out.test.ts diff --git a/server/src/account.test.ts b/server/src/account.test.ts index d031904..182e877 100644 --- a/server/src/account.test.ts +++ b/server/src/account.test.ts @@ -69,7 +69,7 @@ test("the registry reports an account with nothing set up yet", async () => { }); test("app passwords are created, listed once with their secret, and revoked", async () => { - const created = await post("/api/account/app-passwords", { description: "Thunderbird" }); + const created = await post("/api/account/app-passwords", { description: "Thunderbird", current: "demo-password" }); assert.equal(created.status, 200); assert.match(created.body.secret, /^\$app\$/, "the server's generated secret is returned"); assert.ok(created.body.id); @@ -84,8 +84,40 @@ test("app passwords are created, listed once with their secret, and revoked", as assert.deepEqual((await call("/api/account/security")).body.appPasswords, []); }); +test("an app password needs the account password", async () => { + const missing = await post("/api/account/app-passwords", { description: "Stolen" }); + assert.equal(missing.status, 400); + assert.equal(missing.body.error, "missing_fields"); + const wrong = await post("/api/account/app-passwords", { description: "Stolen", current: "not-my-password" }); + assert.equal(wrong.status, 403); + assert.equal(wrong.body.error, "invalid_credentials"); + assert.deepEqual((await call("/api/account/security")).body.appPasswords, [], "nothing was created"); +}); + +test("a checked session cannot mint one through the JMAP proxy instead", async () => { + // Signed in without "my own device", so the proxy reads every request. + const res = await call("/api/jmap", { + method: "POST", + body: JSON.stringify({ using: ["urn:ietf:params:jmap:core"], methodCalls: [["x:AppPassword/set", { create: { n: { description: "Stolen" } } }, "0"]] }), + }); + assert.equal(res.status, 403); + assert.deepEqual((await call("/api/account/security")).body.appPasswords, []); +}); + +test("attachments are kept out of the disk cache of a device that is not the person's own", async () => { + const up = await app.request("/api/upload/a1", { method: "POST", headers: { "x-requested-with": "ihasmail", "content-type": "text/plain", cookie }, body: "hello" }); + assert.equal(up.status, 200); + const { blobId } = (await up.json()) as { blobId: string }; + const name = encodeURIComponent("Invoice_\u202Efdp.exe"); + const res = await app.request(`/api/blob/a1/${blobId}/${name}?accept=text/plain`, { headers: { cookie } }); + assert.equal(res.status, 200); + assert.equal(res.headers.get("cache-control"), "no-store"); + assert.equal(res.headers.get("content-disposition"), "attachment; filename*=UTF-8''Invoice_fdp.exe", "no direction override in the saved name"); + await res.arrayBuffer(); +}); + test("an app password needs a name", async () => { - const res = await post("/api/account/app-passwords", { description: " " }); + const res = await post("/api/account/app-passwords", { description: " ", current: "demo-password" }); assert.equal(res.status, 400); assert.equal(res.body.error, "missing_fields"); }); diff --git a/server/src/adminGate.test.ts b/server/src/adminGate.test.ts index 4b39ede..4c8b887 100644 --- a/server/src/adminGate.test.ts +++ b/server/src/adminGate.test.ts @@ -14,8 +14,18 @@ test("mail, calendars and the rest pass untouched", () => { assert.equal(r.ok, true); }); -test("the account's own registry objects pass", () => { - assert.equal(gateAdministration(req("x:AccountSettings/get", "x:AppPassword/set", "x:PublicKey/get", "x:MaskedEmail/set")).ok, true); +test("the account's own registry objects can be read", () => { + assert.equal(gateAdministration(req("x:AccountSettings/get", "x:AppPassword/get", "x:PublicKey/get", "x:MaskedEmail/query")).ok, true); +}); + +test("but not written: a credential minted here would outlive a borrowed session", () => { + for (const m of ["x:AppPassword/set", "x:AccountPassword/set", "x:MaskedEmail/set"]) { + assert.deepEqual(gateAdministration(req("x:AccountSettings/get", m)), { ok: false, method: m }); + } +}); + +test("API keys are not the account's to reach from here at all", () => { + assert.deepEqual(gateAdministration(req("x:ApiKey/get")), { ok: false, method: "x:ApiKey/get" }); }); test("directory and server objects are refused, and named", () => { diff --git a/server/src/adminGate.ts b/server/src/adminGate.ts index 77b2679..d899c62 100644 --- a/server/src/adminGate.ts +++ b/server/src/adminGate.ts @@ -18,7 +18,7 @@ * The standard JMAP methods (mail, calendars, contacts, files, sharing) are not * touched: they act on what the account can already reach. */ -const SELF_SERVICE = new Set(["AccountSettings", "AccountPassword", "AppPassword", "ApiKey", "PublicKey", "MaskedEmail"]); +const SELF_SERVICE = new Set(["AccountSettings", "AccountPassword", "AppPassword", "PublicKey", "MaskedEmail"]); export type GateResult = { ok: true; body: string } | { ok: false; method: string | null }; @@ -83,8 +83,15 @@ export function gateAdministration(raw: string): GateResult { const name = Array.isArray(call) ? call[0] : undefined; if (typeof name !== "string") return { ok: false, method: null }; if (!name.startsWith("x:")) continue; - const object = name.slice(2).split("/")[0] ?? ""; - if (!SELF_SERVICE.has(object)) return { ok: false, method: name }; + const [object = "", op = ""] = name.slice(2).split("/"); + /* + * Read, never write. The browser sends none of these itself -- password, + * app-password and 2FA changes go through /api/account, which checks the + * account password first -- so a write here could only come from + * somebody working the console of a session on a borrowed machine, and + * `x:AppPassword/set` would hand them a credential that outlives it. + */ + if (!SELF_SERVICE.has(object) || op === "set") return { ok: false, method: name }; } return { ok: true, body: JSON.stringify(parsed) }; } diff --git a/server/src/app.ts b/server/src/app.ts index bb2c7d8..5b8c662 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -11,9 +11,10 @@ import { getConnInfo } from "@hono/node-server/conninfo"; import { config } from "./config.js"; import { fetchPermissions } from "./permissionSchema.js"; import { administrationAllowed, gateAdministration, grantsAdministration } from "./adminGate.js"; -import { SessionStore, type SessionBackend, type LiveSession } from "./sessions.js"; +import { SessionStore, accountKey, type SessionBackend, type LiveSession } from "./sessions.js"; import { RateLimiter } from "./ratelimit.js"; import { rateLimitKey, resolveClientIp } from "./clientip.js"; +import { safeEqual } from "./crypto.js"; import { type AccountInfo, UpstreamError, @@ -385,6 +386,7 @@ export function createApp(basePath = config.basePath): Hono { loginLimiter.reset(limitKey); const { cookie, session } = sessions.create({ username, + account: accountKey(upstreamFor(username), upstream.username || username), password: effectivePassword, remember: Boolean(body.remember), userAgent: c.req.header("user-agent") ?? "", @@ -466,12 +468,12 @@ export function createApp(basePath = config.basePath): Hono { api.get("/auth/sessions", requireSession, (c) => { const session = c.get("session"); - return c.json({ current: session.id, sessions: sessions.listForUser(session.username) }); + return c.json({ current: session.id, sessions: sessions.listForUser(session.account) }); }); api.post("/auth/sessions/revoke-others", requireSession, (c) => { const session = c.get("session"); - const n = sessions.destroyAllForUser(session.username, session.id); + const n = sessions.destroyAllForUser(session.account, session.id); return c.json({ revoked: n }); }); @@ -499,8 +501,8 @@ export function createApp(basePath = config.basePath): Hono { }; /** Guard the endpoints that check a password against brute-forcing. */ - const guarded = (c: Context): Response | null => { - const key = `account|${c.get("session").username.toLowerCase()}`; + const guarded = (c: Context, scope = "account"): Response | null => { + const key = `${scope}|${c.get("session").username.toLowerCase()}`; if (accountLimiter.check(key)) return null; c.header("Retry-After", String(accountLimiter.retryAfterSeconds(key))); return c.json({ error: "rate_limited", message: "Too many attempts. Please wait and try again." }, 429); @@ -538,7 +540,7 @@ export function createApp(basePath = config.basePath): Hono { const otpCode = body.otpCode?.trim(); sessions.reseal(getCookie(c, config.cookieName), otpCode ? `${next}$${otpCode}` : next); forgetUpstreamSession(session.id); - const revoked = sessions.destroyAllForUser(session.username, session.id); + const revoked = sessions.destroyAllForUser(session.account, session.id); return c.json({ ok: true, revokedSessions: revoked }); }); @@ -552,12 +554,26 @@ export function createApp(basePath = config.basePath): Hono { } }); + /* + * An app password is a credential that outlives this session, a password + * change and a sign-out -- so minting one asks for the account password, as + * changing the password does. Otherwise a session left open on somebody + * else's machine is enough to take a permanent key away from it. + */ api.post("/account/app-passwords", requireSession, async (c) => { + // A budget of its own: guessing here never reaches Stalwart (see confirmsPassword). + const limited = guarded(c, "app-password"); + if (limited) return limited; const session = c.get("session"); - const body = await readJson<{ description?: string }>(c); + const body = await readJson<{ description?: string; current?: string }>(c); if (!body) return c.json({ error: "bad_request" }, 400); const description = (body.description ?? "").trim().slice(0, 120); if (!description) return c.json({ error: "missing_fields", message: "Give the app password a name." }, 400); + const current = body.current ?? ""; + if (!current || current.length > 1024) return c.json({ error: "missing_fields", message: "Enter your current password." }, 400); + if (!(await confirmsPassword(session, current))) { + return c.json({ error: "invalid_credentials", message: "That password is not correct." }, 403); + } try { return c.json(await createAppPassword(await accountCtx(c), { description })); } catch (err) { @@ -632,7 +648,7 @@ export function createApp(basePath = config.basePath): Hono { if (sessionKept) forgetUpstreamSession(session.id); } // Other sessions still hold the bare password and will be refused. - const revoked = sessions.destroyAllForUser(session.username, session.id); + const revoked = sessions.destroyAllForUser(session.account, session.id); return c.json({ ok: true, sessionKept, revokedSessions: revoked }); }); @@ -803,7 +819,7 @@ export function createApp(basePath = config.basePath): Hono { const safeInline = inline && isInlineSafe(type); headers.set( "Content-Disposition", - `${safeInline ? "inline" : "attachment"}; filename*=UTF-8''${encodeURIComponent(name)}`, + `${safeInline ? "inline" : "attachment"}; filename*=UTF-8''${encodeURIComponent(withoutBidiControls(name))}`, ); headers.set("X-Content-Type-Options", "nosniff"); // Sandbox everything except the browser's built-in PDF viewer (which needs scripts to render). @@ -822,7 +838,9 @@ export function createApp(basePath = config.basePath): Hono { } else { headers.set("Content-Security-Policy", "sandbox; default-src 'none'; style-src 'unsafe-inline'; img-src data:"); } - headers.set("Cache-Control", "private, max-age=3600"); + // Kept out of the browser's disk cache on a device that is not the + // person's own: signing out wipes what the app stores, not that. + headers.set("Cache-Control", session.remember ? "private, max-age=3600" : "no-store"); return new Response(res.body, { status: 200, headers }); } catch (err) { return upstreamFailure(c, err); @@ -908,6 +926,43 @@ async function readJson(c: Context): Promise { } } +/** + * Is `candidate` the password of the account this session is signed in to? + * + * Compared with the credential the session holds first, which costs nothing + * and tells Stalwart nothing -- its auto-ban counts failures against the + * proxy's address, which every user shares. That credential is the password, + * with a TOTP code after a `$` when one was given at sign-in. A session that + * turning on 2FA moved onto an app password (Stalwart's secrets start + * `$app$`) holds something else, and only then is the candidate put to the + * server. + */ +async function confirmsPassword(session: LiveSession, candidate: string): Promise { + const decoded = Buffer.from(session.authorization.replace(/^Basic /, ""), "base64").toString("utf8"); + const held = decoded.slice(decoded.indexOf(":") + 1); + if (safeEqual(held, candidate)) return true; + const withoutCode = held.replace(/\$\d{6,8}$/, ""); + if (withoutCode !== held && safeEqual(withoutCode, candidate)) return true; + // Holding the password, the comparison above is the answer, and a wrong + // guess never reaches the server's auto-ban. + if (!held.startsWith("$app$")) return false; + try { + const authorization = `Basic ${Buffer.from(`${session.username}:${candidate}`, "utf8").toString("base64")}`; + await fetchUpstreamSession(authorization, upstreamFor(session.username)); + return true; + } catch { + return false; + } +} + +/** + * Direction overrides and isolates, which can make `Invoice_\u202Efdp.exe` + * read as a PDF in the downloads list. A filename has no use for them. + */ +function withoutBidiControls(name: string): string { + return name.replace(/[\u061C\u200E\u200F\u202A-\u202E\u2066-\u2069]/g, ""); +} + /** Name the app password after the browser it will live in. */ function appPasswordName(c: Context): string { const ua = c.req.header("user-agent") ?? ""; diff --git a/server/src/imageproxy.ts b/server/src/imageproxy.ts index 6c8d9fb..5364040 100644 --- a/server/src/imageproxy.ts +++ b/server/src/imageproxy.ts @@ -217,7 +217,8 @@ export async function imageProxyHandler(c: Context) { res.on("close", done); const headers = new Headers({ "Content-Type": type, - "Cache-Control": "private, max-age=86400", + // As for attachments: nothing left in the disk cache of a device that is not the person's own. + "Cache-Control": (c.get("session") as { remember?: boolean } | undefined)?.remember ? "private, max-age=86400" : "no-store", "X-Content-Type-Options": "nosniff", "Content-Security-Policy": "sandbox; default-src 'none'", "Cross-Origin-Resource-Policy": "same-origin", diff --git a/server/src/sessions.test.ts b/server/src/sessions.test.ts index 91ae914..2a2371b 100644 --- a/server/src/sessions.test.ts +++ b/server/src/sessions.test.ts @@ -1,6 +1,6 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { SessionStore } from "./sessions.js"; +import { SessionStore, accountKey } from "./sessions.js"; import { normalizeLocale } from "./upstream.js"; import { deriveKey, open, seal, sha256 } from "./crypto.js"; import { RateLimiter } from "./ratelimit.js"; @@ -30,6 +30,20 @@ test("session store creates, resolves, and refuses tampered cookies", () => { assert.equal(store.resolve(cookie), null); }); +test("sessions group by the account, however its name was typed", () => { + const store = new SessionStore(""); + const key = accountKey("https://mail.example.com", "Alice@Example.com"); + const a = store.create({ username: "alice", account: key, password: "pw", remember: false, userAgent: "", ip: "" }); + const b = store.create({ username: "ALICE@example.com", account: accountKey("https://mail.example.com", "alice@example.com"), password: "pw", remember: false, userAgent: "", ip: "" }); + // The same name on another configured server is another account. + store.create({ username: "alice@example.com", account: accountKey("https://other.example.net", "alice@example.com"), password: "pw", remember: false, userAgent: "", ip: "" }); + assert.equal(a.session.account, b.session.account); + assert.equal(store.listForUser(a.session.account).length, 2); + assert.equal(store.destroyAllForUser(a.session.account, a.session.id), 1); + assert.equal(store.resolve(b.cookie), null, "the other spelling was signed out"); + assert.ok(store.resolve(a.cookie), "this session was kept"); +}); + test("persisted session data does not contain the password", () => { const store = new SessionStore(""); store.create({ username: "u", password: "super-secret-pw", remember: true, userAgent: "", ip: "" }); diff --git a/server/src/sessions.ts b/server/src/sessions.ts index 2b713aa..74d1120 100644 --- a/server/src/sessions.ts +++ b/server/src/sessions.ts @@ -13,6 +13,8 @@ export interface StoredSession { /** sealed JSON {username, password} */ sealedCredentials: string; username: string; + /** Which account this is; see `accountKey`. Absent on sessions saved before it existed. */ + account?: string; createdAt: number; lastSeenAt: number; expiresAt: number; @@ -24,6 +26,8 @@ export interface StoredSession { export interface LiveSession { id: string; username: string; + /** See `accountKey`. */ + account: string; /** Basic Authorization header value for upstream calls. */ authorization: string; remember: boolean; @@ -46,8 +50,27 @@ export interface SessionSummary { ip: string; } +/** + * The key sessions are grouped by for "sign out everywhere else". + * + * Not the username as typed: Stalwart takes `Alice@example.com` and a bare + * `alice` as the same account, and a session opened either way was missing + * from the list and survived the sign-out. The server's own name for the + * account, lower-cased, and the server it lives on -- the same name on two + * configured servers is two accounts. + */ +export function accountKey(upstream: string, canonicalUsername: string): string { + return `${upstream}|${canonicalUsername.trim().toLowerCase()}`; +} + +function accountOf(s: StoredSession): string { + return s.account ?? s.username.trim().toLowerCase(); +} + export interface CreateSessionParams { username: string; + /** From `accountKey`; defaults to the lower-cased username. */ + account?: string; password: string; remember: boolean; userAgent: string; @@ -85,8 +108,9 @@ export interface SessionBackend { resolve(cookie: string | undefined): LiveSession | null; reseal(cookie: string | undefined, password: string): boolean; destroy(id: string): void; - destroyAllForUser(username: string, exceptId?: string): number; - listForUser(username: string): SessionSummary[]; + /** `account` is an `accountKey`, as carried on `LiveSession.account`. */ + destroyAllForUser(account: string, exceptId?: string): number; + listForUser(account: string): SessionSummary[]; } const COOKIE_SEP = "."; @@ -172,6 +196,7 @@ export class SessionStore implements SessionBackend { salt: salt.toString("base64"), sealedCredentials: seal(JSON.stringify({ u: params.username, p: params.password }), key), username: params.username, + account: params.account ?? params.username.trim().toLowerCase(), createdAt: now, lastSeenAt: now, expiresAt: now + ttl, @@ -248,10 +273,10 @@ export class SessionStore implements SessionBackend { if (this.sessions.delete(id)) this.scheduleSave(); } - destroyAllForUser(username: string, exceptId?: string): number { + destroyAllForUser(account: string, exceptId?: string): number { let n = 0; for (const [id, s] of this.sessions) { - if (s.username === username && id !== exceptId) { + if (accountOf(s) === account && id !== exceptId) { this.sessions.delete(id); n++; } @@ -260,11 +285,11 @@ export class SessionStore implements SessionBackend { return n; } - listForUser(username: string): SessionSummary[] { + listForUser(account: string): SessionSummary[] { const out = []; for (const s of this.sessions.values()) { - if (s.username !== username) continue; - const { secretHash: _h, salt: _s, sealedCredentials: _c, ...rest } = s; + if (accountOf(s) !== account) continue; + const { secretHash: _h, salt: _s, sealedCredentials: _c, account: _a, ...rest } = s; out.push(rest); } return out; @@ -274,6 +299,7 @@ export class SessionStore implements SessionBackend { return { id: s.id, username, + account: accountOf(s), authorization: `Basic ${Buffer.from(`${username}:${password}`, "utf8").toString("base64")}`, remember: s.remember, createdAt: s.createdAt, diff --git a/web/src/lib/__tests__/address.test.ts b/web/src/lib/__tests__/address.test.ts index fda7e94..974221f 100644 --- a/web/src/lib/__tests__/address.test.ts +++ b/web/src/lib/__tests__/address.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { formatAddress, initials, isValidEmail, parseAddressList, parseMailto } from "../address"; +import { displayName, formatAddress, initials, isValidEmail, parseAddressList, parseMailto } from "../address"; describe("address parsing", () => { it("parses mixed lists", () => { @@ -55,3 +55,14 @@ describe("mailto URLs", () => { expect(m.to).toHaveLength(1); }); }); + +describe("names that reorder themselves", () => { + const spoof = { name: "support@bank.example\u202E", email: "x@evil.example" }; + it("lose their direction controls when displayed", () => { + expect(displayName({ name: "\u202Egnp.exe\u202C Ann", email: "a@x.io" })).toBe("gnp.exe Ann"); + expect(formatAddress(spoof)).toBe("support@bank.example "); + }); + it("fall back to the address when nothing else is left", () => { + expect(displayName({ name: "\u200F\u202E", email: "a@x.io" })).toBe("a@x.io"); + }); +}); diff --git a/web/src/lib/__tests__/tnef.test.ts b/web/src/lib/__tests__/tnef.test.ts index 9ea677e..2b51387 100644 --- a/web/src/lib/__tests__/tnef.test.ts +++ b/web/src/lib/__tests__/tnef.test.ts @@ -79,6 +79,12 @@ describe("isTnef", () => { }); describe("parseTnef", () => { + it("takes the direction overrides out of a name", () => { + const out = parseTnef(tnef(file("x.bin", "MZ", [ + { id: ATT.attachment, data: mapi([{ id: 0x3707, type: 0x001f, value: "Invoice_\u202Efdp.exe" }]) }, + ]))); + expect(out[0]!.name).toBe("Invoice_fdp.exe"); + }); it("pulls one attachment out, with its name and bytes", () => { const out = parseTnef(tnef(file("report.pdf", "hello"))); expect(out).toHaveLength(1); diff --git a/web/src/lib/address.ts b/web/src/lib/address.ts index 76cbdd7..06af052 100644 --- a/web/src/lib/address.ts +++ b/web/src/lib/address.ts @@ -1,4 +1,5 @@ import type { EmailAddress } from "@/jmap/types"; +import { withoutBidiControls } from "@/lib/text/text"; const EMAIL_RE = /^[^\s@<>"',;]+@[^\s@<>"',;]+\.[^\s@<>"',;]+$/; @@ -48,9 +49,10 @@ export function parseOne(raw: string): EmailAddress | null { export function formatAddress(a: EmailAddress | null | undefined): string { if (!a) return ""; - if (!a.name) return a.email; - const needsQuote = /[,;<>"()\\]/.test(a.name); - const name = needsQuote ? `"${a.name.replace(/(["\\])/g, "\\$1")}"` : a.name; + const clean = a.name ? withoutBidiControls(a.name) : ""; + if (!clean) return a.email; + const needsQuote = /[,;<>"()\\]/.test(clean); + const name = needsQuote ? `"${clean.replace(/(["\\])/g, "\\$1")}"` : clean; return `${name} <${a.email}>`; } @@ -60,7 +62,8 @@ export function formatAddressList(list: EmailAddress[] | null | undefined): stri export function displayName(a: EmailAddress | null | undefined, fallback = "(unknown)"): string { if (!a) return fallback; - if (a.name?.trim()) return a.name.trim(); + const name = a.name ? withoutBidiControls(a.name).trim() : ""; + if (name) return name; return a.email || fallback; } diff --git a/web/src/lib/text/text.ts b/web/src/lib/text/text.ts index 19d04c3..478a453 100644 --- a/web/src/lib/text/text.ts +++ b/web/src/lib/text/text.ts @@ -1,3 +1,12 @@ +/** + * Remove the characters that reorder text around them: direction overrides, + * embeddings, isolates and marks. A sender-supplied name has no honest use for + * them, and `Invoice_\u202Efdp.exe` displays as "Invoice_exe.pdf". + */ +export function withoutBidiControls(s: string): string { + return s.replace(/[\u061C\u200E\u200F\u202A-\u202E\u2066-\u2069]/g, ""); +} + export function escapeHtml(s: string): string { return s.replace(/&/g, "&").replace(//g, ">").replace(/"/g, """).replace(/'/g, "'"); } diff --git a/web/src/lib/tnef.ts b/web/src/lib/tnef.ts index 30c23bd..379c6da 100644 --- a/web/src/lib/tnef.ts +++ b/web/src/lib/tnef.ts @@ -1,3 +1,4 @@ +import { withoutBidiControls } from "@/lib/text/text"; /** * `winmail.dat`, opened. * @@ -210,7 +211,7 @@ export function parseTnef(input: ArrayBuffer | Uint8Array): TnefAttachment[] { current = null; return; } - const name = (current.mapiName || current.title || "attachment").trim() || "attachment"; + const name = withoutBidiControls(current.mapiName || current.title || "attachment").trim() || "attachment"; out.push({ name, type: current.mapiType || guessType(name), diff --git a/web/src/store/__tests__/compose-sign-out.test.ts b/web/src/store/__tests__/compose-sign-out.test.ts new file mode 100644 index 0000000..190102b --- /dev/null +++ b/web/src/store/__tests__/compose-sign-out.test.ts @@ -0,0 +1,53 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useCompose } from "@/store/compose"; +import { useSession } from "@/store/session"; + +/** + * A message being written belongs to the session it was written in. On a + * shared machine the next person to sign in -- after an idle sign-out, with no + * reload in between -- used to find the last one's composer still open. + */ + +beforeEach(() => { + vi.useFakeTimers(); + useSession.setState({ status: "authenticated" }); +}); + +afterEach(() => { + useCompose.setState({ drafts: [], activeKey: null, pendingSends: {} }); + vi.useRealTimers(); +}); + +describe("signing out", () => { + it("closes every composer and stops sends that are still waiting", () => { + const run = vi.fn(async () => {}); + const timer = window.setTimeout(() => void run(), 5000); + useCompose.setState({ + drafts: [{ key: "d1", subject: "Half written" } as never], + activeKey: "d1", + pendingSends: { d2: { timer, toastId: 1, draft: { key: "d2" } as never, run } }, + }); + useSession.setState({ status: "anonymous" }); + expect(useCompose.getState().drafts).toEqual([]); + expect(useCompose.getState().activeKey).toBeNull(); + expect(useCompose.getState().pendingSends).toEqual({}); + vi.advanceTimersByTime(10_000); + expect(run).not.toHaveBeenCalled(); + }); + + it("leaves the composer alone while still signed in", () => { + useCompose.setState({ drafts: [{ key: "d1" } as never], activeKey: "d1" }); + useSession.setState({ pushConnected: true }); + expect(useCompose.getState().drafts).toHaveLength(1); + }); + + it("sends what is inside its undo window before the session goes", async () => { + const run = vi.fn(async () => {}); + const timer = window.setTimeout(() => void run(), 5000); + useCompose.setState({ pendingSends: { d2: { timer, toastId: 1, draft: { key: "d2" } as never, run } } }); + await useCompose.getState().flushPendingSends(); + expect(run).toHaveBeenCalledTimes(1); + vi.advanceTimersByTime(10_000); + expect(run).toHaveBeenCalledTimes(1); + }); +}); diff --git a/web/src/store/compose.ts b/web/src/store/compose.ts index 4436aed..6944a9b 100644 --- a/web/src/store/compose.ts +++ b/web/src/store/compose.ts @@ -7,6 +7,7 @@ import { escapeHtml, htmlToText, quoteText, replySubject, textToHtml } from "@/l import { sanitizeEmailHtml, sanitizeEditorHtml } from "@/lib/text/html"; import { toast } from "@/ui/toast"; import { useMail, FULL_PROPS, BODY_PROPS } from "./mail"; +import { useSession } from "./session"; import { ensureScheduledMailbox, useScheduled } from "./scheduled"; import { formatScheduleTime, holdUntil } from "@/lib/schedule"; import { t as translate } from "@/lib/i18n"; @@ -82,7 +83,9 @@ export interface Draft { interface ComposeState { drafts: Draft[]; activeKey: string | null; - pendingSends: Record; + pendingSends: Record Promise }>; + /** Send everything still inside its undo window now. For signing out, while the session can still send. */ + flushPendingSends(): Promise; open(init?: Partial): string; /** Open a draft holding what the operating system's share sheet sent us. */ openFromShare(share: SharedContent): string; @@ -632,7 +635,16 @@ export const useCompose = create((set, get) => ({ } const toastId = toast.show(translate("Sending…"), { duration: delay * 1000, progress: true, action: { label: translate("Undo"), onClick: () => get().undoSend(key) } }); const timer = window.setTimeout(() => void doSend(), delay * 1000); - set((s) => ({ pendingSends: { ...s.pendingSends, [key]: { timer, toastId, draft: d } } })); + set((s) => ({ pendingSends: { ...s.pendingSends, [key]: { timer, toastId, draft: d, run: doSend } } })); + }, + + async flushPendingSends() { + const pending = Object.values(get().pendingSends); + for (const p of pending) { + window.clearTimeout(p.timer); + toast.dismiss(p.toastId); + } + await Promise.all(pending.map((p) => p.run())); }, undoSend(key) { @@ -999,3 +1011,28 @@ export function draftFromMailto(url: string): Partial { ...(body ? { html: body, text: m.body } : {}), }; } + +/* + * Nothing written in one session is left for the next. + * + * The other stores let go of their data when the session ends; this one used + * to keep its open composers, so on a shared machine the next person to sign + * in -- without a reload, after an idle sign-out, say -- found the last one's + * draft open and could send it. A draft that was saved is still in Drafts on + * the server. A send still in its undo window was sent on the way out if the + * sign-out was a deliberate one (see `logout`); if the session had already + * ended there is nothing left to send it with, so its timer is stopped rather + * than let it fire under whoever signs in next. + */ +useSession.subscribe((s) => { + if (s.status !== "anonymous") return; + const { drafts, pendingSends } = useCompose.getState(); + if (!drafts.length && !Object.keys(pendingSends).length) return; + for (const t of autosaveTimers.values()) window.clearTimeout(t); + autosaveTimers.clear(); + for (const p of Object.values(pendingSends)) { + window.clearTimeout(p.timer); + toast.dismiss(p.toastId); + } + useCompose.setState({ drafts: [], activeKey: null, pendingSends: {} }); +}); diff --git a/web/src/store/session.ts b/web/src/store/session.ts index 259d285..7f069e8 100644 --- a/web/src/store/session.ts +++ b/web/src/store/session.ts @@ -78,6 +78,14 @@ export const useSession = create((set, get) => ({ /* never block signing out over this */ } stopSettingsSync(); + // A message still inside its undo window goes now, while there is a + // session to send it with; signing out is not an undo. + try { + const { useCompose } = await import("./compose"); + await useCompose.getState().flushPendingSends(); + } catch { + /* never block signing out over this */ + } try { await apiFetch("/api/auth/logout", { method: "POST" }); } catch { diff --git a/web/src/ui/filepreview.tsx b/web/src/ui/filepreview.tsx index d7b17c7..899800e 100644 --- a/web/src/ui/filepreview.tsx +++ b/web/src/ui/filepreview.tsx @@ -2,6 +2,7 @@ import { useCallback, useEffect, useMemo, useRef, useState, type ReactNode } fro import { Code2, Download, Eye, Pencil, Printer, Save, Share2, X } from "lucide-react"; import { confirmDialog, Dialog } from "./dialog"; import { formatSize } from "@/lib/format"; +import { withoutBidiControls } from "@/lib/text/text"; import { previewKind, TEXT_PREVIEW_CHARS, TEXT_PREVIEW_MAX } from "@/lib/preview"; import { isMarkdown, renderMarkdown } from "@/lib/text/markdown"; import { canShareFiles, shareFile } from "@/lib/share"; @@ -168,7 +169,7 @@ export function FilePreviewDialog({ const download = () => { const l = document.createElement("a"); l.href = file.url; - l.download = file.name; + l.download = withoutBidiControls(file.name); l.click(); }; try { @@ -226,7 +227,7 @@ export function FilePreviewDialog({ URL.createObjectURL(new Blob([f.data as unknown as BlobPart], { type: f.type })))); + /* + * The type inside a winmail.dat is whatever the sender wrote, and never + * passed the server's check on what may be shown inline. Opened from its + * blob: URL, text/html would render as a page on this origin -- so only + * the types the server itself would show are kept. + */ + setUrls(found.map((f) => URL.createObjectURL(new Blob([f.data as unknown as BlobPart], { type: openableInTab(f.type) ? f.type : "application/octet-stream" })))); setState("done"); } catch { setState("error"); @@ -859,7 +865,7 @@ function AttachmentList({ attachments, accountId, email }: { attachments: EmailB */ const shareAttachment = async (a: EmailBodyPart) => { if (!a.blobId) return; - const name = a.name ?? "attachment"; + const name = withoutBidiControls(a.name ?? "") || "attachment"; const download = () => { const l = document.createElement("a"); l.href = client.downloadUrl(accountId, a.blobId!, name, a.type); @@ -885,16 +891,17 @@ function AttachmentList({ attachments, accountId, email }: { attachments: EmailB ))}
{attachments.map((a, i) => { - const url = a.blobId ? client.downloadUrl(accountId, a.blobId, a.name ?? "attachment", a.type) : "#"; - const inlineUrl = a.blobId ? client.downloadUrl(accountId, a.blobId, a.name ?? "attachment", a.type, true) : "#"; + const name = a.name ? withoutBidiControls(a.name) : null; + const url = a.blobId ? client.downloadUrl(accountId, a.blobId, name ?? "attachment", a.type) : "#"; + const inlineUrl = a.blobId ? client.downloadUrl(accountId, a.blobId, name ?? "attachment", a.type, true) : "#"; return ( - { if (viewable(a)) { ev.preventDefault(); setPreview(a); } }}> + { if (viewable(a)) { ev.preventDefault(); setPreview(a); } }}> {a.type.startsWith("image/") && a.type !== "image/svg+xml" && a.blobId ? : attachmentIcon(a.type, a.name)} - {a.name ?? "(unnamed)"} + {name ?? "(unnamed)"} {formatSize(a.size)} - + {canShareFiles() && a.blobId && } {openableInTab(a.type) && a.blobId && } diff --git a/web/src/views/settings/SecuritySettings.tsx b/web/src/views/settings/SecuritySettings.tsx index 675e572..4379bcf 100644 --- a/web/src/views/settings/SecuritySettings.tsx +++ b/web/src/views/settings/SecuritySettings.tsx @@ -231,6 +231,7 @@ function TwoFactorOff({ reload }: { reload: () => Promise }) { function AppPasswords({ state, reload }: { state: SecurityState | null; reload: () => Promise }) { const [name, setName] = useState(""); + const [current, setCurrent] = useState(""); const [busy, setBusy] = useState(false); const [issued, setIssued] = useState<{ description: string; secret: string } | null>(null); @@ -242,10 +243,11 @@ function AppPasswords({ state, reload }: { state: SecurityState | null; reload: try { const res = await apiFetch<{ id: string; secret: string }>("/api/account/app-passwords", { method: "POST", - body: JSON.stringify({ description: name }), + body: JSON.stringify({ description: name, current }), }); setIssued({ description: name, secret: res.secret }); setName(""); + setCurrent(""); await reload(); } catch (err) { toast.error((err as Error).message); @@ -296,7 +298,12 @@ function AppPasswords({ state, reload }: { state: SecurityState | null; reload: setName(e.target.value)} placeholder={t("Thunderbird on my laptop")} required />
- + {/* A credential that outlives this session: the server asks for the password first. */} +
+ + setCurrent(e.target.value)} required /> +
+ setIssued(null)} title={t("Your new app password")} size="sm"