fix(auth): honor CODEFRAME_AUTH_REQUIRED in WebSocket auth (#676)

[frankbria]

Summary

Fixes #676. CODEFRAME_AUTH_REQUIRED=false (the documented local no-auth opt-out) disabled REST auth but the terminal and session-chat WebSockets still demanded a ?token= JWT and closed the connection — so the sessions UI and embedded terminal were unusable in no-auth mode. The two WS auth helpers also duplicated the JWT-validation logic, which is how they drifted from REST in the first place.

Approach (issue's preferred option 1)

Collapse both near-identical WS auth helpers into a single shared authenticate_websocket() in codeframe/auth/dependencies.py that consults auth_required() first — the same source of truth require_auth() uses for REST:

• No-auth mode → returns (True, None) without requiring a token, mirroring REST's synthetic local principal (user_id=None).
• Auth enabled → validates the ?token= JWT exactly as before (decode → subject → active DB user) and closes with the caller's existing code (4001 terminal / 1008 chat) on any failure.

The per-router _authenticate_websocket wrappers remain (now thin delegates) and just pass their close code. Downstream, the terminal ownership check is skipped when user_id is None, and the per-user terminal counter buckets no-auth terminals together.

Acceptance criteria

• WS connects without a token in no-auth mode, matching REST — terminal + chat.
• REST and WS share a single source of truth for the no-auth decision (auth_required()); duplicated JWT logic removed.
• Tests: WS connects in no-auth mode, and both sockets still reject missing/invalid/expired tokens when auth is enabled.

Testing

• tests/auth/test_websocket_auth.py (new): shared-helper unit tests — no-auth pass-through (True, None), auth-on missing/invalid token closes with the caller's code.
• tests/ui/test_terminal_ws.py: added no-auth connect test; existing auth-rejection tests pinned to CODEFRAME_AUTH_REQUIRED=true; mock contract updated to (ok, user_id).
• tests/api/test_session_chat_ws.py: added no-auth connect test; rejection tests pinned to auth-on.
• Full tests/auth/, test_v2_auth_enforcement.py, test_security_config.py green; ruff clean.

Cross-family review: codex review --base main found no introduced bugs.

Known limitations

• No-auth mode is local-dev only (unchanged scope). In no-auth mode the terminal per-user cap (3) is shared across all local terminals (single None bucket), and session ownership is not enforced — symmetric with REST, which has no identity to enforce when auth is disabled.
• Frontend WS hooks still append the token when one is present; harmless in no-auth mode (the token is ignored).

[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 1 hour, 18 minutes, and 58 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: e9bc49f7-4ed5-4208-b784-92d0218da900

📥 Commits

Reviewing files that changed from the base of the PR and between cd3a394 and 517e9e2.

📒 Files selected for processing (7)

• codeframe/auth/dependencies.py
• codeframe/ui/routers/session_chat_ws.py
• codeframe/ui/routers/terminal_ws.py
• docs/QUICKSTART.md
• tests/api/test_session_chat_ws.py
• tests/auth/test_websocket_auth.py
• tests/ui/test_terminal_ws.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/676-ws-auth-required

---

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

[claude]

---

Code Review - fix(auth): honor CODEFRAME_AUTH_REQUIRED in WebSocket auth (related to #676)

Summary: This is a clean, well-scoped fix. It addresses a real usability bug (WS connections unusable in no-auth local-dev mode), eliminates duplicated JWT logic, and establishes a single source of truth for WS auth that cannot drift from REST again. The approach is solid.

---

What Works Well

• Single source of truth is the right call. Collapsing two near-identical _authenticate_websocket helpers into one authenticate_websocket() in dependencies.py is exactly the right structural fix. Future auth changes only need to happen in one place.
• close_code as keyword-only arg. The * in the signature is a nice touch - it prevents accidental positional callers from getting the close codes backwards.
• Test discipline. Pinning existing rejection tests to CODEFRAME_AUTH_REQUIRED=true via monkeypatch.setenv instead of relying on implicit environment state is the right fix. The new no-auth connect tests exercise the actual auth_required() path (not a mock), giving real confidence.
• Mock contract migration. All the AsyncMock(return_value=1) patches in test_terminal_ws.py are updated to (True, 1) - consistent and complete.
• Ownership check relaxation is correctly scoped. The updated guard if user_id is not None and session_user_id is not None only skips enforcement when there is no identity - symmetric with REST.

---

One Implementation Concern: Lazy Imports

The authenticate_websocket() function defers these imports to call time inside the function body:

import jwt as pyjwt
from sqlalchemy import select
from codeframe.auth import manager
from codeframe.auth.manager import JWT_ALGORITHM, JWT_AUDIENCE, get_async_session_maker

The accompanying comment says: "Read manager.SECRET live: it may be refreshed from .env at server startup (after import), so binding the value at import would stale it."

That comment is correct, but it only justifies using manager.SECRET as an attribute access (which is already what the code does - reading it from the module at call time). It does not require deferring the module imports themselves. The modules are loaded once regardless of where the import statement appears; only the attribute manager.SECRET is read late.

Recommendation: move all four imports to the top of dependencies.py. The manager.SECRET access stays at call time (it already does), so the behavior is identical and the code is more idiomatic. For a module like dependencies.py that is imported on every request path, catching load-time import errors at startup rather than mid-request is preferable.

This is a minor style point, not a correctness issue.

---

Minor Notes

• _user_id in session_chat_ws.py is correctly named with a leading underscore (no ownership check in the chat path uses it). No action needed.
• The dict[Optional[int], int] annotation on _user_terminal_counts is accurate, and the comment explaining the None bucket is helpful.
• test_no_auth_mode_connects_without_token in test_session_chat_ws.py sends a ping and asserts a pong response - a stronger integration check than the terminal test (which just verifies connection). Good.

---

Verdict

Approve with the lazy-import cleanup as a suggestion (not a blocker). The fix is correct, well-tested, and follows the project's existing auth patterns. The structural improvement (single source of truth) is a clear win over the pre-existing duplicated logic.

[frankbria]

Thanks for the review. On the lazy-import suggestion — I kept the imports deferred inside authenticate_websocket() deliberately, to match the established pattern in the same file: get_current_user() already lazily imports the identical symbols (pyjwt, select, manager, JWT_ALGORITHM/JWT_AUDIENCE/get_async_session_maker). Verified there's no circular-import driver (auth.manager doesn't import auth.dependencies), so the late access is purely conventional here. Migrating both functions to top-level imports together is a reasonable follow-up, but doing it for just the new function would split the file across two conventions, so I left it consistent and scoped to #676.

Also flagging for the record: the CodeRabbit check reported pass but its review was actually rate-limited (org spending cap) and did not run — the cross-family review pass for this PR was codex review --base main, which found no introduced bugs.

Docs synced: QUICKSTART no longer claims CODEFRAME_AUTH_REQUIRED=false leaves the sessions UI partially usable (517e9e2).

[claude]

Follow-up on lazy imports (response to frankbria's note):

That's a fair point — I missed that get_current_user() already uses the same deferred-import pattern for the identical symbols. Keeping authenticate_websocket() consistent with the established convention in the file is the right call, and migrating both together as a follow-up (rather than splitting the file mid-PR) is the correct scoping decision.

No outstanding blockers. My approval stands — the fix is correct, well-tested, and the structural improvement (single source of truth) is solid.