Repository navigation
fix: harden EOL converter against data loss and add test suite - #12
TheJavaGuy wants to merge 5 commits into
Conversation
Rewrite src/index.js to fix 14 confirmed defects, each reproduced
against glob 9.3.5 and eol 0.9.1 on Node 24:
1. Binary files destroyed: every file was decoded as UTF-8 and written
back, so invalid bytes became U+FFFD (a PNG header 89 50 4E 47 came
out as EF BF BD 50 4E 47). Now files must pass a lossless UTF-8
round-trip and contain no NUL byte, otherwise they are skipped.
2. UTF-16 files destroyed: BOM FF FE became two U+FFFD and 0D 00 0A 00
became 0A 00 0A 00. Fixed by the same round-trip check.
3. Absolute glob read one file and overwrote another: read used
path.join(cwd, file) while write used the bare file name, so the
pattern /x/a.txt read <cwd>/x/a.txt and wrote into /x/a.txt. Now one
absolute path from glob({ absolute: true }) is used for both.
4. No arguments converted the tool's own source: argv[length-1] was
index.js and the mode fell through to LF. Now exactly two
arguments are required.
5. Any unknown mode silently meant LF (typo "crfl" performed a real
conversion). Mode is now validated against lf|crlf|warmup.
6. Warmup was bypassed when combined with crlf ("warmup crlf x" ran a
real CRLF conversion) because only argv[length-2] was inspected.
Positional parsing is replaced by strict <mode> <glob>.
7. Exit code was always 0: no match, read errors and write errors
(EACCES, ENOSPC) only printed, and a rejected run() was unhandled.
Now usage/no-match errors exit 2 and any per-file failure exits 1.
8. Non-atomic writes: writeFileSync truncated in place, so a crash,
SIGINT or full disk left a zero-length or partial file. Content is
now written to a temp file (flag wx) and renamed over the target.
9. Symlinks were written through: glob returned symlinked files and
writeFileSync followed them to any location on disk. lstat now
rejects anything that is not a regular file.
10. Lone CR was rewritten: eol.lf("a\rb") returns "a\nb". Behaviour is
kept but now documented in README as intentional normalization.
11. Unchanged files were rewritten anyway, bumping mtime on every match
and triggering rebuilds. Output is now compared to input first.
12. No ignore list: node_modules was traversed and rewritten. Default
ignore now covers **/node_modules/** and **/.git/**.
13. Whole-file string decode threw on files above the V8 string limit
and was only warned about. An explicit 256 MiB size cap now skips
such files with a message.
14. Minor: unnecessary .bind(eol), output wrongly called the glob a
"regex", no engines field although glob 9 needs Node 16+, and no
.gitattributes so a CRLF checkout of index.js broke the shebang.
All four addressed.
Also adds a per-run summary (changed/unchanged/skipped/failed) and
updates README for the now-mandatory mode argument and the new
skip/ignore/exit-code behaviour.
- writeAtomic now chmods the temp file so the umask cannot alter the preserved permission bits - add node:test suite with 80% coverage thresholds (currently 100%) - export parseArgs, isUtf8Text, writeAtomic and run for testing; add JSDoc - add GitHub Actions workflow for Node 22 and 24 on Ubuntu and Windows - package.json: engines >=22.8.0, SPDX license id, files field
|
@jurosh I know this PR turned out to be quite large. If you're uncomfortable with merging it I can split it into several small, focused ones. |
|
@jurosh Second try 🙏 |
There was a problem hiding this comment.
🟡 Changes recommended
Conversion can corrupt ambiguous binary or UTF-16 files, write through symlinked directories, lose access metadata, or delete pre-existing temporary files.
8 open issues
What changed in this PR
This PR hardens the line-ending converter against data loss and adds automated tests and cross-platform CI.
Changes:
- Reworks conversion with strict arguments, UTF-8 validation, file filtering, atomic writes, and explicit exit codes.
- Adds unit and CLI tests with coverage thresholds, plus Ubuntu and Windows CI on Node 22 and 24.
- Updates usage documentation, package metadata, and repository line-ending settings.
| File | Description |
|---|---|
test/index.test.js |
Adds tests for validation, conversion, file handling, and CLI behavior. |
src/index.js |
Reworks conversion safeguards, atomic writes, reporting, and testable exports. |
README.md |
Documents mandatory modes, requirements, behavior, and exit codes. |
package.json |
Adds the test command, Node requirement, package file filtering, and SPDX license identifier. |
.github/workflows/test.yml |
Adds a Node 22/24 test matrix on Ubuntu and Windows. |
.gitattributes |
Sets LF endings for files Git treats as text. |
Review effort: Balanced
| if (buf.includes(0)) return false | ||
| return Buffer.from(buf.toString('utf8'), 'utf8').equals(buf) |
There was a problem hiding this comment.
Agreed the README overpromised. No content-only check can rule this out: any UTF-16 file with an ASCII character or line break contains a NUL and is skipped, so only NUL-free data that is also valid UTF-8 slips through. Fixed the README in 3e4027c to state the limit and recommend targeting text files with the glob.
| fs.writeFileSync(tmp, content, { mode, flag: 'wx' }) | ||
| fs.chmodSync(tmp, mode & 0o7777) | ||
| fs.renameSync(tmp, file) |
There was a problem hiding this comment.
Fixed in 3e4027c: replaced temp file + rename with an in-place rewrite (open r+, write, ftruncate). The inode is kept, so owner, group, mode, ACLs and xattrs are preserved.
| try { | ||
| fs.writeFileSync(tmp, content, { mode, flag: 'wx' }) | ||
| fs.chmodSync(tmp, mode & 0o7777) | ||
| fs.renameSync(tmp, file) | ||
| } catch (err) { | ||
| try { fs.unlinkSync(tmp) } catch { /* nothing to clean */ } | ||
| throw err | ||
| } |
There was a problem hiding this comment.
Fixed in 3e4027c: the temp file is gone entirely (files are rewritten in place), so there is no cleanup path left.
| const st = fs.lstatSync(file) | ||
| if (!st.isFile()) { stats.skipped++; console.log(`SKIP not a regular file: ${file}`); continue } |
There was a problem hiding this comment.
Partly agree. Verified with glob 9.3.5: ** does not descend into symlinked directories, so this only happens when the user names the symlinked directory explicitly in the pattern, which is an explicit request. Documented the behaviour in the README in 3e4027c rather than adding a realpath check that would also reject legitimate setups (e.g. a symlinked project directory).
| const tmp = path.join(path.dirname(file), | ||
| `.${path.basename(file)}.${process.pid}.${crypto.randomBytes(4).toString('hex')}.tmp`) |
There was a problem hiding this comment.
Fixed in 3e4027c: no temp file name anymore, files are rewritten in place.
| */ | ||
| async function run(argv = process.argv.slice(2), cwd = process.cwd()) { | ||
| const { mode, pattern } = parseArgs(argv) | ||
| const convert = mode === 'crlf' ? eol.crlf : eol.lf |
There was a problem hiding this comment.
Fixed in 3e4027c: warmup takes an optional target, eolConverter warmup crlf "<glob>" previews a CRLF conversion; warmup <glob> previews LF (the default). Covered by a test.
| } catch (error) { | ||
| console.warn(error) | ||
| const st = fs.lstatSync(file) | ||
| if (!st.isFile()) { stats.skipped++; console.log(`SKIP not a regular file: ${file}`); continue } |
There was a problem hiding this comment.
Fixed in 3e4027c: in-place rewrite keeps the inode, so all hard links see the new content. Added a test.
| if (!isUtf8Text(input)) { stats.skipped++; console.log(`SKIP not UTF-8 text: ${file}`); continue } | ||
| const output = Buffer.from(convert(input.toString('utf8')), 'utf8') | ||
| if (output.equals(input)) { stats.unchanged++; continue } | ||
| if (!dryRun) writeAtomic(file, output, st.mode) |
There was a problem hiding this comment.
Fixed in 3e4027c: open(r+) fails with EACCES on read-only files, which are reported as FAIL and left untouched (same as before this PR). Added a test.
|
@TheJavaGuy Hey, can you take a look at reported items ? Also please add back default to LF (not making this as mandatory) so library can be used also without that extra param:
|
- Make the target optional; a lone glob converts to LF as before - Allow warmup to take a target: warmup [lf|crlf] <glob> - Overwrite files in place instead of temp file + rename so owner, ACLs, xattrs and hard links survive and read-only files are refused - Document encoding-detection and symlinked-directory limits
|
@jurosh Thanks for the review. Addressed in 3e4027c:
|
Summary
Rewrites the converter to fix 14 confirmed defects, several of which destroyed data (binary and UTF-16 files rewritten with U+FFFD, absolute globs writing to the wrong path, symlinks written through, non-atomic writes). Adds a full test suite with coverage thresholds and a CI workflow.
Changes
Converter (
src/index.js)<lf|crlf|warmup> <glob>argument parsing; unknown modes no longer silently mean LF,warmupcan no longer be bypassed**/node_modules/**and**/.git/**are always ignored1if any file failed,2on usage error or no matchTests and CI
node:testsuite covering argument parsing, UTF-8 detection, atomic write, the full run loop and the CLI process; coverage is 100% with 80% thresholds enforcedPackaging and docs
engines.noderaised to>=22.8.0(required by the test runner flags; older lines are end-of-life)Apache-2.0,filesfield so onlysrcships.gitattributesforces LFFiles Changed
src/index.jstest/index.test.js.github/workflows/test.ymlpackage.jsonREADME.md.gitattributesTesting
npm testpasses locally on Node 24.20.0: 35 tests, 100% line, branch and function coverage. Removing the chmod fix makes the two mode-preservation tests fail, confirming they catch the regression. Windows CI job is untested locally.Related Issues
None