diff --git a/server/src/app-config/README.md b/server/src/app-config/README.md index a708be12b..a3c7cec7a 100644 --- a/server/src/app-config/README.md +++ b/server/src/app-config/README.md @@ -68,9 +68,15 @@ PORT, HOST, TRUST_PROXY, SESSION_DURATION(_REMEMBER), MCP_SESSION_TTL, MCP_MAX_SESSION_PER_USER, MCP_SSE_KEEPALIVE, TREK_PLUGIN_RPC_*/LOG_*/MAX_RSS_MB, TREK_PLUGIN_REGISTRY_URL, TREK_WIKI_DIR*, TREK_PLACE_PHOTO_DIR, BACKUP_*, TRANSIT_API_URL, LOG_LEVEL*, ALLOW_INTERNAL_NETWORK*, DEFAULT_LANGUAGE, -TREK_DB_FILE, TREK_DB_JOURNAL_MODE, TREK_DB_SYNCHRONOUS, ENCRYPTION_KEY. +TREK_DB_FILE, TREK_DB_JOURNAL_MODE, TREK_DB_SYNCHRONOUS, ENCRYPTION_KEY**. (* frozen today because the consuming module captures it at import; tests that override these set them at file top, before the SUT import.) +(** ENCRYPTION_KEY is boot-stable only for the cipher key material itself — +`src/config.ts` resolves and freezes it at process start, per the exemption +below. `security.encryptionKeySet` (the storage admin-config encryption gate) +is a *different* read of the same variable: `deriveSecurity()` reads it live +via `readEnv()`/`RuntimeEnvService`, so a value set after boot is observed on +the next call — it does not retroactively unlock the frozen cipher.) **Tombstoned** (boot-aborting since the admin-config slice — configure storage in the admin UI / `data/storage-config.json` instead): `TREK_S3_ENDPOINT`, diff --git a/server/src/nest/storage/storage-admin.service.ts b/server/src/nest/storage/storage-admin.service.ts index dff077200..c3ccdeec8 100644 --- a/server/src/nest/storage/storage-admin.service.ts +++ b/server/src/nest/storage/storage-admin.service.ts @@ -11,6 +11,7 @@ import { import { StorageService } from './storage.service'; import { SEED_CONFIG_PATH } from './storage-paths'; import { + assertNoMaskSentinels, decryptBackendSecrets, encryptionGateError, encryptStorageSecrets, @@ -62,6 +63,10 @@ export class StorageAdminService { /** Full-document replace of the two settings rows. Throws StorageBackendError on any refusal. */ applyConfig(config: StorageConfig): void { const unmasked = unmaskStorageConfig(config, this.storedBackendsRow()); + // unmask only resolves the secret fields it knows about; a mask sentinel + // submitted in a non-secret field would otherwise pass through untouched + // and get persisted verbatim as garbage-in. + assertNoMaskSentinels(unmasked); const plaintext = listPlaintextSecrets(unmasked); if (plaintext.length > 0 && !this.env.env().security.encryptionKeySet) { throw new StorageBackendError(encryptionGateError(plaintext[0]!)); diff --git a/server/src/nest/storage/storage-secrets.ts b/server/src/nest/storage/storage-secrets.ts index 68a99d4f6..8c2b803ab 100644 --- a/server/src/nest/storage/storage-secrets.ts +++ b/server/src/nest/storage/storage-secrets.ts @@ -49,10 +49,19 @@ export function listPlaintextSecrets(config: StorageConfig): PlaintextSecret[] { return found; } -/** The seed file has no mask round-trip — a literal mask sentinel is always a mistake. */ +/** + * No literal mask sentinel may ever become a stored value, in ANY option + * field — not just the fields the type registry marks `secret`. Secret + * fields are already covered structurally (`unmaskStorageConfig` throws + * when a mask has no stored counterpart); this closes the same hole for a + * mask sentinel submitted in a non-secret field, which would otherwise be + * persisted verbatim as garbage-in. Also the seed-file guard: the seed file + * has no mask round-trip, so a literal mask sentinel there is always a + * mistake. + */ export function assertNoMaskSentinels(config: StorageConfig): void { for (const backend of config.backends) { - for (const field of storageSecretFields(backend.type)) { + for (const field of Object.keys(backend.options as Record)) { if ((backend.options as Record)[field] === MASKED_SETTING_VALUE) { throw new StorageBackendError( `backend '${backend.name}' field '${field}' is the mask sentinel — a mask can never become a stored value`, @@ -62,11 +71,11 @@ export function assertNoMaskSentinels(config: StorageConfig): void { } } -/** Fail-closed refusal text: the silent JWT-derived fallback key is not accepted for credentials. */ +/** Fail-closed refusal text: the implicit key persisted in the data directory is not accepted for credentials — it rides inside backups. */ export function encryptionGateError(secret: PlaintextSecret): string { return ( `backend '${secret.backend}' has a plaintext secret '${secret.field}' but ENCRYPTION_KEY is not set — ` + - 'set ENCRYPTION_KEY explicitly to save credentialed storage backends (the JWT-derived fallback key is not accepted)' + 'set ENCRYPTION_KEY explicitly to save credentialed storage backends (the implicit key persisted in the data directory is not accepted: it rides inside backups)' ); } diff --git a/server/tests/e2e/storage-admin.e2e.test.ts b/server/tests/e2e/storage-admin.e2e.test.ts index 59c9b6493..b67c2dee1 100644 --- a/server/tests/e2e/storage-admin.e2e.test.ts +++ b/server/tests/e2e/storage-admin.e2e.test.ts @@ -87,6 +87,12 @@ describe('Storage admin e2e (real auth + admin guard + managed guard + temp SQLi server = app.getHttpServer(); }); + // Clears the persisted rows only — NOT the registry's in-memory last-good + // snapshot (StorageRegistryService keeps state across requests within one + // `app`, and this suite builds `app` once in beforeAll). A test added after + // STORE2E-004 that expects to see fresh defaults must call GET (which + // renders the registry's live snapshot) rather than assume the deleted rows + // reset it — the registry only re-reads app_settings on `reload()`/init. beforeEach(() => { db.exec("DELETE FROM app_settings WHERE key LIKE 'storage.%'"); db.exec('DELETE FROM audit_log'); @@ -99,6 +105,10 @@ describe('Storage admin e2e (real auth + admin guard + managed guard + temp SQLi it('STORE2E-001 401 without a session', async () => { expect((await request(server).get('/api/admin/storage')).status).toBe(401); + // PUT stays behind the same guard chain — pins that the guards are declared + // class-level (JwtAuthGuard + AdminGuard on the controller), not re-added + // per handler where a future handler could forget them. + expect((await request(server).put('/api/admin/storage').send({ backends: [], categories: {} })).status).toBe(401); }); it('STORE2E-002 403 for a non-admin', async () => { diff --git a/server/tests/unit/nest/storage/storage-admin.service.test.ts b/server/tests/unit/nest/storage/storage-admin.service.test.ts index f5a9aff59..5e07a5d5a 100644 --- a/server/tests/unit/nest/storage/storage-admin.service.test.ts +++ b/server/tests/unit/nest/storage/storage-admin.service.test.ts @@ -206,6 +206,18 @@ describe('StorageAdminService.applyConfig', () => { expect(registry.resolve('backups').backendName).toBe('backups-local'); // live state untouched }); + it('STORADM-018 a mask sentinel in a NON-secret field throws and persists nothing', () => { + const { service, uploadsRoot } = makeService(); + const before = readRow(BACKENDS_KEY); + expect(() => + service.applyConfig(configWith(uploadsRoot, { + backends: [{ name: 'off-box', type: 's3', options: { ...S3_OPTIONS, accessKeyId: MASKED_SETTING_VALUE } }], + categories: {}, + })), + ).toThrow("backend 'off-box' field 'accessKeyId' is the mask sentinel — a mask can never become a stored value"); + expect(readRow(BACKENDS_KEY)).toBe(before); + }); + it('STORADM-017 persists both rows in ONE transaction and reloads once', () => { const { service, registry, uploadsRoot } = makeService(); const txSpy = vi.spyOn(db, 'transaction'); diff --git a/server/tests/unit/nest/storage/storage-secrets.test.ts b/server/tests/unit/nest/storage/storage-secrets.test.ts index eeef28c46..7025d4f9a 100644 --- a/server/tests/unit/nest/storage/storage-secrets.test.ts +++ b/server/tests/unit/nest/storage/storage-secrets.test.ts @@ -54,7 +54,7 @@ describe('encryptionGateError', () => { expect(message).toContain('ENCRYPTION_KEY'); expect(message).toContain("'off-box'"); expect(message).toContain("'secretAccessKey'"); - expect(message).toContain('fallback'); + expect(message).toContain('not accepted'); }); }); @@ -104,10 +104,20 @@ describe('unmaskStorageConfig', () => { }); describe('assertNoMaskSentinels', () => { - it('throws when a seed-style config carries the mask sentinel', () => { + it('throws when a seed-style config carries the mask sentinel in a secret field', () => { expect(() => assertNoMaskSentinels(s3Config(MASKED_SETTING_VALUE))).toThrow('mask'); expect(() => assertNoMaskSentinels(s3Config('sk'))).not.toThrow(); }); + + it('throws for a mask sentinel in a NON-secret field too — the check is all-field, not secret-only', () => { + const config: StorageConfig = { + backends: [{ name: 'off-box', type: 's3', options: { ...S3_OPTIONS, accessKeyId: MASKED_SETTING_VALUE } }], + categories: {}, + }; + expect(() => assertNoMaskSentinels(config)).toThrow( + "backend 'off-box' field 'accessKeyId' is the mask sentinel — a mask can never become a stored value", + ); + }); }); describe('decryptBackendSecrets', () => {