MongoUrlBuilder.ToString is being used to build the Url from provided parameters, so having passwords there is probably the expected behavior (I suppose we ought to have another method for that, something like BuildUrl). However this class is user-facing settings class, there is a bigger chances it could be converted to string and logged or even worse outputted as a part of exception. I've checked MongoClientSettings - it does not look like it can leak passwords in the similar way.

We have Ensure.IsNullOrBetween if this is what you are looking for.

As i understand, If the server cannot find any acceptable authentication methods offered by the client, it replies with 0xFF ("no acceptable methods"). The only way the server can select username/password (0x02) is if the client offered it. A case where the server replies with 0x02 but the client did not offer it should not occur in a compliant implementation - that would indicate a protocol violation by the server.

To handle this defensively, what do you think about throwing an error such as:
"Unexpected server response: server replied with but expected one of "?

This would highlight the unexpected behavior to users and, in edge cases where a proxy forces 0x02, might help discourage clients from inadvertently sending credentials.

Yes, this should not happen unless the server is behaving badly, I'm trying to be defensive here.

Regarding the message, I agree, it's a little vague, I'll try to rephrase it so it highlights the issue better.

[nitpick] The method signature looks generic, but internally it casts from Socks5AuthenticationSettings to UsernamePasswordAuthenticationSettings, so it effectively only works with the 0x02 method. Would it make sense to make the signature explicit (e.g. only accept UsernamePasswordAuthenticationSettings)?

I think we can keep the method generic, but throw an exception if the authentication is not for the correct type (that should not happen), so we keep the method open to extension in the future if needed.

According to the RFC-1929, authentication only fails when buffer[1] != Socks5Success. If buffer[0] != SubnegotiationVersion (despite the client specifying 1), that suggests a protocol-noncompliant response from the server/proxy.

The RFC states:

"The VER field contains the current version of the subnegotiation, which is X'01'."

I'd propose making the failure reason explicit here. When buffer[0] != SubnegotiationVersion, we should indicate that this is an unexpected server response rather than a credentials error. For example: "SOCKS5 auth subnegotiation: unexpected VER from server". This would also make troubleshooting easier. What do you think?

I agree!

Casting username or password length to byte works correctly if the int values are ≤ 0xFF. Otherwise, we end up reporting incorrect lengths and writing more bytes than the proxy expects, which could silently corrupt data and result in unexpected responses from the server.

I’d propose enforcing a maximum length (in UTF-8 bytes, not chars/code points) for username and password in UsernamePasswordAuthenticationSettings. That way, it is validate at the API edge and fail fast with a clear error message.

The same applies to var hostLength = EncodeString(targetHost, buffer, 5, nameof(targetHost));. Per RFC 1035 §2.3.4, the maximum domain name length is 255 bytes (0xFF), which i'd propose enforcing in Socks5ProxySettings.

What do you think?

This is what the checked statement is doing, it will throw an OverflowException in case the value cannot be safely converted to byte.
I agree that probably we can be a little more user friendly here, instead of throwing a generic exception. I'll try to add some more validation to our settings class.

Added.

Should we provide a more user-friendly message here indicating what type of error the server returned?

The RFC defines the following failure reply types:

0x01 General SOCKS server failure
0x02 Connection not allowed by ruleset
0x03 Network unreachable
0x04 Host unreachable
0x05 Connection refused
0x06 TTL expired
0x07 Command not supported
0x08 Address type not supported

Makes sense! Added.