Allow HTML in the link text

Try if this patch applies

Thanks for the issue!

To get this committed we definitely need to fix existing tests coverage (the 4 fails on patch #4). We would also need new test coverage to show that this feature works.

I'm also concerned with assessing:
1. Would this be disruptive to any existing sites?
2. Is this a good idea? Would it be wise to severely limit the allowed tags?

Perhaps, it would be more wise to create a toggle for this, making it disabled by default.
This shouldn't cause any disruption to existing websites, but could make the website vulnerable to XSS injections when combined to the Tokens module, so we should pay attention to that.

but could make the website vulnerable to XSS injections when combined to the Tokens module, so we should pay attention to that.

Based on the test results from #4 I don't think XSS is a problem. There's a test dedicated to this exact case:https://git.drupalcode.org/project/file_download_link/-/blob/2.0.x/tests...

That should have failed if the patch introduced a vulnerability. It would be worth doublechecking that locally and maybe adding a few positive assertions to that test.

Great to know!
As this patch seems useful on some situations, I'll work on my suggestion!

Committed this feature.
I've also created some automated tests to test XSS vulnerabilities using Token, but couldn't think of any scenario where a field could be saved without being sanitized first, other than creating a full_html formatting for a text field, like the body, where you could actually inject script tags, so I didn't include them on this commit.
To be extra safe, we could add an Xss::filterAdmin filter, after token replacement, for the link_html.

I see it's possible to add some extra asserts on existing tests.

I've thought about this a bit and I'm interested in getting this feature added.

However, I don't like the implementation in the patch or the MR. The patch is no good because it's breaking BC. There needs to be a new option added with a default value of false. That's what the MR does, which is good. However, the MR adds an entirely new link_html configuration as well. We can re-use the existing form element for the link text.

So what needs to happen here...

First, I don't like using MRs unless it's a really simple change that won't require any iterating. So let's close the MR and work with patches because that's what I prefer.

Second, we'll need to update existing Kernel tests and add new Kernel tests as well. That's a pain because the Kernel test are clumsy. They are among the first tests I ever wrote and they aren't as clean as they could be.

I just opened a new issue to refactor Kernel tests #3349706: Refactor file_download_link Kernel tests

I'm going to postpone this issue on that one. We can come back to this when that's done and it will be much easier to update and add the required coverage.

Third, we should update the IS to make it clear what the solution is here.

I refactored kernel tests just now so I am un-postponing this one.

Hi @astonvictor!

It looks like 3 years ago when I last looked at this, I considered this a breaking change.

What do you think? Is there anyone that could be relying on the current behavior where html tags render as plain text?

Hi @danflanagan8

not sure if someone wants to render tags as plain text.
e.g. <b>Download</b> should be rendered as bold instead of the text with tags.

+ updates are safe for XSS strings.

Ok, I think current me (as opposed to me three years ago) is ok with considering this more of a bugfix than a new breaking feature.

I agree that the test coverage is strong in showing that xss is not a danger.

Thanks!