Tokenizer change in 3.10 breaks many URLs

[steffann]

I'm using https://github.com/emicklei/go-restful/tree/67c9f7e97871832a5773f8a7c66230c7e3322e20/examples/openapi as the test case.

This has the following content in go.mod:

module github.com/emicklei/go-restful/examples/openapi

go 1.14

require (
        github.com/emicklei/go-restful-openapi/v2 v2.8.0
        github.com/emicklei/go-restful/v3 v3.8.0
        github.com/go-openapi/spec v0.20.4
)

Running this:

❯ go install

❯ go run ./restful-openapi.go
2022/11/13 21:18:05 Get the API using http://localhost:8080/apidocs.json
2022/11/13 21:18:05 Open Swagger UI using http://localhost:8080/apidocs/?url=http://localhost:8080/apidocs.json

And in a second window:

❯ curl --silent -D - http://localhost:8080/apidocs.json | head -10
HTTP/1.1 200 OK
Content-Type: application/json
Date: Sun, 13 Nov 2022 20:20:57 GMT
Transfer-Encoding: chunked

{
 "swagger": "2.0",
 "info": {
  "description": "Resource for managing Users",
  "title": "UserService",

Upgraded to github.com/emicklei/go-restful/v3 v3.9.0:

❯ go get github.com/emicklei/go-restful/v3@v3.9.0
go: upgraded github.com/emicklei/go-restful/v3 v3.8.0 => v3.9.0

❯ go run ./restful-openapi.go
2022/11/13 21:21:25 Get the API using http://localhost:8080/apidocs.json
2022/11/13 21:21:25 Open Swagger UI using http://localhost:8080/apidocs/?url=http://localhost:8080/apidocs.json

Still good:

❯ curl --silent -D - http://localhost:8080/apidocs.json | head -10
HTTP/1.1 200 OK
Content-Type: application/json
Date: Sun, 13 Nov 2022 20:21:48 GMT
Transfer-Encoding: chunked

{
 "swagger": "2.0",
 "info": {
  "description": "Resource for managing Users",
  "title": "UserService",

But with 3.10:

❯ go get github.com/emicklei/go-restful/v3@v3.10.0
go: upgraded github.com/emicklei/go-restful/v3 v3.9.0 => v3.10.0

❯ go run ./restful-openapi.go
2022/11/13 21:22:09 Get the API using http://localhost:8080/apidocs.json
2022/11/13 21:22:09 Open Swagger UI using http://localhost:8080/apidocs/?url=http://localhost:8080/apidocs.json

It fails:

❯ curl --silent -D - http://localhost:8080/apidocs.json | head -10
HTTP/1.1 404 Not Found
Date: Sun, 13 Nov 2022 20:22:20 GMT
Content-Length: 19
Content-Type: text/plain; charset=utf-8

404: Page Not Found

Instead it suddenly requires a trailing slash:

❯ curl --silent -D - http://localhost:8080/apidocs.json/ | head -10
HTTP/1.1 200 OK
Content-Type: application/json
Date: Sun, 13 Nov 2022 20:22:50 GMT
Transfer-Encoding: chunked

{
 "swagger": "2.0",
 "info": {
  "description": "Resource for managing Users",
  "title": "UserService",

This is a breaking change that I did not expect in a stable release… And besides, the URL became ugly :)

[emicklei]

thank you for reporting this. The change was made because of a reported Security issue. I will have a close look at your example(s)

[emicklei]

I was able to reproduce the problem.
With a small change to the go-restful-openapi package, I was able to get a working example.
I need to think about is implications before going ahead with this

func NewOpenAPIService(config Config) *restful.WebService {

	ws := new(restful.WebService)
	ws.Produces(restful.MIME_JSON)
	if !config.DisableCORS {
		ws.Filter(enableCORS)
	}
	swagger := BuildSwagger(config)
	resource := specResource{swagger: swagger}
	ws.Route(ws.GET(config.APIPath).To(resource.getSwagger))
	return ws
}

[steffann]

FYI: it isn’t just the openapi endpoint that stopped working. I just use that one as an example. It’s all of the endpoints. /users doesn’t work anymore, only /users/ etc.

[emicklei]

yes, i realise the impact is larger. thinking about rolling it back

[steffann]

What was the security issue? Maybe I can help with finding a better solution.

[emicklei]

What was the security issue? Maybe I can help with finding a better solution.

#497

[steffann]

The feature flag mentioned there doesn’t seem to be implemented. How about a setting with three possible values:

• trailing slash must be present (current behaviour)
• trailing slash must NOT be present (matches the generated Swagger spec)
• trailing slash optional (the old behaviour)

That way everybody can choose for their application what their URL structure is.

If you like this solution I’d be happy to help implement it, and adapt the swagger/openapi code to make sure its output matched the selected option.

[emicklei]

@steffann can you look at bc68247 ?

[lucacome]

@emicklei is this supposed to be fixed? We're seeing changes in behavior in the PR that I linked ☝️

[emicklei]

@lucacome , v3.10.1 is supposed to fix issues introduced with v3.10 which contained changes to fix the reported security issue.
I will have a look at the k8s PR to see why/how the latest version breaks it.

[emicklei]

This was the security issue reported: #497 "There is an inconsistency between how golang(and other packages) and go-restful parses url path."

[steffann]

Sorry I didn’t reply earlier. I’ve been swamped. I haven’t checked the latest code yet. I’ll look at it as soon as I can!

[lucacome]

@emicklei did you get a chance to look into it?

[emicklei]

@lucacome so, the package in its current state requires a different route to match v1/foo and v1/foo/. This is a result of solving the report security issue. The k8s openapi-spec shows paths without a trailing slash. If clients use a request path with a trailing slash then that request will fail.
From the PR log, I saw that the paths have an extra "/" at the end. (https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/115067/pull-kubernetes-verify/1614455634982342656).
How is the spec generated? is that use go-restful-openapi? what version?

[emicklei]

@lucacome @steffann can you please review this proposal?

[emicklei]

(including @SVilgelm for notifications)

#523 currently sets PathJoin as default ; it could also be set to the old behaviour strategy.
Another option is to restore the old behaviour completely and introduce a v4 branch with a Merge strategy with PathJoin as default.

wdyt?

[SVilgelm]

I'm ok with any solution that can bring back the previous logic :)
I can configure if it is available:)))

[SVilgelm]

#523 currently sets PathJoin as default

wrong, it uses old logic as default for now: https://github.com/emicklei/go-restful/pull/523/files#diff-4c596ebcfb13dbd1279de9b297b3d39701d7205967b37349577c276c06b68127R370

[emicklei]

With version 3.10.2 you can make the package behave as it was < 3.10. See https://github.com/emicklei/go-restful/blob/v3/examples/trimslash/restful-hello-world.go

[steffann]

@lucacome @steffann can you please review this proposal?

I'm sorry I haven't been responsive. I'm completely overwhelmed with work and travel at the moment :(

[KeithETruesdell]

I am still seeing this as an issue, unless I am mis understanding the issue.

https://github.com/emicklei/go-restful/blob/v3/examples/trimslash/restful-hello-world.go.

Looking at the example for "trimslash" above, both endpoints, /hello and /hello/ both return 'world' because they are both defined and both going to the same function.
I thought the previous logic was that the endpoint /hello would get hit when making request like curl http://localhost:8080/hello/ even though it ends in the slash.

[SVilgelm]

nope, check this:
https://cs.opensource.google/go/go/+/master:src/net/http/server.go;l=2298-2304
and this https://cs.opensource.google/go/go/+/master:src/net/http/server.go;l=2393
and this https://cs.opensource.google/go/go/+/master:src/net/http/server.go;l=2466

in short, if we register a handler for the /hello/ endpoint then curl http://localhost:8080/hello will receive 301 response with redirect to /hello/
but it does not support backward situation

[SVilgelm]

sorry, did not get your question, ignore my previous comment

[KeithETruesdell]

Just to help explain and utilize the 'trimslash' example i did this simple example...

package main

import (
	"io"
	"log"
	"net/http"

	restful "github.com/emicklei/go-restful/v3"
)

// This example shows the minimal code needed to get a restful.WebService working.
//
// GET http://localhost:8080/hello
// GET http://localhost:8080/hello/

func main() {
	restful.MergePathStrategy = restful.TrimSlashStrategy
	ws := new(restful.WebService)
	ws.Route(ws.GET("/hello").To(hello1))
	ws.Route(ws.GET("/hello/").To(hello2))
	restful.Add(ws)
	log.Fatal(http.ListenAndServe(":8080", nil))
}

func hello1(req *restful.Request, resp *restful.Response) {
	io.WriteString(resp, "world")
}

func hello2(req *restful.Request, resp *restful.Response) {
	io.WriteString(resp, "to you")
}

Obviously, both endpoints are defined, but with different functions giving different responses. The /hello endpoint gives "world" as a response and the /hello/ endpoint gives "to you" as a response.

If I comment out the restful.TrimSlashStrategy so as to use the default behavior, and I have both endpoints defined, when I call /hello i get "world" and when i call /hello/ I get a 404 page not found.
Reading the other security fix, issue #497 , I don't think that is the intended result.

Keeping with the tests and the default behavior, if I comment out the /hello endpoint and call /hello i get the "to you" response and when i call the /hello/ endpoint I get a 404. Just to be clear, default "merge path strategy" and there is no /hello path, when calling the /hello endpoint i get what I would expect from the /hello/ endpoint and the /hello endpoint returns an error.
I think that is actually the use-case for this ticket, right?

Last set of tests...default behavior with the TrimSlashStrategy commented out, and I commented out the /hello/ endpoint. When I call /hello I get "world" and when I call /hello/ i get a 404 error. That was to be expected and seems good. When I comment out the /hello endpoint and call /hello I get "to. you" and when I call /hello/ I get a 404 error. With neither endpoint commented out, when I call /hello I get "world" and when I call /hello/ i get a 404 error.

I reran the same with restful.PathJoinStrategy and got the same results.
I am also realizing that a table might make this better...maybe...but hopefully you understand the test cases here to hopefully understand the problem.

[KeithETruesdell]

I realize my paragraph style is a jumbled mess, so here is a "table" that hopefully helps with the different tests.
Going down are the scenarios or code variations implemented, using the different variables and commenting out different paths and so on.
Across the top are the endpoints called.
And obviously what is in the boxes is the result.

For reference, the code should be the following...
/hello ===> "world"
/hello/ ===> "to you"

| endpoint called ---> | /hello | /hello/ |
| scenario ↓ | -----------| ----------- |
| TrimSlashStrategy - both endpoints active | "world" | "to you" |
| TrimSlash - no /hello | 404 | "to you" |
| TrimSlash - no /hello/ | "world" | 404 |
| PathJoinStrategy - both active | "world" | 404 |
| PathJoin - no /hello | "to you" | 404 |
| PathJoin - no /hello/ | "world" | 404 |
| Default Strategy - both active | "world" | 404 |
| Default - no /hello | "to you" | 404 |
| Default - no /hello/ | "world" | 404 |

previous behavior <= 3.9

| endpoint called ---> | /hello | /hello/ |
| scenario ↓ | -----------| ----------- |
| Default Strategy - both active | "to you" | "to you" |
| Default - no /hello | "to you" | "to you" |
| Default - no /hello/ | "world" | "world" |

[emicklei]

@KeithETruesdell thank you for your elaborate report. The TrimSlashStrategy was created to have the <= 3.9. behaviour. I will redo your tests and see what change I have missed since 3.9

[emicklei]

found a (causal) difference in route.go # tokenizePath :167

3.9

return strings.Split(strings.Trim(path, "/"), "/")

3.10

return strings.Split(strings.TrimLeft(path, "/"), "/")

need to investigate why

[emicklei]

hi to those involved, can you please comment on the current state.
The examples show the behaviours that can be achieved.
Would love to hear your advice on how to proceed.

[emicklei]

I propose to restore the behaviour of 3.9.0 w.r.t route matching and start a new v4 branch instead.
This way existing users do not experience a broken system and the v4 branch can. WDYT?

[emicklei]

fyi, I have started working on a new v4 branch

[emicklei]

working to get the routing spec clear first, see https://github.com/emicklei/go-restful/tree/v4#routing

[emicklei]

So the plan is to restore 3.9.0 behavior in the v3 branch, starting with 3.11 with the option to trim the suffix slash set to true.
@steffann @SVilgelm @lucacome @KeithETruesdell Please have a try (or look) at #535
The new v4 branch will reverse that behavior; slashes are preserved unless specified otherwise.

[emicklei]

fixed in 3.11.0