fix(plugin-hono-server): delegate permission-set resolution to the security service - #10035
Conversation
…curity service (#7616) `/auth/me/permissions` and `/me/apps` re-implemented the caller's permission-set resolution locally — composing the requested names, building their own `sys_permission_set` DB loader and calling the evaluator directly. That made one rule three copies, and it drifted from the enforcement path three times. Both handlers now call `ISecurityService.resolvePermissionSetsForContext` on the `security` service and keep only the projection each surface needs. The fallback for a SecurityPlugin too old to carry the method is deleted (it ships in @objectstack/spec@17.0.0); the degraded branches for a stack with NO SecurityPlugin stay, reached through the service locator so no runtime dependency on plugin-security is taken. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r
…fake WHERE matcher `check:where-matcher` judges every hand-written WHERE matcher in a test file: reading a `$`-prefixed combinator as a field name yields a silent `false`, which is indistinguishable from a row that legitimately did not match, so a case can pass for the wrong reason. Refuse loudly instead, matching the convention the sibling fixtures in this package already follow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r
📓 Docs Drift CheckThis PR changes 1 package(s): 15 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 6 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 7 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin d2bafad9f56f6ebc896270b65773af3f5a460079 && git checkout d2bafad9f56f6ebc896270b65773af3f5a460079
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 09b880b0ce9e2a9b140666ab4a2f75b0c55fe96f 39144a231e7f331a55430483a5257a03b995868d && git checkout -B drift-repro 09b880b0ce9e2a9b140666ab4a2f75b0c55fe96f && git merge --no-ff 39144a231e7f331a55430483a5257a03b995868d
node scripts/docs-audit/affected-docs.mjs --json 09b880b0ce9e2a9b140666ab4a2f75b0c55fe96f
|
…third-state comment The comment argued that the bailed-start widening is right because the previous answer was "the fail-direction #7608 names as the worse one". #7608 says the opposite: it calls the UI-under-reporting direction the MILDER reading of its own defect — "so it presents as 'the console hides things the API allows' rather than as an exposure" — offered as the reassuring contrast to an exposure, not as a grading of one direction against the other. The argument itself is unaffected and stands on its own merits: in that state the plugin registered no middleware, so the access the endpoint reported as withheld was not being withheld by anything. It is a judgement under review, not a recorded ruling, and the comment now says so. Comment-only. No behaviour, no assertion, no conclusion changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r
Fixes #7616
Step 2 of the card:
/auth/me/permissionsand/me/appsstop re-implementingpermission-set resolution and call the enforcement path's own resolution
instead. Step 1 (the contract method plus its exposure on the registered
securityservice) shipped in PR #7829 and is unchanged here — nothing in thisdiff touches
packages/specorplugin-security.What the endpoints used to do
Both handlers resolved the caller's sets themselves: composed the requested
names (positions ∪ explicit sets ∪ the deployment baseline), built their own
sys_permission_setDB loader, and called the permission evaluator directlythrough the
security.permissionshandle. That is one rule in three copies —the enforcement path's, and one per endpoint — and the contract's own header
names the consequence: a consumer that re-derives these answers locally "will
drift the moment the enforcement path changes."
It drifted three times, each divergence found only after it reached a user:
#7608 (both endpoints kept the fallback cliff the additive ADR-0090 D5 baseline
abolishes, so a member's first grant took them from 2 apps to 1), #7555 (an
app-declared
isDefaultset displacedmember_defaulthere rather thancomposing with it), and #6334 (grant aggregation missed
sys_user_position/sys_position_permission_setentirely — retired by delegating toresolveUserAuthzGrants, which is the precedent this extends one step).Both call sites now call
ISecurityService.resolvePermissionSetsForContextandkeep only the projection each surface needs.
The two absences — one dropped, one kept
The card's third constraint is that
plugin-hono-servermust not take a runtimedependency on
plugin-security, which is optional in the stacks these endpointsserve. So the two absences are not the same absence:
@objectstack/spec@17.0.0, so the local copy that stood in for it is deleted rather than kept as a third pathThe contract declares the method optional, so the narrowing is also what the
type demands: the unguarded call does not compile. What changed is only what the
false branch does — it is now the degraded answer rather than a local
resolution.
Before/after on the wire — measured, not asserted
The card flagged that
/me/appsdeliberately projected a narrower column set(
name+systemPermissions+tabPermissions) than/auth/me/permissions,and that delegating changes which columns load on both surfaces.
Measured by booting the real
SecurityPluginover a fake engine and runningboth builds of
plugin-hono-server—origin/mainand this branch — againstone service locator carrying everything a real stack registers, so each
build simply ignores the handles it does not read and both answers come out of
identical data.
/auth/me/permissions/me/appsactive: false)/me/appsnever projectedNo additional field reaches the wire on either endpoint.
/me/appsnowloads the sets whole, and the columns it gained are read by nobody in that
handler — the body is the app list and only the app list. That is pinned
directly, including a
JSON.stringify(body)assertion that no permission-setcolumn leaks through the filter.
Three states do change, and all three are states where the UI plane previously
disagreed with the data plane:
1. A deactivated set whose name matches a live position name. The plugin's
loader drops deactivated
sys_permission_setrows; the local copies did not.Core already filters deactivated sets out of
context.permissions, so this isreachable only through the second reader — a position name reused as a set name.
Before, that row granted
billing.manage, thebillingtab and object/fieldaccess here while granting nothing on the data plane; after, nothing on both.
Narrowing, fail-closed, and a fourth instance of exactly the drift this card
documents.
2. A permission set with a malformed JSON column.
/auth/me/permissionsparsed with a bare
JSON.parse, so one malformed column threw inside its loaderand cost the member the whole set; the plugin's loader degrades that column
alone. Note
/me/appswas unaffected because its narrower loader never parsedthe column that was malformed — the two copies in this one file already
disagreed with each other about the same row.
3. SecurityPlugin present,
start()bailed. This is a third state thetwo-absence framing does not name, and it is the one worth review. The plugin
registers
security.permissionsininit()but thesecurityservice only instart()— which returns early on an engine that cannot take middleware, andthat same early return is before any middleware is registered. So a stack in
that state enforces nothing on the data plane. Before, these endpoints
answered with a restrictive map and 1 of 3 apps, computed against enforcement
that does not exist — so the access they reported as withheld was not being
withheld by anything, and the console was describing a policy no layer applied.
After, they degrade, which is what the degraded branch's own premise ("matches
server behaviour when SecurityPlugin isn't registered") asks for.
This is a widening, so it is called out rather than folded in. The argument for
it is the one just given, on its own merits — it is not a recorded ruling about
which fail-direction is worse, and no such ruling is cited here.
Tests
packages/plugins/plugin-hono-server: 20 files / 225 tests pass, up from215 — plus
typecheckclean.New:
current-user-endpoints-delegated-resolution.test.tspins the delegation,both absences separately, the third state above, the wire measurement, and the
contract's fail-closed stance on a thrown resolution (absence fails open, a
runtime failure must not).
The two existing fixtures were re-dispositioned rather than mass-edited. Both
resolved through
security.permissions, so both moved to the published service.In the #7608 file the additive rule itself now lives in
plugin-security, sothe cases that asserted the call shape were replaced by delegation pins, and the
case that pinned this file's own baseline fallback chain was replaced outright —
that chain is deleted, and an assertion about it would have kept passing while
measuring nothing. What replaces it is the property that makes the deletion
true: the locator carries the
security.*internal handles by default and theendpoints must never ask for them, with the
security.permissionsdoublethrowing if called.
Reverse-verified: with
current-user-endpoints.tsreverted toorigin/mainandthe tests kept, 21 cases fail across the three files, and exactly the four
absence pins stay green — correct, since those degraded branches exist on
maintoo. Restored, re-confirmed 225/225, and the tree is byte-identical to the
commit. The tests import
./current-user-endpointsby relative path, so vitestcompiles the source directly: no
distand noexportsresolution is involved,and no rebuild gates this ablation.
Gates
Re-derived with
node scripts/pm/dispatch-gates.mjsover the real change set(no path arguments) and run at
6eb75a85f:check:cross-package-test-inputs,check:route-envelope,check:slot-lookup,check:test-source-alias,check:type-source-resolution,check:changeset-gate-self-tests,check:objectui-changeset,check:query-options-erasure,check:engine-double-contract,check:where-matcher,check:type-check-coverage,check:type-check-debt,check:nul-bytes,check-adr-0087-registration,check-changeset-no-major,check-empty-changeset,docs-audit/check-affected-docs, andpnpm lint(repo-wide, clean).
The derivation added the changeset and test-file convention families the
dispatch list could not name. One of them caught a real defect in the new
fixture: its WHERE matcher read a
$-prefixed combinator as a field name, whichyields a silent
falseindistinguishable from a row that did not match. Fixedby refusing loudly, as the sibling fixtures do — second commit.
Ratchets:
type-check-debt --re-measurere-measured 33 ledger entries on thebuilt closure — "none above its recorded number ... surplus: none".
where-matcherandslot-lookupboth report "none new" with the baseline keyset verified. Nothing weakened, no baseline moved, no test skipped.
Not touched
The card's related item — the stale doc-comment in
plugin-security'sresolveFallbackPermissionSets— is already current onmain: PR #7829corrected it. Verified, no edit needed, and that file is another lane's anyway.
Generated by Claude Code