-
Notifications
You must be signed in to change notification settings - Fork 30
feat(oauth): implement automatic token refresh for downstream MCPs #2136
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
🧪 BenchmarkShould we run the MCP Gateway benchmark for this PR? React with 👍 to run the benchmark.
Benchmark will run on the next push after you react. |
Release OptionsShould a new version be published when this PR is merged? React with an emoji to vote on the release type:
Current version: Deployment
|
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.
3 issues found across 11 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/mesh/src/storage/downstream-token.ts">
<violation number="1" location="apps/mesh/src/storage/downstream-token.ts:91">
P2: Race condition in upsert: if two concurrent calls both find no existing token, both will attempt to insert, causing a unique constraint violation. Consider using Kysely's `onConflict` for atomic upsert, or wrap in a transaction with proper locking.</violation>
<violation number="2" location="apps/mesh/src/storage/downstream-token.ts:199">
P1: Invalid date strings in `expiresAt` cause `isExpired` to return `false`, treating corrupted tokens as never expired. When `new Date(invalidString).getTime()` returns `NaN`, the comparison `NaN < Date.now()` is always `false`. Consider validating the date and defaulting to expired (fail-safe) for invalid values.</violation>
</file>
<file name="apps/mesh/src/api/routes/downstream-token.ts">
<violation number="1" location="apps/mesh/src/api/routes/downstream-token.ts:39">
P1: Authorization check missing: The comment states "Verify connection exists and user has access" but the code only checks existence. Consider passing `organizationId` to `findById()` to verify user has access to this connection, preventing cross-organization token association.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| async upsert(data: DownstreamTokenData): Promise<DownstreamToken> { | ||
| const existing = await this.get(data.connectionId, data.userId); |
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.
P2: Race condition in upsert: if two concurrent calls both find no existing token, both will attempt to insert, causing a unique constraint violation. Consider using Kysely's onConflict for atomic upsert, or wrap in a transaction with proper locking.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/storage/downstream-token.ts, line 91:
<comment>Race condition in upsert: if two concurrent calls both find no existing token, both will attempt to insert, causing a unique constraint violation. Consider using Kysely's `onConflict` for atomic upsert, or wrap in a transaction with proper locking.</comment>
<file context>
@@ -0,0 +1,246 @@
+ }
+
+ async upsert(data: DownstreamTokenData): Promise<DownstreamToken> {
+ const existing = await this.get(data.connectionId, data.userId);
+ const now = new Date().toISOString();
+
</file context>
✅ Addressed in 47683a8
| } | ||
|
|
||
| const expiresAt = | ||
| token.expiresAt instanceof Date |
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.
P1: Invalid date strings in expiresAt cause isExpired to return false, treating corrupted tokens as never expired. When new Date(invalidString).getTime() returns NaN, the comparison NaN < Date.now() is always false. Consider validating the date and defaulting to expired (fail-safe) for invalid values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/storage/downstream-token.ts, line 199:
<comment>Invalid date strings in `expiresAt` cause `isExpired` to return `false`, treating corrupted tokens as never expired. When `new Date(invalidString).getTime()` returns `NaN`, the comparison `NaN < Date.now()` is always `false`. Consider validating the date and defaulting to expired (fail-safe) for invalid values.</comment>
<file context>
@@ -0,0 +1,246 @@
+ }
+
+ const expiresAt =
+ token.expiresAt instanceof Date
+ ? token.expiresAt
+ : new Date(token.expiresAt);
</file context>
✅ Addressed in ea42fc8
| } | ||
|
|
||
| // Verify connection exists and user has access | ||
| const connection = await ctx.storage.connections.findById(connectionId); |
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.
P1: Authorization check missing: The comment states "Verify connection exists and user has access" but the code only checks existence. Consider passing organizationId to findById() to verify user has access to this connection, preventing cross-organization token association.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/api/routes/downstream-token.ts, line 39:
<comment>Authorization check missing: The comment states "Verify connection exists and user has access" but the code only checks existence. Consider passing `organizationId` to `findById()` to verify user has access to this connection, preventing cross-organization token association.</comment>
<file context>
@@ -0,0 +1,144 @@
+ }
+
+ // Verify connection exists and user has access
+ const connection = await ctx.storage.connections.findById(connectionId);
+ if (!connection) {
+ return c.json({ error: "Connection not found" }, 404);
</file context>
✅ Addressed in a9a76f9
572cd11 to
45c0bd4
Compare
- Add migration 015 with clientId, clientSecret, tokenEndpoint fields - Create DownstreamTokenStorage for persisting OAuth tokens with refresh info - Add token refresh utility using OAuth2 refresh_token grant - Add API endpoints for managing downstream OAuth tokens - Update proxy to use cached tokens and auto-refresh on expiry - Update frontend to save full token info via new API This fixes the issue where re-authentication was needed after hot reload or when access tokens expired. Now tokens are automatically refreshed using the refresh_token grant when they expire.
Main branch migration 015-monitoring-properties takes priority. OAuth branch migration is renumbered to 016 to maintain correct order.
45c0bd4 to
7e20fbc
Compare
Summary by cubic
Implements automatic OAuth token refresh for downstream MCP connections to keep users signed in when access tokens expire or after hot reload. The proxy now uses cached tokens and refreshes them using the refresh_token grant.
New Features
Migration
Written for commit a9a76f9. Summary will update on new commits.