double quote not allowed in URL

[Antasel]

Fixed Issues

$ Expensify/App#24367
Proposal: Expensify/App#24367 (comment)

Tests

Go to a chat and send following test strings and verify that double quote is not allowed in URL.

Test Strings

"om https://www.she.com/"
"om https://www.she.com"
"https://www.she.com/ end"
"https://www.she.com end"
https://www.she.com/path?test="123"
https://www.she.com/path?test="123/"
"https://www.she.com/path?test="123"
"https://www.she.com/path?test="123/"
https://www.she.com/path?test=123"
https://www.she.com/path?test=123/"
https://www.she.com/path?test=/"123"
https://www.she.com/path?test=/"123/"

Demo Video:

Screencast.from.13-9-23.07.02.08.webm

Autotest cases:

ExpensiMark-HTML-test.js new tests

test('Test Url, where double quote is not allowed', () => {
    const urlTestStartString = '"om https://www.she.com/"\n' +
        '"om https://www.she.com"\n' +
        '"https://www.she.com/ end"\n' +
        '"https://www.she.com end"\n' +
        'https://www.she.com/path?test="123"\n' +
        'https://www.she.com/path?test="123/"\n' +
        '"https://www.she.com/path?test="123"\n' +
        '"https://www.she.com/path?test="123/"\n' +
        'https://www.she.com/path?test=123"\n' +
        'https://www.she.com/path?test=123/"\n' +
        'https://www.she.com/path?test=/"123"\n' +
        'https://www.she.com/path?test=/"123/"';
    const urlTestReplacedString = '&quot;om <a href=\"https://www.she.com/\" target=\"_blank\" rel=\"noreferrer noopener\">https://www.she.com/</a>&quot;<br />' +
        '&quot;om <a href=\"https://www.she.com\" target=\"_blank\" rel=\"noreferrer noopener\">https://www.she.com</a>&quot;<br />' +
        '&quot;<a href=\"https://www.she.com/\" target=\"_blank\" rel=\"noreferrer noopener\">https://www.she.com/</a> end&quot;<br />' +
        '&quot;<a href=\"https://www.she.com\" target=\"_blank\" rel=\"noreferrer noopener\">https://www.she.com</a> end&quot;<br />' +
        '<a href=\"https://www.she.com/path?test=\" target=\"_blank\" rel=\"noreferrer noopener\">https://www.she.com/path?test=</a>&quot;123&quot;<br />' +
        '<a href=\"https://www.she.com/path?test=\" target=\"_blank\" rel=\"noreferrer noopener\">https://www.she.com/path?test=</a>&quot;123/&quot;<br />' +
        '&quot;<a href=\"https://www.she.com/path?test=\" target=\"_blank\" rel=\"noreferrer noopener\">https://www.she.com/path?test=</a>&quot;123&quot;<br />' +
        '&quot;<a href=\"https://www.she.com/path?test=\" target=\"_blank\" rel=\"noreferrer noopener\">https://www.she.com/path?test=</a>&quot;123/&quot;<br />' +
        '<a href=\"https://www.she.com/path?test=123\" target=\"_blank\" rel=\"noreferrer noopener\">https://www.she.com/path?test=123</a>&quot;<br />' +
        '<a href=\"https://www.she.com/path?test=123/\" target=\"_blank\" rel=\"noreferrer noopener\">https://www.she.com/path?test=123/</a>&quot;<br />' +
        '<a href=\"https://www.she.com/path?test=/\" target=\"_blank\" rel=\"noreferrer noopener\">https://www.she.com/path?test=/</a>&quot;123&quot;<br />' +
        '<a href=\"https://www.she.com/path?test=/\" target=\"_blank\" rel=\"noreferrer noopener\">https://www.she.com/path?test=/</a>&quot;123/&quot;';
    expect(parser.replace(urlTestStartString)).toBe(urlTestReplacedString);
});

[situchan]

Please also add test cases starting with some text, not directly starting with url.
Also ending with some text.
i.e.
"om https://www.she.com/"
"om https://www.she.com"
which are original test cases

[Antasel]

Please also add test cases starting with some text, not directly starting with url. Also ending with some text. i.e. "om https://www.she.com/" "om https://www.she.com" which are original test cases

it's done

[situchan]

Automated tests pass!

App tests well

Screen.Recording.2023-09-14.at.2.06.03.AM.mov

Screen.Recording.2023-09-14.at.2.14.21.AM.mov

@puneetlath all yours!

[gzqyl]

@Julesssss indeed, the test cases are not completely.
test url string without "?", "https://www.she.com/path?test="123" without "?" you will find it stops at wrong place.
check the follow images, you will know it does not fix the issue.

And it has different behavior with slack still.

[gzqyl]

check the images, the bug still exists

[situchan]

@gzqyl do you find any example url ending with = in path (without query)?

[Antasel]

@gzqyl it's not reasonable, quot; can't be escaped form for double quote, but `%22';
and on current staging app is not allowing quot;, more exactly semi-colon (;)

[gzqyl]

@situchan @Antasel If we just keep the consistent with server, and ignore user's thoughts, I have no words to say.
But, even if the example I showed is not a valid url, the user may think any strings wrapped by "", is a url start from http.
Just like slack, the url link displayed as the user think it is, while not our server think it should be.
I think we should respect the user experience, and deal with any stranger or invalid urls just by our backend, the backend logic user does not need to know.

[Antasel]

@gzqyl I am not sure what's user's thought. Users will just copy & paste url, then see what they want.
current' backend's logic is correct, and this PR's solution as well. check video
There when copy &paste the url of chrome input (just showing ", but it's escaped form %22), double quote became the escaped form %22 on the staging web.

Screencast.from.14-9-23.10.28.14.webm

[gzqyl]

@Antasel you just assume the user will input the correct url, when the user input a strange invalid url, the slack UI still considers the invalid url link as the user thought, while we will force the UI as our server think what should be correct.
If we think the incorrect url we could deal with it as our server does, the solution is ok, while if we think we should follow the user's thoughts, I think we should keep the invalid url link as the valid link.
Any logic about how to process with the url link, it should be server's work, and if any changes in the future, we could get the original input strings from users always, and just change the server's logic is ok to fix possible problems in the future.

[situchan]

@gzqyl in Expensify/App#17387, we matched frontend with backend even though backend php email validation is not perfect. There's no perfect rule in the world.
Same applies here. There's no perfect validation in such edge case urls. As you see, they differ per each popular chatting app.
Let's focus on other issues you can contribute to.
Thanks

[gzqyl]

at some extent, we did more work on the frontend, and get the unexpected UI, though we do not follow slack, the slack process the invalid url link as a valid url link, it will only stop at '"' not others like '=', so the strings start from http(s):// will be only ended at '"' even if the url is wrong like I showed before.
If we care much about the user, we should follow slack as it does a good job, if we care much about the backend, and ignore users, we can use the solution to display the url as our backend expected.
user could see the url style blue colored exactly as our backend will take it, while it may out of the user's expectation, the user may think the url link should be like slack does, only end with '"'.
okay, I just state my opinion about this solution, if the unexpected user cases does not matter, we do not care about the rare cases, it is ok.

[puneetlath]

I think this debate is a bit too theoretical. I think bringing the front-end parity with the back-end is good enough for now.

It's true that there might be even more improvements we could make, but I don't think they are currently worth our time to worry about. Thanks for the discussion!