Skip to content

fix(workflows): harden workflow API auth routes#3559

Open
PlaneInABottle wants to merge 8 commits intosimstudioai:stagingfrom
PlaneInABottle:fix/workflow-api-auth-hardening
Open

fix(workflows): harden workflow API auth routes#3559
PlaneInABottle wants to merge 8 commits intosimstudioai:stagingfrom
PlaneInABottle:fix/workflow-api-auth-hardening

Conversation

@PlaneInABottle
Copy link

Summary

  • hardens workflow API auth and access handling across workflow lifecycle, deployment, and deployed-state routes
  • standardizes behavior across session, API-key, and internal callers
  • includes the final hardening/repair commit to address follow-up route regressions and align the route tests

Problem / current behavior

  • related workflow API routes were not enforcing auth/access through one consistent path
  • some workflow lifecycle and deployment routes supported hybrid/app auth, while neighboring routes still behaved like session-only paths or applied different access checks
  • deploy, deployed, deployments, deployment-version, revert, status, and workflow update/delete flows could diverge on authorization, actor resolution, or regression behavior

Proposed fix / behavior after

  • extend apps/sim/app/api/workflows/middleware.ts so workflow access validation can be driven by explicit action, deployment, and internal-secret options
  • switch apps/sim/app/api/workflows/[id]/route.ts, deploy/route.ts, deployed/route.ts, status/route.ts, and deployments/** routes onto the shared workflow access validation path
  • allow user-backed API-key callers on workflow lifecycle and deployment routes where appropriate, while still requiring a resolved actor identity for audit-bearing mutations
  • preserve internal-caller support where intended and remove route-by-route auth drift
  • include the final hardening/repair follow-up commit (fix: harden workflow API auth routes) to repair route regressions and update the matching route tests

Why this matters

  • makes workflow API auth behavior predictable across related route surfaces
  • reduces route-specific 401/403/regression cases for UI, SDK/server-to-server, and internal workflow callers
  • keeps audit/public-access checks tied to the resolved caller identity instead of route-specific assumptions

Files changed / surfaces affected

  • apps/sim/app/api/workflows/middleware.ts
  • apps/sim/app/api/workflows/[id]/route.ts
  • apps/sim/app/api/workflows/[id]/deploy/route.ts
  • apps/sim/app/api/workflows/[id]/deployed/route.ts
  • apps/sim/app/api/workflows/[id]/status/route.ts
  • apps/sim/app/api/workflows/[id]/deployments/route.ts
  • apps/sim/app/api/workflows/[id]/deployments/[version]/route.ts
  • apps/sim/app/api/workflows/[id]/deployments/[version]/revert/route.ts
  • matching route tests for workflow-by-id, deploy, deployed, status, deployments list, deployment version, and revert flows

Validation / tests run

  • previously reported on this branch: targeted workflow route tests passed
  • previously reported on this branch: app typecheck passed
  • analyzer approved after the final hardening/repair follow-up

Notes / non-goals / follow-ups

  • scoped to auth/access consistency and regression repair for workflow lifecycle and deployment APIs
  • does not intentionally change workflow business logic outside auth/access resolution, actor handling, and the related route-test repairs
  • includes the final hardening/repair follow-up commit in this PR branch

@cursor
Copy link

cursor bot commented Mar 13, 2026

PR Summary

Medium Risk
Touches authentication/authorization and audit attribution across multiple workflow API routes; regressions could block deploy/undeploy/updates or weaken access checks if option defaults are misused.

Overview
Unifies workflow lifecycle and deployment-related API routes (/deploy, /deployed, /status, /deployments, /deployments/[version], /revert, and workflow PUT/DELETE) behind the shared validateWorkflowAccess middleware, replacing route-specific permission checks and enabling consistent hybrid (session/API key/internal) access handling.

Extends validateWorkflowAccess to accept explicit options (action, requireDeployment, allowInternalSecret) so routes can request read/write/admin checks consistently, and updates audit logging to derive actorName/actorEmail from session auth only via new getAuditActorMetadata helper.

Adds/updates Vitest route tests to assert API-key callers can perform deploy/undeploy/activate/revert/delete where allowed and that public-API restrictions and audit actor IDs use the resolved hybrid userId.

Written by Cursor Bugbot for commit a264ee9. This will update automatically on new commits. Configure here.

@vercel
Copy link

vercel bot commented Mar 13, 2026

Someone is attempting to deploy a commit to the Sim Team on Vercel.

A member of the Team first needs to authorize it.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 13, 2026

Greptile Summary

This PR hardens the workflow API auth layer by introducing a shared validateWorkflowAccess middleware and migrating seven route surfaces ([id], deploy, deployed, status, deployments, deployments/[version], deployments/[version]/revert) onto it, replacing a patchwork of session-only and ad-hoc auth checks with a consistent session / API-key / internal-secret path.

Key observations:

  • The overall direction is correct and the standardisation is a genuine improvement over the previous divergent per-route auth.
  • Double authorization in route.ts: Both the DELETE and PUT handlers call validateWorkflowAccess (which internally runs authorizeWorkflowByWorkspacePermission) and then immediately call authorizeWorkflowByWorkspacePermission a second time with the same arguments. The redundant second call wastes a DB round-trip on every request and introduces a small race-condition window where the two checks could return different results.
  • Double session validation in deploy/route.ts: validateLifecycleAdminAccess calls validateWorkflowAccess for all callers (which already completes auth + permission checks for session users), and then, for session callers, discards the result and re-runs validateWorkflowPermissions in full. The first check's result is silently thrown away for this auth type.
  • Dead allowInternalSecret: false option in deployed/route.ts: The allowInternalSecret field is only evaluated inside the requireDeployment: true code path in middleware. Specifying it with requireDeployment: false has no effect and misleads readers.
  • The allowInternalSecret default in middleware.ts is coupled to requireDeployment unnecessarily, which reinforces the same misunderstanding.
  • Test coverage is good — every changed route has a corresponding test file, and the tests verify the new middleware is called with the expected options.

Confidence Score: 3/5

  • Auth hardening direction is correct but two routes contain redundant/double authorization logic that should be resolved before merging.
  • The shared middleware and its adoption across routes is well-structured, and the test suite covers the new paths. However, the double authorizeWorkflowByWorkspacePermission in the [id] route and the double session-validation in the deploy route are real logic issues that add unnecessary DB load and create subtle race-condition windows in security-critical code paths.
  • apps/sim/app/api/workflows/[id]/route.ts (DELETE/PUT double authorization) and apps/sim/app/api/workflows/[id]/deploy/route.ts (double session validation in validateLifecycleAdminAccess).

Important Files Changed

Filename Overview
apps/sim/app/api/workflows/middleware.ts New centralized validateWorkflowAccess function with good structure; the allowInternalSecret default coupling to requireDeployment is misleading and should be cleaned up.
apps/sim/app/api/workflows/[id]/route.ts DELETE and PUT handlers perform a redundant second authorizeWorkflowByWorkspacePermission call after validateWorkflowAccess already authorized the user, creating unnecessary DB round-trips and a race-condition window.
apps/sim/app/api/workflows/[id]/deploy/route.ts Session-authenticated callers are double-validated: validateWorkflowAccess completes auth and permission checks, then validateLifecycleAdminAccess discards the result and re-runs validateWorkflowPermissions for session users.
apps/sim/app/api/workflows/[id]/deployed/route.ts Clean internal-caller bypass with JWT verification; the allowInternalSecret: false option passed to validateWorkflowAccess is dead code and should be removed.
apps/sim/app/api/workflows/[id]/status/route.ts Clean migration to shared validateWorkflowAccess; no issues found.
apps/sim/app/api/workflows/[id]/deployments/route.ts Straightforward use of shared middleware for deployment list read access; no issues found.
apps/sim/app/api/workflows/[id]/deployments/[version]/route.ts Good use of per-action permission escalation (read vs admin for activation); validateDeploymentVersionLifecycleAccess wrapper is clean but slightly redundant given it only forwards to validateWorkflowAccess unchanged.
apps/sim/app/api/workflows/[id]/deployments/[version]/revert/route.ts Clean admin-only guard via shared middleware; actor identity checked before mutation; no issues found.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Incoming Request] --> B{Auth Type?}

    B -->|Bearer JWT| C[verifyInternalToken\ndeployed/route only]
    C -->|valid| D[Skip validateWorkflowAccess\nload deployed state directly]
    C -->|invalid| E[validateWorkflowAccess]

    B -->|Session / API Key / Internal Secret| E

    E --> F[getWorkflowById]
    F -->|not found| G[404 Workflow not found]
    F -->|found, no workspaceId| H[403 Personal workflow deprecated]

    F -->|requireDeployment: false| I[checkHybridAuth]
    I -->|fail / no userId| J[401 Unauthorized]
    I -->|success + userId| K[authorizeWorkflowByWorkspacePermission\naction: read / write / admin]
    K -->|denied| L[403 / 404 Access denied]
    K -->|allowed| M[Return workflow + auth]

    F -->|requireDeployment: true| N{isDeployed?}
    N -->|false| O[403 Workflow not deployed]
    N -->|true| P{Internal Secret header?}
    P -->|valid + allowInternalSecret| Q[Return workflow — no userId]
    P -->|missing / invalid| R[Check X-Api-Key header]
    R -->|missing| S[401 API key required]
    R -->|present| T[authenticateApiKeyFromHeader\nworkspace or personal]
    T -->|fail| U[401 Invalid API key]
    T -->|success| V[updateApiKeyLastUsed\nReturn workflow]

    M --> W[Route handler\nDELETE / PUT / POST / PATCH / GET]
    V --> W
    Q --> W
Loading

Comments Outside Diff (1)

  1. apps/sim/app/api/workflows/[id]/route.ts, line 170-192 (link)

    Redundant double authorization check

    validateWorkflowAccess at line 150 already calls checkHybridAuth followed by authorizeWorkflowByWorkspacePermission with the same action: 'admin'. If that first check passes, the userId is guaranteed to have workspace admin permission. Re-running authorizeWorkflowByWorkspacePermission here with the same userId and action creates:

    1. A redundant DB round-trip on every request.
    2. A window for a race condition — if the workspace membership changes between the two calls, the second check could return a different result, leading to inconsistent behaviour (e.g. the first check passes but the second returns authorization.allowed = false, causing a 403 even though the user was legitimately authorised).

    The workflow object is already available via validation.workflow and the access is already verified, so the second authorizeWorkflowByWorkspacePermission call (and the canDelete guard that depends on it) should be removed in favour of using validation.workflow directly:

    const workflowData = validation.workflow
    if (!workflowData) {
      return NextResponse.json({ error: 'Workflow not found' }, { status: 404 })
    }
    

    The same pattern applies to the PUT handler at lines 403–424.

Last reviewed commit: 73c1328

@PlaneInABottle PlaneInABottle force-pushed the fix/workflow-api-auth-hardening branch from 73c1328 to 888a246 Compare March 13, 2026 08:05
PlaneInABottle and others added 6 commits March 13, 2026 11:24
Co-authored-by: GitHub Copilot <github-copilot[bot]@users.noreply.github.com>
Co-authored-by: GitHub Copilot <github-copilot[bot]@users.noreply.github.com>
Co-authored-by: GitHub Copilot <github-copilot[bot]@users.noreply.github.com>
@PlaneInABottle PlaneInABottle force-pushed the fix/workflow-api-auth-hardening branch from 888a246 to aab58cb Compare March 13, 2026 08:27
@vercel
Copy link

vercel bot commented Mar 13, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Mar 13, 2026 9:09am

Request Review

Use the shared audit actor helper consistently so workflow deletion matches deploy behavior and remove the redundant deploy wrapper raised in review.
Copy link

@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.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Call validateWorkflowAccess directly in workflow deployment lifecycle routes and clean up the related test helper formatting raised in review.
@icecrasher321 icecrasher321 self-assigned this Mar 13, 2026
@icecrasher321
Copy link
Collaborator

@PlaneInABottle this PR might have to wait till our v0.6 release. Lot of auth changes there that might overwrite this potentially

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.

2 participants