verifyAndReceive fails depending on json details #71
Labels
Type: Bug
Something isn't working as documented, or is being fixed
Comments
|
🙏 thanks for opening the issue Dennis! I was not aware of the problem with escape sequences and JSON.parse
|
|
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 |
|
Thanks! I created a test comment at gr2m/sandbox#78 (comment) and retrieved a payload via smee.io, but it looks like smee.io does the JSON transform before storing the webhooks
|
|
I was able to confirm it using a simple server hosted on Glitch:
|
|
@gr2m the main issue is that the library is assuming that console.log(JSON.parse('{"foo":"\\u0061"}'))It's just the character 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: |
|
Got it, that all makes sense 🤔 I’ll need to think about how to migrate to a new version |
|
@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: webhooks.js/middleware/get-payload.js Lines 13 to 16 in c0fade7 data somewhere, and that might break apps using body-parser or any middleware that reads the body before webhooks.js
|
|
The quick and dirty fix is to do this here: Line 10 in 17ec6e3 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. |
👍 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. |
1 task
|
Yes that sounds good! We can move forward with the quickfix by @gimenete! Though I think it will uppercase the I’ve started a pull request at #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 I’d like to figure that out before moving forward with the deprecation and then the breaking change. |
|
#72 is ready for review, could you have a look if it looks good? |
dennissivia
added a commit
to integrations/slack
that referenced
this issue
Apr 12, 2019
Updates the requires version of probot, which includes an update of octokit/webhook.js that fixes signature verification for payloads that contain ANSI codes. In particular `ESC` sequences. See: * octokit/webhooks.js#71 * octokit/webhooks.js#73 * probot/probot#912
dennissivia
added a commit
to integrations/slack
that referenced
this issue
Apr 12, 2019
Updates the requires version of probot, which includes an update of octokit/webhook.js that fixes signature verification for payloads that contain ANSI codes. In particular `ESC` sequences. See: * octokit/webhooks.js#71 * octokit/webhooks.js#73 * probot/probot#912
|
This change causes an unfortunate side effect. If the payload contains strings that start with
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). |
|
This will probably fail if the text ends in |
|
@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? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Summary
verifyAndReceiveuses 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
ESCsequence is json-encoded it might be encoded as\u001bOR\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 capitalB(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 yieldx, 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-payloadandverify-and-receive.I was already working on that when a realised the following compatibility issues:
Compatibility concerns
verifyAndReceivedis 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.request.bodyIt seems as if we need to keep the verification based on parsed data, if we want to support the mix of
body-parserand webhook.js. We could also store the raw body in the request object.A compromise could be:
testing if
req.bodyis defined and if that is the case, we could use the current implementation.Also keep the current implementation of
verifyAndReceiveexposed 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
The text was updated successfully, but these errors were encountered: