Skip to content

improvement(auth): make Microsoft emailVerified derivation total#5157

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/microsoft-emailverified-total
Jun 20, 2026
Merged

improvement(auth): make Microsoft emailVerified derivation total#5157
waleedlatif1 merged 2 commits into
stagingfrom
fix/microsoft-emailverified-total

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • Follow-up hardening to the nOAuth account-linking fix (fix(auth): close nOAuth account takeover via email-based OAuth linking #5156, already merged).
  • deriveMicrosoftEmailVerified cast the verified-email claims to string[] and called .includes via optional chaining, which only guards null/undefined. If a claim ever arrived as a non-array, non-string value (e.g. a number or object), .includes would throw and fail the OAuth getUserInfo flow.
  • Replaced the unchecked as string[] cast with proper Array.isArray guards, so any claim shape resolves to unverified instead of throwing. No behavior change for real Microsoft tokens — purely defensive robustness on a security-relevant helper.

Type of Change

  • Improvement

Testing

  • Extended microsoft.test.ts with non-array claim shapes (string, number, object, null) asserting no throw + unverified.
  • bunx vitest run lib/oauth/microsoft.test.ts — 11 passing.
  • bunx tsc --noEmit — 0 errors. bun run lint — clean.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

deriveMicrosoftEmailVerified cast the verified-email claims to string[]
and called .includes through optional chaining, which only guards
null/undefined. A claim arriving as a non-array, non-string value (e.g.
a number) would throw inside getUserInfo and fail the OAuth flow.
Array-check the claims with a proper type guard so any claim shape
resolves to unverified instead of throwing.
@vercel

vercel Bot commented Jun 20, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jun 20, 2026 10:11pm

Request Review

@cursor

cursor Bot commented Jun 20, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches auth account-linking logic on the Microsoft OAuth path, but changes are defensive with no intended behavior change for valid Microsoft tokens.

Overview
Hardens deriveMicrosoftEmailVerified so malformed Microsoft ID-token claims no longer break OAuth sign-in and cannot be misread as verified.

Verified-email claims are no longer cast to string[] and .includes’d blindly. The helper now requires Array.isArray on verified_primary_email / verified_secondary_email before membership checks, so non-array shapes (string, number, object, null) resolve to unverified instead of throwing during getUserInfo. A string claim equal to the user’s email is explicitly not treated as verified, closing the unsafe-cast footgun where .includes on a string could match.

Tests in microsoft.test.ts cover the new shapes and the string-equals-email case.

Reviewed by Cursor Bugbot for commit 035b81f. Configure here.

@greptile-apps

greptile-apps Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR hardens the deriveMicrosoftEmailVerified helper by replacing unchecked as string[] type casts and optional-chaining ?.includes with explicit Array.isArray guards, ensuring any non-array claim shape resolves to false (unverified) instead of potentially throwing. The companion test file extends coverage with string, number, object, and null claim shapes, plus the important boundary case where a string equal to the email must not be treated as verified.

  • microsoft.ts: Three lines of cast + optional-chain replaced with Array.isArray guards; logic is otherwise identical for well-formed tokens.
  • microsoft.test.ts: Four new assertions for malformed claim shapes, plus a dedicated test locking in that a plain-string claim equal to the email is correctly rejected (the old as string[] cast would have passed it via String.prototype.includes).

Confidence Score: 5/5

Safe to merge — the change is purely defensive and cannot weaken email verification for any well-formed Microsoft token.

The only logic that changes is the handling of malformed claim shapes, which previously could throw and now consistently return false. Real Microsoft tokens with proper string[] claims are unaffected. The new tests lock in both the defensive behavior and the important boundary where a plain-string claim equal to the email must not be treated as verified.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/lib/oauth/microsoft.ts Defensive hardening: unsafe as string[] cast replaced with Array.isArray guards; no behavioral change for well-formed Microsoft tokens.
apps/sim/lib/oauth/microsoft.test.ts Expands test coverage with malformed claim shapes (number, object, null) and the string-equal-to-email boundary regression guard; all 11 cases pass.

Reviews (3): Last reviewed commit: "test(auth): lock in unverified for a str..." | Re-trigger Greptile

Comment thread apps/sim/lib/oauth/microsoft.test.ts
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Add a boundary case asserting a string verified_primary_email/
verified_secondary_email equal to the email resolves to unverified —
the old string[] cast would have returned true via String.includes.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 035b81f. Configure here.

@waleedlatif1 waleedlatif1 merged commit 55f4326 into staging Jun 20, 2026
11 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/microsoft-emailverified-total branch June 20, 2026 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant