diff --git a/integrationTests/components/fixtures/registered-operation/resources.js b/integrationTests/components/fixtures/registered-operation/resources.js index 8933786f96..f9e9796873 100644 --- a/integrationTests/components/fixtures/registered-operation/resources.js +++ b/integrationTests/components/fixtures/registered-operation/resources.js @@ -37,3 +37,16 @@ server.registerOperation({ throw error; }, }); + +server.registerOperation({ + name: 'component_registered_grantable', + requiresSuperUser: true, + execute: async function componentRegisteredGrantable(op) { + return { + granted: true, + executedOnMainThread: isMainThread, + executedOnThreadId: threadId, + username: op.hdb_user?.username ?? null, + }; + }, +}); diff --git a/integrationTests/components/registered-operation.test.ts b/integrationTests/components/registered-operation.test.ts index 4ac138eeb4..34d2e4e486 100644 --- a/integrationTests/components/registered-operation.test.ts +++ b/integrationTests/components/registered-operation.test.ts @@ -6,6 +6,11 @@ * OPERATION_FUNCTION_MAP instances; the ops-API dispatcher runs on the main thread with its * own instance. The cross-thread bridge (server/serverHelpers/registeredOperations.ts) * forwards an unrecognized operation to one registering worker and relays the result. + * + * The same split governs the role `operations` allowlist: registerOperationPermission marks a + * declared op grantable on the worker, but validateOperations is consulted on the main thread. + * Only an integration test crosses that boundary — unit tests register and validate on one + * thread, so the topology, and therefore the gap, is invisible to them. */ import { suite, test, before, after } from 'node:test'; import { strictEqual, ok } from 'node:assert'; @@ -15,6 +20,24 @@ import { setupHarperWithFixture, teardownHarper, type ContextWithHarper } from ' const FIXTURE_PATH = resolve(import.meta.dirname, 'fixtures/registered-operation'); +const GRANTABLE_OP = 'component_registered_grantable'; +const GRANTED_ROLE = 'component_op_granted_role'; +const GRANTED_USER = 'component_op_granted_user'; +const UNGRANTED_ROLE = 'component_op_ungranted_role'; +const UNGRANTED_USER = 'component_op_ungranted_user'; +const USER_PASS = 'Abc1234!'; +// A trust policy the OIDC operation will accept on every axis except the one under test, so a +// rejection can only be about the operation name. Shapes taken from the GitHub Actions profile's +// own requirements: a canonical audience (explicit port, trailing slash) and specific claims. +const TRUST_POLICY = { + issuer: 'https://token.actions.githubusercontent.com', + audience: 'https://my-instance.harperdb.io:9925/', + claims: { + repository_id: '67890', + workflow_ref: 'HarperFast/my-app/.github/workflows/deploy.yml@refs/heads/main', + }, +}; + suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { async function op(body: any): Promise<{ status: number; body: any }> { const { username, password } = ctx.harper.admin; @@ -29,6 +52,18 @@ suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { return { status: response.status, body: await response.json() }; } + async function asUser(username: string, body: any): Promise<{ status: number; body: any }> { + const response = await fetch(ctx.harper.operationsAPIURL, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Authorization': `Basic ${Buffer.from(`${username}:${USER_PASS}`).toString('base64')}`, + }, + body: JSON.stringify(body), + }); + return { status: response.status, body: await response.json() }; + } + before(async () => { // Multiple HTTP workers so the forward actually has a choice of registering threads. await setupHarperWithFixture(ctx, FIXTURE_PATH, { @@ -88,4 +123,122 @@ suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { strictEqual(status, 400, JSON.stringify(body)); ok(JSON.stringify(body).includes('not found'), `expected operation-not-found error, got: ${JSON.stringify(body)}`); }); + + suite('grantable in a role `operations` allowlist across the worker/main boundary', () => { + before(async () => { + // The announcement is fire-and-forget ITC. A successful forward proves the handler ran, and + // it carries the grantable flag in the same message — so this gates on the exact state + // these tests depend on, rather than on elapsed time. + const deadline = Date.now() + 15_000; + for (;;) { + const { status } = await op({ operation: GRANTABLE_OP }); + if (status === 200) break; + if (Date.now() > deadline) throw new Error(`main thread never registered '${GRANTABLE_OP}'`); + await new Promise((resolve) => setTimeout(resolve, 100)); + } + }); + + test('add_role accepts the worker-registered op name in `operations`', async () => { + const { status, body } = await op({ + operation: 'add_role', + role: GRANTED_ROLE, + permission: { operations: [GRANTABLE_OP] }, + }); + strictEqual(status, 200, JSON.stringify(body)); + }); + + test('add_role still rejects an op name no component registered', async () => { + const { status, body } = await op({ + operation: 'add_role', + role: 'component_op_bogus_role', + permission: { operations: ['component_registered_never_declared'] }, + }); + strictEqual(status, 400, JSON.stringify(body)); + ok( + JSON.stringify(body).includes('component_registered_never_declared'), + `expected the offending op name in the error, got: ${JSON.stringify(body)}` + ); + }); + + test('alter_role accepts it too', async () => { + const { status, body } = await op({ + operation: 'alter_role', + id: GRANTED_ROLE, + permission: { operations: [GRANTABLE_OP, 'user_info'] }, + }); + strictEqual(status, 200, JSON.stringify(body)); + }); + + test('a non-super_user granted the op can actually call it', async () => { + const added = await op({ + operation: 'add_user', + role: GRANTED_ROLE, + username: GRANTED_USER, + password: USER_PASS, + active: true, + }); + strictEqual(added.status, 200, JSON.stringify(added.body)); + + const { status, body } = await asUser(GRANTED_USER, { operation: GRANTABLE_OP }); + strictEqual(status, 200, JSON.stringify(body)); + strictEqual(body.granted, true); + strictEqual(body.username, GRANTED_USER); + strictEqual(body.executedOnMainThread, false); + }); + + test('a non-super_user without the grant is still denied (enforcement unchanged)', async () => { + const role = await op({ + operation: 'add_role', + role: UNGRANTED_ROLE, + permission: { operations: ['user_info'] }, + }); + strictEqual(role.status, 200, JSON.stringify(role.body)); + const added = await op({ + operation: 'add_user', + role: UNGRANTED_ROLE, + username: UNGRANTED_USER, + password: USER_PASS, + active: true, + }); + strictEqual(added.status, 200, JSON.stringify(added.body)); + + const { status, body } = await asUser(UNGRANTED_USER, { operation: GRANTABLE_OP }); + strictEqual(status, 403, JSON.stringify(body)); + }); + + test('add_oidc_trust accepts the op in a trust policy scope', async () => { + const { status, body } = await op({ + operation: 'add_oidc_trust', + id: 'component-op-policy', + ...TRUST_POLICY, + user: GRANTED_USER, + operations: [GRANTABLE_OP], + }); + strictEqual(status, 200, JSON.stringify(body)); + }); + + test('add_oidc_trust still rejects an op name no component registered', async () => { + const { status, body } = await op({ + operation: 'add_oidc_trust', + id: 'component-op-bogus-policy', + ...TRUST_POLICY, + user: GRANTED_USER, + operations: ['component_registered_never_declared'], + }); + strictEqual(status, 400, JSON.stringify(body)); + ok( + JSON.stringify(body).includes('not a Harper operation'), + `expected the trust-policy rejection, got: ${JSON.stringify(body)}` + ); + }); + + test('impersonation accepts an inline role naming the op', async () => { + const { status, body } = await op({ + operation: GRANTABLE_OP, + impersonate: { role: { permission: { operations: [GRANTABLE_OP] } } }, + }); + strictEqual(status, 200, JSON.stringify(body)); + strictEqual(body.granted, true); + }); + }); }); diff --git a/security/authn/oidc/trustPolicyOperations.ts b/security/authn/oidc/trustPolicyOperations.ts index b4d1a1f4ee..42fc8f39bd 100644 --- a/security/authn/oidc/trustPolicyOperations.ts +++ b/security/authn/oidc/trustPolicyOperations.ts @@ -53,15 +53,6 @@ function validate(validation: any): void { * caught here, where the reader is the administrator who wrote it. Delegates to the same helper * add_role/alter_role use, so group names resolve identically rather than through a second * definition that could drift. - * - * Known limitation, inherited rather than introduced: that helper's registry of runtime-registered - * operations is process-local, and the OPERATION_REGISTERED bridge propagates only name→thread - * routing, never grantability (server/serverHelpers/registeredOperations.ts). A component's - * `server.registerOperation` runs in a worker while this operation runs on the main thread, so an - * operation registered that way is NOT recognized here and a policy naming one is rejected. It - * fails closed — a rejected policy, never a widened one — and `add_role`, `alter_role`, and - * impersonation validation all share the gap, which is why the fix belongs to that bridge rather - * than to a local workaround here. */ function assertOperationsAreKnown(operations: string[]): void { const invalidOperation = validateOperations(operations); diff --git a/server/DESIGN.md b/server/DESIGN.md index 7c6b5885a5..529b726a5e 100644 --- a/server/DESIGN.md +++ b/server/DESIGN.md @@ -148,6 +148,16 @@ must run on a worker, `registeredOperations.ts` carries that state in the same-p separately from the structured-cloned body. Never attach trusted dispatch state to an operation payload. +`server.registerOperation()` runs per-worker, so anything the **main** thread must later know about a +registered op has to ride the OPERATION_REGISTERED announcement — a module-local registry populated +during registration exists only in the worker that registered. The bridge carries two such facts +today: name→thread routing (for execution forwarding) and `grantable` (so `validateOperations` on +main will accept the name in a role's `operations` allowlist, for add_role/alter_role, impersonation, +and OIDC trust policies). Adding a third main-thread consumer of a worker-registered fact means +extending that message, not reading a registry that main never populated. Grantability is safe to +mirror because it only widens what an allowlist may _name_; enforcement stays on the worker's +`chooseOperation`. + ## Resource ↔ HTTP boundary `REST.ts → http(request, nextHandler)` is the chief integration point: it takes a `Request`, asks the `Resources` registry for a match, builds a `RequestTarget`, and dispatches into the Resource class's static method. Cache headers are translated to `request.expiresAt` / `onlyIfCached` / `noCache` flags within the same function. diff --git a/server/serverHelpers/registeredOperations.ts b/server/serverHelpers/registeredOperations.ts index 5013c3e786..e62ed51124 100644 --- a/server/serverHelpers/registeredOperations.ts +++ b/server/serverHelpers/registeredOperations.ts @@ -11,7 +11,7 @@ * so a request is sent to exactly ONE registering worker (never broadcast-first-wins). * * - Worker: `registerOperation()` announces the name (OPERATION_REGISTERED) to all threads; - * only the main thread records it, as name -> Set. + * only the main thread records it, as name -> Set, plus `grantable`. * - Main: on an OPERATION_FUNCTION_MAP miss, `getRemoteOperationFunction()` supplies a forwarding * function that sends the request body (OPERATION_EXECUTE_REQUEST) to one live registering * worker and awaits the correlated OPERATION_EXECUTE_RESPONSE. @@ -26,7 +26,11 @@ import * as env from '../../utility/environment/environmentManager.ts'; import harperLogger from '../../utility/logging/harper_logger.ts'; import { ServerError } from '../../utility/errors/hdbError.ts'; import { sendItcEvent } from '../threads/itc.js'; -import { onMessageByType, onThreadExit } from '../threads/manageThreads.js'; +import { hasThreadExited, onMessageByType, onThreadExit } from '../threads/manageThreads.js'; +import { + registerWorkerGrantableOperation, + unregisterWorkerGrantableOperation, +} from '../../utility/operationPermissions.ts'; import { runWithOperationAuthorizationBypass } from './operationAuthorizationState.ts'; const operationLog = harperLogger.loggerWithTag('operation'); @@ -59,6 +63,9 @@ export function setLocalOperationDispatch(dispatch: typeof localDispatch) { /** name -> threadIds of workers that registered it (main thread only) */ const registeredByWorker = new Map>(); +// Per originator, not per name: a rolling deploy whose new generation drops `requiresSuperUser` +// must keep routing the name while retracting grantability, which a name-level flag cannot express. +const grantableByWorker = new Map>(); const pendingExecutions = new Map< number, { targetThreadId: number; resolve: (result: any) => void; reject: (error: Error) => void } @@ -71,27 +78,82 @@ let mainListenersAttached = false; * a lost announcement just means the op stays unreachable (the pre-#1736 status quo), and the * broadcast has its own ack timeout. */ -export function announceRegisteredOperation(name: string) { +export function announceRegisteredOperation(name: string, grantable = false) { if (isMainThread) return; sendItcEvent({ type: terms.ITC_EVENT_TYPES.OPERATION_REGISTERED, - message: { name }, + message: { name, grantable }, }).catch((error) => operationLog.error(`Failed to announce registered operation '${name}'`, error)); } /** * ITC handler (all threads receive the broadcast; only main records it). */ -export function operationRegisteredHandler(event: { message: { name: string; originator: number } }) { +export function operationRegisteredHandler(event: { + message?: { name?: string; grantable?: boolean; originator?: number }; +}) { if (!isMainThread) return; - const { name, originator } = event.message; + const { name, grantable, originator } = event?.message ?? {}; if (typeof name !== 'string' || typeof originator !== 'number') return; + // An announcement can lose the race with its own thread's exit, and exit notification fires once + // per thread, so without this the entry would never be cleaned up. Reads manageThreads' tombstone + // rather than keeping a second one: job threads are one-shot workers, so a duplicate here would + // grow per completed job, not per worker restart. + if (hasThreadExited(originator)) { + operationLog.debug(`Ignoring operation '${name}' announced by exited worker thread ${originator}`); + return; + } let workerIds = registeredByWorker.get(name); if (!workerIds) registeredByWorker.set(name, (workerIds = new Set())); workerIds.add(originator); + // Mirroring only widens what an allowlist may name; enforcement stays on the worker's + // chooseOperation. A re-announcement that drops the permission retracts this thread's claim. + setWorkerGrantable(name, originator, grantable === true); operationLog.debug(`Registered operation '${name}' announced by worker thread ${originator}`); } +/** + * A worker that dies mid-execution can never respond, so fail its in-flight forwards rather than + * waiting out the timeout, and forget its registrations (a replacement re-registers on load). + */ +function handleThreadExit(deadThreadId: number) { + for (const [name, workerIds] of registeredByWorker) { + if (!workerIds.delete(deadThreadId)) continue; + // A surviving worker that never declared a permission must not keep the name admissible. + if (workerIds.size === 0) dropRegistration(name); + else setWorkerGrantable(name, deadThreadId, false); + } + for (const [requestId, pending] of pendingExecutions) { + if (pending.targetThreadId === deadThreadId) { + pendingExecutions.delete(requestId); + pending.reject(new ServerError('The worker thread executing this operation exited', 503)); + } + } +} + +function setWorkerGrantable(name: string, threadId: number, grantable: boolean) { + let grantableIds = grantableByWorker.get(name); + if (grantable) { + if (!grantableIds) grantableByWorker.set(name, (grantableIds = new Set())); + grantableIds.add(threadId); + registerWorkerGrantableOperation(name); + } else if (grantableIds?.delete(threadId) && grantableIds.size === 0) { + grantableByWorker.delete(name); + unregisterWorkerGrantableOperation(name); + } +} + +/** + * Forget an operation no live worker offers any more. Both prune paths — thread exit, and a failed + * send discovering a dead port — route through here so a name can never keep a route without an + * owner. Grantability is dropped per owner instead, in `setWorkerGrantable`. + */ +function dropRegistration(name: string) { + registeredByWorker.delete(name); + grantableByWorker.delete(name); + unregisterWorkerGrantableOperation(name); +} + let rotation = 0; /** * Main-thread dispatch fallback: if a worker registered `name`, return a forwarding operation @@ -122,23 +184,15 @@ function attachMainListeners() { if (error) pending.reject(new ServerError(error.message, error.statusCode || 500)); else pending.resolve(result); }); - // A worker that dies mid-execution can never respond; fail its in-flight forwards rather - // than waiting out the timeout, and forget its registrations (a replacement worker - // re-registers on component load). - onThreadExit((deadThreadId: number) => { - for (const [name, workerIds] of registeredByWorker) { - workerIds.delete(deadThreadId); - if (workerIds.size === 0) registeredByWorker.delete(name); - } - for (const [requestId, pending] of pendingExecutions) { - if (pending.targetThreadId === deadThreadId) { - pendingExecutions.delete(requestId); - pending.reject(new ServerError('The worker thread executing this operation exited', 503)); - } - } - }); + onThreadExit(handleThreadExit); } +// Armed at load rather than on first use: serverUtilities imports this module during its own load, +// before any worker exists. Thread-exit notification fires once per thread and is dropped outright +// if no listener is attached yet, so a worker dying before its first announcement is processed +// would otherwise leave a registration nothing could ever clean up. +if (isMainThread) attachMainListeners(); + async function executeRemoteOperation(name: string, body: any, bypassAuth: boolean): Promise { attachMainListeners(); const workerIds = registeredByWorker.get(name); @@ -168,7 +222,10 @@ async function executeRemoteOperation(name: string, body: any, bypassAuth: boole ); } if (!sent) { + // The port is gone, so this thread's claims go with it — grantability included, which + // handleThreadExit would otherwise not retract while other workers still route the name. workerIds.delete(targetThreadId); + setWorkerGrantable(name, targetThreadId, false); continue; } return new Promise((promiseResolve, promiseReject) => { @@ -190,7 +247,7 @@ async function executeRemoteOperation(name: string, body: any, bypassAuth: boole }); }); } - if (registeredByWorker.get(name)?.size === 0) registeredByWorker.delete(name); + if (registeredByWorker.get(name)?.size === 0) dropRegistration(name); throw new ServerError( `Operation '${name}' is registered by a component but no worker thread is available to run it`, 503 diff --git a/server/serverHelpers/serverUtilities.ts b/server/serverHelpers/serverUtilities.ts index c98d82b740..e7c06cc148 100644 --- a/server/serverHelpers/serverUtilities.ts +++ b/server/serverHelpers/serverUtilities.ts @@ -186,6 +186,10 @@ export type OperationDefinition = { requiresSuperUser?: boolean; }; +// Operation names this API installed a permission entry for, so a later registration that drops +// `requiresSuperUser` can retract exactly that entry and nothing else. +const declaredPermissionNames = new Set(); + /** * Register an operation function with the server. * @param operationDefinition @@ -195,7 +199,14 @@ server.registerOperation = (operationDefinition: OperationDefinition) => { if (isDeployValidating()) return; const { name, execute, requiresSuperUser } = operationDefinition; let handler = execute; - if (requiresSuperUser !== undefined) { + if (requiresSuperUser === undefined) { + // A re-registration that drops the flag must also drop the entry the earlier one installed, or + // declaration and enforcement disagree: main retracts the grantable mark while this worker keeps + // honouring an already-persisted role grant. Scoped to names declared through this API, so one it + // never declared is untouched — a component that declared a built-in's name already overwrote + // that entry by declaring it, and this only follows. + if (declaredPermissionNames.delete(name)) opAuth.unregisterOperationPermission(name); + } else { // verifyPerms keys requiredPermissions by the handler's function `.name`, but registered ops // are typically anonymous arrows (all named "execute") which collide and can't be keyed. Wrap // in a FRESH function named after the op so the lookup resolves the right entry. Wrap rather @@ -205,12 +216,14 @@ server.registerOperation = (operationDefinition: OperationDefinition) => { handler = (...args: any[]) => (execute as any)(...args); Object.defineProperty(handler, 'name', { value: name, configurable: true }); opAuth.registerOperationPermission(name, { requiresSu: requiresSuperUser }); + declaredPermissionNames.add(name); } OPERATION_FUNCTION_MAP.set(name as any, new OperationFunctionObject(handler)); // Components load per-worker, so a registration made there is invisible to the main-thread // ops-API dispatcher (each thread has its own OPERATION_FUNCTION_MAP instance). Announce it - // so the main thread can forward calls to this worker (#1736). - if (!isMainThread) announceRegisteredOperation(name); + // so the main thread can forward calls here (#1736), and can mirror the role-allowlist mark that + // registerOperationPermission above made only in this thread's scope. + if (!isMainThread) announceRegisteredOperation(name, requiresSuperUser !== undefined); }; // Register the durable MCP quota policy as a function (see components/mcp/quota.ts). Worker-local, diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 4581f067d1..9182242eae 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -152,6 +152,8 @@ module.exports = { restoreShutdownDeadline, registerWorkerDataProvider, onThreadExit, + hasThreadExited, + notifyThreadExit, registerProcessGroup, unregisterProcessGroup, isThreadRunning, @@ -1119,6 +1121,15 @@ if (isMainThread) { }); } +/** + * Whether a thread has already been reported dead. Sync, unlike `isThreadRunning`, because it only + * reads the tombstone `notifyThreadExit` records below — callers on the exit path need an answer + * without awaiting process-group confirmation. + */ +function hasThreadExited(threadId) { + return notifiedDeadThreadIds.has(threadId); +} + function notifyThreadExit(deadThreadId) { if (deadThreadId == null || notifiedDeadThreadIds.has(deadThreadId)) return; notifiedDeadThreadIds.add(deadThreadId); diff --git a/unitTests/security/authn/oidc/trustPolicyOperations.test.js b/unitTests/security/authn/oidc/trustPolicyOperations.test.js index a1542365b5..ec5e7bddf2 100644 --- a/unitTests/security/authn/oidc/trustPolicyOperations.test.js +++ b/unitTests/security/authn/oidc/trustPolicyOperations.test.js @@ -264,15 +264,10 @@ describe('oidc trustPolicyOperations', () => { assert.strictEqual(installed.mock.rows.size, 0, 'expected nothing stored'); }); - // Delegating to validateOperations rather than checking OPERATIONS_ENUM locally means an - // operation registered in THIS process is accepted. That is the seam, not a promise about - // component-registered operations: the registry is process-local, `add_oidc_trust` runs on the - // main thread, and `server.registerOperation` runs in a worker whose OPERATION_REGISTERED - // announcement carries only name→thread routing, never grantability. So a component's - // operation is NOT recognized here in production, and this same-thread test cannot show that - // — it is asserting the delegation, not the topology. A policy naming such an operation is - // rejected, which fails closed; add_role, alter_role, and impersonation validation share the - // gap, so the fix belongs to that bridge rather than to a workaround here. + // Asserts the delegation to validateOperations, not the worker→main topology: registering here + // puts the mark in this thread's own registry, so a same-thread test cannot distinguish the two. + // The cross-thread path a real component takes is covered in + // integrationTests/components/registered-operation.test.ts. it('accepts an operation registered in this process', async () => { const dynamicOp = 'test_dynamic_scope_op'; opAuth.registerOperationPermission(dynamicOp, { requiresSu: true }); diff --git a/unitTests/server/serverHelpers/serverUtilities.test.js b/unitTests/server/serverHelpers/serverUtilities.test.js index 5a6a4b4007..95bb2dc1ae 100644 --- a/unitTests/server/serverHelpers/serverUtilities.test.js +++ b/unitTests/server/serverHelpers/serverUtilities.test.js @@ -9,6 +9,7 @@ const sandbox = sinon.createSandbox(); const { TEST_JSON_SUPER_USER, TEST_JSON_NON_SU } = require('../../test_data'); const serverUtilities = require('#src/server/serverHelpers/serverUtilities'); const registeredOperations = require('#src/server/serverHelpers/registeredOperations'); +const manageThreads = require('#src/server/threads/manageThreads'); const operationAuthorizationState = require('#src/server/serverHelpers/operationAuthorizationState'); const { runWithDeployValidationGuard } = require('#src/server/serverHelpers/deployValidationState'); const quota = require('#src/components/mcp/quota'); @@ -221,6 +222,143 @@ describe('Test serverUtilities.js module ', () => { }); }); + // Only the receiving half is reachable here — announceRegisteredOperation returns early on the + // main thread, so integrationTests/components/registered-operation.test.ts owns the real hop. + describe('cross-thread grantable operation mirroring', function () { + const { + validateOperations, + registerGrantableOperation, + unregisterGrantableOperation, + unregisterWorkerGrantableOperation, + } = require('#src/utility/operationPermissions'); + const GRANTABLE = 'test_cross_thread_grantable_op'; + const PLAIN = 'test_cross_thread_plain_op'; + const SHARED = 'test_cross_thread_shared_op'; + const ROLLED = 'test_cross_thread_rolled_op'; + const RETRACTED = 'test_cross_thread_retracted_op'; + const ZOMBIE = 'test_cross_thread_zombie_op'; + const FAILED_SEND = 'test_cross_thread_failed_send_op'; + // Thread-exit tombstones are permanent and process-global, so synthetic ids must be ones the + // runtime will never assign to a real worker. + const DECLARING_THREAD = 9_000_061; + const ROUTING_THREAD = 9_000_062; + const DEAD_THREAD = 9_000_071; + const SENDER_THREAD = 9_000_081; + + after(function () { + for (const op of [GRANTABLE, PLAIN, SHARED, ROLLED, RETRACTED, ZOMBIE, FAILED_SEND]) { + unregisterWorkerGrantableOperation(op); + unregisterGrantableOperation(op); + } + }); + + it('makes a worker-announced declared op grantable on the main thread', function () { + assert.notEqual(validateOperations([GRANTABLE]), null); + + registeredOperations.operationRegisteredHandler({ + message: { name: GRANTABLE, grantable: true, originator: 31 }, + }); + + assert.equal(validateOperations([GRANTABLE]), null, 'name should be grantable after the announcement'); + }); + + it('leaves an op that declared no permission ungrantable', function () { + registeredOperations.operationRegisteredHandler({ + message: { name: PLAIN, grantable: false, originator: 31 }, + }); + + assert.notEqual(validateOperations([PLAIN]), null); + assert.equal(typeof registeredOperations.getRemoteOperationFunction(PLAIN), 'function'); + }); + + it('keeps a main-thread registration of the same name independent of the worker mirror', function () { + // restartWorkers loads root components before draining old workers, so a startOnMainThread + // component can claim a name a retiring worker still offers. + registeredOperations.operationRegisteredHandler({ + message: { name: SHARED, grantable: true, originator: 41 }, + }); + registerGrantableOperation(SHARED); + + unregisterWorkerGrantableOperation(SHARED); + assert.equal(validateOperations([SHARED]), null, 'main-thread registration should survive worker revocation'); + + unregisterGrantableOperation(SHARED); + assert.notEqual(validateOperations([SHARED]), null, 'both marks gone should make it ungrantable again'); + }); + + it('retracts grantability when a re-announcement drops the declared permission', function () { + registeredOperations.operationRegisteredHandler({ + message: { name: RETRACTED, grantable: true, originator: 51 }, + }); + assert.equal(validateOperations([RETRACTED]), null); + + registeredOperations.operationRegisteredHandler({ + message: { name: RETRACTED, grantable: false, originator: 51 }, + }); + assert.notEqual(validateOperations([RETRACTED]), null, 'the same thread withdrawing must retract its claim'); + }); + + it('stops being grantable once the last declaring worker is gone, even while another still routes it', function () { + registeredOperations.operationRegisteredHandler({ + message: { name: ROLLED, grantable: true, originator: DECLARING_THREAD }, + }); + registeredOperations.operationRegisteredHandler({ + message: { name: ROLLED, grantable: false, originator: ROUTING_THREAD }, + }); + assert.equal(validateOperations([ROLLED]), null, 'still declared by the first thread'); + + manageThreads.notifyThreadExit(DECLARING_THREAD); + + assert.notEqual(validateOperations([ROLLED]), null, 'no live worker declares it grantable any more'); + assert.equal( + typeof registeredOperations.getRemoteOperationFunction(ROLLED), + 'function', + 'the surviving worker still routes it' + ); + }); + + it('retracts grantability when a failed send prunes the declaring worker', async function () { + const originalThreads = global.threads; + global.threads = { + sendToThread() { + return false; + }, + }; + try { + registeredOperations.operationRegisteredHandler({ + message: { name: FAILED_SEND, grantable: true, originator: SENDER_THREAD }, + }); + assert.equal(validateOperations([FAILED_SEND]), null); + + const forward = registeredOperations.getRemoteOperationFunction(FAILED_SEND, true); + await assert.rejects(forward({ operation: FAILED_SEND }), /no worker thread is available/); + + assert.notEqual( + validateOperations([FAILED_SEND]), + null, + 'a dead port must retract the claim, not just the route' + ); + } finally { + global.threads = originalThreads; + } + }); + + it('ignores an announcement that lost a race with its own thread exit', function () { + manageThreads.notifyThreadExit(DEAD_THREAD); + + registeredOperations.operationRegisteredHandler({ + message: { name: ZOMBIE, grantable: true, originator: DEAD_THREAD }, + }); + + assert.notEqual(validateOperations([ZOMBIE]), null, 'a dead thread must not install a grant'); + assert.equal( + registeredOperations.getRemoteOperationFunction(ZOMBIE), + undefined, + 'nor a route nothing will ever clean up' + ); + }); + }); + describe('operation authorization state', function () { it('is scoped across awaits and restores nested authorization', async function () { assert.strictEqual(operationAuthorizationState.isOperationAuthorizationBypassed(), false); @@ -800,7 +938,15 @@ describe('Test serverUtilities.js module ', () => { // Keep the process-global registries clean — these test-only ops shouldn't leak into other // suites. registerOperation touches three globals (the op-function map plus verifyPerms' // requiredPermissions and the grantable-ops set), so undo all three, not just the map. - for (const op of [SU_OP, OPEN_OP, 'test_name_pinning_op', 'shared_op_a', 'shared_op_b', 'dyn_grantable_op']) { + for (const op of [ + SU_OP, + OPEN_OP, + 'test_name_pinning_op', + 'shared_op_a', + 'shared_op_b', + 'dyn_grantable_op', + 'test_redeclared_op', + ]) { serverUtilities.OPERATION_FUNCTION_MAP.delete(op); op_auth.unregisterOperationPermission(op); } @@ -835,6 +981,50 @@ describe('Test serverUtilities.js module ', () => { assert.equal(validateOperations(['dyn_grantable_op']), null); // grantable after registration }); + it('retracts the permission entry when a re-registration drops requiresSuperUser', function () { + // Declaration and enforcement must not disagree: main retracts the grantable mark on the + // re-announcement, so a role grant persisted while the op was declared must stop being + // honoured here too. Named after the op so verifyPerms resolves the stale entry if it + // survives — an anonymous handler would mask the bug rather than test it. + const op = 'test_redeclared_op'; + // eslint-disable-next-line func-names + const named = { + [op]: async function () { + return {}; + }, + }[op]; + server.registerOperation({ name: op, execute: named, requiresSuperUser: true }); + assert.equal(validateOperations([op]), null, 'grantable while declared'); + assert.equal(op_auth.verifyPerms(nonSuRequest(op, [op]), op), null, 'granted role may call it'); + + server.registerOperation({ name: op, execute: named }); + + assert.notEqual(validateOperations([op]), null, 'no longer grantable once undeclared'); + let threw; + try { + op_auth.verifyPerms(nonSuRequest(op, [op]), op); + } catch (err) { + threw = err; + } + assert.ok(threw, 'a persisted grant must not survive the declaration being dropped'); + assert.equal(threw.statusCode, 400); + }); + + it('does not strip a permission entry this API never registered', function () { + // Ownership protection, exercised against an independent registrant rather than a real + // built-in so the assertion does not depend on mutating shared dispatch state. + const op = 'test_independent_registrant_op'; + op_auth.registerOperationPermission(op, { requiresSu: true }); + try { + server.registerOperation({ name: op, execute: async () => ({}) }); + assert.equal(validateOperations([op]), null, 'an entry this API did not create must survive'); + assert.ok(op_auth.verifyPerms(nonSuRequest(op), op), 'and must still gate a non-super_user'); + } finally { + op_auth.unregisterOperationPermission(op); + serverUtilities.OPERATION_FUNCTION_MAP.delete(op); + } + }); + it('does not touch handler name or register perms when requiresSuperUser is omitted (opt-in)', function () { const def = { name: OPEN_OP, execute: async () => ({}) }; server.registerOperation(def); diff --git a/utility/operationPermissions.ts b/utility/operationPermissions.ts index 3ad4d0fecf..cb0ae6785c 100644 --- a/utility/operationPermissions.ts +++ b/utility/operationPermissions.ts @@ -97,6 +97,11 @@ const validGroups: Set = new Set(Object.keys(OPERATION_PERMISSION_GROUPS // permission) whose names may fall outside OPERATIONS_ENUM. Tracked so they're grantable in a // role's `operations` allowlist (add_role/alter_role/impersonation validate against this set too). const dynamicallyRegisteredOps: Set = new Set(); +// The same, for ops registered on a WORKER and mirrored here by the OPERATION_REGISTERED bridge. +// Deliberately a separate set: the two threads can register the same name independently (a hot +// deploy can add a `startOnMainThread` component while the worker offering that name is retiring), +// so worker-lifecycle cleanup must not be able to revoke a mark this thread made itself. +const workerRegisteredOps: Set = new Set(); /** * Mark a dynamically-registered operation name as a valid target for role `operations` grants. @@ -115,13 +120,24 @@ export function unregisterGrantableOperation(name: string): void { dynamicallyRegisteredOps.delete(name); } +export function registerWorkerGrantableOperation(name: string): void { + workerRegisteredOps.add(name); +} + +export function unregisterWorkerGrantableOperation(name: string): void { + workerRegisteredOps.delete(name); +} + /** * Validates that every entry in an operations array is a known operation name or group name. * Returns the first invalid entry, or null if all entries are valid. */ export function validateOperations(operations: readonly unknown[]): string | null { for (const op of operations) { - if (typeof op !== 'string' || (!validOps.has(op) && !validGroups.has(op) && !dynamicallyRegisteredOps.has(op))) { + if ( + typeof op !== 'string' || + (!validOps.has(op) && !validGroups.has(op) && !dynamicallyRegisteredOps.has(op) && !workerRegisteredOps.has(op)) + ) { return String(op); } }