diff --git a/.changeset/sso-domain-verification-error-code-casing.md b/.changeset/sso-domain-verification-error-code-casing.md new file mode 100644 index 0000000000..f16dc597e2 --- /dev/null +++ b/.changeset/sso-domain-verification-error-code-casing.md @@ -0,0 +1,46 @@ +--- +"@objectstack/plugin-auth": minor +--- + +The two SSO domain-verification admin routes now answer a registered ADR-0112 +error code. `POST /admin/sso/request-domain-verification` and +`POST /admin/sso/verify-domain` shape their failure as +`code: parsed?.code || `, and the default half — the code +ObjectStack itself authors when @better-auth/sso returns none — was lowercase +(#10716, found by #10658): + +| route | wrote | writes instead | +| --- | --- | --- | +| `POST /admin/sso/request-domain-verification` | `request_domain_verification_failed` | `DOMAIN_VERIFICATION_FAILED` | +| `POST /admin/sso/verify-domain` | `verify_domain_failed` | `DOMAIN_VERIFICATION_FAILED` | + +If you match on either lowercase spelling, match on `DOMAIN_VERIFICATION_FAILED` +instead — the two routes are distinguished by their path, as they already were +for every other failure they can answer. + +`DOMAIN_VERIFICATION_FAILED` is reused, not invented: it is already registered +for `@objectstack/plugin-auth` in the error-code ledger, so this PR adds nothing +to `packages/spec` and the emitted vocabulary gets no new member. A new spelling +(`VERIFY_DOMAIN_FAILED`) would have needed a ledger registration to be a legal +`error.code` at all, and — measured while fixing this — an unregistered code in +an `||` fallback slot is currently invisible to BOTH error-code gates, so it +would have shipped as exactly the silent fourth state ADR-0112 D3 exists to +prevent. + +**The vendor pass-through arm is unchanged.** `parsed?.code` still reaches the +caller verbatim, so @better-auth/sso's own diagnosis (`NO_PENDING_VERIFICATION`, +`DOMAIN_VERIFICATION_FAILED`) is never overwritten by ours — the half that would +be silently lost by a handler that stamped our code unconditionally, and it is +pinned in both directions by +`packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts`. +Statuses and messages are untouched on every path. + +ADR-0087 disposition, in prose because the marker vocabulary has no slot for +this shape: nothing is registered and nothing needs to be. The declared wire +contract is `error.code ∈ StandardErrorCode ∪ ERROR_CODE_LEDGER`, and neither +lowercase spelling was ever a member of it — they were undeclared values a +blind gate let through, so this brings the implementation onto the published +contract rather than changing that contract. There is no metadata surface for +`objectstack migrate meta` to rewrite: error codes live in responses, not in +stored metadata. The table above is here for anyone who matched the undeclared +spelling anyway, which is why this ships as `minor` rather than `patch`. diff --git a/packages/plugins/plugin-auth/src/register-sso-provider.ts b/packages/plugins/plugin-auth/src/register-sso-provider.ts index a30734337b..62e381c3ab 100644 --- a/packages/plugins/plugin-auth/src/register-sso-provider.ts +++ b/packages/plugins/plugin-auth/src/register-sso-provider.ts @@ -408,7 +408,11 @@ export async function runRequestDomainVerification( 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).' } } }; } - return { status: resp.status, body: { success: false, error: { code: parsed?.code || 'request_domain_verification_failed', message: parsed?.message || 'Failed to request domain verification' } } }; + // ADR-0112: `parsed?.code` is better-auth's own code passing through — never + // overwritten. The DEFAULT is ours, so it is the registered SCREAMING ledger + // entry for this feature (`DOMAIN_VERIFICATION_FAILED`, @objectstack/plugin-auth) + // rather than a bespoke lowercase spelling (#10716). + return { status: resp.status, body: { success: false, error: { code: parsed?.code || 'DOMAIN_VERIFICATION_FAILED', message: parsed?.message || 'Failed to request domain verification' } } }; } const token = str(parsed?.domainVerificationToken); @@ -462,5 +466,8 @@ export async function runVerifyDomain( } 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.'; } - return { status: resp.status, body: { success: false, error: { code: parsed?.code || 'verify_domain_failed', message } } }; + // ADR-0112, as above: the vendor's code passes through untouched; our default + // is the registered ledger entry (#10716). The `message` above still carries + // the specific diagnosis for each expected failure mode. + return { status: resp.status, body: { success: false, error: { code: parsed?.code || 'DOMAIN_VERIFICATION_FAILED', message } } }; } 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 new file mode 100644 index 0000000000..fadee75d94 --- /dev/null +++ b/packages/plugins/plugin-auth/src/sso-domain-verification-error-codes.test.ts @@ -0,0 +1,118 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #10716 — the two SSO domain-verification routes answer an ADR-0112 error code. + * + * Both handlers shape their failure as `code: parsed?.code || `, + * and the two halves of that expression have DIFFERENT owners: + * + * - `parsed?.code` is @better-auth/sso's own code passing through. It is the + * vendor's diagnosis and must reach the caller unchanged. + * - the default is OURS, so ADR-0112 applies to it: SCREAMING_SNAKE, drawn + * from the registered vocabulary rather than invented at the call site. + * + * Both directions are pinned below, and that pairing is the point. Pinning only + * "our code appears" would go green against a handler that overwrites the + * vendor's code unconditionally — which would swallow exactly the diagnosis the + * caller needs (`NO_PENDING_VERIFICATION` tells them to request the DNS record + * first; a blanket "verification failed" does not). Statuses are pinned in both + * directions too: the inner status passes through untouched. + * + * Rejection cases assert `code` AND `status` per ADR-0112 — a bare "it threw" + * would pass against a handler that refuses everyone. + */ + +import { describe, it, expect } from 'vitest'; +// The `/api` subpath is where the built package exposes the ledger VALUE — the +// root entry re-exports the schemas only. Reading it through the published +// exports map (i.e. `dist`) is deliberate: this asserts against the surface a +// consumer actually gets, not against a source file this test could reach. +import { ERROR_CODE_LEDGER } from '@objectstack/spec/api'; +import { runRequestDomainVerification, runVerifyDomain, type AuthRequestHandler } from './register-sso-provider.js'; + +/** The default this package authors for both routes when the vendor gives no code. */ +const OUR_DEFAULT = 'DOMAIN_VERIFICATION_FAILED'; +/** A vendor code with its own meaning — the one an overwrite would destroy. */ +const VENDOR_CODE = 'NO_PENDING_VERIFICATION'; + +const REQUEST_URL = 'https://app.example/api/v1/auth/admin/sso/request-domain-verification'; +const VERIFY_URL = 'https://app.example/api/v1/auth/admin/sso/verify-domain'; + +/** A fake inner @better-auth/sso endpoint that answers one canned response. */ +function fakeHandle(status: number, body: unknown): AuthRequestHandler { + return async () => + new Response(body === undefined ? '' : JSON.stringify(body), { + status, + headers: { 'content-type': 'application/json' }, + }); +} + +const post = (url: string) => + new Request(url, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ providerId: 'p1', domain: 'acme.example' }), + }); + +describe('#10716 SSO domain verification — our default is a registered ADR-0112 code', () => { + it('request-domain-verification: an uncoded vendor failure answers our SCREAMING default, status passed through', async () => { + const res = await runRequestDomainVerification(fakeHandle(502, { message: 'upstream exploded' }), post(REQUEST_URL)); + + expect(res.body.success).toBe(false); + expect(res.body.error?.code).toBe(OUR_DEFAULT); + expect(res.status).toBe(502); + }); + + 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)); + + expect(res.body.success).toBe(false); + expect(res.body.error?.code).toBe(OUR_DEFAULT); + expect(res.status).toBe(404); + }); + + it('both defaults are SCREAMING_SNAKE and registered for this package — reused, not invented', () => { + expect(OUR_DEFAULT).toMatch(/^[A-Z][A-Z0-9_]*$/); + // The mechanical half of "we reused a ledger entry": if this code were a new + // spelling it would need a `packages/spec` registration, and an unregistered + // code in a fallback slot is invisible to BOTH error-code gates — a silent + // fourth state, which is precisely what ADR-0112 D3 exists to prevent. + expect(ERROR_CODE_LEDGER['@objectstack/plugin-auth']).toContain(OUR_DEFAULT); + }); +}); + +describe('#10716 the vendor pass-through arm is untouched', () => { + it('request-domain-verification: the vendor code and status reach the caller unchanged', async () => { + const res = await runRequestDomainVerification( + fakeHandle(400, { code: VENDOR_CODE, message: 'vendor copy' }), + post(REQUEST_URL), + ); + + expect(res.body.error?.code).toBe(VENDOR_CODE); + expect(res.body.error?.code).not.toBe(OUR_DEFAULT); + expect(res.status).toBe(400); + expect(res.body.error?.message).toBe('vendor copy'); + }); + + it('verify-domain: the vendor code and status reach the caller unchanged', async () => { + const res = await runVerifyDomain(fakeHandle(400, { code: VENDOR_CODE, message: 'vendor copy' }), post(VERIFY_URL)); + + expect(res.body.error?.code).toBe(VENDOR_CODE); + expect(res.body.error?.code).not.toBe(OUR_DEFAULT); + expect(res.status).toBe(400); + // This route substitutes friendlier COPY for known vendor codes — the code + // itself still passes through, which is the half that matters on the wire. + expect(res.body.error?.message).toContain('Request Domain Verification'); + }); + + it('request-domain-verification: the disabled branch keeps its own dedicated code', async () => { + // Unchanged by #10716 and pinned so the rename cannot have blurred the two: + // "the feature is off" is a different answer from "verification failed". + const res = await runRequestDomainVerification(fakeHandle(404, undefined), post(REQUEST_URL)); + + expect(res.body.error?.code).toBe('DOMAIN_VERIFICATION_DISABLED'); + expect(res.status).toBe(400); + }); +}); 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 c1d1433459..4c1801e550 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 @@ -249,7 +249,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 verify_domain_failed while SSO is off', + note: 'admin passes the gate and lands on 404 DOMAIN_VERIFICATION_FAILED while SSO is off', }, // ── #9652: ban / unban moved from the vendor to an ObjectStack mount ──── diff --git a/scripts/check-error-code-casing.mjs b/scripts/check-error-code-casing.mjs index 1444873d88..b1bcc99143 100644 --- a/scripts/check-error-code-casing.mjs +++ b/scripts/check-error-code-casing.mjs @@ -162,27 +162,28 @@ const CODE_POSITION_PATTERNS = [ * an allowlist nobody re-reads. A NEW lowercase code never joins it: the gate * refuses that, and the remedy for a fresh finding is a registered SCREAMING * code, never a line here. + * + * [#10716] It is now EMPTY, and that is this list reaching its designed end + * rather than a list that never had a use: both entries it was created for + * (`request_domain_verification_failed`, `verify_domain_failed`) were renamed + * onto the registered ledger code `DOMAIN_VERIFICATION_FAILED` by the services + * lane, and the owning PR deleted them here — the coordination #10658 asked + * for, so the gate ends up green with zero exceptions. Emptiness costs no + * coverage: --self-test drives the shrink-only semantics against a FIXTURE + * registry, and pins the live one AT zero so a future exception cannot be + * added quietly. */ -const KNOWN_LOWERCASE_CODES = new Map([ - [ - 'packages/plugins/plugin-auth/src/register-sso-provider.ts::request_domain_verification_failed', - 'wire-visible; rename owned by #10716 (services lane)', - ], - [ - 'packages/plugins/plugin-auth/src/register-sso-provider.ts::verify_domain_failed', - 'wire-visible; rename owned by #10716 (services lane); pinned by name in the dogfood suite', - ], -]); +const KNOWN_LOWERCASE_CODES = new Map([]); /** Split findings into the two the wire already carries and everything else. */ -export function partitionKnown(violations) { +export function partitionKnown(violations, registry = KNOWN_LOWERCASE_CODES) { const known = []; const fresh = []; for (const v of violations) { - (KNOWN_LOWERCASE_CODES.has(`${v.file}::${v.literal}`) ? known : fresh).push(v); + (registry.has(`${v.file}::${v.literal}`) ? known : fresh).push(v); } const reached = new Set(known.map((v) => `${v.file}::${v.literal}`)); - const stale = [...KNOWN_LOWERCASE_CODES.keys()].filter((k) => !reached.has(k)).sort(); + const stale = [...registry.keys()].filter((k) => !reached.has(k)).sort(); return { known, fresh, stale }; } @@ -302,8 +303,19 @@ function selfTest() { // [#10658] The shrink-only registry, in both directions. The second one is // the load-bearing half: when the owning card's rename lands, a stale line // must FAIL rather than sit there as a quiet allowlist entry. + // + // [#10716] These drive a FIXTURE registry rather than the live one, which is + // now empty. The semantics being pinned belong to the MECHANISM (an entry + // that stops matching is stale and fails; a new code is never absorbed), and + // they have to survive the live list reaching zero — otherwise emptying it + // would have silently taken the coverage with it. The live list gets its own + // assertion below. const SSO = 'packages/plugins/plugin-auth/src/register-sso-provider.ts'; const row = (literal) => ({ file: SSO, line: 1, literal, form: 'fallback' }); + const fixtureRegistry = new Map([ + [`${SSO}::request_domain_verification_failed`, 'fixture — the shape the live list had before #10716'], + [`${SSO}::verify_domain_failed`, 'fixture — the shape the live list had before #10716'], + ]); const partitionCases = [ [[row('request_domain_verification_failed'), row('verify_domain_failed')], { known: 2, fresh: 0, stale: 0 }, 'both known rows still present'], [ @@ -315,7 +327,7 @@ function selfTest() { [[], { known: 0, fresh: 0, stale: 2 }, 'an empty tree makes every entry stale'], ]; for (const [input, want, label] of partitionCases) { - const got = partitionKnown(input); + const got = partitionKnown(input, fixtureRegistry); const shape = { known: got.known.length, fresh: got.fresh.length, stale: got.stale.length }; if (shape.known !== want.known || shape.fresh !== want.fresh || shape.stale !== want.stale) { console.error(` ✗ self-test "${label}": expected ${JSON.stringify(want)}, got ${JSON.stringify(shape)}`); @@ -323,12 +335,24 @@ function selfTest() { } } + // [#10716] The live registry, at zero. It is closed to new entries by the rule + // above, so "closed" is checked rather than merely written down: a wire-visible + // code that genuinely needs deferring is a call for the ADR-0112 owner to make + // in the open, not a line someone adds back here on the way past. + if (KNOWN_LOWERCASE_CODES.size !== 0) { + console.error( + ` ✗ self-test "the live registry stays empty": KNOWN_LOWERCASE_CODES holds ${KNOWN_LOWERCASE_CODES.size} entry/entries — ` + + `this list is closed (#10658/#10716); a new deferral is an ADR-0112 decision, not a line here.`, + ); + failed++; + } + if (failed) { console.error(`\n✗ check-error-code-casing self-test failed (${failed} case(s)).`); process.exit(1); } console.log( - `✓ check-error-code-casing self-test: ${cases.length} recognizer case(s) + ${partitionCases.length} registry case(s) pass.`, + `✓ check-error-code-casing self-test: ${cases.length} recognizer case(s) + ${partitionCases.length + 1} registry case(s) pass.`, ); }