[BUG]: Express middleware aborts on non-POST requests in v11

[AaronDewes]

What happened?

After migrating to v11, non-post requests on the webhooksPath lead to an error. For v10 and below, next() was called instead.

This causes issues with Probot, which listens on / for webhooks, but also displays a web page on GET requests.

I'm not sure if this is intentional, but if it is, then please make this behaviour configurable.

Versions

@octokit/webhooks v11

Relevant log output

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[wolfy1339]

It's been a while since I have used Probot, but I was under the impression that the other HTTP paths and Webhooks paths were different.

It is intentional that we don't handle POST requests here, however I agree it shouldn't stop further Express routers/middleware from being run

That change was introduced in #740

[wolfy1339]

@baoshan Could you help us understand why the change was made?

[baoshan]

Because:

To respect a proposed general rule that all Octokit middlewares (oauth-app.js, webhooks.js, app.js):

1. do NOT handle requests outside pathPrefix (or path) option but provide a natural way for concrete runtimes/frameworks to handle unhandled requests
2. always handle requests within pathPrefix (or path) option (returns 404 for unknown routes)

These rules make such middleware more predictable and simplify the implementation.

It seems Probot installs webhooks.js at / but does not want webhook.js to handle GET /. So the second proposed rule does not apply anymore.

Is there some room in Probot to enable webhooks.js to handle all requests within its configured .path?

I am flexible and am willing to loose the second rule from webhooks.js (but not oauth-app.js or app.js for now).

[AaronDewes]

Is there some room in Probot to enable webhooks.js to handle all requests within its configured .path?

It would require a breaking change in Probot, or removing the default page. The default page gave users some information about how to proceed, so I would not like to remove it. If the /path changed, every developer would have to go to GitHub to change their settings when updating probot, which would also be annoying.

[baoshan]

I don’t use Probot. If it is an express.js app, can the handler for the default page be placed before webhooks.js (in the chain of middlewares)?

[wolfy1339]

It would require a breaking change in Probot, or removing the default page. The default page gave users some information about how to proceed, so I would not like to remove it. If the / path changed, every developer would have to go to GitHub to change their settings when updating probot, which would also be annoying.

Yes it might be annoying, but it's the case with any breaking change.

If it is an express.js app, can the handler for the default page be placed before webhooks.js (in the chain of middlewares)?

There's a middleware exported, like in this repo, and an express.js app. Depending on how one uses probot they may use the middleware or the express.js app

[baoshan]

There's a middleware exported, like in this repo, and an express.js app. Depending on how one uses probot they may use the middleware or the express.js app

Can both the express.js app and the middleware render the default page before delegating a request to webhook.js?

[AaronDewes]

Probot currently also allow users to overwrite /, so either that behaviour needs to go or the webhook path needs to move, or octokit needs to change this behaviour (I don't want to sound demanding here, it's just the 3 options we have, and of course, how your API works is your decision).

I'm not sure whether @gr2m is still maintaining Octokit (He's not a member of the org), but I'd also like to get his feedback on this.

In my opinion, moving the webhooks path would be the best option for Probot long-term, even if it is annoying.

[baoshan]

it's just the 3 options we have

I still think there may be a 4th one: Probot renders the default page (GET /) before passing the request to webhook.js.

It may depend on how Probot allows users to overwrite /.

Please correct me if that’s impossible.

[AaronDewes]

It allows it by giving access to express directly, which always happens after the middleware is loaded. Maybe this can be changed though, I'll experiment with it a bit.

Thanks for the suggestions!

[gr2m]

In my opinion, moving the webhooks path would be the best option for Probot long-term, even if it is annoying.

In general I agree that we should move the path to receive webhooks in Probot from POST / to POST /api/github/webhooks. Basically to conform what Octokit does: https://github.com/octokit/octokit.js#app-server

[AaronDewes]

Having it on / could also work if we consider probot/probot#1576. So considering this is intended behaviour, I will close this issue and maybe we can continue talking about this in the corresponding probot PR.