amadan

rastrillo / aviso Public

Validate Origin as exactly an origin, and Contact as a real address

Codex's review of New: a prefix check let "https://app.example/"
through, and csrf.SameOrigin compares the browser's Origin header to
Config.Origin byte for byte when Sec-Fetch-Site is absent — so a
trailing slash would have refused every legitimate POST. "mailto:"
alone also passed, and webpush-go would have signed a JWT whose
subject was an empty address. Both are now parsed and refused, and
the stability test constructs two Services on one key and checks
they agree, which the old test never did.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev c3ed31ad1e4775d1e39fd2fe4c56ca186a38d67f parent 940027d
3 files changed, +104 −16
  • aviso.go +43 −4
  • aviso_test.go +43 −12
  • vapid_test.go +18 −0
diff --git a/aviso.go b/aviso.go
index 29c2f35..de016f4 100644
--- a/aviso.go
+++ b/aviso.go
@@ -5,6 +5,7 @@ import (
"errors"
"log/slog"
"net/http"
+ "net/url"
"strings"
"time"
)
@@ -87,11 +88,11 @@ func New(cfg Config) (*Service, error) {
if err != nil {
return nil, err
}
- if !strings.HasPrefix(cfg.Contact, "mailto:") && !strings.HasPrefix(cfg.Contact, "https://") {
- return nil, errors.New("aviso: Config.Contact must be a mailto: or https: URL")
+ if err := validateContact(cfg.Contact); err != nil {
+ return nil, err
}
- if !strings.HasPrefix(cfg.Origin, "https://") && !strings.HasPrefix(cfg.Origin, "http://") {
- return nil, errors.New("aviso: Config.Origin must be an absolute origin like https://app.example.com")
+ if err := validateOrigin(cfg.Origin); err != nil {
+ return nil, err
}
if cfg.Concurrency <= 0 {
cfg.Concurrency = 32
@@ -109,6 +110,44 @@ func New(cfg Config) (*Service, error) {
}, nil
}
+// validateContact admits "mailto:<address>" with a non-empty address
+// containing "@", or an https URL with a host. The push service signs
+// nothing with it but may write to it about abuse, and webpush-go
+// would happily sign a JWT whose subject is "mailto:".
+func validateContact(c string) error {
+ const msg = "aviso: Config.Contact must be mailto:<address> or an https: URL with a host"
+ switch {
+ case strings.HasPrefix(c, "mailto:"):
+ addr := strings.TrimPrefix(c, "mailto:")
+ if addr == "" || !strings.Contains(addr, "@") || strings.ContainsAny(addr, " \t\r\n") {
+ return errors.New(msg)
+ }
+ return nil
+ case strings.HasPrefix(c, "https://"):
+ u, err := url.Parse(c)
+ if err != nil || u.Host == "" || u.User != nil {
+ return errors.New(msg)
+ }
+ return nil
+ }
+ return errors.New(msg)
+}
+
+// validateOrigin requires exactly an origin: scheme, host, optional
+// port, nothing else. csrf.SameOrigin compares the browser's Origin
+// header to this string byte for byte when Sec-Fetch-Site is absent,
+// so a trailing slash or a path would refuse every legitimate POST.
+func validateOrigin(o string) error {
+ const msg = "aviso: Config.Origin must be an absolute origin like https://app.example.com (no path, no trailing slash)"
+ u, err := url.Parse(o)
+ if err != nil || (u.Scheme != "https" && u.Scheme != "http") || u.Host == "" || u.User != nil ||
+ u.Path != "" || u.RawQuery != "" || u.Fragment != "" || u.Opaque != "" ||
+ u.Scheme+"://"+u.Host != o {
+ return errors.New(msg)
+ }
+ return nil
+}
+
// PublicKeyString is the applicationServerKey the browser subscribes
// with: unpadded base64url of the uncompressed P-256 point.
func (s *Service) PublicKeyString() string { return s.pub }
diff --git a/aviso_test.go b/aviso_test.go
index 1020ff5..d0829f1 100644
--- a/aviso_test.go
+++ b/aviso_test.go
@@ -44,21 +44,52 @@ func TestNewRefusesBadConfig(t *testing.T) {
if _, err := aviso.New(c); err == nil {
t.Error("nil DB accepted")
}
- c = good
- c.Contact = "ops@example.test"
- if _, err := aviso.New(c); err == nil {
- t.Error("bare address accepted as Contact")
+ for _, contact := range []string{"ops@example.test", "mailto:", "mailto:nobody", "https://", "https://bad host", "http://example.test", "https://u:p@example.test"} {
+ c = good
+ c.Contact = contact
+ if _, err := aviso.New(c); err == nil {
+ t.Errorf("Contact %q accepted", contact)
+ }
}
- c = good
- c.Origin = "app.example.test"
- if _, err := aviso.New(c); err == nil {
- t.Error("schemeless Origin accepted")
+ for _, contact := range []string{"mailto:ops@example.test", "https://example.test/abuse"} {
+ c = good
+ c.Contact = contact
+ if _, err := aviso.New(c); err != nil {
+ t.Errorf("Contact %q refused: %v", contact, err)
+ }
+ }
+ // csrf.SameOrigin compares the Origin header to this string exactly,
+ // so anything beyond scheme://host[:port] would refuse every POST.
+ for _, origin := range []string{"app.example.test", "https://", "https://app.example.test/", "https://app.example.test/path", "https://app.example.test?x=1", "https://u:p@app.example.test", "ftp://app.example.test"} {
+ c = good
+ c.Origin = origin
+ if _, err := aviso.New(c); err == nil {
+ t.Errorf("Origin %q accepted", origin)
+ }
+ }
+ for _, origin := range []string{"https://app.example.test", "http://localhost:8080"} {
+ c = good
+ c.Origin = origin
+ if _, err := aviso.New(c); err != nil {
+ t.Errorf("Origin %q refused: %v", origin, err)
+ }
}
}
-func TestPublicKeyStringIsStable(t *testing.T) {
- s := newService(t)
- if s.PublicKeyString() == "" {
- t.Fatal("empty public key")
+// The same private key must give the same public key across a
+// restart: browsers compare it on every load, and a key that drifted
+// would make every one of them re-enrol.
+func TestPublicKeyStringIsStableAcrossRestart(t *testing.T) {
+ key, _ := aviso.GenerateKey()
+ mk := func() *aviso.Service {
+ s, err := aviso.New(aviso.Config{DB: openDB(t), PrivateKey: key, Contact: "mailto:x@y", Origin: "https://a"})
+ if err != nil {
+ t.Fatal(err)
+ }
+ return s
+ }
+ a, b := mk(), mk()
+ if a.PublicKeyString() == "" || a.PublicKeyString() != b.PublicKeyString() {
+ t.Fatalf("public key not stable: %q vs %q", a.PublicKeyString(), b.PublicKeyString())
}
}
diff --git a/vapid_test.go b/vapid_test.go
index 50c316b..be946a5 100644
--- a/vapid_test.go
+++ b/vapid_test.go
@@ -71,6 +71,24 @@ func TestParsePrivateKeyRefusesBadInput(t *testing.T) {
}
}
+// New must wire parsePrivateKey's outputs unchanged: two Services on
+// one key agree on the public key and the key id rows are stamped with.
+func TestNewWiresTheSameKeyIdentityEveryTime(t *testing.T) {
+ key, _ := GenerateKey()
+ mk := func() *Service {
+ s, err := New(Config{DB: openInternalDB(t), PrivateKey: key, Contact: "mailto:x@y", Origin: "https://a"})
+ if err != nil {
+ t.Fatal(err)
+ }
+ return s
+ }
+ a, b := mk(), mk()
+ pub, id, _ := parsePrivateKey(key)
+ if a.pub != pub || b.pub != pub || a.keyID != id || b.keyID != id {
+ t.Fatalf("New drifted from parsePrivateKey: %q/%q vs %q/%q", a.pub, a.keyID, pub, id)
+ }
+}
+
func TestKeyIDIsSHA256OfThePublicPoint(t *testing.T) {
priv, _ := GenerateKey()
pub, id, err := parsePrivateKey(priv)