fix(api): refuse NUL bytes at the request edge; test SQL injection by effect

The SQL-injection effect test found that a NUL byte in a query string, path or JSON body reached PostgreSQL and surfaced as a 500. Requests are now buffered up to the existing limit and NUL in any form (raw, JSON escaped, form encoded) is refused with a 400 before a query runs. TestSQLInjectionHasNoEffect sends metacharacter payloads through every search, filter and free-text route and asserts effect: users, policies, sessions and tokens unchanged, seeded content intact, payloads stored verbatim. TestDynamicSQLIdentifiersAreAllowListed scans for statements assembled with Sprintf outside the reviewed allow-list.
2026-07-19 19:23:27 +01:00
parent f715d7500b
commit 59493d9398
2 changed files with 198 additions and 1 deletions
@@ -1,13 +1,16 @@
 package api

 import (
+	"bytes"
 	"context"
 	"crypto/rand"
 	"encoding/hex"
+	"io"
 	"log/slog"
 	"net/http"
 	"runtime/debug"
 	"strconv"
+	"strings"
 	"time"

 	"github.com/andydixon/noema/internal/audit"
@@ -116,10 +119,33 @@ func (s *Server) securityHeaders(next http.Handler) http.Handler {
 	})
 }

+// bodyLimit bounds request bodies and refuses NUL bytes anywhere in the
+// request. PostgreSQL text cannot hold NUL, so a NUL that reached a query
+// would surface as an internal error; refusing it at the edge keeps the
+// answer a deliberate 400 and keeps error paths out of reach of a caller
+// probing for them. Bodies are buffered up to the limit, which the JSON and
+// form decoders would do anyway.
 func (s *Server) bodyLimit(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.Body != nil {
+		if strings.Contains(r.URL.Path, "\x00") || strings.Contains(r.URL.RawPath, "%00") || strings.Contains(r.URL.RawQuery, "%00") || strings.Contains(r.URL.RawQuery, "\x00") {
+			writeError(w, r, http.StatusBadRequest, "invalid_input", "NUL bytes are not accepted")
+			return
+		}
+		if r.Body != nil && r.Body != http.NoBody {
 			r.Body = http.MaxBytesReader(w, r.Body, s.deps.Config.MaxRequestBody)
+			b, err := io.ReadAll(r.Body)
+			if err != nil {
+				writeError(w, r, http.StatusRequestEntityTooLarge, "too_large", "request body too large")
+				return
+			}
+			// A raw NUL, a JSON-escaped one or a form-encoded one all decode to the
+			// same byte the database refuses.
+			if bytes.IndexByte(b, 0) >= 0 || bytes.Contains(b, []byte(`\u0000`)) || bytes.Contains(b, []byte("%00")) {
+				writeError(w, r, http.StatusBadRequest, "invalid_input", "NUL bytes are not accepted")
+				return
+			}
+			r.Body = io.NopCloser(bytes.NewReader(b))
+			r.ContentLength = int64(len(b))
 		}
 		next.ServeHTTP(w, r)
 	})
@@ -0,0 +1,171 @@
+package security
+
+import (
+	"context"
+	"encoding/json"
+	"net/url"
+	"os"
+	"path/filepath"
+	"regexp"
+	"strings"
+	"testing"
+
+	"github.com/andydixon/noema/internal/llm"
+	"github.com/andydixon/noema/internal/memory"
+	"github.com/andydixon/noema/internal/snapshots"
+	"github.com/andydixon/noema/pkg/ids"
+)
+
+// scripted is a language model that always returns the same answer. It
+// stands in for a compromised or hostile provider.
+type scripted struct {
+	text string
+	raw  string
+}
+
+func (s scripted) Complete(_ context.Context, _ llm.Request) (llm.Response, error) {
+	r := llm.Response{Text: s.text, Provider: "scripted", Model: "hostile"}
+	if s.raw != "" && json.Valid([]byte(s.raw)) {
+		r.JSON = json.RawMessage(s.raw)
+	}
+	return r, nil
+}
+
+var sqlPayloads = []string{
+	`'; DROP TABLE memories; --`,
+	`") OR 1=1 --`,
+	`%' UNION SELECT id, mind_id, content FROM memories --`,
+	`x'; UPDATE users SET role='administrator' WHERE username='watcher'; --`,
+	`$1; DELETE FROM policies; --`,
+	`\'); INSERT INTO policies (id, mind_id, capability, target_pattern, action_kind, effect) VALUES (gen_random_uuid(), NULL, 'sudo_shell', '*', '*', 'allow'); --`,
+}
+
+// T9: attacker-controlled strings reach every search, filter and free-text
+// field as parameters. The test asserts effect, not just status codes: row
+// counts across sensitive tables are unchanged, seeded content is intact and
+// the payloads are stored verbatim as data.
+func TestSQLInjectionHasNoEffect(t *testing.T) {
+	h := start(t)
+	ctx := context.Background()
+	m := h.mind("sqli")
+	seed := "the launch window opens at 09:00"
+	code, body := h.do(h.admin, "POST", "/api/v1/minds/sqli/memories", map[string]any{"kind": "semantic", "content": seed, "epistemic": "fact", "confidence": 0.9, "importance": 0.9})
+	if code != 201 {
+		t.Fatalf("seed: %d %s", code, body)
+	}
+	var seeded struct{ ID string }
+	json.Unmarshal(body, &seeded) //nolint:errcheck
+	h.do(h.admin, "POST", "/api/v1/minds/sqli/beliefs", map[string]any{"proposition": "the launch is on schedule", "epistemic": "claim", "weight": 0.5})
+	count := func(table string) int {
+		var n int
+		h.deps.DB.QueryRow(ctx, `SELECT count(*) FROM `+table).Scan(&n) //nolint:errcheck
+		return n
+	}
+	before := map[string]int{}
+	for _, tb := range []string{"users", "policies", "memories", "beliefs", "cognitive_events", "sessions", "api_tokens"} {
+		before[tb] = count(tb)
+	}
+	var adminRoles int
+	h.deps.DB.QueryRow(ctx, `SELECT count(*) FROM users WHERE role='administrator'`).Scan(&adminRoles) //nolint:errcheck
+
+	reads := []string{
+		"/api/v1/minds/sqli/memories?q=%s", "/api/v1/minds/sqli/memories?q=%s&mode=list", "/api/v1/minds/sqli/beliefs?q=%s", "/api/v1/minds/sqli/beliefs?subject=%s",
+		"/api/v1/minds/sqli/events?q=%s", "/api/v1/minds/sqli/events?type=%s&source=%s", "/api/v1/minds/sqli/entities?q=%s", "/api/v1/minds/sqli/goals?status=%s",
+		"/api/v1/minds/sqli/predictions?status=%s", "/api/v1/audit?q=%s&action=%s", "/api/v1/notifications?mind=%s", "/api/v1/llm/calls?role=%s&mind=%s",
+		"/api/v1/events?q=%s&source=%s", "/api/v1/approvals?status=%s", "/api/v1/minds/%s",
+	}
+	for _, p := range sqlPayloads {
+		esc := url.QueryEscape(p)
+		for _, tpl := range reads {
+			path := strings.ReplaceAll(tpl, "%s", esc)
+			if code, _ := h.do(h.admin, "GET", path, nil); code >= 500 {
+				t.Errorf("GET %s → %d", tpl, code)
+			}
+		}
+		// Free-text writes: the payload must land as data.
+		if code, body := h.do(h.admin, "POST", "/api/v1/minds/sqli/observations", map[string]any{"text": p}); code >= 300 {
+			t.Errorf("observe %q → %d %s", p, code, body)
+		}
+		if code, _ := h.do(h.admin, "POST", "/api/v1/minds/sqli/memories", map[string]any{"kind": "episodic", "content": p, "epistemic": "observation", "confidence": 0.5, "importance": 0.5}); code != 201 {
+			t.Errorf("memory %q → %d", p, code)
+		}
+		if code, _ := h.do(h.admin, "POST", "/api/v1/minds/sqli/goals", map[string]any{"title": p, "kind": "task"}); code != 201 {
+			t.Errorf("goal %q → %d", p, code)
+		}
+	}
+	// NUL bytes are refused at the edge with a deliberate 400, never a 500.
+	for _, path := range []string{"/api/v1/minds/sqli/memories?q=a%00b", "/api/v1/minds/sql%00i"} {
+		if code, _ := h.do(h.admin, "GET", path, nil); code != 400 && code != 404 {
+			t.Errorf("NUL in %s → %d", path, code)
+		}
+	}
+	if code, _ := h.do(h.admin, "POST", "/api/v1/minds/sqli/observations", map[string]any{"text": "a\x00b"}); code != 400 {
+		t.Errorf("NUL in body → %d", code)
+	}
+	// Effects: nothing beyond the rows this test deliberately created.
+	for _, tb := range []string{"users", "policies", "sessions", "api_tokens"} {
+		if count(tb) != before[tb] {
+			t.Errorf("%s changed: %d → %d", tb, before[tb], count(tb))
+		}
+	}
+	var adminAfter int
+	h.deps.DB.QueryRow(ctx, `SELECT count(*) FROM users WHERE role='administrator'`).Scan(&adminAfter) //nolint:errcheck
+	if adminAfter != adminRoles {
+		t.Fatal("role escalation through injected string")
+	}
+	if count("beliefs") < before["beliefs"] {
+		t.Error("beliefs lost")
+	}
+	mem, err := h.deps.Memory.Get(ctx, m.ID, ids.ID(seeded.ID))
+	if err != nil || mem.Content != seed {
+		t.Fatalf("seeded memory altered: %+v %v", mem, err)
+	}
+	stored, _ := h.deps.Memory.List(ctx, m.ID, memory.Filter{Limit: 50})
+	found := 0
+	for _, s := range stored {
+		for _, p := range sqlPayloads {
+			if s.Content == p {
+				found++
+			}
+		}
+	}
+	if found < len(sqlPayloads) {
+		t.Fatalf("payloads not stored verbatim as data: %d of %d", found, len(sqlPayloads))
+	}
+}
+
+// T9 (static): the only SQL built from non-parameter strings uses identifiers
+// from code-level allow-lists. Any new fmt.Sprintf that assembles a statement
+// must be reviewed and added here deliberately.
+func TestDynamicSQLIdentifiersAreAllowListed(t *testing.T) {
+	allowed := map[string]bool{"internal/snapshots/archive.go": true, "internal/snapshots/compare.go": true, "internal/storage/migrate.go": true}
+	root := filepath.Join("..", "..")
+	stmt := regexp.MustCompile(`(?i)fmt\.Sprintf\(\s*` + "`" + `[^` + "`" + `]*\b(SELECT|INSERT|UPDATE|DELETE|CREATE|DROP|ALTER)\b`)
+	var offenders []string
+	err := filepath.WalkDir(filepath.Join(root, "internal"), func(path string, d os.DirEntry, err error) error {
+		if err != nil || d.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") {
+			return err
+		}
+		src, _ := os.ReadFile(path)
+		if stmt.Match(src) {
+			rel, _ := filepath.Rel(root, path)
+			if !allowed[filepath.ToSlash(rel)] {
+				offenders = append(offenders, rel)
+			}
+		}
+		return nil
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(offenders) > 0 {
+		t.Fatalf("SQL assembled with Sprintf outside the allow-list; use parameters or extend the list after review: %v", offenders)
+	}
+	// The allow-listed identifiers really are identifiers.
+	ident := regexp.MustCompile(`^[a-z][a-z0-9_]*$`)
+	for _, tb := range snapshots.Tables {
+		if !ident.MatchString(tb) {
+			t.Fatalf("snapshot table name is not a plain identifier: %q", tb)
+		}
+	}
+}