From aa0421aae378d6ee10d2d88a6100d4eefb11b296 Mon Sep 17 00:00:00 2001 From: j3ssie Date: Sat, 8 Aug 2026 14:27:18 +0800 Subject: [PATCH] refactor: consolidate GitHub URL matching and tidy registry-info handler Follow-up cleanup to 94ddad4, no behaviour change. - export installer.IsGitHubURL and drop pkg/cli's isGitHubURLForFetch, which was a third substring copy of the same predicate still gating an Authorization header. internal/functions keeps its own copy: that one also has to match the SSH form (git@github.com:user/repo.git), which has no parseable host. - replace the isTrustedRegistry one-line predicate with the comparison it wrapped, and extract the fallback-URL rule the two mode handlers had copy-pasted into displayRegistryURL, so registryPathOrURL is no longer overwritten mid-function with a different meaning - drop a no-op int64 conversion in the registry size check --- internal/installer/binary.go | 4 +-- internal/installer/download.go | 17 +++++++------ internal/installer/installer_test.go | 2 +- pkg/cli/run.go | 10 ++------ pkg/server/handlers/install.go | 37 ++++++++++++---------------- 5 files changed, 31 insertions(+), 39 deletions(-) diff --git a/internal/installer/binary.go b/internal/installer/binary.go index a7d4ea0..c0e8294 100644 --- a/internal/installer/binary.go +++ b/internal/installer/binary.go @@ -128,7 +128,7 @@ func fetchURL(url string, customHeaders map[string]string) ([]byte, error) { req.Header.Set("User-Agent", core.DefaultUA) // Auto-inject GitHub token for GitHub URLs (helps with rate limiting and private repos) - if isGitHubURL(url) { + if IsGitHubURL(url) { if token := getGitHubToken(); token != "" { req.Header.Set("Authorization", "Bearer "+token) } @@ -155,7 +155,7 @@ func fetchURL(url string, customHeaders map[string]string) ([]byte, error) { if err != nil { return nil, err } - if int64(len(data)) > maxRegistrySize { + if len(data) > maxRegistrySize { return nil, fmt.Errorf("registry exceeds the %d byte limit", maxRegistrySize) } return data, nil diff --git a/internal/installer/download.go b/internal/installer/download.go index c990a54..d007ae7 100644 --- a/internal/installer/download.go +++ b/internal/installer/download.go @@ -83,11 +83,14 @@ func IsZipFile(source string) bool { return st == SourceTypeLocalZip || st == SourceTypeZipURL } -// isGitHubURL checks if a URL is a GitHub URL that can benefit from authentication. -// Matches on the parsed hostname, not a substring: the caller uses this to decide -// whether to attach the GitHub token, and a lookalike host such as -// "evil.tld/?x=github.com" or "github.com.evil.tld" must never receive it. -func isGitHubURL(rawURL string) bool { +// IsGitHubURL checks if a URL is a GitHub URL that can benefit from authentication. +// Matches on the parsed hostname, not a substring: callers use this to decide whether +// to attach the GitHub token, and a lookalike host such as "evil.tld/?x=github.com" or +// "github.com.evil.tld" must never receive it. +// +// This is for http(s) URLs only. It does not recognise the SSH form +// (git@github.com:user/repo.git), which has no parseable host. +func IsGitHubURL(rawURL string) bool { u, err := url.Parse(rawURL) if err != nil { return false @@ -166,7 +169,7 @@ func downloadFileOnce(rawURL, dest string, customHeaders map[string]string) erro req.Header.Set("User-Agent", core.DefaultUA) // Auto-inject GitHub token for GitHub URLs - if isGitHubURL(rawURL) { + if IsGitHubURL(rawURL) { if token := getGitHubToken(); token != "" { req.Header.Set("Authorization", "Bearer "+token) } @@ -233,7 +236,7 @@ func downloadFileWithExternalTool(rawURL, dest string, customHeaders map[string] // Build auth header args for GitHub URLs var authHeaderWget []string var authHeaderCurl []string - if isGitHubURL(rawURL) { + if IsGitHubURL(rawURL) { if token := getGitHubToken(); token != "" { authHeaderWget = []string{"--header", "Authorization: Bearer " + token} authHeaderCurl = []string{"-H", "Authorization: Bearer " + token} diff --git a/internal/installer/installer_test.go b/internal/installer/installer_test.go index 037361a..a605bfb 100644 --- a/internal/installer/installer_test.go +++ b/internal/installer/installer_test.go @@ -129,7 +129,7 @@ func TestIsGitHubURL(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.expect, isGitHubURL(tt.url), + assert.Equal(t, tt.expect, IsGitHubURL(tt.url), "the GitHub token is attached based on this check") }) } diff --git a/pkg/cli/run.go b/pkg/cli/run.go index 6498711..d7ee23a 100644 --- a/pkg/cli/run.go +++ b/pkg/cli/run.go @@ -29,6 +29,7 @@ import ( "github.com/j3ssie/osmedeus/v5/internal/executor" "github.com/j3ssie/osmedeus/v5/internal/fileio" "github.com/j3ssie/osmedeus/v5/internal/heuristics" + "github.com/j3ssie/osmedeus/v5/internal/installer" "github.com/j3ssie/osmedeus/v5/internal/logger" "github.com/j3ssie/osmedeus/v5/internal/parser" "github.com/j3ssie/osmedeus/v5/internal/terminal" @@ -1699,7 +1700,7 @@ func fetchWorkflowFromURL(urlStr string) (*core.Workflow, error) { } // If failed and is a GitHub URL, retry with auth - if isGitHubURLForFetch(urlStr) { + if installer.IsGitHubURL(urlStr) { token := getGitHubTokenForFetch() if token != "" { log.Debug("Retrying with GitHub authentication") @@ -1805,13 +1806,6 @@ func retryableHTTPStatus(code int) bool { } } -// isGitHubURLForFetch checks if the URL is a GitHub URL -func isGitHubURLForFetch(urlStr string) bool { - return strings.Contains(urlStr, "github.com") || - strings.Contains(urlStr, "raw.githubusercontent.com") || - strings.Contains(urlStr, "api.github.com") -} - // getGitHubTokenForFetch returns the GitHub token from settings or environment // Priority: GITHUB_API_KEY (from settings) > GH_TOKEN (from OS env) func getGitHubTokenForFetch() string { diff --git a/pkg/server/handlers/install.go b/pkg/server/handlers/install.go index f5f4a09..c7c8422 100644 --- a/pkg/server/handlers/install.go +++ b/pkg/server/handlers/install.go @@ -36,13 +36,6 @@ func GetRegistryInfo(cfg *config.Config) fiber.Handler { } } -// isTrustedRegistry reports whether the registry source is one the server controls. -// An empty registry_url means the embedded registry; anything else was supplied by the -// caller and its entries must never be executed (see IsBinaryInstalledNoExec). -func isTrustedRegistry(registryPathOrURL string) bool { - return registryPathOrURL == "" -} - // registryInstalledStatus reports install status for one entry, shelling out to its // valide-command only when the registry came from a trusted source. func registryInstalledStatus(name string, entry *installer.BinaryEntry, trusted bool) bool { @@ -52,10 +45,21 @@ func registryInstalledStatus(name string, entry *installer.BinaryEntry, trusted return installer.IsBinaryInstalledNoExec(name, entry) } +// displayRegistryURL names the source to report back to the caller. An empty +// registry_url means LoadRegistry used the embedded copy of the default registry, so +// report that URL rather than an empty string. +func displayRegistryURL(registryPathOrURL string) string { + if registryPathOrURL == "" { + return installer.DefaultRegistryURL + } + return registryPathOrURL +} + // getDirectFetchRegistry returns the direct-fetch registry (existing behavior) func getDirectFetchRegistry(c *fiber.Ctx) error { registryPathOrURL := c.Query("registry_url", "") - trusted := isTrustedRegistry(registryPathOrURL) + // Anything the caller supplied is untrusted: its entries must never be executed + trusted := registryPathOrURL == "" registry, err := installer.LoadRegistry(registryPathOrURL, nil) if err != nil { @@ -65,11 +69,6 @@ func getDirectFetchRegistry(c *fiber.Ctx) error { }) } - // Using installer.DefaultRegistryURL as default for query would break LoadRegistry logic - if registryPathOrURL == "" { - registryPathOrURL = installer.DefaultRegistryURL - } - // Build response with installation status for each binary binariesWithStatus := make(map[string]BinaryStatusEntry) for name, entry := range registry { @@ -96,7 +95,7 @@ func getDirectFetchRegistry(c *fiber.Ctx) error { return c.JSON(fiber.Map{ "registry_mode": "direct-fetch", - "registry_url": registryPathOrURL, + "registry_url": displayRegistryURL(registryPathOrURL), "binaries": binariesWithStatus, }) } @@ -122,7 +121,8 @@ func getNixBuildRegistry(c *fiber.Ctx) error { // Load registry for metadata (desc, tags) - use custom registry_url if provided registryPathOrURL := c.Query("registry_url", "") - trusted := isTrustedRegistry(registryPathOrURL) + // Anything the caller supplied is untrusted: its entries must never be executed + trusted := registryPathOrURL == "" registry, err := installer.LoadRegistry(registryPathOrURL, nil) if err != nil { @@ -138,11 +138,6 @@ func getNixBuildRegistry(c *fiber.Ctx) error { registry = nil } - // Report the concrete source used, matching direct-fetch mode - if registryPathOrURL == "" { - registryPathOrURL = installer.DefaultRegistryURL - } - // Build response with categories and tool metadata categoriesData := make([]map[string]interface{}, 0) for _, cat := range categories { @@ -183,7 +178,7 @@ func getNixBuildRegistry(c *fiber.Ctx) error { return c.JSON(fiber.Map{ "registry_mode": "nix-build", - "registry_url": registryPathOrURL, + "registry_url": displayRegistryURL(registryPathOrURL), "nix_installed": installer.IsNixInstalled(), "categories": categoriesData, })