Skip to content

Allow a component-registered operation to be granted in a role's operations allowlist - #2260

Draft
dawsontoth wants to merge 7 commits into
mainfrom
fix/grantable-component-ops-cross-thread
Draft

Allow a component-registered operation to be granted in a role's operations allowlist#2260
dawsontoth wants to merge 7 commits into
mainfrom
fix/grantable-component-ops-cross-thread

Conversation

@dawsontoth

@dawsontoth dawsontoth commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

An operation a component registers with requiresSuperUser is meant to be grantable in a role's operations allowlist, but naming it in add_role, alter_role, or an impersonation payload was rejected as "not a valid operation name or group" — the grantable mark was made in the worker that registered the operation, while the validation that reads it runs on the main thread. The registration announcement now carries that fact across the thread boundary, so a scoped role can actually be granted a component operation.

Enforcement is unchanged: the main thread only decides whether a name is admissible in an allowlist, and the operation still runs through the registering worker's own chooseOperation.

One finding is disclosed rather than fixed, and is inherited from the registerOperation bridge (#1736) rather than introduced here — the announcement is fire-and-forget, so an add_role naming a component operation can still lose a race against component load at startup. It fails closed (a rejected role), and the existing execution path has the same window.

For the human reviewer

The step-6 framing gate did not clear. The planning review returned Framing-Verdict: better-alternative-exists, and it was right — I adopted its recommendation rather than defending mine, which is why the history carries superseded approaches (squash on merge). The entries below lead with the parts I got wrong, because those are the ones worth your scepticism:

  1. The approach changed after review, and the earlier one was genuinely broken. I first mirrored grantability as a set of names. That cannot enforce the invariant the design claimed ("admissible iff a live worker declares it grantable"): on a rolling deploy whose new generation keeps an operation but drops requiresSuperUser, the routing set stays non-empty via the new workers, so the mark is never revoked — add_role accepts a grant whose execution then fails closed with operation-not-found. Grantability is now tracked per declaring thread (name -> Set<threadId>) and the mirrored mark is re-derived from live claims. What to check: that setWorkerGrantable re-derives on every path, including retraction, and that handleThreadExit retracts one thread's claim while preserving routing for survivors.
  2. My "main-mediated registration" rejection was overstated, and I did not adopt the correction. I disqualified it as requiring server.registerOperation() to become async. The reviewer pointed out that the loader could instead collect synchronous registrations and await one batched acknowledgement before declaring the worker ready — no API change, and it would also close the fire-and-forget startup window disclosed above. I did not do it: it introduces a worker-readiness protocol in the component loader, which is a materially larger change than the one this bug needs. This is the entry most worth overruling. If you want the startup window closed rather than documented, that is the design to ask for, and this PR is the wrong shape for it.
  3. A mixed worker generation can make a granted call succeed intermittently, and I declined to fix it. If the old generation declares an operation grantable and the replacement omits requiresSuperUser, a role grant is accepted while both generations are live, and calls then alternate between succeeding on the declaring worker and returning a permission error on the other. Routing ignores grantableByWorker deliberately: selecting a worker by authorization metadata would require the main thread to know why a caller is authorized (super_user vs. an operations grant), which is the authorization determination this module documents that main must not make — server.registerOperation() calls in a component's resources.js are unreachable via the ops API #1736 split it as main routes, worker enforces. It fails closed (a permission error, never a wrongful success) and it is transient: once the old generation is gone this change retracts grantability and the grant is correctly rejected at write time. If you would rather routing became authorization-aware, that is a design call, and this is where to make it.
  4. grantable is derived as requiresSuperUser !== undefined rather than being a new field on OperationDefinition. It reuses the existing tri-state that already decides whether registerOperationPermission is called at all, so the two cannot drift; the cost is that the wire flag's meaning is implicit in that expression. Note this deliberately treats requiresSuperUser: false as grantable, which matches the tri-state's documented meaning ("grantable AND open to any authenticated user") and keeps worker-registered operations behaving like main-thread ones.
  5. Arming the thread-exit cleanup at module load widens this PR past the stated bug. attachMainListeners() previously ran only on the first forwarded call. Thread-exit notification fires once per thread and is dropped when no listener is attached, so a worker that registered and died before that first call leaked its entries permanently. That is a pre-existing bug in the routing map — I fixed it here because the new mark would otherwise inherit it, but it is separable if you would rather see it on its own.

Also flagged by the planning review and not addressed: it recommended lifecycle tests driven through a real hot restart/deploy, and integration coverage of a mixed old/new generation. The new unit tests drive the exit seam directly instead, so they prove the state machine but not that a real restart_service produces those transitions.

Adjacent bug found while checking symmetry, filed rather than fixed here: #2203 (P2, under [Epic] Component authoring & packaging DX) — a component's own roles.yaml still cannot grant an operation its own resources.js registers, because DEFAULT_CONFIG orders the roles plugin before jsResource and componentLoader.ts:542 iterates config keys in order. Same-thread ordering, different mechanism, survives this fix.

This invalidates documentation of the limitation in two places that are still in flight, neither on a main branch, so there is no companion docs PR to open yet: the Known limitation comment on assertOperationsAreKnown in security/authn/oidc/trustPolicyOperations.ts on #2173's branch, and the "that registry is process-local, so a policy naming one is rejected" paragraph in reference/operations-api/operations.md on an unpushed HarperFast/documentation branch. Whichever of the two PRs lands second should drop both. Nothing currently on documentation main is made false by this change.

Verification

Route (a), extended existing integration testintegrationTests/components/registered-operation.test.ts gains six cases in a nested suite covering add_role, alter_role, impersonation, an end-to-end grant (a non-super_user with the grant executes the operation on a worker), and two negative controls: an unregistered name is still rejected, and a non-super_user without the grant still gets 403. No new CI entry.

HARPER_INTEGRATION_TEST_LOOPBACK_POOL_START=1 HARPER_INTEGRATION_TEST_LOOPBACK_POOL_COUNT=1 \
  npm run test:integration -- --isolation=none integrationTests/components/registered-operation.test.ts
→ 11/11 pass (5 pre-existing + 6 new)

Unit — seven new cases in unitTests/server/serverHelpers/serverUtilities.test.js, covering the rolling-deploy retraction, withdrawal by re-announcement, the exited-thread announcement guard, main/worker mark independence, and the failed-send retraction through its real trigger (sendToThread reporting a dead port). npx mocha "unitTests/server/serverHelpers/*.test.js" → 295 passing, 1 failing. That failure is pre-existing (uwsServer.test.js, "rejects a body over maxBodyBytes with 413"); confirmed by stashing this branch's changes and re-running. Run as the whole suite rather than the changed file alone, because these tests mutate the process-global grantable registries. Also npx mocha unitTests/utility/operationPermissions.test.js unitTests/security/impersonation.test.js unitTests/validation/role_validation.test.js → 133 passing, 0 failing.

fails-on-base — through a detached worktree at the merge-base with origin/main, with only the test changes applied: the new unit cases fail there and reach their intended assertions rather than a setup or compile error; the 4 positive integration cases fail there while both negative controls pass. Everything passes on the branch.

Rebased onto current origin/main (374408e) before review — the branch had fallen 67 commits behind. No conflicts, and origin/main had touched none of these 7 files.

Not run locally: test:unit:main and test:unit:resources abort at load on this machine — another process holds ~/harper/database/data/LOCK, and HDB_ROOT/ROOTPATH overrides do not reroute the path that opens it. Worth knowing separately: mocha exits 0 when it dies this way, so an exit code is not evidence those suites ran. test:integration:all (160 files) was not run locally either: without the loopback address pool configured it can only run sequentially against 127.0.0.1. Both are left to this PR's Actions run.

npm run lint:required clean; npm run format:write produced no changes; npm run build clean; TypeStrip/tabs/node:-prefix/assert-style greps clean over origin/main...HEAD.

Complexity: complicated

Review-Coverage: authored=claude; ran=codex; blocked=gemini(quota); declined=cursor-grok,cursor-composer,domain; rounds=4 @ 2169d46

Human-Review-Need: 3 @ 2169d46

dawsontoth and others added 6 commits August 21, 2026 11:43
`server.registerOperation({ requiresSuperUser })` marks an operation
grantable in a role's `operations` allowlist, but that mark landed only in
the worker that registered it (components load per-worker). Meanwhile
`validateOperations` is consulted on the main thread — by add_role and
alter_role, by impersonation payload validation, and by OIDC trust
policies — so naming a component-registered operation in any of those was
rejected as "not a valid operation name or group" even though the
operation existed and was designed to be grantable.

The OPERATION_REGISTERED announcement already crosses that boundary for
execution routing, so carry grantability on it too and mirror the mark on
main. This only widens what an allowlist may name; enforcement is
unchanged, still running on the worker's own `chooseOperation`.

Also arm the thread-exit cleanup when the registry gains its first entry
rather than on the first forwarded call, so a worker that registers and
exits without ever being called no longer leaks its entries, and revoke
the mirrored mark when the last registering worker is gone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claim a mirrored name for thread-exit cleanup only when the mirror is what
made it admissible. As written, the ownership set was unconditional, so a
name that main had already registered itself — or an enum/group name — was
revoked when the last worker offering the same name exited, which is the
opposite of what the set exists to prevent and of what its comment claimed.
Flagged independently by both review lenses.

Route both prune paths through one `dropRegistration`: the failed-send path
in `executeRemoteOperation` dropped the routing entry without revoking the
mark, so routing and grantability could disagree about whether an op was
still offered.

Also cut the comments the review flagged as narration or as duplicating the
note now carried in server/DESIGN.md.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ownership probe added in the previous commit only protected marks that
predated a worker's announcement, which leaves a reachable hole: on a hot
deploy `restartWorkers` awaits `loadRootComponents()` before it begins
draining the old workers (`server/threads/manageThreads.js`), so a
`startOnMainThread` component can register an operation the retiring worker
also offered. The worker's exit then revoked the main thread's own mark and
role validation started rejecting an operation that was registered and
executable.

Track mirrored names in a separate set that `validateOperations` unions
instead of sharing one. The two threads can now register the same name
independently and neither can revoke the other, which removes the ownership
question rather than narrowing it — the probe and its bookkeeping set are
gone. Found by the round-2 cross-model review, which also supplied this
approach.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The planning review returned better-alternative-exists on the previous
approach, and it was right: a name-level mirror set cannot express which
worker declared the operation grantable, so it did not enforce the invariant
the design claimed ("admissible iff a live worker declares it grantable").

Concretely, a rolling deploy whose new generation keeps an operation but drops
`requiresSuperUser` left the name admissible with no live declarer: the
routing set stayed non-empty via the new workers, so the mark was never
revoked, and `add_role` accepted a grant whose execution then failed closed
with operation-not-found.

Track claims as name -> Set<declaring threadId> and re-derive the mirrored
mark from live claims, so grantability is retracted when a thread withdraws it
or exits even while other workers keep routing the name. Also ignore an
announcement from a thread already reported dead: exit notification is
deduplicated for the process lifetime, so such an entry could never be
cleaned up afterwards.

Extract the thread-exit cleanup and export a test seam for it, which is what
finally lets the revocation paths be tested at all.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two real gaps found by the gemini and cursor-composer legs, corroborating
each other on the second one.

Arm the main-thread listeners at module load instead of on first use.
`attachMainListeners` ran lazily from the registration handler, but
thread-exit notification fires once per thread and is dropped outright when no
listener is attached yet — so a worker that died before its first
announcement was processed left a registration nothing could ever clean up,
and the exited-thread guard never learned about it. serverUtilities imports
this module during its own load, before any worker exists, so arming at load
is well ordered.

Retract grantability when a failed send prunes a dead originator.
`executeRemoteOperation` dropped the routing entry but left the claim, so a
dead worker could keep a name admissible while a surviving worker that never
declared a permission kept routing it — the same false-admissible case this
change exists to close.

Also guard the ITC payload destructure. A malformed OPERATION_REGISTERED with
no `message` would have thrown on the main thread; the envelope is trusted and
in-process, but three review rounds have now flagged it and the guard is one
expression.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Synthetic thread ids in the new tests were small positive integers inserted
permanently into the module-global tombstone set, which the `after` hook
cannot clear — a later suite starting a real worker in the same process could
have been assigned one of those ids and had its legitimate announcement
ignored. Use ids the runtime will never assign.

Cover the failed-send retraction through its production trigger rather than
only the exit seam: a forward whose `sendToThread` reports a dead port must
retract the claim, not just the route.

Also drop comments the review flagged as narrating the line beneath them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements cross-thread mirroring of grantable operations from worker threads to the main thread, allowing the main thread to validate worker-registered operations in role allowlists. The review feedback suggests two performance optimizations in server/serverHelpers/registeredOperations.ts: first, to only perform cleanup logic during thread exit if the exiting thread actually registered the operation, and second, to avoid redundant Map/Set deletions in setWorkerGrantable when removing a thread's grantable claim.

Comment thread server/serverHelpers/registeredOperations.ts
Comment thread server/serverHelpers/registeredOperations.ts
@claude

This comment has been minimized.

Both suggestions from the gemini review, and both behaviour-preserving: a
grantability claim implies a registration, since claims are only recorded
alongside one, so an operation the dead thread was not registered for can have
no claim to retract either.

`handleThreadExit` now continues when the id was not in the routing set, and
`setWorkerGrantable` only re-derives the mirrored mark when a claim was
actually removed, instead of unregistering a name it never held.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant