Skip to content

Loosen omniauth-oauth2 dependency to allow < 2.0#52

Open
jbaxendale-ut wants to merge 2 commits into
synth:mainfrom
user-interviews:loosen-oauth2
Open

Loosen omniauth-oauth2 dependency to allow < 2.0#52
jbaxendale-ut wants to merge 2 commits into
synth:mainfrom
user-interviews:loosen-oauth2

Conversation

@jbaxendale-ut
Copy link
Copy Markdown

@jbaxendale-ut jbaxendale-ut commented May 22, 2026

Allows for depending on the fairly old at this point 1.9: https://github.com/omniauth/omniauth-oauth2/releases/tag/v1.9.0

Mainly has fixes for CSRF timing attack and support for Ruby 3.2 in their CI

Allows for depending on the fairly old at this point 1.9:
https://github.com/omniauth/omniauth-oauth2/releases/tag/v1.9.0

Enabling CI workflow
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the gem’s runtime dependency constraints to permit newer omniauth-oauth2 1.x releases (notably 1.9.x) while still preventing automatic upgrades to 2.x, improving compatibility with security/CI fixes in the 1.x line.

Changes:

  • Relax omniauth-oauth2 runtime dependency from ~> 1.8.0 to allow later 1.x releases up to (but excluding) 2.0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread omniauth-microsoft_graph.gemspec Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@jbaxendale-ut jbaxendale-ut marked this pull request as draft May 22, 2026 13:34
@jbaxendale-ut
Copy link
Copy Markdown
Author

Was getting a warning in my own branch from Codex about potential changed behavior that I want to check out first. I don't think it's related to this gem loosening, as I said it didn't make sense for it to impact things.

For posterity:

› the only thing this fork does is loosen omniauth-oauth2 to allow the 1.9 version, there is no other code change

• You’re right. I described that imprecisely.

I should not have said “fork behavior” as if the UI fork added the Microsoft domain verification logic. If the fork only loosens omniauth-oauth2 to allow 1.9, then the risky behavior is
better described as behavior of the selected omniauth-microsoft_graph version, not a UI-specific fork change.

The dependency line still matters because it changes which gem code we now run, but the failure path is in the gem’s Microsoft Graph strategy/verifier:

  • The strategy derives info.email from Graph’s nullable mail field.
  • The verifier derives email_domain from that info.email.
  • Microsoft Graph can return mail: nil while still returning userPrincipalName.
  • The verifier then calls email_domain.casecmp?(upn_domain), which fails if email_domain is nil.

So the correction is:

The dependency switch exposes an upstream omniauth-microsoft_graph behavior where domain verification assumes mail is present. The UI fork may only be the vehicle that makes that
version compatible/resolvable with this app’s OAuth dependencies.

The options are basically the same, but renamed:

  1. Patch the forked gem source to fix upstream behavior while preserving domain verification.
  2. Patch locally in Rails with an initializer monkey patch.
  3. Configure/skip verification, though the current verifier ordering may still make this unsafe unless confirmed.
  4. Revert/pin away from this gem version if we want to avoid the new behavior entirely.

Recommended plan is still to fix the gem source: fallback mail || userPrincipalName for the effective email, and make the verifier nil-safe.

So this seems unrelated

@jbaxendale-ut jbaxendale-ut deleted the loosen-oauth2 branch May 22, 2026 13:55
@jbaxendale-ut jbaxendale-ut restored the loosen-oauth2 branch May 22, 2026 14:08
@jbaxendale-ut jbaxendale-ut reopened this May 22, 2026
@jbaxendale-ut jbaxendale-ut marked this pull request as ready for review May 22, 2026 14:08
@jbaxendale-ut
Copy link
Copy Markdown
Author

Sorry for all the back and forth with this PR, mixed up in my own branch.

Leaving this PR as just the gemspec adjustment -- some details on my convo with Codex regarding a null mail field is in this PR in our fork repo if interested but I'm not convinced it is actually correct in it's changes

@jbaxendale-ut
Copy link
Copy Markdown
Author

Also if useful, happy to also bump the spec Matrix to 3.4 (which is what I imagine most people are running on these days)

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.

2 participants