Sanitize urls to prevent malicious schemes

app/helpers/url_helpers.rb

@@ -19,6 +19,8 @@ def expand_absolute_urls(content, base_url)
     doc.to_html
   end
 
+  ALLOWED_URL_SCHEMES = ["http", "https"].freeze
+
   def normalize_url(url, base_url)
     uri = URI.parse(url.strip)
 
@@ -29,6 +31,16 @@ def normalize_url(url, base_url)
       uri = URI.join("#{scheme}://#{base_uri.host}", uri)
     end
 
+    return if ALLOWED_URL_SCHEMES.exclude?(uri.scheme.downcase)
+
     uri.to_s
   end
+
+  def safe_normalize_url(url, base_url)
+    return if url.blank?
+
+    normalize_url(url, base_url)
+  rescue URI::InvalidURIError
+    nil
+  end
 end

app/javascript/application.ts

@@ -34,6 +34,7 @@ Backbone.ajax = async function(options) {
 };
 
 _.templateSettings = {
+  escape: /\{\{-(.+?)\}\}/g,
   evaluate: /\{\{(.+?)\}\}/g,
   interpolate: /\{\{=(.+?)\}\}/g
 };

app/repositories/story_repository.rb

@@ -4,12 +4,11 @@ class StoryRepository
   extend UrlHelpers
 
   def self.add(entry, feed)
-    enclosure_url = entry.enclosure_url if entry.respond_to?(:enclosure_url)
     Story.create(
       feed:,
       title: extract_title(entry),
       permalink: extract_url(entry, feed),
-      enclosure_url:,
+      enclosure_url: safe_normalize_url(entry.try(:enclosure_url), feed.url),
       body: extract_content(entry),
       is_read: false,
       is_starred: false,
@@ -83,9 +82,8 @@ def self.read_count
   end
 
   def self.extract_url(entry, feed)
-    return normalize_url(entry.url, feed.url) if entry.url.present?
-
-    entry.enclosure_url if entry.respond_to?(:enclosure_url)
+    safe_normalize_url(entry.url, feed.url) ||
+      safe_normalize_url(entry.try(:enclosure_url), feed.url)
   end
 
   def self.extract_content(entry)

app/views/stories/_templates.html.erb

@@ -23,9 +23,9 @@
   <div class="story-body-container">
     <div class="story-body">
       <h1>
-        <a href="{{= permalink }}">{{= title }}</a>
+        <a href="{{- permalink }}">{{= title }}</a>
         {{ if (enclosure_url) { }}
-          <a class="story-enclosure" target="_blank" href="{{= enclosure_url }}">
+          <a class="story-enclosure" target="_blank" href="{{- enclosure_url }}">
             <i class="fa fa-download"></i>
           </a>
         {{ } }}
@@ -45,7 +45,7 @@
         <div class="story-starred" data-action="click->star-toggle#toggle:stop">
           <i class="fa {{ if(is_starred) { }}fa-star{{ } else { }}fa-star-o{{ } }}" data-star-toggle-target="icon"></i>
         </div>
-        <a class="story-permalink" target="_blank" href="{{= permalink }}">
+        <a class="story-permalink" target="_blank" href="{{- permalink }}">
           <i class="fa fa-external-link"></i>
         </a>
       </div>

spec/helpers/url_helpers_spec.rb

@@ -106,5 +106,35 @@ def helper
       )
       expect(url).to eq("https://github.com/progrium/dokku/releases/tag/v0.4.4")
     end
+
+    it "returns nil for javascript: scheme" do
+      url = helper.normalize_url(
+        "javascript:alert(1)", "https://example.com/feed.xml"
+      )
+      expect(url).to be_nil
+    end
+
+    it "returns nil for data: scheme" do
+      url = helper.normalize_url(
+        "data:text/html;base64,PHNjcmlwdD5hbGVydCgxKTwvc2NyaXB0Pg==",
+        "https://example.com/feed.xml"
+      )
+      expect(url).to be_nil
+    end
+
+    it "returns nil for other non-http schemes" do
+      ["vbscript:msgbox(1)", "file:///etc/passwd"].each do |bad|
+        expect(
+          helper.normalize_url(bad, "https://example.com/feed.xml")
+        ).to be_nil
+      end
+    end
+
+    it "rejects case-mangled javascript: scheme" do
+      url = helper.normalize_url(
+        "JaVaScRiPt:alert(1)", "https://example.com/feed.xml"
+      )
+      expect(url).to be_nil
+    end
   end
 end

spec/javascript/setup.ts

@@ -12,6 +12,7 @@ globalThis._ = underscore;
 globalThis.Backbone = Backbone;
 
 _.templateSettings = {
+  escape: /\{\{-(.+?)\}\}/g,
   evaluate: /\{\{(.+?)\}\}/g,
   interpolate: /\{\{=(.+?)\}\}/g,
 };
@@ -53,9 +54,9 @@ const templateHTML = [
   '  <div class="story-body-container">',
   '    <div class="story-body">',
   "      <h1>",
-  '        <a href="{{= permalink }}">{{= title }}</a>',
+  '        <a href="{{- permalink }}">{{= title }}</a>',
   "        {{ if (enclosure_url) { }}",
-  '          <a class="story-enclosure" target="_blank" href="{{= enclosure_url }}">',
+  '          <a class="story-enclosure" target="_blank" href="{{- enclosure_url }}">',
   '            <i class="fa fa-download"></i>',
   "          </a>",
   "        {{ } }}",
@@ -75,7 +76,7 @@ const templateHTML = [
   '        <div class="story-starred" data-action="click->star-toggle#toggle:stop">',
   '          <i class="fa {{ if(is_starred) { }}fa-star{{ } else { }}fa-star-o{{ } }}" data-star-toggle-target="icon"></i>',
   "        </div>",
-  '        <a class="story-permalink" target="_blank" href="{{= permalink }}">',
+  '        <a class="story-permalink" target="_blank" href="{{- permalink }}">',
   '          <i class="fa fa-external-link"></i>',
   "        </a>",
   "      </div>",

spec/javascript/spec/views/story_view_spec.ts

@@ -144,6 +144,30 @@ describe("StoryView", function () {
       assertPropertyRendered(view.el, story, "enclosure_url");
     });
 
+    it("should escape permalink to prevent attribute injection", function () {
+      var injected = new Story({
+        body: "",
+        enclosure_url: null,
+        headline: "x",
+        is_read: false,
+        is_starred: false,
+        keep_unread: false,
+        lead: "",
+        permalink: "\" onclick=\"alert(1)",
+        pretty_date: "",
+        source: "",
+        title: "x",
+      });
+      var injectedView = new StoryView({ model: injected });
+      injectedView.render();
+
+      var anchors = injectedView.el.querySelectorAll("a[href]");
+      anchors.forEach(function (a) {
+        expect(a.getAttribute("onclick")).toBeNull();
+        expect(a.getAttribute("href")).toBe("\" onclick=\"alert(1)");
+      });
+    });
+
     describe("Handling click on story", function () {
       let toggleStub;
 

spec/repositories/story_repository_spec.rb

@@ -49,13 +49,29 @@ def create_feed(url: "http://blog.golang.org/feed.atom")
         summary: "",
         content: ""
       ).as_null_object
-      allow(described_class).to receive(:normalize_url)
+      allow(described_class).to receive(:normalize_url) { |url, _base| url }
 
       expect(Story).to receive(:create).with(hash_including(enclosure_url: "http://example.com/audio.mp3"))
 
       described_class.add(entry, feed)
     end
 
+    it "drops a javascript: enclosure_url" do
+      feed = create_feed
+      entry = instance_double(
+        Feedjira::Parser::ITunesRSSItem,
+        url: "http://example.com/post",
+        enclosure_url: "javascript:alert(1)",
+        title: "",
+        summary: "",
+        content: ""
+      ).as_null_object
+
+      expect(Story).to receive(:create).with(hash_including(enclosure_url: nil))
+
+      described_class.add(entry, feed)
+    end
+
     it "does not set the enclosure url when not present" do
       feed = create_feed
       entry = instance_double(
@@ -429,6 +445,38 @@ def create_feed(url: "http://blog.golang.org/feed.atom")
 
       expect(described_class.extract_url(entry, feed)).to be_nil
     end
+
+    it "drops a javascript: entry url" do
+      feed = double(url: "http://github.com")
+      entry = double(url: "javascript:alert(1)")
+
+      expect(described_class.extract_url(entry, feed)).to be_nil
+    end
+
+    it "falls through to enclosure_url when entry url is javascript:" do
+      feed = double(url: "http://github.com")
+      entry = double(
+        url: "javascript:alert(1)",
+        enclosure_url: "https://example.com/audio.mp3"
+      )
+
+      expect(described_class.extract_url(entry, feed))
+        .to eq("https://example.com/audio.mp3")
+    end
+
+    it "drops a javascript: enclosure_url fallback" do
+      feed = double(url: "http://github.com")
+      entry = double(url: nil, enclosure_url: "javascript:alert(1)")
+
+      expect(described_class.extract_url(entry, feed)).to be_nil
+    end
+
+    it "returns nil for an unparseable enclosure_url fallback" do
+      feed = double(url: "http://github.com")
+      entry = double(url: nil, enclosure_url: "http://[invalid")
+
+      expect(described_class.extract_url(entry, feed)).to be_nil
+    end
   end
 
   describe ".extract_title" do