From 9b50de45696312b2b356dad00589af0bf0fcfacd Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Wed, 17 Jan 2024 11:30:27 +0000
Subject: [PATCH] SECURITY: Properly escape user content within `<noscript>`

---
 app/helpers/application_helper.rb      |  4 +++
 app/views/layouts/application.html.erb | 16 +++++----
 spec/requests/noscript_escape_spec.rb  | 49 ++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 7 deletions(-)
 create mode 100644 spec/requests/noscript_escape_spec.rb

diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index b644a289475..7193ead23d0 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -738,6 +738,10 @@ module ApplicationHelper
     absolute_url
   end
 
+  def escape_noscript(&block)
+    raw capture(&block).gsub(%r{<(/\s*noscript)}i, '&lt;\1')
+  end
+
   def manifest_url
     # If you want the `manifest_url` to be different for a specific action,
     # in the action set @manifest_url = X. Originally added for chat to add a
diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb
index 6777cea56c4..66dc243e230 100644
--- a/app/views/layouts/application.html.erb
+++ b/app/views/layouts/application.html.erb
@@ -99,15 +99,17 @@
 
     <%= render_google_tag_manager_body_code %>
     <noscript data-path="<%= request.env['PATH_INFO'] %>">
-      <%= render partial: "layouts/noscript_header" %>
+      <%= escape_noscript do %>
+        <%= render partial: "layouts/noscript_header" %>
 
-      <div id="main-outlet" class="wrap" role="main">
-        <!-- preload-content: -->
-        <%= yield %>
-        <!-- :preload-content -->
-      </div>
+        <div id="main-outlet" class="wrap" role="main">
+          <!-- preload-content: -->
+          <%= yield %>
+          <!-- :preload-content -->
+        </div>
 
-      <%= render partial: "layouts/noscript_footer" %>
+        <%= render partial: "layouts/noscript_footer" %>
+      <% end %>
     </noscript>
 
     <%- unless customization_disabled? %>
diff --git a/spec/requests/noscript_escape_spec.rb b/spec/requests/noscript_escape_spec.rb
new file mode 100644
index 00000000000..b76eff22840
--- /dev/null
+++ b/spec/requests/noscript_escape_spec.rb
@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+RSpec.describe "escaping of noscript content" do
+  def noscript_content
+    # Browsers do not parse the contents of noscript tags - they just look for the next string matching `</noscript>`
+    # Can't use nokogiri because it parses documents with the 'scripting flag' disabled, and therefore parses html inside noscript tags
+    noscript_content = response.body.scan(%r{<noscript.*?>(.*?)</noscript>}m).join("\n")
+  end
+
+  it "does not affect normal content" do
+    post = Fabricate(:post, raw: 'This is a post with an image <img alt="<Look at this!>">')
+    get post.url
+
+    expect(noscript_content).to include('<img alt="<Look at this!>">')
+  end
+
+  it "escapes noscript in attribute" do
+    post =
+      Fabricate(
+        :post,
+        raw: 'This is a post with an image <img alt="</noscript>"> containing a noscript end tag',
+      )
+    get post.url
+
+    expect(noscript_content).to include('<img alt="&lt;/noscript>">')
+  end
+
+  it "escapes noscript with trailing whitespace" do
+    post =
+      Fabricate(
+        :post,
+        raw: 'This is a post with an image <img alt="</noscript  >"> containing a noscript end tag',
+      )
+    get post.url
+
+    expect(noscript_content).to include('<img alt="&lt;/noscript  >">')
+  end
+
+  it "escapes noscript with leading whitespace" do
+    # The spec doesn't accept closing tags with leading whitespace. Browsers follow that, but some other parsers are more relaxed so we escape anyway
+    post =
+      Fabricate(
+        :post,
+        raw: 'This is a post with an image <img alt="</  noscript>"> containing a noscript end tag',
+      )
+    get post.url
+
+    expect(noscript_content).to include('<img alt="&lt;/  noscript>">')
+  end
+end