SECURITY: Restrict staff action logs visibility for moderators

Previously, moderators had full access to all staff action logs, which
exposed sensitive information including webhook secrets, API keys, site
settings, private messages, and restricted categories.

This change implements an allowlist approach where moderators can only
see actions relevant to their role (user management, posts, topics,
badges, etc.) while admin-only actions (site settings, webhooks, API
keys, themes, etc.) are hidden.

Additionally, content-level redaction ensures moderators cannot see
details of logs referencing private topics, restricted categories, or
deleted content they don't have access to.

Site setting gates control visibility of category, trust level, and
email actions based on existing moderator permission settings.

Ref - t/171137
This commit is contained in:
Régis Hanol
2026-01-28 17:11:14 +00:00
committed by David Taylor
parent 5e99b52007
commit 9892628a50
12 changed files with 509 additions and 38 deletions
@@ -29,6 +29,9 @@ class Admin::StaffActionLogsController < Admin::StaffController
def diff
@history = UserHistory.find(params[:id])
raise Discourse::NotFound unless guardian.can_see_staff_action_log?(@history)
raise Discourse::NotFound unless guardian.can_see_staff_action_log_content?(@history)
prev = @history.previous_value
cur = @history.new_value
+98 -3
View File
@@ -30,6 +30,11 @@ class UserHistory < ActiveRecord::Base
validates :action, presence: true
# moderators can only see these if the corresponding site setting is enabled
CATEGORY_ACTIONS = %i[create_category change_category_settings delete_category].freeze
TRUST_LEVEL_ACTIONS = %i[change_trust_level lock_trust_level unlock_trust_level].freeze
EMAIL_ACTIONS = %i[check_email add_email update_email destroy_email].freeze
scope :only_staff_actions, -> { where("action IN (?)", UserHistory.staff_action_ids) }
before_save :set_admin_only
@@ -295,8 +300,97 @@ class UserHistory < ActiveRecord::Base
@staff_action_ids ||= staff_actions.map { |a| actions[a] }
end
def self.moderator_visible_actions
@moderator_visible_actions ||= [
# user account
:approve_user,
:activate_user,
:deactivate_user,
:delete_user,
# user profile
:change_name,
:change_username,
:change_title,
:revoke_title,
# trust levels
:change_trust_level,
:lock_trust_level,
:unlock_trust_level,
# emails
:add_email,
:check_email,
:update_email,
:destroy_email,
# suspension
:suspend_user,
:unsuspend_user,
:removed_suspend_user,
:removed_unsuspend_user,
# silence
:silence_user,
:unsilence_user,
:removed_silence_user,
:removed_unsilence_user,
# badges
:grant_badge,
:revoke_badge,
# posts
:post_approved,
:post_rejected,
:post_edit,
:post_locked,
:post_unlocked,
:post_staff_note_create,
:post_staff_note_destroy,
:delete_post,
:delete_post_permanently,
:permanently_delete_post_revisions,
# topics
:topic_published,
:topic_closed,
:topic_opened,
:topic_archived,
:topic_unarchived,
:topic_timestamps_changed,
:topic_slow_mode_set,
:topic_slow_mode_removed,
:recover_topic,
:delete_topic,
:delete_topic_permanently,
# categories
:create_category,
:change_category_settings,
:delete_category,
# tags
:tag_group_create,
:tag_group_change,
:tag_group_destroy,
# watched words
:watched_word_create,
:watched_word_destroy,
:create_watched_word_group,
:update_watched_word_group,
:delete_watched_word_group,
# misc.
:check_personal_message,
:reset_bounce_score,
]
end
def self.site_setting_excluded_actions
excluded = []
excluded.concat(CATEGORY_ACTIONS) unless SiteSetting.moderators_manage_categories
excluded.concat(TRUST_LEVEL_ACTIONS) unless SiteSetting.moderators_change_trust_levels
excluded.concat(EMAIL_ACTIONS) unless SiteSetting.moderators_view_emails
excluded
end
def self.moderator_visible_action_ids
(moderator_visible_actions - site_setting_excluded_actions).map { |a| actions[a] }
end
def self.admin_only_action_ids
@admin_only_action_ids ||= [actions[:change_site_setting]]
@admin_only_action_ids ||= staff_action_ids - moderator_visible_action_ids
end
def self.with_filters(filters)
@@ -343,8 +437,9 @@ class UserHistory < ActiveRecord::Base
.with_filters(opts.slice(*staff_filters))
.only_staff_actions
.order("id DESC")
.includes(:acting_user, :target_user)
query = query.where(admin_only: false) unless viewer && viewer.admin?
.includes(:acting_user, :target_user, :topic, :post, :category)
query = query.where(action: moderator_visible_action_ids) unless viewer&.admin?
query = query.where("created_at >= ?", opts[:start_date].to_time) if opts[:start_date]
query = query.where("created_at <= ?", opts[:end_date].to_time) if opts[:end_date]
@@ -25,7 +25,16 @@ class UserHistorySerializer < ApplicationSerializer
%i[custom custom_staff].include?(key) ? object.custom_type : key.to_s
end
def details
redact_content? ? redacted_content : object.details
end
def context
redact_content? ? nil : object.context
end
def new_value
return nil if redact_content?
if object.new_value
object.new_value_is_json? ? ::JSON.parse(object.new_value) : object.new_value
else
@@ -34,6 +43,7 @@ class UserHistorySerializer < ApplicationSerializer
end
def previous_value
return nil if redact_content?
if object.previous_value
object.previous_value_is_json? ? ::JSON.parse(object.previous_value) : object.previous_value
else
@@ -45,4 +55,15 @@ class UserHistorySerializer < ApplicationSerializer
return nil unless scope.can_see_ip?
object.ip_address.try(:to_s)
end
private
def redact_content?
return @redact_content if defined?(@redact_content)
@redact_content = !scope.can_see_staff_action_log_content?(object)
end
def redacted_content
I18n.t("staff_action_logs.redacted")
end
end
+7 -2
View File
@@ -3,7 +3,7 @@
# Responsible for logging the actions of admins and moderators.
class StaffActionLogger
def self.base_attrs
%i[topic_id post_id context subject ip_address previous_value new_value]
%i[topic_id post_id category_id context subject ip_address previous_value new_value]
end
def initialize(admin)
@@ -888,7 +888,12 @@ class StaffActionLogger
]
UserHistory.create!(
params(opts).merge(action: UserHistory.actions[:post_rejected], details: details.join("\n")),
params(opts).merge(
action: UserHistory.actions[:post_rejected],
details: details.join("\n"),
topic_id: topic&.id,
category_id: reviewable.category_id,
),
)
end
+1
View File
@@ -5645,6 +5645,7 @@ en:
custom: "Notification from %{username} on %{site_title}"
staff_action_logs:
redacted: "[content not visible]"
json_too_long: "Values not logged because they exceed the column length limits"
not_found: "not found"
unknown: "unknown"
+2
View File
@@ -10,6 +10,7 @@ require "guardian/permalink_guardian"
require "guardian/post_guardian"
require "guardian/post_revision_guardian"
require "guardian/sidebar_guardian"
require "guardian/staff_action_log_guardian"
require "guardian/tag_guardian"
require "guardian/topic_guardian"
require "guardian/user_guardian"
@@ -28,6 +29,7 @@ class Guardian
include PostRevisionGuardian
include LocalizationGuardian
include SidebarGuardian
include StaffActionLogGuardian
include TagGuardian
include TopicGuardian
include UserGuardian
+17
View File
@@ -0,0 +1,17 @@
# frozen_string_literal: true
module StaffActionLogGuardian
def can_see_staff_action_log?(user_history)
return true if is_admin?
return false unless is_staff?
UserHistory.moderator_visible_action_ids.include?(user_history.action)
end
def can_see_staff_action_log_content?(user_history)
return true if is_admin?
return false if user_history.topic_id.present? && !can_see_topic?(user_history.topic)
return false if user_history.post_id.present? && !can_see_post?(user_history.post)
return false if user_history.category_id.present? && !can_see_category?(user_history.category)
true
end
end
+1
View File
@@ -273,6 +273,7 @@ module Chat
chat_channel_name: self.name,
previous_value: status_previously_was,
new_value: status,
category_id: category_channel? ? self.chatable_id : nil,
},
)
@@ -63,7 +63,11 @@ module Chat
def log_channel_deletion(guardian:, channel:)
StaffActionLogger.new(guardian.user).log_custom(
DELETE_CHANNEL_LOG_KEY,
{ chat_channel_id: channel.id, chat_channel_name: channel.title(guardian.user) },
{
chat_channel_id: channel.id,
chat_channel_name: channel.title(guardian.user),
category_id: channel.category_channel? ? channel.chatable_id : nil,
},
)
end
@@ -0,0 +1,114 @@
# frozen_string_literal: true
RSpec.describe StaffActionLogGuardian do
fab!(:admin)
fab!(:moderator)
fab!(:user)
describe "#can_see_staff_action_log?" do
it "returns true for admins regardless of action type" do
admin_only_log =
UserHistory.create!(action: UserHistory.actions[:change_site_setting], subject: "title")
expect(Guardian.new(admin).can_see_staff_action_log?(admin_only_log)).to eq(true)
end
it "returns false for non-staff users" do
log = UserHistory.create!(action: UserHistory.actions[:suspend_user])
expect(Guardian.new(user).can_see_staff_action_log?(log)).to eq(false)
end
it "returns true for moderators when action is in moderator_visible_actions" do
log = UserHistory.create!(action: UserHistory.actions[:suspend_user])
expect(Guardian.new(moderator).can_see_staff_action_log?(log)).to eq(true)
end
it "returns false for moderators when action is admin-only" do
log = UserHistory.create!(action: UserHistory.actions[:change_site_setting], subject: "title")
expect(Guardian.new(moderator).can_see_staff_action_log?(log)).to eq(false)
end
it "respects site setting gates for moderators" do
SiteSetting.moderators_manage_categories = true
category = Fabricate(:category)
log =
UserHistory.create!(action: UserHistory.actions[:create_category], category_id: category.id)
expect(Guardian.new(moderator).can_see_staff_action_log?(log)).to eq(true)
SiteSetting.moderators_manage_categories = false
expect(Guardian.new(moderator).can_see_staff_action_log?(log)).to eq(false)
end
end
describe "#can_see_staff_action_log_content?" do
it "returns true for admins regardless of content" do
pm = Fabricate(:private_message_topic)
log = UserHistory.create!(action: UserHistory.actions[:delete_topic], topic_id: pm.id)
expect(Guardian.new(admin).can_see_staff_action_log_content?(log)).to eq(true)
end
it "returns true for moderators when topic is public" do
topic = Fabricate(:topic)
log = UserHistory.create!(action: UserHistory.actions[:delete_topic], topic_id: topic.id)
expect(Guardian.new(moderator).can_see_staff_action_log_content?(log)).to eq(true)
end
it "returns false for moderators when topic is a PM they cannot see" do
pm = Fabricate(:private_message_topic)
log = UserHistory.create!(action: UserHistory.actions[:delete_topic], topic_id: pm.id)
expect(Guardian.new(moderator).can_see_staff_action_log_content?(log)).to eq(false)
end
it "returns false for moderators when post is in a topic they cannot see" do
pm = Fabricate(:private_message_topic)
post = Fabricate(:post, topic: pm)
log = UserHistory.create!(action: UserHistory.actions[:post_edit], post_id: post.id)
expect(Guardian.new(moderator).can_see_staff_action_log_content?(log)).to eq(false)
end
it "returns false for moderators when category is restricted" do
group = Fabricate(:group)
category = Fabricate(:private_category, group:)
log =
UserHistory.create!(
action: UserHistory.actions[:change_category_settings],
category_id: category.id,
)
expect(Guardian.new(moderator).can_see_staff_action_log_content?(log)).to eq(false)
end
it "returns true for moderators when category is public" do
category = Fabricate(:category)
log =
UserHistory.create!(
action: UserHistory.actions[:change_category_settings],
category_id: category.id,
)
expect(Guardian.new(moderator).can_see_staff_action_log_content?(log)).to eq(true)
end
it "returns false when referenced topic is deleted" do
topic = Fabricate(:topic)
log = UserHistory.create!(action: UserHistory.actions[:delete_topic], topic_id: topic.id)
topic.destroy!
expect(Guardian.new(moderator).can_see_staff_action_log_content?(log)).to eq(false)
end
it "returns false when referenced post is deleted" do
post = Fabricate(:post)
log = UserHistory.create!(action: UserHistory.actions[:post_edit], post_id: post.id)
post.destroy!
expect(Guardian.new(moderator).can_see_staff_action_log_content?(log)).to eq(false)
end
it "returns false when referenced category is deleted" do
category = Fabricate(:category)
log =
UserHistory.create!(
action: UserHistory.actions[:change_category_settings],
category_id: category.id,
)
category.destroy!
expect(Guardian.new(moderator).can_see_staff_action_log_content?(log)).to eq(false)
end
end
end
+82 -2
View File
@@ -44,9 +44,10 @@ RSpec.describe UserHistory do
expect(records.size).to eq(3)
end
it "doesn't return records to moderators that only admins should see" do
it "only returns moderator-visible actions to moderators (allowlist approach)" do
records = described_class.staff_action_records(Fabricate(:moderator)).to_a
expect(records).not_to include([change_site_setting])
expect(records).to contain_exactly(change_trust_level)
expect(records).not_to include(change_site_setting, custom_history)
end
it "filters by action" do
@@ -96,4 +97,83 @@ RSpec.describe UserHistory do
end
end
end
describe ".site_setting_excluded_actions" do
it "excludes category actions when moderators_manage_categories is disabled" do
SiteSetting.moderators_manage_categories = false
expect(described_class.site_setting_excluded_actions).to include(
:create_category,
:change_category_settings,
:delete_category,
)
end
it "includes category actions when moderators_manage_categories is enabled" do
SiteSetting.moderators_manage_categories = true
expect(described_class.site_setting_excluded_actions).not_to include(
:create_category,
:change_category_settings,
:delete_category,
)
end
it "excludes trust level actions when moderators_change_trust_levels is disabled" do
SiteSetting.moderators_change_trust_levels = false
expect(described_class.site_setting_excluded_actions).to include(
:change_trust_level,
:lock_trust_level,
:unlock_trust_level,
)
end
it "includes trust level actions when moderators_change_trust_levels is enabled" do
SiteSetting.moderators_change_trust_levels = true
expect(described_class.site_setting_excluded_actions).not_to include(
:change_trust_level,
:lock_trust_level,
:unlock_trust_level,
)
end
it "excludes email actions when moderators_view_emails is disabled" do
SiteSetting.moderators_view_emails = false
expect(described_class.site_setting_excluded_actions).to include(
:check_email,
:add_email,
:update_email,
:destroy_email,
)
end
it "includes email actions when moderators_view_emails is enabled" do
SiteSetting.moderators_view_emails = true
expect(described_class.site_setting_excluded_actions).not_to include(
:check_email,
:add_email,
:update_email,
:destroy_email,
)
end
end
describe ".moderator_visible_action_ids" do
it "returns action IDs for moderator-visible actions" do
ids = described_class.moderator_visible_action_ids
expect(ids).to include(described_class.actions[:suspend_user])
expect(ids).to include(described_class.actions[:delete_topic])
expect(ids).not_to include(described_class.actions[:change_site_setting])
end
it "responds dynamically to site setting changes" do
SiteSetting.moderators_manage_categories = true
expect(described_class.moderator_visible_action_ids).to include(
described_class.actions[:create_category],
)
SiteSetting.moderators_manage_categories = false
expect(described_class.moderator_visible_action_ids).not_to include(
described_class.actions[:create_category],
)
end
end
end
@@ -37,7 +37,7 @@ RSpec.describe Admin::StaffActionLogsController do
"delete_topic",
)
freeze_time 3.days.from_now
StaffActionLogger.new(Discourse.system_user).log_grant_admin(user)
StaffActionLogger.new(Discourse.system_user).log_silence_user(user, details: "test")
freeze_time 2.days.from_now
StaffActionLogger.new(Discourse.system_user).log_user_suspend(user, "reason")
end
@@ -50,7 +50,7 @@ RSpec.describe Admin::StaffActionLogsController do
expect(json["staff_action_logs"].length).to eq(2)
expect(json["staff_action_logs"][0]["action_name"]).to eq("suspend_user")
expect(json["staff_action_logs"][1]["action_name"]).to eq("grant_admin")
expect(json["staff_action_logs"][1]["action_name"]).to eq("silence_user")
end
it "filter logs by end_date" do
@@ -60,7 +60,7 @@ RSpec.describe Admin::StaffActionLogsController do
expect(response.status).to eq(200)
expect(json["staff_action_logs"].length).to eq(2)
expect(json["staff_action_logs"][0]["action_name"]).to eq("grant_admin")
expect(json["staff_action_logs"][0]["action_name"]).to eq("silence_user")
expect(json["staff_action_logs"][1]["action_name"]).to eq("delete_topic")
end
@@ -75,7 +75,7 @@ RSpec.describe Admin::StaffActionLogsController do
expect(response.status).to eq(200)
expect(json["staff_action_logs"].length).to eq(1)
expect(json["staff_action_logs"][0]["action_name"]).to eq("grant_admin")
expect(json["staff_action_logs"][0]["action_name"]).to eq("silence_user")
end
end
@@ -92,29 +92,40 @@ RSpec.describe Admin::StaffActionLogsController do
include_examples "staff action logs accessible"
it "generates logs with pages" do
1
.upto(4)
.each do |idx|
StaffActionLogger.new(Discourse.system_user).log_site_setting_change(
"title",
"value #{idx - 1}",
"value #{idx}",
)
end
4.times do |idx|
StaffActionLogger.new(Discourse.system_user).log_site_setting_change(
"title",
"value #{idx}",
"value #{idx + 1}",
)
end
get "/admin/logs/staff_action_logs.json", params: { limit: 3 }
json = response.parsed_body
expect(response.status).to eq(200)
expect(json["staff_action_logs"].length).to eq(3)
expect(json["staff_action_logs"][0]["new_value"]).to eq("value 4")
expect(response.parsed_body["staff_action_logs"].length).to eq(3)
expect(response.parsed_body["staff_action_logs"][0]["new_value"]).to eq("value 4")
get "/admin/logs/staff_action_logs.json", params: { limit: 3, page: 1 }
expect(response.parsed_body["staff_action_logs"].length).to eq(1)
expect(response.parsed_body["staff_action_logs"][0]["new_value"]).to eq("value 1")
end
json = response.parsed_body
expect(response.status).to eq(200)
expect(json["staff_action_logs"].length).to eq(1)
expect(json["staff_action_logs"][0]["new_value"]).to eq("value 1")
it "sees admin-only actions" do
StaffActionLogger.new(admin).log_site_setting_change("title", "old", "new")
get "/admin/logs/staff_action_logs.json"
expect(response.parsed_body["staff_action_logs"].map { |l| l["action_name"] }).to include(
"change_site_setting",
)
end
it "sees full content for private topics" do
pm = Fabricate(:private_message_topic)
StaffActionLogger.new(admin).log_topic_delete_recover(pm, "delete_topic")
get "/admin/logs/staff_action_logs.json"
expect(response.parsed_body["staff_action_logs"].first["details"]).to include(pm.title)
end
context "when staff actions are extended" do
@@ -122,14 +133,12 @@ RSpec.describe Admin::StaffActionLogsController do
before { UserHistory.stubs(:staff_actions).returns([plugin_extended_action]) }
after { UserHistory.unstub(:staff_actions) }
it "Uses the custom_staff id" do
get "/admin/logs/staff_action_logs.json", params: {}
it "uses custom_staff id for unknown actions" do
get "/admin/logs/staff_action_logs.json"
json = response.parsed_body
action = json["extras"]["user_history_actions"].first
expect(action["id"]).to eq plugin_extended_action.to_s
expect(action["action_id"]).to eq UserHistory.actions[:custom_staff]
action = response.parsed_body["extras"]["user_history_actions"].first
expect(action["id"]).to eq(plugin_extended_action.to_s)
expect(action["action_id"]).to eq(UserHistory.actions[:custom_staff])
end
end
end
@@ -138,6 +147,118 @@ RSpec.describe Admin::StaffActionLogsController do
before { sign_in(moderator) }
include_examples "staff action logs accessible"
it "does not see admin-only actions" do
StaffActionLogger.new(admin).log_site_setting_change("title", "old", "new")
StaffActionLogger.new(admin).log_web_hook(
Fabricate(:web_hook),
UserHistory.actions[:web_hook_create],
)
StaffActionLogger.new(admin).log_api_key(
Fabricate(:api_key),
UserHistory.actions[:api_key_create],
)
get "/admin/logs/staff_action_logs.json"
action_names = response.parsed_body["staff_action_logs"].map { |l| l["action_name"] }
expect(action_names).not_to include(
"change_site_setting",
"web_hook_create",
"api_key_create",
)
end
it "sees full content for public topics" do
topic = Fabricate(:topic)
StaffActionLogger.new(admin).log_topic_delete_recover(topic, "delete_topic")
get "/admin/logs/staff_action_logs.json"
expect(response.parsed_body["staff_action_logs"].first["details"]).to include(topic.title)
end
it "redacts content for private topics" do
pm = Fabricate(:private_message_topic)
StaffActionLogger.new(admin).log_topic_delete_recover(pm, "delete_topic")
get "/admin/logs/staff_action_logs.json"
log = response.parsed_body["staff_action_logs"].first
expect(log["details"]).to eq(I18n.t("staff_action_logs.redacted"))
expect(log["context"]).to be_nil
end
it "redacts content for restricted categories" do
SiteSetting.moderators_manage_categories = true
category = Fabricate(:private_category, group: Fabricate(:group))
StaffActionLogger.new(admin).log_category_creation(category)
get "/admin/logs/staff_action_logs.json"
expect(response.parsed_body["staff_action_logs"].first["details"]).to eq(
I18n.t("staff_action_logs.redacted"),
)
end
it "redacts content when referenced topic is deleted" do
topic = Fabricate(:topic)
StaffActionLogger.new(admin).log_topic_delete_recover(topic, "delete_topic")
topic.destroy!
get "/admin/logs/staff_action_logs.json"
log = response.parsed_body["staff_action_logs"].first
expect(log["details"]).to eq(I18n.t("staff_action_logs.redacted"))
expect(log["context"]).to be_nil
end
it "redacts content when referenced post is deleted" do
post = Fabricate(:post)
StaffActionLogger.new(admin).log_post_edit(post, old_raw: "old content")
post.destroy!
get "/admin/logs/staff_action_logs.json"
expect(response.parsed_body["staff_action_logs"].first["details"]).to eq(
I18n.t("staff_action_logs.redacted"),
)
end
it "redacts content when referenced category is deleted" do
SiteSetting.moderators_manage_categories = true
category = Fabricate(:category)
StaffActionLogger.new(admin).log_category_creation(category)
category.destroy!
get "/admin/logs/staff_action_logs.json"
expect(response.parsed_body["staff_action_logs"].first["details"]).to eq(
I18n.t("staff_action_logs.redacted"),
)
end
it "hides category actions when moderators_manage_categories is disabled" do
SiteSetting.moderators_manage_categories = false
category = Fabricate(:category)
StaffActionLogger.new(admin).log_category_creation(category)
get "/admin/logs/staff_action_logs.json"
action_names = response.parsed_body["staff_action_logs"].map { |l| l["action_name"] }
expect(action_names).not_to include("create_category")
end
it "shows category actions when moderators_manage_categories is enabled" do
SiteSetting.moderators_manage_categories = true
category = Fabricate(:category)
StaffActionLogger.new(admin).log_category_creation(category)
get "/admin/logs/staff_action_logs.json"
action_names = response.parsed_body["staff_action_logs"].map { |l| l["action_name"] }
expect(action_names).to include("create_category")
end
end
context "when logged in as a non-staff user" do
@@ -231,8 +352,15 @@ RSpec.describe Admin::StaffActionLogsController do
context "when logged in as a moderator" do
before { sign_in(moderator) }
include_examples "theme diffs accessible"
include_examples "tag_group diffs accessible"
it "denies access to theme diffs (admin-only action)" do
theme = Fabricate(:theme)
record = StaffActionLogger.new(Discourse.system_user).log_theme_change("{}", theme)
get "/admin/logs/staff_action_logs/#{record.id}/diff.json"
expect(response.status).to eq(404)
end
end
context "when logged in as a non-staff user" do