|
| 1 | +# Codebase Issues Audit |
| 2 | + |
| 3 | +## Audits |
| 4 | + |
| 5 | +| # | Date | Description | New Issues | False Positives | |
| 6 | +| --- | ---------- | ------------------------------------------------------------------ | ---------------------------- | --------------- | |
| 7 | +| 1 | 2026-03-23 | Full codebase audit — all source files, tests, types, build config | 1 CRIT, 3 HIGH, 4 MED, 3 LOW | 0 | |
| 8 | + |
| 9 | +--- |
| 10 | + |
| 11 | +## Fixed Issues |
| 12 | + |
| 13 | +0 issues fixed across 0 audit sessions: |
| 14 | + |
| 15 | +| ID | Issue | Commit | |
| 16 | +| --- | ----- | ------ | |
| 17 | + |
| 18 | +## False Positives Removed |
| 19 | + |
| 20 | +0 issues removed after verification: |
| 21 | + |
| 22 | +| Original ID | Why Removed | |
| 23 | +| ----------- | ----------- | |
| 24 | + |
| 25 | +--- |
| 26 | + |
| 27 | +## Table of Contents |
| 28 | + |
| 29 | +- [Audits](#audits) |
| 30 | +- [Fixed Issues](#fixed-issues) |
| 31 | +- [False Positives Removed](#false-positives-removed) |
| 32 | +- [CRITICAL Issues (1 remaining)](#critical-issues) |
| 33 | +- [HIGH Issues (3 remaining)](#high-issues) |
| 34 | +- [MEDIUM Issues (4 remaining)](#medium-issues) |
| 35 | +- [LOW Issues (3 remaining)](#low-issues) |
| 36 | +- [Summary](#summary) |
| 37 | + |
| 38 | +--- |
| 39 | + |
| 40 | +## CRITICAL Issues |
| 41 | + |
| 42 | +### CRIT-01: Publish route has no authentication — allows arbitrary event injection |
| 43 | + |
| 44 | +**File:** `src/event-bus/index.ts:32-78` |
| 45 | +**Severity:** CRITICAL (any unauthenticated HTTP client can publish arbitrary events into the bus, triggering any registered handler with any payload) |
| 46 | + |
| 47 | +The `/event-bus/publish/:event` POST endpoint is registered without any authentication, authorization, or rate limiting. Any client that can reach the server can inject events. The only guard is the `disableEventPublishRoute` option, which defaults to `undefined` (falsy), meaning the route is **enabled by default**. |
| 48 | + |
| 49 | +This is dangerous because event handlers typically perform business-critical operations (order processing, file handling, etc.), and an attacker can trigger them with crafted payloads. |
| 50 | + |
| 51 | +**Suggested fix:** Either: (1) Default `disableEventPublishRoute` to `true` and require explicit opt-in, or (2) Add a required authentication hook/middleware to this route, or (3) At minimum, document the security implications prominently and add a warning log on registration when the route is enabled. |
| 52 | + |
| 53 | +--- |
| 54 | + |
| 55 | +## HIGH Issues |
| 56 | + |
| 57 | +### HIGH-01: Weak truthiness check in `getHandlerMap` silently swallows falsy handlers |
| 58 | + |
| 59 | +**File:** `src/event-bus/commons.ts:37` |
| 60 | +**Severity:** HIGH (if a handler function is accidentally set to `undefined`, `null`, `0`, or `""` in a handler map, the `if (handler as any)` check silently skips it instead of throwing an error, making handler registration bugs invisible) |
| 61 | + |
| 62 | +```typescript |
| 63 | +if (handler as any) { |
| 64 | + handlerMap.get(key)!.push({ file, handler }); |
| 65 | +} |
| 66 | +``` |
| 67 | + |
| 68 | +The `as any` cast defeats TypeScript's type checking. A handler that is `undefined` (e.g., due to a typo in a re-export) would be silently ignored. The developer would see no error but the event would never be processed. |
| 69 | + |
| 70 | +**Suggested fix:** Either remove the check entirely (TypeScript enforces the type) or throw an explicit error: |
| 71 | + |
| 72 | +```typescript |
| 73 | +if (!handler) { |
| 74 | + throw new Error(`Handler for event "${key}" in file "${file}" is ${handler}`); |
| 75 | +} |
| 76 | +``` |
| 77 | + |
| 78 | +--- |
| 79 | + |
| 80 | +### HIGH-02: `processError` return value `err` is used but may lack `.message` property |
| 81 | + |
| 82 | +**File:** `src/event-bus/commons.ts:110-118` |
| 83 | +**Severity:** HIGH (if `processError` returns an `err` without a `.message` property, the `ErrorWithStatus` message becomes `"file-handler failed with undefined"`, making production debugging very difficult) |
| 84 | + |
| 85 | +```typescript |
| 86 | +({ err, status } = options.processError(err, ctx)); |
| 87 | +if (!ctx.specifiedFile) { |
| 88 | + ctx.publishToPubSub(ctx.eventMsg.event, ctx.eventMsg.data, ctx.file); |
| 89 | +} else { |
| 90 | + throw new ErrorWithStatus( |
| 91 | + status, |
| 92 | + `${ctx.file}-${ctx.handler.name} failed with ${err.message}`, |
| 93 | + ); |
| 94 | +} |
| 95 | +``` |
| 96 | + |
| 97 | +The `processError` interface returns `{ err: any; status: number }`. Since `err` is typed as `any`, accessing `.message` is unsafe. If the caller's `processError` returns a plain string or object without `.message`, the error message is misleading. |
| 98 | + |
| 99 | +**Suggested fix:** Guard the message construction: `err?.message ?? String(err)`. |
| 100 | + |
| 101 | +--- |
| 102 | + |
| 103 | +### HIGH-03: FastifyInstance type augmentation missing for `EventBus` and `FileStore` |
| 104 | + |
| 105 | +**File:** `src/types.ts:3-12` |
| 106 | +**Severity:** HIGH (TypeScript does not know about `fastify.EventBus` or `fastify.FileStore` on the FastifyInstance — only `FastifyRequest.EventBus` is augmented. This forces users to use `(fastify as any).EventBus` or get type errors) |
| 107 | + |
| 108 | +The `declare module "fastify"` block only augments `FastifyRequest` with `EventBus`. It does not augment `FastifyInstance` with either `EventBus` or `FileStore`. The test files confirm this: `integration.spec.ts:156-157` uses `(fastify1 as any).FileStore`. |
| 109 | + |
| 110 | +**Suggested fix:** Add to the module augmentation: |
| 111 | + |
| 112 | +```typescript |
| 113 | +export interface FastifyInstance { |
| 114 | + EventBus: EventBus; |
| 115 | + FileStore: import("./file-store").FileStore; |
| 116 | + _hasEventHandlers: boolean; |
| 117 | +} |
| 118 | +``` |
| 119 | + |
| 120 | +--- |
| 121 | + |
| 122 | +## MEDIUM Issues |
| 123 | + |
| 124 | +### MED-01: Prometheus metrics created without checking for duplicate registration |
| 125 | + |
| 126 | +**File:** `src/event-bus/commons.ts:137-154` |
| 127 | +**Severity:** MEDIUM (if `CreateHandlerRunner` is called multiple times with the same registry, `prom-client` will throw "A metric with the name event_handler_latency_ms has already been registered", crashing the application) |
| 128 | + |
| 129 | +The `Histogram` and `Counter` are created with fixed names `event_handler_latency_ms` and `event_handler_latency_total`. If the event bus plugin is registered in multiple Fastify encapsulation contexts sharing the same `registry`, the second registration will throw. |
| 130 | + |
| 131 | +**Suggested fix:** Either use `prom.register.getSingleMetric()` to check if the metric already exists, or catch the duplicate registration error and reuse the existing metric. |
| 132 | + |
| 133 | +--- |
| 134 | + |
| 135 | +### MED-02: NATS JetStream publisher drops messages silently under backpressure |
| 136 | + |
| 137 | +**File:** `src/event-bus/nats-jetstream.ts:119-126` |
| 138 | +**Severity:** MEDIUM (when `inflight.size >= MAX_INFLIGHT`, the publish function logs an error and returns without publishing — the message is permanently lost with no retry or dead-letter mechanism) |
| 139 | + |
| 140 | +```typescript |
| 141 | +if (inflight.size >= MAX_INFLIGHT) { |
| 142 | + f.log.error({ |
| 143 | + tag: "NATS_JETSTREAM_PUBLISH_BACKPRESSURE", |
| 144 | + event, |
| 145 | + inflightCount: inflight.size, |
| 146 | + }); |
| 147 | + return; // message is dropped |
| 148 | +} |
| 149 | +``` |
| 150 | + |
| 151 | +While MAX_INFLIGHT is 10,000, under sustained load this could drop events. Unlike the error path (which retries 3 times), the backpressure path has no recovery. |
| 152 | + |
| 153 | +**Suggested fix:** Either throw an error so callers know the publish failed, or implement a bounded queue with backpressure signaling. At minimum, add a Prometheus counter for dropped messages so operators can alert on it. |
| 154 | + |
| 155 | +--- |
| 156 | + |
| 157 | +### MED-03: GCP Pub/Sub consumer mutates `process.env.EVENT_SUBSCRIPTION` at runtime |
| 158 | + |
| 159 | +**File:** `src/event-bus/event-consumer/gcp-pubsub.ts:17-31` |
| 160 | +**Severity:** MEDIUM (the consumer builder writes to `process.env.EVENT_SUBSCRIPTION` during auto-discovery, which is a global side-effect that can cause race conditions if multiple consumers are created concurrently or if other code reads this env var) |
| 161 | + |
| 162 | +```typescript |
| 163 | +if ( |
| 164 | + process.env.EVENT_TOPIC && |
| 165 | + !process.env.EVENT_SUBSCRIPTION && |
| 166 | + process.env.APP |
| 167 | +) { |
| 168 | + // ... |
| 169 | + for (const sub of subs[0]) { |
| 170 | + if (sub.name.includes(process.env.APP)) { |
| 171 | + process.env.EVENT_SUBSCRIPTION = sub.name; // global mutation |
| 172 | + } |
| 173 | + } |
| 174 | +} |
| 175 | +``` |
| 176 | + |
| 177 | +**Suggested fix:** Use a local variable instead of mutating `process.env`. Pass the discovered subscription name through the function's scope rather than the global environment. |
| 178 | + |
| 179 | +--- |
| 180 | + |
| 181 | +### MED-04: Counter metric name `event_handler_latency_total` is misleading |
| 182 | + |
| 183 | +**File:** `src/event-bus/commons.ts:149-153` |
| 184 | +**Severity:** MEDIUM (the counter is named `event_handler_latency_total` but it counts handler invocations, not latency — this violates Prometheus naming conventions and will confuse operators) |
| 185 | + |
| 186 | +The histogram already tracks latency (`event_handler_latency_ms`). The counter should be named something like `event_handler_invocations_total` to reflect what it actually measures. |
| 187 | + |
| 188 | +**Suggested fix:** Rename to `event_handler_invocations_total` with `help: "Total number of event handler invocations"`. |
| 189 | + |
| 190 | +--- |
| 191 | + |
| 192 | +## LOW Issues |
| 193 | + |
| 194 | +### LOW-01: `RABBITMQ_TAG` uses weak randomness for consumer tag |
| 195 | + |
| 196 | +**File:** `src/event-bus/rabbitmq-utils.ts:3` |
| 197 | +**Severity:** LOW (using `Math.random()` for the consumer tag could theoretically produce collisions across instances, though the probability is very low with 1e9 range) |
| 198 | + |
| 199 | +```typescript |
| 200 | +export const RABBITMQ_TAG = "" + Math.floor(Math.random() * 1e9); |
| 201 | +``` |
| 202 | + |
| 203 | +**Suggested fix:** Use `crypto.randomUUID()` or `crypto.randomInt()` for stronger uniqueness guarantees. |
| 204 | + |
| 205 | +--- |
| 206 | + |
| 207 | +### LOW-02: Event consumer module has very low test coverage (18.6%) |
| 208 | + |
| 209 | +**File:** `src/event-bus/event-consumer/` (all files except `utils.ts`) |
| 210 | +**Severity:** LOW (the consumer implementations for RabbitMQ, GCP Pub/Sub, Azure ServiceBus, and NATS JetStream have 0% branch coverage and ~10-13% line coverage — only covered by Docker-based integration tests) |
| 211 | + |
| 212 | +While the integration tests in `event-bus.integration.spec.ts` cover these with real brokers, the unit test suite has no coverage for consumer logic (retry behavior, DLQ routing, error handling). |
| 213 | + |
| 214 | +**Suggested fix:** Add unit tests using `fastify.inject()` mocking patterns similar to the publisher route tests, or at least document that coverage relies on integration tests that require Docker. |
| 215 | + |
| 216 | +--- |
| 217 | + |
| 218 | +### LOW-03: Duplicate test description in integration spec |
| 219 | + |
| 220 | +**File:** `src/integration.spec.ts:41` and `src/integration.spec.ts:64` |
| 221 | +**Severity:** LOW (two tests have the identical description "should register both plugins successfully", making test reports ambiguous) |
| 222 | + |
| 223 | +**Suggested fix:** Rename the second test to something like "should register both plugins and perform basic operations". |
| 224 | + |
| 225 | +--- |
| 226 | + |
| 227 | +## Summary |
| 228 | + |
| 229 | +| Severity | Remaining | |
| 230 | +| --------------------------- | ----------- | |
| 231 | +| **CRITICAL** | 1 | |
| 232 | +| **HIGH** | 3 | |
| 233 | +| **MEDIUM** | 4 | |
| 234 | +| **LOW** | 3 | |
| 235 | +| **TOTAL** | **11 open** | |
| 236 | +| **Fixed** | 0 | |
| 237 | +| **False Positives Removed** | 0 | |
0 commit comments