-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: serve public assets on self-hosted deployments #2266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
e3de7f7
c0b8a60
89902a7
c17e012
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,76 @@ | ||
| import { readFileSync } from "node:fs"; | ||
| import { join } from "node:path"; | ||
| import { describe, expect, it } from "vitest"; | ||
| import { NextRequest } from "next/server"; | ||
| import { describe, expect, it, vi } from "vitest"; | ||
| import { proxy } from "../../proxy"; | ||
|
|
||
| vi.mock("@cap/database", () => ({ | ||
| db: () => { | ||
| throw new Error("Database should not be reached on self-hosted routes"); | ||
| }, | ||
| })); | ||
|
|
||
| vi.mock("@cap/database/schema", () => ({ organizations: {} })); | ||
|
|
||
| vi.mock("@cap/env", () => ({ | ||
| buildEnv: { NEXT_PUBLIC_IS_CAP: "false" }, | ||
| serverEnv: () => ({ | ||
| WEB_URL: "https://cap.example.com", | ||
| VERCEL_URL_HOST: undefined, | ||
| VERCEL_BRANCH_URL_HOST: undefined, | ||
| VERCEL_PROJECT_PRODUCTION_URL_HOST: undefined, | ||
| }), | ||
| })); | ||
|
|
||
| const request = (path: string) => | ||
| proxy(new NextRequest(`https://cap.example.com${path}`)); | ||
|
|
||
| const expectServed = async (path: string) => { | ||
| const response = await request(path); | ||
| expect(response.status).toBe(200); | ||
| expect(response.headers.get("location")).toBeNull(); | ||
| }; | ||
|
|
||
| const expectLoginRedirect = async (path: string) => { | ||
| const response = await request(path); | ||
| expect(response.status).toBe(307); | ||
| expect(response.headers.get("location")).toBe( | ||
| "https://cap.example.com/login", | ||
| ); | ||
| }; | ||
|
|
||
| describe("self-hosted proxy routes", () => { | ||
| it("allows browser-based CLI authorization pages", () => { | ||
| const source = readFileSync(join(process.cwd(), "proxy.ts"), "utf8"); | ||
| expect(source).toContain('path.startsWith("/cli/")'); | ||
| }); | ||
|
|
||
| it.each([ | ||
| "/logos/browsers/google-chrome.svg", | ||
| "/illustrations/app.webp", | ||
| "/sounds/start-recording.ogg", | ||
| "/rive/main.riv", | ||
| "/fonts/Geist-Regular.woff2", | ||
| "/site.webmanifest", | ||
| "/.well-known/atproto-did", | ||
| ])("serves the public asset %s instead of redirecting", (path) => | ||
| expectServed(path), | ||
| ); | ||
|
|
||
| it("still redirects page routes to /login", () => | ||
| expectLoginRedirect("/pricing")); | ||
|
|
||
| it("still redirects extension-suffixed route handlers to /login", () => | ||
| expectLoginRedirect("/install-cli.sh")); | ||
|
|
||
| it("does not let a missing file through", () => | ||
| expectLoginRedirect("/logos/missing.svg")); | ||
|
|
||
| it("does not let a directory through", () => expectLoginRedirect("/logos")); | ||
|
|
||
| it("rejects path traversal out of public/", () => | ||
| expectLoginRedirect("/logos/..%2F..%2Fproxy.ts")); | ||
|
|
||
| it("does not treat a share link as an asset", () => | ||
| expectServed("/s/video123")); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,5 @@ | ||||||||||||||
| import { statSync } from "node:fs"; | ||||||||||||||
| import { resolve, sep } from "node:path"; | ||||||||||||||
| import { db } from "@cap/database"; | ||||||||||||||
| import { organizations } from "@cap/database/schema"; | ||||||||||||||
| import { buildEnv, serverEnv } from "@cap/env"; | ||||||||||||||
|
|
@@ -11,6 +13,20 @@ const addHttps = (s?: string) => { | |||||||||||||
| return `https://${s}`; | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| const publicDir = resolve(process.cwd(), "public"); | ||||||||||||||
|
|
||||||||||||||
| const isPublicAsset = (path: string) => { | ||||||||||||||
| let decoded: string; | ||||||||||||||
| try { | ||||||||||||||
| decoded = decodeURIComponent(path); | ||||||||||||||
| } catch { | ||||||||||||||
| return false; | ||||||||||||||
| } | ||||||||||||||
| const file = resolve(publicDir, `.${decoded}`); | ||||||||||||||
| if (!file.startsWith(`${publicDir}${sep}`)) return false; | ||||||||||||||
| return statSync(file, { throwIfNoEntry: false })?.isFile() ?? false; | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A request pathname can make
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/proxy.ts
Line: 27
Comment:
**Filesystem Errors Escape Proxy**
A request pathname can make `statSync` fail for reasons other than a missing entry—for example, an overlong decoded path can produce `ENAMETOOLONG`. Since this call is outside a catch block, the error escapes `proxy()` and returns a 500 instead of following the existing `/login` redirect behavior. Treat filesystem lookup failures as “not a public asset.”
```suggestion
try {
return statSync(file, { throwIfNoEntry: false })?.isFile() ?? false;
} catch {
return false;
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. The lookup is now inside the try, and there are tests for an encoded NUL, a file used as a directory, and an overlong name, all of which redirect to /login. |
||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| const mainOrigins = [ | ||||||||||||||
| "https://cap.so", | ||||||||||||||
| "https://cap.link", | ||||||||||||||
|
|
@@ -54,8 +70,11 @@ export async function proxy(request: NextRequest) { | |||||||||||||
| const hostname = url.hostname; | ||||||||||||||
|
|
||||||||||||||
| if (buildEnv.NEXT_PUBLIC_IS_CAP !== "true") { | ||||||||||||||
| // Files under public/ have no route of their own, so without this every | ||||||||||||||
| // <img src="/logos/..."> on a self-hosted instance redirects to /login. | ||||||||||||||
| if ( | ||||||||||||||
| !( | ||||||||||||||
| isPublicAsset(path) || | ||||||||||||||
| path.startsWith("/s/") || | ||||||||||||||
| path.startsWith("/c/") || | ||||||||||||||
| path.startsWith("/cli/") || | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A request path containing an encoded NUL byte, such as
/%00, is decoded and passed tostatSync. Node rejects that path withERR_INVALID_ARG_VALUE, whichthrowIfNoEntry: falsedoes not suppress. Because this call is outside the later error handler, the self-hosted proxy throws instead of redirecting the request to/login. Catch filesystem validation errors and treat them as a non-asset path.Prompt To Fix With AI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. The lookup is now inside the try, and there are tests for an encoded NUL, a file used as a directory, and an overlong name, all of which redirect to /login.