From 5d062206dbf9c55d7914b90a2b69b710f07ed992 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Thu, 28 Jul 2016 13:54:17 -0400 Subject: [PATCH] SECURITY: Make sure uploaded_urls have corresponding upload records --- app/models/category.rb | 3 +++ app/models/user_profile.rb | 3 +++ lib/discourse.rb | 4 ++++ lib/validators/upload_url_validator.rb | 11 +++++++++++ spec/components/discourse_spec.rb | 16 ++++++++-------- spec/models/category_spec.rb | 18 ++++++++++++++++++ spec/models/user_profile_spec.rb | 17 +++++++++++++++++ 7 files changed, 64 insertions(+), 8 deletions(-) create mode 100644 lib/validators/upload_url_validator.rb diff --git a/app/models/category.rb b/app/models/category.rb index e49291920c6..5e314727133 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -35,6 +35,9 @@ class Category < ActiveRecord::Base validate :email_in_validator + validates :logo_url, upload_url: true + validates :background_url, upload_url: true + validate :ensure_slug before_save :apply_permissions before_save :downcase_email diff --git a/app/models/user_profile.rb b/app/models/user_profile.rb index 7137290326c..6a173d655e8 100644 --- a/app/models/user_profile.rb +++ b/app/models/user_profile.rb @@ -9,6 +9,9 @@ class UserProfile < ActiveRecord::Base before_save :cook after_save :trigger_badges + validates :profile_background, upload_url: true + validates :card_background, upload_url: true + belongs_to :card_image_badge, class_name: 'Badge' has_many :user_profile_views, dependent: :destroy diff --git a/lib/discourse.rb b/lib/discourse.rb index 4d08038deec..8e7d6303c9f 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -194,6 +194,10 @@ module Discourse end end + def self.base_protocol + SiteSetting.force_https? ? "https" : "http" + end + def self.base_url_no_prefix default_port = 80 protocol = "http" diff --git a/lib/validators/upload_url_validator.rb b/lib/validators/upload_url_validator.rb new file mode 100644 index 00000000000..527ad2d4e97 --- /dev/null +++ b/lib/validators/upload_url_validator.rb @@ -0,0 +1,11 @@ +class UploadUrlValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + if value.present? + uri = URI.parse(value) rescue nil + + unless uri && Upload.exists?(url: value) + record.errors[attribute] << (options[:message] || I18n.t('errors.messages.invalid')) + end + end + end +end diff --git a/spec/components/discourse_spec.rb b/spec/components/discourse_spec.rb index b8b108519e0..34035330ca0 100644 --- a/spec/components/discourse_spec.rb +++ b/spec/components/discourse_spec.rb @@ -18,7 +18,7 @@ describe Discourse do context 'base_url' do context 'when https is off' do before do - SiteSetting.expects(:use_https?).returns(false) + SiteSetting.use_https = false end it 'has a non https base url' do @@ -28,7 +28,7 @@ describe Discourse do context 'when https is on' do before do - SiteSetting.expects(:use_https?).returns(true) + SiteSetting.use_https = true end it 'has a non-ssl base url' do @@ -38,7 +38,7 @@ describe Discourse do context 'with a non standard port specified' do before do - SiteSetting.stubs(:port).returns(3000) + SiteSetting.port = 3000 end it "returns the non standart port in the base url" do @@ -63,7 +63,7 @@ describe Discourse do end it 'returns the system user otherwise' do - SiteSetting.stubs(:site_contact_username).returns(nil) + SiteSetting.site_contact_username = nil expect(Discourse.site_contact_user.username).to eq("system") end @@ -76,10 +76,10 @@ describe Discourse do end it "returns S3Store when S3 is enabled" do - SiteSetting.stubs(:enable_s3_uploads?).returns(true) - SiteSetting.stubs(:s3_upload_bucket).returns("s3_bucket") - SiteSetting.stubs(:s3_access_key_id).returns("s3_access_key_id") - SiteSetting.stubs(:s3_secret_access_key).returns("s3_secret_access_key") + SiteSetting.enable_s3_uploads = true + SiteSetting.s3_upload_bucket = "s3bucket" + SiteSetting.s3_access_key_id = "s3_access_key_id" + SiteSetting.s3_secret_access_key = "s3_secret_access_key" expect(Discourse.store).to be_a(FileStore::S3Store) end diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index 8fde29a12d9..7a38bf03bf1 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -12,6 +12,24 @@ describe Category do is_expected.to validate_uniqueness_of(:name).scoped_to(:parent_category_id) end + context "url validation" do + let(:user) { Fabricate(:user) } + + let(:upload) { Fabricate(:upload) } + + it "ensures logo_url is valid" do + expect(Fabricate.build(:category, user: user, logo_url: "---%")).not_to be_valid + expect(Fabricate.build(:category, user: user, logo_url: "http://example.com/made-up.jpg")).not_to be_valid + expect(Fabricate.build(:category, user: user, logo_url: upload.url)).to be_valid + end + + it "ensures background_url is valid" do + expect(Fabricate.build(:category, user: user, background_url: ";test")).not_to be_valid + expect(Fabricate.build(:category, user: user, background_url: "http://example.com/no.jpg")).not_to be_valid + expect(Fabricate.build(:category, user: user, background_url: upload.url)).to be_valid + end + end + it 'validates uniqueness in case insensitive way' do Fabricate(:category, name: "Cats") cats = Fabricate.build(:category, name: "cats") diff --git a/spec/models/user_profile_spec.rb b/spec/models/user_profile_spec.rb index cbacd417693..620b79e12b0 100644 --- a/spec/models/user_profile_spec.rb +++ b/spec/models/user_profile_spec.rb @@ -6,6 +6,23 @@ describe UserProfile do expect(user.user_profile).to be_present end + context "url validation" do + let(:user) { Fabricate(:user) } + let(:upload) { Fabricate(:upload) } + + it "ensures profile_background is valid" do + expect(Fabricate.build(:user_profile, user: user, profile_background: "---%")).not_to be_valid + expect(Fabricate.build(:user_profile, user: user, profile_background: "http://example.com/made-up.jpg")).not_to be_valid + expect(Fabricate.build(:user_profile, user: user, profile_background: upload.url)).to be_valid + end + + it "ensures background_url is valid" do + expect(Fabricate.build(:user_profile, user: user, card_background: ";test")).not_to be_valid + expect(Fabricate.build(:user_profile, user: user, card_background: "http://example.com/no.jpg")).not_to be_valid + expect(Fabricate.build(:user_profile, user: user, card_background: upload.url)).to be_valid + end + end + describe 'rebaking' do it 'correctly rebakes bio' do user_profile = Fabricate(:evil_trout).user_profile