fix(agent-updates): fail-closed on GitHub Releases outage (Bonus 12 #5 / PR #135) - #139
Conversation
…/ PR #135) Closes Bonus 12 finding #5 from PR #135 / bonus12-bug-finder.md. Pre-fix - _remote_release_tags swallowed every Exception and returned []. DNS poison, MITM TLS, GitHub 5xx, and rate-limit bans all collapsed silently into status="already_current" downstream, masking a stale Hermes Agent checkout (CWE-918 SSRF / OWASP CICD-SEC-1 fail-open). - _latest_release had the same broad-catch shape on a parallel path. Post-fix - _remote_release_tags tightens except to the known network/parse modes: urllib.error.URLError (covers HTTPError 4xx/5xx, ContentTooShortError), socket.timeout, TimeoutError, ConnectionError, json.JSONDecodeError. Real failures raise HTTPException(502) with a _redact()-cleaned detail. - non-list payload (200 + {object}) raises 502 (upstream contract violation). - _latest_release distinguishes 404 (no /releases/latest yet -> soft warning) from real outages (5xx -> 502); URLError on latest endpoint preserves the soft-warning fast path because _remote_release_tags is the authoritative gate (if we got that far, tags came back successfully). Behavior change - update_status / staged_update will now flip from "outdated=False"/"already_current" to HTTP 502 during GitHub blips. This is the correct fail-closed posture for a supply-chain update channel (CWE-918 SSRF integrity face). Reviewers should confirm UI tolerates 502. Tests added (12, all green) - 04_testing/pytest/unit/test_agent_updates_ssrf.py * happy paths (success + genuinely empty release list) * socket.timeout, URLError, HTTP 5xx, malformed JSON, non-list payload all raise HTTPException(502) * Bearer-shaped strings in error detail are redacted * latest_release: 404 keeps soft warning; 5xx raises 502; URLError on latest keeps soft warning (downstream gate already authoritative) Verification - py_compile: OK - Focused tests: 12/12 pass - All agent_updates-keyed unit tests: 37/37 pass - Pre-push hook: passed Scope - Bonus 12 finding #5 only - controlled-batch pattern continues. - v0.13.0 update remains formally deferred. - RC v2 commits 2-5 remain paused per user instruction. Swarm provenance - Agent 6 (general-purpose) of the 20-agent Blocker Elimination Swarm produced the unified diff this PR applies. Verified locally before apply; tests added by orchestrator. Follow-ups (separate PRs) - Bonus 12 #6 (config payload redaction asymmetry) - Agent 7 diff ready - Bonus 12 #7 (apply_patch_proposal TOCTOU) - Agent 8 diff ready - Bonus 12 #8 (_call_mcp_tool deadlock pattern) - Agent 9 diff ready - Bonus 13 audit doc errata - Agent 10 diff ready - GITHUB_TOKEN support to escape the 60-req/hr unauth rate limit (out-of-scope; new dominant cause of 502s post-fix) References - https://docs.python.org/3/library/urllib.error.html - https://cwe.mitre.org/data/definitions/918.html (SSRF) - https://owasp.org/www-project-top-10-ci-cd-security-risks/ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
e0b719d
into
feat/hermes3d-7-complete-gui-repo-wiring
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the security and robustness of the Hermes Agent update mechanism by replacing generic exception handling with specific checks for network and parsing errors when querying the GitHub Releases API. It implements a "fail-closed" approach for the primary release tags endpoint, raising a 502 Bad Gateway on failure to prevent the system from incorrectly assuming it is up-to-date during an outage. Review feedback suggests extending this "fail-closed" posture to the latest release endpoint for consistency and explicitly catching UnicodeDecodeError in both API interaction functions to handle malformed responses gracefully and avoid unhandled internal server errors.
| except (urllib.error.URLError, socket.timeout, TimeoutError, ConnectionError, json.JSONDecodeError) as exc: | ||
| # Soft-warning preserved here ONLY because _remote_release_tags is the | ||
| # authoritative gate; if we got this far, tags came back successfully, | ||
| # so a transient hiccup on the latest endpoint is acceptable. | ||
| latest["api_warning"] = _redact(str(exc)) |
There was a problem hiding this comment.
To maintain the "fail-closed" posture described in the PR summary, network outages (timeouts, DNS failures, etc.) on the /latest endpoint should raise an HTTPException(502) rather than falling back to a soft warning. Currently, if this endpoint fails with a URLError, latest["tag"] remains None, which causes downstream logic in update_status to report outdated=False. This effectively fails open by masking potential updates during an outage. Additionally, UnicodeDecodeError should be caught here as well to handle malformed responses gracefully. Note that this change will require updating the corresponding unit test test_latest_release_url_error_keeps_soft_warning.
except (urllib.error.URLError, socket.timeout, TimeoutError, ConnectionError, json.JSONDecodeError, UnicodeDecodeError) as exc:\n raise HTTPException(\n status_code=502,\n detail=f"GitHub Releases latest endpoint unreachable: {_redact(str(exc))[:120]}",\n ) from exc| payload = json.loads(response.read().decode("utf-8")) | ||
| except Exception: | ||
| return [] | ||
| except (urllib.error.URLError, socket.timeout, TimeoutError, ConnectionError, json.JSONDecodeError) as exc: |
There was a problem hiding this comment.
The decode("utf-8") call on line 244 can raise a UnicodeDecodeError if the upstream response contains invalid byte sequences. This exception is currently not caught by the specific tuple, which would result in an unhandled 500 Internal Server Error instead of the intended 502 Bad Gateway. Adding UnicodeDecodeError to the caught exceptions ensures consistent error handling for malformed upstream responses.
| except (urllib.error.URLError, socket.timeout, TimeoutError, ConnectionError, json.JSONDecodeError) as exc: | |
| except (urllib.error.URLError, socket.timeout, TimeoutError, ConnectionError, json.JSONDecodeError, UnicodeDecodeError) as exc: |
1 participant
Summary
Closes Bonus 12 finding #5 from PR #135 /
bonus12-bug-finder.md._remote_release_tagsand_latest_releasepreviously swallowed everyExceptionand returned[]— DNS poison, MITM TLS, GitHub 5xx, and rate-limit bans all collapsed silently intostatus=\"already_current\"downstream, masking a stale Hermes Agent checkout (CWE-918 SSRF / OWASP CICD-SEC-1 fail-open).Patch detail
_remote_release_tags: tightens except to known modes —urllib.error.URLError(coversHTTPError4xx/5xx),socket.timeout,TimeoutError,ConnectionError,json.JSONDecodeError. Real failures raiseHTTPException(502)with a_redact()-cleaned detail. Non-list payload (200 + object) also raises 502 (upstream contract violation)._latest_release: distinguishes 404 ("no /releases/latest yet" → soft warning) from real outages (5xx → 502). URLError on the latest endpoint preserves the soft-warning fast path because_remote_release_tagsis the authoritative gate.Behavior change
update_status/staged_updatewill now flip fromoutdated=False/already_currentto HTTP 502 during GitHub blips. This is the correct fail-closed posture for a supply-chain update channel (CWE-918 integrity face). Reviewers should confirm UI tolerates 502.Tests added (12, all green)
04_testing/pytest/unit/test_agent_updates_ssrf.py:socket.timeout,URLError, HTTP 5xx, malformed JSON, non-list payload all raiseHTTPException(502)_latest_release: 404 keeps soft warning; 5xx raises 502; URLError keeps soft warning (downstream gate authoritative)Test plan
python -m py_compileon touched filesagent_updates-keyed unit tests: 37/37 passScope
Swarm provenance
Agent 6 of the 20-agent Blocker Elimination Swarm produced the unified diff this PR applies. Verified locally before apply; tests added by orchestrator.
Follow-up PRs
apply_patch_proposalTOCTOU) — Agent 8 diff ready_call_mcp_tooldeadlock pattern) — Agent 9 diff readyReferences
🤖 Generated with Claude Code