Skip CSRF check if a valid JWT is passed in

[theundeadmonk]

With 3p cookies being blocked, Rails will no longer be able to set CSRF tokens. This will result in requests failing with an invalid CSRF token error.
When using JWT based auth, a valid signed JWT is as secure than a CSRF token, since we trust that Shopify is the only source that can sign it securely.

This PR introduces a new concern that when included, skips the CSRF check if we are able to successfully extract the shop domain from the JWT.

[theundeadmonk]

Results of 🎩
With 3p Cookies blocked and old version of Shopify App

With 3p Cookies blocked and this version of Shopify App

[ragalie]

LGTM but would be nice to figure out how to write a test around it.

[greatestape]

Could we name this something that describes what it's telling us? like session_token_auth? or valid_session_token_header? or something?

[CautionTapeBot]

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.

[theundeadmonk]

@ragalie Unfortunately, there's no easy way to simulate the whole CSRF test flow.

[ragalie]

Another thought: I think we're explicitly trying to get away from requiring people to subclass AuthenticatedController (in favour of being able to include the module).

Can we move this into the module?

[theundeadmonk]

@ragalie Sure we can do that.

[ragalie]

Since you're using ActiveSupport::Concern, you can just include LoginProtection here. You don't need to do the ancestors check.

See https://api.rubyonrails.org/classes/ActiveSupport/Concern.html

[theundeadmonk]

I'm not sure that's a good idea. CsrfProtection should ideally have nothing to do with LoginProtection. They serve different purposes. If you want, I can have CsrfProtection access the jwt_shopify_domain directly from the request environment.

[ragalie]

I think the middleware exposes that interface right now, so it doesn't bother me if you access the env directly.

My feedback is that under the current approach you have a dependency on LoginProtection, and it's better to use the tools that ActiveSupport::Concern gives you to enforce that dependency than to build your own thing.

[greatestape]

Would it make sense to do something more duck-typing-esque? Like, put some defensive code around the reference to jwt_shopify_domain to raise a meaningful error if it's not present?

[ragalie]

I don't think so, personally. IMO the interface is the keys added to the env. We can either access that interface directly or we can pull in a method we know accesses the interface correctly.

The jwt_shopify_domain method is not the interface and testing that a jwt_shopify_domain method of unknown origin is present is the wrong approach IMO.

[theundeadmonk]

The thing is we don't know if the app is using JWT or cookie based auth (Especially due to the fallbacks where it can use either). So we default to the standard Rails CSRF error.

[greatestape]

Nice tests! Good stuff!

[theundeadmonk]

🎩 On Brave

[dianedouglas-thrive]

I'm getting an invalid CSRF token on a form submission in an embedded, multi-page app, using jwt authentication and turbolinks. I figured this PR would have fixed it but no luck.

When checking for request.env['jwt.shopify_domain'] on the form post request I am getting nil. How could this be?

Authentication seems to work fine otherwise. This is a separate controller from my home controller doing basic CRUD stuff. I'm inheriting from ApplicationController, I'm including ShopifyApp::EmbeddedApp. Is there something else I'm missing there?

I'm on the most recent version of the shopify_app engine, I've followed the documentation, searched the community, and looked carefully through the jwt example app. I'm at a loss for what I could be doing wrong. Any help would be very appreciated.

@theundeadmonk Figured I'd tag you since you seem to be very knowledgable on this subject. Thanks so much for your time!

Edit: Forgot to mention the invalid CSRF token error is only happening in incognito mode, but this is how the app reviewer is testing my app, so I have to make it work there.

[rezaansyed]

I'm getting an invalid CSRF token on a form submission in an embedded, multi-page app, using jwt authentication and turbolinks. I figured this PR would have fixed it but no luck.

When checking for request.env['jwt.shopify_domain'] on the form post request I am getting nil. How could this be?

Authentication seems to work fine otherwise. This is a separate controller from my home controller doing basic CRUD stuff. I'm inheriting from ApplicationController, I'm including ShopifyApp::EmbeddedApp. Is there something else I'm missing there?

I'm on the most recent version of the shopify_app engine, I've followed the documentation, searched the community, and looked carefully through the jwt example app. I'm at a loss for what I could be doing wrong. Any help would be very appreciated.

@theundeadmonk Figured I'd tag you since you seem to be very knowledgable on this subject. Thanks so much for your time!

Edit: Forgot to mention the invalid CSRF token error is only happening in incognito mode, but this is how the app reviewer is testing my app, so I have to make it work there.

Hi @dianedouglas-thrive. The separate controller mentioned should include the ShopifyApp::Authenticated module. This should handle skipping the CSRF checks when the request from the app frontend has a valid JWT session token. In order to make requests from the frontend to the app's backend, the frontend JS needs to have AppBridge instantiated and make requests with the session token.

If you have more questions or info, feel free to log a GitHub issue and we can keep track of it there.

[dianedouglas-thrive]

Hi @rezaansyed, thanks so much for the info! I'll give that a try and if I have any further problems I'll make a separate github issue. Once I get this figured out I can also try to make a PR to help with the documentation on this issue.

2 quick questions if you wouldn't mind clarifying please:

1. Should I be saying include ShopifyApp::Authenticated on the controller, or should I be inheriting from the AuthenticatedController?
2. You said "the frontend JS needs to have AppBridge instantiated and make requests with the session token" - could you point me to an example of this please to make sure I'm doing it correctly if there is one in the example app or docs somewhere?

Thanks again for your time.

[dianedouglas-thrive]

Hi @rezaansyed - sorry to bug you again but I'm still having issues. I can open a separate issue if you like, but wanted to make sure I'm not missing some small step.

If I include ShopifyApp::Authenticated or if I inherit from AuthenticatedController I get redirected a few times and then returned to the apps page with the error:

The app couldn’t be loaded. This app can’t load due to an issue with browser cookies. Try enabling cookies in your browser, switching to another browser, or contacting the developer to get support.

This happens on any controller action, regardless of incognito mode or not.

I also tried to instantiate the AppBridge using this code:

<script>
    document.addEventListener("DOMContentLoaded", async function() {
      var SessionToken = window["app-bridge"].actions.SessionToken
      var app = window.app;

      app.dispatch(
        SessionToken.request(),
      );

      // Save a session token for future requests
      window.sessionToken = await new Promise((resolve) => {
        app.subscribe(SessionToken.ActionType.RESPOND, (data) => {
          resolve(data.sessionToken || "");
        });
      });
    });
  </script>

Is this correct? Do I specifically need to add a hidden field or something to include window.sessionToken in my form submission? I feel like I'm missing something because whether I'm in an incognito window or not request.env['jwt.shopify_domain'] is always nil.

[rezaansyed]

Hi @rezaansyed, thanks so much for the info! I'll give that a try and if I have any further problems I'll make a separate github issue. Once I get this figured out I can also try to make a PR to help with the documentation on this issue.

2 quick questions if you wouldn't mind clarifying please:

1. Should I be saying include ShopifyApp::Authenticated on the controller, or should I be inheriting from the AuthenticatedController?
2. You said "the frontend JS needs to have AppBridge instantiated and make requests with the session token" - could you point me to an example of this please to make sure I'm doing it correctly if there is one in the example app or docs somewhere?

Thanks again for your time.

@dianedouglas-thrive no problem! To answer your questions:

1. You should be using include ShopifyApp::Authenticated module. If you have an AuthenticatedController in the context of your app, you can include it in there and inherit it in the appropriate controller.
2. I recommend going through the Rails + Turbolinks tutorial for building embedded apps with session tokens

[rezaansyed]

@dianedouglas-thrive can you open a new issue for this. It will be easier to track and support the problems you're running into.

[dianedouglas-thrive]

Will do, thanks!

[dianedouglas-thrive]

@rezaansyed added the issue here: #1219

Also, I looked at the tutorial you mentioned above, I'm not using a splash page but since I used the default shopify_app generator as I understand it I should be using the jwt implementation correctly. However, I think there are two things going on here - one is that I am probably missing something in my controller/views meaning that my jwt is getting lost, but there might also be some incognito mode behavior that might actually be a bug that needs to be addressed.

Anyway we can chat more in the other issue. Thanks again!