From c7a0f5b81e6bf18a8fc6773089b3a844d297bbc5 Mon Sep 17 00:00:00 2001 From: Damien Grisonnet Date: Mon, 22 Nov 2021 19:40:23 +0100 Subject: [PATCH] pkg/util/proxy: escape redirect URL Escape the redirect URL written in the Location HTTP header for correctness purposes. Signed-off-by: Damien Grisonnet --- .../apimachinery/pkg/util/proxy/transport.go | 31 +++++++++++-------- .../pkg/util/proxy/transport_test.go | 29 +++++++++++++++++ 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/proxy/transport.go b/staging/src/k8s.io/apimachinery/pkg/util/proxy/transport.go index e2af6d7413e..b9200993703 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/proxy/transport.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/proxy/transport.go @@ -106,7 +106,11 @@ func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) { } if redirect := resp.Header.Get("Location"); redirect != "" { - resp.Header.Set("Location", t.rewriteURL(redirect, req.URL, req.Host)) + targetURL, err := url.Parse(redirect) + if err != nil { + return nil, errors.NewInternalError(fmt.Errorf("error trying to parse Location header: %v", err)) + } + resp.Header.Set("Location", t.rewriteURL(targetURL, req.URL, req.Host)) return resp, nil } @@ -128,14 +132,8 @@ func (rt *Transport) WrappedRoundTripper() http.RoundTripper { // rewriteURL rewrites a single URL to go through the proxy, if the URL refers // to the same host as sourceURL, which is the page on which the target URL -// occurred, or if the URL matches the sourceRequestHost. If any error occurs (e.g. -// parsing), it returns targetURL. -func (t *Transport) rewriteURL(targetURL string, sourceURL *url.URL, sourceRequestHost string) string { - url, err := url.Parse(targetURL) - if err != nil { - return targetURL - } - +// occurred, or if the URL matches the sourceRequestHost. +func (t *Transport) rewriteURL(url *url.URL, sourceURL *url.URL, sourceRequestHost string) string { // Example: // When API server processes a proxy request to a service (e.g. /api/v1/namespace/foo/service/bar/proxy/), // the sourceURL.Host (i.e. req.URL.Host) is the endpoint IP address of the service. The @@ -151,7 +149,7 @@ func (t *Transport) rewriteURL(targetURL string, sourceURL *url.URL, sourceReque isDifferentHost := url.Host != "" && url.Host != sourceURL.Host && url.Host != sourceRequestHost isRelative := !strings.HasPrefix(url.Path, "/") if isDifferentHost || isRelative { - return targetURL + return url.String() } // Do not rewrite scheme and host if the Transport has empty scheme and host @@ -178,7 +176,7 @@ func (t *Transport) rewriteURL(targetURL string, sourceURL *url.URL, sourceReque // rewriteHTML scans the HTML for tags with url-valued attributes, and updates // those values with the urlRewriter function. The updated HTML is output to the // writer. -func rewriteHTML(reader io.Reader, writer io.Writer, urlRewriter func(string) string) error { +func rewriteHTML(reader io.Reader, writer io.Writer, urlRewriter func(*url.URL) string) error { // Note: This assumes the content is UTF-8. tokenizer := html.NewTokenizer(reader) @@ -193,7 +191,14 @@ func rewriteHTML(reader io.Reader, writer io.Writer, urlRewriter func(string) st if urlAttrs, ok := atomsToAttrs[token.DataAtom]; ok { for i, attr := range token.Attr { if urlAttrs.Has(attr.Key) { - token.Attr[i].Val = urlRewriter(attr.Val) + url, err := url.Parse(attr.Val) + if err != nil { + // Do not rewrite the URL if it isn't valid. It is intended not + // to error here to prevent the inability to understand the + // content of the body to cause a fatal error. + continue + } + token.Attr[i].Val = urlRewriter(url) } } } @@ -248,7 +253,7 @@ func (t *Transport) rewriteResponse(req *http.Request, resp *http.Response) (*ht return resp, nil } - urlRewriter := func(targetUrl string) string { + urlRewriter := func(targetUrl *url.URL) string { return t.rewriteURL(targetUrl, req.URL, req.Host) } err := rewriteHTML(reader, writer, urlRewriter) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/proxy/transport_test.go b/staging/src/k8s.io/apimachinery/pkg/util/proxy/transport_test.go index 74511eb36fd..94c4b2e0144 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/proxy/transport_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/proxy/transport_test.go @@ -205,6 +205,35 @@ func TestProxyTransport(t *testing.T) { contentType: "text/html", forwardedURI: "/proxy/node/node1:10250/logs/log.log%00%3Cscript%3Ealert%281%29%3C/script%3E", }, + "redirect rel must be escaped": { + sourceURL: "http://mynode.com/redirect", + transport: testTransport, + redirect: "/redirected/target/%00/", + redirectWant: "http://foo.com/proxy/node/node1:10250/redirected/target/%00%3Cscript%3Ealert%281%29%3C/script%3E/", + forwardedURI: "/proxy/node/node1:10250/redirect", + }, + "redirect abs same host must be escaped": { + sourceURL: "http://mynode.com/redirect", + transport: testTransport, + redirect: "http://mynode.com/redirected/target/%00/", + redirectWant: "http://foo.com/proxy/node/node1:10250/redirected/target/%00%3Cscript%3Ealert%281%29%3C/script%3E/", + forwardedURI: "/proxy/node/node1:10250/redirect", + }, + "redirect abs other host must be escaped": { + sourceURL: "http://mynode.com/redirect", + transport: testTransport, + redirect: "http://example.com/redirected/target/%00/", + redirectWant: "http://example.com/redirected/target/%00%3Cscript%3Ealert%281%29%3C/script%3E/", + forwardedURI: "/proxy/node/node1:10250/redirect", + }, + "redirect abs use reqHost no host no scheme must be escaped": { + sourceURL: "http://mynode.com/redirect", + transport: emptyHostAndSchemeTransport, + redirect: "http://10.0.0.1:8001/redirected/target/%00/", + redirectWant: "http://10.0.0.1:8001/proxy/node/node1:10250/redirected/target/%00%3Cscript%3Ealert%281%29%3C/script%3E/", + forwardedURI: "/proxy/node/node1:10250/redirect", + reqHost: "10.0.0.1:8001", + }, } testItem := func(name string, item *Item) {