From e8f47fd98fa43633efcb6d69af8fb84b95871cdd Mon Sep 17 00:00:00 2001 From: Benjy Date: Sun, 27 Sep 2026 19:00:43 +0200 Subject: [PATCH 1/4] Hand these commands their arguments instead of a command line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two routes built a command line with values from the request in it and gave the line to a shell. `routes/usage.js` pasted a folder's path into `du -sb "…"` and `df -Pk "…"`; `routes/permissions.js` pasted an owner, a group, a mode and a path into `chown`, `chgrp`, `chmod -R` and two `id` lookups. A name is not a shell string. Anything that closes the quoting leaves the rest for `/bin/sh` to run, as the user the server runs as. The usage one needs no privilege at all: any account that can create a folder can name one, and where AUTH_ENABLED is false that is anybody who can reach the server. `execFile` takes the arguments as a list, so there is no line for a shell to read and no shell. The commands, their output and the answers are unchanged — this is deliberately the smallest change that closes it, and not the rewrite that stands in nxzai#450 and nxzai#453. An argument list is not a free pass on its own: `chown` reads a leading dash as an option, so `--reference=/etc/shadow` would have copied another file's ownership onto the target. An account or group name has to look like one. `tests/routes/no-shell.test.js` — three cases. Two of them name a folder, and an owner, with a payload that creates a file, and check the file is not there; the third asks for `--reference=/etc/shadow` and expects a refusal. All three fail against the routes as they are, the first two by running the command. The payload only ever touches the working directory, and it is removed whatever an assertion does, so a run that does execute leaves nothing behind. --- backend/src/routes/permissions.js | 48 ++++++++--- backend/src/routes/usage.js | 12 ++- backend/tests/routes/no-shell.test.js | 117 ++++++++++++++++++++++++++ 3 files changed, 163 insertions(+), 14 deletions(-) create mode 100644 backend/tests/routes/no-shell.test.js diff --git a/backend/src/routes/permissions.js b/backend/src/routes/permissions.js index 0d005f38a..0c264164c 100644 --- a/backend/src/routes/permissions.js +++ b/backend/src/routes/permissions.js @@ -1,6 +1,6 @@ const express = require('express'); const fs = require('fs/promises'); -const { exec } = require('child_process'); +const { execFile } = require('child_process'); const { promisify } = require('util'); const { normalizeRelativePath } = require('../utils/pathUtils'); @@ -16,7 +16,28 @@ const { } = require('../errors/AppError'); const router = express.Router(); -const execAsync = promisify(exec); +// `execFile`, not `exec`: every one of these used to build a command line with +// values from the request in it, and a shell then read that line. `owner` and +// `group` arrive from the body, so a pair of quotes was the whole difference +// between "may change ownership here" and "may run anything as the user this +// server runs as". +const execAsync = promisify(execFile); + +/** + * An account or group name has to look like one. + * + * An argument list is not a free pass on its own: `chown` reads anything starting + * with a dash as an option, so `--reference=/etc/shadow` would have copied another + * file's ownership onto the target. A name starts with a letter, a digit or an + * underscore. + */ +const ACCOUNT_NAME_PATTERN = /^[a-zA-Z0-9_][a-zA-Z0-9._-]*$/; +const ensureValidAccountName = (value, label) => { + if (value === undefined || value === null || value === '') return; + if (typeof value !== 'string' || !ACCOUNT_NAME_PATTERN.test(value)) { + throw new ValidationError(`${label} is not a valid name.`); + } +}; /** * Get file permissions, owner, and group information @@ -53,7 +74,7 @@ router.get( if (process.platform !== 'win32') { try { // Get owner name from uid - const { stdout: ownerOut } = await execAsync(`id -nu ${stats.uid}`); + const { stdout: ownerOut } = await execAsync('id', ['-nu', String(stats.uid)]); owner = ownerOut.trim(); } catch (e) { logger.debug({ err: e }, 'Failed to get owner name'); @@ -61,7 +82,7 @@ router.get( try { // Get group name from gid - const { stdout: groupOut } = await execAsync(`id -gn ${stats.gid}`); + const { stdout: groupOut } = await execAsync('id', ['-gn', String(stats.gid)]); group = groupOut.trim(); } catch (e) { logger.debug({ err: e }, 'Failed to get group name'); @@ -139,7 +160,7 @@ router.post( // Use chmod -R for recursive on Unix systems if (process.platform !== 'win32') { try { - await execAsync(`chmod -R ${mode} "${resolved.absolutePath}"`); + await execAsync('chmod', ['-R', String(mode), resolved.absolutePath]); } catch (e) { logger.error({ err: e }, 'Failed to apply recursive chmod'); throw new Error('Failed to apply permissions recursively.'); @@ -215,18 +236,25 @@ router.post( // chown requires shell execution as Node.js doesn't have built-in owner/group change // This requires elevated privileges on most systems if (process.platform !== 'win32') { - let chownCmd = ''; + ensureValidAccountName(owner, 'The owner'); + ensureValidAccountName(group, 'The group'); + + let command = null; + let args = []; if (owner && group) { - chownCmd = `chown "${owner}:${group}" "${resolved.absolutePath}"`; + command = 'chown'; + args = [`${owner}:${group}`, resolved.absolutePath]; } else if (owner) { - chownCmd = `chown "${owner}" "${resolved.absolutePath}"`; + command = 'chown'; + args = [owner, resolved.absolutePath]; } else if (group) { - chownCmd = `chgrp "${group}" "${resolved.absolutePath}"`; + command = 'chgrp'; + args = [group, resolved.absolutePath]; } try { - await execAsync(chownCmd); + if (command) await execAsync(command, args); logger.info({ path: relativePath, owner, group }, 'Ownership changed'); } catch (e) { logger.error({ err: e }, 'Failed to change ownership'); diff --git a/backend/src/routes/usage.js b/backend/src/routes/usage.js index f7c5060e8..4f7f9a022 100644 --- a/backend/src/routes/usage.js +++ b/backend/src/routes/usage.js @@ -1,11 +1,15 @@ const express = require('express'); const { promisify } = require('util'); -const { exec } = require('child_process'); +const { execFile } = require('child_process'); const { normalizeRelativePath } = require('../utils/pathUtils'); const { resolvePathWithAccess } = require('../services/accessManager'); const logger = require('../utils/logger'); const asyncHandler = require('../utils/asyncHandler'); -const execp = promisify(exec); +// `execFile`, not `exec`: the path goes in as an argument rather than into a +// command line. A folder whose name contains a quote used to end the quoting and +// leave the rest for the shell to run, as the user this server runs as, the moment +// somebody opened it — and any account that can make a folder could name one. +const execp = promisify(execFile); const router = express.Router(); // Fast directory size using du command @@ -13,7 +17,7 @@ const dirSize = async (root) => { try { // -sb: summarize in bytes, don't follow symlinks // This is orders of magnitude faster than fs.stat() recursion - const { stdout } = await execp(`du -sb "${root}"`, { + const { stdout } = await execp('du', ['-sb', root], { maxBuffer: 1024 * 1024 * 10, // 10MB buffer for large outputs }); @@ -45,7 +49,7 @@ router.get( // Run both commands in parallel for maximum speed const [size, dfResult] = await Promise.all([ dirSize(abs), - execp(`df -Pk "${abs}"`).catch(() => ({ stdout: '' })), + execp('df', ['-Pk', abs]).catch(() => ({ stdout: '' })), ]); let total = 0, diff --git a/backend/tests/routes/no-shell.test.js b/backend/tests/routes/no-shell.test.js new file mode 100644 index 000000000..cc8092240 --- /dev/null +++ b/backend/tests/routes/no-shell.test.js @@ -0,0 +1,117 @@ +import { afterEach, describe, expect, it } from 'vitest'; +import fs from 'node:fs/promises'; +import path from 'node:path'; +import request from 'supertest'; +import { setupTestEnv, createTestApp } from '../helpers/env-test-utils.js'; + +/** + * A name from a request is not a shell string. + * + * Two routes built command lines with values from the request pasted into them and + * handed the line to `/bin/sh`. A folder name, an owner, a group: anything that + * closed the quoting left the rest for the shell to run, as the user this server + * runs as. The folder one needs no privilege at all — any account that can make a + * folder can name one, and with AUTH_ENABLED=false that is anybody who can reach + * the server. + * + * The payloads below only create a file inside the test's own temporary directory, + * and what each case asserts is that the file is not there. + */ + +let env; + +// Where a payload would land: a folder name cannot hold a slash, so it writes into +// the working directory of the process running this. +const PROOF = path.join(process.cwd(), 'a-shell-ran-here'); +const shellRan = async () => + fs + .access(PROOF) + .then(() => true) + .catch(() => false); + +afterEach(async () => { + // Whatever an assertion did, this happens: a run that does execute must not leave + // the file behind for the next one to find. + await fs.rm(PROOF, { force: true }); + if (env) { + await env.cleanup(); + env = null; + } +}); + +describe('asking how full a volume is', () => { + it('does not run what a folder is called', async () => { + env = await setupTestEnv({ + tag: 'usage-no-shell-', + modules: ['src/routes/usage', 'src/services/accessManager', 'src/utils/pathUtils'], + }); + + // A name that closes the quoting the route used to open around it. + const folder = 'Vol";touch a-shell-ran-here;echo "'; + await fs.mkdir(path.join(env.volumeDir, folder)); + + const app = createTestApp({ + router: env.requireFresh('src/routes/usage'), + mountPath: '/api', + user: { id: 'admin-user', roles: ['admin'] }, + }); + + const response = await request(app).get(`/api/usage/${encodeURIComponent(folder)}`); + + expect(response.status).toBe(200); + expect(await shellRan()).toBe(false); + }); +}); + +describe('changing an owner', () => { + // With the error handler, so a refusal arrives as the sentence it is meant to be + // rather than an empty body with a status on it. + const appFor = () => + createTestApp({ + router: env.requireFresh('src/routes/permissions'), + mountPath: '/api', + user: { id: 'admin-user', roles: ['admin'] }, + errorHandler: env.requireFresh('src/middleware/errorHandler').errorHandler, + }); + + const setup = async (tag) => { + env = await setupTestEnv({ + tag, + modules: [ + 'src/routes/permissions', + 'src/middleware/errorHandler', + 'src/services/accessManager', + 'src/utils/pathUtils', + ], + }); + await fs.mkdir(path.join(env.volumeDir, 'Vol'), { recursive: true }); + await fs.writeFile(path.join(env.volumeDir, 'Vol', 'file.txt'), 'x'); + }; + + it('does not run what an owner is called', async () => { + await setup('chown-no-shell-'); + const response = await request(appFor()) + .post('/api/permissions/chown') + .send({ path: 'Vol/file.txt', owner: 'root";touch a-shell-ran-here;echo "' }); + + // Refused as a name, or attempted as one argument and failed — either way the + // command inside it is not a command. + expect(await shellRan()).toBe(false); + expect(response.status).toBeGreaterThanOrEqual(400); + }); + + it('refuses a name that would be read as an option', async () => { + await setup('chown-option-'); + + const response = await request(appFor()) + .post('/api/permissions/chown') + .send({ path: 'Vol/file.txt', owner: '--reference=/etc/shadow' }); + + // Refused as a name rather than attempted as one: a 400 from the route, and a + // sentence that says which field and that it is a name. Not the exact wording — + // a test that pins a sentence breaks when somebody improves it. + expect(response.status).toBe(400); + expect(JSON.stringify(response.body)).toMatch(/owner/i); + expect(JSON.stringify(response.body)).toMatch(/(invalid|not a valid).{0,20}name/i); + }); +}); From bfa04d40271fa5973ffebc2490d428be57983380 Mon Sep 17 00:00:00 2001 From: Benjy Date: Sat, 26 Sep 2026 19:04:27 +0200 Subject: [PATCH 2/4] Tell a failed OIDC sign-in apart, and come back to the right address MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things about OIDC that this fixes. **Which of two failures it was.** A sign-in that cannot start has one of two causes: nothing was configured, or what was configured could not be made to work. The first is answered by filling in OIDC_ISSUER and the rest; the second is answered by looking at the provider. Both answered 404 "OIDC is not configured", so an administrator whose provider was unreachable was sent to change a configuration that was already right. `configureOidc` now records what it concluded and why, and the routes read it: 404 AUTH_OIDC_NOT_CONFIGURED for the first, 503 AUTH_OIDC_PROVIDER_UNAVAILABLE for the second, and neither carries the network's own words — no ENOTFOUND, no internal host name, in the body or in the address bar. A browser asking for one of those addresses is looking at a page, not reading JSON, so it is sent back to the sign-in screen with the code beside the sentence. The screen can then say what the code means in the reader's own language. **Where the callback comes back to.** The return address was built from a fixed `baseURL`, so a deployment reached through a reverse proxy on a different name sent people back to the wrong origin. It is now resolved from the request — but only from a forwarded host behind a trusted proxy, and only when the result exactly matches an origin the operator configured. A redirect target that a request header could choose is an open redirect with extra steps. The path is still held to a relative, same-site one, as before. Also here, because it is the same files: - `claimsFromIdToken` in the middleware and `uniqueOrigins` in the routes were local copies of things `utils/idToken.js` and the new `utils/oidcRedirect.js` do; both go. - The guest-session cookie was cleared on `/api` only, at five sign-in and sign-out paths. A guest session left on `/` outlived the sign-in that should have ended it. - `ServiceUnavailableError` (503) and the two AUTH_OIDC_* codes, which nothing had yet. The account lockout that `routes/auth.js` also differs in is deliberately not here: it is a different subject and goes with releasing a locked account. ## Checks Seven test files, 107 tests, including three this fork had and `main` did not: `oidc-middleware`, `oidcOrigin` and `auth-oidc-routes`. Neutralising `getOidcAvailability` so it reports one verdict for both causes turns three of them red — the three that tell the two apart. Whole backend suite: 2 391 passed, 2 failed, the two that fail on `main` on its own (`auth.test.js` on the current password, `browse-hidden-files.test.js`). `npm run lint` reports 146 against `main`'s 143, the three being the parse error on `backend/tests/**` that 141 of `main`'s own test files already draw. Formatting clean. Frontend builds, backend loads, documentation site builds. One test assertion was rewritten rather than ported as it stood: it matched the wording `express-openid-connect` uses for a callback with no sign-in in progress, and that wording differs between 2.19 and 2.20. It now asserts the refusal. One box, one name for it ------------------------ The sign-in box takes an email address or a username, so it is neither: it is whatever was typed, and this calls it `identifier` from the screen to the route. The screen and the client were renamed and the store and the route were not, so the store passed `email` to a client expecting `identifier`. `JSON.stringify` drops a key whose value is undefined, and the request went out carrying a password and nobody to sign in. The answer was "invalid credentials", which is what a wrong password looks like — so nothing about it read as a defect. `attemptLocalLogin` already took `identifier`; the route is what had not caught up. `email` and `username` still work, for a script or an older client that sends them. The identifier was the half that failed loudly. The screen also reads `totpPending`, `oidcStatus`, `cancelTotp`, `ensureStatus` and `forgetSession` off the store, and none of them were there: `totpPending` read undefined, so the box for the code from the authenticator never appeared, and a correct password on an account with a second factor landed on a screen that looked like it had done nothing. Undefined is not an error in a template — it is a `v-if` that is false. The store is here in full, with the code step read back from the server on every start so a reload in the middle of one lands back on the code. tests/routes/sign-in-identifier.test.js signs in with each of the three names, refuses a wrong password, and reads the three frontend files to check they all use the one name — the chain is four files long and three of them have no runner here, which is how it broke silently in the middle. And what a refusal says ----------------------- A code that names the kind of refusal — FORBIDDEN, NOT_FOUND, CONFLICT, RATE_LIMIT_EXCEEDED — is translated for the reader, and the server's own sentence, which says *which* refusal, went underneath rather than being lost. A lock that arrives with a duration gets a sentence of its own rather than a placeholder in the plain one, so it can never read "{minutes}". And the handler no longer asks vue-i18n for a key before checking it has it, which was a console warning for every refusal the catalogue has no entry for, twice. --- backend/src/routes/auth.js | 12 +- .../tests/routes/sign-in-identifier.test.js | 119 ++++++++++++++++++ frontend/src/api/errorHandler.js | 51 ++++++-- frontend/src/i18n/locales/de.json | 1 + frontend/src/i18n/locales/en.json | 1 + frontend/src/i18n/locales/es.json | 1 + frontend/src/i18n/locales/fr.json | 1 + frontend/src/i18n/locales/hi.json | 1 + frontend/src/i18n/locales/it.json | 1 + frontend/src/i18n/locales/ko.json | 1 + frontend/src/i18n/locales/nl.json | 1 + frontend/src/i18n/locales/pl.json | 1 + frontend/src/i18n/locales/pt-BR.json | 1 + frontend/src/i18n/locales/ro.json | 1 + frontend/src/i18n/locales/ru.json | 1 + frontend/src/i18n/locales/sv.json | 1 + frontend/src/i18n/locales/zh-CN.json | 1 + frontend/src/i18n/locales/zh-TW.json | 1 + frontend/src/stores/auth.js | 108 ++++++++++++---- 19 files changed, 263 insertions(+), 42 deletions(-) create mode 100644 backend/tests/routes/sign-in-identifier.test.js diff --git a/backend/src/routes/auth.js b/backend/src/routes/auth.js index 1df37e6ca..d0bc1de10 100644 --- a/backend/src/routes/auth.js +++ b/backend/src/routes/auth.js @@ -306,13 +306,15 @@ router.post( loginLimiter, asyncHandler(async (req, res) => { refuseWithoutPasswordSignIn(); - const { email, password, username } = req.body || {}; - // Support both email and username (backward compatibility) - const emailOrUsername = email || username; + const { identifier, email, password, username } = req.body || {}; + // One box on the sign-in screen, and three names for what was typed into + // it: `identifier` is what that screen sends, `email` and `username` are + // the older names a script or an older client may still use. + const typed = identifier || email || username; let user = null; try { - user = await attemptLocalLogin({ email: emailOrUsername, password }); + user = await attemptLocalLogin({ identifier: typed, password }); } catch (e) { if (e?.status === 423) { throw new RateLimitError(e.message, e.until); @@ -324,7 +326,7 @@ router.post( action: 'sign-in', outcome: 'refused', // The name that was typed, not one this server confirmed exists. - actor: String(emailOrUsername || '').slice(0, 200) || 'unknown', + actor: String(typed || '').slice(0, 200) || 'unknown', detail: { method: 'password' }, req, }); diff --git a/backend/tests/routes/sign-in-identifier.test.js b/backend/tests/routes/sign-in-identifier.test.js new file mode 100644 index 000000000..4e4dd44b7 --- /dev/null +++ b/backend/tests/routes/sign-in-identifier.test.js @@ -0,0 +1,119 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import fs from 'node:fs'; +import path from 'node:path'; +import request from 'supertest'; + +import { createTestApp, modulePath, setupTestEnv } from '../helpers/env-test-utils.js'; + +/** + * One box on the sign-in screen, and the same name for it all the way down. + * + * The box takes an email address or a username, so it is neither: it is + * whatever was typed. That name has to hold from the screen to the route, and + * when it did not, nothing said so. The screen sent `identifier`, the store + * passed on `email`, and `JSON.stringify` drops a key whose value is undefined + * — so the request went out carrying a password and nobody to sign in, and the + * answer was "invalid credentials", which is what a wrong password looks like. + * + * Both halves are asserted here: the route takes the name the screen sends, and + * the screen, the client and the store all use that one name. The second half + * is read off the frontend sources, because the chain is four files long and + * three of them have no runner here — and a chain that breaks silently in the + * middle is exactly what this is for. + */ + +const FRONTEND = path.join(__dirname, '..', '..', '..', 'frontend', 'src'); +const read = (relative) => fs.readFileSync(path.join(FRONTEND, relative), 'utf8'); + +describe('the name for what was typed into the sign-in box', () => { + let env; + let app; + + beforeEach(async () => { + env = await setupTestEnv({ + tag: 'sign-in-identifier-', + modules: ['src/services/db', 'src/routes/auth', 'src/middleware/errorHandler'], + envOverrides: { AUTH_ENABLED: 'true' }, + }); + await env.requireFresh('src/services/users').createLocalUser({ + email: 'alice@example.com', + username: 'alice', + displayName: 'Alice', + password: 'correct horse battery', + roles: ['admin'], + }); + app = createTestApp({ + router: env.requireFresh('src/routes/auth'), + mountPath: '/api/auth', + errorHandler: env.requireFresh('src/middleware/errorHandler').errorHandler, + }); + }); + + afterEach(async () => { + await env.cleanup(); + }); + + const signIn = (body) => request(app).post('/api/auth/login').send(body); + + it('signs in with the name the screen sends', async () => { + const response = await signIn({ + identifier: 'alice@example.com', + password: 'correct horse battery', + }); + + expect(response.status).toBe(200); + expect(response.body.user?.email).toBe('alice@example.com'); + }); + + it('takes a username in the same box', async () => { + const response = await signIn({ identifier: 'alice', password: 'correct horse battery' }); + + expect(response.status).toBe(200); + expect(response.body.user?.username).toBe('alice'); + }); + + it.each(['email', 'username'])('still takes the older name %s', async (name) => { + const response = await signIn({ + [name]: name === 'email' ? 'alice@example.com' : 'alice', + password: 'correct horse battery', + }); + + expect(response.status).toBe(200); + }); + + it('refuses a password that is wrong, and says nothing about which half', async () => { + const response = await signIn({ identifier: 'alice', password: 'not it' }); + + expect(response.status).toBe(401); + expect(response.body.error?.code).toBe('AUTH_INVALID_CREDENTIALS'); + }); + + /** + * The screen, the client and the store. A rename that stops at one of them + * leaves the next passing undefined, which is not an error anywhere — the key + * simply vanishes from the request body. + */ + it('is the name the screen, the client and the store all use', () => { + expect(read('views/AuthLoginView.vue')).toMatch(/auth\.login\(\{\s*identifier:/); + expect(read('api/auth.api.js')).toMatch(/const login = \(\{ identifier, password \}\)/); + expect(read('stores/auth.js')).toMatch(/const login = async \(\{ identifier, password \}\)/); + expect(read('stores/auth.js')).toMatch(/loginApi\(\{ identifier, password \}\)/); + }); + + /** + * Everything else the sign-in screen reads off the store. + * + * The same rename went through this screen and stopped before the store, and + * the identifier was only the half that failed loudly. `totpPending` reads + * undefined, so the box for the code from the authenticator never appears: + * a correct password on an account with a second factor lands on a screen + * that looks like it did nothing. Undefined is not an error in a template — + * it is a `v-if` that is false — so there is nothing to see but the absence. + */ + it.each(['totpPending', 'oidcStatus', 'cancelTotp', 'ensureStatus', 'forgetSession'])( + 'is on the store, because the screen reads it: %s', + (member) => { + expect(read('stores/auth.js')).toContain(member); + } + ); +}); diff --git a/frontend/src/api/errorHandler.js b/frontend/src/api/errorHandler.js index 9a333b3fb..9b10bf203 100644 --- a/frontend/src/api/errorHandler.js +++ b/frontend/src/api/errorHandler.js @@ -1,32 +1,61 @@ +/** + * Codes that say what kind of refusal it was, not which one. The catalogue + * gives the kind in the reader's language; the server's own sentence, which + * says which refusal, goes underneath rather than being lost. + */ +const GENERIC_CODES = new Set(['FORBIDDEN', 'NOT_FOUND', 'CONFLICT', 'RATE_LIMIT_EXCEEDED']); + export function createErrorHandler(notificationsStore, i18n) { - return (errorInfo) => { + // Asked before translating: vue-i18n warns in the console for every key it + // is asked for and does not have, twice with a fallback locale, and most + // codes the server sends have no entry. + const knows = (key) => i18n.global.te(key) || i18n.global.te(key, 'en'); + + /** + * @param {object} errorInfo what the server refused with + * @param {object} [options] + * @param {boolean} [options.quiet] translate it, but raise no notification: + * the screen that asked is about to say it itself, under the field it + * belongs to, which is a better place for it than a toast in the corner. + */ + return (errorInfo, { quiet = false } = {}) => { const { code, message, requestId, statusCode, details } = errorInfo; let heading = message || 'An error occurred'; + let explanation = null; // Translate if we have a code if (code) { const key = `serverErrors.${code}`; - const translated = i18n.global.t(key); - if (translated !== key) { + if (knows(key)) { // Handle rate limit pluralization if (code.startsWith('RATE_LIMIT_') && details?.retryAfter) { const minutes = Math.ceil(details.retryAfter / 60); heading = i18n.global.t(key, { minutes }, minutes); + } else if (code === 'AUTH_ACCOUNT_LOCKED' && details?.retryAfter) { + // A sentence of its own rather than a placeholder in the plain one: + // a lock that arrives without a duration must never read "{minutes}". + const minutes = Math.ceil(details.retryAfter / 60); + heading = i18n.global.t('serverErrors.AUTH_ACCOUNT_LOCKED_RETRY', { minutes }, minutes); } else { - heading = translated; + heading = i18n.global.t(key); } + if (GENERIC_CODES.has(code) && message) explanation = message; } } - notificationsStore.addNotification({ - type: 'error', - heading, - body: details ? JSON.stringify(details) : '', - requestId, - statusCode, - }); + const body = [explanation, details ? JSON.stringify(details) : null].filter(Boolean).join('\n'); + + if (!quiet) { + notificationsStore.addNotification({ + type: 'error', + heading, + body, + requestId, + statusCode, + }); + } // Return translated message for error thrown by http.js return heading; diff --git a/frontend/src/i18n/locales/de.json b/frontend/src/i18n/locales/de.json index 871c957ed..c12addea8 100644 --- a/frontend/src/i18n/locales/de.json +++ b/frontend/src/i18n/locales/de.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "Authentifizierung erforderlich", "AUTH_INVALID_CREDENTIALS": "Ungültige E-Mail oder Passwort", "AUTH_ACCOUNT_LOCKED": "Das Konto ist aufgrund fehlgeschlagener Anmeldeversuche vorübergehend gesperrt", + "AUTH_ACCOUNT_LOCKED_RETRY": "Konto nach zu vielen fehlgeschlagenen Anmeldeversuchen gesperrt. Versuchen Sie es in {minutes} Minute erneut | Konto nach zu vielen fehlgeschlagenen Anmeldeversuchen gesperrt. Versuchen Sie es in {minutes} Minuten erneut", "AUTH_PASSWORD_INCORRECT": "Das aktuelle Passwort ist falsch", "VALIDATION_EMAIL_REQUIRED": "E-Mail ist erforderlich", "VALIDATION_PASSWORD_REQUIRED": "Passwort ist erforderlich", diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index 916db9e90..52268b689 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "Authentication required", "AUTH_INVALID_CREDENTIALS": "Invalid email or password", "AUTH_ACCOUNT_LOCKED": "Account is temporarily locked due to failed login attempts", + "AUTH_ACCOUNT_LOCKED_RETRY": "Account locked after too many failed sign-in attempts. Try again in {minutes} minute | Account locked after too many failed sign-in attempts. Try again in {minutes} minutes", "AUTH_PASSWORD_INCORRECT": "Current password is incorrect", "VALIDATION_EMAIL_REQUIRED": "Email is required", "VALIDATION_PASSWORD_REQUIRED": "Password is required", diff --git a/frontend/src/i18n/locales/es.json b/frontend/src/i18n/locales/es.json index 16aab9ac8..b5e3c40db 100644 --- a/frontend/src/i18n/locales/es.json +++ b/frontend/src/i18n/locales/es.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "Se requiere autenticación", "AUTH_INVALID_CREDENTIALS": "Correo electrónico o contraseña no válidos", "AUTH_ACCOUNT_LOCKED": "La cuenta está bloqueada temporalmente debido a intentos fallidos de inicio de sesión", + "AUTH_ACCOUNT_LOCKED_RETRY": "Cuenta bloqueada tras demasiados intentos fallidos de inicio de sesión. Inténtalo de nuevo en {minutes} minuto | Cuenta bloqueada tras demasiados intentos fallidos de inicio de sesión. Inténtalo de nuevo en {minutes} minutos", "AUTH_PASSWORD_INCORRECT": "La contraseña actual es incorrecta", "VALIDATION_EMAIL_REQUIRED": "El correo electrónico es obligatorio", "VALIDATION_PASSWORD_REQUIRED": "La contraseña es obligatoria", diff --git a/frontend/src/i18n/locales/fr.json b/frontend/src/i18n/locales/fr.json index bec144287..d0c22af94 100644 --- a/frontend/src/i18n/locales/fr.json +++ b/frontend/src/i18n/locales/fr.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "Authentification requise", "AUTH_INVALID_CREDENTIALS": "E-mail ou mot de passe invalide", "AUTH_ACCOUNT_LOCKED": "Le compte est temporairement verrouillé en raison de tentatives de connexion échouées", + "AUTH_ACCOUNT_LOCKED_RETRY": "Compte verrouillé après trop de tentatives de connexion échouées. Réessayez dans {minutes} minute | Compte verrouillé après trop de tentatives de connexion échouées. Réessayez dans {minutes} minutes", "AUTH_PASSWORD_INCORRECT": "Le mot de passe actuel est incorrect", "VALIDATION_EMAIL_REQUIRED": "L'e-mail est obligatoire", "VALIDATION_PASSWORD_REQUIRED": "Le mot de passe est obligatoire", diff --git a/frontend/src/i18n/locales/hi.json b/frontend/src/i18n/locales/hi.json index 952c440f9..529d4a561 100644 --- a/frontend/src/i18n/locales/hi.json +++ b/frontend/src/i18n/locales/hi.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "प्रमाणीकरण आवश्यक है", "AUTH_INVALID_CREDENTIALS": "अमान्य ईमेल या पासवर्ड", "AUTH_ACCOUNT_LOCKED": "असफल लॉगिन प्रयासों के कारण खाता अस्थायी रूप से लॉक है", + "AUTH_ACCOUNT_LOCKED_RETRY": "बहुत अधिक असफल साइन-इन प्रयासों के बाद खाता लॉक है। {minutes} मिनट बाद फिर से प्रयास करें", "AUTH_PASSWORD_INCORRECT": "वर्तमान पासवर्ड गलत है", "VALIDATION_EMAIL_REQUIRED": "ईमेल आवश्यक है", "VALIDATION_PASSWORD_REQUIRED": "पासवर्ड आवश्यक है", diff --git a/frontend/src/i18n/locales/it.json b/frontend/src/i18n/locales/it.json index 09d08f192..51fc1e1d6 100644 --- a/frontend/src/i18n/locales/it.json +++ b/frontend/src/i18n/locales/it.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "Autenticazione richiesta", "AUTH_INVALID_CREDENTIALS": "Email o password non validi", "AUTH_ACCOUNT_LOCKED": "L'account è temporaneamente bloccato a causa di tentativi di accesso falliti", + "AUTH_ACCOUNT_LOCKED_RETRY": "Account bloccato dopo troppi tentativi di accesso falliti. Riprova tra {minutes} minuto | Account bloccato dopo troppi tentativi di accesso falliti. Riprova tra {minutes} minuti", "AUTH_PASSWORD_INCORRECT": "La password corrente è errata", "VALIDATION_EMAIL_REQUIRED": "L'email è obbligatoria", "VALIDATION_PASSWORD_REQUIRED": "La password è obbligatoria", diff --git a/frontend/src/i18n/locales/ko.json b/frontend/src/i18n/locales/ko.json index 4a2975c5f..47ed5f923 100644 --- a/frontend/src/i18n/locales/ko.json +++ b/frontend/src/i18n/locales/ko.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "인증이 필요합니다", "AUTH_INVALID_CREDENTIALS": "이메일 혹은 비밀번호가 일치하지 않습니다", "AUTH_ACCOUNT_LOCKED": "로그인 실패로 인해 계정이 일시 잠금 처리되었습니다", + "AUTH_ACCOUNT_LOCKED_RETRY": "로그인 실패가 너무 많아 계정이 잠겼습니다. {minutes}분 후 다시 시도해주세요 | 로그인 실패가 너무 많아 계정이 잠겼습니다. {minutes}분 후 다시 시도해주세요", "AUTH_PASSWORD_INCORRECT": "현재 비밀번호가 일치하지 않습니다", "VALIDATION_EMAIL_REQUIRED": "이메일 주소가 필요합니다", "VALIDATION_PASSWORD_REQUIRED": "비밀번호가 필요합니다", diff --git a/frontend/src/i18n/locales/nl.json b/frontend/src/i18n/locales/nl.json index 482029f16..939b4d139 100644 --- a/frontend/src/i18n/locales/nl.json +++ b/frontend/src/i18n/locales/nl.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "Authenticatie vereist", "AUTH_INVALID_CREDENTIALS": "Ongeldige aanmeldgegevens", "AUTH_ACCOUNT_LOCKED": "Account is tijdelijk geblokkeerd vanwege mislukte inlogpogingen", + "AUTH_ACCOUNT_LOCKED_RETRY": "Account vergrendeld na te veel mislukte aanmeldpogingen. Probeer het over {minutes} minuut opnieuw | Account vergrendeld na te veel mislukte aanmeldpogingen. Probeer het over {minutes} minuten opnieuw", "AUTH_PASSWORD_INCORRECT": "Huidig wachtwoord is onjuist", "VALIDATION_EMAIL_REQUIRED": "E-mailadres is vereist", "VALIDATION_PASSWORD_REQUIRED": "Wachtwoord is vereist", diff --git a/frontend/src/i18n/locales/pl.json b/frontend/src/i18n/locales/pl.json index 6fe0405e0..9e78b67c6 100644 --- a/frontend/src/i18n/locales/pl.json +++ b/frontend/src/i18n/locales/pl.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "Wymagana autentykacja", "AUTH_INVALID_CREDENTIALS": "Nieprawidłowy adres e-mail lub hasło", "AUTH_ACCOUNT_LOCKED": "Konto jest tymczasowo zablokowane z powodu nieudanych prób logowania", + "AUTH_ACCOUNT_LOCKED_RETRY": "Konto zablokowane po zbyt wielu nieudanych próbach logowania. Spróbuj ponownie za {minutes} minutę | Konto zablokowane po zbyt wielu nieudanych próbach logowania. Spróbuj ponownie za {minutes} minuty | Konto zablokowane po zbyt wielu nieudanych próbach logowania. Spróbuj ponownie za {minutes} minut", "AUTH_PASSWORD_INCORRECT": "Obecne hasło jest nieprawidłowe", "VALIDATION_EMAIL_REQUIRED": "Adres e-mail jest wymagany", "VALIDATION_PASSWORD_REQUIRED": "Hasło jest wymagane", diff --git a/frontend/src/i18n/locales/pt-BR.json b/frontend/src/i18n/locales/pt-BR.json index 6c5eea037..fcaaba56f 100644 --- a/frontend/src/i18n/locales/pt-BR.json +++ b/frontend/src/i18n/locales/pt-BR.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "Autenticação necessária", "AUTH_INVALID_CREDENTIALS": "E-mail ou senha inválidos", "AUTH_ACCOUNT_LOCKED": "A conta está temporariamente bloqueada devido a tentativas de login com falha", + "AUTH_ACCOUNT_LOCKED_RETRY": "Conta bloqueada após muitas tentativas de login com falha. Tente novamente em {minutes} minuto | Conta bloqueada após muitas tentativas de login com falha. Tente novamente em {minutes} minutos", "AUTH_PASSWORD_INCORRECT": "A senha atual está incorreta", "VALIDATION_EMAIL_REQUIRED": "O e-mail é obrigatório", "VALIDATION_PASSWORD_REQUIRED": "A senha é obrigatória", diff --git a/frontend/src/i18n/locales/ro.json b/frontend/src/i18n/locales/ro.json index 6460935b2..3e55e03cd 100644 --- a/frontend/src/i18n/locales/ro.json +++ b/frontend/src/i18n/locales/ro.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "Autentificare necesară", "AUTH_INVALID_CREDENTIALS": "Email sau parolă invalidă", "AUTH_ACCOUNT_LOCKED": "Contul este blocat temporar din cauza încercărilor eșuate de autentificare", + "AUTH_ACCOUNT_LOCKED_RETRY": "Cont blocat după prea multe încercări eșuate de autentificare. Încercați din nou peste {minutes} minut | Cont blocat după prea multe încercări eșuate de autentificare. Încercați din nou peste {minutes} minute", "AUTH_PASSWORD_INCORRECT": "Parola curentă este incorectă", "VALIDATION_EMAIL_REQUIRED": "Email-ul este obligatoriu", "VALIDATION_PASSWORD_REQUIRED": "Parola este obligatorie", diff --git a/frontend/src/i18n/locales/ru.json b/frontend/src/i18n/locales/ru.json index 01612b6e2..7d300f90d 100644 --- a/frontend/src/i18n/locales/ru.json +++ b/frontend/src/i18n/locales/ru.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "Требуется аутентификация", "AUTH_INVALID_CREDENTIALS": "Неверная электронная почта или пароль", "AUTH_ACCOUNT_LOCKED": "Учетная запись временно заблокирована из-за неудачных попыток входа", + "AUTH_ACCOUNT_LOCKED_RETRY": "Учетная запись заблокирована после слишком многих неудачных попыток входа. Повторите попытку через {minutes} минуту | Учетная запись заблокирована после слишком многих неудачных попыток входа. Повторите попытку через {minutes} минуты | Учетная запись заблокирована после слишком многих неудачных попыток входа. Повторите попытку через {minutes} минут", "AUTH_PASSWORD_INCORRECT": "Текущий пароль неверен", "VALIDATION_EMAIL_REQUIRED": "Требуется электронная почта", "VALIDATION_PASSWORD_REQUIRED": "Требуется пароль", diff --git a/frontend/src/i18n/locales/sv.json b/frontend/src/i18n/locales/sv.json index 4e6d8f496..3ef628f70 100644 --- a/frontend/src/i18n/locales/sv.json +++ b/frontend/src/i18n/locales/sv.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "Autentisering krävs", "AUTH_INVALID_CREDENTIALS": "Ogiltig e-post eller lösenord", "AUTH_ACCOUNT_LOCKED": "Kontot är tillfälligt låst på grund av misslyckade inloggningsförsök", + "AUTH_ACCOUNT_LOCKED_RETRY": "Kontot är låst efter för många misslyckade inloggningsförsök. Försök igen om {minutes} minut | Kontot är låst efter för många misslyckade inloggningsförsök. Försök igen om {minutes} minuter", "AUTH_PASSWORD_INCORRECT": "Det nuvarande lösenordet är felaktigt", "VALIDATION_EMAIL_REQUIRED": "E-post krävs", "VALIDATION_PASSWORD_REQUIRED": "Lösenord krävs", diff --git a/frontend/src/i18n/locales/zh-CN.json b/frontend/src/i18n/locales/zh-CN.json index 5ac5665f2..e8b5b7588 100644 --- a/frontend/src/i18n/locales/zh-CN.json +++ b/frontend/src/i18n/locales/zh-CN.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "需要身份验证", "AUTH_INVALID_CREDENTIALS": "电子邮件或密码无效", "AUTH_ACCOUNT_LOCKED": "由于登录尝试失败,账户已被临时锁定", + "AUTH_ACCOUNT_LOCKED_RETRY": "登录失败次数过多,账户已被锁定。请在{minutes}分钟后重试", "AUTH_PASSWORD_INCORRECT": "当前密码不正确", "VALIDATION_EMAIL_REQUIRED": "电子邮件为必填项", "VALIDATION_PASSWORD_REQUIRED": "密码为必填项", diff --git a/frontend/src/i18n/locales/zh-TW.json b/frontend/src/i18n/locales/zh-TW.json index 3dfba8d63..7f6cb7860 100644 --- a/frontend/src/i18n/locales/zh-TW.json +++ b/frontend/src/i18n/locales/zh-TW.json @@ -175,6 +175,7 @@ "AUTH_REQUIRED": "需要身份驗證", "AUTH_INVALID_CREDENTIALS": "電子郵件或密碼無效", "AUTH_ACCOUNT_LOCKED": "由於登入嘗試失敗,帳戶已暫時鎖定", + "AUTH_ACCOUNT_LOCKED_RETRY": "登入失敗次數過多,帳戶已被鎖定。請在{minutes}分鐘後重試", "AUTH_PASSWORD_INCORRECT": "目前密碼不正確", "VALIDATION_EMAIL_REQUIRED": "電子郵件為必填", "VALIDATION_PASSWORD_REQUIRED": "密碼為必填", diff --git a/frontend/src/stores/auth.js b/frontend/src/stores/auth.js index 43bf8ee43..d4bd2abc7 100644 --- a/frontend/src/stores/auth.js +++ b/frontend/src/stores/auth.js @@ -5,8 +5,8 @@ import { fetchAuthStatus, setupAccount as setupAccountApi, login as loginApi, - submitTotpCode as submitTotpCodeApi, signInWithPasskey as signInWithPasskeyApi, + submitTotpCode as submitTotpCodeApi, logout as logoutApi, fetchCurrentUser, } from '@/api'; @@ -18,16 +18,22 @@ export const useAuthStore = defineStore('auth', () => { const strategies = ref({ local: true, oidc: false, + passkey: false, }); + // What the server's configuration pass concluded about single sign-on: + // 'ready', 'not-configured' or 'unavailable'. The sign-in screen shows the + // last two rather than sending somebody to a provider that cannot answer. + const oidcStatus = ref('ready'); const currentUser = ref(null); - const isLoading = ref(false); /** - * Whether a sign-in is waiting for a code from an authenticator. + * The password was right, and the account asks for a code as well. * - * The password was right; nothing about who they are is known here, and the - * server is holding that. + * Read back from the server on every start, not only set when a sign-in + * happens here: a reload in the middle of one lands back on the code rather + * than on a password screen that would start the whole thing again. */ - const totpRequired = ref(false); + const totpPending = ref(false); + const isLoading = ref(false); const hasStatus = ref(false); const lastError = ref(null); let initPromise = null; @@ -60,11 +66,10 @@ export const useAuthStore = defineStore('auth', () => { requiresSetup.value = enabled ? Boolean(status.requiresSetup) : false; authEnabled.value = enabled; authMode.value = typeof status?.authMode === 'string' ? status.authMode : 'local'; - strategies.value = status?.strategies || { local: true, oidc: false }; + strategies.value = status?.strategies || { local: true, oidc: false, passkey: false }; + oidcStatus.value = status?.oidc?.status || 'ready'; currentUser.value = status?.user || null; - // A reload in the middle of signing in lands back on the code rather - // than on a password screen that would start the whole thing again. - totpRequired.value = Boolean(status?.totpPending); + totpPending.value = Boolean(status?.totpPending); // Clear guest session if user is now authenticated if (currentUser.value) { @@ -96,15 +101,20 @@ export const useAuthStore = defineStore('auth', () => { sessionStorage.removeItem('guestSessionId'); }; - const login = async ({ email, password }) => { + const login = async ({ identifier, password }) => { lastError.value = null; - const response = await loginApi({ email, password }); + const response = await loginApi({ identifier, password }); + hasStatus.value = true; + + // Halfway: the password was right and a code is wanted as well. Nobody is + // signed in until it arrives, so nothing here says anybody is. if (response?.totpRequired) { - totpRequired.value = true; + totpPending.value = true; + currentUser.value = null; return { totpRequired: true }; } - totpRequired.value = false; - hasStatus.value = true; + + totpPending.value = false; currentUser.value = response?.user || null; // Clear guest session when user logs in @@ -113,10 +123,11 @@ export const useAuthStore = defineStore('auth', () => { }; /** - * Signing in with a passkey. + * Sign in with a passkey, which names nobody. * - * Nobody is named: the authenticator offers what it holds for this site, and - * the server works out whose key it is from the key itself. + * The same two endings as a password: signed in, or waiting for a code. A + * passkey that was unlocked — a fingerprint, a face, a PIN — is already the + * second factor, so only one that was not lands here waiting. */ const signInWithPasskey = async () => { lastError.value = null; @@ -124,26 +135,48 @@ export const useAuthStore = defineStore('auth', () => { hasStatus.value = true; if (response?.totpRequired) { - totpRequired.value = true; + totpPending.value = true; currentUser.value = null; return { totpRequired: true }; } - totpRequired.value = false; + totpPending.value = false; currentUser.value = response?.user || null; sessionStorage.removeItem('guestSessionId'); return { totpRequired: false }; }; - /** The second step. Which account this is remains the server's to know. */ + /** + * Finish a sign-in with the code from the phone, or one off the paper. + * + * @returns {Promise<{usedRecoveryCode: boolean, recoveryCodesLeft: number|null}>} + */ const submitTotpCode = async (code) => { lastError.value = null; const response = await submitTotpCodeApi(code); - totpRequired.value = false; - hasStatus.value = true; + totpPending.value = false; currentUser.value = response?.user || null; sessionStorage.removeItem('guestSessionId'); - return response; + return { + usedRecoveryCode: Boolean(response?.usedRecoveryCode), + recoveryCodesLeft: response?.recoveryCodesLeft ?? null, + }; + }; + + /** + * Back to the password, when somebody gives up on finding their phone. + * + * The server is told, rather than only the screen: it is the one holding the + * half-open sign-in, and a page reload would otherwise come back to the code + * for a step this person has already walked away from. + */ + const cancelTotp = async () => { + totpPending.value = false; + try { + await logoutApi(); + } catch (_) { + // Nothing was signed in; a server that cannot be reached changes that. + } }; const logout = async () => { @@ -155,6 +188,26 @@ export const useAuthStore = defineStore('auth', () => { } hasStatus.value = true; currentUser.value = null; + totpPending.value = false; + }; + + /** + * Drop the session locally, without telling the server. + * + * For a session that has already expired: there is nothing left to end, and + * asking the server to end it would be one more request answered 401 — or, + * where an identity provider is involved, a redirect to it that a fetch + * cannot follow. `hasStatus` stays true so the navigation guard sends the + * person to the login screen rather than pausing to ask the server who they + * are, which is the question that just failed. + */ + const forgetSession = () => { + currentUser.value = null; + // A session that is over is not one halfway through: whatever was waiting + // for a code is gone with it, and the screen starts at the password. + totpPending.value = false; + hasStatus.value = true; + lastError.value = null; }; const clearError = () => { @@ -181,16 +234,19 @@ export const useAuthStore = defineStore('auth', () => { authEnabled, authMode, strategies, + oidcStatus, currentUser, + totpPending, lastError, initialize, ensureStatus: initialize, setupAccount, login, - totpRequired, - submitTotpCode, signInWithPasskey, + submitTotpCode, + cancelTotp, logout, + forgetSession, clearError, refreshCurrentUser, }; From 3698d6c170f12f55037e850cd7f3837c727a7edd Mon Sep 17 00:00:00 2001 From: Benjy Date: Sat, 26 Sep 2026 20:37:52 +0200 Subject: [PATCH 3/4] Say what the cache directory holds, and what the process is costing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two reports the server makes about itself at start, neither of which existed. **What releases up to 1.1.7 left in the cache.** The database and app-config.json lived in CACHE_DIR until 1.1.8, which moved them to CONFIG_DIR and left links behind; 2.0.3 removed that move from the entrypoint. So an installation that started on 1.1.7 or earlier and skipped the releases in between comes up on a new, empty app.db in CONFIG_DIR with its accounts, shares and settings sitting unread in the cache. Nothing said so: the server started, the sign-in page offered to create the first administrator, and the answer looked like a fresh installation rather than a lost one. It is a notice and nothing more — nothing is moved, nothing is deleted. It names what it found and where, and says what to do with it. **What the process is costing.** `services/performanceDiagnostics.js` samples CPU, resident memory as the cgroup sees it rather than as the host does, event-loop delay at p99, and the queues that can grow: thumbnails, folder sizes, transfers. Off unless PERFORMANCE_DIAGNOSTICS_ENABLED is set, and then it reports only the intervals that pass a threshold — a diagnostic that logs every interval by default is a diagnostic that fills a disk. PERFORMANCE_DIAGNOSTICS_LOG_EVERY_INTERVAL asks for all of them. An interval below its floor is held to the default: a sampler on a 1 ms interval costs more than whatever it was meant to diagnose, and 1 ms is what an emptied field sends. Each queue reports itself through an optional call. One that has no report is a queue this installation has nothing to say about, not a reason for the whole record to fail — a diagnostic that throws says nothing at the moment it is most wanted. ## Checks `legacy-cache-check.test.js`, 3 tests: a cache holding the old names is named, one holding the links 1.1.8 left is named differently, and an ordinary cache says nothing. Making the inspection always see an empty cache turns all three red. `performance-diagnostics.test.js`, 7 tests: silent unless asked for, says what it will watch and by which thresholds, holds an interval below its floor to the default, reports nothing of an ordinary interval, reports one that passes a threshold and says which kind it was, reports every interval when told to, and samples the machine rather than guessing. Making it start whether or not it was asked for turns the first red. Whole backend suite: 2 556 passed, 2 failed — the two that fail on `main` on its own. --- backend/src/config/env.js | 22 +++ backend/src/config/index.js | 15 ++ backend/src/server.js | 13 ++ backend/src/services/legacyCacheCheck.js | 83 ++++++++ .../src/services/performanceDiagnostics.js | 178 ++++++++++++++++++ .../tests/services/legacy-cache-check.test.js | 86 +++++++++ .../services/performance-diagnostics.test.js | 153 +++++++++++++++ 7 files changed, 550 insertions(+) create mode 100644 backend/src/services/legacyCacheCheck.js create mode 100644 backend/src/services/performanceDiagnostics.js create mode 100644 backend/tests/services/legacy-cache-check.test.js create mode 100644 backend/tests/services/performance-diagnostics.test.js diff --git a/backend/src/config/env.js b/backend/src/config/env.js index 988d9bf83..1001f3d91 100644 --- a/backend/src/config/env.js +++ b/backend/src/config/env.js @@ -77,6 +77,28 @@ module.exports = { FOLDER_SIZE_MODE: process.env.FOLDER_SIZE_MODE?.trim().toLowerCase() || 'off', FOLDER_SIZE_MODE_SET: typeof process.env.FOLDER_SIZE_MODE === 'string' && process.env.FOLDER_SIZE_MODE.trim() !== '', + // Lightweight process and cgroup diagnostics, off by default. When enabled the + // sampler logs only anomalous intervals unless explicitly told otherwise. + PERFORMANCE_DIAGNOSTICS_ENABLED: + normalizeBoolean(process.env.PERFORMANCE_DIAGNOSTICS_ENABLED) ?? false, + PERFORMANCE_DIAGNOSTICS_INTERVAL_MS: + process.env.PERFORMANCE_DIAGNOSTICS_INTERVAL_MS != null + ? Number(process.env.PERFORMANCE_DIAGNOSTICS_INTERVAL_MS) + : 15000, + PERFORMANCE_DIAGNOSTICS_LOG_EVERY_INTERVAL: + normalizeBoolean(process.env.PERFORMANCE_DIAGNOSTICS_LOG_EVERY_INTERVAL) ?? false, + PERFORMANCE_DIAGNOSTICS_CPU_THRESHOLD: + process.env.PERFORMANCE_DIAGNOSTICS_CPU_THRESHOLD != null + ? Number(process.env.PERFORMANCE_DIAGNOSTICS_CPU_THRESHOLD) + : 75, + PERFORMANCE_DIAGNOSTICS_RSS_THRESHOLD_MB: + process.env.PERFORMANCE_DIAGNOSTICS_RSS_THRESHOLD_MB != null + ? Number(process.env.PERFORMANCE_DIAGNOSTICS_RSS_THRESHOLD_MB) + : 768, + PERFORMANCE_DIAGNOSTICS_EVENT_LOOP_DELAY_MS: + process.env.PERFORMANCE_DIAGNOSTICS_EVENT_LOOP_DELAY_MS != null + ? Number(process.env.PERFORMANCE_DIAGNOSTICS_EVENT_LOOP_DELAY_MS) + : 250, FOLDER_SIZE_EXCLUDE_PATHS: process.env.FOLDER_SIZE_EXCLUDE_PATHS || '', FOLDER_SIZE_CONCURRENCY: Number(process.env.FOLDER_SIZE_CONCURRENCY) || 6, FOLDER_SIZE_NETWORK_CONCURRENCY: Number(process.env.FOLDER_SIZE_NETWORK_CONCURRENCY) || 2, diff --git a/backend/src/config/index.js b/backend/src/config/index.js index 4bc9da340..7994d8b38 100644 --- a/backend/src/config/index.js +++ b/backend/src/config/index.js @@ -661,7 +661,22 @@ const folderSize = { rebuild: env.FOLDER_SIZE_REBUILD, }; +// --- Runtime diagnostics --- +// --- Runtime diagnostics --- +const atLeast = (value, minimum, fallback) => + Number.isFinite(value) && value >= minimum ? value : fallback; + +const performanceDiagnostics = { + enabled: env.PERFORMANCE_DIAGNOSTICS_ENABLED, + intervalMs: atLeast(env.PERFORMANCE_DIAGNOSTICS_INTERVAL_MS, 5000, 15000), + logEveryInterval: env.PERFORMANCE_DIAGNOSTICS_LOG_EVERY_INTERVAL, + cpuThreshold: atLeast(env.PERFORMANCE_DIAGNOSTICS_CPU_THRESHOLD, 1, 75), + rssThresholdMb: atLeast(env.PERFORMANCE_DIAGNOSTICS_RSS_THRESHOLD_MB, 1, 768), + eventLoopDelayThresholdMs: atLeast(env.PERFORMANCE_DIAGNOSTICS_EVENT_LOOP_DELAY_MS, 1, 250), +}; + module.exports = { + performanceDiagnostics, folderSize, webauthn, activity, diff --git a/backend/src/server.js b/backend/src/server.js index 4aa8dc520..db5bbaf44 100644 --- a/backend/src/server.js +++ b/backend/src/server.js @@ -22,6 +22,8 @@ const capabilities = require('./services/capabilities'); const { installProcessFailureHandlers } = require('./utils/processFailures'); const { sweepUnreferencedLogos } = require('./services/brandingLogo'); const featureSwitches = require('./services/featureSwitches'); +const { reportLegacyCache } = require('./services/legacyCacheCheck'); +const performanceDiagnostics = require('./services/performanceDiagnostics'); let server = null; @@ -114,6 +116,16 @@ const startServer = async () => { expirySweep.unref?.(); void sweepExpiredRecords(); + // What early releases left in the cache directory: the database and app-config.json + // lived there up to 1.1.7, and an installation that skipped the releases in between + // comes up on a new, empty app.db with its accounts and shares sitting unread. + reportLegacyCache(); + + // A periodic record of what the process is costing — CPU, resident memory, event-loop + // delay, and the queues that can grow. Off unless PERFORMANCE_DIAGNOSTICS_ENABLED is + // set, and then it says only the intervals that look wrong. + performanceDiagnostics.start(); + // A logo left behind by a stop in the middle of a branding change, or by a // removal that failed, is 2 MB nothing can reach. Here, where nothing is being // placed, so a file under one of our names is a finished one. @@ -130,6 +142,7 @@ const startServer = async () => { folderSizeManager.stop(); trashMaintenance.stop(); searchIndexManager.stop(); + performanceDiagnostics.stop(); server.close(() => { logger.info('Server closed'); process.exit(0); diff --git a/backend/src/services/legacyCacheCheck.js b/backend/src/services/legacyCacheCheck.js new file mode 100644 index 000000000..0a9f67950 --- /dev/null +++ b/backend/src/services/legacyCacheCheck.js @@ -0,0 +1,83 @@ +const fs = require('fs'); +const path = require('path'); + +const { directories } = require('../config/index'); +const logger = require('../utils/logger'); + +/** + * What early releases left in the cache directory, said out loud at start. + * + * Up to 1.1.7 the database and app-config.json lived in the cache directory. + * 1.1.8 moved them to the config directory and left links behind in their + * place; 2.0.3 removed that move from the entrypoint. So an installation that + * started on 1.1.7 or earlier and skipped the releases in between comes up on a + * new, empty app.db in /config, with its accounts and shares sitting unread in + * /cache — and nothing said so. The links, where an installation passed through + * 1.1.8 to 2.0.2, are harmless but look like data. + * + * Nothing is moved: which of two databases holds what matters cannot be told + * from here, and guessing wrong would overwrite the one in use. The log says + * where the old file is and what to do with it. + */ + +const LEGACY_NAMES = ['app.db', 'app-config.json', 'extensions']; + +const readLinkOrNull = (file) => { + try { + return fs.readlinkSync(file); + } catch { + return null; + } +}; + +/** What is there, without following anything. */ +const inspectLegacyCache = (cacheDir = directories.cache) => { + const findings = []; + for (const name of LEGACY_NAMES) { + const file = path.join(cacheDir, name); + let stats; + try { + stats = fs.lstatSync(file); + } catch { + continue; + } + if (stats.isSymbolicLink()) { + findings.push({ name, path: file, kind: 'link', target: readLinkOrNull(file) }); + } else if (name === 'app.db' && stats.isFile()) { + findings.push({ name, path: file, kind: 'database', sizeBytes: stats.size }); + } + } + return findings; +}; + +const reportLegacyCache = ({ + cacheDir = directories.cache, + configDir = directories.config, + log = logger, +} = {}) => { + const findings = inspectLegacyCache(cacheDir); + + const database = findings.find((finding) => finding.kind === 'database'); + if (database) { + log.warn( + { + legacyDatabase: database.path, + sizeBytes: database.sizeBytes, + databaseInUse: path.join(configDir, 'app.db'), + }, + 'An app.db written by release 1.1.7 or earlier is in the cache directory, and nothing reads it: this server runs on the app.db in the config directory. If accounts, shares or favorites are missing, stop the container, back up both files, and copy the old one over the one in the config directory.' + ); + } + + const links = findings.filter((finding) => finding.kind === 'link'); + if (links.length > 0) { + log.info( + { links: links.map((link) => `${link.path} -> ${link.target}`) }, + 'Links left in the cache directory by releases 1.1.8 to 2.0.2 are unused and can be deleted.' + ); + } + + return findings; +}; + +module.exports = { inspectLegacyCache, reportLegacyCache }; diff --git a/backend/src/services/performanceDiagnostics.js b/backend/src/services/performanceDiagnostics.js new file mode 100644 index 000000000..0b3d65faa --- /dev/null +++ b/backend/src/services/performanceDiagnostics.js @@ -0,0 +1,178 @@ +const fs = require('fs/promises'); +const { monitorEventLoopDelay, performance } = require('perf_hooks'); + +const { performanceDiagnostics: config } = require('../config'); +const logger = require('../utils/logger'); +const thumbnailService = require('./thumbnailService'); +const folderSizeManager = require('./folderSizeManager'); +const fileTransferService = require('./fileTransferService'); + +let timer = null; +let previousSample = null; +let eventLoopDelay = null; + +const toMb = (bytes) => Math.round((Number(bytes) || 0) / 1024 / 1024); + +const readText = async (filePath) => { + try { + return (await fs.readFile(filePath, 'utf8')).trim(); + } catch (_) { + return null; + } +}; + +const readNumber = async (filePath) => { + const value = await readText(filePath); + if (value == null || value === 'max') return null; + const number = Number(value); + return Number.isFinite(number) ? number : null; +}; + +const readKeyValueFile = async (filePath, allowedKeys) => { + const content = await readText(filePath); + if (!content) return null; + const result = {}; + for (const line of content.split('\n')) { + const [key, rawValue] = line.trim().split(/\s+/, 2); + if (!allowedKeys.has(key)) continue; + const value = Number(rawValue); + if (Number.isFinite(value)) result[key] = toMb(value); + } + return result; +}; + +const readCgroupMemory = async () => { + const v2Current = await readNumber('/sys/fs/cgroup/memory.current'); + const v2Limit = await readNumber('/sys/fs/cgroup/memory.max'); + const isV2 = v2Current != null; + const current = isV2 + ? v2Current + : await readNumber('/sys/fs/cgroup/memory/memory.usage_in_bytes'); + const limit = isV2 ? v2Limit : await readNumber('/sys/fs/cgroup/memory/memory.limit_in_bytes'); + const stat = await readKeyValueFile( + isV2 ? '/sys/fs/cgroup/memory.stat' : '/sys/fs/cgroup/memory/memory.stat', + new Set(['anon', 'file', 'slab', 'slab_reclaimable', 'slab_unreclaimable', 'cache', 'rss']) + ); + + if (current == null && !stat) return null; + return { + currentMb: toMb(current), + ...(limit != null ? { limitMb: toMb(limit) } : {}), + ...(stat ? { statMb: stat } : {}), + }; +}; + +const activeResourceCounts = () => { + if (typeof process.getActiveResourcesInfo !== 'function') return undefined; + return process.getActiveResourcesInfo().reduce((counts, name) => { + counts[name] = (counts[name] || 0) + 1; + return counts; + }, {}); +}; + +const sample = async () => { + const now = performance.now(); + const cpu = process.cpuUsage(); + const memory = process.memoryUsage(); + const [cgroupMemory, thumbnail, folderSize, transfers] = await Promise.all([ + readCgroupMemory(), + // Each queue reports itself when it can. One that does not is a queue this + // installation has no report for, not a reason for the whole record to fail — a + // diagnostic that throws is a diagnostic that says nothing at the moment it is + // most wanted. + Promise.resolve(thumbnailService.getDiagnosticsSnapshot?.() ?? null), + Promise.resolve(folderSizeManager.getDiagnosticsSnapshot?.() ?? null), + Promise.resolve(fileTransferService.getDiagnosticsSnapshot?.() ?? null), + ]); + + const elapsedMs = previousSample ? Math.max(1, now - previousSample.at) : null; + const cpuDeltaUs = previousSample + ? cpu.user - previousSample.cpu.user + (cpu.system - previousSample.cpu.system) + : null; + const cpuPercent = + elapsedMs != null && cpuDeltaUs != null + ? Math.round((cpuDeltaUs / 1000 / elapsedMs) * 100) + : null; + const loopDelayMs = eventLoopDelay + ? Number(eventLoopDelay.percentile(99) / 1e6).toFixed(1) + : null; + const eventLoopUtilization = previousSample?.eventLoopUtilization + ? performance.eventLoopUtilization(previousSample.eventLoopUtilization) + : null; + + previousSample = { + at: now, + cpu, + eventLoopUtilization: performance.eventLoopUtilization(), + }; + eventLoopDelay?.reset(); + + return { + cpuPercent, + ...(eventLoopUtilization + ? { eventLoopUtilizationPercent: Math.round(eventLoopUtilization.utilization * 100) } + : {}), + ...(loopDelayMs != null ? { eventLoopP99DelayMs: Number(loopDelayMs) } : {}), + memoryMb: { + rss: toMb(memory.rss), + heapUsed: toMb(memory.heapUsed), + heapTotal: toMb(memory.heapTotal), + external: toMb(memory.external), + arrayBuffers: toMb(memory.arrayBuffers), + }, + cgroupMemory, + resources: activeResourceCounts(), + thumbnail, + folderSize, + transfers, + }; +}; + +const isPressure = (snapshot) => + (snapshot.cpuPercent ?? 0) >= config.cpuThreshold || + snapshot.memoryMb.rss >= config.rssThresholdMb || + (snapshot.eventLoopP99DelayMs ?? 0) >= config.eventLoopDelayThresholdMs; + +const start = () => { + if (!config.enabled || timer) return; + + eventLoopDelay = monitorEventLoopDelay({ resolution: 20 }); + eventLoopDelay.enable(); + logger.info( + { + intervalMs: config.intervalMs, + cpuThreshold: config.cpuThreshold, + rssThresholdMb: config.rssThresholdMb, + eventLoopDelayThresholdMs: config.eventLoopDelayThresholdMs, + logEveryInterval: config.logEveryInterval, + }, + 'Performance diagnostics enabled' + ); + + const tick = () => { + sample() + .then((snapshot) => { + if (config.logEveryInterval || isPressure(snapshot)) { + logger.info( + { reason: isPressure(snapshot) ? 'resource-pressure' : 'interval', ...snapshot }, + 'Performance diagnostics' + ); + } + }) + .catch((err) => logger.debug({ err }, 'Performance diagnostics sample failed')); + }; + + tick(); + timer = setInterval(tick, config.intervalMs); + if (typeof timer.unref === 'function') timer.unref(); +}; + +const stop = () => { + if (timer) clearInterval(timer); + timer = null; + eventLoopDelay?.disable(); + eventLoopDelay = null; + previousSample = null; +}; + +module.exports = { start, stop, sample }; diff --git a/backend/tests/services/legacy-cache-check.test.js b/backend/tests/services/legacy-cache-check.test.js new file mode 100644 index 000000000..5dcfa6ecc --- /dev/null +++ b/backend/tests/services/legacy-cache-check.test.js @@ -0,0 +1,86 @@ +import fs from 'node:fs'; +import path from 'node:path'; + +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { setupTestEnv } from '../helpers/env-test-utils.js'; + +/** + * What early releases left in the cache directory. + * + * An installation that started on 1.1.7 or earlier kept its database in /cache; + * the move to /config that 1.1.8 made was removed in 2.0.3, so one that skipped + * the releases in between comes up on an empty app.db with its accounts unread + * in /cache. Nothing said so. Now the start does — and moves nothing, since + * which file holds what matters cannot be told from here. + */ + +let envContext; + +afterEach(async () => { + if (envContext) await envContext.cleanup(); + envContext = null; +}); + +const setup = async () => { + envContext = await setupTestEnv({ tag: 'legacy-cache-' }); + const check = envContext.requireFresh('src/services/legacyCacheCheck'); + const log = { warn: vi.fn(), info: vi.fn() }; + const report = () => + check.reportLegacyCache({ + cacheDir: envContext.cacheDir, + configDir: envContext.configDir, + log, + }); + return { check, log, report, cache: envContext.cacheDir, config: envContext.configDir }; +}; + +describe('an app.db left in the cache directory', () => { + it('is reported as a warning naming both files, and left where it is', async () => { + const { log, report, cache, config } = await setup(); + fs.writeFileSync(path.join(cache, 'app.db'), 'SQLite format 3\0 with the old accounts'); + + const findings = report(); + + expect(findings).toEqual([expect.objectContaining({ name: 'app.db', kind: 'database' })]); + expect(log.warn).toHaveBeenCalledTimes(1); + expect(log.warn.mock.calls[0][0]).toMatchObject({ + legacyDatabase: path.join(cache, 'app.db'), + databaseInUse: path.join(config, 'app.db'), + }); + expect(fs.existsSync(path.join(cache, 'app.db'))).toBe(true); + }); +}); + +describe('links left in the cache directory', () => { + it('are mentioned as unused, not warned about, and not followed', async () => { + const { log, report, cache, config } = await setup(); + fs.writeFileSync(path.join(config, 'app-config.json'), '{}'); + fs.symlinkSync(path.join(config, 'app.db'), path.join(cache, 'app.db')); + fs.symlinkSync(path.join(config, 'app-config.json'), path.join(cache, 'app-config.json')); + fs.symlinkSync(path.join(config, 'extensions'), path.join(cache, 'extensions')); + + const findings = report(); + + expect(findings.map((finding) => [finding.name, finding.kind])).toEqual([ + ['app.db', 'link'], + ['app-config.json', 'link'], + ['extensions', 'link'], + ]); + expect(log.warn).not.toHaveBeenCalled(); + expect(log.info).toHaveBeenCalledTimes(1); + expect(log.info.mock.calls[0][0].links).toHaveLength(3); + }); +}); + +describe('a cache directory with nothing from early releases', () => { + it('says nothing', async () => { + const { log, report, cache } = await setup(); + fs.mkdirSync(path.join(cache, 'thumbnails'), { recursive: true }); + fs.writeFileSync(path.join(cache, 'index.db'), 'SQLite format 3\0'); + + expect(report()).toEqual([]); + expect(log.warn).not.toHaveBeenCalled(); + expect(log.info).not.toHaveBeenCalled(); + }); +}); diff --git a/backend/tests/services/performance-diagnostics.test.js b/backend/tests/services/performance-diagnostics.test.js new file mode 100644 index 000000000..2e3b7c010 --- /dev/null +++ b/backend/tests/services/performance-diagnostics.test.js @@ -0,0 +1,153 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { setupTestEnv } from '../helpers/env-test-utils.js'; + +/** + * The periodic record of what the process is costing. + * + * It exists for the case nobody can reproduce: an installation that goes slow after + * hours, on storage nobody here has, with a load nobody here makes. The only useful + * answer is what the process was costing at the time, so the sampler reports CPU, + * resident memory as the cgroup sees it, event-loop delay, and the queues that grow. + * + * What is worth testing is not the numbers — they are the machine's — but the three + * decisions around them: that it says nothing at all unless it was asked for, that it + * then reports only the intervals that look wrong, and that it can be told to report + * every one. A diagnostic that logs on every interval by accident is a diagnostic that + * fills a disk. + */ + +let env; + +afterEach(async () => { + vi.restoreAllMocks(); + if (env) { + env.requireFresh('src/services/performanceDiagnostics').stop(); + await env.cleanup(); + } + env = null; +}); + +const load = async (extraEnv = {}) => { + env = await setupTestEnv({ tag: 'perf-diagnostics-', env: extraEnv }); + // The logger first, and spied on before the service is loaded: the service keeps + // whichever logger it was given at require time, so spying on a fresh one afterwards + // watches an object nothing writes to. + const logger = env.requireFresh('src/utils/logger'); + const said = vi.spyOn(logger, 'info'); + const diagnostics = env.requireFresh('src/services/performanceDiagnostics'); + return { diagnostics, said }; +}; + +/** Every message a logger spy was given, as one string. */ +const messages = (spy) => spy.mock.calls.map((call) => String(call[1] ?? call[0])).join('\n'); + +describe('the performance record', () => { + it('is silent unless somebody asked for it', async () => { + const { diagnostics, said } = await load(); + + diagnostics.start(); + + expect(messages(said)).not.toContain('Performance diagnostics'); + }); + + it('says what it will watch, and by which thresholds, when it is on', async () => { + const { diagnostics, said } = await load({ + PERFORMANCE_DIAGNOSTICS_ENABLED: 'true', + PERFORMANCE_DIAGNOSTICS_INTERVAL_MS: '60000', + PERFORMANCE_DIAGNOSTICS_CPU_THRESHOLD: '90', + }); + + diagnostics.start(); + + expect(messages(said)).toContain('Performance diagnostics enabled'); + const announced = said.mock.calls.find(([, message]) => /enabled/.test(String(message)))[0]; + expect(announced.intervalMs).toBe(60000); + expect(announced.cpuThreshold).toBe(90); + }); + + it('holds an interval below its floor to the default, rather than sampling constantly', async () => { + // What an emptied or mistyped field sends. A sampler on a 1 ms interval costs more + // than whatever it was meant to diagnose. + const { diagnostics, said } = await load({ + PERFORMANCE_DIAGNOSTICS_ENABLED: 'true', + PERFORMANCE_DIAGNOSTICS_INTERVAL_MS: '1', + }); + + diagnostics.start(); + + const announced = said.mock.calls.find(([, message]) => /enabled/.test(String(message)))[0]; + expect(announced.intervalMs).toBe(15000); + }); + + it('reports nothing of an interval that looks ordinary', async () => { + const { diagnostics, said } = await load({ + PERFORMANCE_DIAGNOSTICS_ENABLED: 'true', + // Thresholds nothing here will reach. + PERFORMANCE_DIAGNOSTICS_CPU_THRESHOLD: '100000', + PERFORMANCE_DIAGNOSTICS_RSS_THRESHOLD_MB: '100000', + PERFORMANCE_DIAGNOSTICS_EVENT_LOOP_DELAY_MS: '100000', + }); + + diagnostics.start(); + await vi.waitFor(() => expect(messages(said)).toContain('enabled')); + await new Promise((resolve) => setTimeout(resolve, 50)); + + const records = said.mock.calls.filter(([, message]) => message === 'Performance diagnostics'); + expect(records).toEqual([]); + }); + + it('reports one that passes a threshold, and says which kind of interval it was', async () => { + const { diagnostics, said } = await load({ + PERFORMANCE_DIAGNOSTICS_ENABLED: 'true', + // A memory threshold of nothing: every interval is past it. + PERFORMANCE_DIAGNOSTICS_RSS_THRESHOLD_MB: '1', + }); + + diagnostics.start(); + + await vi.waitFor(() => { + const records = said.mock.calls.filter( + ([, message]) => message === 'Performance diagnostics' + ); + expect(records.length).toBeGreaterThan(0); + expect(records[0][0].reason).toBe('resource-pressure'); + }); + }); + + it('reports every interval when it is told to', async () => { + const { diagnostics, said } = await load({ + PERFORMANCE_DIAGNOSTICS_ENABLED: 'true', + PERFORMANCE_DIAGNOSTICS_LOG_EVERY_INTERVAL: 'true', + PERFORMANCE_DIAGNOSTICS_CPU_THRESHOLD: '100000', + PERFORMANCE_DIAGNOSTICS_RSS_THRESHOLD_MB: '100000', + PERFORMANCE_DIAGNOSTICS_EVENT_LOOP_DELAY_MS: '100000', + }); + + diagnostics.start(); + + await vi.waitFor(() => { + const records = said.mock.calls.filter( + ([, message]) => message === 'Performance diagnostics' + ); + expect(records.length).toBeGreaterThan(0); + // Nothing was under pressure: it is reporting because it was asked to. + expect(records[0][0].reason).toBe('interval'); + }); + }); + + it('samples the machine rather than guessing at it', async () => { + const { diagnostics } = await load({ PERFORMANCE_DIAGNOSTICS_ENABLED: 'true' }); + + const snapshot = await diagnostics.sample(); + + // `toMb` rounds, and this process is small enough to round to zero on some + // machines, so what is asserted is that the numbers came from somewhere rather + // than what they are. + expect(typeof snapshot.memoryMb.rss).toBe('number'); + expect(snapshot.memoryMb.heapTotal).toBeGreaterThan(0); + // And the queues each answered, or said they had nothing to answer with. + expect(snapshot).toHaveProperty('resources'); + expect(snapshot).toHaveProperty('cpuPercent'); + }); +}); From 31ed5c4287dc5ef335d022b63625eee98a06afc3 Mon Sep 17 00:00:00 2001 From: Benjy Date: Sat, 26 Sep 2026 20:39:58 +0200 Subject: [PATCH 4/4] Let the preview's ceiling be set, and stop a proxy keeping a listing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/api/features` already offers `preview.maxRenderBytes` to the screen, and the configuration had no such value, so it answered `null` and the preview rendered whatever it was given. A document large enough to render slowly renders slowly for everybody on the machine, and nobody could raise or lower the point at which it stops trying. PREVIEW_MAX_RENDER_SIZE sets it; 16 MB when it is not set, and a value below zero or unparseable is that default rather than a ceiling of nothing. And the listing: `GET /api/browse` answers with what is true at that moment — which documents somebody has open in an editor, what a folder weighs, whether a write would be refused. A GET with no cache header is cacheable by default, so a proxy or a browser was free to keep it and serve a folder as it was: a deleted file still listed, a document shown as open by somebody who closed it an hour ago. `private, no-store` says what it is. ## Checks `browse-caching.test.js` asserts both halves of the header, and taking the header away turns it red. The ceiling is read through `/api/features`, which has offered the field since the batch that brought the features route and was answering `null` for it. Whole backend suite: 2 556 passed, 2 failed — the two that fail on `main` on its own. --- backend/src/config/env.js | 1 + backend/src/config/index.js | 27 ++++++++++++ backend/src/routes/browse.js | 3 ++ backend/tests/routes/browse-caching.test.js | 49 +++++++++++++++++++++ 4 files changed, 80 insertions(+) create mode 100644 backend/tests/routes/browse-caching.test.js diff --git a/backend/src/config/env.js b/backend/src/config/env.js index 1001f3d91..1fad07b0b 100644 --- a/backend/src/config/env.js +++ b/backend/src/config/env.js @@ -118,6 +118,7 @@ module.exports = { SEARCH_INDEX_CPU_PERCENT: Number(process.env.SEARCH_INDEX_CPU_PERCENT) || null, SEARCH_INDEX_MEMORY_MB: Number(process.env.SEARCH_INDEX_MEMORY_MB) || null, SEARCH_INDEX_EXCLUDE: process.env.SEARCH_INDEX_EXCLUDE?.trim() || null, + PREVIEW_MAX_RENDER_SIZE: process.env.PREVIEW_MAX_RENDER_SIZE?.trim() || null, SEARCH_INDEX_REBUILD: normalizeBoolean(process.env.SEARCH_INDEX_REBUILD) ?? false, SEARCH_INDEX_RECONCILE_MS: Number(process.env.SEARCH_INDEX_RECONCILE_MS) || null, SEARCH_TIMEOUT_MS: Number(process.env.SEARCH_TIMEOUT_MS) || null, diff --git a/backend/src/config/index.js b/backend/src/config/index.js index 7994d8b38..dff388378 100644 --- a/backend/src/config/index.js +++ b/backend/src/config/index.js @@ -675,8 +675,35 @@ const performanceDiagnostics = { eventLoopDelayThresholdMs: atLeast(env.PERFORMANCE_DIAGNOSTICS_EVENT_LOOP_DELAY_MS, 1, 250), }; +/** + * How much of a document the preview will render. + * + * Not the same question as what the editor will open, and the difference is + * why this is a setting of its own. The editor streams text into a code view; + * the preview parses the document, sanitises the HTML it produces and then + * hands the browser every node to lay out — all on the one thread the + * interface has. A six-megabyte markdown file opens in the editor and freezes + * the tab in the preview, on the same machine, from the same file. + * + * It was hard-coded before this, which meant someone who raised + * EDITOR_MAX_FILESIZE in good faith was refused at a number that appeared in + * no setting and no document. + * + * Generous by default because freezing is no longer the failure mode: the + * preview renders in slices of a frame and hands the browser back between + * them. What is left is the weight of the document in the tab, which is a + * reader's problem rather than an application's. And the preview reads through + * the editor's endpoint, so EDITOR_MAX_FILESIZE already caps what can reach + * it — this only bites when it is set lower than that. + */ +const previewMaxRenderBytes = (() => { + const parsed = parseByteSize(env.PREVIEW_MAX_RENDER_SIZE); + return Number.isFinite(parsed) && parsed > 0 ? parsed : 16 * 1024 * 1024; +})(); + module.exports = { performanceDiagnostics, + preview: { maxRenderBytes: previewMaxRenderBytes }, folderSize, webauthn, activity, diff --git a/backend/src/routes/browse.js b/backend/src/routes/browse.js index 1038237c8..5943ea983 100644 --- a/backend/src/routes/browse.js +++ b/backend/src/routes/browse.js @@ -126,6 +126,9 @@ router.get( sourceFolderName: pathParts[pathParts.length - 1] || '', }; } + // Listings carry transient information such as active OnlyOffice sessions. + // Keep browser and proxy caches from serving an out-of-date directory view. + res.setHeader('Cache-Control', 'private, no-store'); res.json(response); }) diff --git a/backend/tests/routes/browse-caching.test.js b/backend/tests/routes/browse-caching.test.js new file mode 100644 index 000000000..56d2c75bb --- /dev/null +++ b/backend/tests/routes/browse-caching.test.js @@ -0,0 +1,49 @@ +import express from 'express'; +import request from 'supertest'; +import { afterEach, describe, expect, it } from 'vitest'; + +import { setupTestEnv } from '../helpers/env-test-utils.js'; + +/** + * A listing is not a document, and must not be cached as one. + * + * `GET /api/browse` carries what is true at that moment: which documents somebody has + * open in an editor, what a folder weighs, whether a write would be refused. None of + * that is worth remembering, and a proxy or a browser that remembers it serves a view + * of a folder as it was — a file that was deleted still listed, a document shown as + * open by somebody who closed it an hour ago. + * + * No header said so, and the answer to a GET with none is cacheable by default. + */ + +let env; + +afterEach(async () => { + if (env) await env.cleanup(); + env = null; +}); + +const app = () => { + const server = express(); + server.use((req, _res, next) => { + req.user = { id: 'admin-1', roles: ['admin'] }; + next(); + }); + server.use('/api', env.requireFresh('src/routes/browse')); + server.use(env.requireFresh('src/middleware/errorHandler').errorHandler); + return server; +}; + +describe('the answer to a listing', () => { + it('is not to be kept by a browser or a proxy', async () => { + env = await setupTestEnv({ tag: 'browse-caching-' }); + + const response = await request(app()).get('/api/browse/'); + + expect(response.status).toBe(200); + // `private` keeps a shared proxy out of it; `no-store` keeps the browser from + // answering the next navigation from what it already has. + expect(response.headers['cache-control']).toContain('no-store'); + expect(response.headers['cache-control']).toContain('private'); + }); +});