Skip to content

fix(msteams): reject malformed OAuth token envelopes [AI-assisted]#111638

Open
Alix-007 wants to merge 1 commit into
openclaw:mainfrom
Alix-007:alix/msteams-oauth-token-shape
Open

fix(msteams): reject malformed OAuth token envelopes [AI-assisted]#111638
Alix-007 wants to merge 1 commit into
openclaw:mainfrom
Alix-007:alix/msteams-oauth-token-shape

Conversation

@Alix-007

@Alix-007 Alix-007 commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where Microsoft Teams delegated OAuth exchange or refresh could throw a raw TypeError when Azure returned HTTP 200 with JSON null instead of a token object. Other non-object envelopes are also rejected explicitly at the provider boundary.

Why This Change Was Made

The decoded token response is now treated as unknown and checked for a non-null, non-array object before any token fields are read. Existing expiry validation, exchange refresh-token requirements, refresh-token rotation fallback, timeout handling, SSRF protection, and response release remain unchanged.

User Impact

Malformed Teams OAuth token envelopes fail with the existing actionable MSTeams ... failed: invalid token response fields error. Valid authorization-code exchange and refresh flows behave exactly as before.

Evidence

  • The branch was synchronized once. Exact parent: 25d6f13a1ff864e241d28f4540494ecba43a6421. Exact current head: 1d61563a92b1f54de9a2c479c5be6c5b2ba7dd09. The branch is not being continuously rebased.
  • Focused regression coverage exercises null and array top-level responses through both exchangeMSTeamsCodeForTokens and refreshMSTeamsDelegatedTokens.
  • Exact-head upstream CI completed with 50 passing checks, 35 intentionally skipped/neutral checks, 0 failures, and 0 pending checks.
  • Static validation: oxfmt --check and git diff --check pass.
  • Microsoft documents successful v2 token responses as JSON objects containing access_token and expires_in: OAuth 2.0 authorization code flow.

Production-entry before/after proof

The public, secretless GitHub-hosted proof run checked the immutable parent and head above. Its temporary harness called the exported exchange and refresh production entry points through the real shared guarded-fetch path. A loopback HTTP server returned HTTP 200 with null and []; the URL-only fetch wrapper forwarded the exact RequestInit and AbortSignal, and the server verified the POST method and form fields.

Before the fix, both null paths reached raw property access:

CHECKED_SHA role=before sha=25d6f13a1ff864e241d28f4540494ecba43a6421
RESULT role=before sha=25d6f13a1ff864e241d28f4540494ecba43a6421 flow=exchange payload=null http=200 error_class=TypeError message="Cannot read properties of null (reading 'expires_in')" redirect=loopback request_init=preserved abort_signal=preserved
RESULT role=before sha=25d6f13a1ff864e241d28f4540494ecba43a6421 flow=refresh payload=null http=200 error_class=TypeError message="Cannot read properties of null (reading 'expires_in')" redirect=loopback request_init=preserved abort_signal=preserved
SUMMARY role=before sha=25d6f13a1ff864e241d28f4540494ecba43a6421 cases=4 status=pass

After the fix, both production entry points return the stable validation error. The array cases also pass as controls:

CHECKED_SHA role=after sha=1d61563a92b1f54de9a2c479c5be6c5b2ba7dd09
RESULT role=after sha=1d61563a92b1f54de9a2c479c5be6c5b2ba7dd09 flow=exchange payload=null http=200 error_class=Error message="MSTeams token exchange failed: invalid token response fields" redirect=loopback request_init=preserved abort_signal=preserved
RESULT role=after sha=1d61563a92b1f54de9a2c479c5be6c5b2ba7dd09 flow=exchange payload=array http=200 error_class=Error message="MSTeams token exchange failed: invalid token response fields" redirect=loopback request_init=preserved abort_signal=preserved
RESULT role=after sha=1d61563a92b1f54de9a2c479c5be6c5b2ba7dd09 flow=refresh payload=null http=200 error_class=Error message="MSTeams token refresh failed: invalid token response fields" redirect=loopback request_init=preserved abort_signal=preserved
RESULT role=after sha=1d61563a92b1f54de9a2c479c5be6c5b2ba7dd09 flow=refresh payload=array http=200 error_class=Error message="MSTeams token refresh failed: invalid token response fields" redirect=loopback request_init=preserved abort_signal=preserved
SUMMARY role=after sha=1d61563a92b1f54de9a2c479c5be6c5b2ba7dd09 cases=4 status=pass

The proof workflow is pinned in fork commit 04dc95b79742216ad887d63a674a55bbc520decb. It uses no credentials and does not alter this PR's product diff.

  • Codex autoreview was attempted before commit; the review engine returned HTTP 401 from the Responses WebSocket, so no structured findings were produced. A separate adversarial Codex review of the exact head found no concrete code findings and judged the shared fetch boundary to be the best fix.

AI-assisted (Codex): I reviewed and understand this change. Allow edits from maintainers is enabled.

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: S labels Jul 20, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jul 20, 2026
@clawsweeper

clawsweeper Bot commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 20, 2026, 1:23 AM ET / 05:23 UTC.

Summary
The PR treats Microsoft Teams OAuth JSON as unknown, rejects null and array top-level envelopes before token-field parsing, and adds exchange and refresh regression coverage.

PR surface: Source +14, Tests +33. Total +47 across 2 files.

Reproducibility: no. current-main reproduction was independently established in this read-only review. The linked secretless proof gives a clear source-backed reproduction on the PR parent: an HTTP 200 token response containing null reached raw property access in both exchange and refresh paths.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster ✨ media proof bonus
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] Rebase or otherwise refresh the branch onto current main, then confirm the focused OAuth checks and required CI apply to the exact merge result.

Risk before merge

  • [P1] The branch is behind current main; refresh and re-run the focused checks on the exact merge result so current-base changes cannot alter the OAuth path or test outcome unnoticed.

Maintainer options:

  1. Decide the mitigation before merge
    Rebase the focused plugin-local guard onto current main, retain the null/array exchange and refresh coverage, and land once the exact merged tree has the same behavior proof and required checks.
  2. 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; refresh the exact merge result against current main and complete routine maintainer review before landing.

Security
Cleared: The patch only rejects malformed untrusted OAuth response envelopes before field access; it adds no dependency, workflow, permission, secret-handling, or execution-path surface.

Review details

Best possible solution:

Rebase the focused plugin-local guard onto current main, retain the null/array exchange and refresh coverage, and land once the exact merged tree has the same behavior proof and required checks.

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

No current-main reproduction was independently established in this read-only review. The linked secretless proof gives a clear source-backed reproduction on the PR parent: an HTTP 200 token response containing null reached raw property access in both exchange and refresh paths.

Is this the best way to solve the issue?

Yes. Validating the decoded envelope at the shared Microsoft Teams token-fetch boundary is the narrowest maintainable fix because both production flows use that path and existing token-field validation remains authoritative.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded Microsoft Teams OAuth error-path fix with a user-visible but limited channel-specific impact.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (linked_artifact): The linked secretless GitHub-hosted run compares the immutable parent and PR head through both real exported OAuth entry points, showing raw TypeErrors replaced by stable validation errors for null and array responses.
  • proof: sufficient: Contributor real behavior proof is sufficient. The linked secretless GitHub-hosted run compares the immutable parent and PR head through both real exported OAuth entry points, showing raw TypeErrors replaced by stable validation errors for null and array responses.
Evidence reviewed

PR surface:

Source +14, Tests +33. Total +47 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 16 2 +14
Tests 1 33 0 +33
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 49 2 +47

What I checked:

  • Provider-boundary validation: The patch changes fetchMSTeamsTokens to decode JSON as unknown and rejects null, non-object, and array envelopes before parseMSTeamsTokenResponse reads token fields; this preserves the existing field-level validation error rather than allowing raw property access. (extensions/msteams/src/oauth.token.ts:46, 1d61563a92b1)
  • Both OAuth entry points covered: The PR adds parameterized regression cases for null and array response bodies through both authorization-code exchange and delegated-token refresh paths. (extensions/msteams/src/oauth.test.ts:274, 1d61563a92b1)
  • Real behavior proof: The PR body links a secretless GitHub Actions run that invokes both exported production entry points through guarded fetch. It records the parent head producing raw null-property TypeErrors and the PR head returning the established actionable validation errors for null and arrays. (extensions/msteams/src/oauth.token.ts:104, 1d61563a92b1)
  • Current-base status: GitHub reports the PR as mergeable but behind: its base SHA is bc76bb8 while the review target current main is e45ebac. The existing proof is exact-head evidence, not evidence for the refreshed merge result. (e45ebac97abb)

Likely related people:

  • Alix-007: Authored the current focused Microsoft Teams OAuth validation change and supplied exact-head behavior proof; current-main ownership history could not be established from the available read-only review evidence. (role: proposed-patch author; confidence: low; commits: 1d61563a92b1; files: extensions/msteams/src/oauth.token.ts, extensions/msteams/src/oauth.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 (3 earlier review cycles)
  • reviewed 2026-07-20T04:01:03.392Z sha 984662c :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-20T04:13:41.753Z sha 1d61563 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-20T04:41:10.701Z sha 1d61563 :: needs maintainer review before merge. :: none

@Alix-007
Alix-007 force-pushed the alix/msteams-oauth-token-shape branch from 984662c to 1d61563 Compare July 20, 2026 04:10
@Alix-007

Alix-007 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
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:

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 20, 2026
@BradGroux BradGroux mentioned this pull request Jul 20, 2026
Draft
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 P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Alix-007 @Patrick-Erichsen