fix: serve public assets on self-hosted deployments - #2266
Conversation
When NEXT_PUBLIC_IS_CAP is not "true", proxy.ts redirects every path outside its allowlist to /login, and the matcher only exempts favicon.ico, robots.txt and sitemap.xml. Every other file under apps/web/public therefore 307s to /login on a self-hosted instance and renders broken. Let any path with a file extension through the self-hosted redirect. Such a path is either a static file or a 404, never a page, so the login gate has nothing to protect there, and new asset types work without a matcher edit.
| const isStaticAsset = /\.[a-z0-9]+$/i.test(path); | ||
| if ( | ||
| !( | ||
| isStaticAsset || |
There was a problem hiding this comment.
P2: The extension check can bypass the self-hosted authentication gate for non-static routes
Any URL ending in an extension bypasses login; the code does not verify that it is an actual public asset.
Scope the bypass to verified public assets and test a protected extension-suffixed route.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="apps/web/proxy.ts">
<violation number="1" location="apps/web/proxy.ts:62">
<priority>P2</priority>
<title>The extension check can bypass the self-hosted authentication gate for non-static routes</title>
<evidence>The new `isStaticAsset = /\.[a-z0-9]+$/i.test(path)` condition is sufficient to skip the login redirect, but it does not verify that the path resolves to a file under `public/`. Any protected page, API endpoint, or dynamic route whose URL ends in an extension can therefore be allowed through. The added tests cover real assets and `/pricing`, but do not cover an extension-suffixed protected route.</evidence>
<recommendation>Restrict the bypass to paths known to be public assets (for example, an explicit public-asset path policy or narrowly scoped asset directories) rather than inferring static-file status solely from the URL suffix. Add a regression test for a protected route ending in `.json`, `.csv`, or another extension.</recommendation>
</violation>
</file>
There was a problem hiding this comment.
Fair point, there are extension-suffixed routes (install-cli.sh, the docs catch-all). Reworked: the bypass now requires the path to resolve to an existing file under public/, with traversal out of the directory rejected. Added tests for /install-cli.sh, a missing file, a directory, and ..%2F traversal, all of which still redirect.
There was a problem hiding this comment.
The developer replaced the broad extension-suffix bypass with a filesystem-level verification that the path resolves to an existing file under public/ while rejecting directory traversal, and added regression tests for extension-suffixed routes, missing files, directories, and encoded traversal. This directly resolves the finding and satisfies the recommendation.
An extension alone is not proof of a static asset: install-cli.sh and the docs catch-all are extension-suffixed routes. Resolve the path under public/ and require it to be an existing file, rejecting traversal out of the directory.
| 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. | ||
| const isStaticAsset = /\.[a-z0-9]+$/i.test(path); |
There was a problem hiding this comment.
Extensionless Asset Still Redirects
The extension-only check does not match the existing public file public/.well-known/atproto-did, and the separate allowlist only covers /.well-known/workflow/. On self-hosted deployments, requests for this well-known resource therefore still redirect to /login instead of serving the file, leaving the public-asset fix incomplete.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/proxy.ts
Line: 59
Comment:
**Extensionless Asset Still Redirects**
The extension-only check does not match the existing public file `public/.well-known/atproto-did`, and the separate allowlist only covers `/.well-known/workflow/`. On self-hosted deployments, requests for this well-known resource therefore still redirect to `/login` instead of serving the file, leaving the public-asset fix incomplete.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
This was against the first commit. The current version no longer keys on the extension: it stats the resolved path under public/, so /.well-known/atproto-did is served. Added it to the test cases.
|
hey @greptileai, please review the PR |
|
hey @greptileai, please re-review the PR |
| } | ||
| const file = resolve(publicDir, `.${decoded}`); | ||
| if (!file.startsWith(`${publicDir}${sep}`)) return false; | ||
| return statSync(file, { throwIfNoEntry: false })?.isFile() ?? false; |
There was a problem hiding this comment.
A request path containing an encoded NUL byte, such as /%00, is decoded and passed to statSync. Node rejects that path with ERR_INVALID_ARG_VALUE, which throwIfNoEntry: false does 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
This is a comment left during a code review.
Path: apps/web/proxy.ts
Line: 27
Comment:
**Malformed Paths Can Throw**
A request path containing an encoded NUL byte, such as `/%00`, is decoded and passed to `statSync`. Node rejects that path with `ERR_INVALID_ARG_VALUE`, which `throwIfNoEntry: false` does 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.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
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 file = resolve(publicDir, `.${decoded}`); | ||
| if (!file.startsWith(`${publicDir}${sep}`)) return false; | ||
| return statSync(file, { throwIfNoEntry: false })?.isFile() ?? false; |
There was a problem hiding this 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.”
| return statSync(file, { throwIfNoEntry: false })?.isFile() ?? false; | |
| try { | |
| return statSync(file, { throwIfNoEntry: false })?.isFile() ?? false; | |
| } catch { | |
| return false; | |
| } |
Prompt To Fix With AI
This 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.There was a problem hiding this comment.
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.
statSync still throws for ENOTDIR, ENAMETOOLONG and an encoded NUL byte, and that escaped proxy() as a 500 instead of the /login redirect.
|
hey @greptileai, please re-review the PR |
Problem
On a self-hosted deployment (
NEXT_PUBLIC_IS_CAPunset),proxy.tsredirects every path outside its allowlist to/login, and the matcher only exemptsfavicon.ico,robots.txtandsitemap.xml. Every other file underapps/web/public/therefore 307s to/login, so<img src="/logos/browsers/google-chrome.svg">on onboarding, the OS/browser logos, illustrations, sounds, Rive files and fonts all render broken. cap.so never sees this because it takes theIS_CAPbranch.Reproduced on a Docker build of
24e3c74:Fix
In the self-hosted branch, let a path through when it resolves to an existing file under
apps/web/public/. The proxy already runs in the Node runtime (it imports the database), so this is astatSyncon a resolved path, with traversal out ofpublic/rejected.This avoids maintaining an extension list, which is where #2127 was flagged for still missing
.rivand the recorder sounds, and it does not widen the bypass to non-asset routes:/install-cli.shand the docs catch-all keep their current behaviour because they are not files inpublic/.Related: #2127 takes the extension-allowlist approach for the same bug.
Tests
apps/web/__tests__/unit/proxy-self-hosted.test.tsnow callsproxy()directly withNEXT_PUBLIC_IS_CAPmocked off and asserts:.svg,.webp,.ogg,.riv,.woff2and root-levelsite.webmanifestassets return 200 with no redirect/pricingstill redirects to/login/install-cli.sh, an extension-suffixed route handler, still redirects..%2Ftraversal all still redirect/s/video123is unaffectedConfirmed the asset cases fail on
mainand pass with the change.Validation
The PR appears safe to merge; the current implementation addresses all previous findings without introducing a new actionable defect.
Findings
Fix with agent prompt
Summary
Reviews (4) · Last reviewed commit: "fix: treat filesystem lookup failures as..."