From 1de88757f8173b29f6e4fcb16d6c4ceceef456ed Mon Sep 17 00:00:00 2001 From: Benjy Date: Fri, 25 Sep 2026 09:24:22 +0200 Subject: [PATCH 1/3] Keep what a save through ONLYOFFICE replaces, and never empty the document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Document Server does not send the document back: it sends a URL to fetch it from. The file was emptied first and then written as the answer arrived, so a stream that stopped arriving — a dropped network, a Document Server restarting mid-answer — left an empty file where the work had been. The state the save replaced was gone in every case anyway, since nothing kept it. Both are the same change. The document is pulled into a file of its own beside it and handed to the versions, which keep what it replaces and put the new content in place only once it is whole. A save nobody could finish therefore leaves the document exactly as it was, and the history the engine has been keeping since it arrived is finally written by somebody: the versions have been inert until now, because nothing in the application saved over a file. A save on purpose — the editor's own Save, or the last one made once everybody has left — is marked as such, so the automatic saves in between do not each stand as a state of their own; everyone editing together shares the document key, which is the session those saves belong to. The person credited is the one whose changes the callback carries, or the account the editing session was opened for when it carries none; somebody who came through a share link is credited as the link, since there is no account to name. Seven tests against a Document Server that hands the document over, refuses to, or dies halfway through. Five of them fail on the previous write: the document is emptied by a download that never finishes, and nothing is kept. --- backend/src/routes/onlyoffice.js | 81 ++++++- backend/tests/routes/onlyoffice-save.test.js | 231 +++++++++++++++++++ 2 files changed, 302 insertions(+), 10 deletions(-) create mode 100644 backend/tests/routes/onlyoffice-save.test.js diff --git a/backend/src/routes/onlyoffice.js b/backend/src/routes/onlyoffice.js index 5566d866..e8654af5 100644 --- a/backend/src/routes/onlyoffice.js +++ b/backend/src/routes/onlyoffice.js @@ -3,6 +3,7 @@ const path = require('path'); const fs = require('fs'); const fsp = require('fs/promises'); const crypto = require('crypto'); +const { pipeline } = require('stream/promises'); const axios = require('axios'); const jwt = require('jsonwebtoken'); @@ -10,6 +11,7 @@ const { onlyoffice, public: publicConfig, mimeTypes } = require('../config/index const { normalizeRelativePath } = require('../utils/pathUtils'); const { ensureDir } = require('../utils/fsUtils'); const { resolvePathWithAccess } = require('../services/accessManager'); +const versions = require('../services/versions/operations'); const logger = require('../utils/logger'); const asyncHandler = require('../utils/asyncHandler'); const { ValidationError, UnauthorizedError, ForbiddenError } = require('../errors/AppError'); @@ -33,6 +35,40 @@ const getDocumentType = (ext) => { const resolveMime = (ext) => mimeTypes[ext] || 'application/octet-stream'; +/** + * Pull the saved document into a file of its own, next to the document. + * + * Never into the document itself: it used to be truncated to nothing before + * the Document Server had answered, so a slow network, a restart or a refused + * download left an empty file where the work had been. The versions place the + * temporary file over the document once it is whole. + */ +const fetchDocumentInto = async (downloadUrl, temporaryPath, mode) => { + const response = await axios.get(downloadUrl, { responseType: 'stream', timeout: 30000 }); + await pipeline(response.data, fs.createWriteStream(temporaryPath, { flags: 'wx', mode })); + // The write stream's mode is subject to the umask; this is not. + await fsp.chmod(temporaryPath, mode); +}; + +/** + * Who a callback is saving for. + * + * The Document Server names the people whose changes are in this save; the + * last of them is the one to credit. It says nothing when a save carries no + * history — the first one of a session — and then the account the editing + * session was opened for is the answer. Somebody who came through a share link + * is credited as the link: there is no account to name. + */ +const authorFromCallback = (body, backendCtx) => { + const changes = Array.isArray(body?.history?.changes) ? body.history.changes : []; + const user = changes.length ? changes[changes.length - 1]?.user : null; + const id = user?.id ? String(user.id) : backendCtx?.userId || null; + if ((id && id.startsWith('guest_')) || (!id && backendCtx?.guestSessionId)) { + return { id: null, label: 'share-link' }; + } + return { id, label: user?.name ? String(user.name) : null }; +}; + const getDsJwtFromReq = (req) => { const auth = (req.headers['authorization'] || req.headers['authorizationjwt'] || '').toString(); if (auth.toLowerCase().startsWith('bearer ')) { @@ -308,16 +344,41 @@ router.post( abs = resolved.absolutePath; } await ensureDir(path.dirname(abs)); - // Download updated file from Document Server - const response = await axios.get(body.url, { responseType: 'stream' }); - await fsp.writeFile(abs, Buffer.from([])); // ensure file exists / truncate - const writeStream = fs.createWriteStream(abs); - await new Promise((resolve, reject) => { - response.data.pipe(writeStream); - writeStream.on('finish', resolve); - writeStream.on('error', reject); - }); - logger.debug({ path: relativePath }, 'ONLYOFFICE file updated'); + + // Keep the permissions the document already had; one the editor is + // creating starts private. + let mode = 0o600; + try { + const previous = await fsp.stat(abs); + if (previous.isFile()) mode = previous.mode & 0o777; + } catch { + // A document that is not there yet has nothing to keep. + } + + // A save on purpose — the editor's own Save, or the last one made once + // everybody has left the document — is a state worth keeping. The + // automatic saves in between are not, beyond the checkpoint the + // versions take of a session that runs long. + const explicit = status === 2 || Number(body.forcesavetype) === 1; + + await versions.saveFile( + abs, + (temporaryPath) => fetchDocumentInto(body.url, temporaryPath, mode), + { + purpose: 'onlyoffice', + author: authorFromCallback(body, backendCtx), + source: 'onlyoffice', + session: { + // Everybody editing together shares the document key: it is the + // session, and its saves belong to it rather than each standing + // as a state of its own. + key: typeof body.key === 'string' && body.key ? body.key : null, + startedAt: Number.isFinite(backendCtx?.iat) ? backendCtx.iat * 1000 : null, + }, + explicit, + } + ); + logger.debug({ path: relativePath, status }, 'ONLYOFFICE file updated'); // MUST return {error:0} according to ONLYOFFICE spec return res.json({ error: 0 }); } diff --git a/backend/tests/routes/onlyoffice-save.test.js b/backend/tests/routes/onlyoffice-save.test.js new file mode 100644 index 00000000..f1534790 --- /dev/null +++ b/backend/tests/routes/onlyoffice-save.test.js @@ -0,0 +1,231 @@ +import fs from 'node:fs/promises'; +import http from 'node:http'; +import path from 'node:path'; +import express from 'express'; +import jwt from 'jsonwebtoken'; +import request from 'supertest'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import { modulePath, setupTestEnv } from '../helpers/env-test-utils.js'; + +/** + * A save made through ONLYOFFICE. + * + * The Document Server does not send the document: it sends a URL to fetch it + * from, and the answer to that is what replaces the file. The file used to be + * emptied before the fetch began, so a slow network, a restart or a refused + * download left nothing at all where the work had been — and the state the save + * replaced was gone either way, since nothing kept it. + * + * Both are the same change: the document is fetched into a file of its own and + * handed to the versions, which keep what it replaces and put it in place only + * once it is whole. A document nobody could save is therefore still the + * document, and the history the engine has been keeping since it arrived is + * finally written by somebody. + */ + +const SECRET = 'onlyoffice-save-secret'; +const DOCUMENT = 'Projects/report.docx'; + +let env; +let app; +let alice; +let documentServer; +let serverUrl; +/** What the fake Document Server hands out, and how badly it fails to. */ +let saved; + +const load = (relative) => require(modulePath(relative)); + +const absolute = () => path.join(env.volumeDir, ...DOCUMENT.split('/')); + +beforeEach(async () => { + saved = { content: 'second', refuse: false, dieHalfway: false }; + + documentServer = http.createServer((req, res) => { + if (saved.refuse) { + res.statusCode = 500; + res.end('no'); + return; + } + res.setHeader('Content-Type', 'application/octet-stream'); + if (saved.dieHalfway) { + // Answered, then gone: the network drops, the Document Server restarts. + // This is the shape that used to destroy the document, since the file was + // already empty by the time the stream stopped arriving. + res.setHeader('Content-Length', String(saved.content.length + 100)); + res.write(saved.content.slice(0, 3)); + setTimeout(() => res.destroy(), 20); + return; + } + res.end(saved.content); + }); + await new Promise((resolve) => documentServer.listen(0, '127.0.0.1', resolve)); + serverUrl = `http://127.0.0.1:${documentServer.address().port}`; + + env = await setupTestEnv({ + tag: 'onlyoffice-save-', + env: { + PUBLIC_URL: 'https://files.example.com', + ONLYOFFICE_URL: serverUrl, + ONLYOFFICE_SECRET: SECRET, + }, + }); + + alice = await load('src/services/users').createLocalUser({ + email: 'alice@example.com', + username: 'alice', + displayName: 'Alice', + password: 'secret123', + roles: ['user'], + }); + + await fs.mkdir(path.dirname(absolute()), { recursive: true }); + await fs.writeFile(absolute(), 'first'); + + app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.user = alice; + next(); + }); + app.use('/api', load('src/routes/onlyoffice')); + app.use(load('src/middleware/errorHandler').errorHandler); +}); + +afterEach(async () => { + load('src/services/trash/maintenance').stop(); + await new Promise((resolve) => documentServer.close(resolve)); + await env.cleanup(); +}); + +/** As the Document Server calls back: its own token, and the document to fetch. */ +const callback = (body) => + request(app) + .post('/api/onlyoffice/callback') + .query({ path: DOCUMENT }) + .set('Authorization', `Bearer ${jwt.sign({ any: true }, SECRET)}`) + .send({ url: `${serverUrl}/saved.docx`, key: 'session-key-1', ...body }); + +/** The versions kept for the only document these tests touch. */ +const versionsKept = async () => { + const db = await load('src/services/db').getDb(); + const store = load('src/services/versions/store'); + const files = store.listFiles(db); + if (!files.length) return []; + return store.listVersionsOfFile(db, files[0].id); +}; + +/** What the history says about the content the document holds now. */ +const currentState = async () => { + const db = await load('src/services/db').getDb(); + const store = load('src/services/versions/store'); + return store.listFiles(db)[0] || null; +}; + +const contentOf = async (version) => { + const operations = load('src/services/versions/operations'); + const located = await operations.locateVersion(version.id); + return fs.readFile(located.absolutePath, 'utf8'); +}; + +describe('the document the Document Server hands back', () => { + it('replaces the file, and what it replaced is kept', async () => { + const response = await callback({ status: 2 }); + + expect(response.status).toBe(200); + expect(response.body).toEqual({ error: 0 }); + expect(await fs.readFile(absolute(), 'utf8')).toBe('second'); + + const versions = await versionsKept(); + expect(versions).toHaveLength(1); + expect(await contentOf(versions[0])).toBe('first'); + // Written before the application ever saw the file, so nobody is named: + // what the save itself is credited with is on the state it wrote. + expect(versions[0].source).toBe('external'); + }); + + it('leaves the document alone when the server refuses to hand it over', async () => { + saved.refuse = true; + + const response = await callback({ status: 2 }); + + expect(response.status).toBe(200); + expect(response.body).toEqual({ error: 1 }); + expect(await fs.readFile(absolute(), 'utf8')).toBe('first'); + expect(await versionsKept()).toHaveLength(0); + }); + + /** + * The failure the old order could not survive. The document was emptied + * before the download began, so a stream that stopped arriving — a dropped + * network, a Document Server restarting mid-answer — left an empty file where + * the work had been. Nothing is written until the whole document is here. + */ + it('leaves the document alone when the download dies halfway', async () => { + saved.dieHalfway = true; + + const response = await callback({ status: 2 }); + + expect(response.body).toEqual({ error: 1 }); + expect(await fs.readFile(absolute(), 'utf8')).toBe('first'); + expect(await versionsKept()).toHaveLength(0); + }); + + it('credits the person whose changes it carries', async () => { + await callback({ + status: 2, + history: { changes: [{ user: { id: alice.id, name: 'Alice' } }] }, + }); + + const state = await currentState(); + expect(state.currentSource).toBe('onlyoffice'); + expect(state.currentAuthorId).toBe(alice.id); + expect(state.currentAuthorLabel).toBe('Alice'); + + // And once a second save replaces it, the state she wrote is kept as hers. + saved.content = 'third'; + await callback({ status: 2, key: 'session-key-2' }); + + const versions = await versionsKept(); + const hers = versions.find((version) => version.authorId === alice.id); + expect(hers, 'the save Alice made was not kept as hers').toBeTruthy(); + expect(hers.source).toBe('onlyoffice'); + expect(await contentOf(hers)).toBe('second'); + }); + + /** + * A save from a share link has no account behind it. It used to be credited + * to the guest session's own identifier, which names nobody once the session + * is over. + */ + it('credits a save made through a share link to the link', async () => { + await callback({ + status: 2, + history: { changes: [{ user: { id: 'guest_abc', name: 'Guest User' } }] }, + }); + + const state = await currentState(); + expect(state.currentAuthorId).toBeNull(); + expect(state.currentAuthorLabel).toBe('share-link'); + }); + + /** Force save (6) is the same save as far as the file is concerned. */ + it('saves a forced save too', async () => { + saved.content = 'forced'; + + const response = await callback({ status: 6, forcesavetype: 1 }); + + expect(response.body).toEqual({ error: 0 }); + expect(await fs.readFile(absolute(), 'utf8')).toBe('forced'); + expect(await versionsKept()).toHaveLength(1); + }); + + it('acknowledges a status that saves nothing, and writes nothing', async () => { + const response = await callback({ status: 1 }); + + expect(response.body).toEqual({ error: 0 }); + expect(await fs.readFile(absolute(), 'utf8')).toBe('first'); + expect(await versionsKept()).toHaveLength(0); + }); +}); From a2454f237712d44907a3d41597215ffbd59f4e9f Mon Sep 17 00:00:00 2001 From: Benjy Date: Fri, 25 Sep 2026 09:27:26 +0200 Subject: [PATCH 2/3] Fetch a saved document only from the Document Server it was sent to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The callback says where the saved document is, and the server fetched whatever it was told to. The callback is answered without a session — it is how a separate service reaches us — so anybody able to call it could name any address the server can reach: one on the machine itself, one inside the container's network, a metadata service. The answer was then written into a file in the volume, where it could be read back. The URL now has to come from the Document Server the document was handed to. `ONLYOFFICE_DOWNLOAD_ORIGINS` declares the others it may report itself under, which happens behind a proxy or inside a container network; the configured address is always allowed, and a refusal says in the log which origin was turned away and which are allowed, since that is the only way to tell this apart from a Document Server that cannot be reached. The test answers perfectly well from a server that is not the Document Server, so a refusal cannot be mistaken for a network failure: without the check the document is replaced by what that server handed over. --- backend/src/config/env.js | 1 + backend/src/config/index.js | 8 ++- backend/src/routes/onlyoffice.js | 53 +++++++++++++++++++- backend/tests/routes/onlyoffice-save.test.js | 45 +++++++++++++++++ docs/configuration/environment.md | 1 + 5 files changed, 105 insertions(+), 3 deletions(-) diff --git a/backend/src/config/env.js b/backend/src/config/env.js index 3496f354..d8385610 100644 --- a/backend/src/config/env.js +++ b/backend/src/config/env.js @@ -101,6 +101,7 @@ module.exports = { ONLYOFFICE_LANG: process.env.ONLYOFFICE_LANG?.trim() || 'en', ONLYOFFICE_FORCE_SAVE: normalizeBoolean(process.env.ONLYOFFICE_FORCE_SAVE) || false, ONLYOFFICE_FILE_EXTENSIONS: process.env.ONLYOFFICE_FILE_EXTENSIONS || '', + ONLYOFFICE_DOWNLOAD_ORIGINS: process.env.ONLYOFFICE_DOWNLOAD_ORIGINS || '', // Collabora (WOPI) COLLABORA_URL: process.env.COLLABORA_URL?.trim() || null, diff --git a/backend/src/config/index.js b/backend/src/config/index.js index e06df8fa..82463ca0 100644 --- a/backend/src/config/index.js +++ b/backend/src/config/index.js @@ -164,7 +164,7 @@ if (env.PUBLIC_URL) { // They are considered valid so accessing the app that way doesn't raise the // public-URL mismatch warning, and they're accepted by CORS. PUBLIC_URL remains // the canonical URL used to build absolute links (shares, OIDC callbacks, WOPI). -const parseOriginList = (value) => +const parseOriginList = (value, variableName = 'INTERNAL_URL') => (typeof value === 'string' ? value.split(',') : []) .map((entry) => entry.trim()) .filter(Boolean) @@ -172,7 +172,7 @@ const parseOriginList = (value) => try { return new URL(entry).origin; } catch (err) { - console.warn(`[Config] Invalid INTERNAL_URL entry: ${entry}`); + console.warn(`[Config] Invalid ${variableName} entry: ${entry}`); return null; } }) @@ -323,6 +323,10 @@ const onlyoffice = { extensions: env.ONLYOFFICE_FILE_EXTENSIONS.split(',') .map((s) => s.trim().toLowerCase()) .filter(Boolean), + // Where a saved document may be fetched from, beyond the Document Server's + // own address: it sometimes reports itself under another host than the one it + // is called on, behind a proxy or inside a container network. + downloadOrigins: parseOriginList(env.ONLYOFFICE_DOWNLOAD_ORIGINS, 'ONLYOFFICE_DOWNLOAD_ORIGINS'), }; // Silent JWT mismatches surface to the user as "Document security token is not diff --git a/backend/src/routes/onlyoffice.js b/backend/src/routes/onlyoffice.js index e8654af5..8c88b9dc 100644 --- a/backend/src/routes/onlyoffice.js +++ b/backend/src/routes/onlyoffice.js @@ -35,6 +35,55 @@ const getDocumentType = (ext) => { const resolveMime = (ext) => mimeTypes[ext] || 'application/octet-stream'; +/** + * The addresses a saved document may be fetched from. + * + * The Document Server's own, and any declared beside it: behind a proxy or + * inside a container network it sometimes reports itself under a host other + * than the one it is called on. + */ +const buildAllowedDownloadOrigins = () => { + const origins = new Set(); + const add = (value) => { + if (!value) return; + try { + origins.add(new URL(value).origin); + } catch { + // Ignore malformed configuration entries. + } + }; + add(onlyoffice.serverUrl); + (onlyoffice.downloadOrigins || []).forEach(add); + return origins; +}; + +/** + * The callback says where to fetch the saved document from, and the server + * fetched whatever it was told to: an address on the machine itself, or inside + * the network the container sits in, reached by anyone who can reach the + * callback. It has to come from the Document Server we sent the document to. + */ +const ensureAllowedDownloadUrl = (rawUrl) => { + let parsed; + try { + parsed = new URL(String(rawUrl)); + } catch { + throw new ValidationError('The document URL is not a valid URL.'); + } + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { + throw new ValidationError('The document URL must use HTTP or HTTPS.'); + } + const allowed = buildAllowedDownloadOrigins(); + if (!allowed.has(parsed.origin)) { + logger.warn( + { origin: parsed.origin, allowed: Array.from(allowed) }, + 'ONLYOFFICE callback rejected: document URL origin is not allowed. Add it to ONLYOFFICE_DOWNLOAD_ORIGINS if the Document Server reports a different host.' + ); + throw new ForbiddenError('The document URL does not come from the configured Document Server.'); + } + return parsed.toString(); +}; + /** * Pull the saved document into a file of its own, next to the document. * @@ -330,6 +379,8 @@ router.post( const status = Number(body.status); // See ONLYOFFICE callback statuses: 2 - Save, 6 - Force Save if ((status === 2 || status === 6) && body.url) { + // Only the Document Server we handed the document to may be fetched from. + const downloadUrl = ensureAllowedDownloadUrl(body.url); let abs = null; if (backendCtx && typeof backendCtx.absolutePath === 'string' && backendCtx.absolutePath) { abs = backendCtx.absolutePath; @@ -363,7 +414,7 @@ router.post( await versions.saveFile( abs, - (temporaryPath) => fetchDocumentInto(body.url, temporaryPath, mode), + (temporaryPath) => fetchDocumentInto(downloadUrl, temporaryPath, mode), { purpose: 'onlyoffice', author: authorFromCallback(body, backendCtx), diff --git a/backend/tests/routes/onlyoffice-save.test.js b/backend/tests/routes/onlyoffice-save.test.js index f1534790..7ca9b6a7 100644 --- a/backend/tests/routes/onlyoffice-save.test.js +++ b/backend/tests/routes/onlyoffice-save.test.js @@ -32,6 +32,12 @@ let app; let alice; let documentServer; let serverUrl; +/** The same server under another address, as one behind a proxy reports itself. */ +let mirror; +let mirrorUrl; +/** Somewhere else entirely, answering perfectly well: what must not be fetched. */ +let stranger; +let strangerUrl; /** What the fake Document Server hands out, and how badly it fails to. */ let saved; @@ -63,12 +69,27 @@ beforeEach(async () => { await new Promise((resolve) => documentServer.listen(0, '127.0.0.1', resolve)); serverUrl = `http://127.0.0.1:${documentServer.address().port}`; + stranger = http.createServer((req, res) => { + res.setHeader('Content-Type', 'application/octet-stream'); + res.end('stolen'); + }); + await new Promise((resolve) => stranger.listen(0, '127.0.0.1', resolve)); + strangerUrl = `http://127.0.0.1:${stranger.address().port}`; + + mirror = http.createServer((req, res) => { + res.setHeader('Content-Type', 'application/octet-stream'); + res.end(saved.content); + }); + await new Promise((resolve) => mirror.listen(0, '127.0.0.1', resolve)); + mirrorUrl = `http://127.0.0.1:${mirror.address().port}`; + env = await setupTestEnv({ tag: 'onlyoffice-save-', env: { PUBLIC_URL: 'https://files.example.com', ONLYOFFICE_URL: serverUrl, ONLYOFFICE_SECRET: SECRET, + ONLYOFFICE_DOWNLOAD_ORIGINS: mirrorUrl, }, }); @@ -96,6 +117,8 @@ beforeEach(async () => { afterEach(async () => { load('src/services/trash/maintenance').stop(); await new Promise((resolve) => documentServer.close(resolve)); + await new Promise((resolve) => mirror.close(resolve)); + await new Promise((resolve) => stranger.close(resolve)); await env.cleanup(); }); @@ -221,6 +244,28 @@ describe('the document the Document Server hands back', () => { expect(await versionsKept()).toHaveLength(1); }); + /** + * The callback says where the document is, and the server used to fetch + * whatever it was told to — an address on the machine itself, or one inside + * the container's network, reached by anybody who can reach the callback. + */ + it('fetches nothing from a server that is not the Document Server', async () => { + // Answering, and answering well: a refusal here cannot be the network. + const response = await callback({ status: 2, url: `${strangerUrl}/saved.docx` }); + + expect(response.body).toEqual({ error: 1 }); + expect(await fs.readFile(absolute(), 'utf8')).toBe('first'); + expect(await versionsKept()).toHaveLength(0); + }); + + /** A Document Server behind a proxy reports itself under the declared host. */ + it('fetches from an address declared beside the Document Server', async () => { + const response = await callback({ status: 2, url: `${mirrorUrl}/saved.docx` }); + + expect(response.body).toEqual({ error: 0 }); + expect(await fs.readFile(absolute(), 'utf8')).toBe('second'); + }); + it('acknowledges a status that saves nothing, and writes nothing', async () => { const response = await callback({ status: 1 }); diff --git a/docs/configuration/environment.md b/docs/configuration/environment.md index 439d117e..9c06de09 100644 --- a/docs/configuration/environment.md +++ b/docs/configuration/environment.md @@ -143,6 +143,7 @@ The sharing system (toolbar **Share** button, guest links such as `/share/:token | ----------------------------- | ------------------ | --------------------------------------------------------------------------------------------------------------------------------------- | | `ONLYOFFICE_URL` | _none_ | Public URL for Document Server (must reach your app's `PUBLIC_URL`). | | `ONLYOFFICE_SECRET` | _none_ | JWT secret shared with OnlyOffice Document Server for `/api/onlyoffice` calls. | +| `ONLYOFFICE_DOWNLOAD_ORIGINS` | _none_ | Comma-separated extra origins a saved document may be fetched from. Set it when the Document Server reports itself under another host than `ONLYOFFICE_URL`; that one is always allowed. | | `ONLYOFFICE_LANG` | `en` | Language code for the editor UI. | | `ONLYOFFICE_FORCE_SAVE` | `false` | When true, OnlyOffice forces users to save via the editor UI. | | `ONLYOFFICE_FILE_EXTENSIONS` | _default list_ | Extra file extensions to surface to the Document Server. | From 8bf847c2b66cf8d09b1070a2fa735665325c7193 Mon Sep 17 00:00:00 2001 From: Benjy Date: Fri, 25 Sep 2026 09:29:11 +0200 Subject: [PATCH 3/3] Keep what a save through Collabora replaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collabora hands the document over itself, so nothing here could lose a file the way an unfinished download could. What was missing is the other half: the state each save replaced was dropped, so a document edited all afternoon in Collabora had no history at all, while the same document saved from anywhere else now does. The save goes through the versions. Who saved it is the account the editing session was opened for, and the editor is recorded beside it, so a history shows where each state came from. Collabora saves on its own every few minutes. Those saves belong to the session rather than standing as states of their own, and the session is the lock every co-editor of the document holds — one per open document, which is exactly what a session is. A save somebody asked for, or the one made on closing, is marked as such and is kept even when the timer saves over it a minute later. Five tests, all failing on the plain rename: what a save replaced is gone, and nothing says who saved or how. --- backend/src/routes/collabora.js | 46 ++++-- backend/tests/routes/collabora-save.test.js | 162 ++++++++++++++++++++ 2 files changed, 196 insertions(+), 12 deletions(-) create mode 100644 backend/tests/routes/collabora-save.test.js diff --git a/backend/src/routes/collabora.js b/backend/src/routes/collabora.js index f806803e..28ec3e7e 100644 --- a/backend/src/routes/collabora.js +++ b/backend/src/routes/collabora.js @@ -13,8 +13,15 @@ const asyncHandler = require('../utils/asyncHandler'); const logger = require('../utils/logger'); const { getDiscoveryActionsByExt } = require('../services/collaboraDiscoveryService'); const lockService = require('../services/wopiLockService'); +const versions = require('../services/versions/operations'); const { ValidationError, UnauthorizedError, ForbiddenError } = require('../errors/AppError'); +/** A Collabora save header, under its current name or the one older servers still send. */ +const wopiSaveHeader = (req, name) => + String(req.headers[`x-cool-wopi-${name}`] ?? req.headers[`x-lool-wopi-${name}`] ?? '') + .trim() + .toLowerCase(); + const router = express.Router(); const toExt = (filename = '') => { @@ -282,18 +289,33 @@ router.post( return res.status(409).end(); } - const dir = path.dirname(abs); - const tmp = path.join(dir, `.${path.basename(abs)}.wopi-tmp-${process.pid}-${Date.now()}`); - - const writeStream = fs.createWriteStream(tmp); - await new Promise((resolve, reject) => { - req.pipe(writeStream); - writeStream.on('finish', resolve); - writeStream.on('error', reject); - req.on('error', reject); - }); - - await fsp.rename(tmp, abs); + await versions.saveFile( + abs, + (temporaryPath) => + new Promise((resolve, reject) => { + const writeStream = fs.createWriteStream(temporaryPath, { flags: 'wx' }); + req.pipe(writeStream); + writeStream.on('finish', resolve); + writeStream.on('error', reject); + req.on('error', reject); + }), + { + purpose: 'wopi', + author: { id: tokenPayload.userId || null, label: tokenPayload.userName || null }, + source: 'collabora', + session: { + // One lock per open document, held by everyone editing it together: + // the session its saves belong to. Without one every save would stand + // as a state of its own. + key: requestLock ? `wopi:${requestLock}` : null, + startedAt: Number.isFinite(tokenPayload.iat) ? tokenPayload.iat * 1000 : null, + }, + // Collabora saves on its own every few minutes; the ones somebody asked + // for, and the one made on closing the document, are states worth + // keeping. + explicit: wopiSaveHeader(req, 'isautosave') !== 'true', + } + ); const stat = await fsp.stat(abs); res.setHeader('Cache-Control', 'no-store'); diff --git a/backend/tests/routes/collabora-save.test.js b/backend/tests/routes/collabora-save.test.js new file mode 100644 index 00000000..65530917 --- /dev/null +++ b/backend/tests/routes/collabora-save.test.js @@ -0,0 +1,162 @@ +import fs from 'node:fs/promises'; +import path from 'node:path'; +import express from 'express'; +import jwt from 'jsonwebtoken'; +import request from 'supertest'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import { modulePath, setupTestEnv } from '../helpers/env-test-utils.js'; + +/** + * A save made through Collabora. + * + * Collabora hands the document over itself, so nothing here could destroy a + * file the way a download that stopped arriving could. What was missing is the + * other half: the state each save replaced was dropped, so a document edited + * all afternoon had no history at all — while the same document opened in the + * text editor did. + * + * The saves now go through the versions, which keep what they replace. The + * editor saves on its own every few minutes, and those automatic saves belong + * to the session rather than standing as states of their own: the lock every + * co-editor holds is what says which session a save belongs to. + */ + +const SECRET = 'collabora-save-secret'; +const DOCUMENT = 'Projects/report.docx'; +const FILE_ID = 'file-1'; + +let env; +let app; +let alice; + +const load = (relative) => require(modulePath(relative)); + +const absolute = () => path.join(env.volumeDir, ...DOCUMENT.split('/')); + +beforeEach(async () => { + env = await setupTestEnv({ + tag: 'collabora-save-', + env: { + PUBLIC_URL: 'https://files.example.com', + COLLABORA_URL: 'https://collabora.example.com', + COLLABORA_SECRET: SECRET, + }, + }); + + alice = await load('src/services/users').createLocalUser({ + email: 'alice@example.com', + username: 'alice', + displayName: 'Alice', + password: 'secret123', + roles: ['user'], + }); + + await fs.mkdir(path.dirname(absolute()), { recursive: true }); + await fs.writeFile(absolute(), 'first'); + + app = express(); + app.use('/api', load('src/routes/collabora')); + app.use(load('src/middleware/errorHandler').errorHandler); +}); + +afterEach(async () => { + load('src/services/trash/maintenance').stop(); + await env.cleanup(); +}); + +const accessToken = () => + jwt.sign( + { + fileId: FILE_ID, + absolutePath: absolute(), + canWrite: true, + userId: alice.id, + userName: 'Alice', + }, + SECRET, + { algorithm: 'HS256', expiresIn: 60 } + ); + +/** As Collabora saves: the whole document in the body, and how it came to save. */ +const put = (content, { autosave = false, lock = null } = {}) => { + const call = request(app) + .post(`/api/collabora/wopi/files/${FILE_ID}/contents`) + .query({ access_token: accessToken() }) + .set('Content-Type', 'application/octet-stream'); + if (autosave) call.set('X-COOL-WOPI-IsAutosave', 'true'); + if (lock) call.set('X-WOPI-Lock', lock); + return call.send(Buffer.from(content)); +}; + +const historyOf = async () => { + const db = await load('src/services/db').getDb(); + const store = load('src/services/versions/store'); + const file = store.listFiles(db)[0] || null; + return { + file, + versions: file ? store.listVersionsOfFile(db, file.id) : [], + }; +}; + +const contentOf = async (version) => { + const operations = load('src/services/versions/operations'); + const located = await operations.locateVersion(version.id); + return fs.readFile(located.absolutePath, 'utf8'); +}; + +describe('a document saved from Collabora', () => { + it('is written, and what it replaced is kept', async () => { + const response = await put('second'); + + expect(response.status).toBe(200); + expect(await fs.readFile(absolute(), 'utf8')).toBe('second'); + + const { versions } = await historyOf(); + expect(versions).toHaveLength(1); + expect(await contentOf(versions[0])).toBe('first'); + }); + + it('records who saved it, and through which editor', async () => { + await put('second'); + + const { file } = await historyOf(); + expect(file.currentSource).toBe('collabora'); + expect(file.currentAuthorId).toBe(alice.id); + expect(file.currentAuthorLabel).toBe('Alice'); + }); + + /** + * The editor's own timer, rather than somebody asking: a state on the way to + * the next one. What keeps it from filling the history is the session, and + * the session is the lock every co-editor of the document holds. + */ + it('marks a save the editor made by itself, under the session that made it', async () => { + await put('second', { autosave: true, lock: 'lock-abc' }); + + const { file } = await historyOf(); + expect(file.currentExplicit).toBe(false); + expect(file.currentSession).toBe('wopi:lock-abc'); + }); + + it('marks a save somebody asked for as one', async () => { + await put('second', { lock: 'lock-abc' }); + + const { file } = await historyOf(); + expect(file.currentExplicit).toBe(true); + }); + + /** + * Saved on purpose, then saved again by the timer: the state somebody chose + * is kept, rather than being swallowed by the automatic save that follows it. + */ + it('keeps the state somebody asked for when the timer saves over it', async () => { + await put('asked for', { lock: 'lock-abc' }); + await put('by the timer', { autosave: true, lock: 'lock-abc' }); + + const { versions } = await historyOf(); + const contents = await Promise.all(versions.map(contentOf)); + expect(contents).toContain('asked for'); + expect(await fs.readFile(absolute(), 'utf8')).toBe('by the timer'); + }); +});