net/http: ServeMux not backwards compatible (with Go 1.21) when registering double-slash patterns

rivo commented 2 months ago

What version of Go are you using (`go version`)?

$ go version
go version go1.23.0 darwin/arm64

but the issue appears with go1.22 already, when enhanced routing patterns were introduced.

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (`go env`)?

This has been confirmed on multiple architectures.

What did you do?

I'm registering two http.Handler's, one with the pattern "/" (one slash) and the other with the pattern "//" (two slashes). See the following example code:

https://go.dev/play/p/Gv6F8CMxant

Before go1.22, requesting a URI such as /abc would invoke the one-slash handler. Since go1.22, the same URI invokes the two-slash handler. In fact, due to trailing-slash redirection, now a request for /abc is redirected to /abc/, which did not happen in go1.21.

I realize that such a pattern ("//") does not make much sense. And specifying "GET //" will lead to a panic. But nonetheless, this compatibility change broke our software when upgrading to go1.22. We'll be fine with a "won't fix" in any case but I thought it is relevant enough to open an issue for it. Maybe it will reveal other issues with the enhanced routing patterns.

(The following playground will follow the redirect and output two slashes (go1.22) instead of one slash (go1.21): https://go.dev/play/p/mmC95m4W4ko)

What did you expect to see?

Running request...
200 OK
one slash

What did you see instead?

Running request...
301 Moved Permanently
<a href="/test/">Moved Permanently</a>.

gabyhelp commented 2 months ago

Related Issues and Documentation

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

cherrymui commented 2 months ago

As you have read, https://go.dev/doc/go1.22#enhanced_routing_patterns mentions

This change breaks backwards compatibility in small ways... The change is controlled by a GODEBUG field named httpmuxgo121. Set httpmuxgo121=1 to restore the old behavior.

Have you tried setting GODEBUG=httpmuxgo121=1?

cc @neild for whether this incompatibility is intended.

rivo commented 2 months ago

Have you tried setting GODEBUG=httpmuxgo121=1?

Just tried that. Yes, it reverts it back to the pre go1.22 result. So I guess maybe this was intended? @cherrymui / @nild feel free to close this one if there's nothing unexpected here.

AndrewHarrisSPU commented 2 months ago

I realize that such a pattern ("//") does not make much sense. And specifying "GET //" will lead to a panic. But nonetheless, this compatibility change broke our software when upgrading to go1.22. We'll be fine with a "won't fix" in any case but I thought it is relevant enough to open an issue for it. Maybe it will reveal other issues with the enhanced routing patterns.

Subtly I wonder if "//" does make sense with the path package's cleaning rules. There may be a slight inconsistency in how cleaning applies. Whether or not this is actionable for anything, I elect to throw my hands up - no clue!

A function parsePattern is used for parsing, but not cleaning.

Specifically a comment suggests:

// An unclean path with a method that is not CONNECT can never match,
// because paths are cleaned before matching.

For registration, the serveMux method Handle immediately invokes methods register, and registerErr, without path cleaning. It calls parsePattern on the uncleaned path. Does this contradict a premise that only cleaned paths are parsed?

In contrast, for matching, the ServeMux method findHandler does invoke cleanPath, and mostly follows path cleaning rules.

The "//" and "GET //" routes are specifically tested in pattern_test.go.

golang / go