Skip to content

Commit 43e4f42

Browse files
spikecurtisspikecurtis
committed
fix: stop leaking User into API handlers unless authorized
1 parent 7e6b549 commit 43e4f42

File tree

6 files changed

+43
-22
lines changed

6 files changed

+43
-22
lines changed

‎coderd/coderd.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -652,7 +652,6 @@ func New(options *Options) *API {
652652
r.Get("/roles", api.assignableOrgRoles)
653653
r.Route("/{user}", func(r chi.Router) {
654654
r.Use(
655-
httpmw.ExtractUserParam(options.Database),
656655
httpmw.ExtractOrganizationMemberParam(options.Database),
657656
)
658657
r.Put("/roles", api.putMemberRoles)

‎coderd/httpmw/organizationparam.go

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"net/http"
66

77
"github.com/coder/coder/v2/coderd/database"
8+
"github.com/coder/coder/v2/coderd/database/dbauthz"
89
"github.com/coder/coder/v2/coderd/httpapi"
910
"github.com/coder/coder/v2/codersdk"
1011
)
@@ -25,8 +26,8 @@ func OrganizationParam(r *http.Request) database.Organization {
2526

2627
// OrganizationMemberParam returns the organization membership that allowed the query
2728
// from the ExtractOrganizationParam handler.
28-
func OrganizationMemberParam(r *http.Request) database.OrganizationMember {
29-
organizationMember, ok := r.Context().Value(organizationMemberParamContextKey{}).(database.OrganizationMember)
29+
func OrganizationMemberParam(r *http.Request) OrganizationMember {
30+
organizationMember, ok := r.Context().Value(organizationMemberParamContextKey{}).(OrganizationMember)
3031
if !ok {
3132
panic("developer error: organization member param middleware not provided")
3233
}
@@ -62,14 +63,31 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler
6263
}
6364
}
6465

66+
// OrganizationMember is the database object plus the Username. Including the Username in this
67+
// middleware is preferable to a join at the SQL layer so that we can keep the autogenerated
68+
// database types as they are.
69+
type OrganizationMember struct {
70+
database.OrganizationMember
71+
Username string
72+
}
73+
6574
// ExtractOrganizationMemberParam grabs a user membership from the "organization" and "user" URL parameter.
6675
// This middleware requires the ExtractUser and ExtractOrganization middleware higher in the stack
6776
func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.Handler {
6877
return func(next http.Handler) http.Handler {
6978
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
7079
ctx := r.Context()
80+
// We need to resolve the `{user}` URL parameter so that we can get the userID and
81+
// username. We do this as SystemRestricted since the caller might have permission
82+
// to access the OrganizationMember object, but *not* the User object. So, it is
83+
// very important that we do not add the User object to the request context or otherwise
84+
// leak it to the API handler.
85+
// nolint:gocritic
86+
user, ok := extractUserContext(dbauthz.AsSystemRestricted(ctx), db, rw, r)
87+
if !ok {
88+
return
89+
}
7190
organization := OrganizationParam(r)
72-
user := UserParam(r)
7391

7492
organizationMember, err := db.GetOrganizationMemberByUserID(ctx, database.GetOrganizationMemberByUserIDParams{
7593
OrganizationID: organization.ID,
@@ -87,7 +105,17 @@ func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.H
87105
return
88106
}
89107

90-
ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, organizationMember)
108+
ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, OrganizationMember{
109+
OrganizationMember: organizationMember,
110+
// Here we're making one exception to the rule about not leaking data about the user
111+
// to the API handler, which is to include the username. If the caller has permission
112+
// to read the OrganizationMember, then we're explicitly saying here that they also
113+
// have permission to see the member's username, which is itself uncontroversial.
114+
//
115+
// API handlers need this information for audit logging and returning the owner's
116+
// username in response to creating a workspace.
117+
Username: user.Username,
118+
})
91119
next.ServeHTTP(rw, r.WithContext(ctx))
92120
})
93121
}

‎coderd/httpmw/userparam.go

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99
"github.com/google/uuid"
1010

1111
"github.com/coder/coder/v2/coderd/database"
12-
"github.com/coder/coder/v2/coderd/database/dbauthz"
1312
"github.com/coder/coder/v2/coderd/httpapi"
1413
"github.com/coder/coder/v2/codersdk"
1514
)
@@ -38,11 +37,7 @@ func ExtractUserParam(db database.Store) func(http.Handler) http.Handler {
3837
return func(next http.Handler) http.Handler {
3938
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
4039
ctx := r.Context()
41-
// We need to call as SystemRestricted because this middleware is called from
42-
// organizations/{organization}/members/{user}/ paths, and we need to allow
43-
// org-admins to call these paths --- they might not have sitewide read permissions on users.
44-
// nolint:gocritic
45-
user, ok := extractUserContext(dbauthz.AsSystemRestricted(ctx), db, rw, r)
40+
user, ok := extractUserContext(ctx, db, rw, r)
4641
if !ok {
4742
// response already handled
4843
return

‎coderd/members.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ import (
3131
func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
3232
var (
3333
ctx = r.Context()
34-
user = httpmw.UserParam(r)
3534
organization = httpmw.OrganizationParam(r)
3635
member = httpmw.OrganizationMemberParam(r)
3736
apiKey = httpmw.APIKey(r)
@@ -51,7 +50,7 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
5150

5251
updatedUser, err := api.updateOrganizationMemberRoles(ctx, database.UpdateMemberRolesParams{
5352
GrantedRoles: params.Roles,
54-
UserID: user.ID,
53+
UserID: member.UserID,
5554
OrgID: organization.ID,
5655
})
5756
if err != nil {

‎coderd/users_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,7 @@ func TestDeleteUser(t *testing.T) {
326326
err := client.DeleteUser(context.Background(), firstUser.UserID)
327327
var apiErr *codersdk.Error
328328
require.ErrorAs(t, err, &apiErr)
329-
require.Equal(t, http.StatusForbidden, apiErr.StatusCode())
329+
require.Equal(t, http.StatusBadRequest, apiErr.StatusCode())
330330
})
331331
t.Run("HasWorkspaces", func(t *testing.T) {
332332
t.Parallel()
@@ -930,7 +930,7 @@ func TestGrantSiteRoles(t *testing.T) {
930930
AssignToUser: first.UserID.String(),
931931
Roles: []string{},
932932
Error: true,
933-
StatusCode: http.StatusForbidden,
933+
StatusCode: http.StatusBadRequest,
934934
},
935935
{
936936
// Cannot update your own roles

‎coderd/workspaces.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -316,9 +316,9 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
316316
organization = httpmw.OrganizationParam(r)
317317
apiKey = httpmw.APIKey(r)
318318
auditor = api.Auditor.Load()
319-
user = httpmw.UserParam(r)
319+
member = httpmw.OrganizationMemberParam(r)
320320
workspaceResourceInfo = audit.AdditionalFields{
321-
WorkspaceOwner: user.Username,
321+
WorkspaceOwner: member.Username,
322322
}
323323
)
324324

@@ -339,7 +339,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
339339

340340
// Do this upfront to save work.
341341
if !api.Authorize(r, rbac.ActionCreate,
342-
rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(user.ID.String())) {
342+
rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(member.UserID.String())) {
343343
httpapi.ResourceNotFound(rw)
344344
return
345345
}
@@ -456,7 +456,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
456456
// read other workspaces. Ideally we check the error on create and look for
457457
// a postgres conflict error.
458458
workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(ctx, database.GetWorkspaceByOwnerIDAndNameParams{
459-
OwnerID: user.ID,
459+
OwnerID: member.UserID,
460460
Name: createWorkspace.Name,
461461
})
462462
if err == nil {
@@ -489,7 +489,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
489489
ID: uuid.New(),
490490
CreatedAt: now,
491491
UpdatedAt: now,
492-
OwnerID: user.ID,
492+
OwnerID: member.UserID,
493493
OrganizationID: template.OrganizationID,
494494
TemplateID: template.ID,
495495
Name: createWorkspace.Name,
@@ -555,7 +555,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
555555
ProvisionerJob: *provisionerJob,
556556
QueuePosition: 0,
557557
},
558-
user.Username,
558+
member.Username,
559559
[]database.WorkspaceResource{},
560560
[]database.WorkspaceResourceMetadatum{},
561561
[]database.WorkspaceAgent{},
@@ -576,7 +576,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
576576
workspace,
577577
apiBuild,
578578
template,
579-
user.Username,
579+
member.Username,
580580
))
581581
}
582582

0 commit comments

Comments
 (0)