Quote the Index line like the other file-name headers

[youdie006]

formatPatch writes the file name on seven header lines. Six of them run it through quoteFileNameIfNeeded — rename from, rename to, copy from, copy to, --- and +++. Index: (src/patch/create.ts:416) emitted it raw, twelve lines above the --- line that quotes it.

So createPatch can produce a patch that parsePatch cannot read back:

createPatch('x\n--- evil', 'foo\n', 'bar\n')
// Index: x
// --- evil
// ===================================================================
// --- "x\n--- evil"
// ...
parsePatch(that)  // throws: Missing "+++ ..." file header for evil

createPatch('x\n@@ -1,1 +1,1 @@', ...)
// parsePatch throws: Hunk at line 2 contained invalid line ==============

And a quieter version with no throw — createPatch('with\nnewline.txt', …) round-trips oldFileName correctly but yields index === 'with'.

The parse side gets unquoteIfQuoted, which is the inverse this file already applies to rename from / rename to at src/patch/parse.ts:129. Both halves reuse helpers that were already here; nothing new is introduced.

What this argument does not rest on

I checked, and neither GNU diff nor git emits an Index: line at all — it is a CVS/svn-ism — and git apply ignores it. So there is no external reference saying the line should be quoted. The case here is purely that jsdiff quotes this name everywhere else it writes it, and that its own parser chokes on its own output when it doesn't.

Testing

Two tests in the existing headers handling block, deliberately split so each pins one side: the create-side asserts the emitted text, the parse-side parses a hardcoded string.

• Both fail on master (expected 'Index: x\n--- evil…' to equal 'Index: "x\n--- evil"…' and expected '"x\n--- evil"' to equal 'x\n--- evil') and pass here.
• Mutation-checked in both directions, with a rebuild between each since the tests import from libesm/: reverting create.ts alone fails only the create-side test, reverting parse.ts alone fails only the parse-side test.
• Full suite: 318 passing, against 316 at base — my two tests are the whole delta. yarn lint is clean apart from the pre-existing warning on the TODO at parse.ts:188.
• Also ran a round-trip sweep through createPatch/parsePatch/applyPatch over file names containing a space, a quote, a backslash, a tab, é and 한글, plus content lines that mimic diff syntax — no regressions.

One thing I could not run, honestly

I could not get a meaningful number out of the nyc 100% gate. yarn run-mocha passes --require ./runtime, and on Node 20, 22 and 25 that fails before any test runs — the path has no extension for the ESM resolver, and runtime.js uses require inside a "type": "module" package. This happens on a clean checkout too, so it is not from this change; I worked around it locally with a .cjs shim to run the suite at all. Under nyc the instrumentation then never attaches and every file reports 0%, base included, so the figure is meaningless rather than failing.

The structural argument in the meantime: this changes two existing lines and adds no branch, both call pre-existing helpers, and each changed line is proven executed because reverting it turns exactly one test red. unquoteIfQuoted's false branch is already taken by every ordinary Index: foo patch. Worth confirming on whatever Node version yarn test actually runs for you — and you may want that --require line to point at a .cjs file regardless.

Left alone

The trailing-space file name quirk ('tail ' → 'tail') is real but is the case you explicitly deferred to a major release at parse.ts:188-196.

---

AI assistance disclosure: this patch was found and written with Claude Code. Every quoted output above is verbatim from running it against master and against this branch.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-08T14:06:34.445212Z	2de3afd	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2de3afdf68

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve raw Index filenames from older jsdiff output

For a literal filename such as "report", previous versions of formatPatch emitted Index: "report" while quoting the ---/+++ names as "\"report\"". This unconditional decoding now parses such an older jsdiff patch with index === 'report' but oldFileName === '"report"', so consumers that use index to select the target file can load the wrong file. Restrict unquoting to representations that could have been produced by the new quoting behavior, or reconcile the index with the parsed file headers.

Useful? React with 👍 / 👎.

[ExplodingCabbage]

Interesting. Unlike your previous PR I find some of the AI use a bit annoying here - the changes are reasonable (albeit I don't agree with them - see below) but the PR description looks to be a copy-and-paste from an AI chat session which both is less clear than it could be about the underlying issue and also waffles at tedious and unhelpful length about tests. But never mind - there's nonetheless an interesting underlying issue and I'm grateful that you've brought it to my attention.

To summarise that issue...

The bug

Currently, when jsdiff creates a patch with a header like Index: file.txt, it does not quote the filename nor escape any characters whatsoever in it. Nor does parsePatch ever attempt to unquote or unescape such a filename, even if it is quoted. This works fine except in one case: where the filename itself contains a newline character. In that case, it looks like the end of the Index: ... line. A purposely-crafted perverse/evil filename can even contain further patch content (like --- headers, or an entire diff) after the newline character, which will then appear literally in the patch that jsdiff outputs.

Potentially this results in jsdiff outputting a syntactically invalid patch that it cannot read back. Worse, in theory there could perhaps even be a kind of injection attack possible here (albeit an absurdly niche one that seems unlikely to ever be useful to a real attacker) whereby an attacker who (for some weird reason) controls a filename that will be used in an Index header in a patch created by jsdiff but does not have control over the rest of the patch content can exploit this overflowing from the Index line to write arbitrary content to the patch.

Definitely a real bug, deserving of a fix. But what fix?

@youdie006's proposed fix

This patch proposes fixing this by selectively quoting/escaping filenames in Index: headers if they contain special characters, following the same logic we use for filenames in other places like ---/+++ headers or git diff --index headers. This would indeed prevent filenames with literal newlines from overflowing the Index: header, and thus also eliminate the theoretical injection attack and ensure that jsdiff can always parse the patches that jsdiff itself has output.

But I think there's a case to be made that this is the wrong fix, because while it makes jsdiff's patch creator and patch parser compatible with each other, it makes them inconsistent with every other significant patch-generating tool in the universe. Index: lines are basically only output by SVN and CVS, and neither tool ever quotes filenames in them (confirmed svn by my own experiment, and am taking Claude Opus's word about CVS). Instead SVN simply refuses to work with files that have line feed characters in their name, at all:

mark@fractal:~/svntest/myworkingcopy$ svn add 'foo
> bar'
svn: E160005: Invalid control character '0x0a' in path '/home/mark/svntest/myworkingcopy/foo\012bar'

On the other hand, even though nobody else generates patches with quoted Index: filenames, GNU patch does understand quoted filenames to be quoted, and unquotes them when parsing them. That is, trying to use GNU patch to apply this patch...

Index: "x\n--- evil"
===================================================================
@@ -1,1 +1,1 @@
-foo
+bar

... will successfully apply to a file whose filename is literally

x
--- evil

but will not apply to a file whose filename is literally "x\n--- evil" (including the quotes). And Opus tells me that SVN and CVS never parse the Index: line, despite being the only notable tools that output it. So as far as making the patches we output compatible with tools that might apply them, your proposed fix is good.

The alternative possible simple fix

Copy SVN. Refuse to output an Index: line at all for patches of files with newlines in the filename. This would make us output the same thing as other patch-generation tools.

The ideal fix

Make the quoting & unquoting behaviour configurable via options, to allow consistency/compatibility with whatever other tools the user cares about. I note that this configurability needs to extend to whether or not to unquote the ---/+++ headers too, because (per Opus - needs verifying) these are what SVN and CVS look at when applying patches, and SVN and CVS do not unquote them - i.e. my changes in v9 of jsdiff made the patches with quoted filenames that we output incompatible with SVN's and CVS's patch-applying commands.

If and when we do this, we need to make sure that in the no-quoting case, we still don't allow the kind of overflow/injection/malformation scenario described in this issue (which we can avoid either by throwing an error when a filename contains a newline, or maybe by replacing it with a space or something).

For now

I think that, since it improves our compatibility with GNU patch, avoids outright malformed patches, fixes the hypothetical injection attack, and doesn't seem to break compatibility in any way that is actually meaningful, it makes sense to merge this change despite it making us a little less consistent with the only other popular tools that actually output Index: lines.

In due course, it'd be good to also make the quoting/unquoting behaviour configurable (but still with this behaviour as the default).

[ExplodingCabbage]

@youdie006 I'll wait a while before merging this in case you want to do any further work on it based on my comments above!

[youdie006]

Thanks! I'm happy with the current fix. I think the configurability part is better left for a separate change 🙂