Shopify / shopify-api-js

Shopify Admin API Library for Node. Accelerate development with support for authentication, graphql proxy, webhooks
MIT License
941 stars 392 forks source link

missing cookies in callback request #130

Closed kato-takaomi-ams closed 3 years ago

kato-takaomi-ams commented 3 years ago

I'm trying to make an embedded app and use chrome browser. validateAuthCallback-function throws ShopifyErrors.SessionNotFound.

/api/callback.ts

await Shopify.Auth.validateAuthCallback(request, response, query as AuthQuery);

SessionNotFound [Error]: Cannot complete OAuth process. No session found for the specified shop url: myshop.myshopify.com

sameSite: 'lax', missing cookies in callback request .

    cookies.set(ShopifyOAuth.SESSION_COOKIE_NAME, session.id, {
      signed: true,
      expires: new Date(Date.now() + 60000),
      sameSite: 'lax',
      secure: true,
    });

I've temporarily changed sameSite:'none', and it works, but I don't know how to fix it correctly.

paulomarg commented 3 years ago

Hey @kato-takaomi-ams, are you redirecting the user to the OAuth URL inside the iframe in your Shopify Admin page? The OAuth process should always happen at the top level of the browser, so that cookies can be used (your solution works for Chrome but would fail on Safari).

You can use the App Bridge redirect method to change the browser window URL from within your embedded app to start OAuth. I added a bit more detail into that on https://github.com/Shopify/koa-shopify-auth/issues/61#issuecomment-795574311.

kato-takaomi-ams commented 3 years ago

@paulomarg I can not solve this issue yet. Just idea. https://github.com/Shopify/shopify-node-api/blob/16ae704c897f32ec98d406fb2d1e22319c87d6f5/src/auth/oauth/oauth.ts#L96 By moving 'validate state' && safeCompare(query.state, session.state as string); after 'post /access_token' const postResponse = await client.post(postParams); wouldn't it be possible to get the sessionid from query.shop and postResponse?

  getCallbackSessionId(query.shop, postResponse): string {
    if (postResponse.body.associated_user) {
      return this.getJwtSessionId(shop, postResponse.body.associated_user.id);
    } else {
      return this.getOfflineSessionId(shop);
    }
  },

If this modify is ​​possible, I think the oauth process does not need to use cookies.

paulomarg commented 3 years ago

Thanks for the suggestion @kato-takaomi-ams , but unfortunately we can't use the shop from the query as it is not unique enough - if multiple people attempt to perform OAuth for a same shop at the same time the sessions would overlap and the app could mix up which user is making a request.

There are similar reports of an issue where the library isn't sending the user to the top level during the OAuth process, but I couldn't reproduce it on my end. Could you:

kato-takaomi-ams commented 3 years ago

Thank you for considering my suggest. I thought I could make it cookieless with the same behavior as the current source code... Sorry. I'm trying it with NextJS(api-routes) without koa, so it may be a special situation. I will consider it myself a little more.

Thank you for sparing your precious time for me.

kato-takaomi-ams commented 3 years ago

if nonce/state is unique enough.

In beginAuth, set nonce to the temporary session ID, and In validateAuthCallback, loadSession with query.state.

Is it possible to exclude cookies from the oauth process this way?

paulomarg commented 3 years ago

This seems like an interesting idea, but using the state to track the session has its own issues - mainly that it would be less secure because we'd be passing the session id as a clear value in the URL. With signed cookies, we can help ensure that only the client and server know the session id, which makes it more difficult to sniff them, so apps are less vulnerable to attack.

hartherbert commented 3 years ago

@paulomarg Yeah that's true but still this approach is not usable right now for embedded apps because all the third party cookies are stripped out. How should this work in safari with embedded apps without redirecting the only after the app loads? I get the same error, that no session is found.

Right now this whole oauth process is not usable for someone not allowed to use cookies in iframes and not wanting to load the app and then redirect the user to oauth. The redirect to the oauth should happen on the first request to the app.

kato-takaomi-ams commented 3 years ago

@paulomarg If the session ID leak is a security issue, do you think the same issue is with the session ID of the offline_access_token? offline_access_token is formatted offline_{shop}, always obvious from the URL.

midthun commented 3 years ago

Having the same issue, using version 1.2.0 and there is no way to get past the Shopify.Auth.validateAuthCallback in my embedded app because of the cookies. Getting the error Cannot complete OAuth process. Could not find an OAuth cookie for shop url: xxx after upgrading to 1.2.0

hartherbert commented 3 years ago

@midthun like @paulomarg said, you should use the appbridge on the client to redirect the user to your domain outside the iframe, then complete the auth and return to the iframe. That's how I implemented it now and it's working.

midthun commented 3 years ago

@hobbydevs thank you, I am trying that out now. I seem to be stuck in a loop that it does not find the session. I get redirected and auth completes and I can verify that the session is created, then I try to redirect back to my shop/iframe and the process just repeats itself over and over. Where exactly do you perform this redirect in your app?

midthun commented 3 years ago

I think I got it now, what I was missing was doing an authenticatedFetch (including the Bearer token). The documentation and examples are abstracting all of this, so it took a little while to understand what was going on. Plus the fact that the app is doing a redirect to the embedded app by default (configurable). This page led me in the right direction: https://community.shopify.com/c/Shopify-APIs-SDKs/App-Bridge-React-and-the-new-auth-method/td-p/840009

I am coming from the Angular world, where we usually use http interceptors to add Bearer token and other stuff.

paulomarg commented 3 years ago

Thanks for all the comments folks. I'm just going back to the original discussion here with @kato-takaomi-ams:

@paulomarg If the session ID leak is a security issue, do you think the same issue is with the session ID of the offline_access_token? offline_accesstoken is formatted offline{shop}, always obvious from the URL.

The thing with offline tokens is that you don't want to load them from user input (like getting the shop from a URL or form parameter), since offline tokens are meant to be used for background tasks. In that case, your app's backend should 'know' which offline tokens are OK to use, not the user.

Since ideally the ids for offline tokens aren't passed from the user's browser to the app, the security risk isn't there (even though it would a risk if one did as I mentioned above!).

To answer @hobbydevs's question, it's OK to use cookies for OAuth because it needs to happen at the browser top level (where cookies/localStorage are not a problem) - you can't perform OAuth inside an iframe because it involves redirecting the user between sites. Unfortunately, that is something we can't work around from the app's side.

scole954387 commented 3 years ago

I'm a complete newb but if it helps someone here I've noticed that this happens if I leave the browser alone for on the app install page.

Basically I go to: https://myassigned.ngrok.io/auth?shop=my-store.myshopify.com

When I get the "You are about to install AppName" page I do nothing. If I leave it and try in a minute or so it will give the "Cannot complete OAuth process" internal server error. If I click the install button right away it'll go through properly and the app will install.

I don't know if this is related to the above or just the fact that it's a local dev server. Just thought I'd share when it happens to me that might help someone figure it out.

I'm using the app build with the Shopify CLI by the way.

paulomarg commented 3 years ago

That's expected @scole954387 - the OAuth process has a limit of 60s to be completed, it expires the session after that point so we're not left with pending sessions as that may be a security risk.

abbasharoon commented 3 years ago

I was having the same issue, a trivial one due to mis-configuration. In the app setup, I sat the url for the app to http://localhost:3000 for development however for redirect_uri, I was using ngrok. Since the cookie was sat on the original request, the app wasn't able to retrieve it over ngrok url which is a completely different domain.

Hope that may help some who are having the same issue due to mis-configured urls.

crackthecodeabhi commented 3 years ago

That's expected @scole954387 - the OAuth process has a limit of 60s to be completed, it expires the session after that point so we're not left with pending sessions as that may be a security risk.

@paulomarg while the Shopify QA tests the app for approval, sometimes they click "Install this app" after the 60 seconds limit, by then the cookie expires and they are shown a 500 Internal Server error, since nothing can be done without access to that cookie which was set in beginAuth

This causes rejection of App by Shopify, can we do something about this? Sometime a merchant might want to read all the permissions the app has requested, and might exceed the 60 second limit, thus causing a auth error and unstable state of app installation.

From Shopify's view, app is installed and available in Admin > Apps, but since we couldn't get hold of the shopify_app_session cookie, due to expiry, we can't complete the auth and can't get the accesstoken.

Is there any work around for this? Because this is blocking the app from being approved by Shopify QA.

paulomarg commented 3 years ago

That's a fair point @justoneabhi - when your app calls validateAuthCallback, it will throw a CookieNotFound error in this scenario, since the cookie will have expired.

Your app could catch that error and redirect the user back to restart OAuth. We'll look into fixing the example app to do the same thing so we don't return a plain error, but rather just try again.

Thanks for the feedback!

crackthecodeabhi commented 3 years ago

Hey @paulomarg, i did try that work around before bringing it up here. The CookieNotFound error is exported but in the shopifyAuth package, the error is handled by throwing a Koa 500 error, with cookie not found message. The Error type is lost, so we cannot effectively catch that error.

So only way we could handle is, by checking if the Koa 500 message contains the message "Cookie not found ....", not a good way to handle errors IMO

Is it possible to like, re-throw this error or as root cause of the this 500 exception ? so that we can catch it and handle it?

 if (ctx.path === oAuthCallbackPath) {
      try {
        await Shopify.Auth.validateAuthCallback(ctx.req, ctx.res, ctx.query);

        ctx.state.shopify = await Shopify.Utils.loadCurrentSession(ctx.req, ctx.res, config.accessMode === 'online');

        if (config.afterAuth) {
          await config.afterAuth(ctx);
        }
      }
      catch (e) {
        switch (true) {
          case (e instanceof Shopify.Errors.InvalidOAuthError):
            ctx.throw(400, e.message);
            break;
          case (e instanceof Shopify.Errors.SessionNotFound):
            ctx.throw(403, e.message);
            break;
          default:
            ctx.throw(500, e.message);
            break;
        }
      }
      return;
    }

the above code throws 500 error while handling CookieNotFound Error

paulomarg commented 3 years ago

Yes, we can do that - we'll make it so CookieNotFound and SessionNotFound both go back to OAuth (as they amount to a 403 Unauthorized) instead of returning an error response.

crackthecodeabhi commented 3 years ago

@paulomarg thanks

benfarhner commented 3 years ago

This issue has caused us 3 app rejections and a suspension because we couldn't figure out how the Shopify testers were causing an Internal Server Error, but thanks to this thread I was finally able to repro the issue and determine it's due to the tester waiting 60s before continuing the OAuth process.

  1. Is there a timeline for getting this fixed so we can handle the exception ourselves rather than immediately throwing a 500 error?
  2. Is there a workaround we can use in the meantime so we can get our app approved?

Note: we're using @shopify/koa-shopify-auth but that library has this exact same issue.

valorloff commented 3 years ago

Official sample app have absolutely same OAuth 60-sec-limit problem

charliefuller commented 3 years ago

I'm hitting the same error if I wait 60s on the OAuth page before continuing. This is happening in app.use(shopifyAuth(...)) which I don't think I can catch errors from? @paulomarg

As a workaround I've written this patch to redirect to /auth when hitting SessionNotFound and CookieNotFound errors: https://gist.github.com/charliefuller/e5b65e706886f818d0109f03230810d6

valorloff commented 3 years ago

I've written this patch

hey man, it looks like it works, thanks!

ilugobayo commented 3 years ago

Official sample app have absolutely same OAuth 60-sec-limit problem

So this hasn't been officially fixed then

I've written this patch

hey man, it looks like it works, thanks!

How did you test this? Did you just directly modified the lines of code in the node_modules/@shopify/koa-shopify-auth/dist/src/auth/index.js file?

valorloff commented 3 years ago

So this hasn't been officially fixed then

Yes, hasn't fixed yet

directly modified the lines of code

Yes! exactly! otherwise how long to wait from shopify?

ilugobayo commented 3 years ago

I've been dealing with this for months by now, if that workaround actually works, it will be such good news for my team.

Thank you for the feedback and @charliefuller for sharing that fix.

valorloff commented 3 years ago

Don't forget deploy patched index.js after npm install on production server

ilugobayo commented 3 years ago

Thanks, just tested it on my development environment and it actually works! This is amazing

charliefuller commented 3 years ago

You're welcome but please remember this is just a temporary fix and may have unexpected side effects! You can use patch-package to apply the patch automatically.

Israel001 commented 3 years ago

Hey @kato-takaomi-ams, are you redirecting the user to the OAuth URL inside the iframe in your Shopify Admin page? The OAuth process should always happen at the top level of the browser, so that cookies can be used (your solution works for Chrome but would fail on Safari).

You can use the App Bridge redirect method to change the browser window URL from within your embedded app to start OAuth. I added a bit more detail into that on Shopify/koa-shopify-auth#61 (comment).

Hi, Please can you show an example of using the app bridge redirect method in my case? In my app, when the user visits the / endpoint, i redirect them to the /auth route if they are not authenticated and this is how i handle the auth route (using NestJS with NextJS):

@Get()
async login(@Req() req: Request, @Res() res: Response) {
  if (!req.query.shop)
    return res.send('Expected a valid shop query parameter');
  const authRoute = await Shopify.Auth.beginAuth(
    req,
    res,
    req.query.shop as string,
    'auth/callback',
    true,
  );
  return res.redirect(authRoute);
}

Please note that at this point, the app is not loaded in an iframe yet, the error is happening at first installation. Basically, the flow is user opens my app without being authenticated, app redirects them through nestjs to oauth url, and oauth url redirects them back to my app. What is the correct flow suppose to be?

I assume the issue of the CookieNotFound comes from return res.redirect(...) line, but please how can i modify this so that it will use the app bridge redirection method instead. I really have no clue what to do here. Thanks in advance

paulomarg commented 3 years ago

Hey folks, thanks a lot for your patience and suggestions. Version 4.1.4 is out which should solve this issue!

malipetek commented 2 years ago

@paulomarg how can we solve this issue if we are not using koa?

hgezim commented 2 years ago

Here's how I resolved this issue. The front-end uses Next.js and backend uses Nest.js.

In my auth endpoint (this is what's plugged in the Shopify app settings as the App URL:

public async auth(
  @Res() response: Response,
  @Req() request: Request,
  @Query('shop') shop: string,
) {
  const authRoute = await Shopify.Auth.beginAuth(
    request,
    response,
    shop,
    `${API_PREFIX}/${CONTROLLER_PREFIX}/${SHOPIFY_AUTH_CALLBACK_ENDPOINT}`,
    true,
  );

  response.redirect(authRoute);
}

Then in the SHOPIFY_AUTH_CALLBACK_ENDPOINT endpoint, I have this:

public async authCallback(
  @Query() query: any,
  @Req() request: Request,
  @Res() response: Response,
): Promise<void> {
  try {
    await Shopify.Auth.validateAuthCallback(request, response, query as AuthQuery);
    response.redirect(`${Config.APP_URL}/shopify?${querystring.stringify({ host: query.host })}`);
  } catch (e) {
    const oauthUrl = querystring.stringify({
      fix: `${CONTROLLER_PREFIX}/${SHOPIFY_AUTH_ENDPOINT}?shop=${query.shop}`,
    });

    switch (true) {
      case e instanceof Shopify.Errors.InvalidOAuthError:
        if ((e as Error).message) {
          throw new Error((e as Error).message);
        }
        throw new Error('Failed to complete the Shopify auth process: Inavlid oauth error');
      case e instanceof Shopify.Errors.CookieNotFound:
      case e instanceof Shopify.Errors.SessionNotFound:
        // This is likely because the OAuth session cookie expired before the merchant approved the request
        response.redirect(
          `${FRONTEND_URL}/shopify-auth?${oauthUrl}`,
        );
        break;
      default:
        if ((e as Error).message) {
          throw new Error((e as Error).message);
        }
        throw new Error('Failed to complete the Shopify auth process: Generic auth failured');
    }
  }
}

Then in the shopify-auth page on the front-end, I have this:

import React from 'react';
import { useRouter } from 'next/router';

export const Index = () => {
  const router = useRouter();

  if (router.query.fix) {
    if (typeof window !== 'undefined' && window.top && typeof router.query.fix === 'string') {
      // bust out of Shopify's iframe
      window.top.location = `${API_URL}/${router.query.fix}`; // TODO: replace `fix` with something better
    }
  }

  return (
    <Page>
      <Layout>Authenticating...</Layout>
    </Page>
  );
};

export default Index;

Hope this helps someone!

ExpoTech12 commented 2 years ago

Hi, I'm new to shopify app development, and not as experienced as most of the people on here regarding web dev. I followed the shopify instructions and it says: ┃ Browserslist: caniuse-lite is outdated. Please run: ┃ npx browserslist@latest --update-db ┃ Loaded env from C:\Users\SR\Documents\GitHub\ShopifyApps\thingiscraper.env ┃ (node:17440) [DEP0148] DeprecationWarning: Use of deprecated folder mapping "./" in the "exports" field module resolution of the package at C:\Users\SR\Documents\GitHub\ShopifyApps\thingiscraper\node_modules\next\node_modules\postcss\package.json. ┃ Update this package.json to use a subpath pattern like "./*". ┃ (Use node --trace-deprecation ... to show where the warning was created) ┃ event - compiled successfully ┃ > Ready on http://localhost:8081

I have made sure to update browserslist etc too.

I seem to still be facing the same issue as seen here, in that when I browse to the localhost, I get undefined connection as it can't validate this. So it seems as though this issue hasn't been fixed?

I tried to follow the above details from charliefuller which refer to this: https://gist.github.com/charliefuller/e5b65e706886f818d0109f03230810d6

Is this meant to be pasted into mynodeshopifyapp\node_modules\@shopify\koa-shopify-auth\dist\src\auth\index.js???

Any help would be appreciated.

Cheers Sam

charliefuller commented 2 years ago

I tried to follow the above details from charliefuller which refer to this: https://gist.github.com/charliefuller/e5b65e706886f818d0109f03230810d6

You don't need this workaround anymore as the issue has been fixed. I've deleted the gist to avoid confusion.

molotow11 commented 1 year ago

I still have the same issue, tried sameSite:'none', with no luck.

1

2

It looks like Shopify redirected it back with my cookie existing, but this undefined for some reason.