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

HandleAuthCallback redirectUrl fails when using AppSubscription Charge override #16

Open
melissadonohue opened this issue Jan 13, 2022 · 7 comments

Comments

@melissadonohue
Copy link

melissadonohue commented Jan 13, 2022

Bug Description:

Shopify throws an "Invalid Signature" error on App Subscription Charge acceptance for the merchant and they are unable to continue forward.
Screen Shot 2022-01-13 at 1 14 54 PM

To Reproduce:

in pages/api/auth/callback.js:

Create a custom afterAuth function to create a subscription.
Retrieve the confirmationUrl from the appSubscriptionCreate response
return confirmationUrl from function

          //  const recurringApplicationCharge = ....                    

         //fetch confirmationURL
            const redirectUrl =
                recurringApplicationCharge.body.data.appSubscriptionCreate
                    .confirmationUrl;

            // Redirect to billing url
            return redirectUrl;

Upon returning control to handleAuthCallback.js, the following code block executes:

      res.redirect(
        `${redirectPath || process.env.HOME_PATH}?${querystring.stringify(
          req.query
        )}`

The resulting redirectURL is set to

https://neutrl-test.myshopify.com/admin/charges/6343291/22336962698/RecurringApplicationCharge/confirm_recurring_application_charge?signature=BAh7BzoHaWRsKwiKAGMzBQA6EmF1dG9fYWN0aXZhdGVU--f798ca9351ce0b009fb468ec84047546fe1bc748?code=574ffe545a8611d071742065c1ef679e&hmac=9092dcabfa0b802eb11bfc0cb40e834245ed466c407c20f0beee2636f6d1cec1&host=bmV1dHJsLXRlc3QubXlzaG9waWZ5LmNvbS9hZG1pbg&shop=neutrl-test.myshopify.com&state=f18b54560a470f7f76a03b426918db27&timestamp=1642099550

The issue is that the redirect string is adding in an additional ? whereas in the overridden scenario, it should append an & as there is already a query parameter present in the URL string.

@ctrlaltdylan
Copy link
Owner

ctrlaltdylan commented Jan 13, 2022

@melissadonohue thanks, the library wasn't written with redirecting to a subscription consent URL immediately in the auth handler in mind.

I assume that's the problem specifically you were running into.

Sounds like you were trying to :

  1. Start OAuth
  2. On OAuth callback, generate a subscription confirmation URL
  3. Return this URL to the frontend to redirect the user to

In general the flow this lib supports out of the box is:

  1. Start OAuth
  2. Handle OAuth callback
  3. Home page is loaded
  4. Then allow the user to use the app in free mode, or redirect immediately to a subscription consent URL from a different API route on your app

@melissadonohue
Copy link
Author

Hey @ctrlaltdylan - Understood, I think we need to get the README.md updated in that case, as you call out the ability to do this here:

Screen Shot 2022-01-17 at 11 28 25 AM

@ahmed-adly-khalil
Copy link

@ctrlaltdylan @melissadonohue this problem will be fixed if we change the ? to & here:

`${redirectPath || process.env.HOME_PATH}?${querystring.stringify(

in better words, this line assumed it will take the return url and append 1 param while in the scenario above there is already a param which is the signature from the charge callback.

@ctrlaltdylan is it ok if i open a PR for this? I have the same issue and it would be great if we improve the library for all

@ctrlaltdylan
Copy link
Owner

ctrlaltdylan commented Jan 22, 2022

Thanks for the call out @melissadonohue , I started with a fix months ago and didn't commit it because I was having trouble blending the query params from the subscription redirect against the query params from the oauth callback.

I must have committed the content as if I fixed it, my bad.

But I guess the OAuth params don't need to be blended after all if you're redirecting the user directly back to Shopify for the subscription confirmation.

My main concern was that the frontend would render a 401/error page because the signature failed and app bridge couldn't instantiate.

@ahmed-adly-khalil I would accept either PR, but I could feel a lot better tagging a release if there was a basic test included too. The handleAuthCallback.test.js should include a test case with this scenario.

@ahmed-adly-khalil
Copy link

@ctrlaltdylan sounds great, I'm having even a more complex scenario, my app is not embedded, so I will try to cover that in the PR as well, not sure how exactly but I will work on it

@ahmed-adly-khalil
Copy link

@ctrlaltdylan I guess you also got the error Validation failed: Return url is too long (maximum is 255 characters) when merging oauth params to the charge return url, this makes it a bit harder, maybe the solution now is a custom session?

@ctrlaltdylan
Copy link
Owner

Building off of @melissadonohue 's solution using query string detection, this might solve for most cases:

// if a redirect path or URL is returned from `handleAuthCallback` use it
const newPath = redirectPath || process.env.HOME_PATH;

// if this new path contains a `?`, it's containing a query string and we should not attempt to double query string'ify
const newQueryString = !newPath.includes('?') && `?${querystring.stringify(req.query)}`;

// redirect to the Shopify Subscription page, or the normal internal path with the AppBridge params
res.redirect(newPath + newQueryString);

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

No branches or pull requests

3 participants