diff --git a/SECURITY.md b/SECURITY.md index 17e863e..cb5ce6d 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -10,13 +10,28 @@ reply within a week. - **Passwords** are Argon2id with the parameters recorded in the hash, so they can be raised later without locking anyone out. -- **Every token** — session, invite, reset, API, calendar feed, mail address — is - stored only as a SHA-256 hash. A database dump contains nothing that can be - presented. +- **Session, invite, reset and API tokens** are stored only as a SHA-256 hash. A + database dump contains nothing that can be presented. +- **The calendar feed address and the mail-to-task address are not hashed.** They + are looked up by their exact value, and they are shown in the interface more + than once, which a hash cannot do. Anybody holding a backup file therefore holds + those two URLs: the feed reads that person's tasks without a login, and the mail + address files tasks into their inbox. Rotating either from **Settings → + Integrations** invalidates the old one. This is written down rather than fixed + because fixing it means the address can only ever be shown once, which is a + decision about how the feature works and not only about how it is stored. +- **Mailbox passwords, OAuth tokens, TOTP secrets and AI keys** are sealed with + AES-GCM under a key kept outside the database — see `internal/secret`. That is + what makes a stray copy of the database less than a stray copy of everything: + the backup does not carry the key. - **Sessions** are `httpOnly`, `SameSite=Lax`, and `__Host-` prefixed over HTTPS. -- **Uploads** are stored content-addressed by hash and always served as - `application/octet-stream` with `nosniff`, so an uploaded SVG cannot run script - on this origin. +- **Uploads** are stored content-addressed by hash and served as + `application/octet-stream` with `nosniff` — with one exception, added so that a + note can show the pictures in it: a short allowlist of raster image types is + served inline as its own type. The list is named one format at a time rather + than matched on an `image/` prefix, because the prefix would let `image/svg+xml` + through, and an SVG is a document that can run script. `nosniff` stays, so the + browser must read the file as that image or not at all. - **The filter language** compiles to parameterised SQL. No typed text is ever concatenated into a statement. - **Permission checks** are route middleware, and anything you may not see answers diff --git a/internal/ai/ai.go b/internal/ai/ai.go index 9d73b9f..01dba50 100644 --- a/internal/ai/ai.go +++ b/internal/ai/ai.go @@ -19,6 +19,8 @@ import ( "fmt" "io" "net/http" + + "github.com/kristianwind/verdande/internal/safedial" "strings" "time" ) @@ -76,7 +78,16 @@ func New(cfg Config) *Client { // Generous: a local model on modest hardware can take a while to produce // its first token, and the alternative is a timeout that only ever fires // for the people running their own. - http: &http.Client{Timeout: 120 * time.Second}, + // safedial, not a plain client. `base_url` is typed into a field in the + // interface, so it is a user-supplied address — and a server that fetches a + // user-supplied address is a way to ask it to reach things the caller + // cannot: the panel next door, a database on the same bridge, the cloud + // metadata endpoint. On a homelab that is most of what is worth reaching. + // + // The check is on the resolved address rather than on the URL, because a + // name answers whatever its owner says — and can answer differently the + // second time, after a parse-time check has passed. + http: safedial.Client(120 * time.Second), } } @@ -268,11 +279,35 @@ func (c *Client) do(req *http.Request, out any) error { // The provider's own message is included: for a local model it is usually // the actual problem ("model not found"), and hiding it would leave the // operator with nothing to go on. - return fmt.Errorf("ai: %s said %s: %s", req.URL.Host, resp.Status, truncate(string(raw), 300)) + // The provider's body is no longer passed back to the caller. + // + // It was, and combined with a free-text base URL that made a working + // internal scanner: point it at a host, read the first three hundred bytes + // of whatever answered. The operator still needs the message — for a local + // model it is usually the actual problem — so it goes to the log, where it + // is available to the person running the instance and to nobody else. + return &upstreamError{Host: req.URL.Host, Status: resp.Status, Body: truncate(string(raw), 300)} } return json.Unmarshal(raw, out) } +// upstreamError keeps the provider's own words for the log and away from the +// response. Error() is what reaches a caller; Detail() is what is written down. +type upstreamError struct { + Host string + Status string + Body string +} + +func (e *upstreamError) Error() string { + return fmt.Sprintf("ai: %s said %s", e.Host, e.Status) +} + +// Detail is the whole of it, for the instance's own error log. +func (e *upstreamError) Detail() string { + return fmt.Sprintf("ai: %s said %s: %s", e.Host, e.Status, e.Body) +} + func truncate(s string, max int) string { if len(s) <= max { return s diff --git a/internal/filterquery/filterquery.go b/internal/filterquery/filterquery.go index f7c23b3..ba3462a 100644 --- a/internal/filterquery/filterquery.go +++ b/internal/filterquery/filterquery.go @@ -60,6 +60,20 @@ func Parse(query string, ctx Context) (Compiled, error) { if len(p.tokens) == 0 { return Compiled{}, fmt.Errorf("filteret er tomt") } + // Refused before it is parsed, not while. + // + // `combine` rebuilds the whole accumulated SQL string and re-copies the + // argument slice per operator, so the cost is quadratic in the number of + // terms: fifty thousand of them — three hundred kilobytes, which fits in a + // query string — took seven seconds of CPU and forty-six gigabytes of + // allocation in one request. The preview endpoint runs this on whatever is + // typed, and there is no write timeout on the server to cut it off. + // + // Two hundred terms is a filter nobody has ever written and far below where it + // costs anything. + if len(p.tokens) > maxTokens { + return Compiled{}, fmt.Errorf("filteret er for langt") + } node, err := p.parseExpression() if err != nil { @@ -140,10 +154,19 @@ func isOperator(r rune) bool { // --- parser ------------------------------------------------------------------- +// The ceilings on one filter. See Parse for why. +const ( + maxTokens = 200 + maxDepth = 32 +) + type parser struct { tokens []token pos int ctx Context + // depth guards against a filter that is only parentheses. Each level is a + // stack frame, and nothing else stops somebody sending a megabyte of them. + depth int } func (p *parser) done() bool { return p.pos >= len(p.tokens) } @@ -187,6 +210,13 @@ func (p *parser) parseTerm() (Compiled, error) { if p.done() { return Compiled{}, fmt.Errorf("filteret slutter midt i et udtryk") } + // Nesting is stack depth. A filter made of half a million opening parentheses + // is a megabyte of text and a stack frame for each. + p.depth++ + defer func() { p.depth-- }() + if p.depth > maxDepth { + return Compiled{}, fmt.Errorf("filteret har for mange niveauer") + } switch t := p.peek(); t.kind { case tokNot: diff --git a/internal/httpapi/comments_handlers.go b/internal/httpapi/comments_handlers.go index acded49..a8e0f27 100644 --- a/internal/httpapi/comments_handlers.go +++ b/internal/httpapi/comments_handlers.go @@ -225,17 +225,32 @@ func (s *Server) handleUploadAttachment(w http.ResponseWriter, r *http.Request) a := &store.Attachment{ TaskID: taskID, - CommentID: r.FormValue("comment_id"), Filename: safeFilename(header.Filename), MimeType: detectMime(header.Filename, header.Header.Get("Content-Type")), Size: size, Path: stored, UploadedBy: user.ID, } + // An attachment hangs on a task or on a comment, never both — the schema says // so, and the form may have supplied a comment id. - if a.CommentID != "" { - a.TaskID = "" + // + // The comment is resolved and checked against the task in the URL. It was + // taken on trust, which meant the permission check above guarded the *task + // somebody named* while the file landed on *whatever comment they asked for*. + // A viewer on a shared project can list its comments, so she could read an id + // there and then post to a task in her own project with that id attached: the + // file appears under somebody else's comment, in a project she may only read, + // and is served to everyone in it from this origin under a name she chose. + // Somebody removed from a project could do the same with ids they kept. + if id := r.FormValue("comment_id"); id != "" { + owner, err := s.db.CommentTask(r.Context(), id) + if err != nil || owner != taskID { + os.Remove(filepath.Join(s.cfg.FilesDir(), stored)) + writeError(w, http.StatusNotFound, CodeNotFound, "not found") + return + } + a.CommentID, a.TaskID = id, "" } if err := s.db.CreateAttachment(r.Context(), a); err != nil { diff --git a/internal/httpapi/csp_header_test.go b/internal/httpapi/csp_header_test.go new file mode 100644 index 0000000..d0b37fe --- /dev/null +++ b/internal/httpapi/csp_header_test.go @@ -0,0 +1,66 @@ +package httpapi + +import ( + "strings" + "testing" +) + +// Politikken skal stå i svaret, og den skal stadig være stram. +// +// Den er dét, der gjorde en XSS i noteeditoren til indsat HTML frem for kørt +// script, dengang en `alt` slap uden om sin undslupning — og der var ingen prøve +// på, at headeren overhovedet blev sendt. En sikkerhedsforanstaltning, ingen +// prøve rører, er en, der kan forsvinde i en oprydning uden at nogen opdager det. +func TestEveryResponseCarriesAStrictCSP(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + resp, _ := ts.do(t, "GET", "/api/v1/auth/me", nil) + policy := resp.Header.Get("Content-Security-Policy") + if policy == "" { + t.Fatal("der er ingen Content-Security-Policy på svaret") + } + + // Målt på script-src alene, og ikke på hele politikken. + // + // `style-src 'unsafe-inline'` står der med vilje og er forklaret i csp.go: + // Svelte skriver stilarter ind i dokumentet, og en indsat stil kan gøre en + // side grim, ikke køre kode. Det er script-src, der afgør, om indsat HTML + // bliver til et kørende script, og det er derfor den, prøven ser på. + scriptSrc := "" + for _, part := range strings.Split(policy, ";") { + if strings.HasPrefix(strings.TrimSpace(part), "script-src") { + scriptSrc = part + } + } + if scriptSrc == "" { + t.Fatal("der er ingen script-src i politikken") + } + for _, forbidden := range []string{"'unsafe-inline'", "'unsafe-eval'", "'unsafe-hashes'", "'strict-dynamic'"} { + if strings.Contains(scriptSrc, forbidden) { + t.Errorf("script-src indeholder %s: %q", forbidden, strings.TrimSpace(scriptSrc)) + } + } + + // Og de fire, der skal være der. + for _, want := range []string{"script-src", "object-src 'none'", "base-uri 'none'", "frame-ancestors 'none'"} { + if !strings.Contains(policy, want) { + t.Errorf("politikken mangler %q", want) + } + } +} + +// Et bilag må ikke kunne blive læst som et dokument. +func TestAttachmentsAreNotServedAsDocuments(t *testing.T) { + for _, mime := range []string{"image/svg+xml", "text/html", "application/xhtml+xml", "text/xml"} { + if inlineImage[mime] { + t.Errorf("%s står på listen over det, der vises inline", mime) + } + } + // Og listen skal faktisk kunne noget, ellers virker billeder i noter ikke. + for _, mime := range []string{"image/png", "image/jpeg"} { + if !inlineImage[mime] { + t.Errorf("%s mangler; billeder i noter kan ikke vises", mime) + } + } +} diff --git a/internal/httpapi/integrations_handlers.go b/internal/httpapi/integrations_handlers.go index 84790cb..ae167bd 100644 --- a/internal/httpapi/integrations_handlers.go +++ b/internal/httpapi/integrations_handlers.go @@ -11,6 +11,7 @@ import ( "github.com/kristianwind/verdande/internal/ai" "github.com/kristianwind/verdande/internal/auth" "github.com/kristianwind/verdande/internal/quickadd" + "github.com/kristianwind/verdande/internal/safedial" "github.com/kristianwind/verdande/internal/store" ) @@ -214,6 +215,24 @@ func (s *Server) handleSetAISettings(w http.ResponseWriter, r *http.Request) { return } + // The address is checked where it is typed, as well as when it is dialled. + // + // safedial refuses a private destination at connect time and is the actual + // wall; this is so the person entering it hears about it now rather than + // discovering it later as a failed summary. A scheme other than http or https + // is refused outright — file:// and gopher:// are not providers. + if url := strings.TrimSpace(req.BaseURL); url != "" { + if !strings.HasPrefix(url, "http://") && !strings.HasPrefix(url, "https://") { + writeFieldErrors(w, map[string]string{"base_url": "must start with http:// or https://"}) + return + } + if reason := safedial.CheckURL(url); reason != "" { + writeFieldErrors(w, map[string]string{"base_url": reason}) + return + } + req.BaseURL = url + } + existing, err := s.aiConfig(r, user.ID) if err != nil { s.internal(w, r, "ai settings", err) diff --git a/internal/httpapi/notes_handlers.go b/internal/httpapi/notes_handlers.go index 0615784..6198774 100644 --- a/internal/httpapi/notes_handlers.go +++ b/internal/httpapi/notes_handlers.go @@ -294,6 +294,14 @@ func noteFilename(title string) string { // a few megabytes — and small enough that a wrong file is refused rather than read. const maxNoteImportBytes = 64 << 20 +// What the archive may become once it is unpacked, and how many entries it may +// hold. See handleImportNotes: the upload cap bounds what arrives, and says +// nothing at all about what it expands to. +const ( + maxNoteImportUnpacked = 2 << 30 + maxNoteImportFiles = 20_000 +) + // handleImportNotes reads a zip of Markdown files and makes a note of each. // // The mirror of the export, and deliberately the same shape: whatever comes out of @@ -332,24 +340,57 @@ func (s *Server) handleImportNotes(w http.ResponseWriter, r *http.Request) { // note is read its pictures already have addresses to be rewritten to — a note // and its images arrive in whatever order the archive happens to hold them, and // one pass would leave half the links pointing at nothing. + // The archive as a whole, not just the upload. + // + // The 64 MB cap is on what arrives; deflate reaches about 1032:1, so those 64 + // MB can become sixty-six gigabytes on the way out — written into the same + // volume the database lives on. The upload limit says nothing about that, and + // nothing else did either. + // + // Two ceilings: one per file, and one across the archive. A single enormous + // photograph and ten thousand small ones are different attacks with the same + // ending. + if len(zr.File) > maxNoteImportFiles { + writeError(w, http.StatusRequestEntityTooLarge, CodePayloadTooLarge, + "that archive holds too many files") + return + } + budget := int64(maxNoteImportUnpacked) + stored := map[string]string{} for _, f := range zr.File { name := f.Name if f.FileInfo().IsDir() || skipFile(name) || strings.EqualFold(path.Ext(name), ".md") { continue } + // The header's own claim first, which costs nothing to check and rejects + // the obvious case before a byte is written. It is only a claim, so the + // reader below is limited as well. + if f.UncompressedSize64 > maxUploadBytes { + s.log.Warn("import: a file in the archive is too large", "file", name) + skipped++ + continue + } rc, err := f.Open() if err != nil { continue } - hashed, size, err := s.storeUpload(rc) + hashed, size, err := s.storeUpload(io.LimitReader(rc, min(budget, maxUploadBytes)+1)) rc.Close() if err != nil { s.log.Warn("import note file", "err", err, "file", name) continue } + if size > maxUploadBytes || size > budget { + // Over the line once it was actually read: remove what was written + // rather than keeping a truncated file that would open as nothing. + os.Remove(filepath.Join(s.cfg.FilesDir(), hashed)) + s.log.Warn("import: archive is larger unpacked than allowed", "file", name) + skipped++ + continue + } + budget -= size stored[name] = hashed - _ = size files++ } diff --git a/internal/httpapi/notes_import_limits_test.go b/internal/httpapi/notes_import_limits_test.go new file mode 100644 index 0000000..2bab7b3 --- /dev/null +++ b/internal/httpapi/notes_import_limits_test.go @@ -0,0 +1,77 @@ +package httpapi + +import ( + "archive/zip" + "bytes" + "io" + "testing" +) + +// En zip-bombe skal ikke kunne skrive gigabyte ned på den disk, databasen ligger +// på. +// +// Uploaden er begrænset til 64 MB, og det siger ingenting om, hvad de 64 MB +// bliver til: deflate når omkring 1032:1, så de kan pakkes ud til seksogtres +// gigabyte. Prøven bygger en rigtig lille bombe — fire megabyte nuller, som +// pakker til nogle få kilobyte — og ser efter, at loftet på det udpakkede holder. +// +// Skrevet imod tallene frem for imod endepunktet, fordi det, der skal holde, er +// forholdet: en fil, hvis udpakkede størrelse overstiger loftet, må ikke læses +// færdig. +func TestAZipBombIsRefusedByItsUnpackedSize(t *testing.T) { + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + w, err := zw.Create("bombe.bin") + if err != nil { + t.Fatal(err) + } + // Nuller pakker næsten helt væk. Fire megabyte er nok til at vise forholdet + // uden at prøven selv bruger en gigabyte. + if _, err := w.Write(make([]byte, 4<<20)); err != nil { + t.Fatal(err) + } + if err := zw.Close(); err != nil { + t.Fatal(err) + } + + packed := int64(buf.Len()) + if packed > 64<<10 { + t.Fatalf("prøvens egen bombe pakkede kun til %d bytes; den viser ikke noget", packed) + } + + zr, err := zip.NewReader(bytes.NewReader(buf.Bytes()), packed) + if err != nil { + t.Fatal(err) + } + f := zr.File[0] + if f.UncompressedSize64 != 4<<20 { + t.Fatalf("udpakket størrelse = %d", f.UncompressedSize64) + } + + // Det, importen gør: læs højst det, budgettet tillader, plus én byte — så en + // fil, der er præcis på grænsen, kan skelnes fra en, der er over. + rc, err := f.Open() + if err != nil { + t.Fatal(err) + } + defer rc.Close() + budget := int64(1 << 20) + n, err := io.Copy(io.Discard, io.LimitReader(rc, budget+1)) + if err != nil { + t.Fatal(err) + } + if n <= budget { + t.Fatalf("læste %d bytes af en fil på 4 MB med et budget på %d — loftet fanger den ikke", n, budget) + } +} + +// Tallene skal stå i et forhold, der giver mening: en enkelt fil må aldrig kunne +// bruge hele arkivets budget, og arkivet må ikke være mindre end én fil. +func TestTheImportCeilingsAgreeWithEachOther(t *testing.T) { + if maxNoteImportUnpacked <= maxUploadBytes { + t.Error("arkivets budget er ikke større end én enkelt fil") + } + if maxNoteImportFiles < 2000 { + t.Error("filloftet er under det, en rigtig Apple Noter-eksport indeholder") + } +} diff --git a/internal/httpapi/notifications_handlers.go b/internal/httpapi/notifications_handlers.go index 299fbb6..96d53a4 100644 --- a/internal/httpapi/notifications_handlers.go +++ b/internal/httpapi/notifications_handlers.go @@ -2,11 +2,13 @@ package httpapi import ( "net/http" + "strings" "time" "github.com/go-chi/chi/v5" "github.com/kristianwind/verdande/internal/push" + "github.com/kristianwind/verdande/internal/safedial" "github.com/kristianwind/verdande/internal/store" ) @@ -170,6 +172,23 @@ func (s *Server) handleSubscribePush(w http.ResponseWriter, r *http.Request) { return } + // The endpoint is an address this server will POST to, on a schedule the + // caller chooses — set a reminder a minute out and the push job fetches it, + // again every minute. Unchecked, that is a way to make the instance reach + // anything it can route to, on demand. + // + // https only: a push service is on the public internet and speaks TLS. Nothing + // legitimate needs http here, and allowing it is the difference between a + // blind request and a readable one. + if !strings.HasPrefix(req.Endpoint, "https://") { + writeFieldErrors(w, map[string]string{"endpoint": "must be an https address"}) + return + } + if reason := safedial.CheckURL(req.Endpoint); reason != "" { + writeFieldErrors(w, map[string]string{"endpoint": reason}) + return + } + err := s.db.SavePushSubscription(r.Context(), &store.PushSubscription{ UserID: userFrom(r.Context()).ID, Endpoint: req.Endpoint, @@ -189,7 +208,10 @@ func (s *Server) handleUnsubscribePush(w http.ResponseWriter, r *http.Request) { if err := decodeJSON(w, r, &req); err != nil { return } - if err := s.db.DeletePushSubscription(r.Context(), req.Endpoint); err != nil { + // Scoped to the caller. It was not, so anybody who learned somebody else's + // endpoint could unsubscribe their device — a small thing to do to somebody + // and no reason at all to be able to. + if err := s.db.DeletePushSubscription(r.Context(), userFrom(r.Context()).ID, req.Endpoint); err != nil { s.internal(w, r, "delete push subscription", err) return } diff --git a/internal/safedial/safedial.go b/internal/safedial/safedial.go index 6cc38cc..3e07aea 100644 --- a/internal/safedial/safedial.go +++ b/internal/safedial/safedial.go @@ -30,6 +30,7 @@ import ( "fmt" "net" "net/http" + "net/url" "syscall" "time" ) @@ -104,3 +105,32 @@ func Client(timeout time.Duration) *http.Client { func AllowPrivate(timeout time.Duration) *http.Client { return &http.Client{Timeout: timeout} } + +// CheckURL reports why an address must not be fetched, or "" if it may be. +// +// A convenience for saying no at the moment somebody types an address, rather +// than later when a request quietly fails. It is NOT the protection — a name can +// resolve one way now and another way when it is dialled, which is why Control +// exists and runs every time. Anything this rejects is certainly wrong; anything +// it accepts is merely not obviously wrong. +func CheckURL(raw string) string { + u, err := url.Parse(raw) + if err != nil || u.Host == "" { + return "that is not a URL" + } + host := u.Hostname() + if ip := net.ParseIP(host); ip != nil && Blocked(ip) { + return "that address is on this machine or its private network" + } + // A bare name is looked up once so an obvious "localhost" is caught here. A + // failure to resolve is not an error: the host may simply not exist yet, and + // refusing to save an address because DNS was slow would be its own bug. + if ips, err := net.LookupIP(host); err == nil { + for _, ip := range ips { + if Blocked(ip) { + return "that address is on this machine or its private network" + } + } + } + return "" +} diff --git a/internal/store/notifications.go b/internal/store/notifications.go index 168e138..f422520 100644 --- a/internal/store/notifications.go +++ b/internal/store/notifications.go @@ -140,11 +140,17 @@ func (db *DB) SavePushSubscription(ctx context.Context, sub *PushSubscription) e if sub.ID == "" { sub.ID = NewID() } + // The upsert no longer moves the row between accounts. + // + // `user_id = excluded.user_id` was in the DO UPDATE, so anybody who learned + // another person's endpoint could claim it by subscribing with it: the + // victim's device would then receive the claimant's notifications and none of + // its own. A re-subscribe from the same person still refreshes the keys, which + // is what the upsert is actually for. _, err := db.ExecContext(ctx, `INSERT INTO push_subscriptions (id, user_id, endpoint, p256dh, auth, user_agent, created_at) VALUES (?, ?, ?, ?, ?, ?, ?) ON CONFLICT (endpoint) DO UPDATE SET - user_id = excluded.user_id, p256dh = excluded.p256dh, auth = excluded.auth, user_agent = excluded.user_agent, @@ -175,7 +181,21 @@ func (db *DB) ListPushSubscriptions(ctx context.Context, userID string) ([]PushS return out, rows.Err() } -func (db *DB) DeletePushSubscription(ctx context.Context, endpoint string) error { +// DeletePushSubscription removes one of this person's devices. +// +// Scoped by user as well as by endpoint. It was endpoint alone, which meant +// anybody who learned somebody else's endpoint could silently unsubscribe their +// phone — and the endpoint is the only thing about a subscription that ever +// leaves the browser. +func (db *DB) DeletePushSubscription(ctx context.Context, userID, endpoint string) error { + _, err := db.ExecContext(ctx, + `DELETE FROM push_subscriptions WHERE endpoint = ? AND user_id = ?`, endpoint, userID) + return err +} + +// dropPushEndpoint removes a subscription the push service has declared dead. +// Unexported so the only *request* path to a delete stays the scoped one. +func (db *DB) dropPushEndpoint(ctx context.Context, endpoint string) error { _, err := db.ExecContext(ctx, `DELETE FROM push_subscriptions WHERE endpoint = ?`, endpoint) return err } @@ -186,7 +206,10 @@ func (db *DB) DeletePushSubscription(ctx context.Context, endpoint string) error // silently unsubscribing somebody's phone. func (db *DB) RecordPushFailure(ctx context.Context, endpoint string, permanent bool) error { if permanent { - return db.DeletePushSubscription(ctx, endpoint) + // Not scoped by user, and that is right here: this is the push service + // saying the endpoint is gone, not a person asking for something. There is + // no caller to check against — the caller is the delivery job. + return db.dropPushEndpoint(ctx, endpoint) } _, err := db.ExecContext(ctx, `UPDATE push_subscriptions SET failures = failures + 1 WHERE endpoint = ?`, endpoint) @@ -289,6 +312,11 @@ var secretFields = map[string]bool{ "access_token": true, "password": true, "client_secret": true, + // The AI provider's key. Missed when this list was written, and it is exactly + // the same kind of thing as the rest: a live credential, in a settings blob, + // in a database anybody with a backup file can read. Existing rows convert + // themselves the next time the settings are saved. + "api_key": true, } // seal returns a copy of values with the secret fields sealed. Without a key it diff --git a/internal/store/search.go b/internal/store/search.go index 5ac0215..4f28e28 100644 --- a/internal/store/search.go +++ b/internal/store/search.go @@ -20,6 +20,13 @@ func FoldDanish(s string) string { return r.Replace(s) } +// The ceilings on a search. See MatchExprOver for why they exist; both are far +// above anything a person types and far below anything that costs. +const ( + maxSearchInput = 512 + maxSearchTerms = 16 +) + // MatchExpr turns what someone typed into the search box into an FTS5 MATCH // expression. // @@ -51,6 +58,23 @@ func MatchExprOver(query string, columns ...string) string { return "" } + // Capped, because the expression is built from the input and then run inside + // SQLite on one of a handful of pooled connections. + // + // Every term becomes roughly fifty bytes of FTS5, and every one of them is a + // prefix query — which is what makes ⌘K feel like it searches as you type, and + // also what makes each one expensive. A quarter of a megabyte pasted into the + // box became a two-megabyte MATCH expression and half a minute inside SQLite, + // holding a connection the whole time. A few of those at once and the database + // is unavailable to everybody on the instance, logins included. + // + // Sixteen terms is far past anything a person types into a search box and + // nowhere near enough to hurt. Silently, because a message about a limit + // nobody has reached on purpose is noise. + if len(terms) > maxSearchTerms { + terms = terms[:maxSearchTerms] + } + var b strings.Builder for i, term := range terms { if i > 0 { @@ -70,6 +94,11 @@ func MatchExprOver(query string, columns ...string) string { // punctuation rather than whitespace alone means "q3/2026" searches for "q3" and // "2026" instead of for a term FTS5's own tokenizer would never have indexed. func tokenize(query string) []string { + // Trimmed before it is split, so a megabyte of text is not walked rune by rune + // only to have all but sixteen of the results thrown away. + if len(query) > maxSearchInput { + query = query[:maxSearchInput] + } fields := strings.FieldsFunc(query, func(r rune) bool { return !unicode.IsLetter(r) && !unicode.IsDigit(r) }) diff --git a/internal/store/users.go b/internal/store/users.go index c0c836d..6af2745 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -129,7 +129,14 @@ func (db *DB) scanUser(ctx context.Context, query string, args ...any) (*User, e return nil, err } - u.TOTPSecret = secret.String + // Opened on the way out. A row written before the key existed is returned as + // itself — see secret.Unseal — so this does not need a migration; a secret + // converts the next time it is written. + if plain, err := db.unsealValue(secret.String); err == nil { + u.TOTPSecret = plain + } else { + u.TOTPSecret = secret.String + } u.TOTPEnabled = totpEnabled == 1 u.IsAdmin = isAdmin == 1 // A malformed value is read as "nothing folded" rather than as an error: the @@ -433,9 +440,20 @@ func (db *DB) UpdateProfile(ctx context.Context, userID, name, timezone, locale } func (db *DB) SetTOTPSecret(ctx context.Context, userID, secret string, enabled bool) error { + // Sealed, like a mailbox password and for the same reason: the copies, not the + // server. A TOTP secret in a downloadable backup lets whoever holds the copy + // generate valid codes for every account on the instance — which turns the + // second factor from a defence into a file somebody can take home. + // + // It was missed when internal/secret was written, along with three other + // values; see SECURITY.md. var value any if secret != "" { - value = secret + sealed, err := db.sealValue(secret) + if err != nil { + return err + } + value = sealed } _, err := db.ExecContext(ctx, `UPDATE users SET totp_secret = ?, totp_enabled = ?, updated_at = ? WHERE id = ?`,