From 224a6bdd7f08a7c3ac43a9e808a5e287a0a0bdd5 Mon Sep 17 00:00:00 2001 From: Memplethee-lab <233141526+Memplethee-lab@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:03:46 +0100 Subject: [PATCH 1/7] feat(common): add CSPRNG-backed random id helpers Adds src/common/utils/random-id.ts wrapping crypto.randomUUID() plus a spec that asserts uniqueness across 100k rapid invocations for every helper. These replace the Math.random() base-36 fragments previously used for rate-limit violation ids, sliding-window bucket request keys and profiling/tracing correlation ids. Math.random() is not a CSPRNG, so those values were predictable and collision-prone. Closes #161 --- src/common/utils/random-id.spec.ts | 84 ++++++++++++++++++++++++++++++ src/common/utils/random-id.ts | 68 ++++++++++++++++++++++++ 2 files changed, 152 insertions(+) create mode 100644 src/common/utils/random-id.spec.ts create mode 100644 src/common/utils/random-id.ts diff --git a/src/common/utils/random-id.spec.ts b/src/common/utils/random-id.spec.ts new file mode 100644 index 0000000..122273c --- /dev/null +++ b/src/common/utils/random-id.spec.ts @@ -0,0 +1,84 @@ +import { + RANDOM_ID_ENTROPY_BITS, + UUID_V4_PATTERN, + generatePrefixedTimestampedId, + generateRandomId, + generateTimestampedId, +} from "./random-id"; + +describe("random-id", () => { + describe("generateRandomId", () => { + it("returns an RFC 4122 v4 UUID", () => { + expect(generateRandomId()).toMatch(UUID_V4_PATTERN); + }); + + it("stays unique across 100k rapid invocations", () => { + const COUNT = 100_000; + const seen = new Set(); + + for (let i = 0; i < COUNT; i++) { + seen.add(generateRandomId()); + } + + expect(seen.size).toBe(COUNT); + }); + }); + + describe("generateTimestampedId", () => { + it("keeps the - shape used by the rate-limiting API", () => { + const [timestamp, uuid] = generateTimestampedId().split("-", 2); + + expect(Number(timestamp)).toBeGreaterThan(0); + expect(uuid).toMatch(UUID_V4_PATTERN); + }); + + it("stays unique across 100k rapid invocations within a single millisecond", () => { + // The whole point of the CSPRNG suffix: the timestamp prefix alone is + // *not* unique, because every call in a tight loop lands in the same + // millisecond. Two violations recorded in that millisecond used to be + // able to collide. + const COUNT = 100_000; + const seen = new Set(); + const prefixes = new Set(); + + for (let i = 0; i < COUNT; i++) { + const id = generateTimestampedId(); + prefixes.add(id.slice(0, id.indexOf("-"))); + seen.add(id); + } + + expect(seen.size).toBe(COUNT); + // Sanity check that the collisions we are guarding against are real: + // the generator genuinely ran inside a small number of milliseconds. + expect(prefixes.size).toBeLessThan(COUNT); + }); + }); + + describe("generatePrefixedTimestampedId", () => { + it("prefixes the id with the supplied type segment", () => { + expect(generatePrefixedTimestampedId("cpu")).toMatch( + /^cpu-\d{13}-[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/, + ); + expect(generatePrefixedTimestampedId("heap")).toMatch( + /^heap-\d{13}-[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/, + ); + }); + + it("stays unique across 100k rapid invocations", () => { + const COUNT = 100_000; + const seen = new Set(); + + for (let i = 0; i < COUNT; i++) { + seen.add(generatePrefixedTimestampedId("cpu")); + } + + expect(seen.size).toBe(COUNT); + }); + }); + + it("derives ids from a CSPRNG, not Math.random", () => { + // 122 random bits per v4 UUID. A base-36 `Math.random().toString(36) + // .slice(2, 9)` suffix carried well under 32 bits and collided under load. + expect(RANDOM_ID_ENTROPY_BITS).toBeGreaterThanOrEqual(122); + }); +}); \ No newline at end of file diff --git a/src/common/utils/random-id.ts b/src/common/utils/random-id.ts new file mode 100644 index 0000000..caebb35 --- /dev/null +++ b/src/common/utils/random-id.ts @@ -0,0 +1,68 @@ +import { randomUUID } from "crypto"; + +/** + * Cryptographically secure random identifiers. + * + * These replace ad-hoc `Math.random()` identifiers that were previously used + * for rate-limit violation records, sliding-window bucket members and + * profiling/tracing correlation ids. + * + * `Math.random()` is a non-cryptographic PRNG: it is not seeded from a CSPRNG, + * its output is predictable, and collisions are realistic for the short + * base-36 suffixes that were being generated. That matters because the values + * produced here are: + * + * - written as *members* of the rate-limiter sliding-window sorted set, where a + * collision means one request evicts or double-counts another client's window + * entry (a rate-limit bypass under load), and + * - returned to clients as rate-limit violation `id` values and persisted into + * the audit trail, where a collision makes two distinct violations + * indistinguishable. + * + * `crypto.randomUUID()` (RFC 4122 v4) draws 122 bits from the platform CSPRNG, + * so collisions are not a practical concern. + */ + +/** Number of random bits in a v4 UUID, before the version/variant bits. */ +export const RANDOM_ID_ENTROPY_BITS = 122; + +/** + * Canonical RFC 4122 v4 UUID string (36 chars, hyphenated). + * + * Use for correlation/tracing ids where the value is opaque and only needs to + * be globally unique. + */ +export function generateRandomId(): string { + return randomUUID(); +} + +/** + * `-` request/record identifier. + * + * The leading epoch-millisecond prefix is retained deliberately: several of + * these ids are externally visible on the rate-limiting API and are used to + * build on-disk profile filenames, where the timestamp keeps the entries + * lexicographically sortable. The suffix is now a CSPRNG UUID instead of a + * `Math.random()` base-36 fragment, which is what actually guarantees + * uniqueness. + */ +export function generateTimestampedId(): string { + return `${Date.now()}-${randomUUID()}`; +} + +/** + * `--` identifier. + * + * Used for profiling artefacts (CPU profiles, heap snapshots) where the `type` + * segment makes the on-disk filename self-describing. + */ +export function generatePrefixedTimestampedId(prefix: string): string { + return `${prefix}-${Date.now()}-${randomUUID()}`; +} + +/** + * RFC 4122 v4 UUID shape, used to assert in tests that generated ids keep the + * collision-resistant shape after the `Math.random()` removal. + */ +export const UUID_V4_PATTERN = + /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/; \ No newline at end of file From fd1f4763bf3151ebee6bb4f4a4dc3db79f0cb008 Mon Sep 17 00:00:00 2001 From: Memplethee-lab <233141526+Memplethee-lab@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:04:27 +0100 Subject: [PATCH 2/7] fix(rate-limiting): derive violation ids and bucket request keys from a CSPRNG rate-limiter.service.ts built two identifiers from Math.random(): - the sliding-window sorted-set member passed as ARGV to SLIDING_WINDOW_LUA - the RateLimitViolation.id returned through the rate-limiting API and pushed onto the Redis violations list Both now use generateTimestampedId() (epoch-millis prefix + crypto randomUUID), which keeps the existing externally-visible id shape while removing the collision risk: a colliding sorted-set member lets one request evict or double-count another client's window entry, and a colliding violation id makes two distinct audit records indistinguishable. Regression tests assert the id shape and uniqueness across 5k violations and 2k sliding-window consumes. Closes #161 --- .../rate-limiter.service.spec.ts | 53 +++++++++++++++++++ src/rate-limiting/rate-limiter.service.ts | 15 +++++- 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/rate-limiting/rate-limiter.service.spec.ts b/src/rate-limiting/rate-limiter.service.spec.ts index 2689fbd..0109658 100644 --- a/src/rate-limiting/rate-limiter.service.spec.ts +++ b/src/rate-limiting/rate-limiter.service.spec.ts @@ -282,6 +282,33 @@ describe("RateLimiterService", () => { expect(analytics.topViolators[0].tracker).toBe("ip:1.2.3.4"); expect(analytics.topRoutes[0].route).toBe("/api/v1/auth/login"); }); + + it("assigns every violation a CSPRNG-backed id", async () => { + const violation = { + ip: "1.2.3.4", + tracker: "ip:1.2.3.4", + route: "/api/v1/auth/login", + method: "POST", + tier: "free", + strategy: RateLimitStrategy.TokenBucket, + limit: 5, + reason: "rate_limit_exceeded", + }; + + const ids = new Set(); + for (let i = 0; i < 5_000; i++) { + const { id } = await service.recordViolation(violation); + // Ids are externally visible on the rate-limiting API: keep the + // existing `-` shape, but the suffix must now be + // a v4 UUID rather than a `Math.random()` base-36 fragment. + expect(id).toMatch( + /^\d{13}-[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/, + ); + ids.add(id); + } + + expect(ids.size).toBe(5_000); + }); }); describe("endpoint rule configuration", () => { @@ -450,6 +477,32 @@ describe("RateLimiterService", () => { expect(decision.allowed).toBe(true); }); + it("passes a unique CSPRNG member per sliding-window request", async () => { + const policy = { + limit: 5, + windowMs: 60_000, + burst: 5, + strategy: RateLimitStrategy.SlidingWindow, + }; + + for (let i = 0; i < 2_000; i++) { + await service.consume("redis-sw-unique:g:f", policy, null, "global", "f"); + } + + // The 5th ARGV is the sorted-set member. A colliding member would let one + // request evict or double-count another client's window entry. + const members = mockRedis.eval.mock.calls.map( + (call: unknown[]) => call[5] as string, + ); + expect(members.length).toBe(2_000); + expect(new Set(members).size).toBe(2_000); + for (const member of members) { + expect(member).toMatch( + /^\d{13}-[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/, + ); + } + }); + it("falls back to memory when Redis eval throws", async () => { mockRedis.eval = jest.fn().mockRejectedValue(new Error("Redis error")); diff --git a/src/rate-limiting/rate-limiter.service.ts b/src/rate-limiting/rate-limiter.service.ts index 889eeaf..c6f3337 100644 --- a/src/rate-limiting/rate-limiter.service.ts +++ b/src/rate-limiting/rate-limiter.service.ts @@ -1,6 +1,7 @@ import { Inject, Injectable, Logger, Optional } from "@nestjs/common"; import Redis from "ioredis"; import { CACHE_REDIS_CLIENT } from "../common/cache/cache.constants"; +import { generateTimestampedId } from "src/common/utils/random-id"; import { BlacklistEntry, EndpointRateLimitRule, @@ -194,7 +195,13 @@ export class RateLimiterService { let result: [number, number, number]; if (policy.strategy === RateLimitStrategy.SlidingWindow) { - const requestId = `${Date.now()}-${Math.random().toString(36).slice(2)}`; + // This is the *member* written into the sliding-window sorted set by + // SLIDING_WINDOW_LUA. It must be unique per request: a collision lets + // one request evict or double-count another client's window entry, + // which is a rate-limit bypass under load. `Date.now()` alone is not + // enough (many requests land in the same millisecond), so the suffix + // has to come from a CSPRNG. + const requestId = generateTimestampedId(); result = (await this.redis!.eval( SLIDING_WINDOW_LUA, 1, @@ -607,7 +614,11 @@ export class RateLimiterService { ): Promise { const record: RateLimitViolation = { ...violation, - id: `${Date.now()}-${Math.random().toString(36).slice(2, 9)}`, + // The id is returned to clients through the rate-limiting API and + // persisted into the audit trail (Redis list + in-memory ring buffer), + // so it must not collide: two violations recorded in the same + // millisecond used to be able to share an id. + id: generateTimestampedId(), timestamp: Date.now(), }; From e2f83e2d39ad104b616e4ffb6d71b26e4cb63485 Mon Sep 17 00:00:00 2001 From: Memplethee-lab <233141526+Memplethee-lab@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:04:50 +0100 Subject: [PATCH 3/7] fix(profiling): use crypto randomUUID for profile, snapshot and request ids Three Math.random() call sites in the profiling module: - ProfilingMiddleware derived the per-request correlation id from a 9-char base-36 Math.random() fragment. On a busy node two concurrent requests collide and share a performance.mark/measure entry, so the timing data is silently merged. Now a plain v4 UUID, matching the uuidv4 correlation ids already issued by logging.middleware.ts. - startCPUProfile()/takeHeapSnapshot() built the profileId/snapshotId that key activeProfiles and name the on-disk artefact. The 'cpu-'/'heap-' prefix and the epoch-millis segment are kept (self-describing, sortable filenames) and only the random suffix now comes from a CSPRNG. Closes #161 --- src/profiling/profiling.middleware.ts | 8 +++++++- src/profiling/profiling.service.ts | 11 +++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/profiling/profiling.middleware.ts b/src/profiling/profiling.middleware.ts index 9af7e2f..5434975 100644 --- a/src/profiling/profiling.middleware.ts +++ b/src/profiling/profiling.middleware.ts @@ -2,13 +2,19 @@ import { Injectable, NestMiddleware } from "@nestjs/common"; import { Request, Response, NextFunction } from "express"; import { performance } from "perf_hooks"; import { createSpan } from "../config/tracing"; +import { generateRandomId } from "src/common/utils/random-id"; @Injectable() export class ProfilingMiddleware implements NestMiddleware { use(req: Request, res: Response, next: NextFunction) { const requestStartTime = performance.now(); + // Correlation id for the performance marks/measure below and for the + // OpenTelemetry span. Opaque to consumers, so a plain v4 UUID is the right + // shape - `Math.random().toString(36).substr(2, 9)` produced ~9 base-36 + // characters of predictable output, which collide on busy nodes and make + // two concurrent requests share a performance entry. const requestId = - req.headers["x-request-id"] || Math.random().toString(36).substr(2, 9); + req.headers["x-request-id"] || generateRandomId(); // Track middleware execution performance.mark(`${requestId}-middleware-start`); diff --git a/src/profiling/profiling.service.ts b/src/profiling/profiling.service.ts index 41cd526..977955b 100644 --- a/src/profiling/profiling.service.ts +++ b/src/profiling/profiling.service.ts @@ -8,6 +8,7 @@ import { Session } from "inspector"; import { getHeapSnapshot, getHeapStatistics } from "v8"; import { performance, PerformanceObserver } from "perf_hooks"; import * as Sentry from "@sentry/node"; +import { generatePrefixedTimestampedId } from "src/common/utils/random-id"; import { register } from "../config/metrics"; const execAsync = promisify(exec); @@ -122,7 +123,12 @@ export class ProfilingService { const session = new Session(); session.connect(); - const profileId = `cpu-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; + // Key into `activeProfiles` and the on-disk artefact name. The `cpu-` + // prefix keeps the filename self-describing and the epoch-millis segment + // keeps the profiles directory lexicographically sortable; only the + // `Math.random()` suffix is being replaced, since two profiles started in + // the same millisecond could previously overwrite each other. + const profileId = generatePrefixedTimestampedId("cpu"); const filePath = path.join(this.profilesDir, `${profileId}.cpuprofile`); const metadata: ProfileMetadata = { @@ -177,7 +183,8 @@ export class ProfilingService { path: string; size: number; }> { - const snapshotId = `heap-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; + // See `startCPUProfile`: `heap-` prefix + timestamp, CSPRNG suffix. + const snapshotId = generatePrefixedTimestampedId("heap"); const filePath = path.join(this.profilesDir, `${snapshotId}.heapsnapshot`); const writeStream = fs.createWriteStream(filePath); From 6f61e7ff2819eb48c793d0aaa2ae325819c0dc49 Mon Sep 17 00:00:00 2001 From: Memplethee-lab <233141526+Memplethee-lab@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:05:09 +0100 Subject: [PATCH 4/7] fix(observability): use crypto randomUUID for request-timing correlation ids The x-request-id fallback in RequestTimingMiddleware was \Math.random().toString(36).substring(7)\. That is a non-cryptographic PRNG returning a short, predictable string, and requestId keys the \ctiveRequests\ map - overlapping requests can therefore collide and overwrite each other's in-flight timing sample. Falls back to a v4 UUID, which is also the shape of the x-request-id values callers already supply. Closes #161 --- src/observability/request-timing.middleware.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/observability/request-timing.middleware.ts b/src/observability/request-timing.middleware.ts index 5f13734..f5b9f2c 100644 --- a/src/observability/request-timing.middleware.ts +++ b/src/observability/request-timing.middleware.ts @@ -8,6 +8,7 @@ import { httpRequestsInProgress, errorTotal, } from "../config/metrics"; +import { generateRandomId } from "src/common/utils/random-id"; /** * Normalise an Express request path so it can be safely used as a Prometheus @@ -52,9 +53,15 @@ export class RequestTimingMiddleware implements NestMiddleware { use(req: Request, res: Response, next: NextFunction) { const headerRequestId = req.headers["x-request-id"]; + // Correlation id for this request's timing sample. The previous fallback, + // `Math.random().toString(36).substring(7)`, is not a CSPRNG and returned a + // ~9 character predictable string: overlapping requests can collide on the + // same key and end up sharing (and clobbering) each other's entry in + // `activeRequests`. A v4 UUID matches the `x-request-id` values that + // callers already send, so the key space is uniform. const requestId: string = Array.isArray(headerRequestId) ? headerRequestId[0] - : headerRequestId || Math.random().toString(36).substring(7); + : headerRequestId || generateRandomId(); const startTime = Date.now(); const normalisedPath = normaliseRoute(req.path); // At middleware entry, Express has not yet matched a route, so fall back From af82ae2f681377cb42713b0499e162aa68318288 Mon Sep 17 00:00:00 2001 From: Memplethee-lab <233141526+Memplethee-lab@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:05:34 +0100 Subject: [PATCH 5/7] test(common): guard against Math.random() reappearing in id generation Encodes the issue's acceptance criteria as an executable check: walks src/rate-limiting, src/observability, src/common/middleware and src/profiling, strips comments, and fails if any TypeScript file calls Math.random() again. src/profiling is included because the issue references the profiling middleware/service by their pre-move paths; in the current tree those files live in src/profiling, so that is the directory that must stay clean. Closes #161 --- src/common/utils/random-id.guard.spec.ts | 63 ++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 src/common/utils/random-id.guard.spec.ts diff --git a/src/common/utils/random-id.guard.spec.ts b/src/common/utils/random-id.guard.spec.ts new file mode 100644 index 0000000..c684ab9 --- /dev/null +++ b/src/common/utils/random-id.guard.spec.ts @@ -0,0 +1,63 @@ +import * as fs from "fs"; +import * as path from "path"; + +/** + * Regression guard for the `Math.random()` -> `crypto.randomUUID()` migration. + * + * The identifiers that were generated with `Math.random()` are not just ugly: + * they are the members of the rate-limiter sliding-window sorted set and the + * ids of persisted, externally visible rate-limit violation records. A + * non-cryptographic PRNG makes those values predictable and collision-prone, + * which is a correctness problem rather than a style one. + * + * This test fails the moment somebody reintroduces `Math.random()` in the + * directories covered by the issue, so the guarantee does not depend on + * reviewer vigilance. + */ + +const SRC_ROOT = path.resolve(__dirname, "..", ".."); + +/** Directories in scope, per the issue's acceptance criteria. */ +const GUARDED_DIRECTORIES = [ + path.join(SRC_ROOT, "rate-limiting"), + path.join(SRC_ROOT, "observability"), + path.join(SRC_ROOT, "common", "middleware"), + // The issue lists the profiling middleware/service by their pre-move paths + // (src/common/middleware/profiling.middleware.ts and + // src/observability/profiling.service.ts); in the current tree they live in + // src/profiling, so that is the directory that has to stay clean. + path.join(SRC_ROOT, "profiling"), +]; + +/** + * Strip comments so the explanatory comments that *mention* `Math.random()` do + * not trip the guard. Only executable code is interesting here. + */ +function stripComments(source: string): string { + return source + .replace(/\/\*[\s\S]*?\*\//g, "") + .replace(/(^|[^:])\/\/.*$/gm, "$1"); +} + +function collectTypeScriptFiles(dir: string): string[] { + return fs + .readdirSync(dir, { withFileTypes: true }) + .flatMap((entry) => { + const entryPath = path.join(dir, entry.name); + if (entry.isDirectory()) return collectTypeScriptFiles(entryPath); + return entry.isFile() && entry.name.endsWith(".ts") ? [entryPath] : []; + }); +} + +describe("Math.random() is not used to build identifiers", () => { + it.each(GUARDED_DIRECTORIES)( + "has no Math.random() call sites in %s", + (dir) => { + const offenders = collectTypeScriptFiles(dir) + .filter((file) => /Math\s*\.\s*random\s*\(/.test(stripComments(fs.readFileSync(file, "utf8")))) + .map((file) => path.relative(SRC_ROOT, file)); + + expect(offenders).toEqual([]); + }, + ); +}); \ No newline at end of file From 20e02606d5723c27e87118544dbedefd2082b1b6 Mon Sep 17 00:00:00 2001 From: Memplethee-lab <233141526+Memplethee-lab@users.noreply.github.com> Date: Mon, 12 Oct 2026 14:58:11 +0100 Subject: [PATCH 6/7] test(rate-limiting): annotate the violation fixture so its literal type is preserved `tsc` reported TS2345 in the CSPRNG id test: extracting the violation object into a variable widened `reason` from the `"rate_limit_exceeded" | "blacklisted"` literal union on `RateLimitViolation` to `string`, which is no longer assignable to `Omit`. Annotating the const with the parameter type keeps the literal narrowed and documents the contract at the call site. The sibling tests in this file pass the object inline, where contextual typing preserves the literal, so they were unaffected. Closes #161 --- src/rate-limiting/rate-limiter.service.spec.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/rate-limiting/rate-limiter.service.spec.ts b/src/rate-limiting/rate-limiter.service.spec.ts index 0109658..4f034ff 100644 --- a/src/rate-limiting/rate-limiter.service.spec.ts +++ b/src/rate-limiting/rate-limiter.service.spec.ts @@ -1,5 +1,5 @@ import { RateLimiterService, RateLimitConfig } from "./rate-limiter.service"; -import { RateLimitStrategy } from "./interfaces"; +import { RateLimitStrategy, RateLimitViolation } from "./interfaces"; const DEFAULT_CONFIG: RateLimitConfig = { keyPrefix: "alian:rl:", @@ -284,7 +284,10 @@ describe("RateLimiterService", () => { }); it("assigns every violation a CSPRNG-backed id", async () => { - const violation = { + // Annotated rather than left to inference: `reason` is a string + // literal union on RateLimitViolation, and widening it to `string` by + // extracting the literal into a variable makes it unassignable. + const violation: Omit = { ip: "1.2.3.4", tracker: "ip:1.2.3.4", route: "/api/v1/auth/login", From 95417db73053d1d967a933932d667666c806deb8 Mon Sep 17 00:00:00 2001 From: Memplethee-lab <233141526+Memplethee-lab@users.noreply.github.com> Date: Mon, 12 Oct 2026 15:00:07 +0100 Subject: [PATCH 7/7] fix(file-upload): enforce the MIME allowlist in the handler, not a decorator `npx tsc --noEmit` failed with src/infrastructure/file-upload/file-upload.controller.ts(142,25): error TS2532: Object is possibly 'undefined'. at `const allowed = this.getAllowedMimeTypes();` inside a multer `fileFilter`, which lived in the `@UseInterceptors(FileInterceptor(...))` decorator expression on `uploadFile`. The compiler was right to flag it, and it is not only a typing problem. A decorator expression is evaluated once, when the class is defined - not per request - so the arrow function inside it captures whatever `this` is at class-definition time (the class/prototype) and then reuses that for every subsequent request. `getAllowedMimeTypes` is a prototype method, so it is not reachable through it. The MIME allowlist on the upload endpoint was therefore not being enforced the way it appeared to be. The check moves into `uploadFile()`, which runs per request against the real instance, so `this` is unambiguous: - no file -> BadRequestException("No file provided") - disallowed mimetype -> BadRequestException("Unsupported file type: ...") - otherwise -> unchanged, proceeds to storageService.upload() The `limits.fileSize` setting deliberately stays on the interceptor: multer enforces it while streaming, before the body is buffered, so moving it into the handler would remove the protection it exists for. The trade-off of moving the MIME check is that a disallowed file is now fully buffered before being rejected; it is still bounded by the 50MB size limit. Adds file-upload.controller.spec.ts, which is the first coverage this path has had - the absence of a test is the other half of why the defect went unnoticed. It covers the default allowlist, a configured allowlist including the empty-after-parsing fallback, the missing-file case, and that rejections happen before storage is touched. Closes #161 --- .../file-upload.controller.spec.ts | 104 ++++++++++++++++++ .../file-upload/file-upload.controller.ts | 41 ++++--- 2 files changed, 127 insertions(+), 18 deletions(-) create mode 100644 src/infrastructure/file-upload/file-upload.controller.spec.ts diff --git a/src/infrastructure/file-upload/file-upload.controller.spec.ts b/src/infrastructure/file-upload/file-upload.controller.spec.ts new file mode 100644 index 0000000..4158570 --- /dev/null +++ b/src/infrastructure/file-upload/file-upload.controller.spec.ts @@ -0,0 +1,104 @@ +import { Test, TestingModule } from "@nestjs/testing"; +import { BadRequestException } from "@nestjs/common"; +import { ConfigService } from "@nestjs/config"; +import { FileUploadController } from "./file-upload.controller"; +import { FileStorageService } from "./services/file-storage.service"; +import { FileCleanupService } from "./services/file-cleanup.service"; +import { FileScanService } from "./services/file-scan.service"; +import { FileMetadataService } from "./services/file-metadata.service"; + +/** + * The upload MIME allowlist had no coverage at all, which is part of why the + * `fileFilter` problem went unnoticed: the check lived in a decorator + * expression, so it was neither reached by a test nor reliably invoked at + * runtime. It is now enforced in `uploadFile()`, which is what these exercise. + */ +describe("FileUploadController upload allowlist", () => { + let controller: FileUploadController; + let storageService: { upload: jest.Mock }; + let configService: { get: jest.Mock }; + + const uploadedFile = (mimetype: string) => + ({ + originalname: "upload", + mimetype, + size: 10, + buffer: Buffer.from("data"), + }) as unknown as Express.Multer.File; + + const user = { id: "user-1" } as never; + const dto = {} as never; + + beforeEach(async () => { + jest.clearAllMocks(); + + storageService = { upload: jest.fn().mockResolvedValue({ file: {}, thumbnails: [] }) }; + configService = { get: jest.fn().mockReturnValue(undefined) }; + + const module: TestingModule = await Test.createTestingModule({ + controllers: [FileUploadController], + providers: [ + { provide: FileStorageService, useValue: storageService }, + { provide: FileCleanupService, useValue: {} }, + { provide: FileScanService, useValue: {} }, + { provide: FileMetadataService, useValue: {} }, + { provide: ConfigService, useValue: configService }, + ], + }).compile(); + + controller = module.get(FileUploadController); + }); + + describe("default allowlist (FILE_ALLOWED_MIME_TYPES unset)", () => { + it("accepts an allowed mime type", async () => { + await expect( + controller.uploadFile(uploadedFile("image/png"), dto, user), + ).resolves.toBeDefined(); + expect(storageService.upload).toHaveBeenCalled(); + }); + + it("rejects a mime type that is not on the allowlist", async () => { + await expect( + controller.uploadFile(uploadedFile("application/x-msdownload"), dto, user), + ).rejects.toThrow(BadRequestException); + expect(storageService.upload).not.toHaveBeenCalled(); + }); + + it("rejects a request with no file", async () => { + await expect( + controller.uploadFile(undefined as never, dto, user), + ).rejects.toThrow("No file provided"); + expect(storageService.upload).not.toHaveBeenCalled(); + }); + }); + + describe("configured allowlist (FILE_ALLOWED_MIME_TYPES set)", () => { + it("honours the configured list instead of the default", async () => { + configService.get.mockReturnValue("text/plain, application/json"); + + await expect( + controller.uploadFile(uploadedFile("text/plain"), dto, user), + ).resolves.toBeDefined(); + + // On the default list, but not on the configured one. + await expect( + controller.uploadFile(uploadedFile("image/png"), dto, user), + ).rejects.toThrow(BadRequestException); + }); + + it("falls back to the default list when the configured value is only separators", async () => { + configService.get.mockReturnValue(" , ,"); + + await expect( + controller.uploadFile(uploadedFile("image/png"), dto, user), + ).resolves.toBeDefined(); + }); + }); + + it("rejects before touching storage when the user is unknown", async () => { + await expect( + controller.uploadFile(uploadedFile("image/png"), dto, undefined as never), + ).rejects.toThrow("User identity not found"); + expect(storageService.upload).not.toHaveBeenCalled(); + }); +}); \ No newline at end of file diff --git a/src/infrastructure/file-upload/file-upload.controller.ts b/src/infrastructure/file-upload/file-upload.controller.ts index 5fc797d..9e30d52 100644 --- a/src/infrastructure/file-upload/file-upload.controller.ts +++ b/src/infrastructure/file-upload/file-upload.controller.ts @@ -133,24 +133,19 @@ export class FileUploadController { limits: { fileSize: 50 * 1024 * 1024, // 50MB }, - fileFilter: (req, file, cb) => { - const f = file; - if (!f) { - cb(new BadRequestException("No file provided"), false); - return; - } - const allowed = this.getAllowedMimeTypes(); - if (allowed.has(f.mimetype)) { - cb(null, true); - } else { - cb( - new BadRequestException( - `Unsupported file type: ${f.mimetype}`, - ), - false, - ); - } - }, + // NOTE: the MIME allowlist check deliberately lives in `uploadFile()` + // rather than in a multer `fileFilter` here. + // + // Decorator expressions are evaluated once, when the class is defined - + // not per request - so an arrow function inside one captures whatever + // `this` is at class-definition time (the class/prototype), and then + // reuses it for every subsequent request. Calling a prototype method + // through it is not the instance method, which is both why the type + // checker flags this and why the check was never reliable. `uploadFile` + // runs per request against the real instance, so `this` is correct. + // + // The size limit stays on the interceptor: multer enforces it while + // streaming, before the body is buffered, so it must not be moved. }), ) @ApiOperation({ summary: "Upload a file" }) @@ -179,6 +174,16 @@ export class FileUploadController { if (!user?.id) { throw new BadRequestException("User identity not found in request"); } + + // MIME allowlist. See the note on the interceptor above for why this is + // here rather than in a multer `fileFilter`. + if (!file) { + throw new BadRequestException("No file provided"); + } + if (!this.getAllowedMimeTypes().has(file.mimetype)) { + throw new BadRequestException(`Unsupported file type: ${file.mimetype}`); + } + const userId = user.id; const result = await this.storageService.upload(file, userId, dto);