Skip to content

fix(security): supabase rpc path validation, ssh stream byte cap, storage quota coverage#4605

Merged
waleedlatif1 merged 12 commits into
stagingfrom
fix/sec2
May 15, 2026
Merged

fix(security): supabase rpc path validation, ssh stream byte cap, storage quota coverage#4605
waleedlatif1 merged 12 commits into
stagingfrom
fix/sec2

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

@waleedlatif1 waleedlatif1 commented May 14, 2026

Summary

Four security fixes across storage, SSH, and execution isolation:

1. Supabase vector_search path traversal
functionName was interpolated directly into the RPC URL without validation. Added validateDatabaseIdentifier (same guard used by all other Supabase tools) and encodeURIComponent as defense-in-depth. Prevents prompt-injected path traversal to Supabase admin endpoints via the RPC prefix.

2. SSH read-file-content stream byte cap
The route checked file size via SFTP stat before reading, but a hostile server could lie about the stat and stream more bytes than declared. Added a running totalBytes counter in the stream data handler that destroys the stream and rejects immediately on overflow. The stat pre-check is kept as an early-exit fast path.

3. Storage quota bypass
checkStorageQuota only ran for workspace and mothership contexts. User-driven uploads via knowledge-base, chat, copilot, and execution contexts bypassed billing limits entirely. Moved the quota check before the context branching so it applies to all non-exempt contexts. Exempt contexts (profile-pictures, workspace-logos, og-images) extracted into a shared QUOTA_EXEMPT_STORAGE_CONTEXTS constant in lib/uploads/shared/types.ts.

4. Workflow execution log cross-tenant write (route.ts + logging-session.ts)
An attacker who owns any workflow could call POST /api/workflows/[their-id]/log with a victim's executionId in the body. The route verified workflow ownership but not that the executionId belonged to it — allowing the attacker to overwrite another user's execution log.

  • Route pre-check: before creating a LoggingSession, SELECT to verify the executionId (if already in DB) belongs to the claimed workflow. Returns 404 on mismatch.
  • Query-level scoping (defense-in-depth): added AND workflow_id = ? to all UPDATE/SELECT WHERE clauses in LoggingSession — raw SQL marker persistence queries and Drizzle queries in flushAccumulatedCost/loadExistingCost — so an injected executionId results in a no-op rather than a cross-tenant write.

5. Env-var workspace membership guard (environment/utils.ts)
getPersonalAndWorkspaceEnv could be called with any userId/workspaceId pair and would return decrypted workspace secrets without verifying membership. The primary call sites already did the check, but the function had no internal guard.

  • Added checkWorkspaceAccess at the top of getPersonalAndWorkspaceEnv when workspaceId is provided. Throws if the user lacks access, preventing any code path from inadvertently exfiltrating another workspace's secrets.

Type of Change

  • Bug fix (security)

Test plan

  • Verify Supabase RPC rejects invalid function names (SQL injection / path traversal)
  • Verify SSH stream terminates early when bytes exceed the declared max
  • Verify knowledge-base / copilot / execution uploads hit quota limits
  • Verify a workflow owner cannot overwrite another workflow's execution log via a foreign executionId
  • Verify getPersonalAndWorkspaceEnv throws for non-member workspace
  • Verify normal execution logging and workspace env-var fetch still work

Checklist

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

@vercel
Copy link
Copy Markdown

vercel Bot commented May 14, 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 14, 2026 11:56pm

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 14, 2026

PR Summary

High Risk
Touches multiple security-sensitive code paths (uploads, credential access, SSH streaming, and workflow execution logging) and changes authorization/scoping logic; mistakes could block legitimate requests or leave bypasses. Adds DB query conditions and new guards that need careful review for correctness and performance.

Overview
Closes multiple security bypasses across tools and APIs.

Multipart uploads now enforce checkStorageQuota for all non-exempt storage contexts via shared QUOTA_EXEMPT_STORAGE_CONTEXTS, and the multipart endpoint no longer accepts the logs context (with new tests). The SSH read-file-content route now enforces a streaming byte cap (not just stat) to prevent oversized responses.

Workflow execution result logging adds a pre-check that an executionId belongs to the requested workflow, and LoggingSession now scopes its DB SELECT/UPDATE (including raw SQL marker updates and markExecutionAsFailed) by both executionId and workflowId (with new tests). Supabase vector_search validates functionName as a DB identifier and encodes it in the RPC URL, and the SharePoint site route switches to authorizeCredentialUse instead of manual session/DB credential resolution.

Also adds a workspace access guard to getPersonalAndWorkspaceEnv, and makes a small refactor in ToolInput to satisfy linting; biome.json is updated accordingly.

Reviewed by Cursor Bugbot for commit 1f7c652. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 14, 2026

Greptile Summary

This PR applies five targeted security hardening measures across the Sim application: Supabase RPC path traversal prevention, SSH stream byte-cap enforcement, storage quota coverage broadening, cross-tenant execution log write isolation, and workspace membership gating on env-var decryption.

  • Supabase/SSH/Storage: functionName is now validated via validateDatabaseIdentifier and URI-encoded before use in the RPC URL; the SSH SFTP stream enforces a live byte counter so a lying SFTP server cannot overflow memory; the multipart quota check is lifted before the context branch so knowledge-base, chat, copilot, and execution uploads now all hit the billing limit.
  • Log cross-tenant isolation: POST /api/workflows/[id]/log now pre-selects the DB row for the supplied executionId and rejects with 404 if it belongs to a different workflow; all LoggingSession UPDATE/SELECT queries additionally scope by workflow_id as defense-in-depth.
  • Env-var workspace guard: getPersonalAndWorkspaceEnv calls checkWorkspaceAccess at the top when a workspaceId is provided, closing the path where any userId/workspaceId pair could retrieve decrypted secrets without membership verification.

Confidence Score: 5/5

Safe to merge — all five security fixes are well-scoped, properly tested, and no regressions are introduced in the changed paths.

Each fix is narrow in scope and verified by new unit tests. The cross-tenant log guard adds a pre-check SELECT and tightens every WHERE clause with workflowId; the static markExecutionAsFailed signature change is completed across all call sites. The workspace membership guard in getPersonalAndWorkspaceEnv correctly propagates errors through the existing cache layer. The storage quota lift and SSH stream cap are straightforward additions with no observable side effects on exempt contexts or normal-sized reads.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/app/api/workflows/[id]/log/route.ts Adds a pre-check SELECT to verify executionId ownership before creating a LoggingSession; cross-tenant write is now blocked with a 404 before any state mutation occurs.
apps/sim/lib/logs/execution/logging-session.ts All UPDATE/SELECT WHERE clauses now scope by both workflowId and executionId; markExecutionAsFailed is updated to require workflowId as a non-optional parameter.
apps/sim/lib/environment/utils.ts Added checkWorkspaceAccess guard at the top of getPersonalAndWorkspaceEnv; error is correctly propagated to callers; cache eviction in getEffectiveDecryptedEnv handles the thrown error cleanly.
apps/sim/app/api/files/multipart/route.ts Quota check lifted before context branching to cover all non-exempt contexts; logs removed from ALLOWED_UPLOAD_CONTEXTS to block multipart writes to internal system context.
apps/sim/lib/uploads/shared/types.ts Added QUOTA_EXEMPT_STORAGE_CONTEXTS constant consolidating exemption logic; logs context correctly included in exempt set after previous-review follow-up.
apps/sim/app/api/tools/ssh/read-file-content/route.ts Live byte counter in stream data handler caps reads at maxBytes regardless of what the SFTP stat reported; stream is destroyed immediately and promise rejected on first overflow chunk.
apps/sim/tools/supabase/vector_search.ts functionName validated with validateDatabaseIdentifier and URI-encoded before interpolation into the RPC URL; throws on invalid identifier.
apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts Both markExecutionAsFailed call sites updated to pass pausedExecution.workflowId, completing the API signature change for the static method.

Reviews (5): Last reviewed commit: "fix(logging): scope completeWithCancella..." | Re-trigger Greptile

Comment thread apps/sim/app/api/files/multipart/route.ts Outdated
Comment thread apps/sim/lib/uploads/shared/types.ts
…-var workspace membership guard

Closes two cross-tenant vulnerabilities:

1. Workflow log cross-tenant write (route.ts + logging-session.ts):
   - Route: SELECT before creating LoggingSession to verify executionId belongs
     to the claimed workflowId; reject with 404 if owned by a different workflow.
   - LoggingSession: add workflow_id to all UPDATE/SELECT WHERE clauses
     (raw SQL marker queries, flushAccumulatedCost, loadExistingCost) so
     writes are a no-op if executionId was somehow injected.

2. Env-var workspace membership guard (environment/utils.ts):
   - getPersonalAndWorkspaceEnv now calls checkWorkspaceAccess when workspaceId
     is provided; throws if the userId is not a member, preventing any future
     caller from reading another workspace's decrypted secrets without
     explicit membership verification at the call site.
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/lib/uploads/shared/types.ts
Comment thread biome.json
…site route; scope markExecutionAsFailed by workflowId
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

The previous fix only checked userId equality for personal credentials and
workspace membership (via getUserEntityPermissions) for workspace credentials.
authorizeCredentialUse additionally enforces credentialMember access for
workspace-scoped credentials, matching the standard pattern used by all
other tool selector routes.
Comment thread apps/sim/lib/logs/execution/logging-session.ts Outdated
Making workflowId optional left a footgun — future callers could silently
omit it and the WHERE clause would degrade to executionId-only, losing the
cross-tenant scoping guarantee. All callers already supply workflowId, so
making it required (with string | undefined for the middle params to keep
call sites unchanged) closes the gap without touching any caller.
…x, and workflowId scoping

- log/route.test.ts: verifies cross-tenant executionId guard returns 404
  when the execution belongs to a different workflow, and passes for same
  workflow or fresh executions
- multipart/route.test.ts: verifies fileSize:0 no longer bypasses quota
  check and that the logs context is rejected at the endpoint level
- logging-session.test.ts: verifies markExecutionAsFailed scopes by both
  executionId and workflowId, and that the instance method forwards workflowId
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/lib/logs/execution/logging-session.ts
…ads by workflowId

Both SELECT queries that check execution status before writing a
terminal result were only filtering on executionId. Adds workflowId
to the WHERE clause so all seven reads and writes in LoggingSession
consistently scope by (workflowId, executionId).
@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 1f7c652. Configure here.

@waleedlatif1 waleedlatif1 merged commit d0519c1 into staging May 15, 2026
14 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/sec2 branch May 15, 2026 00:08
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