Tell a failed OIDC sign-in apart, and come back to the right address

backend/src/config/index.js

@@ -1,6 +1,7 @@
 const path = require('path');
 const crypto = require('crypto');
 const env = require('./env');
+const { resolveSessionSecret } = require('./sessionSecret');
 const constants = require('./constants');
 const loggingConfig = require('./logging');
 const { parseByteSize } = require('../utils/env');
@@ -251,7 +252,7 @@ const authMode = determineAuthMode();
 
 const auth = {
   enabled: authMode === 'disabled' ? false : env.AUTH_ENABLED !== false,
-  sessionSecret: env.SESSION_SECRET || crypto.randomBytes(32).toString('hex'),
+  sessionSecret: resolveSessionSecret({ configured: env.SESSION_SECRET, configDir }),
   sessionMaxAgeMs: env.SESSION_MAX_AGE_DAYS * 24 * 60 * 60 * 1000, // Convert days to milliseconds
   mode: authMode,
   oidc: {

backend/src/config/sessionSecret.js

@@ -0,0 +1,135 @@
+const crypto = require('crypto');
+const fs = require('fs');
+const path = require('path');
+const logger = require('../utils/logger');
+
+/**
+ * The secret sessions are signed with, when nobody configured one.
+ *
+ * It was drawn at random at every start. Sessions themselves outlive a restart
+ * — they are kept in CACHE_DIR/sessions.db — but a cookie signed with the
+ * previous secret no longer verifies, so every restart, every upgrade and every
+ * crash signed everyone out. The secrets derived from it (ONLYOFFICE without
+ * ONLYOFFICE_SECRET, the thumbnail links) changed with it.
+ *
+ * So the first start draws one and keeps it in CONFIG_DIR, and later starts read
+ * it back. SESSION_SECRET, when set, always wins and nothing is written.
+ *
+ * Synchronous on purpose: the configuration is required before anything else
+ * runs, and every value derived from the secret is computed at that moment.
+ */
+
+const SECRET_FILE_NAME = 'session-secret';
+const SECRET_PATTERN = /^[0-9a-f]{64}$/i;
+
+const draw = () => crypto.randomBytes(32).toString('hex');
+
+/** Read the stored secret: the secret, `null` when there is none to use, or the error. */
+const readStored = (file) => {
+  let contents;
+  try {
+    contents = fs.readFileSync(file, 'utf8');
+  } catch (error) {
+    if (error.code === 'ENOENT') return { secret: null };
+    return { error };
+  }
+
+  const value = contents.trim();
+  if (SECRET_PATTERN.test(value)) return { secret: value };
+
+  // Nothing of the file's contents is logged: a hand-written secret would
+  // otherwise land in the logs on its way to being replaced.
+  logger.warn(
+    { file, reason: value ? 'not a 64-character hexadecimal secret' : 'empty' },
+    'The stored session secret is unusable and is being replaced; sessions signed with it end ' +
+      'here. To choose the secret yourself, set SESSION_SECRET instead.'
+  );
+  return { secret: null };
+};
+
+/**
+ * Write the secret beside its final name, then rename it into place, so a start
+ * interrupted half-way leaves either no file or a whole one — never a truncated
+ * secret that the next start would have to throw away.
+ *
+ * The staging name is fixed rather than unique: a write that keeps failing (a
+ * full disk) then leaves one stray file that the next attempt reuses, not a new
+ * one per start. Removing it is not this module's business.
+ */
+const store = (configDir, file, secret) => {
+  fs.mkdirSync(configDir, { recursive: true });
+
+  const staging = path.join(configDir, `.${SECRET_FILE_NAME}.tmp`);
+  const fd = fs.openSync(staging, 'w', 0o600);
+  try {
+    // The mode given to open only applies to a file it creates; a staging file
+    // left by an older attempt keeps its own.
+    fs.fchmodSync(fd, 0o600);
+    fs.writeFileSync(fd, `${secret}\n`);
+    fs.fsyncSync(fd);
+  } finally {
+    fs.closeSync(fd);
+  }
+  fs.renameSync(staging, file);
+
+  // The rename only survives a power cut once the directory is on disk too.
+  // Not every platform lets a directory be opened for that, and the file is
+  // already in place, so a refusal here costs nothing but that guarantee.
+  try {
+    const dirFd = fs.openSync(configDir, 'r');
+    try {
+      fs.fsyncSync(dirFd);
+    } finally {
+      fs.closeSync(dirFd);
+    }
+  } catch {
+    /* best effort */
+  }
+};
+
+const warnEphemeral = (configDir, error, action) => {
+  logger.warn(
+    { directory: configDir, code: error.code || null, err: { message: error.message } },
+    `Could not ${action} the session secret in CONFIG_DIR, so a new one is used for this run ` +
+      'only: everyone will be signed out at the next restart. Make CONFIG_DIR writable by the ' +
+      'user the server runs as, or set SESSION_SECRET.'
+  );
+};
+
+/**
+ * @param {object} options
+ * @param {string|null|undefined} options.configured SESSION_SECRET, as read from the environment
+ * @param {string} options.configDir The resolved CONFIG_DIR
+ * @returns {string}
+ */
+const resolveSessionSecret = ({ configured, configDir }) => {
+  if (configured) return configured;
+
+  const file = path.join(configDir, SECRET_FILE_NAME);
+
+  const stored = readStored(file);
+  if (stored.error) {
+    // A file that exists and cannot be read belongs to someone else — another
+    // user, a mount gone wrong. Replacing it would throw away a secret that may
+    // still be good once the permissions are, so it is left alone.
+    warnEphemeral(configDir, stored.error, 'read');
+    return draw();
+  }
+  if (stored.secret) return stored.secret;
+
+  const secret = draw();
+  try {
+    store(configDir, file, secret);
+  } catch (error) {
+    warnEphemeral(configDir, error, 'store');
+    return secret;
+  }
+
+  logger.info(
+    { file },
+    'Generated a session secret and stored it in CONFIG_DIR; sessions now survive restarts'
+  );
+  return secret;
+};
+
+module.exports = { resolveSessionSecret, SECRET_FILE_NAME };

backend/src/errors/AppError.js

@@ -123,6 +123,20 @@ class UnsupportedMediaTypeError extends AppError {
 }
 
 /** 507: the storage cannot hold what is being sent. */
+/**
+ * 503 Service Unavailable - something this server depends on did not answer
+ *
+ * Not the caller's doing and not a misconfiguration: the request was right and
+ * can be made again. Saying 404 here is what sends an administrator to change
+ * settings that are already correct.
+ */
+class ServiceUnavailableError extends AppError {
+  constructor(message = 'Service unavailable', code = null) {
+    super(message, 503, code);
+    this.name = 'ServiceUnavailableError';
+  }
+}
+
 class InsufficientStorageError extends AppError {
   constructor(message = 'Insufficient storage') {
     super(message, 507, 'INSUFFICIENT_STORAGE');
@@ -140,5 +154,6 @@ module.exports = {
   RateLimitError,
   InternalError,
   UnsupportedMediaTypeError,
+  ServiceUnavailableError,
   InsufficientStorageError,
 };

backend/src/errors/errorCodes.js

@@ -10,6 +10,8 @@ const ErrorCodes = {
   AUTH_PASSWORD_INCORRECT: 'AUTH_PASSWORD_INCORRECT',
   AUTH_INVALID_TOTP_CODE: 'AUTH_INVALID_TOTP_CODE',
   AUTH_PASSKEY_REJECTED: 'AUTH_PASSKEY_REJECTED',
+  AUTH_OIDC_NOT_CONFIGURED: 'AUTH_OIDC_NOT_CONFIGURED',
+  AUTH_OIDC_PROVIDER_UNAVAILABLE: 'AUTH_OIDC_PROVIDER_UNAVAILABLE',
 
   // Validation (400)
   VALIDATION_EMAIL_REQUIRED: 'VALIDATION_EMAIL_REQUIRED',

backend/src/middleware/errorHandler.js

@@ -73,9 +73,21 @@ const storageRefusal = (err) => {
   return sentence ? [403, sentence] : null;
 };
 
+/**
+ * The addresses a browser is sent to, rather than fetches, on the way through
+ * an identity provider: where a sign-in starts, and where it comes back.
+ *
+ * A failure at either is a page the person is looking at, so it has to land
+ * back on the sign-in screen. `/login` is the provider's own route, mounted
+ * only when there is a provider to mount; `/api/auth/oidc/login` answers
+ * whether or not there is, which is how the screen learns which of the two
+ * happened.
+ */
+const OIDC_DOCUMENT_PATHS = new Set(['/callback', '/login', '/api/auth/oidc/login']);
+
 const isOidcDocumentRequest = (req) => {
   const path = req?.path || '';
-  if (path !== '/callback') return false;
+  if (!OIDC_DOCUMENT_PATHS.has(path)) return false;
   const accept = typeof req.headers?.accept === 'string' ? req.headers.accept : '';
   const secFetchDest =
     typeof req.headers?.['sec-fetch-dest'] === 'string' ? req.headers['sec-fetch-dest'] : '';
@@ -84,27 +96,18 @@ const isOidcDocumentRequest = (req) => {
   return accept.includes('text/html') || secFetchDest === 'document' || secFetchMode === 'navigate';
 };
 
-const clearOidcSessionCookies = (res) => {
-  // express-openid-connect defaults to "appSession"
-  try {
-    res.clearCookie('appSession', {
-      path: '/',
-      sameSite: 'Lax',
-      secure: true,
-      httpOnly: true,
-    });
-  } catch (_) {
-    /* ignore */
-  }
-  try {
-    res.clearCookie('appSession', {
-      path: '/',
-      sameSite: 'Lax',
-      secure: false,
-      httpOnly: true,
-    });
-  } catch (_) {
-    /* ignore */
+const clearOidcSessionCookies = (req, res) => {
+  const cookieNames = new Set([req.nextExplorerOidcSessionCookieName, 'appSession']);
+  for (const cookieName of cookieNames) {
+    if (!cookieName) continue;
+    try {
+      if (cookieName in req) req[cookieName] = undefined;
+      const cookieOptions = { path: '/', sameSite: 'Lax', httpOnly: true };
+      res.clearCookie(cookieName, { ...cookieOptions, secure: true });
+      res.clearCookie(cookieName, { ...cookieOptions, secure: false });
+    } catch (_) {
+      /* ignore */
+    }
   }
 };
 
@@ -129,10 +132,18 @@ const errorHandler = (err, req, res, next) => {
   // For OIDC callback navigations, redirect back into the SPA so the login screen can show the error.
   // Otherwise, the browser will render the JSON payload as a standalone error page.
   if (!res.headersSent && isOidcDocumentRequest(req)) {
-    clearOidcSessionCookies(res);
-    const nextUrl = `/auth/login?error=${encodeURIComponent(message)}`;
+    clearOidcSessionCookies(req, res);
+    // Same redaction as the JSON body: this one lands in the address bar, browser
+    // history and every proxy log along the way, so a raw server path here travels
+    // further than it would in a response body.
+    const query = new URLSearchParams({ error: sanitizeClientMessage(message) });
+    // The code travels beside the sentence, never instead of it. The screen says
+    // what the codes it knows mean in the reader's own language — which is how "the
+    // provider could not be reached" stops reading as "OIDC is not configured" —
+    // and falls back to the sentence for the ones it does not.
+    if (err.code) query.set('error_code', String(err.code));
     res.setHeader('Cache-Control', 'no-store');
-    res.redirect(302, nextUrl);
+    res.redirect(302, `/auth/login?${query.toString()}`);
     return;
   }