[HOLD for sobit] [$1000] Able to by pass 'Admins only' functionality using :emoji in multiple lines

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to staging dot on web chrome and login with User A
2. Create a new workspace and invite User B
3. Login with User B on another web chrome (to reproduce the bug, place the windows side by side)
4. From User A and User B, go to announce channel
5. From User A, go to top header, go to 'Who can post' (Please do not click any option now)
6. From User B, type any short message (do not click on send).
7. From user A, select 'Admins Only' and see that on The User B side, the chat composer disappears
8. But now, again select the 'All members' on User A side. Whereas on User B side, type in :smile: (first colon and emoji name), and then copy the ":smile:"
9. Then, hold cmd+V to automatically paste it multiple times in a row back-to-back, until you have 5+ rows of the emoji pasted.
10. Do not click Send
11. From User A, again select 'Admins Only' and notice that on User B side, the chat composer doesn't disappear and we are able to send other messages too.

Expected Result:

A user should not be able to type in the workspace after 'Admins only' is selected.

Actual Result:

A user is able to type in the workspace chat after the 'Admins only' setting is selected, by using ":emoji:" pasted in succession for multiple rows. This introduces so type of lag that allows the user to keep typing

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.28-3
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

error-2023-06-05_19.15.24.mp4

Recording.978.mp4

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1685972438902859

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @michaelhaxhiu (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[DanutGavrus]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Able to by pass 'Admins only' functionality using :emoji in multiple lines

What is the root cause of that problem?

Inside ReportActionCompose.js, we call updateFrequentlyUsedEmojis each time we edit the message, each starting a network call. The back-end sends a notification back to the current user for each emoji added, and the notification that the room has changed to admins only comes with a delay. Thus, adding more emojis in chat => easier to reproduce the bug.

What changes do you think we should make in order to solve the problem?

We should debounce the updateFrequentlyUsedEmojis method same as we do with other methods inside ReportActionCompose.js. With these changes:

the bug is no longer reproducible, no matter how many emojis are added in chat.

EDIT: 1000 may be too much for the debounce, I see that the bug is not reproducible with 75 either, so I think we should go with a lower value, 75 or near here.

[DanutGavrus]

If you can't reproduce this bug from the first try, you may also try this:

1. CTRL+C smile:
2. Keep CTRL+V until 6-7 rows are filled in chat on userB
3. Quickly change to "only admins" from userA
4. Come back and start writing other messages on userB

More rows at step 2 = increased delay for the "only admins" change to take effect = easier to reproduce

[michaelhaxhiu]

looking!

[michaelhaxhiu]

Hm this isn't working for me and I'm not sure why

tried this :smile :smile :smile.... format too and no luck

[DanutGavrus]

@michaelhaxhiu if you do not input enough rows of smile: the time window in which you are able to by-pass admins-only is short. Have a look at this:

bypass_admins.mp4

[michaelhaxhiu]

So like after I delete the 6+ rows of smile: then try to rapidly type messages in the workspace #announce?

[DanutGavrus]

Something like that. Each smile: adds a small delay of lets say x. 10 smile: would be 10x, and so on. Thus, 6 rows grant a very large time window. If usersB changes his room to admins only while x time has not passed, the change does not take effect for you(userA), so you may keep typing other messages. By doing so, the change will never take place for you. By waiting longer for x time to pass, it will eventually take effect for you too.

The back-end however, knows about the admins-only, so the messages will not appear to other members, but as I see, no error appears when sending them, and they will remain in your chat afterwards. I still have the messages I've sent in the video in #announce and for me, they appear like they were sent, still no error message appears.

Other small problem is that back-end will still let all other users see when you are typing even if it knows about the admins only, as you may see in the video.

[michaelhaxhiu]

I just tried again and I can't reproduce it still! Sorry.

I'm not sure why. If you or @priya-zha can provide more exact instructions I will be glad to try it.

https://recordit.co/aIyzLMvj0S)

[DanutGavrus]

I think that the last try may look like this:

Have 2 apps opened side by side like in your previous video. On left I see you have the administrator user(userA let's say) and on right you have a user which is not an admin(userB):

1. Copy this text smile: as is;
2. While you have the Settings opened on userA with the current selection of "All members", go to userB, clear the text input, then just keep the paste shortcut pressed for some time(CTRL+V on Windows) until 7-8 rows are filled on right;
3. Right after letting go of the paste shortcut, rapidly change from 'All members' to 'Admins only' on left in userA;
4. Right after changing to 'Admins only', rapidly clear the text input and start typing&sending other messages on right for userB.

[michaelhaxhiu]

Yes I have tried this again just now step by step. I still can't reproduce.

Check it out - https://recordit.co/mU1Br5A7hM

[DanutGavrus]

@michaelhaxhiu I see that you have white spaces in the video, maybe smile: got copied instead of smile:? I think that if you do the exact same thing, but without the white space, it might actually work this time.

[michaelhaxhiu]

Ah ok thanks @DanutGavrus. That did help me reproduce.

So, I see there is a lag of time before the chat function is blocked, and we return an error to userB on the messages that appear to be sent (but failed) after-the-fact.

https://recordit.co/zjytErISsQ

I guess the root bug here is that we should block the chat input more quickly (e.g. instantly after the settings change from Members -> Admins)? Is that right?

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01f42e4aa901fd1f2a

[melvin-bot]

Current assignee @michaelhaxhiu is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

[DanutGavrus]

@michaelhaxhiu Glad I was able to help you reproduce.

I was not aware of those error messages, I will check again tomorrow as it's 0:30am here, and come back to you with a reply.

From what I remember, the fact that the chat was updated to Admins only comes as a notification from the back end to the app. But, each written emoji in the app, makes a call to the back end, which also triggers a notification back to the app, thus creating a queue of notifications which the app has to process one by one. Admins only notification comes at the end of the queue, thus the app does not have the time to process it.

Maybe you could also look above at my proposal when you have the time?

[michaelhaxhiu]

No problem! Talk tomorrow :) And yes your proposal is first to be reviewed :). Thanks for your continued involvement here.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.37-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: UpdateFrequentlyUsedEmojis API spamming #21772

If no regressions arise, payment will be issued on 2023-07-14. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:
• [@sobitneupane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@sobitneupane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@sobitneupane] Determine if we should create a regression test for this bug.
• [@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@michaelhaxhiu] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[grgia]

@sobitneupane @michaelhaxhiu bump on BZ checklist / payment here

[michaelhaxhiu]

Sorry for delay, payment is as follows:

• @priya-zha External issue reporter $250
• @tienifr Contributor that fixed the issue $1000
• @sobitneupane Contributor+ that helped on the issue and/or PR $1000 - paying via newdot

[michaelhaxhiu]

@sobitneupane are you being paid lately via Upwork or newDot?

[michaelhaxhiu]

Invited Priya & Tien to the job in Upwork

[sobitneupane]

@michaelhaxhiu I will request payment via newDot.

[michaelhaxhiu]

Ok cool, got it! Can you work on the checklist as the next step?

[michaelhaxhiu]

Update: Contributor & bug reporter are paid. Just waiting for sobit to confirm payment was made and complete the checklist.

[michaelhaxhiu]

@sobitneupane have you requested payment yet? Can you work on the checklist today?

[sobitneupane]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#6498

• [@sobitneupane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

The issue was introduced while adding the functionality of frequently used emojis 2 years back.

• [@sobitneupane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

It's an edge case and very difficult to reproduce. I don't think this could have been caught in PR review.

• [@sobitneupane] Determine if we should create a regression test for this bug.

No. It is an edge case and might be difficult to test.

[sobitneupane]

Requested payment on newDot.

[michaelhaxhiu]

cool, all set - closing.

[JmillsExpensify]

Reviewed details for @sobitneupane. This is accurate based on summary from Business Reviewer and approved for payment in NewDot.