fix(security,agents): harden SSRF DNS-rebind, fail loudly, cover untested modules#1299
Open
kovtcharov wants to merge 1 commit into
Open
fix(security,agents): harden SSRF DNS-rebind, fail loudly, cover untested modules#1299kovtcharov wants to merge 1 commit into
kovtcharov wants to merge 1 commit into
Conversation
…allbacks, cover untested modules A DNS-rebind attacker could pass the web client's pre-flight IP check with a public address and then have the actual TCP connect resolve to a private/internal one, because validation and connection used separate DNS lookups. Several modules also swallowed errors silently — a corrupt memory-settings file reverted to defaults with no signal, a failed Telegram background PID-file write let startup "succeed" so a supervisor could never stop the process, and a raising system-prompt fragment was dropped from the composed prompt without a trace. And three high-risk modules (DockerAgent's subprocess calls, the home-dir discovery scanner, Jira JQL templating) plus the API completion happy-path had no test coverage. - SSRF: a PinnedIPAdapter resolves once, validates the exact IP it will dial through a single authority, and pins it — closing the rebind window for http and https. HTTPS cert-name verification still binds to the real hostname; the SNI-vhosting limitation is documented, not silently accepted. - Fail loudly: memory-settings load narrows to the real parse errors and logs; Telegram background startup re-raises on PID-write failure; the prompt-fragment drop and rag unload-on-reload now log instead of `except: pass`. - Tests: new suites for DockerAgent (subprocess/path-allowlist), the discovery classifiers, and JQL templating; the API non-streaming completion happy-path is now covered with a mocked backend instead of being skipped.
github-actions
Bot
added
mcp
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
1 participant
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.
A DNS-rebind attacker could slip past the web client's SSRF guard: the pre-flight IP check and the actual TCP connect used separate DNS lookups, so a host could answer the check with a public IP and the connect with a private/internal one. This PR pins the resolved IP and validates the exact address it dials through a single authority, closing the window for both http and https (HTTPS cert-name verification still binds to the real hostname; the SNI-vhosting trade-off is documented, not silently accepted). It also removes three silent-fallback violations that hid real failures, and adds tests to three high-risk modules that had none.
Reviewer-relevant threads:
web/client.py) — the security fix above; worth a close read ofPinnedIPAdapter.@pytest.mark.skip.Test plan
pytest tests/unit/agents/test_discovery.py tests/unit/agents/test_docker_agent.py tests/unit/agents/test_jql_templates.py tests/unit/test_web_client_ip_pinning.py -q— 100 passpytest tests/unit/test_web_client_edge_cases.py tests/test_rag.py -q— no regression (96 pass)pytest tests/test_api.py -q— completion happy-path now runs (no longer skipped)python util/lint.py --black --isort --flake8— clean on changed files