Merge pull request #26 from kristianwind/fix-apple-web-push-subject

Fix: Apple rejected all push notifications (VAPID subject)

internal/httpapi/notifications_handlers.go

@@ -148,15 +148,23 @@ func (s *Server) pushToUser(userID, title, body, projectID string) pushOutcome {
 		return out
 	}
 
-	payload := push.Payload{Title: title, Body: body, URL: "/projekt/" + projectID}
+	target := "/projekt/" + projectID
+	if projectID == "" {
+		target = "/"
+	}
+	payload := push.Payload{Title: title, Body: body, URL: target}
+	subject := s.vapidSubject()
 	for _, sub := range subs {
 		err := push.Send(ctx, push.Subscription{
 			Endpoint: sub.Endpoint, P256dh: sub.P256dh, Auth: sub.Auth,
 		}, payload, push.VAPID{
-			Public: public, Private: private, Subject: "mailto:" + s.cfg.SMTP.From,
+			Public: public, Private: private, Subject: subject,
 		})
 		if err == nil {
 			out.Sent++
+			if delErr := s.db.MarkPushDelivered(ctx, sub.Endpoint); delErr != nil {
+				s.log.Warn("mark push delivered", "err", delErr)
+			}
 			continue
 		}
 		// Warn, not Debug.
@@ -257,6 +265,29 @@ func (s *Server) vapidKeys(ctx contextLike) (string, string, error) {
 	return s.db.InstanceKeys(ctx, push.GenerateVAPIDKeys)
 }
 
+// vapidSubject is the "sub" claim a push service is told to reach the sender at.
+//
+// It has to be one the service will actually accept, and Apple is the strict one:
+// it answers 403 Forbidden to a mailto whose domain is made up — which is exactly
+// what the old fallback, mailto:verdande@example.invalid, was on an instance with
+// no mail set up. Every push to an iPhone or a Mac was rejected, silently as far as
+// the person waiting for the notification could tell.
+//
+// So a real address is used when there is one, and otherwise the instance's own
+// https address — which is a valid VAPID subject, points at something that exists,
+// and Apple accepts. Verified against web.push.apple.com: the mailto gets a 403,
+// the https URL a 201.
+func (s *Server) vapidSubject() string {
+	from := strings.TrimSpace(s.cfg.SMTP.From)
+	if from != "" && strings.Contains(from, "@") && !strings.HasSuffix(from, "@localhost") {
+		return "mailto:" + from
+	}
+	if strings.HasPrefix(s.cfg.BaseURL, "https://") || strings.HasPrefix(s.cfg.BaseURL, "http://") {
+		return s.cfg.BaseURL
+	}
+	return "mailto:verdande@example.invalid"
+}
+
 // --- subscription endpoints -----------------------------------------------------------
 
 type pushSubscriptionRequest struct {

internal/httpapi/push_subject_test.go

@@ -0,0 +1,33 @@
+package httpapi
+
+import (
+	"testing"
+
+	"github.com/kristianwind/verdande/internal/config"
+)
+
+// The VAPID subject has to be one a push service will accept. Apple answers 403 to
+// a mailto with a made-up domain, which is what the no-mail fallback used to be, so
+// every push to an Apple device was refused. With no real address, the instance's
+// own https URL is used instead — valid, real, and accepted.
+func TestVAPIDSubject(t *testing.T) {
+	cases := []struct {
+		name    string
+		from    string
+		baseURL string
+		want    string
+	}{
+		{"a real address is used as-is", "todo@example.dk", "https://verdande.example.dk", "mailto:todo@example.dk"},
+		{"no mail falls back to the https url", "", "https://verdande.example.dk", "https://verdande.example.dk"},
+		{"the localhost default is not a real address", "verdande@localhost", "https://verdande.example.dk", "https://verdande.example.dk"},
+		{"a plain http dev url still serves as a subject", "", "http://localhost:8080", "http://localhost:8080"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			s := &Server{cfg: &config.Config{BaseURL: tc.baseURL, SMTP: config.SMTP{From: tc.from}}}
+			if got := s.vapidSubject(); got != tc.want {
+				t.Errorf("vapidSubject() = %q, want %q", got, tc.want)
+			}
+		})
+	}
+}

internal/store/notifications.go

@@ -204,6 +204,17 @@ func (db *DB) dropPushEndpoint(ctx context.Context, endpoint string) error {
 // has failed enough times. A push service says 404 or 410 when an endpoint is gone,
 // but it also has bad days — forgiving a few failures keeps a flaky network from
 // silently unsubscribing somebody's phone.
+// MarkPushDelivered records that a push went through: the failure count goes back
+// to zero and last_used_at moves forward. Without it a subscription that failed a
+// few times and then recovered would keep those failures on its record and be one
+// flaky night away from the ten that delete it — even though it is working.
+func (db *DB) MarkPushDelivered(ctx context.Context, endpoint string) error {
+	_, err := db.ExecContext(ctx,
+		`UPDATE push_subscriptions SET failures = 0, last_used_at = ? WHERE endpoint = ?`,
+		time.Now().Unix(), endpoint)
+	return err
+}
+
 func (db *DB) RecordPushFailure(ctx context.Context, endpoint string, permanent bool) error {
 	if permanent {
 		// Not scoped by user, and that is right here: this is the push service