Skip to content

msteams: fix sender allowlist bypass when route allowlist is configured (GHSA-g7cr-9h7q-4qxq)#49582

Merged
BradGroux merged 1 commit intoopenclaw:mainfrom
sudie-codes:fix/msteams-sender-allowlist-bypass
Mar 20, 2026
Merged

msteams: fix sender allowlist bypass when route allowlist is configured (GHSA-g7cr-9h7q-4qxq)#49582
BradGroux merged 1 commit intoopenclaw:mainfrom
sudie-codes:fix/msteams-sender-allowlist-bypass

Conversation

@sudie-codes
Copy link
Copy Markdown
Contributor

@sudie-codes sudie-codes commented Mar 18, 2026

Summary

Fixes GHSA-g7cr-9h7q-4qxq — Sender allowlist bypass when a route allowlist (teams config) is configured but the sender allowlist is empty.

What & Why

Problem: When a Teams route-level allowlist (per-team/per-channel config in channels.msteams.teams) is configured but the sender-level groupAllowFrom is empty or absent, the sender check is effectively bypassed — any Teams user can interact with the bot in that channel.

Root cause: resolveSenderScopedGroupPolicy in group-access.ts returns "open" when groupAllowFrom is empty (line 51: params.groupAllowFrom.length > 0 ? "allowlist" : "open"). When a route allowlist (teams config with channel gates) was active but sender allowlist was empty, this downgraded the effective policy from "allowlist" to "open", allowing any Teams user to send messages.

Fix: In extensions/msteams/src/monitor-handler/message-handler.ts, when channelGate.allowlistConfigured && effectiveGroupAllowFrom.length === 0, preserve the configured groupPolicy instead of delegating to resolveSenderScopedGroupPolicy. This ensures an empty sender allowlist with a configured route allowlist defaults to "deny all senders" rather than "allow all senders".

Files changed: extensions/msteams/src/monitor-handler/message-handler.ts

Screenshots

N/A — This is a server-side authorization logic fix. The change affects policy evaluation in the message handler's auth layer. Verification requires a running bot with a configured route allowlist and empty sender allowlist. The fix is validated via the existing regression test.

Test Results

Security

  • Advisory: GHSA-g7cr-9h7q-4qxq
  • SECURITY.md reviewed before implementation
  • Fix preserves all existing allowlist behavior when sender allowlist is non-empty

AI Disclosure

  • AI-assisted (Claude Code with team orchestration)
  • Fully tested — existing regression test validates the fix
  • I understand what the code does: prevents policy downgrade from "allowlist" to "open" when route gates exist but sender allowlist is empty

…ed (GHSA-g7cr-9h7q-4qxq)

When a route-level (teams/channel) allowlist was configured but the sender
allowlist (allowFrom/groupAllowFrom) was empty, resolveSenderScopedGroupPolicy
would downgrade the effective group policy from "allowlist" to "open", allowing
any Teams user to interact with the bot.

The fix: when channelGate.allowlistConfigured is true and effectiveGroupAllowFrom
is empty, preserve the configured groupPolicy ("allowlist") rather than letting
it be downgraded to "open". This ensures an empty sender allowlist with an active
route allowlist means deny-all rather than allow-all.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@openclaw-barnacle openclaw-barnacle bot added channel: msteams Channel integration: msteams size: XS labels Mar 18, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 18, 2026

Greptile Summary

This PR closes a security bypass (GHSA-g7cr-9h7q-4qxq) in the MS Teams message handler where configuring only a route-level team/channel allowlist (channels.msteams.teams) — without a sender allowlist (groupAllowFrom) — caused resolveSenderScopedGroupPolicy to return "open", effectively allowing any Teams user to interact with the bot.

Changes:

  • In message-handler.ts, when channelGate.allowlistConfigured && effectiveGroupAllowFrom.length === 0, the effective senderGroupPolicy is now set to the configured groupPolicy directly rather than delegating to resolveSenderScopedGroupPolicy (which unconditionally returns "open" for empty groupAllowFrom). This preserves the "allowlist" or "disabled" policy and propagates a deny-all result through resolveDmGroupAccessWithLists.
  • A clear inline comment explains the GHSA reference and the specific bypass scenario being closed.
  • The accompanying regression test (message-handler.authz.test.ts) — which was already present in the base branch — now passes.

Behavioral note: The explicit group-message sender check at line 251 (evaluateSenderGroupAccessForPolicy) references groupPolicy directly (not senderGroupPolicy) and was hardened by a prior commit (88aee9161e). This PR closes the remaining policy-propagation gap by ensuring the senderGroupPolicy fed into resolveDmGroupAccessWithLists is also correct when the route allowlist is active.

Confidence Score: 4/5

  • This PR is safe to merge — it is a focused, well-tested security fix with no observed regressions in the surrounding authorization logic.
  • The change is minimal (7 lines, single file), correctly scoped to the vulnerability scenario, accompanied by a targeted regression test, and includes a clear comment referencing the GHSA advisory. Historical commits confirm the surrounding group-message check already uses groupPolicy directly, so the risk of accidental regression is low. Score is 4 rather than 5 because the senderGroupPolicyresolveDmGroupAccessWithListsaccess.decision chain only matters for the DM branch of the authorization check (isDirectMessage && guard), making the relationship between the fix and the described group-chat bypass slightly indirect; a more explicit architectural comment or a second test asserting the access.decision path for group chats would improve confidence further.
  • No files require special attention beyond the changed extensions/msteams/src/monitor-handler/message-handler.ts.

Last reviewed commit: "msteams: fix sender ..."

@BradGroux BradGroux self-assigned this Mar 20, 2026
Copy link
Copy Markdown
Contributor

@BradGroux BradGroux left a comment

Choose a reason for hiding this comment

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

Clean, minimal security fix. The short-circuit when channelGate.allowlistConfigured && effectiveGroupAllowFrom.length === 0 correctly preserves the deny-all behavior instead of delegating to the function that would weaken it.

Recommend adding a regression test for the specific bypass scenario (configured route allowlist + empty sender allowlist = should stay at groupPolicy). Not blocking, this should ship fast.

✅ Merge first in the series.

@BradGroux BradGroux merged commit 897cda7 into openclaw:main Mar 20, 2026
35 of 44 checks passed
JohnJAS pushed a commit to JohnJAS/openclaw that referenced this pull request Mar 22, 2026
…ed (GHSA-g7cr-9h7q-4qxq) (openclaw#49582)

When a route-level (teams/channel) allowlist was configured but the sender
allowlist (allowFrom/groupAllowFrom) was empty, resolveSenderScopedGroupPolicy
would downgrade the effective group policy from "allowlist" to "open", allowing
any Teams user to interact with the bot.

The fix: when channelGate.allowlistConfigured is true and effectiveGroupAllowFrom
is empty, preserve the configured groupPolicy ("allowlist") rather than letting
it be downgraded to "open". This ensures an empty sender allowlist with an active
route allowlist means deny-all rather than allow-all.

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
pholpaphankorn pushed a commit to pholpaphankorn/openclaw that referenced this pull request Mar 22, 2026
…ed (GHSA-g7cr-9h7q-4qxq) (openclaw#49582)

When a route-level (teams/channel) allowlist was configured but the sender
allowlist (allowFrom/groupAllowFrom) was empty, resolveSenderScopedGroupPolicy
would downgrade the effective group policy from "allowlist" to "open", allowing
any Teams user to interact with the bot.

The fix: when channelGate.allowlistConfigured is true and effectiveGroupAllowFrom
is empty, preserve the configured groupPolicy ("allowlist") rather than letting
it be downgraded to "open". This ensures an empty sender allowlist with an active
route allowlist means deny-all rather than allow-all.

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
frankekn pushed a commit to artwalker/openclaw that referenced this pull request Mar 23, 2026
…ed (GHSA-g7cr-9h7q-4qxq) (openclaw#49582)

When a route-level (teams/channel) allowlist was configured but the sender
allowlist (allowFrom/groupAllowFrom) was empty, resolveSenderScopedGroupPolicy
would downgrade the effective group policy from "allowlist" to "open", allowing
any Teams user to interact with the bot.

The fix: when channelGate.allowlistConfigured is true and effectiveGroupAllowFrom
is empty, preserve the configured groupPolicy ("allowlist") rather than letting
it be downgraded to "open". This ensures an empty sender allowlist with an active
route allowlist means deny-all rather than allow-all.

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
minupla pushed a commit to minupla/openclaw that referenced this pull request Mar 23, 2026
…ed (GHSA-g7cr-9h7q-4qxq) (openclaw#49582)

When a route-level (teams/channel) allowlist was configured but the sender
allowlist (allowFrom/groupAllowFrom) was empty, resolveSenderScopedGroupPolicy
would downgrade the effective group policy from "allowlist" to "open", allowing
any Teams user to interact with the bot.

The fix: when channelGate.allowlistConfigured is true and effectiveGroupAllowFrom
is empty, preserve the configured groupPolicy ("allowlist") rather than letting
it be downgraded to "open". This ensures an empty sender allowlist with an active
route allowlist means deny-all rather than allow-all.

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
alexey-pelykh pushed a commit to remoteclaw/remoteclaw that referenced this pull request Mar 24, 2026
…ed (GHSA-g7cr-9h7q-4qxq) (openclaw#49582)

When a route-level (teams/channel) allowlist was configured but the sender
allowlist (allowFrom/groupAllowFrom) was empty, resolveSenderScopedGroupPolicy
would downgrade the effective group policy from "allowlist" to "open", allowing
any Teams user to interact with the bot.

The fix: when channelGate.allowlistConfigured is true and effectiveGroupAllowFrom
is empty, preserve the configured groupPolicy ("allowlist") rather than letting
it be downgraded to "open". This ensures an empty sender allowlist with an active
route allowlist means deny-all rather than allow-all.

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
(cherry picked from commit 897cda7)
alexey-pelykh added a commit to remoteclaw/remoteclaw that referenced this pull request Mar 24, 2026
* fix(msteams): resolve Graph API chat ID for DM file uploads (openclaw#49585)

Fixes openclaw#35822 — Bot Framework conversation.id format is incompatible with
Graph API /chats/{chatId}. Added resolveGraphChatId() to look up the
Graph-native chat ID via GET /me/chats, cached in the conversation store.

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
(cherry picked from commit 06845a1)

* test: fix fetch mock typing

(cherry picked from commit 0f43dc4)

* fix: restore repo-wide gate after upstream sync

(cherry picked from commit 14074d3)

* test(msteams): align adapter doubles with interfaces

(cherry picked from commit 5b7ae24)

* test: tighten msteams regression assertions

(cherry picked from commit c8a36c6)

* test: dedupe msteams attachment redirects

(cherry picked from commit 017c0dc)

* MSTeams: move outbound session routing behind plugin boundary

(cherry picked from commit 028f3c4)

* fix: remove session-route.ts — depends on missing upstream infrastructure

* test(msteams): cover graph helpers

(cherry picked from commit 1ea2593)

* fix(test): split msteams attachment helpers

(cherry picked from commit 23c8af3)

* test: share directory runtime helpers

(cherry picked from commit 38b0986)

* fix: stabilize build dependency resolution (openclaw#49928)

* build: mirror uuid for msteams

Add uuid to both the msteams bundled extension and the root package so the workspace build can resolve @microsoft/agents-hosting during tsdown while standalone extension installs also have the runtime dependency available.

Regeneration-Prompt: |
  pnpm build failed because @microsoft/agents-hosting 1.3.1 requires uuid in its published JS but does not declare it in its package manifest. The msteams extension dynamically imports that package, and the workspace build resolves it from the root dependency graph. Mirror uuid into the root package for workspace builds and keep it in extensions/msteams/package.json so standalone plugin installs also resolve it. Update the lockfile to match the manifest changes.

* build: prune stale plugin dist symlinks

Remove stale dist and dist-runtime plugin node_modules symlinks before tsdown runs. These links point back into extension installs, and tsdown's clean step can traverse them on rebuilds and hollow out the active pnpm dependency tree before plugin-sdk declaration generation runs.

Regeneration-Prompt: |
  pnpm build was intermittently failing in the plugin-sdk:dts phase after earlier build steps had already run. The symptom looked like missing root packages such as zod, ajv, commander, and undici even though a fresh install briefly fixed the problem. Investigate the build pipeline step by step rather than patching TypeScript errors. Confirm whether rebuilds mutate node_modules, identify the first step that does it, and preserve existing runtime-postbuild behavior.
  The key constraint is that dist and dist-runtime plugin node_modules links are intentional for runtime packaging, so do not remove that feature globally. Instead, make rebuilds safe by deleting only stale symlinks left in generated output before invoking tsdown, so tsdown cleanup cannot recurse back into the live pnpm install tree. Verify with repeated pnpm build runs.
(cherry picked from commit 505d140)

* test(msteams): cover store and live directory helpers

(cherry picked from commit 55e0c63)

* test(msteams): cover setup wizard status

(cherry picked from commit 653d69e)

* test: tighten msteams regression assertions

(cherry picked from commit 689a734)

* refactor: share teams drive upload flow

(cherry picked from commit 6b04ab1)

* test(msteams): cover routing and setup

(cherry picked from commit 774a206)

* msteams: extend MSTeamsAdapter and MSTeamsActivityHandler types; implement self() (openclaw#49929)

- Add updateActivity/deleteActivity to MSTeamsAdapter
- Add onReactionsAdded/onReactionsRemoved to MSTeamsActivityHandler
- Implement directory self() to return bot identity from appId credential
- Add tests for self() in channel.directory.test.ts

(cherry picked from commit 7c3af37)

* test(msteams): cover upload and webhook helpers

(cherry picked from commit 7d11f6c)

* msteams: fix sender allowlist bypass when route allowlist is configured (GHSA-g7cr-9h7q-4qxq) (openclaw#49582)

When a route-level (teams/channel) allowlist was configured but the sender
allowlist (allowFrom/groupAllowFrom) was empty, resolveSenderScopedGroupPolicy
would downgrade the effective group policy from "allowlist" to "open", allowing
any Teams user to interact with the bot.

The fix: when channelGate.allowlistConfigured is true and effectiveGroupAllowFrom
is empty, preserve the configured groupPolicy ("allowlist") rather than letting
it be downgraded to "open". This ensures an empty sender allowlist with an active
route allowlist means deny-all rather than allow-all.

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
(cherry picked from commit 897cda7)

* fix(msteams): batch multi-block replies into single continueConversation call (openclaw#29379) (openclaw#49587)

Teams silently drops blocks 2+ when each deliver() opens its own
continueConversation() call. Accumulate rendered messages across all
deliver() calls and flush them together in markDispatchIdle().

On batch failure, retry each message individually so trailing blocks
are not silently lost. Log a warning when any individual messages fail
so flush failures are visible in logs.

(cherry picked from commit 8b5eeba)

* test(msteams): cover poll and file-card helpers

(cherry picked from commit 8ff277d)

* test: dedupe msteams consent auth fixtures

(cherry picked from commit a9d8518)

* refactor: share dual text command gating

(cherry picked from commit b61bc49)

* test: share msteams safe fetch assertions

(cherry picked from commit d4d0091)

* MSTeams: lazy-load runtime-heavy channel paths

(cherry picked from commit da4f825)

* fix(msteams): isolate probe test env credentials

(cherry picked from commit e9078b3)

* test: dedupe msteams policy route fixtures

(cherry picked from commit f2300f4)

* fix: fix remaining openclaw references in cherry-picked msteams files

* fix: adapt cherry-picks for fork TS strictness

* fix: revert cross-cutting refactors, keep msteams-specific changes only

* fix: format cherry-picked files with oxfmt

---------

Co-authored-by: sudie-codes <[email protected]>
Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
Co-authored-by: Peter Steinberger <[email protected]>
Co-authored-by: Vincent Koc <[email protected]>
Co-authored-by: Gustavo Madeira Santana <[email protected]>
Co-authored-by: Josh Lehman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: msteams Channel integration: msteams size: XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants