Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EnsureHtmlIsXml column cleaner regexp mistakenly matches too much #5012

Merged
merged 3 commits into from
Nov 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 7 additions & 15 deletions src/Education.UnitTests/XamlGenerator/RuleXamlBuilderSmokeTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,44 +52,36 @@ public void Create_CheckAllEmbedded()
Console.WriteLine("Checking xaml creation. Count = " + resourceNames.Count());

string[] failures;
using(new AssertIgnoreScope()) // the product code can assert if it encounters an unrecognised tag
using (new AssertIgnoreScope()) // the product code can assert if it encounters an unrecognised tag
{
failures = resourceNames.Where(x => !ProcessResource(x))
.ToArray();
}

failures.Should().BeEquivalentTo(new[]
{
// introduced in sonar-cpp 6.48
"SonarLint.VisualStudio.Rules.Embedded.cpp.S1232.json",
// introduced in dotnet analyzer 9.5
"SonarLint.VisualStudio.Rules.Embedded.csharpsquid.S4433.json",
// possible issues with EnsureHtmlIsXml regexp in rich rule description, need to investigate (same issue for all 5332 rules)
"SonarLint.VisualStudio.Rules.Embedded.cpp.S5332.json",
"SonarLint.VisualStudio.Rules.Embedded.c.S5332.json",
"SonarLint.VisualStudio.Rules.Embedded.csharpsquid.S5332.json",
"SonarLint.VisualStudio.Rules.Embedded.javascript.S5332.json",
"SonarLint.VisualStudio.Rules.Embedded.typescript.S5332.json",
// some issue with diff highlighting
"SonarLint.VisualStudio.Rules.Embedded.csharpsquid.S6640.json",
});
}

private static bool ProcessResource(string fullResourceName)
{
var xamlWriterFactory = new XamlWriterFactory();
var ruleHelpXamlTranslatorFactory = new RuleHelpXamlTranslatorFactory(xamlWriterFactory, new DiffTranslator(xamlWriterFactory));
var xamlGeneratorHelperFactory = new XamlGeneratorHelperFactory(ruleHelpXamlTranslatorFactory);
var ruleInfoTranslator = new RuleInfoTranslator(ruleHelpXamlTranslatorFactory, new TestLogger());
var staticXamlStorage = new StaticXamlStorage(ruleHelpXamlTranslatorFactory);

var simpleXamlBuilder = new SimpleRuleHelpXamlBuilder(ruleHelpXamlTranslatorFactory, xamlGeneratorHelperFactory, xamlWriterFactory);
var richXamlBuilder = new RichRuleHelpXamlBuilder(ruleInfoTranslator, xamlGeneratorHelperFactory, staticXamlStorage, xamlWriterFactory);

try
{
bool res = false;

var data = ReadResource(fullResourceName);
var jsonRuleInfo = LocalRuleMetadataProvider.RuleInfoJsonDeserializer.Deserialize(data);

Expand All @@ -104,7 +96,7 @@ private static bool ProcessResource(string fullResourceName)
Res(richXamlBuilder.Create(jsonRuleInfo, null));
res = true;
}

return res; // simple || rich should be true
}
catch (Exception ex)
Expand Down
20 changes: 18 additions & 2 deletions src/Rules.UnitTests/HtmlXmlCompatibilityHelperTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,28 @@ public void EnsureHtmlIsXml_ClosesBr()
HtmlXmlCompatibilityHelper.EnsureHtmlIsXml("Lalala<br> < br >Hi").Should().BeEquivalentTo("Lalala<br/> < br />Hi");
}

[TestMethod]
public void EnsureHtmlIsXml_ShouldNotMatchBrInText()
{
HtmlXmlCompatibilityHelper.EnsureHtmlIsXml("<p>some text with br in it <em>SASL</em>")
.Should()
.BeEquivalentTo("<p>some text with br in it <em>SASL</em>");
}

[TestMethod]
public void EnsureHtmlIsXml_ClosesCol()
{
HtmlXmlCompatibilityHelper.EnsureHtmlIsXml("Lalala<col span=\"2\" style=\"background-color:red\">Hi")
HtmlXmlCompatibilityHelper.EnsureHtmlIsXml("Lalala<col span=\"2\" style=\"background-color:red\">Hi < col > hello")
.Should()
.BeEquivalentTo("Lalala<col span=\"2\" style=\"background-color:red\"/>Hi < col /> hello");
}

[TestMethod]
public void EnsureHtmlIsXml_ShouldNotMatchColInText()
{
HtmlXmlCompatibilityHelper.EnsureHtmlIsXml("<p>Lightweight Directory Access Protocol (LDAP) servers provide two main authentication methods: the <em>SASL</em>")
.Should()
.BeEquivalentTo("Lalala<col span=\"2\" style=\"background-color:red\"/>Hi");
.BeEquivalentTo("<p>Lightweight Directory Access Protocol (LDAP) servers provide two main authentication methods: the <em>SASL</em>");
}

[TestMethod]
Expand Down
4 changes: 2 additions & 2 deletions src/Rules/HtmlXmlCompatibilityHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ internal static class HtmlXmlCompatibilityHelper
// the empty elements and replace them with elements with closing tags
// e.g. <br> => <br/>
// e.g. <col span="123"> => <col span="123"/>
private static readonly Regex CleanCol = new Regex("(?<element>(<col\\s*)|(col\\s+[^/^>]*))>",
private static readonly Regex CleanCol = new Regex("(?<element>(<\\s*col\\s*)|(<\\s*col\\s+[^/^>]*))>",

Choose a reason for hiding this comment

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

Is the \\s* needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would work in case of something like < col>. I would say it would work for now without it but I can't think of any scenario that this would be harmful.

RegexOptions.Compiled,
Core.RegexConstants.DefaultTimeout);

private static readonly Regex CleanBr = new Regex("(?<element>(<br\\s*)|(br\\s+[^/^>]*))>",
private static readonly Regex CleanBr = new Regex("(?<element>(<\\s*br\\s*)|(<\\s*br\\s+[^/^>]*))>",
RegexOptions.Compiled,
Core.RegexConstants.DefaultTimeout);

Expand Down