Security: validate url scheme for feeds

app/controllers/feeds_controller.rb

@@ -33,25 +33,24 @@ def create
 
       redirect_to("/", flash: { success: t(".success") })
     else
-      flash.now[:error] = feed ? t(".already_subscribed") : t(".feed_not_found")
+      flash.now[:error] = create_error_message(feed)
 
       render(:new)
     end
   end
 
   def update
-    feed = FeedRepository.fetch(params[:id])
-    authorization.check(feed)
+    @feed = FeedRepository.fetch(params[:id])
+    authorization.check(@feed)
 
-    FeedRepository.update_feed(
-      feed,
-      params[:feed_name],
-      params[:feed_url],
-      params[:group_id]
-    )
+    if update_feed
+      flash[:success] = t("feeds.edit.flash.updated_successfully")
+      redirect_to("/feeds")
+    else
+      flash.now[:error] = @feed.error_messages
 
-    flash[:success] = t("feeds.edit.flash.updated_successfully")
-    redirect_to("/feeds")
+      render(:edit)
+    end
   end
 
   def destroy
@@ -61,4 +60,22 @@ def destroy
     flash[:success] = t(".success")
     redirect_to("/feeds")
   end
+
+  private
+
+  def update_feed
+    FeedRepository.update_feed(
+      @feed, params[:feed_name], params[:feed_url], params[:group_id]
+    )
+  end
+
+  def create_error_message(feed)
+    return t("feeds.create.feed_not_found") unless feed
+
+    if feed.errors.of_kind?(:url, :taken)
+      t("feeds.create.already_subscribed")
+    else
+      feed.error_messages
+    end
+  end
 end

app/models/feed.rb

@@ -11,6 +11,10 @@ class Feed < ApplicationRecord
   validates :url, presence: true, uniqueness: { scope: :user_id }
   validates :user_id, presence: true
 
+  # Only on change, so rows stored before this validation existed can still
+  # record status and last_fetched updates.
+  validate :url_scheme_is_allowed, if: :url_changed?
+
   enum :status, { green: 0, yellow: 1, red: 2 }
 
   scope :with_unread_stories_counts,
@@ -37,4 +41,16 @@ def as_fever_json
       last_updated_on_time: last_fetched.to_i
     }
   end
+
+  private
+
+  # Mirrors SafeFetch so we never store a url we would refuse to fetch. This
+  # also keeps `javascript:` out of the feed link rendered on /feeds and out
+  # of the urls handed to Fever clients and OPML exports.
+  def url_scheme_is_allowed
+    return if url.blank?
+    return if SafeFetch::ALLOWED_SCHEMES.include?(SafeFetch.scheme(url))
+
+    errors.add(:url, "must be an http or https address")
+  end
 end

spec/models/feed_spec.rb

@@ -97,4 +97,43 @@
       )
     end
   end
+
+  describe "url scheme validation" do
+    it "allows an http url" do
+      expect(build(:feed, url: "http://example.com/feed")).to be_valid
+    end
+
+    it "allows an https url" do
+      expect(build(:feed, url: "https://example.com/feed")).to be_valid
+    end
+
+    it "rejects a javascript url" do
+      expect(build(:feed, url: "javascript:alert(1)")).not_to be_valid
+    end
+
+    it "explains why a javascript url was rejected" do
+      feed = build(:feed, url: "javascript:alert(1)")
+      feed.valid?
+
+      expect(feed.errors[:url]).to eq(["must be an http or https address"])
+    end
+
+    it "rejects a url with no scheme, which we could never fetch" do
+      expect(build(:feed, url: "example.com/feed")).not_to be_valid
+    end
+
+    it "leaves a blank url to the presence validation alone" do
+      feed = build(:feed, url: "")
+      feed.valid?
+
+      expect(feed.errors[:url]).to eq(["can't be blank"])
+    end
+
+    it "still lets rows stored before the validation record their status" do
+      feed = build(:feed, url: "example.com/legacy")
+      feed.save!(validate: false)
+
+      expect(feed.update(status: :red)).to be(true)
+    end
+  end
 end

spec/requests/feeds_controller_spec.rb

@@ -44,7 +44,7 @@
   describe "#edit" do
     it "displays the feed edit form" do
       login_as(default_user)
-      feed = create(:feed, name: "Rainbows/unicorns", url: "example.com/feed")
+      feed = create(:feed, name: "Rainbows/unicorns", url: "http://example.com/feed")
 
       get "/feeds/#{feed.id}/edit"
 
@@ -65,21 +65,40 @@ def params(feed, **overrides)
 
     it "updates a feed given the id" do
       login_as(default_user)
-      feed = create(:feed, url: "example.com/atom", id: "12", group_id: nil)
+      feed = create(:feed, url: "http://example.com/atom", id: "12")
 
-      feed_url = "example.com/feed"
+      feed_url = "http://example.com/feed"
 
       expect { put("/feeds/#{feed.id}", params: params(feed, feed_url:)) }
         .to change_record(feed, :url).to(feed_url)
     end
 
     it "updates a feed group given the id" do
       login_as(default_user)
-      feed = create(:feed, url: "example.com/atom")
+      feed = create(:feed, url: "http://example.com/atom")
 
       expect { put("/feeds/#{feed.id}", params: params(feed, group_id: 321)) }
         .to change_record(feed, :group_id).to(321)
     end
+
+    it "does not store a url with a disallowed scheme" do
+      login_as(default_user)
+      feed = create(:feed, url: "http://example.com/atom")
+      feed_url = "javascript:alert(document.domain)"
+
+      expect { put("/feeds/#{feed.id}", params: params(feed, feed_url:)) }
+        .not_to change_record(feed, :url)
+    end
+
+    it "reports the failure instead of claiming success" do
+      login_as(default_user)
+      feed = create(:feed, url: "http://example.com/atom")
+      feed_url = "javascript:alert(document.domain)"
+
+      put("/feeds/#{feed.id}", params: params(feed, feed_url:))
+
+      expect(rendered).to have_css(".error")
+    end
   end
 
   describe "#destroy" do
@@ -145,6 +164,35 @@ def with_test_adapter
       end
     end
 
+    context "when the feed document declares a disallowed self url" do
+      feed_url = "http://example.com/"
+      hostile_atom = <<~XML
+        <?xml version="1.0" encoding="utf-8"?>
+        <feed xmlns="http://www.w3.org/2005/Atom">
+          <title>Hostile</title>
+          <link rel="self" href="javascript:alert(document.domain)"/>
+        </feed>
+      XML
+
+      it "does not add the feed" do
+        login_as(default_user)
+        stub_request(:get, feed_url).to_return(status: 200, body: hostile_atom)
+
+        expect { post("/feeds", params: { feed_url: }) }
+          .not_to change(Feed, :count)
+      end
+
+      it "explains the real reason rather than 'already subscribed'" do
+        login_as(default_user)
+        stub_request(:get, feed_url).to_return(status: 200, body: hostile_atom)
+
+        post("/feeds", params: { feed_url: })
+
+        expect(rendered)
+          .to have_css(".error", text: "must be an http or https address")
+      end
+    end
+
     context "when the feed url is one we already subscribe to" do
       feed_url = "http://example.com/"