fix(notes): én note, der ikke kunne tegnes, låste hele ruden

Meldt ind som "jeg kan ikke skifte mellem noter — uanset hvilken jeg trykker på,
åbner de ikke".

Editoren indlæste med `note.body.trim()` uden værn. `body` er en streng fra
serveren hver eneste gang — indtil den ikke er, og så kaster den her inde i et
`$effect`. En effekt, der kaster, tager komponentens opdateringer med sig: den
forrige note bliver stående, og *ingen* note kan åbnes bagefter. Klikket kan ikke
engang lande, hvilket er derfor det ikke ligner en fejl i noten, man klikkede på,
men i hele ruden.

To ting, ikke én. Værnet, så en manglende krop er en tom note. Og en ramme om
tegningen, så det næste, der en dag ikke kan tegnes, heller ikke bliver til det
her: en note, der fejler, viser sin egen tekst uden formatering, og resten af
ruden virker videre.

`loadedId` sættes nu før tegningen frem for efter. Ellers ville effekten prøve
igen ved hver eneste ændring bagefter og kaste hver gang.

`textContent` og ikke `innerHTML` i faldbakken: teksten er ikke tegnet, og at
lægge den ind som markup ville være at køre præcis det, tegningen nægtede.
Gemningen er ikke i fare — den skriver `draft`, som kommer fra svaret og ikke fra
det, der står i editoren, indtil nogen taster i den.

Prøven tvinger formen frem i stedet for at vente på den nyttelast, der gjorde
det. Hvad der end fik `body` til at mangle, er svaret det samme: én note må ikke
kunne låse de tolv hundrede andre ude. Den er set fejle uden rettelsen — med
netop den fejl, konsollen viste.

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"}
+}

web/e2e/smoke.spec.js

@@ -3038,3 +3038,57 @@ test('flere noter kan markeres, arkiveres og hentes frem igen', async ({ page })
 
 	expect(trouble).toEqual([]);
 });
+
+/**
+ * Én note, der ikke kan tegnes, må ikke tage ruden med sig.
+ *
+ * Meldt ind som "jeg kan ikke skifte mellem noter — uanset hvilken jeg trykker
+ * på, åbner de ikke". Editoren indlæste med `note.body.trim()` uden værn, og
+ * `body` er en streng fra serveren hver eneste gang — indtil den ikke er. Så
+ * kaster den inde i et `$effect`, og en effekt, der kaster, tager komponentens
+ * opdateringer med sig: den forrige note bliver stående, og ingen note kan åbnes
+ * bagefter. Klikket kan ikke engang lande.
+ *
+ * Formen fremtvinges her frem for at vente på den nyttelast, der gjorde det:
+ * hvad der end fik `body` til at mangle, er svaret det samme — én note må ikke
+ * kunne låse de tolv hundrede andre ude.
+ */
+test('en note uden krop må ikke låse ruden', async ({ page }) => {
+	const errors = [];
+	page.on('pageerror', (e) => errors.push(e.message));
+
+	await page.goto('/noter');
+	for (const title of ['Alfa', 'Beta']) {
+		await page.getByRole('button', { name: 'Ny note' }).click();
+		const ed = page.getByRole('textbox', { name: 'Notens tekst' });
+		await ed.click();
+		await page.keyboard.type(title);
+		await expect(ed.locator('h1')).toHaveText(title);
+		await page.keyboard.press('Enter');
+		await page.keyboard.type('Brødtekst i ' + title);
+		await expect(ed.locator('p').filter({ hasText: 'Brødtekst i ' + title })).toBeVisible();
+		await page.waitForTimeout(900);
+	}
+	await page.reload();
+	await expect(page.locator('li .row strong').filter({ hasText: 'Beta' })).toBeVisible();
+
+	await page.route('**/api/v1/notes/*', async (route) => {
+		if (route.request().method() !== 'GET') return route.continue();
+		const res = await route.fetch();
+		const note = await res.json();
+		delete note.body;
+		await route.fulfill({ response: res, json: note });
+	});
+
+	await page.locator('li').filter({ hasText: 'Alfa' }).first().click();
+	await page.waitForTimeout(600);
+	expect(errors, 'noten kastede under indlæsningen').toEqual([]);
+
+	await page.unroute('**/api/v1/notes/*');
+	await page.locator('li').filter({ hasText: 'Beta' }).first().click();
+	await expect(
+		page.getByRole('textbox', { name: 'Notens tekst' }).locator('h1'),
+		'ruden er låst: den næste note åbner ikke'
+	).toHaveText('Beta', { timeout: 5000 });
+	expect(errors, 'ruden efterlod en fejl bag sig').toEqual([]);
+});

web/src/lib/components/NoteEditor.svelte

@@ -35,20 +35,53 @@
 	let loadedId = $state(null);
 	$effect(() => {
 		if (!editor || !note || note.id === loadedId) return;
-		// An empty note opens on its title. That is what Apple Notes does, and it is
-		// also what the list needs: the first line becomes the name of the note, so a
-		// note whose first line is body text is a note called by its first sentence.
+
+		// Sat *før* tegningen, ikke efter.
 		//
-		// Tested on the text and not on the rendered result: an empty body renders as
-		// one empty paragraph, not as nothing, so a falsy check here never fired.
-		editor.innerHTML = note.body.trim() ? markdownToHtml(note.body) : '<h1><br></h1>';
+		// Kaster noget herunder, er noten stadig den, der er forsøgt indlæst — og
+		// uden det her ville effekten prøve igen ved hver eneste ændring bagefter og
+		// kaste hver gang.
 		loadedId = note.id;
 		active = {};
 		// Kildevisningen hører til den note, den blev slået til på. Skifter man note,
 		// er det den rige flade, man skal møde — ellers står den næste note som kode,
 		// uden at nogen har bedt om det.
 		asSource = false;
-		colourCode();
+
+		// Én note, der ikke kan tegnes, må ikke tage ruden med sig.
+		//
+		// Det var `note.body.trim()` uden værn, og `body` er en streng fra serveren
+		// hver eneste gang — indtil den ikke er. Så kaster den her inde i et
+		// `$effect`, og en effekt, der kaster, tager komponentens opdateringer med
+		// sig: den forrige note bliver stående, og *ingen* note kan åbnes bagefter.
+		// Klikket kan ikke engang lande. Meldt ind som "jeg kan ikke skifte mellem
+		// noter, uanset hvilken jeg trykker på", hvilket er præcis, hvad det ser ud
+		// som fra den anden side.
+		//
+		// Fejlen står i konsollen frem for i en toast: den hører til den, der kan
+		// rette den, og en note, der viser sin egen tekst uden formatering, siger
+		// allerede til den, der læser den, at noget er galt.
+		//
+		// `textContent` og ikke `innerHTML` i faldbakken — teksten er ikke tegnet, og
+		// at lægge den ind som markup ville være at køre det, tegningen lige nægtede.
+		// Gemningen er ikke i fare: den skriver `draft`, som kommer fra svaret og
+		// ikke fra det, der står i editoren, indtil nogen taster i den.
+		const body = note.body ?? '';
+		try {
+			// An empty note opens on its title. That is what Apple Notes does, and it
+			// is also what the list needs: the first line becomes the name of the
+			// note, so a note whose first line is body text is a note called by its
+			// first sentence.
+			//
+			// Tested on the text and not on the rendered result: an empty body renders
+			// as one empty paragraph, not as nothing, so a falsy check here never
+			// fired.
+			editor.innerHTML = body.trim() ? markdownToHtml(body) : '<h1><br></h1>';
+			colourCode();
+		} catch (e) {
+			editor.textContent = body;
+			console.error('verdande: noten kunne ikke tegnes', note.id, e);
+		}
 	});
 
 	const SYNTAX = [