Fix: add SSRF guard for agent test_db_connection endpoint#14860
Open
dale053 wants to merge 3 commits into
Open
Fix: add SSRF guard for agent test_db_connection endpoint#14860dale053 wants to merge 3 commits into
dale053 wants to merge 3 commits into
Conversation
dosubot
Bot
added
size:M
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA new host-level SSRF guard validates hostnames resolve to globally routable IPs and the test_db_connection endpoint now calls it, returning errors on invalid or unresolvable hosts and using the validated safe_host for all DB connection paths. ChangesSSRF host validation for database connection endpoint
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/apps/restful_apis/agent_api.py (1)
711-714: ⚡ Quick winUse
get_data_error_resultfor client validation errors.
server_error_responsereports a server-side failure, but aValueErrorfromassert_host_is_safeis a request-validation failure (4xx-style). Elsewhere in this file,ValueErrorfrom validators is consistently surfaced viaget_data_error_result(e.g. lines 514-515, 938-939). Recommend aligning with that pattern (already folded into the diff above).🤖 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 `@api/apps/restful_apis/agent_api.py` around lines 711 - 714, The code treats a validation failure from assert_host_is_safe as a server error; change the exception handling to return a client validation response by calling get_data_error_result with the exception message instead of server_error_response. Locate the try/except around assert_host_is_safe(req["host"]) and replace the except block to return get_data_error_result(str(exc)) so it matches the existing pattern used elsewhere in this file (see other ValueError handlers like the ones around lines where get_data_error_result is used).
🤖 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 `@api/apps/restful_apis/agent_api.py`:
- Around line 711-714: The call to assert_host_is_safe currently discards the
validated IP, allowing a TOCTOU DNS-rebind when later passing req["host"] to DB
drivers; change the code to capture the return value (e.g., safe_host =
assert_host_is_safe(req["host"])) and then use safe_host in all subsequent
connection calls instead of req["host"] (apply to
MySQLDatabase/PostgresqlDatabase constructors, pyodbc.connect, ibm_db.connect,
trino.dbapi.connect and the oceanbase/postgres/mssql (SERVER=...)/IBM DB2
(HOSTNAME=...) branches). If hostname preservation is required for TLS/SNI, wrap
the connect call with pin_dns(req["host"], safe_host) instead; ensure you still
call assert_host_is_safe and use its returned IP in the connect logic or the
pin_dns context.
---
Nitpick comments:
In `@api/apps/restful_apis/agent_api.py`:
- Around line 711-714: The code treats a validation failure from
assert_host_is_safe as a server error; change the exception handling to return a
client validation response by calling get_data_error_result with the exception
message instead of server_error_response. Locate the try/except around
assert_host_is_safe(req["host"]) and replace the except block to return
get_data_error_result(str(exc)) so it matches the existing pattern used
elsewhere in this file (see other ValueError handlers like the ones around lines
where get_data_error_result is used).
🪄 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: d7916d9f-0173-4b83-abcf-96964219b045
📒 Files selected for processing (2)
api/apps/restful_apis/agent_api.pycommon/ssrf_guard.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@api/apps/restful_apis/agent_api.py`:
- Around line 711-714: Catch both ValueError and OSError/socket.gaierror from
assert_host_is_safe and treat them as user/data errors by returning
get_data_error_result; additionally add a logging.warning entry when a host is
rejected that records the offending req["host"] and the requesting tenant/user
(pull from req or request context) so rejections are auditable, and if desired
log the full exception at debug level but do not surface resolver details in the
returned get_data_error_result; update the try/except around assert_host_is_safe
to reflect these changes and avoid letting resolver errors bubble up to
server_error_response.
🪄 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: cbca43ff-2ebc-4e14-939e-da0ac9f339fd
📒 Files selected for processing (1)
api/apps/restful_apis/agent_api.py
…connection SSRF guard
Contributor
Author
|
Hi, @wangq8 |
4 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
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.
What problem does this PR solve?
Closes #14858
The
test_db_connectionendpoint in the agent API accepts a user-suppliedhostand connects to it directly via database drivers (MySQL/PostgreSQL) without any validation. This allows an attacker to probe internal network addresses (e.g.127.0.0.1,10.x.x.x, link-local, etc.) through the server — a classic Server-Side Request Forgery (SSRF) vulnerability.This PR adds an SSRF guard that resolves the host and rejects any address that is not globally routable before the database connection is attempted.
Changes:
common/ssrf_guard.py— Addedassert_host_is_safe(), a host-level counterpart of the existingassert_url_is_safe(), designed for non-HTTP protocols (database drivers) where there is no URL to parse.api/apps/restful_apis/agent_api.py— Callassert_host_is_safe(req["host"])at the top oftest_db_connectionso that non-public hosts are rejected early with a clear error message.Fixes #14858
Type of change