feat(skills): threat scan, lockfile pinning, manifest hardening

docs/user-guide/08-skills.md

@@ -143,6 +143,22 @@ hawk skills audit                           # security-scan installed skills
 
 Once installed, skills are discovered from the locations listed above.
 
+### Install-time security and the lockfile
+
+Every install runs two scans on skill content before it is written:
+
+1. **Unicode audit** — dangerous homoglyph/injection characters are
+   stripped (reported as `sanitized`).
+2. **Threat scan** — pattern scan for command injection, data
+   exfiltration, encoded payloads, and network calls, scored 0-100.
+   Content scoring below 30 is **refused**; scores in the warning band
+   install with a printed report.
+
+Each install is recorded in a `skills-lock.json` next to the installed
+skills (user or project scope), pinning the source repo, the cloned
+commit, and the SHA-256 of the installed content. Removing a skill drops
+its lock entry.
+
 ---
 
 ## Where to Go Next

internal/plugin/manifest_hardening.go

@@ -0,0 +1,125 @@
+package plugin
+
+import (
+	"bytes"
+	"encoding/json"
+	"fmt"
+	"os"
+	"path/filepath"
+	"regexp"
+	"unicode/utf8"
+)
+
+// Hardened manifest parsing, ported from Autohand Code CLI's extension
+// loader (Apache-2.0, Copyright 2025 Autohand AI LLC): bounded file size,
+// duplicate-JSON-key rejection, and strict identity patterns. Duplicate
+// keys matter because encoding/json silently keeps the last occurrence,
+// letting a hostile manifest show reviewers one value and hawk another.
+
+const maxManifestBytes = 64 * 1024
+
+var (
+	manifestNamePattern = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9._-]{1,99}$`)
+	manifestSemver      = regexp.MustCompile(`^(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)$`)
+)
+
+// readBoundedManifestFile reads a regular, non-symlink file enforcing a
+// size cap and valid UTF-8.
+func readBoundedManifestFile(path string) ([]byte, error) {
+	info, err := os.Lstat(path) // #nosec G304 -- path is pluginDir/plugin.json under a Hawk-managed plugins root
+	if err != nil {
+		return nil, fmt.Errorf("stat manifest: %w", err)
+	}
+	if info.Mode()&os.ModeSymlink != 0 {
+		return nil, fmt.Errorf("manifest may not be a symlink: %s", filepath.Base(path))
+	}
+	if !info.Mode().IsRegular() {
+		return nil, fmt.Errorf("manifest is not a regular file: %s", filepath.Base(path))
+	}
+	if info.Size() > maxManifestBytes {
+		return nil, fmt.Errorf("manifest exceeds the %d-byte limit", maxManifestBytes)
+	}
+	data, err := os.ReadFile(path) // #nosec G304 -- see Lstat above
+	if err != nil {
+		return nil, fmt.Errorf("read manifest: %w", err)
+	}
+	if !utf8.Valid(data) {
+		return nil, fmt.Errorf("manifest is not valid UTF-8")
+	}
+	return data, nil
+}
+
+// findDuplicateJSONKey reports the first duplicated key in any JSON object
+// in data, or "" when every object has unique keys. The scanner walks the
+// token stream tracking whether each string sits in a key or value slot.
+func findDuplicateJSONKey(data []byte) string {
+	dec := json.NewDecoder(bytes.NewReader(data))
+	dec.UseNumber()
+
+	type frame struct {
+		isObject    bool
+		keys        map[string]bool
+		expectValue bool // object frames: next string is a value, not a key
+	}
+	var stack []*frame
+	top := func() *frame {
+		if len(stack) == 0 {
+			return nil
+		}
+		return stack[len(stack)-1]
+	}
+
+	for {
+		tok, err := dec.Token()
+		if err != nil {
+			return ""
+		}
+		switch t := tok.(type) {
+		case json.Delim:
+			switch t {
+			case '{':
+				stack = append(stack, &frame{isObject: true, keys: map[string]bool{}})
+			case '[':
+				stack = append(stack, &frame{})
+			default:
+				stack = stack[:len(stack)-1]
+				if p := top(); p != nil && p.isObject && p.expectValue {
+					p.expectValue = false // closed container filled the value slot
+				}
+			}
+		case string:
+			f := top()
+			if f != nil && f.isObject {
+				if !f.expectValue {
+					if f.keys[t] {
+						return t
+					}
+					f.keys[t] = true
+					f.expectValue = true
+					continue
+				}
+				f.expectValue = false
+			}
+		default:
+			// Numbers, booleans, null fill a value slot.
+			if f := top(); f != nil && f.isObject && f.expectValue {
+				f.expectValue = false
+			}
+		}
+	}
+}
+
+// ValidateManifestIdentity enforces strict name/version patterns beyond
+// the non-empty checks in ParseManifestV2.
+func ValidateManifestIdentity(name, version string) []string {
+	var issues []string
+	if name != "" && !manifestNamePattern.MatchString(name) {
+		issues = append(issues, fmt.Sprintf(
+			"name %q must start with a letter and use only letters, digits, '.', '_' or '-' (max 100 chars)", name,
+		))
+	}
+	if version != "" && !manifestSemver.MatchString(version) {
+		issues = append(issues, fmt.Sprintf("version %q must be strict semver (MAJOR.MINOR.PATCH)", version))
+	}
+	return issues
+}

internal/plugin/manifest_hardening_test.go

@@ -0,0 +1,76 @@
+package plugin
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+func TestFindDuplicateJSONKey(t *testing.T) {
+	cases := []struct {
+		name string
+		json string
+		want string
+	}{
+		{"object in array", `[{"a":1},{"b":2,"a":3}]`, ""},
+		{"clean object", `{"a":1,"b":2}`, ""},
+		{"top-level dup", `{"a":1,"a":2}`, "a"},
+		{"dup inside nested object", `{"x":{"p":"value with a: colon","q":2,"p":3}}`, "p"},
+		{"string value that looks like key", `{"k":"k","j":"k"}`, ""},
+		{"array of strings", `{"a":["a","a","b"]}`, ""},
+		{"numbers and bools", `{"n":1.5,"t":true,"f":null,"m":{"z":0,"z":1}}`, "z"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := findDuplicateJSONKey([]byte(tc.json)); got != tc.want {
+				t.Fatalf("findDuplicateJSONKey(%s) = %q, want %q", tc.json, got, tc.want)
+			}
+		})
+	}
+}
+
+func TestReadBoundedManifestFile(t *testing.T) {
+	dir := t.TempDir()
+
+	good := filepath.Join(dir, "plugin.json")
+	if err := os.WriteFile(good, []byte(`{"name":"ok","version":"1.0.0"}`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := readBoundedManifestFile(good); err != nil {
+		t.Fatalf("regular file should parse: %v", err)
+	}
+
+	big := filepath.Join(dir, "big.json")
+	if err := os.WriteFile(big, []byte(strings.Repeat("x", maxManifestBytes+1)), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := readBoundedManifestFile(big); err == nil || !strings.Contains(err.Error(), "limit") {
+		t.Fatalf("oversized file must be rejected, got %v", err)
+	}
+
+	invalid := filepath.Join(dir, "bad.json")
+	if err := os.WriteFile(invalid, []byte{0xff, 0xfe}, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := readBoundedManifestFile(invalid); err == nil || !strings.Contains(err.Error(), "UTF-8") {
+		t.Fatalf("invalid UTF-8 must be rejected, got %v", err)
+	}
+}
+
+func TestValidateManifestIdentity(t *testing.T) {
+	if issues := ValidateManifestIdentity("go-review", "1.2.3"); len(issues) != 0 {
+		t.Fatalf("valid identity flagged: %v", issues)
+	}
+	issues := ValidateManifestIdentity("1bad name", "v1.0")
+	if len(issues) != 2 {
+		t.Fatalf("expected both patterns to fail, got %v", issues)
+	}
+	if !strings.Contains(issues[0], "name") || !strings.Contains(issues[1], "semver") {
+		t.Fatalf("unexpected issue text: %v", issues)
+	}
+	// Empty strings are handled by the required-field checks elsewhere.
+	if issues := ValidateManifestIdentity("", ""); len(issues) != 0 {
+		t.Fatalf("empty identity should be skipped here, got %v", issues)
+	}
+}

internal/plugin/manifest_v2.go

@@ -70,9 +70,13 @@ type ManifestHook struct {
 // using the V2 manifest format. It is backward compatible with V1 manifests.
 func ParseManifestV2(pluginDir string) (*ManifestV2, error) {
 	path := filepath.Join(pluginDir, "plugin.json")
-	data, err := os.ReadFile(path) // #nosec G304 -- pluginDir is a locally installed plugin directory under a Hawk-managed plugins root, not raw external input
+	data, err := readBoundedManifestFile(path)
 	if err != nil {
-		return nil, fmt.Errorf("read manifest: %w", err)
+		return nil, err
+	}
+
+	if dup := findDuplicateJSONKey(data); dup != "" {
+		return nil, fmt.Errorf("parse manifest: duplicate JSON key %q", dup)
 	}
 
 	var manifest ManifestV2
@@ -146,6 +150,8 @@ func (m *ManifestV2) ValidateV2() []string {
 	var issues []string
 
 	// Basic V1 validation
+
+	issues = append(issues, ValidateManifestIdentity(m.Name, m.Version)...)
 	if m.Name == "" {
 		issues = append(issues, "name is required")
 	}

internal/plugin/manifest_v2_test.go

@@ -86,7 +86,7 @@ func TestManifestV2_IsV2(t *testing.T) {
 func TestManifestV2_ValidateV2(t *testing.T) {
 	t.Parallel()
 	valid := &ManifestV2{
-		Name: "test", Version: "1.0", Mode: "subprocess",
+		Name: "test", Version: "1.0.0", Mode: "subprocess",
 		Tools: []ManifestTool{{Name: "t", Description: "d", Command: "echo"}},
 	}
 	if err := valid.ValidateV2(); err != nil {