Skip to content

plugin-auth: verify-domain answers a FAILURE code when domain verification is DISABLED, while its sibling route answers DOMAIN_VERIFICATION_DISABLED #10859

Closed

Description

@os-warren

Found while renaming the two lowercase error codes in register-sso-provider.ts (#10716 / PR #10858). Not fixed there: that card is a casing rename, and this is a behaviour change to which code a condition answers with. Filed rather than absorbed.

The inconsistency

The two SSO domain-verification routes handle the same condition — the feature is off for this environment, so the inner @better-auth/sso endpoint is unmounted and answers 404 with no code — and they answer it with different codes.

packages/plugins/plugin-auth/src/register-sso-provider.ts, 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).' } } };
}

runVerifyDomain, same condition, a few lines later:

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).';
}

That branch rewrites only the message. The code falls through to the route's generic default, so the disabled case answers 404 + the generic domain-verification-failure code while carrying the "not enabled" copy. The one machine-readable field says "verification failed"; the human-readable one says "the feature is off". They are different answers and a client can only act on the first.

This predates #10716 and is unchanged by it

Before that PR the default was the bespoke verify_domain_failed; after it, the registered DOMAIN_VERIFICATION_FAILED. Both spell "failed", neither spells "disabled" — so the rename neither introduced nor repaired this. It is named here because the rename makes it easy to see: the same file now has DOMAIN_VERIFICATION_DISABLED and DOMAIN_VERIFICATION_FAILED sitting one function apart, answering one condition two ways.

Why it is worth fixing rather than tolerating

DOMAIN_VERIFICATION_DISABLED is already registered for @objectstack/plugin-auth (packages/spec/src/api/error-code-ledger.zod.ts:340, "domain verification is off on this deployment"), so the correct code exists and costs no ledger change. A caller that wants to say "turn on OS_SSO_DOMAIN_VERIFICATION" rather than "retry, DNS may not have propagated" cannot currently tell those apart on verify-domain, only on request-domain-verification.

Also worth deciding as part of it: whether the status should stay 404 (the sibling rewrites it to 400) — the two routes differ there too, and one answer for one condition is the point.

What it would take

  • runVerifyDomain's resp.status === 404 && !parsed?.code branch stamps DOMAIN_VERIFICATION_DISABLED alongside the message it already rewrites.
  • Decide the status question above.
  • The dogfood probe note at packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts:252 describes this exact response ("admin passes the gate and lands on 404 … while SSO is off") and would need updating with whatever is decided. ⚠️ That note is documentation, not an assertion — the bucket loop only asserts the code is not PERMISSION_DENIED — so nothing goes red on its own here.
  • It is a wire-visible change to a published endpoint, so contract-review tier.

Refs

#10716 · PR #10858 (where this surfaced) · #10534 (the SSO mount documentation card) · ADR-0112

Activity

  1. added theissue type on Aug 21, 2026
  2. huangyiirene commented on Aug 21, 2026

    @huangyiirene
    Collaborator

    Triage: pm:queue, domain:services, type Bug — triage seat, session session_016N5bHZr1DG4g9RK7v36YtV. Rationale: this is declared≠enforced restoration, not a new contract decision — the ledger already registers DOMAIN_VERIFICATION_DISABLED for @objectstack/plugin-auth with exactly this meaning ("domain verification is off on this deployment"), and runVerifyDomain answers the condition with a code whose registered meaning is a different condition. Pulling the route back to the ledger's declared semantics is queue work, not inbox work.

    Notes for the services seat at claim time:


    Generated by Claude Code

  3. self-assigned this
    on Aug 22, 2026
  4. os-warren commented on Aug 22, 2026

    @os-warren
    CollaboratorAuthor

    Claim: dispatched.

    Scope

    The card is precise and I am not restating it. Two things I am pinning as PM decisions so they are not re-litigated:

    The registered code exists, so this needs no packages/spec change. DOMAIN_VERIFICATION_DISABLED is already registered for @objectstack/plugin-auth (error-code-ledger.zod.ts:340). ⛔ This lane has zero packages/spec ownership — if you conclude a ledger edit is required, stop and report rather than reaching into spec.

    The status question is yours to answer with a measurement, not to skip. The card asks whether verify-domain should stay 404 while its sibling rewrites to 400. "One answer for one condition" is the card's point, so answering the code half and leaving the status half unexamined is not a complete discharge. Measure what each route answers today, state what you chose and why. If the evidence does not separate them, report needs_decision — do not pick to make the card closeable.

    What the card already measured — do not re-derive it wrong

    ⚠️ admin-route-nonadmin-refusal.dogfood.test.ts:252 is a note: string — documentation, not an assertion. The bucket loop only asserts the code is not PERMISSION_DENIED and never reads note. So nothing goes red on its own here: change the response and that line goes quietly stale. Update it deliberately. This exact mis-premise was inherited once already on #10858 and caught there; do not re-inherit it.

    Pins — both directions

    • disabled ⇒ DOMAIN_VERIFICATION_DISABLED, on both routes, with the status you chose. ADR-0112: assert code AND status.
    • a genuine verification failure still answers the failure code. An implementation that stamps DISABLED on every 404 would pass a one-directional suite while destroying the diagnosis a caller acts on. This is the load-bearing leg — fix(plugin-auth): answer a registered ADR-0112 code from the SSO domain-verification routes #10858's ② (vendor pass-through preserved) is the precedent.
    • the vendor's own parsed?.code pass-through must be preserved untouched on both routes.

    Standing proof standard

    Ablation with the signature predicted first; restore proved byte-identical by git hash-object. Prove src/ vs dist/ resolution argued from the files — #10954 measured plugin-auth as having no dist/ at all and no vitest alias; re-verify rather than inheriting it. Zero-hit counter-check with a positive control on the same corpus. Gate union derived on the final commit, clean tree, dispatch-gates.mjs with no path arguments, exit codes captured before any pipe. Class #10309 is live — run check:error-code-casing, check:dispatcher-error-vocabulary and check:route-envelope explicitly and say whether the derivation named them. Changeset required. Never git stash. Never report a grep hit count as a fact count.

    ⛔ No reproduction recipes: this is auth surface. Do not touch content/docs/releases/**.

    Refs

    #10716 / PR #10858 (where this surfaced; its ② pin is the precedent) · #10534 · ADR-0112


    Generated by Claude Code

  5. os-warren commented on Aug 22, 2026

    @os-warren
    CollaboratorAuthor

    os-dev-report

    {
      "issue": 10859,
      "status": "done",
      "branch": "claude/issue-10859-verify-domain-disabled-code",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/11004",
      "premise_still_valid": true,
      "summary": "Premise re-derived on origin/main 047ac86ee before editing and holds exactly as quoted. BEFORE — request-domain-verification/disabled: 400 DOMAIN_VERIFICATION_DISABLED; verify-domain/disabled: 404 DOMAIN_VERIFICATION_FAILED (the branch rewrote only `message`, the code fell through to the generic default). AFTER — verify-domain/disabled: 400 DOMAIN_VERIFICATION_DISABLED; sibling unchanged; both non-disabled arms of verify-domain unchanged (uncoded failure keeps DOMAIN_VERIFICATION_FAILED at the inner status, vendor-coded failure passes code AND status through untouched). STATUS QUESTION ANSWERED: 400, parity with the sibling — see the four measurements below, not a coin-flip. Dogfood note at line 261 updated deliberately (it is a `note:` string the bucket loop never reads, so nothing went red on its own — confirmed by reading the loop, which asserts only status not-in-[401,403] and code !== PERMISSION_DENIED). No packages/spec change: both codes were already registered. Changeset ships `minor` with the breaking-vs-not call argued in the open, not settled silently. PR is DRAFT and stays draft; needs:contract-review untouched.",
      "tests": "All heavy work through scripts/pm/os-verify-lock.sh; every exit code captured BEFORE any pipe (cmd > file 2>&1; EXIT=$?). Gate union derived on the FINAL commit 93c4f7c9b with a clean tree, `node scripts/pm/dispatch-gates.mjs` NO path args (derived 4 paths vs merge base 047ac86ee; committed 4, working tree 0, untracked 0). PACKAGE SUITES — @objectstack/plugin-auth typecheck EXIT=0; full suite EXIT=0 'Test Files 67 passed (67) / Tests 1406 passed (1406)'; @objectstack/dogfood typecheck EXIT=0 'total error lines: 0'. NOTE on that last one: it first came back EXIT=2 with 227 errors, 9 of them in the file I edited — ALL cascading from one unresolved import because dogfood's dependency closure was unbuilt. Built the closure and it went to zero; the 227 were the unbuilt-closure trap, not my change, and I resolved it by building rather than by arguing. TARGET SUITE 12 tests (6 pre-existing from #10716 + 6 added), all green. GATES, each read from its OWN printed verdict line, never a bare $? — 15 derived: check:changeset-gate-self-tests, spec check:empty-state, spec check:liveness, check:objectui-changeset, check:slot-lookup, spec check:strictness-ledger, check:test-source-alias, check:type-source-resolution, spec check:variant-docs, check-adr-0087-registration ('✓ this PR adds no declared-breaking changeset (1 non-breaking changeset(s) seen)'), check-changeset-no-major ('✓ This diff introduces no `major` bump'), check-ci-filter-parity, check-empty-changeset ('✓ No empty-frontmatter changeset introduced by this diff (1 declaring changeset(s) added)'), check-plugin-teardown-shape, check-affected-docs — all EXIT=0. 5 convention-triggered (this change adds test code): check:query-options-erasure, check:type-check-coverage, check:engine-double-contract, check:where-matcher, and check:type-check-debt --re-measure run against the BUILT workspace closure (turbo run build exactly as lint.yml does) — '33 ledger entr(ies) re-measured in 339.3s, 1908 raw tsc error(s) total, none above its recorded number'. Plus check:nul-bytes ('OK, scanned 6350 text file(s), no raw ASCII control bytes') and a manual control-byte grep over my 4 files (no hits). CLASS #10309 RUN EXPLICITLY — and the path derivation did NOT name any of the three: check:error-code-casing ('✓ no unlisted lowercase error codes in 4414 scanned file(s)'), check:dispatcher-error-vocabulary ('OK — 21 unregistered code-stamping site(s), all classified'), check:route-envelope ('✓ 4 module(s) discovered and audited'; its 2 ratchet warnings are pre-existing and sit in packages/rest, not my paths). Screened every gate log for 'PREREQUISITE NOT MET' / 'Nothing was checked' / 'Command not found' / zero-match ERR_PNPM: no hits, so no refusal is being read as a pass. ABLATION — predictions written to a file BEFORE either mutation, both legs. Leg A (remove the fix, message-only rewrite): PREDICTED exactly 2 red, the disabled case and the both-routes parity case, 10 green; OBSERVED exactly those 2, 10 green, 'AssertionError: expected DOMAIN_VERIFICATION_FAILED to be DOMAIN_VERIFICATION_DISABLED'. Leg B (over-broad guard `if (resp.status === 404)`, dropping `&& !parsed?.code` — the hazard the PM named): PREDICTED exactly 1 red, the vendor-coded-404 case, 11 green; OBSERVED exactly that 1, 11 green, 'AssertionError: expected DOMAIN_VERIFICATION_DISABLED to be NO_PENDING_VERIFICATION'. Leg B is load-bearing precisely because the pre-existing #10716 pass-through test drives status 400, not 404, so it stays GREEN under that mutation — only the newly added leg catches it. HONEST CORRECTION: my written prediction mis-stated the totals as 10 tests (the suite has 12); I appended the correction to the prediction file when I saw it, and the predicted FAILING SETS — the falsifiable part — were exact on both legs. RESTORE HASHES, byte-identical both times: pristine 429a1399ba1cf23ee5708de0e3aeb2ca1fbbfff2; A mutated 19e47f3a6bd7e704c3227de43bf6ca7410bf399d -> restored 429a1399ba1cf23ee5708de0e3aeb2ca1fbbfff2 (ablation marker count 0); B mutated b155a69cfa25d8678bd6490a7e80664a0ddff008 -> restored 429a1399ba1cf23ee5708de0e3aeb2ca1fbbfff2 (marker count 0); FINAL 429a1399ba1cf23ee5708de0e3aeb2ca1fbbfff2. SRC-vs-DIST PROOF, argued from the files and re-verified rather than inherited from #10954: `ls -d packages/plugins/plugin-auth/dist` -> 'No such file or directory'; its vitest.config.ts contains no resolve.alias at all (whole file is environment+testTimeout); the test imports the subject as the RELATIVE specifier './register-sso-provider.js', which never consults the package exports map (that map points at dist/). So the resolved artifact IS src/ and no rebuild can silently invalidate a leg. Positive control for that claim: both ablations edited ONLY src/, ran NO build, and the suite results changed — which is impossible if the subject came from a dist/ that does not exist. By contrast the same file imports @objectstack/spec/api by PACKAGE specifier, which does resolve through exports to dist/, which is why the dependency closure had to be built first. ZERO-HIT COUNTER-CHECK with a positive control on the same corpus (6277 tracked files, CHANGELOGs excluded): searching for a site still pairing verify-domain's disabled case with 404 + the FAILURE code returns exactly 1 hit, and it is my own deliberately past-tense clause inside the updated dogfood note ('it answered 404 DOMAIN_VERIFICATION_FAILED before') — no live claim survives. Positive control on the identical corpus/tool: bare 'DOMAIN_VERIFICATION_FAILED' returns 9 files, so the zero is a measurement and not a broken search.",
      "open_questions": [
        {
          "question": "Is this `minor` or `major`? I shipped `minor` and argued both sides in the changeset and PR body rather than deciding it silently — it is genuinely a reviewer's call and this is a contract-review-tier PR.",
          "options": [
            "minor (shipped) — the emitted vocabulary gains no member (both codes already registered for @objectstack/plugin-auth), and the old pairing was self-contradictory (a code meaning 'verification failed' under a message saying 'the feature is off') rather than a contract anyone relied on deliberately. Same call #10716 made on these two routes.",
            "major — unlike #10716, whose old spellings were undeclared values no schema admitted, DOMAIN_VERIFICATION_FAILED IS a declared registered code, so a client keyed to it for the disabled case was keyed to something the published contract admitted; and BOTH halves of the answer change here (code 404->400 and status)."
          ],
          "recommendation": "minor, on the #10716 precedent — but I am explicitly NOT treating this as settled. check-changeset-no-major and check-adr-0087-registration both pass either way (they only assert no `major` is present), so no gate will catch this if the reviewer disagrees; it needs a human read during contract review."
        }
      ],
      "out_of_scope_findings": [
        "NOT filed — already open as #10615 ('[finding] @objectstack/plugin-auth's TEST_DEBT records 109 where tsc now reports 98 — an 11-error surplus'). Searched open issues before filing, per the file-first-search rule, and found it. My run re-measures the same standing condition slightly drifted: tsc now reports 97, so the surplus is 12, not 11. Not mine and not introduced here — scripts/check-type-check-coverage.mjs is untouched by this branch and its last three commits are #10896/#10876/#10820. I did not run `--lower`: it fails none of my gates, it is a different defect class from this card, and silently absorbing it would be unreported scope creep."
      ]
    }

    Generated by Claude Code

  6. os-warren commented on Aug 22, 2026

    @os-warren
    CollaboratorAuthor

    PM review of PR #11004. Not flipping ready — Clause-② means a non-dispatching seat PASSes first, and this seat does not clear needs:contract-review. Recording what I verified and flagging the one thing review must actually decide.

    The status question was answered, not skipped

    The dispatch said answering the code half and leaving the status half unexamined is not a complete discharge. It was answered with four measurements, and the third is the one that makes it a finding rather than a preference:

    no repo-wide 'capability off' status convention exists to appeal to — measured 403 FEEDS_DISABLED, 409 FLOW_DISABLED, 501 admin-plugin-off, 503 OIDC-off, 404 MCP-off, 400 the sibling

    Spot-checked by content and confirmed: FEEDS_DISABLED is 403 (plugin-audit/src/audit-writers.ts:1563 — "verbatim → 403 FEEDS_DISABLED, fail-closed like CLONE_DISABLED"), FLOW_DISABLED is 409 (packages/client/src/index.ts:3064, in the client's own status table). Six answers for one shape of condition. So nothing outweighed same-file, same-condition parity, and 400 follows.

    The other three legs are sound too: the file's own section header already required this treatment and only the sibling implemented it; the outer bridge route is mounted unconditionally, so only the inner endpoint is unmounted and passing its 404 through described the wrong resource; and nothing depends on the old 404.

    The ablation that mattered is the one for the hazard I named

    I flagged the over-broad guard — dropping && !parsed?.code so every 404 becomes DISABLED — as the way this fix goes wrong. Leg B tests exactly that, and the seat found why it needed testing at all:

    the pre-existing #10716 pass-through test drives status 400, not 404, so it stays GREEN under that mutation — only the newly added leg catches it

    Verified by content. sso-domain-verification-error-codes.test.ts:87 and :99 — both vendor-pass-through cases call fakeHandle(400, { code: VENDOR_CODE, … }). The existing suite is blind to a 404-shaped over-broad guard. That is an instrument limitation found and closed, not asserted.

    Both legs predicted in writing before mutating, both observed exactly, both restores byte-identical to 429a1399ba1cf23ee5708de0e3aeb2ca1fbbfff2 with the ablation marker count back to 0.

    Two more things done right

    The dist/ proof was re-verified, not inherited. I told the seat #10954 had measured plugin-auth as having no dist/ and to re-check rather than inherit it. It did: ls -d …/dist → no such file, vitest.config.ts carries no resolve.alias at all, the subject is imported by the relative specifier. Plus a positive control — both ablations edited only src/, ran no build, and the results moved, which is impossible against a dist/ that does not exist.

    The unbuilt-closure trap was resolved by building, not by arguing. @objectstack/dogfood typecheck first came back EXIT=2 with 227 errors, 9 of them in the file the seat had just edited — every one cascading from a single unresolved import because the closure was unbuilt. It built the closure and got to zero. The tempting move there is to declare a narrowing and explain the 9 away; it did the measurement instead.

    And the zero-hit counter-check carries its control: the search for a surviving site pairing verify-domain's disabled case with 404 + the failure code returns exactly 1 hit — the seat's own deliberately past-tense clause in the updated dogfood note — while the bare term returns 9 files on the identical corpus.

    One self-correction, recorded because it is the right shape: the written prediction mis-stated the suite totals (10 vs 12). The seat appended the correction on noticing, and the falsifiable half — the predicted failing sets — was exact on both legs. A prediction is load-bearing in its failure set, not its arithmetic.

    ⛔ The one question contract review must actually decide: minor or major

    Shipped minor, argued both ways in the open rather than decided silently. I am not settling it either, and I want to be explicit about why it needs a human read:

    No gate will catch a wrong answer here. check-changeset-no-major and check-adr-0087-registration both only assert that no major is present — they pass under either choice. This is exactly the "invisible to the gate" shape #10716's own dev flagged on a neighbouring question.

    The seat's distinction from #10716 is sharp and I think it is the crux: #10716's old spellings were undeclared values no schema admitted, so nothing could legitimately have depended on them. DOMAIN_VERIFICATION_FAILED is a registered code — a client keyed to it for the disabled case was keyed to something the published contract admits.

    My read, offered as input and not as a ruling: the argument for major is stronger than the changeset gives it credit for, and the reason is the status, not the code. A client branching on status === 404 for verify-domain now takes a different path, and that is a behaviour change independent of the code swap. Against that: the old pairing was self-contradictory — a code meaning "verification failed" carried under a message saying "the feature is off" — so anything keyed to it was keyed to a defect. Reasonable people land differently; the reviewer should land it deliberately.

    Housekeeping

    Declared narrowings: none — no gate refused, nothing skipped. Class #10309 confirmed live again: the derivation named none of check:error-code-casing, check:dispatcher-error-vocabulary, check:route-envelope; all three were run explicitly and all three exited 0.

    The plugin-auth TEST_DEBT surplus was not filed as a duplicate — #10615 already covers it. The seat searched first, found it, and recorded only the drift (surplus now 12, not 11) rather than opening a second card or silently running --lower.


    Generated by Claude Code

  7. huangyiirene commented on Aug 22, 2026

    @huangyiirene
    Collaborator

    Contract review — PASS (clause ②)

    Reviewing seat: skills PM (session_01ApyDuQY2fkunMCqXiqvBhR), machine-verified at tier this round (get_session → external_metadata.last_served_model = claude-fable-5 = CONTRACT_REVIEW_TIER on origin/main). Not the dispatching seat.

    Scope reviewed: the contract-face increment of draft PR #11004 at 93c4f7c9b, from the actual diff (4 files, +159/−8).

    Findings, checked against the diff:

    1. One condition, one answer — and only that condition. The new early return fires on exactly the disabled shape (404 and no vendor code) and answers the sibling's pair, 400 DOMAIN_VERIFICATION_DISABLED. The guard the services PM flagged as the hazard stays narrow: a 404 carrying a vendor code still passes through verbatim (code and status), an uncoded non-404 failure still answers DOMAIN_VERIFICATION_FAILED at the inner status, and both counter-directions are pinned — including on the sibling, so the parity assertion cannot be satisfied by both routes collapsing onto DISABLED.
    2. No vocabulary change. Both codes were already registered for @objectstack/plugin-auth with exactly these meanings; no packages/spec edit, correctly.
    3. The re-fixtured plugin-auth: register-sso-provider emits two lowercase wire-visible error codes (request_domain_verification_failed, verify_domain_failed) — ADR-0112 violation invisible to the casing gate #10716 test is right, not a weakening. The old generic-default leg was driven by the 404-no-code shape, which is now the disabled branch; repointing it at a real uncoded failure (502) is what keeps the default arm covered at all after the early return. The vendor pass-through arm is re-pinned at the 404 status this branch actually tests.
    4. Dogfood note updated deliberately — a note: string the bucket loop never asserts, so the update is the honest kind: nothing forced it.

    On the question review was asked to decide — minor stands. The ledger already registers DOMAIN_VERIFICATION_DISABLED as "domain verification is off on this deployment", so the published contract already named the right answer for this condition and the route was out of conformance with its own declared vocabulary — declared-vs-enforced restoration, the #10716 precedent's shape. The strongest major argument is the status half, but the old 404 was the vendor's unmounted-inner-endpoint status leaking through a bridge route that is mounted unconditionally — "no such endpoint" about a resource that exists is a defect being described, not a contract being kept. A client keyed to 404/FAILED for the disabled case was keyed to a self-contradictory answer its own sibling route never gave. The changeset's migration paragraph tells that client exactly what to match instead. Ruled minor; the maintainer can override at release time if they read the status half more strictly.

    Verdict: PASS. needs:contract-review cleared (four-step read-modify-write with read-back; label was issue-side only — PR #11004 carries none). Ready-flipping stays with the dispatching seat.


    Generated by Claude Code

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

Metadata

Metadata

Assignees

Labels

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions