From 8367fada1353e5eef1880114486af6affd0c501f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Tue, 18 Aug 2026 14:47:04 +0000 Subject: [PATCH] plugins: let a plugin claim top-level paths and divert /api by header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restores the setup the vault extraction regressed: paste the bare Officer URL into a Bitwarden client and log in. Nothing else. The extraction moved the Bitwarden door to /api/vault/bw, which is correct and unusable — a client derives /api, /identity, /icons and /notifications from ONE url it is given, so a prefixed base turns setup into five fields copied by hand into an advanced pane. That is not a feature anyone completes, and the owner said so. My regression, my wrong call: I treated "a plugin cannot serve top-level paths" as a fact about the platform when it was a fact about what had been built. Two additions, both manifest-declared, both narrow: unauthenticatedRootPaths single lowercase segments (`/identity`), served above the gate. Refused if reserved (`/api`, `/auth`, `/plugins`, `/dav`, …), refused if another plugin already claims it, refused if it is anything more structural than one name. protocolHeaders hands `/api/*` to the plugin when the request carries one. Needed because /api is the one namespace both servers claim — Vaultwarden has /api/sync, Officer has /api/chat — and the path cannot tell them apart, so the client says who it is. ROUTING, not authentication: a forged header reaches the plugin, which demands its own credential, and cannot reach anything of Officer's. The worst it does is cost that caller Officer's API. The Bun route table gets an entry per claimed segment, built from the plugin set — `refreshPluginMounts` runs at line ~204, long before serve() at ~378, so the claims are known. This is the constraint I earlier asserted made the whole thing impossible, without checking it. The real cost is smaller and stated in the code: a plugin claiming a root path is not fully installed until `pm2 restart officer`, because the table is fixed once serve() is called. Every claim is logged at boot by plugin — a root path answering without a session should not be something you read a manifest to discover. 13 more tests (30 in the file): reserved segments, malformed segments, two plugins colliding, and segment-not-substring matching so `/identityx` never matches `/identity`. Suite 1074 pass / 0 fail. Co-Authored-By: Claude Opus 5 --- src/server.tsx | 20 +++++ src/servers/hono.ts | 47 ++++++++++- src/servers/plugins/manifest.ts | 99 ++++++++++++++++++++++++ src/servers/plugins/mount.ts | 59 +++++++++++++- src/servers/plugins/public-paths.test.ts | 72 ++++++++++++++++- src/servers/plugins/public-paths.ts | 82 +++++++++++++++++++- 6 files changed, 371 insertions(+), 8 deletions(-) diff --git a/src/server.tsx b/src/server.tsx index 2faaf7ac..cd4b5768 100644 --- a/src/server.tsx +++ b/src/server.tsx @@ -5,6 +5,7 @@ import { join } from 'node:path'; import { honoServer, PROTECTED_API_PREFIXES, UNPROTECTED_API_PREFIXES } from './servers/hono'; import { assertPermissionTotality } from './servers/permissions/totality'; import { refreshPluginMounts, snapshotPlugins } from './servers/plugins/mount'; +import { listPluginRootClaims } from './servers/plugins/public-paths'; import { generatePluginsModule, rebuildFrontend, BUILD_DIR, SHELL_FILE } from './servers/plugins/generate'; import { assertInstallLayout } from './servers/data-path'; import { PORT } from './servers/officer-url.mjs'; @@ -429,6 +430,25 @@ const server = serve({ // '/.well-known/carddav': honoServer.fetch, // '/dav': honoServer.fetch, // '/dav/*': honoServer.fetch, + /* + Top-level paths a plugin claims (`/identity/*`, …), handed to hono BEFORE the SPA fallback. + + They need naming here for the same reason `/dav` did: Bun matches this table first, and only the + paths listed reach hono — anything else falls through to `/*`, which answers 200 with the React + shell. A missing entry therefore looks like a working endpoint returning nonsense rather than a 404, + which is among the least debuggable failures available. + + Built from the plugin set because `refreshPluginMounts` has already run by here (line ~204, well + before `serve()`), so the claims are known. The cost is that adding or removing one needs a restart — + this table is fixed once `serve()` is called. Stated rather than worked around: installing a plugin + that claims a root path is the one install that is not complete until `pm2 restart officer`. + */ + ...Object.fromEntries( + listPluginRootClaims().flatMap(({ segment }) => [ + [`/${segment}`, (req: Request, server: unknown) => honoServer.fetch(req, server)], + [`/${segment}/*`, (req: Request, server: unknown) => honoServer.fetch(req, server)], + ]), + ), ...spaRoutes, '/api': honoServer.fetch, // A CLOSURE, deliberately, and not the bound `honoServer.fetch`. diff --git a/src/servers/hono.ts b/src/servers/hono.ts index 3da18cc4..47f99b57 100644 --- a/src/servers/hono.ts +++ b/src/servers/hono.ts @@ -22,7 +22,7 @@ import { dashboardsRouter } from './api/dashboards'; import { router as fileBrowserRouter } from './api/file-browser/router'; import { devProxyRouter } from './api/dev-proxy/router'; import { pluginsRouter } from './api/plugins/router'; -import { isPluginPublicPath } from './plugins/public-paths'; +import { isPluginPublicPath, pluginForProtocolHeaders } from './plugins/public-paths'; import { agentHandoffRouter } from './api/agent-handoff/router'; // import { slskdRouter } from './api/slskd/router'; // import { transmissionRouter } from './api/transmission/router'; @@ -64,7 +64,20 @@ export type { HonoVariables }; * A plugin's router and where it mounts — a plain pair, so this file needs no plugin knowledge at all. * Built by `plugins/mount.ts`, which is the side that knows what a manifest is. */ -export type MountedPlugin = { prefix: string; router: ReturnType }; +export type MountedPlugin = { + prefix: string; + router: ReturnType; + /** + * An OPTIONAL second router served above the account gate — at declared top-level paths, and for + * `/api/*` requests carrying a declared protocol header. Absent for every plugin that does not have a + * foreign protocol to host; see `unauthenticatedRootPaths` in plugins/manifest.ts. + */ + publicRouter?: ReturnType; + /** Top-level segments this plugin claims, already de-duplicated against other plugins. */ + rootSegments: string[]; + /** Headers that hand an `/api/*` request to `publicRouter` instead of to Officer. */ + protocolHeaders: string[]; +}; // ── The app is BUILT, not assembled once ── // @@ -188,6 +201,36 @@ export function buildHonoApp(plugins: MountedPlugin[] = []): Hono<{ Variables: H app.use((ctx, next) => (isDavPath(ctx.req.path) ? next() : corsMiddleware(ctx, next))); + /* + A plugin's PUBLIC router, mounted above everything — the gate, `userMiddleware`, the whole platform API. + + Two shapes, both for the same reason: a client speaking a protocol Officer does not define, which + derives its own URLs and cannot be told to look elsewhere. See `unauthenticatedRootPaths` in + plugins/manifest.ts for why this exists at all and how narrow it is kept. + + root paths `/identity/*` and friends — a single claimed segment each, refused if reserved or + already taken. + protocol header `/api/*` when the request carries a declared header. Registered BEFORE the platform's + own `/api` mount, because hono matches in registration order and this has to win. + ROUTING, not authentication — a forged header reaches the plugin, which demands its + own credential; it cannot reach anything of Officer's. + */ + for (const plugin of plugins) { + if (!plugin.publicRouter) continue; + for (const segment of plugin.rootSegments) app.route(`/${segment}`, plugin.publicRouter); + } + + const protocolPlugins = plugins.filter((p) => p.publicRouter && p.protocolHeaders.length); + if (protocolPlugins.length) { + app.use('/api/*', async (ctx, next) => { + const appName = pluginForProtocolHeaders(ctx.req.raw.headers); + if (!appName) return next(); + const claimant = protocolPlugins.find((p) => p.prefix === `/${appName}` || p.prefix.endsWith(`/${appName}`)); + if (!claimant?.publicRouter) return next(); + return claimant.publicRouter.fetch(ctx.req.raw, ctx.env); + }); + } + // The authorization gate: a valid non-owner token reaches only what its role grants. Ahead of every // router, and it re-verifies the token itself so it covers routes that never mount userMiddleware. app.use(permissionGateMiddleware); diff --git a/src/servers/plugins/manifest.ts b/src/servers/plugins/manifest.ts index f4c5e1a6..3d2a6ab7 100644 --- a/src/servers/plugins/manifest.ts +++ b/src/servers/plugins/manifest.ts @@ -166,6 +166,48 @@ export type PluginManifest = { */ unauthenticatedPaths?: string[]; + /** + * TOP-LEVEL paths this plugin serves, above the account gate and outside its own `/api/` mount. + * + * ── Why this exists, when `unauthenticatedPaths` already does ── + * + * Some clients are not ours and cannot be told where to look. A Bitwarden client asked for a + * self-hosted server takes ONE URL and derives `/identity`, `/icons`, `/notifications` and `/events` + * from it itself — at the root, always. Hosting the same thing under a plugin's mount is technically + * equivalent and practically useless: it turns "paste your Officer URL" into five fields copied by hand + * into an advanced settings pane, which is not a thing anyone will do and not a thing to ship. + * + * So this is the escape hatch for a protocol the platform does not define and cannot move. It is + * deliberately narrower than it looks: + * + * - one segment only (`/identity`, never `/a/b`), so a plugin claims a name rather than a namespace + * - refused if it collides with anything the platform serves, checked against a reserved list + * - refused if another installed plugin already claims it + * - every entry logged at boot, by plugin, because a root path answering without a session is not + * something anyone should have to read a manifest to discover + * + * Absent for every plugin but one, and it should stay that way — reach for `unauthenticatedPaths` + * unless a client genuinely cannot be pointed anywhere else. + */ + unauthenticatedRootPaths?: string[]; + + /** + * Request headers that hand `/api/*` to this plugin instead of to Officer. + * + * The other half of the same problem. Vaultwarden serves `/api/sync`, `/api/ciphers`; Officer serves + * `/api/chat`, `/api/settings`. A Bitwarden client pointed at the bare Officer URL calls the first set + * at the same prefix as the second, and the PATH cannot tell them apart — so the client says who it is, + * and every Bitwarden client stamps `Bitwarden-Client-Name` (or older ones `Device-Type`). + * + * ── This is ROUTING, not authentication ── + * + * Trusting a client-supplied header is fine here, and it is worth being precise about why: the worst a + * forged header achieves is reaching the plugin's own public router, which then demands whatever + * credential it demands. Nothing is authorised by this. An attacker who sets the header does not gain + * access to Officer's API — they lose it, for their own request. + */ + protocolHeaders?: string[]; + /** * Host binaries this plugin needs. Absent means it is self-sufficient, which offscale and example are. * @@ -254,6 +296,42 @@ export function publicPathProblem(path: unknown): string | null { return null; } +/** + * Top-level names the platform serves itself. A plugin claiming one would shadow it for every request, + * so each is refused by name rather than by ordering luck. + * + * `api` is the important one: it is where everything of Officer's lives, and `protocolHeaders` is the + * only sanctioned way to touch it. + */ +const RESERVED_ROOT = new Set([ + 'api', + 'auth', + 'plugins', + 'assets', + 'build', + 'public', + 'dav', + '.well-known', + 'index.html', +]); + +/** + * Check one declared ROOT path, returning a reason it is refused or null. + * + * Stricter than `publicPathProblem` because the surface is: this is the platform's own namespace, not the + * plugin's, so a plugin gets to claim a single name and nothing structural. + */ +export function rootPathProblem(path: unknown): string | null { + if (typeof path !== 'string' || !path) return 'must be a non-empty string'; + if (!path.startsWith('/')) return `"${path}" must start with "/"`; + const segment = path.slice(1); + if (!/^[a-z][a-z0-9-]{0,38}$/.test(segment)) { + return `"${path}" must be a single lowercase segment, e.g. "/identity"`; + } + if (RESERVED_ROOT.has(segment)) return `"${path}" is reserved by the platform`; + return null; +} + export function mountPrefix(plugin: { appName: string; manifest: { publisher: string } }): string { const { appName } = plugin; return plugin.manifest.publisher === FIRST_PARTY_PUBLISHER @@ -345,6 +423,27 @@ export function manifestProblems(appName: string, manifest: Partial { export async function loadPluginRouter(plugin: DiscoveredPlugin): Promise { if (!plugin.api) return null; - const module = (await import(plugin.api)) as { router?: unknown }; + const module = (await import(plugin.api)) as { router?: unknown; publicRouter?: unknown }; const router = module.router; if (!router || typeof (router as { fetch?: unknown }).fetch !== 'function') { throw new Error(`${plugin.appName}: api/router.ts must export \`router\` (a Hono router)`); } - return { prefix: mountPrefix(plugin), router: router as MountedPlugin['router'] }; + // Optional, and only meaningful alongside a manifest that declares where it is served. A plugin that + // exports one without declaring anything gets it loaded and mounted nowhere, which is the safe way + // round: the manifest is what the owner consented to, not the export. + const publicRouter = + module.publicRouter && typeof (module.publicRouter as { fetch?: unknown }).fetch === 'function' + ? (module.publicRouter as MountedPlugin['router']) + : undefined; + + return { + prefix: mountPrefix(plugin), + router: router as MountedPlugin['router'], + ...(publicRouter ? { publicRouter } : {}), + rootSegments: [], + protocolHeaders: plugin.manifest.protocolHeaders ?? [], + }; } /** The plugins that should be mounted right now: installed, enabled, and carrying an `api/router.ts`. */ @@ -161,7 +183,38 @@ export async function refreshPluginMounts(options: { skipFrontend?: boolean } = console.log(`[plugins] served WITHOUT a session: ${shown}`); } + // Top-level claims, resolved against ONE accumulating set so two plugins cannot both own `/identity`. + // First come wins and the loser is named, rather than the last mount silently shadowing the first. + const taken = new Set(); + const rootClaims: RootClaim[] = []; + const headerClaims: { header: string; appName: string }[] = []; + for (const { plugin } of enabled) { + const { segments, refused } = resolvePluginRootPaths(plugin, taken); + for (const reason of refused) console.error(`[plugins] root path refused — ${reason}`); + for (const segment of segments) rootClaims.push({ segment, appName: plugin.appName }); + for (const header of plugin.manifest.protocolHeaders ?? []) { + headerClaims.push({ header: header.toLowerCase(), appName: plugin.appName }); + } + } + setPluginRootClaims(rootClaims); + setPluginProtocolHeaders(headerClaims); + if (rootClaims.length) { + console.log(`[plugins] top-level paths: ${rootClaims.map((c) => `/${c.segment} (${c.appName})`).join(', ')}`); + } + if (headerClaims.length) { + console.log( + `[plugins] /api diverted by header: ${headerClaims.map((c) => `${c.header} -> ${c.appName}`).join(', ')}`, + ); + } + const mounted = await mountablePlugins(snapshot); + // The claims were resolved above from the manifests; attach them to the entries hono actually mounts, + // so the router set and the claim set can never describe different plugins. + for (const entry of mounted) { + const appName = entry.prefix.split('/').filter(Boolean).pop(); + entry.rootSegments = rootClaims.filter((c) => c.appName === appName).map((c) => c.segment); + entry.protocolHeaders = headerClaims.filter((c) => c.appName === appName).map((c) => c.header); + } rebuildHonoApp(mounted); // Republish every enabled plugin's `assets/` on the way past. diff --git a/src/servers/plugins/public-paths.test.ts b/src/servers/plugins/public-paths.test.ts index 3cc362e2..9ac0a57a 100644 --- a/src/servers/plugins/public-paths.test.ts +++ b/src/servers/plugins/public-paths.test.ts @@ -1,5 +1,14 @@ import { describe, expect, test, afterEach } from 'bun:test'; -import { isPluginPublicPath, resolvePluginPublicPaths, setPluginPublicPaths } from './public-paths'; +import { + isPluginPublicPath, + pluginForProtocolHeaders, + pluginForRootPath, + resolvePluginPublicPaths, + resolvePluginRootPaths, + setPluginProtocolHeaders, + setPluginPublicPaths, + setPluginRootClaims, +} from './public-paths'; import type { DiscoveredPlugin } from './manifest'; // This is an auth boundary: everything matched here skips `userMiddleware` AND the permission gate. The @@ -27,7 +36,11 @@ const plugin = (appName: string, unauthenticatedPaths?: string[], publisher = 'o web: null, }) as DiscoveredPlugin; -afterEach(() => setPluginPublicPaths([])); +afterEach(() => { + setPluginPublicPaths([]); + setPluginRootClaims([]); + setPluginProtocolHeaders([]); +}); describe('resolving a plugin’s declared paths', () => { test('resolves against the plugin’s own mount', () => { @@ -100,3 +113,58 @@ describe('matching a request path', () => { expect(isPluginPublicPath('/api/users')).toBe(false); }); }); + +describe('top-level claims', () => { + const withRoots = (appName: string, roots: string[]) => { + const p = plugin(appName); + (p.manifest as { unauthenticatedRootPaths?: string[] }).unauthenticatedRootPaths = roots; + return p; + }; + + test('a single lowercase segment is claimed', () => { + const { segments, refused } = resolvePluginRootPaths(withRoots('vault', ['/identity', '/icons']), new Set()); + expect(segments).toEqual(['identity', 'icons']); + expect(refused).toEqual([]); + }); + + // The whole point of the reserved list: a plugin claiming these would shadow the platform for every + // request, and `/api` in particular is where all of Officer lives. + test.each([['/api'], ['/auth'], ['/plugins'], ['/dav']])('refuses the reserved segment %s', (root) => { + const { segments, refused } = resolvePluginRootPaths(withRoots('evil', [root]), new Set()); + expect(segments).toEqual([]); + expect(refused).toHaveLength(1); + }); + + test.each([['/a/b'], ['identity'], ['/Identity'], ['/']])('refuses the malformed segment %s', (root) => { + expect(resolvePluginRootPaths(withRoots('evil', [root]), new Set()).segments).toEqual([]); + }); + + test('two plugins cannot both claim one segment — first come, loser named', () => { + const taken = new Set(); + expect(resolvePluginRootPaths(withRoots('vault', ['/identity']), taken).segments).toEqual(['identity']); + const second = resolvePluginRootPaths(withRoots('other', ['/identity']), taken); + expect(second.segments).toEqual([]); + expect(second.refused[0]).toContain('already claimed'); + }); + + test('matching a claimed root is by segment, not string prefix', () => { + setPluginRootClaims([{ segment: 'identity', appName: 'vault' }]); + expect(pluginForRootPath('/identity')).toBe('vault'); + expect(pluginForRootPath('/identity/connect/token')).toBe('vault'); + expect(pluginForRootPath('/identityx')).toBeNull(); + expect(pluginForRootPath('/other')).toBeNull(); + }); + + test('a protocol header hands /api over, and its absence does not', () => { + setPluginProtocolHeaders([{ header: 'bitwarden-client-name', appName: 'vault' }]); + expect(pluginForProtocolHeaders(new Headers({ 'bitwarden-client-name': 'browser' }))).toBe('vault'); + expect(pluginForProtocolHeaders(new Headers({ authorization: 'Bearer x' }))).toBeNull(); + }); + + test('nothing is claimed by default', () => { + setPluginRootClaims([]); + setPluginProtocolHeaders([]); + expect(pluginForRootPath('/identity')).toBeNull(); + expect(pluginForProtocolHeaders(new Headers({ 'bitwarden-client-name': 'browser' }))).toBeNull(); + }); +}); diff --git a/src/servers/plugins/public-paths.ts b/src/servers/plugins/public-paths.ts index f26c168d..fad5c258 100644 --- a/src/servers/plugins/public-paths.ts +++ b/src/servers/plugins/public-paths.ts @@ -1,4 +1,4 @@ -import { mountPrefix, publicPathProblem, type DiscoveredPlugin } from './manifest'; +import { mountPrefix, publicPathProblem, rootPathProblem, type DiscoveredPlugin } from './manifest'; // Which plugin paths are served ABOVE the account gate. // @@ -85,3 +85,83 @@ export function isPluginPublicPath(path: string): boolean { } return false; } + +// ── Top-level claims: root paths, and the header that hands `/api/*` to a plugin ── +// +// Everything above is about a plugin's own mount. This is about the platform's namespace, which a plugin +// may reach into only for a protocol it does not control and cannot move — see `unauthenticatedRootPaths` +// in manifest.ts for the full argument. Kept in the same file because it is the same question ("what does +// a plugin serve without a session") and splitting it would let the two drift. + +export type RootClaim = { segment: string; appName: string }; + +let rootClaims: RootClaim[] = []; +let protocolHeaders: { header: string; appName: string }[] = []; + +/** + * Resolve a plugin's root claims, refusing collisions against a set already taken. + * + * `taken` is threaded in rather than read from module state so the caller can resolve every plugin + * against one accumulating set — first come, first served, deterministic in install order, and the loser + * is told by name rather than silently overwritten. + */ +export function resolvePluginRootPaths( + plugin: DiscoveredPlugin, + taken: Set, +): { segments: string[]; refused: string[] } { + const segments: string[] = []; + const refused: string[] = []; + for (const path of plugin.manifest.unauthenticatedRootPaths ?? []) { + const problem = rootPathProblem(path); + if (problem) { + refused.push(`${plugin.appName}: ${problem}`); + continue; + } + const segment = path.slice(1); + if (taken.has(segment)) { + refused.push(`${plugin.appName}: "/${segment}" is already claimed by another plugin`); + continue; + } + taken.add(segment); + segments.push(segment); + } + return { segments, refused }; +} + +export function setPluginRootClaims(claims: RootClaim[]): void { + rootClaims = claims; +} + +export function listPluginRootClaims(): RootClaim[] { + return rootClaims; +} + +/** Does this top-level path belong to a plugin? Segment match, so `/identityx` never counts. */ +export function pluginForRootPath(path: string): string | null { + for (const { segment, appName } of rootClaims) { + if (path === `/${segment}` || path.startsWith(`/${segment}/`)) return appName; + } + return null; +} + +export function setPluginProtocolHeaders(entries: { header: string; appName: string }[]): void { + protocolHeaders = entries; +} + +export function listPluginProtocolHeaders(): { header: string; appName: string }[] { + return protocolHeaders; +} + +/** + * Which plugin, if any, claims this `/api/*` request by header. + * + * ROUTING, not authentication: a forged header reaches the plugin's own public router, which demands + * whatever credential it demands. The worst outcome for a caller who sets it is losing access to + * Officer's API for that request. + */ +export function pluginForProtocolHeaders(headers: Headers): string | null { + for (const { header, appName } of protocolHeaders) { + if (headers.has(header)) return appName; + } + return null; +}