From ab94a3a5104425ec77833d6d67d6e3b51bffb317 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Wed, 19 Aug 2026 18:47:56 +0000 Subject: [PATCH] notify: a member could list their phones but not register one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `personal` is matched against what remains after the permission's own prefix is stripped, so the entries had to be the sidecar's real paths. they were written '/devices' and '/subscriptions'; the sidecar serves '/_officer/devices'. nothing matched, so read level permitted the GET (a safe method, allowed anyway) and refused the POST and the DELETE — a member could see their registered devices and neither add nor remove one. that is the exact opposite of the comment sitting above it, which says device registration is the caller's own. invisible so far because the owner short-circuits every check and no member has ever been granted notify. found while writing the app-side spec: the mobile client posts to /api/notify/_officer/devices, which is what made the mismatch visible. the regression test pins the real path and fails against the old value — checked, not assumed. --- src/servers/permissions/registry.test.ts | 15 +++++++++++++++ src/servers/permissions/registry.ts | 7 ++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/servers/permissions/registry.test.ts b/src/servers/permissions/registry.test.ts index a4414be6..8f775133 100644 --- a/src/servers/permissions/registry.test.ts +++ b/src/servers/permissions/registry.test.ts @@ -283,3 +283,18 @@ describe('kinds', () => { } }); }); + +test('notify: a read-level member can register and deregister their own device', () => { + // Regression. `personal` was ['/devices'], but the sidecar serves '/_officer/devices' — and `personal` + // is matched against the path AFTER the permission's own prefix is stripped, so it matched nothing. + // A member could GET their devices and neither add nor remove one, which is the opposite of the intent. + const notify = permissionForApiPath('/api/notify/_officer/devices'); + expect(notify?.key).toBe('notify'); + + expect(isRequestAllowedAtLevel(notify!, 'read', 'POST', '/api/notify/_officer/devices')).toBe(true); + expect(isRequestAllowedAtLevel(notify!, 'read', 'DELETE', '/api/notify/_officer/devices/tok')).toBe(true); + expect(isRequestAllowedAtLevel(notify!, 'read', 'GET', '/api/notify/_officer/devices')).toBe(true); + + // Still scoped: read level does not become write level for the rest of the surface. + expect(isRequestAllowedAtLevel(notify!, 'read', 'POST', '/api/notify/_officer/notify')).toBe(false); +}); diff --git a/src/servers/permissions/registry.ts b/src/servers/permissions/registry.ts index 312ef00d..2489b9fa 100644 --- a/src/servers/permissions/registry.ts +++ b/src/servers/permissions/registry.ts @@ -169,7 +169,12 @@ const CORE_REGISTRY: Permission[] = [ kind: 'app', api: ['/notify'], // Device registration is the caller's own — a phone subscribing to its own push channel. - personal: ['/devices', '/subscriptions'], + // + // The `_officer/` segment is NOT decoration: these are the sidecar's paths, and `personal` is matched + // against what remains after the permission's own prefix is stripped. Written as `/devices` it matched + // nothing, so a read-level member could list their phones but not register or delete one — the exact + // opposite of the line above. Invisible in testing because the owner bypasses every check. + personal: ['/_officer/devices', '/_officer/subscriptions'], }, // `vpn` (POST /api/vpn/enroll) was here until 2026-08-14 — the one member-grantable piece of headscale, // minting a pre-auth key bound to the caller. Deleted because it had no caller anywhere: the standalone