fix(workout-session): enforce auth and ownership in sync action (CWE-862/IDOR) - #245

📝 Description

This PR fixes a missing-authorization / IDOR vulnerability in the syncWorkoutSessionAction server action (src/features/workout-session/actions/sync-workout-sessions.action.ts).

The problem

The action was exported from the unauthenticated actionClient:

import { actionClient } from "@/shared/api/safe-actions";

export const syncWorkoutSessionAction = actionClient
  .schema(syncWorkoutSessionSchema)
  .action(async ({ parsedInput }) => {
    const { session } = parsedInput;
    // ...
    await prisma.workoutSession.upsert({
      where: { id: session.id },
      create: { ...sessionData, /* exercises, sets, etc. */ },
      update: { /* ... */ },
    });
  });

Two issues stacked:

1. No authentication gate — actionClient in src/shared/api/safe-actions.ts does not require a session, whereas authenticatedActionClient in the same file does (.use(async ({ next }) => { const user = await getUser(); return next({ ctx: { user } }); })). Any unauthenticated caller could invoke the action.
2. userId trusted from the request body — the userId used for the workoutSession write comes straight from parsedInput.session.userId. It was never compared against the caller's identity.

The only server-side check was prisma.user.findUnique({ where: { id: session.userId } }) — an existence check, not an ownership check. Anyone who knows or guesses another user's ID could:

• Create workout sessions attributed to that user, or
• Overwrite an existing session by supplying its id (the action does a prisma.workoutSession.upsert keyed on session.id, which replaces exercises/sets via deleteMany: {} + create).

This maps to CWE-862 (Missing Authorization) and IDOR.

The fix

Minimal, single-file change:

• Switch the import to authenticatedActionClient, which runs the existing getUser() middleware and populates ctx.user.
• Reject the request if session.userId !== ctx.user.id, before touching Prisma.
• The pre-existing prisma.user.findUnique existence check becomes redundant once ownership is enforced (the caller is the user, and their existence is already guaranteed by getUser()), so it's removed to keep the change tight.

Relevant diff:

-import { actionClient } from "@/shared/api/safe-actions";
+import { authenticatedActionClient } from "@/shared/api/safe-actions";
...
-export const syncWorkoutSessionAction = actionClient.schema(...).action(async ({ parsedInput }) => {
+export const syncWorkoutSessionAction = authenticatedActionClient
+  .schema(syncWorkoutSessionSchema)
+  .action(async ({ parsedInput, ctx }) => {
     const { session } = parsedInput;
+    if (session.userId !== ctx.user.id) {
+      return { serverError: ERROR_MESSAGES.USER_NOT_FOUND };
+    }

No schema changes, no migrations, no API-surface change for legitimate callers (the client already sends its own userId; that value now just has to match the authenticated session).

Proof of concept

Assuming a running dev server and the pre-fix code, the action is invoked through next-safe-action's HTTP transport:

# Attacker is authenticated as user A (or, pre-fix, not authenticated at all).
# Victim's user id is $VICTIM_ID (e.g. leaked via a public profile route).
curl -X POST 'https://<host>/' \
  -H 'Content-Type: application/json' \
  -H 'Next-Action: <server-action-id-for-syncWorkoutSessionAction>' \
  --cookie "$SESSION_COOKIE_FOR_A_OR_EMPTY" \
  -d '[{
        "session": {
          "id": "'$(uuidgen)'",
          "userId": "'$VICTIM_ID'",
          "status": "completed",
          "muscles": [],
          "exercises": [],
          "startedAt": "2025-01-01T00:00:00.000Z",
          "endedAt":   "2025-01-01T00:30:00.000Z"
        }
      }]'

Pre-fix: a row is written to WorkoutSession with userId = $VICTIM_ID. Supplying an existing session.id overwrites the victim's session (upsert update branch does exercises: { deleteMany: {} } and re-creates them).

Post-fix: request is rejected with serverError: USER_NOT_FOUND unless the caller is the victim. In-app, legitimate clients call syncWorkoutSessionAction({ session }) where session.userId already comes from the current user, so nothing breaks.

📋 Checklist

• My code follows the project conventions (uses the existing authenticatedActionClient primitive)
• This PR includes breaking changes
• I have updated documentation if necessary (n/a — internal action, behavior unchanged for legitimate callers)

🗃️ Prisma Migrations (if applicable)

• I have created a migration — not applicable, no schema change.
• I have tested the migration locally — not applicable.

Testing

• Confirmed via src/shared/api/safe-actions.ts that authenticatedActionClient throws ActionError("Session is required!") / "Session is not valid!" when no user is present, so unauthenticated calls now short-circuit before reaching the handler.
• Confirmed ctx.user.id is populated from server-side getUser(), so it cannot be spoofed by the client.
• Walked the happy path: an authenticated client whose payload contains its own userId continues to work — session.userId === ctx.user.id holds and the upsert proceeds unchanged.
• Type-checked the changed file; ctx is correctly inferred from the middleware.

Adversarial review

Before submitting, we tried to disprove this finding. Two potential mitigations were considered: (1) an upstream auth middleware that might have gated actionClient anyway — but the two clients are explicitly distinct in src/shared/api/safe-actions.ts, and only authenticatedActionClient chains getUser(); and (2) whether Prisma-level constraints would block cross-user writes — they don't, WorkoutSession.userId is a plain foreign key with no row-level policy. The prisma.user.findUnique existence check only proves the target user exists, not that the caller is them. The vulnerability is real and the fix closes it at the action boundary, which is the correct layer for this kind of authorization check in a next-safe-action codebase.

🔗 Related Issues

None filed — reporting via PR per the open-source disclosure norm.