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

[Bug]: Empty Response with Content-Type: application/json Throws Error #11145

Closed
SpencerDuball opened this issue Dec 27, 2023 · 6 comments · Fixed by #11164
Closed

[Bug]: Empty Response with Content-Type: application/json Throws Error #11145

SpencerDuball opened this issue Dec 27, 2023 · 6 comments · Fixed by #11164
Labels

Comments

@SpencerDuball
Copy link

What version of React Router are you using?

@remix-run+router@1.14.1

Steps to Reproduce

This is the _index.tsx page of a default Remix app. Clicking the button if running locally will trigger this issue. I have also included an example of the Link component because the issue appears to exist in the same place on different hosting provider such as AWS CloudFront. In AWS CloudFront it appears that empty requests are sent back as Content-Type: application/json which includes client-side navigation with Link components.

import { Link, useFetcher } from "@remix-run/react";
import { type ActionFunctionArgs } from "@remix-run/node";

export async function action({ request }: ActionFunctionArgs) {
  const body = await request.formData();

  if (request.method === "POST") {
    return new Response(null, { headers: [["Content-Type", "application/json"]] });
  }

  return new Response(null, { status: 405, statusText: "Method Not Allowed" });
}

export default function Index() {
  const action = useFetcher();

  return (
    <div>
      {/* Link Issue Parsing Empty Response
          ---------------------------------
          When using a Link component, if the response is an empty response from the server with
          Content-Type: application/json an error will be thrown that the json could not parse correctly.
          NOTE: The Link example will not reproduce locally because the Content-Type header will not be set to 
          application/json, but on other providers such as AWS CloudFront, empty responses have application/json set
          by default! */}
      <li>
        <Link to="/non-existing-route">Will Cause Error</Link>
      </li>
      {/* Fetcher Issue Parsing Empty Response
          ------------------------------------
          When using a fetcher, if an empty response is sent from the server with Content-Type: application/json
          an error will be thrown that the json could not parse correctly. */}
      <action.Form method="post">
        <button type="submit">Fetcher Issue</button>
      </action.Form>
    </div>
  );
}

Expected Behavior

Issue Location
The issue comes from this line in the @remix-run/router package:

data = await result.json();

Proposed Solution
In this section, we are checking if the Content-Type: application/json exists, and then parsing for a JSON response. However, if the response body is null an error will be thrown. I believe adding a check for Content-Length: 0 before attempting to parse for JSON would be good.

Reason For Solution
I have looked in RFC#7231 and RFC#2616 and the way this package is parsing for the Content-Type: application/json certainly is correct - however a bit of a blindspot in the RFCs is how to deal appropriately with Empty responses:

  • Some guidance is to not have a Body and not include a Content-Type header
  • Some guidance is to check for the Content-Length: 0 header value.
  • Some guidance is to only use the Content-Type value.

The RFCs also indicate that the Content-Type is an optional field. It SHOULD be sent but is not required as the client may attempt to parse the payload format on it's own. In this issue I have highlighted where I am recieving an issue from AWS CloudFront which seems to be relying on checking the Content-Length: 0 header.

For these reasons, I think it would be a good improvement to check the Content-Length: 0 even though the current implementation is valid.

Actual Behavior

Currently there is no check for an empty body before attempting to parse for JSON. Right now if the Content-Type: application/json header is sent then React Router/Remix will parse for JSON.

@brophdawg11
Copy link
Contributor

Thanks for the detailed writeup @SpencerDuball! Just to clarify a couple things:

  • What exactly are you returning from your loader to trigger this issue in AWS? Is it simply return null?
  • When you get one of these empty responses from AWS - does it have Content-Length: 0 in it?

I think it's reasonable for us to specifically look for Content-Length: 0 and return null as the data without calling response.json().

@SpencerDuball
Copy link
Author

SpencerDuball commented Jan 5, 2024

Thanks for the detailed writeup @SpencerDuball! Just to clarify a couple things:

  • What exactly are you returning from your loader to trigger this issue in AWS? Is it simply return null?
  • When you get one of these empty responses from AWS - does it have Content-Length: 0 in it?

I think it's reasonable for us to specifically look for Content-Length: 0 and return null as the data without calling response.json().

  • If I return new Response(null | undefined, { status: 404 }); I will get this error.
  • Yes it does return a Content-Length: 0. I actually had an example hosted on CloudFront for this issue but thought it might be overkill to include … I should have kept it up and put the link here as I can see response headers being helpful, whoops 😅

Also a side note, just parsing for req.body() will not throw an error I think we only need a check if trying to parse json. So returning null right away is perfect or just falling back to parsing body would work from my testing.

I will put this site back up to demonstrate the error in an actual environment later today, just got to office and don’t have my personal computer to deploy this one quick. Thanks for having a look at this issue!

@brophdawg11
Copy link
Contributor

ok yeah I was originally worried about double-reading issues if using body but if we only check for null then we shouldn't be consuming the stream

@brophdawg11
Copy link
Contributor

This is resolved by #11164 and should be available in the next release

@brophdawg11 brophdawg11 added the awaiting release This issue have been fixed and will be released soon label Jan 9, 2024
Copy link
Contributor

github-actions bot commented Jan 9, 2024

🤖 Hello there,

We just published version 6.21.2-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 6.21.2 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@brophdawg11 brophdawg11 removed the awaiting release This issue have been fixed and will be released soon label Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants