Messaging: deliver events to companion app via push notifications

[andig]

refs evcc-io/app#16, pairs with evcc-io/app#231, replaces #31906

Lets the companion app receive configured messaging events as real system push notifications — working also when the app is closed, addressing the feedback on #31906. The app registers its device push token with the evcc instance; messages are delivered through the Expo push service (exp.host), which fronts APNs/FCM with the app's credentials. No new evcc cloud service is required; a relay under sponsor.evcc.io remains a future option.

• new AppPush messenger: holds registered device tokens (persisted in settings), sends via Expo push API, prunes tokens Expo reports as DeviceNotRegistered
• registered implicitly with the messaging hub — users only configure messaging events, no service entry needed
• new endpoints POST/DELETE /api/push/token for the app to register/unregister; token format validated, capped at 20 devices
• endpoint is unauthenticated like the other site APIs (LAN trust model) — happy to move it behind auth if preferred

🤖 Generated with Claude Code

[sourcery-ai]

Hey - I've found 3 issues, and left some high level feedback:

• The new /api/push/token endpoints are intentionally unauthenticated but are exposed with a permissive CORS handler; consider tightening CORS (e.g., restricting AllowedOrigins to the known app origin or reusing existing API router middleware) to avoid making this LAN-only assumption accidentally reachable from arbitrary web origins.
• AppPush.Register/Unregister always persist via settings.SetJson, and the tests construct AppPush directly without stubbing persistence; it may be cleaner to inject a storage abstraction or add a no-op/test storage path so unit tests and potential future uses don’t unintentionally write to the global settings backend.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new /api/push/token endpoints are intentionally unauthenticated but are exposed with a permissive CORS handler; consider tightening CORS (e.g., restricting AllowedOrigins to the known app origin or reusing existing API router middleware) to avoid making this LAN-only assumption accidentally reachable from arbitrary web origins.
- AppPush.Register/Unregister always persist via settings.SetJson, and the tests construct AppPush directly without stubbing persistence; it may be cleaner to inject a storage abstraction or add a no-op/test storage path so unit tests and potential future uses don’t unintentionally write to the global settings backend.

## Individual Comments

### Comment 1
<location path="server/http_apppush.go" line_range="13" />
<code_context>
+)
+
+// RegisterAppPushHandlers adds the companion app push token endpoints
+func (s *HTTPd) RegisterAppPushHandlers(m *messenger.AppPush) {
+	api := s.Router().PathPrefix("/api").Subrouter()
+	api.Use(jsonHandler)
</code_context>
<issue_to_address>
**🚨 question (security):** Consider how these push-token endpoints are authenticated and exposed via CORS

These token registration endpoints are under `/api` and use `handlers.CORS` with default (unrestricted) origins, and there’s no explicit auth visible here. Please confirm that:

- Auth/CSRF is enforced by `jsonHandler` or middleware above this router, and
- CORS is limited to trusted origins (e.g. the companion app), if required.

If not, arbitrary origins could register/unregister push tokens and potentially receive notifications or flood the token list.
</issue_to_address>

### Comment 2
<location path="messenger/app.go" line_range="48-57" />
<code_context>
+}
+
+// Register adds a device token
+func (m *AppPush) Register(token string) {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+
+	if slices.Contains(m.tokens, token) {
+		return
+	}
+
+	// drop oldest when full
+	if len(m.tokens) >= maxTokens {
+		m.tokens = m.tokens[len(m.tokens)-maxTokens+1:]
+	}
+
+	m.tokens = append(m.tokens, token)
+	m.persist()
+}
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Optionally validate tokens inside Register/Unregister to harden against misuse

Currently only the HTTP handler validates tokens via `ValidPushToken`, while `Register`/`Unregister` accept any string. This is fine if they’re only ever called through HTTP, but makes misuse likely from future callers (CLI, RPC, internal uses). Consider calling `ValidPushToken` inside `Register` (and `Unregister`) or at least documenting that callers must pass a validated token, so the token store can’t be polluted by invalid values if new entry points bypass the handler.

Suggested implementation:

```golang
 // Register adds a device token. Invalid tokens are ignored.
func (m *AppPush) Register(token string) {
	if !ValidPushToken(token) {
		return
	}

	m.mu.Lock()
	defer m.mu.Unlock()

	if slices.Contains(m.tokens, token) {
		return
	}

	// drop oldest when full
	if len(m.tokens) >= maxTokens {
		m.tokens = m.tokens[len(m.tokens)-maxTokens+1:]
	}

	m.tokens = append(m.tokens, token)
	m.persist()
}

```

1. Apply the same validation in `Unregister` (if present in this file), for example by adding at the top of the function:
   ```go
   if !ValidPushToken(token) {
   	return
   }
   ```
   This ensures invalid tokens are never stored or operated on, regardless of the entry point (HTTP, CLI, RPC, etc.).
2. Optionally extend the `Unregister` comment to mirror `Register`, e.g. “Invalid tokens are ignored.” to document the behavior.
</issue_to_address>

### Comment 3
<location path="messenger/app_test.go" line_range="10-15" />
<code_context>
+	"github.com/stretchr/testify/assert"
+)
+
+func TestValidPushToken(t *testing.T) {
+	assert.True(t, ValidPushToken("ExponentPushToken[xxxxxxxxxxxxxxxxxxxxxx]"))
+	assert.False(t, ValidPushToken(""))
+	assert.False(t, ValidPushToken("foo"))
+	assert.False(t, ValidPushToken("ExponentPushToken[unterminated"))
+	assert.False(t, ValidPushToken("ExponentPushToken["+string(make([]byte, maxTokenLen))+"]"))
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen ValidPushToken tests with clear boundary cases and a table-driven structure

Please add explicit boundary tests for a token of length exactly `maxTokenLen` (valid) and `maxTokenLen+1` (invalid), and consider converting this into a table-driven test (e.g., `{name, token, wantValid}`) using clearer constructions like `strings.Repeat("x", n)`. That will make the length and prefix/suffix cases easier to read and extend, and avoid opaque patterns like `string(make([]byte, maxTokenLen))`.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[andig]

@naltatis no good idea?

[naltatis]

Yes it is, but it's a complex topic that requires time, attention and extensive testing.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• In RegisterAppPushHandlers a new /api subrouter with its own middleware stack is created instead of reusing the existing API router; consider registering these routes on the same router/middlewares used for other API endpoints to avoid diverging behavior (CORS, compression, auth, etc.).
• pushTokenHandler and AppPush.Register both validate the token format; you could simplify the flow by having only the HTTP handler enforce ValidPushToken and letting Register assume valid input (or make Register return an error/boolean) to avoid double validation.
• NewAppPushFromSettings ignores errors from settings.Json(keys.PushTokens, &m.tokens); consider logging or handling this error so that corrupt or unreadable stored tokens don’t fail silently.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `RegisterAppPushHandlers` a new `/api` subrouter with its own middleware stack is created instead of reusing the existing API router; consider registering these routes on the same router/middlewares used for other API endpoints to avoid diverging behavior (CORS, compression, auth, etc.).
- `pushTokenHandler` and `AppPush.Register` both validate the token format; you could simplify the flow by having only the HTTP handler enforce `ValidPushToken` and letting `Register` assume valid input (or make `Register` return an error/boolean) to avoid double validation.
- `NewAppPushFromSettings` ignores errors from `settings.Json(keys.PushTokens, &m.tokens)`; consider logging or handling this error so that corrupt or unreadable stored tokens don’t fail silently.

## Individual Comments

### Comment 1
<location path="messenger/app_test.go" line_range="37-46" />
<code_context>
+	}
+}
+
+func TestAppPushRegister(t *testing.T) {
+	m := &AppPush{log: util.NewLogger("test")}
+
+	m.Register("ExponentPushToken[a]")
+	m.Register("ExponentPushToken[a]") // duplicate
+	m.Register("ExponentPushToken[b]")
+	m.Register("invalid")
+	assert.Equal(t, []string{"ExponentPushToken[a]", "ExponentPushToken[b]"}, m.tokens)
+
+	m.Unregister("ExponentPushToken[a]")
+	assert.Equal(t, []string{"ExponentPushToken[b]"}, m.tokens)
+
+	m.Unregister("ExponentPushToken[unknown]")
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for max token capacity and pruning behavior in AppPush.Register

The test currently covers register/unregister and duplicates but not the `maxTokens` capacity behavior. Please add a test that:
- initializes `m.tokens` with `maxTokens` valid entries,
- registers one additional valid token,
- verifies the slice length remains `maxTokens` and the oldest token was removed.
This will validate the pruning logic and help prevent regressions in capacity handling.

Suggested implementation:

```golang
func TestAppPushRegister(t *testing.T) {
	m := &AppPush{log: util.NewLogger("test")}

	m.Register("ExponentPushToken[a]")
	m.Register("ExponentPushToken[a]") // duplicate
	m.Register("ExponentPushToken[b]")
	m.Register("invalid")
	assert.Equal(t, []string{"ExponentPushToken[a]", "ExponentPushToken[b]"}, m.tokens)

	m.Unregister("ExponentPushToken[a]")
	assert.Equal(t, []string{"ExponentPushToken[b]"}, m.tokens)

	m.Unregister("ExponentPushToken[unknown]")
	assert.Equal(t, []string{"ExponentPushToken[b]"}, m.tokens)
}

func TestAppPushRegisterMaxTokensPrunesOldest(t *testing.T) {
	m := &AppPush{log: util.NewLogger("test")}

	// Fill up to maxTokens with valid, distinct tokens
	for i := 0; i < maxTokens; i++ {
		token := fmt.Sprintf("ExponentPushToken[%d]", i)
		m.Register(token)
	}

	// Sanity check: we should be at capacity
	assert.Len(t, m.tokens, maxTokens)
	assert.Equal(t, "ExponentPushToken[0]", m.tokens[0])

	// Register one more valid token; this should prune the oldest
	newToken := "ExponentPushToken[new]"
	m.Register(newToken)

	// Capacity must remain maxTokens
	assert.Len(t, m.tokens, maxTokens)

	// Oldest token should have been removed
	assert.NotContains(t, m.tokens, "ExponentPushToken[0]")

	// New token should be present
	assert.Contains(t, m.tokens, newToken)
}

```

If `fmt` is not already imported in `messenger/app_test.go`, add it to the import list:

<<<<<<< SEARCH
import (
	"testing"

	"github.com/stretchr/testify/assert"
	"example.com/project/util"
)
=======
import (
	"fmt"
	"testing"

	"github.com/stretchr/testify/assert"
	"example.com/project/util"
)
>>>>>>> REPLACE

Also ensure that `maxTokens` is accessible in this test file (exported or in the same package). If it is not, expose it or add a helper in the test file that mirrors the production value.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Add tests for max token capacity and pruning behavior in AppPush.Register

The test currently covers register/unregister and duplicates but not the maxTokens capacity behavior. Please add a test that:

• initializes m.tokens with maxTokens valid entries,
• registers one additional valid token,
• verifies the slice length remains maxTokens and the oldest token was removed.
  This will validate the pruning logic and help prevent regressions in capacity handling.

Suggested implementation:

func TestAppPushRegister(t *testing.T) {
	m := &AppPush{log: util.NewLogger("test")}

	m.Register("ExponentPushToken[a]")
	m.Register("ExponentPushToken[a]") // duplicate
	m.Register("ExponentPushToken[b]")
	m.Register("invalid")
	assert.Equal(t, []string{"ExponentPushToken[a]", "ExponentPushToken[b]"}, m.tokens)

	m.Unregister("ExponentPushToken[a]")
	assert.Equal(t, []string{"ExponentPushToken[b]"}, m.tokens)

	m.Unregister("ExponentPushToken[unknown]")
	assert.Equal(t, []string{"ExponentPushToken[b]"}, m.tokens)
}

func TestAppPushRegisterMaxTokensPrunesOldest(t *testing.T) {
	m := &AppPush{log: util.NewLogger("test")}

	// Fill up to maxTokens with valid, distinct tokens
	for i := 0; i < maxTokens; i++ {
		token := fmt.Sprintf("ExponentPushToken[%d]", i)
		m.Register(token)
	}

	// Sanity check: we should be at capacity
	assert.Len(t, m.tokens, maxTokens)
	assert.Equal(t, "ExponentPushToken[0]", m.tokens[0])

	// Register one more valid token; this should prune the oldest
	newToken := "ExponentPushToken[new]"
	m.Register(newToken)

	// Capacity must remain maxTokens
	assert.Len(t, m.tokens, maxTokens)

	// Oldest token should have been removed
	assert.NotContains(t, m.tokens, "ExponentPushToken[0]")

	// New token should be present
	assert.Contains(t, m.tokens, newToken)
}

If fmt is not already imported in messenger/app_test.go, add it to the import list:

<<<<<<< SEARCH
import (
"testing"

"github.com/stretchr/testify/assert"
"example.com/project/util"

)

import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
"example.com/project/util"

)

REPLACE

Also ensure that maxTokens is accessible in this test file (exported or in the same package). If it is not, expose it or add a helper in the test file that mirrors the production value.