diff --git a/.github/workflows/backend-tests.yml b/.github/workflows/backend-tests.yml index c16b79a5e..485ec0cc1 100644 --- a/.github/workflows/backend-tests.yml +++ b/.github/workflows/backend-tests.yml @@ -18,7 +18,7 @@ jobs: strategy: fail-fast: false matrix: - node: [14, 16, 18] + node: [16, 18, 20] steps: - name: Checkout repository @@ -55,7 +55,7 @@ jobs: strategy: fail-fast: false matrix: - node: [14, 16, 18] + node: [16, 18, 20] steps: - name: Checkout repository @@ -124,7 +124,7 @@ jobs: - uses: actions/setup-node@v3 with: - node-version: 14 + node-version: 20 cache: 'npm' cache-dependency-path: | src/package-lock.json @@ -157,7 +157,7 @@ jobs: - uses: actions/setup-node@v3 with: - node-version: 14 + node-version: 20 cache: 'npm' cache-dependency-path: | src/package-lock.json diff --git a/.github/workflows/frontend-admin-tests.yml b/.github/workflows/frontend-admin-tests.yml index c10cc0d5a..57336cc76 100644 --- a/.github/workflows/frontend-admin-tests.yml +++ b/.github/workflows/frontend-admin-tests.yml @@ -14,7 +14,7 @@ jobs: strategy: fail-fast: false matrix: - node: [14, 16, 18] + node: [16, 18, 20] steps: - diff --git a/.github/workflows/frontend-tests.yml b/.github/workflows/frontend-tests.yml index 89dd72866..653ee9d60 100644 --- a/.github/workflows/frontend-tests.yml +++ b/.github/workflows/frontend-tests.yml @@ -34,7 +34,7 @@ jobs: - uses: actions/setup-node@v3 with: - node-version: 14 + node-version: 20 cache: 'npm' cache-dependency-path: | src/package-lock.json @@ -98,7 +98,7 @@ jobs: - uses: actions/setup-node@v3 with: - node-version: 14 + node-version: 20 cache: 'npm' cache-dependency-path: | src/package-lock.json @@ -126,7 +126,7 @@ jobs: # Etherpad core dependencies must be installed after installing the # plugin's dependencies, otherwise npm will try to hoist common # dependencies by removing them from src/node_modules and installing them - # in the top-level node_modules. As of v6.14.10, npm's hoist logic appears + # in the top-level node_modules. As of v6.20.10, npm's hoist logic appears # to be buggy, because it sometimes removes dependencies from # src/node_modules but fails to add them to the top-level node_modules. # Even if npm correctly hoists the dependencies, the hoisting seems to diff --git a/.github/workflows/lint-package-lock.yml b/.github/workflows/lint-package-lock.yml index bc05a1a52..e3401a94f 100644 --- a/.github/workflows/lint-package-lock.yml +++ b/.github/workflows/lint-package-lock.yml @@ -22,7 +22,7 @@ jobs: - uses: actions/setup-node@v3 with: - node-version: 14 + node-version: 20 cache: 'npm' cache-dependency-path: | src/package-lock.json diff --git a/.github/workflows/load-test.yml b/.github/workflows/load-test.yml index 9d8c4fd9b..6eb3afad7 100644 --- a/.github/workflows/load-test.yml +++ b/.github/workflows/load-test.yml @@ -22,7 +22,7 @@ jobs: - uses: actions/setup-node@v3 with: - node-version: 14 + node-version: 20 cache: 'npm' cache-dependency-path: | src/package-lock.json @@ -52,7 +52,7 @@ jobs: - uses: actions/setup-node@v3 with: - node-version: 14 + node-version: 20 cache: 'npm' cache-dependency-path: | src/package-lock.json @@ -109,7 +109,7 @@ jobs: - uses: actions/setup-node@v3 with: - node-version: 14 + node-version: 20 cache: 'npm' cache-dependency-path: | src/package-lock.json diff --git a/.github/workflows/rate-limit.yml b/.github/workflows/rate-limit.yml index 7df7aa4ce..005002b08 100644 --- a/.github/workflows/rate-limit.yml +++ b/.github/workflows/rate-limit.yml @@ -22,7 +22,7 @@ jobs: - uses: actions/setup-node@v3 with: - node-version: 14 + node-version: 20 cache: 'npm' cache-dependency-path: | src/package-lock.json diff --git a/.github/workflows/upgrade-from-latest-release.yml b/.github/workflows/upgrade-from-latest-release.yml index a00e9540d..69eb08d8e 100644 --- a/.github/workflows/upgrade-from-latest-release.yml +++ b/.github/workflows/upgrade-from-latest-release.yml @@ -18,7 +18,7 @@ jobs: strategy: fail-fast: false matrix: - node: [14, 16, 18] + node: [16, 18, 20] steps: - name: Check out latest release diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index bcd8532a3..53fe33d07 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -28,7 +28,7 @@ jobs: - uses: actions/setup-node@v3 with: - node-version: 14 + node-version: 20 cache: 'npm' cache-dependency-path: | src/package-lock.json @@ -108,7 +108,7 @@ jobs: - uses: actions/setup-node@v3 with: - node-version: 14 + node-version: 20 cache: 'npm' cache-dependency-path: | etherpad/src/package-lock.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 524899dc0..e82d99ed8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,14 @@ +# Next release + +### Notable enhancements and fixes + +* Security + * Limit requested revisions in timeslider and export to head revision. (affects v1.9.0) + +* Bugfixes + * revisions in `CHANGESET_REQ` (timeslider) and export (txt, html, custom) + are now checked to be numbers. + # 1.9.0 ### Notable enhancements and fixes diff --git a/README.md b/README.md index b798fda60..3af3df511 100644 --- a/README.md +++ b/README.md @@ -46,7 +46,7 @@ We're looking for maintainers and have some funding available. Please contact J ### Requirements -[Node.js](https://nodejs.org/) >= **14.0.0**. +[Node.js](https://nodejs.org/) >= **16.20.1**. ### GNU/Linux and other UNIX-like systems diff --git a/src/node/db/API.js b/src/node/db/API.js index 9b2ecadc7..3f92c47dd 100644 --- a/src/node/db/API.js +++ b/src/node/db/API.js @@ -33,6 +33,7 @@ const exportTxt = require('../utils/ExportTxt'); const importHtml = require('../utils/ImportHtml'); const cleanText = require('./Pad').cleanText; const PadDiff = require('../utils/padDiff'); +const { checkValidRev, isInt } = require('../utils/checkValidRev'); /* ******************** * GROUP FUNCTIONS **** @@ -777,6 +778,13 @@ exports.createDiffHTML = async (padID, startRev, endRev) => { // get the pad const pad = await getPadSafe(padID, true); + const headRev = pad.getHeadRevisionNumber(); + if (startRev > headRev) + startRev = headRev; + + if (endRev > headRev) + endRev = headRev; + let padDiff; try { padDiff = new PadDiff(pad, startRev, endRev); @@ -822,9 +830,6 @@ exports.getStats = async () => { ** INTERNAL HELPER FUNCTIONS * **************************** */ -// checks if a number is an int -const isInt = (value) => (parseFloat(value) === parseInt(value, 10)) && !isNaN(value); - // gets a pad safe const getPadSafe = async (padID, shouldExist, text, authorId = '') => { // check if padID is a string @@ -854,31 +859,6 @@ const getPadSafe = async (padID, shouldExist, text, authorId = '') => { return padManager.getPad(padID, text, authorId); }; -// checks if a rev is a legal number -// pre-condition is that `rev` is not undefined -const checkValidRev = (rev) => { - if (typeof rev !== 'number') { - rev = parseInt(rev, 10); - } - - // check if rev is a number - if (isNaN(rev)) { - throw new CustomError('rev is not a number', 'apierror'); - } - - // ensure this is not a negative number - if (rev < 0) { - throw new CustomError('rev is not a negative number', 'apierror'); - } - - // ensure this is not a float value - if (!isInt(rev)) { - throw new CustomError('rev is a float value', 'apierror'); - } - - return rev; -}; - // checks if a padID is part of a group const checkGroupPad = (padID, field) => { // ensure this is a group pad diff --git a/src/node/db/Pad.js b/src/node/db/Pad.js index b692962f1..a9c87541f 100644 --- a/src/node/db/Pad.js +++ b/src/node/db/Pad.js @@ -172,6 +172,9 @@ class Pad { async getInternalRevisionAText(targetRev) { const keyRev = this.getKeyRevisionNumber(targetRev); + const headRev = this.getHeadRevisionNumber(); + if (targetRev > headRev) + targetRev = headRev; const [keyAText, changesets] = await Promise.all([ this._getKeyRevisionAText(keyRev), Promise.all( diff --git a/src/node/handler/ExportHandler.js b/src/node/handler/ExportHandler.js index f3fde047c..417380866 100644 --- a/src/node/handler/ExportHandler.js +++ b/src/node/handler/ExportHandler.js @@ -29,6 +29,7 @@ const os = require('os'); const hooks = require('../../static/js/pluginfw/hooks'); const TidyHtml = require('../utils/TidyHtml'); const util = require('util'); +const { checkValidRev } = require('../utils/checkValidRev'); const fsp_writeFile = util.promisify(fs.writeFile); const fsp_unlink = util.promisify(fs.unlink); @@ -53,6 +54,12 @@ exports.doExport = async (req, res, padId, readOnlyId, type) => { // tell the browser that this is a downloadable file res.attachment(`${fileName}.${type}`); + if (req.params.rev !== undefined) { + // ensure revision is a number + // modify req, as we use it in a later call to exportConvert + req.params.rev = checkValidRev(req.params.rev); + } + // if this is a plain text export, we can do this directly // We have to over engineer this because tabs are stored as attributes and not plain text if (type === 'etherpad') { diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 9a1885b73..35c76b5d9 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -39,6 +39,7 @@ const stats = require('../stats'); const assert = require('assert').strict; const {RateLimiterMemory} = require('rate-limiter-flexible'); const webaccess = require('../hooks/express/webaccess'); +const { checkValidRev } = require('../utils/checkValidRev'); let rateLimiter; let socketio = null; @@ -1076,10 +1077,14 @@ const handleChangesetRequest = async (socket, {data: {granularity, start, reques if (granularity == null) throw new Error('missing granularity'); if (!Number.isInteger(granularity)) throw new Error('granularity is not an integer'); if (start == null) throw new Error('missing start'); + start = checkValidRev(start); if (requestID == null) throw new Error('mising requestID'); const end = start + (100 * granularity); const {padId, author: authorId} = sessioninfos[socket.id]; const pad = await padManager.getPad(padId, null, authorId); + const headRev = pad.getHeadRevisionNumber(); + if (start > headRev) + start = headRev; const data = await getChangesetInfo(pad, start, end, granularity); data.requestID = requestID; socket.json.send({type: 'CHANGESET_REQ', data}); diff --git a/src/node/utils/ExportHelper.js b/src/node/utils/ExportHelper.js index 7962476e8..48054e7f4 100644 --- a/src/node/utils/ExportHelper.js +++ b/src/node/utils/ExportHelper.js @@ -21,10 +21,14 @@ const AttributeMap = require('../../static/js/AttributeMap'); const Changeset = require('../../static/js/Changeset'); +const { checkValidRev } = require('./checkValidRev'); +/* + * This method seems unused in core and no plugins depend on it + */ exports.getPadPlainText = (pad, revNum) => { const _analyzeLine = exports._analyzeLine; - const atext = ((revNum !== undefined) ? pad.getInternalRevisionAText(revNum) : pad.atext); + const atext = ((revNum !== undefined) ? pad.getInternalRevisionAText(checkValidRev(revNum)) : pad.atext); const textLines = atext.text.slice(0, -1).split('\n'); const attribLines = Changeset.splitAttributionLines(atext.attribs, atext.text); const apool = pad.pool; diff --git a/src/node/utils/checkValidRev.js b/src/node/utils/checkValidRev.js new file mode 100644 index 000000000..862c6a2bd --- /dev/null +++ b/src/node/utils/checkValidRev.js @@ -0,0 +1,34 @@ +'use strict'; + +const CustomError = require('../utils/customError'); + +// checks if a rev is a legal number +// pre-condition is that `rev` is not undefined +const checkValidRev = (rev) => { + if (typeof rev !== 'number') { + rev = parseInt(rev, 10); + } + + // check if rev is a number + if (isNaN(rev)) { + throw new CustomError('rev is not a number', 'apierror'); + } + + // ensure this is not a negative number + if (rev < 0) { + throw new CustomError('rev is not a negative number', 'apierror'); + } + + // ensure this is not a float value + if (!isInt(rev)) { + throw new CustomError('rev is a float value', 'apierror'); + } + + return rev; +}; + +// checks if a number is an int +const isInt = (value) => (parseFloat(value) === parseInt(value, 10)) && !isNaN(value); + +exports.isInt = isInt; +exports.checkValidRev = checkValidRev; diff --git a/src/package-lock.json b/src/package-lock.json index 2261637e6..ac278bb93 100644 --- a/src/package-lock.json +++ b/src/package-lock.json @@ -7331,9 +7331,9 @@ } }, "semver": { - "version": "7.5.2", - "resolved": "https://registry.npmjs.org/semver/-/semver-7.5.2.tgz", - "integrity": "sha512-SoftuTROv/cRjCze/scjGyiDtcUyxw1rgYQSZY7XTmtR5hX+dm76iDbTH8TkLPHCQmlbQVSSbNZCPM2hb0knnQ==", + "version": "7.5.3", + "resolved": "https://registry.npmjs.org/semver/-/semver-7.5.3.tgz", + "integrity": "sha512-QBlUtyVk/5EeHbi7X0fw6liDZc7BBmEaSYn01fMU1OUYbf6GPsbTtd8WmnqbI20SeycoHSeiybkE/q1Q+qlThQ==", "requires": { "lru-cache": "^6.0.0" } diff --git a/src/package.json b/src/package.json index c2b0c736f..7349fc6d3 100644 --- a/src/package.json +++ b/src/package.json @@ -61,7 +61,7 @@ "request": "2.88.2", "resolve": "1.22.2", "security": "1.0.0", - "semver": "^7.5.2", + "semver": "^7.5.3", "socket.io": "^2.4.1", "superagent": "^8.0.9", "terser": "^5.18.1", @@ -93,7 +93,7 @@ "typescript": "^4.9.5" }, "engines": { - "node": ">=14.15.0", + "node": ">=16.20.1", "npm": ">=6.14.0" }, "repository": { diff --git a/src/tests/backend/specs/api/importexportGetPost.js b/src/tests/backend/specs/api/importexportGetPost.js index e90f7c71e..e69f0d120 100644 --- a/src/tests/backend/specs/api/importexportGetPost.js +++ b/src/tests/backend/specs/api/importexportGetPost.js @@ -447,6 +447,175 @@ describe(__filename, function () { }); }); + describe('revisions are supported in txt and html export', function () { + const makeGoodExport = () => ({ + 'pad:testing': { + atext: { + text: 'oofoo\n', + attribs: '|1+6', + }, + pool: { + numToAttrib: { + 0: ['author', 'a.foo'], + }, + nextNum: 1, + }, + head: 2, + savedRevisions: [], + }, + 'globalAuthor:a.foo': { + colorId: '#000000', + name: 'author foo', + timestamp: 1598747784631, + padIDs: 'testing', + }, + 'pad:testing:revs:0': { + changeset: 'Z:1>3+3$foo', + meta: { + author: 'a.foo', + timestamp: 1597632398288, + pool: { + nextNum: 1, + numToAttrib: { + 0: ['author', 'a.foo'], + }, + }, + atext: { + text: 'foo\n', + attribs: '|1+4', + }, + }, + }, + 'pad:testing:revs:1': { + changeset: 'Z:4>1+1$o', + meta: { + author: 'a.foo', + timestamp: 1597632398288, + pool: { + nextNum: 1, + numToAttrib: { + 0: ['author', 'a.foo'], + }, + }, + atext: { + text: 'fooo\n', + attribs: '*0|1+5', + }, + }, + }, + 'pad:testing:revs:2': { + changeset: 'Z:5>1+1$o', + meta: { + author: 'a.foo', + timestamp: 1597632398288, + pool: { + numToAttrib: {}, + nextNum: 0, + }, + atext: { + text: 'foooo\n', + attribs: '*0|1+6', + }, + }, + }, + }); + + const importEtherpad = (records) => agent.post(`/p/${testPadId}/import`) + .attach('file', Buffer.from(JSON.stringify(records), 'utf8'), { + filename: '/test.etherpad', + contentType: 'application/etherpad', + }); + + before(async function () { + // makeGoodExport() is assumed to produce good .etherpad records. Verify that assumption so + // that a buggy makeGoodExport() doesn't cause checks to accidentally pass. + const records = makeGoodExport(); + await deleteTestPad(); + await importEtherpad(records) + .expect(200) + .expect('Content-Type', /json/) + .expect((res) => assert.deepEqual(res.body, { + code: 0, + message: 'ok', + data: {directDatabaseAccess: true}, + })); + await agent.get(`/p/${testPadId}/export/txt`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.equal(res.text, 'oofoo\n')); + }); + + it('txt request rev 1', async function () { + await agent.get(`/p/${testPadId}/1/export/txt`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.equal(res.text, 'ofoo\n')); + }); + + it('txt request rev 2', async function () { + await agent.get(`/p/${testPadId}/2/export/txt`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.equal(res.text, 'oofoo\n')); + }); + + it('txt request rev 1test returns rev 1', async function () { + await agent.get(`/p/${testPadId}/1test/export/txt`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.equal(res.text, 'ofoo\n')); + }); + + it('txt request rev test1 is 403', async function () { + await agent.get(`/p/${testPadId}/test1/export/txt`) + .expect(500) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.match(res.text, /rev is not a number/)); + }); + + it('txt request rev 5 returns head rev', async function () { + await agent.get(`/p/${testPadId}/5/export/txt`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.equal(res.text, 'oofoo\n')); + }); + + it('html request rev 1', async function () { + await agent.get(`/p/${testPadId}/1/export/html`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.match(res.text, /ofoo
/)); + }); + + it('html request rev 2', async function () { + await agent.get(`/p/${testPadId}/2/export/html`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.match(res.text, /oofoo
/)); + }); + + it('html request rev 1test returns rev 1', async function () { + await agent.get(`/p/${testPadId}/1test/export/html`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.match(res.text, /ofoo
/)); + }); + + it('html request rev test1 results in 500 response', async function () { + await agent.get(`/p/${testPadId}/test1/export/html`) + .expect(500) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.match(res.text, /rev is not a number/)); + }); + + it('html request rev 5 returns head rev', async function () { + await agent.get(`/p/${testPadId}/5/export/html`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.match(res.text, /oofoo
/)); + }); + }); + describe('Import authorization checks', function () { let authorize; diff --git a/src/tests/backend/specs/messages.js b/src/tests/backend/specs/messages.js index bccb2584d..643005f12 100644 --- a/src/tests/backend/specs/messages.js +++ b/src/tests/backend/specs/messages.js @@ -77,6 +77,89 @@ describe(__filename, function () { await otherPad.remove(); } }); + + it('CHANGESET_REQ: verify revNum is a number (regression)', async function () { + const otherPadId = `${padId}other`; + assert(!await padManager.doesPadExist(otherPadId)); + const otherPad = await padManager.getPad(otherPadId, 'other text\n'); + let errorCatched = 0; + try { + await otherPad.setText('other text\n'); + await common.sendMessage(roSocket, { + component: 'pad', + padId: otherPadId, // The server should ignore this. + type: 'CHANGESET_REQ', + data: { + granularity: 1, + start: 'test123', + requestID: 'requestId', + }, + }); + assert.equal('This code should never run', 1); + } + catch(e) { + assert.match(e.message, /rev is not a number/); + errorCatched = 1; + } + finally { + await otherPad.remove(); + assert.equal(errorCatched, 1); + } + }); + + it('CHANGESET_REQ: revNum is converted to number if possible (regression)', async function () { + const otherPadId = `${padId}other`; + assert(!await padManager.doesPadExist(otherPadId)); + const otherPad = await padManager.getPad(otherPadId, 'other text\n'); + try { + await otherPad.setText('other text\n'); + const resP = common.waitForSocketEvent(roSocket, 'message'); + await common.sendMessage(roSocket, { + component: 'pad', + padId: otherPadId, // The server should ignore this. + type: 'CHANGESET_REQ', + data: { + granularity: 1, + start: '1test123', + requestID: 'requestId', + }, + }); + const res = await resP; + assert.equal(res.type, 'CHANGESET_REQ'); + assert.equal(res.data.requestID, 'requestId'); + assert.equal(res.data.start, 1); + } + finally { + await otherPad.remove(); + } + }); + + it('CHANGESET_REQ: revNum 2 is converted to head rev 1 (regression)', async function () { + const otherPadId = `${padId}other`; + assert(!await padManager.doesPadExist(otherPadId)); + const otherPad = await padManager.getPad(otherPadId, 'other text\n'); + try { + await otherPad.setText('other text\n'); + const resP = common.waitForSocketEvent(roSocket, 'message'); + await common.sendMessage(roSocket, { + component: 'pad', + padId: otherPadId, // The server should ignore this. + type: 'CHANGESET_REQ', + data: { + granularity: 1, + start: '2', + requestID: 'requestId', + }, + }); + const res = await resP; + assert.equal(res.type, 'CHANGESET_REQ'); + assert.equal(res.data.requestID, 'requestId'); + assert.equal(res.data.start, 1); + } + finally { + await otherPad.remove(); + } + }); }); describe('USER_CHANGES', function () {