Skip to content

Commit 76f644e

Browse files
saby1101saby1101
authored
Merge pull request #4 from stackbox-dev/release/2.14.0
2.14.0
2 parents c001efc + 200c619 commit 76f644e

2 files changed

Lines changed: 79 additions & 149 deletions

File tree

‎ISSUES.md‎

Lines changed: 78 additions & 148 deletions
Original file line numberDiff line numberDiff line change
@@ -2,236 +2,166 @@
22

33
## Audits
44

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 |
5+
| # | Date | Description | New Issues | False Positives |
6+
| --- | ---------- | --------------------------------------------------------------------------------------------------------------- | -------------------- | --------------- |
7+
| 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 |
88

99
---
1010

1111
## Fixed Issues
1212

13-
0 issues fixed across 0 audit sessions:
14-
1513
| ID | Issue | Commit |
1614
| --- | ----- | ------ |
15+
| — | — | — |
1716

1817
## False Positives Removed
1918

20-
0 issues removed after verification:
21-
2219
| Original ID | Why Removed |
2320
| ----------- | ----------- |
21+
| — | — |
2422

2523
---
2624

2725
## Table of Contents
2826

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)
27+
- [HIGH Issues (1 remaining)](#high-issues)
28+
- [MEDIUM Issues (2 remaining)](#medium-issues)
29+
- [LOW Issues (2 remaining)](#low-issues)
3630
- [Summary](#summary)
3731

3832
---
3933

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-
5534
## HIGH Issues
5635

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

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)
38+
**File:** `src/file-store.ts:327`
39+
**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.
6140

6241
```typescript
63-
if (handler as any) {
64-
handlerMap.get(key)!.push({ file, handler });
65-
}
42+
if (err["$metadata"].httpStatusCode === 404) { // no optional chaining
6643
```
6744
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:
45+
The same class's `getInfo()` method at line 425 uses optional chaining correctly:
7146
7247
```typescript
73-
if (!handler) {
74-
throw new Error(`Handler for event "${key}" in file "${file}" is ${handler}`);
75-
}
48+
err["$metadata"]?.httpStatusCode === 404;
7649
```
7750
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)
51+
**Suggested fix:** Apply optional chaining to match `getInfo()`:
8452
8553
```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-
);
54+
if (err["$metadata"]?.httpStatusCode === 404) {
55+
return false;
9456
}
9557
```
9658
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-
10159
---
10260
103-
### HIGH-03: FastifyInstance type augmentation missing for `EventBus` and `FileStore`
61+
## MEDIUM Issues
10462
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)
63+
### MED-01: No TypeScript augmentation for `FastifyInstance.FileStore`
10764
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`.
65+
**File:** `src/types.ts:1-9`, `src/file-store.ts:443`
66+
**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.
10967
110-
**Suggested fix:** Add to the module augmentation:
68+
`file-store.ts` decorates the instance at runtime:
11169
11270
```typescript
113-
export interface FastifyInstance {
114-
EventBus: EventBus;
115-
FileStore: import("./file-store").FileStore;
116-
_hasEventHandlers: boolean;
117-
}
71+
f.decorate("FileStore", new LocalFileStore(dir));
11872
```
11973
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)
74+
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.
12875
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)
76+
**Suggested fix:** Add to `src/types.ts`:
13977
14078
```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
79+
import type { FileStore } from "./file-store";
80+
81+
declare module "fastify" {
82+
interface FastifyInstance {
83+
FileStore: FileStore;
84+
}
14885
}
14986
```
15087
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.
88+
Also remove `diagnostics: false` from `jest.config.js` so TypeScript errors surface during `pnpm test`.
15489
15590
---
15691
157-
### MED-03: GCP Pub/Sub consumer mutates `process.env.EVENT_SUBSCRIPTION` at runtime
92+
### MED-02: `ConfigureAWS` passes `credentialDefaultProvider` as an unrecognised `S3ClientConfig` option
15893
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)
94+
**File:** `src/file-store.ts:475-479`
95+
**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.
16196
16297
```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-
}
98+
import { defaultProvider } from "@aws-sdk/credential-provider-node";
99+
100+
// ...
101+
const client = new S3.S3Client({
102+
region: process.env.AWS_S3_REGION ?? "us-east-1",
103+
credentialDefaultProvider: defaultProvider, // not a valid S3ClientConfig key
104+
});
175105
```
176106
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.
107+
**Suggested fix:** Remove the `credentialDefaultProvider` line and the `defaultProvider` import. If explicit credential control is needed, use the `credentials` field instead: `credentials: defaultProvider()`.
178108
179109
---
180110
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.
111+
## LOW Issues
187112
188-
**Suggested fix:** Rename to `event_handler_invocations_total` with `help: "Total number of event handler invocations"`.
113+
### LOW-01: `streamToBuffer` is unsafe when a stream emits string chunks
189114
190-
---
115+
**File:** `src/utils.ts:6-14`
116+
**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.
191117
192-
## LOW Issues
118+
```typescript
119+
const chunks: Buffer[] = [];
120+
stream.on("data", (chunk) => chunks.push(chunk)); // chunk may be a string
121+
// ...
122+
Buffer.concat(chunks); // throws TypeError if any chunk is a string
123+
```
193124
194-
### LOW-01: `RABBITMQ_TAG` uses weak randomness for consumer tag
125+
The `DataStream` interface's `on("data")` listener signature is `(...args: any[])`, so TypeScript does not catch this.
195126

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)
127+
**Suggested fix:**
198128

199129
```typescript
200-
export const RABBITMQ_TAG = "" + Math.floor(Math.random() * 1e9);
130+
stream.on("data", (chunk: Buffer | string) =>
131+
chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)),
132+
);
201133
```
202134
203-
**Suggested fix:** Use `crypto.randomUUID()` or `crypto.randomInt()` for stronger uniqueness guarantees.
204-
205135
---
206136
207-
### LOW-02: Event consumer module has very low test coverage (18.6%)
137+
### LOW-02: `AzureFileStore.getInfo()` falls back to `new Date()` for missing `lastModified`
208138
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)
139+
**File:** `src/file-store.ts:217`
140+
**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.
211141
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.
142+
```typescript
143+
lastModified: properties.lastModified || new Date(),
144+
```
215145
216-
---
146+
The same pattern exists in `GCPFileStore.getInfo()` at line 300.
217147
218-
### LOW-03: Duplicate test description in integration spec
148+
**Suggested fix:** Throw or surface the absence explicitly rather than substituting the current time:
219149
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)
150+
```typescript
151+
lastModified: properties.lastModified ?? (() => { throw new Error("lastModified missing from Azure response"); })(),
152+
```
222153
223-
**Suggested fix:** Rename the second test to something like "should register both plugins and perform basic operations".
154+
Or, if a sentinel is acceptable, document it clearly and use a fixed epoch (`new Date(0)`) so callers can detect the fallback.
224155
225156
---
226157
227158
## Summary
228159
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 |
160+
| Severity | Remaining |
161+
| --------------------------- | ---------- |
162+
| **HIGH** | 1 |
163+
| **MEDIUM** | 2 |
164+
| **LOW** | 2 |
165+
| **TOTAL** | **5 open** |
166+
| **Fixed** | 0 |
167+
| **False Positives Removed** | 0 |

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@stackbox-dev/fp-plugins",
3-
"version": "2.13.0",
3+
"version": "2.14.0",
44
"description": "Fastify Plugins used in various stackbox applications",
55
"main": "dist/index.js",
66
"types": "dist/index.d.ts",

0 commit comments

Comments
 (0)