Correct Entrypoint Redirect with Stripped or Added Path #3631

dtomcej · 2018-07-16T22:36:21Z

What does this PR do?

Rebuilds entrypoint redirects when used with PathPrefixStrip or AddPrefix

Motivation

Fixes #1957
and issues linked to #2024

More

Added/updated documentation - No documentation needed, as this is intended behavior
Rebuild trailing slash from prefix match
Added/updated tests
Rebuild correct redirect when prefix is added
Added/updated tests

kachkaev · 2018-07-24T14:48:39Z

Can't wait to see this fix landing a release! 😍

#1957 is in top 10 issues by the number of 👍, so this PR is waited by many!

mmatur · 2018-07-27T07:17:18Z

integration/https_test.go

+			expectedURL: "https://example.com:8443/api/bacon/",
+		},
+	}
+	for _, test := range testCases {


Add new line before please

mmatur · 2018-07-27T07:37:50Z

integration/https_test.go

@@ -804,3 +804,116 @@ func modifyCertificateConfFileContent(c *check.C, certFileName, confFileName, en
 		c.Assert(err, checker.IsNil)
 	}
 }
+
+func (s *HTTPSSuite) TestEntrypointHttpsRedirectAndPathPrefixStrip(c *check.C) {
+	cmd, display := s.traefikCmd(withConfigFile("fixtures/https/https_redirect.toml"))


You should maybe simplify this 2 tests in 1.
The only things that change is the host
You could maybe do something like that

func (s *HTTPSSuite) TestEntrypointRedirectAndPath(c *check.C) { cmd, display := s.traefikCmd(withConfigFile("fixtures/https/https_redirect.toml")) defer display(c) err := cmd.Start() c.Assert(err, checker.IsNil) defer cmd.Process.Kill() // wait for Traefik err = try.GetRequest("http://127.0.0.1:8080/api/providers", 1*time.Second, try.BodyContains("Host: example.com")) c.Assert(err, checker.IsNil) client := &http.Client{ CheckRedirect: func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse }, } testCases := []struct { desc string host string sourceURL string expectedURL string }{ { desc: "Stripped URL redirect", host: "example.com", sourceURL: "http://127.0.0.1:8888/api", expectedURL: "https://example.com:8443/api", }, { desc: "Stripped URL with trailing slash redirect", host: "example.com", sourceURL: "http://127.0.0.1:8888/api/", expectedURL: "https://example.com:8443/api/", }, { desc: "Stripped URL with path redirect", host: "example.com", sourceURL: "http://127.0.0.1:8888/api/bacon", expectedURL: "https://example.com:8443/api/bacon", }, { desc: "Stripped URL with path and trailing slash redirect", host: "example.com", sourceURL: "http://127.0.0.1:8888/api/bacon/", expectedURL: "https://example.com:8443/api/bacon/", }, { desc: "AddPrefix with redirect", host: "test.com", sourceURL: "http://127.0.0.1:8888/wtf", expectedURL: "https://test.com:8443/wtf", }, { desc: "AddPrefix with trailing slash redirect", host: "test.com", sourceURL: "http://127.0.0.1:8888/wtf/", expectedURL: "https://test.com:8443/wtf/", }, { desc: "AddPrefix with matching path segment redirect", host: "test.com", sourceURL: "http://127.0.0.1:8888/wtf/foo", expectedURL: "https://test.com:8443/wtf/foo", }, } for _, test := range testCases { test := test req, err := http.NewRequest("GET", test.sourceURL, nil) c.Assert(err, checker.IsNil) req.Host = test.host resp, err := client.Do(req) c.Assert(err, checker.IsNil) defer resp.Body.Close() location := resp.Header.Get("Location") c.Assert(location, checker.Equals, test.expectedURL) } }

Great suggestion

mmatur · 2018-07-27T07:40:01Z

middlewares/redirect/redirect.go

@@ -85,6 +86,28 @@ func (h *handler) ServeHTTP(rw http.ResponseWriter, req *http.Request, next http
 		return
 	}

+	if prefix, prefixOk := req.Context().Value(middlewares.StripPrefixKey).(string); prefixOk {
+		if prefix != "" {


if len(prefix) > 0 {

mmatur · 2018-07-27T07:51:13Z

middlewares/redirect/redirect.go

+	}
+	if addPrefix, addPrefixOk := req.Context().Value(middlewares.AddPrefixKey).(string); addPrefixOk {
+		if addPrefix != "" {
+			parsedURL, err = url.Parse(strings.Replace(parsedURL.String(), addPrefix, "", 1))


err not checked

mmatur · 2018-07-27T07:51:19Z

middlewares/redirect/redirect.go

+			splitURL := strings.SplitN(parsedURL.String(), "/", 4)
+			parsedURL, err = url.Parse(strings.Join(splitURL[:3], "/") + prefix)
+			if len(splitURL) > 3 && splitURL[3] != "" {
+				parsedURL, err = url.Parse(strings.Join(splitURL[:3], "/") + prefix + "/" + splitURL[3])


err not checked

middlewares/stripPrefix.go

mmatur · 2018-07-27T08:19:30Z

middlewares/stripPrefix.go

-func (s *StripPrefix) serveRequest(w http.ResponseWriter, r *http.Request, prefix string) {
+func (s *StripPrefix) serveRequest(w http.ResponseWriter, r *http.Request, prefix string, trailingSlash bool) {
+	if trailingSlash {
+		r = r.WithContext(context.WithValue(r.Context(), StripPrefixSlashKey, trailingSlash))


I think that we can always set this boolean value WDYT?

mmatur · 2018-07-27T09:32:37Z

middlewares/redirect/redirect.go

@@ -85,6 +86,28 @@ func (h *handler) ServeHTTP(rw http.ResponseWriter, req *http.Request, next http
 		return
 	}

+	if prefix, prefixOk := req.Context().Value(middlewares.StripPrefixKey).(string); prefixOk {


You should maybe simplify this part like that

if stripPrefix, stripPrefixOk := req.Context().Value(middlewares.StripPrefixKey).(string); stripPrefixOk { if len(stripPrefix) > 0 { tempPath := parsedURL.Path parsedURL.Path = stripPrefix if len(tempPath) > 0 && tempPath != "/" { parsedURL.Path = stripPrefix + tempPath } if trailingSlash, trailingSlashOk := req.Context().Value(middlewares.StripPrefixSlashKey).(bool); trailingSlashOk { if trailingSlash { if !strings.HasSuffix(parsedURL.Path, "/") { parsedURL.Path = fmt.Sprintf("%s/", parsedURL.Path) } } } } } if addPrefix, addPrefixOk := req.Context().Value(middlewares.AddPrefixKey).(string); addPrefixOk { if len(addPrefix) > 0 { parsedURL.Path = strings.Replace(parsedURL.Path, addPrefix, "", 1) } }

You update parsedURL instead of re parsing the URL multiple times
WDYT?

mmatur · 2018-07-27T16:10:37Z

Could be nice to have this fix in v1.7 WDYT ?

mmatur · 2018-07-27T16:12:12Z

middlewares/stripPrefix.go

@@ -17,18 +26,21 @@ type StripPrefix struct {
 func (s *StripPrefix) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	for _, prefix := range s.Prefixes {
 		if strings.HasPrefix(r.URL.Path, prefix) {
+			trailingSlash := (r.URL.Path == prefix+"/")


Redundant parentheses

Good catch :)

dtomcej · 2018-07-27T18:41:51Z

I can rebase and squeeze this into 1.7!

dtomcej · 2018-07-28T04:33:05Z

Found that I didn't handle/test PathPrefixStripRegex properly. Didn't know that was a matcher 😝 Updated code coming.

juliens

LGTM

mmatur

Great job @dtomcej !! 👏
LGTM

emilevauge

Love this fix @dtomcej
I still have one comment ;)

emilevauge · 2018-07-30T16:20:18Z

integration/https_test.go

+			sourceURL:   "http://127.0.0.1:8888/api/bacon/",
+			expectedURL: "https://foo.com:8443/api/bacon/",
+		},
+	}


I would add few edge cases like double trailing redirect http://127.0.0.1:8888/api/bacon//, no path (with/without) redirect http://127.0.0.1:8888

Added no path + double trailing slash tests.

There is an outstanding "issue" where an entrypoint redirect doesn't work when a frontend isn't matched (I think due to the middleware being applied to frontend chains). This would be out of the scope of this PR.

emilevauge

Great job @dtomcej
LGTM

edasque · 2018-08-13T20:54:25Z

Is this in 1.7.0 rc3? I have a problem that seems similar where PathPrefixStrip: /container1 works when hitting /container1/ but not /container1 (where it doesn't prefix strip the subsequent queries leading to http://192.168.7.229:40080/bower_components/slimScroll/jquery.slimscroll.min.js instead of http://192.168.7.229:40080/organizr/bower_components/bootstrap/dist/css/bootstrap.min.css?v=1.80

marianschmotzer · 2018-08-15T16:09:33Z

I just tried 1.7.0 rc3 and PathPrefixStrip works (or doesn't work to be honest)as mentioned by edasque. I tired even to place redirection to config file -

[entryPoints.https.redirect]                                                                                                                                                                      
     regex = "(http[a-z]{0,1}://[^/]+/([a-zA-Z0-9-]+)[^/])$"                                                                                                                                         
     replacement = "${1}/"

But same results as in 1.6.5 - it doesn't work.

edasque · 2018-08-15T16:51:43Z

@marianschmotzer , do you know if it was supposed to?

marianschmotzer · 2018-08-15T17:23:48Z

@edasque there is this commit 1950de2
So i would say. In any case is quite stupid, but critical bug (at least for us).
My feeling (after looking into that) is that problem is in rewriting response - where if response is mybackslah/ and PathPrefixStrip is mybackslah response paths are not rewritten at all in html code.

marianschmotzer · 2018-09-03T08:53:21Z

@edasque I've creates short blog about this problem with our "fix" you can find it here maybe will be helpful https://medium.com/@smoco/fighting-trailing-slash-problem-c0416023d20e

edasque · 2018-09-03T12:54:51Z

Cool trick @marianschmotzer but I’d rather not add nginx or anything to make something so simple work.

dtomcej · 2018-09-03T13:19:04Z

@marianschmotzer @edasque If you are still having problems, can you please open a new issue instead of bumping a closed PR?

Thanks!

edasque · 2018-09-04T21:03:54Z

@dtomcej please find it here: #3852

@marianschmotzer can you confirm this illustrates the issue correctly.

marianschmotzer · 2018-09-05T07:02:32Z

Hi seems quite clear to me

As outlined in this (traefik/traefik#1957) issue SSL redirection with PathPrefixStrip does not work really well. A solution was provided in this (traefik/traefik#3631) PR, released in 1.7, but it didn't really solve the issue. In fact, there were several subsequent issues opened (traefik/traefik#3999, traefik/traefik#3876) but they got closed. Another issue was opened in the Traefik repo: traefik/traefik#4085 Until then this workaround provides the same functionality.

traefiker added area/middleware size/M status/0-needs-triage labels Jul 16, 2018

dtomcej added kind/bug/confirmed a confirmed bug (reproducible). status/2-needs-review kind/bug/fix a bug fix and removed status/0-needs-triage kind/bug/confirmed a confirmed bug (reproducible). labels Jul 17, 2018

dtomcej changed the title ~~Correct Entrypoint Redirect with Stripped Path~~ Correct Entrypoint Redirect with Stripped or Added Path Jul 20, 2018

dtomcej mentioned this pull request Jul 20, 2018

Wrong HTTPS redirect when also using AddPrefix #2024

Closed

mmatur requested changes Jul 27, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jul 27, 2018

mmatur requested changes Jul 27, 2018

View reviewed changes

dtomcej force-pushed the issue-1957 branch from 1d8d7b5 to cb2eb21 Compare July 27, 2018 18:45

dtomcej changed the base branch from master to v1.7 July 27, 2018 18:46

dtomcej force-pushed the issue-1957 branch from cb2eb21 to a3f3120 Compare July 27, 2018 18:50

dtomcej added this to the 1.7 milestone Jul 27, 2018

dtomcej removed the contributor/waiting-for-corrections label Jul 27, 2018

juliens approved these changes Jul 30, 2018

View reviewed changes

mmatur approved these changes Jul 30, 2018

View reviewed changes

emilevauge requested changes Jul 30, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jul 30, 2018

emilevauge approved these changes Jul 31, 2018

View reviewed changes

traefiker removed the contributor/waiting-for-corrections label Jul 31, 2018

emilevauge added the status/3-needs-merge label Jul 31, 2018

emilevauge removed the status/2-needs-review label Jul 31, 2018

traefiker added bot/merge-retry-1 status/4-merge-in-progress and removed bot/merge-retry-1 labels Jul 31, 2018

dtomcej added 3 commits July 31, 2018 09:02

Fix middleware redirects

b112b3d

fix pathprefixstripregex

1950de2

add double slash and other edge cases

25dc71f

traefiker force-pushed the issue-1957 branch from 6112149 to 25dc71f Compare July 31, 2018 09:02

traefiker removed the status/4-merge-in-progress label Jul 31, 2018

traefiker merged commit 91cafd1 into traefik:v1.7 Jul 31, 2018

traefiker removed the status/3-needs-merge label Jul 31, 2018

traefiker mentioned this pull request Jul 31, 2018

Wrong behaviour of HTTP -> HTTPS redirect with PathPrefixStrip annotation #1957

Closed

This was referenced Aug 6, 2018

Path jump problem #3732

Closed

Wrong behaviour of HTTP -> HTTPS redirect with request-modifier (ReplacePathRegex) #3741

Closed

fwang0 mentioned this pull request Oct 16, 2018

Traefik issues, redirect login error jhipster/generator-jhipster#8578

Closed

1 task

sagikazarmark mentioned this pull request Oct 19, 2018

PathPrefixStrip rule does not work with SSL redirection #4085

Open

sagikazarmark mentioned this pull request Oct 19, 2018

Replace builtin SSL redirection with manual one banzaicloud/banzai-charts#415

Merged

mebel-christ mentioned this pull request Mar 10, 2023

Application redirect (302) in combination with PathPrefix results in unexpected behaviour #9767

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct Entrypoint Redirect with Stripped or Added Path #3631

Correct Entrypoint Redirect with Stripped or Added Path #3631

dtomcej commented Jul 16, 2018 •

edited

Loading

kachkaev commented Jul 24, 2018

mmatur Jul 27, 2018

mmatur Jul 27, 2018

dtomcej Jul 27, 2018

mmatur Jul 27, 2018

mmatur Jul 27, 2018

mmatur Jul 27, 2018

mmatur Jul 27, 2018

mmatur Jul 27, 2018

mmatur commented Jul 27, 2018

mmatur Jul 27, 2018

dtomcej Jul 27, 2018

dtomcej commented Jul 27, 2018

dtomcej commented Jul 28, 2018

juliens left a comment

mmatur left a comment

emilevauge left a comment

emilevauge Jul 30, 2018

dtomcej Jul 30, 2018

emilevauge left a comment

edasque commented Aug 13, 2018

marianschmotzer commented Aug 15, 2018

edasque commented Aug 15, 2018

marianschmotzer commented Aug 15, 2018 •

edited by ldez

Loading

marianschmotzer commented Sep 3, 2018

edasque commented Sep 3, 2018

dtomcej commented Sep 3, 2018

edasque commented Sep 4, 2018

marianschmotzer commented Sep 5, 2018 via email •

edited by ldez

Loading

Correct Entrypoint Redirect with Stripped or Added Path #3631

Correct Entrypoint Redirect with Stripped or Added Path #3631

Conversation

dtomcej commented Jul 16, 2018 • edited Loading

What does this PR do?

Motivation

More

kachkaev commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmatur commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtomcej commented Jul 27, 2018

dtomcej commented Jul 28, 2018

juliens left a comment

Choose a reason for hiding this comment

mmatur left a comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

edasque commented Aug 13, 2018

marianschmotzer commented Aug 15, 2018

edasque commented Aug 15, 2018

marianschmotzer commented Aug 15, 2018 • edited by ldez Loading

marianschmotzer commented Sep 3, 2018

edasque commented Sep 3, 2018

dtomcej commented Sep 3, 2018

edasque commented Sep 4, 2018

marianschmotzer commented Sep 5, 2018 via email • edited by ldez Loading

dtomcej commented Jul 16, 2018 •

edited

Loading

marianschmotzer commented Aug 15, 2018 •

edited by ldez

Loading

marianschmotzer commented Sep 5, 2018 via email •

edited by ldez

Loading