Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix CSP failures for Microsoft social sign-in #4778

Merged
merged 1 commit into from
May 10, 2024
Merged

Fix CSP failures for Microsoft social sign-in #4778

merged 1 commit into from
May 10, 2024

Conversation

milan-cvetkovic
Copy link
Contributor

Add login.microsoftonline.com to CSP allow list for /account/new, /account/edit and /users/new

To reproduce:
/account/edit

  • Login as existing user
  • go to "My Settings"
  • change "External Authentication" to Microsoft
  • click "Save Changes"
  • Page fails to load due to CSP violation

/account/update ?

/users/new

@tomhughes
Copy link
Member

Has there been some change at the Microsoft end to the URLs that are used? Do we still need the live.com URL as well as this?

@tomhughes
Copy link
Member

It looks like this dates to 7428da7 when we changed from the windowslive omniauth plugin to the microsoft_graph plugin. I'm just astonished it has been broken for nearly a year with nobody reporting it!

As far as I can see the live.com URLs can be removed as the new plugin doesn't reference those at all and the old one does.

@milan-cvetkovic @tomhughes
Fix CSP failures for Microsoft social sign-in
b07c758 
Replace login.live.com with login.microsoftonline.com in CSP allow list.

The URL changed with the move from using the omniauth-windowslive plugin
to the omniauth-microsoft_graph plugin but wasn't noticed until now.
@milan-cvetkovic
Copy link
Contributor Author

milan-cvetkovic commented May 10, 2024 •
edited
Loading

As far as I can see the live.com URLs can be removed as the new plugin doesn't reference those at all and the old one does.

It appears so, in the 2 test cases I was able to try - I am not sure how to trigger /account/update route.

Interestingly, if we get rid of the second round trip as it was suggested in #4455, but later re-introduced in Re-introduce additional round trip for verifying auth_provider, we could remove the CSP setup in users_controller.rb entirely:

@tomhughes
Copy link
Member

I've amended it to remove live.com and will merge it shortly. Thanks for the catch.

@tomhughes tomhughes merged commit 3394fe8 into openstreetmap:master May 10, 2024
20 checks passed
@milan-cvetkovic
Copy link
Contributor Author

I've amended it to remove live.com and will merge it shortly. Thanks for the catch.

Hm, I still see live.com...

@tomhughes
Copy link
Member

Argh.. Sorry about that... I missed the -a switch when amending the commit :-( I've pushed a fix now.

@milan-cvetkovic milan-cvetkovic deleted the csp-microsoft branch May 10, 2024 17:00
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@milan-cvetkovic @tomhughes