From d787b3cadaf429ebd8b19d62cd977a2fe29bffa1 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 2 Mar 2026 21:11:20 +0000 Subject: [PATCH] fix(coderd): fix error handling in deleteUserWebpushSubscription (#22500) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary `deleteUserWebpushSubscription` in `coderd/webpush.go` had incorrect error handling that masked database errors as 404 responses. ## Bug `GetWebpushSubscriptionsByUserID` is a `:many` query — it returns `([], nil)` when no rows match, never `sql.ErrNoRows`. The previous `if/else if` chain: ```go if existing, err := api.Database.GetWebpushSubscriptionsByUserID(ctx, user.ID); err != nil && errors.Is(err, sql.ErrNoRows) { // dead code — :many queries never return sql.ErrNoRows } else if idx := slices.IndexFunc(existing, ...); idx == -1 { // real DB errors fall through here, existing is nil, idx is -1 → 404 } ``` Any real database error (connection failure, timeout, authorization error) fell through to the `else if` branch where `slices.IndexFunc(nil, ...)` returns `-1`, returning 404 "subscription not found" instead of 500. ## Fix Split into two separate checks so database errors properly return 500: ```go existing, err := api.Database.GetWebpushSubscriptionsByUserID(ctx, user.ID) if err != nil { // 500 } if idx := slices.IndexFunc(existing, ...); idx == -1 { // 404 } ``` ## Testing Added `TestDeleteWebpushSubscription/database_error_returns_500` which wraps the DB store to inject an error into `GetWebpushSubscriptionsByUserID` and asserts the handler returns 500 (not 404). --- coderd/webpush.go | 11 +++++--- coderd/webpush_test.go | 57 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/coderd/webpush.go b/coderd/webpush.go index 893401552d..3e84aa012e 100644 --- a/coderd/webpush.go +++ b/coderd/webpush.go @@ -88,12 +88,15 @@ func (api *API) deleteUserWebpushSubscription(rw http.ResponseWriter, r *http.Re } // Return NotFound if the subscription does not exist. - if existing, err := api.Database.GetWebpushSubscriptionsByUserID(ctx, user.ID); err != nil && errors.Is(err, sql.ErrNoRows) { - httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ - Message: "Webpush subscription not found.", + existing, err := api.Database.GetWebpushSubscriptionsByUserID(ctx, user.ID) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to get webpush subscriptions.", + Detail: err.Error(), }) return - } else if idx := slices.IndexFunc(existing, func(s database.WebpushSubscription) bool { + } + if idx := slices.IndexFunc(existing, func(s database.WebpushSubscription) bool { return s.Endpoint == req.Endpoint }); idx == -1 { httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ diff --git a/coderd/webpush_test.go b/coderd/webpush_test.go index f41639b99e..467f507881 100644 --- a/coderd/webpush_test.go +++ b/coderd/webpush_test.go @@ -1,13 +1,19 @@ package coderd_test import ( + "context" "net/http" "net/http/httptest" + "sync/atomic" "testing" + "github.com/google/uuid" "github.com/stretchr/testify/require" + "golang.org/x/xerrors" "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" ) @@ -80,3 +86,54 @@ func TestWebpushSubscribeUnsubscribe(t *testing.T) { }) require.Error(t, err, "delete webpush subscription for another user") } + +// testWebpushErrorStore wraps a real database.Store and allows injecting +// errors into GetWebpushSubscriptionsByUserID. +type testWebpushErrorStore struct { + database.Store + getWebpushSubscriptionsErr atomic.Pointer[error] +} + +func (s *testWebpushErrorStore) GetWebpushSubscriptionsByUserID(ctx context.Context, userID uuid.UUID) ([]database.WebpushSubscription, error) { + if err := s.getWebpushSubscriptionsErr.Load(); err != nil { + return nil, *err + } + return s.Store.GetWebpushSubscriptionsByUserID(ctx, userID) +} + +func TestDeleteWebpushSubscription(t *testing.T) { + t.Parallel() + + t.Run("database error returns 500", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitMedium) + + store, ps := dbtestutil.NewDB(t) + wrappedStore := &testWebpushErrorStore{Store: store} + + dv := coderdtest.DeploymentValues(t) + dv.Experiments = []string{string(codersdk.ExperimentWebPush)} + client := coderdtest.New(t, &coderdtest.Options{ + DeploymentValues: dv, + Database: wrappedStore, + Pubsub: ps, + }) + owner := coderdtest.CreateFirstUser(t, client) + memberClient, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) + + // Inject a database error into + // GetWebpushSubscriptionsByUserID. The handler should + // return 500, not mask the error as 404. + dbErr := xerrors.New("database is unavailable") + wrappedStore.getWebpushSubscriptionsErr.Store(&dbErr) + + err := memberClient.DeleteWebpushSubscription(ctx, "me", codersdk.DeleteWebpushSubscription{ + Endpoint: "https://push.example.com/test", + }) + var sdkError *codersdk.Error + require.Error(t, err) + require.ErrorAsf(t, err, &sdkError, "error should be of type *codersdk.Error") + require.Equal(t, http.StatusInternalServerError, sdkError.StatusCode(), "database errors should return 500, not be masked as 404") + }) +}