fix: ipv6 url parse in xhttp

[CyberVacation]

problem 1:

in adapter/outbound/vless.go:614:

requestHost := v.option.XHTTPOpts.Host
if requestHost == "" {
	if v.option.ServerName != "" {
		requestHost = v.option.ServerName
	} else {
		requestHost = v.option.Server
	}
}

1. if tls isn't enabled, v.option.ServerName shouldn't be used.
2. if requestHost == "" and v.option.ServerName == "", it will use v.option.Server, but v.option.Port is unused.

fix:

only use ServerName when TLS enabled, and use v.addr rather than v.option.Server.

problem 2:

in transport/xhttp/client.go, all URL.Scheme is hardcoded "https". But when TLS is disabled, URL.Scheme should be "http". Hardcoded "https" is unexpected behavior.

fix:

Add Scheme in Config in transport/xhttp/config.go.

problem 3:

in transport/xhttp/client.go:210:

protocols := new(http.Protocols)
protocols.SetUnencryptedHTTP2(true)
return &http.Transport{
	DialTLSContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
		raw, err := dialRaw(ctx)
		if err != nil {
			return nil, err
		}
		wrapped, err := wrapTLS(ctx, raw, true)
		if err != nil {
			_ = raw.Close()
			return nil, err
		}
		return wrapped, nil
	},
	IdleConnTimeout: ConnIdleTimeout,
	Protocols:       protocols,
	HTTP2: &http.HTTP2Config{
		SendPingTimeout: keepAlivePeriod,
	},
}

Transport.DialContext isn't set. which means h2c can bypass the injected XHTTP dialer.

fix:

set Transport.DialContext too.

btw I wonder whether code refactor is acceptable. I think there are some unnecessary function calls like NormalizedPath, GetNormalizedUplinkHTTPMethod, etc. in every Dial. Why should these functions be called every Dial? Shouldn't these functions only be called once during initialization?

[wwqgtxx]

We do not consider any scenarios where VLESS is used without TLS (while it might work, it falls outside the scope of our support).

Furthermore, the third issue you mentioned does not exist; since we force the scheme to be HTTPS, there is no possibility of bypassing DialTLSContext.

[CyberVacation]

We do not consider any scenarios where VLESS is used without TLS (while it might work, it falls outside the scope of our support).

It does work. I've tried these setup in Xray-core. While Xray-core client can connect, mihomo client failed to connect. I think it's better to act the same as upstream. And xhttp's design didn't force TLS, so I think xhttp without TLS a valid usage, and it may have possible scenarios (such as restricted network where only http traffic is approved).

[wwqgtxx]

However, the content in your PR doesn't help with what you mentioned; whether you change it or not won't affect usability.

[CyberVacation]

However, the content in your PR doesn't help with what you mentioned; whether you change it or not won't affect usability.

ok, I'll detail.

requestHost := v.option.XHTTPOpts.Host
if requestHost == "" {
	if v.option.ServerName != "" {
		requestHost = v.option.ServerName
	} else {
		requestHost = v.option.Server
	}
}

this will affect the connectivity. example:
if v.option.Server is IPv6 and xhttp.Host is "", it'll raise an error when dial:
connect error: https://2001::db:b:c/path invalid port
This is propbably caused by url parse. And standard http host format should be ip:port, not just ip.

Scheme didn't affect connectivity, but to correspond standard scheme should be http if TLS is disabled.

[wwqgtxx]

In reality, this is simply an error issue; changing the scheme to http won't solve it.

The simplest solution is to manually specify a valid host in your configuration file's xhttp.Host setting; this requires no code changes.

[CyberVacation]

The simplest solution is to manually specify a valid host in your configuration file's xhttp.Host setting; this requires no code changes.

Yes, but as there already have auto fill logic and it have problem, a simple correction in this auto fill logic can make configuration more simple.

changing the scheme to http won't solve it.

changing the scheme is to avoid traffic signature. because https scheme in http traffic is not normal traffic.

don't you think original code have obviously problems no matter whether them affect usability? and fix them doesn't cost much.

[wwqgtxx]

don't you think original code have obviously problems no matter whether them affect usability? and fix them doesn't cost much.

Yes, I don't think there is anything wrong with the original code. To reiterate, non-TLS operation is not supported; it might work, but we do not—and do not need to—guarantee that.

[CyberVacation]

Yes, I don't think there is anything wrong with the original code. To reiterate, non-TLS operation is not supported; it might work, but we do not—and do not need to—guarantee that.

Got it. So this become a simple feature request: Could you please consider supporting XHTTP with non-TLS? I think it can provide more flexibility for user.

Update:

I think the host fallback issue also affects XHTTP with TLS. For example, with server: 2001:abcd::1, port: 443, tls: true, no servername, and a certificate containing the IPv6 address as an IP SAN, the current fallback assigns the raw IPv6 literal to url.URL.Host. This produces an invalid url such as https://2001:abcd::1/path.

Would you accept a smaller PR that only changes the empty-host fallback from v.option.Server to v.addr? And leave the scheme and non-TLS changes out.

[wwqgtxx]

The issue you mentioned later is indeed one worth fixing.

However, simply changing the fallback value to v.addr is unacceptable; this would cause outgoing HTTP requests to unconditionally include the port number and lead to inconsistency in internal code paths.

The proper solution is to generate a valid URL—specifically, by adding the missing brackets to IPv6 addresses that lack them before constructing the URL—rather than expanding the scope of the problem.

[CyberVacation]

However, simply changing the fallback value to v.addr is unacceptable; this would cause outgoing HTTP requests to unconditionally include the port number and lead to inconsistency in internal code paths.

Agreed that always using v.addr unnecessarily adds :443 and changes the existing default-port request form. However for a non-default port, omitting the port also makes the URL authority inconsistent with the actual origin.

Would it be preferable to preserve the current form for port 443, bracket IPv6 address, and append the port only when it's non-default?

if requestHost == "" {
	host := v.option.Server
	if v.option.ServerName != "" {
		host = v.option.ServerName
	}

	if v.option.Port != 443 {
		requestHost = net.JoinHostPort(host, strconv.Itoa(v.option.Port))
	} else if ip := net.ParseIP(host); ip != nil && ip.To4() == nil {
		requestHost = "[" + host + "]"
	} else {
		requestHost = host
	}
}

Does this make sense?

Or maybe a more compact version:

if requestHost == "" {
	host := v.option.Server
	if v.option.ServerName != "" {
		host = v.option.ServerName
	}

	requestHost = net.JoinHostPort(host, strconv.Itoa(v.option.Port))
	if v.option.Port == 443 {
		requestHost = strings.TrimSuffix(requestHost, ":443")
	}
}

[wwqgtxx]

I think dealing with port numbers would complicate matters; I prefer to simply address the issue of IPv6 addresses lacking square brackets.

[CyberVacation]

I think dealing with port numbers would complicate matters; I prefer to simply address the issue of IPv6 addresses lacking square brackets.

It's a simple way to avoid error. But letting Host never include port number is non-compliant with http standards and I'm not sure whether it'll cause issues. If you think Host doesn't need port number then here's a simple solution:

if requestHost == "" {
	if v.option.ServerName != "" {
		requestHost = v.option.ServerName
	} else {
		requestHost = v.option.Server
	}
	if ip := net.ParseIP(requestHost); ip != nil && ip.To4() == nil {
		requestHost = "[" + requestHost + "]"
	}
}

[wwqgtxx]

I find this approach acceptable, but please use the more efficient netip.ParseAddr function.
I have reopened the PR; you can directly force-push your current branch.