fix(terminal): drop dependencies=[] on WS include — silently 403s upgrades#28
Closed
Leolebleis wants to merge 2 commits intomainfrom
Closed
fix(terminal): drop dependencies=[] on WS include — silently 403s upgrades#28Leolebleis wants to merge 2 commits intomainfrom
Leolebleis wants to merge 2 commits intomainfrom
Conversation
…rades Root cause traced via systematic debugging: FastAPI's include_router dependency-list propagation silently rejects WebSocket upgrades with HTTP 403 even when the deps don't raise. PR #27 removed require_api_key from the dependencies list (after PR #26 fixed APIKeyHeader for WS), but the remaining _require_terminal_enabled dep still triggered the rejection. Fix: drop the entire dependencies=[...] arg on the WS router include and move both gates (api-key + chat/MCP enabled) inline in the WS handler. The HTTP routers continue to use dependencies=[Depends(require_api_key)] unchanged — only the WS route is special. Side effects: - _require_terminal_enabled removed from main.py (unused) - _chat_enabled module flag removed (only the gate read it) - terminal/router.py owns its own gate via os.getenv at request time
tmux refuses to attach when TERM is unset or "dumb", emitting "open terminal failed: terminal does not support clear" — the actual output the user sees in the browser. uvicorn's process env has no TERM, so the PTY child inherited none. Set TERM=xterm-256color (what xterm.js emulates, ships in ncurses-base which python:3.13-slim has) in os.environ before exec. Self-tested end-to-end through the WS — 726 bytes of real tmux output flowed through, no error.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Root cause (via systematic debugging)
FastAPI's `app.include_router(..., dependencies=[Depends(...)])` silently rejects WebSocket upgrades with HTTP 403 — regardless of whether the dep itself raises. PR #26 (APIKeyHeader → Header) and #27 (move auth inline) only addressed the auth dep; the remaining `_require_terminal_enabled` dep on `include_router` was still triggering the silent rejection.
Verified by removing the whole `dependencies=[...]` arg: WS upgrade returns 101 immediately and the handler enters. Adding any dep back reverts to 403, even one that doesn't raise.
Fix
Drop `dependencies=[...]` entirely on the terminal WS router include. Both gates (api-key + `FELLOW_CHAT_ENABLED && FELLOW_MCP_ENABLED`) are now inline in the WS handler, evaluated at request time. HTTP routers are unchanged.
Verified