Skip to content

fix: improve OpenRouter API key callback handling with better error handling and user feedback #6469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
+48 −3
Closed

fix: improve OpenRouter API key callback handling with better error handling and user feedback #6469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/activate/handleUri.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ export const handleUri = async (uri: vscode.Uri) => {
const query = new URLSearchParams(uri.query.replace(/\+/g, "%2B"))
const visibleProvider = ClineProvider.getVisibleInstance()

console.log(`[URI Handler] Received URI: ${uri.toString()}`)
console.log(`[URI Handler] Path: ${path}`)
console.log(`[URI Handler] Query params:`, Object.fromEntries(query.entries()))
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

Choose a reason for hiding this comment

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

Security concern: These logging statements expose potentially sensitive query parameters including authorization codes. Could we redact sensitive parameters like 'code' or use a more secure logging approach? Consider filtering out sensitive keys before logging.


if (!visibleProvider) {
console.error(`[URI Handler] No visible provider found`)
return
}

Expand All @@ -24,7 +29,17 @@ export const handleUri = async (uri: vscode.Uri) => {
case "/openrouter": {
const code = query.get("code")
if (code) {
await visibleProvider.handleOpenRouterCallback(code)
try {
await visibleProvider.handleOpenRouterCallback(code)
} catch (error) {
console.error(`[URI Handler] Failed to handle OpenRouter callback:`, error)
// Error is already shown to user in handleOpenRouterCallback
}
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

Choose a reason for hiding this comment

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

Inconsistent error handling: Here the error is caught but not re-thrown, while in ClineProvider.ts errors are re-thrown after showing user messages. Should these follow the same pattern for consistency? Consider whether the URI handler should also re-throw to maintain error propagation.

} else {
console.error(`[URI Handler] OpenRouter callback received without code parameter`)
vscode.window.showErrorMessage(
"OpenRouter authorization failed: No authorization code received. Please try again.",
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot Jul 31, 2025

Choose a reason for hiding this comment

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

User‐facing error messages (e.g. missing code) should use the translation function (t) rather than hardcoded strings.

Suggested change
"OpenRouter authorization failed: No authorization code received. Please try again.",
t("OpenRouter authorization failed: No authorization code received. Please try again."),

This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.

)
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

Choose a reason for hiding this comment

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

Missing internationalization: This user-facing error message is hardcoded. Could this use the i18n system like other parts of the codebase? Consider using something like t('openrouter:errors.no_authorization_code').

}
break
}
Expand Down
34 changes: 32 additions & 2 deletions src/core/webview/ClineProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,22 +1109,33 @@ export class ClineProvider
// OpenRouter

async handleOpenRouterCallback(code: string) {
this.log(`[OpenRouter] Handling callback with code: ${code.substring(0, 10)}...`)

let { apiConfiguration, currentApiConfigName } = await this.getState()

let apiKey: string
try {
const baseUrl = apiConfiguration.openRouterBaseUrl || "https://openrouter.ai/api/v1"
// Extract the base domain for the auth endpoint
const baseUrlDomain = baseUrl.match(/^(https?:\/\/[^\/]+)/)?.[1] || "https://openrouter.ai"

this.log(`[OpenRouter] Exchanging code for API key at: ${baseUrlDomain}/api/v1/auth/keys`)
const response = await axios.post(`${baseUrlDomain}/api/v1/auth/keys`, { code })

if (response.data && response.data.key) {
apiKey = response.data.key
this.log(`[OpenRouter] Successfully received API key: ${apiKey.substring(0, 10)}...`)
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

Choose a reason for hiding this comment

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

Security concern: Logging partial API keys could still be a security risk, especially in production environments. Is this intentional for debugging? Could we avoid logging API key fragments entirely or use a more secure approach like logging just the key length or a hash?

} else {
throw new Error("Invalid response from OpenRouter API")
}
} catch (error) {
this.log(
`Error exchanging code for API key: ${JSON.stringify(error, Object.getOwnPropertyNames(error), 2)}`,
`[OpenRouter] Error exchanging code for API key: ${JSON.stringify(error, Object.getOwnPropertyNames(error), 2)}`,
)

// Show user-friendly error message
vscode.window.showErrorMessage(
`Failed to get OpenRouter API key: ${error instanceof Error ? error.message : "Unknown error"}. Please try again.`,
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

Choose a reason for hiding this comment

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

Missing internationalization: These user-facing error messages are hardcoded. Could these use the i18n system for consistency with the rest of the codebase? This would also help with future localization efforts.

)
throw error
}
Expand All @@ -1136,7 +1147,26 @@ export class ClineProvider
openRouterModelId: apiConfiguration?.openRouterModelId || openRouterDefaultModelId,
}

await this.upsertProviderProfile(currentApiConfigName, newConfiguration)
try {
await this.upsertProviderProfile(currentApiConfigName, newConfiguration)
this.log(`[OpenRouter] Successfully updated provider profile with new API key`)

// Show success message to user
vscode.window.showInformationMessage("OpenRouter API key has been successfully configured!")
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot Jul 31, 2025

Choose a reason for hiding this comment

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

The success message shown to the user is hardcoded. Please use the t() function for localization.

Suggested change
vscode.window.showInformationMessage("OpenRouter API key has been successfully configured!")
vscode.window.showInformationMessage(t("openrouter:success.api_key_configured"))

This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.

Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

Choose a reason for hiding this comment

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

Missing internationalization: Same concern here - this success message should use the i18n system for consistency and future localization support.


// Ensure the webview is updated with the new state
await this.postStateToWebview()
} catch (error) {
this.log(
`[OpenRouter] Error updating provider profile: ${JSON.stringify(error, Object.getOwnPropertyNames(error), 2)}`,
)

// Show user-friendly error message
vscode.window.showErrorMessage(
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot Jul 31, 2025

Choose a reason for hiding this comment

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

User‐facing error messages (e.g. when saving the API key fails) should use t() for translation instead of inline English strings.

This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.

`Failed to save OpenRouter API key: ${error instanceof Error ? error.message : "Unknown error"}. Please try again.`,
)
throw error
}
}

// Glama
Expand Down