Skip to content

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

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

Closed
rivo opened this issue Aug 23, 2024 · 5 comments
Milestone

Comments

@rivo
Copy link

rivo commented Aug 23, 2024

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>.
@cherrymui cherrymui changed the title http.ServeMux not backwards compatible when registering double-slash patterns net/http: ServeMux not backwards compatible when registering double-slash patterns Aug 23, 2024
@cherrymui cherrymui changed the title net/http: ServeMux not backwards compatible when registering double-slash patterns net/http: ServeMux not backwards compatible (with Go 1.21) when registering double-slash patterns Aug 23, 2024
@cherrymui
Copy link
Member

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.

@cherrymui cherrymui added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 23, 2024
@cherrymui cherrymui added this to the Backlog milestone Aug 23, 2024
@rivo
Copy link
Author

rivo commented Aug 23, 2024

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
Copy link

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.

@jba
Copy link
Contributor

jba commented Dec 13, 2024

@rivo This particular incompatibility wasn't intended, but is covered by the caveat that Cherry quoted, and the GODEBUG setting is the right fix.

Closing as WAI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants