fix(security): hosted multi-tenancy hardening — session REST owner-scoping + TOCTOU path revalidation (#704)

[frankbria]

Summary

Hosted multi-tenancy hardening for issue #704 (follow-up to #655). Two independent security fixes.

1. Owner-scope session REST endpoints

POST /api/v2/sessions already persisted user_id and the terminal/chat WebSockets enforced ownership, but the other five REST endpoints queried without an owner filter — in an authenticated multi-user deployment one tenant could enumerate/read/modify/end another tenant's sessions.

• require_auth threaded through list, get, delete, POST/GET messages.
• repo.list() scoped by user_id; per-id handlers gated via a new _get_owned_session helper.
• Returns 404 (not 403) on missing-or-not-yours so session IDs can't be probed.
• No-auth mode (user_id=None, CODEFRAME_AUTH_REQUIRED=false) skips scoping — behavior unchanged.

2. TOCTOU symlink revalidation at WS connect

The stored workspace_path cleared the allowlist at create time, but terminal_ws/session_chat_ws later used the raw stored string as the shell cwd / file-tool scope. A tenant could swap the dir (or an ancestor) for a symlink pointing outside its allowed root in between (TOCTOU).

• New revalidate_workspace_path() re-runs enforce_workspace_allowlist (which .resolve()s symlinks) at WS connect; the socket is closed on rejection, and the freshly-resolved path is used downstream.

Test plan

• tests/ui/test_session_owner_scoping.py — cross-tenant 404 for each endpoint, same-owner 200, no-auth passthrough.
• tests/ui/test_terminal_ws.py::TestTerminalWsRevalidation + tests/api/test_session_chat_ws.py::test_chat_ws_symlink_escape_rejected — symlink-swap → WS closes; legit path connects with the resolved cwd.
• Full related suite green (sessions, terminal/chat WS, [P7.0.1] Workspace path allowlist — prevent authenticated cross-tenant RCE (M1) #655 allowlist, v2 auth enforcement); ruff clean.

Known limitations

A sub-millisecond race remains between revalidation and the shell spawn. True TOCTOU-proof filesystem isolation needs a per-tenant container/chroot or openat2(RESOLVE_NO_SYMLINKS) — infra-level, and gates a hosted launch that isn't deployed yet. Revalidation closes the practical window; container isolation is recommended for hosted ops.

Closes #704

[coderabbitai]

Warning

Review limit reached

@frankbria, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 22 minutes and 44 seconds. Learn how PR review limits work.

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 001f7143-9649-42e2-9100-2249c54ece9b

📥 Commits

Reviewing files that changed from the base of the PR and between 4ac42b6 and 7c16fb4.

📒 Files selected for processing (8)

• codeframe/platform_store/repositories/interactive_sessions.py
• codeframe/ui/dependencies.py
• codeframe/ui/routers/interactive_sessions_v2.py
• codeframe/ui/routers/session_chat_ws.py
• codeframe/ui/routers/terminal_ws.py
• tests/api/test_session_chat_ws.py
• tests/ui/test_session_owner_scoping.py
• tests/ui/test_terminal_ws.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/704-session-owner-scoping-toctou

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review for fix/704 session owner-scoping and TOCTOU path revalidation. This is a well-scoped, surgical security fix. Both changes are independent and cleanly implemented. The test coverage is strong. Issues: (1) Typo in docstring ponytail: label in codeframe/ui/dependencies.py revalidate_workspace_path should be Note:. (2) Terminal WS test uses bare pytest.raises(Exception) missing code assertion for WebSocketDisconnect code 4008. (3) Inline imports inside function bodies style nit. (4) end_session race is acknowledged and fine. Summary: two things to address before merge - the ponytail: docstring typo and the weak pytest.raises(Exception) in the terminal symlink test.

[claude]

Full review (see below). Issues to fix before merge: (1) ponytail: docstring typo in revalidate_workspace_path should be Note:. (2) test_symlink_escape_rejected uses bare pytest.raises(Exception) instead of pytest.raises(WebSocketDisconnect) with code assertion == 4008.

[claude]

test comment - please ignore (will be replaced)

[claude]

Code Review — fix/704 session owner-scoping + TOCTOU path revalidation

This is a well-scoped, surgical security fix. Both changes (REST owner-scoping and WS TOCTOU revalidation) are independent and cleanly implemented. The test coverage is strong.

What's good

• 404-not-403 on missing-or-not-yours is the right call; probing session IDs is blocked without leaking ownership information.
• _get_owned_session helper centralises the ownership check correctly across all five endpoints — no copy-paste drift risk.
• repo.list(user_id=None) skipping the filter in no-auth mode cleanly preserves existing behaviour without a code path branch higher up.
• asyncio.to_thread for revalidation keeps the blocking Path.resolve()/filesystem call off the event loop — correct.
• Owner-scoping test suite covers all five endpoints (list, get, delete, POST message, GET messages) plus the no-auth passthrough. Solid.

Issues

1. Typo in docstring — ponytail: label

codeframe/ui/dependencies.py, revalidate_workspace_path docstring has ponytail: closes the practical window… — this is clearly a leftover artefact that should be Note: (or a plain sentence). The acknowledged limitation is important; the label makes it easy to miss.

2. Terminal WS test uses bare pytest.raises(Exception) — missing code assertion

tests/ui/test_terminal_ws.py::TestTerminalWsRevalidation::test_symlink_escape_rejected catches any exception rather than WebSocketDisconnect with code 4008. The matching chat-WS test correctly asserts the specific close code (1008). A future regression that closes with a different error (e.g. a 500 from an unrelated crash) would still pass this test.

Suggested fix:

with pytest.raises(WebSocketDisconnect) as exc:
    with client.websocket_connect("/ws/sessions/s1/terminal?token=x"):
        pass
assert exc.value.code == 4008

3. Inline imports inside function bodies (style nit)

Both WS handlers do from codeframe.ui.dependencies import revalidate_workspace_path inside the function body rather than at module level. Not a bug, but inconsistent with the rest of each file and obscures the dependency.

4. end_session ownership-check → repo.end() race (acknowledged, fine)

The concurrent-delete case is already handled by if updated is None. Since session ownership cannot change (no transfer API exists), the only race is another delete arriving simultaneously — benign and correctly guarded.

Summary

Two things worth addressing before merge: the ponytail: docstring typo and the weak pytest.raises(Exception) in the terminal symlink test. The inline-import nit is low priority. Everything else is correct and well-tested.

[claude]

Code Review — fix/704 follow-up pass

Prior issues status

Both issues raised in my earlier review have been addressed in the current diff:

• Docstring label — revalidate_workspace_path now correctly reads Note: (not ponytail:). ✅
• Terminal WS test assertion — test_symlink_escape_rejected now uses pytest.raises(WebSocketDisconnect) with assert exc.value.code == 4008. ✅

One remaining nit

tests/api/test_session_chat_ws.py::test_chat_ws_symlink_escape_rejected imports patch and TestClient inline even though both are already imported at module level (lines 15–16). This duplicates the pre-existing pattern in test_chat_ws_ownership_mismatch_closes — not introduced by this PR — but worth cleaning up rather than cementing.

Suggested: remove the inline from unittest.mock import AsyncMock, MagicMock, patch and from fastapi.testclient import TestClient from the function body; add AsyncMock, MagicMock to the module-level from unittest.mock import patch line and FastAPI to a top-level import. WebSocketDisconnect is already imported.

Summary

The security logic is sound, owner-scoping is complete across all five REST endpoints, TOCTOU revalidation is correctly threaded through both WS handlers via asyncio.to_thread, and the test coverage is thorough. The inline-import nit is genuinely minor. Ready to merge after that cleanup.