From ef94266b1201d2b52563f1dda773b2863acc47ab Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Sun, 12 Jul 2026 16:44:56 +0800 Subject: [PATCH 1/2] =?UTF-8?q?fix(settings):=20verified=20authz=20on=20se?= =?UTF-8?q?ttings=20routes=20=E2=80=94=20close=20unauth=20write=20(Critica?= =?UTF-8?q?l)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The settings HTTP routes trusted spoofable identity headers (x-user-id / x-tenant-id / x-permissions) and the write path (setMany) had NO permission gate. On a standard `os serve --server` (settings + HTTP server composed by default, routes on the raw app with no auth middleware) an UNAUTHENTICATED remote client could write tenant- or platform-scoped settings — including the auth security-policy manifest — and enumerate every namespace. - Verified identity: SettingsServicePlugin derives the caller's identity and capabilities from the platform's verified resolution (resolveAuthzContext: session cookie / API key / OAuth), never from request headers. The route default is now SECURE (anonymous, denied). - Capability gates: manifest readPermission/writePermission enforced for HTTP callers (reads of a protected namespace, writes, and actions require the declared capability; writes default to at least the read capability, never ungated). Gated behind an `enforced` flag set only at the HTTP boundary — in-process/boot callers (kernel.getService, seed) are unchanged and keep trusted access. - Unauthenticated HTTP → 403 SETTINGS_FORBIDDEN. Found by an adversarial review of the request→ExecutionContext trust boundary. Tests: service-level enforced-mode gates + route-level deny (anonymous read hidden, write 403, spoofed headers grant nothing, read-only cannot write). service-settings suite 144 green. Co-Authored-By: Claude Opus 4.8 --- .changeset/settings-routes-verified-authz.md | 17 ++++ .../src/settings-routes.test.ts | 91 +++++++++++++++++-- .../service-settings/src/settings-routes.ts | 59 ++++++------ .../src/settings-service-plugin.ts | 48 +++++++++- .../src/settings-service.test.ts | 47 ++++++++++ .../service-settings/src/settings-service.ts | 48 +++++++++- .../src/settings-service.types.ts | 30 ++++++ 7 files changed, 303 insertions(+), 37 deletions(-) create mode 100644 .changeset/settings-routes-verified-authz.md diff --git a/.changeset/settings-routes-verified-authz.md b/.changeset/settings-routes-verified-authz.md new file mode 100644 index 0000000000..3fb40b518f --- /dev/null +++ b/.changeset/settings-routes-verified-authz.md @@ -0,0 +1,17 @@ +--- +'@objectstack/service-settings': minor +--- + +**Security fix (Critical): the settings HTTP routes no longer trust spoofable identity headers, and writes are now capability-gated.** + +Previously `GET/PUT/POST /api/settings/*` derived the caller's identity from `x-user-id` / `x-tenant-id` / `x-permissions` request headers (the route default), and `setMany` performed **no permission check** — so on a standard `os serve --server` deployment (settings + HTTP server composed by default, routes registered on the raw app with no auth middleware) an **unauthenticated** remote client could write tenant- or platform-scoped settings (including the auth security-policy, localization, and company manifests) and enumerate every namespace. + +Fixes: + +- **Verified identity.** `SettingsServicePlugin` now derives the caller's identity and capabilities from the platform's verified resolution (`resolveAuthzContext` — session cookie / API key / OAuth), never from request headers. The route default is now SECURE: it trusts no identity header and yields an anonymous, denied context. +- **Capability gates.** Manifest `readPermission` / `writePermission` are enforced for HTTP callers: reads of a protected namespace, writes, and actions require the declared capability (writes default to at least the read capability, never ungated). Enforced via a new `enforced` flag set only at the HTTP boundary — **in-process/boot callers (`kernel.getService('settings')`, seed) are unchanged** and keep full trusted access. +- Unauthenticated HTTP callers can no longer enumerate protected manifests or write; a `403 SETTINGS_FORBIDDEN` is returned when the capability is missing. + +**Behaviour change:** a deployment that relied on the old header-trusted default must present a real verified session/API-key/OAuth credential (which the console already does). A custom integration may still inject its own `contextFromRequest`. + +Found by an adversarial security review of the request→ExecutionContext trust boundary. diff --git a/packages/services/service-settings/src/settings-routes.test.ts b/packages/services/service-settings/src/settings-routes.test.ts index deaead90de..c54e11da17 100644 --- a/packages/services/service-settings/src/settings-routes.test.ts +++ b/packages/services/service-settings/src/settings-routes.test.ts @@ -41,12 +41,17 @@ function makeReqRes(opts: { params?: Record; body?: any; headers return { req, res, state }; } +// [Finding-1] An authorized admin context (verified, holds the branding +// manifest's setup.access/setup.write capabilities). The production plugin +// derives this from the verified session/API-key; here we inject it directly. +const adminProvider = () => ({ enforced: true, permissions: ['setup.access', 'setup.write'] }); + describe('settings-routes', () => { it('GET /api/settings → manifests', async () => { const http = new MockHttp(); const svc = new SettingsService(); svc.registerManifest(brandingSettingsManifest); - registerSettingsRoutes(http, svc); + registerSettingsRoutes(http, svc, { contextFromRequest: adminProvider }); const h = http.routes.get('GET /api/settings')!; const { req, res, state } = makeReqRes(); @@ -58,7 +63,7 @@ describe('settings-routes', () => { const http = new MockHttp(); const svc = new SettingsService({ env: {} }); svc.registerManifest(brandingSettingsManifest); - registerSettingsRoutes(http, svc); + registerSettingsRoutes(http, svc, { contextFromRequest: adminProvider }); const h = http.routes.get('GET /api/settings/:namespace')!; const { req, res, state } = makeReqRes({ params: { namespace: 'branding' } }); @@ -71,7 +76,7 @@ describe('settings-routes', () => { const http = new MockHttp(); const svc = new SettingsService({ env: { OS_BRANDING_WORKSPACE_NAME: 'X' } }); svc.registerManifest(brandingSettingsManifest); - registerSettingsRoutes(http, svc); + registerSettingsRoutes(http, svc, { contextFromRequest: adminProvider }); const h = http.routes.get('PUT /api/settings/:namespace')!; const { req, res, state } = makeReqRes({ params: { namespace: 'branding' }, body: { workspace_name: 'Y' } }); @@ -94,7 +99,7 @@ describe('settings-routes', () => { const http = new MockHttp(); const svc = new SettingsService({ env: {} }); svc.registerManifest(brandingSettingsManifest); - registerSettingsRoutes(http, svc); + registerSettingsRoutes(http, svc, { contextFromRequest: adminProvider }); const h = http.routes.get('PUT /api/settings/:namespace')!; const { req, res, state } = makeReqRes({ params: { namespace: 'branding' }, body: { values: { workspace_name: 'My Co' } } }); @@ -108,7 +113,7 @@ describe('settings-routes', () => { const http = new MockHttp(); const svc = new SettingsService({ env: {} }); svc.registerManifest(brandingSettingsManifest); - registerSettingsRoutes(http, svc); + registerSettingsRoutes(http, svc, { contextFromRequest: adminProvider }); const h = http.routes.get('PUT /api/settings/:namespace')!; // Exactly what GET returns: { values: { key: { value, source, ... } } } @@ -126,7 +131,7 @@ describe('settings-routes', () => { const svc = new SettingsService({ env: {} }); svc.registerManifest(brandingSettingsManifest); svc.registerAction('branding', 'ping', () => ({ ok: true, message: 'pong' })); - registerSettingsRoutes(http, svc); + registerSettingsRoutes(http, svc, { contextFromRequest: adminProvider }); const h = http.routes.get('POST /api/settings/:namespace/:actionId')!; const { req, res, state } = makeReqRes({ params: { namespace: 'branding', actionId: 'ping' }, body: null }); @@ -134,4 +139,78 @@ describe('settings-routes', () => { expect(state.status).toBe(200); expect(state.body.ok).toBe(true); }); + + // ── [Finding-1] the DEFAULT (no verified provider) is SECURE ────────────── + // Header-trusted identity is gone; an unauthenticated request can neither + // enumerate protected namespaces nor write them. + it('anonymous GET /api/settings hides manifests that require a read capability', async () => { + const http = new MockHttp(); + const svc = new SettingsService(); + svc.registerManifest(brandingSettingsManifest); // requires setup.access + registerSettingsRoutes(http, svc); // secure default → anonymous + enforced + + const h = http.routes.get('GET /api/settings')!; + const { req, res, state } = makeReqRes(); + await h(req, res); + expect(state.body.manifests.length).toBe(0); + }); + + it('anonymous GET /api/settings/:ns is DENIED (403), not served', async () => { + const http = new MockHttp(); + const svc = new SettingsService({ env: {} }); + svc.registerManifest(brandingSettingsManifest); + registerSettingsRoutes(http, svc); + + const h = http.routes.get('GET /api/settings/:namespace')!; + const { req, res, state } = makeReqRes({ params: { namespace: 'branding' } }); + await h(req, res); + expect(state.status).toBe(403); + expect(state.body.error.code).toBe('SETTINGS_FORBIDDEN'); + }); + + it('anonymous PUT /api/settings/:ns is DENIED (403) — the unauthenticated write hole is closed', async () => { + const http = new MockHttp(); + const svc = new SettingsService({ env: {} }); + svc.registerManifest(brandingSettingsManifest); + registerSettingsRoutes(http, svc); + + const h = http.routes.get('PUT /api/settings/:namespace')!; + const { req, res, state } = makeReqRes({ params: { namespace: 'branding' }, body: { workspace_name: 'pwn' } }); + await h(req, res); + expect(state.status).toBe(403); + expect(state.body.error.code).toBe('SETTINGS_FORBIDDEN'); + }); + + it('a spoofed x-user-id / x-permissions header grants NOTHING (default ignores identity headers)', async () => { + const http = new MockHttp(); + const svc = new SettingsService({ env: {} }); + svc.registerManifest(brandingSettingsManifest); + registerSettingsRoutes(http, svc); + + const h = http.routes.get('PUT /api/settings/:namespace')!; + const { req, res, state } = makeReqRes({ + params: { namespace: 'branding' }, + body: { workspace_name: 'pwn' }, + headers: { 'x-user-id': 'attacker', 'x-permissions': 'setup.write,setup.access' }, + }); + await h(req, res); + expect(state.status).toBe(403); + }); + + it('a caller holding only read (setup.access) may read but NOT write', async () => { + const http = new MockHttp(); + const svc = new SettingsService({ env: {} }); + svc.registerManifest(brandingSettingsManifest); + registerSettingsRoutes(http, svc, { contextFromRequest: () => ({ enforced: true, permissions: ['setup.access'] }) }); + + const read = http.routes.get('GET /api/settings/:namespace')!; + const r1 = makeReqRes({ params: { namespace: 'branding' } }); + await read(r1.req, r1.res); + expect(r1.state.body.manifest.namespace).toBe('branding'); + + const write = http.routes.get('PUT /api/settings/:namespace')!; + const r2 = makeReqRes({ params: { namespace: 'branding' }, body: { workspace_name: 'X' } }); + await write(r2.req, r2.res); + expect(r2.state.status).toBe(403); // has setup.access, lacks setup.write + }); }); diff --git a/packages/services/service-settings/src/settings-routes.ts b/packages/services/service-settings/src/settings-routes.ts index f919d4505d..011651b1a1 100644 --- a/packages/services/service-settings/src/settings-routes.ts +++ b/packages/services/service-settings/src/settings-routes.ts @@ -16,6 +16,7 @@ import type { IHttpServer, IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; import { SettingsService } from './settings-service.js'; import { + SettingsForbiddenError, SettingsLockedError, SettingsValidationError, UnknownKeyError, @@ -27,27 +28,23 @@ export interface SettingsRoutesOptions { /** Base path. Default `/api/settings`. */ basePath?: string; /** - * Extract caller identity from the request. The default reads - * `x-user-id` / `x-tenant-id` headers and parses - * `x-permissions` as a comma-separated list — fine for dev and - * straightforward to override in production wiring. + * Derive the VERIFIED caller identity from the request. Production wiring + * (`SettingsServicePlugin`) passes a resolver backed by the platform's + * verified session / API-key / OAuth resolution (`resolveAuthzContext`), so + * `permissions` reflect real capabilities and are never spoofable. + * + * [Finding-1] The default is SECURE: it trusts NO identity header and yields + * an anonymous, `enforced` context (deny protected reads + all writes). The + * old default trusted `x-user-id` / `x-permissions` headers, which let an + * unauthenticated client forge any identity and write platform settings. */ - contextFromRequest?: (req: IHttpRequest) => SettingsContext; + contextFromRequest?: (req: IHttpRequest) => SettingsContext | Promise; } -const defaultContext = (req: IHttpRequest): SettingsContext => { - const header = (name: string): string | undefined => { - const v = req.headers?.[name]; - return Array.isArray(v) ? v[0] : v; - }; - const perms = header('x-permissions'); - return { - userId: header('x-user-id'), - tenantId: header('x-tenant-id'), - permissions: perms ? perms.split(',').map((s) => s.trim()).filter(Boolean) : undefined, - requestId: header('x-request-id'), - }; -}; +// [Finding-1] Secure default: anonymous + enforced. No identity is read from +// request headers — a deployment that wants authenticated settings access must +// wire a verified `contextFromRequest` (the plugin does). +const defaultContext = (_req: IHttpRequest): SettingsContext => ({ enforced: true }); function sendError(res: IHttpResponse, status: number, code: string, message: string, extra?: Record) { res.status(status).json({ error: { code, message, ...extra } }); @@ -63,22 +60,28 @@ export function registerSettingsRoutes( http.get(base, (async (req, res) => { try { - const ctx = ctxOf(req); + const ctx = await ctxOf(req); const manifests = service.listManifests(ctx); await res.json({ manifests }); } catch (err: any) { - sendError(res, 500, 'INTERNAL', err?.message ?? 'Failed to list manifests'); + if (err instanceof SettingsForbiddenError) { + sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { namespace: err.namespace }); + } else { + sendError(res, 500, 'INTERNAL', err?.message ?? 'Failed to list manifests'); + } } }) satisfies RouteHandler); http.get(`${base}/:namespace`, (async (req, res) => { const ns = req.params.namespace; try { - const ctx = ctxOf(req); + const ctx = await ctxOf(req); const payload = await service.getNamespace(ns, ctx); await res.json(payload); } catch (err: any) { - if (err instanceof UnknownNamespaceError) { + if (err instanceof SettingsForbiddenError) { + sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { namespace: err.namespace }); + } else if (err instanceof UnknownNamespaceError) { sendError(res, 404, 'UNKNOWN_NAMESPACE', err.message); } else { sendError(res, 500, 'INTERNAL', err?.message ?? 'Failed to read namespace'); @@ -110,11 +113,13 @@ export function registerSettingsRoutes( ); } try { - const ctx = ctxOf(req); + const ctx = await ctxOf(req); const result = await service.setMany(ns, body, ctx); await res.json({ values: result }); } catch (err: any) { - if (err instanceof SettingsLockedError) { + if (err instanceof SettingsForbiddenError) { + sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { namespace: err.namespace }); + } else if (err instanceof SettingsLockedError) { sendError(res, 409, 'SETTINGS_LOCKED', err.message, { namespace: err.namespace, key: err.key, @@ -138,12 +143,14 @@ export function registerSettingsRoutes( http.post(`${base}/:namespace/:actionId`, (async (req, res) => { const { namespace, actionId } = req.params; try { - const ctx = ctxOf(req); + const ctx = await ctxOf(req); const result = await service.runAction(namespace, actionId, req.body, ctx); const status = result.ok ? 200 : 400; await res.status(status).json(result); } catch (err: any) { - if (err instanceof UnknownNamespaceError) { + if (err instanceof SettingsForbiddenError) { + sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { namespace: err.namespace }); + } else if (err instanceof UnknownNamespaceError) { sendError(res, 404, 'UNKNOWN_NAMESPACE', err.message); } else { sendError(res, 500, 'INTERNAL', err?.message ?? 'Action failed'); diff --git a/packages/services/service-settings/src/settings-service-plugin.ts b/packages/services/service-settings/src/settings-service-plugin.ts index 5d35885115..7096714936 100644 --- a/packages/services/service-settings/src/settings-service-plugin.ts +++ b/packages/services/service-settings/src/settings-service-plugin.ts @@ -1,7 +1,9 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { Plugin, PluginContext } from '@objectstack/core'; -import type { IHttpServer, IDataEngine } from '@objectstack/spec/contracts'; +import { resolveAuthzContext } from '@objectstack/core'; +import type { IHttpServer, IDataEngine, IHttpRequest } from '@objectstack/spec/contracts'; +import type { SettingsContext } from './settings-service.types.js'; import type { SettingsManifest } from '@objectstack/spec/system'; import { SettingsService } from './settings-service.js'; import type { ICryptoProvider } from '@objectstack/spec/contracts'; @@ -200,7 +202,49 @@ export class SettingsServicePlugin implements Plugin { ); return; } - registerSettingsRoutes(http, this.service!, { basePath: this.opts.basePath }); + // [Finding-1] Derive the caller's identity + capabilities from the + // platform's VERIFIED resolution (session cookie / API key / OAuth), NOT + // from spoofable `x-user-id` / `x-permissions` headers. `resolveAuthzContext` + // returns real, server-verified capabilities; `enforced: true` makes the + // service apply the manifest read/write permission gates. An unresolvable + // request fails CLOSED (anonymous + enforced → denied). + const verifiedContextFromRequest = async (req: IHttpRequest): Promise => { + try { + const ql: any = ctx.getService('objectql'); + const headers = new Headers(); + for (const [k, v] of Object.entries(req.headers ?? {})) { + if (v == null) continue; + headers.set(String(k), Array.isArray(v) ? v.join(',') : String(v)); + } + const getSession = async (h: any) => { + try { + const authService: any = ctx.getService('auth'); + let api: any = authService?.api; + if (!api && typeof authService?.getApi === 'function') api = await authService.getApi(); + return await api?.getSession?.({ headers: h }); + } catch { + return undefined; + } + }; + const authz = await resolveAuthzContext({ ql, headers, getSession }); + return { + userId: authz.userId, + tenantId: authz.tenantId, + // Manifest read/write permissions are system CAPABILITIES + // (setup.access / setup.write / manage_platform_settings) — union + // both aggregates so either kind of declared gate resolves. + permissions: [...(authz.systemPermissions ?? []), ...(authz.permissions ?? [])], + enforced: true, + }; + } catch { + return { enforced: true }; // fail closed + } + }; + + registerSettingsRoutes(http, this.service!, { + basePath: this.opts.basePath, + contextFromRequest: verifiedContextFromRequest, + }); ctx.logger?.info?.( 'SettingsServicePlugin: REST routes registered at ' + (this.opts.basePath ?? '/api/settings'), ); diff --git a/packages/services/service-settings/src/settings-service.test.ts b/packages/services/service-settings/src/settings-service.test.ts index 5eba90d41d..cc2111be0e 100644 --- a/packages/services/service-settings/src/settings-service.test.ts +++ b/packages/services/service-settings/src/settings-service.test.ts @@ -174,6 +174,53 @@ describe('SettingsService — listManifests permission filter', () => { }); }); +describe('SettingsService — [Finding-1] enforced (HTTP-boundary) authz', () => { + const admin = { enforced: true, permissions: ['setup.access', 'setup.write'] }; + const anon = { enforced: true }; + + it('listManifests: an enforced caller with no capability sees NOTHING (no pass-through)', () => { + const svc = new SettingsService(); + svc.registerManifest(brandingSettingsManifest); + // Trusted (non-enforced) empty ctx still passes through … + expect(svc.listManifests({ permissions: [] }).length).toBe(1); + // … but an enforced empty ctx does not. + expect(svc.listManifests(anon).length).toBe(0); + expect(svc.listManifests(admin).length).toBe(1); + }); + + it('getNamespace: enforced read requires the manifest readPermission', async () => { + const svc = new SettingsService({ env: {} }); + svc.registerManifest(brandingSettingsManifest); // readPermission: setup.access + await expect(svc.getNamespace('branding', anon)).rejects.toMatchObject({ code: 'SETTINGS_FORBIDDEN' }); + await expect(svc.getNamespace('branding', admin)).resolves.toBeTruthy(); + // Trusted in-process caller (no enforced) is never gated. + await expect(svc.getNamespace('branding', {})).resolves.toBeTruthy(); + }); + + it('setMany: enforced write requires the manifest writePermission (the closed hole)', async () => { + const svc = new SettingsService({ env: {} }); + svc.registerManifest(brandingSettingsManifest); // writePermission: setup.write + // No capability → denied. + await expect(svc.setMany('branding', { workspace_name: 'X' }, anon)).rejects.toMatchObject({ code: 'SETTINGS_FORBIDDEN' }); + // Read-only capability is NOT enough to write. + await expect( + svc.setMany('branding', { workspace_name: 'X' }, { enforced: true, permissions: ['setup.access'] }), + ).rejects.toMatchObject({ code: 'SETTINGS_FORBIDDEN' }); + // Full write capability → allowed. + await expect(svc.setMany('branding', { workspace_name: 'X' }, admin)).resolves.toBeTruthy(); + // Trusted in-process caller (no enforced) writes without a capability. + await expect(svc.setMany('branding', { workspace_name: 'Y' }, {})).resolves.toBeTruthy(); + }); + + it('runAction: enforced action requires the write capability', async () => { + const svc = new SettingsService({ env: {} }); + svc.registerManifest(brandingSettingsManifest); + svc.registerAction('branding', 'ping', () => ({ ok: true, message: 'pong' })); + await expect(svc.runAction('branding', 'ping', null, anon)).rejects.toMatchObject({ code: 'SETTINGS_FORBIDDEN' }); + await expect(svc.runAction('branding', 'ping', null, admin)).resolves.toMatchObject({ ok: true }); + }); +}); + describe('SettingsService — runAction', () => { it('returns an error for unregistered actions', async () => { const svc = new SettingsService(); diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index 50a2f34929..2d9ef4773a 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -22,6 +22,7 @@ import { type SettingsRow, type SettingsServiceOptions, envKeyOf, + SettingsForbiddenError, SettingsLockedError, SettingsValidationError, UnknownKeyError, @@ -208,13 +209,42 @@ export class SettingsService { return reg.manifest; } + /** + * [Finding-1] Capability a manifest demands for an operation. Reads default to + * `setup.access`; writes default to the manifest's `writePermission`, falling + * back to its `readPermission`, then `setup.access` — so a write ALWAYS + * requires at least as much as a read and is never ungated. + */ + private requiredCapability(m: SettingsManifest, op: 'read' | 'write'): string { + return op === 'read' + ? (m.readPermission ?? 'setup.access') + : (m.writePermission ?? m.readPermission ?? 'setup.access'); + } + + /** + * [Finding-1] Enforce the manifest's capability for an ENFORCED (HTTP-boundary) + * caller. Trusted in-process callers (`enforced` unset) are never gated — the + * seed/boot paths that call the service directly keep full access. + */ + private assertPermitted(m: SettingsManifest, op: 'read' | 'write', ctx: SettingsContext): void { + if (!ctx.enforced) return; + const required = this.requiredCapability(m, op); + const held = new Set(ctx.permissions ?? []); + if (!held.has(required)) { + throw new SettingsForbiddenError(m.namespace, required, op); + } + } + /** List all registered manifests, optionally filtered by permission. */ listManifests(ctx: SettingsContext = {}): SettingsManifest[] { const perms = new Set(ctx.permissions ?? []); const all = Array.from(this.registry.values()).map((r) => r.manifest); - // Empty permissions ⇒ pass-through (server-side trust, e.g. boot tests). - if (perms.size === 0) return all; - return all.filter((m) => perms.has(m.readPermission ?? 'setup.access')); + // Empty permissions ⇒ pass-through ONLY for a trusted (non-enforced) + // in-process caller. An ENFORCED HTTP caller with no capabilities sees only + // the manifests it may read — never the whole set (Finding-1: an + // unauthenticated request previously enumerated every namespace). + if (perms.size === 0 && !ctx.enforced) return all; + return all.filter((m) => perms.has(this.requiredCapability(m, 'read'))); } /** Register a handler for an `action_button` declared in a manifest. */ @@ -325,6 +355,9 @@ export class SettingsService { ): Promise { const reg = this.registry.get(namespace); if (!reg) throw new UnknownNamespaceError(namespace); + // [Finding-1] Reading a namespace's values requires the manifest's read + // capability for an enforced (HTTP) caller. + this.assertPermitted(reg.manifest, 'read', ctx); const values: Record = {}; for (const [key] of reg.scopes) { @@ -424,6 +457,11 @@ export class SettingsService { ): Promise> { const reg = this.registry.get(namespace); if (!reg) throw new UnknownNamespaceError(namespace); + // [Finding-1] Writing requires the manifest's write capability for an + // enforced (HTTP) caller. Checked BEFORE any lock/validation work so an + // unauthorized write is rejected outright (this is the gate that was + // missing — the write path previously trusted a spoofable header identity). + this.assertPermitted(reg.manifest, 'write', ctx); // Pre-flight: reject the whole batch if any key is locked or unknown. for (const key of Object.keys(patch)) { @@ -680,6 +718,10 @@ export class SettingsService { ): Promise { const reg = this.registry.get(namespace); if (!reg) throw new UnknownNamespaceError(namespace); + // [Finding-1] Settings actions (test-connection, rotate, reset, …) are + // mutating/side-effecting operations — gate them like a write for an + // enforced (HTTP) caller. + this.assertPermitted(reg.manifest, 'write', ctx); const handler = reg.actions.get(actionId); if (!handler) { // Built-in fallback: every namespace gets a `reset` action that diff --git a/packages/services/service-settings/src/settings-service.types.ts b/packages/services/service-settings/src/settings-service.types.ts index 1ec3e46653..0b0a9ee3f8 100644 --- a/packages/services/service-settings/src/settings-service.types.ts +++ b/packages/services/service-settings/src/settings-service.types.ts @@ -36,6 +36,16 @@ export interface SettingsContext { permissions?: string[]; /** Source IP / request id for audit correlation. */ requestId?: string; + /** + * [Finding-1] Marks a context that arrived across the HTTP trust boundary + * (set by the settings routes) as opposed to a trusted in-process/boot caller. + * When true, the service ENFORCES the manifest's `readPermission` / + * `writePermission` and drops the "empty permissions ⇒ pass-through" escape — + * so an unauthenticated request can neither enumerate protected namespaces nor + * write. In-process callers (seed/boot/kernel.getService('settings')) leave it + * unset and keep full trusted access. + */ + enforced?: boolean; } /** Storage row shape used by both the engine and the in-memory store. */ @@ -234,6 +244,26 @@ export class UnknownKeyError extends Error { } } +/** + * [Finding-1] Thrown when an ENFORCED (HTTP-boundary) caller lacks the + * capability a manifest declares for the operation — `readPermission` for + * reads, `writePermission` for writes/actions. Maps to HTTP 403. Trusted + * in-process callers (no `enforced` flag) never hit this. + */ +export class SettingsForbiddenError extends Error { + readonly code = 'SETTINGS_FORBIDDEN' as const; + constructor( + readonly namespace: string, + readonly required: string, + readonly operation: 'read' | 'write', + ) { + super( + `Access denied: ${operation === 'read' ? 'reading' : 'writing to'} settings namespace ` + + `'${namespace}' requires the '${required}' capability`, + ); + } +} + /** * Thrown when a write would leave the namespace in an invalid state — * a `required` field that is visible under the post-write values is From d50dbbe70699c0b8f0ea22a54e9437f19594493b Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Sun, 12 Jul 2026 17:01:53 +0800 Subject: [PATCH 2/2] fix(security): declare + grant setup.write so admins can write Setup settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enforcing the settings manifests' declared writePermission (previous commit) surfaced a modeling gap: `setup.write` — the write counterpart to `setup.access`, used by the branding/company/localization/feature-flag manifests — was referenced but NEVER declared as a capability or granted to any permission set. Harmless while settings writes went ungated; under enforcement it meant NOBODY (not even an admin) could write those namespaces (403). - Declare `setup.write` in PLATFORM_CAPABILITIES (scope: org). - Grant it to admin_full_access and organization_admin (the sets that already hold setup.access / manage tenant settings). Verified: the analytics-timezone dogfood (which PUTs /api/settings/ localization as the signed-in admin, then buckets) now passes — proving the admin bearer resolves through the settings plugin's verified resolution and the write gate admits it. plugin-security 298, service-settings 144, spec api-surface unchanged. Co-Authored-By: Claude Opus 4.8 --- .changeset/settings-routes-verified-authz.md | 4 ++++ .../plugin-security/src/objects/default-permission-sets.ts | 3 ++- packages/spec/src/security/capabilities.ts | 5 +++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/.changeset/settings-routes-verified-authz.md b/.changeset/settings-routes-verified-authz.md index 3fb40b518f..fe214b8689 100644 --- a/.changeset/settings-routes-verified-authz.md +++ b/.changeset/settings-routes-verified-authz.md @@ -1,5 +1,7 @@ --- '@objectstack/service-settings': minor +'@objectstack/spec': minor +'@objectstack/plugin-security': patch --- **Security fix (Critical): the settings HTTP routes no longer trust spoofable identity headers, and writes are now capability-gated.** @@ -12,6 +14,8 @@ Fixes: - **Capability gates.** Manifest `readPermission` / `writePermission` are enforced for HTTP callers: reads of a protected namespace, writes, and actions require the declared capability (writes default to at least the read capability, never ungated). Enforced via a new `enforced` flag set only at the HTTP boundary — **in-process/boot callers (`kernel.getService('settings')`, seed) are unchanged** and keep full trusted access. - Unauthenticated HTTP callers can no longer enumerate protected manifests or write; a `403 SETTINGS_FORBIDDEN` is returned when the capability is missing. +**`setup.write` capability now real.** Enforcing the manifests' declared `writePermission` surfaced a modeling gap: `setup.write` (the write counterpart to `setup.access`, used by the branding / company / localization / feature-flag manifests) was referenced but never declared or granted — so under enforcement *nobody*, not even an admin, could write those namespaces. It is now a declared platform capability (`PLATFORM_CAPABILITIES`) held by `admin_full_access` and `organization_admin`, alongside `setup.access`. + **Behaviour change:** a deployment that relied on the old header-trusted default must present a real verified session/API-key/OAuth credential (which the console already does). A custom integration may still inject its own `contextFromRequest`. Found by an adversarial security review of the request→ExecutionContext trust boundary. diff --git a/packages/plugins/plugin-security/src/objects/default-permission-sets.ts b/packages/plugins/plugin-security/src/objects/default-permission-sets.ts index fa9af50452..10fb289d26 100644 --- a/packages/plugins/plugin-security/src/objects/default-permission-sets.ts +++ b/packages/plugins/plugin-security/src/objects/default-permission-sets.ts @@ -102,6 +102,7 @@ export const defaultPermissionSets: PermissionSet[] = [ 'manage_metadata', 'manage_platform_settings', 'setup.access', + 'setup.write', 'studio.access', ], }), @@ -154,7 +155,7 @@ export const defaultPermissionSets: PermissionSet[] = [ sys_user_permission_set: { allowRead: true, allowCreate: false, allowEdit: false, allowDelete: false }, sys_user_position: { allowRead: true, allowCreate: false, allowEdit: false, allowDelete: false }, }, - systemPermissions: ['manage_org_users', 'setup.access'], + systemPermissions: ['manage_org_users', 'setup.access', 'setup.write'], rowLevelSecurity: [ { name: 'tenant_isolation', diff --git a/packages/spec/src/security/capabilities.ts b/packages/spec/src/security/capabilities.ts index 791f9a53f2..7df63869e1 100644 --- a/packages/spec/src/security/capabilities.ts +++ b/packages/spec/src/security/capabilities.ts @@ -38,6 +38,11 @@ export const PLATFORM_CAPABILITIES: readonly PlatformCapability[] = [ { name: 'manage_metadata', label: 'Manage Metadata', description: 'Author and publish object/view/flow and other metadata.', scope: 'platform' }, { name: 'manage_platform_settings', label: 'Manage Platform Settings', description: 'Configure global platform settings (mail, storage, AI, licensing, …) and platform-only Setup pages.', scope: 'platform' }, { name: 'setup.access', label: 'Setup Access', description: 'Enter the Setup app shell.', scope: 'platform' }, + // [Finding-1] The write counterpart to `setup.access`: saving changes to + // tenant/Setup settings pages (branding, company, localization, feature + // flags). Previously referenced by settings manifests but never declared or + // granted — which was harmless only while settings writes went ungated. + { name: 'setup.write', label: 'Write Settings', description: 'Save changes to tenant/Setup settings pages.', scope: 'org' }, { name: 'studio.access', label: 'Studio Access', description: 'Enter the Studio metadata-design surfaces.', scope: 'platform' }, ];