From f86e55f104dd02f9cacd362f5ab848d6729c5994 Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 19 Aug 2026 22:46:52 +0200 Subject: [PATCH] =?UTF-8?q?chore(server):=20final-review=20polish=20?= =?UTF-8?q?=E2=80=94=20stale=20CI=20refs,=20dockerignore=20key,=20boundary?= =?UTF-8?q?=20pins,=20zero-byte=20tempfile=20contract=20case?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .dockerignore | 1 + server/src/app-config/env.ts | 11 +++++++++-- server/src/nest/storage/drivers/s3.driver.ts | 9 ++++++++- server/tests/unit/app-config/validate.test.ts | 6 ++++++ .../unit/nest/storage/s3.driver.contract.test.ts | 4 ++-- .../unit/nest/storage/storage-driver.contract.ts | 10 ++++++++++ 6 files changed, 36 insertions(+), 5 deletions(-) diff --git a/.dockerignore b/.dockerignore index a9aa4e24e..72a0b6b6a 100644 --- a/.dockerignore +++ b/.dockerignore @@ -16,6 +16,7 @@ chart/ docs/ docker-compose.yml docker-compose.minio-test.yml +minio-licence.key unraid-template.xml *.sqlite *.sqlite-shm diff --git a/server/src/app-config/env.ts b/server/src/app-config/env.ts index aebd55417..124367454 100644 --- a/server/src/app-config/env.ts +++ b/server/src/app-config/env.ts @@ -84,7 +84,14 @@ const S3_VARS = [ 'TREK_S3_RETRIES', 'TREK_S3_TIMEOUT_MS', ] as const; -const S3_REQUIRED = S3_VARS.slice(0, 4); +// Explicit, not `S3_VARS.slice(0, 4)` — the required set must not silently +// shift if S3_VARS is ever reordered. +const S3_REQUIRED: (typeof S3_VARS)[number][] = [ + 'TREK_S3_ENDPOINT', + 'TREK_S3_BUCKET', + 'TREK_S3_ACCESS_KEY_ID', + 'TREK_S3_SECRET_ACCESS_KEY', +]; /** * All-or-nothing: any TREK_S3_* set means the operator wants off-box storage; @@ -92,7 +99,7 @@ const S3_REQUIRED = S3_VARS.slice(0, 4); * Prefix shape delegates to storage-keys — the single source of key rules. */ function s3Preconditions(raw: RawEnv): string[] { - const present = (key: string): boolean => Boolean(raw[key]?.trim()); + const present = (key: (typeof S3_VARS)[number]): boolean => Boolean(raw[key]?.trim()); if (!S3_VARS.some(present)) return []; const problems: string[] = []; for (const key of S3_REQUIRED) { diff --git a/server/src/nest/storage/drivers/s3.driver.ts b/server/src/nest/storage/drivers/s3.driver.ts index b789efee4..7ae19b122 100644 --- a/server/src/nest/storage/drivers/s3.driver.ts +++ b/server/src/nest/storage/drivers/s3.driver.ts @@ -124,7 +124,8 @@ function isNotFound(err: unknown): boolean { * `signedHeaders()` (aws4.js:311-322, which just map/join whatever * `filterHeaders()` produced) sign `content-length` *twice* * (`SignedHeaders=content-length;content-length;...`, confirmed live - * against MinIO in the repro below). MinIO's own canonicalization of the + * against MinIO (see the manual procedure in docker-compose.minio-test.yml)). + * MinIO's own canonicalization of the * headers it actually received doesn't reproduce that duplicate, so the * signatures disagree — 403. Any non-empty body makes * `headers['content-length']` a truthy number, so aws4's guard never fires @@ -480,6 +481,12 @@ function peekStream(source: Readable, limit: number): Promise<{ chunks: Buffer[] const chunk = Buffer.isBuffer(raw) ? raw : Buffer.from(raw); chunks.push(chunk); size += chunk.length; + // Strictly `>`, not `>=`: a stream ending at exactly `limit` bytes is + // "ended within the limit" and buffers to PutObject, while a + // LocalTempFile of exactly `MULTIPART_THRESHOLD` (see put()'s `size < + // MULTIPART_THRESHOLD` check) goes to Upload instead. Both boundaries + // are correct on their own terms — deliberate asymmetry, not a bug; + // do not "align" them. if (size > limit) { cleanup(); resolve({ chunks, size, ended: false }); diff --git a/server/tests/unit/app-config/validate.test.ts b/server/tests/unit/app-config/validate.test.ts index aeeda636c..b288e3d93 100644 --- a/server/tests/unit/app-config/validate.test.ts +++ b/server/tests/unit/app-config/validate.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, afterEach } from 'vitest'; import { validateEnvAtBoot, readEnv } from '../../../src/app-config/env'; +import { deriveS3 } from '../../../src/app-config/derive'; describe('validateEnvAtBoot', () => { afterEach(() => { @@ -193,6 +194,11 @@ describe('validateEnvAtBoot — S3 all-or-nothing preconditions', () => { /Invalid environment configuration \(2 problems\)/, ); }); + it('S3-BOOT-006: a slash-wrapped key prefix passes boot validation (shape-checked stripped of its slashes), while deriveS3 keeps it un-normalized', () => { + const withPrefix = { ...full, TREK_S3_KEY_PREFIX: '/trek/prod/' }; + expect(() => validateEnvAtBoot(withPrefix)).not.toThrow(); + expect(deriveS3(withPrefix).keyPrefix).toBe('/trek/prod/'); + }); }); describe('readEnv', () => { diff --git a/server/tests/unit/nest/storage/s3.driver.contract.test.ts b/server/tests/unit/nest/storage/s3.driver.contract.test.ts index 39fcce00f..c504bb999 100644 --- a/server/tests/unit/nest/storage/s3.driver.contract.test.ts +++ b/server/tests/unit/nest/storage/s3.driver.contract.test.ts @@ -8,8 +8,8 @@ import { S3Driver } from '../../../../src/nest/storage/drivers/s3.driver'; import { describeStorageDriver, type DriverHarness } from './storage-driver.contract'; /** - * Env-gated: engages only when TREK_TEST_S3_* is set (CI's MinIO job; locally - * via `docker compose -f docker-compose.minio-test.yml up`). Local runs stay + * Env-gated: engages only when TREK_TEST_S3_* is set (manual-only, via + * `docker compose -f docker-compose.minio-test.yml up`). Local runs stay * hermetic. Deviation from local/mirror's bare top-level call: the gate must * wrap describeStorageDriver because the contract file owns its own describe. */ diff --git a/server/tests/unit/nest/storage/storage-driver.contract.ts b/server/tests/unit/nest/storage/storage-driver.contract.ts index c50e0fb8a..472cd9123 100644 --- a/server/tests/unit/nest/storage/storage-driver.contract.ts +++ b/server/tests/unit/nest/storage/storage-driver.contract.ts @@ -98,6 +98,16 @@ export function describeStorageDriver(name: string, makeHarness: () => Promise { + const tmp = await h.makeTempFile(''); + await h.driver.put('a/empty-tmpfile.bin', { tmpPath: tmp }); + expect((await readAll('a/empty-tmpfile.bin')).length).toBe(0); + expect((await h.driver.stat('a/empty-tmpfile.bin'))!.size).toBe(0); + expect(fs.existsSync(tmp)).toBe(false); // ownership transferred + await h.driver.delete('a/empty-tmpfile.bin'); + expect(await h.driver.stat('a/empty-tmpfile.bin')).toBeNull(); + }); + it('rejects getStream on a missing key with StorageNotFoundError', async () => { await expect(h.driver.getStream('a/nope.bin')).rejects.toBeInstanceOf(StorageNotFoundError); });