fix(security): Block SSRF in guardrail webhook URL and require authentication on write endpoints#538
Open
prince-shakyaa wants to merge 1 commit into
Conversation
…tication on write endpoints
Author
|
Hii @saikishu , @e2hln Let me know if you need any changes. Thank You. |
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
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.
fix(security): Block SSRF in guardrail webhook URL and require authentication on write endpoints
Fixes #535
Summary
The FinBot Labs guardrail webhook configuration API (
/labs/api/v1/guardrails) allowed any anonymous (temp-session) user to register an arbitrary URL as their webhook target. Because the server-sideGuardrailHookServicefires an HTTP POST to that URL on every hook invocation - and a/testendpoint lets the caller trigger one immediately - this was a fully exploitable unauthenticated SSRF that could reach Redis, Postgres, cloud metadata endpoints (AWS IMDS), and any other host reachable from the server.Root Cause
Two independent weaknesses combined to create the vulnerability:
finbot/apps/labs/routes/guardrails.pyPUT,POST /toggle,POST /rotate-secret,DELETE,POST /test) usedget_session_context, which accepts anonymous temporary sessionsfinbot/apps/labs/routes/guardrails.pywebhook_urlwas only validated formax_length=2048- no scheme or IP filtering at allvalidate_webhook_url()already existed in the codebase for config validation but was never applied at the route layer.Changes -
finbot/apps/labs/routes/guardrails.py1. SSRF blocklist applied to
webhook_urlbefore storingvalidate_webhook_url()resolves the hostname via DNS and rejects:127.x.x.x,::1)10.*,172.16-31.*,192.168.*)169.254.*,fe80::)http(s)scheme2. All write endpoints require an authenticated (email-bound) session
Endpoints changed:
PUT /labs/api/v1/guardrails- upsert configPOST /labs/api/v1/guardrails/toggle- enable/disablePOST /labs/api/v1/guardrails/rotate-secret- rotate HMAC secretDELETE /labs/api/v1/guardrails- delete configPOST /labs/api/v1/guardrails/test- fire test hookThe read-only endpoints (
GET /labs/api/v1/guardrailsandGET /labs/api/v1/guardrails/activity) remain accessible to temp sessions - they returnnull/ empty arrays for users with no config, which is safe.What Was NOT Changed
validate_webhook_url()itself - reused as-is fromfinbot/core/data/repositories.pyGuardrailHookService- no changes needed; the URL is validated before it ever reaches the serviceget_session_context(no sensitive write operations)Attack Scenarios Blocked
webhook_url = "http://127.0.0.1:6379/"webhook_url = "http://10.0.0.1/admin"/testto trigger SSRFhttp://169.254.169.254/latest/meta-data/(AWS IMDS)Tests
New - Route-level security tests
tests/unit/labs/test_guardrail_route_security.py(new file, covers the exact lines changed by this PR)TestGuardrailWriteEndpointsRequireAuthPUT,POST /toggle,POST /rotate-secret,DELETE,POST /test) return 401 for anonymous (temp) sessions.GETstill returns 200.TestGuardrailWebhookUrlSsrfValidationRun:
Existing - Repository / validator tests (no changes needed)
tests/unit/labs/test_guardrail_config.pyalready covers:validate_webhook_url()allows/blocks URLs correctly (production vs debug mode)upsert()raisesValueErrorfor private IPsThose tests are not modified - they remain green as the underlying repository logic is unchanged.
Manual smoke test
Checklist
validate_webhook_url()utility - no new logicGET) unchanged - no regression for anonymous userstest_guardrail_route_security.py) - 401 + 422 cases