diff --git a/CHANGELOG.md b/CHANGELOG.md index 820290c49bb..b416d66b094 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,30 @@ +# 3.3.9 + +3.3.9 is a security and feature release. It closes a path traversal in plugin uninstall (GHSA-g9xm-3pqr-wcph), adds built-in pad retention, and ships a batch of fixes. + +### Security + +- **Plugins — dependency names are validated on uninstall as well as install (GHSA-g9xm-3pqr-wcph).** GHSA-92hr-gmr6-h8cp made the plugin installer validate dependency names on install, but the uninstall path still passed the raw `package.json` dependency key to `rmSync`/`unlinkSync`. A plugin declaring a dependency named like `zzz/../../..` could therefore delete a directory outside the plugin folder when an admin uninstalled it. Every install and uninstall path now resolves dependency paths through one helper that refuses names that escape the plugin directory. Exploiting it needs an admin to install and then uninstall the malicious plugin. Reported by @baeseungwon1010. + +### New features + +- **Pad retention — delete pads nobody has edited for N days (#8340).** A new `padRetention` setting replaces the unmaintained `ep_delete_after_delay` plugin without its HTML archive copies or tombstone pads. It is off by default and, once enabled, only logs `would delete pad ` until `dryRun` is set to `false`. A periodic sweep reads each pad's last-edit time straight from the database without loading pads into the cache, and an overdue pad is also deleted when someone opens it instead of showing stale content. Deletion uses the same path as the `deletePad` API. Options: `deleteAfterDays`, `warnBeforeDays`, `sweepIntervalHours`, `excludeGroupPads` and `excludePattern`. +- **Settings — hide the deletion-token modal without widening deletion rights (#7996, #8319).** The new `suppressPadDeletionTokenModal` setting (`SUPPRESS_PAD_DELETION_TOKEN_MODAL` in the container image) stops the one-time "Save your pad deletion token" modal from appearing. Until now the only way to hide it was `allowPadDeletionByAllUsers: true`, which also let any editor delete the pad. The token is still issued and still works, and non-creators still can't delete. Thanks to @sirsqm. + +### Notable fixes + +- **Performance — fonts are no longer inlined into the minified CSS (#8268, #8314).** The pad stylesheet embedded every editor and icon font as base64, so `pad.css` was about 1.7 MB and every visitor downloaded all of it before the pad rendered. Fonts are now fetched separately and only when used, and `pad.css` is about 61 KB. Thanks to @sirsqm. +- **Toolbar — a popup opened from the toolbar keeps focus (#8345).** The editor grabbed focus back after every toolbar command, so in current Firefox Escape didn't close the settings or users popup and typing a name didn't reach the name field. +- **Colibris — dark mode colours apply everywhere (#8271).** The light `--text-color` and `--bg-color` values could override the dark ones on elements outside the editor, such as the history banner. Thanks to @bobmullut. +- **Session transfer — failed requests are reported as failures (#8172, #8273).** Errors from the session transfer endpoint were treated as success. They now show a localized error, and a retry clears it. Thanks to @toashu123. +- **Share dialog — "Read only" is now "Use read-only link" (#8181, #8326).** The checkbox switches which link and embed code you copy; it never locked the pad. The new label and an accessible description say so. Thanks to @jum-apzn. +- **Docker — `--build-arg PnpmVersion` is honoured again (#8210, #8320).** Thanks to @sirsqm. + +### Other changes + +- Localisation updates from translatewiki.net. +- Dependency updates, including express 5.3.0, ejs 7.0.1, nodemailer 10.1.1, surrealdb 2.1.1, mssql 12.7.9 and ueberdb2 6.1.18. + # 3.3.8 3.3.8 is a security release. It closes an HTTP API privilege escalation for instances using the built-in OIDC provider with API clients (GHSA-38vj-95q4-gwvx). diff --git a/src/static/js/pluginfw/LinkInstaller.ts b/src/static/js/pluginfw/LinkInstaller.ts index 430e1fa9171..25278fe2116 100644 --- a/src/static/js/pluginfw/LinkInstaller.ts +++ b/src/static/js/pluginfw/LinkInstaller.ts @@ -5,6 +5,7 @@ import {accessSync, constants, rmSync, symlinkSync, unlinkSync} from "node:fs"; import {dependencies, name} from '../../../package.json' import settings from '../../../node/utils/Settings'; import {readFileSync} from "fs"; +import {isValidDependencyName, resolveDependencyPath} from "./dependencyPath"; export class LinkInstaller { private livePluginManager: PluginManager; @@ -92,6 +93,11 @@ export class LinkInstaller { } private async removeSubDependency(_name: string, dependency:string) { + const dependencyPath = resolveDependencyPath(pluginInstallPath, dependency) + if (dependencyPath == null) { + console.error(`Skipping plugin dependency with invalid name: ${dependency}`) + return + } if (this.dependenciesMap.has(dependency)) { console.debug(`Dependency ${dependency} is still being used by other plugins`) return @@ -99,7 +105,7 @@ export class LinkInstaller { // Read sub dependencies try { const json:IPluginInfo = JSON.parse( - readFileSync(path.join(pluginInstallPath, dependency, 'package.json'), 'utf-8')); + readFileSync(path.join(dependencyPath, 'package.json'), 'utf-8')); if(json.dependencies){ for (let [subDependency] of Object.entries(json.dependencies)) { await this.removeSubDependency(dependency, subDependency) @@ -110,11 +116,16 @@ export class LinkInstaller { } private uninstallDependency(dependency: string) { + const dependencyPath = resolveDependencyPath(pluginInstallPath, dependency) + if (dependencyPath == null) { + console.error(`Skipping plugin dependency with invalid name: ${dependency}`) + return + } try { console.debug(`Uninstalling dependency ${dependency}`) // Check if the dependency is already installed - accessSync(path.join(pluginInstallPath, dependency), constants.F_OK) - rmSync(path.join(pluginInstallPath, dependency), { + accessSync(dependencyPath, constants.F_OK) + rmSync(dependencyPath, { force: true, recursive: true }) @@ -146,6 +157,11 @@ export class LinkInstaller { } private async unlinkSubDependency(plugin: string, dependency: string) { + const dependencyPath = resolveDependencyPath(pluginInstallPath, dependency) + if (dependencyPath == null) { + console.error(`Skipping plugin dependency with invalid name: ${dependency}`) + return + } if (this.dependenciesMap.has(dependency)) { this.dependenciesMap.get(dependency)?.delete(plugin) if (this.dependenciesMap.get(dependency)!.size > 0) { @@ -157,7 +173,7 @@ export class LinkInstaller { // Read sub dependencies try { const json:IPluginInfo = JSON.parse( - readFileSync(path.join(pluginInstallPath, dependency, 'package.json'), 'utf-8')); + readFileSync(path.join(dependencyPath, 'package.json'), 'utf-8')); if(json.dependencies){ for (let [subDependency] of Object.entries(json.dependencies)) { await this.unlinkSubDependency(dependency, subDependency) @@ -177,17 +193,8 @@ export class LinkInstaller { } } - // A dependency name comes from a plugin's package.json and is used to build - // filesystem paths (readFileSync / symlinkSync), so validate it is a plain npm - // package name (optional scope, no path separators or "..") before use. - private isValidDependencyName(dependency: string): boolean { - return typeof dependency === 'string' && - !dependency.includes('..') && - /^(@[a-z0-9-._~]+\/)?[a-z0-9-._~]+$/i.test(dependency); - } - private async addSubDependency(plugin: string, dependency: string) { - if (!this.isValidDependencyName(dependency)) { + if (!isValidDependencyName(dependency)) { console.error(`Skipping plugin dependency with invalid name: ${dependency}`) return } @@ -214,7 +221,7 @@ export class LinkInstaller { } private linkDependency(dependency: string) { - if (!this.isValidDependencyName(dependency)) { + if (!isValidDependencyName(dependency)) { console.error(`Skipping plugin dependency with invalid name: ${dependency}`) return } @@ -236,10 +243,15 @@ export class LinkInstaller { } private unlinkDependency(dependency: string) { + const linkPath = resolveDependencyPath(node_modules, dependency) + if (linkPath == null) { + console.error(`Skipping plugin dependency with invalid name: ${dependency}`) + return + } try { // Check if the dependency is already installed - accessSync(path.join(node_modules, dependency), constants.F_OK) - unlinkSync(path.join(node_modules, dependency)) + accessSync(linkPath, constants.F_OK) + unlinkSync(linkPath) } catch (err) { // Symlink does not exist // So nothing to do diff --git a/src/static/js/pluginfw/dependencyPath.ts b/src/static/js/pluginfw/dependencyPath.ts new file mode 100644 index 00000000000..94ee41fe60d --- /dev/null +++ b/src/static/js/pluginfw/dependencyPath.ts @@ -0,0 +1,21 @@ +import path from "path"; + +// A dependency name comes from a plugin's package.json and is used to build +// filesystem paths (readFileSync / symlinkSync / unlinkSync / rmSync), so it +// must be a plain npm package name (optional scope, no path separators, no +// segment starting with ".", no "..") before use. +export const isValidDependencyName = (dependency: unknown): dependency is string => + typeof dependency === 'string' && + !dependency.includes('..') && + /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/i.test(dependency); + +// Returns the absolute path of `dependency` inside `base`, or null if the name +// is invalid or the resolved path would leave `base`. Callers must treat null +// as "skip": never fall back to the raw name. +export const resolveDependencyPath = (base: string, dependency: unknown): string | null => { + if (!isValidDependencyName(dependency)) return null; + const root = path.resolve(base); + const resolved = path.resolve(root, dependency); + if (!resolved.startsWith(root + path.sep)) return null; + return resolved; +}; diff --git a/src/tests/backend-new/specs/dependencyPath.test.ts b/src/tests/backend-new/specs/dependencyPath.test.ts new file mode 100644 index 00000000000..04d9424aa3c --- /dev/null +++ b/src/tests/backend-new/specs/dependencyPath.test.ts @@ -0,0 +1,40 @@ +'use strict'; + +import os from 'os'; +import path from 'path'; +import {describe, expect, it} from 'vitest'; +import { + isValidDependencyName, + resolveDependencyPath, +} from '../../../static/js/pluginfw/dependencyPath'; + +describe('resolveDependencyPath', () => { + const base = path.join(os.tmpdir(), 'ep-base'); + + it('accepts plain and scoped npm names', () => { + expect(resolveDependencyPath(base, 'ep_align')).toBe(path.join(base, 'ep_align')); + expect(resolveDependencyPath(base, '@scope/pkg')).toBe(path.join(base, '@scope', 'pkg')); + }); + + it('rejects traversal, absolute and malformed names', () => { + for (const bad of [ + 'zzz/../../../../tmp/victim', + '../victim', + '..', + '.', + '', + '/etc', + 'a/b', + 'a\\..\\b', + '@scope/../x', + '.hidden', + '@scope/.x', + null, + undefined, + 42, + ]) { + expect(isValidDependencyName(bad), String(bad)).toBe(false); + expect(resolveDependencyPath(base, bad), String(bad)).toBeNull(); + } + }); +}); diff --git a/src/tests/backend/specs/LinkInstaller.ts b/src/tests/backend/specs/LinkInstaller.ts index b9f7aae1da2..9037ad14892 100644 --- a/src/tests/backend/specs/LinkInstaller.ts +++ b/src/tests/backend/specs/LinkInstaller.ts @@ -139,4 +139,80 @@ describe(__filename, function () { assert(msg.includes('nonexistent'), 'error should mention the path'); }); }); + + // GHSA-g9xm-3pqr-wcph: uninstalling a plugin whose package.json declares a + // dependency key like "zzz/../../../../tmp/x" made LinkInstaller rmSync() / + // unlinkSync() a path outside pluginInstallPath / node_modules. Importing + // installer first lets the real LinkInstaller instance load despite the + // circular import. + describe('uninstall path traversal (GHSA-g9xm-3pqr-wcph)', function () { + let installer: any; + let li: any; + let victimRoot: string; + let victimDir: string; + let victimFile: string; + const traversal = (from: string, to: string) => + `zzz/${path.relative(path.join(from, 'zzz'), to)}`; + + before(async function () { + installer = await import('../../../static/js/pluginfw/installer'); + li = installer.linkInstaller; + }); + + beforeEach(function () { + // Keep the victims next to the plugin folder rather than in os.tmpdir(): + // on Windows CI the temp dir is on a different drive, so no relative + // traversal path to it exists. + victimRoot = fs.mkdtempSync( + path.join(path.dirname(installer.pluginInstallPath), '.g9xm-victim-')); + victimDir = path.join(victimRoot, 'victim-dir'); + fs.mkdirSync(victimDir); + fs.writeFileSync(path.join(victimDir, 'marker.txt'), 'keep me'); + victimFile = path.join(victimRoot, 'victim-file.txt'); + fs.writeFileSync(victimFile, 'keep me'); + sinon.stub(console, 'error'); + sinon.stub(console, 'debug'); + }); + + afterEach(function () { + fs.rmSync(victimRoot, {recursive: true, force: true}); + }); + + it('uninstallPlugin does not delete a directory outside pluginInstallPath', async function () { + const evil = traversal(installer.pluginInstallPath, victimDir); + // This is the path the vulnerable code handed to rmSync(). + assert.equal(path.join(installer.pluginInstallPath, evil), victimDir); + sinon.stub(li.livePluginManager, 'getInfo').returns({ + name: 'ep_g9xm_poc', + version: '1.0.0', + location: path.join(installer.pluginInstallPath, 'ep_g9xm_poc'), + mainFile: 'index.js', + dependencies: {[evil]: 'jashkenas/underscore'}, + dependencyDetails: {}, + }); + sinon.stub(li.livePluginManager, 'uninstall').resolves(); + + await li.uninstallPlugin('ep_g9xm_poc'); + + assert(fs.existsSync(path.join(victimDir, 'marker.txt')), 'victim dir was deleted'); + }); + + it('sub-dependency removal does not follow a traversal name', async function () { + const evil = traversal(installer.pluginInstallPath, victimDir); + await li.removeSubDependency('ep_x', evil); + await li.unlinkSubDependency('ep_x', evil); + li.uninstallDependency(evil); + + assert(fs.existsSync(path.join(victimDir, 'marker.txt')), 'victim dir was deleted'); + }); + + it('unlinkDependency does not unlink a file outside node_modules', function () { + const evil = traversal(installer.node_modules, victimFile); + assert.equal(path.join(installer.node_modules, evil), victimFile); + + li.unlinkDependency(evil); + + assert(fs.existsSync(victimFile), 'victim file was unlinked'); + }); + }); });