amadan

rastrillo / aviso Public

Add VAPID key parsing and generation

The public key is derived from the scalar rather than stored beside
it, so an app provisions exactly one secret; the key id is on every
row so a rotated key shows up per subscription instead of as a silent
signing failure. Padded input is refused outright: two spellings of
one key would be two things an operator could paste, and only one is
what aviso-key printed.

Also records Codex's eleven plan-review findings as amendments at the
top of the plan; each is applied in the task it names.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 0f9b4ea3fcafdf5e401201af5bf22c99504238bb parent acb1fef
3 files changed, +186 −0
  • docs/superpowers/plans/2026-09-07-aviso.md +55 −0
  • vapid.go +60 −0
  • vapid_test.go +71 −0
diff --git a/docs/superpowers/plans/2026-09-07-aviso.md b/docs/superpowers/plans/2026-09-07-aviso.md
index da6c6bf..32d7e56 100644
--- a/docs/superpowers/plans/2026-09-07-aviso.md
+++ b/docs/superpowers/plans/2026-09-07-aviso.md
@@ -36,6 +36,61 @@ Address every finding or record in the branch discussion why not. A task is done
---
+## Review amendments (Codex, 2026-09-07, before Task 2)
+
+Codex reviewed this plan against the spec, rastrillo's source and
+webpush-go v1.4.0. Eleven findings; every one is applied to the task
+it names, and the task text below is superseded where they conflict.
+
+1. **Task 6, Send.** The sketch raced (`countSends` read results the
+ goroutines were writing) and leaked goroutines on cancellation.
+ Send now decides up front which rows it will attempt, uses a
+ `sync.WaitGroup`, marks rows it never started with `ctx.Err()` when
+ cancelled while waiting for a slot, always waits for started
+ goroutines, and returns `ctx.Err()` as the batch error whenever the
+ context was cancelled at any point.
+2. **Task 5, guard.** `guardedIP` admitted `0.0.0.0/8`, `192.0.0.0/24`,
+ `192.0.2.0/24`, `198.18.0.0/15`, `198.51.100.0/24`, `203.0.113.0/24`,
+ `240.0.0.0/4`, `2001:db8::/32`, `64:ff9b::/96`. It now carries an
+ explicit reserved-CIDR list on top of the net.IP predicates, and the
+ test corpus names each range.
+3. **Task 6.** `sendOne` calls `validateEndpoint` before sending: a
+ caller can hand `Send` a `Stored` it edited.
+4. **Task 6.** webpush-go prefixes `mailto:` to any subscriber not
+ starting with `https:` (`vapid.go:74`). `New` keeps `Contact` as
+ given for validation and stores the mailto-stripped form for the
+ wire; the header test decodes the JWT and checks `sub`.
+5. **Task 4/6 tests.** `sub()` generates a real P-256 point for
+ `P256dh` and 16 random bytes for `Auth`; webpush-go decodes both
+ before any HTTP happens.
+6. **Task 7/9/11.** Browser `PushSubscription.toJSON()` carries
+ `expirationTime`. The handler no longer uses `DisallowUnknownFields`
+ (the byte cap and field validation are the defence), and every
+ client path projects to `{endpoint, keys}` anyway.
+7. **Task 8 test.** `KEY` is a real 65-byte point, base64url'd in the
+ test from bytes, so `sameKey` round-trips.
+8. **Task 6, TTL.** webpush-go always sends the `TTL` header and `0`
+ means "deliver now or drop", not "service default". Ruling:
+ `Options.TTL == 0` means 24 hours. Recorded as a spec deviation in
+ the branch discussion.
+9. **Task 8/9, callbacks.** `save`/`remove` may resolve to nothing (a
+ callback that validated its own response); only an explicit
+ `{ok: false}`-shaped return, or a rejection, counts as failure.
+10. **Task 9/11/12, worker key.** A worker forgets a module-level
+ variable when it is terminated. `handleSubscriptionChange` takes
+ `{publicKey, save}` where `publicKey()` fetches the key on demand;
+ the helper performs the renewal subscribe itself (from
+ `event.newSubscription`, else `pushManager.subscribe` with the
+ fetched key) and builds the full subscribe body, so the app's
+ `save` only posts.
+11. **Task 13/1, CI.** A `browser` make target
+ (`go test -tags browser -count=1 -run Browser .`) joins `ci` and
+ gets `.amadan/ci.d/50-browser`; it fails, not skips, without
+ Chromium — rastrillo's own rule (`RASTRILLO_BROWSER_OPTIONAL`
+ unset).
+
+---
+
## File structure
| File | Responsibility |
diff --git a/vapid.go b/vapid.go
new file mode 100644
index 0000000..9326733
--- /dev/null
+++ b/vapid.go
@@ -0,0 +1,60 @@
+package aviso
+
+import (
+ "crypto/ecdh"
+ "crypto/rand"
+ "crypto/sha256"
+ "encoding/base64"
+ "errors"
+ "strings"
+)
+
+// ErrEmptyPrivateKey means Config.PrivateKey was empty. It is refused
+// rather than minted: a key generated at boot into local state is a
+// key lost at the next restore, and every browser then holds a
+// subscription nobody can sign for.
+var ErrEmptyPrivateKey = errors.New("aviso: Config.PrivateKey must not be empty; mint one with `go run amadan.net/rastrillo/aviso/cmd/aviso-key`")
+
+// ErrInvalidPrivateKey means Config.PrivateKey is not an unpadded
+// base64url 32-byte P-256 scalar in range.
+var ErrInvalidPrivateKey = errors.New("aviso: Config.PrivateKey is not an unpadded base64url 32-byte P-256 scalar")
+
+// GenerateKey mints a VAPID private key in Config.PrivateKey's format.
+// The public half is derived, never stored: one secret to provision.
+func GenerateKey() (string, error) {
+ k, err := ecdh.P256().GenerateKey(rand.Reader)
+ if err != nil {
+ return "", err
+ }
+ return base64.RawURLEncoding.EncodeToString(k.Bytes()), nil
+}
+
+// parsePrivateKey validates the scalar and derives the two things the
+// rest of the package needs from it: the uncompressed public point
+// (applicationServerKey on the browser side, the VAPID public key on
+// the wire) and a key id — SHA-256 of that point — stored on every
+// row so a rotated key is visible per subscription.
+func parsePrivateKey(s string) (pub, keyID string, err error) {
+ if s == "" {
+ return "", "", ErrEmptyPrivateKey
+ }
+ // Padding is refused, not tolerated: two encodings of one key
+ // would be two strings an operator could paste, and only one of
+ // them is what aviso-key printed.
+ if strings.ContainsAny(s, "=+/") {
+ return "", "", ErrInvalidPrivateKey
+ }
+ raw, err := base64.RawURLEncoding.DecodeString(s)
+ if err != nil || len(raw) != 32 {
+ return "", "", ErrInvalidPrivateKey
+ }
+ // NewPrivateKey rejects zero and out-of-range scalars.
+ k, err := ecdh.P256().NewPrivateKey(raw)
+ if err != nil {
+ return "", "", ErrInvalidPrivateKey
+ }
+ point := k.PublicKey().Bytes()
+ sum := sha256.Sum256(point)
+ return base64.RawURLEncoding.EncodeToString(point),
+ base64.RawURLEncoding.EncodeToString(sum[:]), nil
+}
diff --git a/vapid_test.go b/vapid_test.go
new file mode 100644
index 0000000..59af685
--- /dev/null
+++ b/vapid_test.go
@@ -0,0 +1,71 @@
+package aviso
+
+import (
+ "crypto/ecdh"
+ "crypto/rand"
+ "encoding/base64"
+ "errors"
+ "strings"
+ "testing"
+)
+
+func TestGenerateKeyRoundTrips(t *testing.T) {
+ priv, err := GenerateKey()
+ if err != nil {
+ t.Fatal(err)
+ }
+ raw, err := base64.RawURLEncoding.DecodeString(priv)
+ if err != nil || len(raw) != 32 {
+ t.Fatalf("private key = %q: want 32 unpadded base64url bytes (err %v)", priv, err)
+ }
+ pub, id, err := parsePrivateKey(priv)
+ if err != nil {
+ t.Fatal(err)
+ }
+ pubRaw, err := base64.RawURLEncoding.DecodeString(pub)
+ if err != nil || len(pubRaw) != 65 || pubRaw[0] != 0x04 {
+ t.Fatalf("public key = %q: want 65-byte uncompressed point", pub)
+ }
+ if id == "" || strings.ContainsAny(id, "+/=") {
+ t.Fatalf("key id = %q: want unpadded base64url", id)
+ }
+ // The same key must yield the same id after a restart — Sweep and
+ // Send match rows on it.
+ _, id2, _ := parsePrivateKey(priv)
+ if id != id2 {
+ t.Fatal("key id not deterministic")
+ }
+}
+
+func TestParsePrivateKeyRefusesBadInput(t *testing.T) {
+ if _, _, err := parsePrivateKey(""); !errors.Is(err, ErrEmptyPrivateKey) {
+ t.Fatalf("empty: got %v, want ErrEmptyPrivateKey", err)
+ }
+ zero := base64.RawURLEncoding.EncodeToString(make([]byte, 32))
+ for name, in := range map[string]string{
+ "not base64": "!!!",
+ "short": base64.RawURLEncoding.EncodeToString([]byte("short")),
+ "zero scalar": zero,
+ "padded": zero + "=",
+ } {
+ if _, _, err := parsePrivateKey(in); !errors.Is(err, ErrInvalidPrivateKey) {
+ t.Errorf("%s: got %v, want ErrInvalidPrivateKey", name, err)
+ }
+ }
+}
+
+func TestParsePrivateKeyAgreesWithECDH(t *testing.T) {
+ k, err := ecdh.P256().GenerateKey(rand.Reader)
+ if err != nil {
+ t.Fatal(err)
+ }
+ priv := base64.RawURLEncoding.EncodeToString(k.Bytes())
+ pub, _, err := parsePrivateKey(priv)
+ if err != nil {
+ t.Fatal(err)
+ }
+ want := base64.RawURLEncoding.EncodeToString(k.PublicKey().Bytes())
+ if pub != want {
+ t.Fatalf("public key = %s, want %s", pub, want)
+ }
+}