FIX: with_secure_uploads? could return nil in some cases (#24592)

When we check upload security, one of the checks is to
run `access_control_post.with_secure_uploads?`. The problem
here is that the `topic` for the post could be deleted,
which would make the check return `nil` sometimes instead
of false because of safe navigation. We just need to be
more explicit.
This commit is contained in:
Martin Brennan 2023-11-28 13:12:28 +10:00 committed by GitHub
parent b21a559663
commit 1fc0ce1ac2
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 24 additions and 2 deletions

View File

@ -567,15 +567,18 @@ class Post < ActiveRecord::Base
def with_secure_uploads?
return false if !SiteSetting.secure_uploads?
topic_including_deleted = Topic.with_deleted.find_by(id: self.topic_id)
return false if topic_including_deleted.blank?
# NOTE: This is meant to be a stopgap solution to prevent secure uploads
# in a single place (private messages) for sensitive admin data exports.
# Ideally we would want a more comprehensive way of saying that certain
# upload types get secured which is a hybrid/mixed mode secure uploads,
# but for now this will do the trick.
return topic&.private_message? if SiteSetting.secure_uploads_pm_only?
return topic_including_deleted.private_message? if SiteSetting.secure_uploads_pm_only?
SiteSetting.login_required? || topic&.private_message? || topic&.category&.read_restricted?
SiteSetting.login_required? || topic_including_deleted.private_message? ||
topic_including_deleted.read_restricted_category?
end
def hide!(post_action_type_id, reason = nil, custom_message: nil)

View File

@ -186,6 +186,17 @@ RSpec.describe Post do
expect(post.with_secure_uploads?).to eq(true)
end
context "when the topic is deleted" do
before do
topic.trash!
post.reload
end
it "returns true" do
expect(post.with_secure_uploads?).to eq(true)
end
end
context "if secure_uploads_pm_only" do
before { SiteSetting.secure_uploads_pm_only = true }
@ -202,6 +213,14 @@ RSpec.describe Post do
expect(post.with_secure_uploads?).to eq(true)
end
context "when the topic is deleted" do
before { topic.trash! }
it "returns true" do
expect(post.with_secure_uploads?).to eq(true)
end
end
context "if secure_uploads_pm_only" do
before { SiteSetting.secure_uploads_pm_only = true }