Skip to content
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

[Bug] Mixed params and exact paths can cause a 404 #2682

Closed
rw-access opened this issue Apr 9, 2021 · 3 comments · Fixed by #2706
Closed

[Bug] Mixed params and exact paths can cause a 404 #2682

rw-access opened this issue Apr 9, 2021 · 3 comments · Fixed by #2706
Assignees

Comments

@rw-access
Copy link
Contributor

Router1 "/get/test/abc/"
Router2 "/get/:param/abc/"

I tests these two routers with some requests:
/get/test/abc/ -> Router1
/get/xyz/abc/ -> Router2
/get/tt/abc/ -> 404(Expect Router2)
So, if there are some param starts with the prefix 't' in this example, it will not get the right router.

Originally posted by @Tevic in #2663 (comment)

@rw-access
Copy link
Contributor Author

I don't have the ability to self-assign this, but feel free to assign me.
I've got an idea for a fix.

@g1eny0ung
Copy link
Contributor

g1eny0ung commented Apr 22, 2021

@rw-access I'm also facing this problem, here are the example routes I defined:

r.GET("/", xxx)
r.GET("/:foo", xxx)
r.Group("/api", xxx)
r.GET("/static/*any", xxx)
r.GET("/favicon.ico", xxx)

For the realworld project needed, I try to solve this problem. After debugging, I found that it's related to https://github.com/gin-gonic/gin/pull/2663/files#diff-050830ac4334170e8d335e1b95519052a3461c5b57a4ca38a0531e35a8f3d388R410:

// Try all the non-wildcard children first by matching the indices
idxc := path[0]
for i, c := range []byte(n.indices) {
	if c == idxc {
		n = n.children[i]
		continue walk
	}
}

Since the node indices will build as asf to make a fast deep search. When accessing the route like /archives, the above code will jump into /api route group to find the matching route, then it failed (return 404).

Before #2663, the above code wrapped with an if condition if !n.wildChild so it can be executed normally without any other side effect.

I wanted to propose a PR for repair, but I think it’s best to discuss how to do it. Seems to solve this problem, I think it's needed to add an extra condition like if children have a :param node after c == idxc?Here is an example of linking the above code:

// Try all the non-wildcard children first by matching the indices
idxc := path[0]
for i, c := range []byte(n.indices) {
	if c == idxc {
		childWillWalkTo := n.children[i]

		if strings.Split(childWillWalkTo.path, "/")[0] != strings.Split(path, "/")[0] && strings.HasPrefix(n.children[len(n.children)-1].path, ":") {
			// "api" != "archives" && :foo has :
			continue
		} else {
			n = childWillWalkTo
			continue walk
		}
	}
}

I’m not sure if there are other things to pay attention to. Hope to get your feedback.

@appleboy
Copy link
Member

@rw-access Can you take a look at the PR #2706?

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

Successfully merging a pull request may close this issue.

3 participants