fix(security): explain resolves the ONE authorization aggregation (#6352) - #6764
Merged
Merged
Conversation
) `buildContextForUser` was a hand-written second implementation of `@objectstack/core`'s `resolveAuthzContext` aggregation, kept in step by two comments claiming it mirrored the resolver. Measured over identical rows it did not: it dropped the `sys_member` role positions, every position-bound permission set (`sys_position_permission_set`), the `everyone` anchor's bound sets, the `platform_admin` position projection, `systemPermissions`, the posture rung, `email` and the `ai_seat` synthesis. Permission sets resolve BY NAME from `positions` + `permissions`, and a position-carried set only becomes a name inside the resolver — so any user granted through a position was explained as holding nothing, and the panel reported a denial enforcement never made. It now calls `resolveUserAuthzGrants` and adds presentation only: the ADR-0091 expired / `delegated_from` row annotations the resolver correctly discards, and `hasPlatformAdminGrant`, read back off the resolver's own posture verdict. Pinned by a parity suite running both implementations over the same fixture rows with per-case expected output, so it cannot pass by both sides resolving to nothing. `check:authz-resolver`'s ALLOW entry is narrowed to what still trips its heuristic — the explain-only provenance pass — without touching its remit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BM1tNf5U3nEbHKR4fo5qVQ
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 13 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-zhuang
marked this pull request as ready for review
August 8, 2026 14:43
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.
Fixes #6352
Convergence (option 1), per the findings-sweep promotion's ruled direction. No read-shape obstacle was found:
resolveUserAuthzGrants(ql, userId, opts)is already the userId-driven arbitrary-user entry point (service-automationuses it forrunAs:'user', #3356), so the fallback parity-pin route was not needed. Nopackages/corechange.The premise held, but one of its sub-claims did not
The card filed this as a dormant drift risk: "今天两者是一致的,没有已知的用户可见错误". Measured against
origin/mainbefore touching anything, that is false — the two had already drifted, and the drift is user-visible today.One probe, one
qlserving identical rows to both implementations:resolveUserAuthzGrants(enforcement)buildContextForUser(explain, before)positions['org_admin', 'hr_specialist', 'everyone']['hr_specialist', 'everyone']permissions['payroll_reader', 'hr_tools', 'ai_seat']['payroll_reader']systemPermissions['manage_users']posture'MEMBER'email/org_user_idsFive aggregation inputs the mirror never read: the
sys_memberrole projection (ADR-0095 D3), position-bound permission sets (sys_position_permission_set), theeveryoneanchor's bound sets (ADR-0090 D5), theplatform_adminposition projection (ADR-0068 D2), and the ADR-0024ai_seatsynthesis.Why that is a live defect and not cosmetics.
resolvePermissionSetsForContextresolves permission sets by name fromcontext.positions ∪ context.permissions, and a set carried by a POSITION only ever becomes a name inside the resolver (step 6a). So for any user whose grants arrive through a position — the ordinary way an organization grants access — explain resolved strictly fewer sets than enforcement and reported a denial the runtime never made. A security panel answering "you have no access" about access the user has is exactly the failure the panel exists to prevent.Worth recording for the next reader: the one criterion that had not drifted is the ADR-0091 validity window, because both sides already called the same
isGrantActive/isGrantExpiredmodule. Shared predicate, duplicated aggregation — the duplication was in which tables get read, never in the row rule.The change
buildContextForUsercallsresolveUserAuthzGrants. Everything the panel reports about who holds what now comes from the single governed resolver — positions, permission-set names, theplatform_adminderivation, posture,accessible_org_ids,systemPermissions.What stays explain-side is presentation, and additive:
collectGrantProvenance()re-reads the two grant tables purely to annotate rows the resolver dropped: expired grants (held until … — expired, so "why did access disappear" is answerable) anddelegated_fromorigin. It feeds no verdict; both annotations are decided by the same shared ADR-0091 predicate, not by a re-derived one.hasPlatformAdminGrantis now read back off the resolver's posture verdict (posture === 'PLATFORM_ADMIN'), not recomputed from the grant rows.The returned context additionally carries
systemPermissions,org_user_ids,posture,tabPermissions,email. Additive — no field removed or renamed.One consequence documented in place rather than left for the next reader:
resolveDelegatorContextinheritstenantId/org_user_idsfrom the live principal.buildContextForUsernow returns the resolver's ownorg_user_ids, which without a knowntenantIdis the degenerate[userId]seed. The live principal's real org peer set is the better answer, so that assignment still overwrites it exactly as before; the doc bullet now says so.check:authz-resolver— narrowed, not widenedThe card warns against widening this gate's remit as a shortcut. The remit is untouched:
GRANT_TABLES, the query-shaped criterion and the DELEGATORS list are unchanged. Only the ALLOW reason forexplain-engine.tschanged, and it now claims less: the file still trips the heuristic, but only because of the explain-only provenance pass, and the parity invariant it used to defer to #6352 is named as pinned by tests rather than by this gate. The entry is still live (the file still queries both tables), so it is not dead weight.node scripts/check-single-authz-resolver.mjsand its--self-testboth pass.Reverse verification — direction predicted first: RED
Predicted before running: restoring the deleted mirror should turn the new parity suite red, because the pins assert concrete resolver output the mirror cannot produce. Taken out with
git checkout origin/main -- explain-engine.ts(nevergit stash).9 of 49 red, each naming a specific dropped input:
Restored → 49/49 green.
Honest note on what did not go red: the
ADR-0091 windowscase passes against the mirror too — the shared predicate, again. It is in the suite because the window rule is load-bearing for theplatform_adminderivation (an expired unscopedadmin_full_accessmust confer no rung), not because it discriminates the two implementations.Fixture fidelity — a defect the old shape was blind to
The
buildContextForUserfakes returned every row for a table regardless ofwhere. The mirror filtered in memory, so it produced the right answer anyway. The resolver delegates filtering to the engine, as the real ObjectQL engine does, so a fake that ignoreswherenow reports grants nobody holds — which is what surfaced whenquarter_close_admin(an expired grant) appeared inpermissions. The fakes are replaced with onewhere-honouring stand-in (scalar equality +$in); the comment above it says why that is load-bearing rather than tidiness.Non-vacuity
Under convergence a bare
explain == coreassertion is near-tautological, so each parity case also pins the concrete output the one aggregation must produce (positions, permission names,systemPermissions,accessible_org_ids, posture,platform_admin). The suite cannot pass by both sides resolving to nothing. Cases cover: org role projection, position-bound sets, theeveryoneanchor, bothplatform_adminpolarities,organization_admin→TENANT_ADMIN, and ADR-0091 windows. A seventh case pins that the explain-only annotations are additive — non-emptyexpiredGrants/delegatedPositionswhilepositions/permissionsstay byte-identical to enforcement's.Verification
pnpm --filter @objectstack/plugin-security test→ 828 passed (40 files; 822 before, +6 new)pnpm --filter @objectstack/core test→ 524 passed@objectstack/runtime1702,@objectstack/service-automation858,@objectstack/plugin-dev45,@objectstack/verify17 — all passedpnpm --filter @objectstack/plugin-security typecheck→ clean; repo-wideturbo run typecheck→ 120/120pnpm lint→ cleancheck:*step enumerated from.github/workflows/lint.yml, run one by one → all PASS, includingcheck:authz-resolverand its--self-test.check:i18n,check:i18n-coverageandcheck:app-nav-i18nfirst reported PREREQUISITE NOT MET; re-run after a full workspace build, all three exit 0 ("could not run" is not a pass)pnpm --filter @objectstack/spec check:generated→ 10/10 up to datenode scripts/check-nul-bytes.mjs→ OK; control-byte self-scan of the changed files → no hitsScope
explain-engine.ts+ its tests, thecheck:authz-resolverALLOW reason, one changeset. Nopackages/core, no spec, nocontent/docs/releases/.Two things checked and deliberately not touched:
explain-engine.ts:720's provenance-blind Layer 1 collapse (explain's own remit, out of scope per the card), and #6684'smember_defaultwildcard removal —buildContextForUsernever referencedmember_default, and the'*'grants in the explain fixtures belong toadmin_full_access, which still ships one. Nothing to fix, nothing to file.🤖 Generated with Claude Code
https://claude.ai/code/session_01BM1tNf5U3nEbHKR4fo5qVQ
Generated by Claude Code