Matterbridge: Empty attachments with callbackId crash the Android mobile client

[patcon]

Describe the bug
It seems that we are storing callback_id, which is intended for attachment interaction, inside every message. This is not the intended use of the field, and so the Slack Android client is crashing, and it's leading to odd behavior in message threading.

Docs on usage of callback_id: https://api.slack.com/docs/interactive-message-field-guide

Full matterbridge symptoms is described here: https://github.com/42wim/matterbridge/pull/529#issuecomment-433639117

A fix is perhaps due upstream as well (and I've been in touch with Slack about it), but it would be nice if we could fix it here too :)

To Reproduce
Send a message from desktop or mobile from one channel in Slack A, view it in another channel in Slack B (works going between same or different Slack teams). View the relayed message on Android mobile. Try to turn it into a thread. (The other symptoms are only visible on the pending feature branch that allows threading)

Expected behavior
Not crashing. Proper threading and avatar display.

Screenshots/debug logs

Example of message: https://gist.github.com/patcon/a3952dd6afea89729811b4e510ccf628

Environment (please complete the following information):
Though I first noticed it on my translation branch, I've confirmed that it affects master

Additional context
None.

[patcon]

Ok, seems it gets added here, simply as a way to skip our own messages:
https://github.com/42wim/matterbridge/blob/master/bridge/slack/handlers.go#L311

The event payload in question:

[0006]  INFO slack:        &slack.MessageEvent{Msg:slack.Msg{Type:"message", Channel:"CDL6WH57E", User:"", Text:"testing", Timestamp:"1541439358.030900", ThreadTimestamp:"", IsStarred:false, PinnedTo:[]string(nil), Attachments:[]slack.Attachment{slack.Attachment{Color:"", Fallback:"", CallbackID:"matterbridge_bfg7uu1is9foj7mk5pp0", ID:1, AuthorID:"", AuthorName:"", AuthorSubname:"", AuthorLink:"", AuthorIcon:"", Title:"", TitleLink:"", Pretext:"", Text:"", ImageURL:"", ThumbURL:"", Fields:[]slack.AttachmentField(nil), Actions:[]slack.AttachmentAction(nil), MarkdownIn:[]string(nil), Footer:"", FooterIcon:"", Ts:""}}, Edited:(*slack.Edited)(nil), LastRead:"", Subscribed:false, UnreadCount:0, SubType:"bot_message", Hidden:false, DeletedTimestamp:"", EventTimestamp:"1541439358.030900", BotID:"B8UA3K02G", Username:"patcon (i18n-local-test-en@g0vtw):", Icons:(*slack.Icon)(nil), Inviter:"", Topic:"", Purpose:"", Name:"", OldName:"", Members:[]string(nil), ReplyCount:0, Replies:[]slack.Reply(nil), ParentUserId:"", Files:[]slack.File(nil), Upload:false, Comment:(*slack.Comment)(nil), ItemType:"", ReplyTo:0, Team:"T02G2SXKM", Reactions:[]slack.ItemReaction(nil), ResponseType:"", ReplaceOriginal:false, DeleteOriginal:false}, SubMessage:(*slack.Msg)(nil)}

I'm seeing a BotID in there, so perhaps we could get that from the token, and use that to skip messages, rather than using callback_id?

[patcon]

OK, dug back in the blame and found the originating issue: https://github.com/42wim/matterbridge/issues/219

Intermediate issue (for adding the uuid?): https://github.com/42wim/matterbridge/issues/429

Will read up on them tomorrow

[42wim]

Sorry, going to close this, it's a slack-client issue.
It's also used by matterircd and is needed for multiple matterircd/matterbridge clients on 1 slack.
Also so #264

[jheiselman]

I wonder if this is related to the issue that I reported to Slack. Messages that came from across the bridge cannot be used to start a thread in Slack. In my client, an error occurs. I passed it along to the Slack team to investigate.

[patcon]

Yep, this is it @jheiselman, threads can't be started on mobile.

@42wim I'm not so sure we can push this off on client -- we're using attachments in a way that's not in accordance with docs, leaving out required fields, and using them without any interactivity functionality: https://api.slack.com/docs/interactive-message-field-guide#attachment_fields

Anyhow, I've pushed a commit to the other branch that resolves this. It uses no extra API call (except one during boot) when using a bot token, and one RTM API call per message for legacy user tokens. And if we start storing bot users in the Users slice, then we don't need to make any extra API calls.

Would you consider re-openning to discuss?

[patcon]

Here's the commit in question: https://github.com/42wim/matterbridge/pull/529/commits/dce2bedc7c54b90a98ac094082233e6da303a1ec

Happy to remove it from that PR, but merging that PR without a fix makes the slack experience totally broken on mobile. (Invisible messages in threads, etc.)

[42wim]

Like I said this is needed for running multiple matterbridge and matterircd clients on the same slack instance.

Your solution doesn't fix this, matterircd will have the same botID as matterbridge, because they're using the same token/client.

About leaving out fields, I don't agree:

• fallback is an empty string, and actions are an empty array, this is according to the docs.
• slack doesn't drop it, and you can be sure it drops other stuff that's not according to their API

It's been a long search to uniquely identify clients, and the callback_id is the best there is at the moment.

If mobile crashes, it's obviously a slack bug (regression), because as can be seen in #264 they had fixed it.

About the PR, yes please remove that last commit and maybe it's best to keep it unmerged until slack has fixed the bug ?

[jheiselman]

I've linked this issue in my bug report to Slack HQ. Considering that the desktop client is still able to manage threading with bot sourced messages, they should be able to update the mobile client to have the same behavior. Or, at the very least, we'll find out if they will refuse to fix it given the nature of the issue.

[patcon]

Ok I'll revert and try some other options -- perhaps slight variations in attachment contents will suppress the error in the meantime. Thanks for catching me up Wim.

[patcon]

Was thinking a little more. I don't suppose you would be down to entertain this idea: Using the new ID lookup in the message threading code (#529), and skipping our own messages based on that lookup? Right now, the ring cache keys come back oldest first -- they're ordered by update time. If we reversed the slice, then "our" recent messages would always looped through first, and then doing a lookup for every message isn't too costly.

I know that technically this callback_id _should_ work, and it _not_ working is on Team Slack. But if it's been broken for an extended period twice in a year, it feels like an edge-case that might be nice to sidestep. Again, Android mobile becomes not just broken, but very misleading (hidden messages). Just wondering if we can be defensive in our approach

And apologies is I'm still missing something obvious about why the above won't work

[jheiselman]

I just got word back from Slack support. They are going to be releasing a new Android client that they say resolves this issue. No timeframe, but they did say it would be in the next release.

[patcon]

They just released on Nov 5, and their release notes seem like a new one comes every 2 weeks, so I'll make my calendar :)

[jheiselman]

I'm on their beta track, so I'll let you know if I see it fixed in any upcoming version.

[jheiselman]

Just got the new beta and this is fixed in it! Tested on a Discord > Slack bridge and was able to start a thread in Slacks from a message that originated in Discord.