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

[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

2. Click on Profile and Create a new Workspace

3. Click on the Workspace name and rename it to <my workspace(there should not be gap between < and the workspace name)

4. Notice the workspace name is saved on the #admin room
   

5. Go to workspace settings and add any word or letter after the previous name set and click on save to rename the workspace again.

6. Now on #admins room, send an image and click on it to preview (ex:
   )

Notice that the app crashes.

7. 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?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• 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

Recording.792.mp4

error-2023-06-15_11.21.34.mp4

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]

Triggered auto assignment to @Christinadobrzyn (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

[kushu7]

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.

App/src/components/AttachmentCarousel/index.js

Lines 176 to 177 in 8ba2234

	_.forEach(actions, (action) => htmlParser.write(_.get(action, ['message', 0, 'html'])));
	htmlParser.end();

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

App/src/components/AttachmentCarousel/index.js

Lines 158 to 161 in 8ba2234

	const htmlParser = new HtmlParser({
	onopentag: (name, attribs) => {
	if (name !== 'img' || !attribs.src) {
	return;

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

App/src/components/AttachmentCarousel/index.js

Lines 158 to 177 in 8ba2234

	const htmlParser = new HtmlParser({
	onopentag: (name, attribs) => {
	if (name !== 'img' || !attribs.src) {
	return;
	}
	const expensifySource = attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE];
	// By iterating actions in chronological order and prepending each attachment
	// we ensure correct order of attachments even across actions with multiple attachments.
	attachments.unshift({
	source: tryResolveUrlFromApiRoot(expensifySource || attribs.src),
	isAuthTokenRequired: Boolean(expensifySource),
	file: {name: attribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE]},
	});
	},
	});
	_.forEach(actions, (action) => htmlParser.write(_.get(action, ['message', 0, 'html'])));
	htmlParser.end();

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

Screen.Recording.2023-06-20.at.10.56.48.PM.mov

What alternative solutions did you explore? (Optional)

None

[Christinadobrzyn]

I can reproduce - adding External

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[jstortoise]

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:

App/src/libs/ReportActionsUtils.js

Lines 148 to 177 in 0bbf3fc

	function getSortedReportActions(reportActions, shouldSortInDescendingOrder = false) {
	if (!_.isArray(reportActions)) {
	throw new Error(`ReportActionsUtils.getSortedReportActions requires an array, received ${typeof reportActions}`);
	}
	const invertedMultiplier = shouldSortInDescendingOrder ? -1 : 1;
	return _.chain(reportActions)
	.compact()
	.sort((first, second) => {
	// First sort by timestamp
	if (first.created !== second.created) {
	return (first.created < second.created ? -1 : 1) * invertedMultiplier;
	}
	// Then by action type, ensuring that `CREATED` actions always come first if they have the same timestamp as another action type
	if ((first.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED || second.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) && first.actionName !== second.actionName) {
	return (first.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED ? -1 : 1) * invertedMultiplier;
	}
	// Ensure that `REPORTPREVIEW` actions always come after if they have the same timestamp as another action type
	if ((first.actionName === CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW || second.actionName === CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW) && first.actionName !== second.actionName) {
	return (first.actionName === CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW ? 1 : -1) * invertedMultiplier;
	}
	// Then fallback on reportActionID as the final sorting criteria. It is a random number,
	// but using this will ensure that the order of reportActions with the same created time and action type
	// will be consistent across all users and devices
	return (first.reportActionID < second.reportActionID ? -1 : 1) * invertedMultiplier;
	})
	.value();
	}

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

Screen.Recording.2023-06-30.at.7.54.43.AM.mov

• Working example

Screen.Recording.2023-06-30.at.7.58.02.AM.mov

What alternative solutions did you explore? (Optional)

None

[melvin-bot]

📣 @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:

1. 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.
2. 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.
3. 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]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~01eaf6919d6b87486a

[melvin-bot]

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

[jstortoise]

Proposal

Updated

[melvin-bot]

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

[Christinadobrzyn]

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

[Christinadobrzyn]

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

[rushatgabhane]

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]

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

[marcochavezf]

No update here, wrapping up other higher-priority items

[marcochavezf]

Swamped with other dailies, I will move back this to weekly

[CortneyOfstad]

@marcochavezf any updates? Thanks!

[CortneyOfstad]

@marcochavezf bump ^^^

[CortneyOfstad]

@marcochavezf bump ^^^

[CortneyOfstad]

@marcochavezf Per this post it looks like we're supposed to close anything that is not part of Waves.

To confirm, this doesn't fit any waves and can be closed, correct?

[aimane-chnaif]

This is "crash" so I think it should be fixed. If engineers have no time to fix in backend, we can just do frontend fix to avoid crash.

[CortneyOfstad]

@marcochavezf do you feel like you'll have time to launch a fix for this on the backend? Or should we default to doing something on the front end? Thanks!

[marcochavezf]

Oh sorry, I'm still focused on wave7, I think it still requires a backend fix. I can take a look quickly tomorrow

[CortneyOfstad]

No worries and thank you @marcochavezf!

[marcochavezf]

I'm re-evaluating the issue and it looks like the crash is not happening anymore, correct @aimane-chnaif? Also, I was analyzing the possibility of escaping the name in the backend and I noticed we don't have the same mechanism for workspace names as have for comments, so it would not be an easy change and I think it won't be worth do it if the app is not crashing.

Said that and since this is not part of a wave, I think we should close it @CortneyOfstad

[aimane-chnaif]

Crash now changed to Not found page. The root cause still remains

crash.mov

As it's not crash, let's close for now.
I will bump here to re-open when wave projects are finished and ready to focus these bugs again.

[CortneyOfstad]

Sorry for the delay here — was OoO for the holidays.

Sounds good and thanks @aimane-chnaif!