🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

1. 🟡 Named Google Chat accounts inherit access-control settings from accounts.default (policy/allowlists)

Description

mergeGoogleChatAccountConfig now merges most of channels.googlechat.accounts.default into every non-default account. While intended to share webhook/audience defaults, the implementation inherits all remaining fields, including security-relevant access-control settings (DM/group policies and allowlists).

Impact in multi-account deployments:

• A named account that previously fell back to safe defaults (e.g., dm.policy defaulting to "pairing") will now silently inherit accounts.default.dm / groupPolicy / allowlists.
• If accounts.default is configured more permissively (e.g., dm.policy: "open" or dm.allowFrom: ["*"]), then every named account without an explicit override becomes equally permissive, potentially allowing unauthorized Google Chat users/spaces to trigger automations/commands on those accounts.

Vulnerable code:

const {
  enabled: _ignoredEnabled,
  dangerouslyAllowNameMatching: _ignoredDangerouslyAllowNameMatching,
  serviceAccount: _ignoredServiceAccount,
  serviceAccountRef: _ignoredServiceAccountRef,
  serviceAccountFile: _ignoredServiceAccountFile,
  ...defaultAccountShared
} = defaultAccountConfig;

return { ...defaultAccountShared, ...base, ...account } as GoogleChatAccountConfig;

This is security-sensitive because account.config is later used to enforce inbound access policy (DM/group allowlists, mention-gating, etc.), so inherited permissive defaults change the authorization behavior of other accounts.

Recommendation

Limit what is inherited from accounts.default to an explicit allowlist of fields that are safe/intended to be shared (e.g., webhook/audience routing fields), rather than inheriting almost everything.

For example:

const defaultAccountConfig = resolveAccountConfig(cfg, DEFAULT_ACCOUNT_ID) ?? {};
const sharedFromDefault: Partial<GoogleChatAccountConfig> = {
  audienceType: defaultAccountConfig.audienceType,
  audience: defaultAccountConfig.audience,
  webhookPath: defaultAccountConfig.webhookPath,
  webhookUrl: defaultAccountConfig.webhookUrl,
  ​// optionally: botUser if it is truly shared
};

return { ...sharedFromDefault, ...base, ...account } as GoogleChatAccountConfig;

Alternatively, if broader inheritance is desired, explicitly exclude access-control and policy keys (e.g., dm, dms, groupPolicy, groupAllowFrom, groups, requireMention, allowBots, etc.) so named accounts keep their own safe defaults unless configured.

Analyzed PR: #38492 at commit 5592999

_{Last updated on: 2026-03-07T03:54:25Z}

Greptile Summary

This PR fixes a regression in multi-account Google Chat webhook authentication by making mergeGoogleChatAccountConfig inherit shared fields from accounts.default for named accounts. The new merge order — { ...defaultAccountConfig, ...base, ...account } — correctly gives accounts.default the lowest priority while still allowing top-level and per-account values to override it.

• Core fix is sound: named accounts can now inherit shared audience, audienceType, and webhookPath from accounts.default without duplicating those fields, which matches the stated regression scenario.
• Precedence is correct: the two regression tests confirm that top-level channel config beats accounts.default, and per-account config beats both — consistent with the documented intent.
• enabled inheritance is a footgun: because the inheritance is unconditional, setting enabled: false inside accounts.default (e.g. to disable the anonymous/default account) will silently disable every named account too. This was not possible before the change. The fix would be to strip lifecycle-only fields (enabled, name) from the inherited defaults before merging, or at minimum to document this behavior.
• Minor inefficiency: when resolving the default account itself, resolveAccountConfig is called twice with DEFAULT_ACCOUNT_ID; the result stored in account is never used in that branch.

Confidence Score: 3/5

• Safe to merge for the primary use case, but the unconditional inheritance of enabled: false from accounts.default introduces a subtle regression risk for users who disable just the default account.
• The fix correctly addresses the stated regression and the precedence logic is sound. However, the enabled field inheritance side-effect is a real behavioral change that could silently disable all named accounts in edge-case configurations, warranting attention before merging.
• extensions/googlechat/src/accounts.ts — specifically the enabled field propagation in mergeGoogleChatAccountConfig

_{Last reviewed commit: 3a621e6}