Skip to content

fix(auth): canonicalise sys_member.role at the write, and converge existing rows - #8417

Merged
os-zhuang merged 4 commits into
mainfrom
claude/issue-8317-normalize-member-role
Aug 13, 2026
Merged

fix(auth): canonicalise sys_member.role at the write, and converge existing rows#8417
os-zhuang merged 4 commits into
mainfrom
claude/issue-8317-normalize-member-role

Conversation

@os-zhuang

@os-zhuangos-zhuang commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Fixes#8317

Implements the maintainer's ruling of 2026-08-13 — option A, normalise at the write. Options B (read-seam interception) and C (document the caveat) are not implemented.

The defect

A membership stored with a non-canonical role — Owner, ' owner', OWNER — was an owner to every ObjectStack-side check and a plain member to better-auth.

better-auth 1.7.0-rc.2 reads that column with a raw role.split(","), no trim() and no toLowerCase(), in three branches of dist/plugins/organization/routes/crud-members.mjs:

branchvendor line
removeMember — only an owner may remove an ownerconst roles = toBeRemovedMember.role.split(","); then if (roles.includes(creatorRole))
updateMemberRole — creator protectionconst isUpdatingCreator = toBeUpdatedMember.role.split(",").includes(creatorRole);
organization/leave — last-owner countif (member.role.split(",").includes(creatorRole))

Every ObjectStack reader trims and lower-cases (the #5942 grade ladder, mapMembershipRole). So on such a row the vendor never entered its owner branch and fell through to hasPermission({ member: ['delete'] }) — which an org admin passes.

What landed

1. Write-path canonicalisation (member-role-canonical.ts) — ObjectQL beforeInsert / beforeUpdate hooks on sys_member, registered in auth-plugin.ts at priority 5, ahead of the ADR-0092 identity write guard (10) and the ADR-0024 D5.2 break-glass guard (20), so both judge the value's normal form. They fire for every context, isSystem included — better-auth's adapter, SCIM remaps and import scripts are exactly the paths this exists for.

2. A one-off convergent boot pass (canonicalizeStoredMemberRoles, a kernel:ready hook next to the existing account-issuer backfill) — idempotent, safe to re-run, and it reports a census of every distinct non-canonical spelling with row counts before and after rewriting, rather than a bare number.

Canonicalisation is per token, and the second clause is a measured consumer fact rather than caution:

  • a token that is a membership role (ADR-0108's closed vocabulary) is trimmed and lower-cased;
  • any other token is preserved verbatim apart from trimmingresolve-authz-context.ts projects every token through mapMembershipRole, whose default: arm returns raw.trim() with the case preserved, so a foreign token is a position name a sys_position_permission_set row may be bound to. Lower-casing it would silently re-point that binding;
  • tokens empty after trimming are dropped.

A value carrying no known role is left completely untouched and only reported: no reader can grade it as an owner, so it cannot produce the inversion. A mixed value (Owner,Sales_Manager) is rewritten — it carries owner, so it is the inversion class — to owner,Sales_Manager, foreign case intact. A value-level "all tokens known" rule would have left that hole open; there is a test for exactly it.

The invariant this buys, stated so it can be tested: for any canonicalised value v and any known role R, v.split(',').includes(R) equals parseOrgRoles(v).includes(R).

Verification

The three vendor branches are pinned against predicates EXTRACTED FROM THE INSTALLED VENDOR FILE, not restated in TypeScript. A restatement would put both sides of the comparison on the same source and could not fail for the reason it exists. The test reads crud-members.mjs, matches each branch, asserts the match is unique, and builds the predicate with new Function from the captured bytes. A vendor upgrade that moves or corrects any branch fails the extraction and reddens the suite instead of leaving a pin that verifies nothing.

member-role-canonical.test.ts, 35 tests, all green. It reproduces the inversion first (ladder says owner, all three vendor branches say plain member) and then shows canonicalisation closing it on all three.

Predict-then-mutate ablation — predictions written down before any run:

mutationpredictedobserved
remove .toLowerCase()case family red, whitespace family green16 failedOwner / OWNER / ' Owner ' red, ' owner' / 'owner ' green
remove .trim()whitespace family red, case family green12 failed — exactly complementary
do not register the hooksonly the write-path block, failing as a throw8 failed, all no canonicalisation handler registered
loosen the branch-3 extraction to a 4x-matching patternthe drift tripwire fires on ambiguity12 failed, all pattern matched 4 times ... must identify exactly one branch
drop the id from the pass's update payloadthe assertEngineUpdateDispatch pin is live, not decorative4 failed — the dispatch predicate threw, counted as failed rows

The first two are the anti-vacuity evidence: a vacuous vendor pin would have stayed green under both.

Non-canonical spellings found before rewriting — a static census over every tracked file, 216 role literals in sys_member context, 15 distinct spellings. Every non-canonical inversion-class spelling (Owner, ' Owner ', Admin) is in this PR's own new test file. The pre-existing non-canonical values (sales_manager, sales_rep, not_a_declared_role) are already lower-case and trimmed, carry no known role, and are therefore left untouched by construction — consistent with plugin-auth's 1163 tests and dogfood's 748 passing unchanged. No spelling was found that normalisation would destroy meaning for.

Gates run locally, real results:check:nul-bytes OK · check:engine-double-contract OK (193 pinned, nothing added to the baseline) · check:error-code-casing OK · check:adr-0087-registration OK (no declared-breaking changeset, so no marker owed) · check:empty-changeset OK · check:durability-log-level OK · check:startup-registry-verdict OK · check:type-check-debt OK, re-measured with the full closure built: "none above its recorded number, surplus: none". pnpm --filter @objectstack/plugin-auth typecheck clean, test 1163/1163, @objectstack/dogfood 748 passing.

⚠️check:engine-double-contract does not discover this PR's memory engine: its scan requires two engine siblings besides the verb and this double has one (find). The assertEngineUpdateDispatch pin is there and is live — the ablation above proves it fires — but the gate is not what verifies it.

Decoupled from #8289, deliberately

remove-member-permission-guard.ts is untouched. It reproduces the vendor's predicate byte-for-byte on purpose, including the asymmetry where the target's roles are split without trim() and the caller's with it. After this lands, its refusal population and the vendor's agree by construction — which is the point of leaving it alone. A test pins that: the guard's target half still matches the extracted vendor predicate value for value, and targetCarriesCreatorRole(' owner', 'owner') is still false.

The one hole that stays, named rather than hidden

A write that never reaches ObjectQL — raw SQL, an out-of-band driver fixture — can still store a divergent row after boot. It converges at the next restart. Closing it would mean a database-level constraint, a larger decision than the one ruled here.


Generated by Claude Code


Generated by Claude Code

better-auth reads sys_member.role with a raw split(',') -- no trim, no
lower-case -- so a row stored as 'Owner' or ' owner' is an owner to the
#5942 grade ladder and a plain member to the vendor. Its 'only an owner
may remove an owner' branch therefore never fires and the request falls
through to hasPermission({ member: ['delete'] }), which an org admin
passes: an org admin could remove an owner.
Maintainer ruling 2026-08-13, option A -- normalise at the write:
beforeInsert/beforeUpdate hooks on sys_member plus a one-off convergent
boot pass for existing rows.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73
@vercel

vercelBot commented Aug 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 13, 2026 1:30pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-auth.

8 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/deployment/cli.mdx(via @objectstack/plugin-auth)
  • content/docs/deployment/production-readiness.mdx(via @objectstack/plugin-auth)
  • content/docs/kernel/contracts/cache-service.mdx(via @objectstack/plugin-auth)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/plugin-auth)
  • content/docs/permissions/authentication.mdx(via @objectstack/plugin-auth)
  • content/docs/permissions/sso.mdx(via @objectstack/plugin-auth)
  • content/docs/plugins/index.mdx(via @objectstack/plugin-auth)
  • content/docs/plugins/packages.mdx(via @objectstack/plugin-auth)

2 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/plugin-auth)
  • content/docs/releases/v9.mdx(via @objectstack/plugin-auth)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

…is CJS-typed package)
plugin-auth publishes CommonJS, so under module: NodeNext any import.meta
is a TS1470 — which drifted the package's frozen TEST_DEBT ledger entry
111 -> 112. Fixed the type rather than the ledger: reuse the findUp-from-CWD
idiom rate-limit-storage-isolation.test.ts already established here, and
seed createRequire from the package root so the better-auth file read is
the one THIS package is pinned to. Re-measure is back at 111.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73
@os-zhuang
os-zhuang marked this pull request as ready for review August 13, 2026 13:41
@os-zhuang
os-zhuang added this pull request to the merge queueAug 13, 2026
Merged via the queue into main with commit 73dc89bAug 13, 2026
27 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-8317-normalize-member-role branch August 13, 2026 14:01
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/xlteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

better-auth's org-role matching is case- and whitespace-sensitive, so role='Owner' is an owner to our grade ladder and a plain member to the vendor

2 participants

@os-zhuang@claude