diff --git a/internal/providers/claude/settings.go b/internal/providers/claude/settings.go index 4c18c1e4..e0be5c3a 100644 --- a/internal/providers/claude/settings.go +++ b/internal/providers/claude/settings.go @@ -156,25 +156,29 @@ func LoadSettings(path string) (*Settings, error) { return nil, err } - // Normalize GitHub "repo" format to git URLs. - // Claude Code's native settings.json uses {"source": "github", "repo": "owner/repo"} - // but the rest of moat expects {"source": "git", "url": "https://..."}. - // Claude Code accepts both formats, so the normalized output is safe to write back. + // Validate marketplace entries but preserve their original source shape. + // Claude Code's strictKnownMarketplaces (in remote-settings.json) matches by + // exact {source, repo|url} shape, so {source: github, repo: X} and + // {source: git, url: https://github.com/X.git} are NOT interchangeable — + // emitting a different shape than the user (or host) registered with breaks + // allowlist matching even when both forms refer to the same repository. + // Drop entries with no usable source identity or invalid repo format. for name, entry := range settings.ExtraKnownMarketplaces { - if entry.Source.Source == "github" && entry.Source.URL == "" { + switch entry.Source.Source { + case "github": if entry.Source.Repo == "" { - log.Debug("removing marketplace with empty repo and url from settings", "name", name) + log.Debug("removing github marketplace with empty repo from settings", "name", name) delete(settings.ExtraKnownMarketplaces, name) - } else if validRepoFormat.MatchString(entry.Source.Repo) { - entry.Source.URL = "https://github.com/" + entry.Source.Repo + ".git" - entry.Source.Source = "git" - entry.Source.Repo = "" - settings.ExtraKnownMarketplaces[name] = entry - } else { + } else if !validRepoFormat.MatchString(entry.Source.Repo) { log.Debug("removing marketplace with invalid repo format from settings", "name", name, "repo", entry.Source.Repo) delete(settings.ExtraKnownMarketplaces, name) } + case "git": + if entry.Source.URL == "" { + log.Debug("removing git marketplace with empty url from settings", "name", name) + delete(settings.ExtraKnownMarketplaces, name) + } } } @@ -214,10 +218,10 @@ type KnownMarketplaceSource struct { // This file contains marketplace URLs that Claude Code has registered via // `claude plugin marketplace add`. Returns nil, nil if the file doesn't exist. // -// URL normalization: -// - "github" sources are normalized to git URLs (https://github.com/owner/repo.git) -// - We assume repos don't contain trailing slashes or .git suffixes (Claude CLI standard) -// - Git URLs are used as-is without normalization +// The original source shape is preserved: a "github" source keeps its repo +// field, a "git" source keeps its url field. Strict marketplace allowlists +// (strictKnownMarketplaces in remote-settings.json) match by exact source +// shape, so converting between forms breaks allowlist matching. // // Entries are skipped (with debug logging) if they have: // - Empty repo/URL fields @@ -237,7 +241,7 @@ func LoadKnownMarketplaces(path string) (map[string]MarketplaceEntry, error) { return nil, err } - // Convert to our MarketplaceEntry format + // Convert to our MarketplaceEntry format, preserving the original source shape. result := make(map[string]MarketplaceEntry) for name, km := range raw { entry := MarketplaceEntry{ @@ -246,27 +250,30 @@ func LoadKnownMarketplaces(path string) (map[string]MarketplaceEntry, error) { }, } - // Convert source to git URL format switch km.Source.Source { case "github": - // Validate repo format before URL construction (defense-in-depth) - if km.Source.Repo != "" && validRepoFormat.MatchString(km.Source.Repo) { - entry.Source.Source = "git" - entry.Source.URL = "https://github.com/" + km.Source.Repo + ".git" - } else if km.Source.Repo != "" { + if km.Source.Repo == "" { + log.Debug("skipping github marketplace with empty repo", "name", name) + continue + } + if !validRepoFormat.MatchString(km.Source.Repo) { log.Debug("skipping marketplace with invalid repo format", "name", name, "repo", km.Source.Repo) continue } + entry.Source.Repo = km.Source.Repo case "git": + if km.Source.URL == "" { + log.Debug("skipping git marketplace with empty URL", "name", name) + continue + } entry.Source.URL = km.Source.URL + default: + log.Debug("skipping marketplace with unknown source", "name", name, "source", km.Source.Source) + continue } - if entry.Source.URL != "" { - result[name] = entry - } else { - log.Debug("skipping marketplace with empty URL", "name", name) - } + result[name] = entry } return result, nil @@ -474,11 +481,12 @@ func ConfigToSettings(cfg *config.Config) *Settings { }, } + // Preserve the source shape the user wrote in moat.yaml so + // strictKnownMarketplaces allowlist matching works (it compares + // {source, repo|url} as exact pairs, not by canonical URL). switch spec.Source { case "github": - // Convert github owner/repo to git URL - entry.Source.Source = "git" - entry.Source.URL = "https://github.com/" + spec.Repo + ".git" + entry.Source.Repo = spec.Repo case "git": entry.Source.URL = spec.URL case "directory": diff --git a/internal/providers/claude/settings_test.go b/internal/providers/claude/settings_test.go index e9363a0f..6a34bab0 100644 --- a/internal/providers/claude/settings_test.go +++ b/internal/providers/claude/settings_test.go @@ -60,8 +60,10 @@ func TestLoadSettings(t *testing.T) { } func TestLoadSettingsGitHubRepoFormat(t *testing.T) { - // Claude Code's native settings.json uses "repo" for GitHub marketplaces, - // not "url". LoadSettings must handle this format. + // Claude Code's native settings.json uses {source: github, repo: owner/repo}. + // LoadSettings must preserve that shape so strictKnownMarketplaces allowlist + // matching (which compares source/repo and source/url as exact pairs) still + // works inside the moat container. dir := t.TempDir() settingsPath := filepath.Join(dir, "settings.json") @@ -106,28 +108,26 @@ func TestLoadSettingsGitHubRepoFormat(t *testing.T) { t.Fatalf("ExtraKnownMarketplaces = %d, want 3", len(settings.ExtraKnownMarketplaces)) } - // GitHub "repo" format should be normalized to git URL + // GitHub "repo" format must be preserved verbatim. superpowers := settings.ExtraKnownMarketplaces["superpowers-marketplace"] - if superpowers.Source.Source != "git" { - t.Errorf("superpowers.Source.Source = %q, want %q", superpowers.Source.Source, "git") + if superpowers.Source.Source != "github" { + t.Errorf("superpowers.Source.Source = %q, want %q", superpowers.Source.Source, "github") } - if superpowers.Source.URL != "https://github.com/obra/superpowers-marketplace.git" { - t.Errorf("superpowers.Source.URL = %q, want %q", superpowers.Source.URL, "https://github.com/obra/superpowers-marketplace.git") + if superpowers.Source.Repo != "obra/superpowers-marketplace" { + t.Errorf("superpowers.Source.Repo = %q, want %q", superpowers.Source.Repo, "obra/superpowers-marketplace") + } + if superpowers.Source.URL != "" { + t.Errorf("superpowers.Source.URL should be empty for github source, got %q", superpowers.Source.URL) } gpSkills := settings.ExtraKnownMarketplaces["gp-claude-skills"] - if gpSkills.Source.URL != "https://github.com/thegpvc/gp-claude-skills.git" { - t.Errorf("gp-claude-skills.Source.URL = %q, want %q", gpSkills.Source.URL, "https://github.com/thegpvc/gp-claude-skills.git") + if gpSkills.Source.Source != "github" || gpSkills.Source.Repo != "thegpvc/gp-claude-skills" { + t.Errorf("gp-claude-skills source = %+v, want {github, thegpvc/gp-claude-skills}", gpSkills.Source) } compound := settings.ExtraKnownMarketplaces["compound-engineering-plugin"] - if compound.Source.URL != "https://github.com/EveryInc/compound-engineering-plugin.git" { - t.Errorf("compound.Source.URL = %q, want %q", compound.Source.URL, "https://github.com/EveryInc/compound-engineering-plugin.git") - } - - // Repo field should be cleared after normalization - if superpowers.Source.Repo != "" { - t.Errorf("Repo should be cleared after normalization, got %q", superpowers.Source.Repo) + if compound.Source.Source != "github" || compound.Source.Repo != "EveryInc/compound-engineering-plugin" { + t.Errorf("compound source = %+v, want {github, EveryInc/compound-engineering-plugin}", compound.Source) } // All plugins should be loaded @@ -165,7 +165,7 @@ func TestLoadSettingsInvalidRepoFormat(t *testing.T) { t.Fatalf("LoadSettings: %v", err) } - // Malicious entry should be removed, valid one should be normalized + // Malicious entry should be removed, valid one preserved as-is. if _, ok := settings.ExtraKnownMarketplaces["malicious"]; ok { t.Error("malicious marketplace should have been removed") } @@ -175,11 +175,11 @@ func TestLoadSettingsInvalidRepoFormat(t *testing.T) { } valid := settings.ExtraKnownMarketplaces["valid"] - if valid.Source.Source != "git" { - t.Errorf("valid.Source.Source = %q, want %q", valid.Source.Source, "git") + if valid.Source.Source != "github" { + t.Errorf("valid.Source.Source = %q, want %q", valid.Source.Source, "github") } - if valid.Source.URL != "https://github.com/owner/valid-repo.git" { - t.Errorf("valid.Source.URL = %q, want %q", valid.Source.URL, "https://github.com/owner/valid-repo.git") + if valid.Source.Repo != "owner/valid-repo" { + t.Errorf("valid.Source.Repo = %q, want %q", valid.Source.Repo, "owner/valid-repo") } } @@ -450,13 +450,16 @@ func TestConfigToSettings(t *testing.T) { t.Errorf("ExtraKnownMarketplaces = %d, want 3", len(settings.ExtraKnownMarketplaces)) } - // github source should be converted to git with HTTPS URL + // github source should be preserved as {source: github, repo: owner/repo} ghMarket := settings.ExtraKnownMarketplaces["github-market"] - if ghMarket.Source.Source != "git" { - t.Errorf("github-market.Source.Source = %q, want %q", ghMarket.Source.Source, "git") + if ghMarket.Source.Source != "github" { + t.Errorf("github-market.Source.Source = %q, want %q", ghMarket.Source.Source, "github") } - if ghMarket.Source.URL != "https://github.com/acme/plugins.git" { - t.Errorf("github-market.Source.URL = %q, want %q", ghMarket.Source.URL, "https://github.com/acme/plugins.git") + if ghMarket.Source.Repo != "acme/plugins" { + t.Errorf("github-market.Source.Repo = %q, want %q", ghMarket.Source.Repo, "acme/plugins") + } + if ghMarket.Source.URL != "" { + t.Errorf("github-market.Source.URL should be empty, got %q", ghMarket.Source.URL) } // git source should be preserved @@ -606,19 +609,22 @@ func TestLoadKnownMarketplaces(t *testing.T) { t.Errorf("got %d marketplaces, want 2", len(result)) } - // Check claude-plugins-official + // github sources must be preserved as {source: github, repo: owner/repo} + // so strictKnownMarketplaces matching against the host's registration form works. official := result["claude-plugins-official"] - if official.Source.Source != "git" { - t.Errorf("official.Source.Source = %q, want %q", official.Source.Source, "git") + if official.Source.Source != "github" { + t.Errorf("official.Source.Source = %q, want %q", official.Source.Source, "github") + } + if official.Source.Repo != "anthropics/claude-plugins-official" { + t.Errorf("official.Source.Repo = %q, want %q", official.Source.Repo, "anthropics/claude-plugins-official") } - if official.Source.URL != "https://github.com/anthropics/claude-plugins-official.git" { - t.Errorf("official.Source.URL = %q, want %q", official.Source.URL, "https://github.com/anthropics/claude-plugins-official.git") + if official.Source.URL != "" { + t.Errorf("official.Source.URL should be empty for github source, got %q", official.Source.URL) } - // Check aws-agent-skills aws := result["aws-agent-skills"] - if aws.Source.URL != "https://github.com/itsmostafa/aws-agent-skills.git" { - t.Errorf("aws.Source.URL = %q, want %q", aws.Source.URL, "https://github.com/itsmostafa/aws-agent-skills.git") + if aws.Source.Source != "github" || aws.Source.Repo != "itsmostafa/aws-agent-skills" { + t.Errorf("aws source = %+v, want {github, itsmostafa/aws-agent-skills}", aws.Source) } } diff --git a/internal/run/manager.go b/internal/run/manager.go index 8c7cc05d..74a7a57c 100644 --- a/internal/run/manager.go +++ b/internal/run/manager.go @@ -1627,28 +1627,35 @@ region = %s marketplaceRepos := make(map[string]string) if claudeSettings != nil && hasClaudeCode { - // Build a map of marketplace name -> repo URL from merged settings. - // The claude CLI accepts marketplace repos in several formats: - // - GitHub shorthand: owner/repo - // - HTTPS URLs: https://github.com/owner/repo.git - // - SSH URLs: git@github.com:owner/repo.git - // We normalize GitHub HTTPS URLs to owner/repo format for cleaner output. - // Other URL formats are passed through unchanged. + // Build a map of marketplace name -> repo identity from merged settings. + // MarketplaceConfig.Repo carries the value matching the source shape: + // an "owner/repo" shorthand for source "github", a full URL for source + // "git". Preserving the original shape lets GenerateKnownMarketplaces + // emit the same {source, repo|url} pair the entry was registered with, + // which matters for strictKnownMarketplaces allowlist matching (the + // allowlist compares source/repo and source/url as exact pairs). for name, entry := range claudeSettings.ExtraKnownMarketplaces { - if entry.Source.URL != "" { - // Convert GitHub HTTPS URL to owner/repo format - repo := entry.Source.URL - if strings.HasPrefix(repo, "https://github.com/") { - repo = strings.TrimPrefix(repo, "https://github.com/") - repo = strings.TrimSuffix(strings.TrimSuffix(repo, "/"), ".git") + var repo string + switch entry.Source.Source { + case "github": + if entry.Source.Repo == "" { + continue } - marketplaceRepos[name] = repo - claudeMarketplaces = append(claudeMarketplaces, claude.MarketplaceConfig{ - Name: name, - Source: entry.Source.Source, - Repo: repo, - }) + repo = entry.Source.Repo + case "git": + if entry.Source.URL == "" { + continue + } + repo = entry.Source.URL + default: + continue } + marketplaceRepos[name] = repo + claudeMarketplaces = append(claudeMarketplaces, claude.MarketplaceConfig{ + Name: name, + Source: entry.Source.Source, + Repo: repo, + }) } // Extract enabled plugins, but only those with known marketplace URLs.