Skip to content

Conversation

@cricketthomas
Copy link
Owner

@cricketthomas cricketthomas commented Nov 14, 2025

  • Refactored KvExplorerDb to use per-operation database connections via the new TryCreateDatabaseAndOpenConnection helper, removing the static _connection field and related open/close methods. This change improves resource management and avoids potential connection leaks.
  • Replaced the previous Dispose and connection closing logic with a more robust implementation that ensures connections are properly disposed after each operation.
  • Added new methods to KvExplorerDb for dropping and recreating tables (DropTablesAndRecreate) and deleting the database file (DeleteDatabaseFile), supporting easier database resets and cleanup.
  • Updated target frameworks in Desktop.csproj and KeyVaultExplorer.csproj from net9.0 to net10.0
image

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR upgrades the project from .NET 9.0 to .NET 10.0 and refactors the database connection management in KvExplorerDb to use per-operation connections instead of a static shared connection. However, the PR contains several critical bugs that need to be addressed before merging.

Key changes:

  • Upgraded target frameworks and package dependencies to .NET 10.0 and version 10.0.0
  • Refactored KvExplorerDb to create connections per operation via TryCreateDatabaseAndOpenConnection()
  • Added database cleanup functionality (DropTablesAndRecreate, DeleteDatabaseFile) accessible from settings UI
  • Enhanced MacOSKeyChainService with CLI fallback support and new DeleteFromKeychain method

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 28 comments.

Show a summary per file
File Description
KeyVaultExplorer/KeyVaultExplorer.csproj Updated target framework to net10.0 and upgraded package versions to 10.0.0
Desktop/Desktop.csproj Updated target framework to net10.0 and upgraded package versions
KeyVaultExplorer/Database/KvExplorerDb.cs Refactored to use per-operation connections; added DropTablesAndRecreate and DeleteDatabaseFile methods; contains multiple critical bugs
KeyVaultExplorer/ViewModels/SettingsPageViewModel.cs Added DeleteDatabase command to clear database from UI
KeyVaultExplorer/ViewModels/KeyVaultTreeListViewModel.cs Added sorting service calls; contains unreachable code and incorrect disposal pattern
KeyVaultExplorer/ViewModels/FilterService.cs Attempted to add sorting to filter results; contains critical logic bugs that break filtering
KeyVaultExplorer/ViewModels/SortService.cs New service class providing sorting methods for subscriptions, resource groups, and key vaults
KeyVaultExplorer/Services/MacOSKeyChainService.cs Extensively refactored with CLI fallback support and resource management improvements
KeyVaultExplorer/Services/DatabaseEncryptedPasswordManager.cs Added PurgePasswords method for cleanup operations
KeyVaultExplorer/Services/AuthService.cs Updated iOS keychain security group identifier
KeyVaultExplorer/Models/Constants.cs Added DatabasePasswordFilePath constant and updated Linux keyring schema identifier
KeyVaultExplorer/Views/Pages/SettingsPage.axaml Added UI button for recreating/deleting database
KeyVaultExplorer/App.axaml.cs Removed static connection open/close calls
KeyVaultExplorer/Assets/Info.plist Updated bundle identifier from sidesteplabs to cricketthomas
.github/workflows/codeql.yml Removed scheduled cron job and updated build command to specify solution file
Comments suppressed due to low confidence (2)

KeyVaultExplorer/Database/KvExplorerDb.cs:88

  • The database connection is not opened before executing the command. The connection is created by TryCreateDatabaseAndOpenConnection() but never actually opened. This will cause a runtime error. Add await connection.OpenAsync(); before creating the command on line 79.
        using var connection = await TryCreateDatabaseAndOpenConnection();
        await connection.OpenAsync();
        var command = connection.CreateCommand();
        var query = new StringBuilder("SELECT DisplayName, SubscriptionId, TenantId FROM Subscriptions");

        if (!string.IsNullOrWhiteSpace(tenantId))
        {
            query.Append($" WHERE TenantId = '{tenantId.ToUpperInvariant()}'");
        }
        query.Append(";");
        command.CommandText = query.ToString();
        var reader = command.ExecuteReader();

KeyVaultExplorer/Database/KvExplorerDb.cs:51

  • SQL injection vulnerability: The tenantId parameter is directly concatenated into the SQL query string without parameterization. This should use a parameterized query like query.Append(" WHERE TenantId = @TenantId"); and add the parameter using command.Parameters.Add(new SqliteParameter("@TenantId", tenantId));
        if (!string.IsNullOrWhiteSpace(tenantId))
        {
            query.Append($" WHERE TenantId = '{tenantId}'");

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI commented Nov 15, 2025

@cricketthomas I've opened a new pull request, #137, to work on those changes. Once the pull request is ready, I'll request review from you.

Refactored database connection management to use scoped SQLite connections and added new methods for database operations. Updated target framework to .NET 10.0 and upgraded multiple NuGet packages for improved compatibility and security. Introduced a `SortService` for consistent alphabetical sorting of subscriptions, resource groups, and key vaults.

Enhanced the UI with a "Recreate Database" option and updated repository links to reflect new branding. Improved macOS keychain management with fallback CLI support and added methods for password purging. Cleaned up code, removed redundancies, and ensured better error handling across the application.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cricketthomas cricketthomas merged commit df19986 into master Nov 16, 2025
8 checks passed
@cricketthomas cricketthomas deleted the v1.0.315 branch November 16, 2025 13:12
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.

2 participants