From a4b9b181093a0d1b6eaa82d8a8c500e1694b523d Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 18 Aug 2026 10:01:01 +0100 Subject: [PATCH] Merge pull request #36402 from overleaf/bg-zod-move-safe-path-tests move safe path tests from zodHelpers to web GitOrigin-RevId: 3bb25fa108a4a0da0bed1ae60fabfdd47ed76283 --- .../test/unit/src/zodHelpers.test.ts | 18 --- libraries/validation-tools/zodHelpers.js | 6 +- .../test/unit/src/Project/SafePath.test.mjs | 121 +++++++++++++----- 3 files changed, 92 insertions(+), 53 deletions(-) diff --git a/libraries/validation-tools/test/unit/src/zodHelpers.test.ts b/libraries/validation-tools/test/unit/src/zodHelpers.test.ts index b57ebfd310..baac7fe03f 100644 --- a/libraries/validation-tools/test/unit/src/zodHelpers.test.ts +++ b/libraries/validation-tools/test/unit/src/zodHelpers.test.ts @@ -1,7 +1,6 @@ import { zz } from '../../../zodHelpers' import { describe, expect, it } from 'vitest' import mongodb from 'mongodb' -import SafePath from '../../../../../services/web/app/src/Features/Project/SafePath.mjs' const { ObjectId } = mongodb @@ -426,7 +425,6 @@ describe('zodHelpers', () => { const inputPath = '' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(false) - expect(SafePath.isCleanPath(inputPath)).toBe(false) expect(parsed.error?.issues).toMatchObject([ expect.objectContaining({ message: 'path is empty', @@ -441,7 +439,6 @@ describe('zodHelpers', () => { const inputPath = '/output.pdf' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(true) - expect(SafePath.isCleanPath(inputPath)).toBe(true) expect(parsed.data).toBe(inputPath) }) @@ -449,7 +446,6 @@ describe('zodHelpers', () => { const inputPath = 'foo/' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(false) - expect(SafePath.isCleanPath(inputPath)).toBe(false) expect(parsed.error?.issues).toMatchObject([ expect.objectContaining({ message: 'path is a folder, not a file', @@ -461,7 +457,6 @@ describe('zodHelpers', () => { const inputPath = '../output.pdf' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(false) - expect(SafePath.isCleanPath(inputPath)).toBe(false) expect(parsed.error?.issues).toMatchObject([ expect.objectContaining({ message: 'path traversal detected', @@ -476,7 +471,6 @@ describe('zodHelpers', () => { const inputPath = '.' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(false) - expect(SafePath.isCleanPath(inputPath)).toBe(false) expect(parsed.error?.issues).toMatchObject([ expect.objectContaining({ message: 'path segment is "." or has leading/trailing whitespace', @@ -488,7 +482,6 @@ describe('zodHelpers', () => { const inputPath = ' foobar.tex' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(false) - expect(SafePath.isCleanPath(inputPath)).toBe(false) expect(parsed.error?.issues).toMatchObject([ expect.objectContaining({ message: 'path segment is "." or has leading/trailing whitespace', @@ -500,7 +493,6 @@ describe('zodHelpers', () => { const inputPath = 'foobar.tex ' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(false) - expect(SafePath.isCleanPath(inputPath)).toBe(false) expect(parsed.error?.issues).toMatchObject([ expect.objectContaining({ message: 'path segment is "." or has leading/trailing whitespace', @@ -512,7 +504,6 @@ describe('zodHelpers', () => { const inputPath = 'foo' + String.fromCharCode(0) + '.tex' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(false) - expect(SafePath.isCleanPath(inputPath)).toBe(false) expect(parsed.error?.issues).toMatchObject([ expect.objectContaining({ message: 'path contains a disallowed character', @@ -525,7 +516,6 @@ describe('zodHelpers', () => { const inputPath = 'foo' + String.fromCharCode(0x90) + '.tex' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(false) - expect(SafePath.isCleanPath(inputPath)).toBe(false) expect(parsed.error?.issues).toMatchObject([ expect.objectContaining({ message: 'path contains a disallowed character', @@ -538,7 +528,6 @@ describe('zodHelpers', () => { const inputPath = 'foo\uD800.tex' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(false) - expect(SafePath.isCleanPath(inputPath)).toBe(false) expect(parsed.error?.issues).toMatchObject([ expect.objectContaining({ message: 'path contains a disallowed character', @@ -550,7 +539,6 @@ describe('zodHelpers', () => { const inputPath = 'foo*.tex' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(false) - expect(SafePath.isCleanPath(inputPath)).toBe(false) expect(parsed.error?.issues).toMatchObject([ expect.objectContaining({ message: 'path contains a disallowed character', @@ -562,7 +550,6 @@ describe('zodHelpers', () => { const inputPath = 'foo\\bar.tex' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(false) - expect(SafePath.isCleanPath(inputPath)).toBe(false) expect(parsed.error?.issues).toMatchObject([ expect.objectContaining({ message: 'path contains a disallowed character', @@ -577,7 +564,6 @@ describe('zodHelpers', () => { const inputPath = '__proto__/output.pdf' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(true) - expect(SafePath.isCleanPath(inputPath)).toBe(true) expect(parsed.data).toBe(inputPath) }) @@ -585,7 +571,6 @@ describe('zodHelpers', () => { const inputPath = 'constructor' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(false) - expect(SafePath.isCleanPath(inputPath)).toBe(false) expect(parsed.error?.issues).toMatchObject([ expect.objectContaining({ message: 'path is an unsafe property name', @@ -600,7 +585,6 @@ describe('zodHelpers', () => { const inputPath = 'foo/toString' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(true) - expect(SafePath.isCleanPath(inputPath)).toBe(true) expect(parsed.data).toBe(inputPath) }) @@ -608,7 +592,6 @@ describe('zodHelpers', () => { const inputPath = 'hasOwnProperty' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(false) - expect(SafePath.isCleanPath(inputPath)).toBe(false) expect(parsed.error?.issues).toMatchObject([ expect.objectContaining({ message: 'path is an unsafe property name', @@ -620,7 +603,6 @@ describe('zodHelpers', () => { const inputPath = 'foo/output.pdf' const parsed = zz.safePath().safeParse(inputPath) expect(parsed.success).toBe(true) - expect(SafePath.isCleanPath(inputPath)).toBe(true) expect(parsed.data).toBe(inputPath) }) }) diff --git a/libraries/validation-tools/zodHelpers.js b/libraries/validation-tools/zodHelpers.js index f03b1aacee..46e0c39247 100644 --- a/libraries/validation-tools/zodHelpers.js +++ b/libraries/validation-tools/zodHelpers.js @@ -90,9 +90,9 @@ const zz = { * tree is already validated against at the point of origin -- this * closes the gap for services that receive that path secondhand (e.g. * forwarded into history-v1's archive/zip builder). Parity with - * isCleanPath is asserted directly in zodHelpers.test.ts, which imports - * SafePath.mjs and checks it against every safePath() test case; the only - * remaining difference from that reference is deliberate: BADCHAR_RX's + * isCleanPath is asserted in web's SafePath.test.mjs, which imports + * zz.safePath() and checks it against every isCleanPath test case; the + * only remaining difference from that reference is deliberate: BADCHAR_RX's * leading "/" is not ported here, since "/" is the segment separator, * not a disallowed character, so a traversal payload is still caught * (as "." or "..") per split segment below, just not via this character diff --git a/services/web/test/unit/src/Project/SafePath.test.mjs b/services/web/test/unit/src/Project/SafePath.test.mjs index d970b7f727..1ede28a647 100644 --- a/services/web/test/unit/src/Project/SafePath.test.mjs +++ b/services/web/test/unit/src/Project/SafePath.test.mjs @@ -11,6 +11,7 @@ * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ import { assert, beforeEach, describe, expect, it } from 'vitest' +import { zz } from '@overleaf/validation-tools' import sinon from 'sinon' const modulePath = '../../../../app/src/Features/Project/SafePath' @@ -124,84 +125,140 @@ describe('SafePath', function () { }) describe('isCleanPath', function () { + // Cross-checked against zz.safePath() (libraries/validation-tools/ + // zodHelpers.js), which validates project doc/file paths for + // document-updater -- every case here is expected to agree with it, so + // the two validators can't silently drift apart again. + function checkIsCleanPath(ctx, inputPath, expected) { + const result = ctx.SafePath.isCleanPath(inputPath) + expect(zz.safePath().safeParse(inputPath).success).toBe(expected) + return result.should.equal(expected) + } + + it('should not accept an empty path', function (ctx) { + return checkIsCleanPath(ctx, '', false) + }) + it('should accept a valid filename "main.tex"', function (ctx) { - const result = ctx.SafePath.isCleanPath('main.tex') - return result.should.equal(true) + return checkIsCleanPath(ctx, 'main.tex', true) }) it('should accept a valid path "foo/main.tex"', function (ctx) { - const result = ctx.SafePath.isCleanPath('foo/main.tex') - return result.should.equal(true) + return checkIsCleanPath(ctx, 'foo/main.tex', true) }) it('should accept empty path elements', function (ctx) { - const result = ctx.SafePath.isCleanPath('foo//main.tex') - return result.should.equal(true) + return checkIsCleanPath(ctx, 'foo//main.tex', true) }) it('should not accept an empty filename', function (ctx) { - const result = ctx.SafePath.isCleanPath('foo/bar/') - return result.should.equal(false) + return checkIsCleanPath(ctx, 'foo/bar/', false) }) it('should accept a path that starts with a slash', function (ctx) { - const result = ctx.SafePath.isCleanPath('/etc/passwd') - return result.should.equal(true) + return checkIsCleanPath(ctx, '/etc/passwd', true) + }) + + it('should not accept path traversal', function (ctx) { + return checkIsCleanPath(ctx, '../output.pdf', false) + }) + + it('should not accept a path that is exactly "."', function (ctx) { + return checkIsCleanPath(ctx, '.', false) + }) + + it('should not accept a path segment with leading whitespace', function (ctx) { + return checkIsCleanPath(ctx, 'foo/ bar.tex', false) + }) + + it('should not accept a path segment with trailing whitespace', function (ctx) { + return checkIsCleanPath(ctx, 'foo/bar.tex ', false) + }) + + it('should not accept a path containing a null byte', function (ctx) { + return checkIsCleanPath( + ctx, + 'foo/' + String.fromCharCode(0) + '.tex', + false + ) + }) + + it('should not accept a path containing a C1 control character', function (ctx) { + // \x80-\x9F, per BADCHAR_RX + return checkIsCleanPath( + ctx, + 'foo/' + String.fromCharCode(0x90) + '.tex', + false + ) + }) + + it('should not accept a path containing a lone surrogate', function (ctx) { + // \uD800-\uDFFF, per BADCHAR_RX + return checkIsCleanPath(ctx, 'foo/\uD800.tex', false) + }) + + it('should not accept a path containing a backslash', function (ctx) { + return checkIsCleanPath(ctx, 'foo/bar\\baz.tex', false) }) it('should not accept a path that has an asterisk as the 0th element', function (ctx) { - const result = ctx.SafePath.isCleanPath('*/foo/bar') - return result.should.equal(false) + return checkIsCleanPath(ctx, '*/foo/bar', false) }) it('should not accept a path that has an asterisk as a middle element', function (ctx) { - const result = ctx.SafePath.isCleanPath('foo/*/bar') - return result.should.equal(false) + return checkIsCleanPath(ctx, 'foo/*/bar', false) }) it('should not accept a path that has an asterisk as the filename', function (ctx) { - const result = ctx.SafePath.isCleanPath('foo/bar/*') - return result.should.equal(false) + return checkIsCleanPath(ctx, 'foo/bar/*', false) }) it('should not accept a path that contains an asterisk in the 0th element', function (ctx) { - const result = ctx.SafePath.isCleanPath('f*o/bar/baz') - return result.should.equal(false) + return checkIsCleanPath(ctx, 'f*o/bar/baz', false) }) it('should not accept a path that contains an asterisk in a middle element', function (ctx) { - const result = ctx.SafePath.isCleanPath('foo/b*r/baz') - return result.should.equal(false) + return checkIsCleanPath(ctx, 'foo/b*r/baz', false) }) it('should not accept a path that contains an asterisk in the filename', function (ctx) { - const result = ctx.SafePath.isCleanPath('foo/bar/b*z') - return result.should.equal(false) + return checkIsCleanPath(ctx, 'foo/bar/b*z', false) }) it('should not accept multiple problematic elements', function (ctx) { - const result = ctx.SafePath.isCleanPath('f*o/b*r/b*z') - return result.should.equal(false) + return checkIsCleanPath(ctx, 'f*o/b*r/b*z', false) }) it('should not accept a problematic path with an empty element', function (ctx) { - const result = ctx.SafePath.isCleanPath('foo//*/bar') - return result.should.equal(false) + return checkIsCleanPath(ctx, 'foo//*/bar', false) }) it('should not accept javascript property names', function (ctx) { - const result = ctx.SafePath.isCleanPath('prototype') - return result.should.equal(false) + return checkIsCleanPath(ctx, 'prototype', false) + }) + + it('should not accept a path that is exactly "constructor"', function (ctx) { + return checkIsCleanPath(ctx, 'constructor', false) }) it('should not accept javascript property names in the prototype', function (ctx) { - const result = ctx.SafePath.isCleanPath('hasOwnProperty') - return result.should.equal(false) + return checkIsCleanPath(ctx, 'hasOwnProperty', false) }) it('should not accept javascript property names resulting from substitutions', function (ctx) { - const result = ctx.SafePath.isCleanPath(' proto ') - return result.should.equal(false) + return checkIsCleanPath(ctx, ' proto ', false) + }) + + it('should accept a nested javascript property name (only the top-level path is checked)', function (ctx) { + return checkIsCleanPath(ctx, 'foo/prototype', true) + }) + + it('should accept a __proto__ path segment as a non-empty folder', function (ctx) { + return checkIsCleanPath(ctx, '__proto__/output.pdf', true) + }) + + it('should accept a nested segment matching a built-in Object.prototype method name', function (ctx) { + return checkIsCleanPath(ctx, 'foo/toString', true) }) })