Concurrent iOS pushes can be delivered to the wrong APNs host
- Dominant language
- Go
- Stars
- 8.8k
- Forks
- 887
- PR merge metrics
- No merged PRs in 30d
Description
### Summary
[`getApnsClient`](https://github.com/appleboy/gorush/blob/891b4530db8f5db2beeb44b42665999ab802799f/notify/notification_apns.go#L455-L469) selects the APNs environment by calling `Production()` / `Development()` on the shared global `ApnsClient`. Those two apns2 methods mutate their receiver rather than returning a configured copy, so the call writes to process-global state and hands back that same pointer.
The result is a data race on `ApnsClient.Host`. When two pushes overlap and target different environments, the last writer wins for every in-flight goroutine in both, so a notification sent with `"production": true` can be delivered to the sandbox host and one sent with `"development": true` can be delivered to production.
### Details
[`getApnsClient`](https://github.com/appleboy/gorush/blob/891b4530db8f5db2beeb44b42665999ab802799f/notify/notification_apns.go#L455-L469):
```go
func getApnsClient(cfg *config.ConfYaml, req *PushNotification) (client *apns2.Client) {
switch {
case req.Production:
client = ApnsClient.Production()
case req.Development:
client = ApnsClient.Development()
...
```
[apns2 v0.25.0 `client.go#L137-L146`](https://github.com/sideshow/apns2/blob/v0.25.0/client.go#L137-L146):
```go
func (c *Client) Development() *Client { c.Host = HostDevelopment; return c }
func (c *Client) Production() *Client { c.Host = HostProduction; return c }
```
`apns2.Client` has no mutex, and `ApnsClient` is a single process-wide instance. So each call writes `ApnsClient.Host` and returns the shared pointer. It is a plain data race, and it is unconditional: it happens even when every request agrees on the environment, because both callers still write the field.
It is not merely cosmetic because [`PushToIOS`](https://github.com/appleboy/gorush/blob/891b4530db8f5db2beeb44b42665999ab802799f/notify/notification_apns.go#L494) resolves the client once, then fans out into goroutines that each call [`client.PushWithContext`](https://github.com/appleboy/gorush/blob/891b4530db8f5db2beeb44b42665999ab802799f/notify/notification_apns.go#L505). apns2 reads `c.Host` when it builds each request, not when `getApnsClient` returned, so a concurrent mutation retroactively redirects pushes that were already in flight.
Concurrency here is the default, not an edge case: [`worker_num` falls back to `runtime.NumCPU()`](https://github.com/appleboy/gorush/blob/891b4530db8f5db2beeb44b42665999ab802799f/config/config.go#L363-L365) and [the queue pool runs that many workers](https://github.com/appleboy/gorush/blob/891b4530db8f5db2beeb44b42665999ab802799f/app/worker.go#L72-L78), so two notifications in one `POST /api/push` are enough to overlap.
### Reproduction
The aliasing is deterministic and needs no goroutines. Dropped in `notify/`, this passes on `891b453`:
```go
func TestO2Aliasing(t *testing.T) {
cfg, _ := config.LoadConf()
cfg.Ios.Enabled = true
cfg.Ios.KeyPath = testKeyPath
if err := InitAPNSClient(context.Background(), cfg); err != nil {
t.Fatal(err)
}
prodClient := getApnsClient(cfg, &PushNotification{Production: true})
gotAfterProd := prodClient.Host
devClient := getApnsClient(cfg, &PushNotification{Development: true})
t.Logf("same pointer for prod and dev request: %v", prodClient == devClient)
t.Logf("prod client Host right after its own call: %s", gotAfterProd)
t.Logf("prod client Host after a later dev call: %s", prodClient.Host)
}
```
```
same pointer for prod and dev request: true
prod client Host right after its own call: https://api.push.apple.com
prod client Host after a later dev call: https://api.sandbox.push.apple.com
```
The client handed to the production caller is silently repointed at the sandbox by a later, unrelated development call. Concurrently, the same write is a data race that `-race` names directly:
```
WARNING: DATA RACE
github.com/sideshow/apns2.(*Client).Production()
apns2@v0.25.0/client.go:144 +0x208
github.com/appleboy/gorush/notify.getApnsClient()
notify/notification_apns.go:458 +0x1ec
github.com/appleboy/gorush/notify.PushToIOS()
notify/notification_apns.go:494 +0x364
Previous write:
github.com/sideshow/apns2.(*Client).Development()
apns2@v0.25.0/client.go:138 +0x270
github.com/appleboy/gorush/notify.getApnsClient()
notify/notification_apns.go:460 +0x254
github.com/appleboy/gorush/notify.PushToIOS()
notify/notification_apns.go:494 +0x364
```
A count-based version that fans out two concurrent `PushToIOS` calls (one production, one development, 150 tokens each, behind a stub `RoundTripper`) shows the misdelivery, but the number varies run to run because it depends on scheduling: on the same machine I have seen anywhere from 0 to 150 of the 150 development tokens delivered to the production host. That variance is the point, the misdelivery is intermittent and load-dependent, which is exactly why it is hard to catch in production. The deterministic test and `-race` above are the reliable proof.
### Impact
Apple answers a token used against the wrong environment with a generic `BadDeviceToken`, so the operator sees ordinary token failures with nothing indicating the environment was swapped. Delivery silently fails, at a rate that depends on traffic mix, so it reads as intermittent.
### Suggested fix
`apns2.Client` is a plain four-field struct (`Host`, `Certificate`, `Token`, `HTTPClient`), and both `HTTPClient` and `Token` are safe to share, so a shallow copy per request avoids touching the global at all:
```go
func getApnsClient(cfg *config.ConfYaml, req *PushNotification) *apns2.Client {
client := *ApnsClient
switch {
case req.Production:
client.Host = apns2.HostProduction
case req.Development:
client.Host = apns2.HostDevelopment
default:
if cfg.Ios.Production {
client.Host = apns2.HostProduction
} else {
client.Host = apns2.HostDevelopment
}
}
return &client
}
```
I am happy to send a PR with this plus a regression test if you agree with the approach.
### Notes
`#874` and `#869` are open and both add a `sync.Mutex` to the append paths in `PushToIOS` and `handleNotification`. Neither changes `getApnsClient`, so this host mutation is independent and would remain after either merges. Running the suite with `-race` also surfaces the append race that `#874` already guards, so I have left that out of this report to avoid overlap.
On why CI has not caught this: `make test` and the [testing workflow](https://github.com/appleboy/gorush/blob/891b4530db8f5db2beeb44b42665999ab802799f/.github/workflows/testing.yml) do not pass `-race`, and no test issues two concurrent `PushToIOS` calls with differing environment flags. `go vet` is silent, since it does not model aliasing through a method that returns its own receiver.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in notify/notification_apns.go at getApnsClient and PushToIOS, then inspect the existing notify tests and the InitAPNSClient setup. Add regression coverage showing concurrent production and development requests keep their environments separate, and run the notify tests with the race detector; done means no shared-host race or cross-environment delivery.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- api, backend, testing
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100