search

fabiolb / fabio

Consul Load-Balancing made simple
https://fabiolb.net
MIT License
7.25k stars 621 forks source link

strip doesn't work as expected on redirect #824

Closed far-blue closed 3 years ago

far-blue commented 3 years ago

I'm trying to use strip and redirect together and I'm not getting the behaviour I'd expect.

What I'm trying to do is:

urlprefix-<host>/news strip=/news redirect=301,https://<host>/bulletin/$path

However, the strip isn't working.

Looking at the code here: https://github.com/fabiolb/fabio/blob/231e7f24a13fde2fca2ffd9e8f553c18addac165/route/target.go#L99

It looks like the redirect destination is assembled first, and then the strip prefix is checked. In my case this would first convert /new/foo into /bulletin/news/foo, then attempt to apply the strip - which will fail.

I suspect the code should actually be applying the prefix check and strip to the replaceRawPath var rather than the t.RedirectURL.Path var.

aaronhurt commented 3 years ago

@far-blue You are correct. The path stripping is currently done AFTER the replacement of the $path pseudo variable in the code. I added a new test case and have a patch I'll push shortly.

aaronhurt commented 3 years ago

I also added a new test case for this issue. The fix passes the new test and all existing tests.

https://github.com/fabiolb/fabio/blob/d8c81f8aa08984e6fb0777a8109363fea9b123e6/route/target_test.go#L146-L155

Let me know if this resolves your issue.

far-blue commented 3 years ago

Great, thanks for this. I don't have experience with building go apps but your unit tests capture the behaviour as I'd expect so if they are passing I'm confident the issue will be fixed :)