Skip to content

Conversation

@rathimayur
Copy link
Contributor

@rathimayur rathimayur commented Dec 10, 2025

Description

Type of Change

  • Bug fix - [ ] New feature - [ ] Breaking change - [ ] Plugin update

Checklist

  • PR description clearly describes the changes
  • Target branch is correct (master for features, Releases/* for fixes)
  • Latest code from target branch merged
  • No commented/junk code included
  • No new build warnings or errors
  • All existing unit tests pass
  • New unit tests added for new functionality
  • Cross-platform compatibility verified (Windows/Linux/macOS)
  • CI/CD pipeline passes
  • Code follows project conventions (Act{Platform}{Type}, {Platform}Driver)
  • Repository objects use [IsSerializedForLocalRepository] where needed
  • Error handling uses Reporter.ToLog() pattern
  • Documentation updated for user-facing changes
  • Self-review completed and code review comments addressed

Summary by CodeRabbit

  • Bug Fixes

    • Improved database connection robustness with error handling, host/port validation, and safe fallbacks across multiple database types.
  • Refactor

    • Switched to type-specific, validated connection string construction and updated connection flow to recompute strings before opening connections.
  • Chores

    • Removed the built-in static HTML report generation UI and related report assets (templates, stylesheets, chart scripts and supporting libraries).

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 10, 2025

Walkthrough

Refactored DatabaseOperations to build DB-type-specific connection strings with host/port validation and defensive TNS parsing; Connect now recomputes connection strings for several DB types. Large set of GingerExecutionReport HTML templates, their C# report classes, and report assets (CSS/JS/chart scripts) were removed.

Changes

Cohort / File(s) Summary
Database connection string refactor
Ginger/GingerCoreNET/Database/DatabaseOperations.cs
Replaced ad‑hoc connection-string assembly with DB-type switch and type-specific builders (MSAccess, DB2, PostgreSQL, MySQL, CosmosDb, Hbase, MSSQL); added ValidateHostPort(string, int?); wrapped TNS parsing in try/catch; Connect recomputes/assigns connection strings for MSSQL/PostgreSQL/MySQL; added defensive fallback and logging.
Removed HTML report templates
Ginger/Ginger/Reports/GingerExecutionReport/ActionReport.html, .../ActivityGroupReport.html, .../ActivityReport.html, .../BusinessFlowReport.html, .../GingerExecutionReport.html, .../GingerRunnerReport.html, .../RunSetReport.html
Deleted static HTML templates for execution reports, including placeholders, inline scripts, modal logic, and navigation markup.
Removed C# HTML report classes
Ginger/Ginger/Reports/GingerExecutionReport/HTMLReports/HTMLDetailedReport.cs, .../HTMLReportBase.cs, .../HTMLSummaryReport.cs
Removed HTML report classes and their CreateReport implementations plus related helper methods and properties tied to ReportInfo.
Removed/modified report assets (CSS/JS/chart scripts)
Ginger/Ginger/Reports/GingerExecutionReport/assets/css/styles.css, .../assets/css/font-awesome.css, .../assets/js/jquery.js, .../assets/js/donutchart.js, .../assets/js/circlechart.js, .../assets/js/bootstrap.js
Deleted large CSS files, font-awesome, jQuery, donut/circle chart renderers; bootstrap.js restructured into IIFEs while retaining component code.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Areas requiring extra attention:

  • GingerCoreNET/Database/DatabaseOperations.cs: correctness of each DB-type builder path, credential handling, host/port parsing, IPv6 handling, and fallback logging.
  • ValidateHostPort behavior and edge cases (hostnames, empty host, port bounds).
  • Ensure no remaining references to removed HTML templates, C# report classes, jQuery, or chart assets in code or runtime paths.

Possibly related PRs

Suggested reviewers

  • ravirk91
  • Maheshkale447

Poem

🐰 I hopped through strings of host and port,
I nudged TNS softly when it fell short.
Builders stitched connections neat and bright,
Old pages vanished into the night.
Nibble, test, deploy — the burrow’s just right! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is entirely empty, containing only the template structure with no actual details of changes, type of change selection, or substantive content. Fill in the Description section with a clear summary of changes, select the appropriate Type of Change checkbox, and verify all relevant checklist items before submission.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title is vague and does not clearly convey the main change; 'Git Alerts' is non-specific and the changes show extensive HTML/CSS/JS file deletions alongside database connection string improvements. Clarify the title to reflect the primary change, such as 'Refactor database connection string handling with type-specific builders' or similar, depending on the actual main objective.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch DBOperations-GitAlertsFix

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 80d6bb9 and 844ca5f.

📒 Files selected for processing (1)
  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs (5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use Reporter.ToLog(eLogLevel.ERROR, message) for logging errors

Files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
🧠 Learnings (6)
📚 Learning: 2025-07-09T14:46:34.133Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4245
File: Ginger/GingerCoreNET/ValueExpressionLib/ValueExpression.cs:1533-1535
Timestamp: 2025-07-09T14:46:34.133Z
Learning: In the ValueExpression.cs ReplaceEnvDBWithValue() method, error cases that return without replacing the original expression in mValueCalculated are intentional behavior and working as expected, rather than being inconsistent error handling that should be fixed.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
📚 Learning: 2024-11-29T07:45:52.303Z
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.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
📚 Learning: 2024-07-08T13:53:26.335Z
Learnt from: IamRanjeetSingh
Repo: Ginger-Automation/Ginger PR: 3811
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs:392-407
Timestamp: 2024-07-08T13:53:26.335Z
Learning: When suggesting to avoid throwing `System.Exception` directly, if the user defers the change, acknowledge their decision and note that the change might be considered in future revisions.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
📚 Learning: 2024-09-16T10:13:19.599Z
Learnt from: IamRanjeetSingh
Repo: Ginger-Automation/Ginger PR: 3897
File: Ginger/GingerCoreNET/Telemetry/TelemetryQueue.cs:155-186
Timestamp: 2024-09-16T10:13:19.599Z
Learning: In this codebase, methods prefixed with `Try` (e.g., `TryAddToDBAsync`, `TrySendToCollectorAsync`, `TryDeleteRecordsFromDBAsync`) internally handle exceptions, so additional exception handling in the calling methods is unnecessary.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
📚 Learning: 2025-09-03T17:37:09.279Z
Learnt from: AmanPrasad43
Repo: Ginger-Automation/Ginger PR: 4290
File: Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs:34-39
Timestamp: 2025-09-03T17:37:09.279Z
Learning: In Ginger's ZAP integration, when GetPortFromUrl() returns null (URL without port), avoid using hardcoded fallback ports as they may conflict with other applications. Instead, require explicit port specification in the ZAP configuration and provide clear error messages when port is missing.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
📚 Learning: 2025-02-17T12:13:55.231Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4110
File: Ginger/GingerCoreCommon/ReporterLib/WorkSpaceReporterBase.cs:85-89
Timestamp: 2025-02-17T12:13:55.231Z
Learning: The error handling in WorkSpaceReporterBase.ToConsole() should be kept simple with basic exception catching and console output reset, as per team's preference.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs (2)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
  • Reporter (28-357)
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DatabaseConfigHelper.cs (1)
  • eDBTypes (52-69)
🪛 ast-grep (0.40.0)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs

[warning] 280-280: A secret is hard-coded in the application. Secrets stored in source code, such as credentials, identifiers, and other types of sensitive data, can be leaked and used by internal or external malicious actors. Use environment variables to securely provide credentials and other secrets or retrieve them from a secure vault or Hardware Security Module (HSM).
Context: builder.Password = "{PASS}"
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html

(sqlconnectionstringbuilder-hardcoded-secret-csharp)


[warning] 351-351: A secret is hard-coded in the application. Secrets stored in source code, such as credentials, identifiers, and other types of sensitive data, can be leaked and used by internal or external malicious actors. Use environment variables to securely provide credentials and other secrets or retrieve them from a secure vault or Hardware Security Module (HSM).
Context: builder.Password = "{PASS}"
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html

(sqlconnectionstringbuilder-hardcoded-secret-csharp)

🔇 Additional comments (5)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs (5)

198-202: DB2 now correctly uses TNSCalculated

Using TNSCalculated instead of raw Database.TNS keeps DB2 consistent with other providers and ensures value expressions are evaluated before building the connection string.


204-226: PostgreSQL connection string construction with host/port validation looks correct

Host/port parsing from TNSCalculated, validation via ValidateHostPort, and use of NpgsqlConnectionStringBuilder with {USER}/{PASS} placeholders align well with the overall GetConnectionString pipeline and should behave predictably for host and host:port forms.


228-251: MySQL connection string construction is consistent and fixes the prior port bug

The MySQL branch now:

  • Parses host[:port] from TNSCalculated,
  • Validates the port via ValidateHostPort (with correct assignment to the local port variable), and
  • Uses MySqlConnectionStringBuilder with {USER}/{PASS} placeholders.

This addresses the earlier issue where the parsed port was written to the wrong variable.


176-178: CreateConnectionString now logs failures and falls back safely

Wrapping the DB-type-specific switch in a try/catch and logging via Reporter.ToLog(eLogLevel.ERROR, ...) before falling back to a generic "Data Source=...;User Id={USER};Password={PASS};" string improves diagnosability and avoids silent misconfiguration.

Also applies to: 286-294


299-306: ValidateHostPort is a reasonable central guard for host/port sanity

Centralizing host non-emptiness and port range checks in ValidateHostPort keeps PostgreSQL and MySQL parsing defensive and consistent. Exception types and messages are also appropriate for the calling contexts.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b1b6c93 and 54e2de3.

📒 Files selected for processing (1)
  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs (5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use Reporter.ToLog(eLogLevel.ERROR, message) for logging errors

Files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs (1)
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DatabaseConfigHelper.cs (1)
  • eDBTypes (52-69)
🪛 ast-grep (0.40.0)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs

[warning] 280-280: A secret is hard-coded in the application. Secrets stored in source code, such as credentials, identifiers, and other types of sensitive data, can be leaked and used by internal or external malicious actors. Use environment variables to securely provide credentials and other secrets or retrieve them from a secure vault or Hardware Security Module (HSM).
Context: builder.Password = "{PASS}"
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html

(sqlconnectionstringbuilder-hardcoded-secret-csharp)


[warning] 350-350: A secret is hard-coded in the application. Secrets stored in source code, such as credentials, identifiers, and other types of sensitive data, can be leaked and used by internal or external malicious actors. Use environment variables to securely provide credentials and other secrets or retrieve them from a secure vault or Hardware Security Module (HSM).
Context: builder.Password = "{PASS}"
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html

(sqlconnectionstringbuilder-hardcoded-secret-csharp)

🔇 Additional comments (2)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs (2)

298-305: LGTM!

The ValidateHostPort helper method provides appropriate validation for host and port values with proper boundary checking for port range (1-65535).


257-272: Remove commented-out code block.

Lines 264-272 contain a large commented-out code block. If this alternative implementation is no longer needed, it should be removed.

⛔ Skipped due to learnings
Learnt from: manas-droid
Repo: Ginger-Automation/Ginger PR: 3436
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:6702-6749
Timestamp: 2024-07-26T22:04:12.930Z
Learning: The user has indicated that the commented `CheckifPageLoaded` method is needed for later use, suggesting a requirement to retain the code within the source files for reference or potential re-implementation.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (7)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs (7)

134-147: Syntax error and missing error logging (already flagged).

The issues in this block were identified in a previous review:

  1. Line 136: SqlConnectionStringBuilder scSB = []; is invalid syntax—use new SqlConnectionStringBuilder()
  2. Lines 143-146: Empty catch block swallows exceptions without logging

Per coding guidelines, log errors using Reporter.ToLog.


198-202: Inconsistent use of Database.TNS (already flagged).

This issue was identified in a previous review—use TNSCalculated for consistency with other DB types.


253-255: CosmosDb uses raw credentials (already flagged).

This issue was identified in a previous review—use placeholders {USER} and {PASS} for consistency with other DB types.


289-293: Silent fallback without logging (already flagged).

This issue was identified in a previous review. Per coding guidelines, log the exception using Reporter.ToLog(eLogLevel.ERROR, ...) before falling back.


346-352: Code duplication with CreateConnectionString (already flagged).

This MSSQL connection string building duplicates logic from CreateConnectionString (lines 274-284). Consider removing this duplicate code since GetConnectionString() already handles this.


425-443: Code duplication with CreateConnectionString (already flagged).

This PostgreSQL connection string building duplicates logic from CreateConnectionString (lines 204-226).


483-502: Critical bug: MySQL port is never assigned (already flagged).

Line 489 assigns to port instead of port1, causing:

  1. port1 remains null forever
  2. Validation on line 492 always validates with null port
  3. Port is never set on the connection string (line 501 condition is always false)

Additionally, line 489 uses int.TryParse but port1 is uint?—use uint.TryParse for type consistency.

                     if (TNSCalculated.Contains(':', StringComparison.Ordinal))
                     {
                         var parts = TNSCalculated.Split(':', 2);
                         mySQLHost = parts[0];
-                        if (int.TryParse(parts[1], out int p)) port = p;
+                        if (uint.TryParse(parts[1], out uint p)) port1 = p;
                     }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 54e2de3 and 2aef88e.

📒 Files selected for processing (1)
  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs (5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use Reporter.ToLog(eLogLevel.ERROR, message) for logging errors

Files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
🧠 Learnings (5)
📚 Learning: 2025-07-09T14:46:34.133Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4245
File: Ginger/GingerCoreNET/ValueExpressionLib/ValueExpression.cs:1533-1535
Timestamp: 2025-07-09T14:46:34.133Z
Learning: In the ValueExpression.cs ReplaceEnvDBWithValue() method, error cases that return without replacing the original expression in mValueCalculated are intentional behavior and working as expected, rather than being inconsistent error handling that should be fixed.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
📚 Learning: 2024-11-29T07:45:52.303Z
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.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
📚 Learning: 2024-07-08T13:53:26.335Z
Learnt from: IamRanjeetSingh
Repo: Ginger-Automation/Ginger PR: 3811
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs:392-407
Timestamp: 2024-07-08T13:53:26.335Z
Learning: When suggesting to avoid throwing `System.Exception` directly, if the user defers the change, acknowledge their decision and note that the change might be considered in future revisions.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
📚 Learning: 2024-09-16T10:13:19.599Z
Learnt from: IamRanjeetSingh
Repo: Ginger-Automation/Ginger PR: 3897
File: Ginger/GingerCoreNET/Telemetry/TelemetryQueue.cs:155-186
Timestamp: 2024-09-16T10:13:19.599Z
Learning: In this codebase, methods prefixed with `Try` (e.g., `TryAddToDBAsync`, `TrySendToCollectorAsync`, `TryDeleteRecordsFromDBAsync`) internally handle exceptions, so additional exception handling in the calling methods is unnecessary.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
📚 Learning: 2025-09-03T17:37:09.279Z
Learnt from: AmanPrasad43
Repo: Ginger-Automation/Ginger PR: 4290
File: Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs:34-39
Timestamp: 2025-09-03T17:37:09.279Z
Learning: In Ginger's ZAP integration, when GetPortFromUrl() returns null (URL without port), avoid using hardcoded fallback ports as they may conflict with other applications. Instead, require explicit port specification in the ZAP configuration and provide clear error messages when port is missing.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs (1)
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DatabaseConfigHelper.cs (1)
  • eDBTypes (52-69)
🪛 ast-grep (0.40.0)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs

[warning] 280-280: A secret is hard-coded in the application. Secrets stored in source code, such as credentials, identifiers, and other types of sensitive data, can be leaked and used by internal or external malicious actors. Use environment variables to securely provide credentials and other secrets or retrieve them from a secure vault or Hardware Security Module (HSM).
Context: builder.Password = "{PASS}"
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html

(sqlconnectionstringbuilder-hardcoded-secret-csharp)


[warning] 350-350: A secret is hard-coded in the application. Secrets stored in source code, such as credentials, identifiers, and other types of sensitive data, can be leaked and used by internal or external malicious actors. Use environment variables to securely provide credentials and other secrets or retrieve them from a secure vault or Hardware Security Module (HSM).
Context: builder.Password = "{PASS}"
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html

(sqlconnectionstringbuilder-hardcoded-secret-csharp)

🔇 Additional comments (3)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs (3)

204-251: LGTM!

The PostgreSQL and MySQL connection string construction is well-structured: proper use of type-specific builders, host/port parsing with validation via ValidateHostPort, and consistent placeholder pattern for credentials.


274-284: LGTM!

The MSSQL connection string construction using SqlConnectionStringBuilder is correct. The {PASS} placeholder on line 281 is intentionally replaced by ReplacePasswordInConnectionString() and is not a hardcoded secret (static analysis false positive).


298-305: LGTM!

The ValidateHostPort helper provides clear validation with appropriate exceptions for invalid host or port values.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (6)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs (6)

358-376: Code duplication with CreateConnectionString method.

Lines 359-365 duplicate the MSSQL connection string building logic that exists in CreateConnectionString() (lines 286-296). Since GetConnectionString() at line 368 already invokes CreateConnectionString() when ConnectionStringCalculated is empty, this pre-assignment is redundant and creates a maintenance burden.

Remove the duplicate code:

                 case eDBTypes.MSSQL:
-                    var builder = new SqlConnectionStringBuilder();
-                    builder.DataSource = TNSCalculated;
-                    if (!string.IsNullOrEmpty(Database.Name)) builder.InitialCatalog = Database.Name;
-                    builder.IntegratedSecurity = false;
-                    builder.UserID = "{USER}";
-                    builder.Password = "{PASS}";
-                    Database.ConnectionString = builder.ConnectionString;
                     var sqlConnectionStringBuilder = new SqlConnectionStringBuilder
                     {
                         ConnectionString = GetConnectionString()
                     };

Note: The static analysis warnings about hardcoded secrets ({PASS}) are false positives—these are placeholders replaced by ReplacePasswordInConnectionString().


437-465: Code duplication with CreateConnectionString method.

Lines 438-456 duplicate the PostgreSQL connection string building logic that exists in CreateConnectionString() (lines 216-238). This creates a maintenance burden where any changes must be made in two places.

Remove the duplicate code:

                 case eDBTypes.PostgreSQL:
-                    string postgreSQLHost = TNSCalculated;
-                    int? port = null;
-                    if (TNSCalculated.Contains(':', StringComparison.Ordinal))
-                    {
-                        var parts = TNSCalculated.Split(':', 2);
-                        postgreSQLHost = parts[0];
-                        if (int.TryParse(parts[1], out int p)) port = p;
-                    }
-                    ValidateHostPort(postgreSQLHost, port);
-
-                    var pg = new NpgsqlConnectionStringBuilder
-                    {
-                        Host = postgreSQLHost,
-                        Database = Database.Name ?? string.Empty,
-                        Username = "{USER}",
-                        Password = "{PASS}"
-                    };
-                    if (port.HasValue) pg.Port = port.Value;
-                    Database.ConnectionString = pg.ConnectionString;
-
                     var npgsqlconnectionStringBuilder = new Npgsql.NpgsqlConnectionStringBuilder
                     {
                         ConnectionString = GetConnectionString()
                     };

192-208: MSAccess connection string is incomplete—missing data source.

The active code only prepends a provider to Database.ConnectionString without setting the data source (TNSCalculated). If Database.ConnectionString is empty or doesn't already contain a data source, the resulting connection string will be incomplete and the connection will fail. The commented-out code (lines 194-199) should either be removed or the implementation should be completed.

Apply this diff to complete the implementation:

                 case eDBTypes.MSAccess:
                     {
-                        //var ole = new OleDbConnectionStringBuilder();
-                        //ole.Provider = TNSCalculated.Contains(".accdb") 
-                        //    ? "Microsoft.ACE.OLEDB.12.0":
-                        //    "Microsoft.Jet.OLEDB.4.0";
-                        //ole.DataSource = TNSCalculated;
-                        //Database.ConnectionString = ole.ConnectionString + ";User Id={USER};Password={PASS};";
-
-                        string strProvider;
-                        strProvider = TNSCalculated.Contains(".accdb")
-                            ? "Provider=Microsoft.ACE.OLEDB.12.0;"
-                            : "Provider=Microsoft.Jet.OLEDB.4.0;";
-
-                        Database.ConnectionString = strProvider + Database.ConnectionString;
+                        var ole = new OleDbConnectionStringBuilder
+                        {
+                            Provider = TNSCalculated.Contains(".accdb")
+                                ? "Microsoft.ACE.OLEDB.12.0"
+                                : "Microsoft.Jet.OLEDB.4.0",
+                            DataSource = TNSCalculated
+                        };
+                        Database.ConnectionString = ole.ConnectionString + ";User Id={USER};Password={PASS};";
                         break;
                     }

265-267: CosmosDb bypasses credential decryption—uses raw Database.User and Database.Pass.

This case uses Database.User and Database.Pass directly instead of {USER} and {PASS} placeholders. This bypasses the decryption logic in GetConnectionString() where ReplacePasswordInConnectionString() decrypts and substitutes credentials. This is inconsistent with all other database types and will fail if credentials are encrypted.

Apply this diff:

                 case eDBTypes.CosmosDb:
-                    Database.ConnectionString = $"AccountEndpoint={Database.User};AccountKey={Database.Pass}";
+                    Database.ConnectionString = "AccountEndpoint={USER};AccountKey={PASS}";
                     break;

269-284: Hbase: Missing fallback for TNS without port and dead commented code.

Two issues:

  1. If TNSCalculated doesn't contain a colon (i.e., host.Length != 2), Database.ConnectionString is never set, leaving it potentially null or stale.
  2. Lines 276-284 contain commented-out code that should be removed.

Apply this diff:

                 case eDBTypes.Hbase:
                     var host = TNSCalculated.Split(':');
                     if (host.Length == 2)
                     {
                         Database.ConnectionString = "Server=" + host[0] + ";Port=" + host[1] + ";User Id={USER}; Password={PASS};Database=" + Database.Name + ";";
                     }
+                    else
+                    {
+                        throw new ArgumentException("Hbase TNS must be in host:port format.");
+                    }
                     break;
-                    //{
-                    //    var hostParts = TNSCalculated.Split(':');
-                    //    if (hostParts.Length == 2)
-                    //    {
-                    //        ValidateHostPort(hostParts[0], int.TryParse(hostParts[1], out int p) ? (int?)p : null);
-                    //        Database.ConnectionString = $"Server={hostParts[0]};Port={hostParts[1]};User Id={UserCalculated}; Password={{PASS}};Database={Database.Name};";
-                    //    }
-                    //    break;
-                    //}

496-516: Code duplication with CreateConnectionString method.

Lines 496-515 duplicate the MySQL connection string building logic that exists in CreateConnectionString() (lines 240-263). This is the same DRY violation pattern seen in MSSQL and PostgreSQL cases. Since GetConnectionString() at line 519 already invokes CreateConnectionString() when needed, this pre-assignment is redundant.

Remove the duplicate code:

                 case eDBTypes.MySQL:
-                    string mySQLHost = TNSCalculated;
-                    uint? port1 = null;
-                    if (TNSCalculated.Contains(':', StringComparison.Ordinal))
-                    {
-                        var parts = TNSCalculated.Split(':', 2);
-                        mySQLHost = parts[0];
-                        if (uint.TryParse(parts[1], out uint p)) port1 = p;
-                    }
-
-                    ValidateHostPort(mySQLHost, port1.HasValue ? (int?)port1.Value : null);
-
-                    var my = new MySqlConnectionStringBuilder
-                    {
-                        Server = mySQLHost,
-                        Database = Database.Name ?? string.Empty,
-                        UserID = "{USER}",
-                        Password = "{PASS}"
-                    };
-                    if (port1.HasValue) my.Port = port1.Value;
-                    Database.ConnectionString = my.ConnectionString;
-
                     var mySqlconnectionStringBuilder = new MySqlConnectionStringBuilder
                     {
                         ConnectionString = GetConnectionString()
                     };
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2aef88e and 80d6bb9.

⛔ Files ignored due to path filters (16)
  • Ginger/Ginger/Reports/GingerExecutionReport/Images/GingerLogo.png is excluded by !**/*.png, !**/*.png
  • Ginger/Ginger/Reports/GingerExecutionReport/PreviewDummyReport/RunSet.zip is excluded by !**/*.zip, !**/*.zip
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/fonts/SourceSansPro-Regular.ttf is excluded by !**/*.ttf, !**/*.ttf
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/fonts/fontawesome-webfont.eot is excluded by !**/*.eot, !**/*.eot
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/fonts/fontawesome-webfont.ttf is excluded by !**/*.ttf, !**/*.ttf
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/img/@BeatLogo - Copy.png is excluded by !**/*.png, !**/*.png
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/img/@BeatLogo.png is excluded by !**/*.png, !**/*.png
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/img/@Ginger - Copy.png is excluded by !**/*.png, !**/*.png
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/img/@Ginger.png is excluded by !**/*.png, !**/*.png
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/img/@item_next.png is excluded by !**/*.png, !**/*.png
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/img/@item_prev.png is excluded by !**/*.png, !**/*.png
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/img/comments-icon.jpg is excluded by !**/*.jpg, !**/*.jpg
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/js/Recharts.min.js is excluded by !**/*.min.js, !**/*.min.js
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/js/prop-types.min.js is excluded by !**/*.min.js, !**/*.min.js
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/js/react-dom.min.js is excluded by !**/*.min.js, !**/*.min.js
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/js/react-with-addons.min.js is excluded by !**/*.min.js, !**/*.min.js
📒 Files selected for processing (17)
  • Ginger/Ginger/Reports/GingerExecutionReport/ActionReport.html (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/ActivityGroupReport.html (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/ActivityReport.html (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/BusinessFlowReport.html (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/GingerExecutionReport.html (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/GingerRunnerReport.html (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/HTMLReports/HTMLDetailedReport.cs (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/HTMLReports/HTMLReportBase.cs (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/HTMLReports/HTMLSummaryReport.cs (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/RunSetReport.html (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/css/font-awesome.css (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/css/styles.css (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/js/bootstrap.js (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/js/circlechart.js (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/js/donutchart.js (0 hunks)
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/js/jquery.js (0 hunks)
  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs (5 hunks)
💤 Files with no reviewable changes (16)
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/css/font-awesome.css
  • Ginger/Ginger/Reports/GingerExecutionReport/RunSetReport.html
  • Ginger/Ginger/Reports/GingerExecutionReport/ActionReport.html
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/js/jquery.js
  • Ginger/Ginger/Reports/GingerExecutionReport/ActivityReport.html
  • Ginger/Ginger/Reports/GingerExecutionReport/HTMLReports/HTMLSummaryReport.cs
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/js/circlechart.js
  • Ginger/Ginger/Reports/GingerExecutionReport/GingerRunnerReport.html
  • Ginger/Ginger/Reports/GingerExecutionReport/GingerExecutionReport.html
  • Ginger/Ginger/Reports/GingerExecutionReport/ActivityGroupReport.html
  • Ginger/Ginger/Reports/GingerExecutionReport/BusinessFlowReport.html
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/js/donutchart.js
  • Ginger/Ginger/Reports/GingerExecutionReport/HTMLReports/HTMLDetailedReport.cs
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/css/styles.css
  • Ginger/Ginger/Reports/GingerExecutionReport/assets/js/bootstrap.js
  • Ginger/Ginger/Reports/GingerExecutionReport/HTMLReports/HTMLReportBase.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use Reporter.ToLog(eLogLevel.ERROR, message) for logging errors

Files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
🧠 Learnings (6)
📓 Common learnings
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.
📚 Learning: 2025-07-09T14:46:34.133Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4245
File: Ginger/GingerCoreNET/ValueExpressionLib/ValueExpression.cs:1533-1535
Timestamp: 2025-07-09T14:46:34.133Z
Learning: In the ValueExpression.cs ReplaceEnvDBWithValue() method, error cases that return without replacing the original expression in mValueCalculated are intentional behavior and working as expected, rather than being inconsistent error handling that should be fixed.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
📚 Learning: 2024-11-29T07:45:52.303Z
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.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
📚 Learning: 2024-07-08T13:53:26.335Z
Learnt from: IamRanjeetSingh
Repo: Ginger-Automation/Ginger PR: 3811
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs:392-407
Timestamp: 2024-07-08T13:53:26.335Z
Learning: When suggesting to avoid throwing `System.Exception` directly, if the user defers the change, acknowledge their decision and note that the change might be considered in future revisions.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
📚 Learning: 2024-09-16T10:13:19.599Z
Learnt from: IamRanjeetSingh
Repo: Ginger-Automation/Ginger PR: 3897
File: Ginger/GingerCoreNET/Telemetry/TelemetryQueue.cs:155-186
Timestamp: 2024-09-16T10:13:19.599Z
Learning: In this codebase, methods prefixed with `Try` (e.g., `TryAddToDBAsync`, `TrySendToCollectorAsync`, `TryDeleteRecordsFromDBAsync`) internally handle exceptions, so additional exception handling in the calling methods is unnecessary.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
📚 Learning: 2025-09-03T17:37:09.279Z
Learnt from: AmanPrasad43
Repo: Ginger-Automation/Ginger PR: 4290
File: Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs:34-39
Timestamp: 2025-09-03T17:37:09.279Z
Learning: In Ginger's ZAP integration, when GetPortFromUrl() returns null (URL without port), avoid using hardcoded fallback ports as they may conflict with other applications. Instead, require explicit port specification in the ZAP configuration and provide clear error messages when port is missing.

Applied to files:

  • Ginger/GingerCoreNET/Database/DatabaseOperations.cs
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs (2)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
  • Reporter (28-357)
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DatabaseConfigHelper.cs (1)
  • eDBTypes (52-69)
🪛 ast-grep (0.40.0)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs

[warning] 292-292: A secret is hard-coded in the application. Secrets stored in source code, such as credentials, identifiers, and other types of sensitive data, can be leaked and used by internal or external malicious actors. Use environment variables to securely provide credentials and other secrets or retrieve them from a secure vault or Hardware Security Module (HSM).
Context: builder.Password = "{PASS}"
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html

(sqlconnectionstringbuilder-hardcoded-secret-csharp)


[warning] 363-363: A secret is hard-coded in the application. Secrets stored in source code, such as credentials, identifiers, and other types of sensitive data, can be leaked and used by internal or external malicious actors. Use environment variables to securely provide credentials and other secrets or retrieve them from a secure vault or Hardware Security Module (HSM).
Context: builder.Password = "{PASS}"
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html

(sqlconnectionstringbuilder-hardcoded-secret-csharp)

🔇 Additional comments (1)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs (1)

311-318: LGTM!

The ValidateHostPort helper method provides proper validation for host and port values with appropriate range checking.

@rathimayur rathimayur enabled auto-merge December 10, 2025 13:40
@ravirk91 ravirk91 disabled auto-merge December 11, 2025 05:18
@ravirk91 ravirk91 merged commit 8b925e8 into master Dec 11, 2025
22 of 26 checks passed
@ravirk91 ravirk91 deleted the DBOperations-GitAlertsFix branch December 11, 2025 05:19
This was referenced Dec 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants