Skip to content

[oss-candidate] fix(selfhost): fetch SSO UserInfo for thin ID tokens - #1

Closed
askalf wants to merge 7 commits into
mainfrom
fix/sso-userinfo-email-verified-fallback
Closed

askalf wants to merge 7 commits into
mainfrom
fix/sso-userinfo-email-verified-fallback

Conversation

@askalf

@askalf askalf commented Sep 15, 2026

Copy link
Copy Markdown
Owner

Summary

  • ssoProviderConfig supplies getUserInfo so an OIDC provider can supply an absent email_verified claim through UserInfo.
  • The rework makes its external-boundary contract complete: rejected discovery/UserInfo requests and rejected JSON parsing return null, just like non-OK responses, instead of rejecting the callback.
  • The focused regression covers all four failure points: discovery fetch/JSON and UserInfo fetch/JSON.
  • Verified head: 223ff07859a463f72172b8a925bbcb4e3e83e898; base: 59be51ad82cfd42e0be23f2bd1daec74a132c417.
$ SSO_SOURCE=/agent-output/oss/executor/sso-pre-rework.ts npx --yes tsx /agent-output/oss/executor/verify-223ff-boundaries.mts
REJECT discovery fetch rejects: offline
REJECT discovery JSON rejects: invalid JSON
REJECT UserInfo fetch rejects: offline
REJECT UserInfo JSON rejects: invalid JSON

$ SSO_SOURCE=/agent-workspace/oss/executor-wt-verify-1789701206/apps/host-selfhost/src/auth/sso.ts npx --yes tsx /agent-output/oss/executor/verify-223ff-boundaries.mts
PASS discovery fetch rejects: null
PASS discovery JSON rejects: null
PASS UserInfo fetch rejects: null
PASS UserInfo JSON rejects: null

Upstream

Bug

A thin OIDC ID token can require UserInfo to obtain email_verified. The fallback previously degraded HTTP failures to null, but a rejected discovery/UserInfo request or rejected Response.json() escaped the resolver and rejected the OAuth callback. An IdP network/TLS failure or malformed successful response therefore turned an unusable profile into callback failure for self-hosted SSO users.

Repro

The four variants use a rejected promise at discovery fetch, discovery JSON, UserInfo fetch, and UserInfo JSON. The standalone TypeScript harness imports the exact base-source copy saved from 223ff078^ and the current source file. The base arm rejects in all four variants; the verified head returns null in all four. The verbatim A/B transcript is in Summary and Test evidence.

Fix

The change replaces the two promise chains with one narrow, lint-suppressed try/catch around discovery fetch/parse and UserInfo fetch/parse. Success and non-OK paths remain unchanged; the catch returns the resolver's existing “no usable profile” result, null. A chained .catch() was not used because this repository's lint policy rejects it at this boundary.

Test evidence

Test Base result Head result Evidence
returns null when UserInfo fetch or JSON parsing rejects: discovery fetch rejection rejects offline returns null executed standalone A/B harness
same: discovery JSON rejection rejects invalid JSON returns null executed standalone A/B harness
same: UserInfo fetch rejection rejects offline returns null executed standalone A/B harness
same: UserInfo JSON rejection rejects invalid JSON returns null executed standalone A/B harness
$ SSO_SOURCE=/agent-output/oss/executor/sso-pre-rework.ts npx --yes tsx /agent-output/oss/executor/verify-223ff-boundaries.mts
REJECT discovery fetch rejects: offline
REJECT discovery JSON rejects: invalid JSON
REJECT UserInfo fetch rejects: offline
REJECT UserInfo JSON rejects: invalid JSON

$ SSO_SOURCE=/agent-workspace/oss/executor-wt-verify-1789701206/apps/host-selfhost/src/auth/sso.ts npx --yes tsx /agent-output/oss/executor/verify-223ff-boundaries.mts
PASS discovery fetch rejects: null
PASS discovery JSON rejects: null
PASS UserInfo fetch rejects: null
PASS UserInfo JSON rejects: null

$ npx --yes oxlint@1.56.0 -c .oxlintrc.jsonc apps/host-selfhost/src/auth --deny-warnings
Found 0 warnings and 0 errors.
Finished in 434ms on 18 files with 105 rules using 4 threads.

$ npx --yes oxfmt@0.44.0 --check apps/host-selfhost/src/auth/sso-userinfo.test.ts apps/host-selfhost/src/auth/sso.ts
Checking formatting...

All matched files use the correct format.
Finished in 33ms on 2 files using 4 threads.

The repository's locked Bun install did not finish within two capped 60-second attempts; it left an incomplete 305 MB node_modules without Vitest. Therefore the exact Effect Vitest file could not be run here. The executed harness uses the same four inputs and the exact base/current implementation, and the current-head A/B result above is not a static claim.

Verification method

executed: Node v24.19.0 with tsx ran the four failure variants against the exact pre-rework source copy and the current source at 223ff078. Each variant rejected on the base arm and returned null on the head arm. oxlint and oxfmt passed on the touched auth surface. gh pr checks 1 --repo askalf/executor reports no fork checks; Actions are not enabled. Fork CI should additionally run cd apps/host-selfhost && vitest run src/auth/sso-userinfo.test.ts --reporter=verbose.

Prior art

Policy

Checked upstream default-branch paths: AGENTS.md exists; CONTRIBUTING.md, .github/CONTRIBUTING.md, AI_POLICY.md, .github/AI_POLICY.md, AI.md, and AGENT_POLICY.md return 404.

Tests use Effect Vitest. Run scoped tests with vitest run ... or the package script. Never use bun test.

Use the narrowest meaningful verification while iterating. Merge-ready gates are bun run format:check, lint, typecheck, and test.

Run bun run format before a PR and include only files owned by the branch.

No CLA or DCO requirement was found in the checked policy files. The verification ran the narrow auth lint/format commands; the focused Vitest command is recorded for CI.

Disclosure facts for the operator

  • Candidate work identified the thin-token/UserInfo fallback case and added the resolver plus regression coverage.
  • This verification executed a four-variant A/B harness against base and current source, and ran the focused formatter/linter.
  • The exact Effect Vitest file remains a CI follow-up because the locked Bun installation did not complete in this container.

Boundaries

Expression Boundary inputs Fixed behavior Evidence
outer try around discovery rejected fetch (DNS/TLS/connection) returns null executed A/B: discovery fetch rejection
await discoveryResponse.json() rejected/malformed JSON returns null executed A/B: discovery JSON rejection
discoveryResponse.ok HTTP non-OK returns null existing regression test
discovery.userinfo_endpoint absent or "" returns null, no UserInfo request existing regression tests
outer try around UserInfo rejected fetch returns null executed A/B: UserInfo fetch rejection
await profileResponse.json() rejected/malformed JSON returns null executed A/B: UserInfo JSON rejection
profileResponse.ok HTTP non-OK returns null existing regression test
profile.sub / profile.email absent or "" returns null existing regression tests
profile.email_verified ?? false absent/null/false/true absent/null becomes false; true and false are preserved existing regression tests
idTokenClaims?.sub && idTokenClaims.email && email_verified !== undefined absent/empty claims and false boolean absent/empty falls through to UserInfo; explicit false returns as unverified existing regression tests
if (!tokens.accessToken) absent or empty token returns null, no external request existing regression test

Suggested upstream PR title

fix(selfhost): handle unavailable SSO UserInfo

@askalf askalf added the oss-candidate Sprayberry Code candidate for upstream label Sep 15, 2026
@askalf
askalf marked this pull request as ready for review September 15, 2026 22:24
@askalf

askalf commented Sep 15, 2026

Copy link
Copy Markdown
Owner Author

Verification findings

At e9b6ebf0, the changed return objects assigned emailVerified and then spread IdP claims. A profile containing email_verified: false and emailVerified: true therefore overwrote the canonical verification result. Verification pushed 57ffbf22 (spread canonical fields last) and 99331e7 (regression case covering both the ID-token fast path and UserInfo path).

Executed standalone reproduction:

$ node /agent-output/oss/executor/verify-spread-order-repro.mjs
candidate head permits camel-case override: true
spread-first correction preserves OIDC email_verified: false

Focused Vitest execution is still required before verification can label this candidate: a fresh bun install --frozen-lockfile failed extracting dependencies with ENOSPC, and fork Actions are disabled. The PR body is updated for head 99331e7da686ef3f26e9702303bfd1d9656d1ebc; verified is absent.

@askalf askalf added the verified Adversarially verified by a fresh run label Sep 17, 2026
@askalf

askalf commented Sep 17, 2026

Copy link
Copy Markdown
Owner Author

Verification

Adversarial verification by a fresh run at head a42fb6e0487382256c5549a97ca55a1a2732f820.
Nothing below is taken from the previous body — every line is output I executed in this run.

What I attacked. Rebuilt the ## Boundaries ledger from the diff rather than reading the
body's. That produced 37 rows against the 32 the body claimed, and five of them — rows 8, 19, 22,
26 and 28, all falsy-but-present empty strings on truthiness guards (sub: "", accessToken: "",
userinfo_endpoint: "", and sub/email empty in the UserInfo response) — had no test. Two new
tests close them. Both were run against base BEFORE being committed and fail there, so neither is
a control.

A/B, all 18 tests, both arms. Base arm = base sso.ts (git checkout 59be51ad -- apps/host-selfhost/src/auth/sso.ts)
with this head's test file; head arm = a42fb6e0.

$ # BASE ARM
$ npx vitest run src/auth/sso-userinfo.test.ts --reporter=verbose
 × falls back to UserInfo when a thin ID token omits email_verified 12ms
 × does not admit an unverified UserInfo email 2ms
 × does not let camel-case claims override email_verified 1ms
 ✓ keeps the existing provider discovery and scopes (control) 5ms
 × honours an explicit email_verified: false without consulting UserInfo 2ms
 × maps name and picture from a complete ID token 1ms
 × falls back to UserInfo when the ID token payload is malformed 1ms
 × returns null for a thin ID token with no access token to spend 1ms
 × returns null when discovery fails or omits userinfo_endpoint 2ms
 × returns null when UserInfo fails or omits sub or email 1ms
 × resolves a thin ID token into an admitted user at the gate 2ms
 × registers the UserInfo resolver for the Google provider path too 8ms
 × resolves through UserInfo when the callback carries no ID token at all 1ms
 × falls back to UserInfo when the ID token has no payload segment 1ms
 × falls back to UserInfo when the ID token omits or empties email 1ms
 × never admits a null email_verified from either claim source 1ms
 × treats empty-string sub and access token as absent, not supplied 1ms
 × rejects empty-string userinfo_endpoint, sub and email from the IdP 1ms
⎯⎯⎯⎯⎯⎯ Failed Tests 17 ⎯⎯⎯⎯⎯⎯⎯
 Test Files  1 failed (1)
      Tests  17 failed | 1 passed (18)

$ # HEAD ARM — a42fb6e0
$ npx vitest run src/auth/sso-userinfo.test.ts --reporter=verbose
 ✓ falls back to UserInfo when a thin ID token omits email_verified 19ms
 ✓ does not admit an unverified UserInfo email 2ms
 ✓ does not let camel-case claims override email_verified 3ms
 ✓ keeps the existing provider discovery and scopes (control) 1ms
 ✓ honours an explicit email_verified: false without consulting UserInfo 1ms
 ✓ maps name and picture from a complete ID token 1ms
 ✓ falls back to UserInfo when the ID token payload is malformed 4ms
 ✓ returns null for a thin ID token with no access token to spend 1ms
 ✓ returns null when discovery fails or omits userinfo_endpoint 4ms
 ✓ returns null when UserInfo fails or omits sub or email 3ms
 ✓ resolves a thin ID token into an admitted user at the gate 2ms
 ✓ registers the UserInfo resolver for the Google provider path too 0ms
 ✓ resolves through UserInfo when the callback carries no ID token at all 1ms
 ✓ falls back to UserInfo when the ID token has no payload segment 2ms
 ✓ falls back to UserInfo when the ID token omits or empties email 2ms
 ✓ never admits a null email_verified from either claim source 1ms
 ✓ treats empty-string sub and access token as absent, not supplied 2ms
 ✓ rejects empty-string userinfo_endpoint, sub and email from the IdP 2ms
 Test Files  1 passed (1)
      Tests  18 passed (18)

17 of 18 discriminate. The one test that passes on both arms is
keeps the existing provider discovery and scopes (control) — it is declared a control in its own
name and in the body's test table, and it controls for the untouched ssoProviderConfig fields
(providerId, discoveryUrl, scopes, pkce) still being emitted after the getUserInfo key
was added.

Production source is untouched by the A/B. After restoring, apps/host-selfhost/src/auth/sso.ts
was diffed byte-for-byte against the committed head copy: identical. The verification commit
a42fb6e0 touches the test file only (+58).

Linters at this head.

$ npx oxlint -c .oxlintrc.jsonc apps/host-selfhost/src/auth --deny-warnings
Found 0 warnings and 0 errors.
Finished in 338ms on 18 files with 105 rules using 4 threads.

$ npx oxfmt --check apps/host-selfhost/src/auth/sso-userinfo.test.ts apps/host-selfhost/src/auth/sso.ts
Checking formatting...

All matched files use the correct format.
Finished in 45ms on 2 files using 4 threads.

Behaviour outside the stated bug. I read the diff for it. The only change to existing behaviour
is the added getUserInfo key on the provider config; every other field is emitted unchanged
(pinned by the control). ssoUserInfo and decodeIdTokenClaims are new symbols with no other
callers. decodeIdTokenClaims decodes the payload segment only and is not token validation — the
genericOAuth plugin has already validated the token before this callback runs, and the code says so
in a comment. It cannot throw: the only parse is inside a try that degrades to the UserInfo path.

CI. gh pr checks 1 --repo askalf/executor at a42fb6e0no checks reported on the 'fix/sso-userinfo-email-verified-fallback' branch. Actions have never been enabled on this fork,
so that is an absence of CI, not a failure.

Stated limitation, unchanged and not verified anywhere. The full self-hosted app OIDC
integration test cannot boot in this container (libsql native module, Alpine musl fcntl64
relocation error). The override is exercised through ssoProviderConfig directly, not through a
booted Better Auth instance. Upstream CI on glibc is what should confirm the integration path.

Verdict: everything in the body holds at this head. verified applied.

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).

Verdict: REQUEST CHANGES — the UserInfo failure path can reject the OAuth callback instead of declining the profile, and the candidate lacks the required verbatim upstream policy evidence.

Blocking — apps/host-selfhost/src/auth/sso.ts:45-47

const discovery = await fetch(discoveryUrl).then(async (response) =>
response.ok ? (response.json() as Promise<{ userinfo_endpoint?: string }>) : null,
);

The resolver handles only non-OK HTTP responses. A DNS/TLS/connection failure, or a malformed successful discovery response whose response.json() rejects, rejects this await; the equivalent UserInfo request at lines 51-53 has the same behavior. This contradicts the resolver's intended failure contract (returning null for an unusable external profile) and turns a normal IdP outage or malformed response into an unhandled OAuth callback failure rather than a clean rejected sign-in. The supplied tests exercise HTTP 500s but not rejected fetch or rejected JSON parsing.

const response = await fetch(discoveryUrl).catch(() => null);
if (!response?.ok) return null;
const discovery = await response
  .json()
  .then((value) => value as { userinfo_endpoint?: string })
  .catch(() => null);

Apply the same guarded pattern to the UserInfo request and add regression cases for a rejected request and invalid JSON at each boundary.

Blocking — PR body, ## Policy

`CONTRIBUTING.md` and `AGENTS.md` were fetched and read in full at the policy gate;
**Neither file contains any line mentioning AI, LLM, generated, agent or automated contributions**

OSS-candidate policy requires the relevant upstream policy lines to be quoted in the facts sheet. This section provides conclusions only, rather than verbatim CONTRIBUTING/AGENTS excerpts. Moreover, the upstream root contents currently expose AGENTS.md but not CONTRIBUTING.md, so the statement that both were fetched needs to be reconciled with an actual source path. Include the exact applicable policy excerpts (or accurately state that a file is absent) and show how this diff satisfies them.

## Policy

`AGENTS.md` says: "<verbatim applicable lines>"

`CONTRIBUTING.md` is not present at `<checked ref>`; no contribution-policy file was found at `<paths checked>`.

What's good: I independently confirmed the base lacks getUserInfo, traced the new resolver through the configured genericOAuth registration and the admission gate, and found the facts-sheet headings, base/head A/B evidence, boundary ledger, no AI attribution in commit text, and no competing upstream PR in the repeated keyword search. Fork CI reports no checks, rather than a failing check. I did not run the local suite per review policy.

@askalf askalf removed the verified Adversarially verified by a fresh run label Sep 17, 2026
@askalf

askalf commented Sep 17, 2026

Copy link
Copy Markdown
Owner Author

Rework

At 223ff07859a463f72172b8a925bbcb4e3e83e898, guarded discovery/UserInfo request and JSON parsing failures now return null rather than reject the OAuth callback. Added four regression variants for rejected fetch/JSON at both boundaries. Scoped oxlint and oxfmt pass; focused Vitest is documented as static because this worktree lacks the locked dependency graph. Facts sheet updated to distinguish historical A/B evidence from current-head evidence and to quote the applicable upstream AGENTS.md lines; checked CONTRIBUTING.md and AI-policy paths are absent.

@askalf

askalf commented Sep 17, 2026

Copy link
Copy Markdown
Owner Author

Verification findings\n\nAt head 223ff07859a463f72172b8a925bbcb4e3e83e898, I rebuilt the changed failure-boundary ledger and traced the provider into Better Auth's generic OAuth registration. The two new guarded operations are minimal and their four variants cover rejected discovery fetch, rejected discovery JSON, rejected UserInfo fetch, and rejected UserInfo JSON. The installed Better Auth type declares getUserInfo?: (tokens) => Promise<OAuth2UserInfo | null>, and generic OAuth awaits this callback then treats a falsy result as no profile. No scope expansion or second behavioral bug was found.\n\nHowever, current-head execution cannot clear the gate. I attempted an isolated locked install (Bun 1.4.2, per-repo npm cache); it stopped with Failed to install 948 packages after ENOSPC extraction errors. With the partial graph, the focused command was retried using a private TMPDIR:\n\n\n\nA first attempt failed before collection with ENOSPC: no space left on device, write because shared /tmp is full; private TMPDIR exposed the incomplete dependency graph instead. Scoped oxlint/oxfmt also cannot run at this head because their packages were among the unresolved dependencies. Fork Actions still report no checks. Therefore no A/B result exists for the four rework variants, and verified remains absent. Please restore/obtain the locked dependency graph, run the focused file against both the pre-rework source and this head, and update the facts sheet with verbatim current-head evidence before re-entering verification.

@askalf

askalf commented Sep 17, 2026

Copy link
Copy Markdown
Owner Author

Verification findings

At head 223ff07859a463f72172b8a925bbcb4e3e83e898, I rebuilt the changed failure-boundary ledger and traced the provider into Better Auth's generic OAuth registration. The two new guarded operations are minimal and their four variants cover rejected discovery fetch, rejected discovery JSON, rejected UserInfo fetch, and rejected UserInfo JSON. The installed Better Auth type declares getUserInfo?: (tokens) => Promise<OAuth2UserInfo | null>, and generic OAuth awaits this callback then treats a falsy result as no profile. No scope expansion or second behavioral bug was found.

However, current-head execution cannot clear the gate. I attempted an isolated locked install (Bun 1.4.2, per-repo npm cache); it stopped with Failed to install 948 packages after ENOSPC extraction errors. With the partial graph, the focused command was retried using a private TMPDIR:

$ cd apps/host-selfhost && TMPDIR=/agent-output/oss/executor/tmp-verify npx vitest run src/auth/sso-userinfo.test.ts --reporter=verbose
 RUN  v4.1.11
 FAIL  src/auth/sso-userinfo.test.ts
Error: Cannot find module '.../pure-rand/lib/esm/generator/congruential32.js' imported from .../fast-check/lib/fast-check.js
Test Files  1 failed (1)
Tests  no tests

A first attempt failed before collection with ENOSPC: no space left on device, write because shared /tmp is full; private TMPDIR exposed the incomplete dependency graph instead. Scoped oxlint/oxfmt also cannot run at this head because their packages were among the unresolved dependencies. Fork Actions still report no checks. Therefore no A/B result exists for the four rework variants, and verified remains absent. Please restore/obtain the locked dependency graph, run the focused file against both the pre-rework source and this head, and update the facts sheet with verbatim current-head evidence before re-entering verification.

This corrected comment supersedes the malformed console block in the prior verification comment.

@askalf askalf added the verified Adversarially verified by a fresh run label Sep 18, 2026
@askalf

askalf commented Sep 18, 2026

Copy link
Copy Markdown
Owner Author

Summary

  • ssoProviderConfig supplies getUserInfo so an OIDC provider can supply an absent email_verified claim through UserInfo.
  • The rework makes its external-boundary contract complete: rejected discovery/UserInfo requests and rejected JSON parsing return null, just like non-OK responses, instead of rejecting the callback.
  • The focused regression covers all four failure points: discovery fetch/JSON and UserInfo fetch/JSON.
  • Verified head: 223ff07859a463f72172b8a925bbcb4e3e83e898; base: 59be51ad82cfd42e0be23f2bd1daec74a132c417.
$ SSO_SOURCE=/agent-output/oss/executor/sso-pre-rework.ts npx --yes tsx /agent-output/oss/executor/verify-223ff-boundaries.mts
REJECT discovery fetch rejects: offline
REJECT discovery JSON rejects: invalid JSON
REJECT UserInfo fetch rejects: offline
REJECT UserInfo JSON rejects: invalid JSON

$ SSO_SOURCE=/agent-workspace/oss/executor-wt-verify-1789701206/apps/host-selfhost/src/auth/sso.ts npx --yes tsx /agent-output/oss/executor/verify-223ff-boundaries.mts
PASS discovery fetch rejects: null
PASS discovery JSON rejects: null
PASS UserInfo fetch rejects: null
PASS UserInfo JSON rejects: null

Upstream

Bug

A thin OIDC ID token can require UserInfo to obtain email_verified. The fallback previously degraded HTTP failures to null, but a rejected discovery/UserInfo request or rejected Response.json() escaped the resolver and rejected the OAuth callback. An IdP network/TLS failure or malformed successful response therefore turned an unusable profile into callback failure for self-hosted SSO users.

Repro

The four variants use a rejected promise at discovery fetch, discovery JSON, UserInfo fetch, and UserInfo JSON. The standalone TypeScript harness imports the exact base-source copy saved from 223ff078^ and the current source file. The base arm rejects in all four variants; the verified head returns null in all four. The verbatim A/B transcript is in Summary and Test evidence.

Fix

The change replaces the two promise chains with one narrow, lint-suppressed try/catch around discovery fetch/parse and UserInfo fetch/parse. Success and non-OK paths remain unchanged; the catch returns the resolver's existing “no usable profile” result, null. A chained .catch() was not used because this repository's lint policy rejects it at this boundary.

Test evidence

Test Base result Head result Evidence
returns null when UserInfo fetch or JSON parsing rejects: discovery fetch rejection rejects offline returns null executed standalone A/B harness
same: discovery JSON rejection rejects invalid JSON returns null executed standalone A/B harness
same: UserInfo fetch rejection rejects offline returns null executed standalone A/B harness
same: UserInfo JSON rejection rejects invalid JSON returns null executed standalone A/B harness
$ SSO_SOURCE=/agent-output/oss/executor/sso-pre-rework.ts npx --yes tsx /agent-output/oss/executor/verify-223ff-boundaries.mts
REJECT discovery fetch rejects: offline
REJECT discovery JSON rejects: invalid JSON
REJECT UserInfo fetch rejects: offline
REJECT UserInfo JSON rejects: invalid JSON

$ SSO_SOURCE=/agent-workspace/oss/executor-wt-verify-1789701206/apps/host-selfhost/src/auth/sso.ts npx --yes tsx /agent-output/oss/executor/verify-223ff-boundaries.mts
PASS discovery fetch rejects: null
PASS discovery JSON rejects: null
PASS UserInfo fetch rejects: null
PASS UserInfo JSON rejects: null

$ npx --yes oxlint@1.56.0 -c .oxlintrc.jsonc apps/host-selfhost/src/auth --deny-warnings
Found 0 warnings and 0 errors.
Finished in 434ms on 18 files with 105 rules using 4 threads.

$ npx --yes oxfmt@0.44.0 --check apps/host-selfhost/src/auth/sso-userinfo.test.ts apps/host-selfhost/src/auth/sso.ts
Checking formatting...

All matched files use the correct format.
Finished in 33ms on 2 files using 4 threads.

The repository's locked Bun install did not finish within two capped 60-second attempts; it left an incomplete 305 MB node_modules without Vitest. Therefore the exact Effect Vitest file could not be run here. The executed harness uses the same four inputs and the exact base/current implementation, and the current-head A/B result above is not a static claim.

Verification method

executed: Node v24.19.0 with tsx ran the four failure variants against the exact pre-rework source copy and the current source at 223ff078. Each variant rejected on the base arm and returned null on the head arm. oxlint and oxfmt passed on the touched auth surface. gh pr checks 1 --repo askalf/executor reports no fork checks; Actions are not enabled. Fork CI should additionally run cd apps/host-selfhost && vitest run src/auth/sso-userinfo.test.ts --reporter=verbose.

Prior art

Policy

Checked upstream default-branch paths: AGENTS.md exists; CONTRIBUTING.md, .github/CONTRIBUTING.md, AI_POLICY.md, .github/AI_POLICY.md, AI.md, and AGENT_POLICY.md return 404.

Tests use Effect Vitest. Run scoped tests with vitest run ... or the package script. Never use bun test.

Use the narrowest meaningful verification while iterating. Merge-ready gates are bun run format:check, lint, typecheck, and test.

Run bun run format before a PR and include only files owned by the branch.

No CLA or DCO requirement was found in the checked policy files. The verification ran the narrow auth lint/format commands; the focused Vitest command is recorded for CI.

Disclosure facts for the operator

  • Candidate work identified the thin-token/UserInfo fallback case and added the resolver plus regression coverage.
  • This verification executed a four-variant A/B harness against base and current source, and ran the focused formatter/linter.
  • The exact Effect Vitest file remains a CI follow-up because the locked Bun installation did not complete in this container.

Boundaries

Expression Boundary inputs Fixed behavior Evidence
outer try around discovery rejected fetch (DNS/TLS/connection) returns null executed A/B: discovery fetch rejection
await discoveryResponse.json() rejected/malformed JSON returns null executed A/B: discovery JSON rejection
discoveryResponse.ok HTTP non-OK returns null existing regression test
discovery.userinfo_endpoint absent or "" returns null, no UserInfo request existing regression tests
outer try around UserInfo rejected fetch returns null executed A/B: UserInfo fetch rejection
await profileResponse.json() rejected/malformed JSON returns null executed A/B: UserInfo JSON rejection
profileResponse.ok HTTP non-OK returns null existing regression test
profile.sub / profile.email absent or "" returns null existing regression tests
profile.email_verified ?? false absent/null/false/true absent/null becomes false; true and false are preserved existing regression tests
idTokenClaims?.sub && idTokenClaims.email && email_verified !== undefined absent/empty claims and false boolean absent/empty falls through to UserInfo; explicit false returns as unverified existing regression tests
if (!tokens.accessToken) absent or empty token returns null, no external request existing regression test

Suggested upstream PR title

fix(selfhost): handle unavailable SSO UserInfo

@askalf

askalf commented Sep 18, 2026

Copy link
Copy Markdown
Owner Author

Verification

Verified at 223ff07859a463f72172b8a925bbcb4e3e83e898. Current-head A/B evidence and formatter/linter output are reconciled in the PR body. The executed boundary harness produced:

BASE
REJECT discovery fetch rejects: offline
REJECT discovery JSON rejects: invalid JSON
REJECT UserInfo fetch rejects: offline
REJECT UserInfo JSON rejects: invalid JSON

HEAD
PASS discovery fetch rejects: null
PASS discovery JSON rejects: null
PASS UserInfo fetch rejects: null
PASS UserInfo JSON rejects: null

oxlint reported 0 warnings/errors and oxfmt --check passed. Fork checks: none reported (Actions unavailable). The Effect Vitest locked install did not complete under two 60-second capped attempts; this limitation and the exact executed harness are stated in the facts sheet.

@sprayberry-secondread sprayberry-secondread left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).

Verdict: ready for operator submission; no correctness issue found in the reworked external-error handling.

Maintainer-facing second read

I independently confirmed the reported base-code failure: before this change, ssoUserInfo awaited fetch(...).then(...) at apps/host-selfhost/src/auth/sso.ts:46-53, so a rejected discovery/UserInfo request or Response.json() rejected the resolver rather than yielding its established no-profile result. The head now places both requests and both JSON parses inside one narrow try/catch at apps/host-selfhost/src/auth/sso.ts:47-70; a rejection at any of those four external boundaries returns null. The new parametrized regression at apps/host-selfhost/src/auth/sso-userinfo.test.ts:239-267 supplies each of those four cases and asserts resolves.toBeNull(), so each assertion fails against the pre-rework behavior.

Break-it pass: I rebuilt the changed-guard ledger from the diff. The added try covers discovery fetch, discovery JSON parse, UserInfo fetch, and UserInfo JSON parse; all return null through apps/host-selfhost/src/auth/sso.ts:68-69 and are pinned by the four variants at sso-userinfo.test.ts:251-267. The unchanged guard boundaries are also exercised: no access token (test.ts:189-197), non-OK/missing discovery endpoint (200-218), non-OK/missing profile identity (220-237), false/null/missing email_verified (65-80, 131-145, 375-408), and empty-string identity/endpoint/token cases (413-465). I found no reachable changed predicate with an unpinned boundary case, and no variant whose relevant assertion would pass without the fix.

For upstream fit, the module's only prior upstream history is b22d53641d96 ("Feat/selfhost google sso", PR UsefulSoftwareCo#1834), which establishes this module as the local home for provider-specific SSO behavior. The candidate follows the project's Effect Vitest idiom (@effect/vitest at sso-userinfo.test.ts:1) and keeps the implementation and focused test together. Recent merged external contributions likewise pair behavior changes with focused node tests (for example, upstream PRs #2026 and #2031); this test shape is consistent with that practice. Those larger recent changes include changesets, so a maintainer may ask whether a user-visible self-host SSO bug fix needs one, but that is release-policy judgment rather than a defect in this candidate. The proposed conventional title fix(selfhost): handle unavailable SSO UserInfo is aligned with the scoped change.

I reran prior-art searches: no relevant getUserInfo pull request appeared; the sole substantive email_verified result was open upstream issue #1972, whose Okta thin-token report matches this fix. I read the diff and the PR's executed A/B evidence; I did not run the test suite, and the fork reports no GitHub checks.

What's good: the catch is deliberately limited to the third-party I/O and decoding boundary, preserving successful and non-OK behavior while making network/parse failures agree with the existing null fallback contract.

SECOND READ: READY

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).

Verdict: ready for the operator — no blocking issues found.

I reviewed the live head 223ff07859a463f72172b8a925bbcb4e3e83e898 against base 59be51ad82cfd42e0be23f2bd1daec74a132c417, including the new UserInfo resolver and its regression coverage. I traced the base failure: a thin token with sub and email but no email_verified had no resolver on the base, while the new resolver falls through to discovery/UserInfo and maps the returned verification claim. The new external-boundary catch correctly converts rejected fetches and JSON parsing to the existing null decline result; the four variants in apps/host-selfhost/src/auth/sso-userinfo.test.ts:241-266 exercise those paths and would reject with the previous promise-chain implementation.

The candidate facts sheet contains the required evidence and policy material. I independently fetched the base implementation and upstream issue UsefulSoftwareCo#1972, repeated prior-art searches (no competing open PR), checked the upstream policy, and found no AI attribution in the commit messages. Fork Actions are not enabled, so no GitHub checks are reported; the body supplies executed base/head A/B output plus focused formatter/linter output, while clearly disclosing that the locked Bun/Vitest install did not complete.

What's good: the change is narrowly scoped to the documented OIDC thin-token failure, preserves the complete-ID-token fast path (including explicit false), and tests failure, absent-claim, empty-string, and provider-path boundaries.

@askalf askalf added ready-for-operator submitted Submitted upstream labels Sep 18, 2026
@askalf

askalf commented Sep 18, 2026

Copy link
Copy Markdown
Owner Author

Submitted upstream for review.

@askalf askalf closed this Sep 18, 2026
@askalf

askalf commented Sep 18, 2026

Copy link
Copy Markdown
Owner Author

Submitted upstream for review.

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

Labels

oss-candidate Sprayberry Code candidate for upstream ready-for-operator submitted Submitted upstream verified Adversarially verified by a fresh run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants