Skip to content

fix: protect admins/mods/owner from moderation actions (#174) #181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
BillChirico merged 6 commits into from
Mar 2, 2026
Merged

fix: protect admins/mods/owner from moderation actions (#174) #181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f968a62
fix: address code review feedback on protect-roles (#181)
brian-mwirigi Mar 1, 2026
a50d66f
fix: deep-merge protectRoles defaults, remove dead code from Moderati…
brian-mwirigi Mar 1, 2026
ce85feb
feat: restore protectRoles UI to ModerationSection for future wiring
brian-mwirigi Mar 2, 2026
c20ea73
refactor: fetch config internally in isProtectedTarget for live confi…
brian-mwirigi Mar 2, 2026
4b922de
fix: stale JSDoc, skip fetch loop when protection disabled, validate …
brian-mwirigi Mar 2, 2026
7fc24c3
fix: commit roleIds onChange/onBlur before rebase
brian-mwirigi Mar 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/api/utils/configValidation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export const CONFIG_SCHEMA = {
type: 'object',
properties: {
enabled: { type: 'boolean' },
roleIds: { type: 'array' },
roleIds: { type: 'array', items: { type: 'string' } },
includeAdmins: { type: 'boolean' },
includeModerators: { type: 'boolean' },
includeServerOwner: { type: 'boolean' },
Expand Down
5 changes: 3 additions & 2 deletions src/modules/moderation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,10 +447,11 @@ export function stopTempbanScheduler() {
* configured under `moderation.protectRoles`.
* @param {import('discord.js').GuildMember} target - Target member to check
* @param {import('discord.js').Guild} guild - Discord guild
* @param {Object} config - Bot configuration
* @returns {boolean} True if the target should not be moderated
Comment on lines +448 to +450
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove stale JSDoc param from isProtectedTarget.

Line 449 documents a config parameter that no longer exists in the function signature, which is misleading.

♻️ Suggested doc fix 
 /**
  * Check if a target member is protected from moderation actions.
  * Protected members include the server owner, admins, moderators, and any custom role IDs
  * configured under `moderation.protectRoles`.
  * `@param` {import('discord.js').GuildMember} target - Target member to check
  * `@param` {import('discord.js').Guild} guild - Discord guild
- * `@param` {Object} config - Bot configuration
  * `@returns` {boolean} True if the target should not be moderated
  */
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/modules/moderation.js` around lines 447 - 450, The JSDoc for
isProtectedTarget documents a removed parameter named config; update the comment
block for the function isProtectedTarget in src/modules/moderation.js by
removing the stale "@param {Object} config - Bot configuration" line (or replace
it with any current params if needed) so the JSDoc matches the actual function
signature and return description; ensure the remaining `@param` entries (target,
guild) and the `@returns` line remain accurate.

*/
export function isProtectedTarget(target, guild, config) {
export function isProtectedTarget(target, guild) {
// Fetch config per-invocation so live config edits take effect immediately.
const config = getConfig(guild.id);
/**
* When the protectRoles block is missing from persisted configuration,
* fall back to the intended defaults: protection enabled, include owner,
Expand Down
31 changes: 17 additions & 14 deletions src/modules/triage-respond.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,21 +141,24 @@ export async function sendModerationLog(client, classification, snapshot, channe
// Skip moderation log if any flagged user is a protected role (admin/mod/owner)
const guild = logChannel.guild;
if (guild && targets.length > 0) {
const seenUserIds = new Set();
for (const t of targets) {
if (seenUserIds.has(t.userId)) continue;
seenUserIds.add(t.userId);
try {
const member = await guild.members.fetch(t.userId);
if (isProtectedTarget(member, guild, config)) {
warn('Triage skipped moderation log: target is a protected role', {
userId: t.userId,
channelId,
});
return;
// Skip the expensive member-fetch loop when protection is explicitly disabled.
if (config.moderation?.protectRoles?.enabled !== false) {
const seenUserIds = new Set();
for (const t of targets) {
if (seenUserIds.has(t.userId)) continue;
seenUserIds.add(t.userId);
try {
const member = await guild.members.fetch(t.userId);
if (isProtectedTarget(member, guild)) {
warn('Triage skipped moderation log: target is a protected role', {
userId: t.userId,
channelId,
});
return;
}
} catch {
// Member not in guild or fetch failed — proceed with logging
}
} catch {
// Member not in guild or fetch failed — proceed with logging
}
}
Comment on lines +141 to +163
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside this protected-role loop, isProtectedTarget(member, guild) currently re-fetches config internally (via getConfig) for each member checked. Since sendModerationLog already receives config and may iterate multiple user IDs, this can cause repeated deep-clone/config work. Consider passing the existing config into isProtectedTarget (or adding a variant that accepts config) so the loop stays O(n) without extra config loads.

Copilot uses AI. Check for mistakes.
}
Comment on lines +141 to +164
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sendModerationLog always does guild.members.fetch() for each flagged user before calling isProtectedTarget(). When moderation.protectRoles.enabled is explicitly set to false, this extra fetch work is unnecessary. Add an early guard to skip the protected-role fetch loop entirely when protection is disabled (e.g., check config.moderation?.protectRoles?.enabled === false before iterating).

Copilot uses AI. Check for mistakes.
Expand Down
2 changes: 1 addition & 1 deletion src/utils/modAction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ export async function executeModAction(interaction, opts) {

// Protected-role check (skipped when skipProtection is true)
if (!skipProtection && target) {
if (isProtectedTarget(target, interaction.guild, config)) {
if (isProtectedTarget(target, interaction.guild)) {
warn('Moderation blocked: target is a protected role', {
action,
targetId: target.id,
Expand Down
61 changes: 31 additions & 30 deletions tests/modules/moderation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ vi.mock('../../src/utils/duration.js', () => ({

import { getPool } from '../../src/db.js';
import { error as loggerError, warn as loggerWarn } from '../../src/logger.js';
import { getConfig } from '../../src/modules/config.js';
import {
checkEscalation,
checkHierarchy,
Expand Down Expand Up @@ -597,29 +598,29 @@ describe('moderation module', () => {
roles: { cache: { keys: () => roleIds } },
});

const makeGuild = (ownerId) => ({ ownerId });
const makeGuild = (ownerId) => ({ id: 'guild1', ownerId });

it('returns false when protectRoles is disabled', () => {
const target = makeTarget('user1', ['admin-role']);
const guild = makeGuild('owner1');
const config = {
getConfig.mockReturnValueOnce({
moderation: { protectRoles: { enabled: false } },
permissions: { adminRoleId: 'admin-role' },
};
expect(isProtectedTarget(target, guild, config)).toBe(false);
});
expect(isProtectedTarget(target, guild)).toBe(false);
});

it('returns false when protectRoles config is absent', () => {
const target = makeTarget('user1');
const guild = makeGuild('owner1');
const config = { moderation: {} };
expect(isProtectedTarget(target, guild, config)).toBe(false);
getConfig.mockReturnValueOnce({ moderation: {} });
expect(isProtectedTarget(target, guild)).toBe(false);
});

it('returns true for server owner when includeServerOwner is true', () => {
const target = makeTarget('owner1');
const guild = makeGuild('owner1');
const config = {
getConfig.mockReturnValueOnce({
moderation: {
protectRoles: {
enabled: true,
Expand All @@ -629,14 +630,14 @@ describe('moderation module', () => {
includeServerOwner: true,
},
},
};
expect(isProtectedTarget(target, guild, config)).toBe(true);
});
expect(isProtectedTarget(target, guild)).toBe(true);
});

it('returns false for server owner when includeServerOwner is false', () => {
const target = makeTarget('owner1');
const guild = makeGuild('owner1');
const config = {
getConfig.mockReturnValueOnce({
moderation: {
protectRoles: {
enabled: true,
Expand All @@ -646,14 +647,14 @@ describe('moderation module', () => {
includeServerOwner: false,
},
},
};
expect(isProtectedTarget(target, guild, config)).toBe(false);
});
expect(isProtectedTarget(target, guild)).toBe(false);
});

it('returns true for user with adminRoleId when includeAdmins is true', () => {
const target = makeTarget('user1', ['admin-role']);
const guild = makeGuild('owner1');
const config = {
getConfig.mockReturnValueOnce({
moderation: {
protectRoles: {
enabled: true,
Expand All @@ -664,14 +665,14 @@ describe('moderation module', () => {
},
},
permissions: { adminRoleId: 'admin-role' },
};
expect(isProtectedTarget(target, guild, config)).toBe(true);
});
expect(isProtectedTarget(target, guild)).toBe(true);
});

it('returns false for admin role when includeAdmins is false', () => {
const target = makeTarget('user1', ['admin-role']);
const guild = makeGuild('owner1');
const config = {
getConfig.mockReturnValueOnce({
moderation: {
protectRoles: {
enabled: true,
Expand All @@ -682,14 +683,14 @@ describe('moderation module', () => {
},
},
permissions: { adminRoleId: 'admin-role' },
};
expect(isProtectedTarget(target, guild, config)).toBe(false);
});
expect(isProtectedTarget(target, guild)).toBe(false);
});

it('returns true for user with moderatorRoleId when includeModerators is true', () => {
const target = makeTarget('user1', ['mod-role']);
const guild = makeGuild('owner1');
const config = {
getConfig.mockReturnValueOnce({
moderation: {
protectRoles: {
enabled: true,
Expand All @@ -700,14 +701,14 @@ describe('moderation module', () => {
},
},
permissions: { moderatorRoleId: 'mod-role' },
};
expect(isProtectedTarget(target, guild, config)).toBe(true);
});
expect(isProtectedTarget(target, guild)).toBe(true);
});

it('returns true for user with a custom roleId in protectRoles.roleIds', () => {
const target = makeTarget('user1', ['custom-role']);
const guild = makeGuild('owner1');
const config = {
getConfig.mockReturnValueOnce({
moderation: {
protectRoles: {
enabled: true,
Expand All @@ -717,14 +718,14 @@ describe('moderation module', () => {
includeServerOwner: false,
},
},
};
expect(isProtectedTarget(target, guild, config)).toBe(true);
});
expect(isProtectedTarget(target, guild)).toBe(true);
});

it('returns false for regular user with no protected roles', () => {
const target = makeTarget('user1', ['regular-role']);
const guild = makeGuild('owner1');
const config = {
getConfig.mockReturnValueOnce({
moderation: {
protectRoles: {
enabled: true,
Expand All @@ -735,14 +736,14 @@ describe('moderation module', () => {
},
},
permissions: { adminRoleId: 'admin-role', moderatorRoleId: 'mod-role' },
};
expect(isProtectedTarget(target, guild, config)).toBe(false);
});
expect(isProtectedTarget(target, guild)).toBe(false);
});

it('returns false when no protectedRoleIds resolve (no adminRoleId set) and user is non-owner', () => {
const target = makeTarget('user1', ['some-role']);
const guild = makeGuild('owner1');
const config = {
getConfig.mockReturnValueOnce({
moderation: {
protectRoles: {
enabled: true,
Expand All @@ -753,8 +754,8 @@ describe('moderation module', () => {
},
},
permissions: {},
};
expect(isProtectedTarget(target, guild, config)).toBe(false);
});
expect(isProtectedTarget(target, guild)).toBe(false);
});
});

Expand Down
28 changes: 28 additions & 0 deletions tests/modules/triage-respond.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,34 @@ describe('triage-respond', () => {

expect(safeSend).toHaveBeenCalled();
});

it('should skip the protected-role fetch loop when protectRoles is explicitly disabled', async () => {
const mockFetch = vi.fn();
const mockGuild = { members: { fetch: mockFetch } };
const mockLogChannel = { id: 'log-channel', guild: mockGuild };
const mockClient = {
channels: { fetch: vi.fn().mockResolvedValue(mockLogChannel) },
};

const classification = {
recommendedAction: 'warn',
violatedRule: 'Rule 1',
reasoning: 'Rude message',
targetMessageIds: ['msg1'],
};
const snapshot = [{ messageId: 'msg1', author: 'BadUser', userId: 'user1', content: 'msg' }];
const config = {
triage: { moderationLogChannel: 'log-channel' },
moderation: { protectRoles: { enabled: false } },
};

await sendModerationLog(mockClient, classification, snapshot, 'channel1', config);

// Member fetch should not be called since protection is disabled
expect(mockFetch).not.toHaveBeenCalled();
expect(isProtectedTarget).not.toHaveBeenCalled();
expect(safeSend).toHaveBeenCalled();
});
});

describe('sendResponses', () => {
Expand Down
16 changes: 13 additions & 3 deletions web/src/components/dashboard/config-editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1212,14 +1212,24 @@ export function ConfigEditor() {
<input
type="text"
value={protectRoleIdsRaw}
onChange={(e) => setProtectRoleIdsRaw(e.target.value)}
onBlur={() =>
onChange={(e) => {
const raw = e.target.value;
setProtectRoleIdsRaw(raw);
updateProtectRolesField(
'roleIds',
protectRoleIdsRaw
raw
.split(',')
.map((s) => s.trim())
.filter(Boolean),
);
}}
onBlur={(e) =>
setProtectRoleIdsRaw(
e.target.value
.split(',')
.map((s) => s.trim())
.filter(Boolean)
.join(', '),
)
}
disabled={saving}
Expand Down
Loading
Loading