Skip to content

Commit 7c71053

Browse files
spikecurtisspikecurtis
authored
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.
1 parent fbabb43 commit 7c71053

File tree

6 files changed

+43
-22
lines changed

6 files changed

+43
-22
lines changed

‎coderd/coderd.go

-1
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

+32-4
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

+1-6
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

+1-2
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

+2-2
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

+7-7
Original file line numberDiff line numberDiff line change
@@ -298,9 +298,9 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
298298
organization = httpmw.OrganizationParam(r)
299299
apiKey = httpmw.APIKey(r)
300300
auditor = api.Auditor.Load()
301-
user = httpmw.UserParam(r)
301+
member = httpmw.OrganizationMemberParam(r)
302302
workspaceResourceInfo = audit.AdditionalFields{
303-
WorkspaceOwner: user.Username,
303+
WorkspaceOwner: member.Username,
304304
}
305305
)
306306

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

322322
// Do this upfront to save work.
323323
if !api.Authorize(r, rbac.ActionCreate,
324-
rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(user.ID.String())) {
324+
rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(member.UserID.String())) {
325325
httpapi.ResourceNotFound(rw)
326326
return
327327
}
@@ -438,7 +438,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
438438
// read other workspaces. Ideally we check the error on create and look for
439439
// a postgres conflict error.
440440
workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(ctx, database.GetWorkspaceByOwnerIDAndNameParams{
441-
OwnerID: user.ID,
441+
OwnerID: member.UserID,
442442
Name: createWorkspace.Name,
443443
})
444444
if err == nil {
@@ -471,7 +471,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
471471
ID: uuid.New(),
472472
CreatedAt: now,
473473
UpdatedAt: now,
474-
OwnerID: user.ID,
474+
OwnerID: member.UserID,
475475
OrganizationID: template.OrganizationID,
476476
TemplateID: template.ID,
477477
Name: createWorkspace.Name,
@@ -537,7 +537,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
537537
ProvisionerJob: *provisionerJob,
538538
QueuePosition: 0,
539539
},
540-
user.Username,
540+
member.Username,
541541
[]database.WorkspaceResource{},
542542
[]database.WorkspaceResourceMetadatum{},
543543
[]database.WorkspaceAgent{},
@@ -558,7 +558,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
558558
workspace,
559559
apiBuild,
560560
template,
561-
user.Username,
561+
member.Username,
562562
))
563563
}
564564

0 commit comments

Comments
 (0)