Skip to content

fix: reject lone surrogates in stringify instead of emitting invalid TOML - #60

Open
mahirhir wants to merge 1 commit into
squirrelchat:mistressfrom
mahirhir:fix/stringify-lone-surrogates
Open

fix: reject lone surrogates in stringify instead of emitting invalid TOML#60
mahirhir wants to merge 1 commit into
squirrelchat:mistressfrom
mahirhir:fix/stringify-lone-surrogates

Conversation

@mahirhir

Copy link
Copy Markdown

stringify builds basic strings with JSON.stringify, which escapes a lone surrogate (an unpaired \uD800-\uDFFF code unit) as a \uXXXX sequence. TOML only allows \u/\U escapes for valid Unicode scalar values, and the parser rejects surrogate code points (src/primitive.ts):

if (value < 0 || value > 0x10ffff || (value >= 0xd800 && value <= 0xdfff)) {
    throw new TomlError('invalid unicode escape', { toml: str, ptr: i })
}

So stringify can emit a document that its own parse cannot read:

import { parse, stringify } from 'smol-toml'

const toml = stringify({ a: '\ud800' }) // 'a = "\ud800"\n'
parse(toml)                             // TomlError: invalid unicode escape

A lone surrogate has no representation in a TOML document, so this is the same category as the values stringify already refuses (invalid dates, symbols, functions, null/undefined in arrays). This makes formatString throw a TypeError for lone surrogates rather than producing output that round-trips into a parse error. The guard lives in formatString, so it covers both string values and quoted keys. Valid surrogate pairs such as 𝄞 are scalar values and are still serialized.

Added a test next to the existing rejects ... cases. pnpm test is green.

@DecimalTurn

Copy link
Copy Markdown

If the library doesn't have to report the location and codepoint of the lone surrogate, it would be more performant to use String.prototype.isWellFormed() instead of a regex.

@mahirhir

Copy link
Copy Markdown
Author

Agreed in principle, and the reasoning holds even harder than you put it: this throws a flat TypeError without reporting the index or the codepoint, so the regex isn't buying anything back in exchange for being slower.

The one thing stopping me swapping it is the support range. engines is node >= 18 and the CI matrix runs [18, 20, 22, 24, 26], but String.prototype.isWellFormed only landed in Node 20. On 18 it'd be undefined, so every string would fail on the call itself rather than on the check.

So the options are to leave the regex until 18 is dropped, or write s.isWellFormed?.() with the regex kept as the fallback, which is more code than just the regex. I'd lean toward leaving it as-is for now, but I'm happy to switch it either way.

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.

2 participants