fix: stop double-escaping what a provider sends back

CHANGELOG.md

@@ -141,6 +141,41 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
   each other -- see
   [The three write paths fail differently](docs/caching.md#the-three-write-paths-fail-differently).
 
+### Fixed
+
+- **Google and DeepL translations in HTML mode no longer come back
+  double-escaped.** Both vendors return entity-escaped text -- an
+  apostrophe as `'`, a quote as `"`, an ampersand as `&` -- and
+  the pipeline decoded entities on the way in but never on the way out, so
+  the renderer escaped the vendor's own `&` a second time and a reader saw
+  `didn't` on the page. English is full of apostrophes, so in practice
+  every Google or DeepL translation into English was affected somewhere.
+  `TranslationDiff::Translation::Response.build` now decodes a provider's
+  reply the same way it already decoded the source, symmetrically, for
+  every provider -- named entities, and both the decimal (`'`) and hex
+  (`'`) numeric forms, are decoded; an entity neither decoder
+  recognizes is left exactly as it arrived. See [How it
+  works](docs/how-it-works.md).
+- **Behaviour change: a literal `<` in a source sentence now renders as
+  `&lt;`.** Decoding the fix above exposed a second bug: a provider's own
+  `&lt;` now decoded to a bare `<`, and a bare `<` in front of a letter
+  reads as an opening tag -- a provider could inject markup into the
+  rendered document. A translated `<` that is not shaped like a tag is now
+  escaped on render instead. `if a < b then stop.` used to come back with
+  the bare `<` exactly as written; it now comes back
+  `if a &lt; b then stop.`, the correct HTML encoding of that character and
+  identical once a browser renders it -- but visible to anything comparing
+  output byte-for-byte against an earlier release. `>` is untouched: a
+  stray `>` never opens anything a parser would honour. See [How it
+  works](docs/how-it-works.md#html).
+- **A warm cache keeps serving the corrupted text after you upgrade.** A
+  cache entry's key is derived from the source sentence, not from the value
+  stored under it, so an entry written before this fix is served exactly as
+  it was written until it expires -- upgrading alone does not clear it.
+  Give the configuration a new `cache_namespace`, or let `cache_ttl` lapse,
+  to force every sentence to be retranslated under the fix. See
+  [Caching](docs/caching.md).
+
 ### Security
 
 - `Configuration#inspect` and `Provider#inspect` print `[FILTERED]` in place

docs/caching.md

@@ -17,6 +17,15 @@ its own Redis database). The key format is left alone here on purpose:
 changing its shape invalidates every entry already cached, everywhere, at
 once.
 
+**A cache entry written before a bug fix keeps serving what the bug
+produced.** The key above is built from the source sentence, never from the
+value stored under it, so fixing what a provider's reply decodes to does not
+invalidate what is already cached -- an entry written under the HTML-entity
+double-escaping fixed in the Unreleased CHANGELOG entry is served exactly as
+it was written until it expires. Give the configuration a new
+`cache_namespace`, or let `cache_ttl` lapse, to force every sentence to be
+retranslated under the fix.
+
 Both read and write the same cache, keyed per provider, so switching one
 never serves you the other's translations.
 

docs/how-it-works.md

@@ -59,9 +59,29 @@ Everything below is a collaborator one of the two drives.
    shape.
 
 `TranslationDiff::Markup` is the small module underneath steps 2, 3 and 6: it
-decodes entity references on the way to a provider, encodes `&` and `<` again
-on the way out, and escapes a `<` that opens no tag so `ox` cannot read the
-rest of the sentence as markup.
+decodes entity references on the way to a provider and, in
+`TranslationDiff::Translation::Response.build`, on the way back too, for
+every provider -- Google and DeepL both return HTML-escaped text, and
+without the second decode a vendor's own `&` was escaped a second time, so
+`didn't` came back as `didn&#39;t`. Named entities, and both the decimal
+(`&#39;`) and hex (`&#x27;`) numeric forms, are decoded; an entity neither
+decoder recognizes, or one that would decode to invalid UTF-8, is left
+exactly as it arrived.
+
+Decoding a reply raw would make `&lt;` a bare `<`, and `ox` reads a bare `<`
+in front of a letter as an opening tag -- a provider's own `&lt;b attack`
+would become a real `<b attack>` element. So a reply is escaped the same way
+a source document's own bare angles already are, before it is decoded, and
+`Segment#render` re-encodes a translated sentence with
+`Markup.encode_translation`: `&` is always escaped, and so is a `<` that is
+not shaped like a tag -- a source document's own bare `<` is untouched by
+this. **This is a behaviour change:** `if a < b then stop.` used to come
+back with the bare `<` exactly as written; it now comes back
+`if a &lt; b then stop.`, the correct HTML encoding of that character,
+rendering identically in a browser but visible to anything comparing output
+byte-for-byte against an earlier release. `>` is left alone -- a stray `>`
+never opens anything a parser would honour, so there is nothing to protect
+it from.
 
 *NOTE:* if `:from` is not specified or equal to nil, then the provider's `#detect` will be called once with a sample of text up to 100 characters long to determine the language, and `#translate` will be called separately with the entire text.
         Try to specify `:from` explicitly to save the extra call -- it also improves segmentation, since the segmenter only sees a language when `:from` is given (see [The segmenter contract](contracts.md#the-segmenter-contract)).
@@ -93,3 +113,23 @@ You can pass HTML as like as plain text:
 ```ruby
 TranslationDiff.translate("<b>Black</b>", from: "en", to: "es")
 ```
+
+Nothing marks a `<pre>` or `<code>` block as code. The scanner's `OPAQUE`
+list (see [The steps](#the-steps) above) excludes only `<script>` and
+`<style>`, so a code sample sitting inside `<pre>`/`<code>` is ordinary
+prose to this gem -- cut into sentences and sent to the provider like any
+paragraph. Measured against the live Google API:
+
+```ruby
+TranslationDiff.translate(
+  "<pre><code>curl -s https://example.com/level | jq '.meters'</code></pre>",
+  from: "en", to: "es"
+)
+# => "<pre><code>curl -s https://example.com/level | jq '.metros'</code></pre>"
+```
+
+`meters` came back translated to `metros`, inside the quoted `jq` filter --
+the segmenter cut the block at the quote and handed `meters'` to the
+provider as a sentence of its own. Wrap a block you don't want touched in
+`class="notranslate"`; the providers that honour it (see
+[Providers](providers.md)) leave it exactly as written.

lib/translation_diff/markup.rb

@@ -67,6 +67,15 @@ def self.resolve(name)
   # What a document renders is markup, so text that changed is made safe again -- and only where it is unsafe.
   def self.encode_entities(text) = text.gsub(ENCODABLE, ENCODED)
 
+  # An entity the round trip already produced must not be escaped a second time, and a `<` shaped like a tag is
+  # trusted the same way a source tag already is -- everything else a provider sent back is untrusted new text.
+  TRANSLATED_ENCODABLE = /&(?:amp|lt|gt);|&|<(?!#{TAG_OPENER})/
+
+  # What a translation renders as: unlike #encode_entities, this leaves a provider's own reproduced tags alone.
+  def self.encode_translation(text)
+    text.gsub(TRANSLATED_ENCODABLE) { |match| match.length == 1 ? ENCODED[match] : match }
+  end
+
   # Ox hands back the decoded text of the one element it was given; a name it does not know arrives as the text it was.
   class Resolver < Ox::Sax
     attr_reader :text

lib/translation_diff/segment.rb

@@ -19,6 +19,14 @@ def empty? = core.match?(BLANK)
 
   # Untranslated hands back the bytes it was cut from; a translation is text, so it is encoded as markup on the way out.
   def render
-    "#{@leading}#{translated? ? TranslationDiff::Markup.encode_entities(translation) : @body}#{@trailing}"
+    "#{@leading}#{translated? ? escaped_translation : @body}#{@trailing}"
+  end
+
+  private
+
+  # @body already carries this same escape from Passage; without it, Passage's one shared restore pass would
+  # read a translated `&lt;` as a source document's own bare `<` and hand back markup nobody asked for.
+  def escaped_translation
+    TranslationDiff::Markup.escape_bare_angles(TranslationDiff::Markup.encode_translation(translation))
   end
 end

lib/translation_diff/translation/response.rb

@@ -5,7 +5,13 @@ def self.build(request:, texts:, detected_source: nil, usage: nil)
       ensure_count!(request, texts)
       ensure_strings!(texts)
 
-      new(texts: texts, detected_source: detected_source, usage: usage)
+      # Every provider's text lands here, so it takes the same path @core did: escaped, then decoded once, so an
+      # entity a provider genuinely sent survives as the entity it is rather than the bare character it decodes to.
+      new(texts: texts.map { |text| decoded(text) }, detected_source: detected_source, usage: usage)
+    end
+
+    def self.decoded(text)
+      TranslationDiff::Markup.decode_entities(TranslationDiff::Markup.escape_bare_angles(text))
     end
 
     def self.ensure_count!(request, texts)

test/translation_diff/markup_test.rb

@@ -20,6 +20,15 @@ def echoed(source)
     subject.render
   end
 
+  # What Google and DeepL do: html-escape whatever text they hand back, then what Response.build now undoes.
+  def vendor_escaped(source)
+    subject = passage(source)
+    subject.segments.reject(&:empty?).each do |s|
+      s.translation = TranslationDiff::Markup.decode_entities(CGI.escapeHTML(s.core))
+    end
+    subject.render
+  end
+
   def assert_round_trips(source)
     assert_equal source, passage(source).render, "render must return the source byte for byte"
   end
@@ -66,7 +75,7 @@ def test_a_bare_less_than_survives_rendering_untranslated
   # the same string also contains a bare <.
   def test_a_real_tag_beside_a_bare_less_than_is_still_markup
     assert_equal ["if a < b then", "stop."], cores("if a < b then <b>stop.</b>")
-    assert_equal "IF A < B THEN <b>STOP.</b>", translated("if a < b then <b>stop.</b>")
+    assert_equal "IF A &lt; B THEN <b>STOP.</b>", translated("if a < b then <b>stop.</b>")
   end
 
   def test_a_less_than_immediately_before_a_letter_is_a_tag
@@ -191,4 +200,28 @@ def test_an_entity_inside_markup_is_left_for_the_browser
     assert_round_trips(%(<a href="/x?a=1&b=2" title='q'>Link text.</a> After.))
     assert_round_trips("Before.<![CDATA[raw & unparsed]]>After.")
   end
+
+  # -- a vendor's own escaping -----------------------------------------------
+
+  # Reproduces the shipped bug: Google and DeepL html-escape every reply, apostrophes and quotes included.
+  def test_a_vendor_that_escapes_apostrophes_and_quotes_round_trips_clean
+    assert_equal "He didn't take the boat.", vendor_escaped("He didn't take the boat.")
+    assert_equal %(She said, "We're not ready."), vendor_escaped(%(She said, "We're not ready."))
+  end
+
+  # An ampersand a vendor escaped is undone once, then re-escaped once at render -- never doubled either way.
+  def test_a_vendor_escaped_ampersand_is_not_doubled
+    assert_equal "5 &amp; 7 are important.", vendor_escaped("5 & 7 are important.")
+  end
+
+  # The notranslate span's own text is sent and returned like any other sentence, entity and all.
+  def test_a_vendor_escaped_notranslate_span_keeps_its_ampersand_readable
+    assert_equal %(<span class="notranslate">R&amp;D</span> Fine.),
+                 vendor_escaped(%(<span class="notranslate">R&D</span> Fine.))
+  end
+
+  # One decode pass, never two: a reply already doubly-escaped loses only the level the wire itself added.
+  def test_decoding_a_double_encoded_reply_removes_only_one_level
+    assert_equal "&amp;", TranslationDiff::Markup.decode_entities("&amp;amp;")
+  end
 end

test/translation_diff/pipeline_corpus_test.rb

@@ -60,15 +60,16 @@ def self.method_name_for(name) = :"test_#{name.gsub(/[^a-zA-Z0-9]+/, '_')}"
       document: "Hard\u00A0space here. Fine.",
       echoed: "Hard\u00A0space here. Fine."
     },
+    # A translated `<` not shaped like a tag is escaped now, so it can never read back as one after this fix.
     "bare less-than" => {
       texts: ["if a < b then stop.", "Fine."],
-      document: "if a < b then stop. Fine.",
-      echoed: "if a < b then stop. Fine."
+      document: "if a &lt; b then stop. Fine.",
+      echoed: "if a &lt; b then stop. Fine."
     },
     "bare less-than and greater" => {
       texts: ["5 < 6 and 7 > 6.", "True."],
-      document: "5 < 6 and 7 > 6. True.",
-      echoed: "5 < 6 and 7 > 6. True."
+      document: "5 &lt; 6 and 7 > 6. True.",
+      echoed: "5 &lt; 6 and 7 > 6. True."
     },
     # The recorded limit: `<b` is read as a tag, so the sentence after it is markup and never reaches a provider.
     "bare less-than before a letter" => {

test/translation_diff/provider_entity_decoding_test.rb

@@ -0,0 +1,93 @@
+require "test_helper"
+
+# Round 2 of the entity-decoding fix: a provider's own `&lt;` must never reach the page as a bare, tag-forming `<`.
+class ProviderEntityDecodingTest < ConfiguredTest
+  # What a real vendor does: escape a literal `&` and a `<` that opens no tag; a genuine tag is left alone.
+  def self.vendor_escape(text)
+    text.gsub(/&|<(?!#{TranslationDiff::Markup::TAG_OPENER})/) { |match| match == "&" ? "&amp;" : "&lt;" }
+  end
+
+  # Mimics Google/DeepL: html-aware on the way in too, so an `&lt;` sent as safe HTML is read as the character
+  # it means, not retranslated as four literal letters, before the reply is escaped the way a real vendor's is.
+  class EscapingProvider < TranslationDiff::Provider
+    def self.capabilities
+      TranslationDiff::Capabilities.new(
+        max_request_size: 1_000, max_batch_size: 10, max_text_size: nil,
+        html: :format, notranslate: true, detects_language: false, reports_billing: false
+      )
+    end
+
+    def translate(request)
+      texts = request.texts.map { |text| ProviderEntityDecodingTest.vendor_escape(TranslationDiff::Markup.decode_entities(text)) }
+      TranslationDiff::Translation::Response.build(request: request, texts: texts)
+    end
+
+    def cache_key = "escaping"
+  end
+
+  # A provider that ignores what it is sent and returns a fixed string -- for reproducing the injection directly.
+  class FixedProvider < TranslationDiff::Provider
+    def self.capabilities = EscapingProvider.capabilities
+
+    def initialize(config, text:)
+      super(config)
+      @text = text
+    end
+
+    def translate(request)
+      TranslationDiff::Translation::Response.build(request: request, texts: request.texts.map { @text })
+    end
+
+    def cache_key = "fixed"
+  end
+
+  def tags_in(html) = html.scan(%r{</?[A-Za-z][^>]*>})
+
+  def translate(source, provider)
+    TranslationDiff.translate(source, from: "ru", to: "en", provider: provider)
+  end
+
+  # The shipped hole: a provider's own `&lt;b attack` must never become a real, page-breaking `<b attack>` tag.
+  def test_a_dangerous_tag_shaped_entity_never_forms_a_real_tag
+    provider = FixedProvider.new(TranslationDiff::Configuration.new, text: "Value &lt;b attack here.")
+    output = translate("<p>Value X here.</p>", provider)
+
+    assert_includes output, "&lt;b attack"
+    assert_equal ["<p>", "</p>"], tags_in(output)
+  end
+
+  # DeepL/Google's own reproduction case: `&lt;` and `&amp;` come back as entities -- not bare, not doubled.
+  # `>` was never escaped by this gem, translated or not, so it stays literal; only `<` and `&` are at stake.
+  def test_a_vendor_that_escapes_preserves_comparison_entities
+    provider = EscapingProvider.new(TranslationDiff::Configuration.new)
+    output = translate("<p>Сравните: 5 &lt; 7 &amp;&amp; 7 &gt; 5.</p>", provider)
+
+    assert_equal "<p>Сравните: 5 &lt; 7 &amp;&amp; 7 > 5.</p>", output
+  end
+
+  # The apostrophe/quote corruption this branch already fixed must stay fixed alongside the new tag protection.
+  def test_apostrophe_and_quote_corruption_stays_fixed
+    provider = EscapingProvider.new(TranslationDiff::Configuration.new)
+
+    assert_equal "<p>He didn't take the boat away.</p>", translate("<p>He didn't take the boat away.</p>", provider)
+    assert_equal "<p>5 &amp; 7 are important.</p>", translate("<p>5 & 7 are important.</p>", provider)
+  end
+
+  # The property that would have caught this: a real article's tags, count and sequence, survive the round trip.
+  ARTICLE = <<~HTML.chomp
+    <article>
+    <h1>Guide to Shell Quoting</h1>
+    <p>Compare: 5 &lt; 7 and check the &amp; operator carefully.</p>
+    <p>Run <code>jq &#39;.meters&#39;</code> to extract the field.</p>
+    <p><a href="/x?a=1&amp;b=2" title="A &amp; B">Read more</a> about the topic.</p>
+    <p>The vendor <span class="notranslate">TrustedCo</span> provided this data.</p>
+    </article>
+  HTML
+
+  def test_a_real_articles_tag_count_and_sequence_survive_a_round_trip
+    provider = EscapingProvider.new(TranslationDiff::Configuration.new)
+    output = translate(ARTICLE, provider)
+
+    assert_equal tags_in(ARTICLE), tags_in(output)
+  end
+end

test/translation_diff/providers/null_test.rb

@@ -14,6 +14,14 @@ def test_translate_returns_the_input_unchanged
     assert_equal %w[one two], provider.translate(request).texts
   end
 
+  # Response.build's new decoding step must be a no-op for the one provider that never escapes anything.
+  def test_translate_does_not_touch_text_that_only_looks_like_it_needs_decoding
+    texts = ["AT&T merged.", "5 < 7 is true.", "He didn't go."]
+    request = TranslationDiff::Translation::Request.new(texts: texts, from: :en, to: :ru)
+
+    assert_equal texts, provider.translate(request).texts
+  end
+
   def test_cache_key_is_null
     assert_equal "null", provider.cache_key
   end

test/translation_diff/translation/response_test.rb

@@ -15,6 +15,43 @@ def test_build_returns_the_texts_it_was_given
     assert_equal %w[один два], response.texts
   end
 
+  # -- decoding what a provider sent back -----------------------------------
+
+  # Google and DeepL both HTML-escape their output; undoing that here is what made the round trip symmetric.
+  def test_build_decodes_the_apostrophe_and_quote_a_provider_escaped
+    response = TranslationDiff::Translation::Response.build(
+      request: request, texts: ["He didn't say "hi".", "5 & 7."]
+    )
+
+    assert_equal ["He didn't say \"hi\".", "5 & 7."], response.texts
+  end
+
+  # Numeric, hex and named entities all decode; an entity outside the known set is left exactly as it arrived.
+  def test_build_decodes_numeric_hex_and_named_entities_and_leaves_an_unknown_one_alone
+    response = TranslationDiff::Translation::Response.build(
+      request: request(%w[one]), texts: ["' ' & "   — &nosuch;"]
+    )
+
+    assert_equal ["' ' & \" \u00A0 \u2014 &nosuch;"], response.texts
+  end
+
+  # A provider that never escapes its output -- the :null provider, or any other -- must not have a character
+  # that merely looks like the start of an entity eaten; only a well-formed entity is ever touched.
+  def test_build_leaves_a_non_escaping_providers_output_untouched
+    response = TranslationDiff::Translation::Response.build(
+      request: request, texts: ["AT&T merged.", "5 < 7 is true."]
+    )
+
+    assert_equal ["AT&T merged.", "5 < 7 is true."], response.texts
+  end
+
+  # One decode pass, never two -- a doubly-escaped reply loses only the level the wire itself added.
+  def test_build_decodes_a_double_escaped_reply_only_once
+    response = TranslationDiff::Translation::Response.build(request: request(%w[one]), texts: ["&amp;amp;"])
+
+    assert_equal ["&amp;"], response.texts
+  end
+
   # A short response would shift nils into the results, surfacing much later as a distant NoMethodError.
   def test_build_raises_when_the_provider_returned_the_wrong_number_of_texts
     error = assert_raises(TranslationDiff::ResponseError) do