From df31a0e53a1efa49f2396699d51254455bb81868 Mon Sep 17 00:00:00 2001 From: Mustansir Date: Fri, 14 Nov 2025 10:04:44 +0500 Subject: [PATCH] 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 --- pkg/detectors/jdbc/mysql.go | 38 ++++++--- pkg/detectors/jdbc/mysql_test.go | 118 ++++++++++++++++++++++++++ pkg/detectors/jdbc/postgres.go | 9 +- pkg/detectors/jdbc/postgres_test.go | 100 ++++++++++++++++++++++ pkg/detectors/jdbc/sqlserver.go | 13 ++- pkg/detectors/jdbc/sqlserver_test.go | 120 +++++++++++++++++++++++++++ 6 files changed, 385 insertions(+), 13 deletions(-) create mode 100644 pkg/detectors/jdbc/mysql_test.go create mode 100644 pkg/detectors/jdbc/postgres_test.go create mode 100644 pkg/detectors/jdbc/sqlserver_test.go diff --git a/pkg/detectors/jdbc/mysql.go b/pkg/detectors/jdbc/mysql.go index 605720160..dbfe9241c 100644 --- a/pkg/detectors/jdbc/mysql.go +++ b/pkg/detectors/jdbc/mysql.go @@ -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, diff --git a/pkg/detectors/jdbc/mysql_test.go b/pkg/detectors/jdbc/mysql_test.go new file mode 100644 index 000000000..52a3c79e9 --- /dev/null +++ b/pkg/detectors/jdbc/mysql_test.go @@ -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) + } + }) + } +} diff --git a/pkg/detectors/jdbc/postgres.go b/pkg/detectors/jdbc/postgres.go index 9e262e85e..d196dc257 100644 --- a/pkg/detectors/jdbc/postgres.go +++ b/pkg/detectors/jdbc/postgres.go @@ -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 } diff --git a/pkg/detectors/jdbc/postgres_test.go b/pkg/detectors/jdbc/postgres_test.go new file mode 100644 index 000000000..19099b6a4 --- /dev/null +++ b/pkg/detectors/jdbc/postgres_test.go @@ -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"]) + } + }) + } +} diff --git a/pkg/detectors/jdbc/sqlserver.go b/pkg/detectors/jdbc/sqlserver.go index 4178b99f1..5f4d37e75 100644 --- a/pkg/detectors/jdbc/sqlserver.go +++ b/pkg/detectors/jdbc/sqlserver.go @@ -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"). diff --git a/pkg/detectors/jdbc/sqlserver_test.go b/pkg/detectors/jdbc/sqlserver_test.go new file mode 100644 index 000000000..7522d105e --- /dev/null +++ b/pkg/detectors/jdbc/sqlserver_test.go @@ -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) + } + }) + } +}