-
Notifications
You must be signed in to change notification settings - Fork 8
feat: Add OAuth #2
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
base: main
Are you sure you want to change the base?
Conversation
78eb8d8
to
ee3c6cd
Compare
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.
Pull Request Overview
This PR adds comprehensive OAuth 2.1 authentication support to the MCP TypeScript template with a simple binary configuration approach - authentication is disabled by default but can be easily enabled when needed.
Key Changes
- Optional OAuth 2.1 authentication with JWT token validation and PKCE support
- Modular design allowing complete removal of auth code when not needed
- Enhanced error handling with proper JSON-RPC formatted responses
- Session management and cleanup for MCP connections
Reviewed Changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
src/index.ts | Main server with OAuth middleware integration and session management |
src/config.ts | Extended configuration with OAuth environment variables and validation |
src/auth/ | Complete OAuth 2.1 implementation module with providers, validators, and routes |
package.json | Added OAuth dependencies (jose, express-rate-limit, oauth2-server) |
README.md | Comprehensive OAuth documentation with provider examples |
.env.example | OAuth configuration templates and examples |
console.log( | ||
`🔐 Authentication: ${parsed.ENABLE_AUTH ? "ENABLED" : "DISABLED"}`, | ||
); |
Copilot
AI
Oct 8, 2025
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 console.log violates the structured logging guideline. Use logger from src/logger.ts instead.
Copilot generated this review using guidance from repository custom instructions.
src/config.ts
Outdated
console.log( | ||
`ℹ️ OAUTH_REDIRECT_URI not set, using default: ${parsed.OAUTH_REDIRECT_URI}`, | ||
); |
Copilot
AI
Oct 8, 2025
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 console.log violates the structured logging guideline. Use logger.info() from src/logger.ts instead.
Copilot generated this review using guidance from repository custom instructions.
src/config.ts
Outdated
console.warn( | ||
`⚠️ OAUTH_AUDIENCE not set. Token validation will not check audience. | ||
For production deployments, consider setting OAUTH_AUDIENCE to your API identifier`, | ||
); |
Copilot
AI
Oct 8, 2025
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 console.warn violates the structured logging guideline. Use logger.warn() from src/logger.ts instead.
Copilot generated this review using guidance from repository custom instructions.
const tokens = new Map<string, Token>(); | ||
|
||
// Get client configuration from environment | ||
const config = getConfig(); |
Copilot
AI
Oct 8, 2025
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.
Calling getConfig() at module level can cause initialization order issues. Consider moving this inside functions where it's used or wrapping in a function.
Copilot uses AI. Check for mistakes.
@@ -0,0 +1,88 @@ | |||
import { describe, it, expect, beforeEach } from "vitest"; | |||
import { OAuthProvider } from "./oauth-provider"; |
Copilot
AI
Oct 8, 2025
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.
Import should include .ts extension for consistency with project standards: './oauth-provider.ts'
import { OAuthProvider } from "./oauth-provider"; | |
import { OAuthProvider } from "./oauth-provider.ts"; |
Copilot uses AI. Check for mistakes.
src/auth/discovery.ts
Outdated
export function createAuthorizationServerMetadataHandler() { | ||
return (req: Request, res: Response) => { | ||
try { | ||
// ...existing code... |
Copilot
AI
Oct 8, 2025
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.
This comment appears to be a placeholder or remnant from code generation. It should be removed as it doesn't provide any meaningful information.
// ...existing code... |
Copilot uses AI. Check for mistakes.
logger.info("Initializing OAuth 2.1 authentication with token validation", { | ||
issuer: config.OAUTH_ISSUER, | ||
audience: config.OAUTH_AUDIENCE, | ||
clientId: config.OAUTH_CLIENT_ID, | ||
}); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
an access to OAUTH_ISSUER
This logs sensitive data returned by
an access to OAUTH_AUDIENCE
This logs sensitive data returned by
an access to OAUTH_CLIENT_ID
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
To fix the problem, we should remove or redact the sensitive configuration values from the log statement at line 18. Instead of logging raw values of OAUTH_ISSUER
, OAUTH_AUDIENCE
, and OAUTH_CLIENT_ID
, we can indicate the successful initialization of OAuth authentication without exposing their values. If details are needed for debugging, environment-based conditional logging (e.g., only in development) may be used, but by default, we should not log these values in production.
Specifically, edit src/auth/index.ts:
- Replace the log statement at line 18–22 with a single log message that omits sensitive data, for example:
logger.info("Initializing OAuth 2.1 authentication with token validation");
- No further changes to logging logic or imports are required.
-
Copy modified line R18
@@ -15,11 +15,7 @@ | ||
} | ||
|
||
// TypeScript now knows config.ENABLE_AUTH is true due to discriminated union | ||
logger.info("Initializing OAuth 2.1 authentication with token validation", { | ||
issuer: config.OAUTH_ISSUER, | ||
audience: config.OAUTH_AUDIENCE, | ||
clientId: config.OAUTH_CLIENT_ID, | ||
}); | ||
logger.info("Initializing OAuth 2.1 authentication with token validation"); | ||
|
||
// Create token validator for OAuth 2.1 token validation | ||
const tokenValidator = new OAuthTokenValidator( |
logger.warn("Client secret mismatch", { | ||
clientId, | ||
expectedSecret: client.clientSecret?.substring(0, 3) + "...", | ||
providedSecret: clientSecret.substring(0, 3) + "...", | ||
}); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
an access to OAUTH_CLIENT_SECRET
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
To fix this issue, we need to ensure that no part of the client secret, expected or provided, is logged when there is a mismatch (or at any other time). Instead, we should remove or replace those fields in the logger calls. We can still indicate that a secret was provided (without including any part of its value), and that an expected secret exists, but should not log even partial content.
Changes:
- In the logger call at line 67, remove the
expectedSecret
andprovidedSecret
fields entirely from the log data. - The remainder of the log message can describe an authentication/authorization failure with the associated clientId, but should not expose any form of the secrets themselves.
- No new imports or dependencies are needed.
-
Copy modified lines R69-R70
@@ -66,8 +66,8 @@ | ||
if (clientSecret && client.clientSecret !== clientSecret) { | ||
logger.warn("Client secret mismatch", { | ||
clientId, | ||
expectedSecret: client.clientSecret?.substring(0, 3) + "...", | ||
providedSecret: clientSecret.substring(0, 3) + "...", | ||
secretMismatch: true, | ||
// Do not log any part of the client secret | ||
}); | ||
return false; | ||
} |
redirectUri: originalRequest.redirectUri, | ||
}); | ||
|
||
res.redirect(redirectUrl); |
Check warning
Code scanning / CodeQL
Server-side URL redirect Medium
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
To remediate this untrusted redirect vulnerability, we must validate the redirect_uri
parameter before storing or using it for redirection. The standard approach is to maintain a list of allowed/registered redirect URIs per client, and check that the provided value matches an allowed one. If that's not implemented, as a fallback, we can restrict allowed URIs to ones pointing to local domains (e.g., our application’s host), using origin checking.
The single best way to address this in the current context, without changing the high-level functionality, is to add validation logic that rejects any redirect URI not matching an approved whitelist, or at least ensures it is "local" to the application. Code edits are needed in both places the redirect_uri
is used: (1) in the createAuthorizeHandler
when processing the query parameter, and (2) in the createCallbackHandler
when redirecting. However, it's most appropriate to validate immediately when receiving and storing the redirect_uri
in the pending request (i.e., within createAuthorizeHandler
). That way, the stored value is already guaranteed safe for later use.
To implement this, we should introduce a helper (e.g., isValidRedirectUri
) that checks the redirect_uri
against a whitelist, or at minimum, ensures it is same-origin. In this fix, we'll use a basic same-origin check (customizable later to use a full whitelist). This requires a helper function and modifications to the code in createAuthorizeHandler
to reject bad URIs before storing them in memory. If validation fails, reply with an error instead of continuing the OAuth flow.
The required imports are standard, and no new package needs to be installed.
-
Copy modified lines R7-R17 -
Copy modified lines R75-R87
@@ -4,6 +4,17 @@ | ||
import { getConfig } from "../config.ts"; | ||
import type { OAuthProvider } from "./oauth-provider.ts"; | ||
|
||
|
||
// Helper to validate redirect_uri against our domain (can be replaced with a whitelist) | ||
function isValidRedirectUri(uri: string, allowedOrigin: string): boolean { | ||
try { | ||
const url = new URL(uri, allowedOrigin); | ||
return url.origin === allowedOrigin; | ||
} catch (e) { | ||
return false; | ||
} | ||
} | ||
|
||
interface TokenExchangeResponse { | ||
access_token: string; | ||
token_type: string; | ||
@@ -61,6 +72,19 @@ | ||
code_challenge_method, | ||
} = req.query; | ||
|
||
// Validate redirect_uri to avoid open redirect vulnerability | ||
const allowedOrigin = new URL(config.OAUTH_REDIRECT_URI).origin; | ||
if ( | ||
typeof redirect_uri !== "string" || | ||
!isValidRedirectUri(redirect_uri, allowedOrigin) | ||
) { | ||
logger.warn("Invalid or untrusted redirect_uri provided", { redirect_uri }); | ||
return res.status(400).json({ | ||
error: "invalid_request", | ||
error_description: "Untrusted or invalid redirect_uri", | ||
}); | ||
} | ||
|
||
// Validate required OAuth 2.1 parameters | ||
if (response_type !== "code") { | ||
return res.status(400).json({ |
logger.info("Exchanging authorization code with external provider", { | ||
tokenEndpoint: tokenEndpoint.toString(), | ||
clientId: config.OAUTH_CLIENT_ID, | ||
}); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
an access to OAUTH_CLIENT_ID
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
To fix the problem, we need to avoid logging sensitive data such as config.OAUTH_CLIENT_ID
. In this context, both the info and error logs (lines 325 and 345/347) log the clientId. Since the remainder of the logging is for URLs and status indicators, it's likely sufficient to simply remove the clientId
field from the logged object.
The best way to fix this is:
- In all logger calls within this function, omit
config.OAUTH_CLIENT_ID
entirely from the log data. - This can be done by removing the
clientId
property from the objects passed to logger.info/error. - Alternatively (if needed for debugging), replace with a flag indicating presence/absence or log a placeholder (
[REDACTED]
), though usually even this is unnecessary.
Only edit the shown code in src/auth/routes.ts
, specifically around lines 325, 345, 347, and 327.
@@ -324,7 +324,6 @@ | ||
|
||
logger.info("Exchanging authorization code with external provider", { | ||
tokenEndpoint: tokenEndpoint.toString(), | ||
clientId: config.OAUTH_CLIENT_ID, | ||
}); | ||
|
||
const response = await fetch(tokenEndpoint.toString(), { | ||
@@ -343,7 +342,6 @@ | ||
statusText: response.statusText, | ||
error: errorText, | ||
tokenEndpoint: tokenEndpoint.toString(), | ||
clientId: config.OAUTH_CLIENT_ID, | ||
}); | ||
return null; | ||
} |
logger.error("Token exchange failed", { | ||
status: response.status, | ||
statusText: response.statusText, | ||
error: errorText, | ||
tokenEndpoint: tokenEndpoint.toString(), | ||
clientId: config.OAUTH_CLIENT_ID, | ||
}); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
an access to OAUTH_CLIENT_ID
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
The best way to fix the problem is to avoid logging sensitive configuration data such as the OAuth client ID in error messages. This can be achieved by either removing the field from the logger call, or redacting its value so that only a non-sensitive identifier or a generic message is logged. The recommended fix for this specific context is to remove the logging of clientId: config.OAUTH_CLIENT_ID
in the object passed to logger.error
at line 347, ensuring that sensitive information is not written to logs while maintaining useful error context for debugging. All other information in the logged object is acceptable as it does not expose sensitive credentials.
Only code in the region of the logger call at line 341 in src/auth/routes.ts
needs to be changed: remove or redact the logged value of clientId
.
@@ -343,7 +343,6 @@ | ||
statusText: response.statusText, | ||
error: errorText, | ||
tokenEndpoint: tokenEndpoint.toString(), | ||
clientId: config.OAUTH_CLIENT_ID, | ||
}); | ||
return null; | ||
} |
…ware and configuration
- Added environment configuration for authentication options in .env.example and config.ts. - Introduced GatewayTokenValidator and BuiltinTokenValidator for token validation. - Created OAuthProvider class to handle authorization code flow and token issuance. - Implemented OAuth routes: /authorize, /callback, /token, /introspect, and /revoke. - Added discovery endpoints for OAuth 2.1 compliance. - Updated README.md with detailed authentication setup instructions. - Modified index.ts to conditionally apply authentication middleware based on configuration. - Enhanced error handling and logging throughout the authentication process.
…nd token validation
✅ **Features Added:** - **AUTH_MODE support**: none|resource_server|full modes - **OAuthTokenValidator**: Renamed from GatewayTokenValidator for clarity - **OAuth Provider integration**: Full OAuth server support for 'full' mode - **Comprehensive unit tests**: Config validation and token validator testing ✅ **Components:** - **Config validation**: Proper AUTH_MODE validation with detailed error messages - **Token validation**: JWT + introspection with proper error handling - **OAuth Provider**: Authorization flows for full OAuth server mode - **Unit tests**: 46 tests covering config and token validation scenarios ✅ **AUTH_MODE Behaviors:** - **none**: No authentication required - **resource_server**: Validates tokens from external OAuth provider - **full**: Acts as OAuth authorization server + validates tokens 🔒 **Security**: Proper JWT validation, audience checking, and error handling
Updates OAuth implementation to follow the MCP specification's recommended proxy pattern for external OAuth providers (e.g., Auth0): ## OAuth Proxy Pattern Changes - **Authorization Flow**: Proxy /oauth/authorize requests to external provider - **Token Exchange**: Proxy /oauth/token requests with PKCE validation - **Token Introspection**: Proxy /oauth/introspect to external provider - **Discovery Endpoints**: Advertise proxy endpoints in OAuth metadata ## Configuration Updates - **OAuth Audience Validation**: - `full` mode: Optional with warning if missing - `resource_server` mode: Required, throws error if missing - **Updated AUTH_MODE**: Uses none|full|resource_server values ## Benefits ✅ MCP specification compliant - follows OAuth proxy pattern ✅ Works with external identity providers (Auth0, Okta, etc.) ✅ Maintains MCP client compatibility ✅ Clean separation between OAuth provider and MCP server ## Implementation Details - External OAuth flows proxied through MCP server endpoints - PKCE validation enforced for OAuth 2.1 compliance - Comprehensive unit test coverage (47 passing tests) - Proper error handling and logging for all proxy operations This implements the "MCP way" of OAuth integration as recommended in the MCP specification for external identity provider scenarios. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
logger.info("OAuth authorization server metadata requested", { | ||
issuer: metadata.issuer, | ||
}); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
an access to OAUTH_ISSUER
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
To fix this issue, we should ensure that sensitive data such as the OAuth issuer endpoint is not logged in clear text. The best way to do this while preserving functionality is to remove the sensitive value from the log entry, or redact/mask it if required for troubleshooting. Specifically, in src/auth/discovery.ts
, we should remove issuer: metadata.issuer
from the object logged at line 41, so only non-sensitive information is included. If logging is required for security monitoring, redact the value (issuer: '[REDACTED]'
) rather than log the full string.
No additional dependencies are needed. Only the logging line needs to be changed in this source file.
-
Copy modified line R41
@@ -38,9 +38,7 @@ | ||
], | ||
}; | ||
|
||
logger.info("OAuth authorization server metadata requested", { | ||
issuer: metadata.issuer, | ||
}); | ||
logger.info("OAuth authorization server metadata requested"); | ||
|
||
res.json(metadata); | ||
} catch (error) { |
logger.info("OAuth protected resource metadata requested", { | ||
resource: metadata.resource, | ||
authorization_servers: metadata.authorization_servers, | ||
}); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
an access to OAUTH_ISSUER
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
To fix the problem, you should remove any logging of sensitive configuration values, such as OAUTH_ISSUER
, from info-level log entries. Instead of logging the value of configuration fields (such as issuer URLs or lists of authorization servers), log only high-level operational messages without including the sensitive fields themselves. In this file, that means updating the object passed to logger.info
on line 85 to avoid logging authorization_servers
(i.e., config.OAUTH_ISSUER) or any other potentially sensitive values, and (optionally for extra caution) reconsider what is logged on line 41 as well. Only the call on line 85 logs a direct reference to the sensitive value flagged by CodeQL, so this is the region to change.
No new imports or helper definitions are required. Simply remove the offending fields from the logger statement.
-
Copy modified line R86
@@ -83,8 +83,7 @@ | ||
}; | ||
|
||
logger.info("OAuth protected resource metadata requested", { | ||
resource: metadata.resource, | ||
authorization_servers: metadata.authorization_servers, | ||
// No sensitive configuration is logged here. | ||
}); | ||
|
||
res.json(metadata); |
logger.info("OAuth discovery endpoints registered", { | ||
discovery: [ | ||
"/.well-known/oauth-authorization-server", | ||
"/.well-known/oauth-protected-resource", | ||
"/.well-known/openid_configuration", | ||
], | ||
issuer: config.OAUTH_ISSUER, | ||
}); |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
an access to OAUTH_ISSUER
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
The core fix is to avoid logging the value of config.OAUTH_ISSUER
as clear text, especially since this field may contain sensitive metadata.
The logging statement at line 213 logs an object with the property issuer: config.OAUTH_ISSUER
. To address this, simply remove the issuer
property from the logged object. If information about the presence of an issuer is important for diagnostics, you can log only that OAUTH_ISSUER is configured or indicate it is present, without outputting the actual value.
Edit file src/index.ts
:
- Remove or redact the
issuer
field from the object in the call tologger.info
at line 213. - No changes to imports or library usage are required.
-
Copy modified line R219
@@ -216,7 +216,7 @@ | ||
"/.well-known/oauth-protected-resource", | ||
"/.well-known/openid_configuration", | ||
], | ||
issuer: config.OAUTH_ISSUER, | ||
issuer: config.OAUTH_ISSUER ? "[REDACTED]" : undefined, | ||
}); | ||
} | ||
|
- Simplified the authentication initialization process by removing the OAuthProvider from the `initializeAuth` function. - Enhanced the `createAuthenticationMiddleware` to conditionally use the OAuthProvider for full mode. - Introduced a new `createOAuthProviderAuthMiddleware` for handling authentication with the external OAuth provider. - Added a new `oauth-model.ts` file to manage OAuth-related data and operations, including token and authorization code handling. - Updated the `OAuthProvider` class to store external token information and manage authorization codes with PKCE support. - Implemented a new token exchange flow in the `createCallbackHandler` to handle authorization code exchanges with the external provider. - Enhanced error handling and logging throughout the OAuth flow. - Updated configuration validation to ensure required OAuth parameters are set for full mode. - Registered new OAuth routes and discovery endpoints in the main application setup.
…thentication logic
…ltiple files for clarity
- Replace custom error format with standard JSON-RPC 2.0 structure - Use appropriate error codes: -32600 (Invalid Request), -32000 (Connection Closed), -32603 (Internal Error) - Improves MCP specification compliance 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Enable client authentication for authorization code flow (OAuth 2.1 compliance) - Remove sensitive token fragments from log messages - Replace token substrings with token length for debugging - Improves security by preventing token exposure in logs 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…eration - Replace hardcoded "demo-user" fallbacks with proper random user ID generation - Update variable names from "demoClient" to "configuredClient" for clarity - Generate unique user IDs using randomBytes for better security - Update comments to reflect production considerations - Add generateUserId() method to OAuthProvider for consistent ID generation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Install express-rate-limit dependency for protection against abuse - Add comprehensive rate limiting configuration with JSON-RPC 2.0 error format - Configure different limits: 100/15min for OAuth endpoints, 10/15min for token endpoint - Include structured logging for rate limit violations - Prepare for applying rate limits to sensitive OAuth routes 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Apply comprehensive rate limiting to /oauth/authorize and callback endpoints (100 req/15min) - Apply stricter rate limiting to /oauth/token endpoint (10 req/15min) - Include structured logging for rate limit violations with IP and user agent tracking - Use JSON-RPC 2.0 compliant error responses for rate limit exceeded scenarios - Protect against OAuth abuse and brute force attacks 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add session timestamp tracking for MCP transport connections - Implement automatic cleanup of stale sessions (30+ minutes inactive) - Properly close transport connections to prevent resource leaks - Schedule cleanup every 10 minutes with structured logging - Track both session creation and access times for accurate timeout detection - Log cleanup statistics including cleaned and active session counts 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
46ada82
to
4982461
Compare
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.
Pull Request Overview
Copilot reviewed 20 out of 22 changed files in this pull request and generated 6 comments.
console.log( | ||
`ℹ️ OAUTH_REDIRECT_URI not set, using default: ${redirectUri}`, | ||
); |
Copilot
AI
Oct 8, 2025
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 console.log
directly violates the coding guidelines. Use the structured logger from src/logger.ts
instead.
Copilot generated this review using guidance from repository custom instructions.
// ...existing code... | ||
|
Copilot
AI
Oct 8, 2025
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 comment // ...existing code...
is unclear and doesn't provide meaningful context. Either remove it or replace with a descriptive comment explaining what code would be here.
// ...existing code... |
Copilot uses AI. Check for mistakes.
|
||
const config = { | ||
clientId: "test-client", | ||
clientSecret: "test-secret", |
Copilot
AI
Oct 8, 2025
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 clientSecret
property in the test config object doesn't match the OAuthConfig
interface which doesn't include clientSecret
. This suggests either the test is incorrect or the interface is incomplete.
clientSecret: "test-secret", |
Copilot uses AI. Check for mistakes.
it("should verify PKCE correctly", () => { | ||
// @ts-ignore | ||
expect(provider["verifyPKCE"]("abc", "").toString()).toBe("false"); | ||
// Real PKCE test would require correct challenge | ||
}); |
Copilot
AI
Oct 8, 2025
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 @ts-ignore
to access private methods in tests is an anti-pattern. Consider either making the method internal/protected for testing or testing the public behavior that relies on this private method.
it("should verify PKCE correctly", () => { | |
// @ts-ignore | |
expect(provider["verifyPKCE"]("abc", "").toString()).toBe("false"); | |
// Real PKCE test would require correct challenge | |
}); |
Copilot uses AI. Check for mistakes.
it("should generate user IDs in expected format", () => { | ||
// @ts-ignore | ||
const userId = provider["generateUserId"](); | ||
expect(userId.startsWith("user-")).toBe(true); | ||
expect(userId.length).toBeGreaterThan(10); | ||
}); |
Copilot
AI
Oct 8, 2025
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 @ts-ignore
to access private methods in tests is an anti-pattern. Consider either making the method internal/protected for testing or testing the public behavior that relies on this private method.
it("should generate user IDs in expected format", () => { | |
// @ts-ignore | |
const userId = provider["generateUserId"](); | |
expect(userId.startsWith("user-")).toBe(true); | |
expect(userId.length).toBeGreaterThan(10); | |
}); | |
// Removed test that directly accesses private method generateUserId. | |
// To test user ID format, add a test for a public method that uses generateUserId, if available. | |
// If no such method exists, consider refactoring to allow testing via the public API. |
Copilot uses AI. Check for mistakes.
// Get client configuration from environment | ||
const config = getConfig(); | ||
const configuredClient: Client = { | ||
id: config.OAUTH_CLIENT_ID, | ||
clientSecret: config.OAUTH_CLIENT_SECRET, | ||
redirectUris: [ | ||
"http://localhost:3000/callback", | ||
"vscode://ms-vscode.claude-dev", | ||
], | ||
grants: ["authorization_code"], | ||
}; | ||
|
||
// Initialize client data | ||
clients.set(configuredClient.id, configuredClient); |
Copilot
AI
Oct 8, 2025
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.
Calling getConfig()
at module load time could cause issues if the configuration isn't ready or if this module is imported during testing. Consider moving this call inside functions where it's needed.
// Get client configuration from environment | |
const config = getConfig(); | |
const configuredClient: Client = { | |
id: config.OAUTH_CLIENT_ID, | |
clientSecret: config.OAUTH_CLIENT_SECRET, | |
redirectUris: [ | |
"http://localhost:3000/callback", | |
"vscode://ms-vscode.claude-dev", | |
], | |
grants: ["authorization_code"], | |
}; | |
// Initialize client data | |
clients.set(configuredClient.id, configuredClient); | |
// Client initialization is now performed via initializeClients() | |
/** | |
* Initialize OAuth clients from configuration. | |
* Must be called before using the oauthModel. | |
*/ | |
export function initializeClients() { | |
const config = getConfig(); | |
const configuredClient: Client = { | |
id: config.OAUTH_CLIENT_ID, | |
clientSecret: config.OAUTH_CLIENT_SECRET, | |
redirectUris: [ | |
"http://localhost:3000/callback", | |
"vscode://ms-vscode.claude-dev", | |
], | |
grants: ["authorization_code"], | |
}; | |
clients.set(configuredClient.id, configuredClient); | |
} |
Copilot uses AI. Check for mistakes.
This PR adds OAuth to the template.