[DBOPS-31] Add CSV importer/exporter to DbSeeder

[mkincaid-bw]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/DBOPS-31

📔 Objective

Adding CSV import/export logic. Export works from SQL Server, import works with SQL Server, Postgres, MariaDB, and Sqlite.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[github-actions[bot]]

Checkmarx One – Scan Summary & Details – 0079f96f-65ae-4d7b-bfbe-45fc8e2f44b8

New Issues (8)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	Second_Order_SQL_Injection	/util/Seeder/Migration/Databases/SqlServerExporter.cs: 94	details The application's method executes an SQL query with ExecuteReader, at line 250 of /util/Seeder/Migration/Databases/SqlServerExporter.cs. The app... ID: 3pPHv3tf792B0qVSnFYg2DRafbQ%3D Attack Vector
	Second_Order_SQL_Injection	/util/Seeder/Migration/Databases/SqlServerImporter.cs: 287	details The application's method executes an SQL query with ExecuteNonQuery, at line 526 of /util/Seeder/Migration/Databases/SqlServerImporter.cs. The a... ID: k%2FhQhKRZPRyCfHTfViffXnHXfmc%3D Attack Vector
	Second_Order_SQL_Injection	/util/Seeder/Migration/Databases/SqlServerImporter.cs: 417	details The application's method executes an SQL query with ExecuteNonQuery, at line 448 of /util/Seeder/Migration/Databases/SqlServerImporter.cs. The a... ID: rXR4pSa6POCyXiaHSQxZ2sj40QM%3D Attack Vector
	Second_Order_SQL_Injection	/util/Seeder/Migration/Databases/PostgresImporter.cs: 133	details The application's method executes an SQL query with ExecuteNonQuery, at line 486 of /util/Seeder/Migration/Databases/PostgresImporter.cs. The ap... ID: BecTe1bFeB9qLEZ6ps1lOmheNc0%3D Attack Vector
	Second_Order_SQL_Injection	/util/Seeder/Migration/Databases/PostgresImporter.cs: 133	details The application's method executes an SQL query with ExecuteScalar, at line 461 of /util/Seeder/Migration/Databases/PostgresImporter.cs. The appl... ID: 2W%2FtXmZEr1vvRjaMRZx2WoSgEQU%3D Attack Vector
	Second_Order_SQL_Injection	/util/Seeder/Migration/Databases/PostgresImporter.cs: 133	details The application's method executes an SQL query with ExecuteNonQuery, at line 381 of /util/Seeder/Migration/Databases/PostgresImporter.cs. The ap... ID: %2BnV%2BBi8uLWiJTWCg9LglLVl%2BauM%3D Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1175	details Method at line 1175 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... ID: uuKScSrq88SVVnW1zjDj6uRH2oc%3D Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1065	details Method at line 1065 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... ID: 6sjb2nXtp7uzbtUTxbSEL5Mexa0%3D Attack Vector

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 52.98%. Comparing base (6653b98) to head (d6aa027). :warning: Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6534      +/-   ##
==========================================
- Coverage   52.98%   52.98%   -0.01%     
==========================================
  Files        1909     1909              
  Lines       84810    84810              
  Branches     7633     7633              
==========================================
- Hits        44936    44933       -3     
- Misses      38122    38124       +2     
- Partials     1752     1753       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude[bot]]

Claude finished @mkincaid-bw's task —— View job

---

Code Review Complete

This review focuses on changes since the last Claude review and addresses the status of previously identified findings.

---

Summary of Changes Since Last Review

The author responded to the previous Claude review by:

• ✅ Adding comprehensive unit tests (648 new test lines across 2 test files)
• ✅ Improving identifier validation with better logging for bad CSV data
• ✅ Adding SQL keyword checking to IdentifierValidator
• ⚠️ Several findings remain unresolved (see below)

Note on PR scope: The CSRF issues flagged by Checkmarx in CiphersController.cs are not part of this PR - they appear due to merges from main and should be ignored for this review.

---

Status of Previous Findings

Finding 1: Unit tests added ✅

Status: RESOLVED - Comprehensive test coverage added including:

• CsvHandlerTests.cs (341 lines): CSV export/import, delimiter detection, round-trip testing
• IdentifierValidatorTests.cs (307 lines): Identifier validation, SQL injection attempts, edge cases

Remaining gap: Missing integration tests (author clarified this is a dev utility, not production)

Finding 2: Dynamic SQL construction ⚠️

Status: ACKNOWLEDGED BUT NOT FIXED - Author responded: "table names cannot be parameterized, dynamic SQL cannot be completely eliminated"

Current state: While the validation makes this functionally secure, 22 security scanner alerts remain active, which could complicate security audits.

Finding 3: Bad CSV data handling ✅

Status: RESOLVED - BadDataFound handler now logs warnings instead of silently ignoring:

BadDataFound = context => {
    _logger.LogWarning(
        "Bad CSV data at row {Row}, field {Field}: {RawRecord}",
        context.Context.Parser.Row, context.Field, context.RawRecord);
}

Finding 4: Identifier validation restrictive mode 💭

Status: NOT CHANGED - Author responded: "Several Bitwarden table names are also reserved SQL words, so this would break functionality"

Comment: Valid reasoning. Tables like User, Grant require permissive mode. Current implementation is acceptable.

---

New Issues Found

Finding 1: Test gap for malformed CSV handling

Location: test/Seeder.Test/Migration/CsvHandlerTests.cs:206

The test ImportCsvToData_WithMalformedData_LogsWarning() has incomplete assertions:

// Note: CsvHelper may throw or handle this depending on configuration
// The test verifies that BadDataFound handler logs the issue
// We verify the logger was called with Warning level
if (exception == null)
{
    _logger.Received().Log(...); // Only checks if no exception thrown
}

Issue: Test doesn't verify behavior when CsvHelper does throw an exception. Either handle both paths or use a deterministic malformed CSV format.

Suggested improvement

[Fact]
public void ImportCsvToData_WithMalformedData_LogsWarningAndContinues()
{
    var handler = new CsvHandler(_settings, _logger);
    var csvPath = Path.Combine(_testOutputDir, "test_malformed.csv");
    
    // Create CSV with extra field in second row (deterministic malformation)
    var csvContent = "\"Col1\",\"Col2\"\n\"1\",\"value1\"\n\"2\",\"value2\",\"extra\"";
    File.WriteAllText(csvPath, csvContent);

    // Act & Assert
    var (columns, data) = handler.ImportCsvToData(csvPath);
    
    // Verify warning was logged
    _logger.Received().Log(
        LogLevel.Warning,
        Arg.Any<EventId>(),
        Arg.Is<object>(o => o.ToString()!.Contains("Bad CSV data")),
        Arg.Any<Exception>(),
        Arg.Any<Func<object, Exception?, string>>());
    
    // Verify import continued despite error
    Assert.Equal(2, columns.Count);
    Assert.Equal(2, data.Count);
}

---

Finding 2: IdentifierValidator test doesn't verify actual Bitwarden table names

Location: test/Seeder.Test/Migration/Utils/IdentifierValidatorTests.cs

Tests verify that SQL keywords like "SELECT", "DROP" work in permissive mode, but don't test the actual Bitwarden table names that motivated this design (e.g., User, Grant).

Suggested test addition

[Theory]
[InlineData("User")]
[InlineData("Grant")]
[InlineData("Group")]
public void IsValid_BitwardenReservedWordTableNames_WithoutRestrictiveMode_ReturnsTrue(string tableName)
{
    // These are actual Bitwarden table names that are also SQL keywords
    var result = IdentifierValidator.IsValid(tableName, useRestrictiveMode: false);
    Assert.True(result);
}

[Theory]
[InlineData("User")]
[InlineData("Grant")]
public void IsValid_BitwardenReservedWordTableNames_WithRestrictiveMode_ReturnsFalse(string tableName)
{
    // Verify restrictive mode would break Bitwarden schema
    var result = IdentifierValidator.IsValid(tableName, useRestrictiveMode: true);
    Assert.False(result);
}

---

Finding 3: New security alert on PostgresImporter line 206 is a false positive

Location: util/Seeder/Migration/Databases/PostgresImporter.cs:206

The security scanner flagged a "Second Order SQL Injection" at line 206:

using var reader = command.ExecuteReader(); // Line 206
while (reader.Read())
{
    var colName = reader.GetString(0);
    IdentifierValidator.ValidateOrThrow(colName, "column name"); // Line 212
    columnTypes[colName] = reader.GetString(1);
}

Analysis: This is a false positive. The code:

1. Uses parameterized query (@tableName)
2. Validates all identifiers from information_schema before use
3. Never constructs dynamic SQL from the retrieved data at this location

Recommendation: Add a comment to explain the validation pattern:

// SECURITY: Column names from information_schema are validated immediately
// to prevent second-order SQL injection if schema itself is compromised
IdentifierValidator.ValidateOrThrow(colName, "column name");

---

Critical Issues (❌)

None - All security-critical issues from the previous review have been addressed.

---

Minor Issues (⚠️)

Finding 4: Inconsistent null handling in CsvHandler

Location: util/Seeder/Migration/CsvHandler.cs:262-264

if (row[i] == null)
{
    processedRow[i] = string.Empty; // Exports as empty string
}

But during import (line 299-302):

if (string.IsNullOrEmpty(value))
{
    processedRow[i] = DBNull.Value; // Imports as DBNull
}

Issue: Asymmetry in null handling could cause issues in round-trip scenarios. If a value is null in the database, it exports as empty string "", but on import, "" becomes DBNull.Value. This is correct behavior, but could be confusing.

Impact: Low - Round-trip test passes, so this works as intended. Consider adding a comment explaining the intentional asymmetry.

---

Code Improvements (🎨)

Finding 5: Test file organization could be improved

Location: test/Seeder.Test/

The test project structure is flat with tests directly in Migration/ folder. Consider organizing by feature:

test/Seeder.Test/
├── Migration/
│   ├── CsvHandling/
│   │   └── CsvHandlerTests.cs
│   ├── Validation/
│   │   └── IdentifierValidatorTests.cs
│   └── Integration/  (future)
│       └── EndToEndMigrationTests.cs

This becomes more important if integration tests are added later.

---

Technical Debt (♻️)

Finding 6: No mechanism to suppress security scanner false positives

Location: Multiple importer files

The codebase has 22 security scanner alerts that are false positives due to the validation approach. Consider:

1. SuppressMessage attributes for confirmed false positives:

[SuppressMessage("Security", "SQL_INJECTION", 
    Justification = "Identifiers validated by IdentifierValidator.ValidateOrThrow()")]
private void DisableConstraints() { ... }

2. Centralized dynamic SQL helper that encapsulates validation:

public static class SafeSqlBuilder
{
    public static string QuoteIdentifier(string identifier, string type)
    {
        IdentifierValidator.ValidateOrThrow(identifier, type);
        return $"[{identifier}]"; // SQL Server syntax
    }
}

This would make the validation pattern more obvious to security tools and reviewers.

---

Good Practices Observed

• Comprehensive test coverage for core functionality
• Proper use of IDisposable in test fixtures
• Parameterized tests using [Theory] and [InlineData]
• Security-first design with identifier validation

---

Action Items

For this PR:

1. ✅ Unit tests added - no further action required
2. 💭 Consider adding comment on PostgresImporter.cs:206 to explain security validation pattern
3. 💭 Consider adding tests for actual Bitwarden table names (e.g., User, Grant)
4. 💭 Optional: Add [SuppressMessage] attributes to silence false positive security alerts

Future improvements (not required for this PR):

• Integration tests for end-to-end migration workflows
• Refactor dynamic SQL to use centralized helper methods
• Document asymmetric null handling in CSV export/import

---

Overall Assessment

This PR adds 8,431 lines of well-structured database migration tooling. The security concerns raised in the previous review have been adequately addressed through comprehensive identifier validation and logging improvements. The remaining security scanner alerts are false positives that don't represent actual vulnerabilities.

Recommendation: ✅ Approve with optional improvements

The code is production-ready for its intended use as a development utility. The suggested improvements above are optional enhancements for code clarity and maintainability.

---

[mkincaid-bw]

Note: Claude wrote the bulk of this code.

[withinfocus]

I think @Hinton and I believed the other would leave this comment: this logic should pivot to the migration utility and not be in the seeder; different purpose for the latter. Then we can have a more formal review.