Skip to content

fix(security): SSRF fixes#4548

Merged
waleedlatif1 merged 5 commits intostagingfrom
fix/security-ssrf-token-leaks
May 11, 2026
Merged

fix(security): SSRF fixes#4548
waleedlatif1 merged 5 commits intostagingfrom
fix/security-ssrf-token-leaks

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • Azure TTS: validate `region` param against an allowlist regex in both the contract and at runtime — prevents user-supplied region from redirecting requests (and the Azure subscription key) to an attacker-controlled host
  • Shopify OAuth: apply `shopifyShopDomainSchema` validation to the `shopDomain` cookie before building the server-side fetch URL — was only checked as non-empty, allowing SSRF
  • HubSpot: remove `fullResponse` from the OAuth introspection `logger.info` call — the raw response includes the access token; now only logs non-sensitive metadata
  • Wealthbox OAuth `getUserInfo`: fix wrong endpoint (`/v1/users/me` → `/v1/me`), fix auth header (`ACCESS_TOKEN` → `Authorization: Bearer`), and fix non-deterministic fallback identity that created a new account on every token refresh (`generateId()` removed; fallback now hashes the stable refresh token)

Type of Change

  • Bug fix

Testing

Tested manually. Azure region regex validated against all 33 live Azure Speech Service regions — all pass. Shopify schema validated against Shopify's documented domain format. Wealthbox endpoint and auth header confirmed against dev.wealthbox.com API docs.

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)

- Azure TTS SSRF: validate region against /^[a-z][a-z0-9-]{1,30}[a-z0-9]$/
  in both the contract (tts.ts) and runtime guard in synthesizeWithAzure,
  preventing user-supplied region from redirecting requests to arbitrary hosts
- HubSpot token in logs: remove fullResponse from logger.info call;
  log only non-sensitive metadata (hub_id, hub_domain, user_id) instead
  of the full introspection response which included the access token
- Wealthbox account takeover: replace hardcoded email with per-user identity
  by fetching /v1/users/me; fall back to token-derived stable identifier
  so distinct Wealthbox users no longer share the same email address
- Shopify SSRF: apply shopifyShopDomainSchema (.myshopify.com allowlist)
  to shopDomain from cookie before using it to build the fetch URL
… identity

- Bug 1: Change API endpoint from /v1/users/me to /v1/me (correct Wealthbox API path)
- Bug 2: Replace ACCESS_TOKEN header with Authorization: Bearer <token> (standard OAuth 2.0)
- Bug 3: Remove generateId() from returned id (was non-deterministic, caused duplicate accounts);
  use refresh token (stable, long-lived) instead of access token (rotates every ~2 hours)
  as the hash source for the fallback identity; return null if no token is available
@vercel
Copy link
Copy Markdown

vercel Bot commented May 10, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 11, 2026 2:51am

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 10, 2026

PR Summary

Medium Risk
Tightens validation and identity handling in OAuth and Azure TTS request construction, which is security-relevant and could break edge-case integrations if callers relied on previously-accepted inputs. Also changes Wealthbox user identity derivation, which may affect account linking behavior for existing connections.

Overview
Closes multiple SSRF/token-leak vectors by validating user-controlled host components before server-side fetch calls and by reducing sensitive OAuth logging.

Specifically, the Shopify OAuth store route now enforces shopifyShopDomainSchema on the shopDomain cookie before building the Shopify API URL, and the unified Azure TTS proxy validates region via a strict Azure-region regex in both the API contract and runtime.

OAuth provider hardening includes updating Wealthbox getUserInfo to call the correct /v1/me endpoint with Authorization: Bearer, and making the fallback identity stable by hashing a long-lived token instead of using generateId(). HubSpot token introspection logging is trimmed to metadata-only to avoid logging sensitive response content.

Reviewed by Cursor Bugbot for commit c90c3f2. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 10, 2026

Greptile Summary

This PR applies targeted SSRF and credential-exposure fixes across four areas: Azure TTS region validation, Shopify OAuth shop domain enforcement, HubSpot introspection log sanitization, and a full Wealthbox OAuth rewrite with the correct endpoint, auth header, and SHA-256-derived stable identity.

  • Azure TTS: region is now validated by /^[a-z][a-z0-9-]{1,30}[a-z0-9]$/ at both the Zod contract layer and inside synthesizeWithAzure, preventing an attacker-controlled hostname from redirecting the Azure subscription key.
  • Shopify OAuth: shopifyShopDomainSchema (enforces *.myshopify.com) is checked before the shop-domain value is interpolated into the fetch URL, closing the SSRF vector.
  • HubSpot / Wealthbox: fullResponse (which contained the raw access token) is stripped from the HubSpot logger; the Wealthbox getUserInfo now hits the real /v1/me endpoint with a proper Authorization: Bearer header and falls back to a SHA-256 hash of the refresh token rather than generateId() to prevent duplicate account creation on token rotation.

Confidence Score: 4/5

The four targeted security fixes are sound and well-structured; an SSML injection issue in the Azure TTS route (flagged in a prior review) remains open.

All four fixes close real attack vectors cleanly — the Shopify domain allowlist, the Azure region allowlist with defense-in-depth, the HubSpot log sanitization, and the Wealthbox identity rewrite are each correct and self-consistent. The remaining concern is that voiceId, style, rate, pitch, and text are still interpolated verbatim into the SSML template without XML escaping in the TTS route, a finding carried forward from a prior review that this PR does not address.

apps/sim/app/api/tools/tts/unified/route.ts — SSML string construction still embeds user-supplied fields without XML escaping.

Important Files Changed

Filename Overview
apps/sim/app/api/auth/oauth2/shopify/store/route.ts Adds shopifyShopDomainSchema validation for shopDomain cookie before constructing the fetch URL, preventing SSRF via forged *.myshopify.com enforcement.
apps/sim/app/api/tools/tts/unified/route.ts Adds runtime AZURE_REGION_RE check in synthesizeWithAzure after the Zod-layer validation, providing defense-in-depth against SSRF via the region parameter; SSML injection via voiceId/style/text (flagged in prior review) remains unaddressed.
apps/sim/lib/api/contracts/tools/media/tts.ts Adds .regex() validation to the region field in the unified TTS Zod schema, rejecting invalid Azure region strings at the contract layer.
apps/sim/lib/auth/auth.ts Fixes Wealthbox OAuth (correct endpoint, Bearer auth, SHA-256 stable fallback identity) and strips fullResponse from the HubSpot introspection log to prevent access token leakage.

Sequence Diagram

sequenceDiagram
    participant Client
    participant ShopifyRoute as Shopify Store Route
    participant AzureTTS as Azure TTS Route
    participant WealthboxOAuth as Wealthbox OAuth
    participant ExternalAPI as External API

    Note over ShopifyRoute: SSRF Fix
    Client->>ShopifyRoute: GET shopify/store
    ShopifyRoute->>ShopifyRoute: validate cookie schema
    ShopifyRoute->>ShopifyRoute: shopifyShopDomainSchema check
    alt Domain invalid
        ShopifyRoute-->>Client: "redirect error=shopify_invalid_domain"
    else Domain valid
        ShopifyRoute->>ExternalAPI: fetch shopDomain admin API
        ExternalAPI-->>ShopifyRoute: shop data
        ShopifyRoute-->>Client: redirect shopify_connected
    end

    Note over AzureTTS: SSRF Fix with defense-in-depth
    Client->>AzureTTS: POST tts/unified
    AzureTTS->>AzureTTS: Zod region regex check
    AzureTTS->>AzureTTS: Runtime AZURE_REGION_RE check
    alt Region invalid
        AzureTTS-->>Client: 400 Invalid Azure region
    else Region valid
        AzureTTS->>ExternalAPI: fetch region.tts.speech.microsoft.com
        ExternalAPI-->>AzureTTS: audio buffer
        AzureTTS-->>Client: audio response
    end

    Note over WealthboxOAuth: Auth + Identity Fix
    Client->>WealthboxOAuth: OAuth callback
    WealthboxOAuth->>ExternalAPI: GET api.crmworkspace.com/v1/me
    alt API returns user id
        ExternalAPI-->>WealthboxOAuth: user data
        WealthboxOAuth-->>Client: identity from API
    else API fails
        WealthboxOAuth->>WealthboxOAuth: SHA256 refresh token slice
        WealthboxOAuth-->>Client: stable fallback identity
    end
Loading

Reviews (3): Last reviewed commit: "fix(auth): replace stale wealthbox userI..." | Re-trigger Greptile

Comment thread apps/sim/lib/auth/auth.ts Outdated
Comment thread apps/sim/app/api/auth/oauth2/shopify/store/route.ts
Comment thread apps/sim/lib/auth/auth.ts Outdated
Comment thread apps/sim/lib/auth/auth.ts Outdated
…d userId

- Replace base64 encoding with SHA-256 hash for fallback token-derived identity
  so raw token bytes are never stored in the DB
- Return null early when Wealthbox API response lacks an id field to prevent
  all such users colliding on the wealthbox-undefined account
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

@waleedlatif1 waleedlatif1 changed the title fix(security): SSRF and token-leakage vulnerabilities fix(security): SSRF fixes May 11, 2026
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 b2e9f57. Configure here.

…l endpoint

The dummy URL comment was rendered obsolete when getUserInfo was updated
to fetch from api.crmworkspace.com/v1/me. Align userInfoUrl with the real
endpoint used in the getUserInfo implementation.
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

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 c90c3f2. Configure here.

…ch codebase pattern

All other providers use `${stableId}-${generateId()}` so the account.create.after
hook can strip the UUID suffix, find stale sibling rows, and migrate credential FKs.
Without the suffix the migration logic is skipped and reconnections would hit
duplicate key conflicts instead of gracefully updating credentials.
@waleedlatif1 waleedlatif1 merged commit badfeae into staging May 11, 2026
9 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/security-ssrf-token-leaks branch May 11, 2026 02:52
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