fix(security): SSRF pinning, Twilio webhook auth, copilot token leak, audit-log tenant scoping#4899
Conversation
…binding) clickhouseRequest() validated config.host via validateDatabaseHost() but discarded the resolved IP and called fetch() with the original hostname, triggering a second DNS lookup. A workflow author controlling the host parameter could use DNS rebinding to pass validation against a public IP and then connect to an internal/private address (SSRF). Replace fetch() with secureFetchWithPinnedIP(), connecting to the validated resolvedIP while preserving the hostname for Host/TLS SNI — the same DNS-pinning pattern used by the other DB tools. Set Content-Length explicitly so request framing is identical to the previous fetch. Add tests locking the contract: connection targets the validated IP not the hostname, no request is issued on validation failure, http/https and allowHttp are selected from secure, and body/headers propagate.
…ding window The MCP auth-type probe (detectMcpAuthType) issued raw, unpinned fetch() calls against the user-supplied server URL, re-resolving DNS independently of validateMcpServerSsrf. This re-opened the exact DNS-rebinding (TOCTOU) window the pinned McpClient path was built to close: a hostname that resolves to a public IP during validation could resolve to an internal IP during the probe. The probe now pins to the IP already validated by the caller via createMcpPinnedFetch(resolvedIP); when no pre-validated IP is available it falls back to createSsrfGuardedMcpFetch(), which validates and pins each request. The best-effort session-close DELETE reuses the same pinned fetch. Both call sites (test-connection route and performCreateMcpServer) thread the resolved IP into the probe.
…lot credentials GET /api/copilot/credentials returned each connected account's live, post-refresh OAuth access token in plaintext to any session for that user. The endpoint is only used for credential display/masking and no client reads the token, so drop accessToken from the get_credentials tool output and the copilot credentials response contract. Also removes the incidental refreshTokenIfNeeded side-effect on this read path. Adds regression tests: - get-credentials: asserts the response exposes only masked metadata and never leaks the access/refresh token. - revoke: locks in that revokeMcpOauthTokens routes OAuth discovery and RFC 7009 revocation through the SSRF-guarded fetch (no raw fetch to an attacker-controlled revocation_endpoint).
The twilio (SMS) provider handler implemented no verifyAuth, so the webhook dispatcher queued workflow executions for any request to a known SMS trigger path without validating the Twilio signature — allowing forged inbound SMS events. Only the twilio-voice handler performed signature verification. Extract the shared HMAC-SHA1 signature validation into twilio-signature.ts and wire it into both the SMS and Voice handlers. Verification is enforced when an auth token is configured (parity with Voice); requests without a configured token pass through per the provider-wide optional-secret convention. Add regression tests for both handlers.
…lidated, IP-pinned fetch Knowledge connectors that accept a custom service host/endpoint (S3-compatible endpoints, self-managed GitLab/Sentry hosts, Obsidian vault URLs) performed server-side fetches without the repository's SSRF guard, letting an authenticated user with KB write access probe internal/loopback hosts from the backend. Add secureFetchWithRetry (validateUrlWithDNS + secureFetchWithPinnedIP + the same retry/backoff as fetchWithRetry) and route every request in the s3, gitlab, sentry, and obsidian connectors through it - including pagination and hydration. Gate the S3 plain-http loopback exception to self-hosted deployments.
…undary Actor membership was used as a standalone tenant predicate, letting org admins read members' audit activity from personal workspaces and other tenants. Scope queries to org-attached workspaces plus org-level events, with actor membership only narrowing the scope; validate workspaceId filters against the caller's organization.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview SSRF / DNS rebinding: ClickHouse tool HTTP calls now use Enterprise audit logs: Tenant scope is rebuilt around Twilio webhooks: SMS and Voice handlers delegate to shared Copilot credentials: The Other: Slack OAuth typing fix for Reviewed by Cursor Bugbot for commit 69b3857. Configure here. |
Greptile SummaryThis PR closes five distinct security vulnerabilities across the Sim platform: SSRF/DNS-rebinding windows in ClickHouse and MCP auth-probe requests (fixed by IP-pinning post-validation), unauthenticated Twilio SMS webhooks (now enforced with HMAC-SHA1 signature verification), plaintext OAuth access-token exposure in the Copilot credentials API (response now strips all token fields), and a cross-tenant IDOR in enterprise audit logs (scope now anchored to
Confidence Score: 5/5All five targeted vulnerabilities are correctly addressed with defense-in-depth layering, and each fix is covered by regression tests. Every changed code path has been verified: IP-pinning is correctly threaded from SSRF validation to the actual TCP connection in ClickHouse, MCP probe, and all four KB connectors. The Twilio HMAC-SHA1 implementation matches the documented algorithm and uses constant-time comparison. The audit-log scope is now anchored to the organization FK rather than member ownership, closing the cross-tenant read path. The copilot credentials tool no longer touches or returns OAuth tokens. No regressions were found in related code paths. No files require special attention — the changes are well-scoped, consistently applied, and backed by unit tests covering the security-critical paths. Important Files Changed
Reviews (2): Last reviewed commit: "fix(build): keep connector SSRF fetch ou..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Addresses PR review: when no auth token is set, verifyTwilioAuth skips signature verification (optional-secret convention). Log a warning so operators can detect a webhook running unauthenticated.
…dit scope SQL IN never matches NULL, so system/automated events inside org workspaces were hidden unless includeDeparted=true. The default scope now matches current members OR null-actor rows, still inside the org boundary.
Installed better-auth's OAuth2Tokens no longer declares the raw property; access it through an intersection cast (no behavior change) so type-check passes.
The connectors SSRF fix routed s3/gitlab/sentry/obsidian through secureFetchWithRetry, which transitively imports input-validation.server (and its Node-only `dns/promises`). connectors/registry.ts is imported by client components for connector metadata, so the connector sync code — which only ever runs in server API routes — gets pulled into the client bundle, and Turbopack fails to resolve `dns/promises` (no browser shim). - Move secureFetchWithRetry into a dedicated `secure-fetch.server` module so the shared documents/utils stays client-safe; connectors import from there. - Add a browser-only `turbopack.resolveAlias` stub for `dns`/`dns/promises` (the documented Next 16 remedy). Server bundles keep the real module, so SSRF validation is unaffected — only the never-executed client copy is stubbed. Verified with a full `next build` (compiles successfully, no module errors).
|
@greptile |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 69b3857. Configure here.
1 participant
Summary
Type of Change
Testing
Tested manually and with added unit tests (clickhouse 6, mcp probe 5, copilot credentials 4, twilio 12, connectors 37, audit-logs 15 — all passing)
Checklist