fix: require admin session to download AdminJS PII exports - #2337
Conversation
The GET /admin/download/:filename route serves project/donor email-address CSV exports (PII) from src/server/adminJs/tabs/exports. It is registered as a plain Express route, before and outside the AdminJS authenticated router, so it never inherited any auth check — anyone on the internet could download the files. Extract the handler into createDownloadAdminJsExportHandler, which now requires a valid AdminJS admin session (via getCurrentAdminJsSession) and returns 401 otherwise. getCurrentAdminJsSession throws when the request has no cookie header, so both thrown errors and falsy results are treated as unauthenticated. The existing path-traversal protection is preserved unchanged. The session resolver is injected so the handler can be unit-tested without Redis/DB; adds tests for the no-session, throwing-session, and valid-admin cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // itself (see createDownloadAdminJsExportHandler). | ||
| app.get( | ||
| '/admin/download/:filename', | ||
| createDownloadAdminJsExportHandler(getCurrentAdminJsSession), |
WalkthroughThis PR extracts AdminJS CSV export download logic from inline bootstrap code into a dedicated handler module with dependency injection, path validation, and unit tests. The handler enforces admin authentication, prevents directory traversal, and is wired into the ChangesAdminJS Export Download Handler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 (1)
src/server/adminJsExportDownload.test.ts (1)
31-69: ⚡ Quick winAdd a regression test for traversal-style filenames.
The suite covers the auth branches, but it never exercises the
400 Invalid filenamepath insrc/server/adminJsExportDownload.tsLines 49-55. Since that guard protects a PII export route, add a case like../secrets.csv(or..) and assert400plus nodownload()call.Proposed test
+ it('rejects traversal-style filenames', async () => { + const handler = createDownloadAdminJsExportHandler( + sinon.stub().resolves({ id: 1 }), + ); + const res = buildRes(); + + await handler(buildReq('../secrets.csv'), res); + + assert.isTrue(res.status.calledOnceWith(400)); + assert.isTrue(res.send.calledOnceWith('Invalid filename')); + assert.isFalse(res.download.called); + });🤖 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 `@src/server/adminJsExportDownload.test.ts` around lines 31 - 69, Add a test exercising the filename validation in createDownloadAdminJsExportHandler by calling the handler with a traversal-style filename (e.g. buildReq('../secrets.csv') or buildReq('..')) while supplying a stubbed valid session (sinon.stub().resolves({ id: 1 })) and a buildRes() response; then assert the handler responds with status 400 and does not call res.download (and optionally assert res.send was called with the "Invalid filename" message) to ensure the 400 branch (the guard in createDownloadAdminJsExportHandler) is covered.
🤖 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 `@src/server/bootstrap.ts`:
- Around line 220-223: The /admin/download/:filename route is registered before
the global limiter and falls under the broad /admin bypass, leaving
createDownloadAdminJsExportHandler(getCurrentAdminJsSession) unthrottled; add a
narrow rate limiter middleware specifically to this route (or adjust the global
exempt logic to exclude paths starting with /admin/download/) so requests
hitting getCurrentAdminJsSession do not bypass throttling. Locate the route
registration that calls createDownloadAdminJsExportHandler and wrap it with the
new per-route limiter middleware (or modify the /admin bypass check to skip
"/admin/download/") and ensure the limiter configuration is conservative (low
burst and reasonable window) to prevent Redis/findUserById fan-out on repeated
unauthorized hits.
---
Nitpick comments:
In `@src/server/adminJsExportDownload.test.ts`:
- Around line 31-69: Add a test exercising the filename validation in
createDownloadAdminJsExportHandler by calling the handler with a traversal-style
filename (e.g. buildReq('../secrets.csv') or buildReq('..')) while supplying a
stubbed valid session (sinon.stub().resolves({ id: 1 })) and a buildRes()
response; then assert the handler responds with status 400 and does not call
res.download (and optionally assert res.send was called with the "Invalid
filename" message) to ensure the 400 branch (the guard in
createDownloadAdminJsExportHandler) is covered.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3de05c4-3f2f-4283-b279-c54f11f0c5e9
📒 Files selected for processing (3)
src/server/adminJsExportDownload.test.tssrc/server/adminJsExportDownload.tssrc/server/bootstrap.ts
| app.get( | ||
| '/admin/download/:filename', | ||
| createDownloadAdminJsExportHandler(getCurrentAdminJsSession), | ||
| ); |
There was a problem hiding this comment.
Rate-limit this public download endpoint separately.
Line 220 registers the route before the global limiter, and Lines 266-269 later exempt every /admin request anyway. That leaves /admin/download/:filename unthrottled even though each hit now runs getCurrentAdminJsSession(), which can fan out to Redis and findUserById (src/server/adminJs/adminJs.ts:101-130) before returning 401. Add a narrow limiter here, or exclude /admin/download/ from the /admin bypass.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 222-222: Missing rate limiting
This route handler performs a file system access, but is not rate-limited.
🤖 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 `@src/server/bootstrap.ts` around lines 220 - 223, The
/admin/download/:filename route is registered before the global limiter and
falls under the broad /admin bypass, leaving
createDownloadAdminJsExportHandler(getCurrentAdminJsSession) unthrottled; add a
narrow rate limiter middleware specifically to this route (or adjust the global
exempt logic to exclude paths starting with /admin/download/) so requests
hitting getCurrentAdminJsSession do not bypass throttling. Locate the route
registration that calls createDownloadAdminJsExportHandler and wrap it with the
new per-route limiter middleware (or modify the /admin bypass check to skip
"/admin/download/") and ensure the limiter configuration is conservative (low
burst and reasonable window) to prevent Redis/findUserById fan-out on repeated
unauthorized hits.
Source: Linters/SAST tools
2 participants
Problem
app.get('/admin/download/:filename', ...)insrc/server/bootstrap.tsis unauthenticated. It is registered as a plain Express route before and outside the AdminJS authenticated router (app.use(adminJsRootPath, await getAdminJsRouter())), so Express matches it first and never runs any auth check.The route serves the CSV exports written by the AdminJS projects tab to
src/server/adminJs/tabs/exports— these contain PII (project/donor email addresses). As a result, anyone on the internet could download them, e.g.GET /admin/download/emails.csv.Fix
Extract the handler into
createDownloadAdminJsExportHandler(newsrc/server/adminJsExportDownload.ts). It now requires a valid AdminJS admin session viagetCurrentAdminJsSessionand returns 401 otherwise:getCurrentAdminJsSessionthrows aTypeErrorwhen the request carries no cookie header (cookie.parse(undefined)), so the call is wrapped intry/catchand both thrown errors and falsy results are treated as unauthenticated.path.basename+path.dirname(filePath) !== exportsDir) is copied verbatim.The session resolver is passed in (dependency injection) rather than imported directly, so the handler can be unit-tested without booting Redis/Postgres/AdminJS.
bootstrap.tswires in the realgetCurrentAdminJsSession; the now-unusedpathimport was removed.The legitimate flow is unchanged: when an admin exports from the AdminJS panel, the browser follows the
/admin/download/...redirect with theadminjssession cookie, sogetCurrentAdminJsSessionresolves and the file downloads.Tests
New
src/server/adminJsExportDownload.test.ts(hermetic — no DB/Redis), all passing:res.downloadnot called.res.downloadnot called.Verification
npx tsc --noEmit— clean across the project.eslint— clean (also enforced by the husky pre-commit hook).A full live end-to-end run (real server + cookies) wasn't possible in my environment (no Postgres/Redis available), so behaviour is covered by the unit tests above plus the type/lint checks.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Refactor