Panic in web.shouldSendEventToGuest

Description

Some details: Users on Windows are seeing this with desktop app v4.3.2 connecting to a 5.18.0 server. I'll work on replicating it once I get their Windows version

Server log:

Issue created from a message in Mattermost.

QA Test Steps

Updating user settings like profile picture, name etc. in a cluster setup with guest accounts should not crash other servers in the cluster.

Activity

Show:
Saturnino Abril
February 6, 2020, 7:18 AM

Tested and passed. No new test added.

Agniva De Sarker
January 14, 2020, 8:45 AM

Yeah, it’s not the cache issue. The comment above is the root cause.

And no, your change did not introduce it. It was already there with this line - canSee, err := webCon.App.UserCanSeeOtherUser(webCon.UserId, msg.Data["user"].(*model.User).Id).

 

Claudio Costa
January 14, 2020, 8:25 AM

This panic was initially reported for 5.18.0 so I don’t think the recent cache bug could be the cause since it's not going to be in a release until 5.20. I am more worried about my recent changes to WebSocketEvent in (fixing the data races) which touched these lines but that change also is going in 5.20. I think this problem was either uncovered or generated by . Let me know if you want me to take a deeper look into this.

Agniva De Sarker
January 14, 2020, 7:46 AM

Aha I got it

This happens during cluster message send. When message.ToJson() is executed, the json string is sent throughout the cluster. And then during de-serialization, we are unable to get the user map back.

This is where the culprit is:

 

Changing TestWebSocketEvent to :

 

reproes the crash.

Agniva De Sarker
January 14, 2020, 7:00 AM

I think this is sufferring from the same symptom as . (u *User) SanitizeProfile is being called every time before broadcasting the message. I still don’t see how the user field can get set to a map[string]interface{} instead of *model.User. But there is a chance that SanitizeProfile wipes out some fields and bad things happen. Or even worse, something else messes with the fields which makes the panic happen.

- WDYT ?

Done

Mana

5

Assignee

Agniva De Sarker

QA Assignee

Saturnino Abril

Reporter

Claudio Costa

Epic Link

None

Fix versions

Mattermost Team

Platform

Sprint

None

QA Testing Areas

Other (write in QA test steps)

GitHub Issue

None

Components

None

Severity

None