Skip to content

Commit 38e5b96

Browse files
EmyrkEmyrk
authored
chore: Rbac errors should be returned, and not hidden behind 404 (#7122)
* chore: Rbac errors should be returned, and not hidden behind 404 SqlErrNoRows was hiding actual errors * Replace sql.ErrNoRow checks * Remove sql err no rows check from dbauthz test * Fix to use dbauthz system user
1 parent fa64c58 commit 38e5b96

23 files changed

+50
-72
lines changed

‎coderd/apikey.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@ package coderd
33
import (
44
"context"
55
"crypto/sha256"
6-
"database/sql"
7-
"errors"
86
"fmt"
97
"net"
108
"net/http"
@@ -167,7 +165,7 @@ func (api *API) apiKeyByID(rw http.ResponseWriter, r *http.Request) {
167165

168166
keyID := chi.URLParam(r, "keyid")
169167
key, err := api.Database.GetAPIKeyByID(ctx, keyID)
170-
if errors.Is(err, sql.ErrNoRows) {
168+
if httpapi.Is404Error(err) {
171169
httpapi.ResourceNotFound(rw)
172170
return
173171
}
@@ -202,7 +200,7 @@ func (api *API) apiKeyByName(rw http.ResponseWriter, r *http.Request) {
202200
TokenName: tokenName,
203201
UserID: user.ID,
204202
})
205-
if errors.Is(err, sql.ErrNoRows) {
203+
if httpapi.Is404Error(err) {
206204
httpapi.ResourceNotFound(rw)
207205
return
208206
}
@@ -323,7 +321,7 @@ func (api *API) deleteAPIKey(rw http.ResponseWriter, r *http.Request) {
323321
defer commitAudit()
324322

325323
err = api.Database.DeleteAPIKeyByID(ctx, keyID)
326-
if errors.Is(err, sql.ErrNoRows) {
324+
if httpapi.Is404Error(err) {
327325
httpapi.ResourceNotFound(rw)
328326
return
329327
}

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ func (e NotAuthorizedError) Error() string {
3434

3535
// Unwrap will always unwrap to a sql.ErrNoRows so the API returns a 404.
3636
// So 'errors.Is(err, sql.ErrNoRows)' will always be true.
37-
func (NotAuthorizedError) Unwrap() error {
38-
return sql.ErrNoRows
37+
func (e NotAuthorizedError) Unwrap() error {
38+
return e.Err
3939
}
4040

4141
func IsNotAuthorizedError(err error) bool {

‎coderd/database/dbauthz/setup_test.go

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

33
import (
44
"context"
5-
"database/sql"
65
"fmt"
76
"reflect"
87
"sort"
@@ -219,7 +218,6 @@ func (s *MethodTestSuite) NotAuthorizedErrorTest(ctx context.Context, az *coderd
219218
if err != nil || !hasEmptySliceResponse(resp) {
220219
s.ErrorContainsf(err, "unauthorized", "error string should have a good message")
221220
s.Errorf(err, "method should an error with disallow authz")
222-
s.ErrorIsf(err, sql.ErrNoRows, "error should match sql.ErrNoRows")
223221
s.ErrorAs(err, &dbauthz.NotAuthorizedError{}, "error should be NotAuthorizedError")
224222
}
225223
})

‎coderd/files.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ func (api *API) fileByID(rw http.ResponseWriter, r *http.Request) {
126126
}
127127

128128
file, err := api.Database.GetFileByID(ctx, id)
129-
if errors.Is(err, sql.ErrNoRows) {
129+
if httpapi.Is404Error(err) {
130130
httpapi.ResourceNotFound(rw)
131131
return
132132
}

‎coderd/httpapi/httpapi.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package httpapi
33
import (
44
"bytes"
55
"context"
6+
"database/sql"
67
"encoding/json"
78
"errors"
89
"flag"
@@ -15,6 +16,8 @@ import (
1516
"github.com/go-playground/validator/v10"
1617
"golang.org/x/xerrors"
1718

19+
"github.com/coder/coder/coderd/database/dbauthz"
20+
"github.com/coder/coder/coderd/rbac"
1821
"github.com/coder/coder/coderd/tracing"
1922
"github.com/coder/coder/codersdk"
2023
)
@@ -80,6 +83,16 @@ func init() {
8083
}
8184
}
8285

86+
// Is404Error returns true if the given error should return a 404 status code.
87+
// Both actual 404s and unauthorized errors should return 404s to not leak
88+
// information about the existence of resources.
89+
func Is404Error(err error) bool {
90+
if err == nil {
91+
return false
92+
}
93+
return xerrors.Is(err, sql.ErrNoRows) || dbauthz.IsNotAuthorizedError(err) || rbac.IsUnauthorizedError(err)
94+
}
95+
8396
// Convenience error functions don't take contexts since their responses are
8497
// static, it doesn't make much sense to trace them.
8598

‎coderd/httpmw/groupparam.go

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,9 @@ package httpmw
22

33
import (
44
"context"
5-
"database/sql"
6-
"errors"
75
"net/http"
86

97
"github.com/go-chi/chi/v5"
10-
"golang.org/x/xerrors"
118

129
"github.com/coder/coder/coderd/database"
1310
"github.com/coder/coder/coderd/httpapi"
@@ -45,7 +42,7 @@ func ExtractGroupByNameParam(db database.Store) func(http.Handler) http.Handler
4542
OrganizationID: org.ID,
4643
Name: name,
4744
})
48-
if xerrors.Is(err, sql.ErrNoRows) {
45+
if httpapi.Is404Error(err) {
4946
httpapi.ResourceNotFound(rw)
5047
return
5148
}
@@ -73,7 +70,7 @@ func ExtractGroupParam(db database.Store) func(http.Handler) http.Handler {
7370
}
7471

7572
group, err := db.GetGroupByID(r.Context(), groupID)
76-
if errors.Is(err, sql.ErrNoRows) {
73+
if httpapi.Is404Error(err) {
7774
httpapi.ResourceNotFound(rw)
7875
return
7976
}

‎coderd/httpmw/organizationparam.go

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

33
import (
44
"context"
5-
"database/sql"
6-
"errors"
75
"net/http"
86

97
"github.com/coder/coder/coderd/database"
@@ -47,7 +45,7 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler
4745
}
4846

4947
organization, err := db.GetOrganizationByID(ctx, orgID)
50-
if errors.Is(err, sql.ErrNoRows) {
48+
if httpapi.Is404Error(err) {
5149
httpapi.ResourceNotFound(rw)
5250
return
5351
}
@@ -77,7 +75,7 @@ func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.H
7775
OrganizationID: organization.ID,
7876
UserID: user.ID,
7977
})
80-
if errors.Is(err, sql.ErrNoRows) {
78+
if httpapi.Is404Error(err) {
8179
httpapi.ResourceNotFound(rw)
8280
return
8381
}

‎coderd/httpmw/templateparam.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@ package httpmw
22

33
import (
44
"context"
5-
"database/sql"
6-
"errors"
75
"net/http"
86

97
"github.com/go-chi/chi/v5"
@@ -34,7 +32,7 @@ func ExtractTemplateParam(db database.Store) func(http.Handler) http.Handler {
3432
return
3533
}
3634
template, err := db.GetTemplateByID(r.Context(), templateID)
37-
if errors.Is(err, sql.ErrNoRows) || (err == nil && template.Deleted) {
35+
if httpapi.Is404Error(err) || (err == nil && template.Deleted) {
3836
httpapi.ResourceNotFound(rw)
3937
return
4038
}

‎coderd/httpmw/templateversionparam.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package httpmw
33
import (
44
"context"
55
"database/sql"
6-
"errors"
76
"net/http"
87

98
"github.com/go-chi/chi/v5"
@@ -35,7 +34,7 @@ func ExtractTemplateVersionParam(db database.Store) func(http.Handler) http.Hand
3534
return
3635
}
3736
templateVersion, err := db.GetTemplateVersionByID(ctx, templateVersionID)
38-
if errors.Is(err, sql.ErrNoRows) {
37+
if httpapi.Is404Error(err) {
3938
httpapi.ResourceNotFound(rw)
4039
return
4140
}

‎coderd/httpmw/userparam.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,8 @@ package httpmw
22

33
import (
44
"context"
5-
"database/sql"
65
"net/http"
76

8-
"golang.org/x/xerrors"
9-
107
"github.com/go-chi/chi/v5"
118
"github.com/google/uuid"
129

@@ -71,7 +68,7 @@ func ExtractUserParam(db database.Store, redirectToLoginOnMe bool) func(http.Han
7168
}
7269
//nolint:gocritic // System needs to be able to get user from param.
7370
user, err = db.GetUserByID(dbauthz.AsSystemRestricted(ctx), apiKey.UserID)
74-
if xerrors.Is(err, sql.ErrNoRows) {
71+
if httpapi.Is404Error(err) {
7572
httpapi.ResourceNotFound(rw)
7673
return
7774
}

‎coderd/httpmw/workspacebuildparam.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@ package httpmw
22

33
import (
44
"context"
5-
"database/sql"
6-
"errors"
75
"net/http"
86

97
"github.com/go-chi/chi/v5"
@@ -34,7 +32,7 @@ func ExtractWorkspaceBuildParam(db database.Store) func(http.Handler) http.Handl
3432
return
3533
}
3634
workspaceBuild, err := db.GetWorkspaceBuildByID(ctx, workspaceBuildID)
37-
if errors.Is(err, sql.ErrNoRows) {
35+
if httpapi.Is404Error(err) {
3836
httpapi.ResourceNotFound(rw)
3937
return
4038
}

‎coderd/httpmw/workspaceparam.go

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

33
import (
44
"context"
5-
"database/sql"
6-
"errors"
75
"fmt"
86
"net/http"
97
"strings"
@@ -37,7 +35,7 @@ func ExtractWorkspaceParam(db database.Store) func(http.Handler) http.Handler {
3735
return
3836
}
3937
workspace, err := db.GetWorkspaceByID(ctx, workspaceID)
40-
if errors.Is(err, sql.ErrNoRows) {
38+
if httpapi.Is404Error(err) {
4139
httpapi.ResourceNotFound(rw)
4240
return
4341
}
@@ -74,7 +72,7 @@ func ExtractWorkspaceAndAgentParam(db database.Store) func(http.Handler) http.Ha
7472
Name: workspaceParts[0],
7573
})
7674
if err != nil {
77-
if errors.Is(err, sql.ErrNoRows) {
75+
if httpapi.Is404Error(err) {
7876
httpapi.ResourceNotFound(rw)
7977
return
8078
}

‎coderd/parameters.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ func (api *API) deleteParameter(rw http.ResponseWriter, r *http.Request) {
141141
ScopeID: scopeID,
142142
Name: name,
143143
})
144-
if errors.Is(err, sql.ErrNoRows) {
144+
if httpapi.Is404Error(err) {
145145
httpapi.ResourceNotFound(rw)
146146
return
147147
}

‎coderd/provisionerjobs.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ func (api *API) provisionerJobResources(rw http.ResponseWriter, r *http.Request,
224224
}
225225

226226
// nolint:gocritic // GetWorkspaceAppsByAgentIDs is a system function.
227-
apps, err := api.Database.GetWorkspaceAppsByAgentIDs(ctx, resourceAgentIDs)
227+
apps, err := api.Database.GetWorkspaceAppsByAgentIDs(dbauthz.AsSystemRestricted(ctx), resourceAgentIDs)
228228
if errors.Is(err, sql.ErrNoRows) {
229229
err = nil
230230
}

‎coderd/templates.go

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -407,7 +407,7 @@ func (api *API) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re
407407
Name: templateName,
408408
})
409409
if err != nil {
410-
if errors.Is(err, sql.ErrNoRows) {
410+
if httpapi.Is404Error(err) {
411411
httpapi.ResourceNotFound(rw)
412412
return
413413
}
@@ -419,11 +419,6 @@ func (api *API) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re
419419
return
420420
}
421421

422-
if !api.Authorize(r, rbac.ActionRead, template) {
423-
httpapi.ResourceNotFound(rw)
424-
return
425-
}
426-
427422
createdByNameMap, err := getCreatedByNamesByTemplateIDs(ctx, api.Database, []database.Template{template})
428423
if err != nil {
429424
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
@@ -583,10 +578,6 @@ func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) {
583578
func (api *API) templateDAUs(rw http.ResponseWriter, r *http.Request) {
584579
ctx := r.Context()
585580
template := httpmw.TemplateParam(r)
586-
if !api.Authorize(r, rbac.ActionRead, template) {
587-
httpapi.ResourceNotFound(rw)
588-
return
589-
}
590581

591582
resp, _ := api.metricsCache.TemplateDAUs(template.ID)
592583
if resp == nil || resp.Entries == nil {

0 commit comments

Comments
 (0)