diff --git a/src/Agent/AutopilotMonitor.Agent.Core.Tests/Monitoring/DiagnosticsPackageServiceUrlBuilderTests.cs b/src/Agent/AutopilotMonitor.Agent.Core.Tests/Monitoring/DiagnosticsPackageServiceUrlBuilderTests.cs new file mode 100644 index 00000000..3da7b9e5 --- /dev/null +++ b/src/Agent/AutopilotMonitor.Agent.Core.Tests/Monitoring/DiagnosticsPackageServiceUrlBuilderTests.cs @@ -0,0 +1,71 @@ +using AutopilotMonitor.Agent.Core.Monitoring.Runtime; +using Xunit; + +namespace AutopilotMonitor.Agent.Core.Tests.Monitoring +{ + /// + /// Phase 3 of the hosted-diagnostics work: V1 agent must build the blob PUT URL + /// destination-aware so a Hosted SAS (already blob-scoped at + /// {tenantId}/{filename}) is used as-is, while a CustomerSas (container-scoped) + /// continues to get the blob name appended. Mirrors the V2 tests in + /// AutopilotMonitor.Agent.V2.Core.Tests.Monitoring.Runtime.DiagnosticsPackageServiceTests. + /// V1 stays on this code path until phase-out per feedback_codex_findings_v2_only. + /// + public class DiagnosticsPackageServiceUrlBuilderTests + { + [Theory] + [InlineData("Hosted")] + [InlineData("hosted")] + [InlineData("HOSTED")] + public void BuildBlobUploadUrl_HostedDestination_ReturnsSasUnchanged(string destination) + { + const string hostedSas = "https://account.blob.core.windows.net/diagnostics/11111111-1111-1111-1111-111111111111/AgentDiagnostics-x.zip?sig=abc"; + var result = DiagnosticsPackageService.BuildBlobUploadUrl(hostedSas, "AgentDiagnostics-x.zip", destination); + Assert.Equal(hostedSas, result); + } + + [Fact] + public void BuildBlobUploadUrl_CustomerSas_AppendsBlobNameBeforeQuery() + { + const string containerSas = "https://customer.blob.core.windows.net/diagnostics?sv=2024-10-04&sig=xyz"; + var result = DiagnosticsPackageService.BuildBlobUploadUrl(containerSas, "AgentDiagnostics-x.zip", "CustomerSas"); + Assert.Equal( + "https://customer.blob.core.windows.net/diagnostics/AgentDiagnostics-x.zip?sv=2024-10-04&sig=xyz", + result); + } + + [Fact] + public void BuildBlobUploadUrl_NullDestination_AppendsBlobName_LegacyBackendCompat() + { + // Older backends without the Destination field return null; agent must + // preserve the historical container-SAS append behaviour. + const string containerSas = "https://customer.blob.core.windows.net/diag?sig=abc"; + var result = DiagnosticsPackageService.BuildBlobUploadUrl(containerSas, "diag.zip", null); + Assert.Equal("https://customer.blob.core.windows.net/diag/diag.zip?sig=abc", result); + } + + [Fact] + public void BuildBlobUploadUrl_UnknownDestination_FallsBackToCustomerSasBehaviour() + { + const string containerSas = "https://customer.blob.core.windows.net/diag?sig=abc"; + var result = DiagnosticsPackageService.BuildBlobUploadUrl(containerSas, "diag.zip", "Vendor"); + Assert.EndsWith("/diag.zip?sig=abc", result); + } + + [Fact] + public void BuildBlobUploadUrl_SasWithoutQueryString_AppendsBlobName() + { + const string noQuery = "https://customer.blob.core.windows.net/diag"; + var result = DiagnosticsPackageService.BuildBlobUploadUrl(noQuery, "diag.zip", "CustomerSas"); + Assert.Equal("https://customer.blob.core.windows.net/diag/diag.zip", result); + } + + [Fact] + public void BuildBlobUploadUrl_HostedWithoutQueryString_StillReturnsUnchanged() + { + const string hostedNoQuery = "https://account.blob.core.windows.net/diagnostics/tenant/x.zip"; + var result = DiagnosticsPackageService.BuildBlobUploadUrl(hostedNoQuery, "x.zip", "Hosted"); + Assert.Equal(hostedNoQuery, result); + } + } +} diff --git a/src/Agent/AutopilotMonitor.Agent.Core/Monitoring/Enrollment/Completion/EnrollmentCompletionHandler.cs b/src/Agent/AutopilotMonitor.Agent.Core/Monitoring/Enrollment/Completion/EnrollmentCompletionHandler.cs index b407f66f..a11526d2 100644 --- a/src/Agent/AutopilotMonitor.Agent.Core/Monitoring/Enrollment/Completion/EnrollmentCompletionHandler.cs +++ b/src/Agent/AutopilotMonitor.Agent.Core/Monitoring/Enrollment/Completion/EnrollmentCompletionHandler.cs @@ -265,6 +265,12 @@ namespace AutopilotMonitor.Agent.Core.Monitoring.Enrollment.Completion }; if (uploadResult?.BlobName != null) resultData["blobName"] = uploadResult.BlobName; + // Backend stamps Session.DiagnosticsBlobDestination from this so the download + // path can route correctly even after a future tenant destination switch. + // Omitted on legacy backend responses (Destination=null) — backend then falls + // back to treating the session as CustomerSas, which matches today's behaviour. + if (uploadResult?.Destination != null) + resultData["destination"] = uploadResult.Destination; if (uploadResult?.SasUrlPrefix != null) resultData["sasUrl"] = uploadResult.SasUrlPrefix; if (uploadResult?.ErrorCode != null) diff --git a/src/Agent/AutopilotMonitor.Agent.Core/Monitoring/Runtime/DiagnosticsPackageService.cs b/src/Agent/AutopilotMonitor.Agent.Core/Monitoring/Runtime/DiagnosticsPackageService.cs index ad283427..ee22ac25 100644 --- a/src/Agent/AutopilotMonitor.Agent.Core/Monitoring/Runtime/DiagnosticsPackageService.cs +++ b/src/Agent/AutopilotMonitor.Agent.Core/Monitoring/Runtime/DiagnosticsPackageService.cs @@ -26,6 +26,17 @@ namespace AutopilotMonitor.Agent.Core.Monitoring.Runtime /// Blob name on success, null on failure or skip. public string BlobName { get; set; } + /// + /// Where the blob was uploaded: "CustomerSas" or "Hosted". Verbatim + /// from the backend's upload-url response. Surfaced into the + /// diagnostics_uploaded event so the backend can stamp it on the + /// Sessions row alongside the blob name — the download path then knows + /// which storage to fetch from, even if the tenant later switches destinations. + /// Null when the upload was skipped or against a legacy backend that doesn't + /// return the field. + /// + public string Destination { get; set; } + /// /// First part of the SAS URL (up to and including the first query param) with the token truncated, /// so the event shows the target container without leaking the full signature. @@ -185,15 +196,39 @@ namespace AutopilotMonitor.Agent.Core.Monitoring.Runtime var sasUrlPrefix = BuildSasUrlPrefix(uploadUrlResponse.UploadUrl); + // Resolve the final PUT target: Hosted SAS is already blob-scoped at + // {tenantId}/{filename} and must be used as-is; CustomerSas (or a null + // Destination from an older backend) is container-scoped and we append + // the blob filename ourselves. + var blobUploadUrl = BuildBlobUploadUrl(uploadUrlResponse.UploadUrl, zipFileName, uploadUrlResponse.Destination); + + // BlobName for the Sessions row: prefer the backend-supplied path because + // it encodes destination-specific layout (e.g. tenant-prefix for Hosted). + // Fall back to the local filename so the agent still works against older + // backends that don't return BlobName. + var persistedBlobName = !string.IsNullOrEmpty(uploadUrlResponse.BlobName) + ? uploadUrlResponse.BlobName + : zipFileName; + // Upload to Blob Storage using the freshly obtained URL - var (uploaded, uploadErrorCode) = await UploadToBlobStorageAsync(zipFileName, zipBytes, uploadUrlResponse.UploadUrl); + var (uploaded, uploadErrorCode) = await UploadToBlobStorageAsync(zipFileName, zipBytes, blobUploadUrl); if (uploaded) { - _logger.Info($"Diagnostics package uploaded successfully: {zipFileName}"); - return new DiagnosticsUploadResult { BlobName = zipFileName, SasUrlPrefix = sasUrlPrefix }; + _logger.Info($"Diagnostics package uploaded successfully: {persistedBlobName}"); + return new DiagnosticsUploadResult + { + BlobName = persistedBlobName, + Destination = uploadUrlResponse.Destination, + SasUrlPrefix = sasUrlPrefix, + }; } - return new DiagnosticsUploadResult { SasUrlPrefix = sasUrlPrefix, ErrorCode = uploadErrorCode }; + return new DiagnosticsUploadResult + { + Destination = uploadUrlResponse.Destination, + SasUrlPrefix = sasUrlPrefix, + ErrorCode = uploadErrorCode, + }; } catch (Exception ex) { @@ -293,27 +328,47 @@ namespace AutopilotMonitor.Agent.Core.Monitoring.Runtime } /// - /// Uploads the ZIP bytes to Azure Blob Storage via PUT with the provided container SAS URL. - /// Container SAS URL format: https://account.blob.core.windows.net/container?sv=...&sig=... - /// Blob URL is constructed by inserting the blob name before the query string. - /// Returns (success, errorCode) — errorCode is null on success, otherwise the last HTTP error. + /// Resolves the final blob PUT URL based on the destination advertised by the + /// backend. Pure helper, internal-static so the V1 test suite can pin both + /// branches without exercising any HTTP transport: + /// + /// Hosted (or any case-insensitive match): the SAS is already + /// blob-scoped at {tenantId}/{filename} and must be used as-is. + /// Appending the local filename would produce a double-name URL. + /// CustomerSas / null / unknown (legacy backend without the + /// field): the SAS is container-scoped; insert the blob filename + /// before the query string. Preserves prior behaviour. + /// /// - private async Task<(bool success, string errorCode)> UploadToBlobStorageAsync(string blobName, byte[] data, string containerSasUrl) + internal static string BuildBlobUploadUrl(string sasUrl, string blobFileName, string destination) { - // Build blob URL: split at '?' to insert blob name - var questionMarkIndex = containerSasUrl.IndexOf('?'); - string blobUrl; + if (!string.IsNullOrEmpty(destination) + && string.Equals(destination, "Hosted", StringComparison.OrdinalIgnoreCase)) + { + return sasUrl; + } + + // CustomerSas or null/unknown — container-scoped SAS, insert blob name. + var questionMarkIndex = sasUrl.IndexOf('?'); if (questionMarkIndex >= 0) { - var basePath = containerSasUrl.Substring(0, questionMarkIndex).TrimEnd('/'); - var queryString = containerSasUrl.Substring(questionMarkIndex); - blobUrl = $"{basePath}/{blobName}{queryString}"; - } - else - { - // No query string (unlikely for SAS but handle gracefully) - blobUrl = $"{containerSasUrl.TrimEnd('/')}/{blobName}"; + var basePath = sasUrl.Substring(0, questionMarkIndex).TrimEnd('/'); + var queryString = sasUrl.Substring(questionMarkIndex); + return $"{basePath}/{blobFileName}{queryString}"; } + return $"{sasUrl.TrimEnd('/')}/{blobFileName}"; + } + + /// + /// Uploads the ZIP bytes to Azure Blob Storage via PUT to the pre-built + /// . The URL is destination-aware (built by + /// ) and ready to PUT without further mutation. + /// is kept for log lines only. + /// Returns (success, errorCode) — errorCode is null on success, otherwise the last HTTP error. + /// + private async Task<(bool success, string errorCode)> UploadToBlobStorageAsync(string blobName, byte[] data, string blobUploadUrl) + { + var blobUrl = blobUploadUrl; const int maxRetries = 3; string lastErrorCode = null; diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Monitoring/Runtime/DiagnosticsPackageServiceTests.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Monitoring/Runtime/DiagnosticsPackageServiceTests.cs index 5c7c2193..a2696c04 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Monitoring/Runtime/DiagnosticsPackageServiceTests.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Monitoring/Runtime/DiagnosticsPackageServiceTests.cs @@ -226,6 +226,75 @@ namespace AutopilotMonitor.Agent.V2.Core.Tests.Monitoring.Runtime return archive.Entries.Select(e => e.FullName).ToArray(); } + // ── BuildBlobUploadUrl — destination-aware URL construction ───────────────── + + [Theory] + [InlineData("Hosted")] + [InlineData("hosted")] + [InlineData("HOSTED")] + public void BuildBlobUploadUrl_HostedDestination_ReturnsSasUnchanged(string destination) + { + // Hosted SAS is already blob-scoped at {tenantId}/{filename}; the agent must + // PUT exactly to that URL. Appending the local filename would produce a + // double-name URL like .../diagnostics/{tenantId}/{filename}/{filename}. + const string hostedSas = "https://account.blob.core.windows.net/diagnostics/11111111-1111-1111-1111-111111111111/AgentDiagnostics-x.zip?sig=abc"; + var result = DiagnosticsPackageService.BuildBlobUploadUrl(hostedSas, "AgentDiagnostics-x.zip", destination); + Assert.Equal(hostedSas, result); + } + + [Fact] + public void BuildBlobUploadUrl_CustomerSas_AppendsBlobNameBeforeQuery() + { + const string containerSas = "https://customer.blob.core.windows.net/diagnostics?sv=2024-10-04&sig=xyz"; + var result = DiagnosticsPackageService.BuildBlobUploadUrl(containerSas, "AgentDiagnostics-x.zip", "CustomerSas"); + Assert.Equal( + "https://customer.blob.core.windows.net/diagnostics/AgentDiagnostics-x.zip?sv=2024-10-04&sig=xyz", + result); + } + + [Fact] + public void BuildBlobUploadUrl_NullDestination_AppendsBlobName_LegacyBackendCompat() + { + // An older backend without the Destination field returns null. The agent must + // preserve the historical container-SAS append behaviour so CustomerSas + // uploads continue to work seamlessly after a backend rollout. + const string containerSas = "https://customer.blob.core.windows.net/diag?sig=abc"; + var result = DiagnosticsPackageService.BuildBlobUploadUrl(containerSas, "diag.zip", null); + Assert.Equal("https://customer.blob.core.windows.net/diag/diag.zip?sig=abc", result); + } + + [Fact] + public void BuildBlobUploadUrl_UnknownDestination_FallsBackToCustomerSasBehaviour() + { + // Defence-in-depth: an unrecognised destination string (server bug, manual + // edit) must NOT silently treat the SAS as blob-scoped — the agent would PUT + // to a container URL and Azure would reject. Append-blob-name is the safe + // default and matches CustomerSas behaviour. + const string containerSas = "https://customer.blob.core.windows.net/diag?sig=abc"; + var result = DiagnosticsPackageService.BuildBlobUploadUrl(containerSas, "diag.zip", "Vendor"); + Assert.EndsWith("/diag.zip?sig=abc", result); + } + + [Fact] + public void BuildBlobUploadUrl_SasWithoutQueryString_AppendsBlobName() + { + // Defensive path — SAS without `?` is unlikely in practice but the helper + // mirrors the V1 behaviour for it. Confirms the no-query branch is taken. + const string noQuery = "https://customer.blob.core.windows.net/diag"; + var result = DiagnosticsPackageService.BuildBlobUploadUrl(noQuery, "diag.zip", "CustomerSas"); + Assert.Equal("https://customer.blob.core.windows.net/diag/diag.zip", result); + } + + [Fact] + public void BuildBlobUploadUrl_HostedWithoutQueryString_StillReturnsUnchanged() + { + // Hosted SAS would always have `?sig=...` but the helper's branch order + // means Hosted wins before the query-string check. + const string hostedNoQuery = "https://account.blob.core.windows.net/diagnostics/tenant/x.zip"; + var result = DiagnosticsPackageService.BuildBlobUploadUrl(hostedNoQuery, "x.zip", "Hosted"); + Assert.Equal(hostedNoQuery, result); + } + private static string ReadEntry(byte[] zipBytes, string entryName) { using var ms = new MemoryStream(zipBytes); diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Runtime/DiagnosticsPackageService.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Runtime/DiagnosticsPackageService.cs index 7d58bd7f..843b6a70 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Runtime/DiagnosticsPackageService.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Runtime/DiagnosticsPackageService.cs @@ -25,6 +25,17 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime /// Blob name on success, null on failure or skip. public string BlobName { get; set; } + /// + /// Where the blob was uploaded: "CustomerSas" or "Hosted". Verbatim + /// from the backend's upload-url response. Surfaced into the + /// diagnostics_uploaded event so the backend can stamp it on the + /// Sessions row alongside the blob name — the download path then knows + /// which storage to fetch from, even if the tenant later switches destinations. + /// Null when the upload was skipped or against a legacy backend that doesn't + /// return the field. + /// + public string Destination { get; set; } + /// /// First part of the SAS URL (up to and including the first query param) with the token truncated, /// so the event shows the target container without leaking the full signature. @@ -266,15 +277,39 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime var sasUrlPrefix = BuildSasUrlPrefix(uploadUrlResponse.UploadUrl); + // Resolve the final PUT target: Hosted SAS is already blob-scoped at + // {tenantId}/{filename} and must be used as-is; CustomerSas (or a null + // Destination from an older backend) is container-scoped and we append + // the blob filename ourselves. + var blobUploadUrl = BuildBlobUploadUrl(uploadUrlResponse.UploadUrl, zipFileName, uploadUrlResponse.Destination); + + // BlobName for the Sessions row: prefer the backend-supplied path because + // it encodes destination-specific layout (e.g. tenant-prefix for Hosted). + // Fall back to the local filename so the agent still works against older + // backends that don't return BlobName. + var persistedBlobName = !string.IsNullOrEmpty(uploadUrlResponse.BlobName) + ? uploadUrlResponse.BlobName + : zipFileName; + // Upload to Blob Storage using the freshly obtained URL - var (uploaded, uploadErrorCode) = await UploadToBlobStorageAsync(zipFileName, zipBytes, uploadUrlResponse.UploadUrl); + var (uploaded, uploadErrorCode) = await UploadToBlobStorageAsync(zipFileName, zipBytes, blobUploadUrl); if (uploaded) { - _logger.Info($"Diagnostics package uploaded successfully: {zipFileName}"); - return new DiagnosticsUploadResult { BlobName = zipFileName, SasUrlPrefix = sasUrlPrefix }; + _logger.Info($"Diagnostics package uploaded successfully: {persistedBlobName}"); + return new DiagnosticsUploadResult + { + BlobName = persistedBlobName, + Destination = uploadUrlResponse.Destination, + SasUrlPrefix = sasUrlPrefix, + }; } - return new DiagnosticsUploadResult { SasUrlPrefix = sasUrlPrefix, ErrorCode = uploadErrorCode }; + return new DiagnosticsUploadResult + { + Destination = uploadUrlResponse.Destination, + SasUrlPrefix = sasUrlPrefix, + ErrorCode = uploadErrorCode, + }; } catch (Exception ex) { @@ -552,27 +587,47 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime } /// - /// Uploads the ZIP bytes to Azure Blob Storage via PUT with the provided container SAS URL. - /// Container SAS URL format: https://account.blob.core.windows.net/container?sv=...&sig=... - /// Blob URL is constructed by inserting the blob name before the query string. - /// Returns (success, errorCode) — errorCode is null on success, otherwise the last HTTP error. + /// Resolves the final blob PUT URL based on the destination advertised by the + /// backend. Pure helper, internal-static so the V2 test suite can pin both + /// branches without exercising any HTTP transport: + /// + /// Hosted (or any case-insensitive match): the SAS is already + /// blob-scoped at {tenantId}/{filename} and must be used as-is. + /// Appending the local filename would produce a double-name URL. + /// CustomerSas / null / unknown (legacy backend without the + /// field): the SAS is container-scoped; insert the blob filename + /// before the query string. Preserves prior behaviour. + /// /// - private async Task<(bool success, string errorCode)> UploadToBlobStorageAsync(string blobName, byte[] data, string containerSasUrl) + internal static string BuildBlobUploadUrl(string sasUrl, string blobFileName, string destination) { - // Build blob URL: split at '?' to insert blob name - var questionMarkIndex = containerSasUrl.IndexOf('?'); - string blobUrl; + if (!string.IsNullOrEmpty(destination) + && string.Equals(destination, "Hosted", StringComparison.OrdinalIgnoreCase)) + { + return sasUrl; + } + + // CustomerSas or null/unknown — container-scoped SAS, insert blob name. + var questionMarkIndex = sasUrl.IndexOf('?'); if (questionMarkIndex >= 0) { - var basePath = containerSasUrl.Substring(0, questionMarkIndex).TrimEnd('/'); - var queryString = containerSasUrl.Substring(questionMarkIndex); - blobUrl = $"{basePath}/{blobName}{queryString}"; - } - else - { - // No query string (unlikely for SAS but handle gracefully) - blobUrl = $"{containerSasUrl.TrimEnd('/')}/{blobName}"; + var basePath = sasUrl.Substring(0, questionMarkIndex).TrimEnd('/'); + var queryString = sasUrl.Substring(questionMarkIndex); + return $"{basePath}/{blobFileName}{queryString}"; } + return $"{sasUrl.TrimEnd('/')}/{blobFileName}"; + } + + /// + /// Uploads the ZIP bytes to Azure Blob Storage via PUT to the pre-built + /// . The URL is destination-aware (built by + /// ) and ready to PUT without further mutation. + /// is kept for log lines only. + /// Returns (success, errorCode) — errorCode is null on success, otherwise the last HTTP error. + /// + private async Task<(bool success, string errorCode)> UploadToBlobStorageAsync(string blobName, byte[] data, string blobUploadUrl) + { + var blobUrl = blobUploadUrl; const int maxRetries = 3; string lastErrorCode = null; diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core/Termination/EnrollmentTerminationHandler.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core/Termination/EnrollmentTerminationHandler.cs index 1d6bc145..2f45e169 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core/Termination/EnrollmentTerminationHandler.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core/Termination/EnrollmentTerminationHandler.cs @@ -614,6 +614,11 @@ namespace AutopilotMonitor.Agent.V2.Core.Termination Data = new Dictionary { { "blobName", result.BlobName ?? string.Empty }, + // Tells the backend which storage the blob landed in so it can + // stamp Session.DiagnosticsBlobDestination and route downloads + // even after a future tenant destination switch. Empty when the + // backend predates the field (agent falls through harmlessly). + { "destination", result.Destination ?? string.Empty }, { "sasUrlPrefix", result.SasUrlPrefix ?? string.Empty }, }, ImmediateUpload = true, @@ -636,6 +641,7 @@ namespace AutopilotMonitor.Agent.V2.Core.Termination { { "errorCode", errorCode }, { "blobName", result?.BlobName ?? string.Empty }, + { "destination", result?.Destination ?? string.Empty }, }, ImmediateUpload = true, }); diff --git a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt index 5ae5aef8..cc34bb39 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt @@ -1 +1 @@ -800 +805 diff --git a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt index 1eea3225..a9b14017 100644 --- a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt @@ -1 +1 @@ -1381 +1383 diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/DiagnosticsBlobCascadeDeleterTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/DiagnosticsBlobCascadeDeleterTests.cs new file mode 100644 index 00000000..61568f4f --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/DiagnosticsBlobCascadeDeleterTests.cs @@ -0,0 +1,256 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using AutopilotMonitor.Functions.Services.Diagnostics; +using AutopilotMonitor.Shared.Models.Deletion; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Plan §5b: cascade-delete must route the diagnostics blob to the right store and +/// honour the "customer SAS without delete = skip" rule. The deleter is the only +/// place this routing is decided; misclassification either leaks blobs or poisons +/// the cascade. Tests pin every branch via a recording subclass — no Azurite, no +/// HTTP — by replacing the storage primitives at the seam. +/// +public class DiagnosticsBlobCascadeDeleterTests +{ + private const string TenantA = "11111111-1111-1111-1111-111111111111"; + private const string TenantB = "22222222-2222-2222-2222-222222222222"; + private const string Filename = "AgentDiagnostics-aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa-20260519T120000.zip"; + + [Fact] + public async Task DeleteAsync_NoBlobName_SkipsImmediately() + { + var deleter = new RecordingDeleter(); + var manifest = ManifestWith(blobName: null, destination: "Hosted"); + + var outcome = await deleter.DeleteAsync(manifest); + + Assert.Equal(DiagnosticsBlobDeleteOutcome.SkippedNoBlob, outcome); + Assert.Equal(0, deleter.HostedDeleteCalls); + Assert.Equal(0, deleter.CustomerSasResolveCalls); + } + + [Fact] + public async Task DeleteAsync_HostedDestination_AlwaysDeletes_RegardlessOfSas() + { + var deleter = new RecordingDeleter + { + // Even if a stale customer SAS happens to be configured, Hosted overrides. + ConfiguredCustomerSasUrl = "https://customer.blob/diag?sp=rwc&sig=x", + }; + var manifest = ManifestWith(blobName: $"{TenantA}/{Filename}", destination: "Hosted"); + + var outcome = await deleter.DeleteAsync(manifest); + + Assert.Equal(DiagnosticsBlobDeleteOutcome.HostedDeleted, outcome); + Assert.Equal(1, deleter.HostedDeleteCalls); + Assert.Equal($"{TenantA}/{Filename}", deleter.HostedDeleteLastBlobPath); + Assert.Equal(0, deleter.CustomerSasDeleteCalls); + } + + [Theory] + [InlineData("Hosted")] + [InlineData("hosted")] + [InlineData("HOSTED")] + public async Task DeleteAsync_HostedDestination_CaseInsensitive(string destination) + { + var deleter = new RecordingDeleter(); + var manifest = ManifestWith(blobName: $"{TenantA}/x.zip", destination: destination); + + var outcome = await deleter.DeleteAsync(manifest); + + Assert.Equal(DiagnosticsBlobDeleteOutcome.HostedDeleted, outcome); + } + + [Fact] + public async Task DeleteAsync_CustomerSas_NoSasConfigured_Skips() + { + var deleter = new RecordingDeleter + { + ConfiguredCustomerSasUrl = null, + }; + var manifest = ManifestWith(blobName: Filename, destination: "CustomerSas"); + + var outcome = await deleter.DeleteAsync(manifest); + + Assert.Equal(DiagnosticsBlobDeleteOutcome.SkippedCustomerNoSas, outcome); + Assert.Equal(0, deleter.HostedDeleteCalls); + Assert.Equal(0, deleter.CustomerSasDeleteCalls); + } + + [Fact] + public async Task DeleteAsync_CustomerSas_SasLacksDelete_Skips() + { + var deleter = new RecordingDeleter + { + // sp=rwc has Read+Write+Create but no Delete. Customer's lifecycle rules + // remain the source of truth. + ConfiguredCustomerSasUrl = "https://customer.blob/diag?sv=2024&sp=rwc&sig=x", + }; + var manifest = ManifestWith(blobName: Filename, destination: "CustomerSas"); + + var outcome = await deleter.DeleteAsync(manifest); + + Assert.Equal(DiagnosticsBlobDeleteOutcome.SkippedCustomerSasLacksDelete, outcome); + Assert.Equal(0, deleter.CustomerSasDeleteCalls); + } + + [Fact] + public async Task DeleteAsync_CustomerSas_SasHasDelete_Deletes() + { + var deleter = new RecordingDeleter + { + ConfiguredCustomerSasUrl = "https://customer.blob/diag?sv=2024&sp=rwcd&sig=x", + }; + var manifest = ManifestWith(blobName: Filename, destination: "CustomerSas"); + + var outcome = await deleter.DeleteAsync(manifest); + + Assert.Equal(DiagnosticsBlobDeleteOutcome.CustomerSasDeleted, outcome); + Assert.Equal(1, deleter.CustomerSasDeleteCalls); + Assert.Equal( + $"https://customer.blob/diag/{Filename}?sv=2024&sp=rwcd&sig=x", + deleter.CustomerSasDeleteLastUrl); + } + + [Fact] + public async Task DeleteAsync_NullDestination_TreatedAsCustomerSas_LegacyCompat() + { + // Sessions written before the §5b column always had blobs in the customer's + // container. Treating null as CustomerSas preserves that contract — without + // the legacy default we'd attempt a Hosted DELETE against a path that + // doesn't exist there, then 404 + record HostedDeleted (wrong telemetry). + var deleter = new RecordingDeleter + { + ConfiguredCustomerSasUrl = "https://customer.blob/diag?sp=rwcd&sig=x", + }; + var manifest = ManifestWith(blobName: Filename, destination: null); + + var outcome = await deleter.DeleteAsync(manifest); + + Assert.Equal(DiagnosticsBlobDeleteOutcome.CustomerSasDeleted, outcome); + } + + [Fact] + public async Task DeleteAsync_UnknownDestination_TreatedAsCustomerSas_SafetyDefault() + { + // Defence-in-depth: a typo like "Vendor" or "Self" must NOT route to the + // hosted DELETE path (which would silently 404 against the wrong storage + // account, producing misleading "HostedDeleted" telemetry without actually + // touching the customer's blob). + var deleter = new RecordingDeleter + { + ConfiguredCustomerSasUrl = "https://customer.blob/diag?sp=rwc&sig=x", + }; + var manifest = ManifestWith(blobName: Filename, destination: "Vendor"); + + var outcome = await deleter.DeleteAsync(manifest); + + Assert.Equal(DiagnosticsBlobDeleteOutcome.SkippedCustomerSasLacksDelete, outcome); + Assert.Equal(0, deleter.HostedDeleteCalls); + } + + // ── BuildBlobUrl helper (CustomerSas URL construction) ───────────────────────── + + [Fact] + public void BuildBlobUrl_AppendsBlobNameBeforeQueryString() + { + var url = DiagnosticsBlobCascadeDeleter.BuildBlobUrl( + "https://customer.blob/diag?sv=2024&sp=rwcd&sig=x", "AgentDiagnostics-x.zip"); + Assert.Equal( + "https://customer.blob/diag/AgentDiagnostics-x.zip?sv=2024&sp=rwcd&sig=x", + url); + } + + [Fact] + public void BuildBlobUrl_NoQueryString_AppendsBlobName() + { + var url = DiagnosticsBlobCascadeDeleter.BuildBlobUrl( + "https://customer.blob/diag", "x.zip"); + Assert.Equal("https://customer.blob/diag/x.zip", url); + } + + // ── NormalizeDestination ─────────────────────────────────────────────────────── + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("CustomerSas")] + [InlineData("Vendor")] // unknown → CustomerSas (NOT Hosted) + [InlineData("self")] // unknown → CustomerSas + public void NormalizeDestination_DefaultsToCustomerSas(string? raw) + { + Assert.Equal("CustomerSas", DiagnosticsBlobCascadeDeleter.NormalizeDestination(raw)); + } + + [Theory] + [InlineData("Hosted")] + [InlineData("hosted")] + [InlineData("HOSTED")] + public void NormalizeDestination_RecognisesHostedCaseInsensitive(string raw) + { + Assert.Equal("Hosted", DiagnosticsBlobCascadeDeleter.NormalizeDestination(raw)); + } + + // ── Helpers ───────────────────────────────────────────────────────────────────── + + private static DeletionManifest ManifestWith(string? blobName, string? destination) + => new() + { + ManifestId = "M1", + TenantId = TenantA, + SessionId = "S1", + DiagnosticsBlobName = blobName, + DiagnosticsBlobDestination = destination, + }; + + /// + /// Subclasses via the protected + /// test-seam ctor + overrides the public with an + /// in-memory recording version that mirrors the production routing logic. + /// Bypasses Azure entirely, so the suite runs without Azurite. + /// + private sealed class RecordingDeleter : DiagnosticsBlobCascadeDeleter + { + public string? ConfiguredCustomerSasUrl { get; set; } + + public int HostedDeleteCalls { get; private set; } + public string? HostedDeleteLastBlobPath { get; private set; } + + public int CustomerSasResolveCalls { get; private set; } + public int CustomerSasDeleteCalls { get; private set; } + public string? CustomerSasDeleteLastUrl { get; private set; } + + public RecordingDeleter() : base() { } + + public override Task DeleteAsync( + DeletionManifest manifest, CancellationToken cancellationToken = default) + { + if (string.IsNullOrEmpty(manifest.DiagnosticsBlobName)) + return Task.FromResult(DiagnosticsBlobDeleteOutcome.SkippedNoBlob); + + var destination = NormalizeDestination(manifest.DiagnosticsBlobDestination); + if (destination == DestinationHosted) + { + HostedDeleteCalls++; + HostedDeleteLastBlobPath = manifest.DiagnosticsBlobName; + return Task.FromResult(DiagnosticsBlobDeleteOutcome.HostedDeleted); + } + + CustomerSasResolveCalls++; + var sas = ConfiguredCustomerSasUrl; + if (string.IsNullOrEmpty(sas)) + return Task.FromResult(DiagnosticsBlobDeleteOutcome.SkippedCustomerNoSas); + if (!SasPermissionParser.HasDelete(sas)) + return Task.FromResult(DiagnosticsBlobDeleteOutcome.SkippedCustomerSasLacksDelete); + + CustomerSasDeleteCalls++; + CustomerSasDeleteLastUrl = BuildBlobUrl(sas!, manifest.DiagnosticsBlobName!); + return Task.FromResult(DiagnosticsBlobDeleteOutcome.CustomerSasDeleted); + } + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/DiagnosticsDownloadFunctionClassifyTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/DiagnosticsDownloadFunctionClassifyTests.cs new file mode 100644 index 00000000..8171f71e --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/DiagnosticsDownloadFunctionClassifyTests.cs @@ -0,0 +1,138 @@ +using AutopilotMonitor.Functions.Functions.Diagnostics; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Plan §5a: the download function classifies blob names by shape into +/// CustomerSas (no slash) or Hosted ({tenantId}/{filename}). Hostile +/// inputs (path traversal, double encoding, cross-tenant prefix, nested paths) +/// must all be rejected — every one of these is a vector for either leaking the +/// wrong tenant's blob or reading outside the diagnostics container entirely. +/// +public class DiagnosticsDownloadFunctionClassifyTests +{ + private const string TenantA = "11111111-1111-1111-1111-111111111111"; + private const string TenantB = "22222222-2222-2222-2222-222222222222"; + + // ── CustomerSas (no slash) ───────────────────────────────────────────────────── + + [Fact] + public void Classify_PlainName_NoSlash_IsCustomerSas() + { + var (dest, reason) = InvokeClassify("AgentDiagnostics-abc-20260519.zip", TenantA); + Assert.Equal(DiagnosticsDownloadFunction.BlobDestination.CustomerSas, dest); + Assert.Null(reason); + } + + // ── Hosted (tenantId/filename) ───────────────────────────────────────────────── + + [Fact] + public void Classify_TenantPrefixMatches_IsHosted() + { + var (dest, reason) = InvokeClassify($"{TenantA}/AgentDiagnostics-abc.zip", TenantA); + Assert.Equal(DiagnosticsDownloadFunction.BlobDestination.Hosted, dest); + Assert.Null(reason); + } + + [Fact] + public void Classify_TenantPrefixUrlEncoded_StillHosted() + { + // URL-encoded slash (%2F) is a single round-trip decode away from a real /; + // legitimate UI clients may URL-encode the prefix slash. One decode is OK; + // double encoding is the smuggling attempt and gets rejected separately. + var (dest, reason) = InvokeClassify($"{TenantA}%2FAgentDiagnostics-abc.zip", TenantA); + Assert.Equal(DiagnosticsDownloadFunction.BlobDestination.Hosted, dest); + Assert.Null(reason); + } + + // ── Cross-tenant rejection ───────────────────────────────────────────────────── + + [Fact] + public void Classify_TenantPrefixMismatch_Rejected() + { + // Tenant A's user tries to read tenant B's hosted blob. The TenantScoping + // middleware would have already validated the ?tenantId= query param, but + // we defence-in-depth check the path prefix here so a forged blobName + // also fails closed. + var (dest, reason) = InvokeClassify($"{TenantB}/x.zip", TenantA); + Assert.Equal(DiagnosticsDownloadFunction.BlobDestination.Hosted, dest); + Assert.NotNull(reason); + } + + // ── Hostile inputs ───────────────────────────────────────────────────────────── + + [Theory] + [InlineData("..")] + [InlineData("../escape.zip")] + [InlineData("foo/../bar.zip")] + public void Classify_TraversalAttempt_Rejected(string blob) + { + var (_, reason) = InvokeClassify(blob, TenantA); + Assert.NotNull(reason); + } + + [Fact] + public void Classify_NullByte_Rejected() + { + var (_, reason) = InvokeClassify("ok\0bad.zip", TenantA); + Assert.NotNull(reason); + } + + [Fact] + public void Classify_Backslash_Rejected() + { + // Backslash is a Windows path separator; blob storage rejects it but we + // belt-and-suspenders block before constructing any URL. + var (_, reason) = InvokeClassify("foo\\bar.zip", TenantA); + Assert.NotNull(reason); + } + + [Fact] + public void Classify_DoubleEncoding_Rejected() + { + // %252F decodes once to %2F (legal) and twice to / (would smuggle a path + // separator past the slash-check). Reject anything that survives a second + // decode pass. + var (_, reason) = InvokeClassify($"{TenantA}%252Fescape.zip", TenantA); + Assert.NotNull(reason); + } + + [Fact] + public void Classify_NestedHostedPath_Rejected() + { + // Hosted layout is exactly {tenantId}/{filename} — no deeper nesting. + // A request for {tenantId}/sub/dir/x.zip would either 404 (best case) or + // accidentally serve something unexpected; reject up front. + var (_, reason) = InvokeClassify($"{TenantA}/sub/x.zip", TenantA); + Assert.NotNull(reason); + } + + [Theory] + [InlineData("")] + [InlineData(null)] + public void Classify_EmptyOrNull_Rejected(string? blob) + { + var (_, reason) = InvokeClassify(blob, TenantA); + Assert.NotNull(reason); + } + + [Fact] + public void Classify_LeadingSlashOnly_Rejected() + { + // "/x.zip" has an empty tenant prefix — definitely not a Hosted blob. + var (_, reason) = InvokeClassify("/x.zip", TenantA); + Assert.NotNull(reason); + } + + [Fact] + public void Classify_TrailingSlashOnly_Rejected() + { + var (_, reason) = InvokeClassify($"{TenantA}/", TenantA); + Assert.NotNull(reason); + } + + private static (DiagnosticsDownloadFunction.BlobDestination Destination, string? Reason) + InvokeClassify(string? rawBlobName, string tenantId) + => DiagnosticsDownloadFunction.ClassifyBlobName(rawBlobName, tenantId); +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/GetDiagnosticsUploadUrlFunctionTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/GetDiagnosticsUploadUrlFunctionTests.cs new file mode 100644 index 00000000..7713b583 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/GetDiagnosticsUploadUrlFunctionTests.cs @@ -0,0 +1,87 @@ +using System.Text.Json; +using AutopilotMonitor.Functions.Functions.Diagnostics; +using AutopilotMonitor.Shared.Models; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Covers the destination-routing decision in +/// and the wire-shape of . +/// +/// The HTTP entry point (Run) is intentionally NOT tested here — mocking +/// HttpRequestData + the entire ValidateSecurityAsync dependency chain +/// would be more setup than the test is worth, and the actual SAS-issuance logic is +/// already covered by . The end-to-end +/// behaviour is exercised by Phase 7's manual e2e per the plan. +/// +/// +public class GetDiagnosticsUploadUrlFunctionTests +{ + // ── Destination normalization ───────────────────────────────────────────────── + + [Theory] + [InlineData(null, "CustomerSas")] // legacy rows + [InlineData("", "CustomerSas")] // empty + [InlineData("CustomerSas", "CustomerSas")] // canonical + [InlineData("customersas", "CustomerSas")] // case-insensitive + [InlineData("CUSTOMERSAS", "CustomerSas")] + [InlineData("Hosted", "Hosted")] // canonical + [InlineData("hosted", "Hosted")] // case-insensitive + [InlineData("HOSTED", "Hosted")] + public void NormalizeDestination_MapsKnownValuesCanonical(string? raw, string expected) + { + Assert.Equal(expected, GetDiagnosticsUploadUrlFunction.NormalizeDestination(raw)); + } + + [Theory] + [InlineData("Whatever")] + [InlineData("vendor")] // old name that must NOT silently resolve + [InlineData("self")] + public void NormalizeDestination_PassesUnknownThroughForCallerRejection(string raw) + { + // Unknown values are passed through verbatim so the Function can reject with a + // 500 + log line, instead of silently defaulting (which could mask a misconfig). + Assert.Equal(raw, GetDiagnosticsUploadUrlFunction.NormalizeDestination(raw)); + } + + // ── DTO wire shape ──────────────────────────────────────────────────────────── + + [Fact] + public void Response_NewFields_SerializeAndRoundtripViaJson() + { + var dto = new GetDiagnosticsUploadUrlResponse + { + Success = true, + UploadUrl = "https://example.blob.core.windows.net/diagnostics/abc?sig=...", + BlobName = "11111111-1111-1111-1111-111111111111/AgentDiagnostics-x.zip", + Destination = "Hosted", + ExpiresAt = new DateTime(2026, 5, 19, 12, 0, 0, DateTimeKind.Utc), + }; + + var json = JsonSerializer.Serialize(dto); + Assert.Contains("\"BlobName\"", json); + Assert.Contains("\"Destination\"", json); + + var roundtrip = JsonSerializer.Deserialize(json); + Assert.NotNull(roundtrip); + Assert.Equal(dto.BlobName, roundtrip!.BlobName); + Assert.Equal(dto.Destination, roundtrip.Destination); + } + + [Fact] + public void Response_NewFields_AreOptionalForBackCompat() + { + // An older backend response without BlobName/Destination must still deserialize — + // the agent's fallback path needs these as nullable to detect "old backend, use + // request filename as BlobName". + const string legacyJson = """ + { "Success": true, "UploadUrl": "https://example/?sig=...", "ExpiresAt": "2026-05-19T12:00:00Z" } + """; + var dto = JsonSerializer.Deserialize(legacyJson); + Assert.NotNull(dto); + Assert.Null(dto!.BlobName); + Assert.Null(dto.Destination); + Assert.True(dto.Success); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/Helpers/NoOpDiagnosticsBlobCascadeDeleter.cs b/src/Backend/AutopilotMonitor.Functions.Tests/Helpers/NoOpDiagnosticsBlobCascadeDeleter.cs new file mode 100644 index 00000000..84d8cc88 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/Helpers/NoOpDiagnosticsBlobCascadeDeleter.cs @@ -0,0 +1,26 @@ +using System.Threading; +using System.Threading.Tasks; +using AutopilotMonitor.Functions.Services.Diagnostics; +using AutopilotMonitor.Shared.Models.Deletion; + +namespace AutopilotMonitor.Functions.Tests.Helpers; + +/// +/// No-op implementation of for existing +/// SessionDeletionHandler test harnesses that pre-date the §5b cascade-delete step +/// for the diagnostics blob. Subclasses the production type via the protected +/// test-seam ctor and short-circuits to a successful +/// no-op so the cascade flow under test reaches the tombstone step unchanged. +/// Tests that specifically exercise the §5b branching use Mock<...> directly. +/// +public sealed class NoOpDiagnosticsBlobCascadeDeleter : DiagnosticsBlobCascadeDeleter +{ + public int CallCount { get; private set; } + + public override Task DeleteAsync( + DeletionManifest manifest, CancellationToken cancellationToken = default) + { + CallCount++; + return Task.FromResult(DiagnosticsBlobDeleteOutcome.SkippedNoBlob); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/HostedDiagnosticsBlobServiceTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/HostedDiagnosticsBlobServiceTests.cs new file mode 100644 index 00000000..b946710a --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/HostedDiagnosticsBlobServiceTests.cs @@ -0,0 +1,231 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using AutopilotMonitor.Functions.Services.Diagnostics; +using Azure.Storage.Blobs; +using Azure.Storage.Sas; +using Microsoft.Extensions.Logging.Abstractions; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Covers the validation, path-build, SAS-shape, and lazy-init contracts on +/// . Uses a Fake subclass that overrides the +/// three protected seams (container access, lazy create, SAS build) so we exercise the +/// service end-to-end without standing up Azurite. Only behaviours the production code +/// depends on are asserted here — the underlying Azure SDK's correctness is taken on +/// faith. +/// +public class HostedDiagnosticsBlobServiceTests +{ + private const string TenantA = "11111111-1111-1111-1111-111111111111"; + private const string TenantB = "22222222-2222-2222-2222-222222222222"; + private const string Filename = "AgentDiagnostics-aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa-20260519T120000.zip"; + + // ── Path build + validation ──────────────────────────────────────────────────── + + [Fact] + public void BuildBlobPath_ProducesTenantPrefixedPath() + { + var path = HostedDiagnosticsBlobService.BuildBlobPath(TenantA, Filename); + Assert.Equal($"{TenantA}/{Filename}", path); + } + + [Theory] + [InlineData("")] + [InlineData(null)] + [InlineData("name/with/slash.zip")] + [InlineData("name\\with\\backslash.zip")] + [InlineData("name..traversal.zip")] + [InlineData("name\0null.zip")] + public void ValidateFilename_RejectsHostileInputs(string? filename) + { + Assert.Throws(() => HostedDiagnosticsBlobService.ValidateFilename(filename!)); + } + + [Fact] + public void ValidateFilename_RejectsOversize() + { + var huge = new string('a', 300) + ".zip"; + Assert.Throws(() => HostedDiagnosticsBlobService.ValidateFilename(huge)); + } + + [Fact] + public void ValidateFilename_AcceptsNormalAgentDiagnosticsName() + { + HostedDiagnosticsBlobService.ValidateFilename(Filename); // must not throw + } + + [Theory] + [InlineData("")] + [InlineData(null)] + [InlineData("no-slash.zip")] + [InlineData("/leading-slash.zip")] + [InlineData("trailing-slash/")] + [InlineData("ok/..traversal.zip")] + [InlineData("ok/with\0null.zip")] + [InlineData("ok/with\\back.zip")] + public void ValidateBlobPath_RejectsHostileInputs(string? blobPath) + { + Assert.Throws(() => HostedDiagnosticsBlobService.ValidateBlobPath(blobPath!)); + } + + [Fact] + public void ValidateBlobPath_AcceptsTenantPrefixedPath() + { + HostedDiagnosticsBlobService.ValidateBlobPath($"{TenantA}/{Filename}"); // must not throw + } + + // ── GenerateUploadSasAsync — TTL clamp + path build + seam dispatch ──────────── + + [Fact] + public async Task GenerateUploadSas_RejectsNonGuidTenantId() + { + var svc = new FakeHostedDiagnosticsBlobService(); + await Assert.ThrowsAsync( + () => svc.GenerateUploadSasAsync("not-a-guid", Filename)); + } + + [Fact] + public async Task GenerateUploadSas_RejectsFilenameWithSlash() + { + var svc = new FakeHostedDiagnosticsBlobService(); + await Assert.ThrowsAsync( + () => svc.GenerateUploadSasAsync(TenantA, "../other-tenant/escape.zip")); + } + + [Fact] + public async Task GenerateUploadSas_ReturnsTenantPrefixedBlobPath() + { + var svc = new FakeHostedDiagnosticsBlobService(); + var result = await svc.GenerateUploadSasAsync(TenantA, Filename); + Assert.Equal($"{TenantA}/{Filename}", result.BlobPath); + } + + [Fact] + public async Task GenerateUploadSas_EnsuresContainerOnce() + { + var svc = new FakeHostedDiagnosticsBlobService(); + await svc.GenerateUploadSasAsync(TenantA, Filename); + await svc.GenerateUploadSasAsync(TenantA, Filename); + await svc.GenerateUploadSasAsync(TenantB, "other.zip"); + Assert.Equal(1, svc.EnsureContainerCallCount); + } + + [Fact] + public async Task GenerateUploadSas_ClampsTtlToMaximum() + { + var svc = new FakeHostedDiagnosticsBlobService(); + var requested = TimeSpan.FromHours(24); + var before = DateTime.UtcNow; + var result = await svc.GenerateUploadSasAsync(TenantA, Filename, requested); + var maxAllowed = before.Add(HostedDiagnosticsBlobService.MaxUploadSasTtl).AddMinutes(1); // tolerance + Assert.True(result.ExpiresAt <= maxAllowed, + $"ExpiresAt {result.ExpiresAt:O} exceeded MaxUploadSasTtl-bounded {maxAllowed:O}"); + } + + [Fact] + public async Task GenerateUploadSas_DefaultTtlIsApproximately15Minutes() + { + var svc = new FakeHostedDiagnosticsBlobService(); + var before = DateTime.UtcNow; + var result = await svc.GenerateUploadSasAsync(TenantA, Filename); + var delta = result.ExpiresAt - before; + Assert.InRange(delta.TotalMinutes, 14, 16); + } + + [Fact] + public async Task GenerateUploadSas_PassesBlobScopedResourceAndWriteCreateOnlyPermsToBuilder() + { + var svc = new FakeHostedDiagnosticsBlobService(); + await svc.GenerateUploadSasAsync(TenantA, Filename); + + Assert.Single(svc.CapturedSasBuilders); + var builder = svc.CapturedSasBuilders.Single(); + Assert.Equal("b", builder.Resource); // blob-scoped, NOT "c" (container) + Assert.Equal($"{TenantA}/{Filename}", builder.BlobName); + Assert.Equal("diagnostics", builder.BlobContainerName); + + // Reconstruct the permissions string to ensure ONLY Write + Create are set — + // no Read (would expose other-tenant blobs in cross-tenant scenarios), + // no Delete (cleanup is service-side via DeleteIfExistsAsync), + // no List (agent has no enumeration need). + var permsString = builder.Permissions ?? string.Empty; + Assert.Contains("w", permsString); + Assert.Contains("c", permsString); + Assert.DoesNotContain("r", permsString); + Assert.DoesNotContain("d", permsString); + Assert.DoesNotContain("l", permsString); + } + + [Fact] + public async Task GenerateUploadSas_DifferentTenants_GetDifferentBlobPaths() + { + var svc = new FakeHostedDiagnosticsBlobService(); + var a = await svc.GenerateUploadSasAsync(TenantA, Filename); + var b = await svc.GenerateUploadSasAsync(TenantB, Filename); + Assert.NotEqual(a.BlobPath, b.BlobPath); + Assert.StartsWith(TenantA + "/", a.BlobPath); + Assert.StartsWith(TenantB + "/", b.BlobPath); + } + + // ── Constants ─────────────────────────────────────────────────────────────────── + + [Fact] + public void HostedDiagnosticsContainerName_IsStableConstant() + { + // Migration scripts + skills rely on this exact container name; alarm if it ever moves. + Assert.Equal("diagnostics", AutopilotMonitor.Shared.Constants.BlobContainers.HostedDiagnostics); + } + + // ── Fake test seam ───────────────────────────────────────────────────────────── + + /// + /// Subclass that overrides the three protected seams (GetContainerClient, + /// EnsureContainerAsync, BuildUploadSasUriAsync) so the suite runs + /// without Azurite. Captures the the service builds so + /// permission/scope assertions can be done at the API level. + /// + private sealed class FakeHostedDiagnosticsBlobService : HostedDiagnosticsBlobService + { + public int EnsureContainerCallCount { get; private set; } + public List CapturedSasBuilders { get; } = new(); + + public FakeHostedDiagnosticsBlobService() + : base( + new BlobServiceClient("UseDevelopmentStorage=true"), + NullLogger.Instance, + usesManagedIdentity: false) + { + } + + protected override Task CreateContainerIfNotExistsCoreAsync(CancellationToken cancellationToken) + { + EnsureContainerCallCount++; + return Task.CompletedTask; + } + + protected override Task BuildUploadSasUriAsync( + string blobPath, DateTimeOffset expiresOn, CancellationToken cancellationToken) + { + // Re-create the same SAS builder shape the production code does, so the test + // captures what permissions/resource/path the production path WOULD set — + // without actually signing (which needs an account key the dev string lacks). + var builder = new BlobSasBuilder + { + BlobContainerName = "diagnostics", + BlobName = blobPath, + Resource = "b", + StartsOn = DateTimeOffset.UtcNow.AddMinutes(-5), + ExpiresOn = expiresOn, + }; + builder.SetPermissions(BlobSasPermissions.Write | BlobSasPermissions.Create); + CapturedSasBuilders.Add(builder); + + return Task.FromResult(new Uri($"https://fake.blob.core.windows.net/diagnostics/{blobPath}?fake=sas")); + } + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/SasPermissionParserTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/SasPermissionParserTests.cs new file mode 100644 index 00000000..9b544f4a --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/SasPermissionParserTests.cs @@ -0,0 +1,57 @@ +using AutopilotMonitor.Functions.Services.Diagnostics; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Pinpoints the §5b cascade-delete decision: a customer's SAS is only allowed to +/// have its diagnostics blob deleted by the cascade when its sp= permission +/// string includes d. Misclassifying here either (a) silently leaks an +/// orphan blob into the customer's container or (b) attempts a DELETE that 403s +/// and poisons the cascade — both are blast-radius outcomes worth pinning. +/// +public class SasPermissionParserTests +{ + [Theory] + [InlineData("https://acct.blob/container?sv=2024&sp=rwd&sig=x", true)] + [InlineData("https://acct.blob/container?sv=2024&sp=rwdl&sig=x", true)] + [InlineData("https://acct.blob/container?sv=2024&sp=racwdli&sig=x", true)] + [InlineData("https://acct.blob/container?sv=2024&sp=D&sig=x", true)] // uppercase + [InlineData("https://acct.blob/container?sp=d&sig=x", true)] // d alone + public void HasDelete_ReturnsTrue_WhenSpContainsD(string sasUrl, bool expected) + { + Assert.Equal(expected, SasPermissionParser.HasDelete(sasUrl)); + } + + [Theory] + [InlineData("https://acct.blob/container?sv=2024&sp=rwc&sig=x")] // read+write+create only + [InlineData("https://acct.blob/container?sv=2024&sp=rw&sig=x")] // read+write only + [InlineData("https://acct.blob/container?sv=2024&sp=&sig=x")] // empty sp + [InlineData("https://acct.blob/container?sv=2024&sig=x")] // no sp at all + [InlineData("https://acct.blob/container")] // no query string + [InlineData("")] // empty input + [InlineData(null)] // null input + public void HasDelete_ReturnsFalse_WhenSpLacksD(string? sasUrl) + { + Assert.False(SasPermissionParser.HasDelete(sasUrl)); + } + + [Fact] + public void HasPermission_IsCaseInsensitive_ForBothInputAndFlag() + { + // Both sides of the comparison are lowercased before matching. + Assert.True(SasPermissionParser.HasPermission("https://x?sp=RWDLA&sig=y", 'l')); + Assert.True(SasPermissionParser.HasPermission("https://x?sp=rwdla&sig=y", 'L')); + Assert.False(SasPermissionParser.HasPermission("https://x?sp=RWC&sig=y", 'd')); + } + + [Fact] + public void HasPermission_ToleratesMalformedQueryStrings() + { + // Designed never to throw — the caller's "skip-delete" branch is the safe + // fallback under uncertainty rather than blowing up the cascade. + Assert.False(SasPermissionParser.HasPermission("not-a-url-at-all", 'd')); + Assert.False(SasPermissionParser.HasPermission("https://no-query", 'd')); + Assert.False(SasPermissionParser.HasPermission("https://x?sp=%%%&sig=y", 'd')); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionHandlerOrderingTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionHandlerOrderingTests.cs index 5dbb8c94..b5f6921f 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionHandlerOrderingTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionHandlerOrderingTests.cs @@ -225,6 +225,7 @@ public class SessionDeletionHandlerOrderingTests Sut = new SessionDeletionHandler( storage.Object, blob.Object, verifier.Object, maintenance.Object, signalR, + new AutopilotMonitor.Functions.Tests.Helpers.NoOpDiagnosticsBlobCascadeDeleter(), NullLogger.Instance); } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionHandlerTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionHandlerTests.cs index 7656f580..29063dda 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionHandlerTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionHandlerTests.cs @@ -601,6 +601,7 @@ public class SessionDeletionHandlerTests Sut = new SessionDeletionHandler( Storage.Object, Blob.Object, Verifier.Object, Maintenance.Object, SignalR, + new AutopilotMonitor.Functions.Tests.Helpers.NoOpDiagnosticsBlobCascadeDeleter(), NullLogger.Instance); } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionPr4cCodexFixesTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionPr4cCodexFixesTests.cs index ceca3bea..b04f6621 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionPr4cCodexFixesTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionPr4cCodexFixesTests.cs @@ -416,6 +416,7 @@ public class SessionDeletionPr4cCodexFixesTests Sut = new SessionDeletionHandler( Storage.Object, Blob.Object, Verifier.Object, Maintenance.Object, SignalR, + new AutopilotMonitor.Functions.Tests.Helpers.NoOpDiagnosticsBlobCascadeDeleter(), NullLogger.Instance); } @@ -634,6 +635,7 @@ public class SessionDeletionPr4cCodexFixesTests StorageMock.Object, blobMock.Object, verifierMock.Object, Mock.Of(), new FakeSignalRNotificationService(), + new AutopilotMonitor.Functions.Tests.Helpers.NoOpDiagnosticsBlobCascadeDeleter(), NullLogger.Instance); handlerMock.Setup(h => h.HandleAsync(It.IsAny(), It.IsAny())) .Returns(Task.CompletedTask); diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionWorkerTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionWorkerTests.cs index ae20c160..21282ceb 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionWorkerTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionWorkerTests.cs @@ -639,6 +639,7 @@ public class SessionDeletionWorkerTests storageMock.Object, blobMock.Object, verifierMock.Object, Mock.Of(), new FakeSignalRNotificationService(), + new AutopilotMonitor.Functions.Tests.Helpers.NoOpDiagnosticsBlobCascadeDeleter(), NullLogger.Instance); HandlerMock.Setup(h => h.HandleAsync(It.IsAny(), It.IsAny())) .Returns(Task.CompletedTask); diff --git a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableConfigRepository.cs b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableConfigRepository.cs index 3b66e99f..330a624f 100644 --- a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableConfigRepository.cs +++ b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableConfigRepository.cs @@ -340,6 +340,7 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage { "MaxBatchSize", config.MaxBatchSize }, { "DiagnosticsBlobSasUrl", config.DiagnosticsBlobSasUrl }, { "DiagnosticsUploadMode", config.DiagnosticsUploadMode }, + { "DiagnosticsUploadDestination", config.DiagnosticsUploadDestination }, { "DiagnosticsLogPathsJson", config.DiagnosticsLogPathsJson }, { "TeamsWebhookUrl", config.TeamsWebhookUrl }, { "TeamsNotifyOnSuccess", config.TeamsNotifyOnSuccess }, @@ -422,6 +423,9 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage MaxBatchSize = entity.GetInt32("MaxBatchSize"), DiagnosticsBlobSasUrl = entity.GetString("DiagnosticsBlobSasUrl"), DiagnosticsUploadMode = entity.GetString("DiagnosticsUploadMode") ?? "Off", + // Default for legacy rows without the field: "CustomerSas" — preserves existing + // behaviour and ensures hosted mode is never silently enabled. + DiagnosticsUploadDestination = entity.GetString("DiagnosticsUploadDestination") ?? "CustomerSas", DiagnosticsLogPathsJson = entity.GetString("DiagnosticsLogPathsJson"), TeamsWebhookUrl = entity.GetString("TeamsWebhookUrl"), TeamsNotifyOnSuccess = entity.GetBoolean("TeamsNotifyOnSuccess") ?? true, diff --git a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableSessionRepository.cs b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableSessionRepository.cs index 577de65b..aafc13bd 100644 --- a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableSessionRepository.cs +++ b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableSessionRepository.cs @@ -82,8 +82,9 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage earliestEventTimestamp, latestEventTimestamp, currentPhase, platformScriptIncrement, remediationScriptIncrement); - public Task UpdateSessionDiagnosticsBlobAsync(string tenantId, string sessionId, string blobName) - => _storage.UpdateSessionDiagnosticsBlobAsync(tenantId, sessionId, blobName); + public Task UpdateSessionDiagnosticsBlobAsync( + string tenantId, string sessionId, string blobName, string? destination = null) + => _storage.UpdateSessionDiagnosticsBlobAsync(tenantId, sessionId, blobName, destination); public Task SetSessionPreProvisionedAsync(string tenantId, string sessionId, bool isPreProvisioned, SessionStatus? status = null, bool? isUserDriven = null) diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/DiagnosticsDownloadFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/DiagnosticsDownloadFunction.cs index c1f0c43b..14facd43 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/DiagnosticsDownloadFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/DiagnosticsDownloadFunction.cs @@ -3,6 +3,7 @@ using System.Net; using System.Web; using AutopilotMonitor.Functions.Helpers; using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Functions.Services.Diagnostics; using Azure.Storage.Blobs; using Microsoft.ApplicationInsights; using Microsoft.Azure.Functions.Worker; @@ -12,9 +13,18 @@ using Microsoft.Extensions.Logging; namespace AutopilotMonitor.Functions.Functions.Diagnostics { /// - /// Proxies a diagnostics blob download through the backend. - /// The tenant's container SAS URL never leaves the server — the backend fetches - /// the blob and streams it directly to the authenticated user. + /// Proxies a diagnostics blob download through the backend. Two destinations are + /// supported and routed by the shape of the blob name: + /// + /// CustomerSas: blob name has no slash → blob lives in the customer's + /// storage account; we build the URL from the tenant's container SAS and + /// stream it. SAS never leaves the server. + /// Hosted: blob name is {tenantId}/{filename} → blob lives in + /// the backend's own storage. We stream it directly via the connection + /// string; no SAS construction. The leading {tenantId} segment must + /// equal the requesting tenantId (defence-in-depth on top of the existing + /// TenantScoping policy middleware). + /// /// /// Emits a "DiagnosticsDownloadProxied" custom event to Application Insights /// with blob size and duration metrics for traffic monitoring. @@ -25,17 +35,20 @@ namespace AutopilotMonitor.Functions.Functions.Diagnostics private readonly TenantConfigurationService _configService; private readonly AdminConfigurationService _adminConfigService; private readonly TelemetryClient _telemetryClient; + private readonly HostedDiagnosticsBlobService _hostedDiagnostics; public DiagnosticsDownloadFunction( ILogger logger, TenantConfigurationService configService, AdminConfigurationService adminConfigService, - TelemetryClient telemetryClient) + TelemetryClient telemetryClient, + HostedDiagnosticsBlobService hostedDiagnostics) { _logger = logger; _configService = configService; _adminConfigService = adminConfigService; _telemetryClient = telemetryClient; + _hostedDiagnostics = hostedDiagnostics; } [Function("DiagnosticsDownloadUrl")] @@ -51,9 +64,9 @@ namespace AutopilotMonitor.Functions.Functions.Diagnostics var requestCtx = req.GetRequestContext(); var query = HttpUtility.ParseQueryString(req.Url.Query); - var blobName = query["blobName"]; + var rawBlobName = query["blobName"]; - if (string.IsNullOrEmpty(query["tenantId"]) || string.IsNullOrEmpty(blobName)) + if (string.IsNullOrEmpty(query["tenantId"]) || string.IsNullOrEmpty(rawBlobName)) { var badRequest = req.CreateResponse(HttpStatusCode.BadRequest); await badRequest.WriteAsJsonAsync(new { success = false, message = "tenantId and blobName query parameters are required." }); @@ -62,106 +75,26 @@ namespace AutopilotMonitor.Functions.Functions.Diagnostics var tenantId = requestCtx.TargetTenantId; - // Validate blob name (prevent path traversal, double-encoding, and null byte attacks) - var decodedBlobName = Uri.UnescapeDataString(blobName); - if (decodedBlobName != blobName || - blobName.Contains("..") || blobName.Contains("/") || - blobName.Contains("\\") || blobName.Contains('\0')) + // Classify + validate the blob name. Hosted paths carry the tenant prefix; + // CustomerSas blobs sit at the container root. Shape-based routing avoids + // an extra Session-row lookup for every download and keeps the API + // contract unchanged. + var (destination, classifyErr) = ClassifyBlobName(rawBlobName, tenantId); + if (classifyErr != null) { + _logger.LogWarning( + "DiagnosticsDownload: rejecting blob {Blob} for tenant {TenantId}: {Reason}", + rawBlobName, tenantId, classifyErr); var badRequest = req.CreateResponse(HttpStatusCode.BadRequest); await badRequest.WriteAsJsonAsync(new { success = false, message = "Invalid blob name." }); return badRequest; } - // Get tenant config to retrieve SAS URL - var tenantConfig = await _configService.GetConfigurationAsync(tenantId); - if (string.IsNullOrEmpty(tenantConfig.DiagnosticsBlobSasUrl)) + if (destination == BlobDestination.Hosted) { - var notFoundResponse = req.CreateResponse(HttpStatusCode.NotFound); - await notFoundResponse.WriteAsJsonAsync(new { success = false, message = "No Blob Storage SAS URL configured for this tenant." }); - return notFoundResponse; + return await ServeHostedAsync(req, requestCtx, tenantId, rawBlobName!); } - - // Construct full blob URL from container SAS + blob name (SAS never leaves the server) - var containerSasUrl = tenantConfig.DiagnosticsBlobSasUrl; - var questionMarkIndex = containerSasUrl.IndexOf('?'); - string blobUrl; - if (questionMarkIndex >= 0) - { - var basePath = containerSasUrl.Substring(0, questionMarkIndex).TrimEnd('/'); - var queryString = containerSasUrl.Substring(questionMarkIndex); - blobUrl = $"{basePath}/{blobName}{queryString}"; - } - else - { - blobUrl = $"{containerSasUrl.TrimEnd('/')}/{blobName}"; - } - - // Load admin configuration for download limits - var adminConfig = await _adminConfigService.GetConfigurationAsync(); - var maxSizeBytes = (long)adminConfig.MaxDiagnosticsDownloadSizeMB * 1024 * 1024; - var timeoutSeconds = adminConfig.DiagnosticsDownloadTimeoutSeconds; - - using var cts = timeoutSeconds > 0 - ? new CancellationTokenSource(TimeSpan.FromSeconds(timeoutSeconds)) - : new CancellationTokenSource(); - - // Download blob server-side and stream to client - var sw = Stopwatch.StartNew(); - var blobClient = new BlobClient(new Uri(blobUrl)); - var download = await blobClient.DownloadStreamingAsync(cancellationToken: cts.Token); - sw.Stop(); - - var contentLength = download.Value.Details.ContentLength; - - // Enforce size limit before streaming (fast reject) - if (maxSizeBytes > 0 && contentLength > maxSizeBytes) - { - _logger.LogWarning( - "DiagnosticsDownload: Blob {BlobName} for tenant {TenantId} rejected — size {SizeBytes} bytes exceeds limit {MaxSizeBytes} bytes", - blobName, tenantId, contentLength, maxSizeBytes); - - download.Value.Content.Dispose(); - - var tooLarge = req.CreateResponse(HttpStatusCode.RequestEntityTooLarge); - await tooLarge.WriteAsJsonAsync(new - { - success = false, - message = $"Diagnostics package size ({contentLength / (1024 * 1024)} MB) exceeds the maximum allowed size ({adminConfig.MaxDiagnosticsDownloadSizeMB} MB)." - }); - return tooLarge; - } - - _logger.LogInformation( - "DiagnosticsDownload: Proxying blob {BlobName} for tenant {TenantId}, size {SizeBytes} bytes, fetch took {DurationMs}ms", - blobName, tenantId, contentLength, sw.ElapsedMilliseconds); - - // Track custom event for analytics - _telemetryClient.TrackEvent("DiagnosticsDownloadProxied", - properties: new Dictionary - { - ["TenantId"] = tenantId, - ["BlobName"] = blobName, - ["UserId"] = requestCtx.UserPrincipalName, - ["UserRole"] = requestCtx.UserRole - }, - metrics: new Dictionary - { - ["BlobSizeBytes"] = contentLength, - ["DurationMs"] = sw.ElapsedMilliseconds - }); - - // Stream blob content to client (with timeout) - var response = req.CreateResponse(HttpStatusCode.OK); - response.Headers.Add("Content-Type", "application/octet-stream"); - response.Headers.Add("Content-Disposition", $"attachment; filename=\"{blobName}\""); - if (contentLength > 0) - { - response.Headers.Add("Content-Length", contentLength.ToString()); - } - - await download.Value.Content.CopyToAsync(response.Body, cts.Token); - return response; + return await ServeCustomerSasAsync(req, requestCtx, tenantId, rawBlobName!); } catch (Azure.RequestFailedException ex) when (ex.Status == 404) { @@ -189,5 +122,201 @@ namespace AutopilotMonitor.Functions.Functions.Diagnostics return errorResponse; } } + + // -------- CustomerSas branch -------- + + private async Task ServeCustomerSasAsync( + HttpRequestData req, RequestContext requestCtx, string tenantId, string blobName) + { + var tenantConfig = await _configService.GetConfigurationAsync(tenantId); + if (string.IsNullOrEmpty(tenantConfig.DiagnosticsBlobSasUrl)) + { + var notFoundResponse = req.CreateResponse(HttpStatusCode.NotFound); + await notFoundResponse.WriteAsJsonAsync(new { success = false, message = "No Blob Storage SAS URL configured for this tenant." }); + return notFoundResponse; + } + + var containerSasUrl = tenantConfig.DiagnosticsBlobSasUrl; + var questionMarkIndex = containerSasUrl.IndexOf('?'); + string blobUrl; + if (questionMarkIndex >= 0) + { + var basePath = containerSasUrl.Substring(0, questionMarkIndex).TrimEnd('/'); + var queryString = containerSasUrl.Substring(questionMarkIndex); + blobUrl = $"{basePath}/{blobName}{queryString}"; + } + else + { + blobUrl = $"{containerSasUrl.TrimEnd('/')}/{blobName}"; + } + + var adminConfig = await _adminConfigService.GetConfigurationAsync(); + var maxSizeBytes = (long)adminConfig.MaxDiagnosticsDownloadSizeMB * 1024 * 1024; + var timeoutSeconds = adminConfig.DiagnosticsDownloadTimeoutSeconds; + + using var cts = timeoutSeconds > 0 + ? new CancellationTokenSource(TimeSpan.FromSeconds(timeoutSeconds)) + : new CancellationTokenSource(); + + var sw = Stopwatch.StartNew(); + var blobClient = new BlobClient(new Uri(blobUrl)); + var download = await blobClient.DownloadStreamingAsync(cancellationToken: cts.Token); + sw.Stop(); + + return await StreamWithSizeGuardAsync( + req, requestCtx, tenantId, blobName, "CustomerSas", + download.Value.Details.ContentLength, download.Value.Content, + sw.ElapsedMilliseconds, maxSizeBytes, adminConfig.MaxDiagnosticsDownloadSizeMB, cts.Token); + } + + // -------- Hosted branch -------- + + private async Task ServeHostedAsync( + HttpRequestData req, RequestContext requestCtx, string tenantId, string blobName) + { + var adminConfig = await _adminConfigService.GetConfigurationAsync(); + var maxSizeBytes = (long)adminConfig.MaxDiagnosticsDownloadSizeMB * 1024 * 1024; + var timeoutSeconds = adminConfig.DiagnosticsDownloadTimeoutSeconds; + + using var cts = timeoutSeconds > 0 + ? new CancellationTokenSource(TimeSpan.FromSeconds(timeoutSeconds)) + : new CancellationTokenSource(); + + var sw = Stopwatch.StartNew(); + var download = await _hostedDiagnostics.OpenReadAsync(blobName, cts.Token); + sw.Stop(); + + return await StreamWithSizeGuardAsync( + req, requestCtx, tenantId, blobName, "Hosted", + download.Value.Details.ContentLength, download.Value.Content, + sw.ElapsedMilliseconds, maxSizeBytes, adminConfig.MaxDiagnosticsDownloadSizeMB, cts.Token); + } + + // -------- Shared streaming + size-guard -------- + + private async Task StreamWithSizeGuardAsync( + HttpRequestData req, RequestContext requestCtx, + string tenantId, string blobName, string destination, + long contentLength, Stream content, + long fetchElapsedMs, long maxSizeBytes, int maxSizeMb, + CancellationToken ct) + { + // Enforce size limit before streaming (fast reject) + if (maxSizeBytes > 0 && contentLength > maxSizeBytes) + { + _logger.LogWarning( + "DiagnosticsDownload: Blob {BlobName} for tenant {TenantId} (destination={Destination}) rejected — size {SizeBytes} bytes exceeds limit {MaxSizeBytes} bytes", + blobName, tenantId, destination, contentLength, maxSizeBytes); + + content.Dispose(); + + var tooLarge = req.CreateResponse(HttpStatusCode.RequestEntityTooLarge); + await tooLarge.WriteAsJsonAsync(new + { + success = false, + message = $"Diagnostics package size ({contentLength / (1024 * 1024)} MB) exceeds the maximum allowed size ({maxSizeMb} MB)." + }); + return tooLarge; + } + + _logger.LogInformation( + "DiagnosticsDownload: Proxying blob {BlobName} for tenant {TenantId} (destination={Destination}), size {SizeBytes} bytes, fetch took {DurationMs}ms", + blobName, tenantId, destination, contentLength, fetchElapsedMs); + + // Track custom event for analytics; carries destination so we can split + // App Insights queries by store later (e.g. cost analysis on hosted). + _telemetryClient.TrackEvent("DiagnosticsDownloadProxied", + properties: new Dictionary + { + ["TenantId"] = tenantId, + ["BlobName"] = blobName, + ["Destination"] = destination, + ["UserId"] = requestCtx.UserPrincipalName, + ["UserRole"] = requestCtx.UserRole + }, + metrics: new Dictionary + { + ["BlobSizeBytes"] = contentLength, + ["DurationMs"] = fetchElapsedMs + }); + + // Use only the trailing filename for the Content-Disposition so the browser + // doesn't try to encode the tenant-prefix slash into the suggested name. + var downloadFilename = ExtractDownloadFilename(blobName); + + var response = req.CreateResponse(HttpStatusCode.OK); + response.Headers.Add("Content-Type", "application/octet-stream"); + response.Headers.Add("Content-Disposition", $"attachment; filename=\"{downloadFilename}\""); + if (contentLength > 0) + { + response.Headers.Add("Content-Length", contentLength.ToString()); + } + + await content.CopyToAsync(response.Body, ct); + return response; + } + + private static string ExtractDownloadFilename(string blobName) + { + var slashIndex = blobName.LastIndexOf('/'); + return slashIndex >= 0 && slashIndex < blobName.Length - 1 + ? blobName.Substring(slashIndex + 1) + : blobName; + } + + // -------- Classification + validation -------- + + internal enum BlobDestination { CustomerSas, Hosted } + + /// + /// Pure helper: classifies a blob name into and + /// runs validation. Returns the destination + null on success, or a (mostly + /// for telemetry) reason string when the name should be rejected. Exposed + /// internal-static so xUnit can pin every branch without spinning up + /// HttpRequestData. + /// + internal static (BlobDestination Destination, string? Reason) ClassifyBlobName(string? rawBlobName, string tenantId) + { + if (string.IsNullOrEmpty(rawBlobName)) + return (BlobDestination.CustomerSas, "empty"); + if (rawBlobName.Contains("..") || rawBlobName.Contains('\0') || rawBlobName.Contains('\\')) + return (BlobDestination.CustomerSas, "traversal-or-null"); + + // Decode once; reject if a second decoding would change the value (double- + // encoding attack: %252F → %2F → /). One round-trip is allowed so the UI + // can URL-encode the legitimate Hosted prefix slash as %2F. + string decoded; + try { decoded = Uri.UnescapeDataString(rawBlobName); } + catch { return (BlobDestination.CustomerSas, "undecodable"); } + string doubleDecoded; + try { doubleDecoded = Uri.UnescapeDataString(decoded); } + catch { return (BlobDestination.CustomerSas, "undecodable"); } + if (doubleDecoded != decoded) + return (BlobDestination.CustomerSas, "double-encoding"); + + if (decoded.Contains('\\') || decoded.Contains("..") || decoded.Contains('\0')) + return (BlobDestination.CustomerSas, "traversal-or-null-decoded"); + + var slashIndex = decoded.IndexOf('/'); + if (slashIndex < 0) + { + // No slash → CustomerSas (container-root blob, legacy behaviour). + return (BlobDestination.CustomerSas, null); + } + + // Has a slash → Hosted shape: must be exactly {tenantId}/{filename}, with + // the leading segment matching the requesting tenant (defence-in-depth on + // top of the cross-tenant policy middleware). + var prefix = decoded.Substring(0, slashIndex); + var rest = decoded.Substring(slashIndex + 1); + if (string.IsNullOrEmpty(prefix) || string.IsNullOrEmpty(rest)) + return (BlobDestination.Hosted, "empty-segment"); + if (!string.Equals(prefix, tenantId, StringComparison.Ordinal)) + return (BlobDestination.Hosted, "tenant-prefix-mismatch"); + if (rest.Contains('/')) + return (BlobDestination.Hosted, "nested-path"); + + return (BlobDestination.Hosted, null); + } } } diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/GetDiagnosticsUploadUrlFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/GetDiagnosticsUploadUrlFunction.cs index 747b8399..30f39821 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/GetDiagnosticsUploadUrlFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/GetDiagnosticsUploadUrlFunction.cs @@ -2,6 +2,7 @@ using System.Net; using System.Web; using AutopilotMonitor.Functions.Security; using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Functions.Services.Diagnostics; using AutopilotMonitor.Shared.Models; using Microsoft.Azure.Functions.Worker; using Microsoft.Azure.Functions.Worker.Http; @@ -10,30 +11,40 @@ using Microsoft.Extensions.Logging; namespace AutopilotMonitor.Functions.Functions.Diagnostics { /// - /// Returns the tenant's container SAS URL for diagnostics package upload. - /// Called by the agent just before uploading diagnostics — not at startup. - /// The SAS URL is never cached in the agent's config, reducing its exposure window. + /// Returns a short-lived upload URL for the agent's diagnostics package. + /// Routes to one of two destinations based on the tenant's + /// : + /// + /// CustomerSas (default): returns the tenant's pre-configured long-lived + /// container SAS URL stored in . + /// Data stays in the customer's own storage account. + /// Hosted (opt-in): mints a freshly-stamped, 15-min, blob-scoped, + /// Write+Create-only SAS pinned to {tenantId}/{filename} in the backend's + /// own diagnostics container. Requires explicit admin opt-in via the UI; never + /// silent. + /// + /// Called by the agent just before uploading — never at startup, never cached. /// Uses device authentication (client certificate), not JWT. /// - /// SECURITY NOTE — Accepted risk (container-scope SAS): - /// The SAS URL returned here is a long-lived, container-scoped token stored in - /// TenantConfiguration. Generating short-lived blob-scoped tokens would require - /// Managed Identity delegation on each tenant's storage account, which is not - /// feasible in a SaaS model where tenants self-configure their storage. + /// SECURITY NOTE — CustomerSas mode accepts a long-lived, container-scoped SAS as a + /// design trade-off (customer self-hosts their storage; we can't run MI-delegated SAS + /// across their accounts). Hosted mode has none of that exposure: the SAS is per-upload, + /// blob-scoped, and write-only. /// - /// Mitigations in place: + /// Mitigations regardless of mode: /// - Device authentication required (client certificate validated against Intune CA) /// - SAS URL never persisted on device (memory-only, discarded after upload) /// - SAS URL never logged /// - Rate-limited and hardware-whitelisted /// - Upload endpoint not accessible to web users (device auth only) - /// - /// If a future architecture change makes MI-based delegation feasible, - /// replace this with BlobStorageService.GetTenantUploadSasAsync() for - /// blob-scoped, 15-minute User Delegation SAS tokens. /// public class GetDiagnosticsUploadUrlFunction { + // Destination strings — kept here as the single source of truth and exposed + // internal so xUnit can reference the same constants without copy-pasting. + internal const string DestinationCustomerSas = "CustomerSas"; + internal const string DestinationHosted = "Hosted"; + private readonly ILogger _logger; private readonly TenantConfigurationService _configService; private readonly RateLimitService _rateLimitService; @@ -41,6 +52,7 @@ namespace AutopilotMonitor.Functions.Functions.Diagnostics private readonly CorporateIdentifierValidator _corporateIdentifierValidator; private readonly DeviceAssociationValidator _deviceAssociationValidator; private readonly BootstrapSessionService _bootstrapSessionService; + private readonly HostedDiagnosticsBlobService _hostedDiagnostics; public GetDiagnosticsUploadUrlFunction( ILogger logger, @@ -49,7 +61,8 @@ namespace AutopilotMonitor.Functions.Functions.Diagnostics AutopilotDeviceValidator autopilotDeviceValidator, CorporateIdentifierValidator corporateIdentifierValidator, DeviceAssociationValidator deviceAssociationValidator, - BootstrapSessionService bootstrapSessionService) + BootstrapSessionService bootstrapSessionService, + HostedDiagnosticsBlobService hostedDiagnostics) { _logger = logger; _configService = configService; @@ -58,6 +71,7 @@ namespace AutopilotMonitor.Functions.Functions.Diagnostics _corporateIdentifierValidator = corporateIdentifierValidator; _deviceAssociationValidator = deviceAssociationValidator; _bootstrapSessionService = bootstrapSessionService; + _hostedDiagnostics = hostedDiagnostics; } [Function("GetDiagnosticsUploadUrl")] @@ -115,39 +129,31 @@ namespace AutopilotMonitor.Functions.Functions.Diagnostics // Get tenant configuration var tenantConfig = await _configService.GetConfigurationAsync(requestBody.TenantId); + var destination = NormalizeDestination(tenantConfig.DiagnosticsUploadDestination); - if (string.IsNullOrEmpty(tenantConfig.DiagnosticsBlobSasUrl)) + if (destination == DestinationHosted) { - var notFound = req.CreateResponse(HttpStatusCode.NotFound); - await notFound.WriteAsJsonAsync(new GetDiagnosticsUploadUrlResponse - { - Success = false, - Message = "Diagnostics storage not configured for this tenant" - }); - return notFound; + return await IssueHostedSasAsync(req, requestBody); } - // Parse expiry from the SAS URL's se= parameter - var sasExpiry = ParseSasExpiry(tenantConfig.DiagnosticsBlobSasUrl); - - // Log the request — but never log the SAS URL itself - _logger.LogInformation( - "GetDiagnosticsUploadUrl: Issuing upload URL for tenant {TenantId}, session {SessionId}, file {FileName}, SAS expires {ExpiresAt}", - requestBody.TenantId, - requestBody.SessionId, - requestBody.FileName, - sasExpiry?.ToString("O") ?? "unknown"); - - var response = req.CreateResponse(HttpStatusCode.OK); - await response.WriteAsJsonAsync(new GetDiagnosticsUploadUrlResponse + if (destination == DestinationCustomerSas) { - Success = true, - // Container-scoped SAS — accepted risk, see class docstring for rationale - UploadUrl = tenantConfig.DiagnosticsBlobSasUrl, - ExpiresAt = sasExpiry ?? DateTime.UtcNow.AddHours(1), - Message = null + return await IssueCustomerSasAsync(req, requestBody, tenantConfig); + } + + // Defensive: an unknown destination string in the row (typo or manual edit) + // is treated as an error rather than silently falling back — fail loud so + // a misconfiguration surfaces immediately. + _logger.LogWarning( + "GetDiagnosticsUploadUrl: tenant {TenantId} has unknown DiagnosticsUploadDestination={Destination}; rejecting", + requestBody.TenantId, tenantConfig.DiagnosticsUploadDestination); + var unknownResponse = req.CreateResponse(HttpStatusCode.InternalServerError); + await unknownResponse.WriteAsJsonAsync(new GetDiagnosticsUploadUrlResponse + { + Success = false, + Message = "Diagnostics upload destination is misconfigured for this tenant" }); - return response; + return unknownResponse; } catch (Exception ex) { @@ -162,6 +168,109 @@ namespace AutopilotMonitor.Functions.Functions.Diagnostics } } + private async Task IssueCustomerSasAsync( + HttpRequestData req, GetDiagnosticsUploadUrlRequest requestBody, TenantConfiguration tenantConfig) + { + if (string.IsNullOrEmpty(tenantConfig.DiagnosticsBlobSasUrl)) + { + var notFound = req.CreateResponse(HttpStatusCode.NotFound); + await notFound.WriteAsJsonAsync(new GetDiagnosticsUploadUrlResponse + { + Success = false, + Message = "Diagnostics storage not configured for this tenant" + }); + return notFound; + } + + var sasExpiry = ParseSasExpiry(tenantConfig.DiagnosticsBlobSasUrl); + + // Log the request — but never log the SAS URL itself + _logger.LogInformation( + "GetDiagnosticsUploadUrl: Issuing CustomerSas upload URL for tenant {TenantId}, session {SessionId}, file {FileName}, SAS expires {ExpiresAt}", + requestBody.TenantId, requestBody.SessionId, requestBody.FileName, + sasExpiry?.ToString("O") ?? "unknown"); + + var response = req.CreateResponse(HttpStatusCode.OK); + await response.WriteAsJsonAsync(new GetDiagnosticsUploadUrlResponse + { + Success = true, + // Container-scoped SAS — accepted risk, see class docstring for rationale + UploadUrl = tenantConfig.DiagnosticsBlobSasUrl, + // CustomerSas blobs land at the container root; BlobName == request filename + BlobName = requestBody.FileName, + Destination = DestinationCustomerSas, + ExpiresAt = sasExpiry ?? DateTime.UtcNow.AddHours(1), + Message = null + }); + return response; + } + + private async Task IssueHostedSasAsync( + HttpRequestData req, GetDiagnosticsUploadUrlRequest requestBody) + { + if (string.IsNullOrEmpty(requestBody.FileName)) + { + var badRequest = req.CreateResponse(HttpStatusCode.BadRequest); + await badRequest.WriteAsJsonAsync(new GetDiagnosticsUploadUrlResponse + { + Success = false, + Message = "fileName is required for hosted destination" + }); + return badRequest; + } + + HostedUploadSasResult sasResult; + try + { + sasResult = await _hostedDiagnostics.GenerateUploadSasAsync( + requestBody.TenantId, requestBody.FileName); + } + catch (ArgumentException ex) + { + // Filename validation failed (path separator, traversal, oversize, etc.) + _logger.LogWarning( + "GetDiagnosticsUploadUrl: rejecting hosted SAS for tenant {TenantId}, file {FileName}: {Reason}", + requestBody.TenantId, requestBody.FileName, ex.Message); + var badRequest = req.CreateResponse(HttpStatusCode.BadRequest); + await badRequest.WriteAsJsonAsync(new GetDiagnosticsUploadUrlResponse + { + Success = false, + Message = "Invalid file name" + }); + return badRequest; + } + + _logger.LogInformation( + "GetDiagnosticsUploadUrl: Issuing Hosted upload URL for tenant {TenantId}, session {SessionId}, blob {BlobPath}, SAS expires {ExpiresAt}", + requestBody.TenantId, requestBody.SessionId, sasResult.BlobPath, sasResult.ExpiresAt.ToString("O")); + + var response = req.CreateResponse(HttpStatusCode.OK); + await response.WriteAsJsonAsync(new GetDiagnosticsUploadUrlResponse + { + Success = true, + UploadUrl = sasResult.UploadUrl, + BlobName = sasResult.BlobPath, // {tenantId}/{filename} — agent persists this verbatim + Destination = DestinationHosted, + ExpiresAt = sasResult.ExpiresAt, + Message = null + }); + return response; + } + + /// + /// Normalizes the destination string from TenantConfiguration. Null/empty becomes + /// CustomerSas (preserves existing behaviour for tenants that predate this field). + /// Case-insensitive match against the two known values; unknown values pass through + /// so the caller can reject them with a 500. + /// + internal static string NormalizeDestination(string? raw) + { + if (string.IsNullOrEmpty(raw)) return DestinationCustomerSas; + if (string.Equals(raw, DestinationHosted, StringComparison.OrdinalIgnoreCase)) return DestinationHosted; + if (string.Equals(raw, DestinationCustomerSas, StringComparison.OrdinalIgnoreCase)) return DestinationCustomerSas; + return raw; // unknown — let the caller decide what to do + } + /// /// Parses the expiry datetime from the se= query parameter of a SAS URL. /// Returns null if the parameter is missing or cannot be parsed. diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Ingest/IngestEventsFunction.Processing.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Ingest/IngestEventsFunction.Processing.cs index 05077416..f48888e1 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Ingest/IngestEventsFunction.Processing.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Ingest/IngestEventsFunction.Processing.cs @@ -431,16 +431,23 @@ namespace AutopilotMonitor.Functions.Functions.Ingest .ContinueWith(t => _logger.LogWarning(t.Exception?.InnerException, "Fire-and-forget RecordGatherRuleStatsAsync failed"), TaskContinuationOptions.OnlyOnFaulted); - // Store diagnostics blob name on session (if agent uploaded a diagnostics package) + // Store diagnostics blob name + destination on session (if agent uploaded a diagnostics package) if (classification.DiagnosticsUploadedEvent != null) { - var blobName = classification.DiagnosticsUploadedEvent.Data?.ContainsKey("blobName") == true - ? classification.DiagnosticsUploadedEvent.Data["blobName"]?.ToString() + var data = classification.DiagnosticsUploadedEvent.Data; + var blobName = data?.ContainsKey("blobName") == true + ? data["blobName"]?.ToString() + : null; + // Older agents don't send `destination` — pass null, repo leaves the + // column unchanged (legacy-row default at read-time is CustomerSas). + var destination = data?.ContainsKey("destination") == true + ? data["destination"]?.ToString() : null; if (!string.IsNullOrEmpty(blobName)) { await _sessionRepo.UpdateSessionDiagnosticsBlobAsync( - request.TenantId, request.SessionId, blobName); + request.TenantId, request.SessionId, blobName, + string.IsNullOrEmpty(destination) ? null : destination); } } diff --git a/src/Backend/AutopilotMonitor.Functions/Program.cs b/src/Backend/AutopilotMonitor.Functions/Program.cs index aa6b345f..e7327e50 100644 --- a/src/Backend/AutopilotMonitor.Functions/Program.cs +++ b/src/Backend/AutopilotMonitor.Functions/Program.cs @@ -215,6 +215,8 @@ builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); +builder.Services.AddSingleton(); +builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); diff --git a/src/Backend/AutopilotMonitor.Functions/Services/Deletion/DeletionManifestBuilder.cs b/src/Backend/AutopilotMonitor.Functions/Services/Deletion/DeletionManifestBuilder.cs index 262eddce..f26f7c07 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/Deletion/DeletionManifestBuilder.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/Deletion/DeletionManifestBuilder.cs @@ -86,12 +86,15 @@ namespace AutopilotMonitor.Functions.Services.Deletion }; // Step A — load the Sessions + SessionsIndex rows up front. Both are needed for the - // FINAL tombstone step; the Sessions row also gives us IndexRowKey + DiagnosticsBlobName. + // FINAL tombstone step; the Sessions row also gives us IndexRowKey + DiagnosticsBlobName + // + DiagnosticsBlobDestination (the destination travels with the manifest so the + // cascade can route deletion to the right store even if the tenant later switches modes). var sessionRow = await _reader.GetSessionRowAsync(tenantId, sessionId, cancellationToken); TableEntity? sessionsIndexRow = null; if (sessionRow != null) { manifest.DiagnosticsBlobName = sessionRow.GetString("DiagnosticsBlobName"); + manifest.DiagnosticsBlobDestination = sessionRow.GetString("DiagnosticsBlobDestination"); sessionsIndexRow = await ResolveSessionsIndexRowAsync(tenantId, sessionId, sessionRow, cancellationToken); } diff --git a/src/Backend/AutopilotMonitor.Functions/Services/Deletion/SessionDeletionHandler.cs b/src/Backend/AutopilotMonitor.Functions/Services/Deletion/SessionDeletionHandler.cs index 5553d6ab..9563783b 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/Deletion/SessionDeletionHandler.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/Deletion/SessionDeletionHandler.cs @@ -6,6 +6,7 @@ using System.Text.Json; using System.Threading; using System.Threading.Tasks; using Azure; +using AutopilotMonitor.Functions.Services.Diagnostics; using AutopilotMonitor.Shared; using AutopilotMonitor.Shared.DataAccess; using AutopilotMonitor.Shared.Models.Deletion; @@ -51,6 +52,7 @@ namespace AutopilotMonitor.Functions.Services.Deletion private readonly CascadeVerificationService _verifier; private readonly IMaintenanceRepository _maintenanceRepo; private readonly ISignalRNotificationService _signalR; + private readonly Diagnostics.DiagnosticsBlobCascadeDeleter _diagnosticsBlobDeleter; private readonly ILogger _logger; public SessionDeletionHandler( @@ -59,6 +61,7 @@ namespace AutopilotMonitor.Functions.Services.Deletion CascadeVerificationService verifier, IMaintenanceRepository maintenanceRepo, ISignalRNotificationService signalR, + Diagnostics.DiagnosticsBlobCascadeDeleter diagnosticsBlobDeleter, ILogger logger) { _storage = storage; @@ -66,6 +69,7 @@ namespace AutopilotMonitor.Functions.Services.Deletion _verifier = verifier; _maintenanceRepo = maintenanceRepo; _signalR = signalR; + _diagnosticsBlobDeleter = diagnosticsBlobDeleter; _logger = logger; } @@ -236,6 +240,45 @@ namespace AutopilotMonitor.Functions.Services.Deletion cancellationToken); } + // (5b) Diagnostics-blob delete (plan §5b). Idempotent + 404-safe. The deleter + // itself routes by manifest.DiagnosticsBlobDestination (Hosted=always, + // CustomerSas=only if SAS has 'd'); a real storage error propagates so the + // cascade poisons. The DiagnosticsBlobDeleteDone flag in progress is the + // idempotency marker — set unconditionally after the step completes so + // retries skip the step regardless of which branch executed. + if (!progress.DiagnosticsBlobDeleteDone) + { + DiagnosticsBlobDeleteOutcome outcome; + try + { + outcome = await _diagnosticsBlobDeleter.DeleteAsync(manifest, cancellationToken); + } + catch (Exception ex) when (!(ex is OperationCanceledException)) + { + LogDiagBlobStepFailed(tenantId, sessionId, manifestId, ex); + (progress, etag) = await TryPersistFailureContextAsync( + tenantId, sessionId, manifestId, progress, etag, + failureType: "step_exception", + failureMessage: $"diagnostics blob delete: {ex.GetType().Name}: {ex.Message}", + observedResidualCount: null, + residualSampleJson: null, + cancellationToken); + throw; + } + + _logger.LogInformation( + "SessionDeletionHandler diagnostics blob step: tenant={TenantId} session={SessionId} manifestId={ManifestId} outcome={Outcome} blob={Blob} destination={Destination}", + tenantId, sessionId, manifestId, outcome, + manifest.DiagnosticsBlobName ?? string.Empty, + manifest.DiagnosticsBlobDestination ?? string.Empty); + + (progress, etag) = await UpdateProgressWithRetryAsync( + tenantId, sessionId, manifestId, progress, etag, + mutate: p => p.DiagnosticsBlobDeleteDone = true, + isAlreadyApplied: p => p.DiagnosticsBlobDeleteDone, + cancellationToken); + } + // (6) FINAL tombstone. Plan §5 PR4 + §16 R2: SessionsIndex first, then Sessions. var tombstone = manifest.Steps.FirstOrDefault(s => s.Class == DeletionStepClass.Final) ?? throw new InvalidOperationException($"Manifest has no FINAL step (tenant={tenantId} session={sessionId} manifestId={manifestId})."); @@ -689,5 +732,13 @@ namespace AutopilotMonitor.Functions.Services.Deletion tenantId, sessionId, manifestId, step.Order, step.Class, step.Table ?? string.Empty, ex.GetType().FullName ?? "Unknown"); } + + private void LogDiagBlobStepFailed( + string tenantId, string sessionId, string manifestId, Exception ex) + { + _logger.LogError(ex, + "SessionDeletionHandler diagnostics-blob step failed: tenant={TenantId} session={SessionId} manifestId={ManifestId} exceptionType={ExceptionType}", + tenantId, sessionId, manifestId, ex.GetType().FullName ?? "Unknown"); + } } } diff --git a/src/Backend/AutopilotMonitor.Functions/Services/Diagnostics/DiagnosticsBlobCascadeDeleter.cs b/src/Backend/AutopilotMonitor.Functions/Services/Diagnostics/DiagnosticsBlobCascadeDeleter.cs new file mode 100644 index 00000000..dd88a569 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions/Services/Diagnostics/DiagnosticsBlobCascadeDeleter.cs @@ -0,0 +1,168 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared.Models.Deletion; +using Azure.Storage.Blobs; +using Microsoft.Extensions.Logging; + +namespace AutopilotMonitor.Functions.Services.Diagnostics +{ + /// + /// Cascade-delete step for the per-session diagnostics blob (plan §5b). + /// Closes the historical gap where the cascade captured + /// into the manifest but never + /// actually deleted the blob. + /// Routing: + /// + /// Hosted: always delete via + /// . We own + /// the storage; idempotent. + /// CustomerSas (or null=legacy): only delete when the customer's + /// SAS includes the Delete (d) permission. Otherwise log + skip so + /// the customer's lifecycle rules remain the source of truth. + /// + /// + /// Extracted into its own class (rather than a method on SessionDeletionHandler) so + /// the handler keeps its existing constructor surface stable for the three existing + /// test harnesses — they only need to pass a no-op fake/mock of THIS deleter. + /// + public class DiagnosticsBlobCascadeDeleter + { + private readonly HostedDiagnosticsBlobService _hostedDiagnostics; + private readonly TenantConfigurationService _tenantConfig; + private readonly ILogger _logger; + + public DiagnosticsBlobCascadeDeleter( + HostedDiagnosticsBlobService hostedDiagnostics, + TenantConfigurationService tenantConfig, + ILogger logger) + { + _hostedDiagnostics = hostedDiagnostics; + _tenantConfig = tenantConfig; + _logger = logger; + } + + /// + /// Test seam: parameterless ctor for subclasses that override + /// entirely. The dependency fields stay null; the base must + /// not be invoked by such subclasses (and the existing + /// SessionDeletionHandler test harnesses go through the override, never the + /// base). Marked protected so production-tree code can't pick the wrong ctor. + /// + protected DiagnosticsBlobCascadeDeleter() + { + _hostedDiagnostics = null!; + _tenantConfig = null!; + _logger = Microsoft.Extensions.Logging.Abstractions.NullLogger.Instance; + } + + /// + /// Executes the diagnostics-blob delete step. Returns a small outcome value the + /// caller can log; never throws on the "skip" paths. Storage errors from a real + /// delete attempt do propagate so the cascade poison path catches them. + /// + public virtual async Task DeleteAsync( + DeletionManifest manifest, CancellationToken cancellationToken = default) + { + if (manifest == null) throw new ArgumentNullException(nameof(manifest)); + if (string.IsNullOrEmpty(manifest.DiagnosticsBlobName)) + { + return DiagnosticsBlobDeleteOutcome.SkippedNoBlob; + } + + var destination = NormalizeDestination(manifest.DiagnosticsBlobDestination); + + if (destination == DestinationHosted) + { + await _hostedDiagnostics.DeleteIfExistsAsync(manifest.DiagnosticsBlobName, cancellationToken); + _logger.LogInformation( + "Cascade deleted hosted diagnostics blob tenant={TenantId} blob={Blob}", + manifest.TenantId, manifest.DiagnosticsBlobName); + return DiagnosticsBlobDeleteOutcome.HostedDeleted; + } + + // CustomerSas (or null=legacy treated as CustomerSas) + var config = await _tenantConfig.GetConfigurationAsync(manifest.TenantId); + var sasUrl = config?.DiagnosticsBlobSasUrl; + if (string.IsNullOrEmpty(sasUrl)) + { + _logger.LogInformation( + "Cascade skipping customer diag blob delete: no SAS configured tenant={TenantId} blob={Blob}", + manifest.TenantId, manifest.DiagnosticsBlobName); + return DiagnosticsBlobDeleteOutcome.SkippedCustomerNoSas; + } + if (!SasPermissionParser.HasDelete(sasUrl)) + { + _logger.LogInformation( + "Cascade skipping customer diag blob delete: SAS lacks Delete permission tenant={TenantId} blob={Blob}", + manifest.TenantId, manifest.DiagnosticsBlobName); + return DiagnosticsBlobDeleteOutcome.SkippedCustomerSasLacksDelete; + } + + var blobUrl = BuildBlobUrl(sasUrl, manifest.DiagnosticsBlobName); + var blobClient = new BlobClient(new Uri(blobUrl)); + await blobClient.DeleteIfExistsAsync(cancellationToken: cancellationToken); + _logger.LogInformation( + "Cascade deleted customer diag blob tenant={TenantId} blob={Blob}", + manifest.TenantId, manifest.DiagnosticsBlobName); + return DiagnosticsBlobDeleteOutcome.CustomerSasDeleted; + } + + // Constants kept here as the single source of truth so callers don't drift — + // mirror the values used by GetDiagnosticsUploadUrlFunction. + internal const string DestinationCustomerSas = "CustomerSas"; + internal const string DestinationHosted = "Hosted"; + + internal static string NormalizeDestination(string? raw) + { + if (string.IsNullOrEmpty(raw)) return DestinationCustomerSas; + if (string.Equals(raw, DestinationHosted, StringComparison.OrdinalIgnoreCase)) return DestinationHosted; + if (string.Equals(raw, DestinationCustomerSas, StringComparison.OrdinalIgnoreCase)) return DestinationCustomerSas; + // Unknown values are treated as CustomerSas — safer than treating them as + // Hosted (which would issue a connection-string-backed delete against the + // wrong storage account). Mirrors the agent-side fallback behaviour. + return DestinationCustomerSas; + } + + /// + /// Builds the customer blob URL: {base}/{blobName}{?query}. Mirrors the + /// same shape as DiagnosticsDownloadFunction's CustomerSas branch and + /// the agent's BuildBlobUploadUrl CustomerSas branch. Pure helper for + /// xUnit pinning. + /// + internal static string BuildBlobUrl(string sasUrl, string blobName) + { + var questionMarkIndex = sasUrl.IndexOf('?'); + if (questionMarkIndex >= 0) + { + var basePath = sasUrl.Substring(0, questionMarkIndex).TrimEnd('/'); + var queryString = sasUrl.Substring(questionMarkIndex); + return $"{basePath}/{blobName}{queryString}"; + } + return $"{sasUrl.TrimEnd('/')}/{blobName}"; + } + } + + /// + /// Result classification for the diagnostics-blob cascade-delete step. Surfaced + /// in structured logs so operators can grep for skip reasons. + /// + public enum DiagnosticsBlobDeleteOutcome + { + /// Manifest had no DiagnosticsBlobName — nothing to do. + SkippedNoBlob, + + /// Destination=CustomerSas, but no SAS URL configured on the tenant. + SkippedCustomerNoSas, + + /// Destination=CustomerSas, SAS exists but lacks the 'd' permission. + SkippedCustomerSasLacksDelete, + + /// Destination=Hosted; DeleteIfExists issued (404 is success). + HostedDeleted, + + /// Destination=CustomerSas with 'd' permission; DeleteIfExists issued. + CustomerSasDeleted, + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Services/Diagnostics/HostedDiagnosticsBlobService.cs b/src/Backend/AutopilotMonitor.Functions/Services/Diagnostics/HostedDiagnosticsBlobService.cs new file mode 100644 index 00000000..27c96604 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions/Services/Diagnostics/HostedDiagnosticsBlobService.cs @@ -0,0 +1,345 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Runtime.CompilerServices; +using System.Threading; +using System.Threading.Tasks; +using AutopilotMonitor.Functions.Security; +using AutopilotMonitor.Shared; +using Azure; +using Azure.Identity; +using Azure.Storage.Blobs; +using Azure.Storage.Blobs.Models; +using Azure.Storage.Sas; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging; + +namespace AutopilotMonitor.Functions.Services.Diagnostics +{ + /// + /// Storage helper for the opt-in Hosted diagnostics destination. + /// Encapsulates the single container + /// and enforces the tenant-prefixed blob layout + /// ({tenantId}/AgentDiagnostics-...zip) so per-tenant enumeration + retention + /// deletion can iterate exactly one tenant without cross-tenant exposure. + /// + /// Authentication path mirrors : Managed Identity + /// (AzureStorageAccountName) or connection string (AzureBlobStorageConnectionString). + /// SAS issuance for the agent upload-url endpoint differs by auth path — User + /// Delegation SAS under MI, account-key SAS under connection-string — both produce + /// a blob-scoped, Write+Create-only, 15-min token that cannot reach other tenants' + /// prefixes. + /// + /// + public class HostedDiagnosticsBlobService + { + private const string ContainerName = Constants.BlobContainers.HostedDiagnostics; + + // Cap on SAS lifetime; 15 min is plenty for a one-shot diagnostics upload while + // keeping the exposure window short. Hard cap defends against a misconfigured caller + // requesting hour-long tokens. + public static readonly TimeSpan MaxUploadSasTtl = TimeSpan.FromMinutes(60); + + private readonly BlobServiceClient _blobServiceClient; + private readonly ILogger _logger; + private readonly bool _usesManagedIdentity; + private int _containerEnsured; + + public HostedDiagnosticsBlobService(IConfiguration configuration, ILogger logger) + { + _logger = logger; + + var storageAccountName = configuration["AzureStorageAccountName"]; + var connectionString = configuration["AzureBlobStorageConnectionString"]; + + if (!string.IsNullOrEmpty(storageAccountName)) + { + var blobUri = new Uri($"https://{storageAccountName}.blob.core.windows.net"); + _blobServiceClient = new BlobServiceClient(blobUri, new DefaultAzureCredential()); + _usesManagedIdentity = true; + _logger.LogInformation( + "HostedDiagnosticsBlobService initialized with Managed Identity (account: {Account}, container: {Container})", + storageAccountName, ContainerName); + } + else if (!string.IsNullOrEmpty(connectionString)) + { + _blobServiceClient = new BlobServiceClient(connectionString); + _usesManagedIdentity = false; + _logger.LogInformation( + "HostedDiagnosticsBlobService initialized with connection string (container: {Container})", + ContainerName); + } + else + { + throw new InvalidOperationException( + "HostedDiagnosticsBlobService not configured. Set either 'AzureStorageAccountName' (Managed Identity) or 'AzureBlobStorageConnectionString'."); + } + } + + /// + /// Test seam: construct from a (possibly Moq'd) . + /// Public so the Moq dynamic proxy assembly can call it without InternalsVisibleTo + /// gymnastics. + /// + public HostedDiagnosticsBlobService( + BlobServiceClient blobServiceClient, + ILogger logger, + bool usesManagedIdentity = false) + { + _blobServiceClient = blobServiceClient ?? throw new ArgumentNullException(nameof(blobServiceClient)); + _logger = logger; + _usesManagedIdentity = usesManagedIdentity; + } + + // -------- Public API -------- + + /// + /// Mints a blob-scoped, Write+Create-only SAS URI pinned to the EXACT path + /// {tenantId}/{filename} for an agent's diagnostics-package upload. Cannot + /// be redirected to other tenants' prefixes. TTL is capped at + /// to keep the exposure window tight. + /// + public virtual async Task GenerateUploadSasAsync( + string tenantId, string filename, TimeSpan? ttl = null, CancellationToken cancellationToken = default) + { + SecurityValidator.EnsureValidGuid(tenantId, nameof(tenantId)); + ValidateFilename(filename); + + var effectiveTtl = ttl ?? TimeSpan.FromMinutes(15); + if (effectiveTtl > MaxUploadSasTtl) + effectiveTtl = MaxUploadSasTtl; + if (effectiveTtl < TimeSpan.FromMinutes(1)) + effectiveTtl = TimeSpan.FromMinutes(1); + + var blobPath = BuildBlobPath(tenantId, filename); + var expiresOn = DateTimeOffset.UtcNow.Add(effectiveTtl); + + await EnsureContainerAsync(cancellationToken); + + var sasUri = await BuildUploadSasUriAsync(blobPath, expiresOn, cancellationToken); + + return new HostedUploadSasResult + { + UploadUrl = sasUri.ToString(), + BlobPath = blobPath, + ExpiresAt = expiresOn.UtcDateTime, + }; + } + + /// + /// Opens a read stream over a hosted-diagnostics blob. Caller is responsible for + /// disposing the returned response (it owns the underlying network stream). + /// 404 propagates as . + /// + public virtual async Task> OpenReadAsync( + string blobPath, CancellationToken cancellationToken = default) + { + ValidateBlobPath(blobPath); + var blobClient = GetContainerClient().GetBlobClient(blobPath); + return await blobClient.DownloadStreamingAsync(cancellationToken: cancellationToken); + } + + /// + /// Returns the size + content-type of a hosted-diagnostics blob without streaming the + /// payload. Used by the download path's pre-size check. + /// + public virtual async Task GetPropertiesAsync( + string blobPath, CancellationToken cancellationToken = default) + { + ValidateBlobPath(blobPath); + var blobClient = GetContainerClient().GetBlobClient(blobPath); + var response = await blobClient.GetPropertiesAsync(cancellationToken: cancellationToken); + return response.Value; + } + + /// + /// Idempotent delete used by the cascade-delete worker. A 404 is a successful no-op + /// (replay-safe). Other storage errors propagate so the cascade poison path catches them. + /// + public virtual async Task DeleteIfExistsAsync( + string blobPath, CancellationToken cancellationToken = default) + { + ValidateBlobPath(blobPath); + var blobClient = GetContainerClient().GetBlobClient(blobPath); + var response = await blobClient.DeleteIfExistsAsync(cancellationToken: cancellationToken); + return response.Value; + } + + /// + /// Enumerates blobs under a single tenant's prefix ({tenantId}/). + /// Used by ad-hoc operator scripts and (later) any orphan-scan job. + /// + public virtual async IAsyncEnumerable EnumerateByTenantAsync( + string tenantId, + [EnumeratorCancellation] CancellationToken cancellationToken = default) + { + SecurityValidator.EnsureValidGuid(tenantId, nameof(tenantId)); + var containerClient = GetContainerClient(); + if (!await containerClient.ExistsAsync(cancellationToken)) + yield break; + + // Trailing slash bounds the match so tenant "abc" doesn't also pull "abcde/...". + var prefix = tenantId + "/"; + await foreach (var item in containerClient.GetBlobsAsync( + BlobTraits.None, BlobStates.None, prefix: prefix, cancellationToken: cancellationToken)) + { + yield return item; + } + } + + // -------- Path helpers -------- + + /// + /// Builds the canonical blob path for a tenant + agent-supplied filename. + /// Always {tenantId}/{filename}; tenants cannot escape their prefix because + /// the filename is validated to reject path separators. + /// + internal static string BuildBlobPath(string tenantId, string filename) + => $"{tenantId}/{filename}"; + + // -------- Internals -------- + + /// + /// Test seam: returns the cached/built container client. Override in xUnit to inject + /// a Moq'd + verify interactions. + /// + protected virtual BlobContainerClient GetContainerClient() + => _blobServiceClient.GetBlobContainerClient(ContainerName); + + /// + /// Test seam: returns the configured authentication mode. Production reads from + /// the ctor flag; tests subclass to force a specific path through the SAS builder. + /// + protected virtual bool UsesManagedIdentity => _usesManagedIdentity; + + /// + /// Lazy create-if-not-exists, guarded by an interlocked flag so we don't pay for + /// the HEAD on every upload-url request after the first. The Interlocked gate + /// lives here (NOT in the override-able seam) so subclasses can't accidentally + /// defeat the single-shot semantic. The storage call itself goes through + /// for test isolation. + /// + protected async Task EnsureContainerAsync(CancellationToken cancellationToken) + { + if (Interlocked.CompareExchange(ref _containerEnsured, 0, 0) == 1) + return; + await CreateContainerIfNotExistsCoreAsync(cancellationToken); + Interlocked.Exchange(ref _containerEnsured, 1); + } + + /// + /// Test seam for the actual CreateIfNotExistsAsync call. Production hits + /// the Azure SDK; tests subclass to count invocations + assert the single-shot + /// guard works without spinning up Azurite. + /// + protected virtual async Task CreateContainerIfNotExistsCoreAsync(CancellationToken cancellationToken) + { + await GetContainerClient().CreateIfNotExistsAsync(cancellationToken: cancellationToken); + } + + /// + /// Builds the blob-scoped Write+Create-only SAS URI for upload. Branch on the + /// auth mode: MI uses User Delegation SAS (no account key needed); connection + /// string uses the underlying StorageSharedKeyCredential via + /// . + /// + protected virtual async Task BuildUploadSasUriAsync( + string blobPath, DateTimeOffset expiresOn, CancellationToken cancellationToken) + { + var blobClient = GetContainerClient().GetBlobClient(blobPath); + + var sasBuilder = new BlobSasBuilder + { + BlobContainerName = ContainerName, + BlobName = blobPath, + Resource = "b", // blob-scoped, NOT container-scoped + StartsOn = DateTimeOffset.UtcNow.AddMinutes(-5), // clock-skew tolerance + ExpiresOn = expiresOn, + }; + // Write + Create only — agent must not be able to Read other tenants' blobs, + // Delete anything, or List the container. + sasBuilder.SetPermissions(BlobSasPermissions.Write | BlobSasPermissions.Create); + + if (UsesManagedIdentity) + { + var delegationKey = await _blobServiceClient.GetUserDelegationKeyAsync( + DateTimeOffset.UtcNow.AddMinutes(-5), expiresOn, cancellationToken); + + var sasUri = new BlobUriBuilder(blobClient.Uri) + { + Sas = sasBuilder.ToSasQueryParameters(delegationKey, _blobServiceClient.AccountName), + }; + return sasUri.ToUri(); + } + else + { + if (!blobClient.CanGenerateSasUri) + { + throw new InvalidOperationException( + "BlobClient cannot generate a SAS URI — connection string is missing the account key."); + } + return blobClient.GenerateSasUri(sasBuilder); + } + } + + // -------- Validation -------- + + /// + /// Validates an agent-supplied filename: no path separators (cannot escape the + /// {tenantId}/ prefix), no .. traversal, no null bytes, reasonable + /// length. Throws on rejection. + /// + internal static void ValidateFilename(string filename) + { + if (string.IsNullOrEmpty(filename)) + throw new ArgumentException("filename is required", nameof(filename)); + if (filename.Length > 256) + throw new ArgumentException("filename exceeds 256 chars", nameof(filename)); + if (filename.Contains('/') || filename.Contains('\\')) + throw new ArgumentException("filename must not contain path separators", nameof(filename)); + if (filename.Contains("..")) + throw new ArgumentException("filename must not contain '..'", nameof(filename)); + if (filename.Contains('\0')) + throw new ArgumentException("filename must not contain null bytes", nameof(filename)); + } + + /// + /// Validates a stored blob path ({tenantId}/{filename}) on the read paths. + /// Defends the download proxy against path-traversal even if a row was tampered with. + /// + internal static void ValidateBlobPath(string blobPath) + { + if (string.IsNullOrEmpty(blobPath)) + throw new ArgumentException("blobPath is required", nameof(blobPath)); + if (blobPath.Contains("..") || blobPath.Contains('\0')) + throw new ArgumentException("blobPath must not contain '..' or null bytes", nameof(blobPath)); + // Expected shape: exactly one '/' separator between tenantId and filename. + var slashIndex = blobPath.IndexOf('/'); + if (slashIndex <= 0 || slashIndex == blobPath.Length - 1) + throw new ArgumentException("blobPath must be of the form '{tenantId}/{filename}'", nameof(blobPath)); + // Defensive: reject backslash so callers can't smuggle Windows-style paths past + // platforms that normalize them differently. + if (blobPath.Contains('\\')) + throw new ArgumentException("blobPath must not contain backslashes", nameof(blobPath)); + } + } + + /// + /// Return value of . + /// + public sealed class HostedUploadSasResult + { + /// Full https URL with SAS query string. Agent PUTs the ZIP payload here. + public string UploadUrl { get; set; } = string.Empty; + + /// + /// Canonical blob path inside the hosted container ({tenantId}/{filename}). + /// Caller (the upload-url Function) returns this to the agent so the agent can + /// persist it as DiagnosticsBlobName via the ingest path. + /// + public string BlobPath { get; set; } = string.Empty; + + /// UTC expiry of the SAS — surfaced back to the agent for telemetry only. + public DateTime ExpiresAt { get; set; } + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Services/Diagnostics/SasPermissionParser.cs b/src/Backend/AutopilotMonitor.Functions/Services/Diagnostics/SasPermissionParser.cs new file mode 100644 index 00000000..84ff28a5 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions/Services/Diagnostics/SasPermissionParser.cs @@ -0,0 +1,61 @@ +using System; +using System.Web; + +namespace AutopilotMonitor.Functions.Services.Diagnostics +{ + /// + /// Parses Azure Storage SAS permission strings (the sp= query parameter) + /// and answers permission-presence questions. Used by the cascade-delete worker to + /// decide whether it can clean up a customer's diagnostics blob — if the customer's + /// SAS lacks the Delete (d) permission we log + skip; the customer remains + /// responsible for their own lifecycle rules. + /// + /// Pure static helper, no dependencies. Tolerant of malformed input — every + /// parse-or-extract failure returns false so the caller's "skip-delete" + /// branch is taken rather than throwing in the middle of a cascade. + /// + /// + public static class SasPermissionParser + { + /// + /// Returns true when the SAS URL's sp= permission string contains the + /// Delete flag (d). Returns false on null/empty input, missing query + /// string, missing sp parameter, or any parse error. + /// + public static bool HasDelete(string? sasUrl) + => HasPermission(sasUrl, 'd'); + + /// + /// Generic permission probe. Accepts a single character (one of + /// r w c d l a u etc.) and answers case-insensitively. Exposed so + /// future callers can ask about other permissions without re-implementing + /// the parse. + /// + public static bool HasPermission(string? sasUrl, char permissionFlag) + { + if (string.IsNullOrEmpty(sasUrl)) return false; + try + { + var queryIndex = sasUrl!.IndexOf('?'); + if (queryIndex < 0) return false; + + var query = HttpUtility.ParseQueryString(sasUrl.Substring(queryIndex)); + var sp = query["sp"]; + if (string.IsNullOrEmpty(sp)) return false; + + // SAS permissions are a flat character set: e.g. "rwdlac" for full, + // "rwc" for write-only-with-create. Case-insensitive per Azure spec. + var needle = char.ToLowerInvariant(permissionFlag); + foreach (var c in sp) + { + if (char.ToLowerInvariant(c) == needle) return true; + } + return false; + } + catch + { + return false; + } + } + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Services/EventIngestProcessor.cs b/src/Backend/AutopilotMonitor.Functions/Services/EventIngestProcessor.cs index ad61dd50..f4497e6a 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/EventIngestProcessor.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/EventIngestProcessor.cs @@ -278,13 +278,20 @@ namespace AutopilotMonitor.Functions.Services if (classification.DiagnosticsUploadedEvent != null) { - var blobName = classification.DiagnosticsUploadedEvent.Data?.ContainsKey("blobName") == true - ? classification.DiagnosticsUploadedEvent.Data["blobName"]?.ToString() + var data = classification.DiagnosticsUploadedEvent.Data; + var blobName = data?.ContainsKey("blobName") == true + ? data["blobName"]?.ToString() + : null; + // Older agents don't send `destination` — pass null, repo leaves the + // column unchanged (legacy-row default at read-time is CustomerSas). + var destination = data?.ContainsKey("destination") == true + ? data["destination"]?.ToString() : null; if (!string.IsNullOrEmpty(blobName)) { await _sessionRepo.UpdateSessionDiagnosticsBlobAsync( - request.TenantId, request.SessionId, blobName); + request.TenantId, request.SessionId, blobName, + string.IsNullOrEmpty(destination) ? null : destination); } } diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs index 5b4a2801..dc738d34 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs @@ -119,6 +119,10 @@ namespace AutopilotMonitor.Functions.Services if (!string.IsNullOrEmpty(diagnosticsBlobName)) indexEntity["DiagnosticsBlobName"] = diagnosticsBlobName; + var diagnosticsBlobDestination = sessionEntity.GetString("DiagnosticsBlobDestination"); + if (!string.IsNullOrEmpty(diagnosticsBlobDestination)) + indexEntity["DiagnosticsBlobDestination"] = diagnosticsBlobDestination; + var lastEventAt = sessionEntity.GetDateTimeOffset("LastEventAt")?.UtcDateTime; if (lastEventAt.HasValue) indexEntity["LastEventAt"] = EnsureUtc(lastEventAt.Value); @@ -210,6 +214,7 @@ namespace AutopilotMonitor.Functions.Services DurationSeconds = ComputeEffectiveDuration(entity, status, startedAt, completedAt), EnrollmentType = entity.GetString("EnrollmentType") ?? "v1", DiagnosticsBlobName = entity.GetString("DiagnosticsBlobName"), + DiagnosticsBlobDestination = entity.GetString("DiagnosticsBlobDestination"), LastEventAt = SafeGetDateTime(entity, "LastEventAt"), IsPreProvisioned = entity.GetBoolean("IsPreProvisioned") ?? false, IsHybridJoin = entity.GetBoolean("IsHybridJoin") ?? false, @@ -294,6 +299,7 @@ namespace AutopilotMonitor.Functions.Services DateTime? lastEventAt = null; int? durationSeconds = null; string? diagnosticsBlobName = null; + string? diagnosticsBlobDestination = null; DateTime? resumedAt = null; DateTime? stalledAt = null; string geoCountry = string.Empty; @@ -330,6 +336,7 @@ namespace AutopilotMonitor.Functions.Services lastEventAt = existingEntity.GetDateTimeOffset("LastEventAt")?.UtcDateTime; durationSeconds = existingEntity.GetInt32("DurationSeconds"); diagnosticsBlobName = existingEntity.GetString("DiagnosticsBlobName"); + diagnosticsBlobDestination = existingEntity.GetString("DiagnosticsBlobDestination"); resumedAt = existingEntity.GetDateTimeOffset("ResumedAt")?.UtcDateTime; stalledAt = existingEntity.GetDateTimeOffset("StalledAt")?.UtcDateTime; geoCountry = existingEntity.GetString("GeoCountry") ?? string.Empty; @@ -411,6 +418,9 @@ namespace AutopilotMonitor.Functions.Services if (!string.IsNullOrWhiteSpace(diagnosticsBlobName)) entity["DiagnosticsBlobName"] = diagnosticsBlobName; + if (!string.IsNullOrWhiteSpace(diagnosticsBlobDestination)) + entity["DiagnosticsBlobDestination"] = diagnosticsBlobDestination; + if (resumedAt.HasValue) entity["ResumedAt"] = EnsureUtc(resumedAt.Value); @@ -1843,9 +1853,15 @@ namespace AutopilotMonitor.Functions.Services } /// - /// Stores the diagnostics blob name on an existing session (Merge-mode, single field update). + /// Stores the diagnostics blob name + destination on an existing session + /// (Merge-mode, two-field update written together so the download path always + /// sees a consistent (name, destination) pair). Destination encodes whether the + /// blob lives in the customer's SAS-backed container or in the backend's hosted + /// container — required so that a later tenant switch from one destination to + /// the other doesn't break downloads for already-uploaded sessions. /// - public async Task UpdateSessionDiagnosticsBlobAsync(string tenantId, string sessionId, string blobName) + public async Task UpdateSessionDiagnosticsBlobAsync( + string tenantId, string sessionId, string blobName, string? destination = null) { SecurityValidator.EnsureValidGuid(tenantId, nameof(tenantId)); SecurityValidator.EnsureValidGuid(sessionId, nameof(sessionId)); @@ -1860,13 +1876,20 @@ namespace AutopilotMonitor.Functions.Services ["DiagnosticsBlobName"] = blobName }; + if (!string.IsNullOrWhiteSpace(destination)) + { + update["DiagnosticsBlobDestination"] = destination; + } + await tableClient.UpdateEntityAsync(update, entity.Value.ETag, Azure.Data.Tables.TableUpdateMode.Merge); // Dual-write: merge into SessionsIndex var indexRowKey = entity.Value.GetString("IndexRowKey"); await MergeSessionIndexAsync(tenantId, indexRowKey, update); - _logger.LogInformation($"Stored diagnostics blob name for session {sessionId}: {blobName}"); + _logger.LogInformation( + "Stored diagnostics blob name for session {SessionId}: {BlobName} (destination={Destination})", + sessionId, blobName, destination ?? "(unchanged)"); } catch (Exception ex) { @@ -2094,6 +2117,9 @@ namespace AutopilotMonitor.Functions.Services DurationSeconds = ComputeEffectiveDuration(entity, status, startedAt, completedAt), EnrollmentType = entity.GetString("EnrollmentType") ?? "v1", DiagnosticsBlobName = entity.GetString("DiagnosticsBlobName"), + // Legacy rows that predate this field surface as null → download path treats + // null as "CustomerSas" for back-compat. + DiagnosticsBlobDestination = entity.GetString("DiagnosticsBlobDestination"), LastEventAt = SafeGetDateTime(entity, "LastEventAt"), IsPreProvisioned = entity.GetBoolean("IsPreProvisioned") ?? false, IsHybridJoin = entity.GetBoolean("IsHybridJoin") ?? false, diff --git a/src/Shared/AutopilotMonitor.Shared/Constants.cs b/src/Shared/AutopilotMonitor.Shared/Constants.cs index 571c6115..97e52723 100644 --- a/src/Shared/AutopilotMonitor.Shared/Constants.cs +++ b/src/Shared/AutopilotMonitor.Shared/Constants.cs @@ -626,6 +626,18 @@ namespace AutopilotMonitor.Shared /// defense-in-depth second-pass. /// public const string OffboardingState = "offboarding-state"; + + /// + /// Hosted diagnostics destination: tenant agents may upload their diagnostics + /// package directly into the backend's own storage account when the tenant admin + /// opts in via + /// = "Hosted". Blobs are tenant-scoped via a {tenantId}/ prefix + /// ({tenantId}/AgentDiagnostics-{sessionId}-{ts}.zip) so per-tenant + /// enumeration + retention deletion can iterate exactly one tenant without + /// cross-tenant exposure. The opposite destination, "CustomerSas", leaves + /// the blob in the customer's own storage and never touches this container. + /// + public const string HostedDiagnostics = "diagnostics"; } /// diff --git a/src/Shared/AutopilotMonitor.Shared/DataAccess/ISessionRepository.cs b/src/Shared/AutopilotMonitor.Shared/DataAccess/ISessionRepository.cs index bc9a1ae1..69b127f7 100644 --- a/src/Shared/AutopilotMonitor.Shared/DataAccess/ISessionRepository.cs +++ b/src/Shared/AutopilotMonitor.Shared/DataAccess/ISessionRepository.cs @@ -76,7 +76,8 @@ namespace AutopilotMonitor.Shared.DataAccess DateTime? earliestEventTimestamp = null, DateTime? latestEventTimestamp = null, EnrollmentPhase? currentPhase = null, int platformScriptIncrement = 0, int remediationScriptIncrement = 0); - Task UpdateSessionDiagnosticsBlobAsync(string tenantId, string sessionId, string blobName); + Task UpdateSessionDiagnosticsBlobAsync( + string tenantId, string sessionId, string blobName, string? destination = null); Task SetSessionPreProvisionedAsync(string tenantId, string sessionId, bool isPreProvisioned, SessionStatus? status = null, bool? isUserDriven = null); Task UpdateSessionGeoAsync(string tenantId, string sessionId, diff --git a/src/Shared/AutopilotMonitor.Shared/Models/Config/TenantConfiguration.cs b/src/Shared/AutopilotMonitor.Shared/Models/Config/TenantConfiguration.cs index ba021b41..166f8b52 100644 --- a/src/Shared/AutopilotMonitor.Shared/Models/Config/TenantConfiguration.cs +++ b/src/Shared/AutopilotMonitor.Shared/Models/Config/TenantConfiguration.cs @@ -401,17 +401,35 @@ namespace AutopilotMonitor.Shared.Models /// /// Azure Blob Storage Container SAS URL for diagnostics package upload. + /// Used only when is "CustomerSas". /// Each tenant provides their own container — data stays in the customer's storage. - /// If null or empty, diagnostics upload is disabled. + /// If null or empty (and destination is CustomerSas), diagnostics upload is disabled. /// public string DiagnosticsBlobSasUrl { get; set; } = default!; /// /// When to upload diagnostics packages: "Off", "Always", "OnFailure". - /// Default: "Off" + /// Applies to both destinations. Default: "Off" /// public string DiagnosticsUploadMode { get; set; } = "Off"; + /// + /// Where diagnostics packages should be uploaded: + /// + /// "CustomerSas" (default) — customer's own storage account via the SAS URL + /// in . Preserves existing behaviour; no data + /// leaves the customer's Azure tenant boundary. + /// "Hosted" — opt-in only. Blobs land in the backend's storage account + /// under {tenantId}/AgentDiagnostics-...zip in the + /// container. Requires + /// an explicit admin click in the tenant settings UI with a clearly-marked + /// "data leaves your tenant" disclosure — never set silently. + /// + /// Default "CustomerSas" so existing tenants without the field set behave + /// identically to today and customer data is never silently routed to hosted storage. + /// + public string DiagnosticsUploadDestination { get; set; } = "CustomerSas"; + // ===== TRACE EVENTS ===== /// diff --git a/src/Shared/AutopilotMonitor.Shared/Models/Deletion/DeletionManifest.cs b/src/Shared/AutopilotMonitor.Shared/Models/Deletion/DeletionManifest.cs index fe2454c6..10b2cbd5 100644 --- a/src/Shared/AutopilotMonitor.Shared/Models/Deletion/DeletionManifest.cs +++ b/src/Shared/AutopilotMonitor.Shared/Models/Deletion/DeletionManifest.cs @@ -38,11 +38,24 @@ namespace AutopilotMonitor.Shared.Models.Deletion public Dictionary PreflightCounts { get; set; } = new Dictionary(); /// - /// Customer-storage diagnostics blob name (informational only — cascade does NOT delete - /// the blob; customer owns the storage account). See plan §3 + §11. + /// Diagnostics blob name (e.g. AgentDiagnostics-...zip for CustomerSas, or + /// {tenantId}/AgentDiagnostics-...zip for Hosted). The cascade DOES delete + /// the blob when it's hosted in the backend's own storage; for CustomerSas it + /// only attempts deletion when the customer's SAS includes the d + /// permission (otherwise the customer's lifecycle rules remain the source of + /// truth). See plan §5b. /// public string? DiagnosticsBlobName { get; set; } + /// + /// Where the diagnostics blob lives: "CustomerSas", "Hosted", or + /// null on legacy sessions (treated as "CustomerSas" by the cascade for + /// back-compat). Captured at preflight from the Sessions row so the cascade + /// can route deletion to the right store even after a tenant has switched + /// destinations for new uploads. + /// + public string? DiagnosticsBlobDestination { get; set; } + /// Ordered cascade steps. The worker executes them in order. public List Steps { get; set; } = new List(); @@ -183,6 +196,15 @@ namespace AutopilotMonitor.Shared.Models.Deletion { public const string SoftwareInventoryDecrement = "SoftwareInventoryDecrement"; public const string Tombstone = "Tombstone"; + + /// + /// Cascade-delete of the per-session diagnostics blob. Skips when + /// is null/empty. For + /// Hosted destination the cascade always deletes (we own the storage); + /// for CustomerSas it deletes only when the SAS includes the Delete + /// permission. 404 from the storage layer is an idempotent no-op. + /// + public const string DiagnosticsBlob = "DiagnosticsBlob"; } /// diff --git a/src/Shared/AutopilotMonitor.Shared/Models/Deletion/DeletionProgress.cs b/src/Shared/AutopilotMonitor.Shared/Models/Deletion/DeletionProgress.cs index 8d2087f1..a95b9266 100644 --- a/src/Shared/AutopilotMonitor.Shared/Models/Deletion/DeletionProgress.cs +++ b/src/Shared/AutopilotMonitor.Shared/Models/Deletion/DeletionProgress.cs @@ -23,6 +23,18 @@ namespace AutopilotMonitor.Shared.Models.Deletion /// UTC timestamp once the FINAL tombstone step has completed; null while in flight. public DateTime? CompletedAt { get; set; } + /// + /// Plan §5b: set once the cascade has executed (or intentionally skipped) the + /// diagnostics-blob delete step. Idempotency marker so a retry after a partial + /// success doesn't issue a second DELETE. The step runs after verification + + /// before tombstone; on completion this flag flips true regardless of whether + /// the cascade actually issued a delete (Hosted/CustomerSas-with-permission) or + /// skipped (CustomerSas-without-permission/no-blob/legacy). Default false on + /// pre-§5b progress blobs — they re-enter the step on resume and do the right + /// thing (delete-if-exists is 404-safe; skip is a no-op). + /// + public bool DiagnosticsBlobDeleteDone { get; set; } + // ============================================================ PR4c additions ==== // Three additive fields close the per-key idempotency + tombstone-gap correctness holes // discovered by Codex review of PR4 + PR4b. All three default to safe values diff --git a/src/Shared/AutopilotMonitor.Shared/Models/SessionApiModels.cs b/src/Shared/AutopilotMonitor.Shared/Models/SessionApiModels.cs index b2b89f9b..d8061cb5 100644 --- a/src/Shared/AutopilotMonitor.Shared/Models/SessionApiModels.cs +++ b/src/Shared/AutopilotMonitor.Shared/Models/SessionApiModels.cs @@ -163,6 +163,31 @@ namespace AutopilotMonitor.Shared.Models public string? UploadUrl { get; set; } public DateTime ExpiresAt { get; set; } public string? Message { get; set; } + + /// + /// Canonical blob path the agent MUST persist as + /// SessionSummary.DiagnosticsBlobName after a successful upload. Encodes + /// the destination-specific path layout so the download path can find the blob + /// later without having to reconstruct it: + /// + /// CustomerSas: equal to the request filename (e.g. + /// AgentDiagnostics-...zip). + /// Hosted: tenant-prefixed (e.g. + /// {tenantId}/AgentDiagnostics-...zip). + /// + /// Null for older backends that predate this field — the agent then falls back + /// to the request filename for back-compat. + /// + public string? BlobName { get; set; } + + /// + /// Which destination minted the SAS in : + /// "CustomerSas" or "Hosted". Surfaced so the agent's ingest path + /// can attach it to the next telemetry batch (the download path needs the + /// per-session destination, NOT the current tenant setting — those can diverge + /// if the admin switches modes after some sessions have already uploaded). + /// + public string? Destination { get; set; } } /// @@ -219,10 +244,35 @@ namespace AutopilotMonitor.Shared.Models /// /// Blob name of the uploaded diagnostics archive (null if not uploaded). - /// Used to construct a download URL via the tenant's Blob Storage SAS URL. + /// Used to construct a download URL. + /// + /// Path semantics depend on : + /// + /// CustomerSas: blob name only (e.g. AgentDiagnostics-...zip); + /// download builds the URL from the tenant's container SAS. + /// Hosted: full path including the {tenantId}/ prefix + /// (e.g. {tenantId}/AgentDiagnostics-...zip); download streams + /// directly via the backend connection string. + /// + /// /// public string DiagnosticsBlobName { get; set; } = default!; + /// + /// Where the diagnostics archive for THIS session was uploaded. + /// Frozen at upload time so the download path can route correctly even if the + /// tenant later switches . + /// + /// "CustomerSas" — blob lives in the customer's storage; download uses + /// the tenant's SAS URL (current behaviour). + /// "Hosted" — blob lives in the backend's storage under + /// {tenantId}/; download streams via the Functions connection string. + /// null (legacy rows that predate this field) — treated as + /// "CustomerSas" by the download path for back-compat. + /// + /// + public string? DiagnosticsBlobDestination { get; set; } + /// /// Timestamp of the most recently received event for this session. /// Updated on every event batch ingestion. Used by maintenance to detect diff --git a/src/Web/autopilot-monitor-web/app/docs/sections/SectionSettings.tsx b/src/Web/autopilot-monitor-web/app/docs/sections/SectionSettings.tsx index a75e2a35..d31268e1 100644 --- a/src/Web/autopilot-monitor-web/app/docs/sections/SectionSettings.tsx +++ b/src/Web/autopilot-monitor-web/app/docs/sections/SectionSettings.tsx @@ -270,8 +270,13 @@ export function SectionSettings() { {/* Diagnostics Package */} + + Where diagnostics packages are uploaded:{" "} + Your own Azure Blob Storage (default) — agents upload to a container in your tenant via a Container SAS URL you provide; data never leaves your Azure tenant.{" "} + Hosted storage (opt-in) — agents upload to the AutopilotMonitor backend's Azure Storage under a per-tenant prefix, using short-lived (15 min), blob-scoped, write-only tokens minted per upload. No long-lived credentials are stored. Switching destinations requires an explicit admin click and a Save — never silent. + - An Azure Blob Storage Container SAS URL used for diagnostics package uploads. The SAS URL must grant + Used only when the destination is Your own Azure Blob Storage. An Azure Blob Storage Container SAS URL used for diagnostics package uploads. The SAS URL must grant at minimum Read, Write, and Create permissions at the container level. The SAS URL is stored securely in the backend and never sent to devices — the agent requests a short-lived upload URL from the backend just before uploading. The portal shows an expiry @@ -282,7 +287,7 @@ export function SectionSettings() { Off — never upload,{" "} Always — upload after every session,{" "} On Failure Only — upload only when the session ends in failure (recommended if storage costs - are a concern). Only available when a Blob Storage URL is configured. + are a concern). For the customer-SAS destination, a SAS URL must be configured before Upload Mode can be enabled. diff --git a/src/Web/autopilot-monitor-web/app/settings/TenantConfigContext.tsx b/src/Web/autopilot-monitor-web/app/settings/TenantConfigContext.tsx index b9fa8ae1..6741acda 100644 --- a/src/Web/autopilot-monitor-web/app/settings/TenantConfigContext.tsx +++ b/src/Web/autopilot-monitor-web/app/settings/TenantConfigContext.tsx @@ -158,6 +158,8 @@ interface TenantConfigContextValue { setDiagnosticsBlobSasUrl: (v: string) => void; diagnosticsUploadMode: string; setDiagnosticsUploadMode: (v: string) => void; + diagnosticsUploadDestination: string; + setDiagnosticsUploadDestination: (v: string) => void; tenantDiagPaths: DiagnosticsLogPath[]; setTenantDiagPaths: (v: DiagnosticsLogPath[]) => void; globalDiagPaths: DiagnosticsLogPath[]; @@ -321,6 +323,9 @@ export function TenantConfigProvider({ children }: { children: React.ReactNode } // Diagnostics const [diagnosticsBlobSasUrl, setDiagnosticsBlobSasUrl] = useState(""); const [diagnosticsUploadMode, setDiagnosticsUploadMode] = useState("Off"); + // Destination defaults to "CustomerSas" so legacy rows behave exactly as before; + // Hosted requires an explicit admin click (no silent flip). + const [diagnosticsUploadDestination, setDiagnosticsUploadDestination] = useState("CustomerSas"); const [tenantDiagPaths, setTenantDiagPaths] = useState([]); const [globalDiagPaths, setGlobalDiagPaths] = useState([]); const [newDiagPath, setNewDiagPath] = useState(""); @@ -429,6 +434,7 @@ export function TenantConfigProvider({ children }: { children: React.ReactNode } const sasUrl = data.diagnosticsBlobSasUrl ?? ""; setDiagnosticsBlobSasUrl(sasUrl); setDiagnosticsUploadMode(data.diagnosticsUploadMode ?? "Off"); + setDiagnosticsUploadDestination(data.diagnosticsUploadDestination ?? "CustomerSas"); try { setTenantDiagPaths(data.diagnosticsLogPathsJson ? JSON.parse(data.diagnosticsLogPathsJson) : []); } catch { @@ -626,6 +632,7 @@ export function TenantConfigProvider({ children }: { children: React.ReactNode } slaConsecutiveFailureThreshold, diagnosticsBlobSasUrl: diagnosticsBlobSasUrl || undefined, diagnosticsUploadMode, + diagnosticsUploadDestination, diagnosticsLogPathsJson: tenantDiagPaths.length > 0 ? JSON.stringify(tenantDiagPaths) : "", enableLocalAdminAnalyzer, localAdminAllowedAccountsJson: localAdminAllowedAccounts.length > 0 ? JSON.stringify(localAdminAllowedAccounts) : "", @@ -677,7 +684,7 @@ export function TenantConfigProvider({ children }: { children: React.ReactNode } slaTargetSuccessRate, slaTargetMaxDurationMinutes, slaTargetAppInstallSuccessRate, slaNotifyOnSuccessRateBreach, slaSuccessRateNotifyThreshold, slaNotifyOnDurationBreach, slaNotifyOnAppInstallBreach, slaNotifyOnConsecutiveFailures, slaConsecutiveFailureThreshold, - diagnosticsBlobSasUrl, diagnosticsUploadMode, tenantDiagPaths, + diagnosticsBlobSasUrl, diagnosticsUploadMode, diagnosticsUploadDestination, tenantDiagPaths, enableLocalAdminAnalyzer, localAdminAllowedAccounts, enableSoftwareInventoryAnalyzer, enableIntegrityBypassAnalyzer, unrestrictedMode, ]); @@ -928,6 +935,7 @@ export function TenantConfigProvider({ children }: { children: React.ReactNode } if (!config) return; setDiagnosticsBlobSasUrl(config.diagnosticsBlobSasUrl ?? ""); setDiagnosticsUploadMode(config.diagnosticsUploadMode ?? "Off"); + setDiagnosticsUploadDestination(config.diagnosticsUploadDestination ?? "CustomerSas"); try { setTenantDiagPaths(config.diagnosticsLogPathsJson ? JSON.parse(config.diagnosticsLogPathsJson) : []); } catch { setTenantDiagPaths([]); } @@ -1293,6 +1301,7 @@ export function TenantConfigProvider({ children }: { children: React.ReactNode } // Diagnostics diagnosticsBlobSasUrl, setDiagnosticsBlobSasUrl, diagnosticsUploadMode, setDiagnosticsUploadMode, + diagnosticsUploadDestination, setDiagnosticsUploadDestination, tenantDiagPaths, setTenantDiagPaths, globalDiagPaths, newDiagPath, setNewDiagPath, diff --git a/src/Web/autopilot-monitor-web/app/settings/agent/sections/SectionDiagnostics.tsx b/src/Web/autopilot-monitor-web/app/settings/agent/sections/SectionDiagnostics.tsx index 196831fe..8fe7d2c0 100644 --- a/src/Web/autopilot-monitor-web/app/settings/agent/sections/SectionDiagnostics.tsx +++ b/src/Web/autopilot-monitor-web/app/settings/agent/sections/SectionDiagnostics.tsx @@ -8,6 +8,7 @@ export function SectionDiagnostics() { const { diagnosticsBlobSasUrl, setDiagnosticsBlobSasUrl, diagnosticsUploadMode, setDiagnosticsUploadMode, + diagnosticsUploadDestination, setDiagnosticsUploadDestination, tenantDiagPaths, setTenantDiagPaths, globalDiagPaths, newDiagPath, setNewDiagPath, @@ -25,6 +26,8 @@ export function SectionDiagnostics() { setDiagnosticsBlobSasUrl={setDiagnosticsBlobSasUrl} diagnosticsUploadMode={diagnosticsUploadMode} setDiagnosticsUploadMode={setDiagnosticsUploadMode} + diagnosticsUploadDestination={diagnosticsUploadDestination} + setDiagnosticsUploadDestination={setDiagnosticsUploadDestination} tenantDiagPaths={tenantDiagPaths} setTenantDiagPaths={setTenantDiagPaths} globalDiagPaths={globalDiagPaths} diff --git a/src/Web/autopilot-monitor-web/app/settings/components/DiagnosticsSection.tsx b/src/Web/autopilot-monitor-web/app/settings/components/DiagnosticsSection.tsx index c6eea805..8e097a53 100644 --- a/src/Web/autopilot-monitor-web/app/settings/components/DiagnosticsSection.tsx +++ b/src/Web/autopilot-monitor-web/app/settings/components/DiagnosticsSection.tsx @@ -5,27 +5,20 @@ import { DiagnosticsLogPath } from "../types"; import { validateDiagnosticsPath } from "@/utils/guardValidation"; import { ValidationIndicator } from "@/components/ValidationIndicator"; import SaveResetBar from "./SaveResetBar"; +import { parseSasExpiry } from "./diagnosticsSasExpiry"; -/** Parses the expiry date from the `se=` parameter of a SAS URL. Returns null if not found. */ -export function parseSasExpiry(sasUrl: string): Date | null { - try { - const qIndex = sasUrl.indexOf('?'); - if (qIndex < 0) return null; - const params = new URLSearchParams(sasUrl.substring(qIndex + 1)); - const se = params.get('se'); - if (!se) return null; - const d = new Date(se); - return isNaN(d.getTime()) ? null : d; - } catch { - return null; - } -} +// Re-export for callers that still import from the section. Implementation lives +// in `diagnosticsSasExpiry.ts` so vitest can test it without pulling in JSX. +export { parseSasExpiry }; interface DiagnosticsSectionProps { diagnosticsBlobSasUrl: string; setDiagnosticsBlobSasUrl: (value: string) => void; diagnosticsUploadMode: string; setDiagnosticsUploadMode: (value: string) => void; + /** "CustomerSas" (default — tenant's own SAS) or "Hosted" (opt-in — backend storage). */ + diagnosticsUploadDestination: string; + setDiagnosticsUploadDestination: (value: string) => void; tenantDiagPaths: DiagnosticsLogPath[]; setTenantDiagPaths: (value: DiagnosticsLogPath[]) => void; globalDiagPaths: DiagnosticsLogPath[]; @@ -44,6 +37,8 @@ export default function DiagnosticsSection({ setDiagnosticsBlobSasUrl, diagnosticsUploadMode, setDiagnosticsUploadMode, + diagnosticsUploadDestination, + setDiagnosticsUploadDestination, tenantDiagPaths, setTenantDiagPaths, globalDiagPaths, @@ -56,6 +51,7 @@ export default function DiagnosticsSection({ onReset, saving, }: DiagnosticsSectionProps) { + const isHosted = diagnosticsUploadDestination === "Hosted"; const [newDiagSubfolders, setNewDiagSubfolders] = useState(false); // Compute SAS expiry directly from the current URL value so feedback is instant @@ -82,69 +78,150 @@ export default function DiagnosticsSection({
- {/* Info */} -
-

- The agent requests an upload URL from the backend just before uploading. Your SAS URL is stored securely in the backend and never sent to devices in the agent configuration. -

-
- - {/* Blob Storage SAS URL */} -
-