Escape '#' in OPC part names so signature references aren't truncated - #1063
Open
ChatchawanIllyes wants to merge 2 commits into
Open
Escape '#' in OPC part names so signature references aren't truncated#1063ChatchawanIllyes wants to merge 2 commits into
ChatchawanIllyes wants to merge 2 commits into
Conversation
System.Uri parses an unescaped '#' as introducing a URI fragment. Part names come straight from raw zip entry names, so a content file like "ab#c.txt" was silently split into path "ab" plus fragment "c.txt" when OpcPart built its Uri, corrupting every reference derived from it (the signature manifest Reference/@uri, relationship Target paths, and part identity/equality). Escaping the raw path before constructing the Uri fixes the parsing side, but UriHelpers.ToQualifiedPath() used UriFormat.Unescaped when serializing back to a string, which decoded the escape straight back into a literal '#' -- reproducing the same truncation once the value was written into the signed XML. Switching that one call site to UriFormat.UriEscaped keeps '#' encoded as %23 in the manifest/ relationship XML, while ToPackagePath() (used for actual zip lookups) is left untouched since it must keep matching raw entry names. Fixes dotnet#998
…mismatch CI caught this: Encoding.UTF8 via StreamWriter emits a BOM preamble, but the expected digest was computed over the string's bytes without one, so the digests never matched regardless of the production fix. Writing the same byte array to both the zip entry and the SHA-256 hash removes the discrepancy. The rest of the test already passed on Windows CI before this fix: the part was found intact (no fragment truncation), signing succeeded, and the Reference URI contained %23 with no raw '#' -- i.e. the actual fix in OpcPart.cs/UriHelpers.cs was already verified working. Only this test's own digest comparison was wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #998.
Root cause
OpcPartbuilds itsUridirectly from the raw zip entry name:System.Uritreats an unescaped#as introducing a URI fragment. So a content file named e.g.ab#c.txtgets silently split into pathab+ fragmentc.txtthe moment the part is constructed — corrupting everything derived from thatUri: the signature manifest'sReference/@URI, relationshipTargetpaths, and part identity/equality (OpcPart.Equals/GetHashCode).Escaping the raw path before constructing the
Urifixes the parsing side, butUriHelpers.ToQualifiedPath()— used to serialize theUriback into theReference/@URIand relationshipTargetXML attributes — usedUriFormat.Unescaped, which decodes the escape straight back into a literal#when building the string. So the escape alone doesn't survive into the signed XML; the same truncation would reappear at serialization time. This matches what the issue reporter described: a first attempt to percent-encode#didn't fix verification, because something downstream was still emitting an unescaped value.Fix
OpcPart.cs: escape the raw path (UriHelpers.EscapePartPath, new helper) before constructing both the part'sUriand its relationship-file locationUri.UriHelpers.cs:ToQualifiedPath()now usesUriFormat.UriEscapedinstead ofUriFormat.Unescaped, so#stays encoded as%23in theReference/@URIand relationshipTargetvalues it produces.ToPackagePath()is untouched — it's used for actual zip-entry lookups and must keep returning the raw, unescaped path to match real entry names.Testing
Added
ShouldSignPartWithOctothorpeInNametoOpcPackageSigningTests: signs a package containing a part namedab#c.txt, parses the resulting signature XML, and asserts theReference/@URIcontains%23(not a raw#) and that itsDigestValuematches the SHA-256 of the actual file contents (i.e. the reference resolves to the right part, not a truncated one).Disclosure on verification: I was unable to execute this repo's test suite locally.
Directory.Build.propspinsRuntimeIdentifier=win-x64repo-wide, so the builtSign.Core.dllis a genuine Windows-x64 binary that macOS refuses to load (FileLoadException: The assembly architecture is not compatible with the current process architecture) — not a missing-tool problem, a hard OS/CPU mismatch.Sign.Core.csprojitself does build cleanly (0 warnings/errors) with this change. The new test was written by tracing the fixed code path by hand againstSystem.Uri/UriBuilderdocumented behavior forUriComponents/UriFormat, and by manually checking every existing case inUriHelpersTests.csagainst the changed format flag (none use reserved characters, so none of their assertions change) — but it has not actually been run. Flagging this plainly rather than implying it passed CI locally; happy to iterate on any failures your Windows build turns up.