Skip to content

fix(core)!: a banned user's credentials are refused while the ban holds, at every door - #30287

Draft
objectstack-fleet[bot] wants to merge 6 commits into
mainfrom
claude/sec-hardening-r4
Draft

objectstack-fleet[bot] wants to merge 6 commits into
mainfrom
claude/sec-hardening-r4

Conversation

@objectstack-fleet

@objectstack-fleet objectstack-fleet Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Clause-②: yes (narrowing) — while a user's ban holds, a request authenticated by an API key that user owns is answered as a key that does not exist (the generic 401 UNAUTHENTICATED), at every door that accepts a key. The widening half: @objectstack/core exports the one ban predicate, isUserBanInForce (with USER_BAN_FIELDS and the UserBanFields type).
Metadata-protocol: none
Skills-docs: updated here — content/docs/permissions/authentication.mdx and content/docs/permissions/administrator-guide.mdx state what a ban holds; the dogfood pin api-key-banned-owner.dogfood.test.ts exercises it.

维护者速读

  • 改了什么:用户被封禁期间,其名下的 API key 在所有接受 key 的入口(REST 数据读写、MCP 等)一律按"不存在的 key"拒绝(401);封禁带到期时间的,到期即恢复;key 本身不吊销。
  • 为什么改:封禁本应管住该用户的全部凭据,登录与会话已被拒,key 要与之一致;同时把封禁判定收敛为一个共享谓词。
  • 风险与回滚:每次 key 鉴权多一次按主键读 sys_user(实测约 0.65 ms);被封禁用户的集成会收到 401,属预期;回滚即还原本 PR。
  • 先看:packages/core/src/security/api-key.ts(准入处新增的封禁判定段)、packages/core/src/security/user-ban.ts、packages/qa/dogfood/test/api-key-banned-owner.dogfood.test.ts。
  • 两分钟验证:pnpm --filter @objectstack/dogfood exec vitest run test/api-key-banned-owner.dogfood.test.ts,预期 4/4 通过。
  • 需要你做的:无,席位落地。

What changes

  • The rule. A ban holds every credential of the user. While a user's ban holds, an API key that user owns is refused where the key is resolved to its owner (the shared key-admission seam in @objectstack/core, which every key-accepting door uses), with the same answer a key that does not exist gets: the generic 401 UNAUTHENTICATED, same code, same message, on the REST data door (read and write) and on the MCP door. A ban with an expiry stops binding once it expires, as sign-in does.
  • Not a revocation. No key row is revoked or rewritten, and a refused key is not stamped as used. Lifting the ban (or its expiry passing) admits the same key again, still judged by its own lifecycle: a key left unused past the inactivity window during the ban answers as inactive.
  • One ban predicate. isUserBanInForce (packages/core/src/security/user-ban.ts) mirrors the sign-in check (the ban flag; an expiry lifts it only once strictly past; an unreadable expiry does not lift it). plugin-auth's principal-session mint and agent-forward identity view now ask it instead of their own copies.
  • Order. The key's own lifecycle (revoked / expired / inactive) still answers first; the ban is asked before the tenancy and account-posture questions.
  • One anonymous entry. A key refused for its owner (a ban) or for the tenancy posture (no organization under isolated) resolves to the same entry as a key that does not exist, on every composition: the guest's grants on a face that serves the guest, nothing elsewhere, never a session. Both exits share one helper (anonymousEntry, packages/core/src/security/resolve-authz-context.ts).
  • Failure mode. An owner read that throws is the branded store outage (503), as a sys_api_key read that throws already is; a store with no sys_user table holds no ban.
  • Cost. One primary-key read of sys_user (three columns) per key resolution. Nothing earlier on the admission path loads the user row. Measured on the showcase stack (shared box): 0.650 ms per read (N=1000) against 7.738 ms per key-authenticated GET /data (M=300).
  • Docs: the ban entries in authentication.mdx and administrator-guide.mdx say what a ban holds. The Ban User confirmation and its agent-facing description (sys_user.ban_user) say the user's API keys are refused until the unban and work again after it; zh-CN / ja-JP / es-ES carried, en bundle regenerated (node scripts/check-i18n-bundles.mjs --write).
  • Changeset: .changeset/banned-user-credentials-refused.md (@objectstack/core, @objectstack/plugin-auth, patch, BREAKING narrowing banner, ADR-0087 not-required (no-migration-prescription)).

Tests

  • packages/core/src/security/api-key-banned-owner.test.ts (18): the predicate table; the seam refuses a banned owner's key with no principal; reads the ban columns by id as the system; a timed ban binds until it expires and not after; nothing is written during the ban and lifting it admits the same key; lifecycle answers first; absent owner row keeps today's answer; owner read outage is the branded 503; missing table holds no ban; resolveAuthzContext resolves the same context for a banned owner's key as for a made-up key, never falls through to a session sent beside the key; controls for a non-banned owner and for a session with no key; on a face that serves the guest, with a guest anchor that grants a set, a banned owner's key and an unknown key resolve the same entry (the guest's set, in the one organization).
  • packages/platform-objects/src/identity/action-confirm-one-dialog.test.ts: the Ban User confirmation pin now carries the new sentence and asserts it names the API keys.
  • packages/qa/dogfood/test/api-key-banned-owner.dogfood.test.ts (4, showcase + MCP): before the ban the key reads (200), writes (201) and opens an MCP session (200); while the ban holds every door answers the key exactly as it answers a made-up key (status, code, message, WWW-Authenticate), the refused write does not land, the ended session is 401, sign-in is 403 BANNED_USER, and another member's key keeps working; after the unban the same key is served and the key row is not revoked; a 3-second timed ban binds before expiry and not after, with no sign-in in between.

Acceptance notes

  • A revoked key answers 403 API_KEY_REVOKED by ruling (located lifecycle refusal), so it is distinguishable from a made-up key today; this PR matches the banned owner's key to the made-up key's 401, not to the revoked key's answer.
  • The refused-key path does not stamp last_used_at, so the inactivity window keeps running through a long ban; a key idle past the window when the ban is lifted answers API_KEY_INACTIVE (remedy: a new key).
  • The ban flag is read whether or not better-auth's admin plugin is enabled; with the plugin off nothing writes the flag except a direct row write.

Verification

Head f064a0e418 (findings commits 12c342a23d, f064a0e418 on top of 15b303e0bf). The ablation, package-suite and dogfood readings of the first round were taken on 15b303e0bf; this round's readings are listed under "Patch round".

Ablation (one-shot, not committed)

Mutation if (isUserBanInForce(owner, nowMs) && (owner as any)?.__ablation_marker_r4 === 'never') { through scripts/ablation-replace.mjs (anchor 1 to 0); pnpm --filter @objectstack/core build; ablation-dist-preflight.mjs @objectstack/core __ablation_marker_r4: marker present in 2 built files.

  • unit: Tests 5 failed | 12 passed (17)
  • dogfood door pin: Tests 2 failed | 2 passed (4)
    Restore: git checkout HEAD -- packages/core/src/security/api-key.ts, blob 37adafdc53a5 equals HEAD, git diff HEAD empty; rebuilt; --absent preflight: marker absent from all 14 built files, tree clean.
    A first attempt spelled the mutation false && …; the bundler folded it away, the dist preflight read it absent, and that run's readings were discarded.
Package suites and gates
  • pnpm --filter @objectstack/core typecheck && vitest run: Test Files 141 passed, Tests 3100 passed.
  • pnpm --filter @objectstack/plugin-auth typecheck && vitest run: Test Files 220 passed, Tests 3876 passed, 10 skipped.
  • node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack: 93 derived families run, each exit code recorded before any pipe; --ran reconciliation: 93 run, 0 NOT-MEASURED (derived zero). Two first answered PREREQUISITE NOT MET (exit 3: check:skill-examples, check:dual-build-cjs-loads) and exit 0 after building the unbuilt packages they read.
  • Full dogfood battery: pnpm --filter @objectstack/dogfood test at 15b303e0bf: Test Files 1 failed | 419 passed | 3 skipped (423); Tests 1 failed | 3495 passed | 36 skipped (3532). The one red, permission-set-clone-door.dogfood.test.ts (cloning member_default answers 422 INVALID_METADATA for grants on sys_comment / sys_activity), fails identically with this diff's packages/core/src and packages/plugins/plugin-auth/src reverted to base and rebuilt (Tests 1 failed | 10 passed), so it is not this change; reported to the seat.
Patch round (12c342a, f064a0e)
  • Ablation of the guest-face pin (source-resolved test, no dist leg): return anonymousEntry(ctx, input); in the refused exit replaced by return ctx; through scripts/ablation-replace.mjs (anchor 1 to 0, blob 90c589104c01 to 01c893387b0b): Tests 1 failed | 17 passed (18), the failure being the new pin. Restore: git checkout HEAD --, hash equals HEAD, git diff HEAD empty; Tests 18 passed (18).
  • pnpm --filter @objectstack/core build && typecheck && vitest run at 12c342a: Test Files 141 passed, Tests 3101 passed.
  • pnpm --filter @objectstack/dogfood exec vitest run test/api-key-banned-owner.dogfood.test.ts at 12c342a: Tests 4 passed (4).
  • pnpm --filter @objectstack/platform-objects typecheck && vitest run at f064a0e (uncommitted tree identical): Test Files 110 passed, Tests 1514 passed.
  • pnpm check:i18n: OK (10 packages, all bundles in sync).
  • Derived gates at f064a0e: 96 derived families, each exit code recorded before any pipe; --ran: 93 run (exit 0, including the three new for this head: check:authz-resolver, check:i18n, check:i18n-stale-fill), 3 NOT-MEASURED (check:skill-examples, check:dual-build-cjs-loads, check:type-check-debt: exit 3, their build prerequisite could not take the shared verify lock, held 40+ minutes by another session; all three were exit 0 at 15b303e0bf, and CI runs them).

Generated by Claude Code

A ban holds every credential of the user, at every door that accepts an
API key; a ban with an expiry stops binding once it expires. One ban
predicate (isUserBanInForce) now serves every place that asks.

Claude-Session: https://claude.ai/code/session_01QSeKYn5CmmdShCJifm6WYj
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Nov 5, 2026
@github-actions

github-actions Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/core, @objectstack/platform-objects, @objectstack/plugin-auth, touching 18 documentable anchor(s). ⚠️ 1 changed file(s) yielded no anchor (packages/core/src/security/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/permissions/system-context.mdx (via mintPrincipalSession (symbol, a top-level function))

⛔ 1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v17/17-4.mdx (via organization_membership_ended (literal, a string literal in ApiKeyRefusalReason))

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.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/core/src/security/index.ts) — pages documenting those are invisible to this run
  • 2 anchor(s) matched too much of the corpus to be a work list: sys_user (symbol, 49 pages), sys_user (literal, 49 pages)
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 103 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 50 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 09720e2c8ac970d87a950ba9163233e291e7996d → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 99ccb26c8acc4abed3619188e5fdae943c87af81 — the merge of head 1c3929139aa8e69c657c0a11d311147b6da25eda into base 09720e2c8ac970d87a950ba9163233e291e7996d, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 99ccb26c8acc4abed3619188e5fdae943c87af81 && git checkout 99ccb26c8acc4abed3619188e5fdae943c87af81
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 09720e2c8ac970d87a950ba9163233e291e7996d 1c3929139aa8e69c657c0a11d311147b6da25eda && git checkout -B drift-repro 09720e2c8ac970d87a950ba9163233e291e7996d && git merge --no-ff 1c3929139aa8e69c657c0a11d311147b6da25eda

node scripts/docs-audit/affected-docs.mjs --json 09720e2c8ac970d87a950ba9163233e291e7996d

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 09720e2c8ac970d87a950ba9163233e291e7996d → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

security#1 · os-justin · session_01QSeKYn5CmmdShCJifm6WYj

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: 15b303e0bfc3aaa636cded8c42e4e9171884c404
Local-runs: none
Scope: packages/core/src/security/api-key.ts, packages/core/src/security/user-ban.ts, packages/core/src/security/index.ts, packages/core/src/security/api-key-banned-owner.test.ts, packages/plugins/plugin-auth/src/agent-forward-identity-view.ts, packages/plugins/plugin-auth/src/principal-session-mint.ts, packages/qa/dogfood/test/api-key-banned-owner.dogfood.test.ts + packages/core/src/security/resolve-authz-context.ts, packages/core/src/security/request-execution-context.ts, packages/core/src/security/assemble-execution-context.ts, packages/core/src/index.ts, packages/runtime/src/security/resolve-execution-context.ts, packages/runtime/src/domains/mcp.ts, packages/runtime/src/domains/data.ts (and every runtime domain's shouldDenyAnonymous call), packages/mcp/src/plugin.ts, packages/rest/src/rest-server.ts, packages/services/service-storage/src/storage-service-plugin.ts, packages/types/src/driver-error-classification.ts, packages/plugins/plugin-auth/src/admin-ban-endpoints.ts, packages/plugins/plugin-auth/package.json, better-auth 1.7.12 packages/better-auth/src/plugins/admin/admin.ts (the vendor sign-in hook the predicate mirrors), scripts/check-adr-0087-registration.mjs, docs/adr/0087-metadata-protocol-upgrade-contract.md, scripts/adr-anchors/packages__plugins__plugin-auth__src__agent-forward-identity-view.ts.json. Prose checked sentence by sentence in ③ only: .changeset/banned-user-credentials-refused.md, content/docs/permissions/authentication.mdx, content/docs/permissions/administrator-guide.mdx.

Face: the committed changeset and the PR body both carry Clause-②: yes (narrowing); reviewFace reads a (narrowing) arm on either value as the narrowing face, so this record is at CONTRACT_REVIEW_TIER and --face declaration does not apply. Read-only throughout: diff, PR body, head check-runs, cited code; no worktree, build, test or gate run.

Check-runs read on this head (two readings, the second at the end of the review): every completed run is success — Lint & Repo Gates, Lint · ESLint, Lint · docs & code guards, Lint · repo & tooling gates, Lint · CI/release/engine gates, Lint · PM dispatch-gates, the three scoped lints, Type Check · source gates / consumer gates / debt ledger, Build Core, Build Docs, Build UI Deps, Build & E2E UI, Live E2E UI, Governed Surface Queue Guard, Check Changeset, Check PR Size, Check Documentation Links, PR-body guards, Spec property liveness, Dogfood Regression Gate, Plan Test Core shards, Auto Label, filter. Still in_progress at the last reading: Test Core (8 shards), Test UI (16 shards), Type Check UI, Type Check · workspace. No red. The known main red (permission-set-clone-door dogfood) touches permission-set-clone.ts grants on sys_comment / sys_activity; nothing in this diff reaches that path, so it is not attributable here.

① Derived judgments

  • Accept-set narrowing (the rule). At the one key-admission seam (resolveApiKeyAdmission), after the key's own lifecycle verdict and before the tenancy and account-posture questions, the owner row is read by id as the system (id, banned, ban_expires) and a key whose owner's ban holds is answered on the refused arm with the new reason owner_banned: no principal, no fall-through to a session sent beside the key, nothing written (the use is not stamped). Verified in the tree: hashApiKey and the sys_api_key authentication read exist nowhere else at head (non-test, across packages/, ui/, examples/), so every door that resolves a key passes this check.
  • Wire answer per door (judged from the cited code, not the PR's list): REST data door (rest-server.ts, anonymous: 'fail-closed') — no principal ⇒ its anonymous 401, byte-identical to an unknown key, WWW-Authenticate included (dogfood pin compares all four fields). Runtime dispatcher, MCP HTTP door included (resolve-execution-context.ts ⇒ resolveRequestExecutionContext, per request) — every domain whose first statement is shouldDenyAnonymous answers the same 401 for both; handleMcpRequest refuses with one message for both. MCP stdio — refused at start ("did not resolve to a valid identity …", one generic sentence) and, for a live transport, on the next call (resolvePrincipal re-resolves per read; initial is reused for localization only). Storage family gate reader — acts only on held; the download door's identity comes from the shared composition, so a refused key resolves nobody there. service-print, service-datasource admin routes, cloud-connection, plugin-sharing — all through resolveAuthzContext, same empty context as an unknown key. Server-side the reason is one warn line (reason, key row id, owner id; never the key or hash).
  • Public surface widening. @objectstack/core (root barrel re-exports ./security/index.js) gains isUserBanInForce, USER_BAN_FIELDS, type UserBanFields; the published union ApiKeyRefusalReason gains the member 'owner_banned' (an exhaustive consumer switch on that type will need the new arm — covered by the yes arm of the declaration). ApiKeyAdmission shape is unchanged.
  • plugin-auth's two existing ban checks now call the shared predicate. principal-session-mint.ts: the flag is read in two further spellings ('1', 'true') and a ban whose expiry equals the instant now still holds (previously lifted at equality) — one millisecond stricter, toward the vendor. agent-forward-identity-view.ts: the flag was read by truthiness and is now the closed set {true, 1, '1', 'true'}; equality at the instant now holds (previously lifted). Both rows arrive through the better-auth adapter as booleans, so the practical behaviour is unchanged; the direction of every change is toward the vendor's own rule. Both call sites keep their own refusal text.
  • Predicate parity with the vendor (read at better-auth 1.7.12, session.create.before): flag set and no expiry ⇒ holds; expiry strictly past ⇒ lifts; expiry unreadable ⇒ holds. The predicate matches on each. Two deliberate differences the PR states correctly: it is read-only (the vendor clears a lapsed ban on the row; here a lapsed ban simply stops binding) and its flag test is a closed set rather than truthiness.

② Semver level

  • Changeset .changeset/banned-user-credentials-refused.md: @objectstack/core: patch, @objectstack/plugin-auth: patch; Clause-②: yes (narrowing) and Metadata-protocol: none at line start in both the changeset and the PR body; **BREAKING** banner; one ADR-0087 marker not-required (no-migration-prescription) with its why. Level follows the Metadata-protocol kind alone: none ⇒ patch on both moved packages — correct. The body prescribes no code rewrite (the remedy is operational: lift the ban or mint a key under another user), so the no-migration-prescription category stands; Check Changeset and the repo/tooling lint lane that carries the ADR-0087 gate are green on this head.
  • Packages moved by the diff: core and plugin-auth only; the dogfood package is private and takes no changeset. No spec change, no generated artefact, no ui/ consumer of the changed exports (git grep of the three new names and of isBanned under ui/ at head: none).
  • ADR-0087 D3 obligations: nothing at rest changes, no authorable key moves, so no conversion or migration-chain entry is owed; the disposition says so in one line. ADR-0090 D10 rule 5 anchor on agent-forward-identity-view.ts (scripts/adr-anchors/…) still holds — the invariant "none for a banned or missing user" is implemented by the shared predicate, and the anchored ADR marker remains in the file header.

③ Boundary flags

PR-body open questions, each answered or escalated:

  • Revoked key answers 403 API_KEY_REVOKED (distinguishable) while the banned owner's key matches the unknown key's 401 — answered: the lifecycle arm is distinct by ruling A on the lifecycle card; the ban is an owner question and takes the generic answer, as the org-less refusal does. Correct ordering (lifecycle first) is read in the tree and pinned.
  • The inactivity window keeps running through a ban — answered in the body; prose flag (should-fix, see ④ item 3): the changeset's "Lifting the ban (or letting it expire) admits the same keys again, with nothing reissued" and authentication.mdx's "restoring … its API keys" are unqualified; a key idle past the inactivity window during a long ban answers as inactive after the unban. One clause fixes both.
  • The ban flag is read whether or not the admin plugin is on — true of the tree (the seam reads the row; nothing conditions on the plugin).
  • Whether a ban also revokes keys — escalated as a maintainer question by the rule itself; the PR correctly does not decide it.
  • A key whose owner row is absent keeps today's answer — stated in the body and pinned; escalated (④ item 2): now that the owner row is read on every admission, admitting a credential whose owner no longer exists is a choice the maintainer should make explicitly.
  • Not raised by the PR, noted here as a boundary (not a finding): a banned user's non-credential execution paths (an automation runAs: 'user' through resolveUserAuthzGrants) are outside "credentials" and untouched; a maintainer question if the ban is meant to hold the user's actions as well as their credentials.

Prose, sentence by sentence against the diff:

  • Changeset title and rule paragraph — true: every key resolution passes the seam; the REST and MCP doors answer the generic 401; a ban with an expiry stops binding on its own (no sign-in needed; the predicate is read-only). "At every door that accepts a key … answered as a key that does not exist" — true on every face whose first statement denies anonymous callers; see ④ item 1 for the one face class where the two answers can differ. "No key row is revoked or rewritten" — true (stampAdmittedKey sits below the refusal; pinned). "Who moves" and "The remedy" — true.
  • authentication.mdx ban-user / unban-user lines — true, subject to the inactivity-window qualification above.
  • administrator-guide.mdx "Ban / Unban (blocks sign-in, ends the user's sessions, and refuses their API keys while the ban holds)" and the offboarding row — true: the admin ban endpoint deletes the user's sessions as the vendor handler does, and the key refusal is read in the tree.
  • Disclosure (security-findings.md §2/§6): PR title, body, commits, changeset, docs and both test files stay at rule level — they state what a ban holds and what each door answers, never how a door could be made to misbehave; test names are rules. The ablation paragraph records a mutation and a red count, not an attack. Meets the rule; nothing in the public text needs withdrawing.

④ Design judgment

The narrowing is judged against the published spec on main and the cited doors: it makes a refusal the spec already implies (a ban "holds every credential") real at the one seam every key passes, without taking any declared capability away from a consumer whose owner is not banned (controls: peer key served, non-banned owner admitted, cookie-only session unchanged). The ordering is right (a dead key answers for its own state first; the ban before tenancy and posture, so a banned owner's key never reaches the held arm or the stamp). Failure modes are the existing ones: an owner read that throws is the branded 503, a store with no sys_user table holds no ban, no auth service means no policy but the row's own facts. Timed bans bind until strictly past the expiry and not after, exactly as sign-in; nothing is cached between admissions (the request-grants memo is scoped to one resolution and sits below admission; the stdio transport re-resolves per call). Findings:

  1. Should-fix — guest-serving faces. On the runtime dispatcher (composed with anonymous: 'guest'), an unknown key with no session is served the guest anchor's grants, while a refused key returns before the guest block and is assembled as a guest entry with no grants. Every domain that opens with shouldDenyAnonymous answers both the same 401, so the pinned doors hold; but a face that admits a guest for some method/path and then reads the guest's grants answers the two keys differently whenever the guest anchor grants anything — the refused key gets less (fail-closed), yet the difference is observable. This is the same shape the existing org-less refusal already has, so it is not a regression; it is the one place where the PR's "answered as a key that does not exist at every door" is narrower than written. Fix in a follow-up: on the refused arm resolve the same guest entry an unknown key resolves, or qualify the claim to anonymous-denying faces.
  2. Should-fix (maintainer question) — absent owner row. The seam now reads the owner row on every admission and still admits a key whose owner row does not exist. Keeping today's answer is a legitimate scope boundary and the PR says so; it should be an explicit decision rather than an inherited default.
  3. Should-fix — one prose clause. Changeset and authentication.mdx: qualify "admits the same keys again" with the inactivity window (a key idle past the window during a long ban answers as inactive after the unban, remedy a new key), which the PR body already states.
  4. Observations, no action: the predicate's flag test is a closed set where the vendor uses truthiness (equivalent for boolean columns; a non-canonical truthy spelling would read as not banned); a refused key's timing class differs from an unknown key's by the owner read and the policy read, as the org-less refusal's already does; isMissingTableError recognises a missing table, not an unregistered object — a kernel registering sys_api_key without sys_user would answer 503 on every key, a composition that does not exist (both ship together in platform-objects) and the same classification tryFind already applies.

No must-fix: the rule holds at every door that resolves a key, the refusal is indistinguishable from an unknown key on every door the PR names and on every anonymous-denying face, the key row is untouched, timed bans lapse as sign-in does, and the public text stays at rule level.

Implemented-by: claude/sec-hardening-r4
Reviewed-by: session_01QSeKYn5CmmdShCJifm6WYj

VERDICT: PASS

…gets

A key refused for its owner or for the tenancy posture now resolves to
the same anonymous entry as a key that does not exist, on every
composition, the guest-serving one included. The restore wording in the
changeset and the auth docs is qualified by each key's inactivity window.

Claude-Session: https://claude.ai/code/session_01QSeKYn5CmmdShCJifm6WYj
Co-authored-by: Claude <noreply@anthropic.com>
…keys are refused while the ban holds

The confirm question and the agent-facing description of the ban action
now say that the user's API keys are refused until the unban and work
again after it. Translations carried in zh-CN, ja-JP and es-ES; the en
bundle regenerated.

Claude-Session: https://claude.ai/code/session_01QSeKYn5CmmdShCJifm6WYj
Co-authored-by: Claude <noreply@anthropic.com>
@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

security#1 · os-sales · session_01L6bhgVfjk2i8T2D4RmxsKg

Contract-review-claim: session_01L6bhgVfjk2i8T2D4RmxsKg · head 1c39291 · 2026-11-05T10:54Z

The security#1 seat changed hands (seat post #29740); this seat re-reviews the current head at CONTRACT_REVIEW_TIER after the platform base sync.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

security#1 · os-sales · session_01L6bhgVfjk2i8T2D4RmxsKg

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: 1c3929139aa8e69c657c0a11d311147b6da25eda
Local-runs: none
Scope: packages/core/src/security/api-key.ts, packages/core/src/security/user-ban.ts, packages/core/src/security/index.ts, packages/core/src/security/resolve-authz-context.ts, packages/core/src/security/api-key-banned-owner.test.ts, packages/plugins/plugin-auth/src/agent-forward-identity-view.ts, packages/plugins/plugin-auth/src/principal-session-mint.ts, packages/platform-objects/src/identity/sys-user.object.ts, packages/platform-objects/src/identity/action-confirm-one-dialog.test.ts, packages/platform-objects/src/apps/translations/en.objects.generated.ts, packages/platform-objects/src/apps/translations/zh-CN.objects.generated.ts, packages/platform-objects/src/apps/translations/ja-JP.objects.generated.ts, packages/platform-objects/src/apps/translations/es-ES.objects.generated.ts, packages/qa/dogfood/test/api-key-banned-owner.dogfood.test.ts + packages/core/src/security/request-execution-context.ts, packages/core/src/security/request-grants-memo.ts, packages/core/src/security/admin-standing-surface.ts, packages/core/src/index.ts, packages/core/package.json, packages/rest/src/rest-server.ts, packages/runtime/src/security/resolve-execution-context.ts, packages/runtime/src/domains/mcp.ts, packages/mcp/src/plugin.ts, packages/services/service-storage/src/storage-service-plugin.ts, packages/services/service-print/src/print-routes.ts, packages/services/service-datasource/src/admin-routes.ts, packages/plugins/plugin-sharing/src/sharing-plugin.ts, packages/cloud-connection/src/cloud-connection-plugin.ts, packages/cloud-connection/src/marketplace-install-local-plugin.ts, packages/types/src/driver-error-classification.ts, packages/plugins/plugin-auth/src/admin-ban-endpoints.ts, packages/plugins/plugin-auth/src/auth-schema-config.ts, packages/plugins/plugin-auth/package.json, packages/platform-objects/package.json, .changeset/config.json, scripts/check-changeset-no-major.mjs, scripts/check-single-authz-resolver.mjs, scripts/adr-anchors/packages__plugins__plugin-auth__src__agent-forward-identity-view.ts.json, scripts/adr-anchors/packages__core__src__security__resolve-authz-context.ts.json, docs/adr/0092-sys-user-profile-field-delegation.md, docs/adr/0127-authorization-cache-invalidation-contract-and-ttl-bound.md. Prose checked sentence by sentence in ③ only: .changeset/banned-user-credentials-refused.md, content/docs/permissions/authentication.mdx, content/docs/permissions/administrator-guide.mdx.

Face: the committed changeset and the PR body both open with Clause-②: yes (narrowing) at line start, so this is the narrowing face at CONTRACT_REVIEW_TIER; the scope floor is every path of the diff outside the prose faces, and no path is a remainder. The head is a platform merge of main (parents f064a0e418 and 09720e2c8a): the diff of the head against its main-side parent is byte-identical to the net diff against the merge-base, so the merge carries nothing of the PR's own beyond the two findings commits already on the branch; the text-merged generated en bundle at the head equals its source string. Read-only throughout: the PR body and comments, the file list, the net diff, the cited code at the head, and the head's check-runs; nothing built, run or re-run. This record is a full review of this head: it re-judges the whole diff and, in ④, the delta the two commits after the prior PASS (15b303e0bf, record 6650521523) introduced.

Check-runs on this head, two readings. First (at the start of the review, after the claim comment of 10:54 UTC): every completed run success or skipped; Build Core, Build Docs, Build UI Deps, Live E2E UI, the Lint and Type Check lanes, Test Core 1 to 8 still in_progress. Second (11:05:28 UTC): success on Lint & Repo Gates, Lint · ESLint, Lint · docs & code guards, Lint · repo & tooling gates, Lint · CI/release/engine gates, Lint · PM dispatch-gates, the three scoped lints, Type Check · source gates / consumer gates / debt ledger, Build Core, Build Docs, Build UI Deps, Build & E2E UI, Live E2E UI, Governed Surface Queue Guard, Check Changeset, Check PR Size, Check Documentation Links, PR-body guards, Spec property liveness, Dogfood Regression Gate, Plan Test Core shards, Test Core (7/8), Auto Label, filter; skipped on the opt-in and matrix placeholders; still in_progress at the last reading: Test Core 1 to 6 and 8, Test UI 1 to 16, Type Check UI, Type Check · workspace. No red at either reading. The conclusions are the gate verdicts; the runs still in progress are not a pass.

① Derived judgments

  • Accept-set narrowing, the rule — right. At the one key-admission seam (resolveApiKeyAdmission, api-key.ts 753 to 792), after the key's own lifecycle verdict and before the org-less and account-posture questions, the owner row is read by id as the system (id, banned, ban_expires) and a key whose owner's ban holds answers on the refused arm (owner_banned): no principal, no fall-through to a session sent beside the key, nothing written (stampAdmittedKey runs only for an admitted verdict). Verified in the tree at the head: outside api-key.ts no non-test source anywhere under packages/, ui/ or examples/ hashes an inbound key or reads sys_api_key to authenticate, so every door that resolves a key passes this check.
  • Wire answer per door — right, judged from the cited code. REST data door (rest-server.ts 3412 to 3421, anonymous: 'fail-closed'): no principal ⇒ undefined context ⇒ shouldDenyAnonymous ⇒ ANONYMOUS_DENY_BODY, the same bytes an unknown key gets (the dogfood pin compares status, code, message and WWW-Authenticate on read and write). Runtime dispatcher and MCP HTTP door (resolve-execution-context.ts 199 to 215, anonymous: 'guest'; domains/mcp.ts 81): a context with no userId ⇒ the same 401 for both keys. MCP stdio (mcp/src/plugin.ts 258): resolveAuthzContext with no guest entry ⇒ no userId ⇒ undefined at start, re-resolved per call. Storage (storage-service-plugin.ts 1566, fail-closed), print, datasource admin routes, sharing and cloud-connection: each calls resolveAuthzContext without a guest entry, so a refused key resolves the empty envelope an unknown key resolves. Server side the reason is one warn line carrying the row id, the owner id and the reason, never the key or its hash (warnApiKeyRefusal, 299 to 312).
  • One anonymous entry (commit 12c342a) — right. anonymousEntry (resolve-authz-context.ts 525 to 536) is the one helper both exits call: the refused arm (595) and the no-principal branch (649). On a face that takes the guest entry (request-execution-context.ts 259: only the composition passes it, only for anonymous: 'guest') a refused key is now served the guest anchor's grants exactly as an unknown key is; elsewhere both get nothing. This also moves the existing org-less refusal (organization_required) on a guest face from nothing to the guest's grants; that is the rule (what a request with no credential gets) and the changeset says so. Nothing is pushed into the context before the refused return (scopes are seeded only for an admitted or held principal), so the two envelopes are equal, as pinned with a guest set that really grants something.
  • Public surface widening — right, declared. @objectstack/core root (src/index.ts 73 re-exports security/index.js) gains isUserBanInForce, USER_BAN_FIELDS and the type UserBanFields; the published union ApiKeyRefusalReason gains 'owner_banned'. Every consumer of that union at the head reads it as a type or logs it (warnApiKeyRefusal); no exhaustive switch over it exists outside core, and ui/ names none of the four. ApiKeyAdmission's shape is unchanged (organizationId is optional on the refused arm, so the new return is well-typed).
  • plugin-auth's two existing ban checks now call the shared predicate — right. principal-session-mint.ts 266 and agent-forward-identity-view.ts 170: each keeps its own refusal text. The direction of every behavioural delta is toward the vendor's own rule: a ban whose expiry equals the instant now still holds (both sites previously lifted at equality), and the agent-forward site's flag test moves from truthiness to the closed set. Both rows arrive through the better-auth adapter as booleans (auth-schema-config.ts maps banExpires to ban_expires), so practical behaviour is unchanged. @objectstack/core is already a workspace:* dependency of plugin-auth and core depends on no plugin, so no cycle.
  • Published metadata moved (commit f064a0e) — right as text, see ② for the changeset. sys_user.ban_user's confirmation description and its agent-facing ai.description now say the user's API keys are refused until the unban and work again after it; zh-CN, ja-JP and es-ES carried by hand in the translated bundles (the hand-written half of those files), the en bundle regenerated and byte-equal to the source at the head; the #7309 pin updated and asserting the new clause.
  • Failure modes — right. An owner read that throws is AuthzStoreUnavailableError (503), as a sys_api_key read that throws already is; a sys_user table never provisioned (isMissingTableError(err, 'sys_user'), the one classifier the 2026-08-30 ruling allows) holds no ban; a kernel with no auth service judges the row's own facts. The read sits in front of nothing that caches it: the request-grants memo serves only resolveUserAuthzGrants, and the ban endpoints write through the engine.
  • Ordering — right. Lifecycle, then ban, then org-less, then account posture: a dead key answers for its own state first (pinned), and a banned owner whose account is also held answers as anonymous, not as held, which is the rule's "every credential" reading.

② Semver level

  • Changeset .changeset/banned-user-credentials-refused.md: @objectstack/core: patch, @objectstack/plugin-auth: patch; Clause-②: yes (narrowing) and Metadata-protocol: none at line start in both the changeset and the PR body; **BREAKING** banner; exactly one ADR-0087 marker, not-required (no-migration-prescription), with its why. The level follows the kind alone: none ⇒ patch — right, and Check Changeset is green on this head. The body prescribes an operational remedy (lift the ban, or a key under another user), so the category stands; nothing at rest changes and no authorable key moves, so no ADR-0087 D3 entry is owed. Skills-docs: line present and true (the two docs pages and the dogfood pin are in the diff).
  • Must-fix — a published package the diff moves is missing from the changeset. Commit f064a0e moved published source in @objectstack/platform-objects (src/identity/sys-user.object.ts and the four translations/*.objects.generated.ts bundles; package.json version 18.3.0, not private, access: public), and the changeset still names only core and plugin-auth. The repo's rule is explicit: every touched package without private: true is published, its changeset entry is checked, and a missing one does not enter the queue (review-checklist.md 范围与 changeset; AGENTS.md Post-Task Checklist 3: a fix in a released package takes a patch changeset, never none). Because every package versions in lockstep (.changeset/config.json fixed group, which holds all three), the published level is unaffected; what is missing is that package's own CHANGELOG.md line, the text an upgrader greps. Remedy: add '@objectstack/platform-objects': patch to the changeset frontmatter. The prior record's ② ("core and plugin-auth only") was true of its head and is not true of this one.
  • The dogfood package is private and takes no changeset. No spec change, no generated spec artefact, no ui/ consumer of the changed exports.

③ Boundary flags

PR-body acceptance notes and open questions, each answered or escalated:

  • A revoked key answers 403 API_KEY_REVOKED while the banned owner's key matches the made-up key's 401 — answered: the lifecycle arm is located by ruling; the ban is an owner question and takes the anonymous answer, as the org-less refusal does. The ordering is read in the tree and pinned.
  • The inactivity window keeps running through a ban — answered, and the prose the prior record flagged is now qualified in both the changeset and authentication.mdx (commit 12c342a). Resolved.
  • The flag is read whether or not the admin plugin is on — true of the tree: the seam reads the row; nothing conditions on the plugin.
  • A key whose owner row is absent keeps today's answer — stated and pinned; escalated again (should-fix). The prior record asked that this be an explicit maintainer decision now that the owner row is read on every admission; the PR's 维护者速读 says nothing is needed from the maintainer, so the question is not carried to them. Carry it in one line, or decide it in the PR and say so.
  • Not raised by the PR, noted as a boundary: a banned user's non-credential execution paths (an automation run as the user through resolveUserAuthzGrants) are outside "credentials" and untouched. A maintainer question whether the ban is meant to hold the user's actions as well as their credentials; not this PR's scope.
  • The PR body's Verification names head f064a0e418; the merge head's evidence is its own check-runs, read above. The merge brought nothing of the PR's own (the two net diffs are byte-identical) and the i18n and docs lanes are green on it. Observation, no action.
  • Author is objectstack-fleet[bot]; no login note is owed.
  • Vendor parity: the better-auth sign-in hook (1.7.12) was not re-read in this session (not installed in this checkout; a cross-repository read is refused for the session). Parity is judged from the predicate's stated rules, from plugin-auth's admin-ban-endpoints.ts which mirrors the vendor handler and states the vendor hook refuses sign-in, and from the dogfood controls (sign-in 403 BANNED_USER while the key is refused; a timed ban lapsing with no sign-in in between). The prior record read the vendor source and found parity.

Prose, sentence by sentence against the diff:

  • Changeset: the title and Clause-② sentence (every key resolution passes the seam; REST and MCP answer the generic 401; the widening half names the three exports) — true. Metadata-protocol: none — true. ADR-0087 marker — true. "The rule" (a ban with an expiry stops binding on its own; no sign-in needed) — true, the predicate is read-only and the dogfood pin lapses a timed ban with no sign-in. "Not a revocation" with the inactivity-window clause — true. "One anonymous entry" — true, anonymousEntry on both exits, every composition. "Who moves" and "The remedy" — true.
  • authentication.mdx ban-user and unban-user lines — true; the unban line's inactivity qualification matches the seam.
  • administrator-guide.mdx "Ban / Unban (blocks sign-in, ends the user's sessions, and refuses their API keys while the ban holds)" — true (admin-ban-endpoints.ts deletes the user's sessions as the vendor handler does; the key refusal is read in the tree). Offboarding row — true.
  • Two unchanged sentences now narrower than the rule, not false: sys_user.unban_user's agent-facing description ("so they can sign in again") and the banned field's description ("the user cannot sign in"). Observation only.
  • Disclosure (security-findings.md §2/§6): the PR title, body, commits, changeset, docs and every test name state what a ban holds and what each door answers, never how a door could be made to misbehave; the ablation paragraph records a mutation and a red count. Rule level throughout; nothing in the public text needs withdrawing. Anything this reviewer judged beyond the bytes of the answer stays in the reviewer's local note.

④ Design judgment

The narrowing is judged against the published spec on main and the cited doors: it makes real a refusal the spec already implies (a ban holds every credential of the user; sign-in and sessions were already refused) at the one seam every key passes, without taking any declared capability from a consumer whose owner is not banned (controls pinned: the peer's key served, a non-banned owner admitted, a cookie-only session unchanged). Timed bans bind until strictly past the expiry and not after, exactly as sign-in. Nothing is cached between admissions. Per file:

  • api-key.ts — holds. Probed: the fields read is the shape tryFind already uses; banned (boolean) and ban_expires (datetime) are declared unconditionally on sys_user in platform-objects, so the selected columns exist on every composition that ships it; the read is as the system, so no wall or field permission alters the row; the throw and missing-table arms match the sys_api_key read's; nothing is written on the refused arm.
  • user-ban.ts — holds. Flag set and no expiry ⇒ holds; expiry strictly past ⇒ lifts; '', null, undefined ⇒ no expiry; an unreadable expiry does not lift it; Date, string and number accepted. Observation: the flag test is a closed set where the vendor reads truthiness; equivalent for the boolean column, and the engine coerces the column, so only a direct row write of a non-canonical spelling could differ.
  • index.ts — holds; the three names reach the root barrel.
  • resolve-authz-context.ts — holds. The held and lifecycle arms are untouched; the warn line precedes the refused return; the ADR-0068/0095 anchor on this file (no enforcement-time read of the vendor role; platform-admin derived, never stored) is unaffected. check:authz-resolver keys on a file that reads every grant table, which api-key.ts does not, and the lane carrying it is green.
  • agent-forward-identity-view.ts, principal-session-mint.ts — hold. The ADR-0090 D10 rule 5 anchor ("none for a banned or missing user") is implemented by the shared predicate and its marker remains in the header.
  • sys-user.object.ts, action-confirm-one-dialog.test.ts, the four bundles — hold as text (see ② for the changeset).
  • api-key-banned-owner.test.ts (18) — holds; every pin is non-vacuous (the made-up key is refused on the same door; the guest set grants something). api-key-banned-owner.dogfood.test.ts (4) — holds; it asserts status, code, message and WWW-Authenticate against the made-up key on read, write and MCP initialize, reads the store after the refused write, and restores the env knob it sets. Should-fix: the file's wall (a showcase boot and a 3.5 s wait) is over the 10 s line and it declares no // test-budget: line, which the sibling dogfood files over that line carry on line 2 with their measured wall; declare it, or the lane files the card the rule names.

The commits after 15b303e0bf: item 1 (guest-serving faces) resolved by 12c342a, read in the tree and pinned; item 3 (the prose clause) resolved by 12c342a in both places; item 2 (absent owner row) is a maintainer question and is not carried to the maintainer by the PR, re-escalated in ③. f064a0e is correct as a change and introduced the ② must-fix.

Findings: must-fix — ② the changeset omits @objectstack/platform-objects, a published package the diff moves. Should-fix — ③ carry the absent-owner question to the maintainer; ④ declare the dogfood file's test budget. The rule itself holds at every door that resolves a key, the refusal is the anonymous answer on every composition, the key row is untouched, timed bans lapse as sign-in does, and the public text stays at rule level.

Implemented-by: claude/sec-hardening-r4
Reviewed-by: session_01L6bhgVfjk2i8T2D4RmxsKg

VERDICT: FAIL

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant