From e6b3be460840f1f982d5358198466e7d6f509d21 Mon Sep 17 00:00:00 2001 From: zeripath Date: Sun, 4 Sep 2022 11:47:56 +0100 Subject: Add more checks in migration code (#21011) When migrating add several more important sanity checks: * SHAs must be SHAs * Refs must be valid Refs * URLs must be reasonable Signed-off-by: Andrew Thornton Signed-off-by: Andrew Thornton Co-authored-by: techknowlogick --- services/migrations/github.go | 76 ++++++++++++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 23 deletions(-) (limited to 'services/migrations/github.go') diff --git a/services/migrations/github.go b/services/migrations/github.go index 5f5b430fa9..0ffdbb042a 100644 --- a/services/migrations/github.go +++ b/services/migrations/github.go @@ -51,7 +51,7 @@ func (f *GithubDownloaderV3Factory) New(ctx context.Context, opts base.MigrateOp oldOwner := fields[1] oldName := strings.TrimSuffix(fields[2], ".git") - log.Trace("Create github downloader: %s/%s", oldOwner, oldName) + log.Trace("Create github downloader BaseURL: %s %s/%s", baseURL, oldOwner, oldName) return NewGithubDownloaderV3(ctx, baseURL, opts.AuthUsername, opts.AuthPassword, opts.AuthToken, oldOwner, oldName), nil } @@ -67,6 +67,7 @@ type GithubDownloaderV3 struct { base.NullDownloader ctx context.Context clients []*github.Client + baseURL string repoOwner string repoName string userName string @@ -81,6 +82,7 @@ type GithubDownloaderV3 struct { func NewGithubDownloaderV3(ctx context.Context, baseURL, userName, password, token, repoOwner, repoName string) *GithubDownloaderV3 { downloader := GithubDownloaderV3{ userName: userName, + baseURL: baseURL, password: password, ctx: ctx, repoOwner: repoOwner, @@ -118,6 +120,20 @@ func NewGithubDownloaderV3(ctx context.Context, baseURL, userName, password, tok return &downloader } +// String implements Stringer +func (g *GithubDownloaderV3) String() string { + return fmt.Sprintf("migration from github server %s %s/%s", g.baseURL, g.repoOwner, g.repoName) +} + +// ColorFormat provides a basic color format for a GithubDownloader +func (g *GithubDownloaderV3) ColorFormat(s fmt.State) { + if g == nil { + log.ColorFprintf(s, "") + return + } + log.ColorFprintf(s, "migration from github server %s %s/%s", g.baseURL, g.repoOwner, g.repoName) +} + func (g *GithubDownloaderV3) addClient(client *http.Client, baseURL string) { githubClient := github.NewClient(client) if baseURL != "https://github.com" { @@ -322,33 +338,44 @@ func (g *GithubDownloaderV3) convertGithubRelease(rel *github.RepositoryRelease) Updated: asset.UpdatedAt.Time, DownloadFunc: func() (io.ReadCloser, error) { g.waitAndPickClient() - asset, redirectURL, err := g.getClient().Repositories.DownloadReleaseAsset(g.ctx, g.repoOwner, g.repoName, assetID, nil) + readCloser, redirectURL, err := g.getClient().Repositories.DownloadReleaseAsset(g.ctx, g.repoOwner, g.repoName, assetID, nil) if err != nil { return nil, err } if err := g.RefreshRate(); err != nil { log.Error("g.getClient().RateLimits: %s", err) } - if asset == nil { - if redirectURL != "" { - g.waitAndPickClient() - req, err := http.NewRequestWithContext(g.ctx, "GET", redirectURL, nil) - if err != nil { - return nil, err - } - resp, err := httpClient.Do(req) - err1 := g.RefreshRate() - if err1 != nil { - log.Error("g.getClient().RateLimits: %s", err1) - } - if err != nil { - return nil, err - } - return resp.Body, nil - } - return nil, fmt.Errorf("No release asset found for %d", assetID) + + if readCloser != nil { + return readCloser, nil + } + + if redirectURL == "" { + return nil, fmt.Errorf("no release asset found for %d", assetID) + } + + // Prevent open redirect + if !hasBaseURL(redirectURL, g.baseURL) && + !hasBaseURL(redirectURL, "https://objects.githubusercontent.com/") { + WarnAndNotice("Unexpected AssetURL for assetID[%d] in %s: %s", asset.GetID(), g, redirectURL) + + return io.NopCloser(strings.NewReader(redirectURL)), nil + } + + g.waitAndPickClient() + req, err := http.NewRequestWithContext(g.ctx, "GET", redirectURL, nil) + if err != nil { + return nil, err } - return asset, nil + resp, err := httpClient.Do(req) + err1 := g.RefreshRate() + if err1 != nil { + log.Error("g.RefreshRate(): %s", err1) + } + if err != nil { + return nil, err + } + return resp.Body, nil }, }) } @@ -697,7 +724,7 @@ func (g *GithubDownloaderV3) GetPullRequests(page, perPage int) ([]*base.PullReq SHA: pr.GetHead().GetSHA(), OwnerName: pr.GetHead().GetUser().GetLogin(), RepoName: pr.GetHead().GetRepo().GetName(), - CloneURL: pr.GetHead().GetRepo().GetCloneURL(), + CloneURL: pr.GetHead().GetRepo().GetCloneURL(), // see below for SECURITY related issues here }, Base: base.PullRequestBranch{ Ref: pr.GetBase().GetRef(), @@ -705,10 +732,13 @@ func (g *GithubDownloaderV3) GetPullRequests(page, perPage int) ([]*base.PullReq RepoName: pr.GetBase().GetRepo().GetName(), OwnerName: pr.GetBase().GetUser().GetLogin(), }, - PatchURL: pr.GetPatchURL(), + PatchURL: pr.GetPatchURL(), // see below for SECURITY related issues here Reactions: reactions, ForeignIndex: int64(*pr.Number), }) + + // SECURITY: Ensure that the PR is safe + _ = CheckAndEnsureSafePR(allPRs[len(allPRs)-1], g.baseURL, g) } return allPRs, len(prs) < perPage, nil -- cgit v1.2.3