fix: hashlib FIPS crash + assert→runtime guards in 4 production files - #64808
fix: hashlib FIPS crash + assert→runtime guards in 4 production files#64808AlexFucuson9 wants to merge 1 commit into
Conversation
- agent/context_compressor.py: add usedforsecurity=False to hashlib.md5 used for content dedup (not cryptographic security). Crashes on FIPS-enabled systems (RHEL 9, Ubuntu 22.04 FIPS, AWS AL2023 FIPS). - agent/codex_responses_adapter.py: add usedforsecurity=False to hashlib.sha1 used for function-call ID derivation. Same FIPS crash. - agent/transports/codex_app_server_session.py: replace bare assert with RuntimeError guard for uninitialised codex session. Asserts are stripped under python -O, leaving a silent None dereference. - tui_gateway/server.py: replace bare assert with structured JSON-RPC error return when session lookup yields None. Assert would crash the gateway under -O; the error response is consistent with the existing _sess_nowait error path.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the two remaining non-security hash callsites; both are still present on current main at agent/codex_responses_adapter.py:236 and agent/context_compressor.py:1578.
Problems
- The new
session.activatefallback is unreachable._sess_nowait()returns(None, error)for a missing session attui_gateway/server.py:1486-1488, andsession.activatereturns that error attui_gateway/server.py:6160-6162before the replacement guard. Remove the redundant assertion rather than adding a second error contract. CodexAppServerSession.compact_thread()retains the same post-ensure_started()assertion atagent/transports/codex_app_server_session.py:661that this PR replaces inrun_turn().- The PR adds no coverage in the existing target suites, including
tests/agent/test_context_compressor.py,tests/agent/test_codex_responses_adapter.py, andtests/agent/transports/test_codex_app_server_session.py.
Suggested changes
- Preserve
_sess_nowait()'s existing missing-session response and remove the dead fallback. - Cover
compact_thread()consistently if the assertion-hardening scope remains. - Add focused FIPS-keyword regression tests.
Automated hermes-sweeper review.
| assert session is not None | ||
| if session is None: | ||
| return _err(rid, -32000, "session not found or unavailable") | ||
|
|
There was a problem hiding this comment.
_sess_nowait() already returns an error whenever session is None, and the preceding if err: return err exits first. This fallback is unreachable; remove the redundant assertion instead of introducing a second missing-session error contract.
| result.should_retire = True | ||
| return result | ||
| assert self._client is not None and self._thread_id is not None | ||
| if self._client is None or self._thread_id is None: |
There was a problem hiding this comment.
compact_thread() repeats this exact post-ensure_started() assertion at current main agent/transports/codex_app_server_session.py:661. Please apply the same handling there, or centralize the shared invariant, if this PR is intended to harden these production paths under python -O.
SummaryTwenty-one PRs address or reference this cluster: most annotate non-security MD5/SHA-1 constructors for FIPS compatibility, three change WeCom signature comparison, and two broad SHA-256 sweeps also alter security-sensitive contexts. The diffs range from focused current-path substitutions to overlapping repository-wide batches, while #64808 combines two relevant hash annotations with unrelated and incomplete assertion handling. Related pull requests
Duplicates#52967 is the closed v2 duplicate/superset of #52783, and #73278 is the closed identical duplicate of #72370; #39049 and #43937 overlap #54617 but target the retired path. #48472 is the Skills Hub subset of #62654, #56715 is a focused subset of #52783, and #66857 duplicates the Weixin slice present in #56719 and #65434. Suggested consolidationKeep #64808 open with a salvage path: retain its two hash annotations, remove the unreachable TUI fallback, cover the sibling compact_thread assertion consistently, and add focused constructor and session tests. After carrying #43937's spy and None-handling coverage into live-path #54617, close #39049 and #43937 as superseded duplicates despite their keep_open reviews because their diffs edit the retired gateway path; keep the remaining focused current-path PRs open only for their documented tests or unique sites, and close overlapping subsets once those sites are explicitly carried into a tested consolidation. Cross-PR triage: Reviewed 21 pull requests and 0 issues in this complex. Diffs were read for 20 of 21 PRs (rest unavailable); Assessment working set: 112 kB of PR diffs, 28 kB of issue/PR text, 22 kB of discussion (31 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
…/qqbot/webserver FIPS compliance: hashlib.md5() and hashlib.sha1() without usedforsecurity=False crash on FIPS-enabled systems. These sites were missed by previous batches (NousResearch#56736, NousResearch#64808, NousResearch#77777).
…or FIPS compliance Several hashlib.md5() and hashlib.sha1() calls across the codebase lack usedforsecurity=False, causing ValueError crashes on FIPS-enabled systems (OpenSSL FIPS mode raises EVP_DigestInit_ex for security-tagged hashes). Previous PRs (NousResearch#56736, NousResearch#64808, NousResearch#73278, NousResearch#73800) fixed some sites but missed these files: - agent/context_compressor.py:2837 — md5 for content dedup hashing - agent/codex_responses_adapter.py:333 — sha1 for function call ID seed - plugins/platforms/wecom/adapter.py:1247 — md5 for media chunk upload - plugins/platforms/wecom/wecom_crypto.py:63 — sha1 for WeChat signature - plugins/platforms/sms/adapter.py:281 — sha1 passed to hmac.new() - tools/skills_sync.py:256 — md5 for directory change detection - tools/skills_hub.py:1375,1660,1887,2385,2511 — md5 for cache keys None of these are security-sensitive (content hashing, cache keys, message signatures). usedforsecurity=False is the correct annotation. For the hmac.new() call in sms/adapter.py, a lambda wrapper is used since hmac.new() accepts the digest constructor, not a call result.
Summary
Fixes two categories of bugs found during codebase scan:
1. hashlib FIPS crash (P1)
Two files call
hashlib.md5()/hashlib.sha1()withoutusedforsecurity=False. On FIPS-enabled systems (RHEL 9, Ubuntu 22.04 FIPS, AWS AL2023 FIPS), these calls raiseValueError: [digital envelope routines: EVP_DigestInit_ex] disabled for FIPSand crash the running operation.agent/context_compressor.pyagent/codex_responses_adapter.pyBoth are content-hashing use cases, not cryptographic security —
usedforsecurity=Falseis the correct annotation.Related: Same pattern was previously fixed in #62654 (skills_hub, skills_sync, web_server) and #64062 (qqbot, wecom, yuanbao_media). These two files were missed.
2. assert in production code (P2)
Bare
assertstatements in production paths are stripped underpython -O, leaving silentNonedereferences or swallowed errors.agent/transports/codex_app_server_session.pyassert→raise RuntimeError(...)tui_gateway/server.pyassert→return _err(rid, -32000, ...)Related: Same pattern was previously fixed in #62659 (7 files, 15 sites). These two were missed.
Testing
Risk
Minimal — each change is a single-line substitution with identical runtime behaviour under normal Python. The
usedforsecurity=Falseflag is a no-op on non-FIPS systems. The runtime guards produce the same error that the asserts would, but survive-Omode.