diff --git a/internal/httpapi/caldav_handlers.go b/internal/httpapi/caldav_handlers.go index d4c4f4a..90c1721 100644 --- a/internal/httpapi/caldav_handlers.go +++ b/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 diff --git a/internal/httpapi/integrations_test.go b/internal/httpapi/integrations_test.go index 8aea0b5..5533271 100644 --- a/internal/httpapi/integrations_test.go +++ b/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) diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index afb964b..cf74865 100644 --- a/internal/httpapi/server.go +++ b/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 diff --git a/internal/push/push.go b/internal/push/push.go index 81823df..ccd0ea8 100644 --- a/internal/push/push.go +++ b/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 } diff --git a/internal/push/push_test.go b/internal/push/push_test.go new file mode 100644 index 0000000..614e0fc --- /dev/null +++ b/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"} +}