Skip to content

Commit 2513167

Browse files
EmyrkEmyrk
authored
feat: Return more 404s vs 403s (#2194)
* feat: Return more 404s vs 403s * Return vague 404 in all cases
1 parent dc1de58 commit 2513167

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 code 403: Forbidden", "unexpected error")
110+
require.ErrorContains(t, err, "status code 404", "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 code 403: Forbidden", "unexpected error")
127+
require.ErrorContains(t, err, "status code 404:", "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 code 403: Forbidden", "unexpected error")
181+
require.ErrorContains(t, err, "status code 404:", "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 code 403: Forbidden", "unexpected error")
198+
require.ErrorContains(t, err, "status code 404:", "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
return rbac.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
if err != 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-
if routeAssertions.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+
if routeAssertions.StatusCode != 0 {
414+
assert.Equal(t, routeAssertions.StatusCode, resp.StatusCode, "expect unauthorized")
415+
} else {
416+
// It's either a 404 or 403.
417+
if resp.StatusCode != http.StatusNotFound {
418+
assert.Equal(t, http.StatusForbidden, resp.StatusCode, "expect unauthorized")
419+
}
420+
}
417421
if authorizer.Called != nil {
418422
if routeAssertions.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
if errors.Is(err, sql.ErrNoRows) {
89-
httpapi.Forbidden(rw)
90+
httpapi.ResourceNotFound(rw)
9091
return
9192
}
9293
if err != 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
var apiErr *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
Detail string `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+
func ResourceNotFound(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
func Forbidden(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
if errors.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
if err != nil {
@@ -76,9 +73,7 @@ func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.H
7673
UserID: user.ID,
7774
})
7875
if errors.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
if err != 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
defer res.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) {

0 commit comments

Comments
 (0)