feat(mcp): remote MCP server for workspaces (v1, read-only)#590
feat(mcp): remote MCP server for workspaces (v1, read-only)#590syednahm wants to merge 32 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
# Conflicts: # src/features/workspaces/operations/workspace-tool-schemas.ts
📝 WalkthroughWalkthroughThis PR adds a remote MCP server at ChangesRemote MCP Server and OAuth Integration
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
10 issues found across 42 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="drizzle/0002_mcp_oauth.sql">
<violation number="1" location="drizzle/0002_mcp_oauth.sql:4">
P1: This migration stores JWT signing private keys and bearer tokens in plain `text` columns. A database leak would expose the `jwks.private_key`, allowing attackers to forge JWTs and impersonate users, and would expose every active refresh/access token for session hijacking. Consider encrypting the signing key at rest (e.g., envelope encryption) and hashing refresh tokens before storage per RFC 9700.</violation>
<violation number="2" location="drizzle/0002_mcp_oauth.sql:18">
P2: The `oauth_consent` table currently allows multiple rows for the same `(client_id, user_id)` pair because it only has separate non-unique indexes on each column. Duplicate consent records can lead to inconsistent scope resolution (e.g., which row wins when querying consent). Adding a composite unique index on `(client_id, user_id)` would prevent this. If anonymous consent is not intended, also consider making `user_id` non-nullable.</violation>
<violation number="3" location="drizzle/0002_mcp_oauth.sql:45">
P2: The `oauth_access_token` and `oauth_refresh_token` tables include `expires_at` (and `revoked` on refresh tokens) but don't have indexes on these lifecycle columns. As token volume grows, cleanup and revocation queries will scan the entire table. Consider adding indexes on `expires_at` for both tables and on `revoked` for `oauth_refresh_token` to avoid future performance degradation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| CREATE TABLE `jwks` ( | ||
| `id` text PRIMARY KEY NOT NULL, | ||
| `public_key` text NOT NULL, | ||
| `private_key` text NOT NULL, |
There was a problem hiding this comment.
P1: This migration stores JWT signing private keys and bearer tokens in plain text columns. A database leak would expose the jwks.private_key, allowing attackers to forge JWTs and impersonate users, and would expose every active refresh/access token for session hijacking. Consider encrypting the signing key at rest (e.g., envelope encryption) and hashing refresh tokens before storage per RFC 9700.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At drizzle/0002_mcp_oauth.sql, line 4:
<comment>This migration stores JWT signing private keys and bearer tokens in plain `text` columns. A database leak would expose the `jwks.private_key`, allowing attackers to forge JWTs and impersonate users, and would expose every active refresh/access token for session hijacking. Consider encrypting the signing key at rest (e.g., envelope encryption) and hashing refresh tokens before storage per RFC 9700.</comment>
<file context>
@@ -0,0 +1,101 @@
+CREATE TABLE `jwks` (
+ `id` text PRIMARY KEY NOT NULL,
+ `public_key` text NOT NULL,
+ `private_key` text NOT NULL,
+ `created_at` integer NOT NULL,
+ `expires_at` integer
</file context>
| @@ -0,0 +1,101 @@ | |||
| CREATE TABLE `jwks` ( | |||
There was a problem hiding this comment.
P2: The oauth_access_token and oauth_refresh_token tables include expires_at (and revoked on refresh tokens) but don't have indexes on these lifecycle columns. As token volume grows, cleanup and revocation queries will scan the entire table. Consider adding indexes on expires_at for both tables and on revoked for oauth_refresh_token to avoid future performance degradation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At drizzle/0002_mcp_oauth.sql, line 45:
<comment>The `oauth_access_token` and `oauth_refresh_token` tables include `expires_at` (and `revoked` on refresh tokens) but don't have indexes on these lifecycle columns. As token volume grows, cleanup and revocation queries will scan the entire table. Consider adding indexes on `expires_at` for both tables and on `revoked` for `oauth_refresh_token` to avoid future performance degradation.</comment>
<file context>
@@ -0,0 +1,101 @@
+--> statement-breakpoint
+CREATE UNIQUE INDEX `oauth_client_client_id_unique` ON `oauth_client` (`client_id`);--> statement-breakpoint
+CREATE INDEX `oauthClient_userId_idx` ON `oauth_client` (`user_id`);--> statement-breakpoint
+CREATE TABLE `oauth_refresh_token` (
+ `id` text PRIMARY KEY NOT NULL,
+ `token` text NOT NULL,
</file context>
| `enable_end_session` integer, | ||
| `subject_type` text, | ||
| `scopes` text, | ||
| `user_id` text, |
There was a problem hiding this comment.
P2: The oauth_consent table currently allows multiple rows for the same (client_id, user_id) pair because it only has separate non-unique indexes on each column. Duplicate consent records can lead to inconsistent scope resolution (e.g., which row wins when querying consent). Adding a composite unique index on (client_id, user_id) would prevent this. If anonymous consent is not intended, also consider making user_id non-nullable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At drizzle/0002_mcp_oauth.sql, line 18:
<comment>The `oauth_consent` table currently allows multiple rows for the same `(client_id, user_id)` pair because it only has separate non-unique indexes on each column. Duplicate consent records can lead to inconsistent scope resolution (e.g., which row wins when querying consent). Adding a composite unique index on `(client_id, user_id)` would prevent this. If anonymous consent is not intended, also consider making `user_id` non-nullable.</comment>
<file context>
@@ -0,0 +1,101 @@
+ `enable_end_session` integer,
+ `subject_type` text,
+ `scopes` text,
+ `user_id` text,
+ `created_at` integer,
+ `updated_at` integer,
</file context>
Greptile SummaryThis PR adds a read-only remote MCP server for workspace access. The main changes are:
Confidence Score: 4/5The MCP connection flow can fail before clients receive a usable bearer token.
src/lib/auth.server.ts, src/routes/[.]well-known/oauth-protected-resource/mcp.ts, src/features/account/connections/oauth-api.ts Important Files Changed
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const baseURL = getAuthBaseURL(); | ||
|
|
||
| return betterAuth({ | ||
| disabledPaths: ["/token"], |
There was a problem hiding this comment.
The MCP OAuth flow redirects a client through consent and then requires an authorization-code exchange before /mcp receives a bearer token. With /token disabled here, discovery-based MCP clients can complete consent but fail when they exchange the returned code, so they never obtain the token that verifyMcpBearerToken requires.
| return Response.json( | ||
| { | ||
| resource: getMcpResourceUrl(appOrigin), | ||
| authorization_servers: [appOrigin], |
There was a problem hiding this comment.
This metadata always points clients at appOrigin, but the auth server base can be configured separately through getAuthBaseURL(). In split-origin or reverse-proxy deployments, MCP clients discover the wrong issuer and build authorization/token URLs against the app host, while token verification expects the configured auth issuer/JWKS path, so the OAuth connection flow fails.
| async function readAuthJson<T>(response: Response): Promise<T> { | ||
| const payload = (await response.json()) as T & { message?: string }; |
There was a problem hiding this comment.
readAuthJson parses every auth response as JSON before checking status. If /api/auth/oauth2/delete-consent returns a successful empty response, the revoke mutation throws during response.json(), the UI reports failure, and the consent list is not refreshed even though the server may have deleted the consent.
| export async function getOAuthConsents(): Promise<OAuthConsentRecord[]> { | ||
| return authGet<OAuthConsentRecord[]>("/api/auth/oauth2/get-consents"); | ||
| } |
There was a problem hiding this comment.
The settings UI types get-consents as camelCase fields like clientId and createdAt, while the OAuth tables and Better Auth-facing client metadata use snake_case names like client_id and created_at. If the provider returns raw OAuth records, consent.clientId is undefined, client lookups request client_id=undefined, and authorized apps render with missing or wrong metadata.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
package.json (1)
43-43: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDependency versions verified; note a known transitive-vuln audit gap for the MCP SDK.
@better-auth/oauth-providerand@modelcontextprotocol/sdkversions are valid and current. However,@modelcontextprotocol/sdk@1.29.0pulls in transitive deps (ajv/fast-uri,hono,express-rate-limit) with published advisories that can failnpm audit --audit-level=highin CI, even though the SDK's own semver ranges already allow the fixed versions.If your CI enforces audit gating, consider adding pnpm
overridesfor these transitive packages until the SDK's lockfile is regenerated upstream.Also applies to: 72-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 43, The dependency entry for `@modelcontextprotocol/sdk` is fine, but the current lockfile can still fail high-severity audit checks because it pulls vulnerable transitive packages. Update package.json to add pnpm overrides for the affected transitive dependencies (ajv/fast-uri, hono, express-rate-limit) so CI audit gating passes until the SDK’s lockfile is refreshed upstream; keep the change anchored near the existing dependency declarations alongside `@better-auth/oauth-provider` and `@modelcontextprotocol/sdk`.src/lib/auth.server.ts (1)
224-226: 🔒 Security & Privacy | 🔵 TrivialOpen dynamic client registration needs an abuse guard.
allowDynamicClientRegistration+allowUnauthenticatedClientRegistration: trueexposes an unauthenticated registration endpoint that writes tooauth_client. Without rate limiting or a registration cap this is an unbounded write/DoS and DB-bloat vector. The PR notes per-call rate limiting is not implemented yet — consider gating the registration path specifically (IP-based rate limit, per-origin cap, or short-lived registration tokens) before enabling this in production.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/auth.server.ts` around lines 224 - 226, The dynamic client registration setup in auth.server needs an abuse guard before keeping allowDynamicClientRegistration with allowUnauthenticatedClientRegistration enabled. Add a registration-specific control in the auth registration path, such as IP-based rate limiting, per-origin caps, or short-lived registration tokens, and ensure the logic is enforced where the OAuth client is created so unauthenticated writes to oauth_client cannot be abused.src/features/workspaces/mcp/mcp-tools.ts (1)
22-31: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTool results skip
outputSchema/structuredContent; consider aligning with MCP's schema-validated output pattern.All three tools return only a JSON-stringified text blob via
mcpTextResult, with nooutputSchemadeclared on anyregisterToolcall. Per current MCP TypeScript SDK guidance, tools that want schema-validated output should declareoutputSchemaand returnstructuredContentmatching it (in addition to thecontenttext block for backward compatibility) — this is also explicitly one of this PR's stated goals ("Return compact, schema-validated outputs...").Also consider adding
annotations: { readOnlyHint: true }to each of these tools, since v1 is explicitly read-only.♻️ Example for one tool
server.registerTool( "thinkex_list_workspaces", { description: "List workspaces accessible to the authenticated user.", + outputSchema: { workspaces: z.array(z.object({ id: z.string(), name: z.string(), description: z.string().nullable(), role: z.string() })) }, + annotations: { readOnlyHint: true }, }, async () => runMcpTool({ actor, deniedResultForCode: mcpListWorkspacesAccessDeniedResult, toolName: "thinkex_list_workspaces", run: async () => { /* ... */ }, }), );
mcpTextResultwould then also need to attachstructuredContent: resultalongsidecontent.Also applies to: 67-144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/workspaces/mcp/mcp-tools.ts` around lines 22 - 31, The MCP tools currently return only JSON text through mcpTextResult, so they are not using schema-validated outputs. Update the three registerTool definitions in mcp-tools.ts to declare an outputSchema and return structuredContent alongside the existing content text, keeping mcpTextResult aligned with that pattern for backward compatibility. Also mark these read-only v1 tools with annotations: { readOnlyHint: true } so the tool metadata matches the intended behavior.src/features/workspaces/mcp/mcp-route.test.ts (1)
1-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the authenticated success path.
Current tests only cover the null-path and missing-token cases. Consider mocking
verifyMcpBearerTokento succeed and assertingcreateMcpHandler/registerMcpToolsreceive the expectedactordata (userId, clientId, scopes mapped intoauthContext.props), to guard the prop-mapping wiring against regressions.src/features/workspaces/mcp/mcp-route.ts (1)
21-61: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider CORS/Origin handling on the
/mcphandler.
createMcpHandleraccepts acorsOptions(and the underlying transport expects Origin validation) but neither is configured here. The MCP Streamable HTTP spec states servers "MUST validate the Origin header on all incoming connections to prevent DNS rebinding attacks," and browser-based MCP clients will fail CORS preflight without explicit headers.Given this endpoint is bearer-token protected, DNS-rebinding risk is reduced (a rebound origin can't forge a valid
Authorizationheader), but if any browser-based MCP client is a target, this gap will break connectivity silently.🌐 Suggested addition
const handler = createMcpHandler(buildMcpServer(actor), { authContext: { props: { userId: actor.userId, clientId: actor.clientId, scopes: [...actor.grantedScopes], }, }, + corsOptions: { + allowedOrigins: [/* trusted origins, if browser-based clients are supported */], + }, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/workspaces/mcp/mcp-route.ts` around lines 21 - 61, The /mcp route currently creates the MCP handler without any Origin/CORS configuration, so browser-based clients may fail preflight and the underlying transport won’t perform the expected Origin validation. Update routeMcpRequest to pass explicit corsOptions and any required origin-check settings into createMcpHandler, using the existing buildMcpServer(actor) and authContext setup unchanged, so the MCP endpoint can accept legitimate browser clients while still validating incoming Origins.src/routes/oauth/consent.tsx (1)
101-145: 🔒 Security & Privacy | 🔵 TrivialConsent-phishing risk with dynamically registered/public clients.
Since the OAuth provider layer supports dynamic and public client registration, any third party can register a client and set an arbitrary
client_name/logo_uri/client_urishown here. This is an inherent OAuth consent-screen risk (not specific to this file) worth keeping in mind operationally — e.g. surfacing the actual redirect/callback domain prominently, or rate-limiting/moderating DCR registrations — since users may over-trust a convincing display name/logo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/oauth/consent.tsx` around lines 101 - 145, The consent screen in consent.tsx currently trusts client-provided metadata from dynamic/public registrations, which can be misleading. Update the consent UI rendering in the application details section to prominently show the actual redirect/callback domain or verified origin alongside clientName/client_uri/logo_uri, and ensure the consent flow does not rely on those fields alone for user trust. Also consider adding a moderation/rate-limit gate for DCR registrations in the OAuth provider layer if that is where client registration is handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/account/connections/oauth-api.ts`:
- Around line 58-70: The readAuthJson helper currently parses the body with
response.json() before verifying response.ok, which causes raw SyntaxError
failures for non-JSON or empty responses instead of the intended fallback.
Update readAuthJson in oauth-api.ts to safely handle invalid/empty bodies by
checking the response status and wrapping JSON parsing in error handling, then
fall back to a generic “Request failed” message when parsing fails or no message
is available; keep the existing message extraction logic for valid JSON
responses.
In `@src/features/workspaces/mcp/mcp-read-content-cap.ts`:
- Around line 20-56: The truncation logic in capMcpReadItemsContent can cut a
UTF-16 surrogate pair when slicing item.content at remainingBudget, which can
produce malformed truncated text for emoji or other astral characters. Update
the truncation path to compute a safe cut position that never splits a surrogate
pair before building truncatedContent, then append
MCP_READ_CONTENT_TRUNCATION_NOTICE as before and keep the truncated flag
behavior unchanged.
In `@src/routes/`[.]well-known/oauth-authorization-server.ts:
- Around line 1-11: The oauth-authorization-server metadata route currently
returns via Route and GET without CORS headers, which can block cross-origin
discovery clients. Update the handler in
createFileRoute("/.well-known/oauth-authorization-server") to attach the same
Access-Control-Allow-Origin behavior used by the protected-resource metadata
route, ideally within the GET handler returned by withAuth/auth.handler so
browser-based access works consistently.
In `@src/routes/oauth/consent.tsx`:
- Around line 95-99: The consent screen in consent.tsx traps the user when
client_id is missing because the error message is shown but both the Deny and
Allow paths are blocked. Update the consent action handling in the consent page
component so the user always has an escape route, either by removing the
client_id guard from the Deny button in the consent actions or by adding another
navigation/back-out option when search.client_id is absent. Make the change in
the consent form/button logic that uses search.client_id and isPending so the
page remains dismissible even for invalid requests.
---
Nitpick comments:
In `@package.json`:
- Line 43: The dependency entry for `@modelcontextprotocol/sdk` is fine, but the
current lockfile can still fail high-severity audit checks because it pulls
vulnerable transitive packages. Update package.json to add pnpm overrides for
the affected transitive dependencies (ajv/fast-uri, hono, express-rate-limit) so
CI audit gating passes until the SDK’s lockfile is refreshed upstream; keep the
change anchored near the existing dependency declarations alongside
`@better-auth/oauth-provider` and `@modelcontextprotocol/sdk`.
In `@src/features/workspaces/mcp/mcp-route.ts`:
- Around line 21-61: The /mcp route currently creates the MCP handler without
any Origin/CORS configuration, so browser-based clients may fail preflight and
the underlying transport won’t perform the expected Origin validation. Update
routeMcpRequest to pass explicit corsOptions and any required origin-check
settings into createMcpHandler, using the existing buildMcpServer(actor) and
authContext setup unchanged, so the MCP endpoint can accept legitimate browser
clients while still validating incoming Origins.
In `@src/features/workspaces/mcp/mcp-tools.ts`:
- Around line 22-31: The MCP tools currently return only JSON text through
mcpTextResult, so they are not using schema-validated outputs. Update the three
registerTool definitions in mcp-tools.ts to declare an outputSchema and return
structuredContent alongside the existing content text, keeping mcpTextResult
aligned with that pattern for backward compatibility. Also mark these read-only
v1 tools with annotations: { readOnlyHint: true } so the tool metadata matches
the intended behavior.
In `@src/lib/auth.server.ts`:
- Around line 224-226: The dynamic client registration setup in auth.server
needs an abuse guard before keeping allowDynamicClientRegistration with
allowUnauthenticatedClientRegistration enabled. Add a registration-specific
control in the auth registration path, such as IP-based rate limiting,
per-origin caps, or short-lived registration tokens, and ensure the logic is
enforced where the OAuth client is created so unauthenticated writes to
oauth_client cannot be abused.
In `@src/routes/oauth/consent.tsx`:
- Around line 101-145: The consent screen in consent.tsx currently trusts
client-provided metadata from dynamic/public registrations, which can be
misleading. Update the consent UI rendering in the application details section
to prominently show the actual redirect/callback domain or verified origin
alongside clientName/client_uri/logo_uri, and ensure the consent flow does not
rely on those fields alone for user trust. Also consider adding a
moderation/rate-limit gate for DCR registrations in the OAuth provider layer if
that is where client registration is handled.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e11be5f-3222-461d-a58d-af9885ac3f6d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
.gitignoredocs/MCP.mddrizzle/0002_mcp_oauth.sqldrizzle/meta/0002_snapshot.jsondrizzle/meta/_journal.jsonpackage.jsonpnpm-workspace.yamlsrc/db/schema.tssrc/features/account/components/SettingsNav.tsxsrc/features/account/components/SettingsPage.tsxsrc/features/account/connections/ConnectionsSettingsPage.tsxsrc/features/account/connections/mcp-setup.functions.tssrc/features/account/connections/mcp-setup.tssrc/features/account/connections/oauth-api.tssrc/features/account/connections/oauth-scope-labels.tssrc/features/workspaces/agent-routes.tssrc/features/workspaces/mcp/mcp-audit.tssrc/features/workspaces/mcp/mcp-auth.test.tssrc/features/workspaces/mcp/mcp-auth.tssrc/features/workspaces/mcp/mcp-read-content-cap.test.tssrc/features/workspaces/mcp/mcp-read-content-cap.tssrc/features/workspaces/mcp/mcp-route.test.tssrc/features/workspaces/mcp/mcp-route.tssrc/features/workspaces/mcp/mcp-schemas.tssrc/features/workspaces/mcp/mcp-tool-access.test.tssrc/features/workspaces/mcp/mcp-tool-access.tssrc/features/workspaces/mcp/mcp-tools.test.tssrc/features/workspaces/mcp/mcp-tools.tssrc/features/workspaces/operations/workspace-page-range-schema.tssrc/features/workspaces/operations/workspace-tool-schemas.tssrc/lib/app-origin.tssrc/lib/auth-client.tssrc/lib/auth.server.tssrc/routeTree.gen.tssrc/routes/[.]well-known/oauth-authorization-server.tssrc/routes/[.]well-known/oauth-protected-resource/mcp.tssrc/routes/_protected/settings.tsxsrc/routes/_protected/settings/connections.tsxsrc/routes/_protected/settings/index.tsxsrc/routes/oauth/consent.tsxsrc/server.ts
There was a problem hiding this comment.
0 issues found across 8 files (changes from recent commits).
Requires human review: Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 7 files (changes from recent commits).
Requires human review: Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="drizzle/0002_mcp_oauth.sql">
<violation number="1" location="drizzle/0002_mcp_oauth.sql:4">
P1: This migration stores JWT signing private keys and bearer tokens in plain `text` columns. A database leak would expose the `jwks.private_key`, allowing attackers to forge JWTs and impersonate users, and would expose every active refresh/access token for session hijacking. Consider encrypting the signing key at rest (e.g., envelope encryption) and hashing refresh tokens before storage per RFC 9700.</violation>
<violation number="2" location="drizzle/0002_mcp_oauth.sql:18">
P2: The `oauth_consent` table currently allows multiple rows for the same `(client_id, user_id)` pair because it only has separate non-unique indexes on each column. Duplicate consent records can lead to inconsistent scope resolution (e.g., which row wins when querying consent). Adding a composite unique index on `(client_id, user_id)` would prevent this. If anonymous consent is not intended, also consider making `user_id` non-nullable.</violation>
<violation number="3" location="drizzle/0002_mcp_oauth.sql:45">
P2: The `oauth_access_token` and `oauth_refresh_token` tables include `expires_at` (and `revoked` on refresh tokens) but don't have indexes on these lifecycle columns. As token volume grows, cleanup and revocation queries will scan the entire table. Consider adding indexes on `expires_at` for both tables and on `revoked` for `oauth_refresh_token` to avoid future performance degradation.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/routes/oauth/consent.tsx">
<violation number="1" location="src/routes/oauth/consent.tsx:69">
P2: When `result.url` exists but `window.location.assign()` throws an error, the catch block handles it correctly with a redirect-failure toast but then falls through to the unconditional toast below (`"Authorization completed, but no redirect was returned."`). This is misleading because the redirect URL was returned — it just failed to execute. The user sees two conflicting messages. Add a `return;` statement at the end of the catch block to prevent this incorrect fallthrough.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| try { | ||
| window.location.assign(result.url); | ||
| return; | ||
| } catch (error) { | ||
| setCompletedAction(null); | ||
| toast.error( | ||
| getErrorMessage(error, "Unable to redirect after authorization. Please try again."), |
There was a problem hiding this comment.
P2: When result.url exists but window.location.assign() throws an error, the catch block handles it correctly with a redirect-failure toast but then falls through to the unconditional toast below ("Authorization completed, but no redirect was returned."). This is misleading because the redirect URL was returned — it just failed to execute. The user sees two conflicting messages. Add a return; statement at the end of the catch block to prevent this incorrect fallthrough.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/oauth/consent.tsx, line 69:
<comment>When `result.url` exists but `window.location.assign()` throws an error, the catch block handles it correctly with a redirect-failure toast but then falls through to the unconditional toast below (`"Authorization completed, but no redirect was returned."`). This is misleading because the redirect URL was returned — it just failed to execute. The user sees two conflicting messages. Add a `return;` statement at the end of the catch block to prevent this incorrect fallthrough.</comment>
<file context>
@@ -66,8 +66,15 @@ function OAuthConsentPage() {
setCompletedAction(accept ? "allow" : "deny");
- window.location.assign(result.url);
- return;
+ try {
+ window.location.assign(result.url);
+ return;
</file context>
| try { | |
| window.location.assign(result.url); | |
| return; | |
| } catch (error) { | |
| setCompletedAction(null); | |
| toast.error( | |
| getErrorMessage(error, "Unable to redirect after authorization. Please try again."), | |
| try { | |
| window.location.assign(result.url); | |
| return; | |
| } catch (error) { | |
| setCompletedAction(null); | |
| toast.error( | |
| getErrorMessage(error, "Unable to redirect after authorization. Please try again."), | |
| ); | |
| return; | |
| } |
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/mcp/mcp-authorization.test.ts">
<violation number="1" location="src/features/workspaces/mcp/mcp-authorization.test.ts:1">
P2: The `assertMcpConnectionAuthorized` tests don't verify that the DB query actually filters by `userId` and `clientId`. The mock `where` function accepts any arguments and always returns the same `{ limit }` chain, so a bug that drops the where clause (or one of its conditions) would still pass. Consider asserting that `where` was called with the expected `and(eq(...), eq(...))` conditions after the pass/fail tests to actually validate the authorization filtering.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -0,0 +1,92 @@ | |||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | |||
There was a problem hiding this comment.
P2: The assertMcpConnectionAuthorized tests don't verify that the DB query actually filters by userId and clientId. The mock where function accepts any arguments and always returns the same { limit } chain, so a bug that drops the where clause (or one of its conditions) would still pass. Consider asserting that where was called with the expected and(eq(...), eq(...)) conditions after the pass/fail tests to actually validate the authorization filtering.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/mcp/mcp-authorization.test.ts:
<comment>The `assertMcpConnectionAuthorized` tests don't verify that the DB query actually filters by `userId` and `clientId`. The mock `where` function accepts any arguments and always returns the same `{ limit }` chain, so a bug that drops the where clause (or one of its conditions) would still pass. Consider asserting that `where` was called with the expected `and(eq(...), eq(...))` conditions after the pass/fail tests to actually validate the authorization filtering.</comment>
<file context>
@@ -0,0 +1,92 @@
+import { beforeEach, describe, expect, it, vi } from "vitest";
+
+vi.mock("#/db/server", () => ({
+ createDbContext: vi.fn(),
+}));
+
+vi.mock("better-auth/oauth2", () => ({
+ verifyAccessToken: vi.fn(),
+}));
</file context>
1 participant
Summary
Adds a remote, read-only Model Context Protocol (MCP) server so external AI clients can authorize with ThinkEx and read workspace structure and content over Streamable HTTP at
/mcp. Implements the v1 scope from #571 as a thin protocol adapter over the existing workspace capability layer, protected by OAuth 2.1 with aworkspace:readscope.What's included
Endpoint & transport
/mcpvia Cloudflare AgentscreateMcpHandler()(noMcpAgent/session state), wired into the existing Worker routing without touching AI/kernel/auth routes.Auth & authorization
/.well-known/oauth-protected-resource/mcp(RFC 9728) plus authorization-server metadata; unauthenticated/mcprequests return401with aWWW-Authenticateheader./mcprequest is re-authorized before any tool runs, in layers:resource(<origin>/mcp) andworkspace:readscope required; opaque tokens rejected.(clientId, userId)is re-checked against live OAuth consent in D1, so a disconnected/revoked client is rejected immediately even while its token is otherwise unexpired.Tools (explicit, read-only)
thinkex_list_workspaces,thinkex_workspace_list_items,thinkex_workspace_read_items.list/readcapability operations — neverWorkspaceKernel,DocumentSession, R2, or D1 directly.failedentries; protocol errors are reserved for auth/schema/unknown-tool cases.Supporting changes
drizzle/0002_mcp_oauth.sql).recordMcpToolCall) capturing actor fields per call (not yet wired to telemetry).docs/MCP.md.Testing
pnpm check(format/lint/types) and the production build also pass.v1 limitations
Read-only (no write tools), text/Markdown projections only (no images), no MCP resources/subscriptions, and no per-call rate limiting yet. Full list in
docs/MCP.md.Closes #571
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
/mcp, scope-based MCP authentication, and MCP workspace tools (list workspaces/items and read items).truncatedindicators and safer UTF-16/surrogate handling.