Skip to content

Commit

Permalink
Security: Sync security changes on main (#45083)
Browse files Browse the repository at this point in the history
* * Teams: Appropriately apply user id filter in /api/teams/:id and /api/teams/search
* Teams: Ensure that users searching for teams are only able see teams they have access to
* Teams: Require teamGuardian admin privileges to list team members
* Teams: Prevent org viewers from administering teams
* Teams: Add org_id condition to team count query
* Teams: clarify permission requirements in teams api docs
* Teams: expand scenarios for team search tests
* Teams: mock teamGuardian in tests

Co-authored-by: Dan Cech <dcech@grafana.com>

* remove duplicate WHERE statement

* Fix for CVE-2022-21702

(cherry picked from commit 202d7c190082c094bc1dc13f7fe9464746c37f9e)

* Lint and test fixes

(cherry picked from commit 3e6b67d5504abf4a1d7b8d621f04d062c048e981)

* check content type properly

(cherry picked from commit 70b4458892bf2f776302720c10d24c9ff34edd98)

* basic csrf origin check

(cherry picked from commit 3adaa5ff39832364f6390881fb5b42ad47df92e1)

* compare origin to host

(cherry picked from commit 5443892699e8ed42836bb2b9a44744ff3e970f42)

* simplify url parsing

(cherry picked from commit b2ffbc9513fed75468628370a48b929d30af2b1d)

* check csrf for GET requests, only compare origin

(cherry picked from commit 8b81dc12d8f8a1f07852809c5b4d44f0f0b1d709)

* parse content type properly

(cherry picked from commit 16f76f4902e6f2188bea9606c68b551af186bdc0)

* mentioned get in the comment

(cherry picked from commit a7e61811ef8ae558ce721e2e3fed04ce7a5a5345)

* add content-type: application/json to test HTTP requests

* fix pluginproxy test

* Fix linter when comparing errors

Co-authored-by: Kevin Minehart <kmineh0151@gmail.com>
Co-authored-by: Dan Cech <dcech@grafana.com>
Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
Co-authored-by: Serge Zaitsev <serge.zaitsev@grafana.com>
Co-authored-by: Vardan Torosyan <vardants@gmail.com>
  • Loading branch information
6 people committed Feb 9, 2022
1 parent d3d7411 commit 605d056
Show file tree
Hide file tree
Showing 33 changed files with 290 additions and 44 deletions.
8 changes: 7 additions & 1 deletion docs/sources/http_api/team.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,13 @@ aliases = ["/docs/grafana/latest/http_api/team/"]

# Team API

This API can be used to create/update/delete Teams and to add/remove users to Teams. All actions require that the user has the Admin role for the organization.
This API can be used to manage Teams and Team Memberships.

Access to these API endpoints is restricted as follows:

- All authenticated users are able to view details of teams they are a member of.
- Organization Admins are able to manage all teams and team members.
- If the `editors_can_admin` configuration flag is enabled, Organization Editors are able to view details of all teams and to manage teams that they are Admin members of.

## Team Search With Paging

Expand Down
3 changes: 3 additions & 0 deletions pkg/api/admin_users_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ func putAdminScenario(t *testing.T, desc string, url string, routePattern string
sc := setupScenarioContext(t, url)
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.UserId = testUserID
sc.context.OrgId = testOrgID
Expand Down Expand Up @@ -346,6 +347,7 @@ func adminRevokeUserAuthTokenScenario(t *testing.T, desc string, url string, rou
sc.userAuthTokenService = fakeAuthTokenService
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.UserId = testUserID
sc.context.OrgId = testOrgID
Expand Down Expand Up @@ -464,6 +466,7 @@ func adminCreateUserScenario(t *testing.T, desc string, url string, routePattern
sc := setupScenarioContext(t, url)
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.UserId = testUserID

Expand Down
1 change: 1 addition & 0 deletions pkg/api/alerting_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ func postAlertScenario(t *testing.T, hs *HTTPServer, desc string, url string, ro
sc := setupScenarioContext(t, url)
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.UserId = testUserID
sc.context.OrgId = testOrgID
Expand Down
4 changes: 4 additions & 0 deletions pkg/api/annotations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ func postAnnotationScenario(t *testing.T, desc string, url string, routePattern
sc := setupScenarioContext(t, url)
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.UserId = testUserID
sc.context.OrgId = testOrgID
Expand All @@ -304,6 +305,7 @@ func putAnnotationScenario(t *testing.T, desc string, url string, routePattern s
sc := setupScenarioContext(t, url)
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.UserId = testUserID
sc.context.OrgId = testOrgID
Expand All @@ -328,6 +330,7 @@ func patchAnnotationScenario(t *testing.T, desc string, url string, routePattern
sc := setupScenarioContext(t, url)
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.UserId = testUserID
sc.context.OrgId = testOrgID
Expand All @@ -353,6 +356,7 @@ func deleteAnnotationsScenario(t *testing.T, desc string, url string, routePatte
sc := setupScenarioContext(t, url)
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.UserId = testUserID
sc.context.OrgId = testOrgID
Expand Down
2 changes: 1 addition & 1 deletion pkg/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func (hs *HTTPServer) registerRoutes() {
reqEditorRole := middleware.ReqEditorRole
reqOrgAdmin := middleware.ReqOrgAdmin
reqOrgAdminFolderAdminOrTeamAdmin := middleware.OrgAdminFolderAdminOrTeamAdmin
reqCanAccessTeams := middleware.AdminOrFeatureEnabled(hs.Cfg.EditorsCanAdmin)
reqCanAccessTeams := middleware.AdminOrEditorAndFeatureEnabled(hs.Cfg.EditorsCanAdmin)
reqSnapshotPublicModeOrSignedIn := middleware.SnapshotPublicModeOrSignedIn(hs.Cfg)
redirectFromLegacyPanelEditURL := middleware.RedirectFromLegacyPanelEditURL(hs.Cfg)
authorize := acmiddleware.Middleware(hs.AccessControl)
Expand Down
6 changes: 5 additions & 1 deletion pkg/api/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ func (sc *scenarioContext) fakeReq(method, url string) *scenarioContext {
sc.resp = httptest.NewRecorder()
req, err := http.NewRequest(method, url, nil)
require.NoError(sc.t, err)
req.Header.Add("Content-Type", "application/json")
sc.req = req

return sc
Expand All @@ -117,6 +118,8 @@ func (sc *scenarioContext) fakeReqWithParams(method, url string, queryParams map
panic(fmt.Sprintf("Making request failed: %s", err))
}

req.Header.Add("Content-Type", "application/json")

q := req.URL.Query()
for k, v := range queryParams {
q.Add(k, v)
Expand All @@ -129,6 +132,7 @@ func (sc *scenarioContext) fakeReqWithParams(method, url string, queryParams map
func (sc *scenarioContext) fakeReqNoAssertions(method, url string) *scenarioContext {
sc.resp = httptest.NewRecorder()
req, _ := http.NewRequest(method, url, nil)
req.Header.Add("Content-Type", "application/json")
sc.req = req

return sc
Expand All @@ -140,7 +144,7 @@ func (sc *scenarioContext) fakeReqNoAssertionsWithCookie(method, url string, coo

req, _ := http.NewRequest(method, url, nil)
req.Header = http.Header{"Cookie": sc.resp.Header()["Set-Cookie"]}

req.Header.Add("Content-Type", "application/json")
sc.req = req

return sc
Expand Down
1 change: 1 addition & 0 deletions pkg/api/dashboard_permission_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,7 @@ func updateDashboardPermissionScenario(t *testing.T, ctx updatePermissionContext

sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(ctx.cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.OrgId = testOrgID
sc.context.UserId = testUserID
Expand Down
4 changes: 4 additions & 0 deletions pkg/api/dashboard_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
func TestGetHomeDashboard(t *testing.T) {
httpReq, err := http.NewRequest(http.MethodGet, "", nil)
require.NoError(t, err)
httpReq.Header.Add("Content-Type", "application/json")
req := &models.ReqContext{SignedInUser: &models.SignedInUser{}, Context: &web.Context{Req: httpReq}}
cfg := setting.NewCfg()
cfg.StaticRootPath = "../../public/"
Expand Down Expand Up @@ -1023,6 +1024,7 @@ func postDashboardScenario(t *testing.T, desc string, url string, routePattern s
sc := setupScenarioContext(t, url)
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.SignedInUser = &models.SignedInUser{OrgId: cmd.OrgId, UserId: cmd.UserId}

Expand Down Expand Up @@ -1056,6 +1058,7 @@ func postDiffScenario(t *testing.T, desc string, url string, routePattern string
sc := setupScenarioContext(t, url)
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.SignedInUser = &models.SignedInUser{
OrgId: testOrgID,
Expand Down Expand Up @@ -1095,6 +1098,7 @@ func restoreDashboardVersionScenario(t *testing.T, desc string, url string, rout
sc.sqlStore = mockSQLStore
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.SignedInUser = &models.SignedInUser{
OrgId: testOrgID,
Expand Down
1 change: 1 addition & 0 deletions pkg/api/folder_permission_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,7 @@ func updateFolderPermissionScenario(t *testing.T, ctx updatePermissionContext, h

sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(ctx.cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.OrgId = testOrgID
sc.context.UserId = testUserID
Expand Down
2 changes: 2 additions & 0 deletions pkg/api/folder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ func createFolderScenario(t *testing.T, desc string, url string, routePattern st
sc := setupScenarioContext(t, url)
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.SignedInUser = &models.SignedInUser{OrgId: testOrgID, UserId: testUserID}

Expand Down Expand Up @@ -184,6 +185,7 @@ func updateFolderScenario(t *testing.T, desc string, url string, routePattern st
sc := setupScenarioContext(t, url)
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.SignedInUser = &models.SignedInUser{OrgId: testOrgID, UserId: testUserID}

Expand Down
1 change: 1 addition & 0 deletions pkg/api/frontend_logging_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ func logSentryEventScenario(t *testing.T, desc string, event frontendlogging.Fro
handler := routing.Wrap(func(c *models.ReqContext) response.Response {
sc.context = c
c.Req.Body = mockRequestBody(event)
c.Req.Header.Add("Content-Type", "application/json")
return loggingHandler(c)
})

Expand Down
1 change: 1 addition & 0 deletions pkg/api/http_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,7 @@ func (hs *HTTPServer) addMiddlewaresAndStaticRoutes() {
}

m.Use(middleware.Recovery(hs.Cfg))
m.UseMiddleware(middleware.CSRF(hs.Cfg.LoginCookieName))

hs.mapStatic(m, hs.Cfg.StaticRootPath, "build", "public/build")
hs.mapStatic(m, hs.Cfg.StaticRootPath, "", "public", "/public/views/swagger.html")
Expand Down
1 change: 1 addition & 0 deletions pkg/api/pluginproxy/ds_proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ func (t *handleResponseTransport) RoundTrip(req *http.Request) (*http.Response,
return nil, err
}
res.Header.Del("Set-Cookie")
proxyutil.SetProxyResponseHeaders(res.Header)
return res, nil
}

Expand Down
14 changes: 14 additions & 0 deletions pkg/api/pluginproxy/ds_proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,20 @@ func TestDataSourceProxy_requestHandling(t *testing.T) {
assert.Equal(t, "important_cookie=important_value", proxy.ctx.Resp.Header().Get("Set-Cookie"))
})

t.Run("When response should set Content-Security-Policy header", func(t *testing.T) {
ctx, ds := setUp(t)
var routes []*plugins.Route
secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore())
dsService := datasources.ProvideService(bus.New(), nil, secretsService, &acmock.Mock{})
proxy, err := NewDataSourceProxy(ds, routes, ctx, "/render", &setting.Cfg{}, httpClientProvider, &oauthtoken.Service{}, dsService, tracer)
require.NoError(t, err)

proxy.HandleRequest()

require.NoError(t, writeErr)
assert.Equal(t, "sandbox", proxy.ctx.Resp.Header().Get("Content-Security-Policy"))
})

t.Run("Data source returns status code 401", func(t *testing.T) {
ctx, ds := setUp(t, setUpCfg{
writeCb: func(w http.ResponseWriter, r *http.Request) {
Expand Down
8 changes: 7 additions & 1 deletion pkg/api/pluginproxy/pluginproxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,5 +83,11 @@ func NewApiPluginProxy(ctx *models.ReqContext, proxyPath string, route *plugins.
}
}

return &httputil.ReverseProxy{Director: director}
return &httputil.ReverseProxy{Director: director, ModifyResponse: modifyResponse}
}

func modifyResponse(resp *http.Response) error {
proxyutil.SetProxyResponseHeaders(resp.Header)

return nil
}
41 changes: 41 additions & 0 deletions pkg/api/pluginproxy/pluginproxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"io/ioutil"
"net/http"
"net/http/httptest"
"testing"

"github.com/grafana/grafana/pkg/models"
Expand Down Expand Up @@ -238,6 +239,46 @@ func TestPluginProxy(t *testing.T) {
require.NoError(t, err)
require.Equal(t, `{ "url": "https://dynamic.grafana.com", "secret": "123" }`, string(content))
})

t.Run("When proxying a request should set expected response headers", func(t *testing.T) {
requestHandled := false
backendServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
_, _ = w.Write([]byte("I am the backend"))
requestHandled = true
}))
t.Cleanup(backendServer.Close)

responseWriter := web.NewResponseWriter("GET", httptest.NewRecorder())

route := &plugins.Route{
Path: "/",
URL: backendServer.URL,
}

ctx := &models.ReqContext{
SignedInUser: &models.SignedInUser{},
Context: &web.Context{
Req: httptest.NewRequest("GET", "/", nil),
Resp: responseWriter,
},
}
store := mockstore.NewSQLStoreMock()

store.ExpectedPluginSetting = &models.PluginSetting{
SecureJsonData: map[string][]byte{},
}
proxy := NewApiPluginProxy(ctx, "", route, "", &setting.Cfg{}, store, secretsService)
proxy.ServeHTTP(ctx.Resp, ctx.Req)

for {
if requestHandled {
break
}
}

require.Equal(t, "sandbox", ctx.Resp.Header().Get("Content-Security-Policy"))
})
}

// getPluginProxiedRequest is a helper for easier setup of tests based on global config and ReqContext.
Expand Down
3 changes: 3 additions & 0 deletions pkg/api/plugins.go
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,9 @@ func (hs *HTTPServer) flushStream(stream callResourceClientResponseStream, w htt
w.Header().Add(k, v)
}
}

proxyutil.SetProxyResponseHeaders(w.Header())

w.WriteHeader(resp.Status)
}

Expand Down
48 changes: 48 additions & 0 deletions pkg/api/plugins_test.go
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
package api

import (
"context"
"encoding/json"
"fmt"
"io/ioutil"
"net/http"
"net/http/httptest"
"os"
"path/filepath"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/grafana/grafana-plugin-sdk-go/backend"
"github.com/grafana/grafana/pkg/bus"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/models"
Expand Down Expand Up @@ -185,6 +189,28 @@ func Test_GetPluginAssets(t *testing.T) {
})
}

func TestMakePluginResourceRequest(t *testing.T) {
pluginClient := &fakePluginClient{}
hs := HTTPServer{
Cfg: setting.NewCfg(),
log: log.New(),
pluginClient: pluginClient,
}
req := httptest.NewRequest(http.MethodGet, "/", nil)
resp := httptest.NewRecorder()
pCtx := backend.PluginContext{}
err := hs.makePluginResourceRequest(resp, req, pCtx)
require.NoError(t, err)

for {
if resp.Flushed {
break
}
}

require.Equal(t, "sandbox", resp.Header().Get("Content-Security-Policy"))
}

func callGetPluginAsset(sc *scenarioContext) {
sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec()
}
Expand Down Expand Up @@ -221,3 +247,25 @@ type logger struct {
func (l *logger) Warn(msg string, ctx ...interface{}) {
l.warnings = append(l.warnings, msg)
}

type fakePluginClient struct {
plugins.Client

req *backend.CallResourceRequest
}

func (c *fakePluginClient) CallResource(ctx context.Context, req *backend.CallResourceRequest, sender backend.CallResourceResponseSender) error {
c.req = req
bytes, err := json.Marshal(map[string]interface{}{
"message": "hello",
})
if err != nil {
return err
}

return sender.Send(&backend.CallResourceResponse{
Status: http.StatusOK,
Headers: make(map[string][]string),
Body: bytes,
})
}
1 change: 1 addition & 0 deletions pkg/api/short_url_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ func createShortURLScenario(t *testing.T, desc string, url string, routePattern
sc := setupScenarioContext(t, url)
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
c.Req.Body = mockRequestBody(cmd)
c.Req.Header.Add("Content-Type", "application/json")
sc.context = c
sc.context.SignedInUser = &models.SignedInUser{OrgId: testOrgID, UserId: testUserID}

Expand Down

0 comments on commit 605d056

Please sign in to comment.