integrations / slack

Bring your code to the conversations you care about with GitHub's integration for Slack
https://slack.github.com/
MIT License
3.1k stars 489 forks source link

"Pending reviews" changes backticked word in PR title to unicode junk #1396

Open gthb opened 2 years ago

gthb commented 2 years ago

Describe the bug

A PR with this title:

Remove data cropping from `resolveArea` and `resolveRange`

shows up in the Github Slack app “Pending reviews” reminder with garbled content, rendering in the macOS Slack app as: image and in the Android Slack app as: image

where the backticked words do become code blocks (like in PR views on github.com), but the contents of the code blocks are mangled.

The exact contents of those code blocks (copied from Slack and pasted into xxd) are:

ee 80 a2 2d ee 80 a2 32

and

ee 80 a2 2d ee 80 a2 33

That ee 80 a2 UTF-8 sequence is U+E022, a Private Use codepoint, so not assigned any character by Unicode, but in many places online it is shown with that same Chinese-looking glyph as my Android app shows. Googling that just yields the private-use unicode character again, and Google Translate does not recognize it. It looks almost the same as https://en.wiktionary.org/wiki/%E5%82%91 with a tiny extra two-stroke thing at the top. But I suspect that's not relevant and this is just the result of some other characters stumbling drunkenly through incompatible character encodings.

The fact that these two are identical but for ending with “2” vs “3” suggests an auto-incrementing identifier of temporary variables of some sort. Sure hope those aren't file descriptors resulting from interpreting the backticks as shell command substitution and trying to locally execute what's inside them? 😬

To Reproduce Steps to reproduce the behavior:

  1. Make a PR with a couple of words surrounded by backticks in its title
  2. Set up a Slack integration for that repo, with “pending reviews” reminders activated.
  3. Wait for the “Pending review” reminder for this PR
  4. Observe the rendering of the reminder

Expected behavior I expect the PR title to render as “Remove data cropping from resolveArea and resolveRange” in Slack like it does on github.com.

Desktop (please complete the following information):

Smartphone (please complete the following information):

AlekSi commented 2 years ago

That's a very annoying issue

0x2b3bfa0 commented 2 years ago

Minimal reproducible example

Send a message containing a link whose text contains one or more code blocks. Contents for every code block in the link text will be incorrectly replaced with \ue022-\ue022{number}[^1] where {number} is incremented for every code block.

Request

curl https://www.slack.com/api/chat.postMessage \
--header 'Authorization: Bearer {token}' \
--data text='<http://address|text `with` backticks>' \
--data channel={channel}

Note Replace {channel} by a valid channel identifier and {token} by a valid token.

Result

text -1- backticks
74 65 78 74 20 ee 80 a2 2d ee 80 a2 31 2d 20 62 61 63 6b 74 69 63 6b 73

Additional information

This is a rendering issue on the client side; i.e. React. Messages arrive unaltered from the WebSocket connection up to the BaseMrkdwn.renderLink function.

[^1]: Unicode non-ASCII code points are represented using \u escape sequences.

0x2b3bfa0 commented 2 years ago

Reached out to Slack support, and they replied back stating that “it looks like there's an active discussion around this bug, and it is currently being worked on” and that they've planned release on our development environment “within the next couple days” although they can't give an estimation on when the fix will be deployed to production.

0x2b3bfa0 commented 2 years ago

Bonus bug

Sending back the \ue022 character displays a heart emoji ❤️

curl https://www.slack.com/api/chat.postMessage \
--header 'Authorization: Bearer {token}' \
--data text=$'\ue022' \
--data channel={channel}

Whole U+E000 to U+EFFF range 🤦🏼‍♂️

screenshot

Note that this Unicode range maps to softbank emoji codes on iamcal/emoji-data/emoji.json, the Slack emoji list.

0x2b3bfa0 commented 2 years ago

The fact that these two are identical but for ending with “2” vs “3” suggests an auto-incrementing identifier of temporary variables of some sort. Sure hope those aren't file descriptors resulting from interpreting the backticks as shell command substitution and trying to locally execute what's inside them? 😬

Apparently, they're being generated by BaseMrkdwn.nextKey() and have nothing to do with shell command substitution. :sweat_smile:

class BaseMrkdwn {
  nextKey() {
    return ++this.uid;
  }
  render() {
    this.uid = 0;
  }
}
0x2b3bfa0 commented 2 years ago

Update: the fix should be rolled out to 100% of users by 2022-07-04