diff --git a/docs/plans/universal-harness-access-phase1.md b/docs/plans/universal-harness-access-phase1.md index 37c0e6f24f..a1053ff1ab 100644 --- a/docs/plans/universal-harness-access-phase1.md +++ b/docs/plans/universal-harness-access-phase1.md @@ -149,7 +149,7 @@ PRs 1, 2, 4, and 6 have no dependencies and can be developed/merged in parallel. - For each declarative field (Agent, Policy, Skills): - Local path: return as-is - URL: extract/require integrity hash → validate against `AllowedRemoteResources` → check cache (with re-verification) → if miss and not offline: `fetch.FetchURL` → verify hash → `CachePut` → `AppendFetchAudit` → return cache content path - - Phase 1: single-level only (no transitive deps), security scanning deferred + - Single-level resolution; transitive deps added in Phase 2 (PR 2 of ADR-0038), security scanning deferred **Create `internal/resolve/resolve_test.go`:** - Tests using `httptest.NewTLSServer`: local pass-through, URL fetch+cache, cache hit, hash mismatch, URL not in allowlist, missing hash, offline+miss, offline+hit, security scan failure, mixed harness, audit entries diff --git a/docs/plans/universal-harness-access.md b/docs/plans/universal-harness-access.md index 2732344c41..75ccf71a9f 100644 --- a/docs/plans/universal-harness-access.md +++ b/docs/plans/universal-harness-access.md @@ -953,8 +953,7 @@ type ResolveOpts struct { // ResolveHarness resolves URL-referenced declarative fields (Agent, Policy, // Skills) in the harness to local cache paths. Local paths are left unchanged. -// The harness is modified in place. -// Phase 1: single-level resolution only (no transitive deps). +// The harness is modified in place. Transitive deps supported via MaxDepth. func ResolveHarness(ctx context.Context, h *harness.Harness, opts ResolveOpts) ([]Dependency, error) { var deps []Dependency @@ -971,18 +970,17 @@ func ResolveHarness(ctx context.Context, h *harness.Harness, opts ResolveOpts) ( return deps, nil } -// resolveResourceWithLimits resolves a single resource with depth and count limits. -// Phase 1: depth is always 0 (no transitive resolution), parentRef is unused -// Phase 2+: depth tracking prevents cycles and runaway recursion, parentRef enables relative path resolution -func resolveResourceWithLimits(ctx context.Context, workspaceRoot, ref string, allowedPrefixes []string, policy fetch.FetchPolicy, depth int, resourceCount *int, parentRef string) (string, error) { - // Phase 2+: Check depth limit (Phase 1 always passes since depth=0) - if depth > policy.MaxDepth { - return "", fmt.Errorf("exceeded maximum dependency depth of %d", policy.MaxDepth) +// Note: pseudocode below is illustrative. The implemented API uses resolveURL + +// resolveTransitiveDeps with explicit depth parameters and opts.MaxDepth/MaxResources. +// resolveResourceWithLimits was the design placeholder name; it was not shipped. +func resolveResourceWithLimits(ctx context.Context, workspaceRoot, ref string, allowedPrefixes []string, opts ResolveOpts, depth int, resourceCount *int, parentRef string) (string, error) { + if depth > opts.MaxDepth { + return "", fmt.Errorf("exceeded maximum dependency depth of %d", opts.MaxDepth) } // Check resource count limit (applies to all phases) - if *resourceCount >= policy.MaxResources { - return "", fmt.Errorf("exceeded maximum resource count of %d", policy.MaxResources) + if *resourceCount >= opts.MaxResources { + return "", fmt.Errorf("exceeded maximum resource count of %d", opts.MaxResources) } if harness.IsURL(ref) { diff --git a/internal/resolve/relurl.go b/internal/resolve/relurl.go new file mode 100644 index 0000000000..c8d40611ad --- /dev/null +++ b/internal/resolve/relurl.go @@ -0,0 +1,27 @@ +package resolve + +import ( + "fmt" + "net/url" +) + +// ResolveRelativeURL resolves a relative reference against a parent URL +// using RFC 3986 semantics. Absolute URLs are returned unchanged. The +// caller must validate the resolved URL against allowed prefixes. +func ResolveRelativeURL(parentURL, relRef string) (string, error) { + rel, err := url.Parse(relRef) + if err != nil { + return "", fmt.Errorf("parsing relative ref %q: %w", relRef, err) + } + if rel.IsAbs() { + return relRef, nil + } + + parent, err := url.Parse(parentURL) + if err != nil { + return "", fmt.Errorf("parsing parent URL %q: %w", parentURL, err) + } + + resolved := parent.ResolveReference(rel) + return resolved.String(), nil +} diff --git a/internal/resolve/relurl_test.go b/internal/resolve/relurl_test.go new file mode 100644 index 0000000000..f3d128b608 --- /dev/null +++ b/internal/resolve/relurl_test.go @@ -0,0 +1,103 @@ +package resolve + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestResolveRelativeURL(t *testing.T) { + tests := []struct { + name string + parentURL string + relRef string + want string + wantErr bool + }{ + { + name: "sibling reference", + parentURL: "https://example.com/skills/rust/SKILL.md", + relRef: "../common/SKILL.md", + want: "https://example.com/skills/common/SKILL.md", + }, + { + name: "child reference", + parentURL: "https://example.com/skills/rust/SKILL.md", + relRef: "policies/sandbox.yaml", + want: "https://example.com/skills/rust/policies/sandbox.yaml", + }, + { + name: "absolute URL passthrough", + parentURL: "https://example.com/skills/rust/SKILL.md", + relRef: "https://other.com/skills/common/SKILL.md#sha256=abc", + want: "https://other.com/skills/common/SKILL.md#sha256=abc", + }, + { + name: "path traversal resolves correctly", + parentURL: "https://github.com/org/skills/rust/SKILL.md", + relRef: "../../../../attacker/evil.md", + want: "https://github.com/attacker/evil.md", + }, + { + name: "multiple parent segments", + parentURL: "https://example.com/a/b/c/d/SKILL.md", + relRef: "../../other/sub/SKILL.md", + want: "https://example.com/a/b/other/sub/SKILL.md", + }, + { + name: "fragment preservation", + parentURL: "https://example.com/skills/rust/SKILL.md", + relRef: "../common/SKILL.md#sha256=abc123", + want: "https://example.com/skills/common/SKILL.md#sha256=abc123", + }, + { + name: "bare fragment reference", + parentURL: "https://example.com/skills/rust/SKILL.md", + relRef: "#sha256=abc123", + want: "https://example.com/skills/rust/SKILL.md#sha256=abc123", + }, + { + name: "invalid parent URL", + parentURL: "://bad-url", + relRef: "../sibling.md", + wantErr: true, + }, + { + name: "invalid relRef percent-encoding", + parentURL: "https://example.com/skills/rust/SKILL.md", + relRef: "%xy/invalid.md", + wantErr: true, + }, + { + name: "empty relRef resolves to parent URL", + parentURL: "https://example.com/skills/rust/SKILL.md", + relRef: "", + want: "https://example.com/skills/rust/SKILL.md", + }, + { + name: "empty parentURL with relative ref", + parentURL: "", + relRef: "other/SKILL.md", + want: "/other/SKILL.md", + }, + { + name: "parent URL with no path component", + parentURL: "https://example.com", + relRef: "../foo", + want: "https://example.com/foo", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ResolveRelativeURL(tt.parentURL, tt.relRef) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/internal/resolve/resolve.go b/internal/resolve/resolve.go index 1174287321..81b941e868 100644 --- a/internal/resolve/resolve.go +++ b/internal/resolve/resolve.go @@ -4,10 +4,17 @@ import ( "context" "fmt" "path/filepath" + "strings" "time" "github.com/fullsend-ai/fullsend/internal/fetch" "github.com/fullsend-ai/fullsend/internal/harness" + "github.com/fullsend-ai/fullsend/internal/skill" +) + +const ( + DefaultMaxDepth = 10 + DefaultMaxResources = 50 ) // Dependency records a single URL that was resolved to a local cache path. @@ -25,57 +32,153 @@ type ResolveOpts struct { FetchPolicy fetch.FetchPolicy TraceID string AuditLogPath string + + // MaxDepth controls transitive dependency resolution depth. + // 0 disables transitive resolution (Phase 1 behavior). + // <0 uses DefaultMaxDepth (10). + // + // MaxResources uses different semantics: <=0 always uses + // DefaultMaxResources (50). The asymmetry exists because MaxDepth=0 + // is a meaningful "disable" value, while MaxResources=0 ("allow zero + // resources") would prevent even non-transitive URL resolution. + MaxDepth int + MaxResources int +} + +type resolveState struct { + inProgress map[string]bool + resolved map[string]Dependency + inDeps map[string]bool + resourceCount int + deps []Dependency + maxDepth int + maxResources int } // ResolveHarness resolves URL-referenced declarative fields (Agent, Policy, // Skills) in the harness to local cache paths. Local paths are left unchanged. -// The harness is modified in place. Returns the list of resolved dependencies. +// The harness is modified in place: URL fields are replaced with cache paths, +// and h.Skills may grow to include transitively resolved skill dependencies. +// Returns the deduplicated list of resolved dependencies. +// +// Skills with dependencies: frontmatter are recursively resolved up to +// MaxDepth levels. Diamond dependencies are deduplicated; cycles are rejected. +// Set MaxDepth to 0 to disable transitive resolution. Negative values use +// DefaultMaxDepth (10). // -// Phase 1: single-level resolution only (no transitive deps). +// Trusting a skill means trusting its entire transitive dependency closure: +// a skill's frontmatter can declare relative references that resolve to +// different paths on the same allowed domain. All transitive deps are still +// validated against allowed_remote_resources and SHA256 integrity hashes. +// +// The default limits (depth=10, resources=50) bound worst-case resolution. +// CI environments with untrusted harnesses should set tighter limits. func ResolveHarness(ctx context.Context, h *harness.Harness, opts ResolveOpts) ([]Dependency, error) { - var deps []Dependency + maxDepth := opts.MaxDepth + if maxDepth < 0 { + maxDepth = DefaultMaxDepth + } + maxResources := opts.MaxResources + if maxResources <= 0 { + maxResources = DefaultMaxResources + } + + state := &resolveState{ + inProgress: make(map[string]bool), + resolved: make(map[string]Dependency), + inDeps: make(map[string]bool), + maxDepth: maxDepth, + maxResources: maxResources, + } + + recurse := maxDepth > 0 if h.Agent != "" && harness.IsURL(h.Agent) { - dep, localPath, err := resolveURL(ctx, "agent", h.Agent, h, opts) + dep, localPath, err := resolveURL(ctx, "agent", h.Agent, h, opts, state, false, 0) if err != nil { return nil, fmt.Errorf("resolving agent: %w", err) } h.Agent = localPath - deps = append(deps, dep) + state.appendDependency(dep) } if h.Policy != "" && harness.IsURL(h.Policy) { - dep, localPath, err := resolveURL(ctx, "policy", h.Policy, h, opts) + dep, localPath, err := resolveURL(ctx, "policy", h.Policy, h, opts, state, false, 0) if err != nil { return nil, fmt.Errorf("resolving policy: %w", err) } h.Policy = localPath - deps = append(deps, dep) + state.appendDependency(dep) } for i, s := range h.Skills { if harness.IsURL(s) { - dep, localPath, err := resolveURL(ctx, fmt.Sprintf("skills[%d]", i), s, h, opts) + dep, localPath, err := resolveURL(ctx, fmt.Sprintf("skills[%d]", i), s, h, opts, state, recurse, 0) if err != nil { return nil, fmt.Errorf("resolving skills[%d]: %w", i, err) } - h.Skills[i] = localPath - deps = append(deps, dep) + if !state.inDeps[dep.URL] { + h.Skills[i] = localPath + } else { + h.Skills[i] = "" + } + state.appendDependency(dep) } } - return deps, nil + // Remove entries that were already appended transitively. + filtered := h.Skills[:0] + for _, s := range h.Skills { + if s != "" { + filtered = append(filtered, s) + } + } + h.Skills = filtered + + return state.deps, nil +} + +func (s *resolveState) appendDependency(dep Dependency) { + if s.inDeps[dep.URL] { + return + } + s.inDeps[dep.URL] = true + s.deps = append(s.deps, dep) } -func resolveURL(ctx context.Context, field, rawURL string, h *harness.Harness, opts ResolveOpts) (Dependency, string, error) { +func resolveURL(ctx context.Context, field, rawURL string, h *harness.Harness, + opts ResolveOpts, state *resolveState, recurse bool, depth int, +) (Dependency, string, error) { cleanURL, expectedHash, hasHash := harness.ParseIntegrityHash(rawURL) if !hasHash { - return Dependency{}, "", fmt.Errorf("%s URL must include #sha256=... integrity hash", field) + return Dependency{}, "", fmt.Errorf("%s: URL must include #sha256=... integrity hash", field) + } + if !strings.HasPrefix(cleanURL, "https://") { + return Dependency{}, "", fmt.Errorf("%s: URL scheme must be https: %s", field, cleanURL) + } + + if dep, ok := state.resolved[cleanURL]; ok { + if dep.SHA256 != expectedHash { + return Dependency{}, "", fmt.Errorf( + "%s: URL %s has conflicting integrity hashes: previously resolved with %s, now referenced with %s", + field, cleanURL, dep.SHA256, expectedHash) + } + return dep, dep.LocalPath, nil + } + if state.inProgress[cleanURL] { + return Dependency{}, "", fmt.Errorf("%s: circular dependency detected for %s", field, cleanURL) } + if state.resourceCount >= state.maxResources { + return Dependency{}, "", fmt.Errorf("%s: exceeded maximum resource count of %d for %s", field, state.maxResources, cleanURL) + } + + state.inProgress[cleanURL] = true + defer delete(state.inProgress, cleanURL) + state.resourceCount++ allowedBy := h.MatchingAllowedPrefix(cleanURL) if allowedBy == "" { - return Dependency{}, "", fmt.Errorf("%s URL %q is not in allowed_remote_resources", field, cleanURL) + return Dependency{}, "", fmt.Errorf("%s: URL %q is not in allowed_remote_resources", field, cleanURL) } content, entry, err := fetch.CacheGet(opts.WorkspaceRoot, expectedHash) @@ -93,7 +196,7 @@ func resolveURL(ctx context.Context, field, rawURL string, h *harness.Harness, o actualHash := fetch.ComputeSHA256(content) if actualHash != expectedHash { - return Dependency{}, "", fmt.Errorf("%s integrity check failed: expected %s, got %s", field, expectedHash, actualHash) + return Dependency{}, "", fmt.Errorf("%s: integrity check failed for %s: expected %s, got %s", field, cleanURL, expectedHash, actualHash) } if err := fetch.CachePut(opts.WorkspaceRoot, cleanURL, content); err != nil { @@ -126,11 +229,75 @@ func resolveURL(ctx context.Context, field, rawURL string, h *harness.Harness, o } } - return Dependency{ + if recurse { + if err := resolveTransitiveDeps(ctx, cleanURL, content, h, opts, state, depth+1); err != nil { + return Dependency{}, "", fmt.Errorf("resolving transitive deps for %s (%s): %w", field, cleanURL, err) + } + } + + dep := Dependency{ URL: cleanURL, LocalPath: localPath, SHA256: expectedHash, FetchedAt: fetchedAt, CacheHit: cacheHit, - }, localPath, nil + } + + state.resolved[cleanURL] = dep + + return dep, localPath, nil +} + +// resolveTransitiveDeps parses skill frontmatter and recursively resolves +// declared dependencies. Policy references are fetched as leaf nodes. +// depth is the current nesting level (1 for first-level transitive deps). +func resolveTransitiveDeps(ctx context.Context, parentURL string, content []byte, + h *harness.Harness, opts ResolveOpts, state *resolveState, depth int, +) error { + meta, err := skill.ParseFrontmatter(content) + if err != nil { + return fmt.Errorf("%s: %w", parentURL, err) + } + if meta == nil || (len(meta.Dependencies) == 0 && meta.Policy == "") { + return nil + } + + if depth > state.maxDepth { + return fmt.Errorf("exceeded maximum dependency depth of %d for %s", state.maxDepth, parentURL) + } + + for i, ref := range meta.Dependencies { + resolved, err := ResolveRelativeURL(parentURL, ref) + if err != nil { + return fmt.Errorf("resolving dependency ref %q from %s: %w", ref, parentURL, err) + } + + field := fmt.Sprintf("skills[%s:dep%d]", parentURL, i) + dep, localPath, err := resolveURL(ctx, field, resolved, h, opts, state, true, depth) + if err != nil { + return err + } + + if !state.inDeps[dep.URL] { + h.Skills = append(h.Skills, localPath) + } + state.appendDependency(dep) + } + + if meta.Policy != "" { + resolved, err := ResolveRelativeURL(parentURL, meta.Policy) + if err != nil { + return fmt.Errorf("resolving policy ref %q from %s: %w", meta.Policy, parentURL, err) + } + + field := fmt.Sprintf("policy[%s]", parentURL) + dep, _, err := resolveURL(ctx, field, resolved, h, opts, state, false, depth) + if err != nil { + return err + } + + state.appendDependency(dep) + } + + return nil } diff --git a/internal/resolve/resolve_test.go b/internal/resolve/resolve_test.go index e05a74ef9c..5c0fc64948 100644 --- a/internal/resolve/resolve_test.go +++ b/internal/resolve/resolve_test.go @@ -392,3 +392,590 @@ func TestResolveHarness_EmptyFields(t *testing.T) { require.NoError(t, err) assert.Empty(t, deps) } + +// skillFrontmatter returns SKILL.md content with the given YAML frontmatter fields +// and optional body text after the closing delimiter. +func skillFrontmatter(fields, body string) []byte { + return []byte("---\n" + fields + "---\n" + body) +} + +// TestResolveHarness_TransitiveChain verifies A→B→C transitive resolution: +// all three dependencies are fetched and added to h.Skills. +func TestResolveHarness_TransitiveChain(t *testing.T) { + cContent := []byte("Skill C content — leaf node") + cHash := fetch.ComputeSHA256(cContent) + + var bContent, aContent []byte + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + case "/skills/b.md": + w.Write(bContent) + case "/skills/c.md": + w.Write(cContent) + } + })) + + cURL := fmt.Sprintf("%s/skills/c.md#sha256=%s", srv.URL, cHash) + bContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", cURL), "Skill B content") + bHash := fetch.ComputeSHA256(bContent) + + bURL := fmt.Sprintf("%s/skills/b.md#sha256=%s", srv.URL, bHash) + aContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", bURL), "Skill A content") + aHash := fetch.ComputeSHA256(aContent) + + aURL := fmt.Sprintf("%s/skills/a.md#sha256=%s", srv.URL, aHash) + h := &harness.Harness{ + Skills: []string{aURL}, + AllowedRemoteResources: []string{srv.URL + "/"}, + } + + deps, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: -1, + }) + require.NoError(t, err) + assert.Len(t, deps, 3) + assert.Len(t, h.Skills, 3) + + urls := make(map[string]bool) + for _, d := range deps { + urls[d.URL] = true + } + assert.True(t, urls[srv.URL+"/skills/a.md"]) + assert.True(t, urls[srv.URL+"/skills/b.md"]) + assert.True(t, urls[srv.URL+"/skills/c.md"]) +} + +// TestResolveHarness_DiamondDedup verifies that a diamond graph (A→C, B→C) resolves C +// exactly once and produces no duplicate entries in deps or h.Skills. +func TestResolveHarness_DiamondDedup(t *testing.T) { + cContent := []byte("Skill C content — shared dep") + cHash := fetch.ComputeSHA256(cContent) + + var aContent, bContent []byte + var fetchCount atomic.Int32 + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + case "/skills/b.md": + w.Write(bContent) + case "/skills/c.md": + fetchCount.Add(1) + w.Write(cContent) + } + })) + + cURL := fmt.Sprintf("%s/skills/c.md#sha256=%s", srv.URL, cHash) + aContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", cURL), "Skill A") + aHash := fetch.ComputeSHA256(aContent) + bContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", cURL), "Skill B") + bHash := fetch.ComputeSHA256(bContent) + + h := &harness.Harness{ + Skills: []string{ + fmt.Sprintf("%s/skills/a.md#sha256=%s", srv.URL, aHash), + fmt.Sprintf("%s/skills/b.md#sha256=%s", srv.URL, bHash), + }, + AllowedRemoteResources: []string{srv.URL + "/"}, + } + + deps, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: -1, + }) + require.NoError(t, err) + assert.Len(t, deps, 3) // C, A, B — each exactly once + assert.Len(t, h.Skills, 3) + assert.Equal(t, int32(1), fetchCount.Load()) // C fetched only once + + urls := make(map[string]bool) + for _, d := range deps { + assert.False(t, urls[d.URL], "duplicate dep URL %s", d.URL) + urls[d.URL] = true + } +} + +// TestResolveHarness_CycleDetection verifies that A→B→A is rejected with a cycle error. +// The cycle is detected via the inProgress DFS stack before any hash check on the repeat visit. +func TestResolveHarness_CycleDetection(t *testing.T) { + // Use a placeholder hash for A in B's dep — cycle is detected before integrity check. + placeholderHash := strings.Repeat("a", 64) + + var aContent, bContent []byte + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + case "/skills/b.md": + w.Write(bContent) + } + })) + + aURL := fmt.Sprintf("%s/skills/a.md", srv.URL) + + // B references A with a placeholder hash; cycle fires before hash validation. + bContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s#sha256=%s\n", aURL, placeholderHash), "Skill B") + bHash := fetch.ComputeSHA256(bContent) + + bURL := fmt.Sprintf("%s/skills/b.md#sha256=%s", srv.URL, bHash) + aContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", bURL), "Skill A") + aHash := fetch.ComputeSHA256(aContent) + + h := &harness.Harness{ + Skills: []string{fmt.Sprintf("%s#sha256=%s", aURL, aHash)}, + AllowedRemoteResources: []string{srv.URL + "/"}, + } + + _, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: -1, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "circular dependency") +} + +// TestResolveHarness_MaxDepthExceeded verifies that a chain A→B→C fails when MaxDepth=1, +// allowing one level of transitive resolution (B) but blocking the second (C). +func TestResolveHarness_MaxDepthExceeded(t *testing.T) { + cContent := []byte("Skill C — should not be reached") + cHash := fetch.ComputeSHA256(cContent) + + var aContent, bContent []byte + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + case "/skills/b.md": + w.Write(bContent) + case "/skills/c.md": + w.Write(cContent) + } + })) + + cURL := fmt.Sprintf("%s/skills/c.md#sha256=%s", srv.URL, cHash) + bContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", cURL), "Skill B") + bHash := fetch.ComputeSHA256(bContent) + + bURL := fmt.Sprintf("%s/skills/b.md#sha256=%s", srv.URL, bHash) + aContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", bURL), "Skill A") + aHash := fetch.ComputeSHA256(aContent) + + h := &harness.Harness{ + Skills: []string{fmt.Sprintf("%s/skills/a.md#sha256=%s", srv.URL, aHash)}, + AllowedRemoteResources: []string{srv.URL + "/"}, + } + + _, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: 1, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeded maximum dependency depth") +} + +// TestResolveHarness_MaxResourcesExceeded verifies that resolution stops when the +// resource count reaches MaxResources, returning an error on the next fetch attempt. +func TestResolveHarness_MaxResourcesExceeded(t *testing.T) { + bContent := []byte("Skill B content") + bHash := fetch.ComputeSHA256(bContent) + + var aContent []byte + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + case "/skills/b.md": + w.Write(bContent) + } + })) + + bURL := fmt.Sprintf("%s/skills/b.md#sha256=%s", srv.URL, bHash) + aContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", bURL), "Skill A") + aHash := fetch.ComputeSHA256(aContent) + + h := &harness.Harness{ + Skills: []string{fmt.Sprintf("%s/skills/a.md#sha256=%s", srv.URL, aHash)}, + AllowedRemoteResources: []string{srv.URL + "/"}, + } + + // MaxResources=1: A consumes the single slot; B is rejected. + _, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: -1, + MaxResources: 1, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeded maximum resource count") +} + +// TestResolveHarness_TransitiveNotInAllowlist verifies that a transitive dep whose +// URL does not match allowed_remote_resources is rejected. +func TestResolveHarness_TransitiveNotInAllowlist(t *testing.T) { + bContent := []byte("Skill B content") + bHash := fetch.ComputeSHA256(bContent) + + var aContent []byte + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + case "/skills/b.md": + w.Write(bContent) + } + })) + + bURL := fmt.Sprintf("%s/skills/b.md#sha256=%s", srv.URL, bHash) + aContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", bURL), "Skill A") + aHash := fetch.ComputeSHA256(aContent) + + h := &harness.Harness{ + Skills: []string{fmt.Sprintf("%s/skills/a.md#sha256=%s", srv.URL, aHash)}, + // Only /skills/a.md is allowed; /skills/b.md (the transitive dep) is not. + AllowedRemoteResources: []string{srv.URL + "/skills/a.md"}, + } + + _, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: -1, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "not in allowed_remote_resources") +} + +// TestResolveHarness_TransitiveHashMismatch verifies that a transitive dep whose +// fetched content does not match the declared SHA256 hash is rejected. +func TestResolveHarness_TransitiveHashMismatch(t *testing.T) { + // Declare B with the hash of "expected content" but serve "tampered content". + expectedBContent := []byte("expected B content") + bHash := fetch.ComputeSHA256(expectedBContent) + + var aContent []byte + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + case "/skills/b.md": + w.Write([]byte("tampered B content")) + } + })) + + bURL := fmt.Sprintf("%s/skills/b.md#sha256=%s", srv.URL, bHash) + aContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", bURL), "Skill A") + aHash := fetch.ComputeSHA256(aContent) + + h := &harness.Harness{ + Skills: []string{fmt.Sprintf("%s/skills/a.md#sha256=%s", srv.URL, aHash)}, + AllowedRemoteResources: []string{srv.URL + "/"}, + } + + _, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: -1, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "integrity check failed") +} + +// TestResolveHarness_TransitiveRelativeURL verifies that a relative dependency reference +// in skill frontmatter is resolved against the parent skill's URL via RFC 3986. +func TestResolveHarness_TransitiveRelativeURL(t *testing.T) { + bContent := []byte("Skill B — resolved via relative URL") + bHash := fetch.ComputeSHA256(bContent) + + var aContent []byte + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + case "/common/b.md": + w.Write(bContent) + } + })) + + // A is at /skills/a.md; the relative dep "../common/b.md" resolves to /common/b.md. + relDep := fmt.Sprintf("../common/b.md#sha256=%s", bHash) + aContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", relDep), "Skill A") + aHash := fetch.ComputeSHA256(aContent) + + h := &harness.Harness{ + Skills: []string{fmt.Sprintf("%s/skills/a.md#sha256=%s", srv.URL, aHash)}, + AllowedRemoteResources: []string{srv.URL + "/"}, + } + + deps, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: -1, + }) + require.NoError(t, err) + assert.Len(t, deps, 2) + + urls := make(map[string]bool) + for _, d := range deps { + urls[d.URL] = true + } + assert.True(t, urls[srv.URL+"/common/b.md"], "relative URL should resolve to /common/b.md") +} + +// TestResolveHarness_ConflictingHashesForSameURL verifies that two skills declaring the +// same transitive dep URL with different SHA256 hashes is rejected. +func TestResolveHarness_ConflictingHashesForSameURL(t *testing.T) { + dContent := []byte("Skill D content") + dHash := fetch.ComputeSHA256(dContent) + fakeHash := strings.Repeat("b", 64) + + var aContent, bContent []byte + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + case "/skills/b.md": + w.Write(bContent) + case "/skills/d.md": + w.Write(dContent) + } + })) + + dURL := srv.URL + "/skills/d.md" + aContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s#sha256=%s\n", dURL, dHash), "Skill A") + aHash := fetch.ComputeSHA256(aContent) + bContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s#sha256=%s\n", dURL, fakeHash), "Skill B") + bHash := fetch.ComputeSHA256(bContent) + + h := &harness.Harness{ + Skills: []string{ + fmt.Sprintf("%s/skills/a.md#sha256=%s", srv.URL, aHash), + fmt.Sprintf("%s/skills/b.md#sha256=%s", srv.URL, bHash), + }, + AllowedRemoteResources: []string{srv.URL + "/"}, + } + + _, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: -1, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "conflicting integrity hashes") +} + +// TestResolveHarness_SkillPolicyLeafNode verifies that a skill-level policy reference +// is fetched and recorded in deps but is NOT appended to h.Skills. +func TestResolveHarness_SkillPolicyLeafNode(t *testing.T) { + policyContent := []byte("sandbox: strict") + policyHash := fetch.ComputeSHA256(policyContent) + + var aContent []byte + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + case "/policies/sandbox.yaml": + w.Write(policyContent) + } + })) + + policyURL := fmt.Sprintf("%s/policies/sandbox.yaml#sha256=%s", srv.URL, policyHash) + aContent = skillFrontmatter(fmt.Sprintf("policy: %s\n", policyURL), "Skill A content") + aHash := fetch.ComputeSHA256(aContent) + + h := &harness.Harness{ + Skills: []string{fmt.Sprintf("%s/skills/a.md#sha256=%s", srv.URL, aHash)}, + AllowedRemoteResources: []string{srv.URL + "/"}, + } + + deps, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: -1, + }) + require.NoError(t, err) + assert.Len(t, deps, 2) // skill A + its policy + assert.Len(t, h.Skills, 1) // policy is NOT added to h.Skills + + depURLs := make(map[string]bool) + for _, d := range deps { + depURLs[d.URL] = true + } + assert.True(t, depURLs[srv.URL+"/policies/sandbox.yaml"], "policy should be in deps") + + for _, s := range h.Skills { + assert.NotContains(t, s, "sandbox.yaml", "policy path must not appear in h.Skills") + } +} + +// TestResolveHarness_ZeroMaxDepthDisablesTransitive verifies that MaxDepth=0 prevents +// any transitive dependency resolution even when skills declare dependencies. +func TestResolveHarness_ZeroMaxDepthDisablesTransitive(t *testing.T) { + bContent := []byte("Skill B — must not be fetched") + bHash := fetch.ComputeSHA256(bContent) + + var aContent []byte + var bFetched atomic.Int32 + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + case "/skills/b.md": + bFetched.Add(1) + w.Write(bContent) + } + })) + + bURL := fmt.Sprintf("%s/skills/b.md#sha256=%s", srv.URL, bHash) + aContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", bURL), "Skill A") + aHash := fetch.ComputeSHA256(aContent) + + h := &harness.Harness{ + Skills: []string{fmt.Sprintf("%s/skills/a.md#sha256=%s", srv.URL, aHash)}, + AllowedRemoteResources: []string{srv.URL + "/"}, + } + + deps, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: 0, // disabled + }) + require.NoError(t, err) + assert.Len(t, deps, 1) // only A + assert.Len(t, h.Skills, 1) // only A + assert.Equal(t, int32(0), bFetched.Load()) // B never fetched +} + +// TestResolveHarness_MaxDepthDefaultApplied verifies that MaxDepth<0 uses DefaultMaxDepth +// and enables transitive resolution. +func TestResolveHarness_MaxDepthDefaultApplied(t *testing.T) { + bContent := []byte("Skill B content") + bHash := fetch.ComputeSHA256(bContent) + + var aContent []byte + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + case "/skills/b.md": + w.Write(bContent) + } + })) + + bURL := fmt.Sprintf("%s/skills/b.md#sha256=%s", srv.URL, bHash) + aContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", bURL), "Skill A") + aHash := fetch.ComputeSHA256(aContent) + + h := &harness.Harness{ + Skills: []string{fmt.Sprintf("%s/skills/a.md#sha256=%s", srv.URL, aHash)}, + AllowedRemoteResources: []string{srv.URL + "/"}, + } + + deps, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: -1, // uses DefaultMaxDepth + }) + require.NoError(t, err) + assert.Len(t, deps, 2) // A and B both resolved +} + +// TestResolveHarness_NonHTTPSSchemeRejected verifies that resolveURL rejects URLs whose +// scheme is not https, providing a defense-in-depth check for transitive deps from frontmatter +// that bypass the harness.IsURL guard applied to direct harness fields. +func TestResolveHarness_NonHTTPSSchemeRejected(t *testing.T) { + bContent := []byte("Skill B content") + bHash := fetch.ComputeSHA256(bContent) + + var aContent []byte + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + } + })) + + // Embed an http:// (non-HTTPS) transitive dep in A's frontmatter. + httpDepURL := fmt.Sprintf("http://example.com/skills/b.md#sha256=%s", bHash) + aContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", httpDepURL), "Skill A") + aHash := fetch.ComputeSHA256(aContent) + + h := &harness.Harness{ + Skills: []string{fmt.Sprintf("%s/skills/a.md#sha256=%s", srv.URL, aHash)}, + AllowedRemoteResources: []string{srv.URL + "/", "http://example.com/"}, + } + + _, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: -1, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "scheme must be https") +} + +// TestResolveHarness_DirectAndTransitiveOverlap verifies that a skill appearing both as a +// direct harness skill and as a transitive dep of another skill is deduplicated in h.Skills. +func TestResolveHarness_DirectAndTransitiveOverlap(t *testing.T) { + bContent := []byte("Skill B — shared skill") + bHash := fetch.ComputeSHA256(bContent) + + var aContent []byte + + srv, policy := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/skills/a.md": + w.Write(aContent) + case "/skills/b.md": + w.Write(bContent) + } + })) + + bURL := fmt.Sprintf("%s/skills/b.md#sha256=%s", srv.URL, bHash) + aContent = skillFrontmatter(fmt.Sprintf("dependencies:\n - %s\n", bURL), "Skill A") + aHash := fetch.ComputeSHA256(aContent) + + // Both A and B are direct harness skills; A also depends on B transitively. + h := &harness.Harness{ + Skills: []string{ + fmt.Sprintf("%s/skills/a.md#sha256=%s", srv.URL, aHash), + bURL, + }, + AllowedRemoteResources: []string{srv.URL + "/"}, + } + + deps, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: policy, + MaxDepth: -1, + }) + require.NoError(t, err) + assert.Len(t, deps, 2) // A and B, each exactly once + assert.Len(t, h.Skills, 2) // A's path and B's path, B deduped + + // B must not appear twice in h.Skills. + seen := make(map[string]bool) + for _, s := range h.Skills { + assert.False(t, seen[s], "h.Skills contains duplicate entry %s", s) + seen[s] = true + } +} diff --git a/internal/skill/skill.go b/internal/skill/skill.go index 138904da1b..2db8a48a84 100644 --- a/internal/skill/skill.go +++ b/internal/skill/skill.go @@ -13,6 +13,7 @@ type SkillMeta struct { Name string `yaml:"name"` Description string `yaml:"description,omitempty"` Dependencies []string `yaml:"dependencies,omitempty"` + Policy string `yaml:"policy,omitempty"` } var ( diff --git a/internal/skill/skill_test.go b/internal/skill/skill_test.go index 6780fb3fed..8939c0a97a 100644 --- a/internal/skill/skill_test.go +++ b/internal/skill/skill_test.go @@ -289,3 +289,27 @@ func TestParseFrontmatter_WhitespacePaddedClosingDelimiter(t *testing.T) { t.Errorf("name = %q, want %q", meta.Name, "padded-close") } } + +func TestParseFrontmatter_WithPolicy(t *testing.T) { + content := []byte(`--- +name: rust-conventions +dependencies: + - ../common/cargo-integration/SKILL.md +policy: policies/rust-sandbox.yaml#sha256=bbb222 +--- +# Rust Conventions +`) + meta, err := ParseFrontmatter(content) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if meta == nil { + t.Fatal("expected non-nil meta") + } + if meta.Policy != "policies/rust-sandbox.yaml#sha256=bbb222" { + t.Errorf("policy = %q, want %q", meta.Policy, "policies/rust-sandbox.yaml#sha256=bbb222") + } + if len(meta.Dependencies) != 1 { + t.Fatalf("dependencies length = %d, want 1", len(meta.Dependencies)) + } +}