[codex] fix terminal workspace ownership checks#4572
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEnforce terminal session workspace ownership during creation and WebSocket attach by reading an optional workspaceId query, rejecting cross-workspace reuse/adoption, resetting ownership/status on DB upsert, and adding end-to-end tests. UI wiring passes workspaceId through the terminal transport URL and threads a launcher for session creation. ChangesTerminal workspace ownership enforcement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR scopes terminal session create/adopt/reuse to the owning workspace, fixing a cross-workspace terminal hijack where a persisted pane layout in workspace B could attach to workspace A's terminal session. It also fixes a related bug where the "new terminal" dropdown action wrote a random UUID directly into pane state instead of going through the launcher.
Confidence Score: 4/5The cross-workspace terminal hijack fix is correct and well-tested; the one gap is that the WebSocket attach path only enforces ownership when the client sends the workspaceId param. The core ownership logic in createTerminalSessionInternal is applied unconditionally and backed by two new end-to-end tests. Two minor concerns exist: the WebSocket route workspace check is opt-in rather than mandatory, and terminalPaneLocations is captured before the async launcher call. terminal.ts (WebSocket attach path — workspace check is conditional) and TerminalSessionDropdown.tsx (stale terminalPaneLocations snapshot before await).
|
| Filename | Overview |
|---|---|
| packages/host-service/src/terminal/terminal.ts | Adds getTerminalWorkspaceMismatchError helper and enforces workspace ownership in createTerminalSessionInternal (unconditionally) and in the WebSocket attach path (conditionally, only when the client sends workspaceId). The conditional check in the WebSocket route is an intentional backward-compat gap worth noting. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx | Converts handleNewTerminal to an async flow using launcher.create(). Adds a duplicate-click guard and spinner. terminalPaneLocations is now captured before the await, so the markTerminalForBackground decision can use stale layout data if the pane state changes during the async call. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx | Single-line change: appends ?workspaceId=… to the WebSocket URL before ?themeType=…. Clean and correct. |
| packages/host-service/src/terminal/terminal.adoption.node-test.ts | Adds a second workspace fixture and two regression tests: cross-workspace live-session rejection and cross-workspace adoption-after-restart rejection. Both tests assert the correct error message and verify listTerminalSessions scoping. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx | Threads the launcher prop through UsePaneRegistryOptions and into TerminalSessionDropdown. Dependency array updated correctly. No logic changes. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx | Passes the pre-existing launcher variable into usePaneRegistry. Trivial plumbing change. |
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx:229-234
`terminalPaneLocations` is captured from the store snapshot *before* `await launcher.create()`, so if any pane is closed or moved while the async call is in flight, the `markTerminalForBackground` decision will be based on stale layout data. The original synchronous version didn't have this window. Moving the capture to after the `await` ensures the decision uses the current layout.
```suggestion
const state = context.store.getState();
try {
const nextTerminalId = await launcher.create();
const terminalPaneLocations = getTerminalPaneLocations(context);
if ((terminalPaneLocations.get(terminalId)?.length ?? 0) === 0) {
markTerminalForBackground(terminalId);
```
### Issue 2 of 2
packages/host-service/src/terminal/terminal.ts:1454-1492
**Optional workspace check leaves an attach bypass**
The ownership enforcement in `resolveSessionForAttach` is gated on `requestedWorkspaceId` being truthy, so any WebSocket connection that omits the `?workspaceId=` param (e.g., a reconnect triggered before the updated `TerminalPane` builds the URL, or a future caller that forgets the param) will attach to the live session without any cross-workspace check. `createTerminalSessionInternal` enforces the check unconditionally; the same posture would be stronger here. Consider returning an error when `requestedWorkspaceId` is absent and the session already has a known `workspaceId`, or at minimum logging a warning so the gap is visible.
Reviews (1): Last reviewed commit: "fix terminal dropdown session creation" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const state = context.store.getState(); | ||
| const terminalPaneLocations = getTerminalPaneLocations(context); | ||
| if ((terminalPaneLocations.get(terminalId)?.length ?? 0) === 0) { | ||
| markTerminalForBackground(terminalId); | ||
| try { | ||
| const nextTerminalId = await launcher.create(); | ||
| if ((terminalPaneLocations.get(terminalId)?.length ?? 0) === 0) { | ||
| markTerminalForBackground(terminalId); |
There was a problem hiding this comment.
terminalPaneLocations is captured from the store snapshot before await launcher.create(), so if any pane is closed or moved while the async call is in flight, the markTerminalForBackground decision will be based on stale layout data. The original synchronous version didn't have this window. Moving the capture to after the await ensures the decision uses the current layout.
| const state = context.store.getState(); | |
| const terminalPaneLocations = getTerminalPaneLocations(context); | |
| if ((terminalPaneLocations.get(terminalId)?.length ?? 0) === 0) { | |
| markTerminalForBackground(terminalId); | |
| try { | |
| const nextTerminalId = await launcher.create(); | |
| if ((terminalPaneLocations.get(terminalId)?.length ?? 0) === 0) { | |
| markTerminalForBackground(terminalId); | |
| const state = context.store.getState(); | |
| try { | |
| const nextTerminalId = await launcher.create(); | |
| const terminalPaneLocations = getTerminalPaneLocations(context); | |
| if ((terminalPaneLocations.get(terminalId)?.length ?? 0) === 0) { | |
| markTerminalForBackground(terminalId); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx
Line: 229-234
Comment:
`terminalPaneLocations` is captured from the store snapshot *before* `await launcher.create()`, so if any pane is closed or moved while the async call is in flight, the `markTerminalForBackground` decision will be based on stale layout data. The original synchronous version didn't have this window. Moving the capture to after the `await` ensures the decision uses the current layout.
```suggestion
const state = context.store.getState();
try {
const nextTerminalId = await launcher.create();
const terminalPaneLocations = getTerminalPaneLocations(context);
if ((terminalPaneLocations.get(terminalId)?.length ?? 0) === 0) {
markTerminalForBackground(terminalId);
```
How can I resolve this? If you propose a fix, please make it concise.| @@ -1433,6 +1482,14 @@ export function registerWorkspaceTerminalRoute({ | |||
| error: `Terminal session "${terminalId}" is missing a workspace.`, | |||
| }; | |||
| } | |||
| if (requestedWorkspaceId) { | |||
| const mismatchError = getTerminalWorkspaceMismatchError({ | |||
| terminalId, | |||
| ownerWorkspaceId: record.originWorkspaceId, | |||
| requestedWorkspaceId, | |||
| }); | |||
| if (mismatchError) return { error: mismatchError }; | |||
| } | |||
There was a problem hiding this comment.
The ownership enforcement in resolveSessionForAttach is gated on requestedWorkspaceId being truthy, so any WebSocket connection that omits the ?workspaceId= param (e.g., a reconnect triggered before the updated TerminalPane builds the URL, or a future caller that forgets the param) will attach to the live session without any cross-workspace check. createTerminalSessionInternal enforces the check unconditionally; the same posture would be stronger here. Consider returning an error when requestedWorkspaceId is absent and the session already has a known workspaceId, or at minimum logging a warning so the gap is visible.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/terminal/terminal.ts
Line: 1454-1492
Comment:
**Optional workspace check leaves an attach bypass**
The ownership enforcement in `resolveSessionForAttach` is gated on `requestedWorkspaceId` being truthy, so any WebSocket connection that omits the `?workspaceId=` param (e.g., a reconnect triggered before the updated `TerminalPane` builds the URL, or a future caller that forgets the param) will attach to the live session without any cross-workspace check. `createTerminalSessionInternal` enforces the check unconditionally; the same posture would be stronger here. Consider returning an error when `requestedWorkspaceId` is absent and the session already has a known `workspaceId`, or at minimum logging a warning so the gap is visible.
How can I resolve this? If you propose a fix, please make it concise.
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
# Conflicts: # apps/desktop/src/main/lib/auto-updater.test.ts
![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 `@packages/host-service/src/terminal/terminal.ts`:
- Line 1425: The code currently allows attaching a WebSocket when
requestedWorkspaceId is missing (requestedWorkspaceId =
c.req.query("workspaceId") || null) which skips workspace mismatch checks;
change the attach flow to reject the request immediately if requestedWorkspaceId
is null/undefined by returning an error (e.g., respond with 400/close the
socket) instead of falling back to terminalId-only attachment. Update all
occurrences where requestedWorkspaceId is read/used (same pattern around the
other attach/mismatch checks) so they early-return with a clear error when the
workspaceId query param is absent.
- Around line 1040-1048: The pre-check using db.query.terminalSessions.findFirst
and getTerminalWorkspaceMismatchError is raceable because the code later
unconditionally rewrites terminalSessions.originWorkspaceId; replace the
check-then-write pattern with an atomic claim/update: perform a single
transactional conditional upsert that only sets originWorkspaceId when it is
NULL or already equals the requesting workspaceId (or fails otherwise), e.g. by
using a database-level conditional update/upsert/INSERT ... ON CONFLICT ...
WHERE clause or a SQL transaction with a WHERE originWorkspaceId IS NULL OR
originWorkspaceId = :workspaceId; ensure the conflict handler that rewrites
originWorkspaceId is changed to enforce this same conditional constraint so
concurrent create attempts cannot steal ownership (refer to terminalSessions.id,
originWorkspaceId, workspaceId, and the existing conflict handler logic).
🪄 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: e7bd30f7-1d5d-4628-9da9-9005672aff2d
📒 Files selected for processing (1)
packages/host-service/src/terminal/terminal.ts
| const existingRecord = db.query.terminalSessions | ||
| .findFirst({ where: eq(terminalSessions.id, terminalId) }) | ||
| .sync(); | ||
| const recordMismatchError = getTerminalWorkspaceMismatchError({ | ||
| terminalId, | ||
| ownerWorkspaceId: existingRecord?.originWorkspaceId, | ||
| requestedWorkspaceId: workspaceId, | ||
| }); | ||
| if (recordMismatchError) return { error: recordMismatchError }; |
There was a problem hiding this comment.
The ownership check is still raceable across concurrent creates.
These reads happen before several awaits, and the conflict handler later unconditionally rewrites originWorkspaceId. Two concurrent creates for the same terminalId from different workspaces can both pass the precheck; the loser can then adopt/reuse the daemon session and overwrite the recorded owner, reintroducing cross-workspace takeover through a TOCTOU window. The ownership claim needs to be atomic rather than "check first, overwrite on conflict later".
Also applies to: 1174-1181
🤖 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 `@packages/host-service/src/terminal/terminal.ts` around lines 1040 - 1048, The
pre-check using db.query.terminalSessions.findFirst and
getTerminalWorkspaceMismatchError is raceable because the code later
unconditionally rewrites terminalSessions.originWorkspaceId; replace the
check-then-write pattern with an atomic claim/update: perform a single
transactional conditional upsert that only sets originWorkspaceId when it is
NULL or already equals the requesting workspaceId (or fails otherwise), e.g. by
using a database-level conditional update/upsert/INSERT ... ON CONFLICT ...
WHERE clause or a SQL transaction with a WHERE originWorkspaceId IS NULL OR
originWorkspaceId = :workspaceId; ensure the conflict handler that rewrites
originWorkspaceId is changed to enforce this same conditional constraint so
concurrent create attempts cannot steal ownership (refer to terminalSessions.id,
originWorkspaceId, workspaceId, and the existing conflict handler logic).
| "/terminal/:terminalId", | ||
| upgradeWebSocket((c) => { | ||
| const terminalId = c.req.param("terminalId") ?? ""; | ||
| const requestedWorkspaceId = c.req.query("workspaceId") || null; |
There was a problem hiding this comment.
Require workspaceId on WebSocket attach.
If the query param is omitted, both mismatch checks are skipped and the route falls back to attaching by terminalId alone. That keeps the old cross-workspace attach path alive for stale or malformed clients, which undermines the ownership boundary this PR is trying to enforce. Reject missing workspaceId here instead of silently proceeding.
Suggested tightening
- const requestedWorkspaceId = c.req.query("workspaceId") || null;
+ const requestedWorkspaceId = c.req.query("workspaceId");
…
const resolveSessionForAttach = async (): Promise<
TerminalSession | { error: string }
> => {
+ if (!requestedWorkspaceId) {
+ return { error: "Missing workspaceId" };
+ }
+
const existing = sessions.get(terminalId);
if (existing) {
- if (requestedWorkspaceId) {
- const mismatchError = getTerminalWorkspaceMismatchError({
- terminalId,
- ownerWorkspaceId: existing.workspaceId,
- requestedWorkspaceId,
- });
- if (mismatchError) return { error: mismatchError };
- }
+ const mismatchError = getTerminalWorkspaceMismatchError({
+ terminalId,
+ ownerWorkspaceId: existing.workspaceId,
+ requestedWorkspaceId,
+ });
+ if (mismatchError) return { error: mismatchError };
return existing;
}
…
- if (requestedWorkspaceId) {
- const mismatchError = getTerminalWorkspaceMismatchError({
- terminalId,
- ownerWorkspaceId: record.originWorkspaceId,
- requestedWorkspaceId,
- });
- if (mismatchError) return { error: mismatchError };
- }
+ const mismatchError = getTerminalWorkspaceMismatchError({
+ terminalId,
+ ownerWorkspaceId: record.originWorkspaceId,
+ requestedWorkspaceId,
+ });
+ if (mismatchError) return { error: mismatchError };Also applies to: 1455-1462, 1485-1492
🤖 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 `@packages/host-service/src/terminal/terminal.ts` at line 1425, The code
currently allows attaching a WebSocket when requestedWorkspaceId is missing
(requestedWorkspaceId = c.req.query("workspaceId") || null) which skips
workspace mismatch checks; change the attach flow to reject the request
immediately if requestedWorkspaceId is null/undefined by returning an error
(e.g., respond with 400/close the socket) instead of falling back to
terminalId-only attachment. Update all occurrences where requestedWorkspaceId is
read/used (same pattern around the other attach/mismatch checks) so they
early-return with a clear error when the workspaceId query param is absent.
1 participant
Summary
Root Cause
Terminal sessions were keyed globally by
terminalId. If a persisted pane layout in another workspace referenced the same id, host-service could return or adopt the existing terminal before verifying workspace ownership, making workspace A attach to workspace B's session.One concrete bad-pane writer was also found: the terminal dropdown's “new terminal” action wrote a fresh
crypto.randomUUID()directly into pane state instead of using the terminal launcher, which could persist a pane whose terminal never existed on host-service.Fixes #4434
Validation
bun run lintbun --filter @superset/host-service typecheckbun --filter @superset/desktop typecheckNotes
@xterm/headlessdoes not provide the namedTerminalexport, and Bun cannot loadbetter-sqlite3.Summary by CodeRabbit
New Features
Bug Fixes
Tests