FIX: Tweak upload security emoji check (#8981)

Further on from my earlier PR #8973 also reject upload as secure if its origin URL contains images/emoji. We still check Emoji.all first to try and be canonical.

This may be a little heavy handed (e.g. if an external URL followed this same path it would be a false positive), but there are a lot of emoji aliases where the actual Emoji url is something, but you can have another image that should not be secure that that thing is an alias for. For example slight_smile.png does not show up in Emoji.all BUT slightly_smiling_face does, and it aliases slight_smile e.g. /images/emoji/twitter/slight_smile.png?v=9 and /images/emoji/twitter/slightly_smiling_face.png?v=9 are equivalent.
This commit is contained in:
Martin Brennan 2020-02-17 15:11:15 +10:00 committed by GitHub
parent 9dcc454a07
commit e8efdd60d4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 11 additions and 1 deletions

View File

@ -78,6 +78,7 @@ class UploadSecurity
def based_on_regular_emoji?
return false if @upload.origin.blank?
uri = URI.parse(@upload.origin)
Emoji.all.map(&:url).include?("#{uri.path}?#{uri.query}")
return true if Emoji.all.map(&:url).include?("#{uri.path}?#{uri.query}")
uri.path.include?("images/emoji")
end
end

View File

@ -438,6 +438,15 @@ describe Upload do
upload.update!(secure: false, origin: "http://localhost:3000#{grinning.url}")
expect { upload.update_secure_status }
.not_to change { upload.secure }
expect(upload.reload.secure).to eq(false)
end
it 'does not mark any upload with origin containing images/emoji in the URL' do
SiteSetting.login_required = true
upload.update!(secure: false, origin: "http://localhost:3000/images/emoji/test.png")
expect { upload.update_secure_status }
.not_to change { upload.secure }
expect(upload.reload.secure).to eq(false)
end
end
end