fix(review): J1 — real bugs from post-refactor review sweep (SSRF, tools:false 500, shutdown NameError, +2)#168
Merged
Merged
Conversation
Five concrete findings from the code-review + security-review pass after the
route-extraction series. Each verified against source + pinned by a new test
(tests/test_review_fixes_j1.py, 17 tests).
1. SSRF guard on chat URL auto-fetch (CWE-918, routes/chat.py)
_enrich_messages auto-fetches URLs found in chat content — the prompt-
injection vector. Added _url_host_is_public(): resolves the host and rejects
loopback / private / link-local (incl. 169.254.169.254 metadata) / reserved
/ multicast / non-http. _fetch_url_content now validates pre-fetch AND
follows redirects manually (≤5 hops, re-validating each Location) so a
public URL can't 30x-redirect to an internal one. Keeps the
dashboard_host:0.0.0.0 opt-in safe.
2. UnboundLocalError on POST /api/chat {"tools": false} (routes/chat.py)
last_user_text / has_attachment were bound only inside `if use_tools:`, but
_build_chat_system_prompt(...) is called with both regardless → opaque 500.
Hoisted both before the gate.
3. _enrich_messages repo_dir was one dirname too shallow (routes/chat.py)
After the H1 move into routes/, os.path.dirname(abspath(__file__)) resolves
to routes/, not the repo root where codec_search.py lives. Now climbs two
levels (matches the web_search.py extraction). Was masked only because the
dashboard already has repo root on sys.path.
4. _shutdown_services NameError (codec_dashboard.py)
The handler declared `global _qchat_conn, _vibe_conn` and read them, but
those singletons moved to routes/qchat.py + routes/vibe.py in D1/D2 — they
were never module-level names in codec_dashboard anymore, so shutdown raised
NameError before closing anything. Now closes them in their real modules.
5. /api/run_code ran unsupported languages as python (routes/vibe_exec.py)
ext_map listed java/cpp/sql but cmd_map didn't, so cmd_map.get(lang, [python3.13])
silently fed them to python3.13. Now returns 400 "Unsupported language: X".
Also dropped the dead `body.get("filename", ...)` bare expression.
Bonus parity: the non-stream post-LLM path now strips a non-allowlisted
[SKILL:...] tag from the answer (the streaming path already did) — cosmetic,
the execution-gating invariant was already intact on both paths.
Full suite: 2,072 passed / 77 skipped (+17 new J1 tests). ruff clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
Summary
After the route-extraction series wrapped, I ran a read-only review sweep (code-review + security-review + dead-code agents) over the extracted modules. This PR fixes the 5 real bugs they surfaced. Each is verified against source and pinned by a new test file (
tests/test_review_fixes_j1.py, 17 tests).Findings fixed
routes/chat.pyroutes/chat.pyUnboundLocalError→ opaque 500 onPOST /api/chat {"tools": false}routes/chat.py_enrich_messagesrepo_dironedirnametoo shallow after the H1 movecodec_dashboard.py_shutdown_servicesNameErroron the moved_qchat_conn/_vibe_connroutes/vibe_exec.py/api/run_coderanjava/cpp/sqlas python instead of rejectingDetail
1. SSRF guard.
_enrich_messagesauto-fetches any URL found in chat content — exactly the prompt-injection path. Added_url_host_is_public()(resolves host, rejects loopback / private / link-local incl.169.254.169.254metadata / reserved / multicast / non-http) and made_fetch_url_contentvalidate pre-fetch + follow redirects manually (≤5 hops, re-validating eachLocation) so a public URL can't 302→internal. CODEC is loopback-only by default; this keeps thedashboard_host: 0.0.0.0opt-in safe. (Residual DNS-rebinding TOCTOU accepted for a local app, noted in-code.)2. tools:false 500.
last_user_text/has_attachmentwere bound only insideif use_tools:, but_build_chat_system_prompt(...)is called with both regardless. Hoisted before the gate.3. repo_dir. After H1 moved this code into
routes/,dirname(abspath(__file__))points atroutes/, not the repo root wherecodec_search.pylives. Now climbs two levels (matchesweb_search.py). Was masked because the dashboard already has repo root onsys.path— but the line's behavior silently changed, violating the verbatim-move goal.4. shutdown NameError. The handler did
global _qchat_conn, _vibe_connthen read them — but those singletons moved toroutes/qchat.py+routes/vibe.pyin D1/D2, so they were never module names in codec_dashboard →NameErroron every shutdown before any close ran. Now closes them in their real modules.5. unsupported language.
ext_maplistedjava/cpp/sqlbutcmd_mapdidn't, socmd_map.get(lang, [python3.13])silently ran them as python. Now returns400 Unsupported language: X. Also removed a deadbody.get("filename", ...)bare expression.Bonus parity: the non-stream post-LLM path now strips a non-allowlisted
[SKILL:...]tag from the answer (the streaming path already did). Cosmetic — the execution-gating invariant was intact on both paths already.Test plan
tests/test_review_fixes_j1.py— 17 tests: SSRF block-list (9 params) + public-allow + fetch-returns-empty, hoisted-bindings, two-level repo_dir, shutdown-no-NameError, unsupported-language-400, python-still-acceptedpython3.13 -m pytest --ignore=tests/test_skills.py -q→ 2,072 passed, 77 skippedruff check: 0 issues127.0.0.1/localhost/169.254.169.254/192.168.*/10.*/::1/ftp:/file:/0.0.0.0all blocked;1.1.1.1allowed_shutdown_services()runs clean🤖 Generated with Claude Code