Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit7c71053

Browse files
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 parentfbabb43 commit7c71053

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-
funcOrganizationMemberParam(r*http.Request)database.OrganizationMember {
29-
organizationMember,ok:=r.Context().Value(organizationMemberParamContextKey{}).(database.OrganizationMember)
29+
funcOrganizationMemberParam(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+
typeOrganizationMemberstruct {
70+
database.OrganizationMember
71+
Usernamestring
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
funcExtractOrganizationMemberParam(db database.Store)func(http.Handler) http.Handler {
6877
returnfunc(next http.Handler) http.Handler {
6978
returnhttp.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
returnfunc(next http.Handler) http.Handler {
3938
returnhttp.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
iferr!=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
varapiErr*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
@@ -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
iferr==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)
[8]ページ先頭
©2009-2025 Movatter.jp