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 js for POST redirect back to separate file #1265

Merged
merged 4 commits into from
May 7, 2021

Conversation

greatestape
Copy link
Contributor

@greatestape greatestape commented May 5, 2021

This change adopts an earlier implementation we'd explored for the POST redirect page. Rather than embed the JS in an inline script tag, we're shipping it in a separate file.

This is an immediate response to an issue that was flagged in 18.0.0. This fix introduces a race condition for local dev environments. We don't love that, but it's more important to us to make sure that:

  1. Our latest release doesn't have a feature that might be incompatible with a consumer's CSP
  2. We still have a release out there that unblocks devs who want to update to the latest omniauth

What this PR does

  • Move JS for POST redirect back to separate file
  • Add file to asset manifest

Reviewer's guide to testing

  • Install the gem on an existing app and verify that the oauth install flow still works
    • NOTE: If your local dev env uses the cookie_store session storage strategy, you may encounter 401 errors during oauth due to a race condition between asset requests and /auth/shopify. You should be able to work around for local testing by using a different browser or session storage strategy.
  • Disabling source mapping in Chrome should eliminate the local dev race condition

Things to focus on

  • Does this change work for apps with Content Security Policies?
  • Is the race condition described above acceptable? i.e. is it limited to local dev environments?

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.

Inline JS can cause issues for apps with Content Security Policies set.
@CautionTapeBot
Copy link

We noticed that this PR modifies or enables Subresource Integrity for a script tag.
If the Javascript code is served from another domain (for example our CDN), it will cause a cross-origin error.
This can be solved by setting the crossorigin property to anonymous.

<%= javascript_include_tag ..., integrity: true, crossorigin: 'anonymous' %>

If you have any questions or are unsure about how to move forward with this, ping #help-appsec and we would be happy to help you out. cc: @Shopify/proactive-security

@greatestape greatestape self-assigned this May 5, 2021
Copy link

@DAmatheson DAmatheson left a comment

Choose a reason for hiding this comment

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

Tested with an app and confirmed this unblocked the install flow when unsafe-inline is disallowed.

CHANGELOG.md Outdated Show resolved Hide resolved
@jesalerno84 jesalerno84 merged commit 17c1640 into master May 7, 2021
@jesalerno84 jesalerno84 deleted the un-inline-post-redirect-js branch May 7, 2021 18:10
@jesalerno84 jesalerno84 mentioned this pull request May 7, 2021
4 tasks
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems May 10, 2021 19:49 Inactive
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.

6 participants