fix(security): harden SSO domain registration, webhook path isolation, and CSV export#4813
Conversation
…k path isolation, env secrets, and CSV export
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview SSO: Domains are normalized via Webhooks: CSV export: String headers and cells that start with formula triggers ( Reviewed by Cursor Bugbot for commit 9119b6a. Configure here. |
Greptile SummaryThis PR hardens three distinct attack surfaces: SSO domain registration normalizes and canonicalizes the domain before conflict-checking (preventing cross-tenant hijacking via casing/format variants), webhook path deployment and delivery enforce single-workflow ownership (blocking a second tenant from registering a path already owned by another, with a runtime fallback for pre-existing collisions), and the CSV table export neutralizes formula-injection triggers in both headers and cell values.
Confidence Score: 5/5Safe to merge - all three security fixes are logically sound and backed by focused unit tests. The domain-normalization logic, ownership predicate, and webhook path isolation checks are all correct and well-tested. The runtime fallback in findAllWebhooksForPath provides defense-in-depth even for pre-existing collisions. The CSV formula neutralization follows the standard single-quote text-prefix approach and interacts correctly with escapeCsvField. No broken contract, no data-loss path, and no new auth boundary issue was found. apps/sim/lib/webhooks/utils.server.ts - the tx parameter on findConflictingWebhookPathOwner is never used at either call site, leaving a narrow concurrent-deploy race. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant WebhookRoute as POST /api/webhooks
participant DeployFn as saveTriggerWebhooksForDeploy
participant Guard as findConflictingWebhookPathOwner
participant DB
participant Processor as findAllWebhooksForPath (runtime)
Note over Client,DB: Registration path (route.ts)
Client->>WebhookRoute: "POST {path, workflowId}"
WebhookRoute->>Guard: "{path, workflowId}"
Guard->>DB: "SELECT workflowId WHERE path=X AND isActive=true"
DB-->>Guard: rows
Guard-->>WebhookRoute: "conflictingWorkflowId | null"
alt conflict from another workflow
WebhookRoute-->>Client: 409 PATH_EXISTS
else no conflict
WebhookRoute->>DB: upsert webhook
WebhookRoute-->>Client: 200 OK
end
Note over Client,DB: Deploy path (deploy.ts)
Client->>DeployFn: deploy workflow
DeployFn->>Guard: "{path, workflowId}"
Guard-->>DeployFn: "conflictingWorkflowId | null"
alt conflict detected
DeployFn-->>Client: error 409
else clear
DeployFn->>DB: save webhook config
end
Note over Client,DB: Runtime delivery (processor.ts)
Client->>Processor: incoming request for path
Processor->>DB: SELECT all active webhooks for path
DB-->>Processor: rows
alt cross-workflow collision
Processor->>Processor: pick earliest createdAt as owner
Processor-->>Client: dispatch only to owner workflow
else single workflow
Processor-->>Client: dispatch normally
end
Reviews (6): Last reviewed commit: "refactor(security): consolidate webhook ..." | Re-trigger Greptile |
Address PR review: avoid a full-table scan on every SSO provider registration by filtering candidate rows in SQL with lower(domain) = <normalized>, keeping the in-memory ownership check. Also tighten the normalizeSSODomain TSDoc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…thorization Condense verbose comment blocks to concise TSDoc/single-line form; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the bypassable isInternalFileUrl substring check in resolveInternalKbKey with an origin allow-list (base URL, internal API base URL, TRUSTED_ORIGINS). A crafted external host whose path is /api/files/serve/<victim-key> no longer resolves to the victim key. Relative same-origin URLs are unaffected.
|
@greptile |
|
@cursor review |
…query Match the repo's prevailing `sql`lower(col) = value`` idiom for the case-insensitive SSO domain conflict lookup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ccess Use the same admin check the secrets UI uses (owner, admin permission, or org-admin) so owners and org-admins are not wrongly denied their own decrypted workspace secrets, while read-only members remain restricted to names only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ad in-memory recheck Address PR review: the SQL `lower(domain) = <normalized>` predicate already excludes rows that the in-memory `normalizeSSODomain(...) === domain` recheck claimed to catch, making that recheck dead/misleading code. Match on the canonical lower-cased domain and filter purely by ownership. Malformed legacy values (wildcards, schemes, ports) never match an email domain at sign-in, so excluding them is not a gap. Test DB mock now applies the lower() predicate so the casing-variant case is genuinely exercised. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
findConflictingWebhookPathOwner omitted the isActive filter that the runtime dispatcher (findAllWebhooksForPath) applies, so an inactive but non-archived webhook from another workflow (e.g. after undeploy or failure auto-disable) would permanently block any new deployment on that path even though it never receives deliveries. Align the guard with the runtime isActive + archivedAt filter; the earliest-owner runtime check remains the authoritative cross-tenant protection. Also trims verbose TSDoc on the webhook path-isolation helpers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
…nflict findConflictingWebhookPathOwner now joins workflow and filters isNull(workflow.archivedAt), matching the runtime dispatcher (findAllWebhooksForPath). A webhook on an archived workflow can never receive deliveries at runtime, so it must not block legitimate path reuse with a 409. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
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 237be12. Configure here.
…tate A KB file's owner is now the earliest document referencing its key regardless of state (active/archived/deleted/excluded); access is granted only when that owning document is still active. Closes the residual where an attacker could plant an active document to claim a file whose original document was archived or deleted.
Reverts the knowledge-base file-access work (origin-pinning / owner-pinning / origin allow-list in verifyKBFileAccess) and its test. The other hardening fixes (SSO domain registration, webhook path isolation, workspace env secrets, CSV export) are unchanged. apps/sim/app/api/files/authorization.ts is restored to its origin/staging baseline.
…flict check Self-hosters often register SSO user-scoped via the CLI script (no SSO_ORGANIZATION_ID). If they later enable organizations and reconfigure the same domain org-scoped through the UI, the conflict check previously treated their own user-scoped row as another tenant's and returned a misleading 409. Recognize the caller's own user-scoped provider as owned so that migration is allowed, while still blocking another user's or another org's domain. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Defer to a credential-based access model (separate change). Restores GET /api/workspaces/[id]/environment to main behavior and removes the test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… helper Extract findConflictingWebhookPathOwner to lib/webhooks/utils.server.ts as the single source of truth for cross-tenant path-collision detection, used by both webhook creation paths (deploy sync and the manual /api/webhooks route). This also repairs two latent issues in the manual route's previous inline check, which queried with limit(1) and only webhook.archivedAt: - limit(1) inspected one arbitrary row, so a same-workflow row could mask a foreign collision (false negative). The shared helper scans all matching rows. - It omitted isActive/workflow.archivedAt, so inactive or archived-workflow webhooks (which never receive deliveries) permanently blocked path reuse. The helper mirrors the runtime dispatcher's filter. Same-workflow webhook reuse for upsert is now a separate, explicit lookup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
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 9119b6a. Configure here.
Summary
Type of Change
Testing
Tested manually; unit tests added for each fix (run in CI — this workspace has no local node_modules)
Checklist