diff --git a/app/helpers/url_helpers.rb b/app/helpers/url_helpers.rb index a17ee89cd..8c2a576c1 100644 --- a/app/helpers/url_helpers.rb +++ b/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 diff --git a/app/javascript/application.ts b/app/javascript/application.ts index 3f2c20d83..7e548a771 100644 --- a/app/javascript/application.ts +++ b/app/javascript/application.ts @@ -34,6 +34,7 @@ Backbone.ajax = async function(options) { }; _.templateSettings = { + escape: /\{\{-(.+?)\}\}/g, evaluate: /\{\{(.+?)\}\}/g, interpolate: /\{\{=(.+?)\}\}/g }; diff --git a/app/repositories/story_repository.rb b/app/repositories/story_repository.rb index b9eda7efb..c1096ac8b 100644 --- a/app/repositories/story_repository.rb +++ b/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) diff --git a/app/views/stories/_templates.html.erb b/app/views/stories/_templates.html.erb index a7df7bb07..39a727c58 100644 --- a/app/views/stories/_templates.html.erb +++ b/app/views/stories/_templates.html.erb @@ -23,9 +23,9 @@

- {{= title }} + {{= title }} {{ if (enclosure_url) { }} - + {{ } }} @@ -45,7 +45,7 @@
- +

diff --git a/spec/helpers/url_helpers_spec.rb b/spec/helpers/url_helpers_spec.rb index 1ba7126d0..944a09211 100644 --- a/spec/helpers/url_helpers_spec.rb +++ b/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 diff --git a/spec/javascript/setup.ts b/spec/javascript/setup.ts index 52a3e1cbb..58ed016d7 100644 --- a/spec/javascript/setup.ts +++ b/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 = [ '
', '
', "

", - ' {{= title }}', + ' {{= title }}', " {{ if (enclosure_url) { }}", - ' ', + ' ', ' ', " ", " {{ } }}", @@ -75,7 +76,7 @@ const templateHTML = [ '
', ' ', "
", - ' ', + ' ', ' ', " ", "

", diff --git a/spec/javascript/spec/views/story_view_spec.ts b/spec/javascript/spec/views/story_view_spec.ts index fed627d4e..0d5c4ec9e 100644 --- a/spec/javascript/spec/views/story_view_spec.ts +++ b/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; diff --git a/spec/repositories/story_repository_spec.rb b/spec/repositories/story_repository_spec.rb index c444da0af..2d47c2bc4 100644 --- a/spec/repositories/story_repository_spec.rb +++ b/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