Skip to content

Commit 374d9d3

Browse files
claude[bot]claude
andauthored
fix(plugin-auth)!: an anonymous get-session is refused with the declared 401 envelope, not answered 200 null (#17881)
Fixes #17238 Ruled B by the director seat (decision batch #117 item 4, comment 5642584682). Maintainer, verbatim: 「17238 B」. The seat had recommended A and was corrected, verbatim: 「我改推荐的都是从平台长远合理性角度考虑的」, under the charter rule 「spec 与代码不一致默认改代码,改协议单独立卡非选项」. `GET /api/v1/auth/get-session` now answers an anonymous caller the platform's standard ADR-0112 failure envelope at HTTP 401, instead of `200` + the literal JSON `null`. `SessionResponseSchema` is untouched. ## The defect, and why the code is what moves `ObjectStackClient.auth.me()` declares `Promise<SessionResponse>`, and `SessionResponseSchema` requires `data.session` and `data.user` — so no value of that type means "nobody is signed in", and the most ordinary call a logged-out caller can make resolved to something *outside* the method's own declared type. ⇒ Every value `auth.me()` **returns** is now inside `SessionResponse`. The anonymous case is delivered as a rejection instead: the SDK's `fetch` wrapper already throws on every non-2xx, so the caller gets an error carrying `code: 'UNAUTHENTICATED'` and `httpStatus: 401`. **No runtime edit in `packages/client` was needed** — that fell out of the measurement rather than being assumed. ## Measured, on the merge base and after Driven through a real `AuthManager` (better-auth, default plugin set, in-memory engine) via `handleRequest`: ``` BEFORE AFTER GET /get-session (no credential) 200 application/json null 401 {"success":false,"error":{"code":"UNAUTHENTICATED","message":"Sign in first"}} GET /get-session (unknown cookie) 200 application/json null 401 (same) GET /get-session (signed in) 200 {user,session} 200 {user,session} ← unchanged, asserted by identity POST /get-session (no credential) 404 (no content-type) '' 404 (unchanged) ``` The `POST` row is why the rule is keyed on *the 200-null answer* and not on the path alone: a path-only rule would have converted that 404 into a 401 and claimed a route better-auth does not serve. ## Where it lives `packages/plugins/plugin-auth/src/anonymous-session-refusal.ts`, wired into `AuthManager.handleRequest` beside the existing `/admin/` envelope normalization — the one seam every vendor route passes through. ⚠️ It differs from that sibling in exactly one way, stated in both files: the `/admin/` seam is forbidden to move admission, and this one **is** the admission move. Three narrowings hold everything else still, each pinned by a `toBe` identity assertion: 1. **The `/get-session` endpoint exactly** — not a prefix, not the session family. 2. **Status 200 only** — anything better-auth already refuses, redirects or fails reaches the caller untouched. 3. **A body that is exactly the literal `null`** — compared as *text*, so a body that merely parses to something falsy (`0`, `""`, `false`, `{}`, `[]`) is left alone. ⛔ better-auth's JS API is untouched. `auth.api.getSession()` still returns `null` for an anonymous caller, so execution-context resolution, the platform-admin gates and the SSO bridges behave exactly as before. Only the wire moves. There is a negative-control test for precisely this. **No error code is minted.** `UNAUTHENTICATED` is an existing `StandardErrorCode` member (`packages/spec/src/api/errors.zod.ts:69`), derived from the status through ADR-0112's own `standardErrorCodeForHttpStatus` map — so there is no string literal here to drift, `ERROR_CODE_LEDGER` is unchanged, and **`packages/spec` is not touched by this PR**. ## Client consequences, enumerated (ruling note 3) Note 3 requires these be listed here rather than discovered downstream. The two readings behind the list are posted on the card (comment 5647024737). | consumer | effect | |---|---| | `ObjectStackClient.auth.me()` | **rejects** for an anonymous caller instead of resolving `null`. No source change — `this.fetch` already throws on non-2xx | | `packages/client/src/auth-get-session-envelope.test.ts` block ③ | **flipped, reversal named in place.** It drives the real server, so it went red the moment the 401 landed | | `packages/client/src/client.test.ts` | **re-pointed by hand.** ⚠️ Fetch-mock driven, so it would have stayed **green** while pinning a wire shape no server produces any more — the more dangerous of the two, because it announces nothing | | `packages/cli/src/commands/whoami.ts` | behaviour improves with no code change: a stored-but-invalid token now prints the refusal and exits 1 instead of printing an empty `Current Session`. `requireAuth(token)` already exits earlier when no token is stored | | `packages/plugins/plugin-auth/src/register-sso-provider.ts` | **unaffected.** It guards `if (!resp.ok) return undefined;` — before: `ok`, body `null`, org undefined; after: not `ok`. Same value, best-effort by its own docblock | | console / `../objectui` | no consumer found; nothing branches on the `null` body | `packages/client` was originally fenced out of this card as `domain:cli`'s serial hot file. The dispatching seat lifted the fence for **those two test files only**, having measured that no open PR holds any `packages/client` path and that this PR cannot go green without the block ③ flip. ⛔ No runtime edit was made in that package. ## Ablation — the negative pins are proven able to fail This lane requires every negative pin carry one. The old `200 null` was put back by neutering the guard in `anonymous-session-refusal.ts`, **plugin-auth was rebuilt**, and the marker was proven live in the artifact the client consumes before anything was read: ``` SRC ablation added : 1 (expect 1) SRC blob b7e8f76e2915cae2581a158384822cd8367bcfd3 != HEAD ba3aeb8 -> MUTATION ON DISK ✓ dist/: marker present in 2 built files -- the ablation is live in the artifact the suite consumes. ``` ⚠️ The rebuild is load-bearing and the first attempt caught me: `packages/client` resolves `@objectstack/plugin-auth` through `dist`, so a source-only mutation would have left block ③ **green** and certified a test that could never fail. A second trap the preflight caught: the first marker was written inside a `/* */` comment, which tsup strips — it never reached `dist` at all. Ablated, server suite (4 of 11 red): ``` FAIL src/anonymous-session-refusal.test.ts > ⭐ answers 401 AND the registered code — both, not either AssertionError: expected 200 to be 401 FAIL src/anonymous-session-refusal.test.ts > the refusal body is a conforming ADR-0112 envelope, exhaustively AssertionError: expected [ Array(1) ] to deeply equal [] + [ "body is null, not an envelope object" ] ``` Ablated, client block ③ (2 red) — **and this is the card's own loop closing**: ``` FAIL src/auth-get-session-envelope.test.ts > ③ anonymous is REFUSED > rejects with the declared refusal instead of resolving outside its type AssertionError: promise resolved "null" instead of rejecting ``` The card records that block ③ *"is green under [the ablation] and CANNOT redden on it … it pins the residue, not the fix."* It reddens now. That is the difference between a pin that records a gap and a pin that protects a fix. Restored, rebuilt, and re-run green: `✓ dist/: marker absent from all 12 built files`, `✓ tree: working tree clean against HEAD`, 11/11 and 223/223. ## Verification All at `b8c2b00f8`, the final commit. | run | result | |---|---| | `plugin-auth` full suite | **2283 passed (108 files)** | | `plugin-auth` typecheck (incl. `check:test-typecheck`) | **pass** — test layer compiles | | `client` — the 5 affected files | **250 passed** | | `dispatch-gates --commands` → all run → `--ran` | **70 derived, 70 run, 0 NOT-MEASURED, 0 UNRUN**, all exit 0 (a *derived* zero: every command recorded its exit code) | | eslint, narrowed | 11 of 6657 files, **0 errors / 0 warnings** | The eslint narrowing is a measurement, not a skip: ① the population (6657) is eslint's own enumeration, ② the 11 is the `--format json` count, ③ this repo runs one `eslint.config.mjs` which never enables type-aware linting for any file (its own header, line 328, measured there with a positive control) ⇒ no verdict on an untouched file can have moved. One test outside the new file needed re-pointing: `auth-manager.optional-plugin-isolation.test.ts`'s *"get-session also survives a degraded optional plugin"* used the anonymous `200` as its vehicle for "the route reached better-auth and came back". The claim is unchanged; the assertion now pins `401` **and** `error.code`, plus the recorded degradation, so it cannot pass on a 401 produced for some other reason. ## Changeset `minor`, declared **BREAKING**, with the observable behaviour change stated in as many words (ruling note 6). Its ADR-0087 disposition is `not-required (no-migration-prescription)`: this retires no metadata surface, so `objectstack migrate meta` has nothing to rewrite and no ledger entry can be written for it — what moves is an HTTP status plus an SDK method's promise contract. The same disposition and reasoning a previous HTTP-status unification used. ⚠️ The first draft carried a `FROM -> TO` table, which that gate reads as a migration prescription — correctly, by its own rules — and a prescription refuses this category. The consumer guidance is kept, restated as prose plus a `try`/`catch` example. ⛔ The bang was **not** dropped to get around it; the gate names that move as the one it exists to stop. ## Acceptance notes Noted, not filed — all three are the same class (a pin or a docblock describing a wire shape the server no longer serves), and none is in this card's face: - `packages/client/src/organization-get-active-member-addressing.test.ts` (lines 40, 88, 120) models the anonymous answer as `200 null` in a socket-level double. It stays **green** — the double serves itself — so it is stale rather than broken. Same hazard as `client.test.ts`, which was in scope and fixed; this one is a third `packages/client` file and outside the two-file fence lift. - `packages/client/src/index.ts` docblocks at `:1474` and `:4389` still describe the anonymous answer as `200 null`. ⛔ Deliberately untouched: `index.ts` is the runtime file the fence lift explicitly excluded, and #14313 owns published-annotation moves on it. - Sibling **#17234** is untouched. Ruling note 5 offered it as a possible round-mate, but its live state is `needs-user-decision` + `domain:cli` — in the decision box and another lane's. Two stale card references found while re-verifying the ruling's premises: **#14313** (named in the issue body as the `auth.*` family card) returns **HTTP 404** — it does not exist in this repository; control cards read with the same credential in the same batch resolved normally. And #17234 as above. ## Scope declaration `Clause-②: no`, and the delivery is consistent with it: the accept set narrows (a 401 replaces a 200) rather than widening, `UNAUTHENTICATED` is reused rather than minted, `packages/spec` is not touched, and the new module is package-internal — it is not re-exported from `index.ts` and appears zero times in the published `.d.ts`. ⚠️ **This PR is not independently landable and is deliberately left as a draft.** Block ③ drives the real server, so server and client halves have to land together; they are in this one PR for that reason. --- 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj --- _Generated by [Claude Code](https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c744c0a commit 374d9d3

13 files changed

Lines changed: 667 additions & 43 deletions
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
---
2+
'@objectstack/plugin-auth': minor
3+
---
4+
5+
**BREAKING** — `GET /api/v1/auth/get-session` answers an anonymous caller with the
6+
declared ADR-0112 failure envelope and HTTP 401, instead of HTTP 200 wrapping a JSON `null`.
7+
8+
Until now an unauthenticated session read answered:
9+
10+
```
11+
HTTP 200
12+
null
13+
```
14+
15+
`ObjectStackClient.auth.me()` declares `Promise<SessionResponse>`, and
16+
`SessionResponseSchema` requires `data.session` and `data.user` — so no value of that type
17+
means "nobody is signed in", and the most ordinary call a logged-out caller can make
18+
resolved to something outside the method's own declared type. Ruled by the director seat
19+
(decision batch #117 item 4) under the charter rule
20+
「spec 与代码不一致默认改代码,改协议单独立卡非选项」: the implementation is corrected to
21+
the published contract. `SessionResponseSchema` is untouched.
22+
23+
What changes on the wire:
24+
25+
- **An anonymous or unresolvable credential ⇒ `401` with `error.code: 'UNAUTHENTICATED'`**
26+
and the message `Sign in first`, the same body a raw `/admin/` mount already answers the
27+
same caller with. No error code is minted: `UNAUTHENTICATED` is an existing
28+
`StandardErrorCode` member, derived from the status through ADR-0112's own map, so
29+
`ERROR_CODE_LEDGER` is unchanged.
30+
- **Unchanged:** a signed-in read still answers `200` with `{ user, session }`,
31+
byte-identical. Every other `/auth/*` route is untouched, and so is the `404` that a
32+
method this route does not serve already answered — this change never invents a route.
33+
- **Also unchanged:** better-auth's JS API. `auth.api.getSession()` still returns `null` for
34+
an anonymous caller, so every internal identity read — execution-context resolution, the
35+
platform-admin gates, the SSO bridges — behaves exactly as before. Only the wire moves.
36+
37+
**`@objectstack/client`:** `client.auth.me()` now **rejects** for an anonymous caller
38+
instead of resolving with `null` — the SDK throws on every non-2xx before unwrapping. Every
39+
value the method resolves with is now inside its declared `SessionResponse`. Callers that
40+
inspected the resolved value must move to a `catch`:
41+
42+
```ts
43+
try {
44+
const session = await client.auth.me();
45+
// …signed in
46+
} catch (err: any) {
47+
if (err.code === 'UNAUTHENTICATED') {
48+
// …signed out; err.httpStatus is 401
49+
}
50+
}
51+
```
52+
53+
A caller that branches on the HTTP status directly reads `401` plus
54+
`error.code: 'UNAUTHENTICATED'` where it used to read `200` plus an empty body.
55+
56+
<!-- adr-0087: not-required (no-migration-prescription) retires no metadata surface: no Zod schema, no authorable key, no export, no config field, and no stored sys_metadata row changes shape, so `objectstack migrate meta` has nothing to rewrite and no ledger entry can be written for it. What changes is an HTTP status plus an SDK method's promise contract, and the only channel that reaches those consumers is this changeset itself. -->

‎packages/client/src/auth-get-session-envelope.test.ts‎

Lines changed: 45 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,13 @@
2929
// - `② the raw keys survive` — `.user` is what the field reads today, while
3030
// the declared `.data.user` was `undefined`. The fix must not buy the
3131
// declared shape by breaking the workaround callers were pushed onto.
32-
// - `③ anonymous stays anonymous` — the route serves the literal `null` at
33-
// 200. Pinned as the KNOWN residue: it is still outside `SessionResponse`,
34-
// and this case exists so that stays a measured fact rather than a surprise.
32+
// - `③ anonymous is REFUSED` — REVERSED by #17238. This block used to pin the
33+
// route serving the literal `null` at 200, as the known residue #16760 could
34+
// not close. The residue is now closed at the producer: `/get-session`
35+
// answers an anonymous caller the declared ADR-0112 envelope at 401, so
36+
// `me()` REJECTS and every value it resolves with is inside
37+
// `SessionResponse`. The case is kept, not deleted, because a reversed pin is
38+
// the record that the gap was closed deliberately rather than drifting shut.
3539
// - `④ refreshToken captures a credential that actually works` — the card's
3640
// second consequence, and the one that was NOT a consequence of the envelope
3741
// at all. The firing control is the credential's SPELLING: the client starts
@@ -240,16 +244,43 @@ describe('[#16760] /get-session is lifted into the SessionResponse envelope it d
240244
});
241245
});
242246

243-
describe('③ anonymous stays anonymous — the known residue', () => {
244-
it('answers the literal null rather than a signed-in-looking envelope', async () => {
247+
describe('③ anonymous is REFUSED — the residue #16760 measured, now closed', () => {
248+
// ⚠️ THE REVERSAL, named. Until #17238 this block asserted `res` was the
249+
// literal `null` — and its own comment said it pinned the RESIDUE, not a
250+
// fix: it was green with the lift and without it, so it could never redden
251+
// on the lift's ablation.
252+
//
253+
// What changed is the PRODUCER, not the lift. There is still no
254+
// `SessionResponse` value meaning "nobody is signed in"; the server stopped
255+
// needing one by refusing instead of answering. Ruled by the director seat
256+
// (batch #117 item 4) under 「spec 与代码不一致默认改代码」.
257+
//
258+
// ⛔ What the SDK must still never do is manufacture `{ success: true,
259+
// data: {} }` here — an empty session that reads as a real one. A rejection
260+
// is the opposite of that, and this case is what says so.
261+
it('rejects with the declared refusal instead of resolving outside its type', async () => {
245262
const { client } = await anonymous();
246-
const res = await client.auth.me();
247-
// ⚠️ Still outside `SessionResponse`, deliberately: there is no value of
248-
// that type meaning "nobody is signed in", and widening the published
249-
// return annotation is a contract-review change, not this card's. What
250-
// the lift must never do is manufacture `{ success: true, data: {} }`
251-
// here — an empty session that reads as a real one.
252-
expect(res).toBeNull();
263+
264+
// BOTH halves: the status is what stops a caller reading the answer as a
265+
// session, the code is what it may branch on.
266+
await expect(client.auth.me()).rejects.toMatchObject({
267+
code: 'UNAUTHENTICATED',
268+
httpStatus: 401,
269+
});
270+
});
271+
272+
it('and the refusal is the only way out — nothing resolves to a falsy session', async () => {
273+
// The control for the reversal: a `me()` that silently started resolving
274+
// `null`/`undefined` again would satisfy no assertion above, and this is
275+
// what turns that into a failure rather than a gap.
276+
const { client } = await anonymous();
277+
278+
const settled = await client.auth
279+
.me()
280+
.then((value) => ({ outcome: 'resolved' as const, value }))
281+
.catch((error) => ({ outcome: 'rejected' as const, value: error }));
282+
283+
expect(settled.outcome).toBe('rejected');
253284
});
254285
});
255286

@@ -264,8 +295,8 @@ describe('[#16760] /get-session is lifted into the SessionResponse envelope it d
264295
// that captures nothing leaves it exactly where it started.
265296
//
266297
// ⛔ Not "seed a deliberately wrong token": that unauthenticates the
267-
// client, `/get-session` then answers `null` for the anonymous reason,
268-
// and the case would fail against a CORRECT implementation.
298+
// client, `/get-session` then REFUSES for the anonymous reason (401 since
299+
// #17238), and the case would fail against a CORRECT implementation.
269300
expect(signed, 'the two spellings coincide — this control cannot fire').not.toBe(unsigned);
270301
const before = storedToken(client);
271302
expect(before).toBe(signed);

‎packages/client/src/client.test.ts‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -877,11 +877,26 @@ describe('Auth enhancements', () => {
877877
expect((client as any).token).toBeUndefined();
878878
});
879879

880-
// [#16760] The anonymous answer is the literal `null` at 200. The lift must
881-
// pass it through rather than manufacture a signed-in-looking envelope.
882-
it('me() passes the anonymous null through untouched', async () => {
883-
const { client } = createMockClient(null);
884-
expect(await client.auth.me()).toBeNull();
880+
// [#17238, reversing #16760] The anonymous answer WAS the literal `null` at
881+
// 200, and this case pinned the SDK passing it through. The server no longer
882+
// serves that: `/get-session` refuses an anonymous caller with the declared
883+
// ADR-0112 envelope at 401, so `me()` rejects and every value it RESOLVES
884+
// with is inside its declared `SessionResponse`.
885+
//
886+
// ⚠️ This case is fetch-MOCK driven, which is why the reversal had to be
887+
// made by hand. Left alone it would have stayed GREEN — pinning a wire shape
888+
// no server produces any more, against a mock that keeps producing it. Its
889+
// real-server twin (`auth-get-session-envelope.test.ts` block ③) went red on
890+
// the same change and announced itself; this one could not.
891+
it('me() rejects the anonymous refusal rather than resolving outside its type', async () => {
892+
const { client } = createMockClient(
893+
{ success: false, error: { code: 'UNAUTHENTICATED', message: 'Sign in first' } },
894+
401,
895+
);
896+
await expect(client.auth.me()).rejects.toMatchObject({
897+
code: 'UNAUTHENTICATED',
898+
httpStatus: 401,
899+
});
885900
});
886901

887902
it('signInWithProvider defaults callbackURL to the current page (base-path-correct)', async () => {

‎packages/plugins/plugin-auth/src/admin-revoke-user-session-match-guard.test.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -166,9 +166,14 @@ const cookieFrom = (response: Response): string =>
166166
.join('; ');
167167

168168
/**
169-
* Is this cookie still authenticated? better-auth answers `/get-session` with
170-
* HTTP 200 and a JSON `null` body when the session is gone — NOT a 401 — so a
171-
* status-only assertion would pass against a fully revoked session.
169+
* Is this cookie still authenticated?
170+
*
171+
* Reads the BODY, and that is deliberate: `/get-session` answers a live
172+
* session `200` with `{ user, session }` and an unauthenticated caller `401`
173+
* with the ADR-0112 refusal envelope (#17238), so both legs below are real.
174+
* ⛔ Do not reduce this to a status check — the point of the helper is that a
175+
* session which is gone is proven gone by the absence of a user, not by a
176+
* status this file would then be trusting a single seam to keep emitting.
172177
*/
173178
const isAuthenticated = async (manager: AuthManager, cookie: string): Promise<boolean> => {
174179
const res = await getSession(manager, cookie);

0 commit comments

Comments
 (0)