Allow a component-registered operation to be granted in a role's operations allowlist - #2260
Draft
dawsontoth wants to merge 7 commits into
Draft
Allow a component-registered operation to be granted in a role's operations allowlist#2260dawsontoth wants to merge 7 commits into
dawsontoth wants to merge 7 commits into
Conversation
`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>
Contributor
There was a problem hiding this comment.
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.
This comment has been minimized.
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An operation a component registers with
requiresSuperUseris meant to be grantable in a role'soperationsallowlist, but naming it inadd_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
registerOperationbridge (#1736) rather than introduced here — the announcement is fire-and-forget, so anadd_rolenaming 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:requiresSuperUser, the routing set stays non-empty via the new workers, so the mark is never revoked —add_roleaccepts 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: thatsetWorkerGrantablere-derives on every path, including retraction, and thathandleThreadExitretracts one thread's claim while preserving routing for survivors.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.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 ignoresgrantableByWorkerdeliberately: selecting a worker by authorization metadata would require the main thread to know why a caller is authorized (super_user vs. anoperationsgrant), 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.grantableis derived asrequiresSuperUser !== undefinedrather than being a new field onOperationDefinition. It reuses the existing tri-state that already decides whetherregisterOperationPermissionis 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 treatsrequiresSuperUser: falseas 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.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_serviceproduces 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.yamlstill cannot grant an operation its ownresources.jsregisters, becauseDEFAULT_CONFIGorders therolesplugin beforejsResourceandcomponentLoader.ts:542iterates 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
mainbranch, so there is no companion docs PR to open yet: theKnown limitationcomment onassertOperationsAreKnowninsecurity/authn/oidc/trustPolicyOperations.tson #2173's branch, and the "that registry is process-local, so a policy naming one is rejected" paragraph inreference/operations-api/operations.mdon an unpushedHarperFast/documentationbranch. Whichever of the two PRs lands second should drop both. Nothing currently on documentationmainis made false by this change.Verification
Route (a), extended existing integration test —
integrationTests/components/registered-operation.test.tsgains six cases in a nested suite coveringadd_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.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 (sendToThreadreporting 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. Alsonpx 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, andorigin/mainhad touched none of these 7 files.Not run locally:
test:unit:mainandtest:unit:resourcesabort at load on this machine — another process holds~/harper/database/data/LOCK, andHDB_ROOT/ROOTPATHoverrides 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 against127.0.0.1. Both are left to this PR's Actions run.npm run lint:requiredclean;npm run format:writeproduced no changes;npm run buildclean; TypeStrip/tabs/node:-prefix/assert-style greps clean overorigin/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