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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 directory need auto redirect #705

Closed
suntong opened this issue Aug 5, 2020 · 12 comments
Closed

馃悰 directory need auto redirect #705

suntong opened this issue Aug 5, 2020 · 12 comments

Comments

@suntong
Copy link

suntong commented Aug 5, 2020

Fiber version
v1.11.1 & v1.13.3

Code snippet

Take a look at
https://github.com/suntong/fiber_demo/blob/master/app/static-gz.go

Issue description

The results of visiting

are wrong, while visiting the following are right:

Normal web servers would redirect http://site/directory to http://site/directory/ automatically (note the extra trailing /), so they would not exhibit such differences. I think so should fiber too.

@thomasvvugt
Copy link
Contributor

Hi @suntong,
Fiber has a setting called StrictRouting to change it's behavior to treat /foo and /foo/ as the same.
Redirecting /foo to /foo/ would require you to write a middleware to check for the trailing /, and redirect the client if it's not present.
To my knowledge, there are no such guidelines to specifically use/not use a trailing / for a URL.

@suntong
Copy link
Author

suntong commented Aug 9, 2020

Thanks for your reply @thomasvvugt, so I took a look and found that StrictRouting would actually remove the trailing slashes, which would actually make things worse and not working any more.

Maybe there is no obvious guidelines, but if you take a look at every web server, the above described behaviors are what they actually doing. So maybe it is such a common sense that there is no need for a written guidelines for it.

OK. I'll write a middleware on my own, and submit a PR against this issue.

Thanks

@thomasvvugt
Copy link
Contributor

Hi @suntong,

By default, StrictRouting is disabled. When StrictRouting is enabled, it will indeed redirect the client if it requests a route with any trailing slashes (/).
So we can take advantage of Fiber's default behavior by using a simple middleware which does the job of redirecting the client if there are no trailing slashes present in the original URL. I also took note of the fact that clients can also request files with file extensions.
Take a look at my example below, and let me know if that helps you out!

package main

import (
	"log"
	"regexp"
	"strings"
	
	"github.com/gofiber/fiber"
	"github.com/gofiber/utils"
)

func main() {
	app := fiber.New(&fiber.Settings{
		// By default -> StrictRouting: false,
	})

	// Custom middleware to redirect routes with a trailing `/`
	app.Use(func(c *fiber.Ctx) {
		originalUrl := utils.ImmutableString(c.OriginalURL())

		// Check if the client is requesting a file extension
		extMatch, _ := regexp.MatchString("\\.[a-zA-Z0-9]+$", originalUrl)

		if !strings.HasSuffix(originalUrl, "/") && !extMatch {
			c.Redirect(originalUrl + "/")
		}
		c.Next()
	})

	// Route definitions
	app.Get("/foo/", func(c *fiber.Ctx) {
		c.SendString("I am Foo!")
	})

	log.Fatal(app.Listen(8080))
}

Thanks for asking! 馃殌

@suntong
Copy link
Author

suntong commented Aug 13, 2020

Why it is closed?

@thomasvvugt
Copy link
Contributor

Hi @suntong,

Like I pointed out in my previous comment, there are no such guidelines to specifically use/not use a trailing / for a URL.
I have also provided a working code example which adds a trailing slash, mimicking the behavior you stated.

Let me know if there are any additional comments regarding the issue or the given code example.

@suntong
Copy link
Author

suntong commented Aug 13, 2020

Fine.

Giving the fact that all normal web servers would redirect directories automatically with the extra trailing, I'm still thinking this should be a builtin feature, not even an added middleware, but

It's your code so it is your call.

@Fenny
Copy link
Member

Fenny commented Aug 13, 2020

It's something we worked on a while back, please see https://github.com/gofiber/fiber/blob/master/app.go#L183. But we never finished the implementation because it impacted the router performance too much.

It's not that we do not want to implement this, but I just couldn't find the time yet to make this feature work without losing to much performance.

PR's are welcome, in the meantime, I have to focus on the v1.14.0 release notes and maybe we can take another look at the RedirectFixedPath feature for v1.14.1 馃憤

@suntong
Copy link
Author

suntong commented Aug 13, 2020

Ah, yeah, that make perfect sense. I.e.,

  • adding it as a middleware will have to run the detection for every single request, doing FS IO determining if it is a directory or not, so it will definitely have negative impact on the performance
  • however, if it is a builtin feature from the static file server, since the code would have to know if it is a directory or file anyway, and treat it differently, adding a trailing "/" would literately have nearly zero cost. We just have to find the perfect spot to add it.

@suntong
Copy link
Author

suntong commented Sep 5, 2020

Giving the fact that all normal web servers would redirect directories automatically with the extra trailing, I'm still thinking this should be a builtin feature, not even an added middleware...

Took a further look at the issue to determine where exactly the problem came from, as fiber is relying on the fasthttp to provide the file server service. So I have added a fasthttp-fileserver check --

suntong/fiber_demo@8ab9321

cd fiber_demo/fasthttp-fileserver
go run fileserver.go -dir ../web 

The check points out that fasthttp-fileserver clearly has such feature builtin -- it would redirect directories automatically with the extra trailing -- http://localhost:8080/sample0 would become http://localhost:8080/sample0/ automatically.

So the problem is somewhere within fiber, not the fasthttp.

@suntong
Copy link
Author

suntong commented Sep 5, 2020

So the problem is somewhere within fiber, not the fasthttp.

Seems to have come from

fiber/router.go

Lines 273 to 285 in 29e8f12

PathRewrite: func(ctx *fasthttp.RequestCtx) []byte {
path := ctx.Path()
if len(path) >= prefixLen {
if isStar && getString(path[0:prefixLen]) == prefix {
path = append(path[0:0], '/')
} else {
path = append(path[prefixLen:], '/')
}
}
if len(path) > 0 && path[0] != '/' {
path = append([]byte("/"), path...)
}
return path

What is the logic behind such PathRewrite?
Would it be a better idea just to leave Path Rewrite to fasthttp to handle that?

@suntong
Copy link
Author

suntong commented Sep 24, 2020

See #1490

@tangtanglove
Copy link

Hi @suntong,

By default, StrictRouting is disabled. When StrictRouting is enabled, it will indeed redirect the client if it requests a route with any trailing slashes (/). So we can take advantage of Fiber's default behavior by using a simple middleware which does the job of redirecting the client if there are no trailing slashes present in the original URL. I also took note of the fact that clients can also request files with file extensions. Take a look at my example below, and let me know if that helps you out!

package main

import (
	"log"
	"regexp"
	"strings"
	
	"github.com/gofiber/fiber"
	"github.com/gofiber/utils"
)

func main() {
	app := fiber.New(&fiber.Settings{
		// By default -> StrictRouting: false,
	})

	// Custom middleware to redirect routes with a trailing `/`
	app.Use(func(c *fiber.Ctx) {
		originalUrl := utils.ImmutableString(c.OriginalURL())

		// Check if the client is requesting a file extension
		extMatch, _ := regexp.MatchString("\\.[a-zA-Z0-9]+$", originalUrl)

		if !strings.HasSuffix(originalUrl, "/") && !extMatch {
			c.Redirect(originalUrl + "/")
		}
		c.Next()
	})

	// Route definitions
	app.Get("/foo/", func(c *fiber.Ctx) {
		c.SendString("I am Foo!")
	})

	log.Fatal(app.Listen(8080))
}

Thanks for asking! 馃殌

Although it can solve this problem, but it is not the best solution

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

4 participants