feat: use local HTTP server for OAuth callback - AAP-70006#2695
feat: use local HTTP server for OAuth callback - AAP-70006#2695
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a conditional local loopback OAuth callback and propagates the effective redirect URI through the OAuth flow; introduces OAuth-session sign-out when switching LLM providers; and removes several inline ESLint-disable comments and updates tests to include the new authentication provider dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Ext as Extension
participant Auth as OAuth /o/authorize
participant Local as Local Loopback (127.0.0.1)
participant VSCode as VS Code URI Handler
participant Token as OAuth /o/token
Note over Ext,Auth: login() starts OAuth flow
Ext->>Auth: open /o/authorize?redirect_uri=(local or external)
User->>Auth: authenticate & authorize
Auth-->>User: redirect to redirect_uri?code=CODE
alt redirect to local loopback
User->>Local: GET /callback?code=CODE
Local->>Ext: deliver code + redirect_uri
else redirect to VS Code handler
User->>VSCode: vscode://.../callback?code=CODE
VSCode->>Ext: deliver URI containing code
end
Ext->>Token: POST /o/token with code and effective redirect_uri
Token-->>Ext: return tokens
Ext->>Local: close server (if open)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@goneri Not sure if this was supposed to fix the broken macos login but still does not work for me. I authorized the login and ended up on confirmation page but vscode was still waiting for confirmation and there was no way to manually finish the process. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
ssbarnea
force-pushed
the
goneri/feat-use-local-HTTP-server-for-OAuth-callback_22990
branch
from
ffdc2ea to
a9dd623
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/lightspeed/lightSpeedOAuthProvider.ts`:
- Line 95: The private field _activeRedirectUri is incorrectly shared across the
provider; remove this mutable field and keep the redirect URI scoped to a single
auth attempt by passing the redirectUri as an argument through the call
chain—update login() to obtain the redirectUri and pass it into
handleUriForCode(redirectUri) and then into
requestOAuthAccountFromCode(redirectUri) (and any intermediate helpers that
currently rely on _activeRedirectUri), and remove all reads/writes of
_activeRedirectUri so each login flow uses its own redirectUri without
cross-request mutation.
- Around line 310-320: The callback page currently says "Authentication
successful" before the token exchange and reflects raw query text into HTML;
change the user-facing success string in the res.end call to a neutral message
such as "Authorization code received; you may close this tab" (so it doesn't
claim final success before token exchange) and escape/sanitize the errorMsg
value before interpolating it into the HTML (use an HTML-escape helper or
replace &,<,>,",') to prevent reflected markup; update the res.end branch that
uses errorMsg and keep resolveCode(resolve) behavior unchanged (references:
res.writeHead, res.end, resolveCode, errorMsg).
🪄 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: 37670388-a237-43ba-8134-77e23f8871f6
📒 Files selected for processing (1)
src/features/lightspeed/lightSpeedOAuthProvider.ts
| const useLoopback = | ||
| LightSpeedAuthenticationProvider.LOOPBACK_CALLBACK_HOSTS.includes( | ||
| base_uri, | ||
| ); | ||
|
|
||
| let localServer: | ||
| | Awaited<ReturnType<typeof this.startLocalCallbackServer>> | ||
| | undefined; | ||
| let redirectUri: string; | ||
|
|
||
| if (useLoopback) { | ||
| localServer = await this.startLocalCallbackServer(); | ||
| redirectUri = localServer.redirectUri; | ||
| this._logger.debug( | ||
| `[ansible-lightspeed-oauth] Using local callback redirect URI: ${redirectUri}`, | ||
| ); | ||
| } else { | ||
| redirectUri = this._externalRedirectUri; | ||
| this._logger.debug( | ||
| `[ansible-lightspeed-oauth] Using external redirect URI: ${redirectUri}`, | ||
| ); | ||
| } | ||
|
|
||
| const searchParams = new URLSearchParams([ | ||
| ["response_type", "code"], | ||
| ["code_challenge", getCodeChallenge()], | ||
| ["code_challenge_method", "S256"], | ||
| ["client_id", LIGHTSPEED_CLIENT_ID], | ||
| ["redirect_uri", redirectUri], |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In OAuth 2.0 / PKCE for native apps, does a single authorization request redirect only to the one redirect_uri sent in that request, or can it effectively fall back to a second redirect URI if the first callback path fails?
💡 Result:
In OAuth 2.0 / PKCE for native apps, a single authorization request redirects only to the one redirect_uri sent in that request. The authorization server does not fallback to a second redirect URI if the first callback path fails. The server strictly validates the redirect_uri parameter against registered values for the client using exact string matching (per RFC 6749 Section 3.1.2.3 and RFC 8252 Section 8.4) and redirects the user agent exclusively to that URI if valid. If invalid or mismatched, it returns an error without redirecting. There is no provision in the protocol for automatic fallback to alternative URIs; client apps must ensure the specified redirect_uri is reachable.
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc8252.html
- 2: https://rfc-editor.org/rfc/rfc6749.html
- 3: https://rfcinfo.com/rfc-8252/8/
- 4: https://www.oauth.com/oauth2-servers/redirect-uris/redirect-uri-validation/
- 5: http://www.rfc-editor.org/info/rfc6749
- 6: https://rfcinfo.com/rfc-6749/protocolEndpoint
This does not provide a real vscode:// fallback.
When useLoopback is true, the authorize request at lines 385–390 specifies the loopback redirect_uri. Per OAuth 2.0 (RFC 6749 / RFC 8252), the authorization server redirects exclusively to that one registered URI—there is no automatic fallback to alternative callback handlers. If the loopback callback path fails after the browser step completes, the URI-handler branch at lines 396–400 cannot intercept it. To provide actual recovery, retry the authorization request with this._externalRedirectUri after loopback fails, or have the callback page explicitly hand off to the custom URI handler.
Also applies to: 396–400, 415–439
goneri
force-pushed
the
goneri/feat-use-local-HTTP-server-for-OAuth-callback_22990
branch
from
388995c to
bc721e3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/features/lightspeed/lightSpeedOAuthProvider.ts (1)
311-313:⚠️ Potential issue | 🟡 MinorMessage is premature and potentially misleading.
"Authentication successful" is displayed before the token exchange (line 432) completes. If the subsequent token exchange fails, the user sees a success message in the browser but VS Code remains waiting. This may explain the issue ssbarnea reported where "the browser shows a confirmation page but VS Code remains waiting."
Consider using a neutral message that doesn't claim final success:
✏️ Suggested change
if (code) { res.end( - "<html><body><h1>Authentication successful</h1><p>You can close this tab and return to your editor.</p></body></html>", + "<html><body><h1>Authorization received</h1><p>Return to your editor to finish signing in.</p></body></html>", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/lightspeed/lightSpeedOAuthProvider.ts` around lines 311 - 313, The browser message is sent via res.end(...) before the token exchange completes (the token exchange call at line 432), which can show "Authentication successful" even if exchange fails; change the flow so the HTTP response shows a neutral "You may close this window" message (or wait) and only send a final success page after the token-exchange promise resolves successfully, and send an error/failure page if the token exchange rejects; update the code around the res.end and the token-exchange call so the response is sent conditionally based on the success/failure of that token exchange.
🧹 Nitpick comments (2)
src/features/lightspeed/lightSpeedOAuthProvider.ts (2)
429-435: Consider adding trace logging for OAuth flow debugging.Given the reported issue where VS Code remains waiting after the browser shows success, adding trace logging around the token exchange could help diagnose where the flow stalls:
🔍 Suggested logging for debugging
if (localServer) { candidates.push( - localServer.codePromise.then((code) => - this.requestOAuthAccountFromCode(code, redirectUri), - ) as Promise<OAuthAccount>, + localServer.codePromise.then((code) => { + this._logger.debug( + "[ansible-lightspeed-oauth] Authorization code received from local server, initiating token exchange...", + ); + return this.requestOAuthAccountFromCode(code, redirectUri); + }) as Promise<OAuthAccount>, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/lightspeed/lightSpeedOAuthProvider.ts` around lines 429 - 435, Add trace logging around the local OAuth token exchange to help debug where the flow stalls: before awaiting localServer.codePromise and calling requestOAuthAccountFromCode(…, redirectUri) log that a code was received (include the redirectUri and a masked code), after the promise resolves log success with the returned OAuthAccount (or its id), and on rejection log the error stack; target the block that pushes into candidates (localServer, codePromise, requestOAuthAccountFromCode, redirectUri) so the logs bracket the token-exchange call and its outcome.
336-338: Consider adding error handling for server close.
server.close()can emit an error if the server is not running. For robustness, consider handling potential errors:♻️ Optional improvement
const close = () => { - server.close(); + server.close((err) => { + if (err) { + this._logger.debug( + `[ansible-lightspeed-oauth] Error closing local callback server: ${err.message}`, + ); + } + }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/lightspeed/lightSpeedOAuthProvider.ts` around lines 336 - 338, The close() helper currently calls server.close() without handling errors; update the close function to handle potential failures from server.close by either checking server.listening before calling or using server.close((err) => { /* handle/log err */ }) and/or attaching a one-time 'error' handler on server to surface errors; reference the close function and the server variable so the change ensures errors are logged or propagated instead of being swallowed or crashing the process.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/features/lightspeed/lightSpeedOAuthProvider.ts`:
- Around line 311-313: The browser message is sent via res.end(...) before the
token exchange completes (the token exchange call at line 432), which can show
"Authentication successful" even if exchange fails; change the flow so the HTTP
response shows a neutral "You may close this window" message (or wait) and only
send a final success page after the token-exchange promise resolves
successfully, and send an error/failure page if the token exchange rejects;
update the code around the res.end and the token-exchange call so the response
is sent conditionally based on the success/failure of that token exchange.
---
Nitpick comments:
In `@src/features/lightspeed/lightSpeedOAuthProvider.ts`:
- Around line 429-435: Add trace logging around the local OAuth token exchange
to help debug where the flow stalls: before awaiting localServer.codePromise and
calling requestOAuthAccountFromCode(…, redirectUri) log that a code was received
(include the redirectUri and a masked code), after the promise resolves log
success with the returned OAuthAccount (or its id), and on rejection log the
error stack; target the block that pushes into candidates (localServer,
codePromise, requestOAuthAccountFromCode, redirectUri) so the logs bracket the
token-exchange call and its outcome.
- Around line 336-338: The close() helper currently calls server.close() without
handling errors; update the close function to handle potential failures from
server.close by either checking server.listening before calling or using
server.close((err) => { /* handle/log err */ }) and/or attaching a one-time
'error' handler on server to surface errors; reference the close function and
the server variable so the change ensures errors are logged or propagated
instead of being swallowed or crashing the process.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d36b7a8-deb8-44c7-9caf-73bdfd29070d
📒 Files selected for processing (1)
src/features/lightspeed/lightSpeedOAuthProvider.ts
Replace reliance on the vscode:// URI scheme handler (which is not always functional at OS level) with a local HTTP server on a random port to receive the OAuth redirect. The vscode:// URI handler is kept as a parallel fallback. This behaviour is only enabled for the Lightspeed / SAAS endpoint. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/lightspeed/commands/providerCommands.ts (1)
117-148:⚠️ Potential issue | 🟠 MajorHandle same-provider endpoint changes in this flow too.
Line 146 only signs out when the provider type changes. If the user re-runs this command for the same OAuth-backed provider and changes
apiEndpointin the prompts below, the old session survives even though subsequent requests target a different host. Mirror the endpoint-change check fromhandleSaveSettings()here, or centralize this policy so the two entry points cannot drift.Also applies to: 153-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/lightspeed/commands/providerCommands.ts` around lines 117 - 148, The flow currently only signs out OAuth sessions when the provider type changes; update it to also detect endpoint changes like handleSaveSettings() does: capture the previous apiEndpoint (from llmProviderSettings.getSettings() or similar) before calling llmProviderSettings.setProvider, then after applying the new settings/prompts compare the new apiEndpoint to the previous one and call signOutOAuthSessions() if either the provider type changed or the apiEndpoint changed; reference llmProviderSettings.setProvider, previousProvider, signOutOAuthSessions, and mirror the endpoint-change logic from handleSaveSettings() so both entry points use the same policy.
♻️ Duplicate comments (2)
src/features/lightspeed/lightSpeedOAuthProvider.ts (2)
327-329:⚠️ Potential issue | 🟠 MajorReject loopback bind failures explicitly.
Line 327 only waits for the successful
listeningpath. Bind failures are emitted via'error', so this await never rejects and the login can hang or surface as an unhandled server error instead of a user-facing failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/lightspeed/lightSpeedOAuthProvider.ts` around lines 327 - 329, The current await new Promise block only resolves on server.listen's success and ignores bind errors; update the Promise that wraps server.listen (the one using server.listen(0, "127.0.0.1", () => resolve())) to also attach an 'error' listener that rejects the Promise when the server emits 'error', and ensure you remove the 'error' listener when the 'listening' callback runs to avoid leaks; reference the same server variable and the 'listening' and 'error' events so the login flow fails fast on bind failures instead of hanging.
373-439:⚠️ Potential issue | 🟠 MajorThis still is not a real
vscode://fallback.Line 391 sends the authorize request with the loopback
redirect_uri, so the auth server will only redirect there. The URI-handler branch at Lines 397-439 can race the same attempt, but it cannot recover a broken loopback callback path. To keep the promised fallback, retry authorization withthis._externalRedirectUriafter loopback failure, or have the loopback page explicitly hand off to the custom URI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/lightspeed/lightSpeedOAuthProvider.ts` around lines 373 - 439, The current flow starts the loopback server (startLocalCallbackServer) and opens the authorize URI using the loopback redirect, but never retries or falls back to the external vscode:// handler when the loopback callback fails; update the logic around useLoopback/redirectUri, promiseFromEvent(this._uriHandler.event, this.handleUriForCode(redirectUri)) and the Promise.race candidates so that if the loopback path times out or rejects (including the localServer.codePromise rejection), you programmatically retry the authorization by opening a new authorize URI using this._externalRedirectUri and a new promiseFromEvent bound to handleUriForCode(this._externalRedirectUri), then await that second attempt (and ensure you cancel previous listeners via cancelWaitingForRedirectUrl and any localServer resources) before finally rejecting after LIGHTSPEED_SERVICE_LOGIN_TIMEOUT; also ensure requestOAuthAccountFromCode is reused for both loopback and external CODE cases.
🧹 Nitpick comments (2)
src/features/lightspeed/vue/views/llmProviderMessageHandlers.ts (1)
9-9: Use a type-only import here.Line 9 is only used in annotations. Keeping it as a value import adds a runtime edge into
lightSpeedOAuthProvider.ts; if the build preserves value imports, that closes a new cycle back intosrc/extension.ts.♻️ Proposed fix
-import { LightSpeedAuthenticationProvider } from "@src/features/lightspeed/lightSpeedOAuthProvider"; +import type { LightSpeedAuthenticationProvider } from "@src/features/lightspeed/lightSpeedOAuthProvider";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/lightspeed/vue/views/llmProviderMessageHandlers.ts` at line 9, The import of LightSpeedAuthenticationProvider in llmProviderMessageHandlers.ts is only used for type annotations and should be converted to a type-only import to avoid introducing a runtime dependency and potential cycle; replace the current value import of LightSpeedAuthenticationProvider with a type-only import (e.g., using "import type { LightSpeedAuthenticationProvider } ...") so only the type is emitted and no runtime edge into lightSpeedOAuthProvider.ts is created.test/unit/lightspeed/views/llmProviderMessageHandlers.test.ts (1)
182-190: Assert the new session-cleanup path.The mock added here is never exercised. Please add cases that verify
getSessions()/removeSession()run when the provider or endpoint changes, and stay untouched when neither changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/lightspeed/views/llmProviderMessageHandlers.test.ts` around lines 182 - 190, Add unit tests in test/unit/lightspeed/views/llmProviderMessageHandlers.test.ts that exercise the session-cleanup path by invoking the same message/handler used for provider or endpoint updates (e.g., call the function that handles LLM provider change messages in this test file). Specifically, add one test that simulates changing the provider and assert mockDeps.lightSpeedAuthenticationProvider.getSessions and .removeSession are called, another test that simulates changing only the endpoint and assert they are called, and a test that simulates no change and asserts those mocks are not called; use the existing vi.fn mocks on getSessions/removeSession, clear or reset mocks between tests (vi.clearAllMocks or similar), and assert call counts (toHaveBeenCalled / toHaveBeenCalledTimes) against mockDeps.lightSpeedAuthenticationProvider.getSessions and removeSession.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/features/lightspeed/commands/providerCommands.ts`:
- Around line 117-148: The flow currently only signs out OAuth sessions when the
provider type changes; update it to also detect endpoint changes like
handleSaveSettings() does: capture the previous apiEndpoint (from
llmProviderSettings.getSettings() or similar) before calling
llmProviderSettings.setProvider, then after applying the new settings/prompts
compare the new apiEndpoint to the previous one and call signOutOAuthSessions()
if either the provider type changed or the apiEndpoint changed; reference
llmProviderSettings.setProvider, previousProvider, signOutOAuthSessions, and
mirror the endpoint-change logic from handleSaveSettings() so both entry points
use the same policy.
---
Duplicate comments:
In `@src/features/lightspeed/lightSpeedOAuthProvider.ts`:
- Around line 327-329: The current await new Promise block only resolves on
server.listen's success and ignores bind errors; update the Promise that wraps
server.listen (the one using server.listen(0, "127.0.0.1", () => resolve())) to
also attach an 'error' listener that rejects the Promise when the server emits
'error', and ensure you remove the 'error' listener when the 'listening'
callback runs to avoid leaks; reference the same server variable and the
'listening' and 'error' events so the login flow fails fast on bind failures
instead of hanging.
- Around line 373-439: The current flow starts the loopback server
(startLocalCallbackServer) and opens the authorize URI using the loopback
redirect, but never retries or falls back to the external vscode:// handler when
the loopback callback fails; update the logic around useLoopback/redirectUri,
promiseFromEvent(this._uriHandler.event, this.handleUriForCode(redirectUri)) and
the Promise.race candidates so that if the loopback path times out or rejects
(including the localServer.codePromise rejection), you programmatically retry
the authorization by opening a new authorize URI using this._externalRedirectUri
and a new promiseFromEvent bound to handleUriForCode(this._externalRedirectUri),
then await that second attempt (and ensure you cancel previous listeners via
cancelWaitingForRedirectUrl and any localServer resources) before finally
rejecting after LIGHTSPEED_SERVICE_LOGIN_TIMEOUT; also ensure
requestOAuthAccountFromCode is reused for both loopback and external CODE cases.
---
Nitpick comments:
In `@src/features/lightspeed/vue/views/llmProviderMessageHandlers.ts`:
- Line 9: The import of LightSpeedAuthenticationProvider in
llmProviderMessageHandlers.ts is only used for type annotations and should be
converted to a type-only import to avoid introducing a runtime dependency and
potential cycle; replace the current value import of
LightSpeedAuthenticationProvider with a type-only import (e.g., using "import
type { LightSpeedAuthenticationProvider } ...") so only the type is emitted and
no runtime edge into lightSpeedOAuthProvider.ts is created.
In `@test/unit/lightspeed/views/llmProviderMessageHandlers.test.ts`:
- Around line 182-190: Add unit tests in
test/unit/lightspeed/views/llmProviderMessageHandlers.test.ts that exercise the
session-cleanup path by invoking the same message/handler used for provider or
endpoint updates (e.g., call the function that handles LLM provider change
messages in this test file). Specifically, add one test that simulates changing
the provider and assert mockDeps.lightSpeedAuthenticationProvider.getSessions
and .removeSession are called, another test that simulates changing only the
endpoint and assert they are called, and a test that simulates no change and
asserts those mocks are not called; use the existing vi.fn mocks on
getSessions/removeSession, clear or reset mocks between tests (vi.clearAllMocks
or similar), and assert call counts (toHaveBeenCalled / toHaveBeenCalledTimes)
against mockDeps.lightSpeedAuthenticationProvider.getSessions and removeSession.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e793d82-4ede-41df-9e05-505af21a3e8d
📒 Files selected for processing (14)
eslint.config.mjssrc/extension.tssrc/features/lightspeed/ansibleContext.tssrc/features/lightspeed/commands/providerCommands.tssrc/features/lightspeed/explorerWebviewViewProvider.tssrc/features/lightspeed/lightSpeedOAuthProvider.tssrc/features/lightspeed/providers/rhcustom.tssrc/features/lightspeed/vue/views/llmProviderMessageHandlers.tssrc/features/quickLinks/utils/quickLinksViewProvider.tstest/unit/lightspeed/commands/providerCommands.test.tstest/unit/lightspeed/providers/base.test.tstest/unit/lightspeed/providers/rhcustom.test.tstest/unit/lightspeed/views/llmProviderMessageHandlers.test.tstest/unit/lightspeed/views/llmProviderPanel.test.ts
💤 Files with no reviewable changes (1)
- src/features/lightspeed/providers/rhcustom.ts
✅ Files skipped from review due to trivial changes (6)
- src/features/quickLinks/utils/quickLinksViewProvider.ts
- src/features/lightspeed/explorerWebviewViewProvider.ts
- src/features/lightspeed/ansibleContext.ts
- test/unit/lightspeed/providers/base.test.ts
- test/unit/lightspeed/providers/rhcustom.test.ts
- test/unit/lightspeed/commands/providerCommands.test.ts
- Use neutral "Authorization received" message on callback page since token exchange hasn't completed yet - Escape HTML in error messages to prevent reflected markup - Handle server.listen() failures by attaching an error listener so the promise rejects instead of hanging indefinitely - Only set up the vscode:// URI handler when not using loopback, since OAuth redirects exclusively to the one redirect_uri in the request Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
goneri
force-pushed
the
goneri/feat-use-local-HTTP-server-for-OAuth-callback_22990
branch
from
1eb28ad to
ab3484a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/lightspeed/lightSpeedOAuthProvider.ts`:
- Around line 311-322: The non-null assertion in the escapeHtml function should
be removed; update the replacement callback in escapeHtml (the function defined
as const escapeHtml = (value: string) => ...) to return a safe fallback instead
of using "![char]" — e.g., look up the character in the mapping and if undefined
return the original char (or a default escape) so the linter rule is satisfied
while behavior is unchanged. Ensure the mapping object remains the same and only
the indexing/return expression is changed to use a safe fallback.
🪄 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: 8d16ae26-572c-4412-9d2b-47607e73a94f
📒 Files selected for processing (1)
src/features/lightspeed/lightSpeedOAuthProvider.ts
Move writeHead into each branch so the error path returns 400 instead of 200, as suggested in PR review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Replace reliance on the
vscode://URI scheme handler (which is notalways functional at OS level) with a local HTTP server on a random
port to receive the OAuth redirect. This behaviour is only enabled
for the Lightspeed / SAAS endpoint.
Changes
127.0.0.1:<random-port>/callbackand use it as theredirect_uriwhen the base URI matches SAAS hosts. The
vscode://URI handler isused for all other endpoints.
redirectUrithrough thecall chain (
handleUriForCode→requestOAuthAccountFromCode)instead of storing it in a mutable field, avoiding cross-request
race conditions.
the provider or API endpoint setting changes, so stale tokens are
not reused.
on startup failure instead of hanging indefinitely.
message (token exchange hasn't completed yet), escape HTML in error
messages to prevent reflected markup, and return HTTP 400 on error.
(
argsIgnorePattern: "^_",varsIgnorePattern: "^_") and removenow-redundant
eslint-disable-next-linecomments.Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com