Skip to content

Add exception on failed port string parsing in BindingAddress.cs Resolves #24384 #63135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

h5aaimtron
Copy link
Contributor

Add exception on failed port string parsing in BindingAddress.cs

This PR adds a FormatException in the BindingAddress.cs when an invalid portString cannot be parsed as int.

Description

Users have reported that when passing address strings containing invalid port strings is passed into BindingAddress.Parse(), instead of throwing an exception or generating a warning, the invalid port string is ignored, and the default ports (80/443) are applied. Invalid address strings such as "https://*:{abc}" should throw a FormatException due to the invalid port value 'abc' similar to how FormatExceptions are thrown for invalid hostnames.

Fixes #24384

@github-actions github-actions bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Aug 5, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 5, 2025
h5aaimtron and others added 2 commits August 5, 2025 14:31
Applying suggestion to match linting.

Co-authored-by: Günther Foidl <[email protected]>
Adding additional help message.

Co-authored-by: Günther Foidl <[email protected]>
@h5aaimtron
Copy link
Contributor Author

@gfoidl I added some tests in the PR as well.

h5aaimtron and others added 2 commits August 5, 2025 15:19
I think there's still an extra ' at the end after allowed. Perhaps a . or nothing to end the message?

Co-authored-by: Günther Foidl <[email protected]>
@h5aaimtron
Copy link
Contributor Author

Additional changes will be required to handle NamedPipes. It is odd that the previous code worked as it set a port with named pipes, so will require further research.

@h5aaimtron
Copy link
Contributor Author

While reviewing the test failures, I identified a pre-existing issue in the code. The port parsing code is wrapped in an if statement checking it is not a unix pipe, but fails to check if it is not a named pipe as well. This allowed named pipes to enter the port parsing code block, which combined with my earlier change, resulted in the thrown FormatException. Since named pipes do not have ports and therefore do not need to parse port numbers, I've added the not named pipe condition to the if statement to skip the code block for named or unix pipes.

@h5aaimtron
Copy link
Contributor Author

Latest change fixes support for IPv6 [::1] binding. Test cases had non-bindable addresses marked as valid, so those were removed. For people looking simply to parse a Uri, they should use Uri.TryCreate(), not BindingAddress.Parse() as it infers a valid, bindable address, not just a valid uri.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions 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.

Kestrel port binding default to 80 or 443 / No error/warn/info/debug when "{abc}" provided
2 participants