fix(webpush): set vapid sub in webpush payload

johnstcn · johnstcn · commit 0c4e54bc9c88 · 2025-03-27T15:14:10.000Z
diff --git a/cli/server.go b/cli/server.go
@@ -779,7 +779,10 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
 			// Manage push notifications.
 			experiments := coderd.ReadExperiments(options.Logger, options.DeploymentValues.Experiments.Value())
 			if experiments.Enabled(codersdk.ExperimentWebPush) {
-				webpusher, err := webpush.New(ctx, &options.Logger, options.Database)
+				if !strings.HasPrefix(options.AccessURL.String(), "https://") {
+					options.Logger.Warn(ctx, "access URL is not HTTPS, so web push notifications may not work on some browsers", slog.F("access_url", options.AccessURL.String()))
+				}
+				webpusher, err := webpush.New(ctx, options.Logger.Named("webpush"), options.Database, options.AccessURL.String())
 				if err != nil {
 					options.Logger.Error(ctx, "failed to create web push dispatcher", slog.Error(err))
 					options.Logger.Warn(ctx, "web push notifications will not work until the VAPID keys are regenerated")
diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -284,7 +284,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
 
 	if options.WebpushDispatcher == nil {
 		// nolint:gocritic // Gets/sets VAPID keys.
-		pushNotifier, err := webpush.New(dbauthz.AsNotifier(context.Background()), options.Logger, options.Database)
+		pushNotifier, err := webpush.New(dbauthz.AsNotifier(context.Background()), *options.Logger, options.Database, options.AccessURL.String())
 		if err != nil {
 			panic(xerrors.Errorf("failed to create web push notifier: %w", err))
 		}
diff --git a/coderd/webpush/webpush.go b/coderd/webpush/webpush.go
@@ -41,13 +41,14 @@ type Dispatcher interface {
 // for updates inside of a workspace, which we want to be immediate.
 //
 // See: https://github.com/coder/internal/issues/528
-func New(ctx context.Context, log *slog.Logger, db database.Store) (Dispatcher, error) {
+func New(ctx context.Context, log slog.Logger, db database.Store, vapidSub string) (Dispatcher, error) {
 	keys, err := db.GetWebpushVAPIDKeys(ctx)
 	if err != nil {
 		if !errors.Is(err, sql.ErrNoRows) {
 			return nil, xerrors.Errorf("get notification vapid keys: %w", err)
 		}
 	}
+
 	if keys.VapidPublicKey == "" || keys.VapidPrivateKey == "" {
 		// Generate new VAPID keys. This also deletes all existing push
 		// subscriptions as part of the transaction, as they are no longer
@@ -62,6 +63,7 @@ func New(ctx context.Context, log *slog.Logger, db database.Store) (Dispatcher,
 	}
 
 	return &Webpusher{
+		vapidSub:        vapidSub,
 		store:           db,
 		log:             log,
 		VAPIDPublicKey:  keys.VapidPublicKey,
@@ -71,8 +73,14 @@ func New(ctx context.Context, log *slog.Logger, db database.Store) (Dispatcher,
 
 type Webpusher struct {
 	store database.Store
-	log   *slog.Logger
-
+	log   slog.Logger
+	// VAPID allows us to identify the sender of the message.
+	// This must be a https:// URL or an email address.
+	// Some push services (such as Apple's) require this to be set.
+	vapidSub string
+
+	// public and private keys for VAPID. These are used to sign and encrypt
+	// the message payload.
 	VAPIDPublicKey  string
 	VAPIDPrivateKey string
 }
@@ -148,10 +156,13 @@ func (n *Webpusher) webpushSend(ctx context.Context, msg []byte, endpoint string
 		Endpoint: endpoint,
 		Keys:     keys,
 	}, &webpush.Options{
+		Subscriber:      n.vapidSub,
 		VAPIDPublicKey:  n.VAPIDPublicKey,
 		VAPIDPrivateKey: n.VAPIDPrivateKey,
 	})
 	if err != nil {
+		n.log.Error(ctx, "failed to send webpush notification", slog.Error(err), slog.F("endpoint", endpoint))
+		n.log.Debug(ctx, "webpush notification payload", slog.F("payload", string(cpy)))
 		return -1, nil, xerrors.Errorf("send webpush notification: %w", err)
 	}
 	defer resp.Body.Close()
diff --git a/coderd/webpush/webpush_test.go b/coderd/webpush/webpush_test.go
@@ -253,7 +253,7 @@ func setupPushTest(ctx context.Context, t *testing.T, handlerFunc func(w http.Re
 	server := httptest.NewServer(http.HandlerFunc(handlerFunc))
 	t.Cleanup(server.Close)
 
-	manager, err := webpush.New(ctx, &logger, db)
+	manager, err := webpush.New(ctx, logger, db, "http://example.com")
 	require.NoError(t, err, "Failed to create webpush manager")
 
 	return manager, db, server.URL

Original file line number	Diff line number	Diff line change
`@@ -284,7 +284,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can`
`284`	`284`
`285`	`285`	`if options.WebpushDispatcher == nil {`
`286`	`286`	`// nolint:gocritic // Gets/sets VAPID keys.`
`287`		`- pushNotifier, err := webpush.New(dbauthz.AsNotifier(context.Background()), options.Logger, options.Database)`
	`287`	`+ pushNotifier, err := webpush.New(dbauthz.AsNotifier(context.Background()), *options.Logger, options.Database, options.AccessURL.String())`
`288`	`288`	`if err != nil {`
`289`	`289`	`panic(xerrors.Errorf("failed to create web push notifier: %w", err))`
`290`	`290`	`}`