Support request for node12 template and Stripe middleware

[MrNiftyCoder]

Expected Behaviour

Using this template to authenticate a stripe webhook signature requires access to the rawBody of the request. This field isn't available in the function on the event parameter and should be available to the function, as some implementations may need this.

Current Behaviour

Currently only the parsed body is exposed to the function via the event parameter.

Possible Solution

I did some research and found that many users of express that use the body-parser package to clean up the body have had this issue, one of which is described here https://flaviocopes.com/express-get-raw-body/

There may be a better way to do this but I was able to get this to work. And since you're not accepting pull requests, I have forked this library and implemented two changes as seen below. https://github.com/MrNiftyCoder/templates/blob/master/template/node12/index.js

1. On line 12 of index.js in the root of this template I replaced app.use(bodyParser.json()); with:

   app.use(
   bodyParser.json({
   verify: (req, res, buf) => {
     req.rawBody = buf;
   }
   })
   );

2. In the constructor of the FunctionEvent class I added this.rawBody = req.rawBody

Steps to Reproduce (for bugs)

1. Implement a function based off of the node12 template
2. In the function handler.js attempt to access the event.rawBody
3. This doesn't exist on the event parameter.

Context

One of the benefits and my first attempt at using open functions for my application is to implement all my webhooks as functions. When implementing the stripe webhook I need to verify that stripe is the one sending the events to my function. This is made very easy by checking a signature in the header using a stripe method stripe.webhooks.constructEvent(request.body, sig, endpointSecret). The body passed to this method must be the "rawBody" in order for the signature to be verified. Details can be found here in the stripe documentation under the node tab: https://stripe.com/docs/webhooks/signatures

Your Environment

• Docker version docker version (e.g. Docker 17.0.05 ): latest

• Are you using Docker Swarm or Kubernetes (FaaS-netes)? kubernetes

• Operating System and version (e.g. Linux, Windows, MacOS): linux

• Link to your project or a code example to reproduce issue: Fixed fork (lines 12-18, 25): https://github.com/MrNiftyCoder/templates/blob/master/template/node12/index.js Stripe documentation: https://stripe.com/docs/webhooks/signatures

[alexellis]

Hi thanks for your interest,

Pull requests are accepted, but new and breaking templates are not accepted to this repository. Sorry for that confusion, please do read the README again and let me know if you think that needs to be explained differently.

The community templates are a starting-point and won't automatically suit everyone's needs. Forking the template and having your own is a perfectly reasonable course of action and expected.

I'll try to get to this and to see if there's another solution, or whether the suggested approach makes sense upstream. I assume this is to do with HMAC/request-signing?

Alex

[alexellis]

It may be that we actually need a mode to change the default parser from JSON to just raw.

Try commenting out:

https://github.com/openfaas/templates/blob/master/template/node12/index.js#L12

See what you get.

We could add something like an ENV_VAR to support it - i.e.

functions:
  x:
    image: y
    environment:
      JSON_PARSER: false

[alexellis]

/set title: Support request for node12 template and Stripe middleware

[burtonr]

With the suggestion to simply comment out the line...

// app.use(bodyParser.json());

...the resulting event.body is always: {}:

nifty-raw.1.g9kmq9izfj9m@pop-os    | 2020/01/29 02:28:04 stdout: Event Body: {}

However, using @MrNiftyCoder 's suggestion, both the body (previous functionality) and the buffer (raw body) are available to the function:

nifty-raw.1.k8b5q8pkvbm0@pop-os    | 2020/01/29 02:34:19 stdout: Event Body: { some: 'thing' }
nifty-raw.1.k8b5q8pkvbm0@pop-os    | 2020/01/29 02:34:19 stdout: Raw Event Body: <Buffer 7b 0a 09 22 73 6f 6d 65 22 3a 20 22 74 68 69 6e 67 22 0a 7d>

In order to get this functionality, I think we need to wrap the suggested code with an env var in order to limit it's effect. Reading up on the several issues in expressjs/body-parser, the team there are adamant about not doing this unless you absolutely need to. Double the ram usage per request and easier DoS attacks are two reasons that came up.

With that in mind, I can see the value here, but we need to ensure that the user (creator) of the function knows and wants that. Setting an environment variable could do the trick:

functions:
  striper:
    image: striper:latest
    environment:
      INCLUDE_RAW_BODY: true

Then, the code would look something like this:

if (process.env.INCLUDE_RAW_BODY === 'true') {
    app.use(
        bodyParser.json({
          verify: (req, _, buf) => {
            req.rawBody = buf;
          }
        })
      );
}
app.use(bodyParser.json());

An interesting note on boolean env vars I didn't realize until today on StackOverflow

@MrNiftyCoder would you like to submit a PR for this change? If so, please do read the Contribution Guide to avoid any troubles. Let us know if you have any questions, or if you'd rather someone else create the PR.

Thanks!

[alexellis]

@burtonr can you confirm the content type you were using in your example with curl? The whole statement. If Stripe doesn't need to use bodyRaw in their example code, what's making us need that?

[burtonr]

The content type was:

Content-Type: application/json

You can see in the Stripe documentation that they are using the following code in their example:

app.post('/webhook', bodyParser.raw({type: 'application/json'})

In other words, for every request to /webhook, if a request has the content type "application/json", it will send the body as the raw buffer.

We can't exactly do that for a specific endpoint in the template, but could make it available to a function if a variable was included. That would allow the user to say "Use the raw body for this function", but it would default to the existing method of parsing the JSON.

[MrNiftyCoder]

@burtonr, just to satisfy my curiosity, why would we not be able to apply the bodyParser.raw to specific endpoints in the template? After reading the comment here: https://github.com/expressjs/body-parser/issues/83#issuecomment-78139688 it seems that they don't recommend setting a global bodyParser and that it should be at the individual endpoint.

This is me just throwing out an idea, so it might be crap as I don't have a lot of experience with express and it's middleware. :) Is there a better way to structure the template so that for a given verb, i.e. post, we could apply or override the bodyParser used? Or alternatively, an environment variable switch to expose express and let the user have more control over how it's used.

https://github.com/openfaas/templates/blob/70eda00867c2edc8cf4bc7bf90c2efa62e4ce2e2/template/node12/index.js#L95

[burtonr]

What I meant by "we can't do that for a specific endpoint" is that we couldn't add a specific route to the base template. The difference is between an individual function and the template (from which all functions are built from).

You're right, express say "don't do this globally", but that's for a full featured long running persistent web api. Here, we're dealing with functions, or micro-apis that can be scaled to 0 and up.

That's also part of the reason that if this does become part of the template code, we need to make sure this feature is an "opt-in" functionality so that the user knows what they are doing. Adding additional ram per request. Probably not a big deal for a single function to handle a specific function that could then pass that request to the downstream functions/application.

We don't really want to expose the express code to the function so that the function can focus on the functional code, and not the boilerplate of setting up routes, middleware, etc. The idea is that the creator of the function should be able to simply add some value-add code without having to worry about any of the "how" (this is the "serverless" development experience)

[MrNiftyCoder]

Ok. Thank you for clarifying. I'll try to get a pull request in sometime soon.

[alexellis]

I would like to see if there's a way we can configure the template to act the same way as it did for the Stripe post, where they simply have:

const app = require('express')();
// Use body-parser to retrieve the raw body as a buffer
const bodyParser = require('body-parser');

All they appear to do is access req.body, the implication is that we are installing a middleware which is automatically converting from a buffer to a JSON object, we should be able to suppress that and then offer it as a configuration value via the stack YAML file and an env-var.

It seems like we could optionally enable the raw parser in the template's entrypoint code i.e. something like this:

if(process.env.RAW_BODY) {
  app.post('/*', middleware, bodyParser.raw({type: 'application/json'})
} else {
  app.post('/*', middleware)
}

[MrNiftyCoder]

I'm not going to have time to do a pull request after all, sorry.

[alexellis]

Don't worry, I've discussed it with Burton and we are wanting to own this in the community.