fix: review round 4 — an account switch unloads as the old account, a share link's lockout costs five networks
Deep-review round 4, targeted at round 3's changes (f569b19c..e7f38396). Accounts - Switching, adding or signing out an account moved the tab's pin before the page unloaded, so the players' resume beacons fired on the way out under the NEW account: one account's playback position saved into the other. The pin is now handed to the next page (pinOnNextPage) and taken over at import; clearActiveAccount is gone. - Switching to a suspended account was refused but left it in the switcher, refused on every click; a deactivated one was not refused at all, and made the default it signed every account in the browser out on the next request. Every account _session_user would refuse — gone, deactivated, suspended, revoked — is refused and dropped from the list. Share page (/watch) - An IPv6 client is limited by its /64 (client_ip becomes client_key, for every limiter): keyed on the full address, one subscriber could rotate through endless identities. The comments now state what the per-link ceiling costs: guessing from five networks can keep a link shut for its window, and guesses already running when it fills are still checked. - The link is looked up before the limiter records the attempt, so made-up tokens no longer grow it by an entry each. Forms - Failing the same way twice is announced again: the sign-in, passphrase and setup errors are keyed on the attempt (a clear and a set in one handler left the alert node untouched). - The install wizard's own validation showed the generic error — it threw plain objects its catch did not read. It sets the step's message. Tests: a re-pin leaving the previous account's notices behind; the pin hand-over; switching to an ended account; the /64 viewer; made-up tokens; the repeated alerts. Each mutation-checked RED for its own reason.
This commit is contained in:
@@ -32,9 +32,13 @@ export default function KeyGate({ meId, compact = false }: { meId: number; compa
|
||||
const [confirm, setConfirm] = useState("");
|
||||
const [curPass, setCurPass] = useState("");
|
||||
const [err, setErr] = useState<string | null>(null);
|
||||
// Keys the error: failing the same way twice is a new node, so a screen reader announces it again (a clear
|
||||
// and a set in one handler commit together, leaving the node untouched).
|
||||
const [attempt, setAttempt] = useState(0);
|
||||
const [busy, setBusy] = useState(false);
|
||||
|
||||
async function submit() {
|
||||
setAttempt((n) => n + 1);
|
||||
setErr(null);
|
||||
if (pass.length < 6) return setErr(t("messages.passTooShort"));
|
||||
if (!configured && pass !== confirm) return setErr(t("messages.passMismatch"));
|
||||
@@ -67,6 +71,7 @@ export default function KeyGate({ meId, compact = false }: { meId: number; compa
|
||||
}
|
||||
|
||||
async function doReset() {
|
||||
setAttempt((n) => n + 1);
|
||||
setErr(null);
|
||||
if (hasPassword && !curPass) return setErr(t("messages.resetNeedPass"));
|
||||
setBusy(true);
|
||||
@@ -107,7 +112,7 @@ export default function KeyGate({ meId, compact = false }: { meId: number; compa
|
||||
/>
|
||||
)}
|
||||
{err && (
|
||||
<div role="alert" className="text-sm text-danger">
|
||||
<div key={attempt} role="alert" className="text-sm text-danger">
|
||||
{err}
|
||||
</div>
|
||||
)}
|
||||
@@ -158,7 +163,7 @@ export default function KeyGate({ meId, compact = false }: { meId: number; compa
|
||||
/>
|
||||
)}
|
||||
{err && (
|
||||
<div role="alert" className="text-sm text-danger">
|
||||
<div key={attempt} role="alert" className="text-sm text-danger">
|
||||
{err}
|
||||
</div>
|
||||
)}
|
||||
|
||||
@@ -4,7 +4,8 @@ import { useTranslation } from "react-i18next";
|
||||
import { useQuery } from "@tanstack/react-query";
|
||||
import { useNavigation, useNavigationActions } from "./NavigationProvider";
|
||||
import { Info, LogOut, Settings, Shield, UserPlus, type LucideIcon } from "lucide-react";
|
||||
import { api, clearActiveAccount, setActiveAccount, type Me } from "../lib/api";
|
||||
import { api, type Me } from "../lib/api";
|
||||
import { pinOnNextPage } from "../lib/activeAccount";
|
||||
import * as e2ee from "../lib/e2ee";
|
||||
import { useLiveQuery } from "../lib/useLiveQuery";
|
||||
import { getUnreadCount, subscribe } from "../lib/notifications";
|
||||
@@ -71,7 +72,7 @@ export default function NavSidebar({
|
||||
} catch {
|
||||
/* IndexedDB unavailable (e.g. private mode) — never let it block the logout + reload */
|
||||
}
|
||||
clearActiveAccount();
|
||||
pinOnNextPage(null);
|
||||
location.reload();
|
||||
}
|
||||
|
||||
@@ -86,8 +87,9 @@ export default function NavSidebar({
|
||||
function switchTo(id: number) {
|
||||
// Per-tab switch: point THIS tab at the account and reload; other tabs keep their own
|
||||
// identity. No server round-trip — req() sends the account header (validated against the
|
||||
// browser wallet server-side), so the cookie's default account is left untouched.
|
||||
setActiveAccount(id);
|
||||
// browser wallet server-side), so the cookie's default account is left untouched. The pin moves with
|
||||
// the reload, not before it: the page going away still unloads as the account it was built for.
|
||||
pinOnNextPage(id);
|
||||
// Drop the previous account's in-module sub-view/overlay before the reload (which would
|
||||
// otherwise preserve history.state across the swap). Otherwise the new account lands on a
|
||||
// stale sub-view — e.g. an open chat thread whose partnerId belongs to the OLD identity
|
||||
@@ -334,7 +336,7 @@ export default function NavSidebar({
|
||||
// Adding an account switches THIS tab to it: drop the tab's pin so that on
|
||||
// return from Google it adopts the freshly-added account (the new default)
|
||||
// and pins that. Other tabs keep their own pinned account.
|
||||
clearActiveAccount();
|
||||
pinOnNextPage(null);
|
||||
window.location.href = "/auth/login";
|
||||
}}
|
||||
className="selectable w-full flex items-center gap-2 text-sm px-2 py-2 rounded-lg text-muted hover:text-fg hover:bg-card transition"
|
||||
|
||||
@@ -36,6 +36,9 @@ export default function SetupWizard() {
|
||||
const [stepIdx, setStepIdx] = useState(0);
|
||||
const [busy, setBusy] = useState(false);
|
||||
const [error, setError] = useState("");
|
||||
// Keys the error: failing the same way twice is a new node, so a screen reader announces it again (a clear
|
||||
// and a set in one handler commit together, leaving the node untouched).
|
||||
const [attempt, setAttempt] = useState(0);
|
||||
// Admin account.
|
||||
const [email, setEmail] = useState("");
|
||||
const [password, setPassword] = useState("");
|
||||
@@ -74,19 +77,21 @@ export default function SetupWizard() {
|
||||
};
|
||||
|
||||
// Each step's "Next" action persists what it collected, then advances. Optional steps with empty
|
||||
// fields just advance (skip). Errors surface inline (the setup calls use the quiet flag).
|
||||
// fields just advance (skip). Errors surface inline (the setup calls use the quiet flag); a step's own
|
||||
// validation sets its message and stays.
|
||||
async function next() {
|
||||
setAttempt((n) => n + 1);
|
||||
setError("");
|
||||
setBusy(true);
|
||||
try {
|
||||
if (step === "admin") {
|
||||
if (!/^[^@\s]+@[^@\s]+\.[^@\s]+$/.test(email))
|
||||
throw { detail: t("setup.admin.invalidEmail") };
|
||||
return setError(t("setup.admin.invalidEmail"));
|
||||
if (password.length < PASSWORD_MIN)
|
||||
throw { detail: t("setup.admin.shortPassword", { n: PASSWORD_MIN }) };
|
||||
return setError(t("setup.admin.shortPassword", { n: PASSWORD_MIN }));
|
||||
await api.setupAdmin(token, email, password);
|
||||
} else if (step === "google" && (gid.trim() || gsecret.trim())) {
|
||||
if (!gid.trim() || !gsecret.trim()) throw { detail: t("setup.google.bothRequired") };
|
||||
if (!gid.trim() || !gsecret.trim()) return setError(t("setup.google.bothRequired"));
|
||||
await api.setupConfig(token, {
|
||||
google_client_id: gid.trim(),
|
||||
google_client_secret: gsecret.trim(),
|
||||
@@ -268,7 +273,7 @@ export default function SetupWizard() {
|
||||
)}
|
||||
|
||||
{error && (
|
||||
<p role="alert" className="text-sm text-danger mt-3">
|
||||
<p key={attempt} role="alert" className="text-sm text-danger mt-3">
|
||||
{error}
|
||||
</p>
|
||||
)}
|
||||
|
||||
@@ -398,6 +398,9 @@ function AuthCard({ verify }: { verify: VerifyEntry }) {
|
||||
const [password, setPassword] = useState("");
|
||||
const [busy, setBusy] = useState(false);
|
||||
const [error, setError] = useState("");
|
||||
// Keys the error: failing the same way twice is a new node, so a screen reader announces it again (a clear
|
||||
// and a set in one handler commit together, leaving the node untouched).
|
||||
const [attempt, setAttempt] = useState(0);
|
||||
// Whether to offer "Continue with Google" — hidden when the instance has no Google OAuth
|
||||
// configured. Optimistic (most instances have it); the rare Google-less instance just sees it
|
||||
// disappear once the config loads.
|
||||
@@ -481,6 +484,7 @@ function AuthCard({ verify }: { verify: VerifyEntry }) {
|
||||
|
||||
async function onSubmit(e: React.FormEvent) {
|
||||
e.preventDefault();
|
||||
setAttempt((n) => n + 1);
|
||||
reset();
|
||||
const mail = email.trim().toLowerCase();
|
||||
if (
|
||||
@@ -687,7 +691,7 @@ function AuthCard({ verify }: { verify: VerifyEntry }) {
|
||||
/>
|
||||
)}
|
||||
{error && (
|
||||
<p role="alert" className="text-xs text-danger">
|
||||
<p key={attempt} role="alert" className="text-xs text-danger">
|
||||
{error}
|
||||
</p>
|
||||
)}
|
||||
|
||||
@@ -0,0 +1,91 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// A form's error is announced (role="alert"), and failing the same way again must be announced again. The
|
||||
// clear and the set of a validation failure commit together, so the alert node used to stay untouched on
|
||||
// the second attempt and a screen reader said nothing — a press that seemed to do nothing.
|
||||
//
|
||||
// i18n is mocked to a key-passthrough; the API and the E2EE module are stubbed so nothing reaches out.
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { cleanup, fireEvent, render, screen } from "@testing-library/react";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
|
||||
vi.mock("react-i18next", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("react-i18next")>();
|
||||
return {
|
||||
...actual,
|
||||
useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }),
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("../lib/api", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("../lib/api")>();
|
||||
return {
|
||||
...actual,
|
||||
api: {
|
||||
...actual.api,
|
||||
authConfig: () => Promise.resolve({ google_enabled: false }),
|
||||
messageMyKey: () => Promise.resolve({ configured: true }),
|
||||
me: () => Promise.resolve({ authenticated: false }),
|
||||
setupInfo: () => Promise.resolve({ secrets_manageable: false }),
|
||||
},
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("../lib/e2ee", () => ({ unlock: vi.fn(), setup: vi.fn(), clearDevice: vi.fn() }));
|
||||
|
||||
import KeyGate from "./KeyGate";
|
||||
import SetupWizard from "./SetupWizard";
|
||||
import Welcome from "./Welcome";
|
||||
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
window.history.replaceState(null, "", "/");
|
||||
});
|
||||
|
||||
// Fails twice the same way; the second alert must be a new node carrying the same message.
|
||||
async function failTwice(fail: () => void, message: string) {
|
||||
fail();
|
||||
const first = await screen.findByRole("alert");
|
||||
expect(first.textContent).toBe(message);
|
||||
fail();
|
||||
const second = screen.getByRole("alert");
|
||||
expect(second.textContent).toBe(message);
|
||||
expect(second).not.toBe(first);
|
||||
}
|
||||
|
||||
describe("a form failing the same way twice announces it twice", () => {
|
||||
it("the message passphrase gate", async () => {
|
||||
render(
|
||||
<QueryClientProvider client={new QueryClient()}>
|
||||
<KeyGate meId={1} />
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
const unlock = await screen.findByText("messages.unlock");
|
||||
fireEvent.change(screen.getByPlaceholderText("messages.passphrase"), {
|
||||
target: { value: "abc" },
|
||||
});
|
||||
await failTwice(() => fireEvent.click(unlock), "messages.passTooShort");
|
||||
});
|
||||
|
||||
it("the sign-in card", async () => {
|
||||
render(
|
||||
<Welcome verify={{ outcome: null, email: null, resetPending: false, dismiss: () => {} }} />,
|
||||
);
|
||||
const email = await screen.findByPlaceholderText("welcome.auth.email");
|
||||
fireEvent.change(email, { target: { value: "not-an-address" } });
|
||||
await failTwice(() => fireEvent.submit(email.closest("form")!), "welcome.auth.invalidEmail");
|
||||
});
|
||||
|
||||
it("the install wizard, with the step's own message rather than the generic one", async () => {
|
||||
window.history.replaceState(null, "", "/?token=setup-token");
|
||||
render(<SetupWizard />);
|
||||
fireEvent.click(await screen.findByText("setup.nav.next")); // past the intro
|
||||
fireEvent.change(await screen.findByPlaceholderText("setup.admin.email"), {
|
||||
target: { value: "not-an-address" },
|
||||
});
|
||||
await failTwice(
|
||||
() => fireEvent.click(screen.getByText("setup.nav.next")),
|
||||
"setup.admin.invalidEmail",
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user