Streamline self-hosting and Zero local startup#461
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a "core self-host" local-file storage mode, feature-gates OCR/audio/document conversion when local storage is used, provides env/config helpers and Zero/startup tooling, restructures dev scripts to run Zero alongside the web server, and introduces local filesystem upload/retrieval endpoints and tooling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Web as Next.js API
participant LocalFS as Local Filesystem\n(UPLOADS_DIR)
participant Supabase as Supabase Storage
participant Zero as Zero Cache
rect rgba(200,230,255,0.5)
Client->>Web: POST /api/upload-url or /api/upload-file
Web-->>Client: { mode: "local", uploadUrl: "/api/upload-file" } or signed URL
end
alt mode == local
Client->>Web: POST /api/upload-file (file binary)
Web->>LocalFS: write file to UPLOADS_DIR
Web-->>Client: public `GET /api/files/{owner}/{name}` URL
Web->>Zero: (optionally) notify Zero for workspace sync
else mode == supabase
Client->>Supabase: upload via signed URL (PUT)
Supabase-->>Web: stored file metadata (via callback or URL)
Web->>Zero: (optionally) notify Zero for workspace sync
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 22 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/app/api/files/[...path]/route.ts">
<violation number="1" location="src/app/api/files/[...path]/route.ts:71">
P2: Reading the entire file into memory with `readFile` will cause high memory usage for large media/zip uploads. Consider using `createReadStream` to stream the response, which also enables future support for HTTP `Range` requests (required for video seeking).</violation>
<violation number="2" location="src/app/api/files/[...path]/route.ts:72">
P1: Serving SVG (and other active content types) from the same origin without a restrictive `Content-Security-Policy` header enables stored XSS. Add a CSP header to all file responses to prevent script execution inside served content.</violation>
</file>
<file name="setup.sh">
<violation number="1" location="setup.sh:49">
P2: Escape sed replacement metacharacters in `set_env_value`; unescaped `&` in env values can corrupt `.env` writes.</violation>
</file>
<file name="src/app/api/upload-url/route.ts">
<violation number="1" location="src/app/api/upload-url/route.ts:43">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
New API behavior branch for local storage is added without tests validating the changed response path, increasing regression risk.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return NextResponse.json({ error: "File not found" }, { status: 404 }); | ||
| } | ||
|
|
||
| const file = await readFile(filePath); |
There was a problem hiding this comment.
P2: Reading the entire file into memory with readFile will cause high memory usage for large media/zip uploads. Consider using createReadStream to stream the response, which also enables future support for HTTP Range requests (required for video seeking).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/api/files/[...path]/route.ts, line 71:
<comment>Reading the entire file into memory with `readFile` will cause high memory usage for large media/zip uploads. Consider using `createReadStream` to stream the response, which also enables future support for HTTP `Range` requests (required for video seeking).</comment>
<file context>
@@ -0,0 +1,82 @@
+ return NextResponse.json({ error: "File not found" }, { status: 404 });
+ }
+
+ const file = await readFile(filePath);
+ return new NextResponse(file, {
+ headers: {
</file context>
| const isOfficeUpload = convertUrl !== null; | ||
| const storageMode = getStorageMode(); | ||
|
|
||
| if (storageMode === "local") { |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
New API behavior branch for local storage is added without tests validating the changed response path, increasing regression risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/api/upload-url/route.ts, line 43:
<comment>New API behavior branch for local storage is added without tests validating the changed response path, increasing regression risk.</comment>
<file context>
@@ -40,6 +38,14 @@ async function handlePOST(request: NextRequest) {
const isOfficeUpload = convertUrl !== null;
+ const storageMode = getStorageMode();
+
+ if (storageMode === "local") {
+ return NextResponse.json({
+ mode: "local",
</file context>
Greptile SummaryThis PR introduces a zero-dependency local self-host path: local file storage with a new authenticated
Confidence Score: 3/5Not safe to merge as-is due to a stored XSS vector in the new local file serving route. A P1 security issue (stored XSS via SVG inline serving) caps the score at 4; the presence of a second real functional defect (office upload partial failure in local mode) pulls it to 3. src/app/api/files/[...path]/route.ts (SVG XSS), src/lib/uploads/client-upload.ts (office local-mode partial failure)
|
| Filename | Overview |
|---|---|
| src/app/api/files/[...path]/route.ts | New authenticated local-file serving route; path traversal protection is solid but SVG files are served inline (image/svg+xml) without Content-Disposition, enabling stored XSS. |
| src/lib/uploads/client-upload.ts | Added local-mode fallback path; office document uploads in local mode will always fail at the conversion step after the file is already saved, leaving orphaned files. |
| scripts/dev-zero.mjs | New script that validates Zero env vars and spawns zero-cache; ZERO_COOKIE_DOMAIN is missing from REQUIRED_ENV_KEYS despite being required per docs. |
| src/lib/self-host-config.ts | New config module providing storage mode detection and Supabase/Zero config guards; logic is clean and the fallback-to-supabase heuristic preserves existing deployments correctly. |
| src/app/api/upload-file/route.ts | Added local storage path using saveFileLocally; allows SVG uploads which combined with the new file-serving route creates a stored XSS vector. |
| src/app/api/delete-file/route.ts | Added local deletion path with proper .. and / guards before join; path traversal protections are correct. |
| src/app/api/upload-url/route.ts | Returns mode=local in local storage mode so the client can fall back to /api/upload-file; clean and correct. |
| src/app/api/audio/process/route.ts | Added usesLocalStorage guard that blocks audio transcription in local mode with a clear error; correct. |
| src/app/api/ocr/start/route.ts | Added usesLocalStorage guard blocking OCR in local mode; correct. |
| src/app/api/office-conversion/convert-to-pdf/route.ts | Added usesLocalStorage guard blocking document conversion in local mode; correct, though this causes the orphaned-file issue in client-upload.ts. |
| src/lib/zero/client.ts | Added getZeroConfigError check before constructing the Zero instance to give a clear error when NEXT_PUBLIC_ZERO_SERVER is missing. |
| src/lib/zero/provider.tsx | Added an in-app error UI when Zero is misconfigured; clean fallback for self-hosters. |
| src/lib/supabase-client.ts | Delegated Supabase config check to isSupabaseClientConfigured(); setRealtimeAuth now no-ops when Supabase is unconfigured. |
| src/hooks/workspace/use-workspace-presence.ts | Added isSupabaseClientConfigured guard to skip presence channel setup; cleanup handles null-ref correctly when Supabase is absent. |
| package.json | Changed dev script to start Next.js, AI SDK devtools, and Zero concurrently; -s first means any process crash kills the rest. |
| setup.sh | Added configure_zero_env and set_env_value helpers; creates Zero publication and sets sensible defaults for local dev. |
| .env.example | Reorganised and expanded with Zero, local storage, and optional integration vars; clearer for self-hosters. |
Comments Outside Diff (1)
-
scripts/dev-zero.mjs, line 454-464 (link)ZERO_COOKIE_DOMAINabsent from required-key validationZERO_COOKIE_DOMAINis listed as a required Zero environment variable in bothSELF_HOSTING.mdand.env.example, andsetup.shensures it defaults tolocalhost— but it is not inREQUIRED_ENV_KEYShere. If a developer skipssetup.shor manually configures.envand omits it, Zero will start without the cookie domain, potentially causing session/cookie issues without a clear startup error to guide them.
Reviews (1): Last reviewed commit: "Fix storage mode fallback and local file..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ".webp": "image/webp", | ||
| ".zip": "application/zip", |
There was a problem hiding this comment.
The route serves .svg files with Content-Type: image/svg+xml but no Content-Disposition: attachment header. SVG files can embed <script> elements, so a user who uploads a crafted SVG (which passes the image/svg+xml allowlist in upload-file) and convinces another authenticated user to open /api/files/malicious.svg directly in their browser gets JavaScript execution in the app's origin. Adding Content-Disposition: attachment for SVGs forces a download instead of inline rendering, or you can remove .svg from CONTENT_TYPES so it falls back to application/octet-stream.
| if (isOfficeUpload) { | ||
| return convertOfficeUpload(result.filename, result.url, file.name); |
There was a problem hiding this comment.
In local mode, uploadViaApiRoute saves the file successfully, then convertOfficeUpload immediately POSTs to /api/office-conversion/convert-to-pdf, which is blocked in local mode with a 400 error (getUnsupportedLocalStorageMessage). The result is that the original office file is written to the uploads directory (orphaned) and the upload throws an error. This could be addressed by skipping convertOfficeUpload when the storage mode is local, or by blocking office document uploads at the upload-url stage so the user gets an early, actionable error instead of a partial failure.
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| return new NextResponse(file, { | ||
| headers: { | ||
| "Content-Type": getContentType(relativePath), | ||
| "Cache-Control": "private, max-age=0, must-revalidate", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
The file serving endpoint serves .svg files with Content-Type: image/svg+xml, and SVG uploads are accepted by the upload route (image/svg+xml is in the allowed MIME types list). SVG files can embed <script> tags and inline JavaScript event handlers. When an authenticated user navigates to /api/files/malicious.svg, the browser renders the SVG and executes embedded scripts in the app's origin context (localhost:3000). There are no Content-Security-Policy headers configured in the app. This allows a stored XSS attack: an authenticated attacker uploads a crafted SVG, and any other authenticated user who views it has their session compromised.
// src/app/api/files/[...path]/route.ts
const file = await readFile(filePath);
return new NextResponse(file, {
headers: {
"Content-Type": getContentType(relativePath), // image/svg+xml for .svg
"Cache-Control": "private, max-age=0, must-revalidate",
},
});Fix: add Content-Disposition: attachment for SVG files (forces download instead of render), or serve SVGs as application/octet-stream, or add a Content-Security-Policy: sandbox response header.
| return new NextResponse(file, { | |
| headers: { | |
| "Content-Type": getContentType(relativePath), | |
| "Cache-Control": "private, max-age=0, must-revalidate", | |
| }, | |
| }); | |
| const contentType = getContentType(relativePath); | |
| const responseHeaders: Record<string, string> = { | |
| "Content-Type": contentType, | |
| "Cache-Control": "private, max-age=0, must-revalidate", | |
| }; | |
| if (contentType === "image/svg+xml") { | |
| responseHeaders["Content-Disposition"] = `attachment; filename="${path[path.length - 1]}"`; | |
| } | |
| return new NextResponse(file, { | |
| headers: responseHeaders, | |
| }); |
| console.info(`[PDF_UPLOAD] Local upload fallback: ${total.toFixed(0)}ms`); | ||
| } | ||
|
|
||
| if (isOfficeUpload) { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
When urlData.mode === "local" and the file is an office document, the client first saves the file via uploadViaApiRoute (succeeds — file written to UPLOADS_DIR), then calls convertOfficeUpload() which POSTs to /api/office-conversion/convert-to-pdf. That endpoint now returns HTTP 400 unconditionally in local storage mode because the usesLocalStorage() guard blocks it. The convertOfficeUpload call throws, so uploadFileDirect throws, and the caller sees an error — but the raw .docx/.xlsx file is already saved on disk. The file is orphaned and the user gets a confusing error after a seemingly successful upload.
// src/lib/uploads/client-upload.ts
if (urlData.mode === "local") {
const result = await uploadViaApiRoute(file); // file saved
if (isOfficeUpload) {
return convertOfficeUpload(result.filename, result.url, file.name); // always 400
}Fix: either skip conversion and return the raw upload result in local mode, or reject office document uploads early when in local mode (e.g., check urlData.mode and isOfficeUpload before calling uploadViaApiRoute).
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/app/api/files/[...path]/route.ts">
<violation number="1" location="src/app/api/files/[...path]/route.ts:50">
P2: Escape/sanitize the filename before inserting it into `Content-Disposition` to avoid malformed or injectable header values.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| if (contentType === "image/svg+xml") { | ||
| const filename = relativePath.split("/").at(-1) ?? "download.svg"; | ||
| headers.set("Content-Disposition", `attachment; filename="${filename}"`); |
There was a problem hiding this comment.
P2: Escape/sanitize the filename before inserting it into Content-Disposition to avoid malformed or injectable header values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/api/files/[...path]/route.ts, line 50:
<comment>Escape/sanitize the filename before inserting it into `Content-Disposition` to avoid malformed or injectable header values.</comment>
<file context>
@@ -37,6 +37,22 @@ function isPathInsideDirectory(filePath: string, directory: string): boolean {
+
+ if (contentType === "image/svg+xml") {
+ const filename = relativePath.split("/").at(-1) ?? "download.svg";
+ headers.set("Content-Disposition", `attachment; filename="${filename}"`);
+ }
+
</file context>
| headers.set("Content-Disposition", `attachment; filename="${filename}"`); | |
| headers.set("Content-Disposition", `attachment; filename="${filename.replace(/["\\\r\n]/g, "_")}"`); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/api/delete-file/route.ts (1)
25-81:⚠️ Potential issue | 🟠 MajorNo ownership/authorization check on file deletion.
The handler authenticates the session but never verifies that
session.useractually owns or has access to the file being deleted. Any signed-in user can delete any other user's files just by knowing the filename or URL. This is pre-existing behavior on the Supabase branch (which uses the service-role key and trusts the caller), but it's now extended to local-mode deletes — and self-hosted deployments that share a single Postgres + uploads dir between multiple users will be exposed.If a per-file ownership table doesn't exist yet, at minimum consider scoping deletes to objects whose path encodes the user id, or adding a workspace/owner check before
unlink/supabase.storage.remove.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/delete-file/route.ts` around lines 25 - 81, The DELETE handler currently authenticates via auth.api.getSession but never verifies file ownership, allowing any authenticated user to delete arbitrary files; update DELETE to enforce authorization by resolving the owner before removing: when using local mode (getStorageMode() === "local") validate that the resolved filename (from searchParams or extractFilename(url)) is scoped to session.user.id (for example by requiring filenames to be under a per-user subdirectory like `${session.user.id}/...` or by checking against a per-file ownership table), and likewise when using remote storage call into your metadata/ownership check before calling unlink(filePath) or supabase.storage.remove; ensure you handle missing ownership with a 403 response and only proceed to unlink/remove when the owner matches session.user.id.
🧹 Nitpick comments (8)
src/app/api/audio/process/route.ts (1)
25-32: Consider a more specific status code for capability gating.A 400 implies a malformed client request, but here the server is rejecting a feature unavailable in this deployment mode.
501 Not Implemented(or503) communicates "this deployment cannot do that" more precisely and avoids confusing clients that retry on 400 only for input fixes. Same applies to the matching gates inoffice-conversion/convert-to-pdfandpdf/ocrfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/audio/process/route.ts` around lines 25 - 32, The capability-gating currently returns a 400 in the usesLocalStorage() check (see usesLocalStorage(), getUnsupportedLocalStorageMessage(), and the NextResponse.json call) but should use a more accurate HTTP status; update the response status from 400 to 501 (Not Implemented) for this audio transcription gate and make the identical change in the matching gates in office-conversion/convert-to-pdf and pdf/ocr so clients receive a clear "feature not implemented in this deployment" status.src/lib/supabase-client.ts (1)
44-51: Silent no-op is reasonable; consider a dev-mode log.Returning early when Supabase isn't configured is correct for self-host mode, but a silent no-op can mask misconfigurations in environments that should have Supabase set. A dev-only
console.warnwould surface accidental misconfig without being noisy in production. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/supabase-client.ts` around lines 44 - 51, In setRealtimeAuth, currently returning early when isSupabaseClientConfigured() is false is silent; add a dev-only warning to surface accidental misconfiguration by checking the environment (e.g., process.env.NODE_ENV === 'development' or a similar dev flag) and calling console.warn with a short message that includes the function name and that Supabase is not configured; keep the early return but emit the warning before returning. Reference: setRealtimeAuth, isSupabaseClientConfigured, getSupabaseClient.src/app/api/delete-file/route.ts (1)
65-75: Minor: TOCTOU onexistsSync+unlink.The
existsSynccheck followed byunlinkhas a race window. Simpler and race-free is to callunlinkdirectly and treatENOENTas the "already deleted" case.♻️ Proposed simplification
- const uploadsDir = process.env.UPLOADS_DIR || join(process.cwd(), "uploads"); - const filePath = join(uploadsDir, filename); - - if (!existsSync(filePath)) { - return NextResponse.json({ - success: true, - message: "File not found (may have been deleted already)", - }); - } - - await unlink(filePath); - - return NextResponse.json({ - success: true, - message: "File deleted successfully", - }); + const uploadsDir = process.env.UPLOADS_DIR || join(process.cwd(), "uploads"); + const filePath = join(uploadsDir, filename); + + try { + await unlink(filePath); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "ENOENT") { + return NextResponse.json({ + success: true, + message: "File not found (may have been deleted already)", + }); + } + throw err; + } + + return NextResponse.json({ success: true, message: "File deleted successfully" });The
existsSyncimport can then be dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/delete-file/route.ts` around lines 65 - 75, Replace the TOCTOU pattern that uses existsSync before unlink by calling unlink(filePath) directly and handling the ENOENT error as the "already deleted" success case: construct uploadsDir and filePath as before (symbols: uploadsDir, filePath), attempt await unlink(filePath) inside a try/catch, on catch check err.code === "ENOENT" and return the same NextResponse.json success message for missing files, otherwise rethrow or return an error response; remove the existsSync usage/import since it is no longer needed.src/app/api/upload-file/route.ts (2)
27-31: Memory pressure: full file buffered in RAM.
file.arrayBuffer()materializes the entire upload (up to 50 MB permaxSize) into aBufferbefore writing. Under concurrent uploads in a small self-hosted Node process, this can spike memory significantly (N × 50 MB). For local-disk targets, streamingfile.stream()to acreateWriteStream(...)withpipelineis more memory-friendly and only marginally more code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/upload-file/route.ts` around lines 27 - 31, The current upload handler uses file.arrayBuffer() and Buffer.from(...) which loads the entire file into memory before write, causing memory pressure; change the implementation to stream the incoming file to disk by piping file.stream() into a fs.createWriteStream(join(uploadsDir, filename)) using stream.pipeline/promises to handle errors and backpressure, remove the Buffer.from(...)/writeFile path, and keep returning the same URL; reference the symbols file.arrayBuffer(), file.stream(), writeFile, Buffer.from, uploadsDir, filename, and use pipeline (or pipelinePromise) to perform the streaming write safely.
20-32:saveFileLocallytrusts caller-suppliedfilename— document the contract.The helper writes wherever
filenameresolves underuploadsDir. Today the only caller passes a sanitized${timestamp}-${random}-${sanitizedName}, but if a future caller passes an unsanitized value containing..or absolute-path segments,path.joinwould happily traverse outsideuploadsDir. Consider either:
- Validating inside the helper (
if (filename.includes("/") || filename.includes("..") || isAbsolute(filename)) throw …), or- Adding a JSDoc comment that states the caller must pass a sanitized filename, and resolve+verify the final path is contained in
uploadsDirbefore writing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/upload-file/route.ts` around lines 20 - 32, The saveFileLocally helper currently writes using caller-supplied filename which can lead to directory traversal; update saveFileLocally (and its uploadsDir/filename handling) to either validate the filename upfront (reject if it contains "/" or "\\" or ".." or Path.isAbsolute(filename)) or, after computing the target path, resolve it and assert the resolved path is inside uploadsDir (e.g., compare resolvedTarget.startsWith(resolvedUploadsDir)) and throw a clear error if not; also add a short JSDoc on saveFileLocally describing that filenames must be sanitized if you choose the validation approach..env.example (1)
28-28: Consider leavingZERO_ADMIN_PASSWORDblank by default.Shipping a default value (even one named "change-me-before-sharing") risks self-hosters copying
.env.exampleto.envwithout rotating it, especially if the app boots successfully with the default. Forcing them to set a value (and failing fast on empty/change-me*) is safer.-ZERO_ADMIN_PASSWORD=change-me-before-sharing +# ZERO_ADMIN_PASSWORD must be set to a strong, unique value before exposing this instance. +ZERO_ADMIN_PASSWORD=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example at line 28, The .env.example currently sets ZERO_ADMIN_PASSWORD=change-me-before-sharing which may be copied into production; change the example to leave ZERO_ADMIN_PASSWORD blank (ZERO_ADMIN_PASSWORD=) and add a runtime check (e.g., in your config loader function validateEnv or initApp/startup code that reads env vars) to fail fast if ZERO_ADMIN_PASSWORD is empty or matches the default "change-me" pattern; ensure the validation error message clearly instructs the operator to set a secure admin password before starting.src/app/api/files/[...path]/route.ts (1)
77-81: Path-normalization mismatch with upload/delete routes.
UPLOADS_DIRis normalized viaresolve()here but the write side (src/app/api/upload-file/route.ts:21) and delete side (src/app/api/delete-file/route.ts:65) use barejoin(process.cwd(), "uploads")/join(uploadsDir, filename). If a self-hoster setsUPLOADS_DIRto a relative path that gets evaluated from a different CWD (e.g., a worker process), or to a symlinked directory, the GET handler may resolve to a path that doesn't match where uploads were written. Centralize the directory resolution in a single helper (e.g., addgetUploadsDir()toself-host-config.ts) and use it from all three routes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/files/`[...path]/route.ts around lines 77 - 81, The GET handling normalizes UPLOADS_DIR with resolve(...) while upload and delete routes use join(...), causing mismatches for relative paths or symlinks; create a single helper getUploadsDir() in self-host-config.ts that returns the fully resolved uploads directory (using resolve and any symlink handling you need) and replace all direct uses of resolve(...)/join(...) in src/app/api/files/[...path]/route.ts, src/app/api/upload-file/route.ts and src/app/api/delete-file/route.ts to call getUploadsDir(); also update uses of uploadsDir, filePath and join(uploadsDir, filename) so isPathInsideDirectory(filePath, getUploadsDir()) is used consistently.setup.sh (1)
370-382: ValidateZERO_APP_PUBLICATIONSbefore splicing into SQL.
${ZERO_APP_PUBLICATIONS}is interpolated directly intoCREATE PUBLICATION ${ZERO_APP_PUBLICATIONS} FOR TABLES IN SCHEMA public;. If a user edits.envwith a value containing whitespace, semicolons, or quotes, the statement will either fail confusingly or execute unintended SQL. Local-dev impact is low, but an identifier sanity check (alphanumeric + underscore) gives a clearer error.🛡️ Defensive check
ZERO_APP_PUBLICATIONS=$(get_env_value "ZERO_APP_PUBLICATIONS") ZERO_APP_PUBLICATIONS=${ZERO_APP_PUBLICATIONS:-zero_pub} +if ! [[ "$ZERO_APP_PUBLICATIONS" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then + echo -e "${RED}ERROR: ZERO_APP_PUBLICATIONS must be a valid Postgres identifier (got: '${ZERO_APP_PUBLICATIONS}')${RESET}" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.sh` around lines 370 - 382, The ZERO_APP_PUBLICATIONS value read via get_env_value should be validated before being injected into the CREATE PUBLICATION SQL to prevent whitespace/semicolon/quote or other malicious/invalid identifiers; add a sanity check (e.g., allow only [A-Za-z0-9_]+) on the ZERO_APP_PUBLICATIONS variable and if it fails, print a clear error and exit (or fall back to a safe default) before executing the psql/docker-compose exec calls in the blocks that run the CREATE PUBLICATION command; ensure the check covers both branches (USE_DOCKER_POSTGRES true and false) and reference ZERO_APP_PUBLICATIONS and the CREATE PUBLICATION statement when adding the validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/dev-zero.mjs`:
- Around line 40-60: The loadEnvFiles function sets process.env keys to empty
strings when a higher-priority file contains KEY=, blocking fallback values;
update the logic in loadEnvFiles (which iterates ENV_FILES and calls
parseEnvValue) to skip setting process.env[key] when the parsed value is an
empty string (e.g. if value === "" continue), so that subsequent lower-priority
files can provide a non-empty value; keep the existing guards for missing keys
and already-set env vars intact.
- Around line 76-95: The spawnZero function fails on recent Windows Node.js
versions because child_process.spawn() needs shell: true to execute .cmd/.bat;
update spawnZero (the spawn call creating child) to pass shell: true when
process.platform === "win32" (e.g., set options.shell = true only for Windows)
while keeping stdio: "inherit" and env: process.env so that pnpm.cmd runs
without EINVAL on affected Node versions.
In `@SELF_HOSTING.md`:
- Around line 27-33: Docs show the deprecated docker-compose (v1) command;
update the example that currently lists "docker-compose up -d postgres" to
either use the Compose v2 "docker compose up -d postgres" (preferred) or show
both variants, and optionally note that setup.sh already falls back between them
so users on recent Docker installs should run the v2 form; change the command in
the code block in SELF_HOSTING.md accordingly.
In `@setup.sh`:
- Around line 132-138: The cookie-forwarding booleans are being unconditionally
overwritten by set_env_value; change the calls that set
ZERO_MUTATE_FORWARD_COOKIES and ZERO_QUERY_FORWARD_COOKIES to use
ensure_env_value instead so existing user overrides are preserved, while leaving
the URL/db entries (ZERO_UPSTREAM_DB, ZERO_QUERY_URL, ZERO_MUTATE_URL) as
set_env_value; update the two lines referencing ZERO_MUTATE_FORWARD_COOKIES and
ZERO_QUERY_FORWARD_COOKIES to call ensure_env_value with the same default
"true".
In `@src/app/api/files/`[...path]/route.ts:
- Around line 40-54: The Content-Disposition filename taken from relativePath in
getDownloadHeaders is inserted raw and must be sanitized: replace/remove CR/LF,
double quotes, backslashes, and control chars, normalize or strip to a safe
ASCII subset (e.g., [A-Za-z0-9._-]) with a sensible fallback like
"download.svg", and then set both a safe quoted filename and a RFC‑5987 encoded
UTF-8 filename* parameter (percent-encode non-ASCII) when creating the header so
Unicode names are preserved safely; update getDownloadHeaders to compute a
sanitized filename variable and use it for headers.set("Content-Disposition",
`attachment; filename="safeName"; filename*=UTF-8''encodedName`) while ensuring
no unescaped characters remain.
- Around line 60-91: The handler currently trusts any authenticated session and
serves files from UPLOADS_DIR without ownership checks—update the upload flow to
write files under a per-user namespace (e.g., in the upload route that calls
join(uploadsDir, filename) change to join(uploadsDir, session.user.id,
fileId/filename)), update delete logic similarly, and modify this download
handler (the function using session, params, relativePath, uploadsDir, filePath,
isPathInsideDirectory, existsSync, readFile, getDownloadHeaders) to enforce
ownership by verifying the first path segment equals session.user.id (or perform
a DB lookup for ownership) before resolving and returning the file; ensure
directory creation/permission checks when creating per-user folders and adjust
any path validation to still forbid ".." or empty segments.
In `@src/app/api/upload-file/route.ts`:
- Around line 118-138: The local storage flow saves files to a flat UPLOADS_DIR
making files accessible to any authenticated user; update the upload handler to
namespace local filenames by the uploader's id (e.g., prefix filename with
`${session.user.id}/`) by retrieving session.user.id where getStorageMode() ===
"local" and passing the namespaced path into saveFileLocally (or modify
saveFileLocally to accept a userId). Also update the local file-serving route
(/api/files/[...path]) and the delete-file route to allow and validate paths
containing a single leading user segment (remove the blanket rejection of '/'
and enforce that requested path starts with the authenticated session.user.id)
so files are only served/deleted by their owner.
In `@src/lib/zero/provider.tsx`:
- Around line 21-34: The early return rendering the "Zero is required..." UI
sits between hook calls (useEffect and useMemo) which violates the Rules of
Hooks; move that conditional UI return so all hooks (the useEffect block and the
const zero = useMemo(...) call) are declared/executed first, then after those
hooks check zeroConfigError (the value from getZeroConfigError()) and return the
error UI if present, preserving the same JSX and semantics.
---
Outside diff comments:
In `@src/app/api/delete-file/route.ts`:
- Around line 25-81: The DELETE handler currently authenticates via
auth.api.getSession but never verifies file ownership, allowing any
authenticated user to delete arbitrary files; update DELETE to enforce
authorization by resolving the owner before removing: when using local mode
(getStorageMode() === "local") validate that the resolved filename (from
searchParams or extractFilename(url)) is scoped to session.user.id (for example
by requiring filenames to be under a per-user subdirectory like
`${session.user.id}/...` or by checking against a per-file ownership table), and
likewise when using remote storage call into your metadata/ownership check
before calling unlink(filePath) or supabase.storage.remove; ensure you handle
missing ownership with a 403 response and only proceed to unlink/remove when the
owner matches session.user.id.
---
Nitpick comments:
In @.env.example:
- Line 28: The .env.example currently sets
ZERO_ADMIN_PASSWORD=change-me-before-sharing which may be copied into
production; change the example to leave ZERO_ADMIN_PASSWORD blank
(ZERO_ADMIN_PASSWORD=) and add a runtime check (e.g., in your config loader
function validateEnv or initApp/startup code that reads env vars) to fail fast
if ZERO_ADMIN_PASSWORD is empty or matches the default "change-me" pattern;
ensure the validation error message clearly instructs the operator to set a
secure admin password before starting.
In `@setup.sh`:
- Around line 370-382: The ZERO_APP_PUBLICATIONS value read via get_env_value
should be validated before being injected into the CREATE PUBLICATION SQL to
prevent whitespace/semicolon/quote or other malicious/invalid identifiers; add a
sanity check (e.g., allow only [A-Za-z0-9_]+) on the ZERO_APP_PUBLICATIONS
variable and if it fails, print a clear error and exit (or fall back to a safe
default) before executing the psql/docker-compose exec calls in the blocks that
run the CREATE PUBLICATION command; ensure the check covers both branches
(USE_DOCKER_POSTGRES true and false) and reference ZERO_APP_PUBLICATIONS and the
CREATE PUBLICATION statement when adding the validation.
In `@src/app/api/audio/process/route.ts`:
- Around line 25-32: The capability-gating currently returns a 400 in the
usesLocalStorage() check (see usesLocalStorage(),
getUnsupportedLocalStorageMessage(), and the NextResponse.json call) but should
use a more accurate HTTP status; update the response status from 400 to 501 (Not
Implemented) for this audio transcription gate and make the identical change in
the matching gates in office-conversion/convert-to-pdf and pdf/ocr so clients
receive a clear "feature not implemented in this deployment" status.
In `@src/app/api/delete-file/route.ts`:
- Around line 65-75: Replace the TOCTOU pattern that uses existsSync before
unlink by calling unlink(filePath) directly and handling the ENOENT error as the
"already deleted" success case: construct uploadsDir and filePath as before
(symbols: uploadsDir, filePath), attempt await unlink(filePath) inside a
try/catch, on catch check err.code === "ENOENT" and return the same
NextResponse.json success message for missing files, otherwise rethrow or return
an error response; remove the existsSync usage/import since it is no longer
needed.
In `@src/app/api/files/`[...path]/route.ts:
- Around line 77-81: The GET handling normalizes UPLOADS_DIR with resolve(...)
while upload and delete routes use join(...), causing mismatches for relative
paths or symlinks; create a single helper getUploadsDir() in self-host-config.ts
that returns the fully resolved uploads directory (using resolve and any symlink
handling you need) and replace all direct uses of resolve(...)/join(...) in
src/app/api/files/[...path]/route.ts, src/app/api/upload-file/route.ts and
src/app/api/delete-file/route.ts to call getUploadsDir(); also update uses of
uploadsDir, filePath and join(uploadsDir, filename) so
isPathInsideDirectory(filePath, getUploadsDir()) is used consistently.
In `@src/app/api/upload-file/route.ts`:
- Around line 27-31: The current upload handler uses file.arrayBuffer() and
Buffer.from(...) which loads the entire file into memory before write, causing
memory pressure; change the implementation to stream the incoming file to disk
by piping file.stream() into a fs.createWriteStream(join(uploadsDir, filename))
using stream.pipeline/promises to handle errors and backpressure, remove the
Buffer.from(...)/writeFile path, and keep returning the same URL; reference the
symbols file.arrayBuffer(), file.stream(), writeFile, Buffer.from, uploadsDir,
filename, and use pipeline (or pipelinePromise) to perform the streaming write
safely.
- Around line 20-32: The saveFileLocally helper currently writes using
caller-supplied filename which can lead to directory traversal; update
saveFileLocally (and its uploadsDir/filename handling) to either validate the
filename upfront (reject if it contains "/" or "\\" or ".." or
Path.isAbsolute(filename)) or, after computing the target path, resolve it and
assert the resolved path is inside uploadsDir (e.g., compare
resolvedTarget.startsWith(resolvedUploadsDir)) and throw a clear error if not;
also add a short JSDoc on saveFileLocally describing that filenames must be
sanitized if you choose the validation approach.
In `@src/lib/supabase-client.ts`:
- Around line 44-51: In setRealtimeAuth, currently returning early when
isSupabaseClientConfigured() is false is silent; add a dev-only warning to
surface accidental misconfiguration by checking the environment (e.g.,
process.env.NODE_ENV === 'development' or a similar dev flag) and calling
console.warn with a short message that includes the function name and that
Supabase is not configured; keep the early return but emit the warning before
returning. Reference: setRealtimeAuth, isSupabaseClientConfigured,
getSupabaseClient.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6e7de30-061e-4f9a-93c5-e729ecf2dc82
📒 Files selected for processing (23)
.env.exampleCONTRIBUTING.mdREADME.mdSELF_HOSTING.mdpackage.jsonscripts/dev-zero.mjssetup.shsrc/app/api/audio/process/route.tssrc/app/api/delete-file/route.tssrc/app/api/files/[...path]/route.tssrc/app/api/ocr/start/route.tssrc/app/api/office-conversion/convert-to-pdf/route.tssrc/app/api/pdf/ocr/route.tssrc/app/api/upload-file/route.tssrc/app/api/upload-url/route.tssrc/components/chat/ChatProvider.tsxsrc/components/chat/ChatRuntimes.tsxsrc/hooks/workspace/use-workspace-presence.tssrc/lib/self-host-config.tssrc/lib/supabase-client.tssrc/lib/uploads/client-upload.tssrc/lib/zero/client.tssrc/lib/zero/provider.tsx
💤 Files with no reviewable changes (1)
- src/components/chat/ChatProvider.tsx
| function loadEnvFiles() { | ||
| for (const file of ENV_FILES) { | ||
| const fullPath = resolve(process.cwd(), file); | ||
| if (!existsSync(fullPath)) continue; | ||
|
|
||
| const content = readFileSync(fullPath, "utf8"); | ||
| for (const line of content.split(/\r?\n/)) { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed || trimmed.startsWith("#")) continue; | ||
|
|
||
| const eqIndex = trimmed.indexOf("="); | ||
| if (eqIndex <= 0) continue; | ||
|
|
||
| const key = trimmed.slice(0, eqIndex).trim(); | ||
| if (!key || process.env[key] !== undefined) continue; | ||
|
|
||
| const value = parseEnvValue(trimmed.slice(eqIndex + 1)); | ||
| process.env[key] = value; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Empty values in higher-priority env files block fallback to lower-priority files.
loadEnvFiles writes empty strings into process.env (e.g. KEY= in .env.development.local), and the process.env[key] !== undefined precondition then prevents the same key from being read from .env.local/.env. This trips up the missing-key check (which uses ?.trim() and treats empty as missing) even when a real value is present in a fallback file. Consider skipping lines where the parsed value is empty so subsequent files can supply a non-empty value.
♻️ Suggested fix
- const value = parseEnvValue(trimmed.slice(eqIndex + 1));
- process.env[key] = value;
+ const value = parseEnvValue(trimmed.slice(eqIndex + 1));
+ if (value === "") continue;
+ process.env[key] = value;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function loadEnvFiles() { | |
| for (const file of ENV_FILES) { | |
| const fullPath = resolve(process.cwd(), file); | |
| if (!existsSync(fullPath)) continue; | |
| const content = readFileSync(fullPath, "utf8"); | |
| for (const line of content.split(/\r?\n/)) { | |
| const trimmed = line.trim(); | |
| if (!trimmed || trimmed.startsWith("#")) continue; | |
| const eqIndex = trimmed.indexOf("="); | |
| if (eqIndex <= 0) continue; | |
| const key = trimmed.slice(0, eqIndex).trim(); | |
| if (!key || process.env[key] !== undefined) continue; | |
| const value = parseEnvValue(trimmed.slice(eqIndex + 1)); | |
| process.env[key] = value; | |
| } | |
| } | |
| } | |
| function loadEnvFiles() { | |
| for (const file of ENV_FILES) { | |
| const fullPath = resolve(process.cwd(), file); | |
| if (!existsSync(fullPath)) continue; | |
| const content = readFileSync(fullPath, "utf8"); | |
| for (const line of content.split(/\r?\n/)) { | |
| const trimmed = line.trim(); | |
| if (!trimmed || trimmed.startsWith("#")) continue; | |
| const eqIndex = trimmed.indexOf("="); | |
| if (eqIndex <= 0) continue; | |
| const key = trimmed.slice(0, eqIndex).trim(); | |
| if (!key || process.env[key] !== undefined) continue; | |
| const value = parseEnvValue(trimmed.slice(eqIndex + 1)); | |
| if (value === "") continue; | |
| process.env[key] = value; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/dev-zero.mjs` around lines 40 - 60, The loadEnvFiles function sets
process.env keys to empty strings when a higher-priority file contains KEY=,
blocking fallback values; update the logic in loadEnvFiles (which iterates
ENV_FILES and calls parseEnvValue) to skip setting process.env[key] when the
parsed value is an empty string (e.g. if value === "" continue), so that
subsequent lower-priority files can provide a non-empty value; keep the existing
guards for missing keys and already-set env vars intact.
| function spawnZero() { | ||
| const pnpmCmd = process.platform === "win32" ? "pnpm.cmd" : "pnpm"; | ||
| const forwardedArgs = process.argv.slice(2); | ||
| if (forwardedArgs[0] === "--") { | ||
| forwardedArgs.shift(); | ||
| } | ||
| const args = ["exec", "zero-cache", ...forwardedArgs]; | ||
| const child = spawn(pnpmCmd, args, { | ||
| stdio: "inherit", | ||
| env: process.env, | ||
| }); | ||
|
|
||
| child.on("exit", (code, signal) => { | ||
| if (signal) { | ||
| process.kill(process.pid, signal); | ||
| return; | ||
| } | ||
| process.exit(code ?? 0); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js child_process spawn .cmd .bat EINVAL CVE-2024-27980 shell true requirement
💡 Result:
CVE-2024-27980 is a Windows-specific command-injection bug in Node.js child_process.spawn() / spawnSync() when executing batch files (.bat / .cmd) with attacker-influenced arguments (“BatBadBut”). Even with shell: false, Windows batch execution involves cmd.exe, and Node’s argument handling allowed injection. [1]
Why you now see spawn EINVAL for .cmd / .bat
As part of Node’s April 10, 2024 security fix, Node introduced a breaking change on Windows:
- Node.js will error with
EINVALif you pass a.bator.cmdtospawn()/spawnSync()without{ shell: true }. [1]
That is why “it works only with shell: true” (or started failing after an upgrade).
Does this mean “you must use shell: true”?
Only if you have fully sanitized the input (Node explicitly calls this out). If inputs are sanitized, you can pass { shell: true } to avoid the EINVAL. [1]
If any part of the command/args comes from users or other untrusted sources, shell: true reintroduces classic shell-injection risk (because it runs through a shell).
Safer alternatives (recommended)
- Prefer spawning an
.exedirectly (avoid.cmd/.batwrappers). - If you must run a batch wrapper, consider invoking
cmd.exeyourself with fixed flags and strict allowlisting/escaping of any variable parts (still treat as high-risk).
About --security-revert=CVE-2024-27980
Node notes you can revert the patch with --security-revert=CVE-2024-27980, but they strongly advise against it. [1]
Follow-up: CVE-2024-36138 (incomplete fix bypass)
Node later stated CVE-2024-27980 was an incomplete fix and released updates for CVE-2024-36138 (another Windows batch handling injection path) in July 2024. So you should also ensure you’re patched for that newer issue. [2]
Sources: Node.js April 10, 2024 security release (CVE-2024-27980 / EINVAL behavior / guidance on shell: true) [1]; Node.js July 8, 2024 security release (CVE-2024-36138) [2]; Wiz vulnerability writeup (overview/impact) [3].
Spawning pnpm.cmd on Windows requires shell: true for Node.js ≥18.20.2/20.12.2/21.7.3.
Node.js security fix CVE-2024-27980 (April 2024) blocks child_process.spawn() from executing .bat / .cmd files on Windows unless shell: true is set, throwing EINVAL. This breaks pnpm dev on Windows with recent Node versions.
Fix
const child = spawn(pnpmCmd, args, {
stdio: "inherit",
env: process.env,
+ shell: process.platform === "win32",
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function spawnZero() { | |
| const pnpmCmd = process.platform === "win32" ? "pnpm.cmd" : "pnpm"; | |
| const forwardedArgs = process.argv.slice(2); | |
| if (forwardedArgs[0] === "--") { | |
| forwardedArgs.shift(); | |
| } | |
| const args = ["exec", "zero-cache", ...forwardedArgs]; | |
| const child = spawn(pnpmCmd, args, { | |
| stdio: "inherit", | |
| env: process.env, | |
| }); | |
| child.on("exit", (code, signal) => { | |
| if (signal) { | |
| process.kill(process.pid, signal); | |
| return; | |
| } | |
| process.exit(code ?? 0); | |
| }); | |
| } | |
| function spawnZero() { | |
| const pnpmCmd = process.platform === "win32" ? "pnpm.cmd" : "pnpm"; | |
| const forwardedArgs = process.argv.slice(2); | |
| if (forwardedArgs[0] === "--") { | |
| forwardedArgs.shift(); | |
| } | |
| const args = ["exec", "zero-cache", ...forwardedArgs]; | |
| const child = spawn(pnpmCmd, args, { | |
| stdio: "inherit", | |
| env: process.env, | |
| shell: process.platform === "win32", | |
| }); | |
| child.on("exit", (code, signal) => { | |
| if (signal) { | |
| process.kill(process.pid, signal); | |
| return; | |
| } | |
| process.exit(code ?? 0); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/dev-zero.mjs` around lines 76 - 95, The spawnZero function fails on
recent Windows Node.js versions because child_process.spawn() needs shell: true
to execute .cmd/.bat; update spawnZero (the spawn call creating child) to pass
shell: true when process.platform === "win32" (e.g., set options.shell = true
only for Windows) while keeping stdio: "inherit" and env: process.env so that
pnpm.cmd runs without EINVAL on affected Node versions.
| ```bash | ||
| pnpm install | ||
| cp .env.example .env | ||
| docker-compose up -d postgres | ||
| pnpm db:push | ||
| pnpm dev | ||
| ``` |
There was a problem hiding this comment.
Mention Compose v2 (docker compose) syntax.
docker-compose (v1) reached end-of-life in mid-2023; fresh Docker installs only ship docker compose (v2). setup.sh already falls back between both, but this manual snippet only documents v1, which will fail on common environments.
📝 Suggested wording
pnpm install
cp .env.example .env
-docker-compose up -d postgres
+docker compose up -d postgres # or: docker-compose up -d postgres
pnpm db:push
pnpm dev🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SELF_HOSTING.md` around lines 27 - 33, Docs show the deprecated
docker-compose (v1) command; update the example that currently lists
"docker-compose up -d postgres" to either use the Compose v2 "docker compose up
-d postgres" (preferred) or show both variants, and optionally note that
setup.sh already falls back between them so users on recent Docker installs
should run the v2 form; change the command in the code block in SELF_HOSTING.md
accordingly.
| set_env_value "ZERO_UPSTREAM_DB" "$db_url" | ||
| set_env_value "ZERO_QUERY_URL" "${app_url}/api/zero/query" | ||
| set_env_value "ZERO_MUTATE_URL" "${app_url}/api/zero/mutate" | ||
| set_env_value "ZERO_MUTATE_FORWARD_COOKIES" "true" | ||
| set_env_value "ZERO_QUERY_FORWARD_COOKIES" "true" | ||
| ensure_env_value "ZERO_APP_PUBLICATIONS" "zero_pub" | ||
| ensure_env_value "ZERO_APP_ID" "zero_local_dev" |
There was a problem hiding this comment.
set_env_value unconditionally overwrites cookie-forwarding flags on every rerun.
ZERO_MUTATE_FORWARD_COOKIES and ZERO_QUERY_FORWARD_COOKIES are set with set_env_value (unconditional), so any user override (e.g., a self-hoster who set them to false for a custom auth mode) is silently reset to true if they rerun ./setup.sh. URL fields make sense to refresh because they derive from app_url / db_url, but the boolean toggles should use ensure_env_value.
♻️ Suggested change
set_env_value "ZERO_UPSTREAM_DB" "$db_url"
set_env_value "ZERO_QUERY_URL" "${app_url}/api/zero/query"
set_env_value "ZERO_MUTATE_URL" "${app_url}/api/zero/mutate"
- set_env_value "ZERO_MUTATE_FORWARD_COOKIES" "true"
- set_env_value "ZERO_QUERY_FORWARD_COOKIES" "true"
+ ensure_env_value "ZERO_MUTATE_FORWARD_COOKIES" "true"
+ ensure_env_value "ZERO_QUERY_FORWARD_COOKIES" "true"
ensure_env_value "ZERO_APP_PUBLICATIONS" "zero_pub"
ensure_env_value "ZERO_APP_ID" "zero_local_dev"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set_env_value "ZERO_UPSTREAM_DB" "$db_url" | |
| set_env_value "ZERO_QUERY_URL" "${app_url}/api/zero/query" | |
| set_env_value "ZERO_MUTATE_URL" "${app_url}/api/zero/mutate" | |
| set_env_value "ZERO_MUTATE_FORWARD_COOKIES" "true" | |
| set_env_value "ZERO_QUERY_FORWARD_COOKIES" "true" | |
| ensure_env_value "ZERO_APP_PUBLICATIONS" "zero_pub" | |
| ensure_env_value "ZERO_APP_ID" "zero_local_dev" | |
| set_env_value "ZERO_UPSTREAM_DB" "$db_url" | |
| set_env_value "ZERO_QUERY_URL" "${app_url}/api/zero/query" | |
| set_env_value "ZERO_MUTATE_URL" "${app_url}/api/zero/mutate" | |
| ensure_env_value "ZERO_MUTATE_FORWARD_COOKIES" "true" | |
| ensure_env_value "ZERO_QUERY_FORWARD_COOKIES" "true" | |
| ensure_env_value "ZERO_APP_PUBLICATIONS" "zero_pub" | |
| ensure_env_value "ZERO_APP_ID" "zero_local_dev" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@setup.sh` around lines 132 - 138, The cookie-forwarding booleans are being
unconditionally overwritten by set_env_value; change the calls that set
ZERO_MUTATE_FORWARD_COOKIES and ZERO_QUERY_FORWARD_COOKIES to use
ensure_env_value instead so existing user overrides are preserved, while leaving
the URL/db entries (ZERO_UPSTREAM_DB, ZERO_QUERY_URL, ZERO_MUTATE_URL) as
set_env_value; update the two lines referencing ZERO_MUTATE_FORWARD_COOKIES and
ZERO_QUERY_FORWARD_COOKIES to call ensure_env_value with the same default
"true".
| function getDownloadHeaders(relativePath: string): Headers { | ||
| const contentType = getContentType(relativePath); | ||
| const headers = new Headers({ | ||
| "Cache-Control": "private, max-age=0, must-revalidate", | ||
| "Content-Type": contentType, | ||
| "X-Content-Type-Options": "nosniff", | ||
| }); | ||
|
|
||
| if (contentType === "image/svg+xml") { | ||
| const filename = relativePath.split("/").at(-1) ?? "download.svg"; | ||
| headers.set("Content-Disposition", `attachment; filename="${filename}"`); | ||
| } | ||
|
|
||
| return headers; | ||
| } |
There was a problem hiding this comment.
Sanitize the filename interpolated into Content-Disposition.
relativePath.split("/").at(-1) is interpolated unescaped into attachment; filename="...". Today filenames are server-generated, so this is theoretical — but if the upload pipeline ever accepts user-supplied names (or someone places files in UPLOADS_DIR out-of-band), embedded ", \, or non-ASCII will produce a malformed header (CR/LF will throw via undici). Consider stripping to a safe ASCII subset and using filename*=UTF-8''… per RFC 6266 for Unicode names.
♻️ Minimal hardening
if (contentType === "image/svg+xml") {
- const filename = relativePath.split("/").at(-1) ?? "download.svg";
- headers.set("Content-Disposition", `attachment; filename="${filename}"`);
+ const raw = relativePath.split("/").at(-1) ?? "download.svg";
+ const safe = raw.replace(/[^\w.\-]+/g, "_");
+ headers.set("Content-Disposition", `attachment; filename="${safe}"`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/files/`[...path]/route.ts around lines 40 - 54, The
Content-Disposition filename taken from relativePath in getDownloadHeaders is
inserted raw and must be sanitized: replace/remove CR/LF, double quotes,
backslashes, and control chars, normalize or strip to a safe ASCII subset (e.g.,
[A-Za-z0-9._-]) with a sensible fallback like "download.svg", and then set both
a safe quoted filename and a RFC‑5987 encoded UTF-8 filename* parameter
(percent-encode non-ASCII) when creating the header so Unicode names are
preserved safely; update getDownloadHeaders to compute a sanitized filename
variable and use it for headers.set("Content-Disposition", `attachment;
filename="safeName"; filename*=UTF-8''encodedName`) while ensuring no unescaped
characters remain.
| const session = await auth.api.getSession({ | ||
| headers: await headers(), | ||
| }); | ||
|
|
||
| if (!session) { | ||
| return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); | ||
| } | ||
|
|
||
| const { path } = await params; | ||
| const relativePath = path.join("/"); | ||
| if ( | ||
| !relativePath || | ||
| path.some((segment) => !segment || segment === "." || segment === "..") | ||
| ) { | ||
| return NextResponse.json({ error: "Invalid file path" }, { status: 400 }); | ||
| } | ||
|
|
||
| const uploadsDir = resolve(process.env.UPLOADS_DIR || join(process.cwd(), "uploads")); | ||
| const filePath = resolve(uploadsDir, relativePath); | ||
| if (!isPathInsideDirectory(filePath, uploadsDir)) { | ||
| return NextResponse.json({ error: "Invalid file path" }, { status: 400 }); | ||
| } | ||
|
|
||
| if (!existsSync(filePath)) { | ||
| return NextResponse.json({ error: "File not found" }, { status: 404 }); | ||
| } | ||
|
|
||
| const file = await readFile(filePath); | ||
| return new NextResponse(file, { | ||
| headers: getDownloadHeaders(relativePath), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Authorization gap: any authenticated user can read any uploaded file.
The handler verifies a session exists but never checks that the requested file belongs to (or is shared with) session.user. Combined with the upload route writing files flat under UPLOADS_DIR (src/app/api/upload-file/route.ts:21 uses join(uploadsDir, filename) without a per-user namespace), every signed-in user can fetch every other user's uploads if they obtain or guess a filename. Even with random IDs this fails defense-in-depth and breaks multi-tenant expectations for self-hosters who add a second account.
Recommended fix: scope storage by user (e.g., <userId>/<fileId>) in the upload route, then enforce that the first path segment matches session.user.id here, or look up file ownership in DB before serving.
🛡️ Sketch of an ownership check
if (!session) {
return NextResponse.json({ error: "Unauthorized" }, { status: 401 });
}
const { path } = await params;
const relativePath = path.join("/");
if (
!relativePath ||
path.some((segment) => !segment || segment === "." || segment === "..")
) {
return NextResponse.json({ error: "Invalid file path" }, { status: 400 });
}
+ // Files are stored under <userId>/<filename>; enforce ownership.
+ if (path[0] !== session.user.id) {
+ return NextResponse.json({ error: "Forbidden" }, { status: 403 });
+ }
+
const uploadsDir = resolve(process.env.UPLOADS_DIR || join(process.cwd(), "uploads"));This requires a parallel change in src/app/api/upload-file/route.ts and src/app/api/delete-file/route.ts to write/delete under the user-scoped subpath.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/files/`[...path]/route.ts around lines 60 - 91, The handler
currently trusts any authenticated session and serves files from UPLOADS_DIR
without ownership checks—update the upload flow to write files under a per-user
namespace (e.g., in the upload route that calls join(uploadsDir, filename)
change to join(uploadsDir, session.user.id, fileId/filename)), update delete
logic similarly, and modify this download handler (the function using session,
params, relativePath, uploadsDir, filePath, isPathInsideDirectory, existsSync,
readFile, getDownloadHeaders) to enforce ownership by verifying the first path
segment equals session.user.id (or perform a DB lookup for ownership) before
resolving and returning the file; ensure directory creation/permission checks
when creating per-user folders and adjust any path validation to still forbid
".." or empty segments.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/api/delete-file/route.ts (1)
94-104: Align defense-in-depth with the upload/download routes.
src/app/api/upload-file/route.ts(lines 47-58) andsrc/app/api/files/[...path]/route.ts(line 86-90) bothresolve()UPLOADS_DIRand verify the finalfilePathis inside it viaisPathInsideDirectorybefore touching the filesystem. This branch onlyjoins and relies entirely onparseOwnedLocalPathto guard traversal. It happens to be safe today, but losing that defense-in-depth layer here means any future relaxation ofparseOwnedLocalPath(e.g., supporting nested paths) silently becomes exploitable as a delete-anywhere primitive.♻️ Suggested change
- const uploadsDir = process.env.UPLOADS_DIR || join(process.cwd(), "uploads"); - const filePath = join(uploadsDir, parsedPath.ownerId, parsedPath.fileName); + const uploadsDir = resolve(process.env.UPLOADS_DIR || join(process.cwd(), "uploads")); + const filePath = resolve(uploadsDir, parsedPath.ownerId, parsedPath.fileName); + + if (!isPathInsideDirectory(filePath, uploadsDir)) { + return NextResponse.json({ error: "Invalid filename" }, { status: 400 }); + }…with matching imports:
-import { join } from "node:path"; +import { join, resolve, sep } from "node:path"; @@ +function isPathInsideDirectory(filePath: string, directory: string): boolean { + return filePath === directory || filePath.startsWith(`${directory}${sep}`); +}Better yet, consider lifting
getUploadsDir()/isPathInsideDirectory()/validateStorageSegment()into a shared helper (e.g.,src/lib/local-storage.ts) so all three routes share one implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/delete-file/route.ts` around lines 94 - 104, The delete route currently builds filePath by join() and relies solely on parseOwnedLocalPath; add the same defense-in-depth as upload/download routes by resolving the uploads directory (use the same logic as getUploadsDir or create it), call isPathInsideDirectory(uploadDir, filePath) to verify filePath is contained before touching the FS, and reject the request if the check fails; consider moving getUploadsDir(), isPathInsideDirectory(), and validateStorageSegment into a shared helper (e.g., src/lib/local-storage.ts) and use those helpers in route.ts alongside parseOwnedLocalPath to ensure consistent validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/files/`[...path]/route.ts:
- Around line 96-99: The code currently uses readFile(filePath) which buffers
the entire file into memory; replace this with a streaming response by using
fs.stat(filePath) to get size and fs.createReadStream(filePath) as the response
body, then return new NextResponse(stream, { headers: {
...getDownloadHeaders(relativePath), 'Content-Length': stat.size } }) (or adjust
getDownloadHeaders to accept/merge content-length); update references to
readFile and ensure error handling for missing files uses the same flow.
---
Nitpick comments:
In `@src/app/api/delete-file/route.ts`:
- Around line 94-104: The delete route currently builds filePath by join() and
relies solely on parseOwnedLocalPath; add the same defense-in-depth as
upload/download routes by resolving the uploads directory (use the same logic as
getUploadsDir or create it), call isPathInsideDirectory(uploadDir, filePath) to
verify filePath is contained before touching the FS, and reject the request if
the check fails; consider moving getUploadsDir(), isPathInsideDirectory(), and
validateStorageSegment into a shared helper (e.g., src/lib/local-storage.ts) and
use those helpers in route.ts alongside parseOwnedLocalPath to ensure consistent
validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 627db899-fda7-409f-81b5-5e33ef420c2d
📒 Files selected for processing (4)
src/app/api/delete-file/route.tssrc/app/api/files/[...path]/route.tssrc/app/api/upload-file/route.tssrc/lib/zero/provider.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/zero/provider.tsx
- src/app/api/upload-file/route.ts
| const file = await readFile(filePath); | ||
| return new NextResponse(file, { | ||
| headers: getDownloadHeaders(relativePath), | ||
| }); |
There was a problem hiding this comment.
Stream large files instead of buffering with readFile.
CONTENT_TYPES advertises support for mp4, webm, wav, aac, zip, etc., but readFile(filePath) loads the entire payload into memory before responding. A handful of concurrent requests for a multi-GB upload can exhaust the Node heap and OOM the dev/self-host server. Prefer streaming the file as the body and setting Content-Length from stat().
♻️ Suggested change
-import { existsSync } from "node:fs";
-import { readFile } from "node:fs/promises";
+import { createReadStream, existsSync } from "node:fs";
+import { stat } from "node:fs/promises";
+import { Readable } from "node:stream";
@@
- if (!existsSync(filePath)) {
+ if (!existsSync(filePath)) {
return NextResponse.json({ error: "File not found" }, { status: 404 });
}
- const file = await readFile(filePath);
- return new NextResponse(file, {
- headers: getDownloadHeaders(relativePath),
- });
+ const { size } = await stat(filePath);
+ const headers = getDownloadHeaders(relativePath);
+ headers.set("Content-Length", String(size));
+ const body = Readable.toWeb(createReadStream(filePath)) as ReadableStream<Uint8Array>;
+ return new NextResponse(body, { headers });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/files/`[...path]/route.ts around lines 96 - 99, The code
currently uses readFile(filePath) which buffers the entire file into memory;
replace this with a streaming response by using fs.stat(filePath) to get size
and fs.createReadStream(filePath) as the response body, then return new
NextResponse(stream, { headers: { ...getDownloadHeaders(relativePath),
'Content-Length': stat.size } }) (or adjust getDownloadHeaders to accept/merge
content-length); update references to readFile and ensure error handling for
missing files uses the same flow.
1 participant
Summary
pnpm devthe supported local entrypoint by starting Next.js, AI SDK devtools, and Zero togetherSTORAGE_TYPEis unset and add authenticated local file serving for/api/files/*Verification
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores