Top level partitioning for safari 12 #617

ragalie · 2018-08-10T12:51:01Z

Webkit is releasing ITP 2.0 as part of the OS updates scheduled for Fall 2018. These updates, which will affect merchants using Safari 12, break embedded apps in Shopify admin by preventing apps from using cookies in the iframe. Apps rely on these cookies to authenticate the merchant when the app loads in an iframe.

This PR works around ITP 2.0 by triggering "cookie partitioning". Partitioning is triggered when the user interacts with a domain at the top-level before interacting with it in the iframe. In the partitioned state apps are able to use cookies in the iframe, but those cookies aren't available outside of the iframe context.

These limitations required us to implement a more convoluted flow to ensure that we auth correctly. That flow is diagrammed below, and this PR implements that flow:

ragalie · 2018-08-10T12:53:40Z

app/controllers/shopify_app/sessions_controller.rb

      if sanitized_shop_name.present?
        session['shopify.omniauth_params'] = { shop: sanitized_shop_name }
-        fullpage_redirect_to "#{main_app.root_path}auth/shopify"
+        if session['shopify.cookies_persist'] || !ShopifyApp.configuration.embedded_app?


I think there's a subtle bug here.

When we're in fullpage mode, we might already have the shopify.cookies_persist cookie, which would mean we'd take this path. But the whole reason we're in fullpage mode is that we want to trigger a "fullpage redirect", since that's where our ITP logic currently lives.

We should separate these two concerns by fullpage redirecting to a set_top_level_cookie action or passing a similarly-named parameter that will always trigger the top-level cookie (ITP) logic.

nwtn · 2018-08-10T16:21:08Z

app/assets/javascripts/shopify_app/itp_polyfill.js

+  var versionNumber = isSafari ? parseFloat(userAgent.match(/Version\/(\d+\.?\d*)/)[1]) : null;
+
+  // TODO: Replace with library for checking user-agents
+  return document.hasStorageAccess && isSafari && versionNumber >= 12;


is there any way to do this as a feature check instead of using user agents? i'm concerned about the resiliency of this if, e.g., chrome or another browser go down the same path as safari. or (much less likely) if apple reverses course.

What about on Mobile?
POS: com.jadedpixel.pos Shopify POS/4.8.3 (iPhone; iOS 11.4; Scale/3.00) MobileMiddlewareSupported
Shopify Mobile:Shopify Mobile/iOS/7.6.0 (iPhone10,5 Simulator/com.shopify.ShopifyInternal/11.4.0)
Safari: Mozilla/5.0 (iPhone; CPU iPhone OS 12_0 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Mobile/15E148 Safari/604.1

Do you have a way to test the behaviour on an iOS 12 device? Ive thus far been unable to see auth broken on my device running iOS 12 b6.

cc. @andrewapperley

@davidmuzi I've also not seen apps broken. I'm running the latest iOS 12 beta as well. If we're using UserAgent checks then we would need to check for iOS 12 as well. I'll reply here if I see any announcement from Apple about ITP 2.0 on iOS or if its in the release notes of a new beta.

@nwtn We were only doing feature detection before for document.hasStorageAccess, and the ask to do feature detection came up in reviews with other stakeholders.

@andrewapperley Thanks for clarifying! I'll keep an eye out for announcements as well :)

I agree that a check for hasStorageAccess should be sufficient, otherwise this may miss other webkit based browsers that implement hasStorageAccess. Since storage access is baked right into webkit I expect other browsers will be affected over time. The list of webkit-based browsers is long, including Chrome on iOS according to this: https://en.wikipedia.org/wiki/List_of_web_browsers#WebKit-based

vfonic

I'm really happy to see this being worked on! Thank you guys! :)

I've added some comments, mostly details. I haven't tested these changes.

vfonic · 2018-08-16T14:15:10Z

lib/shopify_app/controller_concerns/login_protection.rb

@@ -57,14 +57,14 @@ def clear_shop_session
      session[:shopify_user] = nil
    end

-    def login_url
+    def login_url(no_cookie_redirect = false)


Would be better if no_cookie_redirect was a keyword argument.

Right now the API to call this method is: https://github.com/Shopify/shopify_app/pull/617/files#diff-350e7187d4b8bbcd1eaee3ff44fc7fb3R68
login_url(true)
while, with keyword argument, it would change to:
login_url(no_cookie_redirect: true)

vfonic · 2018-08-16T14:15:56Z

lib/shopify_app/controller_concerns/login_protection.rb

@@ -5,19 +5,19 @@ module LoginProtection
    class ShopifyDomainNotFound < StandardError; end

    included do
+      after_action :set_test_cookie if ShopifyApp.configuration.embedded_app?


I don't see any tests added for these changes (just some slight modifications). Can you please add some?

vfonic · 2018-08-16T14:16:33Z

config/locales/ja.yml

+  enable_cookies_heading: "Enable cookies from %{app}"
+  enable_cookies_body: "You must manually enable cookies in this browser in order to use %{app} within Shopify."
+  enable_cookies_footer: 'Cookies let the app authenticate you by temporarily storing your preferences and personal information. They expire after 30 days.'
+  enable_cookies_action: 'Enable cookies'


These all are in English (I guess you know that :) ), and you're missing a new line here.

vfonic · 2018-08-16T14:20:36Z

app/assets/javascripts/shopify_app/itp_polyfill.js

+  document.cookie = "shopify.cookies_persist=true";
+  window.location.href = window.shopOrigin + "/admin/apps/" + window.apiKey;
+
+  return false;


You don't need this. The button you've attached the "click" event at, it's type="button", therefore won't do any for submission or navigation. return false to prevent clicks is generally considered to be a bad practice (due to stopping event propagation)

vfonic · 2018-08-16T14:25:59Z

app/controllers/shopify_app/sessions_controller.rb

@@ -14,6 +14,10 @@ def create
      authenticate
    end

+    def enable_cookies
+      @shop = sanitize_shop_param(params)


You could just reuse (call) sanitized_shop_name from LoginProtection. It does almost exactly and I like to avoid using @shop variable name as a name for shop's domain. (Perhaps the best solution would be to rename the variable to @shop_domain)

vfonic · 2018-08-16T14:31:06Z

app/views/shopify_app/sessions/enable_cookies.html.erb

+<html lang="en">
+<head>
+  <meta charset="utf-8" />
+  <link rel="stylesheet" href="https://sdks.shopifycdn.com/polaris/2.5.0/polaris.min.css" />


This "redirect" page is loading 134KB of styles. Can you please extract the required styles and inline them like you did right here below?

vfonic · 2018-08-16T14:31:40Z

app/views/shopify_app/sessions/enable_cookies.html.erb

+    }
+  </style>
+  <base target="_top">
+  <title>Redirecting…</title>


Nitpick: You could use ... (three characters) instead of … (one character)

vfonic · 2018-08-16T14:34:31Z

lib/shopify_app/controller_concerns/login_protection.rb

-      else
-        redirect_to_login
+      return redirect_to_login unless shop_session
+      clear_top_level_oauth_cookie


Is this necessary to call on every request?

Maybe it's better to simply ignore the cookie after you no longer need it? Or clear it somewhere during the authentication/cookie-setting process, once.

We can't ignore it; we need to remove it or there's a risk that we won't run the full flow the next time we can't recover the session.

We need to wait until we confirm that we've actually recovered the session to remove the cookie, and this seems like the least bad place to do that. We could check that the cookie doesn't exist, but that's probably slower than just deleting the key from the hash regardless.

vfonic · 2018-08-16T14:44:29Z

config/locales/en.yml

@@ -2,3 +2,7 @@ en:
  logged_out: 'Successfully logged out'
  could_not_log_in: 'Could not log in to Shopify store'
  invalid_shop_url: 'Invalid shop domain'
+  enable_cookies_heading: "Enable cookies from %{app}"
+  enable_cookies_body: "You must manually enable cookies in this browser in order to use %{app} within Shopify."
+  enable_cookies_footer: 'Cookies let the app authenticate you by temporarily storing your preferences and personal information. They expire after 30 days.'


Do cookies really expire after 30 days? I think this depends on the app, and, as far as I can tell, rails doesn't set expiration on cookies. Sorry I couldn't find anything more useful. :/

https://github.com/rails/rails/blob/39cb84e8bdd4b9a35d68b33222a174d2350aee89/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb#L44

Hey @vfonic, this is enforced by ITP: https://webkit.org/blog/8311/intelligent-tracking-prevention-2-0/, and is the reason this branch is being worked on.

ragalie · 2018-08-21T12:43:51Z

One thing I think we should do: we should check the user agent for POS/mobile and skip the "enable cookies" button in that case.

We know that ITP doesn't apply because we're not using SafariViewControllers to render the embedded apps, but there's no easy way for the app to know that.

I know we were opposed to this for the purposes of determining whether ITP applied in general, but if the user agents are stable enough I think it makes sense in this context.

francinen · 2018-08-21T15:04:50Z

cc @ShayneP

tylerball

A few suggestions, but looks good!

tylerball · 2018-09-04T18:33:06Z

app/assets/javascripts/shopify_app/itp_polyfill.js

@@ -0,0 +1,16 @@
+function setCookieAndRedirect() {


it may be wise to wrap all this code in an IIFE just for safety and to avoid interference from browser extensions, etc.

tylerball · 2018-09-04T18:33:18Z

app/assets/javascripts/shopify_app/redirect.js

@@ -1,5 +1,10 @@
-document.addEventListener("DOMContentLoaded", function() {
+function redirect() {


same suggestion as above

tylerball · 2018-09-04T18:38:21Z

app/views/shopify_app/sessions/enable_cookies.html.erb

+  <base target="_top">
+  <title>Redirecting…</title>
+
+  <script src="https://cdn.shopify.com/s/assets/external/app.js?<%= Time.now.strftime('%Y%m%d%H') %>"></script>


Do we need to load the EASDK here since we're not initializing it?

nwtn · 2018-09-04T18:54:38Z

app/assets/javascripts/shopify_app/itp_polyfill.js

+
+document.addEventListener("DOMContentLoaded", function() {
+  if (document.hasStorageAccess) {
+    var itpContent = document.querySelector('#CookiePartitionPrompt');


minor optimization, but getElementById is faster than querySelector with an id (ref)

nwtn · 2018-09-04T19:00:06Z

app/views/shopify_app/sessions/enable_cookies.html.erb

@@ -0,0 +1,388 @@
+<!DOCTYPE html>
+<html lang="en">


presumably this should change based on locale

nwtn · 2018-09-04T19:02:53Z

app/views/shopify_app/sessions/enable_cookies.html.erb

+<html lang="en">
+<head>
+  <meta charset="utf-8" />
+  <style>


This feels super fragile. I’m assuming you’re embedding this here for performance, so we’re not loading all the Polaris CSS for a single redirect page?

Yes, I think that was the logic. I know we got some pushback around including all of the CSS given the size, especially for something that's relatively static like this page.

jklekotka

LGTM, but given there has been a lot of changes and conditions added to the session controller, I would suggest more 👀and 👍 before you merge the changes

ITP doesn't apply to WebKitView

ragalie assigned ragalie and francinen Aug 10, 2018

ragalie commented Aug 10, 2018

View reviewed changes

nwtn reviewed Aug 10, 2018

View reviewed changes

francinen force-pushed the top-level-partitioning-for-safari-12 branch 2 times, most recently from b77ae63 to 440d389 Compare August 15, 2018 21:14

vfonic reviewed Aug 16, 2018

View reviewed changes

ragalie force-pushed the top-level-partitioning-for-safari-12 branch 2 times, most recently from d4d152c to 1534963 Compare August 16, 2018 19:06

ragalie force-pushed the top-level-partitioning-for-safari-12 branch from 39412d9 to 83eed27 Compare August 21, 2018 12:45

francinen mentioned this pull request Aug 23, 2018

Add initial configuration for translation platform #623

Merged

3 tasks

francinen force-pushed the top-level-partitioning-for-safari-12 branch from 58e7c56 to 5e920ff Compare August 23, 2018 18:03

francinen requested a review from tylerball August 29, 2018 13:36

francinen changed the title ~~[WIP] Top level partitioning for safari 12~~ Top level partitioning for safari 12 Aug 29, 2018

This was referenced Sep 4, 2018

ITP2 fixes Shopify/quilt#268

Merged

ITP2 fixes Shopify/shopify-express#108

Merged

tylerball approved these changes Sep 4, 2018

View reviewed changes

nwtn reviewed Sep 4, 2018

View reviewed changes

ragalie force-pushed the top-level-partitioning-for-safari-12 branch from 0e02279 to 5deb12f Compare September 5, 2018 04:16

jklekotka approved these changes Sep 7, 2018

View reviewed changes

Francine Navarro and others added 8 commits September 10, 2018 08:30

Work around Safari 12 ITP restrictions

f1b3167

Don't display prompt for pos and mobile

371eacb

ITP doesn't apply to WebKitView

IIFE

f9ee88b

Remove unused EASDK load

fd1a037

Set correct lang attribute

e099511

Style

7321b5c

Fix transient test failure

e4e2474

Remove placeholders from non-English locale files

27c76e3

ragalie force-pushed the top-level-partitioning-for-safari-12 branch from a4c808e to 27c76e3 Compare September 10, 2018 12:30

ragalie merged commit bf6b92d into master Sep 10, 2018

ragalie deleted the top-level-partitioning-for-safari-12 branch September 10, 2018 12:53

ragalie deployed to rubygems September 10, 2018 14:30 Active

stlk mentioned this pull request Oct 29, 2018

[WIP] Modify auth flow to support Safari 12 discolabs/django-shopify-auth#46

Closed

brendo mentioned this pull request Jun 18, 2024

Remove references to unused assets and features #1865

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Top level partitioning for safari 12 #617

Top level partitioning for safari 12 #617

ragalie commented Aug 10, 2018 •

edited

Loading

ragalie Aug 10, 2018

nwtn Aug 10, 2018

davidmuzi Aug 10, 2018

andrewapperley Aug 14, 2018

francinen Aug 14, 2018

EiNSTeiN- Aug 14, 2018 •

edited

Loading

vfonic left a comment •

edited

Loading

vfonic Aug 16, 2018

vfonic Aug 16, 2018

vfonic Aug 16, 2018

vfonic Aug 16, 2018

vfonic Aug 16, 2018 •

edited

Loading

vfonic Aug 16, 2018

vfonic Aug 16, 2018

vfonic Aug 16, 2018

ragalie Aug 16, 2018

vfonic Aug 16, 2018

EiNSTeiN- Aug 16, 2018

ragalie commented Aug 21, 2018

francinen commented Aug 21, 2018

tylerball left a comment

tylerball Sep 4, 2018

tylerball Sep 4, 2018

tylerball Sep 4, 2018

nwtn Sep 4, 2018

nwtn Sep 4, 2018

nwtn Sep 4, 2018

ragalie Sep 5, 2018

jklekotka left a comment

		@@ -1,5 +1,10 @@
		document.addEventListener("DOMContentLoaded", function() {
		function redirect() {

Top level partitioning for safari 12 #617

Top level partitioning for safari 12 #617

Conversation

ragalie commented Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EiNSTeiN- Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

vfonic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vfonic Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ragalie commented Aug 21, 2018

francinen commented Aug 21, 2018

tylerball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklekotka left a comment

Choose a reason for hiding this comment

ragalie commented Aug 10, 2018 •

edited

Loading

EiNSTeiN- Aug 14, 2018 •

edited

Loading

vfonic left a comment •

edited

Loading

vfonic Aug 16, 2018 •

edited

Loading