From 63149080cf0e948c96c60bc032366eaa4c73f677 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 18 Aug 2025 23:27:35 +0000 Subject: [PATCH 1/6] Initial plan From 483cc00f4203ccbe1113c53fe3ac514d2027c658 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 18 Aug 2025 23:37:14 +0000 Subject: [PATCH 2/6] Fix memory leak in ImportCertificate method by properly disposing X509Certificate2 Co-authored-by: danmoseley <6385855+danmoseley@users.noreply.github.com> --- src/Shared/CertificateGeneration/CertificateManager.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Shared/CertificateGeneration/CertificateManager.cs b/src/Shared/CertificateGeneration/CertificateManager.cs index aba1240c7b12..1aed9ca68c78 100644 --- a/src/Shared/CertificateGeneration/CertificateManager.cs +++ b/src/Shared/CertificateGeneration/CertificateManager.cs @@ -482,12 +482,13 @@ internal ImportCertificateResult ImportCertificate(string certificatePath, strin { Log.NoHttpsDevelopmentCertificate(GetDescription(certificate)); } + certificate.Dispose(); return ImportCertificateResult.NoDevelopmentHttpsCertificate; } try { - SaveCertificate(certificate); + certificate = SaveCertificate(certificate); } catch (Exception e) { @@ -495,9 +496,11 @@ internal ImportCertificateResult ImportCertificate(string certificatePath, strin { Log.SaveCertificateInStoreError(e.ToString()); } + certificate.Dispose(); return ImportCertificateResult.ErrorSavingTheCertificateIntoTheCurrentUserPersonalStore; } + certificate.Dispose(); return ImportCertificateResult.Succeeded; } From a76f546262eeb6f9b279529b1716b30d8c05eca0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 19 Aug 2025 00:19:48 +0000 Subject: [PATCH 3/6] Add certificate disposal in exception catch block for ImportCertificate method Co-authored-by: danmoseley <6385855+danmoseley@users.noreply.github.com> --- src/Shared/CertificateGeneration/CertificateManager.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Shared/CertificateGeneration/CertificateManager.cs b/src/Shared/CertificateGeneration/CertificateManager.cs index 1aed9ca68c78..3547b3297a4c 100644 --- a/src/Shared/CertificateGeneration/CertificateManager.cs +++ b/src/Shared/CertificateGeneration/CertificateManager.cs @@ -454,7 +454,7 @@ internal ImportCertificateResult ImportCertificate(string certificatePath, strin return ImportCertificateResult.ExistingCertificatesPresent; } - X509Certificate2 certificate; + X509Certificate2? certificate = null; try { Log.LoadCertificateStart(certificatePath); @@ -470,6 +470,7 @@ internal ImportCertificateResult ImportCertificate(string certificatePath, strin { Log.LoadCertificateError(e.ToString()); } + certificate?.Dispose(); return ImportCertificateResult.InvalidCertificate; } From 3a6e45f81924896e8bb0b4452cda7da84a8249bd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 19 Aug 2025 23:15:28 +0000 Subject: [PATCH 4/6] Refactor certificate disposal using try-finally pattern and fix disposal issues in EnsureAspNetCoreHttpsDevelopmentCertificate Co-authored-by: BrennanConroy <7574801+BrennanConroy@users.noreply.github.com> --- .../CertificateManager.cs | 79 +++++++++++-------- 1 file changed, 44 insertions(+), 35 deletions(-) diff --git a/src/Shared/CertificateGeneration/CertificateManager.cs b/src/Shared/CertificateGeneration/CertificateManager.cs index 3547b3297a4c..bc1ae58bb1f3 100644 --- a/src/Shared/CertificateGeneration/CertificateManager.cs +++ b/src/Shared/CertificateGeneration/CertificateManager.cs @@ -330,6 +330,7 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( catch (Exception e) { Log.SaveCertificateInStoreError(e.ToString()); + certificate?.Dispose(); result = EnsureCertificateResult.ErrorSavingTheCertificateIntoTheCurrentUserPersonalStore; return result; } @@ -387,6 +388,7 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( result : EnsureCertificateResult.ErrorExportingTheCertificate; + certificate?.Dispose(); return result; } } @@ -403,21 +405,25 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( break; case TrustLevel.Partial: result = EnsureCertificateResult.PartiallyFailedToTrustTheCertificate; + certificate?.Dispose(); return result; case TrustLevel.None: default: // Treat unknown status (should be impossible) as failure result = EnsureCertificateResult.FailedToTrustTheCertificate; + certificate?.Dispose(); return result; } } catch (UserCancelledTrustException) { result = EnsureCertificateResult.UserCancelledTrustStep; + certificate?.Dispose(); return result; } catch { result = EnsureCertificateResult.FailedToTrustTheCertificate; + certificate?.Dispose(); return result; } @@ -457,52 +463,55 @@ internal ImportCertificateResult ImportCertificate(string certificatePath, strin X509Certificate2? certificate = null; try { - Log.LoadCertificateStart(certificatePath); - certificate = new X509Certificate2(certificatePath, password, X509KeyStorageFlags.Exportable | X509KeyStorageFlags.EphemeralKeySet); - if (Log.IsEnabled()) + try { - Log.LoadCertificateEnd(GetDescription(certificate)); + Log.LoadCertificateStart(certificatePath); + certificate = new X509Certificate2(certificatePath, password, X509KeyStorageFlags.Exportable | X509KeyStorageFlags.EphemeralKeySet); + if (Log.IsEnabled()) + { + Log.LoadCertificateEnd(GetDescription(certificate)); + } } - } - catch (Exception e) - { - if (Log.IsEnabled()) + catch (Exception e) { - Log.LoadCertificateError(e.ToString()); + if (Log.IsEnabled()) + { + Log.LoadCertificateError(e.ToString()); + } + return ImportCertificateResult.InvalidCertificate; } - certificate?.Dispose(); - return ImportCertificateResult.InvalidCertificate; - } - // Note that we're checking Subject, rather than LocalhostHttpsDistinguishedName, - // because the tests use a different subject. - if (!string.Equals(certificate.Subject, Subject, StringComparison.Ordinal) || // Kestrel requires this - !IsHttpsDevelopmentCertificate(certificate)) - { - if (Log.IsEnabled()) + // Note that we're checking Subject, rather than LocalhostHttpsDistinguishedName, + // because the tests use a different subject. + if (!string.Equals(certificate.Subject, Subject, StringComparison.Ordinal) || // Kestrel requires this + !IsHttpsDevelopmentCertificate(certificate)) { - Log.NoHttpsDevelopmentCertificate(GetDescription(certificate)); + if (Log.IsEnabled()) + { + Log.NoHttpsDevelopmentCertificate(GetDescription(certificate)); + } + return ImportCertificateResult.NoDevelopmentHttpsCertificate; } - certificate.Dispose(); - return ImportCertificateResult.NoDevelopmentHttpsCertificate; - } - try - { - certificate = SaveCertificate(certificate); - } - catch (Exception e) - { - if (Log.IsEnabled()) + try { - Log.SaveCertificateInStoreError(e.ToString()); + certificate = SaveCertificate(certificate); + } + catch (Exception e) + { + if (Log.IsEnabled()) + { + Log.SaveCertificateInStoreError(e.ToString()); + } + return ImportCertificateResult.ErrorSavingTheCertificateIntoTheCurrentUserPersonalStore; } - certificate.Dispose(); - return ImportCertificateResult.ErrorSavingTheCertificateIntoTheCurrentUserPersonalStore; - } - certificate.Dispose(); - return ImportCertificateResult.Succeeded; + return ImportCertificateResult.Succeeded; + } + finally + { + certificate?.Dispose(); + } } public void CleanupHttpsCertificates() From e2b7b68f558882b7b9d678c52c503043e798f6e2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 21 Aug 2025 19:10:36 +0000 Subject: [PATCH 5/6] Fix certificate disposal in all early exit cases of EnsureAspNetCoreHttpsDevelopmentCertificate Co-authored-by: BrennanConroy <7574801+BrennanConroy@users.noreply.github.com> --- .../CertificateManager.cs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/Shared/CertificateGeneration/CertificateManager.cs b/src/Shared/CertificateGeneration/CertificateManager.cs index bc1ae58bb1f3..b983980bd909 100644 --- a/src/Shared/CertificateGeneration/CertificateManager.cs +++ b/src/Shared/CertificateGeneration/CertificateManager.cs @@ -236,18 +236,22 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( var currentUserCertificates = ListCertificates(StoreName.My, StoreLocation.CurrentUser, isValid: true, requireExportable: true); var localMachineCertificates = ListCertificates(StoreName.My, StoreLocation.LocalMachine, isValid: true, requireExportable: true); - var certificates = currentUserCertificates.Concat(localMachineCertificates); + var allCertificates = currentUserCertificates.Concat(localMachineCertificates).ToList(); - var filteredCertificates = certificates.Where(c => c.Subject == Subject); + var filteredCertificates = allCertificates.Where(c => c.Subject == Subject).ToList(); if (Log.IsEnabled()) { - var excludedCertificates = certificates.Except(filteredCertificates); + var excludedCertificates = allCertificates.Except(filteredCertificates); Log.FilteredCertificates(ToCertificateDescription(filteredCertificates)); Log.ExcludedCertificates(ToCertificateDescription(excludedCertificates)); } - certificates = filteredCertificates; + // Dispose certificates we're not going to use + var certificatesToDispose = allCertificates.Except(filteredCertificates); + DisposeCertificates(certificatesToDispose); + + var certificates = filteredCertificates; X509Certificate2? certificate = null; var isNewCertificate = false; @@ -319,6 +323,7 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( Log.CreateDevelopmentCertificateError(e.ToString()); } result = EnsureCertificateResult.ErrorCreatingTheCertificate; + DisposeCertificates(certificates); return result; } Log.CreateDevelopmentCertificateEnd(); @@ -331,6 +336,7 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( { Log.SaveCertificateInStoreError(e.ToString()); certificate?.Dispose(); + DisposeCertificates(certificates); result = EnsureCertificateResult.ErrorSavingTheCertificateIntoTheCurrentUserPersonalStore; return result; } @@ -389,6 +395,7 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( EnsureCertificateResult.ErrorExportingTheCertificate; certificate?.Dispose(); + DisposeCertificates(certificates); return result; } } @@ -406,11 +413,13 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( case TrustLevel.Partial: result = EnsureCertificateResult.PartiallyFailedToTrustTheCertificate; certificate?.Dispose(); + DisposeCertificates(certificates); return result; case TrustLevel.None: default: // Treat unknown status (should be impossible) as failure result = EnsureCertificateResult.FailedToTrustTheCertificate; certificate?.Dispose(); + DisposeCertificates(certificates); return result; } } @@ -418,12 +427,14 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( { result = EnsureCertificateResult.UserCancelledTrustStep; certificate?.Dispose(); + DisposeCertificates(certificates); return result; } catch { result = EnsureCertificateResult.FailedToTrustTheCertificate; certificate?.Dispose(); + DisposeCertificates(certificates); return result; } From e9c4aad767e6aaf01901028bdb9d1100dee42fb6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 21 Aug 2025 22:14:17 +0000 Subject: [PATCH 6/6] Fix certificate disposal to only dispose new certificates, not existing ones from collection Co-authored-by: BrennanConroy <7574801+BrennanConroy@users.noreply.github.com> --- .../CertificateManager.cs | 30 +++++++++++++++---- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/src/Shared/CertificateGeneration/CertificateManager.cs b/src/Shared/CertificateGeneration/CertificateManager.cs index b983980bd909..bd7104542fab 100644 --- a/src/Shared/CertificateGeneration/CertificateManager.cs +++ b/src/Shared/CertificateGeneration/CertificateManager.cs @@ -335,7 +335,10 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( catch (Exception e) { Log.SaveCertificateInStoreError(e.ToString()); - certificate?.Dispose(); + if (isNewCertificate) + { + certificate?.Dispose(); + } DisposeCertificates(certificates); result = EnsureCertificateResult.ErrorSavingTheCertificateIntoTheCurrentUserPersonalStore; return result; @@ -394,7 +397,10 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( result : EnsureCertificateResult.ErrorExportingTheCertificate; - certificate?.Dispose(); + if (isNewCertificate) + { + certificate?.Dispose(); + } DisposeCertificates(certificates); return result; } @@ -412,13 +418,19 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( break; case TrustLevel.Partial: result = EnsureCertificateResult.PartiallyFailedToTrustTheCertificate; - certificate?.Dispose(); + if (isNewCertificate) + { + certificate?.Dispose(); + } DisposeCertificates(certificates); return result; case TrustLevel.None: default: // Treat unknown status (should be impossible) as failure result = EnsureCertificateResult.FailedToTrustTheCertificate; - certificate?.Dispose(); + if (isNewCertificate) + { + certificate?.Dispose(); + } DisposeCertificates(certificates); return result; } @@ -426,14 +438,20 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( catch (UserCancelledTrustException) { result = EnsureCertificateResult.UserCancelledTrustStep; - certificate?.Dispose(); + if (isNewCertificate) + { + certificate?.Dispose(); + } DisposeCertificates(certificates); return result; } catch { result = EnsureCertificateResult.FailedToTrustTheCertificate; - certificate?.Dispose(); + if (isNewCertificate) + { + certificate?.Dispose(); + } DisposeCertificates(certificates); return result; }