SECURITY: Make sure uploaded_urls have corresponding upload records

app/models/category.rb

@@ -36,6 +36,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

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
 

lib/discourse.rb

@@ -186,9 +186,13 @@ module Discourse
     ActionController::Base.config.relative_url_root.presence || default_value
   end
 
+  def self.base_protocol
+    SiteSetting.force_https? ? "https" : "http"
+  end
+
   def self.base_url_no_prefix
-    protocol, default_port = SiteSetting.force_https? ? ["https", 443] : ["http", 80]
-    url = "#{protocol}://#{current_hostname}"
+    default_port = SiteSetting.force_https? ? 443 : 80
+    url = "#{base_protocol}://#{current_hostname}"
     url << ":#{SiteSetting.port}" if SiteSetting.port.to_i > 0 && SiteSetting.port.to_i != default_port
     url
   end

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

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(:force_https?).returns(false)
+        SiteSetting.force_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(:force_https?).returns(true)
+        SiteSetting.force_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
 

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")

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

spec/models/user_spec.rb

@@ -339,7 +339,7 @@ describe User do
 
     it 'returns false if user is not the only admin' do
       admin = Fabricate(:admin)
-      second_admin = Fabricate(:admin)
+      Fabricate(:admin)
 
       expect(admin.is_singular_admin?).to eq(false)
     end