Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

[koa-shopify-auth] add prefix support post pull [#1413] #1428

Closed
wants to merge 2,283 commits into from
Closed

[koa-shopify-auth] add prefix support post pull [#1413] #1428

wants to merge 2,283 commits into from

Conversation

kavika-1
Copy link
Contributor

@kavika-1 kavika-1 commented May 10, 2020

Description

Fixes [#1148][#1426]

Apply prefix in koa-shopify-auth to various Safari/ITP redirects.

[#1413] addresses several issues with Safari by bringing in some code from shopify_app. However, that code was not koa-shopify-auth prefix aware. This small set of changes helps address this.

NOTE: There are similar PRs [#1149] and [#1196], hopefully this simply addresses the same issue post [#1413].

Quick discusion: While most of the redirects are related to prefixing "/auth" routes, there is one additional target, appTargetUrl, to which the prefix could be added to as well, since otherwise, upon client side redirect, the server has no idea where this came from. Note that the referer (sic) header is now blocked on safari, and even with the Referrer-Policy set, only the origin is provided not the path, so even that method cannot be used to infer the prefix upon return.

Type of change

  • koa-shopify-auth Patch: Bug/ Documentation fix (non-breaking change which fixes an issue or adds documentation)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above

alanthai and others added 30 commits February 20, 2020 09:09
@shopify/react-form-state README adds mention of @shopify/react-form
[@shopify/react-testing] Update the jest Matches to support the new @types/jest
Allow passing a `fetchPolicy` to `useBackgroundQuery`
…ates-pkg

Use @shopify/dates formatDate function and memoize Intl.NumberFormat
…ll-case

[react-form] Fix isChangeEvent null check
Allow hour12 true to be respected by formatDate
🔥 remove the mentioned of locale from quilt_rails and link the exampl…
…king

Add option to use a custom comparator for dirty state checking
Deprecate @shopify/react-shopify-app-route-propagator
@ragalie
Copy link
Contributor

ragalie commented May 13, 2020

@simonhaenisch I'm sorry that working with us and the library has been frustrating :( We're definitely trying to do a better job maintaining this library. I hope that we can earn back your trust and support, starting by getting this fix into the mainline. Thanks for the feedback on this.

@kavika-1
Copy link
Contributor Author

@ragalie, i signed the CLA a few days back, and if i try to sign again, it says "To sign our CLA 1 field needs to be changed / GitHub username has already signed a CLA". also, for some reason it shows a strange email address synthesized from my local network address, instead of my official public email. any way to restart that process?

otherwise, will work on item 2.

Mallen and others added 5 commits May 13, 2020 11:53
…-plugin (#1423)

* ✨ pass data and url through to app component by default in the webpack plugin

* 🚨 fix broken tests

* Update packages/react-server-webpack-plugin/src/react-server-webpack-plugin.ts

Co-authored-by: Michelle Chen <[email protected]>

* 🧹 PR feedback

Co-authored-by: Michelle Chen <[email protected]>
@marutypes
Copy link
Contributor

@kavika-1 I suspect the CLA check tries to use the email from the commits themselves which would explain why it is showing two versions of you... I'd double check that git config --get user.email returns what you expect 🤔

@ragalie not sure how we should proceed if the CLA but is not working after he checks that :/

@kavika-1
Copy link
Contributor Author

@TheMallen Darn, indeed. I set up a new macos environment, and indeed the user.email (and name) are not set. But don't know how to fix the past?

@marutypes
Copy link
Contributor

marutypes commented May 13, 2020

@kavika-1 I had a similar problem when I refromated my macbook recently, so I figured :P Sorry for all the trouble.

You should be able to do some history rewriting against your branch / clone (it will let you force push to your branch just not master). Github has a little guide for how to do it actually.

If it doesn't work for you I can do it for you, but I'd need to know what email to change it to and to make sure I have your permission as I wouldn't want to mess around with the history of your contributions lightly.

Alternatively, copying over your changes to a new branch would work but probably be more annoying.

@kavika-1
Copy link
Contributor Author

@TheMallen grateful for your experience, i'll give that rewrite a try!

@CautionTapeBot
Copy link

We noticed that this PR either modifies or introduces usage of the dangerouslySetInnerHTML attribute, which can cause cross-site scripting (XSS) vulnerabilities. Our team will take a look soon, but for now we recommend reviewing your code to ensure that this is what you intended to use and that there is not a safe alternative available. Docs are available here.

@CautionTapeBot
Copy link

👋 We noticed that this PR either modifies or introduces a link into help.shopify.com with a hardcoded locale. Help can automatically detect the locale from the HTTP Accept-Language header and redirect the user to the appropriate language.

Should you absolutely need to decide the language to be displayed, please use the locale query parameter.

Please consider updating the offending content.

For more info, check this out in the Home repo README: https://github.com/shopify/help#content-files

Find us at #docs :)

cc: @Shopify/docs-dev

@CautionTapeBot
Copy link

We noticed that this PR modifies the behaviour of CSRF tokens in this application. Our team will take a look soon, but for now please consider what the best CSRF behaviour for your application is. If the controller in question is meant to be used mostly as an API by non-browser clients, a sane option is protect_from_forgery with: :null_session, prepend: true (since APIs don't usually send CSRF tokens or use sessions anyway). If this endpoint is interacted with from a browser (via a form POST or similar), then it is good to use the stricter protect_from_forgery with: :exception. If you'd like to read more about Rails CSRF protection, there's some great Rails documentation on it: https://guides.rubyonrails.org/security.html#csrf-countermeasures.

@kavika-1
Copy link
Contributor Author

@TheMallen sorry seems i didn't quite get those instructions to work properly, and it rewrote the whole history? sorry 'bout that. i am not sure how to revert that change if possible at all, so i'll go to plan "B" and create a new branch/pull request unless you have a magic command to undo that :/

@kavika-1 kavika-1 closed this May 14, 2020
@kavika-1 kavika-1 deleted the koa-auth-add-prefix-to-redirect branch May 14, 2020 00:17
@marutypes
Copy link
Contributor

@kavika-1 sorry didn't get back to you in time, but I see you have a new branch that is passing CLA, so thanks again for the hard work :)

@kavika-1
Copy link
Contributor Author

@TheMallen thanks for your assistance and encouragement, The hard work was self-inflicted!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.