Skip to content

Phase 3: Fix review comments — exception narrowing, disposal safety, robust key migration #51

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adamgell merged 2 commits into from
Feb 20, 2026
Merged

Phase 3: Fix review comments — exception narrowing, disposal safety, robust key migration #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b8d963a
Initial plan
Copilot Feb 20, 2026
987d56b
fix: address review comments — CryptographicException catch, using va…
Copilot Feb 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/Intune.Commander.Core/Extensions/ServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,29 @@ public static IServiceCollection AddIntuneManagerCore(this IServiceCollection se

// One-time migration: copy DataProtection key XML files from the legacy
// location to the new one so existing encrypted profiles remain readable.
if (Directory.Exists(legacyKeysPath) && !Directory.Exists(keysPath))
var legacyKeyFiles = Directory.Exists(legacyKeysPath)
? Directory.GetFiles(legacyKeysPath, "*.xml")
: Array.Empty<string>();

var newKeyDirectoryHasKeys = Directory.Exists(keysPath)
&& Directory.GetFiles(keysPath, "*.xml").Length > 0;
Comment on lines +24 to +29

Copilot AI Feb 20, 2026

Copy link

Choose a reason for hiding this comment

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

Directory.GetFiles(legacyKeysPath, "*.xml") can throw (e.g., access denied / IO errors). Since this migration code runs during DI setup, consider guarding this with a try/catch and treating failures as “no legacy keys found” so the app can still start and generate fresh keys if needed.

Suggested change
var legacyKeyFiles = Directory.Exists(legacyKeysPath)
? Directory.GetFiles(legacyKeysPath, "*.xml")
: Array.Empty<string>();
var newKeyDirectoryHasKeys = Directory.Exists(keysPath)
&& Directory.GetFiles(keysPath, "*.xml").Length > 0;
string[] legacyKeyFiles = Array.Empty<string>();
if (Directory.Exists(legacyKeysPath))
{
try
{
legacyKeyFiles = Directory.GetFiles(legacyKeysPath, "*.xml");
}
catch
{
// Best-effort: if we can't enumerate legacy keys, treat as "no legacy keys found"
legacyKeyFiles = Array.Empty<string>();
}
}
var newKeyDirectoryHasKeys = false;
if (Directory.Exists(keysPath))
{
try
{
newKeyDirectoryHasKeys = Directory.GetFiles(keysPath, "*.xml").Length > 0;
}
catch
{
// Best-effort: if we can't enumerate new keys, treat as "no keys present"
newKeyDirectoryHasKeys = false;
}
}

Copilot uses AI. Check for mistakes.
Comment on lines +24 to +29

Copilot AI Feb 20, 2026

Copy link

Choose a reason for hiding this comment

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

Directory.GetFiles(keysPath, "*.xml") can also throw when the directory exists but isn’t readable. If this is meant to be best-effort migration, consider wrapping this probe in a try/catch and defaulting newKeyDirectoryHasKeys to false on failure (so it won’t crash during startup).

Suggested change
var legacyKeyFiles = Directory.Exists(legacyKeysPath)
? Directory.GetFiles(legacyKeysPath, "*.xml")
: Array.Empty<string>();
var newKeyDirectoryHasKeys = Directory.Exists(keysPath)
&& Directory.GetFiles(keysPath, "*.xml").Length > 0;
string[] legacyKeyFiles;
if (Directory.Exists(legacyKeysPath))
{
try
{
legacyKeyFiles = Directory.GetFiles(legacyKeysPath, "*.xml");
}
catch
{
// Best-effort: if we cannot read legacy keys, treat as none found.
legacyKeyFiles = Array.Empty<string>();
}
}
else
{
legacyKeyFiles = Array.Empty<string>();
}
var newKeyDirectoryHasKeys = false;
if (Directory.Exists(keysPath))
{
try
{
newKeyDirectoryHasKeys = Directory.GetFiles(keysPath, "*.xml").Length > 0;
}
catch
{
// Best-effort: on failure, assume no existing keys so startup does not crash.
newKeyDirectoryHasKeys = false;
}
}

Copilot uses AI. Check for mistakes.

if (legacyKeyFiles.Length > 0 && !newKeyDirectoryHasKeys)
{
try
Directory.CreateDirectory(keysPath);

Comment on lines +31 to +34

Copilot AI Feb 20, 2026

Copy link

Choose a reason for hiding this comment

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

The per-file copy is best-effort, but Directory.CreateDirectory(keysPath) is outside any try/catch now. If directory creation fails (permissions/disk), this will throw out of AddIntuneManagerCore; consider catching around the create+copy block to keep migration failures non-fatal.

Copilot uses AI. Check for mistakes.
foreach (var file in legacyKeyFiles)
{
Directory.CreateDirectory(keysPath);
foreach (var file in Directory.GetFiles(legacyKeysPath, "*.xml"))
File.Copy(file, Path.Combine(keysPath, Path.GetFileName(file)), overwrite: false);
var destinationPath = Path.Combine(keysPath, Path.GetFileName(file));
if (File.Exists(destinationPath))
continue;

try
{
File.Copy(file, destinationPath, overwrite: false);
}
catch { /* best-effort — continue copying remaining keys */ }
}
catch { /* best-effort — new keys will be generated if copy fails */ }
}

services.AddDataProtection()
Expand Down
3 changes: 2 additions & 1 deletion src/Intune.Commander.Core/Services/ProfileEncryptionService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Security.Cryptography;
using Microsoft.AspNetCore.DataProtection;

namespace Intune.Commander.Core.Services;
Expand Down Expand Up @@ -40,7 +41,7 @@ public string Decrypt(string cipherText)
{
return _protector.Unprotect(cipherText);
}
catch
catch (CryptographicException)
{
// Fall back to legacy purpose — will throw if both fail
return _legacyProtector.Unprotect(cipherText);
Expand Down
3 changes: 1 addition & 2 deletions tests/Intune.Commander.Core.Tests/Services/ProfileEncryptionServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public void Decrypt_LegacyPurpose_SucceedsViaFallback()
services.AddDataProtection()
.SetApplicationName("IntuneManager-Tests")
.PersistKeysToFileSystem(new DirectoryInfo(_keysDir));
var sp = services.BuildServiceProvider();
using var sp = services.BuildServiceProvider();
var provider = sp.GetRequiredService<Microsoft.AspNetCore.DataProtection.IDataProtectionProvider>();

// Encrypt directly under the legacy purpose (bypassing ProfileEncryptionService.Encrypt)
Expand All @@ -83,7 +83,6 @@ public void Decrypt_LegacyPurpose_SucceedsViaFallback()
var decrypted = _encryption.Decrypt(legacyEncrypted);

Assert.Equal(plainText, decrypted);
sp.Dispose();
}
}

Expand Down