rastrillo / aviso Public

Store subscription keys canonically; real P-256 fixtures in the node tests

Codex, second pass on the handlers: Go's base64 decoder forgives a
stray CR/LF but webpush-go computes padding from the string's length,
so a key with a trailing newline validated here and then failed every
send — and could still overwrite working keys. Whitespace is refused
and what is stored is the canonical re-encoding, which is also what
Send hands the encryptor.

The node fixtures were byte patterns no browser would accept as an
applicationServerKey; they are real prime256v1 points now, passed as
ArrayBuffers the way the Push API returns them. The prompt test checks
the prompt fired before enable()'s first await resolves, which is the
property that keeps it inside the user gesture.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Paul Campbell pushed by paul@keymail.dev 1 week ago d0b4a4434bd6086d0da7bda9573eec94325bc8d1 parent 7e8d507

3 files changed, +52 −18

http.go +20 −11
http_test.go +12 −0
js/push.test.mjs +20 −7

diff --git a/http.go b/http.go

index d02f4b8..60be1f2 100644

--- a/http.go

+++ b/http.go

@@ -7,6 +7,7 @@ import (

"errors"

"io"

"net/http"

+ "strings"

"amadan.net/rastrillo/rastrillo/csrf"

"amadan.net/rastrillo/rastrillo/sessions"

@@ -83,12 +84,19 @@ func decodeBody(w http.ResponseWriter, r *http.Request, into any) bool {

return true

}

-// validateKeys checks the two RFC 8291 inputs the way webpush-go will

-// before it encrypts: p256dh is a base64url uncompressed P-256 point,

-// auth is 16 base64url bytes. Refusing them here keeps a bad

-// re-subscribe from replacing working keys or deleting previousEndpoint.

-func validateKeys(p256dh, auth string) error {

+// canonicalKeys checks the two RFC 8291 inputs the way webpush-go will

+// before it encrypts — p256dh an uncompressed P-256 point, auth 16

+// bytes, both base64url with or without padding — and returns them

+// re-encoded canonically. Stored canonical, not as sent: Go's decoder

+// forgives a stray CR/LF that webpush-go's padding arithmetic does

+// not, so a key that "validated" verbatim could still fail every

+// send. Refusing bad keys here keeps a mangled re-subscribe from

+// replacing working keys or deleting previousEndpoint.

+func canonicalKeys(p256dh, auth string) (string, string, error) {

decode := func(s string) ([]byte, error) {

+ if strings.ContainsAny(s, "\r\n \t") {

+ return nil, errors.New("whitespace")

+ }

if b, err := base64.RawURLEncoding.DecodeString(s); err == nil {

return b, nil

}

@@ -96,16 +104,16 @@ func validateKeys(p256dh, auth string) error {

}

point, err := decode(p256dh)

if err != nil {

- return errors.New("p256dh is not base64url")

+ return "", "", errors.New("p256dh is not base64url")

}

if _, err := ecdh.P256().NewPublicKey(point); err != nil {

- return errors.New("p256dh is not a P-256 point")

+ return "", "", errors.New("p256dh is not a P-256 point")

}

secret, err := decode(auth)

if err != nil || len(secret) != 16 {

- return errors.New("auth is not 16 base64url bytes")

+ return "", "", errors.New("auth is not 16 base64url bytes")

}

- return nil

+ return base64.RawURLEncoding.EncodeToString(point), base64.RawURLEncoding.EncodeToString(secret), nil

}

type subscribeRequest struct {

@@ -140,7 +148,8 @@ func (s *Service) Subscribe(w http.ResponseWriter, r *http.Request) {

http.Error(w, "endpoint refused", http.StatusBadRequest)

return

}

- if err := validateKeys(req.Subscription.Keys.P256dh, req.Subscription.Keys.Auth); err != nil {

+ p256dh, auth, err := canonicalKeys(req.Subscription.Keys.P256dh, req.Subscription.Keys.Auth)

+ if err != nil {

http.Error(w, "subscription keys refused: "+err.Error(), http.StatusBadRequest)

return

}

@@ -148,7 +157,7 @@ func (s *Service) Subscribe(w http.ResponseWriter, r *http.Request) {

http.Error(w, "previousEndpoint refused", http.StatusBadRequest)

return

}

- sub := Subscription{Endpoint: req.Subscription.Endpoint, P256dh: req.Subscription.Keys.P256dh, Auth: req.Subscription.Keys.Auth}

+ sub := Subscription{Endpoint: req.Subscription.Endpoint, P256dh: p256dh, Auth: auth}

switch err := s.put(r.Context(), subject, sub, req.PreviousEndpoint); {

case errors.Is(err, ErrOwnedElsewhere):

http.Error(w, "endpoint enrolled by another account", http.StatusConflict)

diff --git a/http_test.go b/http_test.go

index 601913b..4087533 100644

--- a/http_test.go

+++ b/http_test.go

@@ -157,6 +157,10 @@ func TestSubscribeRefusesInvalidKeysWithoutTouchingRows(t *testing.T) {

"p256dh compressed": {"p256dh": good["p256dh"][:44], "auth": good["auth"]},

"auth short": {"p256dh": good["p256dh"], "auth": base64.RawURLEncoding.EncodeToString(make([]byte, 15))},

"auth not base64": {"p256dh": good["p256dh"], "auth": "!"},

+ // Go's decoder forgives these; webpush-go's padding arithmetic

+ // does not, so they would be stored and then fail every send.

+ "p256dh newline": {"p256dh": good["p256dh"] + "\n", "auth": good["auth"]},

+ "auth crlf": {"p256dh": good["p256dh"], "auth": good["auth"][:5] + "\r\n" + good["auth"][5:]},

} {

b, _ := json.Marshal(map[string]any{

"subscription": map[string]any{"endpoint": "https://push.example/e1", "keys": k},

@@ -183,6 +187,14 @@ func TestSubscribeRefusesInvalidKeysWithoutTouchingRows(t *testing.T) {

if w := post(t, s.Subscribe, string(b), "alice", true); w.Code != http.StatusNoContent {

t.Fatalf("padded keys: %d %s", w.Code, w.Body)

}

+ // ...and stored canonically, so what Send hands webpush-go is the

+ // unpadded form it decodes.

+ rows, _ := s.List(ctx, "alice")

+ for _, r := range rows {

+ if r.Endpoint == "https://push.example/e2" && (r.P256dh != good["p256dh"] || r.Auth != good["auth"]) {

+ t.Fatalf("keys not stored canonically: %q %q", r.P256dh, r.Auth)

+ }

}

func mustDecode(s string) []byte {

diff --git a/js/push.test.mjs b/js/push.test.mjs

index 38e9ea2..faf8a22 100644

--- a/js/push.test.mjs

+++ b/js/push.test.mjs

@@ -1,17 +1,26 @@

import { test } from "node:test";

import assert from "node:assert/strict";

+import { generateKeyPairSync } from "node:crypto";

import { capabilities, status, enable, reconcile, disable } from "./push.mjs";

-// A real 65-byte uncompressed point (0x04 + 64 bytes), base64url'd

-// from bytes so sameKey's round trip is exact.

-const KEY_BYTES = Uint8Array.from({ length: 65 }, (_, i) => (i === 0 ? 4 : (i * 37) & 0xff));

+// Real P-256 points, as a browser would require of applicationServerKey

+// (an invalid point is an InvalidAccessError there, not a byte

+// mismatch). The browser hands the key back as an ArrayBuffer, so the

+// fakes do too.

+function realPoint() {

+ const { publicKey } = generateKeyPairSync("ec", { namedCurve: "prime256v1" });

+ const jwk = publicKey.export({ format: "jwk" });

+ return Uint8Array.from([4, ...Buffer.from(jwk.x, "base64url"), ...Buffer.from(jwk.y, "base64url")]);

+const KEY_BYTES = realPoint();

const KEY = Buffer.from(KEY_BYTES).toString("base64url");

-const OLD_KEY_BYTES = Uint8Array.from({ length: 65 }, (_, i) => (i === 0 ? 4 : (i * 11) & 0xff));

+const OLD_KEY_BYTES = realPoint();

+const asBuffer = (u8) => u8.buffer.slice(u8.byteOffset, u8.byteOffset + u8.byteLength);

function fakeSub(endpoint, keyBytes) {

return {

endpoint,

- options: { applicationServerKey: keyBytes },

+ options: { applicationServerKey: asBuffer(keyBytes) },

unsubscribed: false,

toJSON() { return { endpoint, expirationTime: null, keys: { p256dh: "P", auth: "A" } }; },

async unsubscribe() { this.unsubscribed = true; return true; },

@@ -60,11 +69,15 @@ test("capabilities reports the four booleans", () => {

assert.deepEqual(capabilities({}), { serviceWorker: false, push: false, notifications: false, standalone: false });

});

-test("enable prompts, subscribes and saves a projected body", async () => {

+test("enable prompts synchronously inside the gesture, subscribes and saves a projected body", async () => {

const e = env("default", "granted");

const reg = fakeRegistration(null);

const { saved, save } = okSave();

- const sub = await enable({ registration: reg, publicKey: KEY, save }, e);

+ const pending = enable({ registration: reg, publicKey: KEY, save }, e);

+ // Before any await resolves: a prompt after an await is outside the

+ // user gesture and browsers deny it.

+ assert.deepEqual(e.calls, ["prompt"]);

+ const sub = await pending;

assert.ok(sub);

assert.deepEqual(e.calls, ["prompt"]);

assert.equal(saved.length, 1);