fix(local-node-host): make the forms surface unavailable to the web plane - #3374
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
thought (non-blocking): Accessibility audit (advisory)The sharded axe audit is report-only while the baseline and runtime budget mature.
Shard 1 reportShard 2 reportShard 3 report |
|
Field-day triage 2026-07-31 — left open, not closed. It is a DRAFT, 29 commits behind, and its card #3367 is still open, so unlike the mid-July drafts I closed today this is live work rather than backlog wearing a PR's clothes. Two things worth knowing before resuming it:
Related open follow-up from today's reviews: #3466 (audit the 15 existing web-session consumers against the three effect-time MUSTs) — relevant if this PR touches that surface. |
This is DIRTY against main, and the conflict is semantic — it needs your call, not a mechanical rebaseI attempted the rebase (its last commit is What changed underneath this PR
Why this PR is NOT obsoleteMain's own comment is explicit that it fixed attribution only:
#3367's deputy is about authorization, and this PR's own experiment table is the evidence — a So the fence still has a job: if the surface cannot authorize a member correctly, do not serve it to What the resolution actually isKeep main's per-request identity (drop the FormsRoutes.Map(desktopPlaneOnly, _engine, _issuer, _verifier, _activeTeam, roles, _timeProvider);
FormDefinitionRoutes.Map(desktopPlaneOnly, _definitionStore, _schemaRegistry, _activeTeam, _timeProvider);The doc comment needs to carry both facts rather than either alone: identity is resolved per request Why I did not just do itTwo reasons. It is a merge on an authorization fence, where "kept both halves" and "kept the wrong If the answer is "yes, still fence", the resolution above is what I would apply, and it should draw a |
CIC ruling: keep the fence, rebase itDecided after the conflict analysis above. The fence stays. Reasoning, recorded so the rebase is not re-litigated: #3437 fixed attribution and main's own comment The resolution to applyKeep main's per-request identity — drop the FormsRoutes.Map(desktopPlaneOnly, _engine, _issuer, _verifier, _activeTeam, roles, _timeProvider);
FormDefinitionRoutes.Map(desktopPlaneOnly, _definitionStore, _schemaRegistry, _activeTeam, _timeProvider);The class doc must carry both facts rather than either alone: identity is resolved per request, and Two things for whoever does it. This is a merge on an authorization fence, where "kept both halves" |
d7621d5 to
7e2fe20
Compare
Rebased onto main per the ruling —
|
github-actions
Bot
added
the
needs-deep-review
Deep review: APPROVE-WITH-FOLLOWUPS — all three
|
ctwoodwa
added
the
deep-review-passed
1a9b1f4 to
06682b4
Compare
…lane HostedFormsApiEndpoint captures the operator's subject, role list and definition owner ONCE at startup and mints a per-request form capability from them. Nothing on that path resolves an IAuthorizationContext, so the web-plane fence installed at that seam cannot see the family: a seam-gate reaches code that ASKS an authorization question, not code that CARRIES a captured answer. Proven, not read. With the sibling authorization fence already merged, a member's POST to the forms submit route returned 201 Created when the OS operator booted Admin and 403 capability-denied when the same operator booted Member. The member's write authority was the operator's, frozen at boot; their read visibility of an Admin-gated field moved the same way. Both route families are now mapped into a desktop-plane-only route group whose endpoint filter refuses while a hosted-web request principal is bound, per ADR 0160 D5 (consume the request principal or be unavailable). The plane signal is deliberately the same one the sibling fence reads, so the two cannot disagree. Refusal only — no permission is resolved, so the surface only gets more closed. The four tests were observed failing under five mutations: the fence condition neutered (three web tests red, desktop green), a blanket refusal (desktop red), the shared carrier narrowed to mutations (both GET-driving tests red), and each route family in turn mapped outside the group (its own test red). Refs: #3367 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N7ge8CQJE4tyGHQSddBYmJ
Three text findings from the deep review. All three are about claims that disagree with the code, on a change whose stated guard IS the comment. The SECURITY banner said the subject, roles and owner are captured once at startup. Only ROLES is, since #3437 made the other two per-request -- so the file carried two mutually exclusive statements and the wrong one was the banner a future author reads first. It now names roles alone and says explicitly not to restore a captured subject or owner to match it. The same stale premise is fixed in two places in the fence test's own docs; the experiment's conclusion survives, because the ROLE is what decides the outcome, but its premise had to be narrowed. "The whole family" was broader than the code. FormDraftRoutes declares the SAME /api/local-node/forms route base, registers elsewhere, and is NOT in this group -- so four routes stay open on the web plane. It is the same deputy by a third mechanism: its handlers resolve IPartyContext from the outer container, so a web-plane member's draft is keyed to the operator's party and the list route returns the operator's drafts to that member. The claim is narrowed to the two families actually mapped here, and the gap is named where the next reader will look. Tracked separately. The acceptance E2E maps both families onto its own unfenced app and asserts the joiner succeeds -- which production now refuses. It is not wrong and must not be deleted: it is the only pin on the per-request identity half. It is now annotated to say it proves ATTRIBUTION and not reachability, so a green "two user acceptance" cannot be read as evidence a member can reach these routes. Also documents what the fence primitive does NOT reach, and that it is fail-OPEN by construction -- absence of a bound web principal means allow. Correct for a desktop-authority surface; on a roster-mutation surface the same default would silently grant genesis-admitter minting. The admission card inherits that polarity and should decide it rather than absorb it. Suite 1,693 passed, 0 failed. Refs: #3367 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H7X6vvwjSyFSPBT9Ss7TgN
06682b4 to
cf49037
Compare
1 participant
Closes #3367
Step 1 first — the deputy is live, and it survives the sibling fence
The experiment, run before anything was built. With PR 3363 (card #3356) already merged into
the base, a signed-in member's request to the forms surface was still judged by the desktop
operator's authority:
POST /api/local-node/forms/{id}/submitTeamRole.AdminTeamRole.Membercapability deniedTheir read visibility of an
Admin-gated field moved the same way (readable / not readable). Themember's answer was the operator's answer, and nothing about the member changed between the two runs.
That the sibling fence was already in the base is the point of this card. #3356 decorates the
single place
IAuthorizationContextis composed, so it reaches code that asks an authorizationquestion. This family never asks:
HostedFormsApiEndpoint.StartAsynccaptures the operator'ssubject, the operator's
ICurrentUser.Rolesand the definition owner once, and the routes mint aper-request capability token from those captured values. A family that has already left the seam
cannot be reached by a seam-gate, however hard that gate is hardened.
Why the flip is at boot rather than mid-run. #3356's proving shape flips the operator's
membership while the host runs, because the seam it fences re-resolves grants on every call. This
family reads the roles once, so a mid-run flip changes nothing — the equivalent lever is the role the
operator holds when
StartAsyncruns, which is why each half of the test builds its own host. Statedthat way the defect is if anything worse: the member does not merely borrow the operator's authority,
they borrow a stale snapshot of it.
The fence
Both route families the endpoint maps go into a desktop-plane-only route group whose endpoint
filter refuses with
403 {"code":"web-plane.route.unavailable"}while a hosted-web request principalis bound — ADR 0160 D5, "either consumes the request principal or is unavailable". No permission
is resolved; the surface only ever gets more closed. Desktop-plane calls are untouched.
Three deliberate choices:
listener's caller-auth and MTW-2: a web request may be authorized as the desktop operator — fence the plane before the front door opens #3356's single-seam decorator. A per-route guard is the forgotten-route
problem. The fence is installed by the endpoint that performs the capture, in the same
MapApiRoutescall, so capture and refusal cannot be separated.NodeCallerAttributionScope.HasBoundWebPrincipal), not asecond carrier. R3-D makes attribution and authorization two readings of one fact, and two carriers
could disagree — a request fenced out of the operator's grants while still minting the operator's
forms capability, or the reverse, with nothing to catch it. It also doubles the number of carriers
that can go silently inert, which card P0 audit: member attribution is INERT in the serving pipeline — every production audit row stamps operator-fallback #3192 already caught happening once. Taking that reuse
carries the obligation its review produced: this third consumer pins its own requirement with a
gated GET, rather than trusting MTW-2: a web request may be authorized as the desktop operator — fence the plane before the front door opens #3356's test to notice a narrowing.
matches the sibling's refusal shape.
Testing
Four tests, each observed failing under a mutation that was run, not reasoned about:
false && …)true || …)GET)FormDefinitionRoutesmapped outside the groupFormsRoutesmapped outside the groupdotnet build apps/local-node-host -warnaserror: clean. Whole-solution build clean apart from apre-existing
CS0162inapps/kitchen-sink, reproduced on an unmodifiedmaincheckout.SelectedSessionRequestPrincipalTests:54-55is not in the diff and is unchanged. Nothing isrenamed or deleted, so there is no semantic-conflict surface.
Sweep — what else carries a principal without asking
The card asked for the shape, not the site. Scanning every
StartAsyncthat maps routes turned uptwo more instances of exactly this shape, both unfenced by either card, plus one adjacent. They
are reported, not fixed — each needs its own decision, and both are reachable by a selected-session
cookie today (neither path is on the listener allowlist):
HostedAdmissionApiEndpoint— capturesadmitterPartyIdat startup and closes it overAdmissionRoutes.Map.AdmissionRouteshas no caller-auth guard of its own beyond the listener,and consults no
IAuthorizationContext. A web-plane member reachingPOST /admission/invitesor/admission/redeemmints and admits as the genesis admitter, signed with the node's key. Thisis roster mutation and it is the most serious of the three.
HostedCommsApiEndpoint— capturesactiveMemberPartyIdat startup and stamps it as theauthor of every appended message. The routes call
callerAuth.Validate, but that returnsAllowon the gate-passed marker the listener sets for a selected-session request, so it is not a plane
boundary. A member's message is authored as the operator's roster party and signed with the node's
key, then merged into a CRDT other nodes accept.
DocumentTemplateRoutes(:136,:190) —PlacedBy = ActiveTeamAuthorizationContext.NodeOperator,a constant rather than a startup capture, on a route family that consults no authorization context.
Attribution rather than authority, so lower severity, but the same "carries a principal without
asking" family.
Two things that look like the shape and are not:
NodeCallerParty.Resolve(http)reads the requestfeature and falls back to the operator only when genuinely unbound — that is the correct pattern, and
it is what the financial routes use.
SchedulingDefinitionRoutescloses overICurrentUser, but everyroute gates
authz.HasPermissionbefore reading it, so #3356 covers it — by gate ordering per site,which is worth knowing is what is holding it.
One thing the sweep surfaced that belongs to nobody yet:
NodeTenant.Resolve(activeTeam)means aweb-plane request's tenant is whatever team the desktop operator has in the foreground. R3-D names
that shape explicitly. It is not this card and not #3356; it may want one of its own.
Sequencing consequence. The card says fencing one deputy and not the other leaves the door open
before shipyard#3329. By the same argument, #3329 should not go live with admission and comms
unfenced.
WebPlaneUnavailableRouteFenceis a reusable instrument, so fencing either is a two-linechange in its hosted endpoint plus its own tests — but the decision is Admiral's, not mine, and
admission in particular may want a deliberate answer rather than a blanket refusal.
Gate chain
commitlint→Build & Test→authchange_type deep review → Admiral arms. Not self-armed;opened as DRAFT.
🤖 Generated with Claude Code
https://claude.ai/code/session_01N7ge8CQJE4tyGHQSddBYmJ