Repository navigation
fix(sandbox): restrict terminal and prod code execution to admins - #1664
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughRestricts terminal feature access to admin users across three layers: the shared ChangesAdmin-only Terminal Access Controls
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/web/src/app/api/pages/`[pageId]/terminal/execute/route.ts:
- Around line 57-60: Replace the inline authorization check `auth.role !==
'admin'` with a call to the centralized permission function from the shared
permissions layer. Instead of directly checking the auth role, use
`getUserAccessLevel()` or `canUserEditPage()` functions imported from
`@pagespace/lib/permissions/permissions` to determine if the user has the
required permissions to access the terminal session. This ensures consistency
with the centralized authorization policy and maintains a single source of truth
for permission evaluation across the application.
In `@apps/web/src/app/api/pages/route.ts`:
- Around line 53-56: Replace the inline authorization check in the TERMINAL page
type validation (the condition checking `auth.role !== 'admin'`) with the
centralized permission helper functions from
`@pagespace/lib/permissions/permissions`. Instead of directly comparing auth.role,
use either getUserAccessLevel() or canUserEditPage() to determine if the current
user has permission to create a TERMINAL page type. Keep the existing audit
request call and error response unchanged, but update the condition logic to
delegate authorization checks to the centralized permission module.
🪄 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: f8d41c74-ccfb-43b5-97c5-f40354b5699c
📒 Files selected for processing (10)
apps/web/src/app/api/pages/[pageId]/terminal/execute/__tests__/route.test.tsapps/web/src/app/api/pages/[pageId]/terminal/execute/route.tsapps/web/src/app/api/pages/__tests__/route.test.tsapps/web/src/app/api/pages/route.tsapps/web/src/components/create/QuickCreatePalette.tsxapps/web/src/components/layout/middle-content/page-views/terminal/TerminalView.tsxapps/web/src/services/api/page-service.tspackages/lib/src/services/sandbox/__tests__/can-run-code.test.tspackages/lib/src/services/sandbox/can-run-code.tspackages/lib/src/services/sandbox/tool-gate.ts
| if (auth.role !== 'admin') { | ||
| auditRequest(req, { eventType: 'authz.access.denied', userId, resourceType: 'terminal_session', resourceId: pageId, details: { reason: 'app_admin_required', method: 'POST' }, riskScore: 0.5 }); | ||
| return NextResponse.json({ error: 'Terminal access requires administrator privileges' }, { status: 403 }); | ||
| } |
There was a problem hiding this comment.
Replace inline role check with centralized permission evaluation (Line 57).
auth.role !== 'admin' is a custom authorization path in an API route and should use the shared permission layer for consistency and policy correctness.
As per coding guidelines, apps/web/src/app/api/**/*.{ts,tsx} must “Use centralized permission logic from @pagespace/lib/permissions/permissions via getUserAccessLevel() and canUserEditPage() functions,” and **/*.{ts,tsx} must “Always use centralized permission functions from packages/lib/src/permissions/. Never roll your own access checks.”
🤖 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 `@apps/web/src/app/api/pages/`[pageId]/terminal/execute/route.ts around lines
57 - 60, Replace the inline authorization check `auth.role !== 'admin'` with a
call to the centralized permission function from the shared permissions layer.
Instead of directly checking the auth role, use `getUserAccessLevel()` or
`canUserEditPage()` functions imported from
`@pagespace/lib/permissions/permissions` to determine if the user has the
required permissions to access the terminal session. This ensures consistency
with the centralized authorization policy and maintains a single source of truth
for permission evaluation across the application.
Source: Coding guidelines
| if (validatedData.type === PageType.TERMINAL && auth.role !== 'admin') { | ||
| auditRequest(request, { eventType: 'authz.access.denied', userId, resourceType: 'page', resourceId: validatedData.driveId, details: { reason: 'app_admin_required', type: validatedData.type, method: 'POST' }, riskScore: 0.5 }); | ||
| return NextResponse.json({ error: 'Terminal pages require administrator privileges' }, { status: 403 }); | ||
| } |
There was a problem hiding this comment.
Use centralized permission helpers for the TERMINAL create gate (Line 53).
This inline auth.role !== 'admin' check bypasses the shared authorization contract for API routes and can drift from canonical policy behavior.
As per coding guidelines, apps/web/src/app/api/**/*.{ts,tsx} must “Use centralized permission logic from @pagespace/lib/permissions/permissions via getUserAccessLevel() and canUserEditPage() functions,” and **/*.{ts,tsx} must “Always use centralized permission functions from packages/lib/src/permissions/. Never roll your own access checks.”
🤖 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 `@apps/web/src/app/api/pages/route.ts` around lines 53 - 56, Replace the inline
authorization check in the TERMINAL page type validation (the condition checking
`auth.role !== 'admin'`) with the centralized permission helper functions from
`@pagespace/lib/permissions/permissions`. Instead of directly comparing auth.role,
use either getUserAccessLevel() or canUserEditPage() to determine if the current
user has permission to create a TERMINAL page type. Keep the existing audit
request call and error response unchanged, but update the condition logic to
delegate authorization checks to the centralized permission module.
Source: Coding guidelines
|
Replying to the two CodeRabbit threads on These are false positives — no change needed. The recommended helpers ( This gate is platform app-admin (
Using the drive-permission helpers here would conflate "drive admin" with "platform admin" — a drive ADMIN is not a platform app-admin, so that would be a security regression, widening access rather than restricting it. The centralized permission layer IS still used for drive access: the terminal execute route additionally calls |
1 participant
Summary
TERMINALpage creation in API validation while enforcing app-admin-only authorizationcanRunCodegate in productionTesting
Summary by CodeRabbit
Release Notes