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]: Potential issues with parameter parsing in findNextParamPosition and handling of routes with multiple : #3289

Open
3 tasks done
789748456v opened this issue Jan 23, 2025 · 2 comments

Comments

@789748456v
Copy link

Bug Description

Hi Fiber team,
I’ve noticed two potential issues in the route-parsing logic:
The branch is main. fiber v3.

  1. Ineffective loop in path.go, func findNextParamPosition:
for found := findNextNonEscapedCharsetPosition(pattern[nextParamPosition+1:], parameterStartChars); found == 0; {
    nextParamPosition++
    if len(pattern) > nextParamPosition {
        break
    }
}

The condition if len(pattern) > nextParamPosition { break } appears to render the loop meaningless. Because nextParamPosition is guaranteed to be < len(pattern), the loop will always break after one iteration, which doesn’t seem to match the intended logic of skipping multiple consecutive : or similar parameter characters.

  1. Route like "/a:::" treated as having a parameter:
    If I register GET("/a:::"), tools like Express Route Tester treat it as a purely static route. However, Fiber currently interprets the final : as a parameter indicator. This creates an inconsistency with other routing systems where multiple : characters at the end might be interpreted as part of a static path or simply invalid syntax.

Could you clarify if this is the intended behavior? If not, might it be possible to update the logic in findNextParamPosition (and related parsing code) so that multiple consecutive : are handled more consistently, and routes like "/a:::" are treated as static?

Thank you for your time and for all your work on Fiber!

How to Reproduce

skip

Expected Behavior

skip

Fiber Version

v3 main

Code Snippet (optional)

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my problem prior to opening this one.
  • I understand that improperly formatted bug reports may be closed without explanation.
Copy link

welcome bot commented Jan 23, 2025

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@ReneWerner87
Copy link
Member

these codes were created as a solution to the problem
#1262

with these PR
https://github.com/gofiber/fiber/pull/1280/files

google Custom methods
https://google.aip.dev/136

With these cases

fiber/path_test.go

Lines 43 to 72 in 25e3992

rp = parseRoute("/v1/some/resource/name\\:customVerb")
require.Equal(t, routeParser{
segs: []*routeSegment{
{Const: "/v1/some/resource/name:customVerb", Length: 33, IsLast: true},
},
params: nil,
}, rp)
rp = parseRoute("/v1/some/resource/:name\\:customVerb")
require.Equal(t, routeParser{
segs: []*routeSegment{
{Const: "/v1/some/resource/", Length: 18},
{IsParam: true, ParamName: "name", ComparePart: ":customVerb", PartCount: 1},
{Const: ":customVerb", Length: 11, IsLast: true},
},
params: []string{"name"},
}, rp)
// heavy test with escaped charaters
rp = parseRoute("/v1/some/resource/name\\\\:customVerb?\\?/:param/*")
require.Equal(t, routeParser{
segs: []*routeSegment{
{Const: "/v1/some/resource/name:customVerb??/", Length: 36},
{IsParam: true, ParamName: "param", ComparePart: "/", PartCount: 1},
{Const: "/", Length: 1, HasOptionalSlash: true},
{IsParam: true, ParamName: "*1", IsGreedy: true, IsOptional: true, IsLast: true},
},
params: []string{"param", "*1"},
wildCardCount: 1,
}, rp)

if you have ideas on how this parsing can be optimized, please create a PR

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

No branches or pull requests

2 participants