Skip to content

fix(security): chat attachment XSS, MCP OAuth SSRF guards, Teams clientState verification#4877

Merged
waleedlatif1 merged 6 commits into
stagingfrom
fix/secv
Jun 4, 2026
Merged

fix(security): chat attachment XSS, MCP OAuth SSRF guards, Teams clientState verification#4877
waleedlatif1 merged 6 commits into
stagingfrom
fix/secv

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • Escape attachment filename/data URL in chat preview and replace document.write with a blob URL (XSS)
  • Guard MCP OAuth discovery and token revocation endpoints against SSRF
  • Verify Microsoft Graph clientState on Teams chat-subscription webhook notifications

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

…caping

Replace document.write with an escaped blob URL preview: HTML-entity
encode the user-controlled filename and data URL, open with
noopener,noreferrer, and revoke the blob URL after navigation.
Route discoverOAuthServerInfo and the RFC 7009 revocation POST through an
SSRF-guarded fetch that validates every request URL via validateMcpServerSsrf
(blocking private/reserved/loopback targets, honoring ALLOWED_MCP_DOMAINS and
self-hosted localhost rules) and pins the connection to the resolved IP to
prevent DNS-rebinding TOCTOU. Previously these fetches used unvalidated global
fetch against URLs taken verbatim from attacker-controllable
authorization-server metadata.
…tifications

The microsoftteams_chat_subscription trigger set clientState=webhook.id when
creating the Graph subscription but never validated it on inbound change
notifications, so any request to the webhook path with a crafted notification
body was treated as authentic (CWE-345). verifyAuth now requires every
notification in the value array to carry a clientState matching the stored
webhook id (constant-time compare) and rejects payloads without notifications.
Validation handshakes (validationToken) are handled before auth and remain
unaffected; outgoing-webhook HMAC auth is unchanged.
@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 4, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jun 4, 2026 4:42pm

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented Jun 4, 2026

PR Summary

High Risk
Touches authentication-adjacent webhook verification and server-side outbound HTTP for OAuth revocation; mistakes could block legitimate Teams triggers or MCP disconnect flows, while fixes address XSS and SSRF.

Overview
This PR hardens three security-sensitive paths: chat attachment preview, MCP OAuth revocation/discovery HTTP, and Microsoft Teams Graph chat-subscription webhooks.

Chat: Image attachment previews no longer use document.write with raw attachment name and dataUrl. A shared openAttachmentPreview helper HTML-escapes those values and opens a blob URL preview tab with noopener,noreferrer, used from both click and keyboard handlers.

MCP OAuth: New createSsrfGuardedMcpFetch runs validateMcpServerSsrf on every outbound URL (for metadata-driven hops like discovery and revocation_endpoint), then uses IP-pinned fetch when allowed. revokeMcpOauthTokens wires this into OAuth discovery and RFC 7009 postRevoke instead of plain fetch. Unit tests cover validation, blocking, and URL objects.

Teams: For microsoftteams_chat_subscription, verifyAuth now requires each notification in the Graph value array to carry a string clientState that matches the webhook id (constant-time compare), with 401 on missing/malformed payloads or missing webhook id; other trigger types are unchanged. New Vitest coverage documents accept/reject cases.

Reviewed by Cursor Bugbot for commit 617073c. Configure here.

… unavailable

Hardens the clientState check so a missing webhook id (theoretically
unreachable, since the row is looked up by primary key) can never collapse
the expected value to an empty string that a forged clientState could match.
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 4, 2026

Greptile Summary

This PR fixes three distinct security issues: XSS in chat attachment preview via document.write, SSRF in MCP OAuth discovery and RFC 7009 token revocation, and missing clientState verification on Microsoft Teams Graph subscription webhook notifications.

  • XSS (message.tsx): Replaces document.write + direct template interpolation with a Blob/createObjectURL approach, escaping filename and data URL through a module-level HTML_ESCAPES map. Properly escapes all five HTML special chars (&, <, >, \", '), uses noopener,noreferrer on the opened window, and revokes the blob URL after 60 s.
  • SSRF (pinned-fetch.ts / revoke.ts): Adds createSsrfGuardedMcpFetch() that calls validateMcpServerSsrf per request before pinning to the resolved IP. Applied to both discoverOAuthServerInfo and postRevoke so each metadata-driven redirect/endpoint hop is independently validated.
  • Teams clientState (microsoft-teams.ts): Enforces per-notification clientState matching against webhook.id using safeCompare for the microsoftteams_chat_subscription trigger; fails closed when webhook.id is absent.

Confidence Score: 5/5

All three security fixes are correctly implemented and safe to merge.

The XSS fix properly escapes both the filename and data URL before interpolation and uses a blob URL to avoid document.write. The SSRF guard validates every outbound URL independently per request, covering both OAuth discovery hops and the revocation endpoint. The Teams clientState check is timing-safe, fails closed on missing webhook.id, and has thorough test coverage across edge cases. No regressions or new attack surfaces introduced.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/app/chat/components/message/message.tsx XSS fix: replaces document.write with blob URL approach; filename and data URL are correctly HTML-escaped via module-level HTML_ESCAPES map before interpolation into the generated HTML.
apps/sim/lib/mcp/pinned-fetch.ts Adds createSsrfGuardedMcpFetch() — validates each outbound URL via validateMcpServerSsrf before issuing the request, then pins to the resolved IP. Falls back to globalThis.fetch when validation returns null (allowlist mode, IP literals, localhost in self-hosted), which is intentional per existing design.
apps/sim/lib/mcp/oauth/revoke.ts Wires createSsrfGuardedMcpFetch into both discoverOAuthServerInfo and postRevoke so every metadata-driven hop is independently SSRF-validated rather than only the initial server URL.
apps/sim/lib/mcp/pinned-fetch.test.ts Adds three focused tests for createSsrfGuardedMcpFetch: validates URL per-request, rejects blocked IPs without issuing the fetch, and handles URL objects correctly.
apps/sim/lib/webhooks/providers/microsoft-teams.ts Adds clientState verification for microsoftteams_chat_subscription trigger: fails closed on missing webhook.id, rejects empty/non-array payloads, and uses safeCompare per notification.
apps/sim/lib/webhooks/providers/microsoft-teams.test.ts New test file with comprehensive coverage: valid clientState, forged state, missing state, non-string state, no value array, empty array, unparseable body, mixed-batch rejection, missing webhook.id, and non-subscription bypass.

Reviews (2): Last reviewed commit: "improvement(chat): hoist HTML escape map..." | Re-trigger Greptile

Comment thread apps/sim/lib/webhooks/providers/microsoft-teams.ts
Comment thread apps/sim/lib/mcp/pinned-fetch.ts
Comment thread apps/sim/app/chat/components/message/message.tsx Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 617073c. Configure here.

@waleedlatif1 waleedlatif1 merged commit c620fdc into staging Jun 4, 2026
10 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/secv branch June 4, 2026 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant