diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 4bba9d9cb3..7d0f8db486 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -3995,6 +3995,8 @@ CREATE UNIQUE INDEX users_email_lower_idx ON users USING btree (lower(email)) WH CREATE UNIQUE INDEX users_username_lower_idx ON users USING btree (lower(username)) WHERE (deleted = false); +CREATE UNIQUE INDEX webpush_subscriptions_user_id_endpoint_idx ON webpush_subscriptions USING btree (user_id, endpoint); + CREATE INDEX workspace_agent_devcontainers_workspace_agent_id ON workspace_agent_devcontainers USING btree (workspace_agent_id); COMMENT ON INDEX workspace_agent_devcontainers_workspace_agent_id IS 'Workspace agent foreign key and query index'; diff --git a/coderd/database/migrations/000479_webpush_subscriptions_unique_endpoint.down.sql b/coderd/database/migrations/000479_webpush_subscriptions_unique_endpoint.down.sql new file mode 100644 index 0000000000..1125b6fe23 --- /dev/null +++ b/coderd/database/migrations/000479_webpush_subscriptions_unique_endpoint.down.sql @@ -0,0 +1 @@ +DROP INDEX IF EXISTS webpush_subscriptions_user_id_endpoint_idx; diff --git a/coderd/database/migrations/000479_webpush_subscriptions_unique_endpoint.up.sql b/coderd/database/migrations/000479_webpush_subscriptions_unique_endpoint.up.sql new file mode 100644 index 0000000000..01a16f69ae --- /dev/null +++ b/coderd/database/migrations/000479_webpush_subscriptions_unique_endpoint.up.sql @@ -0,0 +1,21 @@ +-- Make webpush subscriptions idempotent on (user_id, endpoint). +-- +-- Without a unique constraint, a re-subscribe with the same endpoint +-- (which Apple Web Push and other push services do when keys rotate +-- without endpoint deactivation, including after a PWA reinstall on +-- iOS) inserts a duplicate row carrying the new keys. Dispatch then +-- delivers to both endpoints; the device cannot decrypt the old one +-- and silently drops it. +-- +-- Dedupe existing rows before adding the index. Keep the freshest row +-- per (user_id, endpoint) since it most likely matches the device's +-- current p256dh / auth keys. The duplicates being deleted here are +-- by definition stale. +DELETE FROM webpush_subscriptions a +USING webpush_subscriptions b +WHERE a.user_id = b.user_id + AND a.endpoint = b.endpoint + AND (a.created_at, a.id) < (b.created_at, b.id); + +CREATE UNIQUE INDEX webpush_subscriptions_user_id_endpoint_idx + ON webpush_subscriptions (user_id, endpoint); diff --git a/coderd/database/querier.go b/coderd/database/querier.go index ab0ac71e33..a31d7e3813 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -876,6 +876,10 @@ type sqlcQuerier interface { InsertUserGroupsByID(ctx context.Context, arg InsertUserGroupsByIDParams) ([]uuid.UUID, error) InsertUserLink(ctx context.Context, arg InsertUserLinkParams) (UserLink, error) InsertVolumeResourceMonitor(ctx context.Context, arg InsertVolumeResourceMonitorParams) (WorkspaceAgentVolumeResourceMonitor, error) + // Inserts or updates a webpush subscription. The (user_id, endpoint) pair + // is unique; re-subscribing the same endpoint replaces the keys instead of + // inserting a duplicate row. This is the recovery path after a PWA reinstall + // on iOS, where the browser may keep the same endpoint with rotated keys. InsertWebpushSubscription(ctx context.Context, arg InsertWebpushSubscriptionParams) (WebpushSubscription, error) InsertWorkspace(ctx context.Context, arg InsertWorkspaceParams) (WorkspaceTable, error) InsertWorkspaceAgent(ctx context.Context, arg InsertWorkspaceAgentParams) (WorkspaceAgent, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 33ddec0052..7dd5657ee6 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -14336,6 +14336,10 @@ func (q *sqlQuerier) GetWebpushSubscriptionsByUserID(ctx context.Context, userID const insertWebpushSubscription = `-- name: InsertWebpushSubscription :one INSERT INTO webpush_subscriptions (user_id, created_at, endpoint, endpoint_p256dh_key, endpoint_auth_key) VALUES ($1, $2, $3, $4, $5) +ON CONFLICT (user_id, endpoint) DO UPDATE + SET endpoint_p256dh_key = EXCLUDED.endpoint_p256dh_key, + endpoint_auth_key = EXCLUDED.endpoint_auth_key, + created_at = EXCLUDED.created_at RETURNING id, user_id, created_at, endpoint, endpoint_p256dh_key, endpoint_auth_key ` @@ -14347,6 +14351,10 @@ type InsertWebpushSubscriptionParams struct { EndpointAuthKey string `db:"endpoint_auth_key" json:"endpoint_auth_key"` } +// Inserts or updates a webpush subscription. The (user_id, endpoint) pair +// is unique; re-subscribing the same endpoint replaces the keys instead of +// inserting a duplicate row. This is the recovery path after a PWA reinstall +// on iOS, where the browser may keep the same endpoint with rotated keys. func (q *sqlQuerier) InsertWebpushSubscription(ctx context.Context, arg InsertWebpushSubscriptionParams) (WebpushSubscription, error) { row := q.db.QueryRowContext(ctx, insertWebpushSubscription, arg.UserID, diff --git a/coderd/database/queries/notifications.sql b/coderd/database/queries/notifications.sql index bf65855925..01e029fda3 100644 --- a/coderd/database/queries/notifications.sql +++ b/coderd/database/queries/notifications.sql @@ -196,8 +196,16 @@ FROM webpush_subscriptions WHERE user_id = @user_id::uuid; -- name: InsertWebpushSubscription :one +-- Inserts or updates a webpush subscription. The (user_id, endpoint) pair +-- is unique; re-subscribing the same endpoint replaces the keys instead of +-- inserting a duplicate row. This is the recovery path after a PWA reinstall +-- on iOS, where the browser may keep the same endpoint with rotated keys. INSERT INTO webpush_subscriptions (user_id, created_at, endpoint, endpoint_p256dh_key, endpoint_auth_key) VALUES ($1, $2, $3, $4, $5) +ON CONFLICT (user_id, endpoint) DO UPDATE + SET endpoint_p256dh_key = EXCLUDED.endpoint_p256dh_key, + endpoint_auth_key = EXCLUDED.endpoint_auth_key, + created_at = EXCLUDED.created_at RETURNING *; -- name: DeleteWebpushSubscriptions :exec diff --git a/coderd/database/unique_constraint.go b/coderd/database/unique_constraint.go index b3b2a99201..c7d45e1844 100644 --- a/coderd/database/unique_constraint.go +++ b/coderd/database/unique_constraint.go @@ -153,6 +153,7 @@ const ( UniqueUserSecretsUserNameIndex UniqueConstraint = "user_secrets_user_name_idx" // CREATE UNIQUE INDEX user_secrets_user_name_idx ON user_secrets USING btree (user_id, name); UniqueUsersEmailLowerIndex UniqueConstraint = "users_email_lower_idx" // CREATE UNIQUE INDEX users_email_lower_idx ON users USING btree (lower(email)) WHERE ((deleted = false) AND (email <> ''::text)); UniqueUsersUsernameLowerIndex UniqueConstraint = "users_username_lower_idx" // CREATE UNIQUE INDEX users_username_lower_idx ON users USING btree (lower(username)) WHERE (deleted = false); + UniqueWebpushSubscriptionsUserIDEndpointIndex UniqueConstraint = "webpush_subscriptions_user_id_endpoint_idx" // CREATE UNIQUE INDEX webpush_subscriptions_user_id_endpoint_idx ON webpush_subscriptions USING btree (user_id, endpoint); UniqueWorkspaceAppAuditSessionsUniqueIndex UniqueConstraint = "workspace_app_audit_sessions_unique_index" // CREATE UNIQUE INDEX workspace_app_audit_sessions_unique_index ON workspace_app_audit_sessions USING btree (agent_id, app_id, user_id, ip, user_agent, slug_or_port, status_code); UniqueWorkspaceProxiesLowerNameIndex UniqueConstraint = "workspace_proxies_lower_name_idx" // CREATE UNIQUE INDEX workspace_proxies_lower_name_idx ON workspace_proxies USING btree (lower(name)) WHERE (deleted = false); UniqueWorkspacesOwnerIDLowerIndex UniqueConstraint = "workspaces_owner_id_lower_idx" // CREATE UNIQUE INDEX workspaces_owner_id_lower_idx ON workspaces USING btree (owner_id, lower((name)::text)) WHERE (deleted = false); diff --git a/coderd/webpush/webpush.go b/coderd/webpush/webpush.go index dc91d1eb9f..f554c3870a 100644 --- a/coderd/webpush/webpush.go +++ b/coderd/webpush/webpush.go @@ -29,6 +29,22 @@ import ( const defaultSubscriptionCacheTTL = 3 * time.Minute +// isStaleSubscriptionStatus reports whether a status code from a push +// service indicates that the subscription is permanently invalid and +// should be removed from the database. Other 4xx and 5xx responses +// (rate limits, transient failures) leave the subscription in place +// so it can be retried on the next dispatch. +func isStaleSubscriptionStatus(statusCode int) bool { + switch statusCode { + case http.StatusBadRequest, // 400: malformed subscription per the push service. + http.StatusForbidden, // 403: Apple BadJwtToken / VAPID rejected, key rotation. + http.StatusNotFound, // 404: FCM/Mozilla endpoint no longer valid. + http.StatusGone: // 410: standard "subscription expired" signal. + return true + } + return false +} + // Dispatcher is an interface that can be used to dispatch // web push notifications to clients such as browsers. type Dispatcher interface { @@ -203,11 +219,23 @@ func (n *Webpusher) Dispatch(ctx context.Context, userID uuid.UUID, msg codersdk return xerrors.Errorf("send webpush notification: %w", err) } - if statusCode == http.StatusGone { - // The subscription is no longer valid, remove it. + if isStaleSubscriptionStatus(statusCode) { + // Remove subscriptions that the push service has marked as + // permanently invalid (Apple returns 403 BadJwtToken and 404 + // for invalidated subscriptions, FCM returns 404 for + // expired endpoints, all push services return 410 for + // permanently gone subscriptions, and 400 indicates a + // malformed subscription that cannot be retried). Without + // this, stale rows accumulate after PWA reinstalls and the + // in-memory cache keeps trying to deliver to dead + // subscriptions. mu.Lock() cleanupSubscriptions = append(cleanupSubscriptions, subscription.ID) mu.Unlock() + } + + if statusCode == http.StatusGone { + // 410 Gone is informational, not a delivery error. return nil } @@ -221,24 +249,43 @@ func (n *Webpusher) Dispatch(ctx context.Context, userID uuid.UUID, msg codersdk }) } - err = eg.Wait() - if err != nil { - return xerrors.Errorf("send webpush notifications: %w", err) - } + dispatchErr := eg.Wait() - if len(cleanupSubscriptions) > 0 { - // nolint:gocritic // These are known to be invalid subscriptions. - err = n.store.DeleteWebpushSubscriptions(dbauthz.AsNotifier(ctx), cleanupSubscriptions) - if err != nil { - n.log.Error(ctx, "failed to delete stale push subscriptions", slog.Error(err)) - } else { - n.pruneSubscriptions(userID, cleanupSubscriptions) - } + // Always remove subscriptions that the push service rejected as + // permanently invalid, even when sibling deliveries returned a + // non-stale error. The cleanup must run before the error return so a + // transient delivery failure on one subscription cannot block the + // deletion of a 410/404/403/400 sibling. Without this ordering, + // stale rows accumulate after PWA reinstalls and silently mask the + // new subscription on every subsequent dispatch. + n.cleanupStaleSubscriptions(ctx, userID, cleanupSubscriptions) + + if dispatchErr != nil { + return xerrors.Errorf("send webpush notifications: %w", dispatchErr) } return nil } +// cleanupStaleSubscriptions deletes the rows the push service flagged as +// permanently invalid (see isStaleSubscriptionStatus) and clears the cached +// entries for the affected user. Failures are logged at error level rather +// than returned: the caller is in the middle of returning a delivery error +// and shouldn't have its error shadowed by a cleanup failure. The cache +// prune is gated on a successful database delete so a partial state cannot +// leak into the cache. +func (n *Webpusher) cleanupStaleSubscriptions(ctx context.Context, userID uuid.UUID, ids []uuid.UUID) { + if len(ids) == 0 { + return + } + // nolint:gocritic // These are known to be invalid subscriptions. + if err := n.store.DeleteWebpushSubscriptions(dbauthz.AsNotifier(ctx), ids); err != nil { + n.log.Error(ctx, "failed to delete stale push subscriptions", slog.Error(err)) + return + } + n.pruneSubscriptions(userID, ids) +} + func (n *Webpusher) subscriptionsForUser(ctx context.Context, userID uuid.UUID) ([]database.WebpushSubscription, error) { if subscriptions, ok := n.cachedSubscriptions(userID); ok { return subscriptions, nil diff --git a/coderd/webpush/webpush_test.go b/coderd/webpush/webpush_test.go index 61c83d4bd4..8a30214d89 100644 --- a/coderd/webpush/webpush_test.go +++ b/coderd/webpush/webpush_test.go @@ -102,12 +102,14 @@ func TestPush(t *testing.T) { }) t.Run("FailedDelivery", func(t *testing.T) { + // 5xx responses are transient failures. The subscription should + // remain after a failed delivery so it can be retried later. t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) manager, store, serverURL := setupPushTest(ctx, t, func(w http.ResponseWriter, r *http.Request) { assertWebpushPayload(t, r) - w.WriteHeader(http.StatusBadRequest) - w.Write([]byte("Invalid request")) + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte("Internal server error")) }) user := dbgen.User(t, store, database.User{}) @@ -123,7 +125,7 @@ func TestPush(t *testing.T) { msg := randomWebpushMessage(t) err = manager.Dispatch(ctx, user.ID, msg) require.Error(t, err) - assert.Contains(t, err.Error(), "Invalid request") + assert.Contains(t, err.Error(), "Internal server error") subscriptions, err := store.GetWebpushSubscriptionsByUserID(ctx, user.ID) require.NoError(t, err) @@ -131,6 +133,138 @@ func TestPush(t *testing.T) { assert.Equal(t, subscriptions[0].ID, sub.ID, "The subscription should not be deleted") }) + // StaleSubscriptionStatuses verifies that documented permanent-failure + // status codes from the push service cause the subscription to be + // deleted. iOS Safari returns 404 and 403 BadJwtToken for invalidated + // subscriptions, FCM returns 404 for endpoints that are no longer + // valid, and a 400 means the subscription cannot be used. + t.Run("StaleSubscriptionStatuses", func(t *testing.T) { + t.Parallel() + cases := []struct { + name string + statusCode int + body string + expectError bool + expectErrorMsg string + }{ + { + name: "NotFound", + statusCode: http.StatusNotFound, + body: "Not Found", + expectError: true, + expectErrorMsg: "Not Found", + }, + { + name: "Forbidden", + statusCode: http.StatusForbidden, + body: "BadJwtToken", + expectError: true, + expectErrorMsg: "BadJwtToken", + }, + { + name: "BadRequest", + statusCode: http.StatusBadRequest, + body: "Invalid request", + expectError: true, + expectErrorMsg: "Invalid request", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + manager, store, serverURL := setupPushTest(ctx, t, func(w http.ResponseWriter, r *http.Request) { + assertWebpushPayload(t, r) + w.WriteHeader(tc.statusCode) + w.Write([]byte(tc.body)) + }) + user := dbgen.User(t, store, database.User{}) + _, err := store.InsertWebpushSubscription(ctx, database.InsertWebpushSubscriptionParams{ + UserID: user.ID, + Endpoint: serverURL, + EndpointAuthKey: validEndpointAuthKey, + EndpointP256dhKey: validEndpointP256dhKey, + CreatedAt: dbtime.Now(), + }) + require.NoError(t, err) + + msg := randomWebpushMessage(t) + err = manager.Dispatch(ctx, user.ID, msg) + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectErrorMsg) + } else { + require.NoError(t, err) + } + + subscriptions, err := store.GetWebpushSubscriptionsByUserID(ctx, user.ID) + require.NoError(t, err) + assert.Len(t, subscriptions, 0, "Stale subscription should be deleted on %d", tc.statusCode) + }) + } + }) + + // StaleAndFailedSubscriptions verifies that a stale subscription + // returning 404 is cleaned up even when a sibling subscription's + // delivery fails with a transient error in the same Dispatch call. + // Regression test for the case where a delivery error short-circuits + // stale subscription cleanup, leaving permanently invalid rows in + // the database. + t.Run("StaleAndFailedSubscriptions", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + manager, store, server500URL := setupPushTest(ctx, t, func(w http.ResponseWriter, r *http.Request) { + assertWebpushPayload(t, r) + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte("transient error")) + }) + + serverStale := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assertWebpushPayload(t, r) + w.WriteHeader(http.StatusNotFound) + })) + defer serverStale.Close() + serverStaleURL := serverStale.URL + + user := dbgen.User(t, store, database.User{}) + + subFailed, err := store.InsertWebpushSubscription(ctx, database.InsertWebpushSubscriptionParams{ + UserID: user.ID, + Endpoint: server500URL, + EndpointAuthKey: validEndpointAuthKey, + EndpointP256dhKey: validEndpointP256dhKey, + CreatedAt: dbtime.Now(), + }) + require.NoError(t, err) + + _, err = store.InsertWebpushSubscription(ctx, database.InsertWebpushSubscriptionParams{ + UserID: user.ID, + Endpoint: serverStaleURL, + EndpointAuthKey: validEndpointAuthKey, + EndpointP256dhKey: validEndpointP256dhKey, + CreatedAt: dbtime.Now(), + }) + require.NoError(t, err) + + msg := randomWebpushMessage(t) + err = manager.Dispatch(ctx, user.ID, msg) + // Should still surface a delivery error from one of the + // failing siblings. errgroup returns whichever goroutine + // finishes with an error first, so the error may originate + // from either the 500 or the 404 sibling. The contract we + // care about is that the stale (404) subscription is + // cleaned up regardless of which error wins the race. + require.Error(t, err) + + // The stale subscription should have been cleaned up regardless. + subscriptions, err := store.GetWebpushSubscriptionsByUserID(ctx, user.ID) + require.NoError(t, err) + if assert.Len(t, subscriptions, 1, "Only the transiently failing subscription should remain") { + assert.Equal(t, subFailed.ID, subscriptions[0].ID, "The transiently failing subscription should not be deleted") + } + }) + t.Run("MultipleSubscriptions", func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) diff --git a/coderd/webpush_test.go b/coderd/webpush_test.go index 696a12052a..1151e0757c 100644 --- a/coderd/webpush_test.go +++ b/coderd/webpush_test.go @@ -13,6 +13,7 @@ import ( "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" @@ -93,6 +94,57 @@ func TestWebpushSubscribeUnsubscribe(t *testing.T) { require.Error(t, err, "delete webpush subscription for another user") } +// TestWebpushSubscribeOverwritesKeys verifies that re-subscribing with the +// same endpoint and rotated keys overwrites the existing row in place rather +// than inserting a duplicate. This is the reinstall path: on iOS, deleting +// the PWA from the home screen and reinstalling can yield the same endpoint +// with new p256dh / auth keys, and Coder must replace the stored keys so +// dispatch encrypts with the keys the device can decrypt. +func TestWebpushSubscribeOverwritesKeys(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + store, ps := dbtestutil.NewDB(t) + client := coderdtest.New(t, &coderdtest.Options{ + WebpushDispatcher: &testWebpushDispatcher{}, + Database: store, + Pubsub: ps, + }) + owner := coderdtest.CreateFirstUser(t, client) + memberClient, member := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) + + const endpoint = "https://push.example.com/subscription/reinstall" + const secondAuthKey = "AnotherAuthKey/yV1FuojuRmHP42==" + const secondP256dhKey = "BNNL5ZaTfK81qhXOx23+wewhigUeFb632jN6LvRWCFH1ubQr77FE/9qV1FuojuRmHP42zmf34rXgW80OvUVDgABc=" + + // First subscribe with the original keys. + err := memberClient.PostWebpushSubscription(ctx, "me", codersdk.WebpushSubscription{ + Endpoint: endpoint, + AuthKey: validEndpointAuthKey, + P256DHKey: validEndpointP256dhKey, + }) + require.NoError(t, err, "initial subscribe") + + // Re-subscribe with the same endpoint but rotated keys. This + // simulates the post-reinstall path on iOS where the browser + // retains the endpoint but rotates p256dh / auth. + err = memberClient.PostWebpushSubscription(ctx, "me", codersdk.WebpushSubscription{ + Endpoint: endpoint, + AuthKey: secondAuthKey, + P256DHKey: secondP256dhKey, + }) + require.NoError(t, err, "re-subscribe with rotated keys") + + // The second subscribe must replace the keys in place; we should + // see exactly one row carrying the new keys. + subs, err := store.GetWebpushSubscriptionsByUserID(dbauthz.AsSystemRestricted(ctx), member.ID) + require.NoError(t, err) + require.Len(t, subs, 1, "re-subscribe should overwrite the row, not append a duplicate") + require.Equal(t, endpoint, subs[0].Endpoint) + require.Equal(t, secondAuthKey, subs[0].EndpointAuthKey, "auth key should be the latest one") + require.Equal(t, secondP256dhKey, subs[0].EndpointP256dhKey, "p256dh key should be the latest one") +} + func TestWebpushSubscribeRejectsInvalidEndpoint(t *testing.T) { t.Parallel() diff --git a/site/src/serviceWorker.test.ts b/site/src/serviceWorker.test.ts index b398bfbb1e..faf12f7276 100644 --- a/site/src/serviceWorker.test.ts +++ b/site/src/serviceWorker.test.ts @@ -5,7 +5,11 @@ import type { WebpushMessage } from "#/api/typesGenerated"; // top level during module evaluation. const mockShowNotification = vi.fn(() => Promise.resolve()); -const mockRegistration = { showNotification: mockShowNotification }; +const mockSubscribe = vi.fn(); +const mockRegistration = { + showNotification: mockShowNotification, + pushManager: { subscribe: mockSubscribe }, +}; const mockMatchAll = vi.fn< () => Promise< @@ -326,3 +330,138 @@ describe("serviceWorker notificationclick handler", () => { expect(mockClients.openWindow).toHaveBeenCalledWith("/agents"); }); }); + +// Helper to build a minimal PushSubscriptionChangeEvent-like object. +// Mirrors the spec shape consumed by the service worker handler. +function makePushSubscriptionChangeEvent( + oldSubscription: { + endpoint: string; + options?: { applicationServerKey?: ArrayBuffer | Uint8Array | string }; + } | null, + newSubscription: { + endpoint: string; + options?: { applicationServerKey?: ArrayBuffer | Uint8Array | string }; + toJSON: () => unknown; + } | null, +) { + let waitUntilPromise: Promise = Promise.resolve(); + return { + oldSubscription, + newSubscription, + waitUntil: (p: Promise) => { + waitUntilPromise = p; + }, + get _waitUntilPromise() { + return waitUntilPromise; + }, + }; +} + +describe("serviceWorker pushsubscriptionchange handler", () => { + const originalFetch = globalThis.fetch; + let mockFetch: ReturnType; + + beforeEach(() => { + mockFetch = vi.fn(() => + Promise.resolve({ ok: true, status: 204 } as Response), + ); + globalThis.fetch = mockFetch as unknown as typeof fetch; + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + }); + + it("re-subscribes and POSTs the new subscription when the browser supplies one", async () => { + const applicationServerKey = new Uint8Array([1, 2, 3]).buffer; + const newSubscription = { + endpoint: "https://push.example.com/new", + options: { applicationServerKey }, + toJSON: () => ({ + endpoint: "https://push.example.com/new", + keys: { auth: "new-auth", p256dh: "new-p256dh" }, + }), + }; + const oldSubscription = { + endpoint: "https://push.example.com/old", + options: { applicationServerKey }, + }; + + const event = makePushSubscriptionChangeEvent( + oldSubscription, + newSubscription, + ); + handlers.pushsubscriptionchange(event); + await event._waitUntilPromise; + + // The browser already supplied newSubscription, so we must + // not call subscribe again. + expect(mockSubscribe).not.toHaveBeenCalled(); + + // Should have POSTed the new subscription and remove + // the old endpoint via DELETE. + const calls = mockFetch.mock.calls; + expect(calls).toHaveLength(2); + + const [postUrl, postInit] = calls[0]; + expect(postUrl).toBe("/api/v2/users/me/webpush/subscription"); + expect(postInit?.method).toBe("POST"); + const postBody = JSON.parse(postInit?.body as string); + expect(postBody).toEqual({ + endpoint: "https://push.example.com/new", + auth_key: "new-auth", + p256dh_key: "new-p256dh", + }); + + const [deleteUrl, deleteInit] = calls[1]; + expect(deleteUrl).toBe("/api/v2/users/me/webpush/subscription"); + expect(deleteInit?.method).toBe("DELETE"); + const deleteBody = JSON.parse(deleteInit?.body as string); + expect(deleteBody).toEqual({ + endpoint: "https://push.example.com/old", + }); + }); + + it("re-subscribes via pushManager when no newSubscription is provided", async () => { + const applicationServerKey = new Uint8Array([4, 5, 6]).buffer; + const oldSubscription = { + endpoint: "https://push.example.com/old", + options: { applicationServerKey }, + }; + const freshSubscription = { + endpoint: "https://push.example.com/fresh", + toJSON: () => ({ + endpoint: "https://push.example.com/fresh", + keys: { auth: "fresh-auth", p256dh: "fresh-p256dh" }, + }), + }; + mockSubscribe.mockResolvedValue(freshSubscription); + + const event = makePushSubscriptionChangeEvent(oldSubscription, null); + handlers.pushsubscriptionchange(event); + await event._waitUntilPromise; + + expect(mockSubscribe).toHaveBeenCalledWith({ + userVisibleOnly: true, + applicationServerKey, + }); + + const calls = mockFetch.mock.calls; + expect(calls).toHaveLength(2); + const postBody = JSON.parse(calls[0][1]?.body as string); + expect(postBody).toEqual({ + endpoint: "https://push.example.com/fresh", + auth_key: "fresh-auth", + p256dh_key: "fresh-p256dh", + }); + }); + + it("does nothing when there is no application server key to recover", async () => { + const event = makePushSubscriptionChangeEvent(null, null); + handlers.pushsubscriptionchange(event); + await event._waitUntilPromise; + + expect(mockSubscribe).not.toHaveBeenCalled(); + expect(mockFetch).not.toHaveBeenCalled(); + }); +}); diff --git a/site/src/serviceWorker.ts b/site/src/serviceWorker.ts index eafd09925b..90777eb854 100644 --- a/site/src/serviceWorker.ts +++ b/site/src/serviceWorker.ts @@ -54,6 +54,64 @@ self.addEventListener("push", (event) => { ); }); +// Handle key rotation. The Push API spec requires the user agent to fire +// pushsubscriptionchange when the push service rotates the keys. Without +// this handler the device's local subscription updates but the server keeps +// the old keys, which is the failure mode after a PWA reinstall on iOS: +// the bell stays green client-side because pushManager.getSubscription() +// returns the new subscription, but coderd encrypts to the old keys and +// the device silently drops the message. +self.addEventListener("pushsubscriptionchange", (event) => { + event.waitUntil(handlePushSubscriptionChange(event)); +}); + +async function handlePushSubscriptionChange( + event: PushSubscriptionChangeEvent, +): Promise { + // Prefer the application server key the browser already negotiated, so + // we don't depend on the React side delivering a fresh VAPID key into + // the worker. Fall back to the old subscription's options when the new + // one is null (the browser revoked without auto-renewing). + const applicationServerKey = + event.newSubscription?.options.applicationServerKey ?? + event.oldSubscription?.options.applicationServerKey; + if (!applicationServerKey) { + return; + } + + const subscription = + event.newSubscription ?? + (await self.registration.pushManager.subscribe({ + userVisibleOnly: true, + applicationServerKey, + })); + + const json = subscription.toJSON(); + if (!json.endpoint || !json.keys) { + return; + } + + await fetch("/api/v2/users/me/webpush/subscription", { + method: "POST", + headers: { "Content-Type": "application/json" }, + credentials: "include", + body: JSON.stringify({ + endpoint: json.endpoint, + auth_key: json.keys.auth, + p256dh_key: json.keys.p256dh, + }), + }); + + if (event.oldSubscription) { + await fetch("/api/v2/users/me/webpush/subscription", { + method: "DELETE", + headers: { "Content-Type": "application/json" }, + credentials: "include", + body: JSON.stringify({ endpoint: event.oldSubscription.endpoint }), + }); + } +} + // Handle notification click — navigate to the specific chat or agents page. self.addEventListener("notificationclick", (event) => { event.notification.close();