amadan

rastrillo / aviso Public

Handlers: validate the subscription keys and refuse trailing JSON

Codex's review of the handlers: any non-empty p256dh and auth were
stored, so a mangled re-subscribe could replace working keys and
delete previousEndpoint before webpush-go ever refused them. Both are
now checked the way the encryptor will — a P-256 point, 16 secret
bytes — before put runs, and a refused request provably leaves rows
untouched. The decoder also required only one JSON value and dropped
the rest, so a body with trailing garbage half-applied; a second
Decode must now hit EOF. Boundary tests for the 8 KiB body and
2048-byte endpoint caps, and the cross-owner previousEndpoint case at
the handler level.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev e71b6bb6800a4f7c0377d7b6b3974a76b8e90ab7 parent d7684ea
2 files changed, +137 −6
  • http.go +44 −6
  • http_test.go +93 −0
diff --git a/http.go b/http.go
index d432ec1..d02f4b8 100644
--- a/http.go
+++ b/http.go
@@ -1,6 +1,8 @@
package aviso
import (
+ "crypto/ecdh"
+ "encoding/base64"
"encoding/json"
"errors"
"io"
@@ -54,22 +56,58 @@ func (s *Service) gate(w http.ResponseWriter, r *http.Request) string {
// subscription. The byte cap and field validation are the defence.
func decodeBody(w http.ResponseWriter, r *http.Request, into any) bool {
body := http.MaxBytesReader(w, r.Body, maxSubscribeBody)
- if err := json.NewDecoder(body).Decode(into); err != nil {
+ dec := json.NewDecoder(body)
+ tooLarge := func(err error) bool {
var mbe *http.MaxBytesError
- if errors.As(err, &mbe) {
+ return errors.As(err, &mbe)
+ }
+ if err := dec.Decode(into); err != nil {
+ if tooLarge(err) {
http.Error(w, "body too large", http.StatusRequestEntityTooLarge)
return false
}
http.Error(w, "bad request body", http.StatusBadRequest)
return false
}
- if _, err := io.Copy(io.Discard, body); err != nil { // trailing bytes over the cap
- http.Error(w, "body too large", http.StatusRequestEntityTooLarge)
+ // Exactly one JSON value: a second Decode must hit EOF. Anything
+ // else — trailing garbage, a second document — is refused rather
+ // than silently dropped, so a mangled body cannot half-apply.
+ if err := dec.Decode(new(json.RawMessage)); !errors.Is(err, io.EOF) {
+ if tooLarge(err) {
+ http.Error(w, "body too large", http.StatusRequestEntityTooLarge)
+ return false
+ }
+ http.Error(w, "bad request body", http.StatusBadRequest)
return false
}
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 {
+ decode := func(s string) ([]byte, error) {
+ if b, err := base64.RawURLEncoding.DecodeString(s); err == nil {
+ return b, nil
+ }
+ return base64.URLEncoding.DecodeString(s)
+ }
+ point, err := decode(p256dh)
+ if err != nil {
+ 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")
+ }
+ secret, err := decode(auth)
+ if err != nil || len(secret) != 16 {
+ return errors.New("auth is not 16 base64url bytes")
+ }
+ return nil
+}
+
type subscribeRequest struct {
Subscription struct {
Endpoint string `json:"endpoint"`
@@ -102,8 +140,8 @@ func (s *Service) Subscribe(w http.ResponseWriter, r *http.Request) {
http.Error(w, "endpoint refused", http.StatusBadRequest)
return
}
- if req.Subscription.Keys.P256dh == "" || req.Subscription.Keys.Auth == "" {
- http.Error(w, "subscription keys missing", http.StatusBadRequest)
+ if err := validateKeys(req.Subscription.Keys.P256dh, req.Subscription.Keys.Auth); err != nil {
+ http.Error(w, "subscription keys refused: "+err.Error(), http.StatusBadRequest)
return
}
if req.PreviousEndpoint != "" && validateEndpoint(req.PreviousEndpoint) != nil {
diff --git a/http_test.go b/http_test.go
index bdb0fab..601913b 100644
--- a/http_test.go
+++ b/http_test.go
@@ -108,10 +108,32 @@ func TestSubscribeGating(t *testing.T) {
if w := post(t, s.Subscribe, "{not json", "alice", true); w.Code != http.StatusBadRequest {
t.Errorf("bad json: %d", w.Code)
}
+ for name, tail := range map[string]string{"garbage": "garbage", "second doc": `{"x":1}`, "stray brace": "}"} {
+ if w := post(t, s.Subscribe, body+tail, "alice", true); w.Code != http.StatusBadRequest {
+ t.Errorf("trailing %s: %d", name, w.Code)
+ }
+ }
huge := `{"subscription":{"endpoint":"https://push.example/` + strings.Repeat("x", 8193) + `"}}`
if w := post(t, s.Subscribe, huge, "alice", true); w.Code != http.StatusRequestEntityTooLarge {
t.Errorf("oversize body: %d", w.Code)
}
+ // A valid body padded with whitespace to exactly the cap passes;
+ // one byte over does not.
+ exact := body + strings.Repeat(" ", 8192-len(body))
+ if w := post(t, s.Subscribe, exact, "alice", true); w.Code != http.StatusNoContent {
+ t.Errorf("8192-byte body: %d", w.Code)
+ }
+ if w := post(t, s.Subscribe, exact+" ", "alice", true); w.Code != http.StatusRequestEntityTooLarge {
+ t.Errorf("8193-byte body: %d", w.Code)
+ }
+ // Endpoint length: 2048 accepted, 2049 refused.
+ prefix := "https://push.example/"
+ if w := post(t, s.Subscribe, subscribeBody(s, prefix+strings.Repeat("e", 2048-len(prefix))), "alice", true); w.Code != http.StatusNoContent {
+ t.Errorf("2048-byte endpoint: %d", w.Code)
+ }
+ if w := post(t, s.Subscribe, subscribeBody(s, prefix+strings.Repeat("e", 2049-len(prefix))), "alice", true); w.Code != http.StatusBadRequest {
+ t.Errorf("2049-byte endpoint: %d", w.Code)
+ }
r := httptest.NewRequest(http.MethodGet, "/aviso/subscribe", nil)
w := httptest.NewRecorder()
s.Subscribe(w, r)
@@ -120,6 +142,57 @@ func TestSubscribeGating(t *testing.T) {
}
}
+// Bad keys are refused before put, so a mangled re-subscribe can
+// neither replace working keys nor delete previousEndpoint.
+func TestSubscribeRefusesInvalidKeysWithoutTouchingRows(t *testing.T) {
+ s := newService(t)
+ ctx := context.Background()
+ _ = post(t, s.Subscribe, subscribeBody(s, "https://push.example/old"), "alice", true)
+ _ = post(t, s.Subscribe, subscribeBody(s, "https://push.example/e1"), "alice", true)
+ before, _ := s.List(ctx, "alice")
+ good := keys()
+ for name, k := range map[string]map[string]string{
+ "p256dh not base64": {"p256dh": "!", "auth": good["auth"]},
+ "p256dh not a point": {"p256dh": base64.RawURLEncoding.EncodeToString(make([]byte, 65)), "auth": good["auth"]},
+ "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": "!"},
+ } {
+ b, _ := json.Marshal(map[string]any{
+ "subscription": map[string]any{"endpoint": "https://push.example/e1", "keys": k},
+ "publicKey": s.PublicKeyString(),
+ "previousEndpoint": "https://push.example/old",
+ })
+ if w := post(t, s.Subscribe, string(b), "alice", true); w.Code != http.StatusBadRequest {
+ t.Errorf("%s: %d", name, w.Code)
+ }
+ }
+ after, _ := s.List(ctx, "alice")
+ if len(after) != 2 || after[0].Auth != before[0].Auth || after[1].Auth != before[1].Auth || after[1].Revision != before[1].Revision {
+ t.Fatalf("rows changed by refused requests:\n%+v\n%+v", before, after)
+ }
+ // Padded base64url, which some toJSON() implementations emit, is fine.
+ padded := map[string]string{
+ "p256dh": base64.URLEncoding.EncodeToString(mustDecode(good["p256dh"])),
+ "auth": base64.URLEncoding.EncodeToString(mustDecode(good["auth"])),
+ }
+ b, _ := json.Marshal(map[string]any{
+ "subscription": map[string]any{"endpoint": "https://push.example/e2", "keys": padded},
+ "publicKey": s.PublicKeyString(),
+ })
+ if w := post(t, s.Subscribe, string(b), "alice", true); w.Code != http.StatusNoContent {
+ t.Fatalf("padded keys: %d %s", w.Code, w.Body)
+ }
+}
+
+func mustDecode(s string) []byte {
+ b, err := base64.RawURLEncoding.DecodeString(s)
+ if err != nil {
+ panic(err)
+ }
+ return b
+}
+
// Real browsers' toJSON() carries expirationTime; a strict decoder
// would refuse every genuine subscription.
func TestSubscribeToleratesBrowserFields(t *testing.T) {
@@ -156,6 +229,20 @@ func TestSubscribeHonoursPreviousEndpoint(t *testing.T) {
if w := post(t, s.Subscribe, string(b), "alice", true); w.Code != http.StatusBadRequest {
t.Fatalf("bad previousEndpoint accepted: %d", w.Code)
}
+ // Naming another subject's endpoint as previous is a 204 that
+ // deletes nothing of theirs.
+ _ = post(t, s.Subscribe, subscribeBody(s, "https://push.example/bobs"), "bob", true)
+ b, _ = json.Marshal(map[string]any{
+ "subscription": map[string]any{"endpoint": "https://push.example/new3", "keys": keys()},
+ "publicKey": s.PublicKeyString(),
+ "previousEndpoint": "https://push.example/bobs",
+ })
+ if w := post(t, s.Subscribe, string(b), "alice", true); w.Code != http.StatusNoContent {
+ t.Fatalf("%d %s", w.Code, w.Body)
+ }
+ if rows, _ := s.List(context.Background(), "bob"); len(rows) != 1 {
+ t.Fatal("alice's previousEndpoint deleted bob's row")
+ }
}
func TestUnsubscribe(t *testing.T) {
@@ -178,6 +265,12 @@ func TestUnsubscribe(t *testing.T) {
if w := post(t, s.Unsubscribe, `{}`, "alice", true); w.Code != http.StatusBadRequest {
t.Errorf("missing endpoint: %d", w.Code)
}
+ if w := post(t, s.Unsubscribe, body+"garbage", "alice", true); w.Code != http.StatusBadRequest {
+ t.Errorf("trailing garbage: %d", w.Code)
+ }
+ if rows, _ := s.List(ctx, "alice"); len(rows) != 1 {
+ t.Fatal("a refused unsubscribe removed the row")
+ }
if w := post(t, s.Unsubscribe, body, "alice", true); w.Code != http.StatusNoContent {
t.Errorf("own: %d", w.Code)
}