Pull Request Comments are not correctly escaped (unparseable JSON)


#1

Hi,

I am using attempting to retrieve all reviews on a Pull Request, using the API docs here. However, it appears that if any of the comments in a review are spanned over mulitple lines (that is, a newline character exists, and in fact any special character that requires escaping), then these characters are not correctly escaped:

"body": "A multi-line comment, however...\r\nIs not escaped properly.",

This, of course, is invalid JSON and fails to parse.

Here’s a working example:

curl -XGET -H "Accept: application/vnd.github.loki-preview+json" https://api.github.com/repos/resin-io/procbots-public-test/pulls/23/reviews

And the result:
[ { "id": x, "user": { "login": "sablekeech", "id": x, "avatar_url": "https://avatars0.githubusercontent.com/u/25102929?v=3", "gravatar_id": "", "url": "https://api.github.com/users/sablekeech", "html_url": "https://github.com/sablekeech", "followers_url": "https://api.github.com/users/sablekeech/followers", "following_url": "https://api.github.com/users/sablekeech/following{/other_user}", "gists_url": "https://api.github.com/users/sablekeech/gists{/gist_id}", "starred_url": "https://api.github.com/users/sablekeech/starred{/owner}{/repo}", "subscriptions_url": "https://api.github.com/users/sablekeech/subscriptions", "organizations_url": "https://api.github.com/users/sablekeech/orgs", "repos_url": "https://api.github.com/users/sablekeech/repos", "events_url": "https://api.github.com/users/sablekeech/events{/privacy}", "received_events_url": "https://api.github.com/users/sablekeech/received_events", "type": "User", "site_admin": false }, "body": "A single line comment is fine.", "state": "COMMENTED", "html_url": "https://github.com/resin-io/procbots-public-test/pull/23#pullrequestreview-43981827", "pull_request_url": "https://api.github.com/repos/resin-io/procbots-public-test/pulls/23", "_links": { "html": { "href": "https://github.com/resin-io/procbots-public-test/pull/23#pullrequestreview-43981827" }, "pull_request": { "href": "https://api.github.com/repos/resin-io/procbots-public-test/pulls/23" } }, "submitted_at": "2017-06-14T10:55:42Z", "commit_id": "x" }, { "id": x, "user": { "login": "janercordanders", "id": x, "avatar_url": "https://avatars1.githubusercontent.com/u/x", "gravatar_id": "", "url": "https://api.github.com/users/janercordanders", "html_url": "https://github.com/janercordanders", "followers_url": "https://api.github.com/users/janercordanders/followers", "following_url": "https://api.github.com/users/janercordanders/following{/other_user}", "gists_url": "https://api.github.com/users/janercordanders/gists{/gist_id}", "starred_url": "https://api.github.com/users/janercordanders/starred{/owner}{/repo}", "subscriptions_url": "https://api.github.com/users/janercordanders/subscriptions", "organizations_url": "https://api.github.com/users/janercordanders/orgs", "repos_url": "https://api.github.com/users/janercordanders/repos", "events_url": "https://api.github.com/users/janercordanders/events{/privacy}", "received_events_url": "https://api.github.com/users/janercordanders/received_events", "type": "User", "site_admin": false }, "body": "A multi-line comment, however...\r\nIs not escaped properly.", "state": "APPROVED", "html_url": "https://github.com/resin-io/procbots-public-test/pull/23#pullrequestreview-43982027", "pull_request_url": "https://api.github.com/repos/resin-io/procbots-public-test/pulls/23", "_links": { "html": { "href": "https://github.com/resin-io/procbots-public-test/pull/23#pullrequestreview-43982027" }, "pull_request": { "href": "https://api.github.com/repos/resin-io/procbots-public-test/pulls/23" } }, "submitted_at": "2017-06-14T10:56:44Z", "commit_id": "x" } ]

It would be really great to get this fixed, as we’re relying on it to improve the reviewing stage of our CI process.

Thanks and best regards,

Heds


#2

Hi @hedss :wave:

What you’re reporting here doesn’t have anything to do with the GitHub Apps feature, as far as I can tell. This forum should be used only for discussing the GitHub Apps feature (e.g. how to build them and use APIs as an App), not for discussing general GitHub API problems/features.

If you want to report a problem with the GitHub REST API (unrelated to GitHub Apps), then you should reach out to Support via https://github.com/contact

Also, as far as I can tell, that’s valid JSON and https://jsonlint.com/ doesn’t complain when I tried to validate it there:

If you decide to reach out to GitHub Support to report this – I recommend being a bit more clear and precise about why you believe this is not valid JSON, why you believe additional escaping is needed, and how exactly you expected the string to be returned (again, don’t do that here since it’s not related to GitHub Apps).

Thanks!


#3

Hi @izuzak,

Thanks for the response! The last time I had a generic issue with the API (although using it as part of a Github App, as I’m doing here), I used the contact form and was asked to post into the forum. This was for the the thread here, consequently I’ve done the same this time.

I will send a query using the contact form again, however.

I’d like to point you at both https://www.ietf.org/rfc/rfc4627.txt and http://www.json.org/, which state that for characters in a value string ‘All Unicode characters may be placed within the quotation marks except for the characters that must be escaped: quotation mark, reverse solidus, and the control characters (U+0000 through U+001F).’ \r, \n, 't etc. are all control characters, and the values being returned in a Review body section are not correctly escaped, according to the JSON specification.

Indeed, this seems to hold true, as JSON.parse(), certainly under V8 and therefore Node.JS, does not parse this.

Thanks!


#4

Hi @hedss,

The last time I had a generic issue with the API (although using it as part of a Github App, as I’m doing here), I used the contact form and was asked to post into the forum. This was for the the thread here, consequently I’ve done the same this time.

Perhaps there’s been a misunderstanding, so let me clarify. The problem you’re describing here is not in any way related to the GitHub Apps feature – you can reproduce it even without using GitHub Apps, as you’ve demonstrated above. Problems like that should be reported to GitHub Support.

But imagine that the problem you described was reproducible only when you fetch comments as a GitHub App, but not when you fetch them otherwise (e.g. using a personal access token or OAuth token). In that case, it’s probable that the problem is related to the GitHub Apps feature somehow. Such problems should be reported here.

Perhaps when you first reached out to GitHub Support it wasn’t clear if and how the problem was related to GitHub Apps, so you were directed here. If if you could clarify that with future reports – that would be very helpful and would probably reduce confusion on both ends. :heart:

I will send a query using the contact form again, however.

Thanks! And thanks for the links – that helps! I could have sworn that \n was fine in JSON, but definitely looks like I was wrong. I’ll followup on your message you sent to Support. :bow:


#5

Hi @izuzak,

Thank you for the clarification on support, that clears it up tonnes!

I’m actually completely wrong on the JSON front. I’m not entirely sure why I thought this, and in fact the RTF spec. agrees with

{"body": "A multi-line comment, however...\r\nIs not escaped properly."}

as \n is escaping the control code (which would be a raw character of U+000A).

Apologies for the confusion and the erroneous ticket! :pensive:

Best regards,

Heds