fix: support git clone when /tmp has noexec (#10146)

Resolves #9733 (alternative to #10136)

Instead of setting `GIT_ASKPASS`, instruct git to use the credential-store helper with a dedicated file.
The tests have been adjusted accordingly.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10146
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: oliverpool <git@olivier.pfad.fr>
Co-committed-by: oliverpool <git@olivier.pfad.fr>

modules/git/repo.go

@@ -133,7 +133,55 @@ func CloneWithArgs(ctx context.Context, args TrustedCmdArgs, from, to string, op
 		return err
 	}
 
-	cmd := NewCommandContextNoGlobals(ctx, args...).AddArguments("clone")
+	cmd := NewCommandContextNoGlobals(ctx, args...)
+
+	envs := os.Environ()
+	parsedFromURL, err := url.Parse(from)
+	if err == nil {
+		envs = proxy.EnvWithProxy(parsedFromURL)
+	}
+
+	fromURL := from
+	sanitizedFrom := from
+
+	// If the clone URL has credentials, build a credential file for usage by git-credential-store
+	// to prevent credential leak in the process list.
+	// https://git-scm.com/docs/git-credential-store#_storage_format
+	// credential.helper adjustment must be set before the git subcommand
+	if strings.Contains(from, "://") && strings.Contains(from, "@") {
+		sanitizedFrom = util.SanitizeCredentialURLs(from)
+		if parsedFromURL != nil {
+			credentialsFile, err := os.CreateTemp("", "forgejo-clone-credentials-")
+			if err != nil {
+				return err
+			}
+			credentialsPath := credentialsFile.Name()
+
+			defer func() {
+				_ = credentialsFile.Close()
+				if err := util.Remove(credentialsPath); err != nil {
+					log.Warn("Unable to remove temporary file %q: %v", credentialsPath, err)
+				}
+			}()
+			_, err = credentialsFile.Write([]byte(parsedFromURL.String()))
+			if err != nil {
+				return err
+			}
+			err = credentialsFile.Close()
+			if err != nil {
+				return err
+			}
+
+			cmd.AddArguments("-c").AddDynamicArguments("credential.helper=store --file=" + credentialsPath)
+
+			// remove the password from the URL argument
+			parsedFromURL.User = url.User(parsedFromURL.User.Username())
+			fromURL = parsedFromURL.String()
+		}
+	}
+
+	cmd.AddArguments("clone")
+
 	if opts.SkipTLSVerify {
 		cmd.AddArguments("-c", "http.sslVerify=false")
 	}
@@ -162,81 +210,6 @@ func CloneWithArgs(ctx context.Context, args TrustedCmdArgs, from, to string, op
 		cmd.AddArguments("-b").AddDynamicArguments(opts.Branch)
 	}
 
-	envs := os.Environ()
-	parsedFromURL, err := url.Parse(from)
-	if err == nil {
-		envs = proxy.EnvWithProxy(parsedFromURL)
-	}
-
-	fromURL := from
-	sanitizedFrom := from
-
-	// If the clone URL has credentials, sanitize it and store the credentials in
-	// a temporary file that git will access.
-	if strings.Contains(from, "://") && strings.Contains(from, "@") {
-		sanitizedFrom = util.SanitizeCredentialURLs(from)
-		if parsedFromURL != nil {
-			if pwd, has := parsedFromURL.User.Password(); has {
-				parsedFromURL.User = url.User(parsedFromURL.User.Username())
-				fromURL = parsedFromURL.String()
-
-				credentialsFile, err := os.CreateTemp(os.TempDir(), "forgejo-clone-credentials")
-				if err != nil {
-					return err
-				}
-				credentialsPath := credentialsFile.Name()
-
-				defer func() {
-					_ = credentialsFile.Close()
-					if err := util.Remove(credentialsPath); err != nil {
-						log.Warn("Unable to remove temporary file %q: %v", credentialsPath, err)
-					}
-				}()
-
-				// Make it read-write.
-				if err := credentialsFile.Chmod(0o600); err != nil {
-					return err
-				}
-
-				// Write the password.
-				if _, err := fmt.Fprint(credentialsFile, pwd); err != nil {
-					return err
-				}
-
-				askpassFile, err := os.CreateTemp(os.TempDir(), "forgejo-askpass")
-				if err != nil {
-					return err
-				}
-				askpassPath := askpassFile.Name()
-
-				defer func() {
-					_ = askpassFile.Close()
-					if err := util.Remove(askpassPath); err != nil {
-						log.Warn("Unable to remove temporary file %q: %v", askpassPath, err)
-					}
-				}()
-
-				// Make it executable.
-				if err := askpassFile.Chmod(0o700); err != nil {
-					return err
-				}
-
-				// Write the password script.
-				if _, err := fmt.Fprintf(askpassFile, "exec cat %s", credentialsPath); err != nil {
-					return err
-				}
-
-				// Close it, so that Git can use it and no busy errors arise.
-				_ = askpassFile.Close()
-				_ = credentialsFile.Close()
-
-				// Use environments to specify that git should ask for credentials, this
-				// takes precedences over anything else https://git-scm.com/docs/gitcredentials#_requesting_credentials.
-				envs = append(envs, "GIT_ASKPASS="+askpassPath)
-			}
-		}
-	}
-
 	cmd.SetDescription(fmt.Sprintf("clone branch %s from %s to %s (shared: %t, mirror: %t, depth: %d)", opts.Branch, sanitizedFrom, to, opts.Shared, opts.Mirror, opts.Depth))
 	cmd.AddDashesAndList(fromURL, to)
 

modules/git/repo_test.go

@@ -55,7 +55,6 @@ func TestRepoGetDivergingCommits(t *testing.T) {
 
 func TestCloneCredentials(t *testing.T) {
 	calledWithoutPassword := false
-	askpassFile := ""
 	credentialsFile := ""
 
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
@@ -77,8 +76,7 @@ func TestCloneCredentials(t *testing.T) {
 		user, password, ok := bytes.Cut(rawAuth, []byte{':'})
 		assert.True(t, ok)
 
-		// First time around Git tries without password (that's specified in the clone URL).
-		// It demonstrates it doesn't immediately uses askpass.
+		// First time around Git must try without password (password was removed from the clone URL to not appear as argument).
 		if len(password) == 0 {
 			assert.EqualValues(t, "oauth2", user)
 			calledWithoutPassword = true
@@ -93,22 +91,15 @@ func TestCloneCredentials(t *testing.T) {
 
 		tmpDir := os.TempDir()
 
-		// Verify that the askpass implementation was used.
-		files, err := fs.Glob(os.DirFS(tmpDir), "forgejo-askpass*")
+		// Verify that the credential store was used.
+		files, err := fs.Glob(os.DirFS(tmpDir), "forgejo-clone-credentials-*")
 		require.NoError(t, err)
 		for _, fileName := range files {
-			fileContent, err := os.ReadFile(filepath.Join(tmpDir, fileName))
+			credentialsFile = filepath.Join(tmpDir, fileName)
+			fileContent, err := os.ReadFile(credentialsFile)
 			require.NoError(t, err)
 
-			credentialsPath, ok := bytes.CutPrefix(fileContent, []byte(`exec cat `))
-			assert.True(t, ok)
-
-			fileContent, err = os.ReadFile(string(credentialsPath))
-			require.NoError(t, err)
-			assert.EqualValues(t, "some_token", fileContent)
-
-			askpassFile = filepath.Join(tmpDir, fileName)
-			credentialsFile = string(credentialsPath)
+			assert.True(t, bytes.Contains(fileContent, []byte(`http`)), string(fileContent))
 		}
 	}))
 
@@ -120,12 +111,9 @@ func TestCloneCredentials(t *testing.T) {
 	require.NoError(t, Clone(t.Context(), serverURL.String(), t.TempDir(), CloneRepoOptions{}))
 
 	assert.True(t, calledWithoutPassword)
-	assert.NotEmpty(t, askpassFile)
 	assert.NotEmpty(t, credentialsFile)
 
-	// Check that the helper files are gone.
-	_, err = os.Stat(askpassFile)
-	require.ErrorIs(t, err, fs.ErrNotExist)
+	// Check that the credential file is gone.
 	_, err = os.Stat(credentialsFile)
 	require.ErrorIs(t, err, fs.ErrNotExist)
 }