Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Learnt from: AmanPrasad43
Repo: Ginger-Automation/Ginger PR: 4286
File: Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs:287-309
Timestamp: 2025-08-29T09:35:46.020Z
Learning: In Ginger ZAP security testing, alert names should preserve their original formatting with spaces and hyphens. When comparing alert names in EvaluateScanResultAPI and similar methods, use StringComparison.OrdinalIgnoreCase for case-insensitive matching but do not normalize by removing spaces or hyphens, as alert names like "False Positive" need to remain as separate words. Confirmed by AmanPrasad43 in PR #4286.

Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4011
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:102-105
Timestamp: 2024-11-29T07:45:52.303Z
Learning: When reviewing the password encryption implementation in `GingerCoreNET/RunLib/CLILib`, consider that the existing approach is acceptable, and changes to enhance security aspects are not required.

Ginger/GingerCoreNET/Database/DatabaseOperations.cs (3)

274-285: The new helper method is not being used, leaving duplication unresolved.

This code segment duplicates the exact logic now present in GetMSSQLConnectionStringBuilder() (lines 574-580). The past review comment specifically requested extracting this duplication into a helper, which was done, but CreateConnectionString() was not updated to use it.

Apply this diff to use the helper:

                     case eDBTypes.MSSQL:
-                        var builder = new SqlConnectionStringBuilder
-                        {
-                            DataSource = TNSCalculated
-                        };
-
-                        if (!string.IsNullOrEmpty(Database.Name)) builder.InitialCatalog = Database.Name;
-                        builder.IntegratedSecurity = false;
-                        builder.UserID = UserCalculated;
-                        builder.Password = EncryptionHandler.DecryptwithKey(PassCalculated);
+                        var builder = GetMSSQLConnectionStringBuilder();
                         Database.ConnectionString = builder.ConnectionString;
                         break;

---

432-433: PostgreSQL flow now stores plaintext password in Database.ConnectionString.

Lines 432-433 now pass the decrypted password directly to the builder, and line 436 assigns pg.ConnectionString (containing the plaintext password) to Database.ConnectionString. This mirrors the concern raised in a previous review: if Database is serialized or logged elsewhere, the plaintext password may be exposed.

Consider using a local variable for the resolved connection string:

                         var pg = new NpgsqlConnectionStringBuilder
                         {
                             Host = postgreSQLHost,
                             Database = Database.Name ?? string.Empty,
                             Username = UserCalculated,
                             Password = EncryptionHandler.DecryptwithKey(PassCalculated)
                         };
                         if (port.HasValue) pg.Port = port.Value;
-                        Database.ConnectionString = pg.ConnectionString;
 
                         oConn = new NpgsqlConnection(pg.ConnectionString);

Also applies to: 436-436

---

486-487: MySQL flow now stores plaintext password in Database.ConnectionString.

Similar to the PostgreSQL case, lines 486-487 pass the decrypted password directly to the builder, and line 490 assigns the connection string containing the plaintext password to Database.ConnectionString. This creates the same security exposure if Database is serialized or logged.

Apply a similar fix:

                         var my = new MySqlConnectionStringBuilder
                         {
                             Server = mySQLHost,
                             Database = Database.Name ?? string.Empty,
                             UserID = UserCalculated,
                             Password = EncryptionHandler.DecryptwithKey(PassCalculated)
                         };
                         if (port1.HasValue) my.Port = port1.Value;
-                        Database.ConnectionString = my.ConnectionString;
 
                         oConn = new MySqlConnection
                         {
                             ConnectionString = my.ConnectionString
                         };

Also applies to: 490-490

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Learnt from: AmanPrasad43
Repo: Ginger-Automation/Ginger PR: 4286
File: Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs:287-309
Timestamp: 2025-08-29T09:35:46.020Z
Learning: In Ginger ZAP security testing, alert names should preserve their original formatting with spaces and hyphens. When comparing alert names in EvaluateScanResultAPI and similar methods, use StringComparison.OrdinalIgnoreCase for case-insensitive matching but do not normalize by removing spaces or hyphens, as alert names like "False Positive" need to remain as separate words. Confirmed by AmanPrasad43 in PR #4286.

Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4011
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:102-105
Timestamp: 2024-11-29T07:45:52.303Z
Learning: When reviewing the password encryption implementation in `GingerCoreNET/RunLib/CLILib`, consider that the existing approach is acceptable, and changes to enhance security aspects are not required.