Tokenizer change in 3.10 breaks many URLs about go-restful HOT 33 CLOSED

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)

from go-restful.

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

from go-restful.

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
}

from go-restful.

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.

from go-restful.

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

from go-restful.

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

from go-restful.

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

#497

from go-restful.

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.

from go-restful.

@steffann can you look at bc68247 ?

from go-restful.

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

from go-restful.

@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.

from go-restful.

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

from go-restful.

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!

from go-restful.

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

from go-restful.

@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?

from go-restful.

@lucacome @steffann can you please review this proposal?

from go-restful.

(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?

from go-restful.

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

from go-restful.

#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

from go-restful.

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

from go-restful.

@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 :(

from go-restful.

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.

from go-restful.

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

from go-restful.

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.

from go-restful.

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" |

from go-restful.

@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

from go-restful.

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

from go-restful.

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.

from go-restful.

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?

from go-restful.

fyi, I have started working on a new v4 branch

from go-restful.

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

from go-restful.

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.

from go-restful.

fixed in 3.11.0

from go-restful.