refactor: one place decides which bucket, on which S3, with which credentials

Three S3 configurations had grown separately — S3_* for documents,
GENERATED_IMAGES_S3_* for images, and AUDIO_BACKUPS_S3_* after them — with
different key names and their own client construction. That is why moving
storage meant hunting through several files.

src/utils/objectStorage.js now resolves settings for any purpose: its own
variables first, then the shared S3_* ones, with a per-purpose bucket name
(S3_BUCKET_AUDIO_BACKUPS). One endpoint plus three bucket names is enough
for the whole app, and a purpose that needs its own account still overrides
everything. Audio backups and documents use it; generated images keeps its
own tested storage module, whose variable names the resolver already
understands.

Nothing existing has to change: S3_ACCESS_KEY_ID, S3_SECRET_ACCESS_KEY and
the AWS_* fallbacks still resolve, and path-style addressing keeps each
purpose's previous default — off for documents, so a Backblaze endpoint
behaves as before, on where a custom endpoint implies MinIO. A _FILE
credential now always beats an inline one, so a mounted secret cannot be
shadowed by an inherited environment variable.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dv6sqaY6Vq3ChZHMem3cnU

docs/speech.md

@@ -92,6 +92,32 @@ drift apart.
 
 Treat audio backups as sensitive clinical data even when encrypted.
 
+### Where object storage settings come from
+
+`src/utils/objectStorage.js` resolves them for every purpose the same way, so
+moving the app to a different MinIO — or to a real S3 — is one set of variables
+rather than three schemes. For a purpose (`documents`, `generated-images`,
+`audio-backups`) it reads, in order:
+
+1. that purpose's own variables — `AUDIO_BACKUPS_S3_ENDPOINT`, `..._BUCKET`,
+   `..._REGION`, `..._ACCESS_KEY[_FILE]`, `..._SECRET_KEY[_FILE]`;
+2. the shared ones — `S3_ENDPOINT`, `S3_REGION`, `S3_ACCESS_KEY[_FILE]`,
+   `S3_SECRET_KEY[_FILE]`;
+3. a per-purpose bucket — `S3_BUCKET_AUDIO_BACKUPS`, `S3_BUCKET_GENERATED_IMAGES`.
+
+So one endpoint plus three bucket names covers everything, while a purpose that
+needs its own account overrides all of it. A `_FILE` variant always beats an
+inline value, because a mounted secret should not be shadowed by an inherited
+environment variable. No bucket means "not configured", which is never an error
+— all three are optional.
+
+Every name previously accepted still works, including `S3_ACCESS_KEY_ID`,
+`S3_SECRET_ACCESS_KEY` and the `AWS_*` fallbacks for documents, and
+`GENERATED_IMAGES_S3_*`. Path-style addressing keeps each purpose's old default
+(off for documents, so Backblaze keeps working) unless `S3_FORCE_PATH_STYLE`
+says otherwise; a custom endpoint turns it on where there was no older default,
+because that is nearly always MinIO.
+
 ### Switching audio backups to MinIO
 
 Storing every recording, rather than only the failures, makes object storage

src/routes/documents.js

@@ -27,30 +27,15 @@ var ALLOWED_TYPES = [
 
 // Lazy-load S3 client
 var _s3Client = null;
+// Settings come from src/utils/objectStorage.js, so documents, generated images
+// and audio backups all resolve the same way: the purpose's own variables, then
+// the shared S3_* ones. Every name this route has ever accepted still works.
 function getS3Client() {
   if (_s3Client) return _s3Client;
   try {
-    var { S3Client } = require('@aws-sdk/client-s3');
-    var region = process.env.S3_REGION || process.env.AWS_BEDROCK_REGION || 'us-east-1';
-    var config = { region: region };
-
-    // Custom endpoint for S3-compatible providers (Backblaze B2, MinIO, etc.)
-    if (process.env.S3_ENDPOINT) {
-      config.endpoint = process.env.S3_ENDPOINT;
-      config.forcePathStyle = process.env.S3_FORCE_PATH_STYLE === 'true'; // Required for MinIO
-    }
-
-    // Credentials: use S3-specific keys first, fall back to AWS keys
-    var accessKey = process.env.S3_ACCESS_KEY_ID || process.env.AWS_ACCESS_KEY_ID;
-    var secretKey = process.env.S3_SECRET_ACCESS_KEY || process.env.AWS_SECRET_ACCESS_KEY;
-    if (accessKey && secretKey) {
-      config.credentials = {
-        accessKeyId: accessKey,
-        secretAccessKey: secretKey
-      };
-    }
-
-    _s3Client = new S3Client(config);
+    var store = require('../utils/objectStorage').storeFor('documents');
+    if (!store) return null;
+    _s3Client = store.client;
     return _s3Client;
   } catch (e) {
     return null;

src/utils/audioBackupStore.js

@@ -5,12 +5,13 @@
 // material: every one is kept for 24 hours, whether its transcription
 // succeeded or not, and it is always compressed and encrypted at rest.
 //
-// Payload goes to object storage when AUDIO_BACKUPS_S3_* is configured, and
-// to Postgres otherwise. Metadata (owner, module, sizes, expiry) always lives
-// in Postgres, so listing, ownership and expiry work the same either way.
+// Payload goes to object storage when one is configured for the audio-backups
+// purpose (see src/utils/objectStorage.js — its own AUDIO_BACKUPS_S3_* or the
+// shared S3_* plus a bucket name), and to Postgres otherwise. Metadata (owner,
+// module, sizes, expiry) always lives in Postgres, so listing, ownership and
+// expiry work the same either way.
 // ============================================================
 
-var fs = require('fs');
 var zlib = require('zlib');
 var db = require('../db/database');
 var cryptoUtil = require('./crypto');
@@ -28,37 +29,16 @@ function gunzip(buffer) {
   });
 }
 
-// Credentials come from files, as the rest of the stack does, so they are never
-// environment strings in a process listing.
-function readSecret(value, file) {
-  if (file) return fs.readFileSync(file, 'utf8').trim();
-  return value || '';
-}
+var objectStorage = require('./objectStorage');
 
 var _client = null;
 function objectStore(env) {
-  env = env || process.env;
-  if (!env.AUDIO_BACKUPS_S3_ENDPOINT || !env.AUDIO_BACKUPS_S3_BUCKET) return null;
   if (_client) return _client;
-  var { S3Client } = require('@aws-sdk/client-s3');
-  _client = {
-    Bucket: env.AUDIO_BACKUPS_S3_BUCKET,
-    client: new S3Client({
-      endpoint: env.AUDIO_BACKUPS_S3_ENDPOINT,
-      region: env.AUDIO_BACKUPS_S3_REGION || 'us-east-1',
-      forcePathStyle: true,
-      credentials: {
-        accessKeyId: readSecret(env.AUDIO_BACKUPS_S3_ACCESS_KEY, env.AUDIO_BACKUPS_S3_ACCESS_KEY_FILE),
-        secretAccessKey: readSecret(env.AUDIO_BACKUPS_S3_SECRET_KEY, env.AUDIO_BACKUPS_S3_SECRET_KEY_FILE)
-      },
-      maxAttempts: 2,
-      requestHandler: { connectionTimeout: 3000, requestTimeout: 20000 }
-    })
-  };
+  _client = objectStorage.storeFor('audio-backups', env);
   return _client;
 }
 
-function isObjectStoreConfigured(env) { return !!objectStore(env); }
+function isObjectStoreConfigured(env) { return objectStorage.isConfigured('audio-backups', env); }
 
 // Keyed by owner so one user's recordings can never be addressed by another,
 // even if an id leaks.

src/utils/objectStorage.js

@@ -0,0 +1,115 @@
+// ============================================================
+// OBJECT STORAGE SETTINGS
+// One place that answers "which bucket, on which S3, with which credentials",
+// so pointing the whole app at a different MinIO — or at a real S3 — is one set
+// of variables rather than three schemes that grew separately.
+//
+// Each purpose (documents, generated images, audio backups) reads, in order:
+//
+//   1. its own variables       AUDIO_BACKUPS_S3_ENDPOINT, ...
+//   2. the shared ones         S3_ENDPOINT, S3_REGION, S3_ACCESS_KEY, ...
+//   3. a per-purpose bucket    S3_BUCKET_AUDIO_BACKUPS
+//
+// So one external endpoint plus three bucket names is enough, while a purpose
+// that needs its own account keeps overriding everything. Credentials can be
+// given inline or, preferably, as a path to a file — a file keeps them out of
+// the process environment, which is where `docker inspect` reads from.
+// ============================================================
+
+var fs = require('fs');
+
+// Historic names, kept working so no deployment has to be edited to upgrade.
+var LEGACY = {
+  documents: {
+    endpoint: ['S3_ENDPOINT'],
+    region: ['S3_REGION', 'AWS_BEDROCK_REGION'],
+    bucket: ['S3_BUCKET'],
+    accessKey: ['S3_ACCESS_KEY_ID', 'AWS_ACCESS_KEY_ID'],
+    secretKey: ['S3_SECRET_ACCESS_KEY', 'AWS_SECRET_ACCESS_KEY'],
+    forcePathStyle: ['S3_FORCE_PATH_STYLE']
+  }
+};
+
+function prefixOf(purpose) {
+  return String(purpose).toUpperCase().replace(/[^A-Z0-9]+/g, '_') + '_S3_';
+}
+
+function firstValue(env, names) {
+  for (var i = 0; i < names.length; i++) {
+    var value = env[names[i]];
+    if (value !== undefined && value !== null && String(value) !== '') return String(value);
+  }
+  return '';
+}
+
+// A *_FILE variant always wins over an inline value: if someone has gone to the
+// trouble of mounting a secret, an inherited environment variable must not
+// quietly take precedence over it.
+function readCredential(env, names) {
+  var fileNames = names.map(function (name) { return name + '_FILE'; });
+  var file = firstValue(env, fileNames);
+  if (file) {
+    try { return fs.readFileSync(file, 'utf8').trim(); }
+    catch (e) { throw new Error('Cannot read credential file ' + file + ': ' + e.message); }
+  }
+  return firstValue(env, names);
+}
+
+/**
+ * Resolve the settings for one purpose. Returns null when no endpoint and no
+ * bucket can be found, which callers read as "not configured" — never as an
+ * error, because every one of these is optional.
+ */
+function settingsFor(purpose, env) {
+  env = env || process.env;
+  var own = prefixOf(purpose);
+  var legacy = LEGACY[purpose] || {};
+
+  var endpoint = firstValue(env, [own + 'ENDPOINT'].concat(legacy.endpoint || [], ['S3_ENDPOINT']));
+  var bucket = firstValue(env, [own + 'BUCKET', 'S3_BUCKET_' + own.replace(/_S3_$/, '')].concat(legacy.bucket || []));
+  if (!bucket) return null;
+
+  var region = firstValue(env, [own + 'REGION'].concat(legacy.region || [], ['S3_REGION'])) || 'us-east-1';
+  var accessKey = readCredential(env, [own + 'ACCESS_KEY'].concat(legacy.accessKey || [], ['S3_ACCESS_KEY']));
+  var secretKey = readCredential(env, [own + 'SECRET_KEY'].concat(legacy.secretKey || [], ['S3_SECRET_KEY']));
+
+  // MinIO needs path-style addressing; hosted S3 does not care, and Backblaze
+  // prefers virtual-hosted. An explicit setting always wins. Where a purpose
+  // already had its own default, that default is kept: documents has always
+  // been off unless asked for, so a Backblaze endpoint keeps working.
+  var forced = firstValue(env, [own + 'FORCE_PATH_STYLE'].concat(legacy.forcePathStyle || [], ['S3_FORCE_PATH_STYLE']));
+  var forcePathStyle = forced ? forced === 'true' : (legacy.forcePathStyle ? false : !!endpoint);
+
+  return {
+    purpose: purpose,
+    endpoint: endpoint || undefined,
+    region: region,
+    bucket: bucket,
+    forcePathStyle: forcePathStyle,
+    credentials: (accessKey && secretKey) ? { accessKeyId: accessKey, secretAccessKey: secretKey } : undefined
+  };
+}
+
+function isConfigured(purpose, env) { return !!settingsFor(purpose, env); }
+
+// Built per call site; callers cache their own client.
+function createClient(settings) {
+  var { S3Client } = require('@aws-sdk/client-s3');
+  var config = {
+    region: settings.region,
+    maxAttempts: 2,
+    requestHandler: { connectionTimeout: 3000, requestTimeout: 20000 }
+  };
+  if (settings.endpoint) { config.endpoint = settings.endpoint; config.forcePathStyle = settings.forcePathStyle; }
+  if (settings.credentials) config.credentials = settings.credentials;
+  return new S3Client(config);
+}
+
+// Convenience: settings plus a client, or null when the purpose is unconfigured.
+function storeFor(purpose, env) {
+  var settings = settingsFor(purpose, env);
+  if (!settings) return null;
+  return { settings: settings, Bucket: settings.bucket, client: createClient(settings) };
+}
+
+module.exports = { settingsFor, isConfigured, createClient, storeFor, prefixOf };

test/backend-hardening.test.js

@@ -81,3 +81,48 @@ test('admin routers state their own authentication, not mount order', () => {
   assert.match(auth, /async function adminMiddleware\(req, res, next\) \{\s*\n\s*if \(!req\.user \|\| req\.user\.role !== 'admin'\)/,
     'and the role check stays a role check, so it fails closed on its own');
 });
+
+// Three S3 schemes grew separately — S3_*, GENERATED_IMAGES_S3_*, and later
+// audio backups — which is why pointing the app at a different MinIO meant
+// hunting through three files. One resolver answers it for every purpose.
+test('object storage resolves the same way for every purpose', () => {
+  const storage = require('../src/utils/objectStorage');
+
+  // One endpoint plus a bucket name per purpose is enough for all of them.
+  const shared = { S3_ENDPOINT: 'https://s3.example.com', S3_ACCESS_KEY: 'AK', S3_SECRET_KEY: 'SK',
+    S3_BUCKET_AUDIO_BACKUPS: 'audio', S3_BUCKET_GENERATED_IMAGES: 'images', S3_BUCKET: 'docs' };
+  for (const [purpose, bucket] of [['audio-backups', 'audio'], ['generated-images', 'images'], ['documents', 'docs']]) {
+    const resolved = storage.settingsFor(purpose, shared);
+    assert.equal(resolved.bucket, bucket, purpose + ' finds its bucket');
+    assert.equal(resolved.endpoint, 'https://s3.example.com');
+    assert.deepEqual(resolved.credentials, { accessKeyId: 'AK', secretAccessKey: 'SK' });
+  }
+
+  // A purpose that needs its own account still overrides everything.
+  const overridden = storage.settingsFor('audio-backups',
+    Object.assign({}, shared, { AUDIO_BACKUPS_S3_ENDPOINT: 'http://assets:9000', AUDIO_BACKUPS_S3_BUCKET: 'audio-backups' }));
+  assert.equal(overridden.endpoint, 'http://assets:9000');
+  assert.equal(overridden.bucket, 'audio-backups');
+
+  // Existing deployments keep working untouched, including the old key names.
+  const legacy = storage.settingsFor('documents',
+    { S3_BUCKET: 'd', S3_REGION: 'us-west-004', S3_ENDPOINT: 'https://b2', S3_ACCESS_KEY_ID: 'A', S3_SECRET_ACCESS_KEY: 'B' });
+  assert.equal(legacy.region, 'us-west-004');
+  assert.deepEqual(legacy.credentials, { accessKeyId: 'A', secretAccessKey: 'B' });
+  // Documents defaulted path-style off; a Backblaze endpoint must keep working.
+  assert.equal(legacy.forcePathStyle, false);
+  assert.equal(storage.settingsFor('audio-backups', { AUDIO_BACKUPS_S3_BUCKET: 'a', AUDIO_BACKUPS_S3_ENDPOINT: 'http://assets:9000' }).forcePathStyle, true,
+    'but MinIO needs it, so a custom endpoint turns it on where there is no older default');
+
+  // No bucket means "not configured" — never an error, since all of this is optional.
+  assert.equal(storage.settingsFor('audio-backups', {}), null);
+  assert.equal(storage.isConfigured('audio-backups', {}), false);
+
+  // A mounted secret must not be overridden by an inherited environment value.
+  const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path');
+  const keyFile = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'objstore-')), 'key');
+  fs.writeFileSync(keyFile, 'from-file\n');
+  const fileWins = storage.settingsFor('audio-backups',
+    { AUDIO_BACKUPS_S3_BUCKET: 'a', AUDIO_BACKUPS_S3_ACCESS_KEY: 'inline', AUDIO_BACKUPS_S3_ACCESS_KEY_FILE: keyFile, AUDIO_BACKUPS_S3_SECRET_KEY: 'S' });
+  assert.equal(fileWins.credentials.accessKeyId, 'from-file');
+});

test/transcription-memory-policy.test.js

@@ -159,7 +159,9 @@ test('every recording is kept for 24 hours, not only the failures', () => {
     'a storage failure must not lose the transcription someone is waiting for');
 
   // Object storage when configured, the encrypted database column otherwise.
-  assert.match(store, /if \(!env\.AUDIO_BACKUPS_S3_ENDPOINT \|\| !env\.AUDIO_BACKUPS_S3_BUCKET\) return null;/);
+  // Which one, and with what credentials, is resolved centrally now.
+  assert.match(store, /_client = objectStorage\.storeFor\('audio-backups', env\);/);
+  assert.match(store, /objectStorage\.isConfigured\('audio-backups', env\)/);
   assert.match(store, /cryptoUtil\.encryptBuffer\(compressed\)/, 'compressed and encrypted either way');
   assert.match(store, /'recordings\/' \+ userId \+ '\/'/, 'keys are scoped to their owner');
   assert.match(store, /WHERE id = \$1 AND user_id = \$2 AND expires_at > NOW\(\)/, 'ownership and expiry are in the query');