fix(install): require complete bounded remote manifests

2026-11-07 10:45:59 -10:00 · 2026-10-04 11:51:27 +08:00
1 parent d98832f695
commit dabc3481d2
@@ -3,6 +3,8 @@ package installsource
 import (
 	"errors"
 	"fmt"
+
+	"reasonix/internal/contract/tool"
 )

 // RiskLevel classifies how dangerous an action is. The install-capability skill
@@ -40,9 +42,9 @@ var (
 	// that escapes the expected skill roots — typically an attempt to
 	// read arbitrary host files.
 	ErrUnsafeLinkTarget = errors.New("install_source: link target escapes skill roots")
-	// ErrSourceUnreadable: a URL did not respond, returned non-2xx, or a
-	// local path was not readable.
-	ErrSourceUnreadable = errors.New("install_source: source is not readable")
+	// ErrSourceUnreadable: a URL could not supply a complete bounded response,
+	// or a local path was not readable.
+	ErrSourceUnreadable = tool.Refusal{Code: "install.source_unreadable", Message: "install_source: source is not readable"}
 	// ErrManifestMissing: a path was reachable but contained no installable
 	// artifact (no SKILL.md, no .mcp.json, no executable, etc.).
 	ErrManifestMissing = errors.New("install_source: no installable manifest")
@@ -2,6 +2,7 @@ package installsource

 import (
 	"context"
+	"fmt"
 	"io"
 	"net/http"
 	"time"
@@ -21,35 +22,35 @@ const defaultFetchLimit = 2 << 20

 // fetchText performs a bounded GET on sourceURL using the tool's HTTP client.
 // It applies defaultFetchTimeout unless the caller's context already has a
-// tighter deadline, and never reads more than defaultFetchLimit bytes.
+// tighter deadline, and reads one byte past the size limit to detect overflow.
 func (t *Tool) fetchText(ctx context.Context, sourceURL string) (string, error) {
-	if _, hasDeadline := ctx.Deadline(); !hasDeadline {
-		var cancel context.CancelFunc
-		ctx, cancel = context.WithTimeout(ctx, defaultFetchTimeout)
-		defer cancel()
-	}
+	ctx, cancel := context.WithTimeout(ctx, defaultFetchTimeout)
+	defer cancel()
 	req, err := http.NewRequestWithContext(ctx, http.MethodGet, sourceURL, nil)
 	if err != nil {
-		return "", newErr(ErrSourceUnreadable, "%s: %v", sourceURL, err)
+		return "", newErr(ErrSourceUnreadable, "request: %v", err)
 	}
 	if req.Header.Get("User-Agent") == "" {
 		req.Header.Set("User-Agent", "reasonix-install/1.0")
 	}
 	resp, err := t.httpClient.Do(req)
 	if err != nil {
-		return "", newErr(ErrSourceUnreadable, "%s: %v", sourceURL, err)
+		return "", newErr(ErrSourceUnreadable, "request: %v", err)
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden {
-		return "", newErr(ErrAuthRequired, "%s: HTTP %d", sourceURL, resp.StatusCode)
+		return "", &sourceHTTPError{sentinel: ErrAuthRequired, status: resp.StatusCode}
 	}
-	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		return "", newErr(ErrSourceUnreadable, "%s: HTTP %d", sourceURL, resp.StatusCode)
+	if resp.StatusCode < 200 || resp.StatusCode >= 300 || resp.StatusCode == http.StatusPartialContent {
+		return "", &sourceHTTPError{sentinel: ErrSourceUnreadable, status: resp.StatusCode}
 	}
-	limited := io.LimitReader(resp.Body, defaultFetchLimit)
+	limited := io.LimitReader(resp.Body, defaultFetchLimit+1)
 	body, err := io.ReadAll(limited)
 	if err != nil {
-		return "", newErr(ErrSourceUnreadable, "%s: read body: %v", sourceURL, err)
+		return "", newErr(ErrSourceUnreadable, "read body: %v", err)
+	}
+	if len(body) > defaultFetchLimit {
+		return "", &hostFactError{sentinel: ErrSourceUnreadable, facts: fmt.Sprintf("response exceeds %d-byte limit", defaultFetchLimit)}
 	}
 	return string(body), nil
 }
@@ -0,0 +1,78 @@
+package installsource
+
+import (
+	"context"
+	"errors"
+	"io"
+	"net/http"
+	"strings"
+	"testing"
+	"testing/iotest"
+
+	"reasonix/internal/base/secrets"
+	"reasonix/internal/contract/tool"
+)
+
+func TestManifestFetchKeepsSafeDiagnosticFactsAndIdentity(t *testing.T) {
+	const source = "https://fixture-user:[email protected]/SKILL.md?token=fixture-token#fixture-fragment"
+	for _, tc := range []struct {
+		name, facts string
+		status      int
+		body        string
+		readError   bool
+		requestErr  error
+		sentinel    error
+	}{
+		{name: "overflow", status: http.StatusOK, body: strings.Repeat("x", defaultFetchLimit+1), sentinel: ErrSourceUnreadable, facts: "response exceeds 2097152-byte limit"},
+		{name: "partial", status: http.StatusPartialContent, sentinel: ErrSourceUnreadable, facts: "HTTP 206"},
+		{name: "not-found", status: http.StatusNotFound, sentinel: ErrSourceUnreadable, facts: "HTTP 404"},
+		{name: "unauthorized", status: http.StatusUnauthorized, sentinel: ErrAuthRequired, facts: "HTTP 401"},
+		{name: "forbidden", status: http.StatusForbidden, sentinel: ErrAuthRequired, facts: "HTTP 403"},
+		{name: "read-error", status: http.StatusOK, readError: true, sentinel: ErrSourceUnreadable},
+		{name: "transport-error", requestErr: errors.New("fixture-peer-prose " + source), sentinel: ErrSourceUnreadable},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			client := &http.Client{Transport: manifestSizeRoundTrip(func(r *http.Request) (*http.Response, error) {
+				if tc.requestErr != nil {
+					return nil, tc.requestErr
+				}
+				var reader io.Reader = strings.NewReader(tc.body)
+				if tc.readError {
+					reader = iotest.ErrReader(errors.New("fixture-peer-prose " + source))
+				}
+				return &http.Response{StatusCode: tc.status, Header: make(http.Header), Body: io.NopCloser(reader), Request: r}, nil
+			})}
+			tl := &Tool{httpClient: client}
+			body, err := tl.fetchText(context.Background(), source)
+			if body != "" || !errors.Is(err, tc.sentinel) {
+				t.Fatalf("body=%q err=%v; want empty body and source identity", body, err)
+			}
+			projected := secrets.DiagnosticError(err)
+			if !errors.Is(projected, tc.sentinel) {
+				t.Fatal("diagnostic projection lost the source identity")
+			}
+			if errors.Is(tc.sentinel, ErrSourceUnreadable) {
+				var refusal tool.Refusal
+				if !errors.As(projected, &refusal) || refusal.Code != "install.source_unreadable" {
+					t.Fatalf("diagnostic projection lost the refusal: %v", projected)
+				}
+			}
+			for _, display := range []string{err.Error(), projected.Error()} {
+				for _, secret := range []string{"fixture-user", "fixture-password", "fixture-token", "fixture-fragment", "fixture-peer-prose", "example.test"} {
+					if strings.Contains(display, secret) {
+						t.Fatalf("diagnostic exposed %q: %s", secret, display)
+					}
+				}
+				if tc.facts != "" && !strings.Contains(display, tc.facts) {
+					t.Fatalf("diagnostic omitted host facts %q: %s", tc.facts, display)
+				}
+			}
+			if tc.name == "overflow" {
+				var facts *hostFactError
+				if !errors.As(projected, &facts) {
+					t.Fatal("overflow bypassed the existing host fact owner")
+				}
+			}
+		})
+	}
+}
@@ -0,0 +1,80 @@
+package installsource
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"io"
+	"net/http"
+	"strings"
+	"testing"
+	"time"
+
+	"reasonix/internal/base/testenv"
+	"reasonix/internal/contract/tool"
+)
+
+func TestOversizedManifestExecuteCarriesSourceIdentity(t *testing.T) {
+	srv := manifestSizeServer(t, strings.Repeat("x", defaultFetchLimit+1), false)
+	tl := NewTool(Options{ProjectRoot: testenv.TempDir(t), HomeDir: testenv.TempDir(t), HTTPClient: srv.Client(), RequireApprovedPlan: true})
+	raw, err := json.Marshal(map[string]string{"source": srv.URL + "/SKILL.md", "kind": "skill"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	out, err := tl.Execute(context.Background(), raw)
+	var refusal tool.Refusal
+	if out != "" || !errors.Is(err, ErrSourceUnreadable) || !errors.As(err, &refusal) || refusal.Code != "install.source_unreadable" {
+		t.Fatalf("source failure identity: output=%q err=%v refusal=%+v", out, err, refusal)
+	}
+	if !strings.Contains(err.Error(), "install.source_unreadable") {
+		t.Fatalf("rendered error lost its identity: %v", err)
+	}
+}
+
+func TestFetchTextRejectsPartialResponse(t *testing.T) {
+	for _, status := range []int{http.StatusOK, http.StatusCreated, http.StatusPartialContent} {
+		client := &http.Client{Transport: manifestSizeRoundTrip(func(r *http.Request) (*http.Response, error) {
+			if r.Header.Get("Range") != "" {
+				t.Fatal("manifest fetch requested a range")
+			}
+			return &http.Response{StatusCode: status, Header: make(http.Header),
+				Body: io.NopCloser(strings.NewReader("complete fixture")), Request: r}, nil
+		})}
+		tl := &Tool{httpClient: client}
+		got, err := tl.fetchText(context.Background(), "https://example.test/SKILL.md")
+		if status == http.StatusPartialContent {
+			if got != "" || !errors.Is(err, ErrSourceUnreadable) {
+				t.Fatalf("partial response accepted: body=%q err=%v", got, err)
+			}
+		} else if err != nil || got != "complete fixture" {
+			t.Fatalf("complete status %d: body=%q err=%v", status, got, err)
+		}
+	}
+}
+
+func TestFetchTextDeadlineStaysBounded(t *testing.T) {
+	for _, budget := range []time.Duration{0, time.Hour, time.Second} {
+		ctx := context.Background()
+		if budget > 0 {
+			var cancel context.CancelFunc
+			ctx, cancel = context.WithTimeout(ctx, budget)
+			defer cancel()
+		}
+		start := time.Now()
+		client := &http.Client{Transport: manifestSizeRoundTrip(func(r *http.Request) (*http.Response, error) {
+			deadline, ok := r.Context().Deadline()
+			if !ok || deadline.After(start.Add(defaultFetchTimeout+100*time.Millisecond)) {
+				t.Errorf("fetch budget %s has unbounded deadline %v", budget, deadline)
+			}
+			if parent, ok := ctx.Deadline(); ok && deadline.After(parent) {
+				t.Errorf("fetch widened caller deadline: got %v want <= %v", deadline, parent)
+			}
+			return &http.Response{StatusCode: http.StatusOK, Header: make(http.Header),
+				Body: io.NopCloser(strings.NewReader("fixture")), Request: r}, nil
+		})}
+		tl := &Tool{httpClient: client}
+		if _, err := tl.fetchText(ctx, "https://example.test/SKILL.md"); err != nil {
+			t.Fatal(err)
+		}
+	}
+}
@@ -0,0 +1,137 @@
+package installsource
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"fmt"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"reasonix/internal/base/testenv"
+)
+
+func TestFetchTextSizeBoundary(t *testing.T) {
+	for _, streamed := range []bool{false, true} {
+		for _, size := range []int{defaultFetchLimit - 1, defaultFetchLimit, defaultFetchLimit + 1} {
+			t.Run(fmt.Sprintf("streamed=%t/bytes=%d", streamed, size), func(t *testing.T) {
+				body := strings.Repeat("x", size)
+				srv := manifestSizeServer(t, body, streamed)
+				tl := NewTool(Options{ProjectRoot: testenv.TempDir(t), HomeDir: testenv.TempDir(t), HTTPClient: srv.Client()})
+				got, err := tl.fetchText(context.Background(), srv.URL+"/SKILL.md")
+				if size > defaultFetchLimit {
+					if !errors.Is(err, ErrSourceUnreadable) || got != "" {
+						t.Fatalf("oversized read: bytes=%d err=%v; want no content and ErrSourceUnreadable", len(got), err)
+					}
+					return
+				}
+				if err != nil || got != body {
+					t.Fatalf("bounded read: bytes=%d err=%v; want all %d bytes", len(got), err, size)
+				}
+			})
+		}
+	}
+}
+
+func TestOversizedDirectManifestReturnsReadError(t *testing.T) {
+	for _, tc := range []struct{ kind, path, prefix, pad string }{
+		{"auto", "/SKILL.md", "---\nname: large-skill\ndescription: Large skill fixture\n---\n", "x"},
+		{"skill", "/SKILL.md", "---\nname: large-skill\ndescription: Large skill fixture\n---\n", "x"},
+		{"auto", "/.mcp.json", `{"mcpServers":{"counter":{"url":"https://example.com/mcp"}}}`, " "},
+		{"mcp", "/.mcp.json", `{"mcpServers":{"counter":{"url":"https://example.com/mcp"}}}`, " "},
+	} {
+		t.Run(tc.kind+tc.path, func(t *testing.T) {
+			body := tc.prefix + strings.Repeat(tc.pad, defaultFetchLimit+1-len(tc.prefix))
+			srv := manifestSizeServer(t, body, true)
+			root, home := testenv.TempDir(t), testenv.TempDir(t)
+			tl := NewTool(Options{ProjectRoot: root, HomeDir: home, HTTPClient: srv.Client(), RequireApprovedPlan: true})
+			raw, err := json.Marshal(map[string]any{"source": srv.URL + tc.path, "kind": tc.kind})
+			if err != nil {
+				t.Fatal(err)
+			}
+			out, err := tl.Execute(context.Background(), raw)
+			if !errors.Is(err, ErrSourceUnreadable) || out != "" {
+				t.Fatalf("oversized %s manifest produced %d response bytes and err=%v; want the read error", tc.kind, len(out), err)
+			}
+			for _, path := range []string{root, home} {
+				entries, err := os.ReadDir(path)
+				if err != nil || len(entries) != 0 {
+					t.Fatalf("preview wrote to %s: entries=%d err=%v", path, len(entries), err)
+				}
+			}
+		})
+	}
+}
+
+func TestSkillAtFetchLimitInstallsAllBytes(t *testing.T) {
+	prefix := "---\nname: large-skill\ndescription: Large skill fixture\n---\n"
+	body := prefix + strings.Repeat("x", defaultFetchLimit-len(prefix)-len("\nEnd.\n")) + "\nEnd.\n"
+	srv := manifestSizeServer(t, body, true)
+	root, home := testenv.TempDir(t), testenv.TempDir(t)
+	t.Setenv("REASONIX_HOME", filepath.Join(home, ".reasonix"))
+	tl := NewTool(Options{ProjectRoot: root, HomeDir: home, HTTPClient: srv.Client(), RequireApprovedPlan: true})
+	args := map[string]any{"source": srv.URL + "/SKILL.md", "kind": "auto", "mode": "copy", "scope": "global"}
+	preview := execInstall(t, tl, args)
+	if !preview.OK || preview.Applied || len(preview.Actions) != 1 {
+		t.Fatal("limit-sized skill did not produce one unapplied preview")
+	}
+	args["apply"], args["planId"] = true, preview.PlanID
+	out := execInstall(t, tl, args)
+	if !out.OK || !out.Applied || len(out.Actions) != 1 {
+		t.Fatal("limit-sized skill did not install")
+	}
+	got, err := os.ReadFile(out.Actions[0].Target)
+	if err != nil || string(got) != body {
+		t.Fatalf("installed skill: bytes=%d err=%v; want all %d bytes", len(got), err, len(body))
+	}
+}
+
+func TestManifestReadErrorKeepsGitHubTreeFolderFallback(t *testing.T) {
+	oldAPI := githubAPIBaseURL
+	githubAPIBaseURL = "https://api.example.test"
+	t.Cleanup(func() { githubAPIBaseURL = oldAPI })
+	client := &http.Client{Transport: manifestSizeRoundTrip(func(r *http.Request) (*http.Response, error) {
+		status, body := http.StatusNotFound, "not a direct manifest"
+		switch r.URL.String() {
+		case "https://api.example.test/repos/acme/skills/contents/group.md?ref=main":
+			status = http.StatusOK
+			body = `[{"name":"SKILL.md","path":"group.md/SKILL.md","type":"file","download_url":"https://raw.example.test/SKILL.md"}]`
+		case "https://raw.example.test/SKILL.md":
+			status = http.StatusOK
+			body = "---\nname: folder-skill\ndescription: Directory skill fixture\n---\nRead the project.\n"
+		}
+		return &http.Response{StatusCode: status, Header: make(http.Header), Body: io.NopCloser(strings.NewReader(body)), Request: r}, nil
+	})}
+	tl := &Tool{root: testenv.TempDir(t), httpClient: client, preparePlugin: func(context.Context, string, string) (string, string, func(), error) {
+		return "", "", nil, ErrManifestMissing
+	}}
+	actions, _, err := tl.planURL(context.Background(), request{
+		Source: "https://github.com/acme/skills/tree/main/group.md", Kind: "auto", Mode: "copy", Scope: "project", scopeExplicit: true,
+	})
+	if err != nil || len(actions) != 1 || actions[0].Kind != "skill" || actions[0].Name != "folder-skill" {
+		t.Fatalf("GitHub tree folder ending in .md: actions=%d err=%v; want its skill", len(actions), err)
+	}
+}
+
+type manifestSizeRoundTrip func(*http.Request) (*http.Response, error)
+
+func (f manifestSizeRoundTrip) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) }
+
+func manifestSizeServer(t *testing.T, body string, streamed bool) *httptest.Server {
+	t.Helper()
+	srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		if streamed {
+			w.(http.Flusher).Flush()
+		} else {
+			w.Header().Set("Content-Length", fmt.Sprint(len(body)))
+		}
+		_, _ = w.Write([]byte(body))
+	}))
+	t.Cleanup(srv.Close)
+	return srv
+}
@@ -50,15 +50,18 @@ func (t *Tool) planURL(ctx context.Context, req request) ([]action, []string, er
 			return nil, warnings, err
 		}
 	}
-	if req.Kind == "mcp" && !looksLikeMarkdownURL(rawURL) && !looksLikeMCPJSONURL(rawURL) {
+	manifestURL := looksLikeMarkdownURL(rawURL) || looksLikeMCPJSONURL(rawURL)
+	if req.Kind == "mcp" && !manifestURL {
 		return []action{t.remoteMCPAction(req, rawURL)}, nil, nil
 	}
-	if looksLikeMarkdownURL(rawURL) || looksLikeMCPJSONURL(rawURL) || rawURL != req.Source {
+	if manifestURL || rawURL != req.Source {
 		actions, warnings, err := t.planDownloadedURL(ctx, req, rawURL)
 		if err == nil && len(actions) > 0 {
 			return actions, warnings, nil
 		}
-		if req.Kind != "auto" {
+		// A GitHub tree may name a directory ending in .md or .mcp.json.
+		_, repoURL := parseGitHubRepoSource(req.Source)
+		if req.Kind != "auto" || (manifestURL && !repoURL) {
 			return nil, warnings, err
 		}
 	}
@@ -0,0 +1,36 @@
+package agent
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"reasonix/internal/base/testenv"
+	"reasonix/internal/contract/event"
+	"reasonix/internal/contract/provider"
+	"reasonix/internal/contract/tool"
+	"reasonix/internal/ext/installsource"
+	"reasonix/internal/state/sessionstore"
+)
+
+func TestInstallSourceReadRefusalReachesExecutorOutput(t *testing.T) {
+	srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = w.Write([]byte(strings.Repeat("x", (2<<20)+1)))
+	}))
+	t.Cleanup(srv.Close)
+	reg := tool.NewRegistry()
+	reg.Add(installsource.NewTool(installsource.Options{
+		ProjectRoot: testenv.TempDir(t), HomeDir: testenv.TempDir(t), RequireApprovedPlan: true,
+		HTTPClient: srv.Client(),
+	}))
+	a := New(nil, reg, sessionstore.NewSession(""), Options{}, event.Discard)
+	out := a.executeOne(context.Background(), &a.turn, provider.ToolCall{
+		ID: "manifest-read", Name: "install_source", Arguments: fmt.Sprintf(`{"source":%q,"kind":"skill"}`, srv.URL+"/SKILL.md"),
+	})
+	if out.refusalCode != "install.source_unreadable" || !strings.Contains(out.output, "install.source_unreadable") {
+		t.Fatalf("executor lost source identity: code=%q output=%q", out.refusalCode, out.output)
+	}
+}