[BUG]: dependabot_alert events fail to verify

[timomeh]

What happened?

Observed Behavior

I noticed that webhooks.verify() returns false for dependabot_alert events, but only for this event type. All other events verify successfully.

To reproduce, I created a small script:

import { Webhooks } from '@octokit/webhooks'

const webhooks = new Webhooks({ secret: 'redacted' })

const check_run_event = {
  sign: "sha256=redacted", // from the X-Hub-Signature-256 header of the check_run event
  data: { /* copied a check_run event payload from the App Settings */ }
}
const dependabot_alert_event = {
  sign: "sha256=redacted", // from the X-Hub-Signature-256 header of the dependabot_alert event
  data: { /* copied a dependabot_alert event payload from the App Settings */ }
}

await webhooks.verify(dependabot_alert_event.data, dependabot_alert_event.sign)
// -> false

await webhooks.verify(check_run_event.data, check_run_event.sign)
// -> true

Expected Behavior

Given that other events verify successfully, I would've expected that the dependabot_alert event also verifies successfully.

Note

I'm unsure if this is a bug of @octokit/webhooks, or maybe even a miscalculation of the X-Hub-Signature-256 header coming from GitHub-Hookshot. In case it really seems to be an issue of Github-Hookshot, where can I report this?

I'm aware that the dependabot_alert event is still in beta.

Versions

@octokit/webhooks v10.3.1

Relevant log output

No logs are printed

Code of Conduct

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

[wolfy1339]

This isn't the right repository for issues with the @octokit/webhooks npm package. The proper repo would be https://github.com/octokit/webhooks.js

@gr2m @timrogers Can this issue be transferred to the proper repo.

---

That is odd. There's nothing printed to the console at all?
Would you mind sharing the payload you received that fails the validation?

[timomeh]

@wolfy1339 Ah, sorry for the wrong repo – too many tabs open simultaneously.

Looking at the source of the @octokit/webhooks npm package, the verify call doesn't include any debug logging. The verfication itself is in the @octokit/webhooks-methods package, which also doesn't seem to include any debug logging.

Here's the output of the failing payload. It's a private repo, so I redacted some sensitive info.

{
  "action": "created",
  "alert": {
    "number": 45,
    "state": "open",
    "dependency": {
      "package": {
        "ecosystem": "npm",
        "name": "decode-uri-component"
      },
      "manifest_path": "yarn.lock",
      "scope": "runtime"
    },
    "security_advisory": {
      "ghsa_id": "GHSA-w573-4hg7-7wgq",
      "cve_id": "CVE-2022-38900",
      "summary": "decode-uri-component vulnerable to Denial of Service (DoS)",
      "description": "decode-uri-component 0.2.0 is vulnerable to Improper Input Validation resulting in DoS.",
      "severity": "low",
      "identifiers": [
        {
          "value": "GHSA-w573-4hg7-7wgq",
          "type": "GHSA"
        },
        {
          "value": "CVE-2022-38900",
          "type": "CVE"
        }
      ],
      "references": [
        {
          "url": "https://nvd.nist.gov/vuln/detail/CVE-2022-38900"
        },
        {
          "url": "https://github.com/SamVerschueren/decode-uri-component/issues/5"
        },
        {
          "url": "https://github.com/sindresorhus/query-string/issues/345"
        },
        {
          "url": "https://github.com/advisories/GHSA-w573-4hg7-7wgq"
        }
      ],
      "published_at": "2022-11-28T15:30:24Z",
      "updated_at": "2022-11-28T23:36:02Z",
      "withdrawn_at": null,
      "vulnerabilities": [
        {
          "package": {
            "ecosystem": "npm",
            "name": "decode-uri-component"
          },
          "severity": "low",
          "vulnerable_version_range": "<= 0.2.0",
          "first_patched_version": null
        }
      ],
      "cvss": {
        "vector_string": null,
        "score": 0.0
      },
      "cwes": [
        {
          "cwe_id": "CWE-20",
          "name": "Improper Input Validation"
        }
      ]
    },
    "security_vulnerability": {
      "package": {
        "ecosystem": "npm",
        "name": "decode-uri-component"
      },
      "severity": "low",
      "vulnerable_version_range": "<= 0.2.0",
      "first_patched_version": null
    },
    "url": "https://api.github.com/repos/gigs/_redacted_/dependabot/alerts/45",
    "html_url": "https://github.com/gigs/_redacted_/security/dependabot/45",
    "created_at": "2022-11-29T13:32:39Z",
    "updated_at": "2022-11-29T13:32:39Z",
    "dismissed_at": null,
    "dismissed_by": null,
    "dismissed_reason": null,
    "dismissed_comment": null,
    "fixed_at": null
  },
  "repository": { /* redacted */ },
  "organization": {
    "login": "gigs",
    "id": 70662358,
    "node_id": "MDEyOk9yZ2FuaXphdGlvbjcwNjYyMzU4",
    "url": "https://api.github.com/orgs/gigs",
    "repos_url": "https://api.github.com/orgs/gigs/repos",
    "events_url": "https://api.github.com/orgs/gigs/events",
    "hooks_url": "https://api.github.com/orgs/gigs/hooks",
    "issues_url": "https://api.github.com/orgs/gigs/issues",
    "members_url": "https://api.github.com/orgs/gigs/members{/member}",
    "public_members_url": "https://api.github.com/orgs/gigs/public_members{/member}",
    "avatar_url": "https://avatars.githubusercontent.com/u/70662358?v=4",
    "description": ""
  },
  "sender": {
    "login": "github",
    "id": 9919,
    "node_id": "MDEyOk9yZ2FuaXphdGlvbjk5MTk=",
    "avatar_url": "https://avatars.githubusercontent.com/u/9919?v=4",
    "gravatar_id": "",
    "url": "https://api.github.com/users/github",
    "html_url": "https://github.com/github",
    "followers_url": "https://api.github.com/users/github/followers",
    "following_url": "https://api.github.com/users/github/following{/other_user}",
    "gists_url": "https://api.github.com/users/github/gists{/gist_id}",
    "starred_url": "https://api.github.com/users/github/starred{/owner}{/repo}",
    "subscriptions_url": "https://api.github.com/users/github/subscriptions",
    "organizations_url": "https://api.github.com/users/github/orgs",
    "repos_url": "https://api.github.com/users/github/repos",
    "events_url": "https://api.github.com/users/github/events{/privacy}",
    "received_events_url": "https://api.github.com/users/github/received_events",
    "type": "Organization",
    "site_admin": false
  },
  "installation": {
    "id": /* redacted */,
    "node_id": "/* redacted */"
  }
}

[wolfy1339]

Thanks! That will do the trick.
I'll look into it later

[timomeh]

Just to give as much info as possible: the dependabot_alert.dismissed event also fails the verification

{
  "action": "dismissed",
  "alert": {
    "number": 45,
    "state": "dismissed",
    "dependency": {
      "package": {
        "ecosystem": "npm",
        "name": "decode-uri-component"
      },
      "manifest_path": "yarn.lock",
      "scope": "runtime"
    },
    "security_advisory": {
      "ghsa_id": "GHSA-w573-4hg7-7wgq",
      "cve_id": "CVE-2022-38900",
      "summary": "decode-uri-component vulnerable to Denial of Service (DoS)",
      "description": "decode-uri-component 0.2.0 is vulnerable to Improper Input Validation resulting in DoS.",
      "severity": "low",
      "identifiers": [
        {
          "value": "GHSA-w573-4hg7-7wgq",
          "type": "GHSA"
        },
        {
          "value": "CVE-2022-38900",
          "type": "CVE"
        }
      ],
      "references": [
        {
          "url": "https://nvd.nist.gov/vuln/detail/CVE-2022-38900"
        },
        {
          "url": "https://github.com/SamVerschueren/decode-uri-component/issues/5"
        },
        {
          "url": "https://github.com/sindresorhus/query-string/issues/345"
        },
        {
          "url": "https://github.com/advisories/GHSA-w573-4hg7-7wgq"
        }
      ],
      "published_at": "2022-11-28T15:30:24Z",
      "updated_at": "2022-11-28T23:36:02Z",
      "withdrawn_at": null,
      "vulnerabilities": [
        {
          "package": {
            "ecosystem": "npm",
            "name": "decode-uri-component"
          },
          "severity": "low",
          "vulnerable_version_range": "<= 0.2.0",
          "first_patched_version": null
        }
      ],
      "cvss": {
        "vector_string": null,
        "score": 0.0
      },
      "cwes": [
        {
          "cwe_id": "CWE-20",
          "name": "Improper Input Validation"
        }
      ]
    },
    "security_vulnerability": {
      "package": {
        "ecosystem": "npm",
        "name": "decode-uri-component"
      },
      "severity": "low",
      "vulnerable_version_range": "<= 0.2.0",
      "first_patched_version": null
    },
    "url": "https://api.github.com/repos/gigs/_redacted_/dependabot/alerts/45",
    "html_url": "https://github.com/gigs/_redacted_/security/dependabot/45",
    "created_at": "2022-11-29T13:32:39Z",
    "updated_at": "2022-11-29T17:25:12Z",
    "dismissed_at": "2022-11-29T17:25:12Z",
    "dismissed_by": {
      "login": "timomeh",
      "id": 4227520,
      "node_id": "MDQ6VXNlcjQyMjc1MjA=",
      "avatar_url": "https://avatars.githubusercontent.com/u/4227520?v=4",
      "gravatar_id": "",
      "url": "https://api.github.com/users/timomeh",
      "html_url": "https://github.com/timomeh",
      "followers_url": "https://api.github.com/users/timomeh/followers",
      "following_url": "https://api.github.com/users/timomeh/following{/other_user}",
      "gists_url": "https://api.github.com/users/timomeh/gists{/gist_id}",
      "starred_url": "https://api.github.com/users/timomeh/starred{/owner}{/repo}",
      "subscriptions_url": "https://api.github.com/users/timomeh/subscriptions",
      "organizations_url": "https://api.github.com/users/timomeh/orgs",
      "repos_url": "https://api.github.com/users/timomeh/repos",
      "events_url": "https://api.github.com/users/timomeh/events{/privacy}",
      "received_events_url": "https://api.github.com/users/timomeh/received_events",
      "type": "User",
      "site_admin": false
    },
    "dismissed_reason": "not_used",
    "dismissed_comment": "foo bar",
    "fixed_at": null
  },
  "repository": { /* redacted */ },
  "organization": {
    "login": "gigs",
    "id": 70662358,
    "node_id": "MDEyOk9yZ2FuaXphdGlvbjcwNjYyMzU4",
    "url": "https://api.github.com/orgs/gigs",
    "repos_url": "https://api.github.com/orgs/gigs/repos",
    "events_url": "https://api.github.com/orgs/gigs/events",
    "hooks_url": "https://api.github.com/orgs/gigs/hooks",
    "issues_url": "https://api.github.com/orgs/gigs/issues",
    "members_url": "https://api.github.com/orgs/gigs/members{/member}",
    "public_members_url": "https://api.github.com/orgs/gigs/public_members{/member}",
    "avatar_url": "https://avatars.githubusercontent.com/u/70662358?v=4",
    "description": ""
  },
  "sender": {
    "login": "timomeh",
    "id": 4227520,
    "node_id": "MDQ6VXNlcjQyMjc1MjA=",
    "avatar_url": "https://avatars.githubusercontent.com/u/4227520?v=4",
    "gravatar_id": "",
    "url": "https://api.github.com/users/timomeh",
    "html_url": "https://github.com/timomeh",
    "followers_url": "https://api.github.com/users/timomeh/followers",
    "following_url": "https://api.github.com/users/timomeh/following{/other_user}",
    "gists_url": "https://api.github.com/users/timomeh/gists{/gist_id}",
    "starred_url": "https://api.github.com/users/timomeh/starred{/owner}{/repo}",
    "subscriptions_url": "https://api.github.com/users/timomeh/subscriptions",
    "organizations_url": "https://api.github.com/users/timomeh/orgs",
    "repos_url": "https://api.github.com/users/timomeh/repos",
    "events_url": "https://api.github.com/users/timomeh/events{/privacy}",
    "received_events_url": "https://api.github.com/users/timomeh/received_events",
    "type": "User",
    "site_admin": false
  },
  "installation": {
    "id": /* redacted */,
    "node_id": "/* redacted */"
  }
}

[kfcampbell]

I've transferred this issue to octokit/webhooks.js in accordance with @wolfy1339's wishes above. Thanks for troubleshooting, @wolfy1339!

[timomeh]

I found the bug – but I'm not sure if it should be fixed by webhook consumers, or by GitHub itself.

Root Cause

The bug happens due to the JSON parsing of the request body, when the request body contains a whole number in the form of a floating point number (e.g. 1.0).

JSON.parse("1.0") // -> 1
JSON.stringify(JSON.parse("1.0")) // -> "1"

The GitHub Webhook Body for dependabot_alert events contains the following:

      "cvss": {
        "vector_string": null,
        "score": 0.0
      },

Running this through JSON.parse() turns it into this:

  {
      cvss: {
        vector_string: null,
        score: 0
      }
  }

And then stringifying this back into JSON results in this:

      "cvss": {
        "vector_string": null,
        "score": 0
      },

Since this isn't equal to the original request body, the signature cannot be calculated correctly, and thus verification fails.

Proposed solution

IMHO this should be fixed by GitHub, not by the webhooks.js verification. Because potentially every JS Implementation is affected, as long as it's using a parsed JSON body, instead of the original utf-8 encoded request body string.

Instead, the request body should not include whole floating point numbers (e.g. 0.0), instead it should only be 0. This would prevent the issue from happening at all.

Everything else would lead to a bad developer experience, e.g. telling developers not to stringify the JSON-parsed payload, and use the original version instead.

I opened a Ticket with GitHub Support.

Update: This was discussed by GitHub Staff and decided to be kept as floating point number.

What is affected

• Passing a previously-parsed request body into webhooks.verify() (and similar methods)
• createNodeMiddleware(), because it uses the parsed request body if req.body is present, and parses it itself if it's not present.

Workaround

A) Users can pass the raw body string into webhooks.verify()
B) toNormalizedJsonString() could include a patch to convert cvss.score back into a floating point number.

How did I find this

I tried to verify the webhook without octokit/webhooks.js, and the verification still failed. I then tried validating it using Ruby, and the verification was successful in Ruby. So it had to be related to Node.

Comparing the stringified JSON from Node and from Ruby, the Ruby version still included the 0.0, but the Node version only included the 0.

[wolfy1339]

A) Users can pass the raw body string into webhooks.verify()

It is planned to not allow objects to be passed anymore, and only allow strings. Maybe it's a good idea to start the process of deprecating the functionality
#589

B) toNormalizedJsonString() could include a patch to convert cvss.score back into a floating point number.

What happens if this happens with another event? We can't just keep adding workarounds for every event.
Also see above, passing the raw body is the preferred method

[timomeh]

I agree that it makes sense if it doesn't deal with objects at all and how they should be serialized.

What happens if this happens with another event? We can't just keep adding workarounds for every event.

It was only meant as temporary workaround. I'm a fan of offering nice upgrade paths: fixing the specific issue, deprecating objects in a minor version, removing them completely in a major version. But of course I'd also understand if you don't want to deal with this glue code and remove objects right away. In the end, it's also temporary fixable using patch-package.

For whom it's useful, here's a patch to be used with patch-package, which fixes the floating point serialization for the specific cvss.score value.

diff --git a/node_modules/@octokit/webhooks/dist-node/index.js b/node_modules/@octokit/webhooks/dist-node/index.js
index fa94b1e..6f80672 100644
--- a/node_modules/@octokit/webhooks/dist-node/index.js
+++ b/node_modules/@octokit/webhooks/dist-node/index.js
@@ -161,7 +161,9 @@ function createEventHandler(options) {
  * GitHub sends its JSON with an indentation of 2 spaces and a line break at the end
  */
 function toNormalizedJsonString(payload) {
-  const payloadString = JSON.stringify(payload);
+  let payloadString = JSON.stringify(payload);
+  // ensure cvss is correctly stringified as floating point
+  payloadString = payloadString.replace(/"cvss":{("vector_string":.*,"score":\d*)}/gm, (s, p1) => s.replace(p1, p1 + '.0'));
   return payloadString.replace(/[^\\]\\u[\da-f]{4}/g, s => {
     return s.substr(0, 3) + s.substr(3).toUpperCase();
   });
diff --git a/node_modules/@octokit/webhooks/dist-src/to-normalized-json-string.js b/node_modules/@octokit/webhooks/dist-src/to-normalized-json-string.js
index 09413b5..84258f7 100644
--- a/node_modules/@octokit/webhooks/dist-src/to-normalized-json-string.js
+++ b/node_modules/@octokit/webhooks/dist-src/to-normalized-json-string.js
@@ -2,7 +2,9 @@
  * GitHub sends its JSON with an indentation of 2 spaces and a line break at the end
  */
 export function toNormalizedJsonString(payload) {
-    const payloadString = JSON.stringify(payload);
+    let payloadString = JSON.stringify(payload);
+    // ensure cvss is correctly stringified as floating point
+    payloadString = payloadString.replace(/"cvss":{("vector_string":.*,"score":\d*)}/gm, (s, p1) => s.replace(p1, p1 + '.0'));
     return payloadString.replace(/[^\\]\\u[\da-f]{4}/g, (s) => {
         return s.substr(0, 3) + s.substr(3).toUpperCase();
     });

[wolfy1339]

Are you able to pass the raw payload in string format? That would alleviate your issues

See #790 where I am deprecating passing the payload as an object

[timomeh]

I'm using Probot which uses createNodeMiddleware()

[github-actions]

🎉 This issue has been resolved in version 11.0.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀