Skip to content

Commit d50ffa7

Browse files
coadlercoadler
authored
fix: exit reset password request before passwords are compared (#13856)
1 parent 3894ae1 commit d50ffa7

File tree

5 files changed

+47
-4
lines changed

5 files changed

+47
-4
lines changed

‎coderd/coderd.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1018,6 +1018,7 @@ func New(options *Options) *API {
10181018
})
10191019
r.Put("/appearance", api.putUserAppearanceSettings)
10201020
r.Route("/password", func(r chi.Router) {
1021+
r.Use(httpmw.RateLimit(options.LoginRateLimit, time.Minute))
10211022
r.Put("/", api.putUserPassword)
10221023
})
10231024
// These roles apply to the site wide permissions.

‎coderd/httpmw/authz_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ func TestAsAuthzSystem(t *testing.T) {
1818
t.Parallel()
1919
userActor := coderdtest.RandomRBACSubject()
2020

21-
base := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
21+
base := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
2222
actor, ok := dbauthz.ActorFromContext(r.Context())
2323
assert.True(t, ok, "actor should exist")
2424
assert.True(t, userActor.Equal(actor), "actor should be the user actor")
@@ -80,7 +80,7 @@ func TestAsAuthzSystem(t *testing.T) {
8080
mwAssertUser,
8181
)
8282
r.Handle("/", base)
83-
r.NotFound(func(writer http.ResponseWriter, request *http.Request) {
83+
r.NotFound(func(http.ResponseWriter, *http.Request) {
8484
assert.Fail(t, "should not hit not found, the route should be correct")
8585
})
8686
})

‎coderd/notifications/manager_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,13 @@ import (
77
"testing"
88
"time"
99

10-
"github.com/coder/serpent"
1110
"github.com/google/uuid"
1211
"github.com/stretchr/testify/assert"
1312
"github.com/stretchr/testify/require"
1413
"golang.org/x/xerrors"
1514

1615
"cdr.dev/slog"
1716
"cdr.dev/slog/sloggers/slogtest"
18-
1917
"github.com/coder/coder/v2/coderd/database"
2018
"github.com/coder/coder/v2/coderd/database/dbgen"
2119
"github.com/coder/coder/v2/coderd/database/dbmem"
@@ -24,6 +22,7 @@ import (
2422
"github.com/coder/coder/v2/coderd/notifications/dispatch"
2523
"github.com/coder/coder/v2/coderd/notifications/types"
2624
"github.com/coder/coder/v2/testutil"
25+
"github.com/coder/serpent"
2726
)
2827

2928
func TestBufferedUpdates(t *testing.T) {

‎coderd/users.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -913,6 +913,11 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) {
913913
defer commitAudit()
914914
aReq.Old = user
915915

916+
if !api.Authorize(r, policy.ActionUpdatePersonal, user) {
917+
httpapi.ResourceNotFound(rw)
918+
return
919+
}
920+
916921
if !httpapi.Read(ctx, rw, r, &params) {
917922
return
918923
}

‎coderd/users_test.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"github.com/stretchr/testify/require"
2020
"golang.org/x/exp/slices"
2121
"golang.org/x/sync/errgroup"
22+
"golang.org/x/xerrors"
2223

2324
"github.com/coder/coder/v2/coderd/audit"
2425
"github.com/coder/coder/v2/coderd/coderdtest"
@@ -826,6 +827,7 @@ func TestUpdateUserPassword(t *testing.T) {
826827
})
827828
require.NoError(t, err, "member should login successfully with the new password")
828829
})
830+
829831
t.Run("MemberCanUpdateOwnPassword", func(t *testing.T) {
830832
t.Parallel()
831833
auditor := audit.NewMock()
@@ -853,6 +855,7 @@ func TestUpdateUserPassword(t *testing.T) {
853855
require.Len(t, auditor.AuditLogs(), numLogs)
854856
require.Equal(t, database.AuditActionWrite, auditor.AuditLogs()[numLogs-1].Action)
855857
})
858+
856859
t.Run("MemberCantUpdateOwnPasswordWithoutOldPassword", func(t *testing.T) {
857860
t.Parallel()
858861
client := coderdtest.New(t, nil)
@@ -867,6 +870,41 @@ func TestUpdateUserPassword(t *testing.T) {
867870
})
868871
require.Error(t, err, "member should not be able to update own password without providing old password")
869872
})
873+
874+
t.Run("AuditorCantTellIfPasswordIncorrect", func(t *testing.T) {
875+
t.Parallel()
876+
auditor := audit.NewMock()
877+
adminClient := coderdtest.New(t, &coderdtest.Options{Auditor: auditor})
878+
879+
adminUser := coderdtest.CreateFirstUser(t, adminClient)
880+
881+
auditorClient, _ := coderdtest.CreateAnotherUser(t, adminClient,
882+
adminUser.OrganizationID,
883+
rbac.RoleAuditor(),
884+
)
885+
886+
_, memberUser := coderdtest.CreateAnotherUser(t, adminClient, adminUser.OrganizationID)
887+
numLogs := len(auditor.AuditLogs())
888+
889+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
890+
defer cancel()
891+
892+
err := auditorClient.UpdateUserPassword(ctx, memberUser.ID.String(), codersdk.UpdateUserPasswordRequest{
893+
Password: "MySecurePassword!",
894+
})
895+
numLogs++ // add an audit log for user update
896+
897+
require.Error(t, err, "auditors shouldn't be able to update passwords")
898+
var httpErr *codersdk.Error
899+
require.True(t, xerrors.As(err, &httpErr))
900+
// ensure that the error we get is "not found" and not "bad request"
901+
require.Equal(t, http.StatusNotFound, httpErr.StatusCode())
902+
903+
require.Len(t, auditor.AuditLogs(), numLogs)
904+
require.Equal(t, database.AuditActionWrite, auditor.AuditLogs()[numLogs-1].Action)
905+
require.Equal(t, int32(http.StatusNotFound), auditor.AuditLogs()[numLogs-1].StatusCode)
906+
})
907+
870908
t.Run("AdminCanUpdateOwnPasswordWithoutOldPassword", func(t *testing.T) {
871909
t.Parallel()
872910
auditor := audit.NewMock()

0 commit comments

Comments
 (0)