From 090e433432dc108cfcc4ef0f14ee6220e39153ce Mon Sep 17 00:00:00 2001 From: ubeddulla khan Date: Fri, 10 Jul 2026 13:16:34 +0530 Subject: [PATCH] fix(diff): strip control chars from generated diff headers --- lib/utils/patch-diff.js | 12 ++++++--- test/lib/utils/patch-diff.js | 12 +++++++++ workspaces/libnpmdiff/lib/format-diff.js | 19 +++++++++---- workspaces/libnpmdiff/test/format-diff.js | 33 +++++++++++++++++++++++ 4 files changed, 68 insertions(+), 8 deletions(-) diff --git a/lib/utils/patch-diff.js b/lib/utils/patch-diff.js index b0ab9a3aa0bff..5d50aaf5ca32e 100644 --- a/lib/utils/patch-diff.js +++ b/lib/utils/patch-diff.js @@ -7,6 +7,11 @@ const { join, sep } = require('node:path') const IGNORE = new Set(['node_modules', '.git']) +// filenames come from the untrusted package being patched; a name with an +// embedded newline would otherwise forge extra diff header lines in the patch +// eslint-disable-next-line no-control-regex +const cleanHeader = str => str.replace(/[\u0000-\u001f\u007f-\u009f]/g, '') + // Recursively list file paths under dir, relative and posix-separated. const listFiles = async dir => { const out = [] @@ -69,16 +74,17 @@ const diffDirs = async (originalDir, editedDir, ignore = new Set()) => { continue } + const safe = cleanHeader(file) let patch = createTwoFilesPatch( - `a/${file}`, `b/${file}`, a || '', b || '', '', '' + `a/${safe}`, `b/${safe}`, a || '', b || '', '', '' ).replace('===================================================================\n', '') // mark adds and deletes with /dev/null so the apply step creates/removes files if (a === null) { - patch = patch.replace(`--- a/${file}\t`, '--- /dev/null\t') + patch = patch.replace(`--- a/${safe}\t`, '--- /dev/null\t') } if (b === null) { - patch = patch.replace(`+++ b/${file}\t`, '+++ /dev/null\t') + patch = patch.replace(`+++ b/${safe}\t`, '+++ /dev/null\t') } result += patch } diff --git a/test/lib/utils/patch-diff.js b/test/lib/utils/patch-diff.js index dd571651be8ec..16b3832c72ad5 100644 --- a/test/lib/utils/patch-diff.js +++ b/test/lib/utils/patch-diff.js @@ -147,3 +147,15 @@ t.test('round-trip: applying the diff reproduces the edited tree', async t => { t.equal(read(orig, 'lib', 'deep', 'x.js'), 'after\n', 'nested file matches edit') t.notOk(existsSync(resolve(orig, 'del.js')), 'deleted file was removed') }) + +t.test('control chars in a filename cannot forge diff headers', async t => { + const { writeFileSync } = require('node:fs') + const dir = t.testdir({ orig: {}, edit: {} }) + // an extracted package can carry a filename with an embedded newline + const name = 'a.js\n--- x\n+++ y\n@@ -1 +1 @@\n-real\n+forged' + writeFileSync(resolve(dir, 'orig', name), 'one\n') + writeFileSync(resolve(dir, 'edit', name), 'two\n') + const { diff } = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit')) + t.notMatch(diff, /^\+forged$/m, 'newline in name does not inject a diff line') + t.notMatch(diff, /^--- x$/m, 'newline in name does not forge a --- header') +}) diff --git a/workspaces/libnpmdiff/lib/format-diff.js b/workspaces/libnpmdiff/lib/format-diff.js index f50738207c854..3793e65b2b48b 100644 --- a/workspaces/libnpmdiff/lib/format-diff.js +++ b/workspaces/libnpmdiff/lib/format-diff.js @@ -19,15 +19,22 @@ const color = (colorStr, colorId) => { return colorStr.replace(/[^\n\r]+/g, open + '$&' + close) } +// tarball entry names and non-registry manifest versions are attacker +// controlled; drop control characters so a name/version containing a newline +// can't forge extra `diff --git`/`---`/`+++` header lines in the output +// eslint-disable-next-line no-control-regex +const cleanHeader = str => String(str).replace(/[\u0000-\u001f\u007f-\u009f]/g, '') + const formatDiff = async ({ files, opts = {}, refs, versions }) => { let res = '' const srcPrefix = opts.diffNoPrefix ? '' : opts.diffSrcPrefix || 'a/' const dstPrefix = opts.diffNoPrefix ? '' : opts.diffDstPrefix || 'b/' for (const filename of files.values()) { + const safeName = cleanHeader(filename) const names = { - a: `${srcPrefix}${filename}`, - b: `${dstPrefix}${filename}`, + a: `${srcPrefix}${safeName}`, + b: `${dstPrefix}${safeName}`, } let fileMode = '' @@ -49,7 +56,7 @@ const formatDiff = async ({ files, opts = {}, refs, versions }) => { } if (opts.diffNameOnly) { - res += `${filename}\n` + res += `${safeName}\n` continue } @@ -74,8 +81,10 @@ const formatDiff = async ({ files, opts = {}, refs, versions }) => { header(`new mode ${modes.b}`) } } - /* eslint-disable-next-line max-len */ - header(`index ${opts.tagVersionPrefix || 'v'}${versions.a}..${opts.tagVersionPrefix || 'v'}${versions.b} ${fileMode}`) + const prefix = opts.tagVersionPrefix || 'v' + const va = cleanHeader(versions.a) + const vb = cleanHeader(versions.b) + header(`index ${prefix}${va}..${prefix}${vb} ${fileMode}`) if (await shouldPrintPatch(filename)) { patch += jsDiff.createTwoFilesPatch( diff --git a/workspaces/libnpmdiff/test/format-diff.js b/workspaces/libnpmdiff/test/format-diff.js index 911961792a2b6..aceb632722ea0 100644 --- a/workspaces/libnpmdiff/test/format-diff.js +++ b/workspaces/libnpmdiff/test/format-diff.js @@ -467,3 +467,36 @@ t.test('format multiple files patch', async t => { 'should output expected result for multiple files' ) }) + +t.test('control chars in a tarball entry name cannot forge diff headers', async t => { + // tar entry paths are attacker controlled and may contain a newline + const filename = 'a.js\ndiff --git a/HIDDEN b/HIDDEN\n--- a/HIDDEN\n+++ b/HIDDEN' + const files = new Set([filename]) + const refs = new Map(Object.entries({ + [`a/${filename}`]: { content: 'one\n', mode: '100644' }, + [`b/${filename}`]: { content: 'two\n', mode: '100644' }, + })) + const res = await formatDiff({ + files, + refs, + versions: { a: '1.0.0', b: '2.0.0' }, + }) + t.notMatch(res, /^diff --git a\/HIDDEN b\/HIDDEN$/m, 'no forged diff --git line') + t.notMatch(res, /^--- a\/HIDDEN$/m, 'no forged --- line') + t.notMatch(res, /^\+\+\+ b\/HIDDEN$/m, 'no forged +++ line') +}) + +t.test('control chars in a manifest version cannot forge diff lines', async t => { + const files = new Set(['foo.js']) + const refs = new Map(Object.entries({ + 'a/foo.js': { content: 'one\n', mode: '100644' }, + 'b/foo.js': { content: 'two\n', mode: '100644' }, + })) + const res = await formatDiff({ + files, + refs, + // a git/file spec's version is read verbatim from package.json + versions: { a: '1.0.0\n+forged version line', b: '2.0.0' }, + }) + t.notMatch(res, /^\+forged version line$/m, 'version newline does not inject a line') +})