Conversation
Verification findingsAt 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: falseFocused Vitest execution is still required before verification can label this candidate: a fresh |
VerificationAdversarial verification by a fresh run at head What I attacked. Rebuilt the A/B, all 18 tests, both arms. Base arm = base $ # 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 Production source is untouched by the A/B. After restoring, 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 CI. Stated limitation, unchanged and not verified anywhere. The full self-hosted app OIDC Verdict: everything in the body holds at this head. |
sprayberry-redline
left a comment
There was a problem hiding this comment.
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.
ReworkAt |
Verification findings\n\nAt head
|
Verification findingsAt head 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 $ 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 testsA first attempt failed before collection with This corrected comment supersedes the malformed console block in the prior verification comment. |
Summary
$ 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: nullUpstream
BugA thin OIDC ID token can require UserInfo to obtain ReproThe 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 FixThe change replaces the two promise chains with one narrow, lint-suppressed Test evidence
$ 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 Verification method
Prior art
PolicyChecked upstream default-branch paths:
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
Boundaries
Suggested upstream PR titlefix(selfhost): handle unavailable SSO UserInfo |
VerificationVerified at 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
|
sprayberry-secondread
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
|
Submitted upstream for review. |
|
Submitted upstream for review. |
Summary
ssoProviderConfigsuppliesgetUserInfoso an OIDC provider can supply an absentemail_verifiedclaim through UserInfo.null, just like non-OK responses, instead of rejecting the callback.223ff07859a463f72172b8a925bbcb4e3e83e898; base:59be51ad82cfd42e0be23f2bd1daec74a132c417.Upstream
UsefulSoftwareCo/executor, default branchmain.59be51ad82cfd42e0be23f2bd1daec74a132c417.apps/host-selfhost/src/auth/sso.ts(ssoUserInfo,ssoProviderConfig) andapps/host-selfhost/src/auth/sso-userinfo.test.ts.email_verifiedonly from /userinfo UsefulSoftwareCo/executor#1972.Bug
A thin OIDC ID token can require UserInfo to obtain
email_verified. The fallback previously degraded HTTP failures tonull, but a rejected discovery/UserInfo request or rejectedResponse.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 returnsnullin 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/catcharound 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
returns null when UserInfo fetch or JSON parsing rejects: discovery fetch rejectionofflinenullinvalid JSONnullofflinenullinvalid JSONnullThe repository's locked Bun install did not finish within two capped 60-second attempts; it left an incomplete 305 MB
node_moduleswithout 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 withtsxran the four failure variants against the exact pre-rework source copy and the current source at223ff078. Each variant rejected on the base arm and returnednullon the head arm.oxlintandoxfmtpassed on the touched auth surface.gh pr checks 1 --repo askalf/executorreports no fork checks; Actions are not enabled. Fork CI should additionally runcd apps/host-selfhost && vitest run src/auth/sso-userinfo.test.ts --reporter=verbose.Prior art
gh search prs --repo UsefulSoftwareCo/executor "getUserInfo" --state all --limit 20— no competing SSO UserInfo PR.gh search prs --repo UsefulSoftwareCo/executor "email_verified" --state all --limit 20— none.gh search issues --repo UsefulSoftwareCo/executor "email_verified"— open issue [bug] SSO fails with any IdP that returnsemail_verifiedonly from /userinfo UsefulSoftwareCo/executor#1972, no linked PR.Policy
Checked upstream default-branch paths:
AGENTS.mdexists;CONTRIBUTING.md,.github/CONTRIBUTING.md,AI_POLICY.md,.github/AI_POLICY.md,AI.md, andAGENT_POLICY.mdreturn 404.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
Boundaries
tryaround discoverynullawait discoveryResponse.json()nulldiscoveryResponse.oknulldiscovery.userinfo_endpoint""null, no UserInfo requesttryaround UserInfonullawait profileResponse.json()nullprofileResponse.oknullprofile.sub/profile.email""nullprofile.email_verified ?? falseidTokenClaims?.sub && idTokenClaims.email && email_verified !== undefinedif (!tokens.accessToken)null, no external requestSuggested upstream PR title
fix(selfhost): handle unavailable SSO UserInfo