feat(mcp): add drive scoping for MCP tokens #309
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughThis PR introduces drive-scoped MCP tokens, allowing tokens to be restricted to specific drives during creation. It adds database schema and migration for storing drive associations, authentication helpers to enforce drive-level access control, updates API routes to validate drive scopes, and extends the token creation UI for drive selection. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TokenAPI as POST /api/auth/mcp-tokens
participant DriveService
participant Database
participant Response
Client->>TokenAPI: Create token with driveIds
TokenAPI->>TokenAPI: Validate schema (name, driveIds)
alt driveIds provided
loop for each driveId
TokenAPI->>DriveService: getDriveAccess(userId, driveId)
DriveService-->>TokenAPI: access allowed?
alt no access
TokenAPI->>Response: 403 Forbidden
end
end
end
TokenAPI->>Database: begin transaction
TokenAPI->>Database: insert mcpTokens<br/>(name, userId, isScoped=true)
Database-->>TokenAPI: tokenId
alt driveIds provided
loop for each driveId
TokenAPI->>Database: insert mcpTokenDrives<br/>(tokenId, driveId)
Database-->>TokenAPI: ✓
end
end
TokenAPI->>Database: commit transaction
TokenAPI->>DriveService: get drive names for driveIds
DriveService-->>TokenAPI: driveScopes[]
TokenAPI->>Response: 200 OK with token + driveScopes
Response-->>Client: token (masked) + driveScopes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/api/auth/mcp-tokens/route.ts (1)
57-72:⚠️ Potential issue | 🟠 MajorToken and drive scope creation should be wrapped in a transaction.
If the
mcpTokenDrivesinsert (line 66) fails after the token is created (line 57), the token would exist without the intended drive scopes. This could result in a token having broader access than the user intended—a potential security concern.🔒 Proposed fix: Wrap in a transaction
- // Store ONLY the hash in the database - const [newToken] = await db.insert(mcpTokens).values({ - userId, - tokenHash, - tokenPrefix, - name, - }).returning(); - - // If drive scopes are specified, create the junction table entries - if (driveIds && driveIds.length > 0) { - await db.insert(mcpTokenDrives).values( - driveIds.map(driveId => ({ - tokenId: newToken.id, - driveId, - })) - ); - } + // Store token and drive scopes atomically + const newToken = await db.transaction(async (tx) => { + const [token] = await tx.insert(mcpTokens).values({ + userId, + tokenHash, + tokenPrefix, + name, + }).returning(); + + // If drive scopes are specified, create the junction table entries + if (driveIds && driveIds.length > 0) { + await tx.insert(mcpTokenDrives).values( + driveIds.map(driveId => ({ + tokenId: token.id, + driveId, + })) + ); + } + + return token; + });
🤖 Fix all issues with AI agents
In `@apps/web/src/app/api/auth/mcp-tokens/route.ts`:
- Around line 141-144: The driveScopes mapping in token.driveScopes can crash if
scope.drive is undefined; update the mapping in the driveScopes generation
(token.driveScopes -> driveScopes) to defensively handle missing drives by
filtering out entries where scope.drive is falsy (or using a conditional map
that skips/omits undefined drives) before accessing scope.drive.id and
scope.drive.name so the response never tries to read properties of an undefined
drive.
In `@packages/db/src/schema/auth.ts`:
- Around line 112-122: The unique constraint definition on the mcpTokenDrives
table is using index() instead of uniqueIndex(), which causes migration drift;
change the tokenDriveUnique index declaration inside the mcpTokenDrives table
callback to use
uniqueIndex('mcp_token_drives_token_drive_unique').on(table.tokenId,
table.driveId) (replace the current index(...) call) so the schema uses a unique
index for tokenId + driveId and matches the 0057_mcp_token_drives.sql migration.
🧹 Nitpick comments (7)
apps/web/src/app/api/mcp/documents/route.ts (1)
101-143: Optional: reuse the page lookup to avoid a second queryThe new scope check fetches the page’s driveId, and later the handler fetches the page again. Consider fetching the page once (including driveId) and reusing it for both scope checks and the operation flow to save a round-trip.
Possible refactor
- // Check drive scope restrictions before permission check - if (allowedDriveIds.length > 0) { - // Get the page's drive ID to check scope - const pageInfo = await db.query.pages.findFirst({ - where: eq(pages.id, pageId), - columns: { driveId: true }, - }); - - if (!pageInfo) { - return NextResponse.json({ error: 'Page not found' }, { status: 404 }); - } - - if (!allowedDriveIds.includes(pageInfo.driveId)) { + const page = await db.query.pages.findFirst({ + where: eq(pages.id, pageId), + }); + + if (!page) { + return NextResponse.json({ error: 'Page not found' }, { status: 404 }); + } + + if (allowedDriveIds.length > 0 && !allowedDriveIds.includes(page.driveId)) { loggers.api.warn('MCP document access denied - drive not in token scope', { userId, pageId, - pageDriveId: pageInfo.driveId, + pageDriveId: page.driveId, allowedDriveIds, }); return NextResponse.json( { error: 'This token does not have access to this drive' }, { status: 403 } ); } - } + } @@ - // Fetch the page - const page = await db.query.pages.findFirst({ - where: eq(pages.id, pageId), - }); - - if (!page) { - return NextResponse.json({ error: 'Page not found' }, { status: 404 }); - }apps/web/src/components/layout/middle-content/page-views/settings/mcp/MCPSettingsView.tsx (3)
89-99: Consider showing a toast or inline message if drives fail to load.The silent error handling is reasonable for backward compatibility, but users might be confused if the drive selection UI is empty without explanation. Consider adding a subtle indicator when drives can't be loaded.
💡 Optional: Add user feedback on drive load failure
const loadDrives = async () => { try { const response = await fetchWithAuth('/api/drives'); if (response.ok) { const driveList = await response.json(); setDrives(driveList); + } else { + console.error('Failed to load drives:', response.status); } } catch (error) { console.error('Error loading drives:', error); } };
116-126: Client-side driveScopes reconstruction is fragile.The code reconstructs
driveScopesfrom localdrivesstate after token creation, but the server only returnsdriveIds. If the server response includeddriveScopes(with names), this client-side mapping wouldn't be necessary. The current approach could show "Unknown" ifdrivesstate is stale.This works for now since the GET endpoint returns full
driveScopes, and the list will refresh correctly on next load.
333-351: Consider adding an empty state message when no drives are available.If
drives.length === 0(either because the user has no drives or loading failed), the UI shows nothing in the drive selection area. A brief message explaining this would improve UX.💡 Optional: Add empty state for drives
{drives.length > 0 && ( <div className="border rounded-md p-3 max-h-48 overflow-y-auto space-y-2"> {drives.map((drive) => ( <div key={drive.id} className="flex items-center space-x-2"> <Checkbox id={`drive-${drive.id}`} checked={selectedDriveIds.includes(drive.id)} onCheckedChange={() => toggleDriveSelection(drive.id)} /> <label htmlFor={`drive-${drive.id}`} className="text-sm font-medium leading-none peer-disabled:cursor-not-allowed peer-disabled:opacity-70 cursor-pointer" > {drive.name} </label> </div> ))} </div> )} + + {drives.length === 0 && ( + <p className="text-sm text-muted-foreground italic"> + No drives available. Token will have access to all future drives. + </p> + )}apps/web/src/app/api/auth/mcp-tokens/route.ts (1)
36-42: Sequential drive validation could be parallelized.The current implementation validates drives sequentially. For users with many drives, parallel validation would be more efficient. However, the current approach is acceptable since most users likely scope to a small number of drives.
💡 Optional: Parallelize drive access validation
if (driveIds && driveIds.length > 0) { - const invalidDriveIds: string[] = []; - - for (const driveId of driveIds) { - const access = await getDriveAccess(driveId, userId); - // User must be owner, admin, or member to scope a token to this drive - if (!access.isOwner && !access.isMember) { - invalidDriveIds.push(driveId); - } - } + const accessResults = await Promise.all( + driveIds.map(async (driveId) => ({ + driveId, + access: await getDriveAccess(driveId, userId), + })) + ); + + const invalidDriveIds = accessResults + .filter(({ access }) => !access.isOwner && !access.isMember) + .map(({ driveId }) => driveId); if (invalidDriveIds.length > 0) {apps/web/src/app/api/auth/__tests__/mcp-tokens.test.ts (1)
97-156: Consider adding tests for drive-scoped token creation.The POST tests don't cover:
- Creating a token with
driveIdsparameter- Validation failure (403) when user lacks access to specified drives
- Verifying
driveIdsis passed to the database insertThese test gaps could allow regressions in the drive scoping feature.
Would you like me to help generate test cases for drive-scoped token creation?
apps/web/src/lib/auth/__tests__/auth-middleware.test.ts (1)
153-180: Consider adding a test case for tokens with actual drive scopes.All MCP token mocks use
driveScopes: []. Adding a test with non-emptydriveScopeswould verify thatallowedDriveIdsis correctly populated from the token's drive scopes.
Allow MCP tokens to be optionally scoped to specific drives, reducing cross-contamination and limiting access for coding agents. Changes: - Add mcp_token_drives junction table for drive scopes - Update token creation API to accept optional driveIds array - Update token validation to return allowedDriveIds - Filter drives list and document access by token scope - Add drive selector UI in MCP settings token creation dialog - Show drive scope badges on existing tokens When a token has no drive scopes, it has access to all user drives (backward compatible). When scoped, it can only access those drives. https://claude.ai/code/session_013UGoFEMg858nTDbUVCBMqt
- Use getDriveAccess for proper zero trust validation when creating scoped tokens - users can now scope to any drive they have access to (owned OR member), not just owned drives - Use listAccessibleDrives in MCP drives API to properly include shared drives, filtered by token scope - Add @pagespace/lib/services/drive-service export to package.json - Update mcp-tokens test with drive-service mock https://claude.ai/code/session_013UGoFEMg858nTDbUVCBMqt
- Use uniqueIndex() instead of index() for tokenDriveUnique constraint - Wrap token and drive scope creation in a transaction for atomicity - Add defensive null check for scope.drive in driveScopes mapping - Update tests to mock db.transaction() instead of db.insert() https://claude.ai/code/session_013UGoFEMg858nTDbUVCBMqt
The security tests were failing because the mock for authenticateMCPRequest didn't include the new allowedDriveIds field required by the route's drive scoping logic. Also added the isMCPAuthResult mock to match the route import. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit addresses three critical security issues: 1. Register migration in Drizzle journal - 0057_mcp_token_drives.sql was not registered in _journal.json, preventing it from running during db:migrate. 2. Fail-closed security on drive deletion - Added isScoped field to mcp_tokens table. When a scoped token's drives are all deleted via FK cascade, the token now denies all access instead of granting unrestricted access (privilege escalation prevention). 3. Enforce drive scope in all MCP routes - Added checkMCPDriveScope and checkMCPPageScope helpers to auth module, and enforced scope checking in critical routes: - /api/pages/[pageId] (GET, PATCH, DELETE) - /api/drives/[driveId] (GET, PATCH, DELETE) - /api/drives/[driveId]/pages (GET) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit completes the drive scoping implementation by:
1. Adding centralized scope enforcement helpers to auth module:
- checkMCPDriveScope() - validates driveId access for scoped tokens
- checkMCPPageScope() - validates pageId by looking up its driveId
- checkMCPCreateScope() - blocks scoped tokens from creating drives
- filterDrivesByMCPScope() - filters drive lists by token scope
- getAllowedDriveIds() - extracts scope from auth result
2. Applying scope checks to all MCP-enabled routes:
- /api/drives (GET filters by scope, POST blocked for scoped tokens)
- /api/drives/[driveId] (all methods check scope)
- /api/drives/[driveId]/pages (check scope)
- /api/drives/[driveId]/agents (check scope)
- /api/drives/[driveId]/search/glob (check scope)
- /api/drives/[driveId]/search/regex (check scope)
- /api/pages (POST checks target drive scope)
- /api/pages/[pageId] (all methods check page's drive scope)
3. Fixing token API response contract:
- POST now returns driveScopes: { id, name }[] matching GET format
- Deduplicates driveIds input to prevent unique constraint violations
4. Updating test mocks for new scope helpers
https://claude.ai/code/session_013UGoFEMg858nTDbUVCBMqt
Clean up duplicate MCP scope checks that were introduced during rebase. The remote branch had boolean-returning scope helpers while this branch uses NextResponse-returning helpers. Keep only the NextResponse version which is cleaner (returns response directly on failure). https://claude.ai/code/session_013UGoFEMg858nTDbUVCBMqt
Removes redundant top-level import of 'pages' from @pagespace/db. The checkMCPPageScope function already does a dynamic import when needed. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2witstudios
force-pushed
the
claude/mcp-drive-scoping-xvI6n
branch
from
ddbdef4 to
2e068c8
Compare
- Expose isScoped flag in token list API to distinguish unusable tokens (scoped but all drives deleted) from truly unscoped tokens - Add tokenScopable query param to /api/drives to filter out page-permission-only drives that cannot be scoped to MCP tokens - Update MCP settings UI to show "No access" badge for unusable scoped tokens - Filter drive picker to only show owner/member drives Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The manually created migration 0063_mcp_token_drives.sql was conflicting with Drizzle's db:generate step in CI. When CI ran db:generate before db:migrate, it created a second migration (0064) that tried to add the isScoped column without IF NOT EXISTS, causing a duplicate column error. Solution: Regenerate the migration using Drizzle Kit properly so that CI's db:generate step will see "no schema changes" and not create a duplicate migration. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add filterDrivesByMCPScope and checkMCPCreateScope mocks to drives route test - Update listAccessibleDrives expectations to include tokenScopable parameter - Add test case for tokenScopable query parameter - Add checkMCPCreateScope mock to pages route test These changes align tests with the new MCP token drive scoping feature that adds scope filtering to API routes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/app/api/mcp/documents/route.ts`:
- Around line 101-105: The code assigns allowedDriveIds from
auth.allowedDriveIds without guarding for undefined which can cause a runtime
error when later accessing .length; update the assignment in the block that
checks isMCPAuthResult(auth) (the allowedDriveIds local variable and
isMCPAuthResult(auth) check) to coalesce or default undefined to an empty array
(e.g., use the nullish coalescing pattern so allowedDriveIds =
auth.allowedDriveIds ?? []) or otherwise ensure you convert/validate
auth.allowedDriveIds to a string[] before use.
🧹 Nitpick comments (3)
packages/db/src/schema/auth.ts (1)
113-113: Move import to top of file for consistency.The
drivesimport is placed mid-file after themcpTokenstable definition. While this works, it breaks the conventional import organization pattern used in the rest of the file (all other imports are at the top).Suggested fix
Move this import to line 4 with the other schema imports:
import { relations, sql } from 'drizzle-orm'; import { createId } from '@paralleldrive/cuid2'; import { chatMessages } from './core'; +import { drives } from './core';Then remove line 113.
apps/web/src/app/api/drives/__tests__/route.test.ts (1)
140-145: Consider adding tests for MCP scope enforcement scenarios.The new test verifies
tokenScopable=trueis passed correctly, but there are no tests exercising the actual MCP scope filtering or create blocking behavior. Consider adding tests that:
- Mock
filterDrivesByMCPScopeto return a subset of drives and verify the response is filtered- Mock
checkMCPCreateScopeto return an error and verify POST returns 403apps/web/src/components/layout/middle-content/page-views/settings/mcp/MCPSettingsView.tsx (1)
90-101: Surface non-OK drive load failures to users.
Right now, a non-OK response is silent, which can leave the scope selector empty without feedback.Suggested tweak
const loadDrives = async () => { try { // Only fetch drives that can be scoped to tokens (owned + member, not page-permission-only) const response = await fetchWithAuth('/api/drives?tokenScopable=true'); if (response.ok) { const driveList = await response.json(); setDrives(driveList); + } else { + throw new Error('Failed to load drives'); } } catch (error) { console.error('Error loading drives:', error); + toast.error('Failed to load drives'); } };
Comment on lines
+101
to
+105
| // Get allowed drive IDs from token scope (empty means no restrictions) | ||
| let allowedDriveIds: string[] = []; | ||
| if (isMCPAuthResult(auth)) { | ||
| allowedDriveIds = auth.allowedDriveIds; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against missing allowedDriveIds to avoid runtime errors.
If auth.allowedDriveIds is ever undefined, the later .length check will throw. Add a fallback to keep this safe for older or malformed MCP results.
✅ Suggested fix
- let allowedDriveIds: string[] = [];
- if (isMCPAuthResult(auth)) {
- allowedDriveIds = auth.allowedDriveIds;
- }
+ let allowedDriveIds: string[] = [];
+ if (isMCPAuthResult(auth)) {
+ allowedDriveIds = auth.allowedDriveIds ?? [];
+ }🤖 Prompt for AI Agents
In `@apps/web/src/app/api/mcp/documents/route.ts` around lines 101 - 105, The code
assigns allowedDriveIds from auth.allowedDriveIds without guarding for undefined
which can cause a runtime error when later accessing .length; update the
assignment in the block that checks isMCPAuthResult(auth) (the allowedDriveIds
local variable and isMCPAuthResult(auth) check) to coalesce or default undefined
to an empty array (e.g., use the nullish coalescing pattern so allowedDriveIds =
auth.allowedDriveIds ?? []) or otherwise ensure you convert/validate
auth.allowedDriveIds to a string[] before use.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow MCP tokens to be optionally scoped to specific drives, reducing
cross-contamination and limiting access for coding agents.
Changes:
When a token has no drive scopes, it has access to all user drives
(backward compatible). When scoped, it can only access those drives.
https://claude.ai/code/session_013UGoFEMg858nTDbUVCBMqt
Summary by CodeRabbit
Release Notes