C#: Fix an incorrect merge conflict resolution. #312
Merged
Conversation
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
GOOD since it didnt normalize the path after the concat. The logic added in 4dfa886 was flawed since `Path.Combine(x, y)` is not a normalized path even when `x` is normalized (since `y` may contain `..` segments).
dilanbhalla
approved these changes
Dec 18, 2025
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.
The
TaintedPathQuery.qllhas a history of incorrect merge conflict resolution, and yesterday I contributed to that history when trying to resolve conflicts for our 2.23.8 upgrade 😂 Here's a brief history lesson:cs/path-injection#237 I fixed a source of FPs that was reported to us, but never upstreamed this change to GitHub. I should've done this right away, but apparently forgot about it. Sorry!And at this point all my changes from #237 had basically been accidentially conflict resolved away 😅
Lesson learned! Merge conflict resolution is hard, and I should have upstreamed the changes ASAP to avoid stuff like this going forward.
As a small "bonus" I also discovered that the logic I added in 4dfa886 was not sound and I have reverted that in fa9f02a (and updated the tests to reflect this).