fix(patch): reject binary file changes in npm patch commit

docs/lib/content/commands/npm-patch.md

@@ -39,6 +39,11 @@ package literally named like a subcommand must use the explicit form, e.g.
     writes the unified diff to `<patches-dir>/<name>@<version>.patch`, adds the
     entry to `patchedDependencies`, and updates `package-lock.json`.
 
+    Patches are text diffs, so binary files (images, fonts, wasm, native
+    addons) cannot be patched. If a binary file in the edit directory differs
+    from the original, the commit fails with `EPATCHBINARY` and lists the
+    files to revert.
+
 * `npm patch ls`
 
     Lists registered patches and how many installed nodes each one matches.

lib/utils/patch-diff.js

@@ -2,6 +2,7 @@
 // Used by `npm patch commit` to capture edits against a clean tarball.
 // The output is consumed by Arborist's apply step (jsdiff parsePatch).
 const { createTwoFilesPatch } = require('diff')
+const { isUtf8 } = require('node:buffer')
 const { readdir, readFile } = require('node:fs/promises')
 const { join, sep } = require('node:path')
 
@@ -27,9 +28,12 @@ const listFiles = async dir => {
   return out
 }
 
+// A NUL byte or invalid UTF-8 means the file cannot round-trip through a text diff.
+const isBinary = buf => buf !== null && (buf.includes(0) || !isUtf8(buf))
+
 const readMaybe = async file => {
   try {
-    return await readFile(file, 'utf8')
+    return await readFile(file)
   } catch {
     return null
   }
@@ -39,6 +43,7 @@ const readMaybe = async file => {
 // Added files use `--- /dev/null`, deleted files use `+++ /dev/null`.
 // The root package.json is excluded: Arborist resolves the pre-patch manifest, so a patched manifest would apply to disk without being honored.
 // ignore holds extra root-relative filenames the caller keeps out of the diff, e.g. the patch-update marker.
+// A binary file that differs throws EPATCHBINARY because a text diff cannot represent it.
 const diffDirs = async (originalDir, editedDir, ignore = new Set()) => {
   const [origFiles, editFiles] = await Promise.all([
     listFiles(originalDir),
@@ -48,13 +53,14 @@ const diffDirs = async (originalDir, editedDir, ignore = new Set()) => {
 
   let result = ''
   let packageJsonChanged = false
+  const binaryFiles = []
   for (const file of all) {
     const native = file.split('/').join(sep)
     const [a, b] = await Promise.all([
       readMaybe(join(originalDir, native)),
       readMaybe(join(editedDir, native)),
     ])
-    if (a === b) {
+    if (a === b || (a && b && a.equals(b))) {
       continue
     }
 
@@ -69,8 +75,13 @@ const diffDirs = async (originalDir, editedDir, ignore = new Set()) => {
       continue
     }
 
+    if (isBinary(a) || isBinary(b)) {
+      binaryFiles.push(file)
+      continue
+    }
+
     let patch = createTwoFilesPatch(
-      `a/${file}`, `b/${file}`, a || '', b || '', '', ''
+      `a/${file}`, `b/${file}`, a ? a.toString() : '', b ? b.toString() : '', '', ''
     ).replace('===================================================================\n', '')
 
     // mark adds and deletes with /dev/null so the apply step creates/removes files
@@ -82,6 +93,12 @@ const diffDirs = async (originalDir, editedDir, ignore = new Set()) => {
     }
     result += patch
   }
+  if (binaryFiles.length) {
+    throw Object.assign(
+      new Error(`binary files cannot be patched: ${binaryFiles.join(', ')}. Revert them in the edit directory and commit again.`),
+      { code: 'EPATCHBINARY', files: binaryFiles }
+    )
+  }
   return { diff: result, packageJsonChanged }
 }
 

test/lib/commands/patch.js

@@ -494,6 +494,31 @@ t.test('commit: only package.json changed warns and writes no patch', async t =>
   t.match(logs.warn.join('\n'), /only package.json changed/, 'warns package.json is not patchable')
 })
 
+t.test('commit: a changed binary file rejects with EPATCHBINARY and writes no patch', async t => {
+  const { npm, registry } = await loadMockNpm(t, {
+    config: { 'ignore-scripts': true, audit: false },
+    strictRegistryNock: false,
+    prefixDir: basePrefix(),
+  })
+  await setupDep(npm, registry)
+  await npm.exec('install', [])
+
+  const editDir = path.join(npm.prefix, 'clean-edit')
+  await pacote.extract(`${DEP_NAME}@${DEP_VERSION}`, editDir, npm.flatOptions)
+  fs.writeFileSync(path.join(editDir, 'index.js'), 'module.exports = () => "patched"\n')
+  fs.writeFileSync(path.join(editDir, 'logo.png'), Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x00]))
+
+  await t.rejects(
+    npm.exec('patch', ['commit', editDir]),
+    { code: 'EPATCHBINARY', files: ['logo.png'] }
+  )
+  t.notOk(
+    fs.existsSync(path.join(npm.prefix, 'patches', `${DEP_NAME}@${DEP_VERSION}.patch`)),
+    'no patch file written'
+  )
+  t.notOk(readJson(path.join(npm.prefix, 'package.json')).patchedDependencies, 'no patchedDependencies added')
+})
+
 t.test('commit: package.json change alongside code is dropped with a warning', async t => {
   const { npm, logs, registry } = await loadMockNpm(t, {
     config: { 'ignore-scripts': true, audit: false },

test/lib/utils/patch-diff.js

@@ -147,3 +147,46 @@ 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('changed binary files are rejected with EPATCHBINARY', async t => {
+  const dir = t.testdir({
+    orig: {
+      'mod.bin': Buffer.from([0x00, 0x01, 0x02]),
+      'latin1.txt': Buffer.from([0x63, 0x61, 0x66, 0xe9]),
+      'same.bin': Buffer.from([0x00, 0xff]),
+      'text.js': 'a\n',
+    },
+    edit: {
+      'mod.bin': Buffer.from([0x00, 0x01, 0x03]),
+      'latin1.txt': Buffer.from([0x63, 0x61, 0x66, 0xe8]),
+      'add.wasm': Buffer.from([0x00, 0x61, 0x73, 0x6d]),
+      'same.bin': Buffer.from([0x00, 0xff]),
+      'text.js': 'b\n',
+    },
+  })
+  await t.rejects(
+    diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit')),
+    { code: 'EPATCHBINARY', files: ['add.wasm', 'latin1.txt', 'mod.bin'] }
+  )
+})
+
+t.test('deleting a binary file is rejected with EPATCHBINARY', async t => {
+  const dir = t.testdir({
+    orig: { 'gone.png': Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x00]) },
+    edit: {},
+  })
+  await t.rejects(
+    diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit')),
+    { code: 'EPATCHBINARY', files: ['gone.png'], message: /gone\.png/ }
+  )
+})
+
+t.test('unchanged binary files do not block a text diff', async t => {
+  const dir = t.testdir({
+    orig: { 'img.png': Buffer.from([0x89, 0x00]), 'index.js': 'a\n' },
+    edit: { 'img.png': Buffer.from([0x89, 0x00]), 'index.js': 'b\n' },
+  })
+  const { diff } = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
+  t.match(diff, 'a/index.js', 'text change is captured')
+  t.notMatch(diff, 'img.png', 'unchanged binary is not in the diff')
+})