fix(uploads): drop the stray 'use server' directive that enables Server Actions app-wide - #6335
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview
Reviewed by Cursor Bugbot for commit c39ddeb. Configure here. |
Greptile SummaryThe PR removes the upload utility module’s stray
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/uploads/utils/file-utils.server.ts | Removes the module-level Server Action directive without changing the upload utility implementations. |
| scripts/check-client-boundary-imports.ts | Extends the CI boundary checker to reject Server Action directives and consolidates directive parsing. |
Reviews (2): Last reviewed commit: "fix(scripts): match boundary directives ..." | Re-trigger Greptile
…er Actions app-wide `file-utils.server.ts` was the repo's only `'use server'` module, and the sole reason Next's `hasServerActions()` returned true. With actions registered, Next loses its early-404 escape hatch for Server Action requests — and it classifies a request as an action from headers alone, with no body inspection and no auth. Any unauthenticated `POST` with `Content-Type: multipart/form-data` to any App Router path therefore took the non-fetch action path, which bare-throws and surfaces as an HTTP 500. Nothing invokes these functions as Server Actions: every one of the ~77 importers is server-side, with zero `'use client'` importers. The directive was a misuse of `'use server'` where "server-only module" was meant — the `.server.ts` suffix already carries that convention. Extends check-client-boundary-imports.ts to fail on any `'use server'` directive so this cannot regress.
A directive keeps its meaning when a note follows it on the same line, so strip a trailing '//' or block comment before matching. Shared by the 'use client' and 'use server' detectors.
13d579a to
c39ddeb
Compare
|
@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 c39ddeb. Configure here.
Problem
Any unauthenticated
POSTwithContent-Type: multipart/form-datato any App Router path returns HTTP 500.Next decides a request is a Server Action from headers alone —
next/dist/server/app-render/server-action-request-meta.js:36, no body inspection, no auth. The fetch-style action path returns a graceful 404, but the non-fetch multipart path does a barethrow(action-handler.js:589,743) that becomes a 500.Next has an escape hatch at
action-handler.js:412: whenhasServerActions()is false it returns 404 early and none of this runs. Sim had exactly one'use server'file —apps/sim/lib/uploads/utils/file-utils.server.ts— and it was the sole reason that flag was true.Availability impact
~180 requests (60/min for 3 min from a single IP) is enough to drive the ALB 5xx alarm
...-alb-high-error-percentage(>5% for 3 consecutive 60s periods) and page on-call. It fired today at 17:22:16Z. This is an unauthenticated, zero-cost DoS against the alerting path.Auth-surface impact
'use server'also published every export of that module as a remotely invocable endpoint with no auth wrapper:downloadFileFromUrl,resolveInternalFileUrl,downloadFileFromStorage,downloadServableFileFromStorage,resolveFileInputToUrl. Several take a caller-supplied URL and fetch it — SSRF-shaped. Exploitability was not confirmed (Next's action IDs are build-derived and hard to guess), but the surface should not exist at all. Removing the directive removes it.Fix
Delete the directive. Nothing ever invoked these as Server Actions.
server-onlywas considered and rejected: it is not currently a dependency, is used nowhere in the repo, and the.server.tssuffix already carries the server-only convention here. Adding a dependency for a one-line deletion is not the minimal fix.Verification
Only
'use server'in the repo (rgoverapps/+packages/): 1 hit, this file.Zero
'use client'importers: all ~77 importers are route handlers, executor code, and other.server.tsmodules. NouseActionState/useFormState/<form action={...}>anywhere.Actions manifest — the load-bearing check.
.next/server/server-reference-manifest.json:After:
{"node":{},"edge":{},"encryptionKey":"..."}—hasServerActions()is now false, so Next takes the early-404 path and the multipart 500 is gone.(Both builds compiled successfully and then failed identically at page-data collection on
Missing DATABASE_URL— a local-env limitation, unrelated to this change and present on both sides. The manifest is written at compile time, so the evidence is valid.)Regression guard
scripts/check-client-boundary-imports.ts(already wired into CI asbun run check:client-boundary) now also fails on any'use server'directive — module prologue or inline in a function body — acrossapps/andpackages/. Directive detection was factored into a sharedleadingDirective()helper rather than duplicated.Confirmed the guard fails without the fix:
and passes with it.
Checks
bunx tsc --noEmit -p tsconfig.json— cleanbun run lint— 23/23 tasks passbunx vitest run lib/uploads/ providers/file-attachments.server.test.ts executor/utils/file-tool-processor.test.ts lib/execution/payloads/materialization.server.test.ts— 315 passed / 30 filesbun run check:client-boundary— passesNot verified