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 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 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); 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 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); diff --git a/src/rate-limiting/rate-limiter.service.spec.ts b/src/rate-limiting/rate-limiter.service.spec.ts index 2689fbd..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:", @@ -282,6 +282,36 @@ 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 () => { + // 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", + 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 +480,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(), };