fix: harden HTTP MCP risk tools#17
Merged
Vishisht16 merged 2 commits intoMay 17, 2026
Merged
Conversation
|
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughHardens HTTP MCP: defaults HTTP bind to localhost and adds optional Bearer-token auth, introduces read-only trajectory snapshot()/to_dict(), validates/clamps escalation query inputs, updates integrations and admin API to use snapshots, adjusts CLI/README, and adds tests for auth, validation, and snapshot read-only behavior. ChangesSecurity hardening and read-only risk snapshot
sequenceDiagram
participant HTTPClient
participant FastMCP
participant BearerTokenAuth
participant GetSessionRiskTool
participant snapshot
participant to_dict
participant HTTPResponse
HTTPClient->>FastMCP: POST /call/get_session_risk
alt HUMANE_PROXY_ADMIN_KEY is set
FastMCP->>BearerTokenAuth: validate Authorization Bearer token
alt token valid
BearerTokenAuth->>FastMCP: authenticated
else token missing or invalid
BearerTokenAuth->>HTTPResponse: 403 Forbidden
end
else no HUMANE_PROXY_ADMIN_KEY
FastMCP->>GetSessionRiskTool: proceed (no auth)
end
GetSessionRiskTool->>snapshot: snapshot(session_id)
Note over snapshot: read-only, no side effects
snapshot->>to_dict: TrajectoryResult -> dict
to_dict->>GetSessionRiskTool: dict with spike_detected, trend, scores, counts
GetSessionRiskTool->>HTTPResponse: JSON response
HTTPResponse->>HTTPClient: 200 OK + risk snapshot
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🧹 Nitpick comments (1)
tests/test_mcp_security.py (1)
19-19: ⚡ Quick winFragile test: accessing
__defaults__[0]couples to parameter order.If the
serve_httpsignature changes (e.g., a new parameter is added beforehost), the index will shift and this assertion will break or check the wrong default.Consider testing the actual behavior by calling
serve_http()without arguments in a controlled environment, or useinspect.signature()to verify the default by parameter name.♻️ More robust alternative using inspect
+import inspect + def test_http_mcp_defaults_to_localhost(): assert MCP_DEFAULT_HOST == "127.0.0.1" - assert serve_http.__defaults__[0] == "127.0.0.1" + sig = inspect.signature(serve_http) + assert sig.parameters['host'].default == "127.0.0.1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_mcp_security.py` at line 19, The test directly indexes serve_http.__defaults__[0], which is fragile because parameter order can change; update the test to assert the default for the host parameter by name using inspect.signature(serve_http).parameters['host'].default or, alternatively, exercise real behavior by calling serve_http() without arguments in a controlled environment and asserting it binds to "127.0.0.1"; locate the assertion referencing serve_http.__defaults__ and replace it with the inspect-based lookup or the no-arg invocation to make the test robust to signature reordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@humane_proxy/escalation/query.py`:
- Around line 21-27: The code currently sets normalized_category =
category.strip() if category else None but leaves whitespace-only strings as ""
instead of None; change this so that after stripping you treat empty strings as
None (e.g., set normalized_category = category.strip() if category and
category.strip() else None), then keep the existing membership check against
ALLOWED_ESCALATION_CATEGORIES using normalized_category and return
normalized_limit and normalized_category; this ensures whitespace-only inputs
become None and don't narrow queries.
In `@humane_proxy/mcp_server.py`:
- Around line 190-196: The current check only flags hosts in _PUBLIC_BIND_HOSTS
(e.g. "0.0.0.0"/"::") so concrete non-loopback IPs (like 192.168.x.x) bypass the
warning; change the condition to detect any non-loopback, non-local bind
instead: use the host variable and ipaddress.ip_address(host) (with try/except
for names) to determine if the address is a loopback/unspecified; if the host is
not loopback/unspecified and os.environ.get(MCP_TOKEN_ENV, "").strip() is empty,
emit the same logger.warning (retain the existing message and MCP_TOKEN_ENV
reference) so all non-loopback binds trigger the token warning instead of only
_PUBLIC_BIND_HOSTS.
In `@humane_proxy/risk/trajectory.py`:
- Around line 215-227: snapshot() currently hardcodes spike_detected=False and
thus discards any previously detected spike state; change it to read the spike
flag from the stored history tuples instead: use the second element of the most
recent entry in session_history (the same deque assigned to history) to set
spike_detected (falling back to False if history is empty). Update the
TrajectoryResult construction in snapshot() to use that derived spike_detected
value while keeping trend/_trend_for_scores(scores), window_scores,
category_counts(_category_counts(session_id)) and message_count logic unchanged.
---
Nitpick comments:
In `@tests/test_mcp_security.py`:
- Line 19: The test directly indexes serve_http.__defaults__[0], which is
fragile because parameter order can change; update the test to assert the
default for the host parameter by name using
inspect.signature(serve_http).parameters['host'].default or, alternatively,
exercise real behavior by calling serve_http() without arguments in a controlled
environment and asserting it binds to "127.0.0.1"; locate the assertion
referencing serve_http.__defaults__ and replace it with the inspect-based lookup
or the no-arg invocation to make the test robust to signature reordering.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a07be66-abd0-45fd-a810-546f5cdd0034
📒 Files selected for processing (11)
README.mdhumane_proxy/cli.pyhumane_proxy/escalation/query.pyhumane_proxy/integrations/autogen.pyhumane_proxy/integrations/crewai.pyhumane_proxy/integrations/llamaindex.pyhumane_proxy/mcp_server.pyhumane_proxy/risk/trajectory.pytests/test_integrations_smoke.pytests/test_mcp_security.pytests/test_trajectory.py
Owner
|
@ionfwsrijan good work refactoring
Note: You must sign the CLA provided in the conversation for your code to be merged. Also, please check issues raised by CodeRabbit as inline comments after raising a PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
humane_proxy/api/admin.py (1)
373-390:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPurge in-memory trajectory state on session delete.
DELETE /admin/sessions/{session_id}removes DB rows but does not clear trajectory state. Becauseget_session_risknow usessnapshot(session_id), deleted sessions can still expose prior risk metadata from memory. Please clear per-session trajectory/snapshot state in this handler (or via a dedicated erasure helper) alongside the SQL delete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@humane_proxy/api/admin.py` around lines 373 - 390, The delete_session_data handler removes DB rows but doesn't clear the in-memory trajectory/snapshot, so callers of get_session_risk (which uses snapshot(session_id)) can still see erased data; update delete_session_data to also purge the in-memory state after the SQL delete by invoking the snapshot/trajectory erasure API used elsewhere (e.g. call the existing snapshot-clearing helper or manager — something like clear_session_snapshot(session_id) or trajectory_store.delete(session_id) — immediately after the DB DELETE and before returning), or add a small erasure helper and call it from delete_session_data to ensure per-session trajectory/snapshot state is removed from memory.
🧹 Nitpick comments (1)
tests/test_integrations_smoke.py (1)
88-92: ⚡ Quick winAssert the AutoGen risk payload too, not only no-mutation behavior.
get_session_risk(sid)is called but its returned data isn’t verified, so wrong payload regressions can slip through.Proposed fix
- get_session_risk(sid) + result = get_session_risk(sid) + assert result["message_count"] == before_count assert len(session_history[sid]) == before_count assert len(_category_history[sid]) == before_count🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_integrations_smoke.py` around lines 88 - 92, The test currently calls get_session_risk(sid) but doesn't verify its return value; update the test to capture the result (e.g., risk = get_session_risk(sid)) and add assertions that the returned payload has the expected shape and values (check required keys/fields and their types/values relevant to this scenario) in addition to the existing immutability checks on session_history[sid] and _category_history[sid]; reference get_session_risk, sid, before_count, session_history and _category_history when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_admin_api.py`:
- Around line 126-130: The test uses a fixed sid ("sess-1") which can leak
global trajectory state and cause flaky tests; change the test to generate a
unique session id for each run (e.g., using uuid.uuid4() or pytest's
tmp_path/fixture) and replace the hardcoded sid variable used in the
analyze(...) calls so each invocation of analyze(sid, ...) uses a fresh unique
id; ensure the variable named sid and all analyze(...) calls in this test are
updated accordingly.
---
Outside diff comments:
In `@humane_proxy/api/admin.py`:
- Around line 373-390: The delete_session_data handler removes DB rows but
doesn't clear the in-memory trajectory/snapshot, so callers of get_session_risk
(which uses snapshot(session_id)) can still see erased data; update
delete_session_data to also purge the in-memory state after the SQL delete by
invoking the snapshot/trajectory erasure API used elsewhere (e.g. call the
existing snapshot-clearing helper or manager — something like
clear_session_snapshot(session_id) or trajectory_store.delete(session_id) —
immediately after the DB DELETE and before returning), or add a small erasure
helper and call it from delete_session_data to ensure per-session
trajectory/snapshot state is removed from memory.
---
Nitpick comments:
In `@tests/test_integrations_smoke.py`:
- Around line 88-92: The test currently calls get_session_risk(sid) but doesn't
verify its return value; update the test to capture the result (e.g., risk =
get_session_risk(sid)) and add assertions that the returned payload has the
expected shape and values (check required keys/fields and their types/values
relevant to this scenario) in addition to the existing immutability checks on
session_history[sid] and _category_history[sid]; reference get_session_risk,
sid, before_count, session_history and _category_history when adding these
assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6cf233b-4d8f-4b36-ac42-1f189e66d112
📒 Files selected for processing (8)
humane_proxy/api/admin.pyhumane_proxy/escalation/query.pyhumane_proxy/mcp_server.pyhumane_proxy/risk/trajectory.pytests/test_admin_api.pytests/test_integrations_smoke.pytests/test_mcp_security.pytests/test_trajectory.py
🚧 Files skipped from review as they are similar to previous changes (2)
- humane_proxy/escalation/query.py
- tests/test_trajectory.py
Author
|
@Vishisht16 I've made the changes. You may merge it now. |
Vishisht16
approved these changes
May 17, 2026
Vishisht16
added
bug
10 tasks
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes the HTTP MCP security hardening issue by defaulting HTTP MCP to localhost, reusing the existing
HUMANE_PROXY_ADMIN_KEYbearer auth pattern, makingget_session_riskread-only through a new trajectorysnapshot(), and bounding recent escalation queries.Fixes #15
Type of Change
Checklist
pytest tests/ -v)Note: focused affected tests pass with
44 passed. Fullpytest tests/ -vcould not collect locally becausefastapiis missing in this environment.