feat(registry): periodic hosted-origin re-verify + lapse→re-claim (#5749 gap #1 follow-up)#5762
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
684bebd to
d4e6654
Compare
There was a problem hiding this comment.
Approving. Lapse→re-claim is the right shape: it relaxes the staleness of workos_organization_id, never the bind gate. You still only become owner by proving origin control with your own adcp_claim token.
Things I checked
- No takeover vector. A lapse is only reachable through the verifier's permanent-failure branches (4xx / invalid JSON / missing-or-mismatched
authoritative_location) — each a verdict on the publisher's own origin.safeFetchrunsredirect: 'manual'restricted to the same registrable domain, so an off-path attacker can't redirect AAO's probe to flip a verified row. Transient (5xx/429/3xx/timeout/ECONN) preserves verified state.security-reviewer: LOW, bind invariant preserved end-to-end. - Lock holds pre-lapse. While verified+locked to ORG_A,
issueDomainClaim(property-db.ts:920-926) refuses a different org's claim, so ORG_B can't even plant aclaim_token. AndbindOwnerFromVerifiedClaim's WHERE fails all three OR branches for ORG_B on a live lock. - The ordering fix is load-bearing. Moving
stampChecked(true)to afterbindOwnerFromVerifiedClaim(hosted-property-origin-verifier.ts:303-317) is the crux: stamp-first would re-armorigin_verified_at, making the newOR origin_verified_at IS NULLguard false and blocking the legitimate re-bind. First-time binds still pass via theworkos_organization_id IS NULL OR = claimant_org_idclauses. - New clause is tightly scoped.
origin_verified_at IS NULLcovers exactly never-verified rows (alreadyworkos_organization_id IS NULL→ no widening) and genuinely-lapsed rows. No live lock hasorigin_verified_at IS NULL. A NULL-verified row with no pending claim binds no one. - TTL work-list query (
property-db.ts:990-1015):origin_verified_at IS NOT NULLexcludes never-verified rows;ORDER BY origin_last_checked_at ASC NULLS FIRST+LIMITdrains oldest first. BothrecordOriginVerificationandtouchOriginLastCheckedAtstamporigin_last_checked_at = NOW(), so every outcome moves a row out of the window — no re-selection loop. - Lifecycle + reentrancy (
crawler.ts):unref()keeps the hourly tick from holding the process open,.catchswallows tick rejections, and thehostedReverifyProcessingguard sits flush against thetrywith afinallythat always clears it. Thelapsedcount!outcome.verified && outcome.reason !== 'transient'is correct against theVerificationOutcomeunion. - brand-handlers UTC math (
brand-handlers.ts:940-952): reject only onendMs < startOfTodayMskeeps a campaign ending today licensable, matching the inclusive23:59:59Zsuccess path. (This rides in from the #5760 stack and drops out on merge.)
Follow-ups (non-blocking — file as issues)
- Cross-process bind/clear race (
security-reviewer: MEDIUM, defensive). ThehostedReverifyProcessingflag only serializes ticks within one process. Two concurrent verifier runs on the same domain — the hourly job overlapping a user-triggeredverify-origin, or two instances — read-decide-write onorigin_verified_atwith no row lock, each in its own autocommit txn. No takeover (a bind still needs a matching origin token), but it can flap lock state. Wrap the read-decide-write behindSELECT ... FOR UPDATEor an optimisticWHERE origin_last_checked_at = <read-value>guard before this is heavily relied on. Already implied by your "multi-pending-claim griefing table" follow-up.
Minor nits (non-blocking)
- Batch callback has no per-item try/catch.
processHostedOriginReverification'sprocessWithConcurrencycallback (crawler.ts) doesn't wrap its body, unlike the two sibling call sites atcrawler.ts:2068and:2177. An unexpected throw fromverifyHostedPropertyOriginaborts the whole batch's accounting rather than degrading to a per-item skip. The verifier catches its own failures and each item already persistedorigin_last_checked_at, so it's robustness, not correctness.
No schema source touched, so no changeset owed. Bind/lapse/re-bind cycle and the TTL inclusion+exclusion are both covered by behavioral tests.
LGTM. Follow-ups noted below.
There was a problem hiding this comment.
Lapse→re-claim is the right shape: clearing the lock without deleting the row, and keeping the re-bind token-gated, means a transferred domain becomes reclaimable without ever becoming takeable. Verification drives ownership, not the caller.
Things I checked
- Re-bind stays origin-driven.
bindOwnerFromVerifiedClaim(property-db.ts:986-988) widens only which existingworkos_organization_idmay be overwritten viaOR origin_verified_at IS NULL. The bind target is stillworkos_organization_id = claimant_org_idgated byclaim_token = $2 AND claimant_org_id IS NOT NULL. A lapse clears the lock; it does not waive the requirement that the new claimant'sadcp_claimtoken be served at the real origin.security-reviewer: not a takeover primitive, tenant isolation holds. - Verifier ordering. Moving
bindOwnerFromVerifiedClaimahead ofstampChecked(true)(hosted-property-origin-verifier.ts:303→328) is correct, not just convenient. The bind'sUPDATE ... WHERE ... RETURNINGreadsorigin_verified_atfrom the row inside the same statement; stamping verified first would re-arm the lock and block the legitimate re-bind. The bind is atomic — no mis-bind race between two concurrent verifications. - Lapse classification.
crawler.ts:2247—!outcome.verified && outcome.reason !== 'transient'exactly partitions the verifier's outcomes: every permanent reason routes throughstampChecked(false)(clearsorigin_verified_at), onlytransienthitsstampChecked('preserve').processWithConcurrencywritesresults[i]by captured index, so.filter(Boolean).lengthis an honest lapse count. - Work-list query.
getHostedPropertiesDueForReverification(property-db.ts:990-996) scopes toorigin_verified_at IS NOT NULL— unverified/community rows have no verification state to lapse and are correctly excluded. Test asserts exactly that. - DNS classification.
EAI_AGAINnow pinned transient before the permanent regex, andEAI_narrowed toEAI_NONAME(hosted-property-origin-verifier.ts:142) — a temporary resolver blip no longer permanently demotes a verified publisher. Right direction. brand-handlers.ts:940-952—endMs < startOfTodayMs(midnight-UTC-today) matches the success path's inclusivevalid_until = ...T23:59:59Z. A campaign ending today stays licensable. Test-plan: PR reports bind 7/7, verifier 10/10, typecheck clean; no unchecked manual checkbox on the path this PR changes.- Changeset. None present, but CI's
check-changeset-protocol-scope.cjsenforces changesets only forstatic/schemas/source/**/docs/reference/**. This PR isserver/src/**+ tests only — no wire surface, no changeset required. Not a block.
Follow-ups (non-blocking — file as issues)
- Reclassify SSRF/private-IP rejections as transient.
classifyFetchError(hosted-property-origin-verifier.ts:142) matches the substringprivate→ permanent → lapse. A victim whose DNS an attacker can influence (rebinding to a private IP) trips the SSRF guard's"resolved to a private or internal IP address", which lapses the lock and demotes promoted authorizations. This is denial-of-ownership, not takeover (re-bind still needs origin control), and the regex pre-dates this PR — but the new hourly job now fires it automatically instead of only on a manualverify-origin. An origin resolving to a private IP is an attack signal, not "I no longer point at AAO" — route it to transient. (security-reviewerMedium.) - Add failure-count hysteresis before lapsing. The lapse fires on the first permanent-classified response (
:226-230,:254-256) — no "N consecutive failures over M hours." Combined with the above, a single spoofed NXDOMAIN releases a verified owner's lock. Anorigin_consecutive_failurescounter reset on any success/transient would close it. (security-reviewerMedium.)
Minor nits (non-blocking)
- Double-start leaks the interval.
startPeriodicHostedOriginReverification(crawler.ts:2228) overwriteshostedReverifyIntervalIdwithout clearing an existing timer. Only called once fromhttp.ts:9585, so not reachable today — guard it anyway if a restart path ever appears. stopPeriodicHostedOriginReverificationis defined but never called (crawler.ts:2230). Matches the siblingstartPeriodicManagerRevalidationpattern and.unref()keeps it from blocking exit, so this is hygiene-only.
Approving on the strength of the token-gated re-bind plus the atomic bind ordering. Two security Follow-ups are availability-only and pre-existing levers — worth an issue, not a hold.
Security review — clean (no takeover possible)A
Minor findings (all fixed in
|
gap #1 follow-up) Bind-on-verify (#5752) binds a domain to its owner when the origin pointer points at AAO. But domains change hands and publishers remove pointers, so a verified row needs re-checking on a TTL — otherwise a transferred domain keeps its stale owner forever and can never be re-claimed. - Periodic re-verify job (crawler): startPeriodicHostedOriginReverification re-runs verifyHostedPropertyOrigin over verified rows past a 24h TTL, bounded batch + concurrency, mirroring the manager-revalidation pattern. Wired into startup (hourly tick). A permanent failure (origin no longer points at AAO) clears origin_verified_at via the existing verifier path — the owner lock lapses. Transient failures (5xx/429/timeout) leave verified state intact by design. - Lapse → re-claim: bindOwnerFromVerifiedClaim now also binds when origin_verified_at IS NULL, so a lapsed domain's stale workos_organization_id no longer blocks a new owner from re-binding. issueDomainClaim already keys its refusal on origin_verified_at, so a lapsed domain is re-claimable. - Verifier ordering fix: bind BEFORE stamping origin_verified_at, so the bind guard sees the pre-verification (lapsed) state rather than the lock it's about to re-arm. (Found by the re-bind test.) - getHostedPropertiesDueForReverification: the work-list query (verified rows past the TTL; unverified rows are never candidates). - The row is never deleted on lapse — the rid is forever; verification and the owner binding lapse, content stays. Tests: lapse releases the lock and lets a new owner re-claim and re-bind; the due-for-reverification query excludes unverified rows. Existing bind + verifier suites unchanged (7/7 and 10/10). Stacked on #5760 (the time-bomb fix) — that commit drops out on rebase once #5760 merges. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d4e6654 to
086d457
Compare
There was a problem hiding this comment.
The lapse→re-claim mechanism is the right shape — binding stays anchored to which token the real origin carries, and the lapse clears only origin_verified_at, never workos_organization_id, so a transferred domain is re-claimable but a takeover still requires origin control. One blocker in the failure classification undercuts the PR's own central claim.
MUST FIX
classifyFetchError lapses a legitimately-verified domain on a transient DNS failure. hosted-property-origin-verifier.ts:152 adds could not resolve hostname to the unresolvable (permanent) set. But safeFetch resolves DNS through validateHostResolution (utils/url-security.ts:174-186), which Promise.allSettleds resolve4/resolve6 and throws the single literal string "Could not resolve hostname" whenever both reject — and both reject for transient EAI_AGAIN/SERVFAIL exactly as they do for permanent NXDOMAIN. So a routine resolver hiccup now classifies as permanent → stampChecked(false) clears origin_verified_at → the re-verify job counts a "lapse," demotes the promoted authorizations, and marks the row re-claimable.
This violates the verifier's own load-bearing invariant — hosted-property-origin-verifier.ts:44-47 and :209-211: "a network blip MUST NOT flip a previously-verified row to unverified." And the PR body's "a lapse only happens when the real origin stops pointing at AAO" is false as implemented: a SERVFAIL is not the origin ceasing to point at AAO.
What breaks for adopters: a verified publisher with a momentary DNS resolver outage loses its origin attestation (demoted authorizations) and is marked re-claimable. Because the lapse stamps origin_last_checked_at = NOW(), the row won't be re-checked for the full 24h TTL — so the false lapse persists up to 24h unless verify-origin is manually re-triggered. No takeover results (re-binding still needs origin control, confirmed by security-reviewer — verdict otherwise sound, no High), but the demotion + re-claim window is a concrete, reproducible regression.
The EAI_AGAIN → transient early-guard added in this same diff is a precise guard against a string that never reaches this function in production — it reads as a safety net that isn't load-bearing.
Fix: don't treat the collapsed "could not resolve hostname" as unconditionally permanent. Either (a) require N consecutive failures across ticks before clearing, or (b) classify on a typed cause (err.cause?.code — ENOTFOUND/EAI_NONAME = permanent, EAI_AGAIN/SERVFAIL = transient) rather than the message string, which means having validateHostResolution distinguish NXDOMAIN from temporary-failure and propagate it.
Things I checked
- Bind-before-stamp ordering is correct. First-verify:
origin_verified_atis NULL pre-stamp → OR-clause permits the intended first bind. Owner re-verify:claim_tokenis NULL in the DB after the first bind, soclaim_token = $2never matches and the bind is a correct no-op (property-db.ts:979). origin_verified_at IS NULLOR-clause is idempotent and still token-gated.bindOwnerFromVerifiedClaim(property-db.ts:983-987) keepsclaim_token = $2 AND claimant_org_id IS NOT NULLmandatory and nulls the token on bind; the new clause only relaxes the stale-org-id guard for genuinely-lapsed rows.- Staging vector is closed.
issueDomainClaim(property-db.ts:920-926) refuses a token to a different org whileorigin_verified_atis set — an attacker can't pre-stage a token to race a lapse. - Work-list excludes unverified rows.
getHostedPropertiesDueForReverificationfiltersorigin_verified_at IS NOT NULL; test confirms the community/unverified row is never a candidate. - TTL advances on every outcome. Both
recordOriginVerification(property-db.ts:874-875) andtouchOriginLastCheckedAt(:894) stamporigin_last_checked_at— no row gets re-checked every tick. - Job plumbing mirrors manager-revalidation.
unref(), re-entrancy guard, boundedBATCH_SIZE=50/CONCURRENCY=4,.catch()on the tick,lapsedcount keyed onreason !== 'transient'— all sound (crawler.ts:2225-2284).
Follow-ups (non-blocking — file as issues)
- Bind + stamp aren't transactional (
security-reviewerMedium). The bind UPDATE andrecordOriginVerification(domain, true)are two un-transactioned writes; a concurrent lapse tick or crash between them can leave(workos_organization_id set, origin_verified_at NULL, is_public TRUE)— publicly serving while re-claimable. Fold the verified stamp into the bind UPDATE's RETURNING, or wrap both in one transaction. - Flapping dwell. Consider a minimum dwell on
origin_verified_at = NULLbefore honoring a re-bind so a flapping origin can't churn ownership. Hardening, not exploitable.
Minor nits (non-blocking)
EAI_narrowed toEAI_NONAME.hosted-property-origin-verifier.ts:152— the old broadEAI_matchedEAI_FAIL/EAI_NODATA; those now fall through totransient. Direction is safe (keeps the lock), but the narrowing is silent — enumerate or comment it.- Redundant
?.onunref().crawler.ts—setIntervalalways returns a Timeout;this.hostedReverifyIntervalId?.unref()only matters under a future refactor.
Fix the classification (or justify why the transient-DNS demote is acceptable) and this is a clean ship. The lapse design itself is right — it's the permanent-vs-transient boundary that's mislabeled.
There was a problem hiding this comment.
Approving. The core security question — does OR origin_verified_at IS NULL in bindOwnerFromVerifiedClaim open a domain-takeover path — resolves no: the clause widens when a row is re-claimable, never what binding requires. Re-binding still demands the claim token appear at the real origin, so a lapse can't be parlayed into taking over a domain the attacker doesn't control.
Things I checked
- Bind-before-stamp is load-bearing and correct. Moving
bindOwnerFromVerifiedClaimahead ofstampChecked(true)(hosted-property-origin-verifier.ts:308-317, stamp at:347) means the bind guard reads the live pre-verificationorigin_verified_at IS NULL, so a lapsed row is re-bindable. Stamping first would re-arm the lock and block the legitimate re-bind. The comment explains it; the re-bind test catches the regression. - Classification fails closed toward preserving the lock.
classifyFetchErrortests the transient regex (EAI_AGAIN|ESERVFAIL|ETIMEOUT|ECONNREFUSED) before the unresolvable one, andcollectErrorTextflattensmessage/code/codes[]/thecausechain into one string — so a mixedcodes: ['EAI_AGAIN','ENOTFOUND']resolves transient.dnsResolutionFailureCause(url-security.ts:191-211) setscodewith precedencetransient ?? permanent ?? first. A bare "Could not resolve hostname" with no DNS code is deliberately transient. Every ambiguous case keeps the legit owner's lock — under-lapse, not over-lapse. - Lapse never reassigns ownership. A permanent failure only NULLs
origin_verified_atviarecordOriginVerification(domain, false);workos_organization_idis untouched until a new origin-attested token binds it.issueDomainClaim's refusal and the bind guard are now symmetric onorigin_verified_at. - SSRF enforcement unchanged. The
url-security.tschanges are additive error-classification plumbing (extractDnsErrorCode,dnsResolutionFailureCause,unresolvedHostnameError);isPrivateHostname, per-address private-IP rejection,ssrfSafeLookup, and per-hop redirect re-validation are all intact. The 3xx comment change is documentation-only. - Job is bounded.
processHostedOriginReverification(crawler.ts) usesBATCH_SIZE=50against aLIMIT $2query,CONCURRENCY=4via the existingprocessWithConcurrencypool, a try/finallyhostedReverifyProcessingguard, andunref()on the interval.getHostedPropertiesDueForReverificationcorrectly excludes unverified rows (origin_verified_at IS NOT NULL) — nothing to lapse there. - No changeset in the PR — not required: the diff touches only
server/src/**and tests, none of the wire-surface paths. Server runtime behavior, not a wire change.
Follow-ups (non-blocking — file as issues)
- The job runs on a single instance's
setIntervalwith an in-processhostedReverifyProcessingflag. With multiple replicas there's no cross-instance lock, so each replica re-verifies the due batch — harmless (idempotent verify, last-writer-wins SQL), just redundant origin fetches.SELECT ... FOR UPDATE SKIP LOCKEDon the work-list query if replica count grows. - The lapse path releases an owner lock on a single permanent-classified re-verify with no grace window. The classifier is conservative enough that this is fine, but a "two consecutive permanent failures before lapse" policy would harden against a one-off resolver anomaly that happens to surface a permanent code.
Minor nits (non-blocking)
- Document the per-process guard. One line on
hostedReverifyProcessingnoting it's per-instance and cross-replica overlap is safe-but-wasteful, so a future reader doesn't mistake it for a distributed lock.crawler.ts.
Third re-verify follow-up in the registry ownership series and it lands the same way the prior two did — tight diff, the tradeoff named in the body, the invariant (ownership only ever established by an origin-attested token) preserved. LGTM.
Follow-up to #5752 (bind-on-verify). Closes the domain-transfer / stale-pointer window the security review flagged.
Problem
Bind-on-verify binds a domain to its owner while the origin pointer points at AAO. But domains change hands and publishers remove pointers — without re-checking, a transferred domain keeps its stale owner forever and can never be re-claimed (the stale
workos_organization_idwould block a new owner's bind).Change
crawler.ts):startPeriodicHostedOriginReverificationre-runsverifyHostedPropertyOriginover verified rows past a 24h TTL (bounded batch + concurrency, mirroring manager-revalidation), wired into startup as an hourly tick. A permanent failure (origin no longer points at AAO) clearsorigin_verified_atvia the existing verifier path — the owner lock lapses. Transient failures (5xx/429/timeout) leave verified state intact by design.property-db.ts):bindOwnerFromVerifiedClaimnow also binds whenorigin_verified_at IS NULL, so a lapsed domain's staleworkos_organization_idno longer blocks a new owner.issueDomainClaimalready keys its refusal onorigin_verified_at, so a lapsed domain is re-claimable.origin_verified_at, so the bind guard sees the pre-verification (lapsed) state instead of the lock it's about to re-arm. (Caught by the re-bind test.)getHostedPropertiesDueForReverification: the work-list query — verified rows past the TTL; unverified rows are never candidates.Why it's not a takeover vector
A lapse only happens when the real origin stops pointing at AAO (404/mismatch) — an attacker can't force it (no origin control). After a legit lapse, re-binding still requires making the origin point at the new claimant's token — which still requires origin control. The
origin_verified_at IS NULLguard clause only widens re-claim for genuinely-lapsed domains.Tests
New: lapse releases the lock and lets a new owner re-claim + re-bind; due-for-reverification excludes unverified rows. Existing bind (now 7/7) + verifier (10/10) suites green; typecheck clean.
Follow-ups
DNS-TXT pointer flavor; multi-pending-claim griefing table; owner-confirmation of staged
properties[]on bind; extend owner-lock to Addie/admin write paths (#5751).🤖 Generated with Claude Code