fix(fs): detect binary files that contain no NUL byte - #115
Open
Aikiooo wants to merge 1 commit into
Open
Conversation
`FS_READ` decided binary vs text with `buffer.includes(0)`. JPEG, PNG, PDF
and ZIP files do not have to contain a NUL byte, and Node's
`toString("utf-8")` is not fatal — invalid sequences become U+FFFD instead
of being reported. Those files were sent with `encoding: "utf-8"`, so the
client re-encoded the replaced characters and the bytes were lost.
Use the heuristic the app already applies in `isLikelyBinaryBytes` — a NUL
byte, a fatal UTF-8 decode, then a control-character ratio — so both ends
agree on what counts as binary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
FS_READdecides whether a file is binary with a NUL-byte check:Plenty of binary formats do not contain a NUL byte in the bytes read — JPEG, PNG, PDF and ZIP all have contents where one is not guaranteed. When the check misses, the buffer is sent as
buffer.toString("utf-8")withencoding: "utf-8".toString("utf-8")is not fatal: invalid sequences are replaced with U+FFFD rather than throwing. The client seesencoding: "utf-8"and re-encodes that string, so the replaced bytes are gone for good. This affects previews and downloads, which trust theencodingthe host reports rather than sniffing the payload themselves.Change
Reuse the heuristic the app already applies in
isLikelyBinaryBytes: a NUL byte, a fatal UTF-8 decode, then a control-character ratio above 0.3. Both ends now agree on what counts as binary.No protocol change — the same
FS_READ/FS_READ_RESULTshapes and the sameencodingvalues, just more files classified asbase64.Risk
Text in a legacy single-byte encoding (Windows-1252, Latin-1) with a high proportion of control characters would now be classified as binary. That is the trade-off the app already makes with the same heuristic on its own reads. UTF-8, UTF-8 with a BOM, and empty files stay text.
Tests
The
clipackage has no test setup, so I did not add a test rather than introduce a framework as part of this change.tscandbiome checkare clean.Notes
GIT_READhas the same weakness, but it reads withencoding: "utf-8"up front, so a fix there also means switching that call toencoding: "buffer". I left it out to keep this to a single behavior change.