Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions src/server/adminJsExportDownload.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import { assert } from 'chai';
import sinon from 'sinon';
import { Request, Response } from 'express';
import { createDownloadAdminJsExportHandler } from './adminJsExportDownload';

describe(
'createDownloadAdminJsExportHandler() test cases',
downloadAdminJsExportTestCases,
);

function downloadAdminJsExportTestCases() {
const buildReq = (filename: string): Request =>
({ params: { filename }, headers: {} }) as unknown as Request;

const buildRes = () => {
const res: any = {};
res.statusCode = undefined;
res.status = sinon.stub().callsFake((code: number) => {
res.statusCode = code;
return res;
});
res.send = sinon.stub().returnsThis();
res.download = sinon.stub();
return res as Response & {
status: sinon.SinonStub;
send: sinon.SinonStub;
download: sinon.SinonStub;
};
};

it('responds 401 and serves no file when there is no admin session', async () => {
const handler = createDownloadAdminJsExportHandler(
sinon.stub().resolves(false),
);
const res = buildRes();

await handler(buildReq('emails.csv'), res);

assert.isTrue(res.status.calledOnceWith(401));
assert.isTrue(res.send.calledOnceWith('Unauthorized'));
assert.isFalse(res.download.called);
});

it('responds 401 when resolving the session throws (e.g. no cookie header)', async () => {
// getCurrentAdminJsSession throws TypeError when there is no cookie header.
const handler = createDownloadAdminJsExportHandler(
sinon.stub().rejects(new TypeError('argument str must be a string')),
);
const res = buildRes();

await handler(buildReq('emails.csv'), res);

assert.isTrue(res.status.calledOnceWith(401));
assert.isFalse(res.download.called);
});

it('serves the requested export from the exports dir for a valid admin session', async () => {
const handler = createDownloadAdminJsExportHandler(
sinon.stub().resolves({ id: 1 }),
);
const res = buildRes();

await handler(buildReq('emails.csv'), res);

assert.isFalse(res.status.calledWith(401));
assert.isTrue(res.download.calledOnce);
const servedPath = res.download.firstCall.args[0] as string;
assert.match(servedPath, /adminJs\/tabs\/exports\/emails\.csv$/);
});
}
58 changes: 58 additions & 0 deletions src/server/adminJsExportDownload.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import path from 'path';
import { Request, Response } from 'express';
import { logger } from '../utils/logger';
import type { IncomingMessage } from 'connect';

/**
* Resolves the current AdminJS admin session for a request (from its signed
* session cookie). Mirrors the signature of `getCurrentAdminJsSession` in
* `./adminJs/adminJs`; a truthy value means "authenticated admin", `false`
* means "no valid session". It is injected (rather than imported directly) so
* this module — and its unit test — stay decoupled from the AdminJS/Redis
* machinery.
*/
export type AdminJsSessionResolver = (req: IncomingMessage) => Promise<unknown>;

// AdminJS-generated CSV exports live here (e.g. project/donor email-address
// lists). Resolved relative to this file so it matches where projectsTab
// writes them (src/server/adminJs/tabs/exports).
const exportsDir = path.join(__dirname, '/adminJs/tabs/exports');

/**
* Builds the handler for `GET /admin/download/:filename`, which serves the
* AdminJS CSV exports created from the projects tab.
*
* Those exports contain PII (project/donor email addresses). The route is
* registered as a plain Express route, OUTSIDE the AdminJS authenticated
* router, so it does NOT inherit AdminJS auth. Without the explicit session
* check below, anyone could download the files. We therefore require a valid
* admin session and reject everything else with 401.
*/
export const createDownloadAdminJsExportHandler = (
getCurrentAdminJsSession: AdminJsSessionResolver,
) => {
return async (req: Request, res: Response): Promise<void> => {
// Require a valid admin session. `getCurrentAdminJsSession` throws when the
// request carries no cookie header, so treat any failure (thrown or falsy)
// as "not authenticated".
let isAuthenticatedAdmin = false;
try {
isAuthenticatedAdmin = Boolean(await getCurrentAdminJsSession(req));
} catch (e) {
logger.error('admin export download auth check failed', e);
}
if (!isAuthenticatedAdmin) {
res.status(401).send('Unauthorized');
return;
}

// Prevent path traversal: reduce to a bare filename (strips any `../`),
// then confirm the resolved path is directly inside the exports dir.
const filePath = path.join(exportsDir, path.basename(req.params.filename));
if (path.dirname(filePath) !== exportsDir) {
res.status(400).send('Invalid filename');
return;
}
res.download(filePath);
};
};
31 changes: 14 additions & 17 deletions src/server/bootstrap.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// @ts-check
import http from 'http';
import path from 'path';
import { Resource } from '@adminjs/typeorm';
import { ApolloServer } from '@apollo/server';
import { ApolloServerPluginLandingPageGraphQLPlayground } from '@apollo/server-plugin-landing-page-graphql-playground';
Expand Down Expand Up @@ -37,7 +36,12 @@
import { logger } from '../utils/logger';
import { flushSentryAndExit } from '../utils/globalErrorHandlers';
import { isTrustedVercelRequest } from '../utils/ipWhitelist';
import { adminJsRootPath, getAdminJsRouter } from './adminJs/adminJs';
import {
adminJsRootPath,
getAdminJsRouter,
getCurrentAdminJsSession,
} from './adminJs/adminJs';
import { createDownloadAdminJsExportHandler } from './adminJsExportDownload';
// import { apiGivRouter } from '../routers/apiGivRoutes';
import { AppDataSource, CronDataSource } from '../orm';
import {
Expand Down Expand Up @@ -209,21 +213,14 @@
limit: (config.get('UPLOAD_FILE_MAX_SIZE') as number) || '5mb',
});

// To download email addresses of projects in AdminJS projects tab
app.get('/admin/download/:filename', (req, res) => {
const exportsDir = path.join(__dirname, '/adminJs/tabs/exports');
// Prevent path traversal: reduce to a bare filename (strips any `../`),
// then confirm the resolved path is directly inside the exports dir.
const filePath = path.join(
exportsDir,
path.basename(req.params.filename),
);
if (path.dirname(filePath) !== exportsDir) {
res.status(400).send('Invalid filename');
return;
}
res.download(filePath);
});
// Download email-address CSV exports generated from the AdminJS projects
// tab. These contain PII and this route lives outside the AdminJS
// authenticated router, so the handler enforces a valid admin session
// itself (see createDownloadAdminJsExportHandler).
app.get(
'/admin/download/:filename',
createDownloadAdminJsExportHandler(getCurrentAdminJsSession),

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
a file system access
, but is not rate-limited.
);
Comment on lines +220 to +223

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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


// Lightweight "hello world" health check for deploy verification.
// Defined BEFORE global CORS middleware so it's always reachable from any origin.
Expand Down
Loading