Fix XML escaping in GetManifestForRegisteredProvider#2351
Conversation
- Add using directive for Microsoft.Diagnostics.Utilities namespace - Escape all user-provided strings when writing to XML attribute values and text content - Properly handle string ID references: use raw IDs in message references but escape when writing id attributes - Ensures consistency between message="$(string.ID)" references and <string id="ID"/> entries Fixed locations: - enumName and valueName in valueMap/bitMap elements and string table - keyValue.Value in keyword elements and string table - task.Name in task elements and string table - keyValue.Value in opcode elements and string table Added comprehensive test for XML escaping using Microsoft-Windows-Ntfs provider that: - Validates manifest is well-formed XML by parsing with XmlDocument - Verifies all elements can be queried via XPath - Provides detailed error context if parsing fails
There was a problem hiding this comment.
Remove this assert. It's not necessary.
There was a problem hiding this comment.
Removed the unnecessary assert in commit 10a3cd6.
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
|
@copilot, the current code will take the actual string values and XML-escape them, but it also uses the value to make the stringId. If the value needs to be XML-escaped, then the string ID will also need to be modified. Please modify the code to keep the XML-escaping of the value, but when constructing string IDs, write and use a helper method that looks at the first and last characters of the valueName, and if they are '<' and '>', strip them off. Otherwise, XML-escape the ID. Please do this for all string IDs. |
…g IDs Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
…ackets Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
Simplified the approach to always XML-escape string IDs in commit 790ccd5. This avoids any potential collisions while ensuring the message references match the string table IDs. |
There was a problem hiding this comment.
Concatenate enumName and valueName first, then call MakeStringId on the result.
There was a problem hiding this comment.
Updated to concatenate strings before calling XmlUtilities.XmlEscape in commit a0cfabe.
There was a problem hiding this comment.
Rather than using this helper, just replace it with calls to XmlUtilities.XmlEscape. This will also allow for simplifying some cases above where we pass the same value multiple times.
There was a problem hiding this comment.
Removed the MakeStringId helper and replaced with direct calls to XmlUtilities.XmlEscape in commit a0cfabe. Also optimized to avoid escaping the same values multiple times.
…th optimizations Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
brianrob
requested review from
StephenMolloy,
cincuranet,
leculver,
marklio and
mconnew
as code owners
marklio
left a comment
marklio
left a comment
There was a problem hiding this comment.
It feels like this should be leaning on some of our built-in XML generation capability rather than rolling our own XML encoder. I can't think of a good reason to not use XmlWriter or XLinq here. This looks like an improvement in correctness, but it's really pretty impossible to say this is a "complete" fix.
|
Thanks @marklio. That makes sense. I'll merge this one and see how well copilot can convert this to using XmlWriter. |
Clean-up cases where when generating an XML-based manifest, we generate invalid XML.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.