From 7c71053eab0da062c110f40217ed967ffde9f7cb Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Wed, 11 Oct 2023 09:41:14 +0400 Subject: [PATCH] fix: stop leaking User into API handlers unless authorized Fixes an issue where we extracted the `{user}` parameter from the URL and added it to the API Handler context regardless of whether the caller had permission to read the User. --- coderd/coderd.go | 1 - coderd/httpmw/organizationparam.go | 36 ++++++++++++++++++++++++++---- coderd/httpmw/userparam.go | 7 +----- coderd/members.go | 3 +-- coderd/users_test.go | 4 ++-- coderd/workspaces.go | 14 ++++++------ 6 files changed, 43 insertions(+), 22 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 6cb43b71dd..9436718fac 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -652,7 +652,6 @@ func New(options *Options) *API { r.Get("/roles", api.assignableOrgRoles) r.Route("/{user}", func(r chi.Router) { r.Use( - httpmw.ExtractUserParam(options.Database), httpmw.ExtractOrganizationMemberParam(options.Database), ) r.Put("/roles", api.putMemberRoles) diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index 85e94ef4a0..76b085f019 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -5,6 +5,7 @@ import ( "net/http" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/codersdk" ) @@ -25,8 +26,8 @@ func OrganizationParam(r *http.Request) database.Organization { // OrganizationMemberParam returns the organization membership that allowed the query // from the ExtractOrganizationParam handler. -func OrganizationMemberParam(r *http.Request) database.OrganizationMember { - organizationMember, ok := r.Context().Value(organizationMemberParamContextKey{}).(database.OrganizationMember) +func OrganizationMemberParam(r *http.Request) OrganizationMember { + organizationMember, ok := r.Context().Value(organizationMemberParamContextKey{}).(OrganizationMember) if !ok { panic("developer error: organization member param middleware not provided") } @@ -62,14 +63,31 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler } } +// OrganizationMember is the database object plus the Username. Including the Username in this +// middleware is preferable to a join at the SQL layer so that we can keep the autogenerated +// database types as they are. +type OrganizationMember struct { + database.OrganizationMember + Username string +} + // ExtractOrganizationMemberParam grabs a user membership from the "organization" and "user" URL parameter. // This middleware requires the ExtractUser and ExtractOrganization middleware higher in the stack func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() + // We need to resolve the `{user}` URL parameter so that we can get the userID and + // username. We do this as SystemRestricted since the caller might have permission + // to access the OrganizationMember object, but *not* the User object. So, it is + // very important that we do not add the User object to the request context or otherwise + // leak it to the API handler. + // nolint:gocritic + user, ok := extractUserContext(dbauthz.AsSystemRestricted(ctx), db, rw, r) + if !ok { + return + } organization := OrganizationParam(r) - user := UserParam(r) organizationMember, err := db.GetOrganizationMemberByUserID(ctx, database.GetOrganizationMemberByUserIDParams{ OrganizationID: organization.ID, @@ -87,7 +105,17 @@ func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.H return } - ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, organizationMember) + ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, OrganizationMember{ + OrganizationMember: organizationMember, + // Here we're making one exception to the rule about not leaking data about the user + // to the API handler, which is to include the username. If the caller has permission + // to read the OrganizationMember, then we're explicitly saying here that they also + // have permission to see the member's username, which is itself uncontroversial. + // + // API handlers need this information for audit logging and returning the owner's + // username in response to creating a workspace. + Username: user.Username, + }) next.ServeHTTP(rw, r.WithContext(ctx)) }) } diff --git a/coderd/httpmw/userparam.go b/coderd/httpmw/userparam.go index 8a8310672c..03bff9bbb5 100644 --- a/coderd/httpmw/userparam.go +++ b/coderd/httpmw/userparam.go @@ -9,7 +9,6 @@ import ( "github.com/google/uuid" "github.com/coder/coder/v2/coderd/database" - "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/codersdk" ) @@ -38,11 +37,7 @@ func ExtractUserParam(db database.Store) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - // We need to call as SystemRestricted because this middleware is called from - // organizations/{organization}/members/{user}/ paths, and we need to allow - // org-admins to call these paths --- they might not have sitewide read permissions on users. - // nolint:gocritic - user, ok := extractUserContext(dbauthz.AsSystemRestricted(ctx), db, rw, r) + user, ok := extractUserContext(ctx, db, rw, r) if !ok { // response already handled return diff --git a/coderd/members.go b/coderd/members.go index 91083cbb89..038851870c 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -31,7 +31,6 @@ import ( func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { var ( ctx = r.Context() - user = httpmw.UserParam(r) organization = httpmw.OrganizationParam(r) member = httpmw.OrganizationMemberParam(r) apiKey = httpmw.APIKey(r) @@ -51,7 +50,7 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { updatedUser, err := api.updateOrganizationMemberRoles(ctx, database.UpdateMemberRolesParams{ GrantedRoles: params.Roles, - UserID: user.ID, + UserID: member.UserID, OrgID: organization.ID, }) if err != nil { diff --git a/coderd/users_test.go b/coderd/users_test.go index ad6581c250..2755a7491a 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -326,7 +326,7 @@ func TestDeleteUser(t *testing.T) { err := client.DeleteUser(context.Background(), firstUser.UserID) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) }) t.Run("HasWorkspaces", func(t *testing.T) { t.Parallel() @@ -930,7 +930,7 @@ func TestGrantSiteRoles(t *testing.T) { AssignToUser: first.UserID.String(), Roles: []string{}, Error: true, - StatusCode: http.StatusForbidden, + StatusCode: http.StatusBadRequest, }, { // Cannot update your own roles diff --git a/coderd/workspaces.go b/coderd/workspaces.go index d90b763fb9..51c4f21de7 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -298,9 +298,9 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req organization = httpmw.OrganizationParam(r) apiKey = httpmw.APIKey(r) auditor = api.Auditor.Load() - user = httpmw.UserParam(r) + member = httpmw.OrganizationMemberParam(r) workspaceResourceInfo = audit.AdditionalFields{ - WorkspaceOwner: user.Username, + WorkspaceOwner: member.Username, } ) @@ -321,7 +321,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req // Do this upfront to save work. if !api.Authorize(r, rbac.ActionCreate, - rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(user.ID.String())) { + rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(member.UserID.String())) { httpapi.ResourceNotFound(rw) return } @@ -438,7 +438,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req // read other workspaces. Ideally we check the error on create and look for // a postgres conflict error. workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(ctx, database.GetWorkspaceByOwnerIDAndNameParams{ - OwnerID: user.ID, + OwnerID: member.UserID, Name: createWorkspace.Name, }) if err == nil { @@ -471,7 +471,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req ID: uuid.New(), CreatedAt: now, UpdatedAt: now, - OwnerID: user.ID, + OwnerID: member.UserID, OrganizationID: template.OrganizationID, TemplateID: template.ID, Name: createWorkspace.Name, @@ -537,7 +537,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req ProvisionerJob: *provisionerJob, QueuePosition: 0, }, - user.Username, + member.Username, []database.WorkspaceResource{}, []database.WorkspaceResourceMetadatum{}, []database.WorkspaceAgent{}, @@ -558,7 +558,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req workspace, apiBuild, template, - user.Username, + member.Username, )) }