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"