Skip to content

Commit d9353b9

Browse files
os-warrenclaude
andauthored
fix(plugin-auth): verify-domain answers DOMAIN_VERIFICATION_DISABLED for the disabled condition (#11004)
Both SSO domain-verification bridge routes recognise the same shape - the inner @better-auth/sso endpoint is unmounted for this env and answers 404 with no code - and answered it differently. runRequestDomainVerification returns 400 + DOMAIN_VERIFICATION_DISABLED; runVerifyDomain rewrote only the message and let the code fall through to the generic failure default, so the response carried "the feature is off" copy under a code meaning "verification failed". verify-domain now returns the sibling's answer for that condition: 400 + DOMAIN_VERIFICATION_DISABLED. The status moves with the code because the inner 404 describes the INNER endpoint, while this bridge route is mounted unconditionally. The rewrite stays keyed to the disabled shape (404 WITHOUT a code): a genuine failure still answers DOMAIN_VERIFICATION_FAILED, and the vendor pass-through arm is untouched on both routes. Both directions are pinned. Part of #10859 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx Co-authored-by: Claude <noreply@anthropic.com>
1 parent 15ea214 commit d9353b9

4 files changed

Lines changed: 159 additions & 8 deletions

File tree

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+
`POST /admin/sso/verify-domain` now answers the DISABLED condition the way its
6+
sibling always has. When SSO domain verification is off for an environment,
7+
`@better-auth/sso` never mounts the inner endpoint and answers `404` with no
8+
code. Both bridge routes recognise that shape, and they used to answer it
9+
differently (#10859):
10+
11+
| route | answered | answers instead |
12+
| --- | --- | --- |
13+
| `POST /admin/sso/request-domain-verification` | `400` `DOMAIN_VERIFICATION_DISABLED` | unchanged |
14+
| `POST /admin/sso/verify-domain` | `404` `DOMAIN_VERIFICATION_FAILED` | `400` `DOMAIN_VERIFICATION_DISABLED` |
15+
16+
`verify-domain` rewrote only the `message` for that branch and let the code fall
17+
through to its generic failure default, so the response carried "the feature is
18+
off" copy under a code that means "verification failed". A caller can only act
19+
on the machine-readable half, and the two halves disagreed. The status moves
20+
with the code: the inner `404` describes the INNER endpoint, which is unmounted,
21+
whereas this bridge route is mounted unconditionally — passing that status
22+
through said "no such endpoint" about a resource that exists.
23+
24+
If you match on `DOMAIN_VERIFICATION_FAILED` (or on `404`) to detect the
25+
disabled case on `verify-domain`, match on `DOMAIN_VERIFICATION_DISABLED` (or on
26+
`400`) instead — the same pair `request-domain-verification` has always
27+
answered. The distinction is worth having: `DISABLED` means "turn on
28+
`OS_SSO_DOMAIN_VERIFICATION`", `FAILED` means "the DNS TXT record is not visible
29+
yet, retry".
30+
31+
**No `packages/spec` change, and the emitted vocabulary gains no member.** Both
32+
codes are already registered for `@objectstack/plugin-auth` in the error-code
33+
ledger, with exactly these meanings (`DOMAIN_VERIFICATION_DISABLED` — "domain
34+
verification is off on this deployment"). This route was emitting a *declared*
35+
code whose registered meaning is a different condition, so this is
36+
declared-vs-enforced restoration rather than a new contract decision.
37+
38+
**A genuine verification failure still answers the failure code, and the vendor
39+
pass-through arm is untouched on both routes.** The rewrite is keyed to the
40+
disabled shape specifically — `404` *without* a code. A `404` that carries
41+
`@better-auth/sso`'s own code is the vendor's diagnosis and reaches the caller
42+
verbatim, status included, as does every non-404 failure. That direction is the
43+
load-bearing one — an implementation keyed to "any 404", or to `!resp.ok`, would
44+
satisfy the disabled case while destroying the diagnosis a caller acts on — and
45+
it is pinned in both directions in
46+
`packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts`.
47+
48+
Shipped as `minor`, following the same call the casing rename on these two
49+
routes made (#10716). The argument for it: the vocabulary is unchanged, and the
50+
old pairing was self-contradictory rather than a contract anyone could have
51+
relied on deliberately. The argument against it, stated here rather than
52+
settled: unlike that rename — whose old spellings were undeclared values no
53+
schema admitted — `DOMAIN_VERIFICATION_FAILED` *is* a declared, registered code,
54+
so a client keyed to it for this case was keyed to something the published
55+
contract admitted, and both halves of the answer change. A reviewer who reads
56+
that as `major` is not reading it wrong; this PR does not decide it silently.

‎packages/plugins/plugin-auth/src/register-sso-provider.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -457,11 +457,21 @@ export async function runVerifyDomain(
457457
if (resp.ok) {
458458
return { status: 200, body: { success: true, data: { providerId, verified: true, message: 'Domain ownership verified — this provider can now sign users in.' } } };
459459
}
460+
// The feature is OFF for this env: `@better-auth/sso` never mounted the inner
461+
// endpoint, so its `404` describes the INNER route. THIS route is mounted
462+
// unconditionally, so passing that status through says "no such endpoint"
463+
// about a resource that exists. Answer the sibling's answer instead — same
464+
// condition, same code, same status (#10859). Before this, the branch
465+
// rewrote only the `message` and let the code fall through to the generic
466+
// default below, so the machine-readable field said "verification failed"
467+
// while the human-readable one said "the feature is off"; a caller can only
468+
// act on the first.
469+
if (resp.status === 404 && !parsed?.code) {
470+
return { status: 400, body: { success: false, error: { code: 'DOMAIN_VERIFICATION_DISABLED', message: 'Domain verification is not enabled for this environment (set OS_SSO_DOMAIN_VERIFICATION).' } } };
471+
}
460472
// Friendlier copy for the expected failure modes.
461473
let message = parsed?.message || 'Domain verification failed';
462-
if (resp.status === 404 && !parsed?.code) {
463-
message = 'Domain verification is not enabled for this environment (set OS_SSO_DOMAIN_VERIFICATION).';
464-
} else if (parsed?.code === 'NO_PENDING_VERIFICATION') {
474+
if (parsed?.code === 'NO_PENDING_VERIFICATION') {
465475
message = 'No pending verification — click “Request Domain Verification” first to get the DNS record.';
466476
} else if (parsed?.code === 'DOMAIN_VERIFICATION_FAILED') {
467477
message = 'DNS TXT record not found yet. Add the record shown when you requested verification, allow time for DNS to propagate, then retry.';

‎packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts‎

Lines changed: 89 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,13 +64,18 @@ describe('#10716 SSO domain verification — our default is a registered ADR-011
6464
});
6565

6666
it('verify-domain: an uncoded vendor failure answers our SCREAMING default, status passed through', async () => {
67-
// The 404-without-a-code shape: SSO domain verification is off for this env.
68-
// This is the response the dogfood admin-route probe observes.
69-
const res = await runVerifyDomain(fakeHandle(404, undefined), post(VERIFY_URL));
67+
// RE-FIXTURED by #10859. This case used to be driven by `fakeHandle(404,
68+
// undefined)` — which is the DISABLED shape, not a verification failure, and
69+
// now answers `DOMAIN_VERIFICATION_DISABLED` (see the #10859 describe below).
70+
// Pointing the generic-default leg at that shape would have left the default
71+
// arm of `verify-domain` with no coverage at all once the disabled branch
72+
// returned early, so it is driven by a real uncoded failure instead — the
73+
// same `502` its sibling above uses.
74+
const res = await runVerifyDomain(fakeHandle(502, { message: 'upstream exploded' }), post(VERIFY_URL));
7075

7176
expect(res.body.success).toBe(false);
7277
expect(res.body.error?.code).toBe(OUR_DEFAULT);
73-
expect(res.status).toBe(404);
78+
expect(res.status).toBe(502);
7479
});
7580

7681
it('both defaults are SCREAMING_SNAKE and registered for this package — reused, not invented', () => {
@@ -116,3 +121,83 @@ describe('#10716 the vendor pass-through arm is untouched', () => {
116121
expect(res.status).toBe(400);
117122
});
118123
});
124+
125+
describe('#10859 the DISABLED condition gets ONE answer across both routes', () => {
126+
/**
127+
* The feature being off is a different condition from a verification failing,
128+
* and the two SSO domain-verification routes used to answer it differently:
129+
* the sibling rewrote to `400 DOMAIN_VERIFICATION_DISABLED`, while
130+
* `verify-domain` rewrote only the MESSAGE and let the code fall through to
131+
* the generic failure default — so its machine-readable field said
132+
* "verification failed" while its human-readable one said "the feature is
133+
* off". A caller can only act on the first.
134+
*
135+
* Both halves are asserted per ADR-0112 (`code` AND `status`), and the
136+
* counter-direction below is the load-bearing half: stamping `DISABLED` on
137+
* every failure would pass a one-directional suite while destroying exactly
138+
* the diagnosis the caller acts on.
139+
*/
140+
const DISABLED = 'DOMAIN_VERIFICATION_DISABLED';
141+
/** The inner endpoint is unmounted when the feature is off: 404, no code. */
142+
const disabledInner = () => fakeHandle(404, undefined);
143+
144+
it('verify-domain: the disabled condition answers the dedicated code at the sibling’s status', async () => {
145+
const res = await runVerifyDomain(disabledInner(), post(VERIFY_URL));
146+
147+
expect(res.body.success).toBe(false);
148+
expect(res.body.error?.code).toBe(DISABLED);
149+
expect(res.status).toBe(400);
150+
expect(res.body.error?.message).toContain('OS_SSO_DOMAIN_VERIFICATION');
151+
});
152+
153+
it('both routes answer the SAME code and the SAME status for the SAME condition', async () => {
154+
// The card's governing invariant, asserted directly rather than inferred
155+
// from the two per-route cases above: one answer for one condition.
156+
const verify = await runVerifyDomain(disabledInner(), post(VERIFY_URL));
157+
const request = await runRequestDomainVerification(disabledInner(), post(REQUEST_URL));
158+
159+
expect(verify.body.error?.code).toBe(request.body.error?.code);
160+
expect(verify.status).toBe(request.status);
161+
expect(verify.body.error?.code).toBe(DISABLED);
162+
expect(verify.status).toBe(400);
163+
});
164+
165+
it('DOMAIN_VERIFICATION_DISABLED is registered for this package — reused, not invented', () => {
166+
expect(DISABLED).toMatch(/^[A-Z][A-Z0-9_]*$/);
167+
expect(ERROR_CODE_LEDGER['@objectstack/plugin-auth']).toContain(DISABLED);
168+
});
169+
170+
// ── the load-bearing direction: DISABLED is NOT stamped on every failure ──
171+
172+
it('verify-domain: a genuine verification failure still answers the FAILURE code', async () => {
173+
// Uncoded, but not the disabled shape. An implementation that keyed on
174+
// `!resp.ok` instead of the 404-without-a-code shape would answer DISABLED
175+
// here and tell the admin to flip an env var that is already on.
176+
const res = await runVerifyDomain(fakeHandle(502, { message: 'upstream exploded' }), post(VERIFY_URL));
177+
178+
expect(res.body.error?.code).toBe(OUR_DEFAULT);
179+
expect(res.body.error?.code).not.toBe(DISABLED);
180+
expect(res.status).toBe(502);
181+
});
182+
183+
it('verify-domain: a 404 that CARRIES a vendor code is the vendor’s diagnosis, not DISABLED', async () => {
184+
// The disabled shape is 404 *without* a code. A 404 that carries one is the
185+
// vendor answering, and both its code and its status pass through untouched
186+
// — the arm #10716 pinned, re-pinned here at the status this branch tests.
187+
const res = await runVerifyDomain(fakeHandle(404, { code: VENDOR_CODE, message: 'vendor copy' }), post(VERIFY_URL));
188+
189+
expect(res.body.error?.code).toBe(VENDOR_CODE);
190+
expect(res.body.error?.code).not.toBe(DISABLED);
191+
expect(res.status).toBe(404);
192+
});
193+
194+
it('request-domain-verification: a genuine failure still answers the FAILURE code', async () => {
195+
// The sibling's counter-direction, so the parity assertion above cannot be
196+
// satisfied by both routes collapsing onto DISABLED.
197+
const res = await runRequestDomainVerification(fakeHandle(502, { message: 'upstream exploded' }), post(REQUEST_URL));
198+
199+
expect(res.body.error?.code).toBe(OUR_DEFAULT);
200+
expect(res.body.error?.code).not.toBe(DISABLED);
201+
expect(res.status).toBe(502);
202+
});
203+
});

‎packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ function expectationsFor(targetUserId: string): Record<string, RouteExpectation>
258258
'POST /api/v1/auth/admin/sso/verify-domain': {
259259
bucket: 'objectstack-gate',
260260
body: { providerId: 'refusal-probe-oidc' },
261-
note: 'admin passes the gate and lands on 404 DOMAIN_VERIFICATION_FAILED while SSO is off',
261+
note: 'admin passes the gate and lands on 400 DOMAIN_VERIFICATION_DISABLED while SSO is off (#10859 — it answered 404 DOMAIN_VERIFICATION_FAILED before, out of step with the sibling above)',
262262
},
263263

264264
// ── #9652: ban / unban moved from the vendor to an ObjectStack mount ────

0 commit comments

Comments
 (0)