Skip to content

Commit d7d1a04

Browse files
Kira-PilotKira-Pilot
committed
Merge remote-tracking branch 'origin/main' into cli-ui-copyedits
2 parents 4a2900f + 2513167 commit d7d1a04

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) {

‎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-
if errors.Is(err, sql.ErrNoRows) {
37-
httpapi.Write(rw, http.StatusNotFound, httpapi.Response{
38-
Message: fmt.Sprintf("Template %q does not exist.", templateID),
39-
})
35+
if errors.Is(err, sql.ErrNoRows) || (err == nil && template.Deleted) {
36+
httpapi.ResourceNotFound(rw)
4037
return
4138
}
4239
if err != nil {
@@ -47,13 +44,6 @@ func ExtractTemplateParam(db database.Store) func(http.Handler) http.Handler {
4744
return
4845
}
4946

50-
if template.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
if errors.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
if err != 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
if userQuery == "me" {
4952
user, err = db.GetUserByID(r.Context(), APIKey(r).UserID)
53+
if xerrors.Is(err, sql.ErrNoRows) {
54+
httpapi.ResourceNotFound(rw)
55+
return
56+
}
5057
if err != 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
if errors.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
if err != 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
if errors.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
if err != 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 := range added {
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 := range removed {
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
var apiErr *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
var apiErr *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)