fix: decode 8-bit string fields as latin-1 instead of ascii - #456
Open
johansolve wants to merge 1 commit into
Open
fix: decode 8-bit string fields as latin-1 instead of ascii#456johansolve wants to merge 1 commit into
johansolve wants to merge 1 commit into
Conversation
Node's 'ascii' encoding masks off bit 7 rather than validating the input, so any byte at 0x80 or above silently becomes a different character. A B&G Vulcan 7 sends PGN 129285 route names as STRING_LAU with control byte 1 and latin-1 characters: "Hättan-Askim" arrives as 48 e4 74 74 ... and decoded as 'Hdttan-Askim'. The plotter shows the name correctly, so the corruption is ours. latin-1 is backwards compatible for the range that matters: for genuine 7-bit ASCII the two encodings produce identical output, so no correct decode can break. Above 0x7f it at least preserves the byte value, which a consumer can reinterpret; 'ascii' destroys it. This follows the same reasoning as 82d12b2, which moved STRING_LZ off 'ascii' for the same class of problem. Applies to STRING_LAU, STRING_FIX and 'String with start/stop byte'. toPgn already writes these fields with charCodeAt, i.e. latin-1, so this also makes the round trip symmetric for high bytes rather than introducing an asymmetry. Visible behaviour change: padding or junk bytes at 0x80 and above in STRING_FIX now render as U+0080-U+00FF instead of their bit-7-stripped equivalents. The test case is a real 129285 captured off the boat, including the fast-packet reassembly, so the regression is pinned to a packet a shipping plotter actually sends.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPGN string decoding now uses Latin-1 for four string formats. A PGN 129285 fixture verifies decoding of Latin-1 route and waypoint names, including ChangesLatin-1 decoding
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
johansolve
marked this pull request as ready for review
August 3, 2026 14:57
Author
|
Please label as Fix |
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.
The problem
Node's
'ascii'encoding masks off bit 7 rather than validating its input, so any byte at0x80or above silently decodes to a different character.A B&G Vulcan 7 sends PGN 129285 route names as
STRING_LAUwith control byte 1 and latin-1 characters. The Swedish route nameHättan-Askimarrives on the bus as:and canboatjs decodes it as
Hdttan-Askim—0xe4becomes0x64. The plotter itself displays the name correctly, so the corruption happens on our side. Downstream this reaches Signal K asnavigation.courseRhumbline.activeRoute.name, which then no longer matches the same route stored in the resources API, where the name is intact because it took an HTTP path instead of an N2K one.The change
'ascii'→'latin1'in the three readers that decode 8-bit string fields:STRING_LAU,STRING_FIX, andString with start/stop byte.This is backwards compatible where it counts. For genuine 7-bit ASCII the two encodings produce byte-identical output, so no currently correct decode can break. Above
0x7f, latin-1 at least preserves the byte value — a consumer that knows better can reinterpret withBuffer.from(s, 'latin1')— whereas'ascii'destroys it irrecoverably.It follows the same reasoning as 82d12b2, which moved
STRING_LZoff'ascii'for this class of problem.toPgnalready writes these fields withcharCodeAt, i.e. latin-1, so this removes a round-trip asymmetry for high bytes rather than introducing one.Visible behaviour change: padding or junk bytes at
0x80and above inSTRING_FIXnow render as U+0080–U+00FF instead of their bit-7-stripped equivalents. Both are garbage; the output differs.I checked
test/pgns/for existing expectations containing non-ASCII characters before and after — there are none besides the one added here.Test case
A real 129285 captured off the boat, including the full fast-packet reassembly, so the regression is pinned to a packet a shipping plotter actually sends.
It carries
skipEncoderTest, and the reason is worth stating because it is not what it first looks like: the Vulcan NUL-terminates itsSTRING_LAUfields and counts the terminator in the length byte, while the encoder omits it. Re-encoding therefore yields three bytes fewer (0x0fvs0x0efor the route name, and the same for each waypoint name). The reader handles both forms; changing the encoder would move the bytes under every otherSTRING_LAUencode test, so that is deliberately left alone. The0x7ffffffflat/lon in the first waypoint, which is what I first assumed was the cause, round-trips exactly.Known follow-up, deliberately not in this PR
STRING_LZis now the one remaining inconsistency: it is read as'utf-8'but written withcharCodeAt, so'ä'encodes to0xe4and reads back as U+FFFD. That affects 21 fields across the PGN definitions. Since the UTF-8 read was a deliberate choice in #296, it seemed wrong to reverse it as a side effect of this fix — but the read and write sides do disagree, and it may be worth a look.CI
The suite is red on
masterbefore this change: four failures aroundSimnet: Command AP NoDriftandSimnet: Command AP Change Course, which reproduce on an untouchedc584d70with the@canboat/ts-pgnsversion that^1.11.9currently resolves to. This branch goes from 188 passing / 4 failing to 189 passing / 4 failing — the same four.prettier --check lib testandeslintare both clean.The new test was mutation-checked with clean builds: reverting the
STRING_LAUline to'ascii'makes it fail, restoring'latin1'makes it pass. (Worth noting for anyone verifying locally:tsc -bis incremental and will not rebuild the file on a revert, which makes the fix look like it has no effect.)Co-Authored by Claude Code Opus 5.
Summary by CodeRabbit
Bug Fixes
Tests