From fd9801adfe97575d17e03b530b809f5af7f93964 Mon Sep 17 00:00:00 2001 From: Jan Dubois Date: Mon, 6 Oct 2025 23:43:22 -0700 Subject: [PATCH] Allow github:ORG// repos to redirect to other repos in the same org Signed-off-by: Jan Dubois --- hack/bats/tests/url-github.bats | 102 +++++++++++++++++++++----- pkg/limatmpl/github.go | 126 +++++++++++++++++++++----------- pkg/limatmpl/locator.go | 39 +++++++--- 3 files changed, 196 insertions(+), 71 deletions(-) diff --git a/hack/bats/tests/url-github.bats b/hack/bats/tests/url-github.bats index b7b3163f9e9..6853ac8adc2 100644 --- a/hack/bats/tests/url-github.bats +++ b/hack/bats/tests/url-github.bats @@ -6,19 +6,33 @@ load "../helpers/load" # The jandubois/jandubois GitHub repo has been especially constructed to test # various features of the github URL scheme: # -# * repo defaults to org when not specified -# * filename defaults to .lima.yaml when only a path is specified -# * .yaml default extension -# * .lima.yaml files may be treated as symlinks -# * default branch lookup when not specified +# * repo defaults to org when not specified +# * filename defaults to .lima.yaml when only a path is specified +# * .yaml default extension +# * .lima.yaml files may be treated as symlinks +# * default branch lookup when not specified +# * github:ORG// repos can redirect to another github:ORG URL in the same ORG # -# The repo files are: +# The jandubois/jandubois repo files are: # -# ├── .lima.yaml -> templates/demo.yaml -# ├── docs -# │ └── .lima.yaml -> ../templates/demo.yaml -# └── templates -# └── demo.yaml +# ├── .lima.yaml -> templates/demo.yaml +# ├── back +# │ └── .lima.yaml -> github:jandubois//loop/ +# ├── docs +# │ └── .lima.yaml -> ../templates/demo.yaml +# ├── invalid +# │ ├── org +# │ │ └── .lima.yaml -> github:lima-vm +# │ └── tag +# │ └── .lima.yaml -> github:jandubois//@v0.0.0 +# ├── loop +# │ └── .lima.yaml -> github:jandubois//back/ +# ├── redirect +# │ └── .lima.yaml -> github:jandubois/lima/templates/default +# ├── templates +# │ └── demo.yaml "base: template:default" +# └── yaml +# └── .lima.yaml "{}" # # Both the `main` branch and the `v0.0.0` tag have this layout. @@ -52,7 +66,7 @@ URLS=( ) url() { - run_e "$1" limactl template url "$2" + run_e "$1" limactl --debug template url "$2" } test_jandubois_url() { @@ -71,9 +85,14 @@ for url in "${URLS[@]}"; do bats_test_function --description "$url" -- test_jandubois_url "$url" done -@test '.lima.yaml is retained when it is not a symlink' { - url -0 'github:jandubois//test/' - assert_output 'https://raw.githubusercontent.com/jandubois/jandubois/main/test/.lima.yaml' +@test '.lima.yaml is retained when it exits and is not a symlink' { + url -0 'github:jandubois//yaml/' + assert_output 'https://raw.githubusercontent.com/jandubois/jandubois/main/yaml/.lima.yaml' +} + +@test 'non-existing .lima.yaml returns an error' { + url -1 'github:jandubois//missing/' + assert_fatal 'file "https://raw.githubusercontent.com/jandubois/jandubois/main/missing/.lima.yaml" not found or inaccessible: status 404' } @test 'hidden files without an extension get a .yaml extension' { @@ -88,15 +107,62 @@ done @test 'github: URLs are EXPERIMENTAL' { url -0 'github:jandubois' - assert_stderr --regexp 'warning.+GitHub locator .* replaced with .* EXPERIMENTAL' + assert_warning "The github: scheme is still EXPERIMENTAL" } -@test 'Empty github: url returns an error' { +# Invalid URLs +@test 'empty github: url returns an error' { url -1 'github:' assert_fatal 'github: URL must contain at least an ORG, got ""' } -@test 'Missing org returns an error' { +@test 'missing org returns an error' { url -1 'github:/jandubois' assert_fatal 'github: URL must contain at least an ORG, got ""' } + +# github: redirects in github:ORG// repos +@test 'org redirects can point to different repo and may switch the branch name' { + url -0 'github:jandubois//redirect/' + # Note that the default branch in jandubois/jandubois is main, but in jandubois/lima it is master + assert_debug 'Locator "github:jandubois//redirect/" replaced with "github:jandubois/lima/templates/default"' + assert_debug 'Locator "github:jandubois/lima/templates/default" replaced with "https://raw.githubusercontent.com/jandubois/lima/master/templates/default.yaml"' + assert_output 'https://raw.githubusercontent.com/jandubois/lima/master/templates/default.yaml' +} + +@test 'org redirects propagate an explicit branch/tag to the other repo' { + url -0 'github:jandubois//redirect/@v1.2.1' + assert_debug 'Locator "github:jandubois//redirect/@v1.2.1" replaced with "github:jandubois/lima/templates/default@v1.2.1"' + assert_debug 'Locator "github:jandubois/lima/templates/default@v1.2.1" replaced with "https://raw.githubusercontent.com/jandubois/lima/v1.2.1/templates/default.yaml"' + assert_output 'https://raw.githubusercontent.com/jandubois/lima/v1.2.1/templates/default.yaml' +} + +@test 'org redirects cannot point to another org' { + url -1 'github:jandubois//invalid/org/' + assert_fatal 'redirect "github:lima-vm" is not a "github:jandubois" URL…' +} + +@test 'org redirects with branch cannot point to another org' { + url -1 'github:jandubois//invalid/org/@main' + assert_fatal 'redirect "github:lima-vm" is not a "github:jandubois" URL…' +} + +@test 'org redirects cannot include a branch or tag' { + url -1 'github:jandubois//invalid/tag/' + assert_fatal 'redirect "github:jandubois//@v0.0.0" must not include a branch/tag/sha…' +} + +@test 'org redirects with tag cannot include a branch or tag' { + url -1 'github:jandubois//invalid/tag/@v0.0.0' + assert_fatal 'redirect "github:jandubois//@v0.0.0" must not include a branch/tag/sha…' +} + +@test 'org redirects must not create circular redirects' { + url -1 'github:jandubois//loop/' + assert_fatal 'custom locator "github:jandubois//loop/" has a redirect loop' +} + +@test 'org redirects with branch must not create circular redirects' { + url -1 'github:jandubois//back/@main' + assert_fatal 'custom locator "github:jandubois//back/@main" has a redirect loop' +} diff --git a/pkg/limatmpl/github.go b/pkg/limatmpl/github.go index 1a3191dc739..816a685f05b 100644 --- a/pkg/limatmpl/github.go +++ b/pkg/limatmpl/github.go @@ -16,6 +16,8 @@ import ( "strings" ) +const defaultFilename = ".lima.yaml" + // transformGitHubURL transforms a github: URL to a raw.githubusercontent.com URL. // Input format: ORG/REPO[/PATH][@BRANCH] // @@ -25,43 +27,37 @@ import ( // If PATH is just a directory (trailing slash), it will be set to .lima.yaml // IF FILE is .lima.yaml and contents looks like a symlink, it will be replaced by the symlink target. func transformGitHubURL(ctx context.Context, input string) (string, error) { - // Check for explicit branch specification with @ at the end - var branch string - if idx := strings.LastIndex(input, "@"); idx != -1 { - branch = input[idx+1:] - input = input[:idx] - } + input, origBranch, _ := strings.Cut(input, "@") parts := strings.Split(input, "/") for len(parts) < 2 { parts = append(parts, "") } - org := parts[0] if org == "" { return "", fmt.Errorf("github: URL must contain at least an ORG, got %q", input) } - // If REPO is omitted (github:ORG or github:ORG//PATH), default it to ORG repo := cmp.Or(parts[1], org) - pathPart := strings.Join(parts[2:], "/") + filePath := strings.Join(parts[2:], "/") - if pathPart == "" { - pathPart = ".lima.yaml" + if filePath == "" { + filePath = defaultFilename } else { - // If path ends with /, it's a directory, so append .lima - if strings.HasSuffix(pathPart, "/") { - pathPart += ".lima" + // If path ends with / then it's a directory, so append .lima + if strings.HasSuffix(filePath, "/") { + filePath += defaultFilename } // If the filename (excluding first char for hidden files) has no extension, add .yaml - filename := path.Base(pathPart) + filename := path.Base(filePath) if !strings.Contains(filename[1:], ".") { - pathPart += ".yaml" + filePath += ".yaml" } } // Query default branch if no branch was specified + branch := origBranch if branch == "" { var err error branch, err = getGitHubDefaultBranch(ctx, org, repo) @@ -71,13 +67,24 @@ func transformGitHubURL(ctx context.Context, input string) (string, error) { } // If filename is .lima.yaml, check if it's a symlink/redirect to another file - if path.Base(pathPart) == ".lima.yaml" { - if redirectPath, err := resolveGitHubSymlink(ctx, org, repo, branch, pathPart); err == nil { - pathPart = redirectPath - } + if path.Base(filePath) == defaultFilename { + return resolveGitHubSymlink(ctx, org, repo, branch, filePath, origBranch) } + return githubUserContentURL(org, repo, branch, filePath), nil +} - return fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/%s", org, repo, branch, pathPart), nil +func githubUserContentURL(org, repo, branch, filePath string) string { + return fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/%s", org, repo, branch, filePath) +} + +func getGitHubUserContent(ctx context.Context, org, repo, branch, filePath string) (*http.Response, error) { + url := githubUserContentURL(org, repo, branch, filePath) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) + if err != nil { + return nil, fmt.Errorf("failed to create request: %w", err) + } + req.Header.Set("User-Agent", "lima") + return http.DefaultClient.Do(req) } // getGitHubDefaultBranch queries the GitHub API to get the default branch for a repository. @@ -108,7 +115,6 @@ func getGitHubDefaultBranch(ctx context.Context, org, repo string) (string, erro if err != nil { return "", fmt.Errorf("failed to read GitHub API response: %w", err) } - if resp.StatusCode != http.StatusOK { return "", fmt.Errorf("GitHub API returned status %d: %s", resp.StatusCode, string(body)) } @@ -116,53 +122,89 @@ func getGitHubDefaultBranch(ctx context.Context, org, repo string) (string, erro var repoData struct { DefaultBranch string `json:"default_branch"` } - if err := json.Unmarshal(body, &repoData); err != nil { return "", fmt.Errorf("failed to parse GitHub API response: %w", err) } - if repoData.DefaultBranch == "" { return "", fmt.Errorf("repository %s/%s has no default branch", org, repo) } - return repoData.DefaultBranch, nil } // resolveGitHubSymlink checks if a file at the given path is a symlink/redirect to another file. -// If the file contains a single line without YAML content, it's treated as a path to the actual file. -// Returns the redirect path if found, or the original path otherwise. -func resolveGitHubSymlink(ctx context.Context, org, repo, branch, filePath string) (string, error) { - url := fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/%s/%s", org, repo, branch, filePath) - - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) - if err != nil { - return "", fmt.Errorf("failed to create request: %w", err) - } - - req.Header.Set("User-Agent", "lima") - - resp, err := http.DefaultClient.Do(req) +// If the file contains a single line without newline, space, or colon then it's treated as a path to the actual file. +// Returns a URL to the redirect path if found, or a URL for original path otherwise. +func resolveGitHubSymlink(ctx context.Context, org, repo, branch, filePath, origBranch string) (string, error) { + resp, err := getGitHubUserContent(ctx, org, repo, branch, filePath) if err != nil { return "", fmt.Errorf("failed to fetch file: %w", err) } defer resp.Body.Close() + // Special rule for branch/tag propagation for github:ORG// requests. + if resp.StatusCode == http.StatusNotFound && repo == org { + defaultBranch, err := getGitHubDefaultBranch(ctx, org, repo) + if err == nil { + return resolveGitHubRedirect(ctx, org, repo, defaultBranch, filePath, branch) + } + } if resp.StatusCode != http.StatusOK { - return "", fmt.Errorf("file not found or inaccessible: status %d", resp.StatusCode) + return "", fmt.Errorf("file %q not found or inaccessible: status %d", resp.Request.URL, resp.StatusCode) } // Read first 1KB to check the file content buf := make([]byte, 1024) n, err := resp.Body.Read(buf) if err != nil && !errors.Is(err, io.EOF) { - return "", fmt.Errorf("failed to read file content: %w", err) + return "", fmt.Errorf("failed to read %q content: %w", resp.Request.URL, err) } content := string(buf[:n]) + // Symlink can also be a github: redirect if we are in a github:ORG// repo. + if repo == org && strings.HasPrefix(content, "github:") { + return validateGitHubRedirect(content, org, origBranch, resp.Request.URL.String()) + } + // A symlink must be a single line (without trailing newline), no spaces, no colons if !(content == "" || strings.ContainsAny(content, "\n :")) { // symlink is relative to the directory of filePath - return path.Join(path.Dir(filePath), content), nil + filePath = path.Join(path.Dir(filePath), content) + } + return githubUserContentURL(org, repo, branch, filePath), nil +} + +// resolveGitHubRedirect checks if a file at the given path is a github: URL to another file within the same repo. +// Returns the URL, or an error if the file doesn't exist, or doesn't start with github:ORG. +func resolveGitHubRedirect(ctx context.Context, org, repo, defaultBranch, filePath, origBranch string) (string, error) { + // Refetch the filepath from the defaultBranch + resp, err := getGitHubUserContent(ctx, org, repo, defaultBranch, filePath) + if err != nil { + return "", fmt.Errorf("failed to fetch file: %w", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("file %q not found or inaccessible: status %d", resp.Request.URL, resp.StatusCode) + } + body, err := io.ReadAll(resp.Body) + if err != nil { + return "", fmt.Errorf("failed to read %q content: %w", resp.Request.URL, err) + } + return validateGitHubRedirect(string(body), org, origBranch, resp.Request.URL.String()) +} + +func validateGitHubRedirect(body, org, origBranch, url string) (string, error) { + redirect, _, _ := strings.Cut(body, "\n") + redirect = strings.TrimSpace(redirect) + + if !strings.HasPrefix(redirect, "github:"+org+"/") { + return "", fmt.Errorf(`redirect %q is not a "github:%s" URL (from %q)`, redirect, org, url) + } + if strings.ContainsRune(redirect, '@') { + return "", fmt.Errorf("redirect %q must not include a branch/tag/sha (from %q)", redirect, url) + } + // If the origBranch is empty, then we need to look up the default branch in the redirect + if origBranch != "" { + redirect += "@" + origBranch } - return filePath, nil + return redirect, nil } diff --git a/pkg/limatmpl/locator.go b/pkg/limatmpl/locator.go index d9a4e19f595..3a94cf4ff20 100644 --- a/pkg/limatmpl/locator.go +++ b/pkg/limatmpl/locator.go @@ -296,7 +296,7 @@ func InstNameFromYAMLPath(yamlPath string) (string, error) { return s, nil } -func TransformCustomURL(ctx context.Context, locator string) (string, error) { +func transformCustomURL(ctx context.Context, locator string) (string, error) { u, err := url.Parse(locator) if err != nil || len(u.Scheme) <= 1 { return locator, nil @@ -313,12 +313,7 @@ func TransformCustomURL(ctx context.Context, locator string) (string, error) { } if u.Scheme == "github" { - newLocator, err := transformGitHubURL(ctx, u.Opaque) - if err != nil { - return "", err - } - logrus.Warnf("GitHub locator %q replaced with %q is still EXPERIMENTAL", locator, newLocator) - return newLocator, nil + return transformGitHubURL(ctx, u.Opaque) } plugin, err := plugins.Find("url-" + u.Scheme) @@ -350,9 +345,31 @@ func TransformCustomURL(ctx context.Context, locator string) (string, error) { } return "", fmt.Errorf("command %q failed: %w", cmd.String(), err) } - newLocator := strings.TrimSpace(string(stdout)) - if newLocator != locator { - logrus.Debugf("Custom locator %q replaced with %q", locator, newLocator) + return strings.TrimSpace(string(stdout)), nil +} + +func TransformCustomURL(ctx context.Context, locator string) (string, error) { + seen := make(map[string]bool) + origLocator := locator + githubSchemeDetected := false + + for !seen[locator] { + seen[locator] = true + if strings.HasPrefix(locator, "github:") { + githubSchemeDetected = true + } + newLocator, err := transformCustomURL(ctx, locator) + if err != nil { + return "", err + } + if newLocator == locator { + if githubSchemeDetected { + logrus.Warn("The github: scheme is still EXPERIMENTAL") + } + return newLocator, nil + } + logrus.Debugf("Locator %q replaced with %q", locator, newLocator) + locator = newLocator } - return newLocator, nil + return "", fmt.Errorf("custom locator %q has a redirect loop", origLocator) }