Skip to content

fix: guard json.loads() against non-JSON HTTP responses in security_audit - #31819

Open
annguyenNous wants to merge 1 commit into
NousResearch:mainfrom
annguyenNous:fix/json-loads-guard
Open

fix: guard json.loads() against non-JSON HTTP responses in security_audit#31819
annguyenNous wants to merge 1 commit into
NousResearch:mainfrom
annguyenNous:fix/json-loads-guard

Conversation

@annguyenNous

Copy link
Copy Markdown
Contributor

Problem

When the OSV API returns non-JSON (HTML error pages, 502/503 gateway errors), json.loads() crashes with JSONDecodeError. The callers only catch network errors (URLError, TimeoutError, ConnectionError), so the JSON parse error propagates as an unhandled exception.

Fix

Wrap json.loads() in _http_post_json() and _http_get_json() with try/except that converts JSONDecodeError/UnicodeDecodeError into a RuntimeError with context (status code + response preview).

Update callers to also catch RuntimeError:

  • _osv_query_batch() line 330
  • _fetch_one() line 405

Before vs After

Scenario Before After
OSV returns HTML 502 JSONDecodeError crash RuntimeError("OSV API returned non-JSON response (502): ...")
OSV returns empty body JSONDecodeError crash RuntimeError("OSV API returned non-JSON response (200): b''")

Tests

  • File passes py_compile validation
  • Error messages include status code and response preview for debugging

…udit

When the OSV API returns non-JSON (HTML error pages, 502/503 gateway
errors), json.loads() crashes with JSONDecodeError. The callers only
catch network errors (URLError, TimeoutError, ConnectionError), so
the JSON parse error propagates as an unhandled exception.

Fix: wrap json.loads() in _http_post_json() and _http_get_json() with
try/except that converts JSONDecodeError/UnicodeDecodeError into a
RuntimeError with context (status code + response preview). Update
callers to also catch RuntimeError.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have labels May 25, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Subset of #30138 which guards json.loads() in 5 files including security_audit/osv_check. Also overlaps with #29696 (osv-check subset) and #29019 (broader superset with atomic writes, same author).

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the OSV response-decoding failure path. The premise remains present on current main: hermes_cli/security_audit.py:291 and :297 call json.loads() without handling decode or JSON errors, and their callers at :319-322 and :394-397 do not catch those errors.

Problems

  • The changed error paths have no regression coverage. tests/hermes_cli/test_security_audit.py:1-298 contains parser, orchestration, and CLI-exit tests, but no tests for _http_post_json(), _http_get_json(), malformed JSON, or invalid UTF-8.

Suggested changes

  • Add mocked-response tests for malformed JSON and invalid UTF-8 in both helpers, then verify the distinct caller contracts: _osv_query_batch() reports the wrapped failure and _osv_fetch_details() returns its fallback Vulnerability.

The member note linking #30138, #29696, and #29019 is useful overlap context. This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants