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.

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

Still fighting the good fight with oAuth #589

Closed
resistorsoftware opened this issue May 13, 2018 · 15 comments
Closed

Still fighting the good fight with oAuth #589

resistorsoftware opened this issue May 13, 2018 · 15 comments

Comments

@resistorsoftware
Copy link

Using Sinatra, and testing out the most basic oAuth App install flow, the old CSRF invalid credentials popped up again. Decided to look into it. Much deeper than usual. Came up with nothing! The only cure seems to be using an OmniAuth block ignoring provider state. I am all over HTTPS all the time, and this pattern has worked forever for me, until now, and until this project spawned from what I thought was perfectly good working App code.

use OmniAuth::Builder do
  provider :shopify, ENV.fetch('SHOPIFY_API_KEY'), ENV.fetch('SHOPIFY_SECRET_KEY'), scope: SCOPE, provider_ignores_state: true
end

Am curious as to what the cure for this ailment is. I set Rack::Csrf to ignore the GET /auth/shopify/callback that triggers this but to no avail.

INFO -- omniauth: (shopify) Setup endpoint detected, running now.
INFO -- omniauth: (shopify) Request phase initiated.
INFO -- omniauth: (shopify) Setup endpoint detected, running now.
INFO -- omniauth: (shopify) Callback phase initiated.
ERROR -- omniauth: (shopify) Authentication failure! csrf_detected: 
OmniAuth::Strategies::OAuth2::CallbackError, csrf_detected | CSRF detected
ERROR -- omniauth: (shopify) Authentication failure! invalid_credentials: 
OmniAuth::Strategies::OAuth2::CallbackError, csrf_detected | CSRF detected
Rack app error handling request { GET /auth/shopify/callback }
#<OmniAuth::Strategies::OAuth2::CallbackError: OmniAuth::Strategies::OAuth2::CallbackError>

Anyone out there figure this out? With provider ignores state off, I get to the /auth/shopify/callback and can see the CSRF token, and even state in the OmniAuth state key. Seems fiendishly hard, as stated here in the warnings... writing into the OmniAuth gem issues is an option I suppose, but I am hoping this community has some pointers.

Thanks

@vfonic
Copy link
Contributor

vfonic commented May 13, 2018

Every once in a while I get stuck at the same place, spend several hours and then revert to whatever I can revert and leave it for some other time. :)

I'd also really appreciate if someone who figured this out could share the solution.

@wnm
Copy link
Contributor

wnm commented May 14, 2018

the oauth issues that I'm aware of are #533 and #567

For #533, this is what @EiNSTeiN- wrote on the issue:

In safari you need to visit a page in a top level browsing context (i.e. not in an iframe) to set a cookie. You should be able to use the helper this gem provides to redirect to your app outside of the iframe, set your session cookie and initiate the oauth flow afterwards.

To fix the issue in my app, I copied authenticated_controller in my app, and copy and pasted all methods from login_protection.rb which are using theredirect_to_login method, then changed this line in the redirect_to_login method:

redirect_to login_url

into

fullpage_redirect_to login_url

This seems to fix the Safari issues, but also forced all logins to do the full page redirect login dance...

@resistorsoftware
Copy link
Author

Using the pattern from #533 does not eliminate the need for me to use:

provider_ignores_state: true

In the OmniAuth setup for Shopify. I have seen Shopify try using Just JS, then HTML with JS, then just rendering HTML direct, now HTML with JS again, all in the hopes the get oAuth to work securely with state. Seems to be pie in the sky at the moment.

At least it works fine with ignoring state.

@EiNSTeiN-
Copy link
Contributor

You should definitely NOT use provider_ignores_state: true, because shopify does not ignore the state parameter, and using this option makes your application's OAuth flow vulnerable to Cross Site Request Forgery. The csrf_detected error is a security check, please do not disable it. If you find yourself needing this option, it is most likely due to a misconfiguration somewhere else in your application, so please check

  1. your session storage works correctly
  2. the session cookie are being saved properly in your browser during the whole oauth authorization flow
  3. the oauth flow is both initiated and terminated on https, on the same domain (your session storage will likely not work unless this is the case)
  4. something else specific to your application configuration deletes the cookies or prevents your session storage from working properly

@resistorsoftware
Copy link
Author

  1. My session storage works fine: Rack::Session::Cookie is hard to screw up.
  2. See # 1. I don't save cookies. OmniAuth and other code would be doing that
  3. Yes. HTTPS is defacto norm for me, all the time, even on localhost testing
  4. I delete exactly zero cookies in any code I ever write. I let them expire when browser closes most of the time, or Rack::Session::Cookie has a timeout of a few days.. whatever

I stripped out that provider_ignores_state and stripped out Rack::Csrf and my last install worked without issue. Back to square one. A sense of how flakey all this oAuth is :)

@vfonic
Copy link
Contributor

vfonic commented May 22, 2018

Hey guys,

I spent last three days debugging my app. Today I realized that there is a bug somewhere outside of my app's code (I think it might be in omniauth-oauth2 gem itself).

Here's the video where I'm showing the FULL repro steps (sorry for the length, I wanted to show you that it fails on a brand new rails app and Shopify app created in Partners portal):
https://www.dropbox.com/s/39z47r5y03c2kbk/Shopify%20CSRF%20token.mov?dl=0

Repro steps:

  1. rails new shopify_app_csrf_token_issue --database=postgresql --skip-coffee --skip-turbolinks --skip-test --skip-yarn --skip-action-cable --skip-system-test

  2. cd shopify_app_csrf_token_issue

  3. echo "gem 'shopify_app'" >> Gemfile; bundle install

  4. rails generate shopify_app --api_key your_api_key --secret your_app_secret (replace with real keys)

  5. Deploy app or start local server (also setup the db)

  6. Open Chrome Incognito

  7. Visit app login page (on HTTPS)

  8. Type in shop's url

  9. Login to shop (when redirected)

  10. Install the app

Repo (with just 2 commits - rails new and "add shopify app tokens"):
https://github.com/vfonic/shopify_app_csrf_token_issue

I've tested in:

  • Chrome
  • Chrome Incognito
  • Firefox
  • Safari

The ONLY browser from above, in which it doesn't fail, is Chrome (I removed the cookies before testing).

I believe this caused my apps to lose quite some installs (and revenue) and a huge amount of my time. I hope someone will find the time to have a look. Thanks!

/cc @EiNSTeiN- (CC-ing since this is a closed issue and those can sometimes be missed)

Also to note:

  • I'm not even sure anymore that I can repro this every time.
  • I even had it happen on one store but not on the other (the two (development) stores were as identical as possible).
  • I couldn't make it fail on development through ngrok tunnel (it would successfully install the app).
  • This issue happens both on rails 5.2.0 and 5.1.6
  • I have always been logged out of the store before initiating the app install process

@resistorsoftware
Copy link
Author

resistorsoftware commented May 22, 2018

I did not close the issue due to any real resolution. I closed it as it seems to be an issue bigger than the issue I raised. I ended up wasting many hours seeing the same results as your video shows Victor. oAuth remains a flakey slab of floating fat on otherwise decent tasting gravy. And every time sometime does a PR and adds a new flavour to Authentication, I scratch my head and wonder how it all affects the one true way we should be doing this. My code is now officially FRANKENCLANKER HODGEPODGE whatever... I feel like I could use oAuth to open just about any shopify App with a mere polite knock on the door rather than needing a military grade door hammer thing like the narc squad might use to get into a den of meth heads.

@vfonic
Copy link
Contributor

vfonic commented May 22, 2018

oAuth remains a flakey slab of floating fat on otherwise decent tasting gravy.

Great metaphor there! :D

@EiNSTeiN-
Copy link
Contributor

EiNSTeiN- commented May 22, 2018

I'm attempting to reproduce but getting a different error (uri not whitelisted). Would you be able to setup the app again so I can get to the same point as you? I'm using https://glacial-plains-47049.herokuapp.com/login

@vfonic
Copy link
Contributor

vfonic commented May 22, 2018

Thanks, Francois!
I re-generated the Shopify tokens so that they are different from the ones in the video, which basically broke the app. I just pushed the new tokens to heroku app now:
https://glacial-plains-47049.herokuapp.com/

@vfonic
Copy link
Contributor

vfonic commented May 22, 2018

I have finally found an acceptable solution for me.

What happens here is: app gets installed, but user sees the error page because of CSRF token issue. The important bit is that app gets installed (read: approved by the user). Now we can request the auth token from Shopify.
To do this, we can simply redirect user to auth flow again ('/auth/shopify') and, instead of seeing install screen, user will get redirected and logged in (read: our app will get access token, set shop's session and redirect user to logged in app).

All in all, here's the entire change needed to have the user experience feel as if everything is fine:

config/routes.rb:

get '/auth/failure', to: redirect('/auth/shopify')

This code is far from perfect. Ideally, we would check if the correct session parameter is set (or if the auth failure happened for some other reason, etc.), but it works for the case I need it to work and doesn't open any CSRF vulnerabilities (as far as I understand what's happening).

@resistorsoftware
Copy link
Author

Be neat to this exploited... :)

@tylerball
Copy link
Contributor

I've done some digging on this and this is an issue with the underlying oauth2 middleware: omniauth/omniauth-oauth2#95

I'm going to work on submitting a PR to fix.

@sergey-alekseev
Copy link
Contributor

@tylerball have you ever got a chance to work on a PR to fix?

@overallduka
Copy link

+1, is not working on Safari

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

No branches or pull requests

7 participants