Fix host-only cookie shadowing OTP loop (#116)

e2e/step-definitions/session-reuse-bugs.steps.ts

@@ -478,3 +478,89 @@ When(
       .click()
   },
 )
+
+// ---------------------------------------------------------------------------
+// Stale host-only cookies surviving from sessions predating PR #103
+// (GitHub issue #116).
+//
+// Pre-PR-#103 sign-ins set dev-id/ses-id host-only on the pds-core host.
+// A long-time user's jar still holds those entries indefinitely — and
+// post-PR-#103, the host-only stale pair shadows everything because the
+// `cookie` package's parse() keeps the first occurrence per name and
+// per RFC 6265 host-only comes first in the Cookie header.
+//
+// These helpers establish the affected-user starting state directly:
+// only the host-only stale pair exists, no Domain-scoped pair, no live
+// device session in the DB. We unconditionally clear the cookie jar
+// first so the step works whether the preceding Background left it
+// empty (e.g. "a returning user has a PDS account" resets the context)
+// or populated (e.g. "the user has completed one OAuth sign-in" left
+// a fresh Domain-scoped pair). The post-plant assertions then prove
+// only the host-only entries we placed are present.
+// ---------------------------------------------------------------------------
+
+const STALE_HOST_ONLY_DEV_ID = 'dev-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
+const STALE_HOST_ONLY_SES_ID = 'ses-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'
+
+Given(
+  'the browser jar holds only a stale host-only dev-id and ses-id pair',
+  async function (this: EpdsWorld) {
+    const page = getPage(this)
+    const ctx = page.context()
+    const pdsHost = new URL(testEnv.pdsUrl).host
+
+    // Wipe any cookies a preceding Background step deposited. The
+    // affected-user starting state has nothing on the jar except the
+    // stale host-only pair; if the Background ran "the user has
+    // completed one OAuth sign-in" first, there'd be a fresh
+    // Domain-scoped pair to remove.
+    await ctx.clearCookies()
+
+    // Playwright's cookie API treats `domain` without a leading dot as
+    // host-only — exactly the scope an upstream-set cookie used pre-PR-#103.
+    await ctx.addCookies([
+      {
+        name: 'dev-id',
+        value: STALE_HOST_ONLY_DEV_ID,
+        domain: pdsHost,
+        path: '/',
+        httpOnly: true,
+        secure: true,
+        sameSite: 'Lax',
+      },
+      {
+        name: 'ses-id',
+        value: STALE_HOST_ONLY_SES_ID,
+        domain: pdsHost,
+        path: '/',
+        httpOnly: true,
+        secure: true,
+        sameSite: 'Lax',
+      },
+    ])
+
+    // Sanity-check: the jar should now hold exactly one dev-id and one
+    // ses-id, both host-only. If something else is there (a leftover
+    // Domain-scoped pair from a previous scenario, etc.) we want a clear
+    // failure here rather than a confusing assertion downstream.
+    const all = await ctx.cookies()
+    const devEntries = all.filter((c) => c.name === 'dev-id')
+    const sesEntries = all.filter((c) => c.name === 'ses-id')
+    expect(
+      devEntries.length,
+      `Expected exactly 1 dev-id entry (host-only stale) after planting, got ${devEntries.length}: ${JSON.stringify(devEntries)}`,
+    ).toBe(1)
+    expect(
+      sesEntries.length,
+      `Expected exactly 1 ses-id entry (host-only stale) after planting, got ${sesEntries.length}: ${JSON.stringify(sesEntries)}`,
+    ).toBe(1)
+    expect(
+      devEntries[0].domain,
+      `dev-id should be host-only on ${pdsHost} but Playwright reports domain=${devEntries[0].domain}`,
+    ).toBe(pdsHost)
+    expect(
+      sesEntries[0].domain,
+      `ses-id should be host-only on ${pdsHost} but Playwright reports domain=${sesEntries[0].domain}`,
+    ).toBe(pdsHost)
+  },
+)

features/session-reuse-bugs.feature

@@ -95,6 +95,34 @@ Feature: Session-reuse resilience against stale device cookies
     Then the browser lands on the auth-service email-and-OTP form
     And the response clears the dev-id and ses-id cookies
 
+  # ---------------------------------------------------------------------------
+  # Pre-PR-#103 host-only cookies shadow the freshly-set Domain-scoped pair.
+  # Reproduces GitHub issue #116. A user whose browser jar holds a host-only
+  # dev-id/ses-id pair from before cookie-domain broadening shipped — when
+  # the cookie parser keeps the first occurrence per name and per RFC 6265
+  # §5.4 the host-only stale entry comes first — the welcome-page-guard
+  # validates the wrong values, bounces to auth-service with prompt=login,
+  # and the user loops on the OTP form. The scenario's Given clears the
+  # jar (overriding what the Background's OAuth sign-in deposited) and
+  # plants only the stale host-only pair, matching the actual state of
+  # an affected user's browser.
+  # ---------------------------------------------------------------------------
+
+  Scenario: Stale host-only cookies don't trap the user in an OTP loop
+    # The Background's account-creation step recorded the trusted demo as
+    # an authorised client (PDS_SIGNUP_ALLOW_CONSENT_SKIP), so a returning
+    # login skips consent and lands on /welcome. Pre-fix the user never
+    # gets there — every post-callback /oauth/authorize hop bounces
+    # because the host-only stale pair shadows the freshly-set
+    # Domain-scoped pair.
+    Given the browser jar holds only a stale host-only dev-id and ses-id pair
+    When the demo client starts a new OAuth flow
+    And the user enters the test email on the login page
+    Then the login page shows an OTP verification form
+    And an OTP email arrives in the mail trap
+    When the user enters the OTP code
+    Then the browser is redirected back to the demo client
+
   # ---------------------------------------------------------------------------
   # Flow 1 hint-vs-bindings gate: when login_hint resolves to an email that
   # is not bound to the current device, auth-service must skip session reuse

packages/pds-core/src/__tests__/cookie-domain.test.ts

@@ -103,6 +103,58 @@ describe('rewriteSetCookie (HYPER-268)', () => {
     expect(rewriteSetCookie('just-a-name', domain)).toBe('just-a-name')
   })
 
+  describe('clearing cookies (issue #116)', () => {
+    // Clears must pass through unchanged so callers can emit a host-only
+    // clear (no Domain=) alongside a Domain-scoped clear and have the
+    // browser actually evict the host-only entry. Auto-injecting Domain=
+    // here makes the host-only clear identical to the Domain-scoped one
+    // and the host-only stale cookie survives forever.
+
+    it('passes through a Max-Age=0 host-only clear unchanged', () => {
+      const input = 'dev-id=; Max-Age=0; Path=/'
+      expect(rewriteSetCookie(input, domain)).toBe(input)
+    })
+
+    it('passes through a Max-Age=0 clear for each device cookie name', () => {
+      for (const name of ['dev-id', 'ses-id', 'dev-id:hash', 'ses-id:hash']) {
+        const input = `${name}=; Max-Age=0; Path=/`
+        expect(rewriteSetCookie(input, domain)).toBe(input)
+      }
+    })
+
+    it('passes through Max-Age=0 with surrounding whitespace', () => {
+      const input = 'dev-id=;  Max-Age = 0 ; Path=/'
+      expect(rewriteSetCookie(input, domain)).toBe(input)
+    })
+
+    it('passes through a clear with a past Expires date', () => {
+      const input = 'dev-id=; Path=/; Expires=Thu, 01 Jan 1970 00:00:00 GMT'
+      expect(rewriteSetCookie(input, domain)).toBe(input)
+    })
+
+    it('still injects Domain on a normal Set-Cookie with a future Expires', () => {
+      const future = new Date(
+        Date.now() + 365 * 24 * 60 * 60 * 1000,
+      ).toUTCString()
+      const input = `dev-id=abc; Path=/; Expires=${future}`
+      expect(rewriteSetCookie(input, domain)).toBe(
+        `${input}; Domain=pds.example`,
+      )
+    })
+
+    it('still injects Domain on a normal Set-Cookie with positive Max-Age', () => {
+      const input = 'dev-id=abc; Max-Age=3600; Path=/'
+      expect(rewriteSetCookie(input, domain)).toBe(
+        `${input}; Domain=pds.example`,
+      )
+    })
+
+    it('does not double-inject on a Domain-scoped clear', () => {
+      const input = 'dev-id=; Max-Age=0; Path=/; Domain=pds.example'
+      expect(rewriteSetCookie(input, domain)).toBe(input)
+    })
+  })
+
   it('exports the set of device cookie names', () => {
     expect(DEVICE_COOKIE_NAMES.size).toBe(4)
     expect(DEVICE_COOKIE_NAMES.has('dev-id')).toBe(true)

packages/pds-core/src/cookie-domain.ts

@@ -61,6 +61,14 @@ export const DEVICE_COOKIE_NAMES = new Set<string>([
  * - Returns the input unchanged if it doesn't target a device cookie.
  * - Returns the input unchanged if it already has a `Domain=` attribute
  *   (case-insensitive) — never double-scopes.
+ * - Returns the input unchanged if it is a clearing cookie (`Max-Age=0`
+ *   or a past `Expires`). Browsers only clear a host-only cookie if the
+ *   clearing Set-Cookie itself carries no `Domain=`; auto-scoping a
+ *   clear would silently neuter callers (e.g. welcome-page-guard) that
+ *   intentionally emit BOTH a host-only clear and a Domain-scoped clear
+ *   to evict cookies in both scopes. Without this guard the host-only
+ *   variant of a stale device cookie can never be removed once the
+ *   middleware is installed — see GitHub issue #116.
  * - Otherwise appends `; Domain=<domain>` to the end of the value.
  *
  * Set-Cookie values look like "name=value; Path=/; HttpOnly; Secure".
@@ -75,9 +83,28 @@ export function rewriteSetCookie(value: string, domain: string): string {
   if (!DEVICE_COOKIE_NAMES.has(name)) return value
   // Already has Domain attribute? Don't double-inject.
   if (/;\s*Domain=/i.test(value)) return value
+  if (isClearingCookie(value)) return value
   return `${value}; Domain=${domain}`
 }
 
+/**
+ * True when a Set-Cookie value expresses an explicit cookie eviction
+ * — either `Max-Age=0` (RFC 6265 §5.2.2: "If delta-seconds is less
+ *  than or equal to zero (0), let expiry-time be the earliest
+ *  representable date") or a past `Expires=` date. Numeric `Max-Age`
+ *  is the canonical form upstream uses for host-only/Domain-scoped
+ *  clears in welcome-page-guard.
+ */
+function isClearingCookie(value: string): boolean {
+  if (/;\s*Max-Age\s*=\s*-?0+\b/i.test(value)) return true
+  const expiresMatch = /;\s*Expires\s*=\s*([^;]+)/i.exec(value)
+  if (expiresMatch) {
+    const ts = Date.parse(expiresMatch[1].trim())
+    if (!Number.isNaN(ts) && ts <= Date.now()) return true
+  }
+  return false
+}
+
 /**
  * Apply {@link rewriteSetCookie} to a Set-Cookie header value, which
  * Node's http module allows to be either a single string or an array

packages/pds-core/src/index.ts

@@ -136,6 +136,16 @@ async function main() {
   const handleDomain = process.env.PDS_HOSTNAME || 'localhost'
   const pdsUrl = cfg.service.publicUrl || `https://${handleDomain}`
 
+  // The shared parent domain for cross-subdomain device-session cookies, or
+  // null when auth-service and pds-core are on unrelated hostnames (e.g.
+  // Railway preview envs, where both services live under up.railway.app
+  // and cookies stay host-only). Computed once and reused for both the
+  // cookie-domain broadening middleware and the host-only-twin clear in
+  // /oauth/epds-callback. When null, the broadening is a no-op so device
+  // cookies are themselves host-only — emitting host-only clears in that
+  // case would evict the freshly-minted session cookies.
+  const cookieDomain = deriveCookieDomain(authHostname, handleDomain)
+
   const pds = await PDS.create(cfg, secrets)
   const ctx = pds.ctx
   const provider = ctx.oauthProvider
@@ -250,6 +260,31 @@ async function main() {
       )
       const { deviceId, deviceMetadata } = deviceInfo
 
+      // Step 1b (issue #116): when this deployment broadens device-session
+      // cookies to a shared parent domain, evict any host-only twin that
+      // a pre-PR-#103 session may have left in the browser jar. Browsers
+      // store host-only and Domain-scoped cookies of the same name as
+      // distinct entries; when both are present, the cookie parser picks
+      // the host-only one first per RFC 6265 §5.4 ordering, shadowing the
+      // fresh Domain-scoped pair we just emitted in Step 1. The explicit
+      // host-only Max-Age=0 clear here forces the browser to evict the
+      // stale twin before the very next request, so the welcome-page
+      // guard at /oauth/authorize sees only the fresh pair. Idempotent —
+      // emitting clears for cookies that don't exist is a no-op.
+      // The cookie-domain middleware passes Max-Age=0 lines through
+      // unchanged (also added in #116) so these clears reach the browser
+      // without a Domain= attribute and match the host-only scope.
+      // Skipped on deployments where the cookie-domain broadening is a
+      // no-op (auth-service and pds-core on unrelated hostnames, e.g.
+      // Railway preview envs under up.railway.app) — there the device
+      // cookies set in Step 1 are themselves host-only, so emitting a
+      // host-only clear would evict them.
+      if (cookieDomain) {
+        for (const name of ['dev-id', 'dev-id:hash', 'ses-id', 'ses-id:hash']) {
+          res.append('Set-Cookie', `${name}=; Max-Age=0; Path=/`)
+        }
+      }
+
       // Step 2: Refresh the PAR request expiry timer.
       // Call get() WITHOUT deviceId so it doesn't bind one — the stock
       // oauthMiddleware will bind the browser's deviceId when we redirect
@@ -653,14 +688,10 @@ async function main() {
   // and bounces to auth-service with stale cookies cleared when either
   // check fails. All other requests pass through unchanged.
 
-  const welcomeGuardCookieDomain = deriveCookieDomain(
-    authHostname,
-    handleDomain,
-  )
   const welcomePageGuardMiddleware = createWelcomePageGuard({
     authHostname,
     provider: provider ?? null,
-    cookieDomain: welcomeGuardCookieDomain,
+    cookieDomain,
     logger,
   })
   pds.app.use(welcomePageGuardMiddleware)
@@ -864,7 +895,6 @@ async function main() {
   // Upstream's DeviceManager has no domain option, so we rewrite headers
   // rather than pass config.
 
-  const cookieDomain = welcomeGuardCookieDomain
   if (cookieDomain) {
     const cookieDomainMiddleware = createCookieDomainMiddleware(cookieDomain)