Skip to content
Permalink
Browse files Browse the repository at this point in the history
Security: Fix do not forward login cookie in outgoing requests
(cherry picked from commit 54a32fc83b233f5910495b5fcca0b4f881221538)
  • Loading branch information
marefr authored and dsotirakis committed Sep 29, 2022
1 parent 4dd56e4 commit c658816
Show file tree
Hide file tree
Showing 6 changed files with 37 additions and 11 deletions.
3 changes: 2 additions & 1 deletion pkg/api/metrics_test.go
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/web/webtest"

"golang.org/x/oauth2"
Expand Down Expand Up @@ -498,7 +499,7 @@ func TestAPIEndpoint_Metrics_ParseDashboardQueryParams(t *testing.T) {
// `/ds/query` endpoint test
func TestAPIEndpoint_Metrics_QueryMetricsV2(t *testing.T) {
qds := query.ProvideService(
nil,
setting.NewCfg(),
nil,
nil,
&fakePluginRequestValidator{},
Expand Down
2 changes: 1 addition & 1 deletion pkg/api/pluginproxy/ds_proxy.go
Expand Up @@ -214,7 +214,7 @@ func (proxy *DataSourceProxy) director(req *http.Request) {
}
}

proxyutil.ClearCookieHeader(req, keepCookieNames)
proxyutil.ClearCookieHeader(req, keepCookieNames, []string{proxy.cfg.LoginCookieName})
req.Header.Set("User-Agent", fmt.Sprintf("Grafana/%s", setting.BuildVersion))

jsonData := make(map[string]interface{})
Expand Down
2 changes: 1 addition & 1 deletion pkg/api/plugins.go
Expand Up @@ -555,7 +555,7 @@ func (hs *HTTPServer) makePluginResourceRequest(w http.ResponseWriter, req *http
}
}

proxyutil.ClearCookieHeader(req, keepCookieModel.KeepCookies)
proxyutil.ClearCookieHeader(req, keepCookieModel.KeepCookies, []string{hs.Cfg.LoginCookieName})
proxyutil.PrepareProxyRequest(req)

body, err := ioutil.ReadAll(req.Body)
Expand Down
2 changes: 1 addition & 1 deletion pkg/services/query/query_test.go
Expand Up @@ -5,9 +5,9 @@ import (
"net/http"
"testing"

"github.com/grafana/grafana-plugin-sdk-go/backend"
"golang.org/x/oauth2"

"github.com/grafana/grafana-plugin-sdk-go/backend"
"github.com/grafana/grafana/pkg/api/dtos"
"github.com/grafana/grafana/pkg/components/simplejson"
"github.com/grafana/grafana/pkg/models"
Expand Down
23 changes: 18 additions & 5 deletions pkg/util/proxyutil/proxyutil.go
Expand Up @@ -3,6 +3,7 @@ package proxyutil
import (
"net"
"net/http"
"sort"
)

// PrepareProxyRequest prepares a request for being proxied.
Expand All @@ -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)
}
}
Expand Down
16 changes: 14 additions & 2 deletions pkg/util/proxyutil/proxyutil_test.go
Expand Up @@ -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")
})

Expand All @@ -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"))
})
}

0 comments on commit c658816

Please sign in to comment.