fix: keep the whitespace which came from an interpolated value - #883
Open
jcbrand wants to merge 4 commits into
Open
fix: keep the whitespace which came from an interpolated value#883jcbrand wants to merge 4 commits into
jcbrand wants to merge 4 commits into
Conversation
jcbrand
force-pushed
the
fix-interpolated-whitespace
branch
11 times, most recently
from
August 13, 2026 13:36
e59a986 to
45fe32d
Compare
`stripWhitespace` removed every whitespace-only text node from a parsed
stanza, which silently deleted content:
stx`<title type="xhtml">
<div xmlns="http://www.w3.org/1999/xhtml">${parts}</div>
</title>`
with `parts` = `[a('@bob'), '\n', a('https://example.org/x')]` dropped the
line break, and the receiving client had no way to tell it had been there.
XHTML-IM was unaffected, its markup sitting in an already-exempt `<body>`,
but Atom (XEP-0277) `type="xhtml"` constructs broke.
Whitespace in an stx template has two origins which mean opposite things: the
static parts are markup, so the indentation between their tags is formatting
and has to go, while everything interpolated with `${}` is data, so its
whitespace is content and has to stay. Concatenating the two into one string
lost the distinction.
So a `${}` value is no longer written into the text which is parsed. A
processing instruction stands in for it, and is replaced by the value's nodes
once the tree exists. A value only stands in for nodes where an element
encloses it: one at document level is the stanza itself rather than content
within one, so it is still written into the text and parsed as the markup it
is, and its whitespace is stripped like a template's.
An attribute value is the one position which cannot be covered that way, since
no XML parser accepts a placeholder there, so a value goes into the template
text and comes back through the parser. The parser normalises the whitespace in
an attribute value (XML 1.0 § 3.3.3), which lost the same whitespace again in
the one place it could not be seen by reading the template:
stx`<message xmlns="jabber:client" id="${'a\nb'}"/>` -> id="a b"
A tab and a line feed are therefore written as a character reference, which is
what carries them, so an attribute keeps what a value put there like every
other position. Markup interpolated into an attribute value is untouched, as is
a value elsewhere in a tag, which the conditional-attribute idiom relies on.
`stripWhitespace` also honours `xml:space` (XML 1.0 § 2.10), which is how to
keep whitespace written in a template or in a string passed to
`Stanza.toElement`, the one case substitution cannot cover. Its `<body>`
exemption now applies to the element passed in as well as to its children.
Interpolated markup is moved into the stanza with `adoptNode`, which the Node
shim polyfills, rather than deep-copied with `importNode`: the document it was
parsed in is dropped as soon as it has been read. That and gathering a
fragment's namespace declarations once per element rather than once per value
roughly halve the cost of building a stanza with many interpolated children.
`Builder` puts text and attribute values straight into the DOM, and
`Builder.serialize` writes them back out through `xmlescape`, which handles
only `& < > ' "`. No parser sees the value at either end, so a character
outside the `Char` production of XML 1.0 § 2.2 reached the wire intact:
$msg().c('body').t('a\u0000b') -> <body>a\u0000b</body>
The server answers such a stream with `not-well-formed` and closes it, so the
cost is the connection rather than the one message. This predates the `stx`
work and was as true in the browser as under Node; the fixes there reach only
values which are parsed or interpolated.
`xmlText()` therefore moves into `xml-chars.ts`, beside the character class
the `stx` path and the Node shim already share, and is called from the three
places a string enters a tree unparsed: `xmlTextNode`, the attribute setting
in `xmlElement`, and `Builder.attrs`. The `stx` path had its own copy of the
same normalisation, which this replaces, so a value now reads the same
whether it was interpolated or built.
Normalising a value's line endings on the way in is not enough on its own for
an attribute, because `xmlescape` leaves whitespace alone and a parser
normalises the whitespace in an attribute value (XML 1.0 § 3.3.3). A line feed
written literally is read back as a space, so the sender was left holding a
different stanza from everyone it sent it to, which is the divergence the
normalisation is there to prevent. `Builder.serialize` therefore writes a tab,
a line feed and a carriage return in an attribute value as a character
reference, which is the only way an attribute can carry one. Element content
is untouched, since a parser leaves the whitespace between tags as it stands.
Two changes for existing callers, neither able to cost a stanza which was
sendable before: `Strophe.xmlTextNode` and `Strophe.xmlElement` now throw
rather than return a node which cannot be sent, and line endings are
normalised as XML 1.0 § 2.11 requires, so a CR read back is not always the CR
which was passed in.
A value interpolated into a CDATA section went through `xmlescape`, but a
CDATA section does not read entities back, so the peer read the entity itself:
stx`<c><![CDATA[${'a & b'}]]></c>` -> <c><![CDATA[a & b]]></c>
Every `&`, `<`, `>`, `'` and `"` a value carried into one was corrupted this
way. Such a value is now written as its own characters, unescaped. The one
sequence it cannot carry is `]]>`, so the section is closed and opened again
around it, which a parser reads back as a single run of character data. The
split is applied after the parts of a value are joined, catching a `]]>` which
exists only because one part ended in `]]` and the next began with `>`.
Not a way out of the section, as it happens: escaping the `>` kept the section
closed while corrupting everything in it.
stx`<message xmlns="jabber:client"><!-- ${'hello'} --><c/></message>`
sent `<message xmlns="jabber:client"><c/></message>`. `Builder.serialize` has
no case for a comment node, so the comment and the value in it were dropped
on the way to the wire, and nothing said so: a value in a comment is
character data rather than a node, so no slot was written and `assertFilled`
had nothing to count. It is the one position where the loss cannot be caught
afterwards, so it is caught before, whatever the value holds. A comment the
template wrote itself is left alone.
Not an injection: a comment may not hold `--` at all, so a value carrying the
`-->` which would end one does not parse, and the parser already said so.
The refusal only reached a comment the cursor had read, which left out the
one a value opened at document level:
stx`${unsafeXML('<message xmlns="jabber:client"><!--')}${'SECRET'}${unsafeXML('--></message>')}`
What such a value opens is deliberately not counted, so that a stanza may be
opened by one and closed by another, and the counting was skipped by not
reading the value at all. So the cursor believed it was still in element
content while the parser was inside the comment, and `SECRET` went out as
nothing at all, with no slot written for `assertFilled` to miss either. Read
the value and put the depth back afterwards: not counting what it opens is
the property worth keeping, and reading it is a separate question.
That carries the CDATA section such a value opens as well, where the same
omission escaped a value as though it were markup, so `a & b` came back out
of the stanza as the five characters `a & b`. A section opened there is
refused outright rather than written into: nothing after it is markup, so
nothing can close it again, and the value carrying the `]]>` which would is
character data there like any other. Write the section in the template text
instead.
jcbrand
force-pushed
the
fix-interpolated-whitespace
branch
from
August 14, 2026 11:31
45fe32d to
475aaf4
Compare
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
1 participant
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.
stripWhitespaceremoved every whitespace-only text node from a parsed stanza, which silently deleted content:with
parts=[a('@bob'), '\n', a('https://example.org/x')]dropped the line break, and the receiving client had no way to tell it had been there.XHTML-IM was unaffected, since its markup sits in a
<body>and<body>subtrees were already exempt, but Atom (XEP-0277)type="xhtml"constructs broke.Whitespace in an stx template has two origins which mean opposite things. The static parts are markup, so the indentation between their tags is formatting and has to go; that is why the function exists. Everything interpolated with
${}is data, so its whitespace is content and has to stay.We lost the ability to discern between the two when concatenating everything into a single string.
The fix is to no longer write
${}values into the text which is parsed. Instead a processing instruction stands in for it, and each one is replaced by its value's nodes once the tree exists.Alongside this,
stripWhitespacehonoursxml:space(XML 1.0 § 2.10), which is how to keep whitespace written in a template or in a string passed toStanza.toElement, the one case substitution cannot cover. Its<body>exemption is folded into the same predicate and now applies to the element passed in as well as to its children.A value only stands in for nodes where an element encloses it. One interpolated at document level is the stanza itself rather than content inside one, so it is written into the text and parsed as the markup it is, and its whitespace is stripped like a template's.