fix(msteams): accept strict Bot Framework and Entra service tokens#56631
fix(msteams): accept strict Bot Framework and Entra service tokens#56631BradGroux merged 2 commits intoopenclaw:mainfrom
Conversation
openclaw-barnacle
bot
added
channel: msteams
There was a problem hiding this comment.
Pull request overview
This PR improves observability for inbound Microsoft Teams messages that are intentionally dropped by policy decisions, addressing previously silent drops reported in #56603.
Changes:
- Add
info-level logs for DM drops caused bydmPolicydecisions (disabled / not allowlisted). - Add
info-level logs for group message drops caused by route allowlists and group sender allowlists/policy. - Add authz-focused tests asserting
info-level drop logging for DM allowlist rejection and group empty-allowlist scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| extensions/msteams/src/monitor-handler/message-handler.ts | Emits new info logs for policy-based inbound drop paths (DM + group) to make default logging diagnosable. |
| extensions/msteams/src/monitor-handler/message-handler.authz.test.ts | Adds regression tests to assert the new info-level drop reasons are logged for key policy drop scenarios. |
Greptile SummaryThis PR addresses issue #56603 by promoting all six inbound-policy drop paths in the MS Teams message handler from silent Key observations:
Confidence Score: 5/5Safe to merge — purely additive observability change with no logic or data-handling impact. All findings are P2 (style/improvement suggestions). The changes are correct and well-scoped: only logging calls are added, no routing or authorization behaviour is altered. The two minor issues do not block merge. No files require special attention.
|
| Filename | Overview |
|---|---|
| extensions/msteams/src/monitor-handler/message-handler.ts | Adds log.info() calls for all six inbound policy-drop paths. Existing log.debug?.() calls are preserved, resulting in redundant dual-logging for four paths where payloads are identical. |
| extensions/msteams/src/monitor-handler/message-handler.authz.test.ts | Adds two new test cases for the DM allowlist-reject and empty group-allowlist drop paths. Four other new info logs remain without test assertions. |
Comments Outside Diff (1)
-
extensions/msteams/src/monitor-handler/message-handler.ts, line 198-212 (link)Redundant dual log calls with identical payloads
For four of the six drop paths the new
log.info(...)call and the retainedlog.debug?.()call carry the same message string and exactly the same fields. At debug log level every drop event will therefore produce two identical lines. The affected paths are:"dropping group message (not in team/channel allowlist)"— lines 198–211"dropping group message (groupPolicy: disabled)"— lines 228–233"dropping group message (groupPolicy: allowlist, no allowlist)"— lines 237–242"dropping group message (not in groupAllowFrom)"— lines 252–261
The
"dropping dm (not allowlisted)"and"dropping dm (dms disabled)"calls are fine because the new info-level calls include extra fields (dmPolicy,reason,sender,label) not present in their debug counterparts.For the four fully-redundant cases, consider removing the
log.debug?.()lines that are now completely subsumed by the correspondinglog.info()calls.Prompt To Fix With AI
This is a comment left during a code review. Path: extensions/msteams/src/monitor-handler/message-handler.ts Line: 198-212 Comment: **Redundant dual log calls with identical payloads** For four of the six drop paths the new `log.info(...)` call and the retained `log.debug?.()` call carry the same message string and exactly the same fields. At debug log level every drop event will therefore produce two identical lines. The affected paths are: - `"dropping group message (not in team/channel allowlist)"` — lines 198–211 - `"dropping group message (groupPolicy: disabled)"` — lines 228–233 - `"dropping group message (groupPolicy: allowlist, no allowlist)"` — lines 237–242 - `"dropping group message (not in groupAllowFrom)"` — lines 252–261 The `"dropping dm (not allowlisted)"` and `"dropping dm (dms disabled)"` calls are fine because the new info-level calls include extra fields (`dmPolicy`, `reason`, `sender`, `label`) not present in their debug counterparts. For the four fully-redundant cases, consider removing the `log.debug?.()` lines that are now completely subsumed by the corresponding `log.info()` calls. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/msteams/src/monitor-handler/message-handler.authz.test.ts
Line: 200-289
Comment:
**Incomplete test coverage for new info logs**
The PR adds six new `log.info(...)` calls across six distinct drop paths, but only two of them are exercised by new tests:
| Drop path | Tested? |
|---|---|
| `"dropping dm (dms disabled)"` | ✗ |
| `"dropping dm (not allowlisted)"` | ✓ (new test) |
| `"dropping group message (not in team/channel allowlist)"` | ✗ |
| `"dropping group message (groupPolicy: disabled)"` | ✗ |
| `"dropping group message (groupPolicy: allowlist, no allowlist)"` | ✓ (new test) |
| `"dropping group message (not in groupAllowFrom)"` | ✗ |
The untested paths are the main motivation for the PR (silent drops that are hard to diagnose), so it's worth adding assertions for at least the `"dms disabled"` and `"not in groupAllowFrom"` cases to guard against message text or field regressions.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/msteams/src/monitor-handler/message-handler.ts
Line: 198-212
Comment:
**Redundant dual log calls with identical payloads**
For four of the six drop paths the new `log.info(...)` call and the retained `log.debug?.()` call carry the same message string and exactly the same fields. At debug log level every drop event will therefore produce two identical lines. The affected paths are:
- `"dropping group message (not in team/channel allowlist)"` — lines 198–211
- `"dropping group message (groupPolicy: disabled)"` — lines 228–233
- `"dropping group message (groupPolicy: allowlist, no allowlist)"` — lines 237–242
- `"dropping group message (not in groupAllowFrom)"` — lines 252–261
The `"dropping dm (not allowlisted)"` and `"dropping dm (dms disabled)"` calls are fine because the new info-level calls include extra fields (`dmPolicy`, `reason`, `sender`, `label`) not present in their debug counterparts.
For the four fully-redundant cases, consider removing the `log.debug?.()` lines that are now completely subsumed by the corresponding `log.info()` calls.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "msteams: log policy-based inbound drops ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("logs an info drop reason when dmPolicy allowlist rejects a sender", async () => { | ||
| const { deps } = createDeps({ | ||
| channels: { | ||
| msteams: { | ||
| dmPolicy: "allowlist", | ||
| allowFrom: ["trusted-aad"], | ||
| }, | ||
| }, | ||
| } as OpenClawConfig); | ||
|
|
||
| const handler = createMSTeamsMessageHandler(deps); | ||
| await handler({ | ||
| activity: { | ||
| id: "msg-drop-dm", | ||
| type: "message", | ||
| text: "hello", | ||
| from: { | ||
| id: "attacker-id", | ||
| aadObjectId: "attacker-aad", | ||
| name: "Attacker", | ||
| }, | ||
| recipient: { | ||
| id: "bot-id", | ||
| name: "Bot", | ||
| }, | ||
| conversation: { | ||
| id: "a:personal-chat", | ||
| conversationType: "personal", | ||
| }, | ||
| channelData: {}, | ||
| attachments: [], | ||
| }, | ||
| sendActivity: vi.fn(async () => undefined), | ||
| } as unknown as Parameters<typeof handler>[0]); | ||
|
|
||
| expect(deps.log.info).toHaveBeenCalledWith( | ||
| "dropping dm (not allowlisted)", | ||
| expect.objectContaining({ | ||
| sender: "attacker-aad", | ||
| dmPolicy: "allowlist", | ||
| reason: "dmPolicy=allowlist (not allowlisted)", | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it("logs an info drop reason when group policy has an empty allowlist", async () => { | ||
| const { deps } = createDeps({ | ||
| channels: { | ||
| msteams: { | ||
| dmPolicy: "pairing", | ||
| allowFrom: [], | ||
| groupPolicy: "allowlist", | ||
| groupAllowFrom: [], | ||
| }, | ||
| }, | ||
| } as OpenClawConfig); | ||
|
|
||
| const handler = createMSTeamsMessageHandler(deps); | ||
| await handler({ | ||
| activity: { | ||
| id: "msg-drop-group", | ||
| type: "message", | ||
| text: "hello", | ||
| from: { | ||
| id: "attacker-id", | ||
| aadObjectId: "attacker-aad", | ||
| name: "Attacker", | ||
| }, | ||
| recipient: { | ||
| id: "bot-id", | ||
| name: "Bot", | ||
| }, | ||
| conversation: { | ||
| id: "19:[email protected]", | ||
| conversationType: "groupChat", | ||
| }, | ||
| channelData: {}, | ||
| attachments: [], | ||
| }, | ||
| sendActivity: vi.fn(async () => undefined), | ||
| } as unknown as Parameters<typeof handler>[0]); | ||
|
|
||
| expect(deps.log.info).toHaveBeenCalledWith( | ||
| "dropping group message (groupPolicy: allowlist, no allowlist)", | ||
| expect.objectContaining({ | ||
| conversationId: "19:[email protected]", | ||
| }), | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The PR adds six new log.info(...) calls across six distinct drop paths, but only two of them are exercised by new tests:
| Drop path | Tested? |
|---|---|
"dropping dm (dms disabled)" |
✗ |
"dropping dm (not allowlisted)" |
✓ (new test) |
"dropping group message (not in team/channel allowlist)" |
✗ |
"dropping group message (groupPolicy: disabled)" |
✗ |
"dropping group message (groupPolicy: allowlist, no allowlist)" |
✓ (new test) |
"dropping group message (not in groupAllowFrom)" |
✗ |
The untested paths are the main motivation for the PR (silent drops that are hard to diagnose), so it's worth adding assertions for at least the "dms disabled" and "not in groupAllowFrom" cases to guard against message text or field regressions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/monitor-handler/message-handler.authz.test.ts
Line: 200-289
Comment:
**Incomplete test coverage for new info logs**
The PR adds six new `log.info(...)` calls across six distinct drop paths, but only two of them are exercised by new tests:
| Drop path | Tested? |
|---|---|
| `"dropping dm (dms disabled)"` | ✗ |
| `"dropping dm (not allowlisted)"` | ✓ (new test) |
| `"dropping group message (not in team/channel allowlist)"` | ✗ |
| `"dropping group message (groupPolicy: disabled)"` | ✗ |
| `"dropping group message (groupPolicy: allowlist, no allowlist)"` | ✓ (new test) |
| `"dropping group message (not in groupAllowFrom)"` | ✗ |
The untested paths are the main motivation for the PR (silent drops that are hard to diagnose), so it's worth adding assertions for at least the `"dms disabled"` and `"not in groupAllowFrom"` cases to guard against message text or field regressions.
How can I resolve this? If you propose a fix, please make it concise.
BradGroux
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d162d282c0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const legacyResult = await botFrameworkValidator.validateAccessToken(token, overrides); | ||
| if (legacyResult != null) { |
There was a problem hiding this comment.
validate() currently wraps both issuer checks in a single try/catch, so if botFrameworkValidator.validateAccessToken(...) throws, the function returns false immediately and never evaluates the Entra validator. This causes valid Entra-signed inbound requests to be rejected whenever the legacy path errors (for example issuer/JWKS mismatch behavior surfaced as exceptions), defeating the intended dual-issuer fallback and producing 401s for legitimate traffic.
Useful? React with 👍 / 👎.
…penclaw#56631) * msteams: log policy-based inbound drops at info level * fix(msteams): validate Bot Framework and Entra service token issuers --------- Co-authored-by: Brad Groux <[email protected]>
…penclaw#56631) * msteams: log policy-based inbound drops at info level * fix(msteams): validate Bot Framework and Entra service token issuers --------- Co-authored-by: Brad Groux <[email protected]>
…penclaw#56631) * msteams: log policy-based inbound drops at info level * fix(msteams): validate Bot Framework and Entra service token issuers --------- Co-authored-by: Brad Groux <[email protected]>
…penclaw#56631) * msteams: log policy-based inbound drops at info level * fix(msteams): validate Bot Framework and Entra service token issuers --------- Co-authored-by: Brad Groux <[email protected]>
Summary
Root cause fixed
Webhook auth only validated tokens against the legacy Bot Framework issuer (https://api.botframework.com) and Bot Framework JWKS. Valid Entra-signed service tokens were rejected even with correct signature and audience.
Testing
Closes #56603