diff --git a/app/controllers/feeds_controller.rb b/app/controllers/feeds_controller.rb index 5c4805d11..6e98798ff 100644 --- a/app/controllers/feeds_controller.rb +++ b/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 diff --git a/app/models/feed.rb b/app/models/feed.rb index 17ffca406..33682bcad 100644 --- a/app/models/feed.rb +++ b/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 diff --git a/spec/models/feed_spec.rb b/spec/models/feed_spec.rb index a78f05c25..0cb08ac88 100644 --- a/spec/models/feed_spec.rb +++ b/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 diff --git a/spec/requests/feeds_controller_spec.rb b/spec/requests/feeds_controller_spec.rb index 671395ee7..54b5ce561 100644 --- a/spec/requests/feeds_controller_spec.rb +++ b/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,9 +65,9 @@ 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) @@ -75,11 +75,30 @@ def params(feed, **overrides) 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 + + + Hostile + + + 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/"