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] incorrect behavior of RawBodyParams #1819

Closed
4 tasks
derevnjuk opened this issue Mar 26, 2022 · 4 comments
Closed
4 tasks

[BUG] incorrect behavior of RawBodyParams #1819

derevnjuk opened this issue Mar 26, 2022 · 4 comments

Comments

@derevnjuk
Copy link
Contributor

Information

  • Version:: 6.x
  • Packages: platform-express

In implementing HTTP signature verification (e.g. Stripe or GitHub), it might be useful to get the raw body. Currently, it is almost impossible to use RawBodyParams since it has limitations by content type and length.

RawBodyParams returns a string instead of an instance of Buffer

In docs stated that RawBodyParams returns the value from request.body as a Buffer (see https://api-docs.tsed.io/api/platform/platform-params/types/decorators/RawBodyParams.html).

But it returns a string since chunk is implicitly casting to string due to using a string concatenation in rawBodyMiddleware:

request.rawBody = "";
request.on("data", (chunk) => {
  request.rawBody += chunk;
});

Moreover, rawBodyMiddleware doesn't care about a charset at the same time.

Expecting data events to be synchronously

The middleware may not run well until the reading is finished. That means, rawBody may be undefined in some cases when consuming data takes more time than usual:

request.rawBody = "";
request.on("data", (chunk) => {
  request.rawBody += chunk;
});
next();

To fix that, you have to rewrite the middleware as follows:

request.rawBody = "";
request.on("data", (chunk) => {
  request.rawBody += chunk;
});
request.on("end", (chunk) => {
  next();
});

There is also another problem since the stream is switched in flowing mode by subscribing to the data event, it may lead to some race between different body parsers. For instance, you can use $afterInit to register express.json() middleware that consumes all data from the request preventing catching rawBody:

const parseBody = (req, res, next) => {
  const chunks = [];

  if (req._readableState.ended) {
    console.warn('cannot obtain raw body since stream is ended')
    return next()
  }

  req.on('data', data => {
    chunks.push(data);
  });

  req.on('end', () => {
    req.rawBody = Buffer.concat(chunks);
    next();
  });
};

app.post('/', express.json(), parseBody, (req, res) => {
  console.log(req.body);
  console.log(req.rawBody);

  res.send('Hello World!')
})

Since the readable stream is ended, the previous example will be stuck, and the current implementation will just set an empty string to req.rawBody.

Acceptance criteria

  • User should be able to get rawBody independently of using other body parsers
  • User should be able to override a middleware to parse a raw body
  • RawBodyParams should return a Buffer instead of a string by default
  • User should be able to configure RawBodyParams to obtain a string according to charset
@Romakita
Copy link
Collaborator

Hello @derevnjuk

perfect issue description ;)

User should be able to get rawBody independently of using other body parsers
is no really possible. When BodyParser consume the request data isn’t possible after to use the rawBodyMiddleware. And if the rawBodyMiddleware consume the req.data it the same thing.

I’m not sure how to fix that correctly. If you have a solution or an idea, let’s go ;)

See you
Romain

Romakita added a commit that referenced this issue Apr 10, 2022
Now bodyParser is embed directly in the core and can be configured.

Closes: #1819
@Romakita
Copy link
Collaborator

Romakita commented Apr 10, 2022

Hello @derevnjuk
I worked on the bodyParser issue. Finally, The only correct way to manage that is to add bodyParser for each endpoint depending on the collected metadata.

it doesn’t fix issue if the server add a body parser during the $beforeRoutesInit. But if the body parser is removed, it works as expected.

There is no better option ^^.

The pr is almost ready. I need to update the documention.

see you
Romain

Romakita added a commit that referenced this issue Apr 11, 2022
Now bodyParser is embed directly in the core and can be configured.

Closes: #1819
@Romakita
Copy link
Collaborator

🎉 This issue has been resolved in version 6.111.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Romakita
Copy link
Collaborator

🎉 This issue has been resolved in version 7.0.0-beta.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

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