fix(msteams): ignore blank certificate settings by wahaha1223 · Pull Request #109112 · openclaw/openclaw

wahaha1223 · 2026-07-16T15:18:02Z

What Problem This Solves

Fixes an issue where operators using Microsoft Teams federated authentication could have whitespace-only certificate settings reported as configured. Gateway startup would then try to read a certificate file whose path consisted only of spaces instead of treating the certificate credential as missing.

Why This Change Was Made

Federated certificate settings now use owner-local config-over-environment resolvers. Blank values no longer shadow valid environment values, certificate paths keep their original filesystem bytes, and identifier-like thumbprint and managed-identity values remain normalized. Secret authentication and managed-identity selection are unchanged.

User Impact

Microsoft Teams setup and status now reject blank certificate configuration consistently, and a blank config field can correctly fall back to a valid environment setting without changing the selected certificate path.

AI-assisted.

Evidence

Local-machine production-function proof on Node v24.18.0:

Before / negative control against exact latest main aaa37a0c264a9b2bc25c6d5a8e98ede130455fed: in a detached worktree, imported extensions/msteams/src/token.ts, set appId, tenantId, authType: "federated", and whitespace-only config/env certificate fields, then called both exported credential functions.
```
{"configured":true,"resolved":{"type":"federated","appId":"app-id","tenantId":"tenant-id","certificatePath":"   ","certificateThumbprint":"   "}}
```
This is RED: the production resolver returned federated credentials that createFederatedApp passes to readFileSync, and channel/setup callers use the resolver result as their configured predicate.
After on current exact head c670d785a614a33c58385e8e53e9f565c5dee733: imported the same exported production functions, cleared the Microsoft Teams federated-auth environment, evaluated whitespace-only config, then evaluated the same config with padded valid environment values.
```
{"head":"c670d785a614a33c58385e8e53e9f565c5dee733","blank":{"configured":false,"resolved":null},"fallback":{"configured":true,"resolved":{"type":"federated","appId":"app-id","tenantId":"tenant-id","certificatePath":"  /env/cert.pem  ","certificateThumbprint":"EEFF0011"}}}
```
The padded certificate path remains byte-for-byte unchanged because it is a filesystem path; the thumbprint is normalized as an identifier.
Focused regression and controls on the same exact head:
```
node scripts/run-vitest.mjs extensions/msteams/src/token.test.ts
Test Files  1 passed (1)
Tests       23 passed (23)
```
The focused file also keeps valid certificate config/env, secret credentials, system/user managed identity, and explicit useManagedIdentity=false behavior covered.
./node_modules/.bin/oxfmt --check extensions/msteams/src/token.ts extensions/msteams/src/token.test.ts passed on the same exact tree; git diff --check passed.

Not tested: a live Microsoft Teams certificate login, because no tenant certificate credentials were available. The changed behavior is entirely before SDK/network dispatch; the proof drives the exported production credential resolver that all channel, setup, probe, graph, monitor, and send-context callers consume.

clawsweeper · 2026-07-16T16:40:12Z

Codex review: needs maintainer review before merge. Reviewed July 19, 2026, 12:18 AM ET / 04:18 UTC.

Summary
The PR makes Microsoft Teams federated certificate settings treat whitespace-only config or environment values as unset while preserving nonblank certificate-path bytes and normalizing identifier-like values.

PR surface: Source +23, Tests +36. Total +59 across 2 files.

Reproducibility: yes. for the resolver-level bug: the supplied before/after production-function transcript exercises the exported credential functions with whitespace-only certificate values, and the focused regression tests cover the corrected outcomes.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

[P2] Refresh the branch against current main and rerun the focused resolver test before merge.

Risk before merge

[P1] The branch is behind current main; rebase or refresh the merge result and rerun the focused credential-resolver proof before landing.

Maintainer options:

Decide the mitigation before merge
Keep the owner-local resolver split, preserving filesystem-path bytes while treating whitespace-only certificate settings as absent, then land only after a refreshed review against current main.
Pause or close
Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

[P2] No mechanical repair is indicated; a maintainer should refresh the branch against current main and make the normal merge decision for this compatibility-sensitive credential-resolution change.

Security
Cleared: The two-file diff changes only local credential-string resolution and tests; it adds no dependency, workflow, permission, secret-handling, or supply-chain surface.

Review details

Best possible solution:

Keep the owner-local resolver split, preserving filesystem-path bytes while treating whitespace-only certificate settings as absent, then land only after a refreshed review against current main.

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

Yes for the resolver-level bug: the supplied before/after production-function transcript exercises the exported credential functions with whitespace-only certificate values, and the focused regression tests cover the corrected outcomes.

Is this the best way to solve the issue?

Yes. Keeping normalized presence checks separate from returned certificate paths is the narrowest solution: it avoids blank values shadowing the environment without corrupting valid filesystem-path bytes.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 16e967eb7f53.

Label changes

Label changes:

remove merge-risk: 🚨 compatibility: Current PR review selected no merge-risk labels.
remove merge-risk: 🚨 auth-provider: Current PR review selected no merge-risk labels.

Label justifications:

P2: This is a bounded Microsoft Teams federated-auth configuration fix with limited blast radius and a focused regression test.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR supplies red/green terminal output from the exported production credential resolver on the exact head plus focused regression results; no live Teams tenant was available, but the changed behavior occurs before SDK or network dispatch.
proof: sufficient: Contributor real behavior proof is sufficient. The PR supplies red/green terminal output from the exported production credential resolver on the exact head plus focused regression results; no live Teams tenant was available, but the changed behavior occurs before SDK or network dispatch.

Evidence reviewed

PR surface:

Source +23, Tests +36. Total +59 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	30	7	+23
Tests	1	36	0	+36
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	66	7	+59

What I checked:

Federated resolver behavior: The patch adds separate resolvers: path presence is checked with normalized emptiness while the returned nonblank path retains its original string; identifier-like values use normalized optional strings. (extensions/msteams/src/token.ts:51, c670d785a614)
Regression coverage: The added tests cover whitespace-only certificate settings resolving as absent and blank config values falling back to padded environment paths without trimming those filesystem-path bytes. (extensions/msteams/src/token.test.ts:146, c670d785a614)
After-fix production-function proof: The PR body records direct calls to the exported production credential resolver on the exact head: whitespace-only values produce no configured federated credentials, while a padded environment path is preserved and the thumbprint is normalized. (extensions/msteams/src/token.ts:112, c670d785a614)
Continuity with the preceding review: The immediately preceding ClawSweeper cycle identified only preservation of nonblank certificate-path bytes; the current head adds an explicit assertion for the padded environment path and the previous review reported no remaining findings. (extensions/msteams/src/token.test.ts:159, c670d785a614)
Current-main refresh needed: GitHub reports the PR as cleanly mergeable but behind main; this does not demonstrate a defect, but maintainer review should use the refreshed merge result rather than the old base SHA. (16e967eb7f53)

Likely related people:

unresolved: The available review context identifies the proposed branch but does not provide sufficient current-main file-history provenance to attribute ownership without treating the PR author as an owner. (role: current-main area owner not established; confidence: low; files: extensions/msteams/src/token.ts, extensions/msteams/src/token.test.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Review history (5 earlier review cycles)

reviewed 2026-07-16T15:26:05.954Z sha ebb41b3 :: needs maintainer review before merge. :: none
reviewed 2026-07-19T00:31:55.966Z sha 28d6c00 :: needs real behavior proof before merge. :: none
reviewed 2026-07-19T00:46:01.626Z sha 28d6c00 :: needs maintainer review before merge. :: none
reviewed 2026-07-19T01:28:56.491Z sha 28d6c00 :: needs changes before merge. :: [P1] Preserve nonblank certificate paths verbatim
reviewed 2026-07-19T03:35:58.297Z sha c670d78 :: needs maintainer review before merge. :: none

wahaha1223 · 2026-07-19T00:42:33Z

@clawsweeper re-review

clawsweeper · 2026-07-19T00:42:35Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Complete
Detail: The durable review comment was published and its verdict was queued for target-serialized routing.
Run: https://github.com/openclaw/clawsweeper/actions/runs/29668652027
Updated: 2026-07-19T01:33:26.276Z

wahaha1223 · 2026-07-19T03:32:59Z

@clawsweeper re-review

clawsweeper · 2026-07-19T03:33:02Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Complete
Detail: The durable review comment was published and its verdict was queued for target-serialized routing.
Run: https://github.com/openclaw/clawsweeper/actions/runs/29673111983
Updated: 2026-07-19T04:21:38.866Z

openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: S labels Jul 16, 2026

BradGroux mentioned this pull request Jul 17, 2026

WORKING: All Microsoft Issues and PRs (refresh) #74163

Draft

wahaha1223 force-pushed the fix/msteams-ignore-blank-certificate-settings branch from ebb41b3 to 28d6c00 Compare July 19, 2026 00:28

fix(msteams): ignore blank certificate settings

c670d78

wahaha1223 force-pushed the fix/msteams-ignore-blank-certificate-settings branch from 28d6c00 to c670d78 Compare July 19, 2026 03:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(msteams): ignore blank certificate settings#109112

fix(msteams): ignore blank certificate settings#109112
wahaha1223 wants to merge 1 commit into
openclaw:mainfrom
wahaha1223:fix/msteams-ignore-blank-certificate-settings

wahaha1223 commented Jul 16, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Jul 16, 2026 •

edited

Loading

Uh oh!

wahaha1223 commented Jul 19, 2026

Uh oh!

clawsweeper Bot commented Jul 19, 2026 •

edited

Loading

Uh oh!

wahaha1223 commented Jul 19, 2026

Uh oh!

clawsweeper Bot commented Jul 19, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

wahaha1223 commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Problem This Solves

Why This Change Was Made

User Impact

Evidence

Uh oh!

clawsweeper Bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wahaha1223 commented Jul 19, 2026

Uh oh!

clawsweeper Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wahaha1223 commented Jul 19, 2026

Uh oh!

clawsweeper Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

wahaha1223 commented Jul 16, 2026 •

edited

Loading

clawsweeper Bot commented Jul 16, 2026 •

edited

Loading

clawsweeper Bot commented Jul 19, 2026 •

edited

Loading

clawsweeper Bot commented Jul 19, 2026 •

edited

Loading