fix: guard json.loads against non-JSON HTTP responses - #30138
fix: guard json.loads against non-JSON HTTP responses#30138annguyenNous wants to merge 1 commit into
Conversation
Multiple tools and gateway adapters parse HTTP responses with json.loads() without catching json.JSONDecodeError. When servers return non-JSON bodies (HTML error pages, 502 gateways, empty responses), these crash with unhandled JSONDecodeError. Changes: - tools/osv_check.py: wrap OSV API response parsing in try/except - tools/discord_tool.py: add JSONDecodeError to existing except clause - tools/browser_cdp_tool.py: guard WebSocket frame parsing (2 sites) - gateway/platforms/api_server.py: guard SQLite response cache read, also add None check for row[0] - gateway/platforms/weixin.py: guard _api_post and _api_get helpers All guards return empty/fallback values or raise RuntimeError with context, matching existing error-handling patterns in each file.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for covering several real malformed-response paths. Four targeted parsers remain unguarded on current main: tools/osv_check.py:165, tools/discord_tool.py:116, tools/browser_cdp_tool.py:237,271, and gateway/platforms/weixin.py:389,413.
Problems
gateway/platforms/api_server.py:459-471already has a stronger corrupt-cache implementation: it catches the decode failure, logs it, evicts the poisoned entry, and returnsNone. The PR's stale hunk should not replace it.tools/osv_check.py:46-51already logs fail-open query failures. Returning[]from the parser would keep the allow behavior but suppress that diagnostic.- No malformed-response regression tests are included. Existing Discord and Weixin tests cover valid and HTTP-error responses, not invalid successful bodies.
Suggested changes
- Salvage the four still-live guards, omit the ResponseStore hunk, preserve OSV diagnostics, and add targeted malformed-JSON tests for each path.
Automated hermes-sweeper review.
| self._conn.commit() | ||
| return json.loads(row[0]) | ||
| try: | ||
| return json.loads(row[0]) |
There was a problem hiding this comment.
Current main already handles this more robustly at gateway/platforms/api_server.py:459-471: it logs and deletes the corrupt cache row before returning None. Please omit this stale hunk during salvage so a poisoned entry is not retained.
| raw = resp.read() | ||
| try: | ||
| result = json.loads(raw) | ||
| except (json.JSONDecodeError, ValueError): |
There was a problem hiding this comment.
check_package_for_malware() currently catches this failure and logs a debug fail-open reason at tools/osv_check.py:46-51. Returning [] here preserves the allow result but loses that diagnostic; log invalid JSON here or propagate a contextual exception instead.
Problem
Multiple tools and gateway adapters parse HTTP responses with
json.loads()without catchingjson.JSONDecodeError. When servers return non-JSON bodies (HTML error pages, 502 gateways, empty responses), these crash with unhandledJSONDecodeError.Fix
Add
json.JSONDecodeErrorguards to 5 files across tools/ and gateway/:tools/osv_check.py:151[]on invalid JSONtools/discord_tool.py:90JSONDecodeErrorto except clausetools/browser_cdp_tool.py:143,177continueon malformed framesgateway/platforms/api_server.py:377gateway/platforms/weixin.py:386,406Tests
py_compilesyntax check