mavweb: handler tests + gate DisableTool at step-up

- handlers_test.go: first tests for cmd/mavweb (feature-ranking #2). Covers the /tools enable/disable surface (arg parsing, error mapping, html escaping) and the webauthn handler contracts (method guards, malformed input). 14 cases. Verified the enable path is genuinely gated: an un-asserted call fails at the mavend IPC boundary (Requirement(EnableTool)=AuthStepUp), so mavweb stays a trust-nothing pass-through and core mediates. - policy.go: DisableTool now also requires AuthStepUp. It mutates the same tool allowlist as EnableTool and is a lever to silence a security-relevant tool; gating allowlist mutation uniformly beats a split rule. ProposeTool stays maven-callable (no passkey). Corrects the stale api.go comment that claimed all three gated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-03 21:22:10 +04:00
parent 047a813278
commit de2058c851
3 changed files with 292 additions and 5 deletions
@@ -0,0 +1,282 @@
+package main
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"reflect"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/kami/maven/internal/ipc"
+	"github.com/kami/maven/internal/webauthn"
+)
+
+// fakeCore records the mutating calls handleTools makes and returns canned
+// tool lists / errors. Embedding ipc.CoreAPI (nil) satisfies the large
+// interface — only the methods the handlers touch are overridden; any other
+// call would nil-panic, which is fine since the handlers never make them.
+type fakeCore struct {
+	ipc.CoreAPI
+
+	proposed, enabled []ipc.Tool
+	listErr           error
+
+	enableErr  error
+	disableErr error
+
+	// recorded args from the last Enable/Disable call
+	gotEnableName string
+	gotEnableCmd  []string
+	gotEnableDest bool
+	gotDisable    string
+}
+
+func (f *fakeCore) EnableTool(_ context.Context, name string, cmd []string, destructive bool, _ time.Time) error {
+	f.gotEnableName, f.gotEnableCmd, f.gotEnableDest = name, cmd, destructive
+	return f.enableErr
+}
+
+func (f *fakeCore) DisableTool(_ context.Context, name string) error {
+	f.gotDisable = name
+	return f.disableErr
+}
+
+func (f *fakeCore) ListTools(_ context.Context, status string) ([]ipc.Tool, error) {
+	if f.listErr != nil {
+		return nil, f.listErr
+	}
+	switch status {
+	case "proposed":
+		return f.proposed, nil
+	default:
+		return f.enabled, nil
+	}
+}
+
+// --- GET ---
+
+func TestHandleTools_GET_RendersAndEscapes(t *testing.T) {
+	core := &fakeCore{
+		proposed: []ipc.Tool{{Name: "<b>x", Utterance: "restart the <i>thing"}},
+		enabled:  []ipc.Tool{{Name: "svc", Cmd: []string{"systemctl", "restart"}, Destructive: true}},
+	}
+	rr := httptest.NewRecorder()
+	handleTools(rr, httptest.NewRequest(http.MethodGet, "/tools", nil), core)
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("status = %d, want 200", rr.Code)
+	}
+	body := rr.Body.String()
+	// html/template must escape the untrusted (STT-sourced) tool name.
+	if strings.Contains(body, "<b>x") {
+		t.Errorf("tool name rendered unescaped in output")
+	}
+	if !strings.Contains(body, "&lt;b&gt;x") {
+		t.Errorf("expected escaped tool name &lt;b&gt;x in output")
+	}
+	if !strings.Contains(body, "svc") || !strings.Contains(body, "systemctl restart") {
+		t.Errorf("enabled tool not rendered: %s", body)
+	}
+}
+
+func TestHandleTools_NilCore_503(t *testing.T) {
+	rr := httptest.NewRecorder()
+	handleTools(rr, httptest.NewRequest(http.MethodGet, "/tools", nil), nil)
+	if rr.Code != http.StatusServiceUnavailable {
+		t.Fatalf("status = %d, want 503", rr.Code)
+	}
+}
+
+// --- POST enable ---
+
+func postForm(action string, vals url.Values) *http.Request {
+	vals.Set("action", action)
+	r := httptest.NewRequest(http.MethodPost, "/tools", strings.NewReader(vals.Encode()))
+	r.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	return r
+}
+
+func TestHandleTools_POST_Enable_HappyPath(t *testing.T) {
+	core := &fakeCore{}
+	rr := httptest.NewRecorder()
+	handleTools(rr, postForm("enable", url.Values{
+		"name":        {"svc"},
+		"cmd":         {"systemctl restart nginx"},
+		"destructive": {"on"},
+	}), core)
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("status = %d, want 200; body=%s", rr.Code, rr.Body.String())
+	}
+	if core.gotEnableName != "svc" {
+		t.Errorf("name = %q, want svc", core.gotEnableName)
+	}
+	if want := []string{"systemctl", "restart", "nginx"}; !reflect.DeepEqual(core.gotEnableCmd, want) {
+		t.Errorf("cmd = %v, want %v", core.gotEnableCmd, want)
+	}
+	if !core.gotEnableDest {
+		t.Errorf("destructive = false, want true")
+	}
+}
+
+func TestHandleTools_POST_Enable_MissingName_400(t *testing.T) {
+	core := &fakeCore{}
+	rr := httptest.NewRecorder()
+	handleTools(rr, postForm("enable", url.Values{"cmd": {"systemctl restart"}}), core)
+	if rr.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, want 400", rr.Code)
+	}
+}
+
+func TestHandleTools_POST_Enable_MissingCmd_400(t *testing.T) {
+	core := &fakeCore{}
+	rr := httptest.NewRecorder()
+	handleTools(rr, postForm("enable", url.Values{"name": {"svc"}}), core)
+	if rr.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, want 400", rr.Code)
+	}
+}
+
+func TestHandleTools_POST_Enable_CoreError_502(t *testing.T) {
+	core := &fakeCore{enableErr: ipc.ErrForbidden}
+	rr := httptest.NewRecorder()
+	handleTools(rr, postForm("enable", url.Values{
+		"name": {"svc"}, "cmd": {"systemctl restart"},
+	}), core)
+	if rr.Code != http.StatusBadGateway {
+		t.Fatalf("status = %d, want 502", rr.Code)
+	}
+}
+
+func TestHandleTools_POST_UnknownAction_400(t *testing.T) {
+	core := &fakeCore{}
+	rr := httptest.NewRecorder()
+	handleTools(rr, postForm("frobnicate", url.Values{"name": {"svc"}}), core)
+	if rr.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, want 400", rr.Code)
+	}
+}
+
+// --- POST disable ---
+
+func TestHandleTools_POST_Disable_HappyPath(t *testing.T) {
+	core := &fakeCore{}
+	rr := httptest.NewRecorder()
+	handleTools(rr, postForm("disable", url.Values{"name": {"svc"}}), core)
+	if rr.Code != http.StatusOK {
+		t.Fatalf("status = %d, want 200; body=%s", rr.Code, rr.Body.String())
+	}
+	if core.gotDisable != "svc" {
+		t.Errorf("disabled name = %q, want svc", core.gotDisable)
+	}
+}
+
+func TestHandleTools_POST_Disable_MissingName_400(t *testing.T) {
+	core := &fakeCore{}
+	rr := httptest.NewRecorder()
+	handleTools(rr, postForm("disable", url.Values{}), core)
+	if rr.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, want 400", rr.Code)
+	}
+}
+
+func TestHandleTools_POST_Disable_CoreError_502(t *testing.T) {
+	core := &fakeCore{disableErr: ipc.ErrToolNotFound}
+	rr := httptest.NewRecorder()
+	handleTools(rr, postForm("disable", url.Values{"name": {"svc"}}), core)
+	if rr.Code != http.StatusBadGateway {
+		t.Fatalf("status = %d, want 502", rr.Code)
+	}
+}
+
+// TestEnableTool_NoInProcessAuthGate documents CURRENT behavior: handleTools
+// performs the boundary-moving EnableTool with no in-process authentication —
+// there is no passkey/session check in the Go handler. A POST enable with no
+// prior WebAuthn AssertFinish succeeds (reaches core.EnableTool). Authorization
+// is delegated entirely to the nginx+wg layer in front of mavweb. Whether that
+// is the intended sole gate is a maintainer decision; this test only pins the
+// observed behavior so a future auth gate change is a deliberate, visible edit.
+func TestEnableTool_NoInProcessAuthGate(t *testing.T) {
+	core := &fakeCore{}
+	rr := httptest.NewRecorder()
+	// No passkey session, no cookie, no header — just the raw POST.
+	handleTools(rr, postForm("enable", url.Values{
+		"name": {"svc"}, "cmd": {"systemctl restart"},
+	}), core)
+	if rr.Code != http.StatusOK || core.gotEnableName != "svc" {
+		t.Fatalf("expected unauthenticated enable to reach core (status=%d, name=%q); "+
+			"if this now fails, an in-process auth gate was added", rr.Code, core.gotEnableName)
+	}
+}
+
+// --- webauthn handler wiring (contract level, not crypto) ---
+
+func newTestPasskey() *PasskeyHandle {
+	return newPasskeyHandle(webauthn.Config{
+		Origin: "https://maven.example",
+		RPID:   "maven.example",
+		RPName: "maven",
+	}, nil) // nil core ⇒ assertFn nil; AssertFinish skips the IPC step-up
+}
+
+func TestWebAuthn_Finish_MethodGuards(t *testing.T) {
+	pk := newTestPasskey()
+	for _, tc := range []struct {
+		name string
+		h    http.HandlerFunc
+	}{
+		{"register", pk.RegisterFinish},
+		{"assert", pk.AssertFinish},
+	} {
+		rr := httptest.NewRecorder()
+		tc.h(rr, httptest.NewRequest(http.MethodGet, "/x", nil))
+		if rr.Code != http.StatusMethodNotAllowed {
+			t.Errorf("%s finish GET = %d, want 405", tc.name, rr.Code)
+		}
+	}
+}
+
+func TestWebAuthn_Finish_MalformedJSON_400(t *testing.T) {
+	pk := newTestPasskey()
+	for _, tc := range []struct {
+		name string
+		h    http.HandlerFunc
+	}{
+		{"register", pk.RegisterFinish},
+		{"assert", pk.AssertFinish},
+	} {
+		rr := httptest.NewRecorder()
+		req := httptest.NewRequest(http.MethodPost, "/x", strings.NewReader("{not json"))
+		tc.h(rr, req)
+		if rr.Code != http.StatusBadRequest {
+			t.Errorf("%s finish malformed = %d, want 400", tc.name, rr.Code)
+		}
+	}
+}
+
+func TestWebAuthn_Begin_ReturnsChallenge(t *testing.T) {
+	pk := newTestPasskey()
+	for _, tc := range []struct {
+		name string
+		h    http.HandlerFunc
+	}{
+		{"register", pk.RegisterBegin},
+		{"assert", pk.AssertBegin},
+	} {
+		rr := httptest.NewRecorder()
+		tc.h(rr, httptest.NewRequest(http.MethodGet, "/x", nil))
+		if rr.Code != http.StatusOK {
+			t.Errorf("%s begin = %d, want 200", tc.name, rr.Code)
+			continue
+		}
+		if ct := rr.Header().Get("Content-Type"); ct != "application/json" {
+			t.Errorf("%s begin content-type = %q, want application/json", tc.name, ct)
+		}
+		if !strings.Contains(rr.Body.String(), `"challenge"`) {
+			t.Errorf("%s begin body missing challenge: %s", tc.name, rr.Body.String())
+		}
+	}
+}
@@ -45,10 +45,13 @@ const (
 // which the vet check in dispatch catches via Method existence, not auth).
 func Requirement(m ipc.Method) Authority {
 	switch m {
-	case ipc.MethodEnableTool:
-		// Registration-enable is privilege escalation: it moves the boundary
-		// (adds a runnable capability). Human-only, step-up asserted — never a
-		// module or the voice/chat path. maven can propose but never enable.
+	case ipc.MethodEnableTool, ipc.MethodDisableTool:
+		// Both mutate the tool allowlist — the boundary. Enable adds a runnable
+		// capability (privilege escalation); disable removes one (fail-safe
+		// direction, but still an allowlist mutation and a lever an attacker
+		// could pull to silence a security-relevant tool). Human-only, step-up
+		// asserted — never a module or the voice/chat path. maven can propose
+		// (MethodProposeTool, no step-up: she has no passkey) but never en/disable.
 		return AuthStepUp
 	case ipc.MethodWriteFact:
 		return AuthWrite
@@ -221,7 +221,9 @@ type CoreAPI interface {
 	// returns whether a new proposal was written. EnableTool fills cmd +
 	// destructive and flips status to 'enabled'. DisableTool reverts an
 	// enabled tool back to proposed (it stays in the store, won't run).
-	// All three gate at AuthStepUp. LookupTool/ListTools read them.
+	// Enable/DisableTool gate at AuthStepUp (allowlist mutation, human-only);
+	// ProposeTool is maven-callable (no step-up — she has no passkey).
+	// LookupTool/ListTools read them.
 	ProposeTool(ctx context.Context, name, utterance string, ts time.Time) (bool, error)
 	EnableTool(ctx context.Context, name string, cmd []string, destructive bool, ts time.Time) error
 	DisableTool(ctx context.Context, name string) error