fix(sikkerhed): CalDAV lod en viewer skrive, og Web Push kunne ringe indad

To huller fundet ved en gennemgang efter dagens arbejde. Begge har en prøve, der
er set fejle før rettelsen.

CalDAV PUT afgjorde adgang på det projekt, klienten *nævnte i adressen*, og tog
derefter opgavens id fra den samme adresse. Intet bandt de to sammen. En viewer
på et delt projekt kunne pege på et projekt, de selv ejer — indbakken, som alle
har og alle må redigere — og lægge den delte opgaves id i filnavnet. Projekt-
tjekket gik godt på indbakken, og skrivningen landede på en opgave, de kun må
læse. GetTask lukkede det ikke: den svarer "må denne person se det", og det må en
viewer. `CanEdit` er spørgsmålet, og det er det samme, handleUpdateTask altid har
stillet — API-døren har spurgt hele tiden, CalDAV-døren spurgte ikke.

Opgaven skal nu også ligge, hvor adressen siger. Uden det kan en redaktør på to
projekter nå det ene gennem det andets adresse; intet eskaleres af det i sig
selv, men en adresse, der har lov at lyve om hvor noget bor, er sådan sagen
ovenfor kom ind.

Web Push blev leveret med en almindelig http.Client. Adressen kommer fra
browserens abonnement — altså fra den, der er logget ind — og serveren POSTer til
den på et skema, de selv vælger: sæt en påmindelse et minut ude, og jobbet går
igen hvert minut. Tilmeldingen afviser en privat adresse, og den kontrol er ikke
nok alene: den kører på URL'en som skrevet, og et navn, den godkender, kan svare
127.0.0.1 næste gang det slås op. Det er præcis den sætning, `internal/safedial`
selv står og advarer om. Leveringen går gennem safedial nu, som tjekker den
opslåede adresse ved forbindelsen og hvert viderestillingshop på samme måde.
Kontrollen ved tilmelding bliver, fordi den siger nej, mens nogen kigger på
skærmen.

Og en hærdning: /inbound/mail har fået et loft. Det er den svagere af de to
offentlige skriveveje — beaconet skriver en række for en fremmed, denne skriver
ingenting uden en 256-bit adresse, man skal kende i forvejen — men adressen deles
med mailservere og står på en indstillingsside, og når den først er kendt, var der
intet loft på, hvad der kunne laves med den. Tallet er sat generøst: modtageren er
mailserveren, én adresse der leverer det hele.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

internal/httpapi/caldav_handlers.go

@@ -231,6 +231,33 @@ func (s *Server) handleCalDAVPut(w http.ResponseWriter, r *http.Request) {
 
 	existing, getErr := s.db.GetTask(r.Context(), taskID, user.ID)
 	if getErr == nil {
+		// The task decides, not the URL.
+		//
+		// The check above is about the project the *client named*, and a client
+		// names both halves of this address. Asking the project for permission and
+		// then taking the task id from the same string ties nothing together: a
+		// viewer on a shared project could name a project of their own — the Inbox,
+		// which everybody owns and may edit — and put the shared task's id in the
+		// filename. The project check passed on the Inbox and the write landed on a
+		// task they may only read.
+		//
+		// GetTask does not close it either: it answers "may this person see it",
+		// and a viewer may. `CanEdit` is the question, and it is the same one
+		// handleUpdateTask asks — the API door has always asked it.
+		role, err := store.TaskRole(r.Context(), s.db, taskID, user.ID)
+		if err != nil || !role.CanEdit() {
+			http.Error(w, "forbidden", http.StatusForbidden)
+			return
+		}
+		// And it has to live where the address says it does. Without this an editor
+		// on two projects can reach either through the other's URL — nothing is
+		// escalated by that on its own, but a URL allowed to lie about where a
+		// resource lives is how the case above got in.
+		if existing.ProjectID != projectID {
+			http.NotFound(w, r)
+			return
+		}
+
 		// An update. Only the fields a task client can express are touched;
 		// everything else the task carries stays as it is.
 		content := parsed.Summary

internal/httpapi/integrations_test.go

@@ -250,6 +250,116 @@ func (ts *testServer) apiToken(t *testing.T, userID string) string {
 	return token
 }
 
+// The promise of sharing has to hold at every door, not just the API's.
+//
+// TestViewerCannotMutateThroughTheAPI asserts a viewer cannot write, and it
+// asserts it against /api/v1. CalDAV is a second door into the same tasks, and it
+// authorised differently: it checked the caller's standing in the project *named
+// in the URL* and then took the task id from the URL as well. Nothing tied the
+// two together.
+//
+// So a viewer on somebody else's project could name a project of their own —
+// their Inbox, which everybody owns and everybody may edit — and put the other
+// project's task id in the filename. The project check passed on the Inbox, and
+// the write landed on a task in a project they may only read.
+func TestAViewerCannotWriteThroughCalDAV(t *testing.T) {
+	ts := newTestServer(t)
+	ts.bootstrap(t)
+	ts.newUser(t, "viewer@example.dk", "Viewer")
+
+	_, project := ts.do(t, "POST", "/api/v1/projects", map[string]any{"name": "Delt"})
+	projectID := project["id"].(string)
+	_, task := ts.do(t, "POST", "/api/v1/tasks", map[string]any{
+		"content": "en opgave", "project_id": projectID,
+	})
+	taskID := task["id"].(string)
+
+	if resp, body := ts.do(t, "POST", "/api/v1/projects/"+projectID+"/invites", map[string]any{
+		"email": "viewer@example.dk", "role": "viewer",
+	}); resp.StatusCode != http.StatusOK {
+		t.Fatalf("invite: %d %v", resp.StatusCode, body)
+	}
+
+	them, err := ts.db.UserByEmail(t.Context(), "viewer@example.dk")
+	if err != nil {
+		t.Fatal(err)
+	}
+	token := ts.apiToken(t, them.ID)
+
+	// Their own Inbox: a project they own and may edit, and the one every account
+	// has. It is what makes the URL check pass.
+	projects, err := ts.db.ListProjects(t.Context(), them.ID, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var inbox string
+	for _, p := range projects {
+		if p.IsInbox {
+			inbox = p.ID
+		}
+	}
+	if inbox == "" {
+		t.Fatal("the viewer has no inbox")
+	}
+
+	vtodo := "BEGIN:VCALENDAR\r\nVERSION:2.0\r\nBEGIN:VTODO\r\nUID:" + taskID +
+		"\r\nSUMMARY:kapret\r\nSTATUS:COMPLETED\r\nEND:VTODO\r\nEND:VCALENDAR\r\n"
+
+	resp := ts.dav(t, "PUT", "/caldav/"+them.ID+"/"+inbox+"/"+taskID+".ics",
+		token, them.Email, vtodo)
+	if resp.StatusCode < 400 {
+		t.Errorf("a viewer wrote through CalDAV: status %d", resp.StatusCode)
+	}
+
+	// And the task is as its owner left it. The status alone is not the assertion:
+	// the question is whether anything moved.
+	_, after := ts.do(t, "GET", "/api/v1/tasks/"+taskID, nil)
+	if after["content"] != "en opgave" {
+		t.Errorf("the task was renamed by a viewer: %v", after["content"])
+	}
+	if after["completed"] != false {
+		t.Error("the task was completed by a viewer")
+	}
+}
+
+// A task may only be addressed under the project it is actually in.
+//
+// The same hole seen from the other side: an editor on two projects could reach a
+// task in one of them through the other's URL. Nothing is escalated by it — they
+// may edit both — but it is the check that was missing, and a URL that lies about
+// where a resource lives is how the viewer case got in.
+func TestCalDAVRefusesATaskFromAnotherProject(t *testing.T) {
+	ts := newTestServer(t)
+	ts.bootstrap(t)
+
+	user, err := ts.db.UserByEmail(t.Context(), "kristian@example.dk")
+	if err != nil {
+		t.Fatal(err)
+	}
+	token := ts.apiToken(t, user.ID)
+
+	_, one := ts.do(t, "POST", "/api/v1/projects", map[string]any{"name": "Et"})
+	_, two := ts.do(t, "POST", "/api/v1/projects", map[string]any{"name": "To"})
+	_, task := ts.do(t, "POST", "/api/v1/tasks", map[string]any{
+		"content": "hører til Et", "project_id": one["id"].(string),
+	})
+	taskID := task["id"].(string)
+
+	vtodo := "BEGIN:VCALENDAR\r\nVERSION:2.0\r\nBEGIN:VTODO\r\nUID:" + taskID +
+		"\r\nSUMMARY:flyttet i smug\r\nEND:VTODO\r\nEND:VCALENDAR\r\n"
+
+	resp := ts.dav(t, "PUT", "/caldav/"+user.ID+"/"+two["id"].(string)+"/"+taskID+".ics",
+		token, user.Email, vtodo)
+	if resp.StatusCode < 400 {
+		t.Errorf("a task was written under the wrong project: status %d", resp.StatusCode)
+	}
+
+	_, after := ts.do(t, "GET", "/api/v1/tasks/"+taskID, nil)
+	if after["content"] != "hører til Et" {
+		t.Errorf("the task changed: %v", after["content"])
+	}
+}
+
 func TestCalDAVRequiresCredentials(t *testing.T) {
 	ts := newTestServer(t)
 	ts.bootstrap(t)

internal/httpapi/server.go

@@ -63,6 +63,7 @@ type Server struct {
 	// the endpoints that check a secret are limited separately and more tightly
 	// than the rest of the API.
 	loginLimiter *limiter
+	mailLimiter  *limiter
 	resetLimiter *limiter
 	// Second factors are counted against the *account*, not the address. An
 	// address bucket is defeated by anybody with more than one address, which is
@@ -85,6 +86,11 @@ func New(cfg *config.Config, db *store.DB, log *slog.Logger, web fs.FS) *Server
 		mail:         mail.New(cfg.SMTP, cfg.BaseURL, log),
 		hub:          realtime.NewHub(log),
 		loginLimiter: newLimiter(10, 15*time.Minute),
+		// Generous on purpose: the caller is the mail server, one address delivering
+		// everything, and a personal instance gets a handful of forwarded mails a
+		// day. Sixty a minute never touches real delivery and still puts a ceiling
+		// on the one public write path that has none.
+		mailLimiter:  newLimiter(60, time.Minute),
 		resetLimiter: newLimiter(5, time.Hour),
 		// Tighter than the password bucket, and for a whole hour. Somebody typing
 		// their own code gets it right on the first or second try; ten in an hour is
@@ -122,7 +128,15 @@ func New(cfg *config.Config, db *store.DB, log *slog.Logger, web fs.FS) *Server
 
 	// Inbound mail. Delivered by the mail server rather than by a browser, and
 	// authenticated by the token in the recipient address.
-	r.Post("/inbound/mail", s.handleInboundMail)
+	//
+	// Rate limited for the same reason the beacon is: it is reachable by anybody
+	// who can route to this instance. It is the weaker case of the two — the
+	// beacon writes a row for a stranger, this one writes nothing without a
+	// 256-bit address it has to already know — but an address is shared with mail
+	// servers and printed on a settings page, and once it is known there was no
+	// ceiling at all on what could be made with it.
+	r.Method(http.MethodPost, "/inbound/mail",
+		s.rateLimit(s.mailLimiter, http.HandlerFunc(s.handleInboundMail)))
 
 	// MCP with the token in the query string, for clients that cannot send a
 	// header. Claude's custom-connector dialog takes a URL and nothing else, so

internal/push/push.go

@@ -32,6 +32,8 @@ import (
 	"net/url"
 	"strings"
 	"time"
+
+	"github.com/kristianwind/verdande/internal/safedial"
 )
 
 type Subscription struct {
@@ -101,7 +103,19 @@ func Send(ctx context.Context, sub Subscription, payload Payload, vapid VAPID) e
 	req.Header.Set("Urgency", "normal")
 	req.Header.Set("Authorization", "vapid t="+token+", k="+vapid.Public)
 
-	resp, err := (&http.Client{Timeout: 15 * time.Second}).Do(req)
+	// safedial, not a plain client. The endpoint comes from the browser's
+	// subscription, which is to say from whoever is signed in, and this server
+	// POSTs to it on a schedule they choose — set a reminder a minute out and the
+	// job goes again every minute.
+	//
+	// The subscribe handler already refuses a private address, and that check is
+	// not enough on its own: it runs on the URL as written, and a name it approves
+	// can answer 127.0.0.1 the next time it is looked up. safedial checks the
+	// *resolved* address at connect time, which is the only moment the address
+	// that will actually be used is known — and it checks every redirect hop the
+	// same way. The parse-time check stays, because it says no while somebody is
+	// looking at the screen.
+	resp, err := safedial.Client(15 * time.Second).Do(req)
 	if err != nil {
 		return err
 	}

internal/push/push_test.go

@@ -0,0 +1,66 @@
+package push
+
+import (
+	"context"
+	"crypto/ecdh"
+	"crypto/rand"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+)
+
+// A subscription must not be able to point the server at its own network.
+//
+// The endpoint is whatever the browser handed over, which is to say whatever the
+// person signed in handed over, and this server POSTs to it on a schedule they
+// choose. The subscribe handler refuses a private address when it is written
+// down — and that check runs on the URL as written, so a name it approves can
+// answer 127.0.0.1 the next time it is looked up. This is the other half: the
+// address that will actually be dialled, checked at the moment of dialling.
+func TestSendRefusesToReachInside(t *testing.T) {
+	reached := false
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		reached = true
+		w.WriteHeader(http.StatusCreated)
+	}))
+	defer srv.Close()
+	// httptest listens on 127.0.0.1, which is exactly the destination in question.
+	if !strings.Contains(srv.URL, "127.0.0.1") {
+		t.Skipf("the test server is not on loopback: %s", srv.URL)
+	}
+
+	err := Send(context.Background(), testSubscription(t, srv.URL), Payload{Title: "hej"}, testVAPID(t))
+	if err == nil {
+		t.Fatal("Send reached a loopback address without complaining")
+	}
+	if reached {
+		t.Error("the request arrived: the wall let it through and only the answer failed")
+	}
+}
+
+func testSubscription(t *testing.T, endpoint string) Subscription {
+	t.Helper()
+	key, err := ecdh.P256().GenerateKey(rand.Reader)
+	if err != nil {
+		t.Fatal(err)
+	}
+	auth := make([]byte, 16)
+	if _, err := rand.Read(auth); err != nil {
+		t.Fatal(err)
+	}
+	return Subscription{
+		Endpoint: endpoint,
+		P256dh:   b64.EncodeToString(key.PublicKey().Bytes()),
+		Auth:     b64.EncodeToString(auth),
+	}
+}
+
+func testVAPID(t *testing.T) VAPID {
+	t.Helper()
+	pub, priv, err := GenerateVAPIDKeys()
+	if err != nil {
+		t.Fatal(err)
+	}
+	return VAPID{Public: pub, Private: priv, Subject: "mailto:kristian@example.dk"}
+}