fix: stop double-escaping what a provider sends back - #24
Merged
Conversation
Google and DeepL HTML-escape every reply, so an apostrophe, quote or ampersand in the translation came back as ', " or &. Our own render step re-escaped the leading & of each, corrupting the page with visible junk like '. Response.build is where every provider's texts already converge to be shape-checked, so it decodes them there too -- symmetric with the existing input-side decode, and automatic for a third-party provider without a six-way per-provider hook. Verified live against both vendors: apostrophes, quotes and ampersands now round-trip correctly, and a notranslate span with an entity is unaffected. Cache entries written before this fix keep the doubled text until they expire; bump cache_namespace or let cache_ttl lapse to clear them.
Decoding a provider's entities (previous commit) exposed a second bug: Passage renders every fragment then runs one restore pass over the whole string to undo the escaping it applied to the source's own bare angles. A translated `<` looked exactly like that source-level escaping to that pass, so it got "restored" to a bare `<` -- and a bare `<` in front of a letter reads as an opening tag. A provider's own `<b attack` came back as a real `<b attack>` element. Response.build now escapes a provider's raw text the same way the input path escapes @Body before decoding it, so an entity the provider genuinely sent survives as the entity it is rather than the character it decodes to. Segment#render pairs this with a new Markup.encode_translation: it re-escapes a translation the same way encode_entities does, except it leaves a `<` shaped like a tag alone, trusting a provider's reproduced markup (a notranslate span, say) the same way a source tag already is. Verified live against both vendors: the apostrophe/quote fix from the previous commit still holds, and "5 < 7 && 7 > 5" now comes back with its entities intact rather than as bare `<`/`&`.
Covers what fix/provider-entity-decoding changed for users: the double-escaping corruption is gone, a translated bare < now renders as < (a behaviour change), and a warm cache keeps serving the old corrupted text until it expires or cache_namespace changes. Also notes that <pre>/<code> blocks are ordinary prose to this gem and are translated unless wrapped in class="notranslate", measured against the live Google API.
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.
Found by translating a real HTML article through the live Google and DeepL
APIs — not by a test, and not by three rounds of review before it.
What was broken
Every translation into English through Google or DeepL came back corrupted
wherever it contained an apostrophe, a quote or an ampersand:
which renders on the page as
didn't. English is full of apostrophes,so in practice every Google or DeepL translation into English was damaged
somewhere.
Both vendors return entity-escaped HTML — confirmed against the raw APIs,
not inferred. The pipeline decoded entities on the way in, so a provider is
sent characters rather than entities, and re-encoded
&and<at render— but never decoded what the provider sent back. The vendor's
&wastreated as a literal ampersand and escaped a second time.
The fix decodes a provider's returned text with the same decoder the input
path uses, in
Translation::Response.build— the one place every provider'soutput passes through, including the two that override
translateentirely,and including a provider somebody writes themselves.
The hole that opened, and closed
Decoding the vendor's
<produced a bare<, and a bare<in front ofa letter is a tag. A provider returning
<b attackput a real<bintothe rendered document:
The second commit closes it: a translated
<that is not shaped like a tagis escaped on render, so a provider's markup survives and a provider's text
cannot become markup. The test that would have caught it is now there — the
tag count and tag sequence of a real HTML article must be identical before
and after a round trip through a vendor that escapes.
What changes for a user
A literal
<in the source now comes back as<rather than bare.if a < b then stop.becomesif a < b then stop., which is the correctHTML encoding of that character and renders identically. Byte-for-byte
comparison against a previous version will show it.
>is left alone; it isunambiguous.
Warm caches keep the old text. A cache key is derived from the source
sentence, not from the stored value, so entries written before this fix keep
being served until they expire. Bump
cache_namespace, or letcache_ttllapse.
Verified live
Both vendors,
ru→en, through the gem's public API:A 68-tag article — lists, a table, a blockquote, a figure, an HTML comment,
an attribute carrying
&, anotranslatespan — round-trips with thetag sequence identical, the span untouched, the link's attributes intact and
its text translated.
Also documented, because the same article exposed it: nothing tells the gem
that
<pre>and<code>are code, so their contents are translated likeprose.
class="notranslate"is the remedy.684 runs, rubocop clean.