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

Commit2513167

Browse files
authored
feat: Return more 404s vs 403s (#2194)
* feat: Return more 404s vs 403s* Return vague 404 in all cases
1 parentdc1de58 commit2513167

31 files changed

+231
-155
lines changed

‎cli/autostart_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ func TestAutostart(t *testing.T) {
107107
clitest.SetupConfig(t,client,root)
108108

109109
err:=cmd.Execute()
110-
require.ErrorContains(t,err,"status code403: Forbidden","unexpected error")
110+
require.ErrorContains(t,err,"status code404","unexpected error")
111111
})
112112

113113
t.Run("unset_NotFound",func(t*testing.T) {
@@ -124,7 +124,7 @@ func TestAutostart(t *testing.T) {
124124
clitest.SetupConfig(t,client,root)
125125

126126
err:=cmd.Execute()
127-
require.ErrorContains(t,err,"status code403: Forbidden","unexpected error")
127+
require.ErrorContains(t,err,"status code404:","unexpected error")
128128
})
129129
}
130130

‎cli/ttl_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ func TestTTL(t *testing.T) {
178178
clitest.SetupConfig(t,client,root)
179179

180180
err:=cmd.Execute()
181-
require.ErrorContains(t,err,"status code403: Forbidden","unexpected error")
181+
require.ErrorContains(t,err,"status code404:","unexpected error")
182182
})
183183

184184
t.Run("Unset_NotFound",func(t*testing.T) {
@@ -195,7 +195,7 @@ func TestTTL(t *testing.T) {
195195
clitest.SetupConfig(t,client,root)
196196

197197
err:=cmd.Execute()
198-
require.ErrorContains(t,err,"status code403: Forbidden","unexpected error")
198+
require.ErrorContains(t,err,"status code404:","unexpected error")
199199
})
200200

201201
t.Run("TemplateMaxTTL",func(t*testing.T) {

‎coderd/authorize.go

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77

88
"cdr.dev/slog"
99

10-
"github.com/coder/coder/coderd/httpapi"
1110
"github.com/coder/coder/coderd/httpmw"
1211
"github.com/coder/coder/coderd/rbac"
1312
)
@@ -17,12 +16,18 @@ func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Act
1716
returnrbac.Filter(r.Context(),api.Authorizer,roles.ID.String(),roles.Roles,action,objects)
1817
}
1918

20-
func (api*API)Authorize(rw http.ResponseWriter,r*http.Request,action rbac.Action,object rbac.Objecter)bool {
19+
// Authorize will return false if the user is not authorized to do the action.
20+
// This function will log appropriately, but the caller must return an
21+
// error to the api client.
22+
// Eg:
23+
//if !api.Authorize(...) {
24+
//httpapi.Forbidden(rw)
25+
//return
26+
//}
27+
func (api*API)Authorize(r*http.Request,action rbac.Action,object rbac.Objecter)bool {
2128
roles:=httpmw.AuthorizationUserRoles(r)
2229
err:=api.Authorizer.ByRoleName(r.Context(),roles.ID.String(),roles.Roles,action,object.RBACObject())
2330
iferr!=nil {
24-
httpapi.Forbidden(rw)
25-
2631
// Log the errors for debugging
2732
internalError:=new(rbac.UnauthorizedError)
2833
logger:=api.Logger

‎coderd/coderd_test.go

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -380,9 +380,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
380380
// By default, all omitted routes check for just "authorize" called
381381
routeAssertions=routeCheck{}
382382
}
383-
ifrouteAssertions.StatusCode==0 {
384-
routeAssertions.StatusCode=http.StatusForbidden
385-
}
386383

387384
// Replace all url params with known values
388385
route=strings.ReplaceAll(route,"{organization}",admin.OrganizationID.String())
@@ -413,7 +410,14 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
413410

414411
if!routeAssertions.NoAuthorize {
415412
assert.NotNil(t,authorizer.Called,"authorizer expected")
416-
assert.Equal(t,routeAssertions.StatusCode,resp.StatusCode,"expect unauthorized")
413+
ifrouteAssertions.StatusCode!=0 {
414+
assert.Equal(t,routeAssertions.StatusCode,resp.StatusCode,"expect unauthorized")
415+
}else {
416+
// It's either a 404 or 403.
417+
ifresp.StatusCode!=http.StatusNotFound {
418+
assert.Equal(t,http.StatusForbidden,resp.StatusCode,"expect unauthorized")
419+
}
420+
}
417421
ifauthorizer.Called!=nil {
418422
ifrouteAssertions.AssertAction!="" {
419423
assert.Equal(t,routeAssertions.AssertAction,authorizer.Called.Action,"resource action")

‎coderd/files.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ func (api *API) postFile(rw http.ResponseWriter, r *http.Request) {
2222
apiKey:=httpmw.APIKey(r)
2323
// This requires the site wide action to create files.
2424
// Once created, a user can read their own files uploaded
25-
if!api.Authorize(rw,r,rbac.ActionCreate,rbac.ResourceFile) {
25+
if!api.Authorize(r,rbac.ActionCreate,rbac.ResourceFile) {
26+
httpapi.Forbidden(rw)
2627
return
2728
}
2829

@@ -86,7 +87,7 @@ func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) {
8687
}
8788
file,err:=api.Database.GetFileByHash(r.Context(),hash)
8889
iferrors.Is(err,sql.ErrNoRows) {
89-
httpapi.Forbidden(rw)
90+
httpapi.ResourceNotFound(rw)
9091
return
9192
}
9293
iferr!=nil {
@@ -97,8 +98,10 @@ func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) {
9798
return
9899
}
99100

100-
if!api.Authorize(rw,r,rbac.ActionRead,
101+
if!api.Authorize(r,rbac.ActionRead,
101102
rbac.ResourceFile.WithOwner(file.CreatedBy.String()).WithID(file.Hash)) {
103+
// Return 404 to not leak the file exists
104+
httpapi.ResourceNotFound(rw)
102105
return
103106
}
104107

‎coderd/files_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func TestDownload(t *testing.T) {
5050
_,_,err:=client.Download(context.Background(),"something")
5151
varapiErr*codersdk.Error
5252
require.ErrorAs(t,err,&apiErr)
53-
require.Equal(t,http.StatusForbidden,apiErr.StatusCode())
53+
require.Equal(t,http.StatusNotFound,apiErr.StatusCode())
5454
})
5555

5656
t.Run("Insert",func(t*testing.T) {

‎coderd/gitsshkey.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ import (
1414
func (api*API)regenerateGitSSHKey(rw http.ResponseWriter,r*http.Request) {
1515
user:=httpmw.UserParam(r)
1616

17-
if!api.Authorize(rw,r,rbac.ActionUpdate,rbac.ResourceUserData.WithOwner(user.ID.String())) {
17+
if!api.Authorize(r,rbac.ActionUpdate,rbac.ResourceUserData.WithOwner(user.ID.String())) {
18+
httpapi.ResourceNotFound(rw)
1819
return
1920
}
2021

@@ -62,7 +63,8 @@ func (api *API) regenerateGitSSHKey(rw http.ResponseWriter, r *http.Request) {
6263
func (api*API)gitSSHKey(rw http.ResponseWriter,r*http.Request) {
6364
user:=httpmw.UserParam(r)
6465

65-
if!api.Authorize(rw,r,rbac.ActionRead,rbac.ResourceUserData.WithOwner(user.ID.String())) {
66+
if!api.Authorize(r,rbac.ActionRead,rbac.ResourceUserData.WithOwner(user.ID.String())) {
67+
httpapi.ResourceNotFound(rw)
6668
return
6769
}
6870

‎coderd/httpapi/httpapi.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,14 @@ type Error struct {
7676
Detailstring`json:"detail" validate:"required"`
7777
}
7878

79+
// ResourceNotFound is intentionally vague. All 404 responses should be identical
80+
// to prevent leaking existence of resources.
81+
funcResourceNotFound(rw http.ResponseWriter) {
82+
Write(rw,http.StatusNotFound,Response{
83+
Message:"Resource not found or you do not have access to this resource",
84+
})
85+
}
86+
7987
funcForbidden(rw http.ResponseWriter) {
8088
Write(rw,http.StatusForbidden,Response{
8189
Message:"Forbidden.",

‎coderd/httpmw/organizationparam.go

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"database/sql"
66
"errors"
7-
"fmt"
87
"net/http"
98

109
"github.com/coder/coder/coderd/database"
@@ -45,9 +44,7 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler
4544

4645
organization,err:=db.GetOrganizationByID(r.Context(),orgID)
4746
iferrors.Is(err,sql.ErrNoRows) {
48-
httpapi.Write(rw,http.StatusNotFound, httpapi.Response{
49-
Message:fmt.Sprintf("Organization %q does not exist.",orgID),
50-
})
47+
httpapi.ResourceNotFound(rw)
5148
return
5249
}
5350
iferr!=nil {
@@ -76,9 +73,7 @@ func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.H
7673
UserID:user.ID,
7774
})
7875
iferrors.Is(err,sql.ErrNoRows) {
79-
httpapi.Write(rw,http.StatusForbidden, httpapi.Response{
80-
Message:"Not a member of the organization.",
81-
})
76+
httpapi.ResourceNotFound(rw)
8277
return
8378
}
8479
iferr!=nil {

‎coderd/httpmw/organizationparam_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ func TestOrganizationParam(t *testing.T) {
144144
rtr.ServeHTTP(rw,r)
145145
res:=rw.Result()
146146
deferres.Body.Close()
147-
require.Equal(t,http.StatusForbidden,res.StatusCode)
147+
require.Equal(t,http.StatusNotFound,res.StatusCode)
148148
})
149149

150150
t.Run("Success",func(t*testing.T) {

‎coderd/httpmw/templateparam.go

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"database/sql"
66
"errors"
7-
"fmt"
87
"net/http"
98

109
"github.com/go-chi/chi/v5"
@@ -33,10 +32,8 @@ func ExtractTemplateParam(db database.Store) func(http.Handler) http.Handler {
3332
return
3433
}
3534
template,err:=db.GetTemplateByID(r.Context(),templateID)
36-
iferrors.Is(err,sql.ErrNoRows) {
37-
httpapi.Write(rw,http.StatusNotFound, httpapi.Response{
38-
Message:fmt.Sprintf("Template %q does not exist.",templateID),
39-
})
35+
iferrors.Is(err,sql.ErrNoRows)|| (err==nil&&template.Deleted) {
36+
httpapi.ResourceNotFound(rw)
4037
return
4138
}
4239
iferr!=nil {
@@ -47,13 +44,6 @@ func ExtractTemplateParam(db database.Store) func(http.Handler) http.Handler {
4744
return
4845
}
4946

50-
iftemplate.Deleted {
51-
httpapi.Write(rw,http.StatusNotFound, httpapi.Response{
52-
Message:fmt.Sprintf("Template %q does not exist.",templateID),
53-
})
54-
return
55-
}
56-
5747
ctx:=context.WithValue(r.Context(),templateParamContextKey{},template)
5848
chi.RouteContext(ctx).URLParams.Add("organization",template.OrganizationID.String())
5949
next.ServeHTTP(rw,r.WithContext(ctx))

‎coderd/httpmw/templateversionparam.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"database/sql"
66
"errors"
7-
"fmt"
87
"net/http"
98

109
"github.com/go-chi/chi/v5"
@@ -34,9 +33,7 @@ func ExtractTemplateVersionParam(db database.Store) func(http.Handler) http.Hand
3433
}
3534
templateVersion,err:=db.GetTemplateVersionByID(r.Context(),templateVersionID)
3635
iferrors.Is(err,sql.ErrNoRows) {
37-
httpapi.Write(rw,http.StatusNotFound, httpapi.Response{
38-
Message:fmt.Sprintf("Template version %q does not exist.",templateVersionID),
39-
})
36+
httpapi.ResourceNotFound(rw)
4037
return
4138
}
4239
iferr!=nil {

‎coderd/httpmw/userparam.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,11 @@ package httpmw
22

33
import (
44
"context"
5+
"database/sql"
56
"net/http"
67

8+
"golang.org/x/xerrors"
9+
710
"github.com/go-chi/chi/v5"
811
"github.com/google/uuid"
912

@@ -47,6 +50,10 @@ func ExtractUserParam(db database.Store) func(http.Handler) http.Handler {
4750

4851
ifuserQuery=="me" {
4952
user,err=db.GetUserByID(r.Context(),APIKey(r).UserID)
53+
ifxerrors.Is(err,sql.ErrNoRows) {
54+
httpapi.ResourceNotFound(rw)
55+
return
56+
}
5057
iferr!=nil {
5158
httpapi.Write(rw,http.StatusInternalServerError, httpapi.Response{
5259
Message:"Internal error fetching user.",

‎coderd/httpmw/workspacebuildparam.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"database/sql"
66
"errors"
7-
"fmt"
87
"net/http"
98

109
"github.com/go-chi/chi/v5"
@@ -34,9 +33,7 @@ func ExtractWorkspaceBuildParam(db database.Store) func(http.Handler) http.Handl
3433
}
3534
workspaceBuild,err:=db.GetWorkspaceBuildByID(r.Context(),workspaceBuildID)
3635
iferrors.Is(err,sql.ErrNoRows) {
37-
httpapi.Write(rw,http.StatusNotFound, httpapi.Response{
38-
Message:fmt.Sprintf("Workspace build %q does not exist.",workspaceBuildID),
39-
})
36+
httpapi.ResourceNotFound(rw)
4037
return
4138
}
4239
iferr!=nil {

‎coderd/httpmw/workspaceparam.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"database/sql"
66
"errors"
7-
"fmt"
87
"net/http"
98

109
"github.com/coder/coder/coderd/database"
@@ -32,9 +31,7 @@ func ExtractWorkspaceParam(db database.Store) func(http.Handler) http.Handler {
3231
}
3332
workspace,err:=db.GetWorkspaceByID(r.Context(),workspaceID)
3433
iferrors.Is(err,sql.ErrNoRows) {
35-
httpapi.Write(rw,http.StatusNotFound, httpapi.Response{
36-
Message:fmt.Sprintf("Workspace %q does not exist.",workspaceID),
37-
})
34+
httpapi.ResourceNotFound(rw)
3835
return
3936
}
4037
iferr!=nil {

‎coderd/members.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,15 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
3939
added,removed:=rbac.ChangeRoleSet(member.Roles,impliedTypes)
4040
for_,roleName:=rangeadded {
4141
// Assigning a role requires the create permission.
42-
if!api.Authorize(rw,r,rbac.ActionCreate,rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
42+
if!api.Authorize(r,rbac.ActionCreate,rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
43+
httpapi.Forbidden(rw)
4344
return
4445
}
4546
}
4647
for_,roleName:=rangeremoved {
4748
// Removing a role requires the delete permission.
48-
if!api.Authorize(rw,r,rbac.ActionDelete,rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
49+
if!api.Authorize(r,rbac.ActionDelete,rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
50+
httpapi.Forbidden(rw)
4951
return
5052
}
5153
}

‎coderd/organizations.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,10 @@ import (
1919
func (api*API)organization(rw http.ResponseWriter,r*http.Request) {
2020
organization:=httpmw.OrganizationParam(r)
2121

22-
if!api.Authorize(rw,r,rbac.ActionRead,rbac.ResourceOrganization.
22+
if!api.Authorize(r,rbac.ActionRead,rbac.ResourceOrganization.
2323
InOrg(organization.ID).
2424
WithID(organization.ID.String())) {
25+
httpapi.ResourceNotFound(rw)
2526
return
2627
}
2728

@@ -32,8 +33,8 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) {
3233
apiKey:=httpmw.APIKey(r)
3334
// Create organization uses the organization resource without an OrgID.
3435
// This means you need the site wide permission to make a new organization.
35-
if!api.Authorize(rw,r,rbac.ActionCreate,
36-
rbac.ResourceOrganization) {
36+
if!api.Authorize(r,rbac.ActionCreate,rbac.ResourceOrganization) {
37+
httpapi.Forbidden(rw)
3738
return
3839
}
3940

‎coderd/organizations_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ func TestOrganizationByUserAndName(t *testing.T) {
3030
_,err:=client.OrganizationByName(context.Background(),codersdk.Me,"nothing")
3131
varapiErr*codersdk.Error
3232
require.ErrorAs(t,err,&apiErr)
33-
require.Equal(t,http.StatusForbidden,apiErr.StatusCode())
33+
require.Equal(t,http.StatusNotFound,apiErr.StatusCode())
3434
})
3535

3636
t.Run("NoMember",func(t*testing.T) {
@@ -45,7 +45,7 @@ func TestOrganizationByUserAndName(t *testing.T) {
4545
_,err=other.OrganizationByName(context.Background(),codersdk.Me,org.Name)
4646
varapiErr*codersdk.Error
4747
require.ErrorAs(t,err,&apiErr)
48-
require.Equal(t,http.StatusForbidden,apiErr.StatusCode())
48+
require.Equal(t,http.StatusNotFound,apiErr.StatusCode())
4949
})
5050

5151
t.Run("Valid",func(t*testing.T) {

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp