gitserver: Clean up output redaction (#61857)

Since we refactored VCSSyncer recently to take control of the remote URL, we now do redaction at this layer, so this TODO comment can be removed.

Test plan:

E2E test suite still passes.

Author: eseliger

cmd/gitserver/internal/server.go

@@ -924,11 +924,6 @@ func (s *Server) doBackgroundRepoUpdate(repo api.RepoName, revspec string) error
 
 		dir := s.fs.RepoDir(repo)
 
-		remoteURL, err := s.getRemoteURL(ctx, repo)
-		if err != nil {
-			return errors.Wrap(err, "failed to determine Git remote URL")
-		}
-
 		syncer, err := s.getVCSSyncer(ctx, repo)
 		if err != nil {
 			return errors.Wrap(err, "get VCS syncer")
@@ -942,11 +937,8 @@ func (s *Server) doBackgroundRepoUpdate(repo api.RepoName, revspec string) error
 		defer git.CleanTmpPackFiles(s.logger, dir)
 
 		output, err := syncer.Fetch(ctx, repo, dir, revspec)
-		// TODO: Move the redaction also into the VCSSyncer layer here, to be in line
-		// with what clone does.
-		redactedOutput := urlredactor.New(remoteURL).Redact(string(output))
 		// best-effort update the output of the fetch
-		if err := s.db.GitserverRepos().SetLastOutput(serverCtx, repo, redactedOutput); err != nil {
+		if err := s.db.GitserverRepos().SetLastOutput(serverCtx, repo, string(output)); err != nil {
 			s.logger.Warn("Setting last output in DB", log.Error(err))
 		}
 
@@ -955,7 +947,7 @@ func (s *Server) doBackgroundRepoUpdate(repo api.RepoName, revspec string) error
 				return err
 			}
 			if output != nil {
-				return errors.Wrapf(err, "failed to fetch repo %q with output %q", repo, redactedOutput)
+				return errors.Wrapf(err, "failed to fetch repo %q with output %q", repo, string(output))
 			} else {
 				return errors.Wrapf(err, "failed to fetch repo %q", repo)
 			}

cmd/gitserver/internal/vcssyncer/git.go

@@ -3,8 +3,6 @@ package vcssyncer
 import (
 	"bytes"
 	"context"
-	"github.com/sourcegraph/sourcegraph/internal/gitserver/protocol"
-	"github.com/sourcegraph/sourcegraph/internal/lazyregexp"
 	"io"
 	"os"
 	"os/exec"
@@ -13,12 +11,13 @@ import (
 
 	"github.com/sourcegraph/log"
 
-	"github.com/sourcegraph/sourcegraph/internal/api"
-
 	"github.com/sourcegraph/sourcegraph/cmd/gitserver/internal/common"
 	"github.com/sourcegraph/sourcegraph/cmd/gitserver/internal/executil"
 	"github.com/sourcegraph/sourcegraph/cmd/gitserver/internal/git"
 	"github.com/sourcegraph/sourcegraph/cmd/gitserver/internal/urlredactor"
+	"github.com/sourcegraph/sourcegraph/internal/api"
+	"github.com/sourcegraph/sourcegraph/internal/gitserver/protocol"
+	"github.com/sourcegraph/sourcegraph/internal/lazyregexp"
 	"github.com/sourcegraph/sourcegraph/internal/wrexec"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 )
@@ -63,33 +62,31 @@ func (s *gitRepoSyncer) IsCloneable(ctx context.Context, repoName api.RepoName)
 		return errors.Wrapf(err, "failed to get remote URL source for %s", repoName)
 	}
 
-	{
-		remoteURL, err := source.RemoteURL(ctx)
-		if err != nil {
-			return errors.Wrapf(err, "failed to get remote URL for %s", repoName)
-		}
+	remoteURL, err := source.RemoteURL(ctx)
+	if err != nil {
+		return errors.Wrapf(err, "failed to get remote URL for %s", repoName)
+	}
 
-		args := []string{"ls-remote", remoteURL.String(), "HEAD"}
-		ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
-		defer cancel()
+	args := []string{"ls-remote", remoteURL.String(), "HEAD"}
+	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+	defer cancel()
 
-		r := urlredactor.New(remoteURL)
-		cmd := exec.CommandContext(ctx, "git", args...)
+	r := urlredactor.New(remoteURL)
+	cmd := exec.CommandContext(ctx, "git", args...)
 
-		// Configure the command to be able to talk to a remote.
-		executil.ConfigureRemoteGitCommand(cmd, remoteURL)
+	// Configure the command to be able to talk to a remote.
+	executil.ConfigureRemoteGitCommand(cmd, remoteURL)
 
-		out, err := s.recordingCommandFactory.WrapWithRepoName(ctx, log.NoOp(), repoName, cmd).WithRedactorFunc(r.Redact).CombinedOutput()
-		if err != nil {
-			if ctxerr := ctx.Err(); ctxerr != nil {
-				err = ctxerr
-			}
-			if len(out) > 0 {
-				redactedOutput := urlredactor.New(remoteURL).Redact(string(out))
-				err = errors.Wrap(err, "failed to check remote access: "+redactedOutput)
-			}
-			return err
+	out, err := s.recordingCommandFactory.WrapWithRepoName(ctx, log.NoOp(), repoName, cmd).WithRedactorFunc(r.Redact).CombinedOutput()
+	if err != nil {
+		if ctxerr := ctx.Err(); ctxerr != nil {
+			err = ctxerr
+		}
+		if len(out) > 0 {
+			redactedOutput := r.Redact(string(out))
+			err = errors.Wrap(err, "failed to check remote access: "+redactedOutput)
 		}
+		return err
 	}
 
 	return nil

cmd/gitserver/internal/vcssyncer/syncer.go

@@ -64,6 +64,10 @@ type VCSSyncer interface {
 	// to lazily fetch package versions. More details at
 	// https://github.com/sourcegraph/sourcegraph/issues/37921#issuecomment-1184301885
 	// Beware that the revspec parameter can be any random user-provided string.
+	// 🚨 SECURITY:
+	// Output returned from this function should NEVER contain sensitive information.
+	// The VCSSyncer implementation is responsible of redacting potentially
+	// sensitive data like secrets.
 	Fetch(ctx context.Context, repoName api.RepoName, dir common.GitDir, revspec string) ([]byte, error)
 }