From a1105d265196eb04aac33fa6e7036df4f1fa2cf7 Mon Sep 17 00:00:00 2001
From: Salim Afiune Maya <afiune@mondoo.com>
Date: Tue, 4 Feb 2025 13:23:17 -0800
Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20=20github:=20fix=20precedence=20?=
 =?UTF-8?q?flag=20vs=20env-var=20(#5144)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* 🐛  github: fix precedence flag vs env-var

Github provide has two authentication methods.

1. Application credentials
2. Personal access token

We give precedence to the former and, if both auth methods are provided,
we will output a warning.

Closes https://github.com/mondoohq/cnquery/issues/5036

---------

Signed-off-by: Salim Afiune Maya <afiune@mondoo.com>
---
 providers/github/connection/connection.go |  5 -----
 providers/github/provider/provider.go     | 21 ++++++++++++++++++---
 test/providers/github_test.go             | 18 ++++++++++++++++++
 3 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/providers/github/connection/connection.go b/providers/github/connection/connection.go
index 429e06758..69b06b2bb 100644
--- a/providers/github/connection/connection.go
+++ b/providers/github/connection/connection.go
@@ -7,7 +7,6 @@ import (
 	"context"
 	"net/http"
 	"net/url"
-	"os"
 	"strconv"
 	"time"
 
@@ -71,10 +70,6 @@ func connectionOptionsFromConfigOptions(conf *inventory.Config) (opts githubConn
 	opts.EnterpriseURL = conf.Options[OPTION_ENTERPRISE_URL]
 	opts.Token = conf.Options[OPTION_TOKEN]
 
-	if opts.Token == "" {
-		opts.Token = os.Getenv("GITHUB_TOKEN")
-	}
-
 	for _, cred := range conf.Credentials {
 		switch cred.Type {
 
diff --git a/providers/github/provider/provider.go b/providers/github/provider/provider.go
index 9637bf726..862ff8009 100644
--- a/providers/github/provider/provider.go
+++ b/providers/github/provider/provider.go
@@ -62,6 +62,13 @@ func (s *Service) ParseCLI(req *plugin.ParseCLIReq) (*plugin.ParseCLIRes, error)
 		conf.Options[connection.OPTION_ENTERPRISE_URL] = string(x.Value)
 	}
 
+	// Github provide has two authentication methods.
+	//
+	// 1. Application credentials
+	// 2. Personal access token
+	//
+	// We give precedence to the former and, if both auth methods are provided,
+	// we will output a warning.
 	isAppAuth := false
 	appId, ok := flags[connection.OPTION_APP_ID]
 	if ok && len(appId.Value) > 0 {
@@ -73,20 +80,28 @@ func (s *Service) ParseCLI(req *plugin.ParseCLIReq) (*plugin.ParseCLIRes, error)
 		pk := req.Flags[connection.OPTION_APP_PRIVATE_KEY]
 		conf.Options[connection.OPTION_APP_PRIVATE_KEY] = string(pk.Value)
 		isAppAuth = true
+		log.Debug().Msg("application credentials provided")
 	}
 
 	token := ""
 	if x, ok := flags["token"]; ok && len(x.Value) != 0 {
 		token = string(x.Value)
+		log.Debug().Msg("loaded token from flag")
 	}
-	if token == "" {
+	if token == "" && len(os.Getenv("GITHUB_TOKEN")) != 0 {
 		token = os.Getenv("GITHUB_TOKEN")
+		log.Debug().Msg("loaded token from GITHUB_TOKEN env variable")
 	}
 	if token == "" && !isAppAuth {
-		return nil, errors.New("a valid GitHub authentication is required, pass --token '<yourtoken>', set GITHUB_TOKEN environment variable or provide GitHub App credentials")
+		return nil, errors.New("a valid GitHub authentication is required, pass --token '<yourtoken>', " +
+			"set GITHUB_TOKEN environment variable or provide GitHub App credentials")
 	}
 	if token != "" {
-		conf.Credentials = append(conf.Credentials, vault.NewPasswordCredential("", token))
+		if isAppAuth {
+			log.Warn().Msg("both authentication methods provided, using application credentials")
+		} else {
+			conf.Credentials = append(conf.Credentials, vault.NewPasswordCredential("", token))
+		}
 	}
 
 	// discovery flags
diff --git a/test/providers/github_test.go b/test/providers/github_test.go
index c972759ad..38fdc1ba0 100644
--- a/test/providers/github_test.go
+++ b/test/providers/github_test.go
@@ -57,4 +57,22 @@ func TestGithubScanFlags(t *testing.T) {
 			"could not read private key", // expected! it means we loaded the flags
 		)
 	})
+	t.Run("github scan with both auth methods, prefer app credentials", func(t *testing.T) {
+		// NOTE this will fail but, it will load the flags and fail with the right message
+		r := test.NewCliTestRunner("./cnquery", "scan", "github", "repo", "foo",
+			// personal access token
+			"--token", "abc",
+			// application credentials
+			"--app-id", "123", "--app-installation-id", "456", "--app-private-key", "private-key.pem",
+		)
+		err := r.Run()
+		require.NoError(t, err)
+		assert.Equal(t, 1, r.ExitCode())
+		assert.NotNil(t, r.Stdout())
+		assert.NotNil(t, r.Stderr())
+
+		assert.Contains(t, string(r.Stderr()),
+			"could not read private key", // expected! it means we use app credentials
+		)
+	})
 }