-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add PKCE integration for Hugging Face #7881
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! I've reviewed the PKCE integration for Hugging Face and found several issues that need attention. The implementation works but has security and code quality concerns that should be addressed.
| */ | ||
|
|
||
| // Hugging Face OAuth client ID for PKCE flow | ||
| export const HUGGING_FACE_OAUTH_CLIENT_ID = "aba045f7-aceb-4e53-9247-5c85d7c2b7cb" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security concern: While OAuth client IDs are public, hardcoding them makes rotation difficult and prevents environment-specific configurations. Consider using environment variables or configuration files to allow overriding for different environments (dev/staging/prod).
| headers: { "Content-Type": "application/x-www-form-urlencoded" }, | ||
| }) | ||
|
|
||
| const accessToken: string | undefined = response.data?.access_token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing validation of the access token format. Consider validating the token structure and checking that it's a non-empty string before using it.
| } | ||
|
|
||
| // Optional state validation (if state was provided in the callback) | ||
| if (returnedState && pkceData.state && returnedState !== pkceData.state) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The state validation is incomplete. It only checks for mismatch but doesn't verify the state format or prevent replay attacks. Consider adding timestamp validation to ensure the state is fresh (e.g., not older than 10 minutes).
| } | ||
|
|
||
| await this.upsertProviderProfile(currentApiConfigName, newConfiguration) | ||
| } catch (error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error handling doesn't clean up stored PKCE data on all error paths. The secrets should be deleted in a finally block to ensure cleanup happens regardless of success or failure.
| const state = message.values?.state | ||
| if (typeof verifier === "string" && typeof state === "string" && verifier.length > 0 && state.length > 0) { | ||
| try { | ||
| await provider.context.secrets.store("huggingFacePkce", JSON.stringify({ verifier, state })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using context.secrets for temporary PKCE data isn't ideal as secrets are meant for long-term credentials. Consider using extension global state or in-memory storage for temporary OAuth flow data.
| // Open externally via extension | ||
| vscode.postMessage({ type: "openExternal", url: authUrl }) | ||
| } catch (e) { | ||
| console.error("Failed to start Hugging Face OAuth:", e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for PKCE challenge generation. The catch block only logs to console but doesn't notify the user. Consider showing a user-friendly error message.
| const pkceData = pkceSecret ? JSON.parse(pkceSecret) : undefined | ||
|
|
||
| if (!pkceData || !pkceData.verifier || !pkceData.state) { | ||
| throw new Error("PKCE verifier or state not found in extension state.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error messages don't follow the i18n pattern used elsewhere in the codebase. Should use the translation function for consistency.
|
|
||
| // HuggingFace | ||
|
|
||
| async handleHuggingFaceCallback(code: string, returnedState?: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting common OAuth handling logic. The pattern here is duplicated across Glama, OpenRouter, and Requesty providers. A shared OAuth handler would reduce code duplication and make maintenance easier.
Important
Adds PKCE-based OAuth integration for Hugging Face, updating UI components, configuration, and localization to support the new authentication flow.
HuggingFace.tsxandWelcomeView.tsx.handleUri.tsto handle Hugging Face OAuth callback.storeHuggingFacePkcemessage handling inwebviewMessageHandler.ts.HUGGING_FACE_OAUTH_CLIENT_IDinoauth-constants.ts.getHuggingFaceAuthUrl()inurls.tsfor generating OAuth URLs.ApiOptions.tsxandSettingsView.tsxto include Hugging Face settings.ApiOptions.tsx.pkce-challengetopackage.jsonfor PKCE support.This description was created by
for 9d94c33. You can customize this summary. It will automatically update as commits are pushed.