Fix OIDC server_error after account delete + re-create - #453

src/idp/credentials.js:413

• The expired Set-Cookie strings omit Secure and SameSite=Lax, but the originals are set by oidc-provider with both attributes whenever the issuer is https or NODE_ENV=production (see src/idp/provider.js cookies.long/short config at lines 107–121). For pure deletion the browser only matches by (name, domain, path), so this generally works, but the inconsistency means proxies/intermediaries that enforce attribute parity (and some cookie-handling libraries used in tests) can keep the original cookie around. Mirroring Secure (when applicable) and SameSite=Lax would make the clear match the set, and is what the comment in interactions.js implies should happen.

  const expired = 'Path=/; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT; HttpOnly';
  reply.header('Set-Cookie', [
    `_session=; ${expired}`,
    `_session.sig=; ${expired}`,
    `_session.legacy=; ${expired}`,
    `_session.legacy.sig=; ${expired}`,
  ]);

src/idp/credentials.js:413

• reply.header('Set-Cookie', [...]) in Fastify replaces any prior Set-Cookie value already set on this response. If anything earlier in the request lifecycle (e.g. a future hook, oidc-provider middleware, or the success-rendering path in handleAccountDeleteForm) sets a cookie on the same reply, it will be silently dropped here. Using an append pattern (e.g. reply.raw.appendHeader/Fastify's getHeader('set-cookie') merge, or @fastify/cookie's clearCookie) would be safer than an unconditional overwrite.

  reply.header('Set-Cookie', [
    `_session=; ${expired}`,
    `_session.sig=; ${expired}`,
    `_session.legacy=; ${expired}`,
    `_session.legacy.sig=; ${expired}`,
  ]);

src/idp/cookies.js:36

• reply.header('Set-Cookie', [...]) in Fastify replaces any previously set Set-Cookie header value rather than appending. If any prior code on these account-deletion paths (e.g. error redirects, session middleware, or future additions) sets cookies on the same reply, those would be silently dropped by this call. Consider using reply.raw.appendHeader('Set-Cookie', cookie) per cookie (or accumulating an existing array) so the expiration headers are additive rather than destructive.

export function expireSessionCookies(reply, request) {
  const secure = request?.protocol === 'https' || process.env.NODE_ENV === 'production';
  reply.header('Set-Cookie', buildExpiredHeaders(secure));
}

src/idp/cookies.js:48

• The secure flag here is computed from request.protocol === 'https' or NODE_ENV === 'production', but the original cookies were set by oidc-provider with secure: process.env.NODE_ENV === 'production' || issuer.startsWith('https://') (provider.js:113,120). When the issuer is https://... but NODE_ENV is not production and the deletion request itself comes in via plain HTTP (e.g. behind a proxy that strips X-Forwarded-Proto, or local dev), the original cookie will have been set with Secure while the expiring header will not. Browsers don't require attribute-match for cookie deletion (only name/path/domain), so this typically still works — but the divergence between "set" and "expire" attribute logic is a maintainability hazard and could matter if cookie attributes (e.g. Domain) are added later. Consider deriving secure from the same condition used in provider.js, or sharing the predicate.

export function expireSessionCookies(reply, request) {
  const secure = request?.protocol === 'https' || process.env.NODE_ENV === 'production';
  reply.header('Set-Cookie', buildExpiredHeaders(secure));
}

/**
 * Expire oidc-provider session cookies on a Koa context.
 *
 * Used by renderError in provider.js for stale-session recovery,
 * where the response object is a Koa ctx, not a Fastify reply.
 *
 * @param {object} ctx - Koa context object
 */
export function expireSessionCookiesKoa(ctx) {
  const secure = ctx.secure || process.env.NODE_ENV === 'production';
  ctx.set('Set-Cookie', buildExpiredHeaders(secure));

src/idp/provider.js:421

• ctx.redirect() without an explicit status defaults to 302. If the originating request was a non-GET (e.g. POST to /idp/auth form-post mode, or the token endpoint), the browser will switch to GET on retry and lose the body, producing a confusing failure rather than the intended retry. Additionally, redirecting back to originalUrl for non-browser endpoints (token, userinfo, introspection) makes no sense — those errors are returned as JSON to a programmatic client, which won't follow the redirect or honor Set-Cookie. Consider gating the retry on ctx.method === 'GET' and/or on the route being the authorization endpoint.

      if (isStaleSessionCrash && !alreadyRetried) {
        expireSessionCookiesKoa(ctx);
        const separator = reqUrl.includes('?') ? '&' : '?';
        ctx.redirect(`${reqUrl}${separator}_stale_retry=1`);
        return;
      }

src/idp/provider.js:419

• The _stale_retry=1 query parameter is appended to the redirect URL but never stripped afterward. It will end up in browser history, server access logs, and (if the OIDC flow propagates the original request_uri/state somewhere) potentially in downstream redirects too. It's also exposed to the relying party in any error redirects back to the client. Consider documenting this side effect or using a short-lived cookie marker instead of a query param.

      const reqUrl = ctx.req?.originalUrl || ctx.request?.url || ctx.url || '';
      const alreadyRetried = reqUrl.includes('_stale_retry=1');

      if (isStaleSessionCrash && !alreadyRetried) {
        expireSessionCookiesKoa(ctx);
        const separator = reqUrl.includes('?') ? '&' : '?';
        ctx.redirect(`${reqUrl}${separator}_stale_retry=1`);