Add support for inline image Markdown - attribute fix #661

kidroca · 2024-03-12T13:59:55Z

This PR is an optional followup for #658 if we'd like to patch a problem where image attribute content is being parsed to HTML as detailed in: #658 (comment)

Fixed Issues

$ Expensify/App#37246

Tests

Unit/Integration Tests Covering the Change:
- The changes are covered by unit tests added to ExpensiMark-HTML-test.js and ExpensiMark-Markdown-test.js. These tests validate that MD content designated for the alt attribute is escaped as HTML entities so it isn't converted to html tags by following rules
Tests Performed to Validate Changes:
- Ran all newly added tests and ensured they pass, verifying the correct conversion of markdown to HTML <img> tags and vice versa.
- Manually tested the parsing of markdown containing images in various formats and contexts to ensure the output matches expectations.
- Checked the handling of invalid image URLs to ensure they remain unchanged, as expected.

QA

QA Validation Steps:
- Test the handling of invalid Markdown image syntax, such as using a broken or incorrectly formatted URL (e.g., ![image](invalid-link)). These should not be parsed and ought to remain displayed as the raw Markdown text, without conversion or rendering as an image.

MonilBhavsar · 2024-03-14T14:12:17Z

lib/ExpensiMark.js

+
+        // A map of MD characters to their HTML entity equivalent
+        const entities = {
+            '*': '&ast;',


A backtick is missing here?

I've intentionally left out inline code and code blocks. These rules are applied first, converting anything that matches into HTML. Therefore, for input like:

![# fake-heading *bold* _italic_ ~strike~ `code`](https://example.com/image.png)

The result is:

<img src="<a href="https://example.com/image.png" target="_blank" rel="noreferrer noopener">https://example.com/image.png</a>" alt="# fake-heading &ast;bold&ast; &lowbar;italic&lowbar; ~strike~ <code>code</code>" />

This approach reveals a limitation in our rule application method, but I'm unable to cover it easily in the current PR

The aforementioned bug has been resolved by modifying the autolink pattern to recognize self-closing tags such as img. The absence of backticks is due to the fact that when escapeMarkdownEntities is executed, all backticks have already been transformed into <code> or <pre>

MonilBhavsar · 2024-03-14T14:12:51Z

Puneet, adding you as reviewer as you're assigned to the issue

MonilBhavsar · 2024-03-14T14:13:25Z

Looks like this commit is not verified caa6cf2

situchan · 2024-03-14T14:23:38Z

This should be on hold for #658, right?

marcaaron

Left a comment - not really a blocker but seems worth fixing now since we're in here.

marcaaron · 2024-03-18T21:02:12Z

__tests__/ExpensiMark-HTML-test.js

+
+    test('Image with alt text containing markdown', () => {
+        const testString = '![# fake-heading *bold* _italic_ ~strike~ [:-)]](https://example.com/image.png)';
+        const resultString = '<img src="https://example.com/image.png" alt="&num; fake-heading &ast;bold&ast; &lowbar;italic&lowbar; &#126;strike&#126; &lbrack;:-)&rbrack;" />';


hmm what about if the alt is:

[cool" onerror="alert('xss')"]

or something lol - I am trying hard to make the xss happen 😂

Thanks for the idea. The general HTML escaping logic, which is enabled by default, appears to cover this case. It results in something like the following:

Given the input:

![test" onerror="alert('xss')](https://example.com/image.png)

I attempted to exploit the alt attribute by prematurely closing it with a " to insert an additional attribute.

The outcome is:

<img src="https://example.com/image.png" alt="test" onerror="alert('xss')" />

However, since the quotes are properly escaped, the entire string is treated as part of the alt attribute, preventing any JavaScript execution.

I've added a test just in case

Nice thanks!

marcaaron · 2024-03-18T21:04:52Z

lib/ExpensiMark.js

+                name: 'image',
+                regex: MARKDOWN_IMAGE_REGEX,
+                replacement: (match, g1, g2) => `<img src="${Str.sanitizeURL(g2)}" alt="${this.escapeMarkdownEntities(g1)}" />`,
+                rawInputReplacement: (match, g1, g2) => `<img src="${Str.sanitizeURL(g2)}" alt="${this.escapeMarkdownEntities(g1)}" data-raw-href="${g2}" data-link-variant="labeled" />`


Escaping is good for the "markdown entities" - but why display them at all? The alt tag will show a bunch of junk if the image fails to load or they hover over it.

I considered keeping the text plain initially, but realized that it wouldn't allow us to revert to the original Markdown message from the HTML. This way, the original format is retained.

Regarding hover text, the alt attribute doesn't display text on hover. For that, Markdown has an extended syntax (![alt](src "title text")) that adds a title attribute to the resulting HTML. However, we don't support this feature, similarly to our approach with links.

If an image fails to load, the fallback is something like:

But NewDot and react-native-web don't display this fallback since they render a hidden <img> tag instead.

Ah hmm. Quite weird. Perhaps we can filter it before rendering i.e. not actually modify the underlying html field. But we can look into that in the App PR. Thanks for the explanation.

Related to: Expensify#658 (comment) Content intended for the alt attribute in images is being incorrectly parsed from Markdown to HTML if it contains MD special characters

kidroca · 2024-03-19T19:09:56Z

Looks like this commit is not verified caa6cf2

Fixed

This should be on hold for #658, right?

Yes, and #658 was just merged

I found a bug I've described here #661 (comment) and I'm currently trying to fix it

marcaaron · 2024-03-19T19:11:25Z

Thanks @kidroca!

Let us know when it's ready for review and I can give this a merge. 🙇

kidroca · 2024-03-19T21:03:11Z

I've addressed the bug related to the autolink rule by adjusting the pattern to exclude self-closing tags, like <img />. The pull request is ready for review. However, I'm cautious about declaring this the definitive fix for HTML within attributes. Given the current order of rule application, where code blocks and inline code precede others, HTML tags such as <code> ... </code> could still appear within the alt attribute.

This PR aimed to find a minimal solution to prevent these occurrences, but it couldn't fully achieve that, so we might consider looking for a different approach.

we could merge the current PR as it provides some coverage
we can turn to a broader audience for ideas
we can test a bit to make certain there are no undetected problems

marcaaron · 2024-03-19T21:08:59Z

Given the current order of rule application, where code blocks and inline code precede others, HTML tags such as ... could still appear within the alt attribute.

That sounds kind of edge case to me. Can you give a practical example of what a user might do and what weird behavior would result?

marcaaron · 2024-03-19T21:10:05Z

we could merge the current PR as it provides some coverage

I'd lean towards this unless there are any glaring major regressions or limitations.

kidroca · 2024-03-19T21:18:46Z

Given the current order of rule application, where code blocks and inline code precede others, HTML tags such as ... could still appear within the alt attribute.

That sounds kind of edge case to me. Can you give a practical example of what a user might do and what weird behavior would result?

The latest test has an example:

    test('No html inside the src attribute', () => {
        const testString = '![`code`](https://example.com/image.png)';
        const resultString = '<img src="https://example.com/image.png" alt="<code>code</code>" />';
        expect(parser.replace(testString)).toBe(resultString);
    })

As you can see this particular input would result in html inside the alt attribute
The part [`code`] is first converted to html [<code>code</code>] and later added to the alt attribute
It doesn't seem to lead to anything breaking, but preferably should be escaped as I'm not sure it's valid
We've we able to escape <strong> and alike because their rules run after the image...

marcaaron · 2024-03-19T21:25:03Z

Ah I see. Well, I think it's valid HTML - but definitely leaves us with semantically strange alt tags. Tbh, I don't think we need to worry about this right now. We can try to address the alt tag inconsistencies as a follow up.

marcaaron

I'm gonna merge this one because I think we've already got some funky alt tags with the escape characters and creating an inline code block link doesn't seem all that common. And it's only the alt tag that gets affected - which seems like a separate problem from allowing inline images.

tomekzaw · 2024-03-29T21:18:09Z

__tests__/ExpensiMark-HTML-test.js

+    test('No html inside the src attribute', () => {
+        const testString = '![`code`](https://example.com/image.png)';
+        const resultString = '<img src="https://example.com/image.png" alt="<code>code</code>" />';
+        expect(parser.replace(testString)).toBe(resultString);
+    })


@kidroca Shouldn't it be alt="<code>code</code>" instead of alt="<code>code</code>"?

It doesn't seem to lead to anything breaking, but preferably should be escaped as I'm not sure it's valid

This breaks Live Markdown library 😢 Not sure if < and > characters are supported in HTML attributes.

Ideally, the text inside the alt attribute should be alt="`code`" or use escaped entities. However, achieving this proved challenging without significant changes. The code rules execute first, generating a string like [<code>code</code>](...), and subsequent rules then move the text into the alt attribute.

I introduced this test to address a scenario where using the syntax ![code](https://example.com/image.png) incorrectly generated an <img> tag with a src attribute like <img src="<a href="..." />" />". This format, obviously, failed to load the image.

How does this impact Live Markdown functionality?

How does this impact Live Markdown functionality?

Currently, Live Markdown uses pretty basic algorithm for parsing HTML that alternates between finding < and then >.

The output <img src="..." alt="<code>...</code>" ... /> breaks the parser as the > from <code> gets matched instead of the (self-)closing > from img tag.

According to the HTML standard, > is not a valid character in HTML attribute:

Attributes have a name and a value. Attribute names must consist of one or more characters other than controls, U+0020 SPACE, U+0022 ("), U+0027 ('), U+003E (>), U+002F (/), U+003D (=), and noncharacters.

Is there a chance we could somehow escape the alt attribute?

Hi,
The text you're referring to in your quote is regarding the attribute name - an attribute name cannot contain the specified characters

According to the HTML standard, > is not a valid character in HTML attribute:

Attributes have a name and a value. Attribute names must consist of one or more characters other than controls, U+0020 SPACE, U+0022 ("), U+0027 ('), U+003E (>), U+002F (/), U+003D (=), and noncharacters.

Attribute values on the other hand, are less restricted as long as the value is wrapped in matching quotes. There's a specific text in the shared spec clearly stating that < and > are supported as long as the attribute value is wrapped in quotes.

The attribute value can remain unquoted if it doesn't contain ASCII whitespace or any of " ' ` = < or >. Otherwise, it has to be quoted using either single or double quotes.

In any case I'll look into it a bit further to see if there's anything I can do to mitigate the issue

The text you're referring to in your quote is regarding the attribute name

@kidroca oops, you're right, sorry for mistake!

In any case I'll look into it a bit further to see if there's anything I can do to mitigate the issue

I believe we could use a more complex HTML parser in Live Markdown.

cc @robertKozik who adapted ExpensiMark to Live Markdown.

But does reverting back when editing work? When I've tested it yesterday I think typing ```test``` as img name, and trying to edit it leads to test string. I may be wrong tho, as I'm not remember exactly nwm I was wrong 😃

Yeah your alternative solution will indeed solve our issues as we wouldn't have an < or > inside attributes. What is ETA for merging it?

We seem to be in a bit of a dead lock situation here. To advance my expensify-common PR, I need to demonstrate its effectiveness within the main branch of NewDot. Would the solution I proposed here be acceptable to you: Expensify/App#38952 (comment)?

Feel free to utilize this snapshot of expensify-common in your projects:

"expensify-common": "git+ssh://[email protected]/kidroca/expensify-common.git#3ad1c3322f4ec8999483955afe6f55c95e78172a",

This snapshot reflects the changes in my PR for expensify-common.

Incorporating it into your PR would resolve your issue

It would allow QA to test the MD Image feature.

But we should note to test this in the Tests section of your PR.

This in turn would allow approve the merge of the expensify-common PR

Letting you to finally switch back expensify-common to the Expensify's repo

I'll be ready to cover any follow-up work related to the MD image feature.

What are your thoughts?

It's a bit tricky because we're incorporating the expensify-common version inside react-native-live-markdown as well. So we would need to point to your branch within the new library version, which isn't my preferred approach. If possible, merging your expensify-common PR as it is would be great. If not, we can go with a patch containing your changes.

Here's how I envision the process:

Merge your expensify-common PR (if possible).

Publish a new version of react-native-live-markdown with both our and your fixes.

Create a joint PR for react-native-live-markdown and expensify-common in the E/App repo.

Let me just test your changes in live-markdown beforehand

@kidroca Apart from the one problem I've mentioned here everything looks great. I think we can proceed with the patch inside the react-native-live-markdown so it will unblock you - does it sound good for you?

kidroca requested a review from a team as a code owner March 12, 2024 13:59

melvin-bot bot requested review from MonilBhavsar and removed request for a team March 12, 2024 14:00

This was referenced Mar 12, 2024

Add support for inline image Markdown #658

Merged

HIGH: [Polish] Add support for inline image Markdown Expensify/App#37246

Closed

MonilBhavsar reviewed Mar 14, 2024

View reviewed changes

MonilBhavsar requested a review from puneetlath March 14, 2024 14:12

marcaaron previously approved these changes Mar 18, 2024

View reviewed changes

kidroca dismissed marcaaron’s stale review via 8eda663 March 19, 2024 11:14

kidroca force-pushed the kidroca/md-image-syntax-sanitize branch 3 times, most recently from 8c0b7a1 to eafc7a1 Compare March 19, 2024 11:31

Fix ExpensiMark: prevent html tags in alt attribute

6a95384

Related to: Expensify#658 (comment) Content intended for the alt attribute in images is being incorrectly parsed from Markdown to HTML if it contains MD special characters

kidroca force-pushed the kidroca/md-image-syntax-sanitize branch from eafc7a1 to 6a95384 Compare March 19, 2024 12:33

Merge branch 'main' into kidroca/md-image-syntax-sanitize

6838521

ExpensiMark: fix html inside the src attribute

76b8141

ExpensiMark: fix lint errors

7e45bc0

marcaaron approved these changes Mar 19, 2024

View reviewed changes

marcaaron merged commit ed80215 into Expensify:main Mar 19, 2024
5 checks passed

tomekzaw reviewed Mar 29, 2024

View reviewed changes

tomekzaw mentioned this pull request Mar 29, 2024

[HOLD for Payment] HIGH: [Live Markdown] [$500] Fix inline image syntax in live markdown Expensify/App#39275

Closed

puneetlath removed their request for review April 1, 2024 14:42

kidroca deleted the kidroca/md-image-syntax-sanitize branch April 3, 2024 12:36

bernhardoj mentioned this pull request Jul 30, 2024

[Ready for Payment][$250] Android - Chat - On sending attachment+text, on edit url is not shown as hyperlink Expensify/App#46491

Closed

6 tasks

bernhardoj mentioned this pull request Aug 15, 2024

Fix autolink isn't recognized when put before img markdown #782

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for inline image Markdown - attribute fix #661

Add support for inline image Markdown - attribute fix #661

kidroca commented Mar 12, 2024 •

edited

Loading

MonilBhavsar Mar 14, 2024

kidroca Mar 19, 2024

kidroca Mar 19, 2024

MonilBhavsar commented Mar 14, 2024

MonilBhavsar commented Mar 14, 2024

situchan commented Mar 14, 2024

marcaaron left a comment

marcaaron Mar 18, 2024

kidroca Mar 19, 2024 •

edited

Loading

marcaaron Mar 19, 2024

marcaaron Mar 18, 2024 •

edited

Loading

kidroca Mar 19, 2024

marcaaron Mar 19, 2024

kidroca commented Mar 19, 2024

marcaaron commented Mar 19, 2024

kidroca commented Mar 19, 2024

marcaaron commented Mar 19, 2024

marcaaron commented Mar 19, 2024

kidroca commented Mar 19, 2024

marcaaron commented Mar 19, 2024

marcaaron left a comment

tomekzaw Mar 29, 2024 •

edited

Loading

kidroca Apr 1, 2024

tomekzaw Apr 1, 2024

kidroca Apr 2, 2024

tomekzaw Apr 2, 2024

robertKozik Apr 3, 2024 •

edited

Loading

kidroca Apr 3, 2024 •

edited

Loading

robertKozik Apr 4, 2024

robertKozik Apr 4, 2024

robertKozik Apr 5, 2024

Add support for inline image Markdown - attribute fix #661

Add support for inline image Markdown - attribute fix #661

Conversation

kidroca commented Mar 12, 2024 • edited Loading

Fixed Issues

Tests

QA

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonilBhavsar commented Mar 14, 2024

MonilBhavsar commented Mar 14, 2024

situchan commented Mar 14, 2024

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidroca Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidroca commented Mar 19, 2024

marcaaron commented Mar 19, 2024

kidroca commented Mar 19, 2024

marcaaron commented Mar 19, 2024

marcaaron commented Mar 19, 2024

kidroca commented Mar 19, 2024

marcaaron commented Mar 19, 2024

marcaaron left a comment

Choose a reason for hiding this comment

tomekzaw Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertKozik Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

kidroca Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidroca commented Mar 12, 2024 •

edited

Loading

kidroca Mar 19, 2024 •

edited

Loading

marcaaron Mar 18, 2024 •

edited

Loading

tomekzaw Mar 29, 2024 •

edited

Loading

robertKozik Apr 3, 2024 •

edited

Loading

kidroca Apr 3, 2024 •

edited

Loading