From 93c4f7c9b5795e7a3376325f3c913826ee478e5a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 03:41:42 +0000 Subject: [PATCH] fix(plugin-auth): verify-domain answers DOMAIN_VERIFICATION_DISABLED for the disabled condition 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 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- ...y-domain-disabled-answers-disabled-code.md | 56 +++++++++++ .../plugin-auth/src/register-sso-provider.ts | 16 +++- ...so-domain-verification-error-codes.test.ts | 93 ++++++++++++++++++- ...min-route-nonadmin-refusal.dogfood.test.ts | 2 +- 4 files changed, 159 insertions(+), 8 deletions(-) create mode 100644 .changeset/sso-verify-domain-disabled-answers-disabled-code.md diff --git a/.changeset/sso-verify-domain-disabled-answers-disabled-code.md b/.changeset/sso-verify-domain-disabled-answers-disabled-code.md new file mode 100644 index 0000000000..77f100b382 --- /dev/null +++ b/.changeset/sso-verify-domain-disabled-answers-disabled-code.md @@ -0,0 +1,56 @@ +--- +"@objectstack/plugin-auth": minor +--- + +`POST /admin/sso/verify-domain` now answers the DISABLED condition the way its +sibling always has. When SSO domain verification is off for an environment, +`@better-auth/sso` never mounts the inner endpoint and answers `404` with no +code. Both bridge routes recognise that shape, and they used to answer it +differently (#10859): + +| route | answered | answers instead | +| --- | --- | --- | +| `POST /admin/sso/request-domain-verification` | `400` `DOMAIN_VERIFICATION_DISABLED` | unchanged | +| `POST /admin/sso/verify-domain` | `404` `DOMAIN_VERIFICATION_FAILED` | `400` `DOMAIN_VERIFICATION_DISABLED` | + +`verify-domain` rewrote only the `message` for that branch and let the code fall +through to its generic failure default, so the response carried "the feature is +off" copy under a code that means "verification failed". A caller can only act +on the machine-readable half, and the two halves disagreed. The status moves +with the code: the inner `404` describes the INNER endpoint, which is unmounted, +whereas this bridge route is mounted unconditionally — passing that status +through said "no such endpoint" about a resource that exists. + +If you match on `DOMAIN_VERIFICATION_FAILED` (or on `404`) to detect the +disabled case on `verify-domain`, match on `DOMAIN_VERIFICATION_DISABLED` (or on +`400`) instead — the same pair `request-domain-verification` has always +answered. The distinction is worth having: `DISABLED` means "turn on +`OS_SSO_DOMAIN_VERIFICATION`", `FAILED` means "the DNS TXT record is not visible +yet, retry". + +**No `packages/spec` change, and the emitted vocabulary gains no member.** Both +codes are already registered for `@objectstack/plugin-auth` in the error-code +ledger, with exactly these meanings (`DOMAIN_VERIFICATION_DISABLED` — "domain +verification is off on this deployment"). This route was emitting a *declared* +code whose registered meaning is a different condition, so this is +declared-vs-enforced restoration rather than a new contract decision. + +**A genuine verification failure still answers the failure code, and the vendor +pass-through arm is untouched on both routes.** The rewrite is keyed to the +disabled shape specifically — `404` *without* a code. A `404` that carries +`@better-auth/sso`'s own code is the vendor's diagnosis and reaches the caller +verbatim, status included, as does every non-404 failure. That direction is the +load-bearing one — an implementation keyed to "any 404", or to `!resp.ok`, would +satisfy the disabled case while destroying the diagnosis a caller acts on — and +it is pinned in both directions in +`packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts`. + +Shipped as `minor`, following the same call the casing rename on these two +routes made (#10716). The argument for it: the vocabulary is unchanged, and the +old pairing was self-contradictory rather than a contract anyone could have +relied on deliberately. The argument against it, stated here rather than +settled: unlike that rename — whose old spellings were undeclared values no +schema admitted — `DOMAIN_VERIFICATION_FAILED` *is* a declared, registered code, +so a client keyed to it for this case was keyed to something the published +contract admitted, and both halves of the answer change. A reviewer who reads +that as `major` is not reading it wrong; this PR does not decide it silently. diff --git a/packages/plugins/plugin-auth/src/register-sso-provider.ts b/packages/plugins/plugin-auth/src/register-sso-provider.ts index 62e381c3ab..429a1399ba 100644 --- a/packages/plugins/plugin-auth/src/register-sso-provider.ts +++ b/packages/plugins/plugin-auth/src/register-sso-provider.ts @@ -457,11 +457,21 @@ export async function runVerifyDomain( if (resp.ok) { return { status: 200, body: { success: true, data: { providerId, verified: true, message: 'Domain ownership verified — this provider can now sign users in.' } } }; } + // The feature is OFF for this env: `@better-auth/sso` never mounted the inner + // endpoint, so its `404` describes the INNER route. THIS route is mounted + // unconditionally, so passing that status through says "no such endpoint" + // about a resource that exists. Answer the sibling's answer instead — same + // condition, same code, same status (#10859). Before this, the branch + // rewrote only the `message` and let the code fall through to the generic + // default below, so the machine-readable field said "verification failed" + // while the human-readable one said "the feature is off"; a caller can only + // act on the first. + if (resp.status === 404 && !parsed?.code) { + 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).' } } }; + } // Friendlier copy for the expected failure modes. let message = parsed?.message || 'Domain verification failed'; - if (resp.status === 404 && !parsed?.code) { - message = 'Domain verification is not enabled for this environment (set OS_SSO_DOMAIN_VERIFICATION).'; - } else if (parsed?.code === 'NO_PENDING_VERIFICATION') { + if (parsed?.code === 'NO_PENDING_VERIFICATION') { message = 'No pending verification — click “Request Domain Verification” first to get the DNS record.'; } else if (parsed?.code === 'DOMAIN_VERIFICATION_FAILED') { message = 'DNS TXT record not found yet. Add the record shown when you requested verification, allow time for DNS to propagate, then retry.'; diff --git a/packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts b/packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts index fadee75d94..7213137978 100644 --- a/packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts +++ b/packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts @@ -64,13 +64,18 @@ describe('#10716 SSO domain verification — our default is a registered ADR-011 }); it('verify-domain: an uncoded vendor failure answers our SCREAMING default, status passed through', async () => { - // The 404-without-a-code shape: SSO domain verification is off for this env. - // This is the response the dogfood admin-route probe observes. - const res = await runVerifyDomain(fakeHandle(404, undefined), post(VERIFY_URL)); + // RE-FIXTURED by #10859. This case used to be driven by `fakeHandle(404, + // undefined)` — which is the DISABLED shape, not a verification failure, and + // now answers `DOMAIN_VERIFICATION_DISABLED` (see the #10859 describe below). + // Pointing the generic-default leg at that shape would have left the default + // arm of `verify-domain` with no coverage at all once the disabled branch + // returned early, so it is driven by a real uncoded failure instead — the + // same `502` its sibling above uses. + const res = await runVerifyDomain(fakeHandle(502, { message: 'upstream exploded' }), post(VERIFY_URL)); expect(res.body.success).toBe(false); expect(res.body.error?.code).toBe(OUR_DEFAULT); - expect(res.status).toBe(404); + expect(res.status).toBe(502); }); 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', () => { expect(res.status).toBe(400); }); }); + +describe('#10859 the DISABLED condition gets ONE answer across both routes', () => { + /** + * The feature being off is a different condition from a verification failing, + * and the two SSO domain-verification routes used to answer it differently: + * the sibling rewrote to `400 DOMAIN_VERIFICATION_DISABLED`, while + * `verify-domain` rewrote only the MESSAGE and let the code fall through to + * the generic failure default — so its machine-readable field said + * "verification failed" while its human-readable one said "the feature is + * off". A caller can only act on the first. + * + * Both halves are asserted per ADR-0112 (`code` AND `status`), and the + * counter-direction below is the load-bearing half: stamping `DISABLED` on + * every failure would pass a one-directional suite while destroying exactly + * the diagnosis the caller acts on. + */ + const DISABLED = 'DOMAIN_VERIFICATION_DISABLED'; + /** The inner endpoint is unmounted when the feature is off: 404, no code. */ + const disabledInner = () => fakeHandle(404, undefined); + + it('verify-domain: the disabled condition answers the dedicated code at the sibling’s status', async () => { + const res = await runVerifyDomain(disabledInner(), post(VERIFY_URL)); + + expect(res.body.success).toBe(false); + expect(res.body.error?.code).toBe(DISABLED); + expect(res.status).toBe(400); + expect(res.body.error?.message).toContain('OS_SSO_DOMAIN_VERIFICATION'); + }); + + it('both routes answer the SAME code and the SAME status for the SAME condition', async () => { + // The card's governing invariant, asserted directly rather than inferred + // from the two per-route cases above: one answer for one condition. + const verify = await runVerifyDomain(disabledInner(), post(VERIFY_URL)); + const request = await runRequestDomainVerification(disabledInner(), post(REQUEST_URL)); + + expect(verify.body.error?.code).toBe(request.body.error?.code); + expect(verify.status).toBe(request.status); + expect(verify.body.error?.code).toBe(DISABLED); + expect(verify.status).toBe(400); + }); + + it('DOMAIN_VERIFICATION_DISABLED is registered for this package — reused, not invented', () => { + expect(DISABLED).toMatch(/^[A-Z][A-Z0-9_]*$/); + expect(ERROR_CODE_LEDGER['@objectstack/plugin-auth']).toContain(DISABLED); + }); + + // ── the load-bearing direction: DISABLED is NOT stamped on every failure ── + + it('verify-domain: a genuine verification failure still answers the FAILURE code', async () => { + // Uncoded, but not the disabled shape. An implementation that keyed on + // `!resp.ok` instead of the 404-without-a-code shape would answer DISABLED + // here and tell the admin to flip an env var that is already on. + const res = await runVerifyDomain(fakeHandle(502, { message: 'upstream exploded' }), post(VERIFY_URL)); + + expect(res.body.error?.code).toBe(OUR_DEFAULT); + expect(res.body.error?.code).not.toBe(DISABLED); + expect(res.status).toBe(502); + }); + + it('verify-domain: a 404 that CARRIES a vendor code is the vendor’s diagnosis, not DISABLED', async () => { + // The disabled shape is 404 *without* a code. A 404 that carries one is the + // vendor answering, and both its code and its status pass through untouched + // — the arm #10716 pinned, re-pinned here at the status this branch tests. + const res = await runVerifyDomain(fakeHandle(404, { code: VENDOR_CODE, message: 'vendor copy' }), post(VERIFY_URL)); + + expect(res.body.error?.code).toBe(VENDOR_CODE); + expect(res.body.error?.code).not.toBe(DISABLED); + expect(res.status).toBe(404); + }); + + it('request-domain-verification: a genuine failure still answers the FAILURE code', async () => { + // The sibling's counter-direction, so the parity assertion above cannot be + // satisfied by both routes collapsing onto DISABLED. + const res = await runRequestDomainVerification(fakeHandle(502, { message: 'upstream exploded' }), post(REQUEST_URL)); + + expect(res.body.error?.code).toBe(OUR_DEFAULT); + expect(res.body.error?.code).not.toBe(DISABLED); + expect(res.status).toBe(502); + }); +}); diff --git a/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts b/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts index abd2e29f86..90539d6f52 100644 --- a/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts +++ b/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts @@ -258,7 +258,7 @@ function expectationsFor(targetUserId: string): Record 'POST /api/v1/auth/admin/sso/verify-domain': { bucket: 'objectstack-gate', body: { providerId: 'refusal-probe-oidc' }, - note: 'admin passes the gate and lands on 404 DOMAIN_VERIFICATION_FAILED while SSO is off', + 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)', }, // ── #9652: ban / unban moved from the vendor to an ObjectStack mount ────