gitserver: Clean up output redaction

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)
 }