From 3e1106b2d95a3b752764cc49de3af4743c45dee5 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Sat, 18 Jul 2026 21:23:35 -0400 Subject: [PATCH] fix(security): tab-nabbing hardening + /acl self-lockout guard (SEC-3/4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SEC-3: add `noopener,noreferrer` to the 5 `window.open(_blank)` sites that don't use the returned handle (UserChips, OidcManageAccount, OtherDevices x2, Verification), closing reverse tab-nabbing. SSOStage is intentionally excluded — it needs the window handle + intact opener for its origin-checked SSO postMessage handshake. SEC-4: guard the `/acl` slash command against bricking the room. - Extract the ACL glob helpers (isValidServerPattern/globToRegExp/matchesAnyGlob) from RoomServerACL into a shared utils/serverAcl.ts (+ unit test) so the command and the settings editor validate identically. - Default a MISSING allow list to `*` only when the room has NO existing ACL (a first `/acl -d x` otherwise sent `allow: []`, which bricks the room); an existing ACL's absent/empty allow is preserved, not silently widened. - Reject invalid globs; fail CLOSED on the universally-catastrophic cases (empty allow, or a `*` deny) even when the local domain is unknown; and reject any change that would ban this homeserver (self-lockout). Guard hardened per two review passes (fail-closed on unknown domain; no silent federation widening). Co-Authored-By: Claude Opus 4.8 --- src/app/components/user-profile/UserChips.tsx | 2 +- .../features/room-settings/RoomServerACL.tsx | 52 +------------------ .../settings/account/OidcManageAccount.tsx | 6 ++- .../settings/devices/OtherDevices.tsx | 2 + .../settings/devices/Verification.tsx | 1 + src/app/hooks/useCommands.ts | 44 +++++++++++++++- src/app/utils/serverAcl.test.ts | 44 ++++++++++++++++ src/app/utils/serverAcl.ts | 52 +++++++++++++++++++ 8 files changed, 149 insertions(+), 54 deletions(-) create mode 100644 src/app/utils/serverAcl.test.ts create mode 100644 src/app/utils/serverAcl.ts diff --git a/src/app/components/user-profile/UserChips.tsx b/src/app/components/user-profile/UserChips.tsx index 53818b433..799019677 100644 --- a/src/app/components/user-profile/UserChips.tsx +++ b/src/app/components/user-profile/UserChips.tsx @@ -114,7 +114,7 @@ export function ServerChip({ server }: { server: string }) { size="300" radii="300" onClick={() => { - window.open(`https://${server}`, '_blank'); + window.open(`https://${server}`, '_blank', 'noopener,noreferrer'); close(); }} > diff --git a/src/app/features/room-settings/RoomServerACL.tsx b/src/app/features/room-settings/RoomServerACL.tsx index 089682567..98531dd69 100644 --- a/src/app/features/room-settings/RoomServerACL.tsx +++ b/src/app/features/room-settings/RoomServerACL.tsx @@ -32,6 +32,7 @@ import { AsyncStatus, useAsyncCallback } from '../../hooks/useAsyncCallback'; import { SequenceCard } from '../../components/sequence-card'; import { SequenceCardStyle } from '../common-settings/styles.css'; import { stopPropagation } from '../../utils/keyboard'; +import { isValidServerPattern, matchesAnyGlob } from '../../utils/serverAcl'; import { useModalStyle } from '../../hooks/useModalStyle'; // ── Types ───────────────────────────────────────────────────────────────────── @@ -48,57 +49,6 @@ const DEFAULT_ACL: ServerAclContent = { allow_ip_literals: false, }; -// ── Validation ──────────────────────────────────────────────────────────────── - -/** - * Validate a server-name glob for an ACL entry. - * - * Matrix ACL `allow`/`deny` entries are globs where `*` (any run of chars) and - * `?` (single char) may appear ANYWHERE — e.g. `*`, `*.example.com`, - * `1.2.3.*`, `10.0.0.?`, `*.evil.*`, `*bad*`. We therefore validate the *glob* - * rather than a concrete hostname: - * - reject empty / whitespace-only - * - allow only hostname/IP chars plus the wildcards `*` and `?` - * (letters, digits, dots, hyphens, colons for ports/IPv6 — NO underscore) - * - reject consecutive/leading/trailing dots (`...`, `.foo`, `foo.`) - * - reject entries with no alphanumeric or wildcard char (bare `-`, lone `:`) - */ -function isValidServerPattern(value: string): boolean { - const v = value.trim(); - if (!v) return false; - // Only hostname/IP glob chars — wildcards may appear at any position. - if (!/^[A-Za-z0-9.:*?-]+$/.test(v)) return false; - // Structural rules for the dotted parts. - if (v.startsWith('.') || v.endsWith('.') || v.includes('..')) return false; - // Must carry actual signal — reject pure punctuation like `-`, `:` or `-.-`. - if (!/[A-Za-z0-9*?]/.test(v)) return false; - return true; -} - -/** - * Convert an ACL glob (`*` = any run, `?` = single char) to an anchored RegExp, - * escaping every other regex metacharacter. Used only for local self-ban - * detection — never sent to the server. - */ -function globToRegExp(glob: string): RegExp { - const escaped = glob.replace(/[.+^${}()|[\]\\]/g, '\\$&'); - const pattern = escaped.replace(/\*/g, '.*').replace(/\?/g, '.'); - // Case-INsensitive: Synapse's glob_to_regex uses IGNORECASE and hostnames are - // case-insensitive, so a deny like `MATRIX.foo.org` must still be detected as - // self-banning `matrix.foo.org` (otherwise the warning is a false negative). - return new RegExp(`^${pattern}$`, 'i'); -} - -function matchesAnyGlob(domain: string, globs: string[]): boolean { - return globs.some((glob) => { - try { - return globToRegExp(glob).test(domain); - } catch { - return false; - } - }); -} - // ── Server list sub-component ───────────────────────────────────────────────── type ServerListProps = { diff --git a/src/app/features/settings/account/OidcManageAccount.tsx b/src/app/features/settings/account/OidcManageAccount.tsx index 1fb3dbff0..46ef66e7a 100644 --- a/src/app/features/settings/account/OidcManageAccount.tsx +++ b/src/app/features/settings/account/OidcManageAccount.tsx @@ -20,7 +20,11 @@ export function OidcManageAccount() { const open = useCallback(() => { const authUrl = authMetadata?.account_management_uri ?? authMetadata?.issuer; if (!authUrl) return; - window.open(withSearchParam(authUrl, { action: accountManagementActions.profile }), '_blank'); + window.open( + withSearchParam(authUrl, { action: accountManagementActions.profile }), + '_blank', + 'noopener,noreferrer', + ); }, [authMetadata, accountManagementActions]); if (!authMetadata) return null; diff --git a/src/app/features/settings/devices/OtherDevices.tsx b/src/app/features/settings/devices/OtherDevices.tsx index e1e8aada4..896aea07c 100644 --- a/src/app/features/settings/devices/OtherDevices.tsx +++ b/src/app/features/settings/devices/OtherDevices.tsx @@ -38,6 +38,7 @@ export function OtherDevices({ devices, refreshDeviceList, showVerification }: O action: accountManagementActions.sessionsList, }), '_blank', + 'noopener,noreferrer', ); }, [authMetadata, accountManagementActions]); @@ -52,6 +53,7 @@ export function OtherDevices({ devices, refreshDeviceList, showVerification }: O device_id: deviceId, }), '_blank', + 'noopener,noreferrer', ); }, [authMetadata, accountManagementActions], diff --git a/src/app/features/settings/devices/Verification.tsx b/src/app/features/settings/devices/Verification.tsx index 2de34e31f..1665e6bf2 100644 --- a/src/app/features/settings/devices/Verification.tsx +++ b/src/app/features/settings/devices/Verification.tsx @@ -278,6 +278,7 @@ export function DeviceVerificationOptions() { action: accountManagementActions.crossSigningReset, }), '_blank', + 'noopener,noreferrer', ); return; } diff --git a/src/app/hooks/useCommands.ts b/src/app/hooks/useCommands.ts index 8e3e33d88..2fbf996fd 100644 --- a/src/app/hooks/useCommands.ts +++ b/src/app/hooks/useCommands.ts @@ -25,6 +25,7 @@ import { import { useRoomNavigate } from './useRoomNavigate'; import { Membership, StateEvent } from '../../types/matrix/room'; import { getStateEvent, sendStateEvent } from '../utils/room'; +import { isValidServerPattern, matchesAnyGlob } from '../utils/serverAcl'; import { splitWithSpace } from '../utils/common'; import { createRoomEncryptionState } from '../components/create-room'; @@ -509,8 +510,15 @@ export const useCommands = (mx: MatrixClient, room: Room): CommandRecord => { StateEvent.RoomServerAcl, )?.getContent(); + // When there is NO existing ACL, default allow to `*` (allow all) so a + // first `/acl -d evil.com` doesn't send `allow: []` — which per spec + // allows NO server and bricks the room. But do NOT synthesize `*` for a + // room that already HAS an ACL whose allow key is absent/empty: that + // would silently widen a deny-all room to allow-all. Such a state is + // preserved as-is and then rejected by the guard below (fail closed, + // rather than silently opening federation). const aclContent: RoomServerAclEventContent = { - allow: serverAcl?.allow ? [...serverAcl.allow] : [], + allow: serverAcl ? (serverAcl.allow ? [...serverAcl.allow] : []) : ['*'], allow_ip_literals: serverAcl?.allow_ip_literals, deny: serverAcl?.deny ? [...serverAcl.deny] : [], }; @@ -534,6 +542,40 @@ export const useCommands = (mx: MatrixClient, room: Room): CommandRecord => { aclContent.allow?.sort(); aclContent.deny?.sort(); + // Validate every added glob (empty/whitespace/malformed patterns). + const invalidPattern = [...allowList, ...denyList].find( + (servername) => !isValidServerPattern(servername), + ); + if (invalidPattern) { + throw new Error(`/acl: invalid server pattern "${invalidPattern}"`); + } + + const resultAllow = aclContent.allow ?? []; + const resultDeny = aclContent.deny ?? []; + + // Fail CLOSED on the universally-catastrophic cases regardless of + // whether we can resolve our own server: an empty allow list bans every + // server, and a `*` deny bans everyone. + if (resultAllow.length === 0 || resultDeny.includes('*')) { + throw new Error( + '/acl: refused — this would ban all servers from the room. Use Room Settings → Server ACL to edit safely.', + ); + } + + // Refuse a change that would lock THIS homeserver out of the room — a + // room-bricking footgun (denying the local server, or an allow list that + // no longer matches it). Mirrors the Room Settings ACL editor's self-ban + // detection. + const localDomain = mx.getDomain() ?? ''; + if ( + localDomain && + (!matchesAnyGlob(localDomain, resultAllow) || matchesAnyGlob(localDomain, resultDeny)) + ) { + throw new Error( + `/acl: refused — this would ban this homeserver (${localDomain}) from the room`, + ); + } + await sendStateEvent(mx, room.roomId, StateEvent.RoomServerAcl, aclContent); }, }, diff --git a/src/app/utils/serverAcl.test.ts b/src/app/utils/serverAcl.test.ts new file mode 100644 index 000000000..20bd81999 --- /dev/null +++ b/src/app/utils/serverAcl.test.ts @@ -0,0 +1,44 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { isValidServerPattern, globToRegExp, matchesAnyGlob } from './serverAcl'; + +test('isValidServerPattern: accepts hostnames, IPs, and globs', () => { + for (const v of [ + '*', + '*.example.com', + 'matrix.org', + '1.2.3.*', + '10.0.0.?', + '*.evil.*', + '*bad*', + ]) { + assert.equal(isValidServerPattern(v), true, v); + } +}); + +test('isValidServerPattern: rejects empty, malformed, and non-glob junk', () => { + for (const v of ['', ' ', '.foo', 'foo.', 'a..b', '-', ':', 'under_score', 'a,b', 'a b']) { + assert.equal(isValidServerPattern(v), false, v); + } +}); + +test('globToRegExp: * = any run, ? = single char, other metachars escaped', () => { + assert.equal(globToRegExp('*').test('anything.org'), true); + assert.equal(globToRegExp('*.evil.com').test('a.evil.com'), true); + assert.equal(globToRegExp('*.evil.com').test('evil.com'), false); // needs the leading label + assert.equal(globToRegExp('1.2.3.?').test('1.2.3.4'), true); + assert.equal(globToRegExp('1.2.3.?').test('1.2.3.45'), false); + // The dot is a literal, not a regex wildcard. + assert.equal(globToRegExp('a.b').test('axb'), false); +}); + +test('globToRegExp: case-insensitive (hostnames + Synapse IGNORECASE)', () => { + assert.equal(globToRegExp('MATRIX.foo.org').test('matrix.foo.org'), true); +}); + +test('matchesAnyGlob: true if any glob matches (self-ban detection)', () => { + assert.equal(matchesAnyGlob('lotusguild.org', ['*']), true); + assert.equal(matchesAnyGlob('lotusguild.org', ['*.evil.com', 'lotus*.org']), true); + assert.equal(matchesAnyGlob('lotusguild.org', ['*.evil.com']), false); + assert.equal(matchesAnyGlob('lotusguild.org', []), false); +}); diff --git a/src/app/utils/serverAcl.ts b/src/app/utils/serverAcl.ts new file mode 100644 index 000000000..c26e61b00 --- /dev/null +++ b/src/app/utils/serverAcl.ts @@ -0,0 +1,52 @@ +// Shared helpers for `m.room.server_acl` server-name globs, used by both the +// Room Settings ACL editor and the `/acl` slash command so their validation and +// self-lockout detection stay identical. + +/** + * Validate a server-name glob for an ACL entry. + * + * Matrix ACL `allow`/`deny` entries are globs where `*` (any run of chars) and + * `?` (single char) may appear ANYWHERE — e.g. `*`, `*.example.com`, + * `1.2.3.*`, `10.0.0.?`, `*.evil.*`, `*bad*`. We therefore validate the *glob* + * rather than a concrete hostname: + * - reject empty / whitespace-only + * - allow only hostname/IP chars plus the wildcards `*` and `?` + * (letters, digits, dots, hyphens, colons for ports/IPv6 — NO underscore) + * - reject consecutive/leading/trailing dots (`...`, `.foo`, `foo.`) + * - reject entries with no alphanumeric or wildcard char (bare `-`, lone `:`) + */ +export function isValidServerPattern(value: string): boolean { + const v = value.trim(); + if (!v) return false; + // Only hostname/IP glob chars — wildcards may appear at any position. + if (!/^[A-Za-z0-9.:*?-]+$/.test(v)) return false; + // Structural rules for the dotted parts. + if (v.startsWith('.') || v.endsWith('.') || v.includes('..')) return false; + // Must carry actual signal — reject pure punctuation like `-`, `:` or `-.-`. + if (!/[A-Za-z0-9*?]/.test(v)) return false; + return true; +} + +/** + * Convert an ACL glob (`*` = any run, `?` = single char) to an anchored RegExp, + * escaping every other regex metacharacter. Used only for local self-ban + * detection — never sent to the server. + */ +export function globToRegExp(glob: string): RegExp { + const escaped = glob.replace(/[.+^${}()|[\]\\]/g, '\\$&'); + const pattern = escaped.replace(/\*/g, '.*').replace(/\?/g, '.'); + // Case-INsensitive: Synapse's glob_to_regex uses IGNORECASE and hostnames are + // case-insensitive, so a deny like `MATRIX.foo.org` must still be detected as + // self-banning `matrix.foo.org` (otherwise the warning is a false negative). + return new RegExp(`^${pattern}$`, 'i'); +} + +export function matchesAnyGlob(domain: string, globs: string[]): boolean { + return globs.some((glob) => { + try { + return globToRegExp(glob).test(domain); + } catch { + return false; + } + }); +}