Bug bash feedback

Author: stuartpa

SECURITY.md

@@ -4,15 +4,15 @@
 
 Microsoft takes the security of our software products and services seriously, which includes all source code repositories managed through our GitHub organizations, which include [Microsoft](https://github.com/Microsoft), [Azure](https://github.com/Azure), [DotNet](https://github.com/dotnet), [AspNet](https://github.com/aspnet), [Xamarin](https://github.com/xamarin), and [our GitHub organizations](https://opensource.microsoft.com/).
 
-If you believe you have found a security vulnerability in any Microsoft-owned repository that meets [Microsoft's definition of a security vulnerability](https://docs.microsoft.com/en-us/previous-versions/tn-archive/cc751383(v=technet.10)), please report it to us as described below.
+If you believe you have found a security vulnerability in any Microsoft-owned repository that meets [Microsoft's definition of a security vulnerability](https://docs.microsoft.com/previous-versions/tn-archive/cc751383(v=technet.10)), please report it to us as described below.
 
 ## Reporting Security Issues
 
 **Please do not report security vulnerabilities through public GitHub issues.**
 
 Instead, please report them to the Microsoft Security Response Center (MSRC) at [https://msrc.microsoft.com/create-report](https://msrc.microsoft.com/create-report).
 
-If you prefer to submit without logging in, send email to [[email protected]](mailto:[email protected]).  If possible, encrypt your message with our PGP key; please download it from the [Microsoft Security Response Center PGP Key page](https://www.microsoft.com/en-us/msrc/pgp-key-msrc).
+If you prefer to submit without logging in, send email to [[email protected]](mailto:[email protected]).  If possible, encrypt your message with our PGP key; please download it from the [Microsoft Security Response Center PGP Key page](https://www.microsoft.com/msrc/pgp-key-msrc).
 
 You should receive a response within 24 hours. If for some reason you do not, please follow up via email to ensure we received your original message. Additional information can be found at [microsoft.com/msrc](https://www.microsoft.com/msrc). 
 
@@ -36,6 +36,6 @@ We prefer all communications to be in English.
 
 ## Policy
 
-Microsoft follows the principle of [Coordinated Vulnerability Disclosure](https://www.microsoft.com/en-us/msrc/cvd).
+Microsoft follows the principle of [Coordinated Vulnerability Disclosure](https://www.microsoft.com/msrc/cvd).
 
 <!-- END MICROSOFT SECURITY.MD BLOCK -->

cmd/modern/main_test.go

@@ -37,10 +37,10 @@ func TestDisplayHints(t *testing.T) {
 }
 
 func TestCheckErr(t *testing.T) {
+	rootCmd = cmdparser.New[*Root](dependency.Options{})
+	rootCmd.loggingLevel = 4
+	checkErr(nil)
 	assert.Panics(t, func() {
-		rootCmd = cmdparser.New[*Root](dependency.Options{})
-		rootCmd.loggingLevel = 4
-		checkErr(nil)
 		checkErr(errors.New("test error"))
 	})
 }

cmd/modern/root.go

@@ -28,16 +28,19 @@ func (c *Root) DefineCommand(...cmdparser.CommandOptions) {
 	// Example usage steps
 	steps := []string{"sqlcmd create mssql --accept-eula --using https://aka.ms/AdventureWorksLT.bak"}
 
-	if runtime.GOOS == "windows" {
+	if runtime.GOOS == "windows" || runtime.GOOS == "darwin" {
 		steps = append(steps, "sqlcmd open ads")
 	}
 
 	steps = append(steps, `sqlcmd query "SELECT @@version"`)
 	steps = append(steps, "sqlcmd delete")
 
-	examples := []cmdparser.ExampleOptions{{
-		Description: "Install/Create, Query, Uninstall SQL Server",
-		Steps:       steps}}
+	examples := []cmdparser.ExampleOptions{
+		{Description: "Install/Create, Query, Uninstall SQL Server",
+			Steps: steps},
+		{Description: "View configuration information and connection strings",
+			Steps: []string{"sqlcmd config view", "sqlcmd config cs"}},
+	}
 
 	commandOptions := cmdparser.CommandOptions{
 		Use: "sqlcmd",
@@ -67,8 +70,8 @@ func (c *Root) SubCommands() []cmdparser.Command {
 		cmdparser.New[*root.Uninstall](dependencies),
 	}
 
-	// BUG:(stuartpa) - Add Mac / Linux support
-	if runtime.GOOS == "windows" {
+	// BUG(stuartpa): - Add Linux support
+	if runtime.GOOS == "windows" || runtime.GOOS == "darwin" {
 		subCommands = append(subCommands, cmdparser.New[*root.Open](dependencies))
 	}
 
@@ -91,7 +94,7 @@ func (c *Root) IsValidSubCommand(command string) bool {
 
 func (c *Root) addGlobalFlags() {
 
-	// BUG:(stuartpa) - This is a temporary flag until we have migrated
+	// BUG(stuartpa): - This is a temporary flag until we have migrated
 	// the kong impl to cobra.  sqlcmd -? will show the kong help (all the back-compat
 	// flags), sqlcmd --? will show the kong "did you mean one of" help.
 	var unused bool
@@ -102,14 +105,23 @@ func (c *Root) addGlobalFlags() {
 		Usage:     "help for backwards compatibility flags (-S, -U, -E etc.)",
 	})
 
+	// BUG(stuartpa): - This is a temporary flag until we have migrated
+	// the kong impl to cobra.  The implementation of --version is coming from
+	// kong, but we need to add it to the list of flags so that it shows up in the --help
+	c.AddFlag(cmdparser.FlagOptions{
+		Bool:  &unused,
+		Name:  "version",
+		Usage: "print version of sqlcmd",
+	})
+
 	c.AddFlag(cmdparser.FlagOptions{
 		String:        &c.configFilename,
 		DefaultString: config.DefaultFileName(),
 		Name:          "sqlconfig",
-		Usage:         "Configuration file",
+		Usage:         "configuration file",
 	})
 
-	/* BUG:(stuartpa) - At the moment this is a top level flag, but it doesn't
+	/* BUG(stuartpa): - At the moment this is a top level flag, but it doesn't
 	work with all sub-commands (e.g. query), so removing for now.
 	c.AddFlag(cmdparser.FlagOptions{
 		String:        &c.outputType,
@@ -124,6 +136,6 @@ func (c *Root) addGlobalFlags() {
 		Int:        (*int)(&c.loggingLevel),
 		DefaultInt: 2,
 		Name:       "verbosity",
-		Usage:      "Log level, error=0, warn=1, info=2, debug=3, trace=4",
+		Usage:      "log level, error=0, warn=1, info=2, debug=3, trace=4",
 	})
 }

cmd/modern/root/config.go

@@ -24,9 +24,9 @@ func (c *Config) DefineCommand(...cmdparser.CommandOptions) {
 		SubCommands: c.SubCommands(),
 		Examples: []cmdparser.ExampleOptions{
 			{
-				Description: "Add context for existing endpoint and user",
+				Description: "Add context for existing endpoint and user (use SQLCMD_PASSWORD or SQLCMDPASSWORD)",
 				Steps: []string{
-					fmt.Sprintf("%s SQLCMD_PASSWORD=<password>", pal.CreateEnvVarKeyword()),
+					fmt.Sprintf("%s SQLCMD_PASSWORD=<placeholderpassword>", pal.CreateEnvVarKeyword()),
 					"sqlcmd config add-user --name sa1434 --username sa",
 					fmt.Sprintf("%s SQLCMD_PASSWORD=", pal.CreateEnvVarKeyword()),
 					"sqlcmd config add-endpoint --name ep1434 --address localhost --port 1434",

cmd/modern/root/config/add-context_test.go

@@ -16,16 +16,16 @@ func TestAddContext(t *testing.T) {
 }
 
 func TestNegAddContext(t *testing.T) {
+	cmdparser.TestSetup(t)
 	assert.Panics(t, func() {
-		cmdparser.TestSetup(t)
 		cmdparser.TestCmd[*AddContext]("--endpoint does-not-exist")
 	})
 }
 
 func TestNegAddContext2(t *testing.T) {
+	cmdparser.TestSetup(t)
+	cmdparser.TestCmd[*AddEndpoint]()
 	assert.Panics(t, func() {
-		cmdparser.TestSetup(t)
-		cmdparser.TestCmd[*AddEndpoint]()
 		cmdparser.TestCmd[*AddContext]("--endpoint endpoint --user does-not-exist")
 	})
 }

cmd/modern/root/config/add-user.go

@@ -30,13 +30,21 @@ func (c *AddUser) DefineCommand(...cmdparser.CommandOptions) {
 		Short: "Add a user",
 		Examples: []cmdparser.ExampleOptions{
 			{
-				Description: "Add a user",
+				Description: "Add a user (using the SQLCMD_PASSWORD environment variable)",
 				Steps: []string{
-					fmt.Sprintf(`%s SQLCMD_PASSWORD=<password>`, pal.CreateEnvVarKeyword()),
+					fmt.Sprintf(`%s SQLCMD_PASSWORD=<placeholderpassword>`, pal.CreateEnvVarKeyword()),
 					"sqlcmd config add-user --name my-user --username user1",
 					fmt.Sprintf(`%s SQLCMD_PASSWORD=`, pal.CreateEnvVarKeyword()),
 				},
 			},
+			{
+				Description: "Add a user (using the SQLCMDPASSWORD environment variable)",
+				Steps: []string{
+					fmt.Sprintf(`%s SQLCMDPASSWORD=<placeholderpassword>`, pal.CreateEnvVarKeyword()),
+					"sqlcmd config add-user --name my-user --username user1",
+					fmt.Sprintf(`%s SQLCMDPASSWORD=`, pal.CreateEnvVarKeyword()),
+				},
+			},
 		},
 		Run: c.run}
 
@@ -59,7 +67,7 @@ func (c *AddUser) DefineCommand(...cmdparser.CommandOptions) {
 	c.AddFlag(cmdparser.FlagOptions{
 		String: &c.username,
 		Name:   "username",
-		Usage:  "The username (provide password in SQLCMD_PASSWORD environment variable)",
+		Usage:  "The username (provide password in SQLCMD_PASSWORD or SQLCMDPASSWORD environment variable)",
 	})
 
 	c.encryptPasswordFlag()
@@ -95,24 +103,35 @@ func (c *AddUser) run() {
 	}
 
 	if c.authType == "basic" {
-		if os.Getenv("SQLCMD_PASSWORD") == "" {
+		if os.Getenv("SQLCMD_PASSWORD") == "" && os.Getenv("SQLCMDPASSWORD") == "" {
 			output.FatalWithHints([]string{
-				"Provide password in the SQLCMD_PASSWORD environment variable"},
+				"Provide password in the SQLCMD_PASSWORD (or SQLCMDPASSWORD) environment variable"},
 				"Authentication Type 'basic' requires a password")
 		}
 
 		if c.username == "" {
 			output.FatalfWithHintExamples([][]string{
 				{"Provide a username with the --username flag",
-					"sqlcmd config add-user --username stuartpa"},
+					"sqlcmd config add-user --username sa"},
 			},
-				"Username not provider")
+				"Username not provided")
 		}
 
+		if os.Getenv("SQLCMD_PASSWORD") != "" &&
+			os.Getenv("SQLCMDPASSWORD") != "" {
+			output.FatalWithHints([]string{
+				"Unset one of the environment variables SQLCMD_PASSWORD or SQLCMDPASSWORD"},
+				"Both environment variables SQLCMD_PASSWORD and SQLCMDPASSWORD are set. ")
+		}
+
+		password := os.Getenv("SQLCMD_PASSWORD")
+		if password == "" {
+			password = os.Getenv("SQLCMDPASSWORD")
+		}
 		user.BasicAuth = &sqlconfig.BasicAuthDetails{
 			Username:          c.username,
 			PasswordEncrypted: c.encryptPassword,
-			Password:          secret.Encode(os.Getenv("SQLCMD_PASSWORD"), c.encryptPassword),
+			Password:          secret.Encode(password, c.encryptPassword),
 		}
 	}
 

cmd/modern/root/config/add-user_test.go

@@ -10,42 +10,57 @@ import (
 	"testing"
 )
 
+// TestAddUser tests that the `sqlcmd config add-user` command
+// works as expected
 func TestAddUser(t *testing.T) {
-	os.Setenv("SQLCMD_PASSWORD", "it's-a-secret")
+
+	// SQLCMDPASSWORD is already set in the build pipelines, so don't
+	// overwrite it here.
+	if os.Getenv("SQLCMDPASSWORD") == "" {
+		os.Setenv("SQLCMDPASSWORD", "it's-a-secret")
+	}
 	cmdparser.TestSetup(t)
 	cmdparser.TestCmd[*AddUser]("--username user1")
 }
 
+// TestNegAddUser tests that the `sqlcmd config add-user` command
+// fails when the auth-type is invalid
 func TestNegAddUser(t *testing.T) {
+	cmdparser.TestSetup(t)
 	assert.Panics(t, func() {
-		cmdparser.TestSetup(t)
 		cmdparser.TestCmd[*AddUser]("--username user1 --auth-type bad-bad")
 	})
 }
 
+// TestNegAddUser2 tests that the `sqlcmd config add-user` command
+// fails when the auth-type is not basic and --encrypt-password is set
 func TestNegAddUser2(t *testing.T) {
+	cmdparser.TestSetup(t)
 	assert.Panics(t, func() {
-		cmdparser.TestSetup(t)
 		cmdparser.TestCmd[*AddUser]("--username user1 --auth-type other --encrypt-password")
 	})
 }
 
+// TestNegAddUser3 tests that the `sqlcmd config add-user` command
+// fails when the SQLCMD_PASSWORD environment variable is not set
 func TestNegAddUser3(t *testing.T) {
-	assert.Panics(t, func() {
-
-		os.Setenv("SQLCMD_PASSWORD", "")
+	if os.Getenv("SQLCMDPASSWORD") != "" {
+		os.Setenv("SQLCMDPASSWORD", "")
+	}
 
-		cmdparser.TestSetup(t)
+	cmdparser.TestSetup(t)
+	assert.Panics(t, func() {
 		cmdparser.TestCmd[*AddUser]("--username user1")
 	})
 }
 
+// TestNegAddUser4 tests that the `sqlcmd config add-user` command
+// when username is not set
 func TestNegAddUser4(t *testing.T) {
-	assert.Panics(t, func() {
-
-		os.Setenv("SQLCMD_PASSWORD", "whatever")
+	os.Setenv("SQLCMD_PASSWORD", "whatever")
 
-		cmdparser.TestSetup(t)
+	cmdparser.TestSetup(t)
+	assert.Panics(t, func() {
 		cmdparser.TestCmd[*AddUser]()
 	})
 }