fix: unauthenticated Gmail/Calendar webhooks, OAuth identity spoof, and avatar SSRF - #49
Closed
Gmin2 wants to merge 2 commits into
Closed
fix: unauthenticated Gmail/Calendar webhooks, OAuth identity spoof, and avatar SSRF#49Gmin2 wants to merge 2 commits into
Gmin2 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds Pub/Sub webhook push authentication using OIDC token verification, introduces regex validation for Microsoft Graph webhook tokens, hardens avatar URL filtering with IP and domain allowlisting, enforces provider identity verification in OAuth flows, and implements constant-time client-state comparison for Microsoft webhooks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Pub/Sub Client
participant Webhooks as Webhook Router
participant Auth as verify_pubsub_push
participant Google as Google OAuth
participant Handler as Request Handler
Client->>Webhooks: POST with Authorization header
Webhooks->>Auth: verify_pubsub_push(request)
Auth->>Auth: Extract Bearer token
Auth->>Google: verify_oauth2_token(token, audience)
alt Token Valid
Google-->>Auth: Token claims
Auth->>Auth: Verify email == pubsub_push_service_account
alt Email Matches
Auth-->>Webhooks: ✓ Success
Webhooks->>Handler: Process notification
Handler-->>Client: 200 OK
else Email Mismatch
Auth-->>Webhooks: ✗ PubSubAuthError
Webhooks-->>Client: 202 Accepted (logged)
end
else Token Invalid
Google-->>Auth: Verification Error
Auth-->>Webhooks: ✗ PubSubAuthError
Webhooks-->>Client: 202 Accepted (logged)
end
sequenceDiagram
autonumber
participant User as User Agent
participant OAuth as OAuth Endpoint
participant App as Application
participant Provider as Provider API
participant Database as User DB
User->>OAuth: Initiate OAuth flow
OAuth-->>User: Auth code
User->>App: Callback with auth code + credentials
App->>App: verify access_token available?
alt Token Present
App->>Provider: GET /me (fetch identity)
Provider-->>App: Identity (email, name, avatar)
App->>App: Compare provider email == auth email
alt Match
App->>Database: Store oauth_data (provider fields)
Database-->>App: Success
App-->>User: ✓ Logged in
else Mismatch
App-->>User: ✗ Email verification failed
end
else No Token
App->>App: Use request body data
App-->>User: ✓ Logged in (legacy path)
end
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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (5)
core-api/api/routers/webhooks.py (1)
285-287: Minor inconsistency: GET raisesHTTPException, POST returnsResponse.The GET endpoint raises
HTTPException(status_code=400)while the POST endpoint returnsResponse(status_code=400). Both work, but consistency would improve maintainability.♻️ Consistent error handling
if not _MS_VALIDATION_TOKEN_RE.match(validation_token): logger.warning("📡 [Microsoft] Rejected validation token (bad shape)") - return Response(status_code=400) + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="invalid validation token")Also applies to: 316-318
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/api/routers/webhooks.py` around lines 285 - 287, The GET handler currently raises HTTPException on invalid validationToken while the POST handler returns Response; make error handling consistent by using the same mechanism in both places (prefer raising HTTPException for both). Update the validation branches that use _MS_VALIDATION_TOKEN_RE (the GET branch using logger.warning and raise HTTPException and the analogous POST branch at the other block) so both paths return the same type of error (HTTPException with status_code=status.HTTP_400_BAD_REQUEST and the same detail message) and keep the logger.warning calls but ensure they precede the raised exception.core-api/tests/unit/test_microsoft_webhook_validate.py (1)
36-41: Minor: Comment is slightly misleading abouthmac.compare_digestbehavior.The comment suggests
hmac.compare_digest"only fails on equal-length inputs in some libraries" — this isn't quite accurate. Python'shmac.compare_digesthandles unequal-length byte strings correctly by returningFalsewithout timing leaks. The test itself is valid and valuable.📝 Suggested comment fix
def test_constant_time_compare_handles_unequal_lengths(provider): - # hmac.compare_digest only fails on equal-length inputs in some libraries. - # Make sure unequal-length values are rejected without raising. + # Verify that unequal-length clientState values are rejected without raising. notification = {"clientState": "short"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/tests/unit/test_microsoft_webhook_validate.py` around lines 36 - 41, The test comment for test_constant_time_compare_handles_unequal_lengths is misleading about hmac.compare_digest; update the comment to accurately state that Python's hmac.compare_digest returns False for unequal-length inputs (without timing leaks) and that this test ensures provider.validate_notification correctly rejects unequal-length clientState values without raising an exception; reference the test name and provider.validate_notification when editing the comment.core-api/tests/unit/test_avatar_url_guard.py (1)
16-29: Good SSRF test coverage; consider adding IPv6-mapped IPv4 test cases.The test suite covers key attack vectors well. Consider adding test cases for IPv6-mapped IPv4 addresses (e.g.,
https://[::ffff:127.0.0.1]/aorhttps://[::ffff:169.254.169.254]/) which some SSRF bypasses use to evade IP blocklists.💡 Additional test cases to consider
"https://[::1]/a", # ipv6 loopback + "https://[::ffff:127.0.0.1]/a", # ipv6-mapped loopback + "https://[::ffff:169.254.169.254]/", # ipv6-mapped metadata "", # empty🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/tests/unit/test_avatar_url_guard.py` around lines 16 - 29, Add IPv6-mapped IPv4 test cases to the existing parametrized test_rejects_unsafe_urls to catch SSRF bypasses that represent IPv4 addresses inside IPv6 brackets; update the list of urls passed to test_rejects_unsafe_urls so it includes examples like "https://[::ffff:127.0.0.1]/a" and "https://[::ffff:169.254.169.254]/" and assert _is_safe_avatar_url(url) is False for those entries to ensure the validation in _is_safe_avatar_url rejects IPv6-mapped IPv4 forms.core-api/api/services/auth.py (2)
284-287: Consider catching broader socket exceptions.
socket.getaddrinfocan raisesocket.herrororsocket.timeoutin addition tosocket.gaierror. These would propagate up and potentially cause unexpected 500 errors.🛡️ Broader exception handling
try: infos = socket.getaddrinfo(host, None) - except socket.gaierror: + except OSError: # Covers gaierror, herror, timeout, etc. return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/api/services/auth.py` around lines 284 - 287, The current try/except around socket.getaddrinfo only catches socket.gaierror; extend the exception handling to also catch socket.herror and socket.timeout (or a tuple of these exceptions) so DNS/name resolution failures won't propagate as 500s; update the try block surrounding the socket.getaddrinfo(host, None) call in auth.py (the same block that currently excepts socket.gaierror) to catch the broader socket exceptions and return False on those errors.
275-296: Good SSRF mitigation; note inherent TOCTOU limitation with DNS validation.The allowlist and IP validation are solid. However, there's an inherent TOCTOU (time-of-check-time-of-use) gap: DNS is resolved here for validation, but
httpxresolves again when making the actual request. A sophisticated attacker with DNS control could return a safe IP during validation and a malicious IP during fetch.This is a known limitation of application-layer SSRF protections. For defense-in-depth, consider network-layer controls (e.g., egress firewall blocking RFC1918/metadata IPs) if not already in place. The current implementation is still a significant improvement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/api/services/auth.py` around lines 275 - 296, The URL validation in _is_safe_avatar_url is vulnerable to a TOCTOU DNS race because the code resolves hostnames for checks but httpx will re-resolve when fetching; to fix, either enforce network-layer egress controls (block RFC1918/metadata ranges) or ensure the fetch uses the exact IP(s) validated: resolve host once inside _is_safe_avatar_url (or a helper), pass that IP to the httpx request URL, set the Host header to the original hostname, and ensure TLS SNI/hostname verification uses the original host (use a custom transport or client options in the httpx call that performs TLS handshake with server_hostname=original host) so the request cannot be redirected to a different IP after validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core-api/api/routers/webhooks.py`:
- Around line 285-287: The GET handler currently raises HTTPException on invalid
validationToken while the POST handler returns Response; make error handling
consistent by using the same mechanism in both places (prefer raising
HTTPException for both). Update the validation branches that use
_MS_VALIDATION_TOKEN_RE (the GET branch using logger.warning and raise
HTTPException and the analogous POST branch at the other block) so both paths
return the same type of error (HTTPException with
status_code=status.HTTP_400_BAD_REQUEST and the same detail message) and keep
the logger.warning calls but ensure they precede the raised exception.
In `@core-api/api/services/auth.py`:
- Around line 284-287: The current try/except around socket.getaddrinfo only
catches socket.gaierror; extend the exception handling to also catch
socket.herror and socket.timeout (or a tuple of these exceptions) so DNS/name
resolution failures won't propagate as 500s; update the try block surrounding
the socket.getaddrinfo(host, None) call in auth.py (the same block that
currently excepts socket.gaierror) to catch the broader socket exceptions and
return False on those errors.
- Around line 275-296: The URL validation in _is_safe_avatar_url is vulnerable
to a TOCTOU DNS race because the code resolves hostnames for checks but httpx
will re-resolve when fetching; to fix, either enforce network-layer egress
controls (block RFC1918/metadata ranges) or ensure the fetch uses the exact
IP(s) validated: resolve host once inside _is_safe_avatar_url (or a helper),
pass that IP to the httpx request URL, set the Host header to the original
hostname, and ensure TLS SNI/hostname verification uses the original host (use a
custom transport or client options in the httpx call that performs TLS handshake
with server_hostname=original host) so the request cannot be redirected to a
different IP after validation.
In `@core-api/tests/unit/test_avatar_url_guard.py`:
- Around line 16-29: Add IPv6-mapped IPv4 test cases to the existing
parametrized test_rejects_unsafe_urls to catch SSRF bypasses that represent IPv4
addresses inside IPv6 brackets; update the list of urls passed to
test_rejects_unsafe_urls so it includes examples like
"https://[::ffff:127.0.0.1]/a" and "https://[::ffff:169.254.169.254]/" and
assert _is_safe_avatar_url(url) is False for those entries to ensure the
validation in _is_safe_avatar_url rejects IPv6-mapped IPv4 forms.
In `@core-api/tests/unit/test_microsoft_webhook_validate.py`:
- Around line 36-41: The test comment for
test_constant_time_compare_handles_unequal_lengths is misleading about
hmac.compare_digest; update the comment to accurately state that Python's
hmac.compare_digest returns False for unequal-length inputs (without timing
leaks) and that this test ensures provider.validate_notification correctly
rejects unequal-length clientState values without raising an exception;
reference the test name and provider.validate_notification when editing the
comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 131af1a4-da5d-421d-bef6-9c9b7befd032
📒 Files selected for processing (8)
core-api/api/config.pycore-api/api/routers/webhooks.pycore-api/api/services/auth.pycore-api/api/services/microsoft/microsoft_webhook_provider.pycore-api/lib/webhook_auth.pycore-api/tests/unit/test_avatar_url_guard.pycore-api/tests/unit/test_microsoft_webhook_validate.pycore-api/tests/unit/test_pubsub_webhook_auth.py
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.
The core-api handles inbound requests from google, microsoft, and the oauth callback i found 5 bugs that are all reproducible with curl against a local supabase stack, the gmail webhook accepted any unauthenticated POST. you can forge a pub/sub shaped body and the server will run a real query against push_subscriptions with whatever email you put in:
returns 200, server logs show
GET .../push_subscriptions?...&ext_connections.provider_email=eq.victim%40example.comfrom the attacker controlled email. the calendar webhook had the same hole, just send it-d '{}'and it processes the dispatch. the fix: pub/sub configured with a service account sends a google signed oidc bearer on every push, we now verify that token againstpubsub_push_service_accountbefore touching anything. if it doesn't check out we still return 200 with a log warning instead of 401, otherwise pub/sub will keep retrying the forged message with exponential backoff. The microsoft webhook compared clientState with plain!=which leaks timing, and the validation handshake endpoint reflected literally any input back as text/plain:curl -i 'http://localhost:8000/api/webhooks/microsoft?validationToken=ARBITRARY_<script>alert(1)</script>'echoes the script tag right back. swapped the compare to hmac.compare_digest, locked validationToken down to
[A-Za-z0-9_-]{1,1024}, and removed a log line that printed a clientState checkmark before validation had actually happened, it was always green even for forged notifications.the bigger one was
/api/auth/complete-oauth. it accepted client supplied provider_user_id, name, avatar_url, and access_token and persisted them without ever calling the provider to confirm anything. with a real supabase JWT you can write whatever you want into ext_connections:curl -X POST http://localhost:8000/api/auth/complete-oauth \ -H "Authorization: Bearer $JWT" \ -H 'Content-Type: application/json' \ -d '{"user_id":"...","email":"attacker@coreoss-pentest.io","provider":"google","provider_user_id":"GOOGLE-ID-OF-DIFFERENT-PERSON-1234567890","access_token":"ya29.NOT-A-REAL-TOKEN"}'returns 200 and the row lands with
provider_user_id = GOOGLE-ID-OF-DIFFERENT-PERSON-1234567890and a fabricated token encrypted at rest. now whenever access_token is supplied we callget_user_info(access_token, provider)and overwrite those fields with whatever the provider returns, and bail if the email tied to the token doesn't match the authenticated user.Last one is the avatar download.
_download_avatar_to_r2didhttpx.get(avatar_url, follow_redirects=True)against any url the client wanted, no host check, no IP filter. i ran a tiny python http.server on 127.0.0.1:9999 that logs hits, setavatar_url=http://127.0.0.1:9999/aws-metadata-iam/security-credentials/admin-rolein the complete-oauth body, and the trap logged:so an authenticated user could probe internal services or hit cloud metadata. fix is the obvious one: https only, allowlist
*.googleusercontent.comandgraph.microsoft.com, resolve the host withsocket.getaddrinfoand reject anything private/loopback/link-local/reserved/multicast, and turn redirect following off so a 302 to 169.254.169.254 can't slip in. after these changes every one of those exploits comes back 200 with a log warning (pub/sub paths, by design so we don't trigger retry storms) or 400 (oauth and validation token), nothing hits the database, and the trap server stays empty. added tests undertests/unit/for the avatar guard, microsoft clientState compare, and the pub/sub verifier, with some testsSummary by CodeRabbit
New Features
Bug Fixes
Tests