[HOLD for payment 2023-02-08] [$250] Strip workspace name of html or handle validation php side reported by @kerupuksambel

[kavimuru]

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:

1. Open staging.new.expensify.com
2. Create new workspace
3. Change the name of the workspace with <b> </b> and save it

Expected Result:

Either the workspace name changed to after HTML-escaping (something like <b> </b> ), or returned error after the HTML tag got stripped and only the space remained

Actual Result:

The workspace name changed to no name

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.2.21-4 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/198730117-135e716c-ada7-4069-b153-df931cee5fc1.mp4

https://user-images.githubusercontent.com/43996225/198730150-69afdab6-b896-461c-945c-97c8914b4e39.mp4

Expensify/Expensify Issue URL: Issue reported by: @kerupuksambel Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1666949886613589

View all open jobs on GitHub

[melvin-bot[bot]]

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

[NicMendonca]

I am having trouble reproducing this. Asked in slack here.

[NicMendonca]

Okay, I got it. Am able to reproduce with using HTML spacing:

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[ctkochan22]

Looks related to https://github.com/Expensify/App/issues/12000

[trjExpensify]

Got it, so does it need to be internal as per this comment? https://github.com/Expensify/App/issues/12000#issuecomment-1296576574

[trjExpensify]

@ctkochan22 bump on the above question?

[s77rt]

Proposal

diff --git a/src/pages/workspace/WorkspaceSettingsPage.js b/src/pages/workspace/WorkspaceSettingsPage.js
index e778ecf89..feefb74c7 100644
--- a/src/pages/workspace/WorkspaceSettingsPage.js
+++ b/src/pages/workspace/WorkspaceSettingsPage.js
@@ -56,7 +56,7 @@ class WorkspaceSettingsPage extends React.Component {
         if (this.props.policy.isPolicyUpdating) {
             return;
         }
-        const name = values.name.trim();
+        const name = values.name?.replace(/(<([^>]+)>)/ig, '').trim();
         const outputCurrency = values.currency;
         Policy.updateGeneralSettings(this.props.policy.id, name, outputCurrency);
         Keyboard.dismiss();
@@ -64,7 +64,8 @@ class WorkspaceSettingsPage extends React.Component {

     validate(values) {
         const errors = {};
-        if (!values.name || !values.name.trim().length) {
+        const name = values.name?.replace(/(<([^>]+)>)/ig, '').trim();
+        if (!name || !name.length) {
             errors.name = this.props.translate('workspace.editor.nameIsRequiredError');
         }
         return errors;

https://user-images.githubusercontent.com/16493223/200658772-e73798d4-fca0-4843-9ae9-3c46fb5073c5.mp4

[ctkochan22]

Got it, so does it need to be internal as per this comment? https://github.com/Expensify/App/issues/12000#issuecomment-1296576574

I think i'm wrong about that. They seem like separate issues.

[rushatgabhane]

@s77rt can you please explain the proposal and why it'd fix the issue?

[s77rt]

@rushatgabhane

1. Strip any html tags (using regex (<([^>]+)>)/ig
2. Trim the output
3. Check if the name is defined and has a length (>0)

[s77rt]

Another Proposal (idea)

Instead of stripping the tags, we can encode them, so the user would have the ability to name the workspace something like <test> WokrspaceA. However the tags gets stripped in the API. As long as the API strips the tags (and not escape them) the first proposal is the best fit here.

[ctkochan22]

Seems like https://stackoverflow.com/questions/31515999/how-to-remove-all-html-tags-from-a-string has a better regex option. Strangely we don't do this anywhere else either.

I think we should also validate the end result on the PHP side, and have a properly caught error.

[ctkochan22]

@s77rt thanks for your proposal. Let me try doing a PHP side error handler first and see how that goes

[s77rt]

Updated Proposal

diff --git a/src/pages/workspace/WorkspaceSettingsPage.js b/src/pages/workspace/WorkspaceSettingsPage.js
index e778ecf89..a20413c68 100644
--- a/src/pages/workspace/WorkspaceSettingsPage.js
+++ b/src/pages/workspace/WorkspaceSettingsPage.js
@@ -56,7 +56,7 @@ class WorkspaceSettingsPage extends React.Component {
         if (this.props.policy.isPolicyUpdating) {
             return;
         }
-        const name = values.name.trim();
+        const name = values.name.replace(/<(.|\n)*?>/g, '').trim();
         const outputCurrency = values.currency;
         Policy.updateGeneralSettings(this.props.policy.id, name, outputCurrency);
         Keyboard.dismiss();
@@ -64,7 +64,7 @@ class WorkspaceSettingsPage extends React.Component {

     validate(values) {
         const errors = {};
-        if (!values.name || !values.name.trim().length) {
+        if (!values.name || !values.name.replace(/<(.|\n)*?>/g, '').trim().length) {
             errors.name = this.props.translate('workspace.editor.nameIsRequiredError');
         }
         return errors;

• The change is clearer this way
• The first proposal will return a literal "undefined" in case the name was undefined (due to using .replace with an undefined)
• Uses the suggested regex

https://user-images.githubusercontent.com/16493223/200758101-0d5e4597-ac18-4983-96fe-618250f657cf.mp4

[trjExpensify]

@ctkochan22 does this need the Internal label? Also, it's a Bug in the App repo, so we should keep this on Daily as we look to achieve BugZero for WAQ.

Can we confirm what the expected result is here and update the OP. Is this is?

• If the workspace name includes HTML tags with no value between them (i.e <b> <b>, treat this as "blank" and show the error message that a workspace name is required.
• If the workspace name includes HTML tags with a value between them (i.e <b> My workspace <b>), display My workspace as the workspace name. ✅ - this is working as expected

FWIW, I thought I'd test this out on Profile > FirstName/LastName fields to compare the behaviour and how we handle it, but we have a fall back on the primaryLogin when a name isn't set:

<b> value <b> ✅ (same scenario for the workspace name)

<b> <b> blank (thus reverting to the primaryLogin, which is okay in this use-case because a name is optional).

Thanks!

[ctkochan22]

Sorry @trjExpensify going to go back on what I said, and bring this back internal.

My reasoning is as a rule of thumb, we always should have error catches as far server-side as possible and in each layer. Auth is ideal. Then PHP. Then client-side.

I'm going to add a catch PHP side, and we'll return an error.

@trjExpensify How do you think we should handle this error? Do you happen to know what our convention is with errors we don't explicitly need users to "clear"

[melvin-bot[bot]]

A Contributor Manager will be assigned to issue payment via Upwork if we deploy an associated Pull Request to production. Per Contributing.md.

[trjExpensify]

Cool, switched the label.

@trjExpensify How do you think we should handle this error? Do you happen to know what our convention is with errors we don't explicitly need users to "clear"

Yep, let's follow the guidelines for form alerts: https://github.com/Expensify/App/blob/main/contributingGuides/FORMS.md#form-alerts

We largely have what we need here:

[ctkochan22]

The error will look a little different because its an error being returned form the php side.

Following how the code in the Form works at the moment.

[trjExpensify]

Got it. It would be better if that was in-line to the field in error really.

[ctkochan22]

Yeah, I'm trying to see if I can make that happen. Currently I only know how to do that as a pending error, meaning the user would have to click an "x" to get rid of it. Which I don't like.

There must be a way

[trjExpensify]

When there's a will, there's a way. 😉

[ctkochan22]

So we are discussing how to do inline here: https://expensify.slack.com/archives/C03TQ48KC/p1668128270722689

• Seems like a new PR could allow us to add inline errors - https://github.com/Expensify/App/pull/11465

In the meantime, this is the Web PR to get it fixed: https://github.com/Expensify/Web-Expensify/pull/35548

[rushatgabhane]

unassigning since internal

[s77rt]

@rushatgabhane may be external as well (client side validation)

[trjExpensify]

Okay awesome, so @ctkochan22 are we holding that PR on @youssef-lr's here?

[youssef-lr]

My PR will be ready today. But I think we should also add client side validation for this.

[trjExpensify]

👋 What's the latest on that PR, @youssef-lr @ctkochan22? It looks like it has conflicts now and its been in review for a week.

[ctkochan22]

Merged @youssef-lr 's PR. We should be able to test the web pr now. But we'll need to wait for the pr to hit staging before we test

[trjExpensify]

What's the latest on that one now as the issue has closed?

[youssef-lr]

@trjExpensify I just merged @ctkochan22's Web PR. I think we'll still need to handle validation in the frontend as well before we can close this out.

[trjExpensify]

Awesome. Thanks for the update!

[trjExpensify]

Okay, what's the next step here? Who's taking the PR on the front-end validation if that's deemed a requirement to close this one out?

[trjExpensify]

Bump @youssef-lr @ctkochan22?

[ctkochan22]

Was ooo last week for a family emergency. I can do it

[melvin-bot[bot]]

@youssef-lr, @ctkochan22, @trjExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[trjExpensify]

Given the conversation in the linked issue, are we done here? Maybe asked differently, what's the client-side verification for if we have it already on the forms?

[youssef-lr]

@trjExpensify I think what's left to implement here is stripping the name from HTML tags then verifying if it's empty, we're doing this in the backend and it needs to be done in the client as well. I think @ctkochan22 might have already implemented this and hasn't made the PR yet. The linked issue only shows validation of a completely empty name, e.g. ' ' vs '<b></b>' (both of these should be considered empty).

[trjExpensify]

Okay, I'm still slightly confused by that linked issue if it's not addressing the client-side validation follow-up required for this issue. I'm sure @ctkochan22 can confirm!

[trjExpensify]

I think @ctkochan22 might have already implemented this and hasn't made the PR yet.

@ctkochan22 can you confirm on this please?

[melvin-bot[bot]]

@youssef-lr, @ctkochan22, @trjExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[ctkochan22]

Yes, been playing catch up after 3 unexpected weeks of OOO. I'll have a PR out today for sure.

[trjExpensify]

Thanks, @ctkochan22! Is the PR up to link here?

[ctkochan22]

So currently this is what happens when you try to submit an html encoded name that results in spaces:

We might need to update the copy?

[trjExpensify]

Yeah, it might be a tad confusing as the user thinks they have defined a name don't they?

[melvin-bot[bot]]

@youssef-lr, @ctkochan22, @trjExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!