fix(doctor): detect groupPolicy=allowlist with empty groupAllowFrom (#28477)

* fix(doctor): detect groupPolicy=allowlist with empty groupAllowFrom

The existing `detectEmptyAllowlistPolicy` check only covers
`dmPolicy="allowlist"` with empty `allowFrom`. After the .26 security
hardening (`resolveDmGroupAccessDecision` fails closed on empty
allowlists), `groupPolicy="allowlist"` without `groupAllowFrom` or
`allowFrom` silently drops all group/channel messages with only a
verbose-level log.

Add a parallel check: when `groupPolicy` is `"allowlist"` and neither
`groupAllowFrom` nor `allowFrom` has entries, surface a doctor warning
with remediation steps.

Closes #27552

* fix: align empty-array semantics with runtime resolveGroupAllowFromSources

The runtime treats groupAllowFrom: [] as unset and falls back to
allowFrom, but the doctor check used ?? which treats [] as authoritative.
This caused a false warning when groupAllowFrom was explicitly empty but
allowFrom had entries.

Match runtime behavior: treat empty groupAllowFrom arrays as unset
before falling back to allowFrom.

* fix: scope group allowlist check to sender-based channels only

* fix: align doctor group allowlist semantics (#28477) (thanks @tonydehnke)

---------

Co-authored-by: mukhtharcm <mukhtharcm@gmail.com>
This commit is contained in:
Tony Dehnke
2026-02-28 16:15:10 +07:00
committed by GitHub
parent 5d51e99537
commit f1bf558685
3 changed files with 133 additions and 32 deletions

View File

@@ -19,6 +19,21 @@ function expectGoogleChatDmAllowFromRepaired(cfg: unknown) {
expect(typed.channels.googlechat.allowFrom).toBeUndefined();
}
async function collectDoctorWarnings(config: Record<string, unknown>): Promise<string[]> {
const noteSpy = vi.spyOn(noteModule, "note").mockImplementation(() => {});
try {
await runDoctorConfigWithInput({
config,
run: loadAndMaybeMigrateDoctorConfig,
});
return noteSpy.mock.calls
.filter((call) => call[1] === "Doctor warnings")
.map((call) => String(call[0]));
} finally {
noteSpy.mockRestore();
}
}
type DiscordGuildRule = {
users: string[];
roles: string[];
@@ -56,31 +71,59 @@ describe("doctor config flow", () => {
});
it("does not warn on mutable account allowlists when dangerous name matching is inherited", async () => {
const noteSpy = vi.spyOn(noteModule, "note").mockImplementation(() => {});
try {
await runDoctorConfigWithInput({
config: {
channels: {
slack: {
dangerouslyAllowNameMatching: true,
accounts: {
work: {
allowFrom: ["alice"],
},
},
const doctorWarnings = await collectDoctorWarnings({
channels: {
slack: {
dangerouslyAllowNameMatching: true,
accounts: {
work: {
allowFrom: ["alice"],
},
},
},
run: loadAndMaybeMigrateDoctorConfig,
});
},
});
expect(doctorWarnings.some((line) => line.includes("mutable allowlist"))).toBe(false);
});
const doctorWarnings = noteSpy.mock.calls
.filter((call) => call[1] === "Doctor warnings")
.map((call) => String(call[0]));
expect(doctorWarnings.some((line) => line.includes("mutable allowlist"))).toBe(false);
} finally {
noteSpy.mockRestore();
}
it("does not warn about sender-based group allowlist for googlechat", async () => {
const doctorWarnings = await collectDoctorWarnings({
channels: {
googlechat: {
groupPolicy: "allowlist",
accounts: {
work: {
groupPolicy: "allowlist",
},
},
},
},
});
expect(
doctorWarnings.some(
(line) => line.includes('groupPolicy is "allowlist"') && line.includes("groupAllowFrom"),
),
).toBe(false);
});
it("warns when imessage group allowlist is empty even if allowFrom is set", async () => {
const doctorWarnings = await collectDoctorWarnings({
channels: {
imessage: {
groupPolicy: "allowlist",
allowFrom: ["+15551234567"],
},
},
});
expect(
doctorWarnings.some(
(line) =>
line.includes('channels.imessage.groupPolicy is "allowlist"') &&
line.includes("does not fall back to allowFrom"),
),
).toBe(true);
});
it("drops unknown keys on repair", async () => {