-
Notifications
You must be signed in to change notification settings - Fork 432
Fix XML Equality Check by Comparing Parsed XML Structure Instead of Raw Strings #3166
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
base: dev
Are you sure you want to change the base?
Changes from all commits
20b1ab0
ac4d54d
3664823
7ef0393
1438d3a
658442d
a453cab
49c10d0
399f855
664fbc1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| using System.Security.Cryptography.X509Certificates; | ||
| using System.Text.Json; | ||
| using System.Text.RegularExpressions; | ||
| using System.Xml.Linq; | ||
| using Microsoft.IdentityModel.JsonWebTokens; | ||
| using Microsoft.IdentityModel.Protocols; | ||
| using Microsoft.IdentityModel.Protocols.OpenIdConnect; | ||
|
|
@@ -143,6 +144,7 @@ public class IdentityComparer | |
| { typeof(SignedInfo).ToString(), CompareAllPublicProperties }, | ||
| { typeof(SigningCredentials).ToString(), CompareAllPublicProperties }, | ||
| { typeof(string).ToString(), AreStringsEqual }, | ||
| { typeof(XDocument).ToString(), AreXmlsEqual }, | ||
| { typeof(SymmetricSecurityKey).ToString(), CompareAllPublicProperties }, | ||
| { typeof(TimeSpan).ToString(), AreTimeSpansEqual }, | ||
| { typeof(TokenValidationParameters).ToString(), CompareAllPublicProperties }, | ||
|
|
@@ -1171,6 +1173,147 @@ public static bool AreStringDictionariesEqual(Object object1, Object object2, Co | |
| context.Diffs.AddRange(localContext.Diffs); | ||
| return localContext.Diffs.Count == 0; | ||
| } | ||
| public static bool AreXmlsEqual(object xml1, object xml2, CompareContext context) | ||
| { | ||
| try | ||
| { | ||
| return AreXmlsEqual((XDocument)xml1, (XDocument)xml2, "xml1", "xml2", context); | ||
| } | ||
| catch (InvalidCastException ex) | ||
| { | ||
| context.Diffs.Add($"unable to cast {xml1.ToString()} and/or {xml2.ToString()} to xml document. Exception: '{ex}'."); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| private static bool AreXmlsEqual(XDocument xml1, XDocument xml2, string name1, string name2, CompareContext context) | ||
| { | ||
| var localContext = new CompareContext(context); | ||
| if (!ContinueCheckingEquality(xml1, xml2, localContext)) | ||
| return context.Merge(localContext); | ||
|
|
||
| if (ReferenceEquals(xml1, xml2)) | ||
| return true; | ||
|
|
||
| if (!CompareXmlElements(xml1.Root, xml2.Root, localContext)) | ||
| { | ||
| localContext.Diffs.Add($"'{name1}' != '{name2}', StringComparison: '{context.StringComparison}'"); | ||
| localContext.Diffs.Add($"'{xml1.ToString()}'"); | ||
| localContext.Diffs.Add($"!="); | ||
| localContext.Diffs.Add($"'{xml2.ToString()}'"); | ||
| } | ||
|
|
||
| return context.Merge(localContext); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Compares two XML elements for equality, ignoring order of attributes and child elements. | ||
| /// Ignore X509 certificate elements and attributes. | ||
| /// </summary> | ||
| /// <param name="elem1">The first XML element to compare.</param> | ||
| /// <param name="elem2">The second XML element to compare.</param> | ||
| /// <param name="localContext"></param> | ||
| /// <returns>True if the elements are considered equal, otherwise false.</returns> | ||
| private static bool CompareXmlElements(XElement elem1, XElement elem2, CompareContext localContext) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider utilizing localContext to add information about the differences found which could help with debugging. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| { | ||
| // Ensure both elements exist; if one is null while the other isn't, they are not equal. | ||
| if (elem1 == null || elem2 == null) | ||
| { | ||
| localContext.Diffs.Add($"one of the xml elements is null"); | ||
| localContext.Diffs.Add($"'{elem1.ToString()}'"); | ||
| localContext.Diffs.Add($"!="); | ||
| localContext.Diffs.Add($"'{elem2.ToString()}'"); | ||
| return false; | ||
| } | ||
|
|
||
| // Compare element names; if they are different, the elements are not equal. | ||
| if (elem1.Name != elem2.Name) | ||
| { | ||
| localContext.Diffs.Add($"xml element names are not equal, StringComparison: '{localContext.StringComparison}'"); | ||
| localContext.Diffs.Add($"'{elem1.Name.ToString()}'"); | ||
| localContext.Diffs.Add($"!="); | ||
| localContext.Diffs.Add($"'{elem2.Name.ToString()}'"); | ||
| return false; | ||
| } | ||
|
|
||
| // Ignore comparison for elements related to X509 certificates. | ||
| if (elem1.Name.ToString().Contains("X509")) | ||
| return true; | ||
|
|
||
| // Retrieve and order attributes by name to ensure order-independent comparison. | ||
| var attrs1 = elem1.Attributes().OrderBy(a => a.Name.ToString()).ToList(); | ||
| var attrs2 = elem2.Attributes().OrderBy(a => a.Name.ToString()).ToList(); | ||
|
|
||
| // If the number of attributes differs, the elements are not equal. | ||
| if (attrs1.Count != attrs2.Count) | ||
| { | ||
| localContext.Diffs.Add($"number of xml element attributes are not the same"); | ||
| localContext.Diffs.Add($"'{attrs1.Count}'"); | ||
| localContext.Diffs.Add($"!="); | ||
| localContext.Diffs.Add($"'{attrs2.Count}'"); | ||
| return false; | ||
| } | ||
|
|
||
| // Compare attributes | ||
| for (int i = 0; i < attrs1.Count; i++) | ||
| { | ||
| // Compare attribute names; if different, the elements are not equal. | ||
| if (attrs1[i].Name != attrs2[i].Name) | ||
| { | ||
| localContext.Diffs.Add($"the xml element attribute names are not equal, StringComparison: '{localContext.StringComparison}'"); | ||
| localContext.Diffs.Add($"'{attrs1[i].Name}'"); | ||
| localContext.Diffs.Add($"!="); | ||
| localContext.Diffs.Add($"'{attrs2[i].Name}'"); | ||
| return false; | ||
| } | ||
|
|
||
| // Ignore attributes related to X509 certificates. | ||
| if (attrs1[i].Name.ToString().Contains("X509")) | ||
| continue; | ||
|
|
||
| // Compare attribute values using the specified string comparison method. | ||
| if (!string.Equals(attrs1[i].Value, attrs2[i].Value, localContext.StringComparison)) | ||
| { | ||
| localContext.Diffs.Add($"the xml element attribute values are not equal, StringComparison: '{localContext.StringComparison}'"); | ||
| localContext.Diffs.Add($"'{attrs1[i].Value}'"); | ||
| localContext.Diffs.Add($"!="); | ||
| localContext.Diffs.Add($"'{attrs2[i].Value}'"); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // Retrieve and order child elements by name to ensure order-independent comparison. | ||
| var children1 = elem1.Elements().OrderBy(e => e.Name.ToString()).ToList(); | ||
| var children2 = elem2.Elements().OrderBy(e => e.Name.ToString()).ToList(); | ||
|
|
||
| // If the number of child elements differs, the elements are not equal. | ||
| if (children1.Count != children2.Count) | ||
| { | ||
| localContext.Diffs.Add($"number of xml element childrens are not the same"); | ||
| localContext.Diffs.Add($"'{children1.Count}'"); | ||
| localContext.Diffs.Add($"!="); | ||
| localContext.Diffs.Add($"'{children2.Count}'"); | ||
| return false; | ||
| } | ||
|
|
||
| // Recursively compare child elements. | ||
| for (int i = 0; i < children1.Count; i++) | ||
| { | ||
| if (!CompareXmlElements(children1[i], children2[i], localContext)) | ||
| return false; // Child elements mismatch | ||
| } | ||
|
|
||
| // If the element has no children, compare its values. | ||
| if (children1.Count == 0 && !string.Equals(elem1.Value.Trim(), elem2.Value.Trim(), localContext.StringComparison)) | ||
| { | ||
| localContext.Diffs.Add($"the xml element value are not equal, StringComparison: '{localContext.StringComparison}'"); | ||
| localContext.Diffs.Add(elem1.Value.Trim()); | ||
| localContext.Diffs.Add("!="); | ||
| localContext.Diffs.Add(elem2.Value.Trim()); | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| public static bool AreStringsEqual(object object1, object object2, CompareContext context) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| using System.Security.Cryptography; | ||
| using System.Text; | ||
| using System.Xml; | ||
| using System.Xml.Linq; | ||
| using Microsoft.IdentityModel.TestUtils; | ||
| using Microsoft.IdentityModel.Tokens; | ||
| using Xunit; | ||
|
|
@@ -114,7 +115,21 @@ public void WriteKeyInfo(DSigSerializerTheoryData theoryData) | |
| theoryData.Serializer.WriteKeyInfo(writer, keyInfo); | ||
| writer.Flush(); | ||
| var xml = Encoding.UTF8.GetString(ms.ToArray()); | ||
| IdentityComparer.AreEqual(theoryData.Xml, xml); | ||
|
|
||
| // Compare the original XML with the re-serialized XML. | ||
| // Parsing the XML strings into XDocument ensures that the comparison is based on | ||
| // structural and content equality rather than raw string differences (formatting, whitespace,...). | ||
| IdentityComparer.AreEqual(XDocument.Parse(theoryData.Xml), XDocument.Parse(xml), context); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding tests for different xml structures and null values to verify IdentityComparer behaves as expected when comparing XML. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi Sruke, thank you for the review. Since these tests focus on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree that creating a new file is better for organization. |
||
| } | ||
| catch (InvalidCastException ex) | ||
| { | ||
| context.Diffs.Add($"InvalidCastException: {ex.Message}"); | ||
| theoryData.ExpectedException.ProcessException(ex, context.Diffs); | ||
| } | ||
| catch (XmlException ex) | ||
| { | ||
| context.Diffs.Add($"XmlException: {ex.Message}"); | ||
| theoryData.ExpectedException.ProcessException(ex, context.Diffs); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.