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

Move app data back into body #1362

Merged
merged 2 commits into from
Feb 1, 2022
Merged

Move app data back into body #1362

merged 2 commits into from
Feb 1, 2022

Conversation

hannachen
Copy link

What this PR does

Follow-up to #1247

An unintentional side-effect was introduced while cleaning up .erb templates. Moving app information from the body to a <script> tag violates the unsafe-inline Content Security Policy.

Reviewer's guide to testing

  1. Clone this branch
  2. In a terminal, use Shopify CLI to create and configure a new rails app: shopify app create rails
  3. Using a text editor, open the Gemfile inside the newly created app
  4. Modify the entry of shopify_app to point to this branch in your local environment, it should looks similar to this:
    gem 'shopify_app', path: '~/src/github.com/Shopify/shopify_app'
  5. In a terminal, start your app server with shopify app serve, this will also output a ngrok url
  6. In a browser, follow the ngrok url to install the app on a test store. It will look similar to this:
    https://1587ef2f72d5.ngrok.io/login?shop=sabotender-shop.myshopify.com
    1. Test 1: On your local environment, remove the shop entry for the store inside the app db (this will simulate one of the cases)
    2. Test 2: Update your generated app scope inside file config/intializers/shopify_app.rb to trigger a redirect from within the shop frame
  7. In a browser, open the installed app. i.e. https://sabotender-shop.myshopify.com/admin/apps/test-app-1
  8. Ensure that the entire redirect flow takes the browser back to the app.

Things to focus on

  1. Focus on install/uninstall
    1. Test 1: Case of a possible incomplete install, where the shop thinks the app is installed, but the app does not have the shop data
    2. Test 2: Case of an app updating access scope, and requires re-auth by breaking out of the shop frame
  2. Other areas that might kick off a top level redirect, or use the methods fullpage_redirect_to, authenticate_at_top_level, or enable_cookie_access (Safari browser with strict 3rd party cookie policy)

Checklist

Before submitting the PR, please consider if any of the following are needed:

  • Update CHANGELOG.md if the changes would impact users
  • Update README.md, if appropriate.
  • Update any relevant pages in /docs, if necessary
  • For security fixes, the Disclosure Policy must be followed.

Copy link
Member

@henrytao-me henrytao-me left a comment

Choose a reason for hiding this comment

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

🎩-ed. LGTM

Screen Shot 2022-02-01 at 14 47 13

@hannachen hannachen merged commit 3a75d14 into master Feb 1, 2022
@hannachen hannachen deleted the fix-unsafe-inline branch February 1, 2022 22:27
@hannachen hannachen mentioned this pull request Feb 2, 2022
4 tasks
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