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

Koa Session Cookie Blocking Reinstallation #727

Closed
chadhamre opened this issue May 31, 2019 · 9 comments · Fixed by #844 or #940
Closed

Koa Session Cookie Blocking Reinstallation #727

chadhamre opened this issue May 31, 2019 · 9 comments · Fixed by #844 or #940

Comments

@chadhamre
Copy link

Overview

When using @shopify/koa-shopify-auth to handle auth for my Shopify App, I was unable to reinstall the app, after uninstalling it, without first clearing the browser cache. After some troubleshooting, it seems that that the browser will pass the expired koa session cookies with the reinstallation attempt, which results in a failed redirect.

I'm wondering if anybody else has run into this issue.

@esteban-filardi
Copy link

I think I'm also having this issue. The session variables persists the app removal, and it seems that when trying to install the application again, the previous session is detected (including the now invalid stored access token) and generates a failure during app installation.

@chadhamre
Copy link
Author

@esteban-filardi That's the one! I'm glad I'm not crazy. I found a bizarre way to work around it until such a time that the package is patched through a simple middleware that drops the session cookie under certain circumstances. Please let me know if you figure out a better solution.

server.use(async (ctx, next) => {
    if (ctx.request.header.cookie) {
      if (
        (ctx.request.url.split("?")[0] === "/" &&
          ctx.request.querystring.split("&") &&
          ctx.request.querystring.split("&")[0].split("=")[0] === "hmac" &&
          ctx.request.querystring.split("&")[1].split("=")[0] !== "locale") ||
        (ctx.request.url.split("?")[0] === "/auth/callback" &&
          ctx.request.querystring.split("&") &&
          ctx.request.querystring.split("&")[1].split("=")[0] === "hmac")
      ) {
        {
          console.log("DROP COOKIES", ctx.request.url);
          ctx.request.header.cookie = ctx.request.header.cookie
            .split(" ")
            .filter(
              item =>
                ["koa:sess", "koa:sess.sig"].indexOf(item.split("=")[0]) === -1
            )
            .join(" ");
        }
      }
    }
    await next();
  });

@Popesites
Copy link

Popesites commented Jun 11, 2019

I experienced this problem when building my app. I experienced an error when uninstalling and then installing the app.

Steps to reproduce:

  1. Download & configure starter files
  2. Install app on Shopify store using the Partners Admin > Apps > App_Name > App Actions > Install on development store
  3. Uninstall app within the development store
  4. Attempt step number two again

You will be redirected to the app page with proper authentication, as opposed to the app installation screen within the store's Shopify Admin.

I can confirm the above code @chadhamre provided solved the issue for me. There may be a more optimal approach to solve this, but I haven't currently looked into it.

@AKarmanov
Copy link

Same issue here, I can't believe this wasn't tested by package developers given that this (being able to uninstall and reinstall your app) is an official requirement.

@amardeepsingh20
Copy link

amardeepsingh20 commented Jul 25, 2019

Having the same issue. Is there any official update on this? Doesn't feel like this middleware has been tested well.

Also, I am having another issue with VerifyRequest() middleware.. If a merchant has 2 shops and he installs an app on both (logging in the same browser), then the second one does not go through auth() at all.. verifyRequest() lets in the second one without checking. I will be creating a new issue regarding this.

@katiedavis
Copy link
Contributor

Hi @chadhamre and everyone. Thanks for your patience. This issue should be resolved and was released yesterday. If you upgrade to 3.1.31 you should see the fix.

@Popesites
Copy link

@katiedavis did it fix the issue mentioned by @amardeep9911 as well?

@katiedavis
Copy link
Contributor

@Popesites I don't think this would cover that. That will need to be addressed in a separate task probably.

@Popesites
Copy link

Ok thanks. I'm going to try and come up with a workaround if it isn't addressed.

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