NavLink component is active on wrong path potential fix #48627

[adanmaftei]

NavLink component is active on the wrong path potential fix #48627

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

Added an additional check for the separator character, to check if it is an allowed char in a URI, such as '/' or '?'.
Check the attachment file for some testing evidence.

Fixes #48627

[ghost]

Thanks for your PR, @adanmaftei. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[adanmaftei]

@dotnet-policy-service agree

[etemi]

Hi @adanmaftei

If I did everything right and understand the issue correctly, then the method IsStrictlyPrefixWithSeparator in your latest commit will return true but I think it should be false to fix issue #48627:

// returns true but should be false
Console.WriteLine(IsStrictlyPrefixWithSeparator("/abc-def", "/abc")); 

I also created a Pull Request for the same issue: #48718
My approach should be less "visible/observable". But my PR will fail the following of your test cases:

// Code in my PR returns true but this is also the current behavior. 
// This is what I mean with "less visible/observable" change
Console.WriteLine(MyIsStrictlyPrefixWithSeparator("/abc*def", "/abc")); 

I think that NavLink should only check the path and ignore query & fragment part of URLs (that would be a breaking change) or allow customization.

[adanmaftei]

Hi @etemi!

I did another commit and got these results below, it was a quick attempt to fix the issue.

[adanmaftei]

@dotnet-policy-service agree

[MackinnonBuck]

Thanks for the effort you've put into this, @adanmaftei. We're going to move forward with #48718 at this time. That change is scoped to a smaller set of separators, which makes it less likely to unintentionally break customers depending on the existing behavior.

[ghost]

Hi @MackinnonBuck. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.