octokit / octokit.js

The all-batteries-included GitHub SDK for Browsers, Node.js, and Deno.
MIT License
7.06k stars 1.03k forks source link

createNodeMiddleware(app) requires oauth.clientId and oauth.clientSecret #2211

Open stergion opened 2 years ago

stergion commented 2 years ago

Please avoid duplicates

Reproducible test case

import { App, createNodeMiddleware } from "octokit";
import "dotenv/config";
import express from "express";

const ghApp = new App({
  appId: process.env.GITHUB_APP_ID,
  privateKey: process.env.GITHUB_PRIVATE_KEY,
  webhooks: {
    secret: process.env.GITHUB_WEBHOOK_SECRET,
  },
});

const app = express();
app.use(createNodeMiddleware(ghApp));
app.listen(3000);

Please select the environment(s) that are relevant to your bug report

Version

octokit@1.7.1

What happened?

I copied the code from the octokit.js readme.md wanting to test github webhooks. In the example code oauth.clientId and oauth.clientSecret are not set. Also in the octokit/app.js Constructor oauth.clientId and oauth.clientSecret are not required.

When I try to run my code, I get the error: throw new Error("[@octokit/app] oauth.clientId / oauth.clientSecret options are not set");

I have to say though, in octokit/app.js Usage oauth.clientId, oauth.clientSecret are provided in the code example.

Is this a documentation error or a code bug?

Would you be interested in contributing a fix?

RUGMJ commented 1 year ago

Was a fix for this found? I'm having the same error now

stergion commented 1 year ago

I just added the oauth: { clientId: null!, clientSecret: null! }, in the App options:


const ghApp = new App({
  appId: process.env.GITHUB_APP_ID,
  privateKey: process.env.GITHUB_PRIVATE_KEY,
  webhooks: {
    secret: process.env.GITHUB_WEBHOOK_SECRET,
  },
  oauth: { clientId: null!, clientSecret: null! },
});
gr2m commented 1 year ago

I agree that both the webhooks config and the oauth config should be optional. I think I tried that when implementing this initially, I think the types just got so complicated that I decided not to, at least not initially.

You should be able to just set webhooks.secret or oauth.{clientId,clientSecret} to empty strings: ""

didley commented 7 months ago

I'm not sure exactly what the difference is but there's another import of createNodeMiddleware that doesn't require oauth to be passed. It's used in these docs and this example repo.

import { createNodeMiddleware } from '@octokit/webhooks';

I was unable to get the octokit import to work and was getting 504 responses. But was successful with the @octokit/webhooks import.

gr2m commented 7 months ago

@octokit/webhooks is the underlying library for only webhooks: https://github.com/octokit/webhooks.js. That's why it does not require OAuth settings. It also doesn't set an octokit instance on the context of event handlers

corsin-ragettli commented 6 days ago

I agree that both the webhooks config and the oauth config should be optional. I think I tried that when implementing this initially, I think the types just got so complicated that I decided not to, at least not initially.

You should be able to just set webhooks.secret or oauth.{clientId,clientSecret} to empty strings: ""

When I set secret to "", I get the following error:

Error: [@octokit/webhooks] options.secret required

If I set the secret, I get the following error when calling the webhook:

{"error":"Error: [@octokit/webhooks] signature does not match event payload and secret"}

So there is currently no way for me to call a webhook, Im using GHES 3.13.5

gr2m commented 5 days ago

If you receive webhooks, you must set a webhook secret in your GitHub App configuration and configure your code accordingly. It's security best practice and Octokit enforces it. We only discuss not to set any webhooks options in the App constructor for use cases where webhooks are not used at all.

corsin-ragettli commented 4 days ago

If you receive webhooks, you must set a webhook secret in your GitHub App configuration and configure your code accordingly. It's security best practice and Octokit enforces it. We only discuss not to set any webhooks options in the App constructor for use cases where webhooks are not used at all.

I understand and agree that it should be best practice, but I cannot have webhook secrets because of the error I get (see https://github.com/octokit/octokit.js/issues/2211#issuecomment-2498069389). Was there a breaking change which is not supported by GHES 3.13.5 regarding webhook secrets?

wolfy1339 commented 4 days ago

Please open up a new issue with all relevant details and we can help out to see why this is happening.