Skip to content

2.14.0 #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
saby1101 merged 1 commit into from
May 9, 2026
Merged

2.14.0 #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
226 changes: 78 additions & 148 deletions ISSUES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,236 +2,166 @@

## Audits

| # | Date | Description | New Issues | False Positives |
| --- | ---------- | ------------------------------------------------------------------ | ---------------------------- | --------------- |
| 1 | 2026-03-23 | Full codebase audit — all source files, tests, types, build config | 1 CRIT, 3 HIGH, 4 MED, 3 LOW | 0 |
| # | Date | Description | New Issues | False Positives |
| --- | ---------- | --------------------------------------------------------------------------------------------------------------- | -------------------- | --------------- |
| 1 | 2026-05-09 | Initial audit — file-store, utils, types, all specs (cloud providers + utils only; LocalFileStore out of scope) | 1 HIGH, 2 MED, 2 LOW | 0 |

---

## Fixed Issues

0 issues fixed across 0 audit sessions:

| ID | Issue | Commit |
| --- | ----- | ------ |
| — | — | — |

## False Positives Removed

0 issues removed after verification:

| Original ID | Why Removed |
| ----------- | ----------- |
| — | — |

---

## Table of Contents

- [Audits](#audits)
- [Fixed Issues](#fixed-issues)
- [False Positives Removed](#false-positives-removed)
- [CRITICAL Issues (1 remaining)](#critical-issues)
- [HIGH Issues (3 remaining)](#high-issues)
- [MEDIUM Issues (4 remaining)](#medium-issues)
- [LOW Issues (3 remaining)](#low-issues)
- [HIGH Issues (1 remaining)](#high-issues)
- [MEDIUM Issues (2 remaining)](#medium-issues)
- [LOW Issues (2 remaining)](#low-issues)
- [Summary](#summary)

---

## CRITICAL Issues

### CRIT-01: Publish route has no authentication — allows arbitrary event injection

**File:** `src/event-bus/index.ts:32-78`
**Severity:** CRITICAL (any unauthenticated HTTP client can publish arbitrary events into the bus, triggering any registered handler with any payload)

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**.

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.

**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.

---

## HIGH Issues

### HIGH-01: Weak truthiness check in `getHandlerMap` silently swallows falsy handlers
### HIGH-01: Unsafe `err["$metadata"].httpStatusCode` access in `S3FileStore.exists()`

**File:** `src/event-bus/commons.ts:37`
**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)
**File:** `src/file-store.ts:327`
**Severity:** HIGH — if the thrown error has no `$metadata` property (network error, credentials error, SDK version change), this line throws `TypeError: Cannot read properties of undefined`, masking the original error entirely.

```typescript
if (handler as any) {
handlerMap.get(key)!.push({ file, handler });
}
if (err["$metadata"].httpStatusCode === 404) { // no optional chaining
```

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.

**Suggested fix:** Either remove the check entirely (TypeScript enforces the type) or throw an explicit error:
The same class's `getInfo()` method at line 425 uses optional chaining correctly:

```typescript
if (!handler) {
throw new Error(`Handler for event "${key}" in file "${file}" is ${handler}`);
}
err["$metadata"]?.httpStatusCode === 404;
```

---

### HIGH-02: `processError` return value `err` is used but may lack `.message` property

**File:** `src/event-bus/commons.ts:110-118`
**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)
**Suggested fix:** Apply optional chaining to match `getInfo()`:

```typescript
({ err, status } = options.processError(err, ctx));
if (!ctx.specifiedFile) {
ctx.publishToPubSub(ctx.eventMsg.event, ctx.eventMsg.data, ctx.file);
} else {
throw new ErrorWithStatus(
status,
`${ctx.file}-${ctx.handler.name} failed with ${err.message}`,
);
if (err["$metadata"]?.httpStatusCode === 404) {
return false;
}
```

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.

**Suggested fix:** Guard the message construction: `err?.message ?? String(err)`.

---

### HIGH-03: FastifyInstance type augmentation missing for `EventBus` and `FileStore`
## MEDIUM Issues

**File:** `src/types.ts:3-12`
**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)
### MED-01: No TypeScript augmentation for `FastifyInstance.FileStore`

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`.
**File:** `src/types.ts:1-9`, `src/file-store.ts:443`
**Severity:** MEDIUM — users of this library get no compile-time type safety when accessing `fastify.FileStore`; IDE autocomplete does not surface the `FileStore` interface. The `tsconfig.json` is strict (`noImplicitAny`, `strictNullChecks`), making this a meaningful gap.

**Suggested fix:** Add to the module augmentation:
`file-store.ts` decorates the instance at runtime:

```typescript
export interface FastifyInstance {
EventBus: EventBus;
FileStore: import("./file-store").FileStore;
_hasEventHandlers: boolean;
}
f.decorate("FileStore", new LocalFileStore(dir));
```

---

## MEDIUM Issues

### MED-01: Prometheus metrics created without checking for duplicate registration

**File:** `src/event-bus/commons.ts:137-154`
**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)
But `src/types.ts` only augments `FastifySchema`. The required `FastifyInstance` augmentation is missing. The Jest config (`jest.config.js:10`) sets `diagnostics: false` in ts-jest, which suppresses TypeScript errors during test runs and masks this gap.

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.

**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.

---

### MED-02: NATS JetStream publisher drops messages silently under backpressure

**File:** `src/event-bus/nats-jetstream.ts:119-126`
**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)
**Suggested fix:** Add to `src/types.ts`:

```typescript
if (inflight.size >= MAX_INFLIGHT) {
f.log.error({
tag: "NATS_JETSTREAM_PUBLISH_BACKPRESSURE",
event,
inflightCount: inflight.size,
});
return; // message is dropped
import type { FileStore } from "./file-store";

declare module "fastify" {
interface FastifyInstance {
FileStore: FileStore;
}
}
```

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.

**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.
Also remove `diagnostics: false` from `jest.config.js` so TypeScript errors surface during `pnpm test`.

---

### MED-03: GCP Pub/Sub consumer mutates `process.env.EVENT_SUBSCRIPTION` at runtime
### MED-02: `ConfigureAWS` passes `credentialDefaultProvider` as an unrecognised `S3ClientConfig` option

**File:** `src/event-bus/event-consumer/gcp-pubsub.ts:17-31`
**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)
**File:** `src/file-store.ts:475-479`
**Severity:** MEDIUM — `credentialDefaultProvider` is not a standard `S3ClientConfig` field in `@aws-sdk/client-s3`; it is silently ignored. If the intent was to explicitly set the credential provider, that intent is not fulfilled and credentials fall back to the SDK's ambient default chain with no indication anything is wrong.

```typescript
if (
process.env.EVENT_TOPIC &&
!process.env.EVENT_SUBSCRIPTION &&
process.env.APP
) {
// ...
for (const sub of subs[0]) {
if (sub.name.includes(process.env.APP)) {
process.env.EVENT_SUBSCRIPTION = sub.name; // global mutation
}
}
}
import { defaultProvider } from "@aws-sdk/credential-provider-node";

// ...
const client = new S3.S3Client({
region: process.env.AWS_S3_REGION ?? "us-east-1",
credentialDefaultProvider: defaultProvider, // not a valid S3ClientConfig key
});
```

**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.
**Suggested fix:** Remove the `credentialDefaultProvider` line and the `defaultProvider` import. If explicit credential control is needed, use the `credentials` field instead: `credentials: defaultProvider()`.

---

### MED-04: Counter metric name `event_handler_latency_total` is misleading

**File:** `src/event-bus/commons.ts:149-153`
**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)

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.
## LOW Issues

**Suggested fix:** Rename to `event_handler_invocations_total` with `help: "Total number of event handler invocations"`.
### LOW-01: `streamToBuffer` is unsafe when a stream emits string chunks

---
**File:** `src/utils.ts:6-14`
**Severity:** LOW — if `setEncoding()` was called on the stream before passing it to `streamToBuffer`, Node.js emits `string` chunks instead of `Buffer`s. Pushing strings into the `Buffer[]` array causes `Buffer.concat()` to throw a `TypeError` at runtime.

## LOW Issues
```typescript
const chunks: Buffer[] = [];
stream.on("data", (chunk) => chunks.push(chunk)); // chunk may be a string
// ...
Buffer.concat(chunks); // throws TypeError if any chunk is a string
```

### LOW-01: `RABBITMQ_TAG` uses weak randomness for consumer tag
The `DataStream` interface's `on("data")` listener signature is `(...args: any[])`, so TypeScript does not catch this.

**File:** `src/event-bus/rabbitmq-utils.ts:3`
**Severity:** LOW (using `Math.random()` for the consumer tag could theoretically produce collisions across instances, though the probability is very low with 1e9 range)
**Suggested fix:**

```typescript
export const RABBITMQ_TAG = "" + Math.floor(Math.random() * 1e9);
stream.on("data", (chunk: Buffer | string) =>
chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)),
);
```

**Suggested fix:** Use `crypto.randomUUID()` or `crypto.randomInt()` for stronger uniqueness guarantees.

---

### LOW-02: Event consumer module has very low test coverage (18.6%)
### LOW-02: `AzureFileStore.getInfo()` falls back to `new Date()` for missing `lastModified`

**File:** `src/event-bus/event-consumer/` (all files except `utils.ts`)
**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)
**File:** `src/file-store.ts:217`
**Severity:** LOW — when Azure returns properties without a `lastModified` timestamp, the code silently substitutes the current time, making the returned `FileInfo` appear freshly modified. Callers using `lastModified` for cache invalidation or change detection will receive incorrect data.

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).

**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.
```typescript
lastModified: properties.lastModified || new Date(),
```

---
The same pattern exists in `GCPFileStore.getInfo()` at line 300.

### LOW-03: Duplicate test description in integration spec
**Suggested fix:** Throw or surface the absence explicitly rather than substituting the current time:

**File:** `src/integration.spec.ts:41` and `src/integration.spec.ts:64`
**Severity:** LOW (two tests have the identical description "should register both plugins successfully", making test reports ambiguous)
```typescript
lastModified: properties.lastModified ?? (() => { throw new Error("lastModified missing from Azure response"); })(),
```

**Suggested fix:** Rename the second test to something like "should register both plugins and perform basic operations".
Or, if a sentinel is acceptable, document it clearly and use a fixed epoch (`new Date(0)`) so callers can detect the fallback.

---

## Summary

| Severity | Remaining |
| --------------------------- | ----------- |
| **CRITICAL** | 1 |
| **HIGH** | 3 |
| **MEDIUM** | 4 |
| **LOW** | 3 |
| **TOTAL** | **11 open** |
| **Fixed** | 0 |
| **False Positives Removed** | 0 |
| Severity | Remaining |
| --------------------------- | ---------- |
| **HIGH** | 1 |
| **MEDIUM** | 2 |
| **LOW** | 2 |
| **TOTAL** | **5 open** |
| **Fixed** | 0 |
| **False Positives Removed** | 0 |
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@stackbox-dev/fp-plugins",
"version": "2.13.0",
"version": "2.14.0",
"description": "Fastify Plugins used in various stackbox applications",
"main": "dist/index.js",
"types": "dist/index.d.ts",
Expand Down
Loading