Skip to content

Commit 96d40d4

Browse files
saby1101claude
andcommitted
fix: optional chaining on S3 metadata, string-safe streamToBuffer, epoch fallback for lastModified
- HIGH-01: add ?. to err["$metadata"] access in S3FileStore.exists() - LOW-01: guard streamToBuffer data chunks against string-mode streams - LOW-02: use new Date(0) instead of new Date() when Azure/GCP lastModified is absent Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 247cb86 commit 96d40d4

3 files changed

Lines changed: 15 additions & 89 deletions

File tree

ISSUES.md

Lines changed: 9 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,17 @@
55
| # | Date | Description | New Issues | False Positives |
66
| --- | ---------- | --------------------------------------------------------------------------------------------------------------- | -------------------- | --------------- |
77
| 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 |
8-
| 2 | 2026-05-10 | Re-verify MED-02 after researching credentialDefaultProvider in S3Client source | 0 | 1 (MED-02) |
8+
| 2 | 2026-05-10 | Re-verify MED-02 after researching credentialDefaultProvider in S3Client source; fix HIGH-01, LOW-01, LOW-02 | 0 | 1 (MED-02) |
99

1010
---
1111

1212
## Fixed Issues
1313

14-
| ID | Issue | Commit |
15-
| --- | ----- | ------ |
16-
||||
14+
| ID | Issue | Commit |
15+
| ------- | ------------------------------------------------------------------------- | ------- |
16+
| HIGH-01 | Unsafe `err["$metadata"].httpStatusCode` access in `S3FileStore.exists()` | pending |
17+
| LOW-01 | `streamToBuffer` unsafe with string-mode streams | pending |
18+
| LOW-02 | Azure/GCP `getInfo()` fallback to `new Date()` for missing `lastModified` | pending |
1719

1820
## False Positives Removed
1921

@@ -25,40 +27,11 @@
2527

2628
## Table of Contents
2729

28-
- [HIGH Issues (1 remaining)](#high-issues)
2930
- [MEDIUM Issues (1 remaining)](#medium-issues)
30-
- [LOW Issues (2 remaining)](#low-issues)
3131
- [Summary](#summary)
3232

3333
---
3434

35-
## HIGH Issues
36-
37-
### HIGH-01: Unsafe `err["$metadata"].httpStatusCode` access in `S3FileStore.exists()`
38-
39-
**File:** `src/file-store.ts:327`
40-
**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.
41-
42-
```typescript
43-
if (err["$metadata"].httpStatusCode === 404) { // no optional chaining
44-
```
45-
46-
The same class's `getInfo()` method at line 425 uses optional chaining correctly:
47-
48-
```typescript
49-
err["$metadata"]?.httpStatusCode === 404;
50-
```
51-
52-
**Suggested fix:** Apply optional chaining to match `getInfo()`:
53-
54-
```typescript
55-
if (err["$metadata"]?.httpStatusCode === 404) {
56-
return false;
57-
}
58-
```
59-
60-
---
61-
6235
## MEDIUM Issues
6336

6437
### MED-01: No TypeScript augmentation for `FastifyInstance.FileStore`
@@ -90,60 +63,11 @@ Also remove `diagnostics: false` from `jest.config.js` so TypeScript errors surf
9063

9164
---
9265

93-
## LOW Issues
94-
95-
### LOW-01: `streamToBuffer` is unsafe when a stream emits string chunks
96-
97-
**File:** `src/utils.ts:6-14`
98-
**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.
99-
100-
```typescript
101-
const chunks: Buffer[] = [];
102-
stream.on("data", (chunk) => chunks.push(chunk)); // chunk may be a string
103-
// ...
104-
Buffer.concat(chunks); // throws TypeError if any chunk is a string
105-
```
106-
107-
The `DataStream` interface's `on("data")` listener signature is `(...args: any[])`, so TypeScript does not catch this.
108-
109-
**Suggested fix:**
110-
111-
```typescript
112-
stream.on("data", (chunk: Buffer | string) =>
113-
chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)),
114-
);
115-
```
116-
117-
---
118-
119-
### LOW-02: `AzureFileStore.getInfo()` falls back to `new Date()` for missing `lastModified`
120-
121-
**File:** `src/file-store.ts:217`
122-
**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.
123-
124-
```typescript
125-
lastModified: properties.lastModified || new Date(),
126-
```
127-
128-
The same pattern exists in `GCPFileStore.getInfo()` at line 300.
129-
130-
**Suggested fix:** Throw or surface the absence explicitly rather than substituting the current time:
131-
132-
```typescript
133-
lastModified: properties.lastModified ?? (() => { throw new Error("lastModified missing from Azure response"); })(),
134-
```
135-
136-
Or, if a sentinel is acceptable, document it clearly and use a fixed epoch (`new Date(0)`) so callers can detect the fallback.
137-
138-
---
139-
14066
## Summary
14167

14268
| Severity | Remaining |
14369
| --------------------------- | ---------- |
144-
| **HIGH** | 1 |
14570
| **MEDIUM** | 1 |
146-
| **LOW** | 2 |
147-
| **TOTAL** | **4 open** |
148-
| **Fixed** | 0 |
149-
| **False Positives Removed** | 0 |
71+
| **TOTAL** | **1 open** |
72+
| **Fixed** | 3 |
73+
| **False Positives Removed** | 1 |

src/file-store.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ class AzureFileStore implements FileStore {
214214
return {
215215
size: properties.contentLength || 0,
216216
contentType: properties.contentType || "application/octet-stream",
217-
lastModified: properties.lastModified || new Date(),
217+
lastModified: properties.lastModified || new Date(0),
218218
};
219219
} catch (err) {
220220
if (err.statusCode === 404) {
@@ -297,7 +297,7 @@ class GCPFileStore implements FileStore {
297297
contentType: metadata.contentType || "application/octet-stream",
298298
lastModified: metadata.updated
299299
? new Date(metadata.updated)
300-
: new Date(),
300+
: new Date(0),
301301
};
302302
} catch (err) {
303303
if (err.code === 404) {
@@ -324,7 +324,7 @@ class S3FileStore implements FileStore {
324324
if (err instanceof S3.NoSuchKey) {
325325
return false;
326326
}
327-
if (err["$metadata"].httpStatusCode === 404) {
327+
if (err["$metadata"]?.httpStatusCode === 404) {
328328
return false;
329329
}
330330
throw err;

src/utils.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@ export interface DataStream {
55
export function streamToBuffer(stream: DataStream): Promise<Buffer> {
66
return new Promise<Buffer>((resolve, reject) => {
77
const chunks: Buffer[] = [];
8-
stream.on("data", (chunk) => chunks.push(chunk));
8+
stream.on("data", (chunk: Buffer | string) =>
9+
chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)),
10+
);
911
stream.on("error", (err) => reject(err));
1012
stream.on("end", () =>
1113
resolve(chunks.length === 1 ? chunks[0] : Buffer.concat(chunks)),

0 commit comments

Comments
 (0)