Security: Sync security changes on main (#45083) (#45140)

* Security: Sync security changes on main (#45083)

* * 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>
(cherry picked from commit 605d056136)

* Apply suggestions from code review

* remove uneeded test fix from patch

Co-authored-by: Dimitris Sotirakis <sotirakis.dim@gmail.com>
Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
Co-authored-by: jguer <joao.guerreiro@grafana.com>
This commit is contained in:
Vardan Torosyan
2022-02-09 20:43:42 +01:00
committed by GitHub
co-authored by Dimitris Sotirakis Marcus Efraimsson jguer
parent 1f1d599815
commit ff8e53a347
32 changed files with 283 additions and 44 deletions
+3
View File
@@ -316,6 +316,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
@@ -372,6 +373,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
@@ -473,6 +475,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
+1
View File
@@ -168,6 +168,7 @@ func postAlertScenario(t *testing.T, desc string, url string, routePattern strin
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
+4
View File
@@ -277,6 +277,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
@@ -302,6 +303,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
@@ -326,6 +328,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
@@ -351,6 +354,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
+1 -1
View File
@@ -28,7 +28,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)
+5 -1
View File
@@ -101,6 +101,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
@@ -116,6 +117,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)
@@ -128,6 +131,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
@@ -139,7 +143,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
+1
View File
@@ -434,6 +434,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
+4
View File
@@ -35,6 +35,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/"
@@ -1133,6 +1134,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}
@@ -1166,6 +1168,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,
@@ -1201,6 +1204,7 @@ func restoreDashboardVersionScenario(t *testing.T, desc string, url string, rout
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,
+1
View File
@@ -403,6 +403,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
+2
View File
@@ -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}
@@ -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}
+1
View File
@@ -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)
})
+1
View File
@@ -441,6 +441,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")
+1
View File
@@ -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
}
+14
View File
@@ -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) {
+7 -1
View File
@@ -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
}
+36
View File
@@ -4,6 +4,7 @@ import (
"context"
"io/ioutil"
"net/http"
"net/http/httptest"
"testing"
"github.com/grafana/grafana/pkg/bus"
@@ -245,6 +246,41 @@ 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,
},
}
proxy := NewApiPluginProxy(ctx, "", route, "", &setting.Cfg{}, 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.
+3
View File
@@ -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)
}
+48
View File
@@ -1,9 +1,12 @@
package api
import (
"context"
"encoding/json"
"fmt"
"io/ioutil"
"net/http"
"net/http/httptest"
"os"
"path/filepath"
"testing"
@@ -11,6 +14,7 @@ import (
"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"
@@ -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()
}
@@ -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
View File
@@ -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}
+16 -6
View File
@@ -132,16 +132,11 @@ func (hs *HTTPServer) SearchTeams(c *models.ReqContext) response.Response {
page = 1
}
var userIdFilter int64
if hs.Cfg.EditorsCanAdmin && c.OrgRole != models.ROLE_ADMIN {
userIdFilter = c.SignedInUser.UserId
}
query := models.SearchTeamsQuery{
OrgId: c.OrgId,
Query: c.Query("query"),
Name: c.Query("name"),
UserIdFilter: userIdFilter,
UserIdFilter: userFilter(hs.Cfg.EditorsCanAdmin, c),
Page: page,
Limit: perPage,
SignedInUser: c.SignedInUser,
@@ -188,17 +183,32 @@ func (hs *HTTPServer) getTeamAccessControlMetadata(c *models.ReqContext, teamID
return accesscontrol.GetResourcesMetadata(c.Req.Context(), userPermissions, "teams", teamIDs)[key], nil
}
// UserFilter returns the user ID used in a filter when querying a team
// 1. If the user is a viewer or editor, this will return the user's ID.
// 2. If EditorsCanAdmin is enabled and the user is an editor, this will return models.FilterIgnoreUser (0)
// 3. If the user is an admin, this will return models.FilterIgnoreUser (0)
func userFilter(editorsCanAdmin bool, c *models.ReqContext) int64 {
userIdFilter := c.SignedInUser.UserId
if (editorsCanAdmin && c.OrgRole == models.ROLE_EDITOR) || c.OrgRole == models.ROLE_ADMIN {
userIdFilter = models.FilterIgnoreUser
}
return userIdFilter
}
// GET /api/teams/:teamId
func (hs *HTTPServer) GetTeamByID(c *models.ReqContext) response.Response {
teamId, err := strconv.ParseInt(web.Params(c.Req)[":teamId"], 10, 64)
if err != nil {
return response.Error(http.StatusBadRequest, "teamId is invalid", err)
}
query := models.GetTeamByIdQuery{
OrgId: c.OrgId,
Id: teamId,
SignedInUser: c.SignedInUser,
HiddenUsers: hs.Cfg.HiddenUsers,
UserIdFilter: userFilter(hs.Cfg.EditorsCanAdmin, c),
}
if err := hs.SQLStore.GetTeamById(c.Req.Context(), &query); err != nil {
+3
View File
@@ -25,6 +25,9 @@ func (hs *HTTPServer) GetTeamMembers(c *models.ReqContext) response.Response {
}
query := models.GetTeamMembersQuery{OrgId: c.OrgId, TeamId: teamId}
if err := hs.teamGuardian.CanAdmin(c.Req.Context(), query.OrgId, query.TeamId, c.SignedInUser); err != nil {
return response.Error(403, "Not allowed to list team members", err)
}
if err := hs.SQLStore.GetTeamMembers(c.Req.Context(), &query); err != nil {
return response.Error(500, "Failed to get Team Members", err)
+12 -3
View File
@@ -20,6 +20,14 @@ import (
"github.com/stretchr/testify/require"
)
type TeamGuardianMock struct {
result error
}
func (t *TeamGuardianMock) CanAdmin(ctx context.Context, orgId int64, teamId int64, user *models.SignedInUser) error {
return t.result
}
func setUpGetTeamMembersHandler(t *testing.T, sqlStore *sqlstore.SQLStore) {
const testOrgID int64 = 1
var userCmd models.CreateUserCommand
@@ -43,9 +51,10 @@ func TestTeamMembersAPIEndpoint_userLoggedIn(t *testing.T) {
settings := setting.NewCfg()
sqlStore := sqlstore.InitTestDB(t)
hs := &HTTPServer{
Cfg: settings,
License: &licensing.OSSLicensingService{},
SQLStore: sqlStore,
Cfg: settings,
License: &licensing.OSSLicensingService{},
SQLStore: sqlStore,
teamGuardian: &TeamGuardianMock{},
}
loggedInUserScenarioWithRole(t, "When calling GET on", "GET", "api/teams/1/members",
+3
View File
@@ -35,6 +35,7 @@ func TestTeamAPIEndpoint(t *testing.T) {
hs := setupSimpleHTTPServer(nil)
hs.SQLStore = sqlstore.InitTestDB(t)
hs.Cfg.EditorsCanAdmin = true
loggedInUserScenario(t, "When calling GET on", "/api/teams/search", "/api/teams/search", func(sc *scenarioContext) {
_, err := hs.SQLStore.CreateTeam("team1", "", 1)
require.NoError(t, err)
@@ -109,6 +110,7 @@ func TestTeamAPIEndpoint(t *testing.T) {
}
c.OrgRole = models.ROLE_EDITOR
c.Req.Body = mockRequestBody(models.CreateTeamCommand{Name: teamName})
c.Req.Header.Add("Content-Type", "application/json")
hs.CreateTeam(c)
assert.Equal(t, createTeamCalled, 1)
assert.Equal(t, addTeamMemberCalled, 0)
@@ -125,6 +127,7 @@ func TestTeamAPIEndpoint(t *testing.T) {
}
c.OrgRole = models.ROLE_EDITOR
c.Req.Body = mockRequestBody(models.CreateTeamCommand{Name: teamName})
c.Req.Header.Add("Content-Type", "application/json")
createTeamCalled, addTeamMemberCalled = 0, 0
hs.CreateTeam(c)
assert.Equal(t, createTeamCalled, 1)