fix(api): authenticated file endpoint reflects any origin with credentials, enabling cross-origin theft of private user files - #2291
Conversation
…nticated file endpoint GET /v1/misc/static/:objectKey is guarded by JwtAuthGuard and serves users' private files, but reflected the request Origin into Access-Control-Allow-Origin while sending Access-Control-Allow-Credentials: true. With cookie-based sessions, any website could read a logged-in user's private files cross-origin. Only grant credentialed CORS to origins in the ORIGIN allowlist (the same configuration the global CORS middleware in main.ts uses). Other origins receive no Access-Control-Allow-Origin, so browsers block cross-origin reads, while Cross-Origin-Resource-Policy: cross-origin keeps legitimate no-cors embedding working. Adds regression tests for allowlisted, unknown, and missing origins. Also updates the stale controller spec mocks (ApiKeyService, PrismaService) required by the current JwtAuthGuard.
📝 WalkthroughWalkthrough
ChangesCORS header control
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/src/modules/misc/misc.controller.ts (1)
50-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine constants for fixed CORS strings.
'origin', header names, and fixed header values are new magic strings. Define named constants for these values so the CORS contract has one maintainable definition.As per coding guidelines: “Avoid magic numbers and strings - use named constants in TypeScript/JavaScript.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/misc/misc.controller.ts` around lines 50 - 62, Define named constants for the configuration key `'origin'`, the CORS header names, and fixed header values used by the allowed-origin logic in the controller. Update the `allowedOrigins` lookup and `headers` construction to reference those constants while preserving the existing CORS behavior.Source: Coding guidelines
apps/api/src/modules/misc/misc.controller.spec.ts (1)
39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the handler response without
any.
(controller as any)bypasses TypeScript checks and tests a private implementation detail. TestserveStaticwith mocked request and response objects. Cover both the 200 and 304 paths. This verifies that the CORS headers reach the HTTP response.As per coding guidelines: “Avoid using
anytype whenever possible - useunknowntype instead with proper type guards” and “Always define explicit return types for functions, especially for public APIs.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/misc/misc.controller.spec.ts` around lines 39 - 40, Replace the buildCorsHeaders helper in the misc controller spec with tests that invoke serveStatic using mocked request and response objects, avoiding any and private-method access. Assert both 200 and 304 response paths, including that the generated CORS headers are applied to the HTTP response, and give all test helpers explicit return types.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/modules/misc/misc.controller.ts`:
- Around line 55-63: Initialize headers with Vary: Origin before the origin
allowlist conditional so every response, including rejected- and missing-origin
cases, carries it. Keep the existing allowed-origin headers unchanged, and
update the corresponding rejected-origin and missing-origin tests to assert the
Vary header.
---
Nitpick comments:
In `@apps/api/src/modules/misc/misc.controller.spec.ts`:
- Around line 39-40: Replace the buildCorsHeaders helper in the misc controller
spec with tests that invoke serveStatic using mocked request and response
objects, avoiding any and private-method access. Assert both 200 and 304
response paths, including that the generated CORS headers are applied to the
HTTP response, and give all test helpers explicit return types.
In `@apps/api/src/modules/misc/misc.controller.ts`:
- Around line 50-62: Define named constants for the configuration key
`'origin'`, the CORS header names, and fixed header values used by the
allowed-origin logic in the controller. Update the `allowedOrigins` lookup and
`headers` construction to reference those constants while preserving the
existing CORS behavior.
🪄 Autofix
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 Plus
Run ID: 121f07a2-3e04-4255-ac0d-b5e73247a0ea
📒 Files selected for processing (2)
apps/api/src/modules/misc/misc.controller.spec.tsapps/api/src/modules/misc/misc.controller.ts
| const headers: Record<string, string> = { | ||
| 'Cross-Origin-Resource-Policy': 'cross-origin', | ||
| }; | ||
|
|
||
| if (origin && allowedOrigins.includes(origin)) { | ||
| headers['Access-Control-Allow-Origin'] = origin; | ||
| headers['Access-Control-Allow-Credentials'] = 'true'; | ||
| headers['Vary'] = 'Origin'; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Set Vary: Origin for all responses.
This response varies by request Origin, but rejected-origin responses omit Vary: Origin. A cache can store that variant as origin-independent and reuse it for an allowlisted origin. Set Vary: Origin in headers before the conditional. Add assertions for Vary in the rejected-origin and missing-origin tests.
Proposed fix
const headers: Record<string, string> = {
'Cross-Origin-Resource-Policy': 'cross-origin',
+ 'Vary': 'Origin',
};
if (origin && allowedOrigins.includes(origin)) {
headers['Access-Control-Allow-Origin'] = origin;
headers['Access-Control-Allow-Credentials'] = 'true';
- headers['Vary'] = 'Origin';
}📝 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.
| const headers: Record<string, string> = { | |
| 'Cross-Origin-Resource-Policy': 'cross-origin', | |
| }; | |
| if (origin && allowedOrigins.includes(origin)) { | |
| headers['Access-Control-Allow-Origin'] = origin; | |
| headers['Access-Control-Allow-Credentials'] = 'true'; | |
| headers['Vary'] = 'Origin'; | |
| } | |
| const headers: Record<string, string> = { | |
| 'Cross-Origin-Resource-Policy': 'cross-origin', | |
| 'Vary': 'Origin', | |
| }; | |
| if (origin && allowedOrigins.includes(origin)) { | |
| headers['Access-Control-Allow-Origin'] = origin; | |
| headers['Access-Control-Allow-Credentials'] = 'true'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/modules/misc/misc.controller.ts` around lines 55 - 63,
Initialize headers with Vary: Origin before the origin allowlist conditional so
every response, including rejected- and missing-origin cases, carries it. Keep
the existing allowed-origin headers unchanged, and update the corresponding
rejected-origin and missing-origin tests to assert the Vary header.
|
Update: reproduced end-to-end against the published Docker images. I spun up the stock self-hosted stack ( 1. Victim signs up, uploads a private file curl -c cookies.txt -X POST http://localhost:5700/api/v1/auth/email/signup \
-H 'Content-Type: application/json' \
-d '{"email":"victim@example.com","password":"..."}'
curl -c cookies.txt -X POST http://localhost:5700/api/v1/auth/email/login -d '{...same...}'
# -> Set-Cookie: _rf_access=<JWT>; HttpOnly; Secure; SameSite=Lax
curl -b cookies.txt -X POST http://localhost:5700/api/v1/misc/upload \
-F "file=@secret.txt;type=text/plain" -F "visibility=private"
# -> {"storageKey":"static/05f79648-....txt"}2. Attacker origin reads the private file curl -b cookies.txt -H 'Origin: https://evil.example.com' -i \
http://localhost:5700/api/v1/misc/static/05f79648-c41a-4cd4-aaa5-14282e88821e.txtAny origin string is reflected. No preflight is even needed, since this is a CORS "simple request": the response headers alone authorize the cross-origin read. 3. Browser confirmation I logged a browser into the local instance, then served an attacker page from a different origin whose only code is: fetch('http://localhost:5700/api/v1/misc/static/05f79648-....txt', { credentials: 'include' })
.then(r => r.text()) // resolves to the victim's private file bodyThe page rendered Controls
On this PR's branch, step 2 and 3 fail the CORS check because |
1 participant
Summary
The authenticated file endpoint
GET /v1/misc/static/:objectKey(guarded byJwtAuthGuard, serving users' private files after an ownership check) reflects the requestOriginheader intoAccess-Control-Allow-Originand simultaneously sendsAccess-Control-Allow-Credentials: true.Because Refly sessions are stored in cookies, this is exploitable today: any website can make a logged-in victim's browser fetch their private files cross-origin and read the responses. No clicks, no warnings, no visible trace for the victim. Loading an attacker page is enough.
Impact
objectKeyvalues are enumerable or leak through any listing endpoint, an attacker can harvest files from every victim who loads their page, not just one.Cross-Origin-Resource-Policy: cross-originadditionally allows the protected content to be embedded directly into attacker pages, independent of the CORS issue.Proof of concept
From any origin, while the victim is logged in:
The browser attaches the session cookie, the server reflects the attacker's origin with credentials allowed, and the response is readable. Identified with umbra, an open source security scanner I maintain, then verified by manual code review against
main. I did not test against any live deployment.Root cause
CORS safety depends on never combining a reflected untrusted origin with credentials. With both present, the browser sends the victim's cookies and exposes the response body to the attacker's JavaScript. The ownership check still passes because the request is legitimately authenticated as the victim.
Notably, the application already does this correctly at the global level:
main.tsconfigures CORS with a fixed allowlist from theORIGINenv var. This endpoint overrode that policy with unconditional reflection.Fix
Only grant credentialed CORS to origins in the existing
ORIGINallowlist, the same configuration the global middleware uses:Access-Control-Allow-Origin+ credentials +Vary: Origin).Access-Control-Allow-Origin, so browsers block cross-origin reads of authenticated responses.Cross-Origin-Resource-Policy: cross-originis preserved, so legitimate no-cors embedding (images and similar) keeps working.servePublicStaticendpoint is intentionally unchanged: it serves public data, and tightening it could break third-party consumers.Tests
Added regression tests covering allowlisted origins (credentialed access preserved), unknown origins (no CORS headers), and requests without an
Originheader. All pass:npx jest src/modules/misc/misc.controller.spec.ts, 4/4.The controller spec also needed stale mocks refreshed (
ApiKeyService,PrismaService), sinceJwtAuthGuardgained dependencies after the spec was written. Without that, the suite could not run at all.Related hardening notes (out of scope here)
CustomThrottlerGuard.shouldSkipinapps/api/src/modules/app.module.tsdisables rate limiting wheneverNODE_ENV !== 'production'. A deployment missing that variable silently loses brute-force protection on auth endpoints.deploy/searxng/settings.ymlcommits a fixed 64-hex SearXNGsecret_keymounted by the default compose setup, so default self-hosted installs share the same HMAC secret.Happy to split those into follow-ups if you want them addressed.
Summary by CodeRabbit