fix(security): salvage security hardening PRs #6928 #6932 #6924 #6925 - #7094
Merged
Conversation
…ache, reject header injection dingtalk.py: The session_webhook URL from incoming DingTalk messages is POSTed to without any origin validation (line 290), enabling SSRF attacks via crafted webhook URLs (e.g. http://169.254.169.254/ to reach cloud metadata). Add a regex check that only accepts the official DingTalk API origin (https://api.dingtalk.com/). Also cap _session_webhooks dict at 500 entries with FIFO eviction to prevent unbounded memory growth from long-running gateway instances. api_server.py: The X-Hermes-Session-Id request header is accepted and echoed back into response headers (lines 675, 697) without sanitization. A session ID containing \r\n enables HTTP response splitting / header injection. Add a check that rejects session IDs containing control characters (\r, \n, \x00). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…de URLs before secret check
error_classifier.py: Message-only auth errors ("invalid api key", "unauthorized",
etc.) were classified as retryable=True (line 707), inconsistent with the HTTP 401
path (line 432) which correctly uses retryable=False + should_fallback=True. The
mismatch causes 3 wasted retries with the same broken credential before fallback,
while 401 errors immediately attempt fallback. Align the message-based path to
match: retryable=False, should_fallback=True.
web_tools.py: The _PREFIX_RE secret-detection check in web_extract_tool() runs
against the raw URL string (line 1196). URL-encoded secrets like %73k-1234... (
sk-1234...) bypass the filter because the regex expects literal ASCII. Add
urllib.parse.unquote() before the check so percent-encoded variants are also caught.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…s, close server on error Three silent `except Exception` blocks in approval.py (lines 345, 387, 469) return fallback values with zero logging — making it impossible to debug callback failures, allowlist load errors, or config read issues. Add logger.warning/error calls that match the pattern already used by save_permanent_allowlist() and _smart_approve() in the same file. In mcp_oauth.py, narrow the overly-broad `except Exception` in get_tokens() and get_client_info() to the specific exceptions Pydantic's model_validate() can raise (ValueError, TypeError, KeyError), and include the exception message in the warning. Also wrap the _wait_for_callback() polling loop in try/finally so the HTTPServer is always closed — previously an asyncio.CancelledError or any exception in the loop would leak the server socket. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…failure, webhook route error severity prompt_builder.py: The `hidden_div` detection pattern uses `.*` which does not match newlines in Python regex (re.DOTALL is not passed). An attacker can bypass detection by splitting the style attribute across lines: `<div style="color:red;\ndisplay: none">injected content</div>` Replace `.*` with `[\s\S]*?` to match across line boundaries. credential_files.py: `_load_config_files()` catches all exceptions at DEBUG level (line 171), making YAML parse failures invisible in production logs. Users whose credential files silently fail to mount into sandboxes have no diagnostic clue. Promote to WARNING to match the severity pattern used by the path validation warnings at lines 150 and 158 in the same function. webhook.py: `_reload_dynamic_routes()` logs JSON parse failures at WARNING (line 265) but the impact — stale/corrupted dynamic routes persisting silently — warrants ERROR level to ensure operator visibility in alerting pipelines. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
The same .* pattern vulnerable to newline bypass that was fixed in prompt_builder.py (PR #6925) also existed in skills_guard.py. Changed to [\s\S]*? to match across newlines.
Session continuation now requires API_SERVER_KEY to be configured. Update TestSessionIdHeader tests to use auth_adapter with Bearer token.
Contributor
|
This was referenced Apr 10, 2026
Closed
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
2 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.
Summary
Salvage of 4 security/hardening PRs onto current main:
Cherry-picked (contributor authorship preserved):
should_fallback=Trueto message-based auth error path (aligning with 401 code path),urllib.parse.unquote()before secret-prefix regex check to prevent percent-encoded bypassexcept Exceptionblocks in approval.py, narrow OAuth catches in mcp_oauth.py, wrap polling loop in try/finally for server_close().*→[\s\S]*?to match across newlines (injection bypass), promote credential YAML parse failure log to WARNING, webhook route error to ERRORFollow-up fixes:
tools/skills_guard.py(missed by fix: hidden_div regex bypass with newlines, credential config silent failure, webhook route error severity #6925)TestSessionIdHeadertests to useauth_adapterwith Bearer token (required after fix(security): require auth for session continuation, warn on missing API key #6930 merged)Test plan