Skip to content

Conversation

@cdbullard
Copy link
Contributor

@cdbullard cdbullard commented Sep 1, 2022

Fix #74662

Adding private method EncodeUserName to replace any problematic characters "/" / "\" / "?" / "#" / "@" with their appropriate values to prevent throwing a UriFormatException.

The code from MihaZupan's comment on this issue was implemented for this fix with minor changes.

@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-System.Net labels Sep 1, 2022
@dnfadmin
Copy link

dnfadmin commented Sep 1, 2022

CLA assistant check
All CLA requirements met.

@ghost
Copy link

ghost commented Sep 1, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

#74662

Adding private method EncodeUserName to replace any problematic characters "/" / "\" / "?" / "#" / "@" with their appropriate values to prevent throwing a UriFormatException.

The code from MihaZupan's comment on this issue was implemented for this fix with minor changes.

Author: cdbullard
Assignees: -
Labels:

area-System.Net, community-contribution

Milestone: -

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cdbullard!

Could you please extend this change to also include the Password property, and also add some tests for these cases?

@MihaZupan MihaZupan added this to the 8.0.0 milestone Sep 1, 2022
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
@skyoxZ
Copy link
Contributor

skyoxZ commented Sep 5, 2022

This could be a break change.

Consider if users find UriBuilder.Uri.ToString() is buggy so they write a helper ToFullEncodedString as below:

var builder = new UriBuilder("https://www.microsoft.com")
{
    UserName = "mallory@hackers.net",
    Password = "hunter2",
};
Console.WriteLine(builder.ToFullEncodedString());

public static class UriBuilderExtension
{
    public static string ToFullEncodedString(this UriBuilder builder)
    {
        string plainUserName = builder.UserName;
        builder.UserName = System.Web.HttpUtility.UrlEncode(plainUserName);
        string result = builder.Uri.ToString();
        builder.UserName = plainUserName;
        return result;
    }
}

@MihaZupan
Copy link
Member

MihaZupan commented Sep 5, 2022

I suppose it's possible.

We can avoid taking that risk by deferring the encoding to the ToString instead of the setters.

vsb.Append(username);
string password = Password;
if (password.Length != 0)
{
vsb.Append(':');
vsb.Append(password);

@skyoxZ
Copy link
Contributor

skyoxZ commented Sep 5, 2022

And maybe : should also be encoded there.

@MihaZupan
Copy link
Member

I would avoid changing : for now. Uri doesn't enforce the exact format of UserInfo.
That is, new Uri("http://name:::password@host/").UserInfo is valid and changing it would be a breaking change.

@cdbullard
Copy link
Contributor Author

Thanks for the feedback--I have pushed a change that I believe will address this by moving the EncodeUserInfo call to the ToString() method per MihaZupan's comment, and also added a new unit test for this situation. Please let me know if any other changes should be made, thank you!

…derTests.cs

Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
@MihaZupan
Copy link
Member

Test failure is #74795

@MihaZupan MihaZupan merged commit e23d22e into dotnet:main Sep 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UriBuilder should percent-encode its UserName or Password properties

4 participants