[codex] Align INTERCOM with HiveMind-core#106
Conversation
The automated sentinels have completed their watch. 💂♂️I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthI've checked the repo's hydration (aka documentation density). 💧 Latest Version: ✅ 🔍 LintAnalysis complete! Check out the details below. 📊 ❌ ruff: issues found — see job log 🏷️ Release PreviewI've checked the 'Platform Support' matrix. 💻 Current:
🚀 Release Channel Compatibility Predicted next version:
🔒 Security (pip-audit)I've checked the security of our API endpoints. 🌐 ✅ No known vulnerabilities found (61 packages scanned). ⚖️ License CheckEnsuring no unlicensed code has snuck in. 🕵️ ✅ No license violations found (43 packages). License distribution: 12× MIT License, 6× Apache Software License, 6× MIT, 4× Apache-2.0, 3× BSD-3-Clause, 2× ISC License (ISCL), 2× PSF-2.0, 1× Apache Software License; BSD License, +7 more Full breakdown — 43 packages
Copyright (c) 2022 Phil Ewels Permission is hereby granted, free of charge, to any person obtaining a copy The above copyright notice and this permission notice shall be included in all THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 📊 CoverageI've mapped out the test coverage for you! 🗺️ ❌ 57.4% total coverage Files below 80% coverage (6 files)
Full report: download the 🔨 Build TestsThe build bots are giving this a thumbs up. 👍
❌ 3.10: Install OK, tests failed The automation never sleeps, but I might reboot. 💤 |
|
Important Review skippedDraft detected. 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/unittests/test_protocol_intercom.py (1)
27-44: Consider adding test coverage for the legacy hybrid envelope path.The test validates the new
"ciphertext"-only envelope format, but the implementation also supports the legacy"encrypted_key"format for backward compatibility. A test for the legacy path would ensure it doesn't regress.Additionally, consider adding negative tests for:
- Decryption failure (wrong key)
- Missing
ciphertextfield- Malformed base64
📝 Example test for legacy envelope
def test_handle_intercom_accepts_legacy_hybrid_envelope(self): proto = _make_protocol() proto.identity.public_key = "client-pubkey" inner = HiveMessage(HiveMessageType.BUS, Message("speak", {"utterance": "hello"}, {})) # Legacy envelope includes encrypted_key msg = HiveMessage(HiveMessageType.INTERCOM, {"ciphertext": "...", "encrypted_key": "...", "iv": "..."}, target_pubkey="client-pubkey") with patch("hivemind_bus_client.protocol.load_RSA_key", return_value=object()): with patch("hivemind_bus_client.protocol.hybrid_decrypt", return_value=inner.serialize().encode("utf-8")): with patch.object(proto, "handle_bus") as mock_handle_bus: self.assertTrue(proto.handle_intercom(msg)) mock_handle_bus.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_protocol_intercom.py` around lines 27 - 44, Add a unit test that covers the legacy hybrid envelope path: create a test named test_handle_intercom_accepts_legacy_hybrid_envelope that mirrors test_handle_intercom_accepts_core_rsa_envelope but constructs HiveMessage with "ciphertext", "encrypted_key" and "iv" fields and proto.identity.public_key set; patch hivemind_bus_client.protocol.load_RSA_key to return a key-like object and patch hivemind_bus_client.protocol.hybrid_decrypt to return inner.serialize().encode("utf-8"), then call proto.handle_intercom(msg) and assert it returns True and that proto.handle_bus was called once with a HiveMessageType.BUS payload; additionally add short negative tests that patch decrypt_RSA or hybrid_decrypt to raise/return invalid data and assert proto.handle_intercom returns False (or does not call handle_bus) for decryption failure, missing "ciphertext", and malformed base64 cases.hivemind_bus_client/protocol.py (1)
543-566: PING type not handled in_dispatch_intercom_payload.The dispatcher handles most message types but omits
PING. Looking athandle_propagate(line 391-392),PINGmessages are handled via_handle_ping. If aPINGcan be embedded in an INTERCOM payload, it would hit the "Unsupported INTERCOM payload type" warning.If PING-over-INTERCOM is a valid use case, add the handler:
+ if message.msg_type == HiveMessageType.PING: + # Wrap in PROPAGATE for _handle_ping compatibility + wrapper = HiveMessage(HiveMessageType.PROPAGATE, payload=message) + self._handle_ping(wrapper) + return TrueIf intentionally unsupported, this is fine as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_bus_client/protocol.py` around lines 543 - 566, The dispatcher _dispatch_intercom_payload currently omits handling HiveMessageType.PING and will log it as unsupported if a PING appears inside an INTERCOM payload; add a branch that checks for message.msg_type == HiveMessageType.PING and route it to the existing ping handler (call self.hm._handle_ping(message) or the same path used by handle_propagate) so PING messages are processed correctly; update the conditional ordering in _dispatch_intercom_payload to include this new case and ensure it returns True after handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hivemind_bus_client/protocol.py`:
- Around line 508-523: The RSA envelope path currently decodes the signature but
never verifies it; before decrypting call verify_RSA using the sender's public
key and the raw ciphertext (or the hybrid envelope's encrypted_key/ciphertext as
appropriate), base64-decode pload["signature"] and pass it and the ciphertext to
verify_RSA, and if verification fails log and return False; only after
successful verify_RSA proceed to decrypt with decrypt_RSA or hybrid_decrypt and
then HiveMessage.deserialize. Locate this logic around load_RSA_key,
decrypt_RSA, hybrid_decrypt, and HiveMessage.deserialize and add the signature
check there (use verify_RSA to mirror sign_RSA used by the sender).
---
Nitpick comments:
In `@hivemind_bus_client/protocol.py`:
- Around line 543-566: The dispatcher _dispatch_intercom_payload currently omits
handling HiveMessageType.PING and will log it as unsupported if a PING appears
inside an INTERCOM payload; add a branch that checks for message.msg_type ==
HiveMessageType.PING and route it to the existing ping handler (call
self.hm._handle_ping(message) or the same path used by handle_propagate) so PING
messages are processed correctly; update the conditional ordering in
_dispatch_intercom_payload to include this new case and ensure it returns True
after handling.
In `@test/unittests/test_protocol_intercom.py`:
- Around line 27-44: Add a unit test that covers the legacy hybrid envelope
path: create a test named test_handle_intercom_accepts_legacy_hybrid_envelope
that mirrors test_handle_intercom_accepts_core_rsa_envelope but constructs
HiveMessage with "ciphertext", "encrypted_key" and "iv" fields and
proto.identity.public_key set; patch hivemind_bus_client.protocol.load_RSA_key
to return a key-like object and patch
hivemind_bus_client.protocol.hybrid_decrypt to return
inner.serialize().encode("utf-8"), then call proto.handle_intercom(msg) and
assert it returns True and that proto.handle_bus was called once with a
HiveMessageType.BUS payload; additionally add short negative tests that patch
decrypt_RSA or hybrid_decrypt to raise/return invalid data and assert
proto.handle_intercom returns False (or does not call handle_bus) for decryption
failure, missing "ciphertext", and malformed base64 cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0766d767-9920-4215-952c-7a56fd1c49d5
📒 Files selected for processing (4)
hivemind_bus_client/client.pyhivemind_bus_client/protocol.pytest/unittests/test_client.pytest/unittests/test_protocol_intercom.py
| if isinstance(pload, dict) and "ciphertext" in pload: | ||
| try: | ||
| private_key = load_RSA_key(self.identity.private_key) | ||
| decrypted = hybrid_decrypt(private_key, pload).decode("utf-8") | ||
| if "encrypted_key" in pload: | ||
| # Backward-compatible with older hybrid AES+RSA envelopes. | ||
| decrypted = hybrid_decrypt(private_key, pload).decode("utf-8") | ||
| else: | ||
| ciphertext = pybase64.b64decode(pload["ciphertext"]) | ||
| decrypted = decrypt_RSA(private_key, ciphertext).decode("utf-8") | ||
| message._payload = HiveMessage.deserialize(decrypted) | ||
| except Exception as e: | ||
| except Exception: | ||
| if k: | ||
| LOG.error("failed to decrypt INTERCOM message!") | ||
| else: | ||
| LOG.debug("failed to decrypt INTERCOM, not for us") | ||
| return False |
There was a problem hiding this comment.
Missing signature verification on the new RSA envelope path.
The sender signs the ciphertext with sign_RSA, but the receiver doesn't verify the signature before trusting the message. The signature field in the payload is decoded but never verified against the sender's public key.
Without signature verification:
- An attacker could replay or modify INTERCOM messages
- The signature field serves no purpose
Consider adding signature verification using verify_RSA before decryption, or document why signature verification is intentionally deferred (e.g., if handled at a different layer).
🔒 Proposed fix to add signature verification
if "encrypted_key" in pload:
# Backward-compatible with older hybrid AES+RSA envelopes.
decrypted = hybrid_decrypt(private_key, pload).decode("utf-8")
else:
ciphertext = pybase64.b64decode(pload["ciphertext"])
+ # Verify signature if sender's public key is available
+ if "signature" in pload and message.source_peer:
+ # TODO: Retrieve sender's public key and verify
+ # signature = pybase64.b64decode(pload["signature"])
+ # verify_RSA(sender_pubkey, ciphertext, signature)
+ pass
decrypted = decrypt_RSA(private_key, ciphertext).decode("utf-8")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hivemind_bus_client/protocol.py` around lines 508 - 523, The RSA envelope
path currently decodes the signature but never verifies it; before decrypting
call verify_RSA using the sender's public key and the raw ciphertext (or the
hybrid envelope's encrypted_key/ciphertext as appropriate), base64-decode
pload["signature"] and pass it and the ciphertext to verify_RSA, and if
verification fails log and return False; only after successful verify_RSA
proceed to decrypt with decrypt_RSA or hybrid_decrypt and then
HiveMessage.deserialize. Locate this logic around load_RSA_key, decrypt_RSA,
hybrid_decrypt, and HiveMessage.deserialize and add the signature check there
(use verify_RSA to mirror sign_RSA used by the sender).
1 participant
What changed
This PR aligns websocket
INTERCOMsend and receive behavior with currentHiveMind-core.Why
HiveMind-corecurrently expects an RSAciphertext/signatureenvelope forINTERCOM, while the websocket client had drifted to a different envelope shape and also routed wrappedINTERCOMframes incorrectly inBROADCAST,PROPAGATE, andQUERYhandlers.Impact
Direct encrypted messages use the same envelope shape as
HiveMind-coreand the client now dispatches decrypted inner hive messages consistently. The receive path remains backward-compatible with the older hybrid envelope format.Validation
python -m unittest test.unittests.test_protocol_intercompython -m unittest test.unittests.test_clientSummary by CodeRabbit
Release Notes
New Features
Tests