Skip to content

fix(msteams): gate startup user allowlist resolution [AI]#79003

Merged
pgondhi987 merged 4 commits intoopenclaw:mainfrom
pgondhi987:fix/fix-580
May 8, 2026
Merged

fix(msteams): gate startup user allowlist resolution [AI]#79003
pgondhi987 merged 4 commits intoopenclaw:mainfrom
pgondhi987:fix/fix-580

Conversation

@pgondhi987
Copy link
Copy Markdown
Contributor

@pgondhi987 pgondhi987 commented May 7, 2026

Summary

  • Problem: MS Teams startup allowlist preparation resolved configured user entries before applying the existing name-matching policy.
  • Why it matters: Startup config preparation and runtime access checks should enforce the same identity-matching boundary.
  • What changed: allowFrom and groupAllowFrom Graph-backed user resolution now only runs when dangerouslyAllowNameMatching is enabled.
  • What did NOT change (scope boundary): Team/channel route resolution, config shape, permissions, and network call definitions are unchanged.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

  • Related: N/A
  • This PR fixes a bug or regression

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: MS Teams startup no longer rewrites user allowlist entries through Graph lookup unless mutable name matching is explicitly enabled.
  • Real environment tested: Not run in this metadata drafting step.
  • Exact steps or command run after this patch: Not run in this metadata drafting step.
  • Evidence after fix (screenshot, recording, terminal capture, console output, redacted runtime log, linked artifact, or copied live output): Added lifecycle regression coverage in extensions/msteams/src/monitor.lifecycle.test.ts.
  • Observed result after fix: Source-level change gates user allowlist resolution behind isDangerousNameMatchingEnabled.
  • What was not tested: Live MS Teams tenant behavior and local command validation.
  • Before evidence (optional but encouraged): N/A

Root Cause (if applicable)

  • Root cause: The MS Teams startup resolver merged Graph-resolved user IDs into allowlists without consulting the existing name-matching opt-in used by runtime access checks.
  • Missing detection / guardrail: Lifecycle tests did not assert that startup user allowlist resolution is skipped when name matching is disabled.
  • Contributing context (if known): Team/channel route lookup and user identity lookup shared the startup preparation flow, but only user identity lookup needs the runtime name-matching guard.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file: extensions/msteams/src/monitor.lifecycle.test.ts
  • Scenario the test should lock in: Startup skips user allowlist Graph resolution by default, preserves configured entries, still resolves team/channel route keys, and resolves users only when name matching is enabled.
  • Why this is the smallest reliable guardrail: The lifecycle test covers the boundary where monitor startup rewrites config before registering message handlers.
  • Existing test that already covers this (if any): Existing runtime policy tests covered name matching after startup, but not startup config rewriting.
  • If no new test is added, why not: N/A

User-visible / Behavior Changes

MS Teams allowFrom and groupAllowFrom entries that rely on user-name or email lookup are no longer resolved at startup unless channels.msteams.dangerouslyAllowNameMatching is enabled. Stable user IDs continue to work as configured.

Diagram (if applicable)

Before:
startup config -> user lookup -> rewritten allowlist -> runtime access check

After:
startup config -> name-matching opt-in check -> optional user lookup -> runtime access check

Security Impact (required)

  • New permissions/capabilities? (Yes/No) No
  • Secrets/tokens handling changed? (Yes/No) No
  • New/changed network calls? (Yes/No) No
  • Command/tool execution surface changed? (Yes/No) No
  • Data access scope changed? (Yes/No) No
  • If any Yes, explain risk + mitigation: N/A

Repro + Verification

Environment

  • OS: Not run in this metadata drafting step
  • Runtime/container: Not run in this metadata drafting step
  • Model/provider: N/A
  • Integration/channel (if any): MS Teams
  • Relevant config (redacted): channels.msteams.allowFrom / channels.msteams.groupAllowFrom with dangerouslyAllowNameMatching unset or enabled

Steps

  1. Configure MS Teams with allowFrom and groupAllowFrom entries that require Graph user lookup.
  2. Start the provider with dangerouslyAllowNameMatching unset.
  3. Confirm startup preserves those entries and does not invoke user allowlist resolution.
  4. Repeat with dangerouslyAllowNameMatching: true.
  5. Confirm startup resolves and merges user IDs only in the opt-in case.

Expected

  • User allowlist Graph resolution is skipped by default and only runs when name matching is explicitly enabled.

Actual

  • Implemented in extensions/msteams/src/monitor.ts; regression coverage added in extensions/msteams/src/monitor.lifecycle.test.ts.

Evidence

Attach at least one:

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

  • Verified scenarios: Source-level review of the startup resolver path and regression test assertions for disabled and enabled name matching.
  • Edge cases checked: allowFrom, groupAllowFrom, and team/channel route resolution staying active independently of user lookup.
  • What you did not verify: Local test execution, live MS Teams tenant behavior, or CI.

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.

Compatibility / Migration

  • Backward compatible? (Yes/No) Yes
  • Config/env changes? (Yes/No) No
  • Migration needed? (Yes/No) No
  • If yes, exact upgrade steps: N/A

Risks and Mitigations

  • Risk: Operators relying on startup lookup for mutable MS Teams user entries must explicitly enable the existing opt-in.
    • Mitigation: Stable user IDs remain supported without the opt-in, and the opt-in preserves the previous lookup behavior when intentionally enabled.

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: S maintainer Maintainer-authored PR labels May 7, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 7, 2026 •
edited
Loading

Codex review: needs real behavior proof before merge.

Summary
The branch gates MS Teams startup Graph user allowlist resolution behind dangerouslyAllowNameMatching, preserves stable user ID entries, and adds lifecycle tests for disabled and enabled name matching.

Reproducibility: yes. source-reproducible: current main resolves MS Teams user allowlists through Graph at startup before applying the existing dangerous name-matching policy. A live tenant run was not performed in this read-only review.

Real behavior proof
Needs real behavior proof before merge: The PR body says no real environment or exact command was run; lifecycle tests are useful but do not satisfy the external PR real behavior proof gate. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
External PR needs contributor-supplied real behavior proof and a changelog entry; the protected maintainer label also requires human handling.

Security
Cleared: The diff narrows MS Teams Graph lookup by default and does not add permissions, dependencies, secrets handling, workflows, or new network surfaces.

Review findings

  • [P3] Add the required changelog entry — extensions/msteams/src/monitor.ts:111
Review details

Best possible solution:

Land the narrow startup-gating fix after the contributor adds redacted real behavior proof and a single-line Unreleased changelog entry, then let maintainer review handle the protected label.

Do we have a high-confidence way to reproduce the issue?

Yes, source-reproducible: current main resolves MS Teams user allowlists through Graph at startup before applying the existing dangerous name-matching policy. A live tenant run was not performed in this read-only review.

Is this the best way to solve the issue?

Yes for the code direction: gating only user Graph lookup while keeping stable IDs and team/channel route resolution intact is the narrow maintainable fix. Merge should wait for proof and changelog cleanup.

Full review comments:

  • [P3] Add the required changelog entry — extensions/msteams/src/monitor.ts:111
    This PR changes user-facing MS Teams allowlist behavior, and repo policy requires a CHANGELOG.md entry for user-facing fix changes. Please add a single-line Unreleased entry before merge.
    Confidence: 0.86

Overall correctness: patch is correct
Overall confidence: 0.82

What I checked:

Likely related people:

  • steipete: Introduced the id-only allowlist default with the dangerous name-matching opt-in, then recently maintained the dangerous-name matching helpers and MS Teams allowlist surfaces. (role: introduced policy and recent maintainer; confidence: high; commits: cfa44ea6b4f7, 161d9841dc6a, b97ba0ade2b6; files: src/config/dangerous-name-matching.ts, extensions/msteams/runtime-api.ts, extensions/msteams/src/policy.ts)
  • BradGroux: Authored a recent MS Teams startup allowlist resolver fix for team/channel key resolution and is co-credited on recent MS Teams monitor/error-path work adjacent to this startup preparation path. (role: MS Teams allowlist resolver contributor; confidence: medium; commits: 568b0a22bb02, eecda912ee75; files: extensions/msteams/src/resolve-allowlist.ts, extensions/msteams/src/monitor.ts, docs/channels/msteams.md)
  • vincentkoc: Recently maintained MS Teams plugin SDK import boundaries, allowlist imports, and MS Teams docs around the affected plugin surface. (role: recent adjacent maintainer; confidence: medium; commits: 3cc83cb81ec3, 603567754559, 462b96b33fa9; files: extensions/msteams/src/monitor.ts, extensions/msteams/src/policy.ts, docs/channels/msteams.md)

Remaining risk / open question:

  • The PR body provides test-only proof and explicitly says no real environment or exact command was run after the patch.
  • This user-facing MS Teams fix has no CHANGELOG.md entry in the branch diff.
  • The protected maintainer label means this PR needs explicit maintainer handling before merge.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 95a1c915312a.

@pgondhi987
Copy link
Copy Markdown
Contributor Author

pgondhi987 commented May 8, 2026

Not applicable to this automation stage; changelog/release-note and external real behavior proof requirements are handled outside auto-pr stages.

Quoted comment from @clawsweeper:

Codex review: needs real behavior proof before merge.

Summary
The branch gates MS Teams startup Graph user allowlist resolution behind dangerouslyAllowNameMatching, preserves stable user ID entries, and adds lifecycle tests for disabled and enabled name matching.

Reproducibility: yes. source-reproducible: current main resolves MS Teams user allowlists through Graph at startup before applying the existing dangerous name-matching policy. A live tenant run was not performed in this read-only review.

Real behavior proof
Needs real behavior proof before merge: The PR body says no real environment or exact command was run; lifecycle tests are useful but do not satisfy the external PR real behavior proof gate. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
External PR needs contributor-supplied real behavior proof and a changelog entry; the protected maintainer label also requires human handling.

Security
Cleared: The diff narrows MS Teams Graph lookup by default and does not add permissions, dependencies, secrets handling, workflows, or new network surfaces.

Review findings

  • [P3] Add the required changelog entry — extensions/msteams/src/monitor.ts:111
Review details

Best possible solution:

Land the narrow startup-gating fix after the contributor adds redacted real behavior proof and a single-line Unreleased changelog entry, then let maintainer review handle the protected label.

Do we have a high-confidence way to reproduce the issue?

Yes, source-reproducible: current main resolves MS Teams user allowlists through Graph at startup before applying the existing dangerous name-matching policy. A live tenant run was not performed in this read-only review.

Is this the best way to solve the issue?

Yes for the code direction: gating only user Graph lookup while keeping stable IDs and team/channel route resolution intact is the narrow maintainable fix. Merge should wait for proof and changelog cleanup.

Full review comments:

  • [P3] Add the required changelog entry — extensions/msteams/src/monitor.ts:111
    This PR changes user-facing MS Teams allowlist behavior, and repo policy requires a CHANGELOG.md entry for user-facing fix changes. Please add a single-line Unreleased entry before merge.
    Confidence: 0.86

Overall correctness: patch is correct
Overall confidence: 0.82

What I checked:

Likely related people:

  • steipete: Introduced the id-only allowlist default with the dangerous name-matching opt-in, then recently maintained the dangerous-name matching helpers and MS Teams allowlist surfaces. (role: introduced policy and recent maintainer; confidence: high; commits: cfa44ea6b4f7, 161d9841dc6a, b97ba0ade2b6; files: src/config/dangerous-name-matching.ts, extensions/msteams/runtime-api.ts, extensions/msteams/src/policy.ts)
  • BradGroux: Authored a recent MS Teams startup allowlist resolver fix for team/channel key resolution and is co-credited on recent MS Teams monitor/error-path work adjacent to this startup preparation path. (role: MS Teams allowlist resolver contributor; confidence: medium; commits: 568b0a22bb02, eecda912ee75; files: extensions/msteams/src/resolve-allowlist.ts, extensions/msteams/src/monitor.ts, docs/channels/msteams.md)
  • vincentkoc: Recently maintained MS Teams plugin SDK import boundaries, allowlist imports, and MS Teams docs around the affected plugin surface. (role: recent adjacent maintainer; confidence: medium; commits: 3cc83cb81ec3, 603567754559, 462b96b33fa9; files: extensions/msteams/src/monitor.ts, extensions/msteams/src/policy.ts, docs/channels/msteams.md)

Remaining risk / open question:

  • The PR body provides test-only proof and explicitly says no real environment or exact command was run after the patch.
  • This user-facing MS Teams fix has no CHANGELOG.md entry in the branch diff.
  • The protected maintainer label means this PR needs explicit maintainer handling before merge.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 95a1c915312a.

@pgondhi987 pgondhi987 merged commit c1edfaf into openclaw:main May 8, 2026
15 of 16 checks passed
@BradGroux BradGroux mentioned this pull request May 8, 2026
Draft
@github-actions github-actions Bot mentioned this pull request May 8, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request May 9, 2026
@pgondhi987
fix(msteams): gate startup user allowlist resolution [AI] (openclaw#7…
3a49da2 
…9003)

* fix: gate msteams user allowlist name resolution

* addressing codex review

* docs: add changelog entry for PR merge
rogerdigital pushed a commit to rogerdigital/openclaw that referenced this pull request May 9, 2026
@pgondhi987 @rogerdigital
fix(msteams): gate startup user allowlist resolution [AI] (openclaw#7…
5203e0c 
…9003)

* fix: gate msteams user allowlist name resolution

* addressing codex review

* docs: add changelog entry for PR merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

channel: msteams Channel integration: msteams maintainer Maintainer-authored PR size: S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@pgondhi987