search

octokit / webhooks.js

GitHub webhook events toolset for Node.js
MIT License
313 stars 79 forks source link

verifyAndReceive fails depending on json details #71

Closed dennissivia closed 5 years ago

dennissivia commented 5 years ago

Summary

verifyAndReceive uses a parsed representation of the passed json data. This can lead to false negative signature verifications. One Example is the signature verification failure for ANSI/ISO-6429 codes a.k.a. escape sequences. If Any GitHub event body contains an escape sequence, the verification will always fail, even though everything is valid.

Details

When the ESC sequence is json-encoded it might be encoded as \u001b OR \u001B. The node json parser will always encode this sequence with a lowercase b, which means that the verification will fail if the signature was generated with a capital B (which is already the case). Since this (and other encoding details) are ambiguous in the json standard, the only safe option is to verify the signature with the raw body we receive and not with the parsed representation, since JSON.stringify(JSON.parse(x)) does not necessarily yield x, thus causing a false negative. This however is only one example, that already happens. In general, the only way that will guarantee that we properly verify the signature header is, using the data in the format it was encoded in the first place. This means, before parsing it.

Suggestion

In order to solve the problem, middleware/middleware might need to be adjusted, together with get-payload and verify-and-receive. I was already working on that when a realised the following compatibility issues:

Compatibility concerns

  1. verifyAndReceived is part of the public API, meaning people are using it directly and outside of the middleware context. (I saw it in some PR discussions). This means this function has to stay and still work with parsed data instead of json.
  2. Since this PR octokit is supporting the usage in combination with body-parser, which will also store parsed information in request.body

It seems as if we need to keep the verification based on parsed data, if we want to support the mix of body-parser and webhook.js. We could also store the raw body in the request object.

A compromise could be: testing if req.body is defined and if that is the case, we could use the current implementation. Also keep the current implementation of verifyAndReceive exposed and (maybe?) deprecate it. In order to fix our bug, we could just add a second verify and receive implementation that does the verification with the raw data. This however would duplicate a lot of code and I am not sure what you think about this.

I hope we can figure out a way to fix this bug. This is just my view on things, maybe somebody has a better suggestion on how to address this issue.

Related issues

/cc @gr2m

gr2m commented 5 years ago

🙏 thanks for opening the issue Dennis!

I was not aware of the problem with escape sequences and JSON.parse

> JSON.parse('"\\u001B"')
< '\u001b'
  1. could you get a pull request draft going with a failing test?
  2. How can I trigger an event that includes an escape sequence such as ESC?
dennissivia commented 5 years ago

1: Will take a little bit of time, because I have to craft a signature, but I will create it asap

2: I triggered the issue by doing end-to-end requests, via the GitHub API via curl like this:

curl -i \
  -H "Authorization: token $API_TOKEN" \
  -H "Content-Type: application/vnd.github.v3+json" \
  -d '{"body":"Foo\n\u001B[34mbar: \u2665\u2665\u2665\u2665\u2665\u2665\u2665\u2665\nthis-is-lost\u001b[0m\u001B[2K" }' \
  https://api.github.com/repos/:owner/:repo/issues/:issueID/comments"

The bare minimum for a failing payload would be \u001B aka ESC. Github will make this a capital B before encoding, when the request is created via the API (like above) and thus it will fail, when webhook.js will use the JS representation (lowercase b) for the validation.

gr2m commented 5 years ago

Thanks! I created a test comment at https://github.com/gr2m/sandbox/issues/78#issuecomment-481409917 and retrieved a payload via smee.io, but it looks like smee.io does the JSON transform before storing the webhooks

"body": "Foo\n\u001b[34mbar: ♥♥♥♥♥♥♥♥\nthis-is-lost\u001b[0m\u001b[2K" 
{
    "host": "smee.io",
    "connection": "close",
    "accept": "*/*",
    "user-agent": "GitHub-Hookshot/38ab7f6",
    "x-github-event": "issue_comment",
    "x-github-delivery": "1340a790-5b01-11e9-9e36-475fe7c6e71e",
    "content-type": "application/json",
    "x-hub-signature": "sha1=b56d20007f63e1042638b6ace91bb59e8ac4deed",
    "x-request-id": "e08b9687-b11f-48bf-82a9-8bb6efad73de",
    "x-forwarded-for": "192.30.252.45",
    "x-forwarded-proto": "https",
    "x-forwarded-port": "443",
    "via": "1.1 vegur",
    "connect-time": "1",
    "x-request-start": "1554839579071",
    "total-route-time": "0",
    "content-length": "8472",
    "body": {
        "action": "created",
        "issue": {
            "url": "https://api.github.com/repos/gr2m/sandbox/issues/78",
            "repository_url": "https://api.github.com/repos/gr2m/sandbox",
            "labels_url": "https://api.github.com/repos/gr2m/sandbox/issues/78/labels{/name}",
            "comments_url": "https://api.github.com/repos/gr2m/sandbox/issues/78/comments",
            "events_url": "https://api.github.com/repos/gr2m/sandbox/issues/78/events",
            "html_url": "https://github.com/gr2m/sandbox/issues/78",
            "id": 431161080,
            "node_id": "MDU6SXNzdWU0MzExNjEwODA=",
            "number": 78,
            "title": "Testing escape sequences in webhooks",
            "user": {
                "login": "gr2m",
                "id": 39992,
                "node_id": "MDQ6VXNlcjM5OTky",
                "avatar_url": "https://avatars3.githubusercontent.com/u/39992?v=4",
                "gravatar_id": "",
                "url": "https://api.github.com/users/gr2m",
                "html_url": "https://github.com/gr2m",
                "followers_url": "https://api.github.com/users/gr2m/followers",
                "following_url": "https://api.github.com/users/gr2m/following{/other_user}",
                "gists_url": "https://api.github.com/users/gr2m/gists{/gist_id}",
                "starred_url": "https://api.github.com/users/gr2m/starred{/owner}{/repo}",
                "subscriptions_url": "https://api.github.com/users/gr2m/subscriptions",
                "organizations_url": "https://api.github.com/users/gr2m/orgs",
                "repos_url": "https://api.github.com/users/gr2m/repos",
                "events_url": "https://api.github.com/users/gr2m/events{/privacy}",
                "received_events_url": "https://api.github.com/users/gr2m/received_events",
                "type": "User",
                "site_admin": false
            },
            "labels": [],
            "state": "open",
            "locked": false,
            "assignee": null,
            "assignees": [],
            "milestone": null,
            "comments": 0,
            "created_at": "2019-04-09T19:52:10Z",
            "updated_at": "2019-04-09T19:52:58Z",
            "closed_at": null,
            "author_association": "OWNER",
            "body": "https://github.com/octokit/webhooks.js/issues/71#issuecomment-481394835"
        },
        "comment": {
            "url": "https://api.github.com/repos/gr2m/sandbox/issues/comments/481409917",
            "html_url": "https://github.com/gr2m/sandbox/issues/78#issuecomment-481409917",
            "issue_url": "https://api.github.com/repos/gr2m/sandbox/issues/78",
            "id": 481409917,
            "node_id": "MDEyOklzc3VlQ29tbWVudDQ4MTQwOTkxNw==",
            "user": {
                "login": "gr2m",
                "id": 39992,
                "node_id": "MDQ6VXNlcjM5OTky",
                "avatar_url": "https://avatars3.githubusercontent.com/u/39992?v=4",
                "gravatar_id": "",
                "url": "https://api.github.com/users/gr2m",
                "html_url": "https://github.com/gr2m",
                "followers_url": "https://api.github.com/users/gr2m/followers",
                "following_url": "https://api.github.com/users/gr2m/following{/other_user}",
                "gists_url": "https://api.github.com/users/gr2m/gists{/gist_id}",
                "starred_url": "https://api.github.com/users/gr2m/starred{/owner}{/repo}",
                "subscriptions_url": "https://api.github.com/users/gr2m/subscriptions",
                "organizations_url": "https://api.github.com/users/gr2m/orgs",
                "repos_url": "https://api.github.com/users/gr2m/repos",
                "events_url": "https://api.github.com/users/gr2m/events{/privacy}",
                "received_events_url": "https://api.github.com/users/gr2m/received_events",
                "type": "User",
                "site_admin": false
            },
            "created_at": "2019-04-09T19:52:58Z",
            "updated_at": "2019-04-09T19:52:58Z",
            "author_association": "OWNER",
            "body": "Foo\n\u001b[34mbar: ♥♥♥♥♥♥♥♥\nthis-is-lost\u001b[0m\u001b[2K"
        },
        "repository": {
            "id": 102985470,
            "node_id": "MDEwOlJlcG9zaXRvcnkxMDI5ODU0NzA=",
            "name": "sandbox",
            "full_name": "gr2m/sandbox",
            "private": false,
            "owner": {
                "login": "gr2m",
                "id": 39992,
                "node_id": "MDQ6VXNlcjM5OTky",
                "avatar_url": "https://avatars3.githubusercontent.com/u/39992?v=4",
                "gravatar_id": "",
                "url": "https://api.github.com/users/gr2m",
                "html_url": "https://github.com/gr2m",
                "followers_url": "https://api.github.com/users/gr2m/followers",
                "following_url": "https://api.github.com/users/gr2m/following{/other_user}",
                "gists_url": "https://api.github.com/users/gr2m/gists{/gist_id}",
                "starred_url": "https://api.github.com/users/gr2m/starred{/owner}{/repo}",
                "subscriptions_url": "https://api.github.com/users/gr2m/subscriptions",
                "organizations_url": "https://api.github.com/users/gr2m/orgs",
                "repos_url": "https://api.github.com/users/gr2m/repos",
                "events_url": "https://api.github.com/users/gr2m/events{/privacy}",
                "received_events_url": "https://api.github.com/users/gr2m/received_events",
                "type": "User",
                "site_admin": false
            },
            "html_url": "https://github.com/gr2m/sandbox",
            "description": "just playing around don’t mind me",
            "fork": false,
            "url": "https://api.github.com/repos/gr2m/sandbox",
            "forks_url": "https://api.github.com/repos/gr2m/sandbox/forks",
            "keys_url": "https://api.github.com/repos/gr2m/sandbox/keys{/key_id}",
            "collaborators_url": "https://api.github.com/repos/gr2m/sandbox/collaborators{/collaborator}",
            "teams_url": "https://api.github.com/repos/gr2m/sandbox/teams",
            "hooks_url": "https://api.github.com/repos/gr2m/sandbox/hooks",
            "issue_events_url": "https://api.github.com/repos/gr2m/sandbox/issues/events{/number}",
            "events_url": "https://api.github.com/repos/gr2m/sandbox/events",
            "assignees_url": "https://api.github.com/repos/gr2m/sandbox/assignees{/user}",
            "branches_url": "https://api.github.com/repos/gr2m/sandbox/branches{/branch}",
            "tags_url": "https://api.github.com/repos/gr2m/sandbox/tags",
            "blobs_url": "https://api.github.com/repos/gr2m/sandbox/git/blobs{/sha}",
            "git_tags_url": "https://api.github.com/repos/gr2m/sandbox/git/tags{/sha}",
            "git_refs_url": "https://api.github.com/repos/gr2m/sandbox/git/refs{/sha}",
            "trees_url": "https://api.github.com/repos/gr2m/sandbox/git/trees{/sha}",
            "statuses_url": "https://api.github.com/repos/gr2m/sandbox/statuses/{sha}",
            "languages_url": "https://api.github.com/repos/gr2m/sandbox/languages",
            "stargazers_url": "https://api.github.com/repos/gr2m/sandbox/stargazers",
            "contributors_url": "https://api.github.com/repos/gr2m/sandbox/contributors",
            "subscribers_url": "https://api.github.com/repos/gr2m/sandbox/subscribers",
            "subscription_url": "https://api.github.com/repos/gr2m/sandbox/subscription",
            "commits_url": "https://api.github.com/repos/gr2m/sandbox/commits{/sha}",
            "git_commits_url": "https://api.github.com/repos/gr2m/sandbox/git/commits{/sha}",
            "comments_url": "https://api.github.com/repos/gr2m/sandbox/comments{/number}",
            "issue_comment_url": "https://api.github.com/repos/gr2m/sandbox/issues/comments{/number}",
            "contents_url": "https://api.github.com/repos/gr2m/sandbox/contents/{+path}",
            "compare_url": "https://api.github.com/repos/gr2m/sandbox/compare/{base}...{head}",
            "merges_url": "https://api.github.com/repos/gr2m/sandbox/merges",
            "archive_url": "https://api.github.com/repos/gr2m/sandbox/{archive_format}{/ref}",
            "downloads_url": "https://api.github.com/repos/gr2m/sandbox/downloads",
            "issues_url": "https://api.github.com/repos/gr2m/sandbox/issues{/number}",
            "pulls_url": "https://api.github.com/repos/gr2m/sandbox/pulls{/number}",
            "milestones_url": "https://api.github.com/repos/gr2m/sandbox/milestones{/number}",
            "notifications_url": "https://api.github.com/repos/gr2m/sandbox/notifications{?since,all,participating}",
            "labels_url": "https://api.github.com/repos/gr2m/sandbox/labels{/name}",
            "releases_url": "https://api.github.com/repos/gr2m/sandbox/releases{/id}",
            "deployments_url": "https://api.github.com/repos/gr2m/sandbox/deployments",
            "created_at": "2017-09-09T21:16:50Z",
            "updated_at": "2019-02-24T22:41:09Z",
            "pushed_at": "2019-03-04T02:11:56Z",
            "git_url": "git://github.com/gr2m/sandbox.git",
            "ssh_url": "git@github.com:gr2m/sandbox.git",
            "clone_url": "https://github.com/gr2m/sandbox.git",
            "svn_url": "https://github.com/gr2m/sandbox",
            "homepage": null,
            "size": 29,
            "stargazers_count": 1,
            "watchers_count": 1,
            "language": null,
            "has_issues": true,
            "has_projects": true,
            "has_downloads": true,
            "has_wiki": false,
            "has_pages": false,
            "forks_count": 1,
            "mirror_url": null,
            "archived": false,
            "disabled": false,
            "open_issues_count": 5,
            "license": null,
            "forks": 1,
            "open_issues": 5,
            "watchers": 1,
            "default_branch": "latest"
        },
        "sender": {
            "login": "gr2m",
            "id": 39992,
            "node_id": "MDQ6VXNlcjM5OTky",
            "avatar_url": "https://avatars3.githubusercontent.com/u/39992?v=4",
            "gravatar_id": "",
            "url": "https://api.github.com/users/gr2m",
            "html_url": "https://github.com/gr2m",
            "followers_url": "https://api.github.com/users/gr2m/followers",
            "following_url": "https://api.github.com/users/gr2m/following{/other_user}",
            "gists_url": "https://api.github.com/users/gr2m/gists{/gist_id}",
            "starred_url": "https://api.github.com/users/gr2m/starred{/owner}{/repo}",
            "subscriptions_url": "https://api.github.com/users/gr2m/subscriptions",
            "organizations_url": "https://api.github.com/users/gr2m/orgs",
            "repos_url": "https://api.github.com/users/gr2m/repos",
            "events_url": "https://api.github.com/users/gr2m/events{/privacy}",
            "received_events_url": "https://api.github.com/users/gr2m/received_events",
            "type": "User",
            "site_admin": false
        },
        "installation": {
            "id": 743367,
            "node_id": "MDIzOkludGVncmF0aW9uSW5zdGFsbGF0aW9uNzQzMzY3"
        }
    },
    "query": {},
    "timestamp": 1554839579075
}

I'll try a minimal setup to retrieve the webhook without using smee.io, just using bare Node.js

gr2m commented 5 years ago

I was able to confirm it using a simple server hosted on Glitch: https://glitch.com/edit/#!/rocky-den?path=server.js:17:27

"body": "Foo\n\u001B[34mbar: ♥♥♥♥♥♥♥♥\nthis-is-lost\u001B[0m\u001B[2K" 
{
    "action": "created",
    "issue": {
        "url": "https://api.github.com/repos/gr2m/sandbox/issues/78",
        "repository_url": "https://api.github.com/repos/gr2m/sandbox",
        "labels_url": "https://api.github.com/repos/gr2m/sandbox/issues/78/labels{/name}",
        "comments_url": "https://api.github.com/repos/gr2m/sandbox/issues/78/comments",
        "events_url": "https://api.github.com/repos/gr2m/sandbox/issues/78/events",
        "html_url": "https://github.com/gr2m/sandbox/issues/78",
        "id": 431161080,
        "node_id": "MDU6SXNzdWU0MzExNjEwODA=",
        "number": 78,
        "title": "Testing escape sequences in webhooks",
        "user": {
            "login": "gr2m",
            "id": 39992,
            "node_id": "MDQ6VXNlcjM5OTky",
            "avatar_url": "https://avatars3.githubusercontent.com/u/39992?v=4",
            "gravatar_id": "",
            "url": "https://api.github.com/users/gr2m",
            "html_url": "https://github.com/gr2m",
            "followers_url": "https://api.github.com/users/gr2m/followers",
            "following_url": "https://api.github.com/users/gr2m/following{/other_user}",
            "gists_url": "https://api.github.com/users/gr2m/gists{/gist_id}",
            "starred_url": "https://api.github.com/users/gr2m/starred{/owner}{/repo}",
            "subscriptions_url": "https://api.github.com/users/gr2m/subscriptions",
            "organizations_url": "https://api.github.com/users/gr2m/orgs",
            "repos_url": "https://api.github.com/users/gr2m/repos",
            "events_url": "https://api.github.com/users/gr2m/events{/privacy}",
            "received_events_url": "https://api.github.com/users/gr2m/received_events",
            "type": "User",
            "site_admin": false
        },
        "labels": [],
        "state": "open",
        "locked": false,
        "assignee": null,
        "assignees": [],
        "milestone": null,
        "comments": 1,
        "created_at": "2019-04-09T19:52:10Z",
        "updated_at": "2019-04-09T20:14:44Z",
        "closed_at": null,
        "author_association": "OWNER",
        "body": "https://github.com/octokit/webhooks.js/issues/71#issuecomment-481394835"
    },
    "comment": {
        "url": "https://api.github.com/repos/gr2m/sandbox/issues/comments/481417364",
        "html_url": "https://github.com/gr2m/sandbox/issues/78#issuecomment-481417364",
        "issue_url": "https://api.github.com/repos/gr2m/sandbox/issues/78",
        "id": 481417364,
        "node_id": "MDEyOklzc3VlQ29tbWVudDQ4MTQxNzM2NA==",
        "user": {
            "login": "gr2m",
            "id": 39992,
            "node_id": "MDQ6VXNlcjM5OTky",
            "avatar_url": "https://avatars3.githubusercontent.com/u/39992?v=4",
            "gravatar_id": "",
            "url": "https://api.github.com/users/gr2m",
            "html_url": "https://github.com/gr2m",
            "followers_url": "https://api.github.com/users/gr2m/followers",
            "following_url": "https://api.github.com/users/gr2m/following{/other_user}",
            "gists_url": "https://api.github.com/users/gr2m/gists{/gist_id}",
            "starred_url": "https://api.github.com/users/gr2m/starred{/owner}{/repo}",
            "subscriptions_url": "https://api.github.com/users/gr2m/subscriptions",
            "organizations_url": "https://api.github.com/users/gr2m/orgs",
            "repos_url": "https://api.github.com/users/gr2m/repos",
            "events_url": "https://api.github.com/users/gr2m/events{/privacy}",
            "received_events_url": "https://api.github.com/users/gr2m/received_events",
            "type": "User",
            "site_admin": false
        },
        "created_at": "2019-04-09T20:14:44Z",
        "updated_at": "2019-04-09T20:14:44Z",
        "author_association": "OWNER",
        "body": "Foo\n\u001B[34mbar: ♥♥♥♥♥♥♥♥\nthis-is-lost\u001B[0m\u001B[2K"
    },
    "repository": {
        "id": 102985470,
        "node_id": "MDEwOlJlcG9zaXRvcnkxMDI5ODU0NzA=",
        "name": "sandbox",
        "full_name": "gr2m/sandbox",
        "private": false,
        "owner": {
            "login": "gr2m",
            "id": 39992,
            "node_id": "MDQ6VXNlcjM5OTky",
            "avatar_url": "https://avatars3.githubusercontent.com/u/39992?v=4",
            "gravatar_id": "",
            "url": "https://api.github.com/users/gr2m",
            "html_url": "https://github.com/gr2m",
            "followers_url": "https://api.github.com/users/gr2m/followers",
            "following_url": "https://api.github.com/users/gr2m/following{/other_user}",
            "gists_url": "https://api.github.com/users/gr2m/gists{/gist_id}",
            "starred_url": "https://api.github.com/users/gr2m/starred{/owner}{/repo}",
            "subscriptions_url": "https://api.github.com/users/gr2m/subscriptions",
            "organizations_url": "https://api.github.com/users/gr2m/orgs",
            "repos_url": "https://api.github.com/users/gr2m/repos",
            "events_url": "https://api.github.com/users/gr2m/events{/privacy}",
            "received_events_url": "https://api.github.com/users/gr2m/received_events",
            "type": "User",
            "site_admin": false
        },
        "html_url": "https://github.com/gr2m/sandbox",
        "description": "just playing around don’t mind me",
        "fork": false,
        "url": "https://api.github.com/repos/gr2m/sandbox",
        "forks_url": "https://api.github.com/repos/gr2m/sandbox/forks",
        "keys_url": "https://api.github.com/repos/gr2m/sandbox/keys{/key_id}",
        "collaborators_url": "https://api.github.com/repos/gr2m/sandbox/collaborators{/collaborator}",
        "teams_url": "https://api.github.com/repos/gr2m/sandbox/teams",
        "hooks_url": "https://api.github.com/repos/gr2m/sandbox/hooks",
        "issue_events_url": "https://api.github.com/repos/gr2m/sandbox/issues/events{/number}",
        "events_url": "https://api.github.com/repos/gr2m/sandbox/events",
        "assignees_url": "https://api.github.com/repos/gr2m/sandbox/assignees{/user}",
        "branches_url": "https://api.github.com/repos/gr2m/sandbox/branches{/branch}",
        "tags_url": "https://api.github.com/repos/gr2m/sandbox/tags",
        "blobs_url": "https://api.github.com/repos/gr2m/sandbox/git/blobs{/sha}",
        "git_tags_url": "https://api.github.com/repos/gr2m/sandbox/git/tags{/sha}",
        "git_refs_url": "https://api.github.com/repos/gr2m/sandbox/git/refs{/sha}",
        "trees_url": "https://api.github.com/repos/gr2m/sandbox/git/trees{/sha}",
        "statuses_url": "https://api.github.com/repos/gr2m/sandbox/statuses/{sha}",
        "languages_url": "https://api.github.com/repos/gr2m/sandbox/languages",
        "stargazers_url": "https://api.github.com/repos/gr2m/sandbox/stargazers",
        "contributors_url": "https://api.github.com/repos/gr2m/sandbox/contributors",
        "subscribers_url": "https://api.github.com/repos/gr2m/sandbox/subscribers",
        "subscription_url": "https://api.github.com/repos/gr2m/sandbox/subscription",
        "commits_url": "https://api.github.com/repos/gr2m/sandbox/commits{/sha}",
        "git_commits_url": "https://api.github.com/repos/gr2m/sandbox/git/commits{/sha}",
        "comments_url": "https://api.github.com/repos/gr2m/sandbox/comments{/number}",
        "issue_comment_url": "https://api.github.com/repos/gr2m/sandbox/issues/comments{/number}",
        "contents_url": "https://api.github.com/repos/gr2m/sandbox/contents/{+path}",
        "compare_url": "https://api.github.com/repos/gr2m/sandbox/compare/{base}...{head}",
        "merges_url": "https://api.github.com/repos/gr2m/sandbox/merges",
        "archive_url": "https://api.github.com/repos/gr2m/sandbox/{archive_format}{/ref}",
        "downloads_url": "https://api.github.com/repos/gr2m/sandbox/downloads",
        "issues_url": "https://api.github.com/repos/gr2m/sandbox/issues{/number}",
        "pulls_url": "https://api.github.com/repos/gr2m/sandbox/pulls{/number}",
        "milestones_url": "https://api.github.com/repos/gr2m/sandbox/milestones{/number}",
        "notifications_url": "https://api.github.com/repos/gr2m/sandbox/notifications{?since,all,participating}",
        "labels_url": "https://api.github.com/repos/gr2m/sandbox/labels{/name}",
        "releases_url": "https://api.github.com/repos/gr2m/sandbox/releases{/id}",
        "deployments_url": "https://api.github.com/repos/gr2m/sandbox/deployments",
        "created_at": "2017-09-09T21:16:50Z",
        "updated_at": "2019-02-24T22:41:09Z",
        "pushed_at": "2019-03-04T02:11:56Z",
        "git_url": "git://github.com/gr2m/sandbox.git",
        "ssh_url": "git@github.com:gr2m/sandbox.git",
        "clone_url": "https://github.com/gr2m/sandbox.git",
        "svn_url": "https://github.com/gr2m/sandbox",
        "homepage": null,
        "size": 29,
        "stargazers_count": 1,
        "watchers_count": 1,
        "language": null,
        "has_issues": true,
        "has_projects": true,
        "has_downloads": true,
        "has_wiki": false,
        "has_pages": false,
        "forks_count": 1,
        "mirror_url": null,
        "archived": false,
        "disabled": false,
        "open_issues_count": 5,
        "license": null,
        "forks": 1,
        "open_issues": 5,
        "watchers": 1,
        "default_branch": "latest"
    },
    "sender": {
        "login": "gr2m",
        "id": 39992,
        "node_id": "MDQ6VXNlcjM5OTky",
        "avatar_url": "https://avatars3.githubusercontent.com/u/39992?v=4",
        "gravatar_id": "",
        "url": "https://api.github.com/users/gr2m",
        "html_url": "https://github.com/gr2m",
        "followers_url": "https://api.github.com/users/gr2m/followers",
        "following_url": "https://api.github.com/users/gr2m/following{/other_user}",
        "gists_url": "https://api.github.com/users/gr2m/gists{/gist_id}",
        "starred_url": "https://api.github.com/users/gr2m/starred{/owner}{/repo}",
        "subscriptions_url": "https://api.github.com/users/gr2m/subscriptions",
        "organizations_url": "https://api.github.com/users/gr2m/orgs",
        "repos_url": "https://api.github.com/users/gr2m/repos",
        "events_url": "https://api.github.com/users/gr2m/events{/privacy}",
        "received_events_url": "https://api.github.com/users/gr2m/received_events",
        "type": "User",
        "site_admin": false
    },
    "installation": {
        "id": 743367,
        "node_id": "MDIzOkludGVncmF0aW9uSW5zdGFsbGF0aW9uNzQzMzY3"
    }
}
gimenete commented 5 years ago

@gr2m the main issue is that the library is assuming that JSON.stringify() will generate the same output than the original raw payload. And that can break at any moment. If github starts to send the JSON prettified, the current implementation would break for example. If github starts to add spaces between array items, the signature implementation will break, and so on. It is possible that the current implementation is failing not only for ansi codes but for other characters, because you can escape any character in JSON: it's up to the implementation to decide which ones should be escaped. Try this:

console.log(JSON.parse('{"foo":"\\u0061"}'))

It's just the character a, but escaped.

The solution is to always calculate the signature from the original, raw, payload. Not the result of stringifying the data. In fact, all the functions: sign, verify, verifyAndReceive,... should deprecate their support for signing objects and only support strings or buffers.

gr2m commented 5 years ago

Got it, that all makes sense 🤔 I’ll need to think about how to migrate to a new version

gimenete commented 5 years ago

@gr2m I think the library should print a deprecation warning if it receives something that is neither a string nor a buffer. Also, the payload should always be read like this: https://github.com/octokit/webhooks.js/blob/c0fade79854b19efda2b837356741b0eceb94c54/middleware/get-payload.js#L13-L16 and keep the data somewhere, and that might break apps using body-parser or any middleware that reads the body before webhooks.js

gimenete commented 5 years ago

The quick and dirty fix is to do this here: https://github.com/octokit/webhooks.js/blob/17ec6e3dc434d39caf542efba017509339be8a52/sign/index.js#L10

JSON.stringify(payload).replace(/\\u[\da-z]{4}/g, s => s.toUpperCase())

But whenever possible it should always use the raw payload instead. Because this fix does not guarantee that the result is going to be the same than the original payload. However, at least, fixes the known problem with the ESC character used in ansi codes.

dennissivia commented 5 years ago

The quick and dirty fix is to do this here:

👍 Yeah, I was thinking about the same. For now this would solve the problem without breaking existing apps. However I would like to also add a deprecation message with the same change, as well as pointing out that the support for also having body-parser or similar solutions at the same time is also deprecated.

After that we can consider a major release (after the release with the bugfix) that works the other way around or even is incompatible with other body-parsers. (like throwing an exception if the content is already read and parsed). This should be another issue/pr however so that we can ship this change in a pragmatic and fast fashion. Would you be fine with that @gr2m ?

gr2m commented 5 years ago

Yes that sounds good!

We can move forward with the quickfix by @gimenete! Though I think it will uppercase the \u prefix, too, not only the 4 characters after. Also I think the sequence is hexidecimal, so the right regex would be /\\u[\da-f]{4}/g, right?

I’ve started a pull request at https://github.com/octokit/webhooks.js/pull/72, I want to verify it with a test app before merging.

Right now I wonder how that would work with Amazon Lambda / Firebase Functions etc? I think we get a request.rawBody property, but I’m not yet sure how that would work, I haven’t worked with these environments myself yet.

I’d like to figure that out before moving forward with the deprecation and then the breaking change.

gr2m commented 5 years ago

https://github.com/octokit/webhooks.js/pull/72 is ready for review, could you have a look if it looks good?

hugopeixoto commented 5 years ago

This change causes an unfortunate side effect. If the payload contains strings that start with \u, the replacer kicks in and signature validation fails. I ran into this issue in a dependabot PR, which contained the following text:

Fix: exclude \u000d so new line won't convert to text

I quick-fixed this by changing the replacer to:

function toNormalizedJsonString (payload) {
  return JSON.stringify(payload).replace(/[^\\]\\u[\da-f]{4}/g, s => {
    return s.substr(0, 3) + s.substr(3).toUpperCase()
  })
}

This is still not the perfect solution, but I guess it helps? I'll make a PR with this (including tests).

hugopeixoto commented 5 years ago

This will probably fail if the text ends in \ followed by an escape sequence. Unsure if this is addressable with a single regular expression.

gr2m commented 5 years ago

@hugopeixoto could you please start a separate issue with a reproducible test case? You can use https://runkit.com/ to create it. Or even better, send a pull request with a failing test, and we take it from there?