Fix JDBC Detector Bugs (#4548)

* add tests * skip verification if host or password is empty * incorporate zach's suggestion * change logs verbosity level to 2 * revert unintentional change --------- Co-authored-by: Amaan Ullah <[email protected]>
2025-11-14 10:04:44 +05:00
parent e7b3dbfb0f
commit df31a0e53a
6 changed files with 385 additions and 13 deletions
@@ -51,7 +51,7 @@ func isMySQLErrorDeterminate(err error) bool {
 	return false
 }

-func parseMySQL(_ logContext.Context, subname string) (jdbc, error) {
+func parseMySQL(ctx logContext.Context, subname string) (jdbc, error) {
 	// expected form: [subprotocol:]//[user:password@]HOST[/DB][?key=val[&key=val]]
 	if !strings.HasPrefix(subname, "//") {
 		return nil, errors.New("expected host to start with //")
@@ -59,15 +59,27 @@ func parseMySQL(_ logContext.Context, subname string) (jdbc, error) {

 	// need for hostnames that have tcp(host:port) format required by this database driver
 	cfg, err := mysql.ParseDSN(strings.TrimPrefix(subname, "//"))
-	if err == nil {
-		return &mysqlJDBC{
-			conn:     subname[2:],
-			userPass: cfg.User + ":" + cfg.Passwd,
-			host:     fmt.Sprintf("tcp(%s)", cfg.Addr),
-			params:   "timeout=5s",
-		}, nil
+	if err != nil {
+		// fall back to URI parsing
+		return parseMySQLURI(ctx, subname)
 	}

+	if cfg.Addr == "" || cfg.Passwd == "" {
+		ctx.Logger().WithName("jdbc").
+			V(2).
+			Info("Skipping invalid MySQL URL - no password or host found")
+		return nil, fmt.Errorf("missing host or password in connection string")
+	}
+	return &mysqlJDBC{
+		conn:     subname[2:],
+		userPass: cfg.User + ":" + cfg.Passwd,
+		host:     fmt.Sprintf("tcp(%s)", cfg.Addr),
+		params:   "timeout=5s",
+	}, nil
+}
+
+func parseMySQLURI(ctx logContext.Context, subname string) (jdbc, error) {
+
 	// for standard URI format, which is all i've seen for JDBC
 	u, err := url.Parse(subname)
 	if err != nil {
@@ -88,11 +100,15 @@ func parseMySQL(_ logContext.Context, subname string) (jdbc, error) {
 		pass = v
 	}

-	userAndPass := user
-	if pass != "" {
-		userAndPass = userAndPass + ":" + pass
+	if u.Host == "" || pass == "" {
+		ctx.Logger().WithName("jdbc").
+			V(2).
+			Info("Skipping invalid MySQL URL - no password or host found")
+		return nil, fmt.Errorf("missing host or password in connection string")
 	}

+	userAndPass := user + ":" + pass
+
 	return &mysqlJDBC{
 		conn:     subname[2:],
 		userPass: userAndPass,
@@ -0,0 +1,118 @@
+package jdbc
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	logContext "github.com/trufflesecurity/trufflehog/v3/pkg/context"
+)
+
+func TestParseMySQLMissingCredentials(t *testing.T) {
+	tests := []struct {
+		name        string
+		subname     string
+		shouldBeNil bool
+		reason      string
+	}{
+		{
+			name:        "no password - should return nil",
+			subname:     "//examplehost.net:3306/dbname?user=admin",
+			shouldBeNil: true,
+			reason:      "no password present",
+		},
+		{
+			name:        "no password (tcp format) - should return nil",
+			subname:     "//tcp(examplehost.net:3306)/dbname?user=admin",
+			shouldBeNil: true,
+			reason:      "no password present in tcp format",
+		},
+		{
+			name:        "no host - should return nil",
+			subname:     "///dbname?user=admin&password=secret123",
+			shouldBeNil: true,
+			reason:      "no host present",
+		},
+		{
+			name:        "no host and no password - should return nil",
+			subname:     "///dbname",
+			shouldBeNil: true,
+			reason:      "no host or password present",
+		},
+		{
+			name:        "valid with host and password - should succeed",
+			subname:     "//examplehost.net:3306/dbname?user=root&password=secret123",
+			shouldBeNil: false,
+		},
+		{
+			name:        "valid with tcp(host:port) format - should succeed",
+			subname:     "//root:secret123@tcp(examplehost.net:3306)/dbname",
+			shouldBeNil: false,
+		},
+		{
+			name:        "valid with localhost - should succeed",
+			subname:     "//localhost/dbname?user=root&password=secret123",
+			shouldBeNil: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := logContext.AddLogger(context.Background())
+			j, err := parseMySQL(ctx, tt.subname)
+
+			if tt.shouldBeNil {
+				if j != nil {
+					t.Errorf("parseMySQL() expected nil (%s), got: %v", tt.reason, j)
+				}
+			} else {
+				if j == nil {
+					t.Errorf("parseMySQL() returned nil, expected valid connection. err = %v", err)
+				}
+				if err != nil {
+					t.Errorf("parseMySQL() unexpected error = %v", err)
+				}
+			}
+		})
+	}
+}
+
+func TestParseMySQLUsernameRecognition(t *testing.T) {
+	tests := []struct {
+		name         string
+		subname      string
+		wantUsername string
+	}{
+		{
+			name:         "user parameter specified",
+			subname:      "//localhost:3306/dbname?user=myuser&password=mypass",
+			wantUsername: "myuser",
+		},
+		{
+			name:         "no user specified - default root",
+			subname:      "//localhost:3306/dbname?password=mypass",
+			wantUsername: "root",
+		},
+		{
+			name:         "user specified (tcp format)",
+			subname:      "//myuser:secret123@tcp(localhost:3306)/dbname",
+			wantUsername: "myuser",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := logContext.AddLogger(context.Background())
+			j, err := parseMySQL(ctx, tt.subname)
+			if err != nil {
+				t.Fatalf("parseMySQL() error = %v", err)
+			}
+
+			mysqlConn := j.(*mysqlJDBC)
+			if !strings.Contains(mysqlConn.userPass, tt.wantUsername) {
+				t.Errorf("Connection string does not contain expected username '%s'\nGot: %s\nExpected: %s",
+					tt.wantUsername, mysqlConn.userPass, tt.wantUsername)
+			}
+		})
+	}
+}
@@ -59,7 +59,7 @@ func joinKeyValues(m map[string]string, sep string) string {
 	return strings.Join(data, sep)
 }

-func parsePostgres(_ logContext.Context, subname string) (jdbc, error) {
+func parsePostgres(ctx logContext.Context, subname string) (jdbc, error) {
 	// expected form: [subprotocol:]//[user:password@]HOST[/DB][?key=val[&key=val]]

 	if !strings.HasPrefix(subname, "//") {
@@ -107,6 +107,13 @@ func parsePostgres(_ logContext.Context, subname string) (jdbc, error) {
 		params["password"] = v
 	}

+	if params["host"] == "" || params["password"] == "" {
+		ctx.Logger().WithName("jdbc").
+			V(2).
+			Info("Skipping invalid Postgres URL - no password or host found")
+		return nil, fmt.Errorf("missing host or password in connection string")
+	}
+
 	return &postgresJDBC{subname[2:], params}, nil
 }

@@ -0,0 +1,100 @@
+package jdbc
+
+import (
+	"context"
+	"testing"
+
+	logContext "github.com/trufflesecurity/trufflehog/v3/pkg/context"
+)
+
+func TestParsePostgresMissingCredentials(t *testing.T) {
+	tests := []struct {
+		name        string
+		subname     string
+		shouldBeNil bool
+		reason      string
+	}{
+		{
+			name:        "no password - should return nil (nothing to verify)",
+			subname:     "//examplehost.net:5432/dbname?user=admin",
+			shouldBeNil: true,
+			reason:      "no password present",
+		},
+		{
+			name:        "no host - should return nil (invalid connection)",
+			subname:     "///dbname?user=admin&password=secret123",
+			shouldBeNil: true,
+			reason:      "no host present",
+		},
+		{
+			name:        "no host and no password - should return nil",
+			subname:     "///dbname",
+			shouldBeNil: true,
+			reason:      "no host or password present",
+		},
+		{
+			name:        "valid with host and password - should succeed",
+			subname:     "//examplehost.net:5432/dbname?user=admin&password=secret123",
+			shouldBeNil: false,
+		},
+		{
+			name:        "valid with localhost and password - should succeed",
+			subname:     "//localhost/dbname?user=postgres&password=secret123",
+			shouldBeNil: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := logContext.AddLogger(context.Background())
+			j, err := parsePostgres(ctx, tt.subname)
+
+			if tt.shouldBeNil {
+				if j != nil {
+					t.Errorf("parsePostgres() expected nil (%s), got: %v", tt.reason, j)
+				}
+			} else {
+				if j == nil {
+					t.Errorf("parsePostgres() returned nil, expected valid connection. err = %v", err)
+				}
+				if err != nil {
+					t.Errorf("parsePostgres() unexpected error = %v", err)
+				}
+			}
+		})
+	}
+}
+
+func TestParsePostgresUsernameRecognition(t *testing.T) {
+	tests := []struct {
+		name         string
+		subname      string
+		wantUsername string
+	}{
+		{
+			name:         "user parameter specified",
+			subname:      "//localhost:5432/dbname?user=myuser&password=mypass",
+			wantUsername: "myuser",
+		},
+		{
+			name:         "user and password specified",
+			subname:      "//myuser:mypassword@localhost:5432/dbname",
+			wantUsername: "myuser",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := logContext.AddLogger(context.Background())
+			j, err := parsePostgres(ctx, tt.subname)
+			if err != nil {
+				t.Fatalf("parsePostgres() error = %v", err)
+			}
+
+			pgConn := j.(*postgresJDBC)
+			if pgConn.params["user"] != tt.wantUsername {
+				t.Errorf("expected username '%s', got '%s'", tt.wantUsername, pgConn.params["user"])
+			}
+		})
+	}
+}
@@ -42,6 +42,7 @@ func parseSqlServer(ctx logContext.Context, subname string) (jdbc, error) {
 	conn := strings.TrimPrefix(subname, "//")

 	port := "1433"
+	user := "sa"
 	var password, host string

 	for i, param := range strings.Split(conn, ";") {
@@ -66,10 +67,20 @@ func parseSqlServer(ctx logContext.Context, subname string) (jdbc, error) {
 			host = value
 		case "port":
 			port = value
+		case "user", "uid", "user id":
+			user = value
+
 		}
 	}

-	urlStr := fmt.Sprintf("sqlserver://sa:%s@%s:%s?database=master&connection+timeout=5", password, host, port)
+	if password == "" || host == "" {
+		ctx.Logger().WithName("jdbc").
+			V(2).
+			Info("Skipping invalid SQL Server URL - no password or host found")
+		return nil, fmt.Errorf("missing host or password in connection string")
+	}
+
+	urlStr := fmt.Sprintf("sqlserver://%s:%s@%s:%s?database=master&connection+timeout=5", user, password, host, port)
 	jdbcUrl, err := url.Parse(urlStr)
 	if err != nil {
 		ctx.Logger().WithName("jdbc").
@@ -0,0 +1,120 @@
+package jdbc
+
+import (
+	"context"
+	"fmt"
+	"strings"
+	"testing"
+
+	logContext "github.com/trufflesecurity/trufflehog/v3/pkg/context"
+)
+
+func TestParseSqlServerMissingCredentials(t *testing.T) {
+	tests := []struct {
+		name        string
+		subname     string
+		shouldBeNil bool
+		reason      string
+	}{
+		{
+			name:        "no password - should return nil (nothing to verify)",
+			subname:     "//examplehost.net;databaseName=QRG1;sendStringParametersAsUnicode=false",
+			shouldBeNil: true,
+			reason:      "no password present",
+		},
+		{
+			name:        "no host - should return nil (invalid connection)",
+			subname:     "//;password=secret123",
+			shouldBeNil: true,
+			reason:      "no host present",
+		},
+		{
+			name:        "no host and no password - should return nil",
+			subname:     "//;databaseName=QRG1",
+			shouldBeNil: true,
+			reason:      "no host or password present",
+		},
+		{
+			name:        "csm-1584-example",
+			subname:     "//examplehost.net;databaseName=QRG1;sendStringParametersAsUnicode=false;loginTimeout=4;applicationName=pdal-cat-hierarchy-loader-v1;",
+			shouldBeNil: true,
+			reason:      "no password present",
+		},
+		{
+			name:        "valid with both host and password - should succeed",
+			subname:     "//examplehost.net;password=secret123",
+			shouldBeNil: false,
+		},
+		{
+			name:        "valid with host:port and password - should succeed",
+			subname:     "//examplehost.net:1433;password=secret123",
+			shouldBeNil: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := logContext.AddLogger(context.Background())
+
+			j, err := parseSqlServer(ctx, tt.subname)
+
+			if tt.shouldBeNil {
+				if j != nil {
+					t.Errorf("parseSqlServer() expected nil (%s), but got connection: %v",
+						tt.reason, j)
+				}
+			} else {
+				if j == nil {
+					t.Errorf("parseSqlServer() returned nil, expected valid connection. err = %v", err)
+				}
+				if err != nil {
+					t.Errorf("parseSqlServer() unexpected error = %v", err)
+				}
+			}
+		})
+	}
+}
+
+// This test demonstrates the username is ignored when parsing the JDBC URL. Instead the default username "sa" is always used.
+func TestParseSqlServerUserIgnoredBug2(t *testing.T) {
+	tests := []struct {
+		name         string
+		subname      string // the part after "jdbc:sqlserver:"
+		wantUsername string
+	}{
+		{
+			name:         "user parameter specified",
+			subname:      "//localhost:1433;user=myuser;password=mypass",
+			wantUsername: "myuser",
+		},
+		{
+			name:         "user id parameter specified",
+			subname:      "//localhost:1433;user id=admin;password=secret",
+			wantUsername: "admin",
+		},
+		{
+			name:         "no user specified - should default to sa",
+			subname:      "//localhost:1433;password=mypass",
+			wantUsername: "sa",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := logContext.AddLogger(context.Background())
+
+			j, err := parseSqlServer(ctx, tt.subname)
+			if err != nil {
+				t.Fatalf("parseSqlServer() error = %v", err)
+			}
+
+			sqlServerConn := j.(*sqlServerJDBC)
+			expectedPrefix := fmt.Sprintf("sqlserver://%s:", tt.wantUsername)
+
+			if !strings.Contains(sqlServerConn.connStr, expectedPrefix) {
+				t.Errorf("Connection string does not contain expected username '%s'\nGot: %s\nExpected to contain: %s",
+					tt.wantUsername, sqlServerConn.connStr, expectedPrefix)
+			}
+		})
+	}
+}