diff --git a/notify/notification_apns.go b/notify/notification_apns.go index 961c67e83..342b043fc 100644 --- a/notify/notification_apns.go +++ b/notify/notification_apns.go @@ -452,20 +452,25 @@ func GetIOSNotification(req *PushNotification) *apns2.Notification { return notification } -func getApnsClient(cfg *config.ConfYaml, req *PushNotification) (client *apns2.Client) { +func getApnsClient(cfg *config.ConfYaml, req *PushNotification) *apns2.Client { + // Copy the shared client so setting the host per request does not mutate the + // process-wide ApnsClient. apns2's Production/Development mutate the receiver, + // so calling them on the global would race and could redirect concurrent + // pushes to the wrong host. HTTPClient and Token are safe to share. + client := *ApnsClient switch { case req.Production: - client = ApnsClient.Production() + client.Host = apns2.HostProduction case req.Development: - client = ApnsClient.Development() + client.Host = apns2.HostDevelopment default: if cfg.Ios.Production { - client = ApnsClient.Production() + client.Host = apns2.HostProduction } else { - client = ApnsClient.Development() + client.Host = apns2.HostDevelopment } } - return client + return &client } // PushToIOS provide send notification to APNs server. diff --git a/notify/notification_apns_host_test.go b/notify/notification_apns_host_test.go new file mode 100644 index 000000000..ad4900916 --- /dev/null +++ b/notify/notification_apns_host_test.go @@ -0,0 +1,92 @@ +package notify + +import ( + "context" + "sync" + "testing" + + "github.com/appleboy/gorush/config" + + "github.com/sideshow/apns2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// getApnsClient must pick the APNs host from the per-request override first, +// then fall back to the iOS config default. +func TestGetApnsClientHost(t *testing.T) { + cfg, err := config.LoadConf() + require.NoError(t, err) + cfg.Ios.Enabled = true + cfg.Ios.KeyPath = testKeyPath + require.NoError(t, InitAPNSClient(context.Background(), cfg)) + + tests := []struct { + name string + cfgProduction bool + req *PushNotification + wantHost string + }{ + {"request production override", false, &PushNotification{Production: true}, apns2.HostProduction}, + {"request development override", true, &PushNotification{Development: true}, apns2.HostDevelopment}, + {"config production default", true, &PushNotification{}, apns2.HostProduction}, + {"config development default", false, &PushNotification{}, apns2.HostDevelopment}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg.Ios.Production = tt.cfgProduction + got := getApnsClient(cfg, tt.req) + assert.Equal(t, tt.wantHost, got.Host) + }) + } +} + +// Resolving a client for one request must not change the host of a client +// already handed to another request. Before the fix, getApnsClient returned the +// shared global and mutated its Host in place, so a later development request +// silently repointed an earlier production client at the sandbox. +func TestGetApnsClientNoSharedMutation(t *testing.T) { + cfg, err := config.LoadConf() + require.NoError(t, err) + cfg.Ios.Enabled = true + cfg.Ios.KeyPath = testKeyPath + require.NoError(t, InitAPNSClient(context.Background(), cfg)) + + prod := getApnsClient(cfg, &PushNotification{Production: true}) + require.Equal(t, apns2.HostProduction, prod.Host) + + // Unrelated later request for the other environment. + dev := getApnsClient(cfg, &PushNotification{Development: true}) + + assert.Equal(t, apns2.HostProduction, prod.Host, + "production client host was mutated by a later development request") + assert.Equal(t, apns2.HostDevelopment, dev.Host) + assert.NotSame(t, ApnsClient, prod, "must not hand back the shared global client") +} + +// Concurrent resolution for different environments must not race on the shared +// global. Run with -race to exercise the regression. +func TestGetApnsClientConcurrent(t *testing.T) { + cfg, err := config.LoadConf() + require.NoError(t, err) + cfg.Ios.Enabled = true + cfg.Ios.KeyPath = testKeyPath + require.NoError(t, InitAPNSClient(context.Background(), cfg)) + + var wg sync.WaitGroup + for range 50 { + wg.Add(2) + go func() { + defer wg.Done() + c := getApnsClient(cfg, &PushNotification{Production: true}) + assert.Equal(t, apns2.HostProduction, c.Host) + }() + go func() { + defer wg.Done() + c := getApnsClient(cfg, &PushNotification{Development: true}) + assert.Equal(t, apns2.HostDevelopment, c.Host) + }() + } + wg.Wait() +}