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

Documentation and example of webhook signature check (through rawBody) #5491

Open
2 tasks done
oom- opened this issue May 31, 2024 · 7 comments
Open
2 tasks done

Documentation and example of webhook signature check (through rawBody) #5491

oom- opened this issue May 31, 2024 · 7 comments
Labels
feature request New feature to be added

Comments

@oom-
Copy link

oom- commented May 31, 2024

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

I'm currently aiming to verify the signature of a webhook route using the raw body data.

There is already a lot of issues around the subject since 2018:

There is still no clear documentation regarding signature verification or how to obtain a raw body inside the route handler to perform webhook signature check.

I think that the best option would still just to have a rawBody: true option on the route as @mcollina suggested here:
issuecomment-619153284

This way, the raw body would appear only on selected routes to avoid unnecessary memory consumption.

Motivation

Enhance Fastify's usability and create a more welcoming environment.

Example

Payload:

{
   "msg" : "test"
}
fastify.post("/example", { rawBody: true }, (request, reply) => {
   console.log(request.rawBody); // "{\n   \"msg\" : \"text\"\n}"
   console.log(request.body); // { msg: 'text' }
});

In my project I'm loading dynamically all routes from folders and subfolders, for an unknown reason fastify-raw-body wasn't working for me.

There is the implementation I made to address my issue for now (added a preParsing hook to the route and using raw-body library to read the stream).

//Add rawBody to Webhooksroutes
routeObj.opt.preParsing = (request: any, reply, payload, done) => {
    getRawBody(payload, { length: null, limit: request.routeOptions.bodyLimit, encoding: "utf8" }, (err, str) => {
        if (!err) { request.rawBody = str; }
    });
    done(null, payload); // `done` not called in the callback of `getRawBody` to let the original `contentTypeParse` hook `payload.on('data')` before `getRawBody` starts to read the stream
};
@metcoder95
Copy link
Member

I believe it can enable a good set of use cases the fact that we allow to the parser being skipped on certain routes based on a flag.

+1 on this.

Although here I'd like to see what are we interested the most, the intention of skipping the parser or the intention of just have the raw-body. I'd personally see more useful the former as it enables more use cases (e.g. pipelining between body reading and signature verification).

@metcoder95 metcoder95 added the feature request New feature to be added label Jun 6, 2024
@xr0master
Copy link

xr0master commented Sep 2, 2024

I noticed that guys are working on version 5 and planning changes to data parsing. This might be a good opportunity to add a body to the FastifyRequest object's raw property. In my opinion, it may be under raw. I also believe that this will not require a lot of code.

I am aware of the fastify-raw-body npm package, but I feel it does not align with the current Fastify philosophy. In the end, this package was written many years ago.

P.S. IMO, you should update the title as it does not match the future request.

@mcollina
Copy link
Member

mcollina commented Sep 2, 2024

Adding req.body would create significant performance issues unfortunately.

@xr0master
Copy link

@mcollina definitely, but that's why it should be disabled by default, then there shouldn't be any difference in performance.

On the other hand, adding a preParsing hook for a specific router only requires a few lines of code, and maybe that's more than enough — something like the code below.

preParsing: (request, reply, payload, done) => {
  let data = '';

  payload.on('data', (chunk: string) => {
    data += chunk;
  });

  payload.on('end', () => {
    request.raw.body = data;
  });

  done(null, payload);
},

@metcoder95
Copy link
Member

It looks like a plugin, and the fastify-raw-body plugin already does that; why should be added to core if it can be a plugin?

@xr0master
Copy link

xr0master commented Sep 3, 2024

Plugins typically add new features rather than restoring initial behavior. It often comes down to deciding what belongs in the core versus what should be handled by plugins. However, I agree with you — it makes sense for this to be a plugin. It would be ideal if the plugin were maintained by the Fastify team under the @fastify scope.

The fastify-raw-body brings too many extra packages for a straightforward thing, IMO.

@gurgunday
Copy link
Member

  fastify.addContentTypeParser(
    "application/json",
    { parseAs: "buffer" },
    (request, payload, done) => {
      done(null, payload);
    },
  );

For things like Stripe, adding this parser gives you the raw request.body for your desired contentType

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature to be added
Projects
None yet
Development

No branches or pull requests

5 participants