Skip to content

Conversation

@slang25
Copy link
Contributor

@slang25 slang25 commented Apr 3, 2023

This library is great! I found that it doesn't HTML escape single quotes, so here's a small PR...

@ltrzesniewski
Copy link
Owner

Thanks! I thought single quotes didn't really need to be escaped. Did you find a case where it's an issue?

Copy link
Owner

@ltrzesniewski ltrzesniewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also other places in the code where HTML escaping is performed. Those would need to be updated as well.

@slang25
Copy link
Contributor Author

slang25 commented Apr 3, 2023

So I coded this up late last night, hence the mistakes 😄
What led me to create this PR is that my test started failing after switching from RazorLight, it is expecting ' for '.
From some research, it should be encoded to either ' or ' (this is the hex representation), however I chose the hex implementation as it's what the other razor libraries do, and that's what the System.Text.Encodings.Web.HtmlEncoder.Default implementation does:

System.Text.Encodings.Web.HtmlEncoder.Default.Encode("foo's").ShouldBe("foo's");

So copying this would lead to less surprises for future adopters.

@ltrzesniewski
Copy link
Owner

Ok, thanks for the explanation. I'll review this in more detail tonight, but I can already say that:

  • You didn't implement the #else case in HtmlHelper
  • HtmlTemplate.Write needs the same changes

@slang25 slang25 force-pushed the improve-html-encode branch from e4d37e8 to 181e40e Compare April 3, 2023 12:43
@slang25 slang25 force-pushed the improve-html-encode branch from 181e40e to a0c9930 Compare April 3, 2023 12:43
@slang25
Copy link
Contributor Author

slang25 commented Apr 3, 2023

Thanks for catching those @ltrzesniewski, changes made

while (true)
{
var idx = valueSpan.IndexOfAny("&<>\"");
var idx = valueSpan.IndexOfAny("&<>\"`'");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That backtick looks wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no excuse this time 🤦‍♂️

@slang25
Copy link
Contributor Author

slang25 commented Apr 3, 2023

I'm thinking this should also handle chars outside of the ASCII range, let me take a quick look at that too

@ltrzesniewski
Copy link
Owner

I don't think that's really required, everything handles Unicode now.

@ltrzesniewski ltrzesniewski merged commit 52e5d47 into ltrzesniewski:main Apr 3, 2023
@ltrzesniewski
Copy link
Owner

I released this in v0.4.3 - thanks! 🙂

@slang25 slang25 deleted the improve-html-encode branch April 3, 2023 20:57
@slang25
Copy link
Contributor Author

slang25 commented Apr 3, 2023

Amazing, thanks @ltrzesniewski

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants