Shopify_app: Safari issue

[almeidaz]

I'm still getting a problem with omniauth and safari users

Started GET "/auth/shopify" for 78.233.xxx.xxx at 2017-12-22 06:41:45 +0100
I, [2017-12-22T06:41:45.586176 #11308] INFO -- omniauth: (shopify) Setup endpoint detected, running now.
I, [2017-12-22T06:41:45.587191 #11308] INFO -- omniauth: (shopify) Request phase initiated.
E, [2017-12-22T06:41:45.587679 #11308] ERROR -- omniauth: (shopify) Authentication failure! invalid_site encountered.

We have to consider that shopify_app doesn't works for safari client ?

[jamiemtdwyer]

Can you enable third party cookies in Safari and let me know if that helps?

On Dec 22, 2017 01:15, "almeidaz" [e-mail] wrote:

I'm still getting a problem with omniauth and safari users

Started GET "/auth/shopify" for 78.233.xxx.xxx at 2017-12-22 06:41:45 +0100
I, [2017-12-22T06:41:45.586176 #11308] INFO -- omniauth: (shopify) Setup
endpoint detected, running now.
I, [2017-12-22T06:41:45.587191 #11308] INFO -- omniauth: (shopify) Request
phase initiated.
E, [2017-12-22T06:41:45.587679 #11308] ERROR -- omniauth: (shopify)
Authentication failure! invalid_site encountered.

We have to consider that shopify_app doesn't works for safari client ?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/Shopify/shopify_app/issues/533, or mute the thread
https://github.com/notifications/unsubscribe-auth/APQ7PLo-t1o3MAzdzQ4UG_80O1hfNK-8ks5tC0j8gaJpZM4RKrt2
.

[almeidaz]

Thanks for reply. I heard about this tweak but i can't tell to potential client to do that. Especially when other apps doesn't need this kind of tweak. You can test my app here : https://apps.shopify.com/osaria-audio-player. It' currently a real problem. All people using IOS install and uninstall app instantly cause app doesn't works for them.
The most curious thing is : the first request to the app return the error mentioned before but when you come back right after(2nd request) the app working.

[almeidaz]

I have retest https://github.com/Shopify/billing-demo-app based on shopify_app 7.2 and there is no problem with this environment. I know it's very old version and many improvement have been made since.
But IOS users are able to access app without error in this context( shopify_app 7.2).

[kevinhughes27]

We've known about issues with the ESDK and Apple browsers for a bit and unfortunately we haven't found a good fix yet.

Its interesting that it works in the previous version of ShopifyApp. This change came to mind and is one of the only changes to the authentication flow https://github.com/Shopify/shopify_app/pull/460. I'd recommend reverting this code on master and seeing if it fixes the issue. Then we can plan a new course of action.

[almeidaz]

Unfortunately, it's not working. i 'm still searching.

[almeidaz]

In omniauth.rb : shopify_auth_params is empty for safari users

Then, how can we retrieve current shop domain in omniauth ? (without cookies i mean)

[vfonic]

@almeidaz @kevinhughes27 it actually does work when you revert the changes from #460. You also need to remove the extra generated code from config/initializers/omniauth.rb.

Here are the full repro steps:

1. Install an app in the store
2. Open Safari incognito (you must not have app's cookies)
3. Login into store's admin
4. Navigate to the app

Expected:
App performs automatic redirect+login

Actual:
App performs automatic redirect and raises an omniauth invalid_site error.

@kevinhughes27 what do you suggest as the solution?

EDIT: For what it's worth, I've added back these two lines and the app logged in successfully (following above mentioned steps):
https://github.com/Shopify/shopify_app/pull/460/files#diff-350e7187d4b8bbcd1eaee3ff44fc7fb3R47

[almeidaz]

Thanks, it's working great. No more raises invalid_site error.

[kevinhughes27]

Thanks for following up. It looks like Safari is not letting anything into the session object which is causing the issue. We might be able to detect the browser and use the session for browsers where it is supported and fallback to the url parameter for Safari but I'll have to defer to @EiNSTeiN- and the shopify security team for how this should be fixed.

[EiNSTeiN-]

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.

[joshreeves]

Working off of the comment from @EiNSTeiN- I was able to fix it by overriding redirect_to_login in my own AuthenticatedController like so:

module ShopifyApp
  class AuthenticatedController < ApplicationController
    include ShopifyApp::Localization
    include ShopifyApp::LoginProtection
    include ShopifyApp::EmbeddedApp

    protect_from_forgery with: :exception
    before_action :login_again_if_different_shop
    around_action :shopify_session

    protected
      def redirect_to_login
        if request.xhr?
          head :unauthorized
        else
          session[:return_to] = request.fullpath if request.get?
          if browser.safari? && params[:shop]
            fullpage_redirect_to login_url
          else
            redirect_to login_url
          end
        end
      end
  end
end

I'm using the browser gem to detect if it is Safari. Alternatively you could use fullpage_redirect_to for all browsers if you didn't want to do browser detection.

EDIT: I added a check for params[:shop] because fullpage_redirect_to will break if the shop param is not present (ie. often a request coming from somewhere other than the Shopify admin panel).

[thatdom]

@joshreeves thanks for sharing this, it fixes problem for us.

[jamiemtdwyer]

Alternatively you could use fullpage_redirect_to for all browsers if you didn't want to do browser detection.

Are there any side-effects to this? This could be worth exploring in a PR

[thatdom]

We're running this fix in production and noticed one problem: on some requests we're getting error ShopifyApp::LoginProtection::ShopifyDomainNotFound

Looks like it's coming this line: fullpage_redirect_to login_url

[joshreeves]

@jamiemtdwyer - There is a downside. The authentication process feels a bit clunkier because it spends more time outside of the Shopify admin wrapped iframe. That is why I am only targeting Safari.

@thatdom - We are running in production too and I have not seen that issue. Is it happening for specific shops or is it random?

[edavis10]

@joshreeves I've been seeing it for every non-authenticated request to the app outside of Shopify. Bots trigger it a lot, e.g. indexing the root or slack trying to load a preview link from the domain. Looks like a bit over 200 occurrences in the past two weeks.

So far I haven't seen any customers hit it but it's a mess if we have to choose between the core code breaking in Safari for customers, a patch that throws a ton of false exceptions, or sniffing the browser agent on the server.

[joshreeves]

@edavis10 - Ah, ok. Looks like the issue with the requests you are describing is that the shop param is not present. When that is the case, the gem is raising the ShopifyDomainNotFound because the fullpage_redirect_to method relies on the shop param existing.

I added a check in the above code for params[:shop] before calling fullpage_redirect_to. Agreed that this is not a complete solution, but a temporary hack fix.

[thatdom]

@joshreeves Thanks for sharing all this info! We ended up using this code:

 def redirect_to_login
      if request.xhr?
        head :unauthorized
      else
        session[:return_to] = request.fullpath if request.get?
        domain = sanitized_shop_name || session[:shopify_domain]
        ua = request.user_agent

        if ua =~ /Safari/ && ua !~ /Chrome|CriOS|PhantomJS|FxiOS/ && domain.present?
          fullpage_redirect_to login_url
        else
          redirect_to login_url
        end
      end
end

Just when we thought that we fixed this, we started getting reports from users - they still end up with /auth/failure. We're looking now into logs and trying to figure out what's going on. It's such a frustrating bug.

[EiNSTeiN-]

This may be due to a recent change in firefox 58 (released in january): https://blog.mozilla.org/blog/2018/01/23/latest-firefox-quantum-release-now-available-with-new-features/

I believe this type of tracking protection is available in all browsers in one form or another, so it may be better to just use fullpage_redirect_to at all times. We may want to look at improving that page so it doesn't feel like an interruption.

[vfonic]

Would it be possible to revert the changes made in #460? This bug keeps coming again and again and is really frustrating and takes a lot of work that could be invested in something else.

It's been reported more than 2 months ago, there's a "temporary" fix of reverting the breaking change and my app has been declined twice (!!!) because of this. Every time I submit an app, I have to wait about two weeks for it to be reviewed, just to be denied because I ended up changing something unintentionally around the code that fixes this (because I forget that I still need the "hack" to have this working, and when something break in authentication, the first thing I do, I remove the hack, and since I'm using Chrome, it all seems to be working just fine, until my users start reporting that they can't log into the app and then I waste a day or two juggling around what could go wrong...)

Thanks!

[vfonic]

Without this redirect, the app doesn't set cookies right in Safari. The latest released gem version is broken in Safari.

[david719]

Hey! Thanks for the heads up about this. I actually reviewed your most recent submission earlier today. After each review, you'll notice that you typically receive an automated email outlining the next steps in the review process.

In the automated email you would have received today, you would see the steps outlined across the top which essentially states that once you make the required changes to the app, you can respond to the email and it will come directly to me where we can continue with the review process from there. This is only the case for apps that are currently in the review process and haven't been immediately rejected for missing critical components. In other words, your app wasn't rejected today but is currently in the review process which can eventually be published once everything is done. As stated in the email, you'll just need to make the relevant changes and let me know via email once you are all set.

I hope this additional clarification helps and I hope to hear from you soon!

[almeidaz]

I had also facing this problem when i sent my first app to the review. Shopify_app does a useless redirect (i think) before embedded to the SDK. I opened a thread about it : https://github.com/Shopify/shopify_app/issues/500 but nobody replied.

This problem happen with all web browser (not only safari). To get around this problem, I've done a dirty tweak with css hiding container during the redirection.

Anyway, it' s good to see app reviewer around here and take time to consider problem with Shopify_app.
I hope, we will find a solution because i really like Shopify_app.

[vfonic]

@david719 sorry for my tone in my previous comment. I'm just upset because of the reasons explained here The App Store review process

In the automated email you would have received today, you would see the steps outlined across the top which essentially states that once you make the required changes to the app

The difference this time is that I don't think there's anything I need to change. These are the two options right now (using this repo's library):

1. Use this repo's library code, without any modification

   This leaves Safari completely broken, meaning that shop owners using Safari can't install the app properly. This is bad for my app's reputation and can cause users to leave negative reviews and I might also lose revenue and app installs. (I guess the downsides are obvious)

2. Slightly modify the repo's code (to how it was written before) to perform a full page redirect to the app's url and then redirect back to embedded app

   This, slightly (1 second) awkward solution, works cross-browser. I believe this approach, that works for everyone, is better than having the app broken in Safari.

It's the exact same behavior of this library from earlier versions, since v6.0.0, until v8.2.5 which was released on 5th December 2017 (two and a half years later): https://github.com/Shopify/shopify_app/commit/54dbb099582662cd14672409a87c9e806c5cea5d https://rubygems.org/gems/shopify_app/versions
So this behavior was the default behavior for all the apps using this gem for the last two+ years. And now it became a problem, where, the solution proposed doesn't seem to work. I'll test again with a new rails app to see if I can really login to the shop in Safari. I'm pretty sure I already did this test and confirmed that it broken.

What do you suggest me to do?
Should I add the check for the browser and only make the full page redirect for Safari, or use this library's code (most probably break Safari) and wait for repo maintainers to fix the issue?

[nwtn]

Closing because I believe this is solved by the ITP-related fixes that merged in late 2018 and early 2019. Feel free to re-open if this is still a problem.

[paynecodes]

@nwtn Can you or someone else point to the general solution to this problem?

Edit: I think I found Shopify’s general advice, but... there has to be a better way. :/

[vfonic]

@jpdesigndev the latest gem versions should have this bug fixed, aka, if you use v9+, you shouldn't be having this issue. If you still have an issue similar to this, and you can provide an example repo, that would help a lot. (Since this issue is considered fixed) At least if you can narrow down the repro steps, that might be enough.

[paynecodes]

@vfonic I apologize for not specifying, but I don’t actually use this gem. I’m building my own Node.js server. I believe this: https://help.shopify.com/en/api/guides/itp-impact is going to lead me down the right path.

[vfonic]

No worries! Hope you figure it out! I remember I spent a lot of time trying to make it work. :)
I think Shopify team also released some packages for Node.js development, be sure to check these out, if you haven't done so already.

https://github.com/Shopify