fix: encode carriage returns so HTML helpers round-trip them#213
Merged
Conversation
html.EscapeString leaves a raw carriage return unescaped, and WriteHTMLInner copied a caller's raw CR into textarea/pre content verbatim. Browser input-stream preprocessing rewrites a raw CR (and CRLF) to LF before tokenization, so the DOM attribute value or textarea/pre content differed from the supplied logical value, breaking the documented leading-CR guarantee and exact-name form bindings. Encode each CR as the numeric character reference in AppendAttrValue and in the newline-sensitive innerHTML path of WriteHTMLInner. The reference is decoded back to CR after preprocessing, so carriage returns survive parsing. Non-newline-sensitive elements still carry their trusted innerHTML verbatim. Add DOM round-trip tests that model browser input-stream preprocessing and character-reference decoding (via html.UnescapeString) alongside exact-output assertions. Fixes #202
Address review of the initial fix. Encoding carriage returns as in WriteHTMLInner was wrong for both newline-sensitive elements: - textarea: HTMLTextAreaElement.value normalizes every CR and CRLF to LF (the HTML standard's textarea value normalization), and jaws.js sends elem.value back to the server, so a carriage return in textarea content can never round-trip regardless of encoding. - pre: pre permits phrasing content such as script and comments, where character references are not decoded, so rewriting every CR corrupts trusted markup (e.g. //x doThing() leaves doThing() commented out). Revert WriteHTMLInner to write innerHTML verbatim and document why carriage returns are not encoded there. The AppendAttrValue encoding stays, as attribute values genuinely round-trip through getAttribute. Replace the flawed DOM round-trip test (which modeled textContent) with one that models HTMLTextAreaElement.value and pins the CR/CRLF-to-LF contract, and add a pre regression case guarding against CR encoding inside nested markup.
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.
Fixes #202
Problem
AppendAttrValuedelegates tohtml.EscapeString, which does not escape a carriage return, so a multiline attribute value read back from the DOM has its\rturned into\n(browser input-stream preprocessing rewrites rawCR/CRLFtoLFbefore tokenization). Encoding theCRas makesgetAttributereport it unchanged.Fix
AppendAttrValuenow encodes each\ras the numeric character reference (via a small sharedappendEscapeCRhelper). The parser decodes it back toCRafter preprocessing, so the value round-trips throughgetAttribute.Attr,AppendAttr,WriteHTMLTag, andWriteHTMLInputinherit the fix.Why
WriteHTMLInner(textarea/pre) is not changedThe issue also asked to encode
CRintextarea/preinner content, but that turns out to be wrong for both:HTMLTextAreaElement.valuenormalizes everyCRandCRLFtoLF(WHATWG textarea value normalization), and jaws.js sendselem.valueback to the server (jaws.js#L157). So a carriage return in textarea content can never round-trip through the value the browser reports, regardless of encoding —<textarea>\n hello</textarea>givestextContent == "\rhello"butvalue == "\nhello".prepermits phrasing content including<script>and comments, where character references are not decoded (script-data / comment parsing). Rewriting everyCRwould corrupt trusted markup, e.g.//x\rdoThing()would become the literal//x doThing(), commenting outdoThing().So
WriteHTMLInnerwritesinnerHTMLverbatim (unchanged behavior); the doc comment now explains the textarea/preCRsemantics and points callers who needCRretention toAppendAttrValue.Tests
attribute encoding (singleCRandCRLF).TestAppendAttrValue_DOMRoundTrip— modelsgetAttribute(input-stream preprocessing + character-reference decoding via stdlibhtml.UnescapeString) and confirms the logical value is recovered.TestWriteHTMLInner_TextareaValueNormalization— modelsHTMLTextAreaElement.valueand pins the true contract: textarea content round-trips withCR/CRLFcollapsed toLF.textarea/precontent is written verbatim, including apre+<script>regression case guarding againstCRencoding inside nested markup.Dependency surface stays stdlib-only. Package coverage remains 100% under
-race;go vet,gofumpt,staticcheck,golangci-lint, andgosecare clean.