Fix case-insensitive URI matching - #118574
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where package part lookups failed when URI part names differed only by ASCII case. The fix updates the URI comparison logic to perform case-insensitive matching instead of case-sensitive matching.
Key changes:
- Updates the
ValidatedPartUri.Comparemethod to use case-insensitive string comparison - Adds a comprehensive test to verify that part names with different cases are treated as equivalent
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.IO.Packaging/src/System/IO/Packaging/PackUriHelper.cs | Updates URI comparison logic from case-sensitive to case-insensitive using StringComparison.OrdinalIgnoreCase |
| src/libraries/System.IO.Packaging/tests/PartPieceTests.cs | Adds test case to verify that parts with different case URIs are treated as the same part |
rzikm
left a comment
There was a problem hiding this comment.
Did you try to verify with the original repro?
rzikm
left a comment
There was a problem hiding this comment.
LGTM, but I would prefer @ericstj to take a look as well.
@alinpahontu2912 did you also check all other usages of this type which may rely on equality, and validated the fix against the original repro from the issue?
|
Hey @rzikm, the original repro case now works, but I haven't checked for other usages |
|
The change looks ok - can you think of any side-effects of this that folks might notice? Anything where some behavior might have crept in while this bug was present that would be broken with the fix? The best I can think of is some zip that had entries that only differ in case and we somehow consumed that before and can't or will consume differently after the fix. Think through and either manually test or add tests. If you find a behavior change, consider documenting it. We don't have a lot of testing of this component in dotnet/runtime - we try not to touch it too much. It might be interesting to try consuming it in WPF and/or OpenXML to see if it works well. |
|
Hey @rzikm @ericstj can youcheck this again? I checked for other usages of the ValidatedUriPart class and there aren't that many, all are inside the packaging library. To me it looks like these changes are not affecting something else. I also read through the ECMA documentation, and found: 6.2.2.3 Part name equivalence and integrity in an abstract package Equivalence of part names shall be determined by ASCII case-insensitive matching. [...] EXAMPLE 1 If an abstract package contains a part named "/a", the name of another part in that abstract package cannot be "/a" or "/A"., so entries that differ only in case should not be allowed anyway. I added a new test to check for this. |
ericstj
left a comment
There was a problem hiding this comment.
This change looks good, thank you for adding the test.
One last case to consider - suppose a zip already has entries that differ only in case - what happens on read before and after the change?
Your added test proves that folks can't add a part through IO.Packaging API, but it's possible that the zip itself might have the problem.
Mainly interested in exploring potential changes/breaking behavior - even for a "non-compliant" package - as it could be interesting to document or tweak the implementation if it behaves poorly. For example - if a zip would load and work before, but now throws on open, we might want to address it. If it loads and just gives slightly different behavior when using the part API - we would probably just document "don't do that".
|
Hey @ericstj, thanks for the review and the comment. I checked locally, and non-compliant packages that previously worked will now throw errors when trying to open them. From what I understand, this throw is expected, since such packages shouldn’t work in the first place. So while this is technically a breaking change, should we just document that the behavior now aligns with the spec and that packages with parts differing only by case are not supported? Is it okay to merge these changes as-is, and if we document the behavior change, where do we write about it ? |
|
Would those packages open before - I guess on .NET 5 - .NET 9? What happens on .NETFramework? Folks do weird things like misuse the Packaging API as a general purpose ZIP API. Very old code did this from before we added Zip support in .NETFramework 4.x. So while we might have a good argument for fixing this, the end user who is broken might not care about that argument. Here are some things to consider: If we're comfortable with the answers to all these, we can take it as is and file a breaking change doc. I'm fine with that as an early change in .NET 11 to get this in previews and have plenty of bake time. If we were to take this to ask-mode for .NET 10, I would hope we have a very good case against this breaking many customers. |
|
Added When you commit this breaking change:
Tagging @dotnet/compat for awareness of the breaking change. |
|
Hey, I checked how this behaves in .NET Framework and URI matching there is already case-insensitive, so this change aligns with that behavior. Also, if you try to create two parts that differ only by casing (/part and /PART) through the Packaging API, it already throws (Cannot add part for the specified URI because it is already in the package.), so such a package can’t be authored legitimately. |
|
Did you test the opening of the bad zip in .NET Framework - the one that opened in |
|
Hey @ericstj, thanks for the comment. Yep, I tested the bad ZIP in .NET Framework. Package.Open throws on the case-conflicting parts in all versions, while opening as a ZIP archive works since ZIP doesn’t enforce OPC rules. Opened a breaking change issue here: dotnet/docs#48174 |
|
That's good, that means "bad behavior" was only introduced on core where we have better APIs for folks to use. Much less likely that folks have expectation that this works. Thank you for following up. |
|
@alinpahontu2912 Just a reminder that the breaking change doc issue still needs to be filed for this. The automated comment has instructions for what's expected, and you can review other breaking change issues in the dotnet/docs repo for reference examples if needed. Thanks and nice work! |
|
Removing |
The package lookup fails when part names differ only by ASCII case, because the comparer is doing a case-sensitive match.
This PR updates the URI comparer logic to validate equal case-insensitive strings.
Fixes #112783