fix: check for valid content-length before calling request.formData() for HEAD requests

[MarcMogdanz]

WHY are these changes introduced?

Currently the login handler will call request.formData() if no ?shop=xxx param was found and it's a GET request. In Remix HEAD requests will be handled as GET requests, so if no shop param was provided in a HEAD request, e.g. by just visiting /auth/login (using the shopify remix template), it would then call the request.formData() function on an empty body and throw a TypeError: Could not parse content as FormData.

WHAT is this pull request doing?

The PR enhances the current if (request.method === 'GET' && !shopParam) check to also check if the Content-Length header is truthy/above zero. If so, it should be safe to call request.formData(). It does not impact any other request types such as POST.

Type of change

• Patch: Bug (non-breaking change which fixes an issue)
• Minor: New feature (non-breaking change which adds functionality)
• Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• I have used yarn changeset to create a draft changelog entry (do NOT update the CHANGELOG.md files manually)
• I have added/updated tests for this change
• I have documented new APIs/updated the documentation for modified APIs (for public APIs)

[MarcMogdanz]

I have signed the CLA!

[lizkenyon]

Hi there @MarcMogdanz, and sorry for the slow response! 👋

I think we are generally good with this! I tested this out in an app, and seems to be functioning as expected
I was having trouble reproducing the type error you were seeing. Could you provide the steps for reproduction for getting your original error?

[MarcMogdanz]

Hey @lizkenyon,
Sorry for the late reply!

I've created a sample repo (which basically is just the basic Shopify Remix template): https://github.com/MarcMogdanz/shopify-app-js-pull-662

The following are all of the relevant requests, the easiest way to replicate it is to use an API client.

1. GET /auth/login
   This will render the login form as expected.

2. GET /auth/login?shop=xxx.myshopify.com
   This will skip the login form and automatically redirect to authenticate via Shopify.

3. HEAD /auth/login
   This request is causing the issue. There's no ?shop=xxx param which means it can't automatically redirect. Since Remix treats HEAD requests as GET requests it will still invoke the loader function in /app/routes/auth.login/route.tsx but request.method will be set to HEAD and not GET. In the current version of the shopify-app-js package it will then assume/fallback to handling the request as if it is a POST request triggered by submitting the login form rendered through request 1 since the if (request.method === 'GET' && !shopParam) check doesn't go trough.

4. HEAD /auth/login?shop=xxx.myshopify.com
   Will be treated the same as 2. and work without issues.

Now thinking about it again, a potentially simpler fix for this would be to only change the check to also check for HEAD and change it up to something like this:

if ((request.method === 'GET' || request.method === 'HEAD') && !shopParam) {
  return {}
}

Should still do the trick and require a lot less changes to the test suite.

[lizkenyon]

Hey @MarcMogdanz

Let go with your new suggested fix!

Now thinking about it again, a potentially simpler fix for this would be to only change the check to also check for HEAD and change it up to something like this:

if ((request.method === 'GET' || request.method === 'HEAD') && !shopParam) {
  return {}
}