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

Issues with body parser. #4654

Closed
2 tasks done
withmask opened this issue Jul 23, 2021 · 2 comments
Closed
2 tasks done

Issues with body parser. #4654

withmask opened this issue Jul 23, 2021 · 2 comments

Comments

@withmask
Copy link

withmask commented Jul 23, 2021

Considering the following setup:

  app.use(
    helmet(),
    (req, res, next) => {
      console.log(req.headers);
      if (req.header('Content-Type')?.toLowerCase() != 'application/json')
        return res
          .status(415)
          .json({ status: false, message: 'Unsupported Media Type' });

      next();
    },
    express.json({ limit: '10b' }),
    //@ts-expect-error
    (error, req: express.Request, res: express.Response, next) => {
      console.log(res.destroyed);
      console.error('Failed parsing body', error);
      res.status(400).json({ status: false, message: 'Invalid body provided' });
    }
  )

This snippet works as intended when the Content-Length header matches the Length of the actual body. However if the client gives false data, express acts with weird behavior.

  • When the Content-Length is shorter than the body size, res.status(400).json({ status: false, message: 'Invalid body provided' }) Only returns an empty body and a 400 status.
    image

    Even when Content-Length is not equal to zero, the same behavior happens
    image

    And obviously the middle-ware is actually executed and i indeed receive logs of 'Failed parsing body',
    image

  • When the Content-Length is bigger than the body size, express appears to await forever and never closes the request.

    image

This problem have been fixed by using the following snippet

req.setTimeout(1000 * 60, () => {
        if (res.headersSent) return;
        console.log('Request timed out');
        res
          .status(408)
          .json({
            status: false,
            message: 'Request timeout'
          })
          .end();
      });

Note that i added if (res.headersSent) return; Because this is triggered twice for some reason.

@dougwilson
Copy link
Contributor

Yes, these behaviors are expected. It is how any body reading module would work, as the content-lenght header is actually part of the framing in http. When you make it longer than the body, Node.js (the http module from Node.js is what controls this) will never emit the 'end' event, instead it will wait until the server timeout that is set to receive the nody. When you make it shorter, extra bytes are recieved after the http body, which is a framing error and Node.js itself will close the http conecton (which is why you never can send a response); the response sent is what is defined on your http server (outside of express).

@dougwilson
Copy link
Contributor

As for why your callback to req.setTimeout is involed twice for the same request, that is strange. That API is direct to Node.js and the core code, which is what triggers your callback. It may be a bug in Node.js, as I don't think it is expected to be called multiple times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants