[$4000] App crashes if you send an image immediately after renaming the workspace with <

kavimuru commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Go to staging dot on web chrome
Click on Profile and Create a new Workspace
Click on the Workspace name and rename it to <my workspace(there should not be gap between < and the workspace name)
Notice the workspace name is saved on the #admin room
Go to workspace settings and add any word or letter after the previous name set and click on save to rename the workspace again.
Now on #admins room, send an image and click on it to preview (ex: )

Notice that the app crashes.

App doesn't crash if workspace name is set to a normal name. Happens if you rename the workspace with '<' 2 times.

Expected Result:

Preview should work when a workspace is named with <

Actual Result:

App crashes if you send an image immediately after renaming the workspace with <

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?

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

Version Number: 1.3.29-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

https://github.com/Expensify/App/assets/43996225/92fc94a1-fbe1-4b4f-bb42-b9036cb79949

https://github.com/Expensify/App/assets/43996225/53b7632e-4cfe-40ca-bb19-b97ecc9c5137

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~012613bc6123fc7e67
Upwork Job ID: 1671273021780578304
Last Price Increase: 2023-08-15

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] 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
[x] 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.
[x] 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.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

kushu7 commented 1 year ago

Proposal

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

App crashes if you send an image immediately after renaming the workspace with <

What is the root cause of that problem?

we parse html to get images using open tag img and add into attachment but problem is here as we are writing string to htmlParser using write() we are appending messages over and over.

https://github.com/Expensify/App/blob/8ba223457b9a2e8d02f1c57d7c21fd7f0c523573/src/components/AttachmentCarousel/index.js#L176-L177

so in this condition when we rename workspace we get html as updated the name of this workspace from "TEST" to "<TEST" here we have open tag "<TEST" and we append next html that is containing image gets appending like updated the name of this workspace from "TEST" to "<TEST"<img src="https://www.expensify.com/chat-attachmen…sify-height="2532" data-expensify-width="1170" />

So we get openTag as test"<img {src: 'src' in onopentag it will get return early here

https://github.com/Expensify/App/blob/8ba223457b9a2e8d02f1c57d7c21fd7f0c523573/src/components/AttachmentCarousel/index.js#L158-L161

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

https://github.com/Expensify/App/blob/8ba223457b9a2e8d02f1c57d7c21fd7f0c523573/src/components/AttachmentCarousel/index.js#L158-L177 We are just looking for attachments. We can update this to parse only attachments and this way we can avoid unnecessary parsing.

_.forEach(actions, (action) => {
            if (!_.get(action, 'isAttachment', false)) return;
            htmlParser.write(_.get(action, ['message', 0, 'html']))
        });

Video

https://github.com/Expensify/App/assets/31707153/09d5cc06-faf2-4cfd-9706-f20f7b88145d

What alternative solutions did you explore? (Optional)

None

Christinadobrzyn commented 1 year ago

I can reproduce - adding External

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~012613bc6123fc7e67

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

jstortoise commented 1 year ago

Proposal

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

App crashes if you send an image immediately after renaming the workspace with <

What is the root cause of that problem?

< affects html parsing by mixing html tags. We need to escape html characters to fix this issue. e.g. in this case, expected attachment tag name is img but returned myworkspaces"<img

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

To solve this problem, I think we should escape html characters. e.g. > => >, < => <, & => &, " => ", ' => '. And also, this can occur script attack security issue. But which code should we update? There're several ways to avoid or fix this issue. But we can fix this easily on FE side by converting html when fetching from onyx storage This is what I suggest to do. Because, even we do a trick for attachment html parsing, there might be another issue occurred. So, by escaping html characters from onyx storage on fetching step, we can avoid these kinda issues. And I saw that all kinda data is fetched by onyx.getSortedReportActions(). We don't need to convert all html strings. We just need to convert Workspace updated comments

Current code: https://github.com/Expensify/App/blob/0bbf3fccfce0994ddf4fd9e95fb33dd9edbd1868/src/libs/ReportActionsUtils.js#L148-L177 We can add the following code after sorting:

        .map((action) => {
            if (action.actionName === CONST.REPORT.ACTIONS.TYPE.POLICYCHANGELOG.UPDATE_NAME) {
                action.message = action.message.map((message) => {
                    message.html = message.html.replace(/</g, i => `&lt;`).replace(/>/g, i => `&gt;`).replace(/&/g, '&amp').replace(/'/g, '&#39;').replace(/"/g, '&quot;');
                    return message;
                });
            }
            return action;
        })
        .value();

Why this needed?

Current issue occurs another issue. After changing workspace name to <myworkspace, if we change workspace name to <myworkspace__additional_str_here__, it doesn't show correctly on chat window.
Issued screenshot Above code will fix this issue too

https://github.com/Expensify/App/assets/28853367/a9774110-5dd8-46fb-a2a3-db52f7fc8e44

Working example

https://github.com/Expensify/App/assets/28853367/79bac725-1c26-4278-a484-eb52c5e49a92

What alternative solutions did you explore? (Optional)

None

melvin-bot[bot] commented 1 year ago

📣 @jstortoise! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

jstortoise commented 1 year ago

Contributor details
Your Expensify account email: jslegend119@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01eaf6919d6b87486a

melvin-bot[bot] commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

jstortoise commented 1 year ago

Proposal

Updated

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @marcochavezf (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

Christinadobrzyn commented 1 year ago

@rushatgabhane can you please review the proposals when you have time? Thank you!

Christinadobrzyn commented 1 year ago

Just a little nudge @rushatgabhane - let me know if you'd like to see more proposals!

rushatgabhane commented 1 year ago

We should reset htmlParser string or append
in each parse here

@kushu7 reset on every parse would be expensive. And adding a line break might lead to bugs. I agree with your root cause tho!

rushatgabhane commented 1 year ago

@jstortoise will the UI show "<" as < ?

jstortoise commented 1 year ago

No, it'll show <

jstortoise commented 1 year ago

@jstortoise will the UI show "<" as < ?

We use htmlparser, so < will be shown as < on UI. e.g. <span><test</span> will be shown as <test

kushu7 commented 1 year ago

adding a line break might lead to bugs.

I think no, as we are just parsing html to get img tags. Could you please tell me which type of bug can occur?

And here htmlParser is just used for getting img tags, nothing related to ui done here, so converting to escape characters. I don't see any value here. You can see full function

https://github.com/Expensify/App/blob/8ba223457b9a2e8d02f1c57d7c21fd7f0c523573/src/components/AttachmentCarousel/index.js#L158-L177

kushu7 commented 1 year ago

Proposal Updated https://github.com/Expensify/App/issues/21133#issuecomment-1599216847

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

rushatgabhane commented 1 year ago

g/etting to this today evening

jstortoise commented 1 year ago

Proposal

Updated

jstortoise commented 1 year ago

@rushatgabhane Updated proposal. Attached screenshots

Christinadobrzyn commented 1 year ago

Let us know how these proposals look or if you'd like to see more - @rushatgabhane

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 year ago

@marcochavezf @rushatgabhane @Christinadobrzyn this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

rushatgabhane commented 1 year ago

Hi @marcochavezf @Christinadobrzyn, I don't completely understand this issue and the proposals here. Could you please re-add the External label to assign a different C+? Thanks

jstortoise commented 1 year ago

@rushatgabhane This issue is that workspace name and chat message have different format. Workspace name is just text, not html. But chat message is html. So, if we change workspace name, api generates html from workspace name. If we input html string on workspace, it'll be applied to reports. It means that api doesn't handle html characters when renaming workspace name. That's the reason of this issue. As I mentioned on my proposal, there're 2 ways to fix this issue. 1st one is to fix on backend side by converting html characters. 2nd one is my proposal to fix on frontend side by converting html characters when getting report list. This will fix all possible issues. This proposal is not just trick. Iam sure this is the main solution

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

Christinadobrzyn commented 1 year ago

Welcome @aimane-chnaif! You were assigned as C+ based on https://github.com/Expensify/App/issues/21133#issuecomment-1621548393

I think there are a few proposals so let us know if you see any that look like a good fit or if you'd like to see more proposals!

jstortoise commented 1 year ago

@Christinadobrzyn why my proposal declined?

shubham1206agra commented 1 year ago

@Christinadobrzyn @aimane-chnaif I think you need to sanitize all input fields as it might be security threat by inserting script tag inside request. If possible, do the same on the backend side too.

shubham1206agra commented 1 year ago

And there's another problem I can set workspace name to <> but I can't set it to <lol>. Is this intentional?

Christinadobrzyn commented 1 year ago

@Christinadobrzyn why my proposal declined?

I don't think we've decided on a proposal - I added @aimane-chnaif as the C+ taking over for @rushatgabhane based on https://github.com/Expensify/App/issues/21133#issuecomment-1621548393

@aimane-chnaif will be reviewing the proposals.

Christinadobrzyn commented 1 year ago

not overdue - we're still reviewing proposals!

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 year ago

@marcochavezf @Christinadobrzyn @aimane-chnaif this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

Christinadobrzyn commented 1 year ago

hey @aimane-chnaif let us know if any of these proposals will work or if you'd like to see more. Thanks!

aimane-chnaif commented 1 year ago

@Christinadobrzyn yes, I'd like to see more proposals.

aimane-chnaif commented 1 year ago

There's another bug which has the same root cause.

I think this should also be fixed here.

aimane-chnaif commented 1 year ago

I think the ideal solution is to escape specific characters like <, > when save as html. ref: https://stackoverflow.com/questions/7381974/which-characters-need-to-be-escaped-in-html

shubham1206agra commented 1 year ago

And there's another problem I can set workspace name to <> but I can't set it to <lol>. Is this intentional?

@aimane-chnaif Can you confirm this?

aimane-chnaif commented 1 year ago

And there's another problem I can set workspace name to <> but I can't set it to . Is this intentional?

I don't see any reason why it's intentional. Should be bug I think. Btw, all of these workspace names which include html tags are edge cases and couldn't be easily caught and handled in initial implementation.

shubham1206agra commented 1 year ago

And there's another problem I can set workspace name to <> but I can't set it to . Is this intentional?

I don't see any reason why it's intentional. Should be bug I think. Btw, all of these workspace names which include html tags are edge cases and couldn't be easily caught and handled in initial implementation.

Then let me see the issue with the validator then And how can we escape tags of all types.

Christinadobrzyn commented 1 year ago

I'm going to be ooo until July 31st so going to unassign and assign a new teammate.

@CortneyOfstad At this time, we're receiving and reviewing proposals. I'll take this when I'm back if it's still open.

melvin-bot[bot] commented 1 year ago

Current assignee @Christinadobrzyn is eligible for the Bug assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

Expensify / App