🐛 Replace hand-rolled OIDC with oauth4webapi and add proper logout

[ibolton336]

Resolves #1459

Summary

• Replace the hand-rolled OIDC implementation with oauth4webapi, a zero-dependency, spec-compliant OIDC client library
• Discovery via .well-known/openid-configuration replaces hardcoded /authorize and /token paths
• PKCE, token exchange, and refresh handled by the library with proper response validation
• Store id_token and call end_session_endpoint on sign-out for proper server-side OIDC session invalidation
• Revoke PAT on Hub server via DELETE /auth/tokens/:id during sign-out — previously PATs were only deleted locally, leaving orphaned 10-year tokens on the server
• Store PAT id from exchange response to enable server-side revocation

Supersedes #1454, #1456
Closes #1453

Test plan

• OIDC sign-in via browser (auth code + PKCE flow through loopback server)
• Token refresh works after sign-in
• Sign-out ends the OIDC session server-side (re-login requires credentials)
• Sign-out revokes the PAT on the Hub (verify via Hub API/logs)
• Graceful handling when Hub is unreachable during logout (local cleanup still proceeds)
• Backward compatibility with stored tokens that don't have idToken or PAT id fields
• Webpack bundle builds successfully

🤖 Generated with Claude Code

Summary by CodeRabbit

Summary by CodeRabbit

• Bug Fixes

  • OIDC sign-out now properly invalidates the server-side OIDC session and revokes the associated personal access token before disconnecting.

• Improvements

  • Upgraded the OIDC flow to use oauth4webapi, delivering more spec-compliant discovery, authorization/token exchange, refresh handling, and end-session behavior.
  • Persist and restore additional OIDC identity information to improve session continuity.
  • More reliable automatic selection of authentication method based on whether username/password are configured.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: eb936390-10cb-4fff-89a3-aaa9ba72edab

📥 Commits

Reviewing files that changed from the base of the PR and between 820e437 and 618a295.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (1)

• vscode/core/package.json

🚧 Files skipped from review as they are similar to previous changes (1)

• vscode/core/package.json

---

📝 Walkthrough

Walkthrough

The PR replaces the bespoke OIDC flow in OIDCAuthCodeFlow with oauth4webapi, adding discovery, PKCE, authorization-code exchange, refresh, and end-session handling. OIDCTokens now includes idToken, which is persisted in token storage and re-exported through the hub barrel while OIDCTokenResponse is removed. HubConnectionManager now stores patId, revokes the PAT and ends the OIDC session on logout, and persists PAT metadata with the token. hubConfigStorage now records auth.method and auth.oidcClientId.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

Possibly related PRs

• konveyor/editor-extensions#1419: This PR builds on the OIDC and Hub authentication plumbing in HubConnectionManager and related token storage changes.
• konveyor/editor-extensions#1453: Directly related change set covering PAT revocation and OIDC session termination on sign-out.

Suggested reviewers

• djzager

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is specific, concise, and accurately reflects the main OIDC and logout changes.
Description check	✅ Passed	The description includes a clear summary and test plan, and no required sections are missing from the provided template.
Linked Issues check	✅ Passed	The changes address both linked issues by using the configured auth method and by revoking the PAT and ending the OIDC session on logout.
Out of Scope Changes check	✅ Passed	The modified files all support OIDC sign-in/logout flow, token storage, or related config; no unrelated changes are evident.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

vscode/core/src/hub/HubConnectionManager.ts (2)

895-909: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Do not persist long-lived PATs without a revocation id.

If POST /hub/auth/tokens returns a token but no id, logout cannot revoke it and the PR’s orphaned 10-year-token fix is bypassed.

🛡️ Proposed fix

       if (!data.token) {
         this.logger.warn("PAT exchange returned no token");
         return;
       }
+      if (typeof data.id !== "number") {
+        this.logger.warn("PAT exchange returned no token id; refusing to persist non-revocable PAT");
+        return;
+      }
 
       // Success — switch to PAT
       this.bearerToken = data.token;
       this.usingPAT = true;
-      this.patId = data.id ?? null;
+      this.patId = data.id;
       // PATs are long-lived; use null to signal "no expiration" to the UI
       this.tokenExpiresAt = data.expiration ? new Date(data.expiration).getTime() : null;
 
       // Persist PAT
-      await this.storePAT(data.token, this.patId);
+      await this.storePAT(data.token, data.id);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@vscode/core/src/hub/HubConnectionManager.ts` around lines 895 - 909, The code
persists the PAT token via the storePAT method call even when data.id is not
present, which creates a security risk because tokens without an ID cannot be
revoked later. Add a guard condition before calling storePAT to ensure the
method is only invoked when data.id is present and not null, preventing the
persistence of unrevocable long-lived PATs.

---

953-972: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clear stale patId when retrieval or cleanup lacks an id.

retrievePAT() leaves the previous patId in memory for legacy/no-id PAT records, so a later logout can DELETE the wrong token id for the current bearer token.

🧹 Proposed fix

   private async retrievePAT(): Promise<string | null> {
     if (!this.extensionContext) {
       return null;
     }
     try {
+      this.patId = null;
       const key = this.getPATStorageKey();
       const raw = await this.extensionContext.secrets.get(key);
       if (!raw) {
         return null;
       }
       // Support both legacy (plain token) and new (JSON with username) formats
       try {
         const parsed = JSON.parse(raw) as { token: string; username?: string; id?: number };
+        if (!parsed.token || typeof parsed.token !== "string") {
+          return null;
+        }
         if (parsed.username) {
           this.username = parsed.username;
         }
-        if (parsed.id) {
+        if (typeof parsed.id === "number") {
           this.patId = parsed.id;
         }
         return parsed.token;
       } catch {
         // Legacy plain-text token
         return raw;
@@
   private async clearPAT(): Promise<void> {
+    this.patId = null;
     if (!this.extensionContext) {
       return;
     }

Also applies to: 1006-1012

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@vscode/core/src/hub/HubConnectionManager.ts` around lines 953 - 972, The
retrievePAT() method leaves stale patId values in memory when encountering
legacy PAT records or PAT records without an id field, which causes subsequent
logout operations to potentially delete the wrong token. In the retrievePAT()
method, explicitly clear this.patId to null when the parsed PAT object does not
contain an id field, ensuring that old patId values from previous PAT records do
not persist. Apply the same fix to the cleanup code section mentioned in the
comment (lines 1006-1012) to ensure consistent behavior across all PAT handling
paths.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@vscode/core/src/hub/HubConnectionManager.ts`:
- Around line 657-665: The oidcLogout() method checks if this.oidcAuthCode
exists before calling endSession(), but after a restart when authenticating via
stored PAT, oidcAuthCode is not initialized even though a persisted idToken may
exist. Before the if (this.oidcAuthCode) check in the oidcLogout() method, add
logic to hydrate and restore the OIDC tokens from persisted storage if they are
available. This ensures that endSession() can be properly called with the
correct token information regardless of whether the OIDC session was initialized
during the current startup or restored from a previous authentication.
- Around line 991-997: The PAT revocation logic in the try block starting with
the fetchFn call does not validate the response status before logging success.
If the DELETE request returns an error status code (401, 403, 500, etc.), the
logger.info call still executes, masking the actual failure. Capture the
response object returned by fetchFn, check its status or ok property to verify
the request succeeded, and only proceed to log the success message if the
response indicates success (2xx status). If the response indicates failure,
throw an error or log an appropriate error message instead of the success
message.

In `@vscode/core/src/hub/OIDCAuthCodeFlow.ts`:
- Around line 157-163: The fetchFn call in the try block does not check the HTTP
response status, so non-OK responses (400, 401, 500, etc.) are treated as
success. Modify the code to capture the response object from the fetchFn call,
then check if the response.ok property is false or if the status code indicates
an error, and throw an Error with descriptive details when a non-OK status is
detected. This ensures server-side logout rejections are properly handled as
failures rather than silently succeeding.
- Around line 193-196: The allowInsecureRequests flag is currently being set
unconditionally on every OAuth request, which removes security safeguards for
all issuers including remote ones. Create a private helper method in the
OIDCAuthCodeFlow class (such as oauthRequestOptions) that conditionally includes
the allowInsecureRequests flag only when the issuer URL is a localhost address
(http://localhost, http://127.0.0.1, or http://[::1]), and returns an object
without this flag for all other issuers. Replace the inline object literal that
spreads oauth.allowInsecureRequests with fetchOptions at lines 193-196, 300-303,
and 332-337 with calls to this helper method, ensuring the flag is only applied
for local development scenarios.
- Around line 287-291: The current code reconstructs the callback URL using only
the code and state parameters, which loses other parameters needed for security
validation. To fix this, update the OAuthCallbackResult interface to include the
original callback URL object as a new property, modify
OIDCLoopbackServer.handleCallback() to capture and return the original URL from
the callback request, and then pass that preserved original URL directly to
validateAuthResponse instead of reconstructing it with only code and state
parameters. This will enable proper validation of the iss parameter and error
responses as required by RFC 9207.

---

Outside diff comments:
In `@vscode/core/src/hub/HubConnectionManager.ts`:
- Around line 895-909: The code persists the PAT token via the storePAT method
call even when data.id is not present, which creates a security risk because
tokens without an ID cannot be revoked later. Add a guard condition before
calling storePAT to ensure the method is only invoked when data.id is present
and not null, preventing the persistence of unrevocable long-lived PATs.
- Around line 953-972: The retrievePAT() method leaves stale patId values in
memory when encountering legacy PAT records or PAT records without an id field,
which causes subsequent logout operations to potentially delete the wrong token.
In the retrievePAT() method, explicitly clear this.patId to null when the parsed
PAT object does not contain an id field, ensuring that old patId values from
previous PAT records do not persist. Apply the same fix to the cleanup code
section mentioned in the comment (lines 1006-1012) to ensure consistent behavior
across all PAT handling paths.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f9efd99d-fdb8-4bf0-99c1-76d56716b1eb

📥 Commits

Reviewing files that changed from the base of the PR and between 7d4488d and a6b61e5.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (7)

• changes/unreleased/1453-oidc-oauth4webapi.yaml
• vscode/core/package.json
• vscode/core/src/hub/HubConnectionManager.ts
• vscode/core/src/hub/OIDCAuthCodeFlow.ts
• vscode/core/src/hub/OIDCTokenStorage.ts
• vscode/core/src/hub/index.ts
• vscode/core/src/utilities/hubConfigStorage.ts

[djzager]

Good direction — delegating to oauth4webapi eliminates a class of subtle protocol bugs. One concern on refresh behavior, plus +1 on two open CodeRabbit threads (replied there separately).

[djzager]

All three concerns addressed: refresh token preserved on transient errors, allowInsecureRequests gated to loopback, and full callback URL passed to validateAuthResponse for RFC 9207 support. Nice work.