feat(auth): add OIDC SSO support for private deployments

[likai1130]

Implements OpenID Connect authentication for enterprise private deployments as requested in #2188. This adds a new oidc-required auth mode alongside the existing standard mode (Google/GitHub).

Changes

Server:

• Add OIDC configuration (FIRST_TREE_OIDC_ISSUER, CLIENT_ID, CLIENT_SECRET)
• Add FIRST_TREE_AUTH_MODE to control auth provider availability
• Implement OIDC authorization code flow with PKCE (S256)
• Add discovery, token exchange, id_token verification (JWKS), and UserInfo endpoint support
• Boot validation ensures complete OIDC config or fails fast
• Guard Google/GitHub routes when in oidc-required mode
• Block link/unlink operations for OIDC identities

Web:

• Add "Continue with SSO" button on login page (shown when authMode=oidc-required)
• Hide Google/GitHub buttons in OIDC mode

Standards compliance:

• PKCE code_challenge_method: S256
• State/nonce validation with encrypted HttpOnly cookies
• UserInfo fetch to supplement id_token profile data (supports older IdPs like GitLab v11)
• Issuer validation matches discovery document

Configuration

FIRST_TREE_AUTH_MODE=oidc-required
FIRST_TREE_OIDC_ISSUER=https://idp.example.com
FIRST_TREE_OIDC_CLIENT_ID=<client-id>
FIRST_TREE_OIDC_CLIENT_SECRET=<secret>
FIRST_TREE_PUBLIC_URL=https://first-tree.example.com

See docs/oidc-sso-guide.md for deployment instructions and QA cases in packages/qa/cases/cross-surface/oidc-sso.md.

Summary

• what changed?
• why does it matter?

Validation

• pnpm check
• pnpm typecheck
• pnpm test

Change Surface

• apps/cli public CLI or help output
• tree onboarding / binding / inspection behavior
• shipped or planned skill topology
• docs or contributor-facing repository metadata
• CI / packaging / release plumbing

Notes

• package or install behavior changes:
• docs or tests updated to match:
• follow-up work:

[github-actions]

All contributors are covered by the First Tree CLA.
_{Posted by the CLA Assistant Lite bot.}

[yuezengwu]

Requesting changes: this implementation does not yet satisfy the deployment-required authentication and OIDC security contract in #2188.

Blockers:

1. oidc-required is not enforced server-side. packages/server/src/api/bootstrap/config.ts:42-45 still reports configured Google/GitHub clients as sign-in options, and there are no mode guards on password login, Google start/callback, GitHub start/dev-callback, or legacy sign-in callbacks. Those routes can still mint sessions. In addition, packages/server/src/api/auth/github.ts:680-708 always mints and returns a new First Tree session, including capability intents; in OIDC mode, authenticated GitHub link/unlink/install flows must keep the existing OIDC session and never sign in as the GitHub identity. Please implement the required stable 403 sign-in-method-disabled behavior while preserving separately configured GitHub/GitLab capability flows.

2. A UserInfo subject mismatch does not fail closed. packages/server/src/services/oidc.ts:113-115 correctly throws on a mismatched sub, but packages/server/src/api/auth/oidc.ts:136-149 catches that error, logs a warning, and continues to create/reuse an identity from the ID token. A mismatched subject must terminate the callback before any identity write.

3. Required ID-token/discovery validation is missing. verifyIdToken casts the payload to OidcIdTokenClaims without requiring a non-empty sub or iat, and does not validate azp when aud has multiple values. A validly signed token with no sub can reach JSON.stringify([issuer, undefined]) and create a shared [..., null] identity. Production HTTPS is checked only for the configured issuer; discovered authorization, token, JWKS, and UserInfo endpoints are not validated. Validate the discovery/token response and required claims at runtime before identity resolution.

4. Unverified email is persisted as account data. packages/server/src/api/auth/oidc.ts:156-165 uses claims.email regardless of email_verified. It also lets UserInfo replace email while retaining an ID-token verification flag through object spreading. Only persist/use the final email when that same claim set explicitly has email_verified === true; otherwise store null and do not derive the username from it.

5. PKCE/state cookie requirements are incomplete. packages/server/src/api/auth/oidc.ts:50-51 stores the state nonce and verifier as independently encrypted cookies rather than binding the verifier to the state nonce. The early terminal paths at lines 63-92 also return without expiring either cookie. Bind the verifier to the state nonce and clear both cookies on provider error, malformed callback, rejected state, missing verifier, success, and later terminal failures.

6. The provider schemas and Account Settings behavior conflate two different capabilities. packages/shared/src/schemas/oauth.ts:3-5,34-57 expands the linkable/account provider schema to include oidc, then the server manually rejects it. #2188 requires an internal auth-identity/sign-in provider type that can include OIDC while the Account Settings link/unlink schema remains google | github. Also, packages/server/src/api/me-auth-providers.ts:195-200 leaves Google available in OIDC mode, so Google link/unlink remains exposed even though it must be unavailable there.

7. The Web implementation covers only the login page. packages/web/src/pages/invite-accept.tsx has no OIDC action, so signed-out invitees in oidc-required see no usable provider. authProviderForCallbackPath() still classifies every /auth/complete callback as Google, so OIDC analytics are misattributed, and sign-in-method-disabled is absent from the shared/web error mappings. Existing bootstrap mocks/tests also still use the old two-provider payload, causing the new strict parser to fail closed instead of exercising the intended UI.

8. Issuer handling rejects supported IdPs and changes identity keys. packages/shared/src/config/server-config.ts:121-135 forbids issuer paths and normalizes the value to url.origin. #2188 forbids userinfo/query/fragment but intentionally does not forbid paths; Keycloak/Azure-style issuer paths must remain usable. The exact configured issuer string must be compared with discovery and used in (issuer, sub), not silently rewritten.

There are no deterministic tests for the new OIDC routes, claims, mode guards, cookie lifecycle, collisions, or dormant-standard behavior, and docs/cli-reference.md was not updated as required. Please add the contract-level Server/Shared/Web coverage from #2188; the QA markdown case does not replace those tests. docs/dev-database-reset.md is unrelated to this feature and should be removed from this PR unless there is a separate scoped reason for it.

Core-data note for maintainers: this changes the shared authentication-provider model and identity creation behavior, although it adds no database migration or new business table. Please re-check the provider-type split and existing auth_identities invariants during the next review.

[baixiaohang]

Recommendation: request changes

• Rationale: I agree with the blocking review already submitted; the current implementation does not yet enforce deployment-required OIDC, and two additional blast-radius issues should be fixed in the same pass.

Risk level: B-high

• Path baseline: packages/server/** makes the baseline B-low.
• Semantic lift: the change also modifies the Web bootstrap/data-layer contract in packages/web/src/hooks/use-server-channel.ts, lifting it to B-high.

PR summary

• Author / repo: likai1130 / agent-team-foundation/first-tree
• Problem: private deployments need enterprise OIDC to be the only account sign-in method without changing the global-user/multi-Team model or disabling separately configured GitHub/GitLab capabilities.
• Approach: add an oidc-required deployment mode, an authorization-code + PKCE callback, OIDC identities keyed by (issuer, sub), and an SSO action in the Web login surface.
• Impacted modules: shared configuration/auth schemas, Server auth and identity services/routes, Web bootstrap/login behavior, operator docs, and cross-surface QA assets.

Review findings

❌ 1. The additive bootstrap change is not rolling-deploy compatible. authProviderAvailabilitySchema now requires oidc, and extractAuthProviderAvailability() converts an older Server payload such as { google: true, github: true } into { google: false, github: false, oidc: false }. A new Web bundle served while an older Server is still in rotation therefore hides every standard sign-in option and can lock users out. Parse a missing oidc field as false while preserving the old fields, and add a mixed-version regression test. [R5 / packages/shared/src/schemas/oauth.ts:7, packages/web/src/hooks/use-server-channel.ts:45]

❌ 2. The unrelated “Users only (keep other data)” reset recipe is materially destructive. TRUNCATE auth_identities, users CASCADE propagates into tables that reference users (including members, invitations, and connect codes, with further dependent data possible), so it does not preserve the rest of the database as the heading promises. Remove docs/dev-database-reset.md from this OIDC PR or replace it in a separately scoped change with a verified, accurately described reset procedure. [R4 / docs/dev-database-reset.md:14]

⚠️ 3. The IdP network boundary has no timeout and logs the token endpoint's raw non-2xx response body. Discovery, token exchange, UserInfo, and JWKS fetches can therefore hold an auth request open indefinitely, while an arbitrary provider error payload is copied into the logged exception. Use bounded request timeouts/response handling and log a stable sanitized error rather than the raw body. [R4 / packages/server/src/services/oidc.ts:35]

The existing review already captures the primary mode-enforcement, callback/session, claim validation, UserInfo, verified-email, cookie binding/lifecycle, provider-schema split, invite/Web, issuer, deterministic-test, and documentation blockers, so I am not duplicating them here.

Action taken

• Submitted a comment review with additional blocking blast-radius findings; the existing request-changes review remains the controlling GitHub state.

[yuezengwu]

Thanks for the follow-up. The new commit fixes several earlier findings (UserInfo mismatch now fails closed, basic sub/iat/azp checks were added, IdP fetches are bounded/sanitized, the invite page has an SSO action, the destructive reset doc was removed, and CLI docs were added), but the PR still does not meet #2188. Requesting changes on the latest head for the remaining blockers below.

1. oidc-required is still bypassable and the sign-in/capability split is still incomplete. packages/server/src/api/auth.ts:7-17 still accepts password login and mints a session. packages/server/src/api/bootstrap/config.ts:42-45 still advertises configured Google/GitHub clients as sign-in providers instead of returning { google: false, github: false, oidc: true }. packages/server/src/api/me-auth-providers.ts:195-200 still leaves Google link/unlink available. Most importantly, the GitHub install callback is allowed through at packages/server/src/api/auth/github.ts:300-309, but completeOauthFlow() still finds/creates a GitHub-authenticated account and always mints access/refresh tokens at lines 695-710. In OIDC mode the install flow must remain anchored to the existing kickoff user, must not create/sign in a GitHub-only user, and must return to Settings without replacing the OIDC session.

2. The required provider-schema split and rolling compatibility are still missing. packages/shared/src/schemas/oauth.ts:3-57 still puts oidc in the same schema used by Account Settings link/unlink. authProviderAvailabilitySchema also still requires the new field, so a new Web bundle receiving an older { google, github } payload fails parsing and hides every provider. Parse absent oidc as false, keep the linkable provider schema google | github, and use a separate internal/sign-in identity type. The existing Shared/Web/Server tests still contain exact two-field expectations, so the source currently demonstrates that neither the mixed-version contract nor the updated bootstrap shape is covered.

3. Several OIDC security requirements remain unresolved. The callback only expires the state/PKCE cookies after state and PKCE parsing (packages/server/src/api/auth/oidc.ts:64-112); provider errors, malformed callbacks, rejected state, provider mismatch, missing verifier, nonce mismatch, and malformed PKCE payload all return without clearing them. Discovery still trusts authorization_endpoint, token_endpoint, jwks_uri, and userinfo_endpoint without enforcing HTTPS in production. The email fix is also incomplete: { ...claims, ...userInfo } can replace the email while retaining email_verified: true from the ID token when UserInfo omits its own verification flag, so a different UserInfo email is still accepted as verified. Track email and its verification status from the same source. Finally, server-config.ts:131-133 still strips a trailing slash, despite #2188 requiring the exact configured issuer string to be compared and stored in (issuer, sub) without silent rewriting.

4. OIDC completion/error analytics are still broken. authProviderForCallbackPath() classifies every /auth/complete callback as Google, and readAttempt() explicitly accepts only Google/GitHub (packages/web/src/auth/auth-analytics.ts:65-66,92-108). An OIDC attempt is therefore discarded/misattributed after the round trip. sign-in-method-disabled is also still absent from the shared error-code and Web copy/analytics mappings. Carry or recover the actual callback provider and add the bounded error code end to end.

5. The new tests do not cover the controlling failure paths. The mode suite tests only Google/GitHub /start; it does not cover password login, bootstrap flags, callbacks/legacy intent, GitHub install session preservation, Account Settings availability, OIDC route/cookie lifecycle, UserInfo/email behavior, dormant standard mode, or identity collision/concurrency. The validation test also tries to assign to the imported jose module namespace ((jose as any).createRemoteJWKSet = ...), which is not a valid ESM mocking strategy; use Vitest mocking or dependency injection. Please add deterministic regression coverage for the acceptance criteria rather than only isolated start-route/claim checks.

6. Remove the unrelated screenshots and purge the exposed personal data. image/img.png, img_1.png, img_2.png, and img_3.png are unreferenced GitHub/CLA setup screenshots unrelated to the shipped OIDC feature. image/img_3.png publicly exposes a personal email address. Remove all four files and purge the image blobs from the PR branch history; a later deletion commit alone will leave the sensitive screenshot accessible in this public PR's commit history.

Core-data note remains unchanged: this modifies the shared authentication-provider model and auth_identities resolution semantics, with no database migration/new business table. Maintainers should re-check those invariants after the schema split and callback flow are corrected.

[yuezengwu]

Thanks for the follow-up. The new commit fixes several earlier findings (UserInfo mismatch now fails closed, basic sub/iat/azp checks were added, IdP fetches are bounded/sanitized, the invite page has an SSO action, the destructive reset doc was removed, and CLI docs were added), but the PR still does not meet #2188. Requesting changes on the latest head for the remaining blockers below.

1. oidc-required is still bypassable and the sign-in/capability split is still incomplete. packages/server/src/api/auth.ts:7-17 still accepts password login and mints a session. packages/server/src/api/bootstrap/config.ts:42-45 still advertises configured Google/GitHub clients as sign-in providers instead of returning { google: false, github: false, oidc: true }. packages/server/src/api/me-auth-providers.ts:195-200 still leaves Google link/unlink available. Most importantly, the GitHub install callback is allowed through at packages/server/src/api/auth/github.ts:300-309, but completeOauthFlow() still finds/creates a GitHub-authenticated account and always mints access/refresh tokens at lines 695-710. In OIDC mode the install flow must remain anchored to the existing kickoff user, must not create/sign in a GitHub-only user, and must return to Settings without replacing the OIDC session.

2. The required provider-schema split and rolling compatibility are still missing. packages/shared/src/schemas/oauth.ts:3-57 still puts oidc in the same schema used by Account Settings link/unlink. authProviderAvailabilitySchema also still requires the new field, so a new Web bundle receiving an older { google, github } payload fails parsing and hides every provider. Parse absent oidc as false, keep the linkable provider schema google | github, and use a separate internal/sign-in identity type. The existing Shared/Web/Server tests still contain exact two-field expectations, so the source currently demonstrates that neither the mixed-version contract nor the updated bootstrap shape is covered.

3. Several OIDC security requirements remain unresolved. The callback only expires the state/PKCE cookies after state and PKCE parsing (packages/server/src/api/auth/oidc.ts:64-112); provider errors, malformed callbacks, rejected state, provider mismatch, missing verifier, nonce mismatch, and malformed PKCE payload all return without clearing them. Discovery still trusts authorization_endpoint, token_endpoint, jwks_uri, and userinfo_endpoint without enforcing HTTPS in production. The email fix is also incomplete: { ...claims, ...userInfo } can replace the email while retaining email_verified: true from the ID token when UserInfo omits its own verification flag, so a different UserInfo email is still accepted as verified. Track email and its verification status from the same source. Finally, server-config.ts:131-133 still strips a trailing slash, despite #2188 requiring the exact configured issuer string to be compared and stored in (issuer, sub) without silent rewriting.

4. OIDC completion/error analytics are still broken. authProviderForCallbackPath() classifies every /auth/complete callback as Google, and readAttempt() explicitly accepts only Google/GitHub (packages/web/src/auth/auth-analytics.ts:65-66,92-108). An OIDC attempt is therefore discarded/misattributed after the round trip. sign-in-method-disabled is also still absent from the shared error-code and Web copy/analytics mappings. Carry or recover the actual callback provider and add the bounded error code end to end.

5. The new tests do not cover the controlling failure paths. The mode suite tests only Google/GitHub /start; it does not cover password login, bootstrap flags, callbacks/legacy intent, GitHub install session preservation, Account Settings availability, OIDC route/cookie lifecycle, UserInfo/email behavior, dormant standard mode, or identity collision/concurrency. The validation test also tries to assign to the imported jose module namespace ((jose as any).createRemoteJWKSet = ...), which is not a valid ESM mocking strategy; use Vitest mocking or dependency injection. Please add deterministic regression coverage for the acceptance criteria rather than only isolated start-route/claim checks.

6. Remove the unrelated screenshots and purge the exposed personal data. image/img.png, img_1.png, img_2.png, and img_3.png are unreferenced GitHub/CLA setup screenshots unrelated to the shipped OIDC feature. image/img_3.png publicly exposes a personal email address. Remove all four files and purge the image blobs from the PR branch history; a later deletion commit alone will leave the sensitive screenshot accessible in this public PR's commit history.

Core-data note remains unchanged: this modifies the shared authentication-provider model and auth_identities resolution semantics, with no database migration/new business table. Maintainers should re-check those invariants after the schema split and callback flow are corrected.

[baixiaohang]

Recommendation: request changes

• Rationale: the latest head fixes several earlier issues, but it still contains additional contract violations beyond the active request-changes review.

Risk level: B-high

• Path baseline: the Server authentication changes are B-low.
• Semantic lift: the Web bootstrap/data-layer contract raises this to B-high.

PR summary

• Author / repo: likai1130 / agent-team-foundation/first-tree
• Problem: private deployments need enterprise OIDC to be the only account sign-in method while retaining the existing global-user, multi-Team, and separately configured capability workflows.
• Approach: select the deployment auth mode through configuration, add OIDC Authorization Code + PKCE sign-in, and expose an SSO action on signed-out Web surfaces.
• Impacted modules: Shared auth/config schemas, Server auth and identity routes/services, Web bootstrap/login/invite behavior, tests, and operator docs.

Additional review findings

❌ 1. The new invite SSO surface does not compile. InviteAcceptCard declares oidcHref and oidcAvailable in its parameter type but omits both from the destructuring list, then reads them in the JSX (packages/web/src/pages/invite-accept.tsx:149-219). The current TypeScript check reports both names as undefined, so signed-out invite entry cannot ship in this state. Destructure the props and add the oidc-required invite regression case.

❌ 2. Legacy Google callbacks can still bypass oidc-required. The callback guard only rejects explicit sign-in, link, and unlink values after the provider token exchange (packages/server/src/api/auth/google.ts:106-114). A valid legacy signed state with a missing intent therefore falls through to the existing sign-in completion and can mint a session, even though #2188 explicitly requires missing legacy intent to be treated as disabled. Resolve the effective intent and reject it before provider exchange/session creation.

❌ 3. The forbidden GitHub dev callback is not side-effect free. It upserts a github_app_installations row before checking authMode (packages/server/src/api/auth/github.ts:367-410), then returns a redirect although the contract requires a stable 403 sign-in-method-disabled. Move the mode guard before profile/installation work. Also make the bounded callback error type include the code used by the live callback; CallbackErrorCode currently omits sign-in-method-disabled (packages/server/src/api/auth/github.ts:424-439).

❌ 4. The ID-token/token-response boundary still relies on TypeScript casts for security-critical runtime requirements. exchangeOidcCode() casts arbitrary JSON to OidcTokenSet, while verifyIdToken() does not require exp to be present and supplies no accepted-algorithm policy (packages/server/src/services/oidc.ts:65-145). A type declaration does not validate provider input; #2188 requires malformed token responses, missing expiry, unsigned tokens, and unexpected algorithms to fail deterministically. Validate the response and required claims at runtime and pass an explicit algorithm allowlist derived from a validated discovery policy.

❌ 5. OIDC errors after state validation discard the validated destination. Every token, ID-token, UserInfo, and bootstrap failure calls redirectError(), which hard-codes next: "/" (packages/server/src/api/auth/oidc.ts:117-205,243-246). This breaks the required preservation of next, including invite entry. Pass verified.next through terminal post-validation errors while keeping pre-validation input untrusted.

Action taken

• Submitted a follow-up comment review on head ae830698075f6c149b4aaed9fd99a36d3b55f3cf; the existing request-changes review remains the controlling state.

[baixiaohang]

Recommendation: request changes

• Rationale: this force-pushed head removes the screenshots from branch ancestry, but restores a destructive unrelated reset guide and leaves every previously reviewed OIDC code blocker unchanged.

Risk level: B-high

• Path baseline: the Server authentication changes are B-low.
• Semantic lift: the Web bootstrap/data-layer contract raises this to B-high.

Incremental review

❌ 1. docs/dev-database-reset.md has been restored even though the previous review explicitly identified it as both unrelated and materially misleading. The section titled “Users only (keep other data)” runs TRUNCATE auth_identities, users CASCADE; PostgreSQL will also truncate tables that reference users, including membership, invitation, and connect-code data, so the command does not preserve the rest of the database as claimed. Remove this file from the OIDC PR. Any replacement belongs in a separately scoped change with a verified and accurately described reset procedure. [docs/dev-database-reset.md:12-16]

✅ The four unreferenced screenshots, including the one containing personal account information, are absent from the new head's ancestry. This resolves the branch-history portion of the privacy finding.

No Server, Shared, Web, OIDC guide, QA, or OIDC test content changed between reviewed head ae830698075f6c149b4aaed9fd99a36d3b55f3cf and current head d23aac4395e4c8ef418940a923389665b1668bed. The active request-changes findings therefore remain applicable without restating them here.

Action taken

• Submitted an incremental comment review on the current head; the existing request-changes review remains the controlling state.

[yuezengwu]

Requesting changes again on the force-pushed head. Compared with the previous reviewed head ae8306980, this update changes no product code: it only removes the four image/img*.png screenshots and re-adds docs/dev-database-reset.md. Therefore every authentication, security, rolling-compatibility, Web, and test blocker in the active reviews remains unresolved, including the additional invite compile failure, legacy Google callback bypass, GitHub dev-callback side effects, runtime token validation gaps, and loss of validated next.

The screenshot removal is welcome and the sensitive image is no longer in the current branch history. However, the destructive reset document has regressed back into the PR. docs/dev-database-reset.md:14-15 labels TRUNCATE auth_identities, users CASCADE as “Users only (keep other data)”, but CASCADE removes rows in user-dependent tables and can propagate further; the command does not preserve the rest of the database as promised. Remove this unrelated document from the OIDC PR. Any safe developer reset procedure should be a separately scoped, dependency-aware change with accurate blast-radius wording.

Please address the existing request-changes reviews before the next rereview; the controlling code remains byte-for-byte unchanged from the previously rejected head.

Core-data note remains unchanged: the PR modifies the shared authentication-provider model and auth_identities resolution semantics, with no database migration or new business table.

[yuezengwu]

Thanks for the substantial follow-up. This head removes the unrelated destructive reset guide and keeps the screenshots out of branch ancestry; it also fixes several earlier items (password/start/dev-callback guards, bootstrap sign-in flags, invite props, UserInfo subject handling, paired verified-email selection, bounded IdP fetches, and preservation of validated next). I am still requesting changes because the latest diff remains unshippable and does not yet satisfy #2188.

1. The provider-schema split leaves the TypeScript model inconsistent, so the PR does not compile. ExternalAccountProfile.provider still uses the Account Settings-only AuthProvider (google | github) in packages/shared/src/external-account.ts:1-10, but api/auth/oidc.ts constructs that profile with provider: "oidc". Server AuthProviderAvailability is likewise still Record<AuthProvider, boolean> (packages/server/src/services/auth-identity.ts:3-31), while lines 74-75 index it with oidc and multiple callers return/pass an oidc property. me-auth-providers.ts also compares the now-linkable-only route provider with "oidc", and packages/server/src/api/auth/github.ts:523-538 still omits sign-in-method-disabled from CallbackErrorCode even though line 385 passes it. Carry the internal/sign-in provider type through identity profiles and mode-aware availability, while keeping only the route/linkable schema restricted.

2. GitHub capability linking is disabled in oidc-required. packages/server/src/api/me-auth-providers.ts:211-220 returns { google: false, github: false, oidc: true }, so /me/auth-providers/github/{link,unlink}/start reports GitHub unavailable even when the GitHub App is configured. That object copies public sign-in availability into the separate Account Settings capability boundary. In OIDC mode Google must be unavailable, but GitHub availability must still follow oauth.githubApp; OIDC may count internally for last-provider checks without appearing as a linkable row.

3. A legacy Google callback with no intent can still sign in. The mode guard at packages/server/src/api/auth/google.ts:106-114 rejects only explicit sign-in, link, or unlink. A valid legacy state with intent === undefined falls through to completeGoogleSignIn() and can create/reuse a user and mint a session. Resolve the effective intent (missing means legacy sign-in) and reject it before provider exchange/session work, with the required sign-in-method-disabled completion error.

4. The discovery/PKCE boundary still trusts provider/cookie casts instead of validating them. packages/server/src/services/oidc.ts:48-68 casts arbitrary discovery JSON and never requires valid non-empty authorization/token/JWKS endpoints or validates a signing-algorithm policy; missing endpoints are even filtered out of the production HTTPS check. verifyIdToken() then hard-codes an algorithm list rather than using validated discovery metadata, and treats every array-valued aud as multi-audience (:179-187). packages/server/src/api/auth/oidc.ts:108-116 similarly accepts parsed PKCE JSON without requiring a non-empty string nonce/verifier. Discovery at callback line 127 is also outside the bounded error redirect, so a discovery/network failure after state validation becomes a generic 500. Validate all runtime inputs and map every terminal provider/network failure to a stable error.

5. OIDC completion analytics are still discarded and the provider fragment is not bounded. packages/web/src/auth/auth-analytics.ts:94-110 still accepts only Google/GitHub stored attempts, so every OIDC attempt created before the round trip is deleted and no OIDC result is joined to it. packages/web/src/pages/oauth-complete.tsx:84-85 casts any fragment string to AuthProvider, allowing arbitrary provider cardinality into the analytics path. Parse only google | github | oidc, accept OIDC in readAttempt(), and add the callback regression.

6. The required deterministic coverage is still missing; two claimed GitHub install tests are literal no-op placeholders. packages/server/src/__tests__/github-install-oidc-mode.test.ts:43-55 uses expect(true).toBe(true) for both the no-session OIDC install contract and standard-mode behavior. No Shared/Web tests were changed, so the mixed-version bootstrap parser, login/invite surfaces, OIDC attempt round trip, and error mappings are not covered. The new Server tests also do not exercise signed legacy Google/GitHub callbacks, an actual kickoff-anchored GitHub install, a successful OIDC state+PKCE+token+UserInfo flow, same-email/different-subject behavior, or OIDC identity convergence. Replace placeholders with deterministic acceptance-level regressions from #2188.

Core-data note remains: this changes the shared authentication-provider model and auth_identities resolution/usable-auth semantics, with no migration or new business table. Maintainers should re-check those invariants after the type split and callback/capability fixes.

[baixiaohang]

Recommendation: request changes

• Rationale: the latest head closes several earlier gaps, but the OIDC-mode GitHub installation success path is not consumable by Web, and UserInfo can still overwrite validated security claims.

Risk level: B-high

• Path baseline: the Server authentication changes are B-low.
• Semantic lift: the Web callback/bootstrap data flow raises the review to B-high.

PR summary

• Author / repo: likai1130 / agent-team-foundation/first-tree
• Problem: private deployments need enterprise OIDC to be the only account sign-in method while separately configured GitHub capabilities remain usable.
• Approach: enforce a deployment authentication mode, add OIDC Authorization Code + PKCE sign-in, and preserve authenticated capability workflows without replacing the OIDC session.
• Impacted modules: Shared authentication/configuration schemas, Server identity and OAuth routes/services, Web login/invite/callback behavior, tests, and operator documentation.

Additional review findings

❌ 1. A successful GitHub App installation in oidc-required cannot complete in Web using the existing OIDC session. The Server intentionally avoids minting replacement tokens and returns only install metadata (packages/server/src/api/auth/github.ts:933-940), but the completion page treats every callback without access and refresh as a failed sign-in (packages/web/src/pages/oauth-complete.tsx:128-139). Consequently the successful capability flow lands on “Sign-in did not complete” instead of returning to Settings. Handle an error-free callbackIntent=install as an authenticated capability completion: preserve the current session, apply the pinned organization when present, clear the fragment, and navigate to the validated destination. Add a deterministic cross-surface regression for the metadata-only callback. [R4/R5]

❌ 2. The UserInfo merge is broader than the stated profile-only trust boundary. After validating only that UserInfo has the same sub, { ...claims, ...userInfo, ...emailSource } lets the response replace verified iss, aud, exp, iat, or nonce (packages/server/src/api/auth/oidc.ts:165-181). The merged claims.iss is then persisted as identity metadata at line 206, so an arbitrary UserInfo issuer can disagree with the exact configured/validated issuer even though the identity key uses the configured value. Whitelist the intended profile claims from UserInfo, retain identity and security claims from the verified ID token/configuration, and runtime-validate the UserInfo shape. [R1/R4]

Action taken

• Submitted a follow-up comment review on head c933f258c111e965038a4782ddb18d4a9712e9a1; the existing request-changes review remains the controlling state.

[yuezengwu]

Thanks for fixing the provider-type propagation: ExternalAccountProfile now carries the sign-in provider union, mode-aware availability uses the Shared shape, the impossible OIDC link-route comparisons are gone, and the GitHub callback error union now includes sign-in-method-disabled. Those changes resolve the first blocker from my previous review.

I am still requesting changes on this exact head. The commit changes only six files for the type cleanup, so the remaining contract blockers are unchanged:

1. GitHub capability linking remains disabled in oidc-required. packages/server/src/api/me-auth-providers.ts:212-220 still returns github: false in OIDC mode. That makes /me/auth-providers/github/{link,unlink}/start unavailable even when the GitHub App is configured. Account Settings capability availability must be { google: false, github: Boolean(oauth.githubApp), oidc: true } internally, while the response continues listing only linkable Google/GitHub rows.

2. Legacy Google callback state still bypasses the required mode. packages/server/src/api/auth/google.ts:106-114 rejects only an explicit intent === "sign-in"; a valid legacy state with no intent still reaches completeGoogleSignIn() and can mint a session. Treat missing intent as sign-in and reject it before provider exchange/session work.

3. The GitHub install success response is still not consumable by Web. Server intentionally returns a metadata-only callbackIntent=install fragment without replacement tokens (packages/server/src/api/auth/github.ts:933-940), but packages/web/src/pages/oauth-complete.tsx:128-139 treats every tokenless success as “Sign-in did not complete.” Handle an error-free install completion using the existing OIDC session, apply the pinned org when present, clear the fragment, and navigate to validated next.

4. OIDC provider inputs still cross the trust boundary through unchecked casts/merges. Discovery JSON and PKCE JSON are not runtime-validated for all required fields; signing algorithms are hard-coded rather than derived from validated discovery metadata; callback discovery failure is outside the bounded redirect path; and { ...claims, ...userInfo } in packages/server/src/api/auth/oidc.ts:165-206 lets UserInfo replace validated iss, aud, exp, iat, or nonce. Whitelist only intended profile claims from a runtime-validated UserInfo response and retain identity/security claims from the verified ID token/configuration.

5. Web analytics and acceptance coverage remain incomplete. packages/web/src/auth/auth-analytics.ts:94-110 still rejects stored OIDC attempts, while oauth-complete.tsx:84-85 still casts an arbitrary fragment provider instead of parsing the bounded union. The two central GitHub-install tests remain literal expect(true).toBe(true) placeholders (packages/server/src/__tests__/github-install-oidc-mode.test.ts:43-55), and there is still no deterministic Shared/Web or end-to-end callback coverage for the controlling mixed-version, legacy-state, metadata-only install, successful OIDC, collision, and analytics paths.

Core-data note remains: this changes the shared authentication-provider model and auth_identities resolution/usable-auth semantics, with no migration or new business table. Re-check those invariants after the remaining callback and capability fixes.

[yuezengwu]

Thanks for adding the mixed-version schema regression: the new assertion correctly verifies that an older { google, github } bootstrap payload parses with oidc: false.

Requesting changes again on this exact head. Compared with the previously reviewed head 0b991070e, this force-update adds only that one test expectation in packages/shared/src/__tests__/oauth-schema.test.ts; product code is unchanged. The remaining blockers from the prior reviews therefore still apply:

1. packages/server/src/api/me-auth-providers.ts:212-220 still sets github: false in oidc-required, disabling the separately configured GitHub link/unlink capability that #2188 requires to remain available.
2. packages/server/src/api/auth/google.ts:106-114 still treats only explicit intent="sign-in" as disabled; a valid legacy state with no intent can still reach completeGoogleSignIn() and mint a session.
3. The Server's metadata-only GitHub install success (packages/server/src/api/auth/github.ts:933-940) is still rejected by Web as missing access/refresh tokens (packages/web/src/pages/oauth-complete.tsx:128-139) instead of continuing with the existing OIDC session.
4. Discovery and PKCE payloads remain incompletely runtime-validated, callback discovery failure remains outside the bounded redirect, and the broad UserInfo spread can still replace validated ID-token security claims before metadata/profile resolution (packages/server/src/api/auth/oidc.ts:165-206).
5. readAttempt() still rejects OIDC and the callback still casts an arbitrary fragment provider; the central GitHub install tests remain expect(true).toBe(true) placeholders. The mixed-version parser assertion is useful, but it does not replace deterministic callback, capability-session, successful OIDC, collision/concurrency, and Web analytics coverage.

Core-data note remains unchanged: the PR changes shared authentication-provider and auth_identities resolution/usable-auth semantics, with no migration or new business table.

[likai1130]

I have read the CLA Document and I hereby sign the CLA

[yuezengwu]

Thanks for the targeted fixes. This head now keeps configured GitHub Account Settings capability flows available in oidc-required, while Google remains unavailable, and it treats a legacy missing Google callback intent as sign-in so that path no longer mints a session. Those resolve the first two findings from my previous review.

I am still requesting changes on the current exact head for the remaining blockers:

1. Metadata-only GitHub install success still fails in Web. The Server deliberately returns callbackIntent=install plus destination/org metadata without access or refresh tokens so the existing OIDC session remains active (packages/server/src/api/auth/github.ts:933-940). packages/web/src/pages/oauth-complete.tsx:128-139 still treats every tokenless completion as “Sign-in did not complete.” Handle an error-free install completion as an authenticated capability return: clear the fragment, apply the pinned organization when present, and navigate to validated next without adopting new tokens.

2. OIDC trust-boundary validation is still incomplete. fetchDiscovery() casts arbitrary JSON, does not require valid non-empty authorization/token/JWKS endpoints or validate advertised signing algorithms, and filters missing endpoints out of its production HTTPS check (packages/server/src/services/oidc.ts:36-69). PKCE JSON is likewise cast without requiring non-empty string nonce/verifier, callback discovery remains outside the bounded error redirect (packages/server/src/api/auth/oidc.ts:97-139), and array-valued aud is treated as multi-audience even when it has one element. Validate these inputs and derive the accepted ID-token algorithm policy from validated discovery metadata.

3. UserInfo can still overwrite verified identity/security claims. After checking only sub, { ...claims, ...userInfo, ...emailSource } allows UserInfo to replace the verified iss, aud, exp, iat, or nonce; the resulting claims.iss is persisted in metadata (packages/server/src/api/auth/oidc.ts:165-206). Runtime-validate UserInfo and whitelist only the intended profile fields, retaining security/identity claims from the verified ID token and configured issuer.

4. OIDC completion analytics remain broken and unbounded. packages/web/src/auth/auth-analytics.ts:94-110 still rejects stored OIDC attempts, so the attempt is discarded after the provider round trip. oauth-complete.tsx:84-85 still casts any fragment value to the provider union rather than parsing google | github | oidc, allowing arbitrary provider cardinality.

5. The controlling behavior is still not covered deterministically. This commit changes only the two Server source files and adds no regression tests for either fix. The two central GitHub install tests remain literal expect(true).toBe(true) placeholders (packages/server/src/__tests__/github-install-oidc-mode.test.ts:43-55), and there is still no actual metadata-only Web completion, successful OIDC callback, UserInfo claim-boundary, collision/concurrency, or OIDC analytics regression. Replace placeholders and cover the acceptance paths from #2188.

Core-data note remains: the PR changes shared authentication-provider and auth_identities resolution/usable-auth semantics, with no migration or new business table. Re-check those invariants after the remaining callback/trust-boundary fixes.

[baixiaohang]

Recommendation: request changes

• Rationale: this head restores the GitHub capability surface and prevents the legacy Google callback from minting a session, but the disabled callback still crosses the provider boundary before its mode guard.

Risk level: B-high

• Path baseline: the Server authentication changes are B-low.
• Semantic lift: the Web callback/bootstrap data flow raises the review to B-high.

PR summary

• Author / repo: likai1130 / agent-team-foundation/first-tree
• Problem: private deployments need enterprise OIDC to be the only account sign-in method while separately configured GitHub capabilities remain usable.
• Approach: enforce a deployment authentication mode, add OIDC Authorization Code + PKCE sign-in, and preserve authenticated capability workflows without replacing the OIDC session.
• Impacted modules: Shared authentication/configuration schemas, Server identity and OAuth routes/services, Web login/invite/callback behavior, tests, and operator documentation.

Additional review finding

❌ 1. The legacy Google callback guard still runs after the disabled provider exchange. The new effective-intent logic prevents a session from being minted, but exchangeGoogleCode() is called first (packages/server/src/api/auth/google.ts:89-115). An expired or malformed code therefore returns provider-exchange-failed instead of the required stable sign-in-method-disabled, and a valid code is unnecessarily consumed against a sign-in method the deployment forbids. After state/provider validation, resolve the effective intent and reject oidc-required before nonce/provider exchange work. Add a signed legacy-state regression that asserts the exchange dependency is never called. [R4]

Action taken

• Submitted a follow-up comment review on head c82efd0b2047b234cb09cea7beb27000fef83db3; the existing request-changes state remains controlling.