[main] Plugin fixes (#57399)

* Plugins: Remove support for V1 manifests

* Plugins: Make proxy endpoints not leak sensitive HTTP headers

* Security: Fix do not forward login cookie in outgoing requests

(cherry picked from commit 4539c33fce)

Co-authored-by: Will Browne <wbrowne@users.noreply.github.com>

pkg/util/proxyutil/proxyutil.go

@@ -3,6 +3,7 @@ package proxyutil
 import (
 	"net"
 	"net/http"
+	"sort"
 )
 
 // PrepareProxyRequest prepares a request for being proxied.
@@ -26,19 +27,31 @@ func PrepareProxyRequest(req *http.Request) {
 	}
 }
 
-// ClearCookieHeader clear cookie header, except for cookies specified to be kept.
-func ClearCookieHeader(req *http.Request, keepCookiesNames []string) {
-	var keepCookies []*http.Cookie
+// ClearCookieHeader clear cookie header, except for cookies specified to be kept (keepCookiesNames) if not in skipCookiesNames.
+func ClearCookieHeader(req *http.Request, keepCookiesNames []string, skipCookiesNames []string) {
+	keepCookies := map[string]*http.Cookie{}
 	for _, c := range req.Cookies() {
 		for _, v := range keepCookiesNames {
 			if c.Name == v {
-				keepCookies = append(keepCookies, c)
+				keepCookies[c.Name] = c
 			}
 		}
 	}
 
+	for _, v := range skipCookiesNames {
+		delete(keepCookies, v)
+	}
+
 	req.Header.Del("Cookie")
-	for _, c := range keepCookies {
+
+	sortedCookies := []string{}
+	for name := range keepCookies {
+		sortedCookies = append(sortedCookies, name)
+	}
+	sort.Strings(sortedCookies)
+
+	for _, name := range sortedCookies {
+		c := keepCookies[name]
 		req.AddCookie(c)
 	}
 }

pkg/util/proxyutil/proxyutil_test.go

@@ -49,7 +49,7 @@ func TestClearCookieHeader(t *testing.T) {
 		require.NoError(t, err)
 		req.AddCookie(&http.Cookie{Name: "cookie"})
 
-		ClearCookieHeader(req, nil)
+		ClearCookieHeader(req, nil, nil)
 		require.NotContains(t, req.Header, "Cookie")
 	})
 
@@ -60,8 +60,20 @@ func TestClearCookieHeader(t *testing.T) {
 		req.AddCookie(&http.Cookie{Name: "cookie2"})
 		req.AddCookie(&http.Cookie{Name: "cookie3"})
 
-		ClearCookieHeader(req, []string{"cookie1", "cookie3"})
+		ClearCookieHeader(req, []string{"cookie1", "cookie3"}, nil)
 		require.Contains(t, req.Header, "Cookie")
 		require.Equal(t, "cookie1=; cookie3=", req.Header.Get("Cookie"))
 	})
+
+	t.Run("Clear cookie header with cookies to keep and skip should clear Cookie header and keep cookies", func(t *testing.T) {
+		req, err := http.NewRequest(http.MethodGet, "/", nil)
+		require.NoError(t, err)
+		req.AddCookie(&http.Cookie{Name: "cookie1"})
+		req.AddCookie(&http.Cookie{Name: "cookie2"})
+		req.AddCookie(&http.Cookie{Name: "cookie3"})
+
+		ClearCookieHeader(req, []string{"cookie1", "cookie3"}, []string{"cookie3"})
+		require.Contains(t, req.Header, "Cookie")
+		require.Equal(t, "cookie1=", req.Header.Get("Cookie"))
+	})
 }

pkg/util/proxyutil/reverse_proxy.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	glog "github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/services/contexthandler"
 )
 
 // StatusClientClosedRequest A non-standard status code introduced by nginx
@@ -66,6 +67,13 @@ func NewReverseProxy(logger glog.Logger, director func(*http.Request), opts ...R
 // wrapDirector wraps a director and adds additional functionality.
 func wrapDirector(d func(*http.Request)) func(req *http.Request) {
 	return func(req *http.Request) {
+		list := contexthandler.AuthHTTPHeaderListFromContext(req.Context())
+		if list != nil {
+			for _, name := range list.Items {
+				req.Header.Del(name)
+			}
+		}
+
 		d(req)
 		PrepareProxyRequest(req)
 

pkg/util/proxyutil/reverse_proxy_test.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/services/contexthandler"
 	"github.com/stretchr/testify/require"
 )
 
@@ -30,6 +31,11 @@ func TestReverseProxy(t *testing.T) {
 		req.Header.Set("Referer", "https://test.com/api")
 		req.RemoteAddr = "10.0.0.1"
 
+		const customHeader = "X-CUSTOM"
+		req.Header.Set(customHeader, "val")
+		ctx := contexthandler.WithAuthHTTPHeader(req.Context(), customHeader)
+		req = req.WithContext(ctx)
+
 		rp := NewReverseProxy(log.New("test"), func(req *http.Request) {
 			req.Header.Set("X-KEY", "value")
 		})
@@ -49,6 +55,7 @@ func TestReverseProxy(t *testing.T) {
 		require.Empty(t, resp.Cookies())
 		require.Equal(t, "sandbox", resp.Header.Get("Content-Security-Policy"))
 		require.NoError(t, resp.Body.Close())
+		require.Empty(t, actualReq.Header.Get(customHeader))
 	})
 
 	t.Run("When proxying a request using WithModifyResponse should call it before default ModifyResponse func", func(t *testing.T) {