Skip to content

fix: escape backslashes in values and keys so they round-trip - #307

Open
mahirhir wants to merge 1 commit into
npm:mainfrom
mahirhir:fix/escape-backslash-roundtrip
Open

mahirhir wants to merge 1 commit into
npm:mainfrom
mahirhir:fix/escape-backslash-roundtrip

Conversation

@mahirhir

Copy link
Copy Markdown

ini.parse(ini.stringify(x)) is not stable when a value or key contains a backslash.

unsafe treats \ as an escape character: in the unquoted branch it unescapes \\, \; and \#. safe only escaped ; and #, never the backslash itself, so a backslash that is written verbatim is read back as an escape sequence. Two backslashes collapse to one, and a backslash placed before ; or # gets swallowed.

const ini = require('ini')

// a value of two backslashes comes back as one
ini.parse(ini.stringify({ k: '\\\\' }))      // { k: '\\' }
ini.parse(ini.stringify({ p: 'C:\\\\tmp' })) // { p: 'C:\\tmp' }

This makes safe escape the backslash as well, so it is the inverse of unsafe. Section names are left unchanged: they use \. to escape literal dots in key paths, so safe is called with escapeBackslash = false for the section to avoid doubling those structural backslashes. The existing snapshots are byte-identical.

Round-trip tests are added in test/bar.js (two backslashes, a Windows-style path, \;/\#, and a backslash in a key). npm test passes at 100% coverage, and reverting the one-line change makes the new cases fail.

This is the behaviour #9 had in mind: "a \\ needs to be a \ and not an escape".

`unsafe` treats `\` as an escape character and unescapes `\\`, `\;` and
`\#`, but `safe` only escaped `;` and `#`. A value or key containing a
backslash therefore did not survive stringify -> parse: a value of two
backslashes was written as `k=\\` and read back as a single backslash.

`safe` now escapes the backslash as well. Section names are left
unchanged because they rely on `\.` to escape literal dots in key paths.
@mahirhir
mahirhir requested a review from a team as a code owner June 29, 2026 23:29
@mahirhir

Copy link
Copy Markdown
Author

No review on this since June, so here it is measured rather than described. I ran lib/ini.js exactly as it stands on main. The file has zero require calls, so nothing was stubbed or rewritten to make this run.

lib/ini.js sha256 = 007123836ffec243   bytes = 7123

Round-tripping parse(stringify(x)):

value                       main
no backslash                ok
semicolon                   ok
hash                        ok
one backslash               ok
windows path                ok
two backslashes             FAIL
backslash then semicolon    FAIL
backslash then hash         FAIL
trailing backslash          ok

fails on main = 3 of 9

The scope is narrower than the PR title suggests, and I would rather say so. A lone backslash round-trips. C:\tmp\x round-trips. What fails is a backslash immediately followed by \, ; or #, which is exactly the set unsafe unescapes. That is why this has survived: the common Windows-path case is fine.

The failure is silent truncation rather than an error:

{ k: 'a\;b' }  ->  "k=a\\;b\r\n"  ->  { k: 'a\' }

safe escapes ; and # but not \, while unsafe treats \ as an escape character and consumes the next one. The writer and the reader disagree about a single character, and the reader wins.

With this PR's line applied to that same file:

mutation applied: bytes 7123 -> 7174 (+51)
fails with the patch = 0 of 9
CONTROL rows the patch moved = 3   (0 would mean the change does nothing)

I also measured compatibility in both directions, since a change to safe changes bytes on disk:

value            new write -> current read     current write -> new read
"a;b"            ok                            ok
"C:\tmp\x"       ok                            ok
"a\\b"           ok                            MISMATCH
"a\;b"           ok                            MISMATCH

new-write / current-read mismatches = 0

A file written by the patched version is read correctly by the current version, for every case, so shipping this does not strand anyone's existing reader. The three mismatches in the other column are the original bug rather than something the change introduces: unsafe is untouched by this PR, and those are values the current stringify cannot write correctly in the first place.

One deliberate exception in the diff: section names pass safe(opt.section, false) and keep their current behaviour, because \. is meaningful there for escaping a literal dot in a key path.

One thing I am not claiming: nothing reports on this branch.

$ gh pr checks 307 --repo npm/ini
no checks reported on the 'fix/escape-backslash-roundtrip' branch

So the numbers above are mine, run locally against your main file, not a CI result. The added test in test/bar.js is the thing to run. Happy to rebase, or to take the narrower framing and retitle this to say backslash-before-\;# rather than backslashes generally. No attribution needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant