Better errors from Download: Make GetFileSasUrl nullable (#3229)

This allows us to generate 404s when someone attempts to download from a non-existent container. At the moment we generate a 500 which isn't useful, or very good looks-wise.
microsoft · Jun 27, 2023 · 7c0dc7b · 7c0dc7b
1 parent 4b43753
commit 7c0dc7b
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 26 deletions.
diff --git a/src/ApiService/ApiService/Functions/Download.cs b/src/ApiService/ApiService/Functions/Download.cs
@@ -1,4 +1,5 @@
-using System.Web;
+using System.Net;
+using System.Web;
 using Azure.Storage.Sas;
 using Microsoft.Azure.Functions.Worker;
 using Microsoft.Azure.Functions.Worker.Http;
@@ -45,6 +46,16 @@ public async Async.Task<HttpResponseData> Run([HttpTrigger(AuthorizationLevel.An
             BlobSasPermissions.Read,
             TimeSpan.FromMinutes(5));
 
+        if (sasUri is null) {
+            // Note that we do not validate the existence of the file, only the container.
+            return await _context.RequestHandling.NotOk(req,
+                Error.Create(
+                    ErrorCode.INVALID_CONTAINER,
+                    "container not found"),
+                "generating download file SAS",
+                HttpStatusCode.NotFound);
+        }
+
         return RequestHandling.Redirect(req, sasUri);
     }
 }
diff --git a/src/ApiService/ApiService/onefuzzlib/Containers.cs b/src/ApiService/ApiService/onefuzzlib/Containers.cs
@@ -20,7 +20,7 @@ public interface IContainers {
 
     public Async.Task<BlobContainerClient?> FindContainer(Container container, StorageType storageType);
 
-    public Async.Task<Uri> GetFileSasUrl(Container container, string name, StorageType storageType, BlobSasPermissions permissions, TimeSpan? duration = null);
+    public Async.Task<Uri?> GetFileSasUrl(Container container, string name, StorageType storageType, BlobSasPermissions permissions, TimeSpan? duration = null);
     public Async.Task SaveBlob(Container container, string name, string data, StorageType storageType);
     public Async.Task<Guid> GetInstanceId();
 
@@ -145,8 +145,12 @@ private static readonly BlobContainerSasPermissions _containerCreatePermissions
         return null;
     }
 
-    public async Async.Task<Uri> GetFileSasUrl(Container container, string name, StorageType storageType, BlobSasPermissions permissions, TimeSpan? duration = null) {
-        var client = await FindContainer(container, storageType) ?? throw new Exception($"unable to find container: {container} - {storageType}");
+    public async Async.Task<Uri?> GetFileSasUrl(Container container, string name, StorageType storageType, BlobSasPermissions permissions, TimeSpan? duration = null) {
+        var client = await FindContainer(container, storageType);
+        if (client is null) {
+            return null;
+        }
+
         var blobClient = client.GetBlobClient(name);
         var timeWindow = SasTimeWindow(duration ?? TimeSpan.FromDays(30));
         return _storage.GenerateBlobSasUri(permissions, blobClient, timeWindow);

diff --git a/src/ApiService/ApiService/onefuzzlib/Events.cs b/src/ApiService/ApiService/onefuzzlib/Events.cs
@@ -114,9 +114,16 @@ public async Async.Task<OneFuzzResult<DownloadableEventMessage>> GetDownloadable
                 return eventMessageResult.ErrorV;
             }
 
-            var sasUrl = await _containers.GetFileSasUrl(WellKnownContainers.Events, eventId.ToString(), StorageType.Corpus, BlobSasPermissions.Read);
+            var sasUrl = await _containers.GetFileSasUrl(
+                WellKnownContainers.Events,
+                eventId.ToString(),
+                StorageType.Corpus,
+                BlobSasPermissions.Read);
+
             if (sasUrl == null) {
-                return OneFuzzResult<DownloadableEventMessage>.Error(ErrorCode.UNABLE_TO_FIND, $"Could not find container for event with id {eventId}");
+                return OneFuzzResult<DownloadableEventMessage>.Error(
+                    ErrorCode.UNABLE_TO_FIND,
+                    $"Could not find container for event with id {eventId}");
             }
 
             var eventMessage = eventMessageResult.OkV!;
@@ -133,8 +140,19 @@ public async Async.Task<OneFuzzResult<DownloadableEventMessage>> GetDownloadable
         }
 
         public async Task<DownloadableEventMessage> MakeDownloadable(EventMessage eventMessage) {
-            await _containers.SaveBlob(WellKnownContainers.Events, eventMessage.EventId.ToString(), JsonSerializer.Serialize(eventMessage, _options), StorageType.Corpus);
-            var sasUrl = await _containers.GetFileSasUrl(WellKnownContainers.Events, eventMessage.EventId.ToString(), StorageType.Corpus, BlobSasPermissions.Read);
+            await _containers.SaveBlob(
+                WellKnownContainers.Events,
+                eventMessage.EventId.ToString(),
+                JsonSerializer.Serialize(eventMessage, _options),
+                StorageType.Corpus);
+
+            var sasUrl = await _containers.GetFileSasUrl(
+                WellKnownContainers.Events,
+                eventMessage.EventId.ToString(),
+                StorageType.Corpus,
+                BlobSasPermissions.Read)
+                // events container should always exist
+                ?? throw new InvalidOperationException("Events container is missing");
 
             return new DownloadableEventMessage(
                 eventMessage.EventId,

diff --git a/src/ApiService/ApiService/onefuzzlib/Extension.cs b/src/ApiService/ApiService/onefuzzlib/Extension.cs
@@ -1,4 +1,5 @@
-using System.Text.Json;
+using System.IO;
+using System.Text.Json;
 using System.Text.Json.Serialization;
 using System.Threading.Tasks;
 using Azure.Core;
@@ -374,7 +375,13 @@ public async Async.Task<IList<VirtualMachineScaleSetExtensionData>> FuzzExtensio
         return extensions.Select(extension => extension.GetAsVirtualMachineScaleSetExtension()).ToList();
     }
 
-    public async Task<Dictionary<string, VirtualMachineExtensionData>> ReproExtensions(AzureLocation region, Os reproOs, Guid reproId, ReproConfig reproConfig, Container? setupContainer) {
+    public async Task<Dictionary<string, VirtualMachineExtensionData>> ReproExtensions(
+        AzureLocation region,
+        Os reproOs,
+        Guid reproId,
+        ReproConfig reproConfig,
+        Container? setupContainer) {
+
         // TODO: what about contents of repro.ps1 / repro.sh?
         var report = await _context.Reports.GetReport(reproConfig.Container, reproConfig.Path);
         var checkedReport = report.EnsureNotNull($"invalid report: {reproConfig}");
@@ -399,13 +406,13 @@ await _context.Containers.GetFileSasUrl(
                 reproConfig.Path,
                 StorageType.Corpus,
                 BlobSasPermissions.Read
-            ),
+            ) ?? throw new InvalidDataException($"failed to get repro config url: container '{reproConfig.Container}' missing"),
             await _context.Containers.GetFileSasUrl(
                 inputBlob.Container,
                 inputBlob.Name,
                 StorageType.Corpus,
                 BlobSasPermissions.Read
-            )
+            ) ?? throw new InvalidDataException($"failed to get input blob url: container '{inputBlob.Container}' missing"),
         };
 
         List<string> reproFiles;
@@ -437,21 +444,22 @@ await _context.Containers.SaveBlob(
         );
 
         foreach (var reproFile in reproFiles) {
-            urls.AddRange(new List<Uri>()
-            {
+            urls.Add(
                 await _context.Containers.GetFileSasUrl(
                     WellKnownContainers.ReproScripts,
                     reproFile,
                     StorageType.Config,
                     BlobSasPermissions.Read
-                ),
-                await _context.Containers.GetFileSasUrl(
+                ) // this container should always exist
+                ?? throw new InvalidOperationException("repro scripts container missing"));
+
+            urls.Add(await _context.Containers.GetFileSasUrl(
                     WellKnownContainers.TaskConfigs,
                     $"{reproId}/{scriptName}",
                     StorageType.Config,
                     BlobSasPermissions.Read
-                )
-            });
+                ) // this container should always exist
+                ?? throw new InvalidOperationException("task configs container missing"));
         }
 
         var baseExtension = await AgentConfig(region, reproOs, AgentMode.Repro, urls: urls, withSas: true);
@@ -472,13 +480,17 @@ public async Task<IList<VMExtensionWrapper>> ProxyManagerExtensions(Region regio
             WellKnownContainers.ProxyConfigs,
             $"{region}/{proxyId}/config.json",
             StorageType.Config,
-            BlobSasPermissions.Read);
+            BlobSasPermissions.Read)
+            // this should always exist
+            ?? throw new InvalidOperationException("proxy config container missing");
 
         var proxyManager = await _context.Containers.GetFileSasUrl(
             WellKnownContainers.Tools,
             $"linux/onefuzz-proxy-manager",
             StorageType.Config,
-            BlobSasPermissions.Read);
+            BlobSasPermissions.Read)
+            // this should always exist
+            ?? throw new InvalidOperationException("tools container missing");
 
         var baseExtension =
             await AgentConfig(region, Os.Linux, AgentMode.Proxy, new List<Uri> { config, proxyManager }, true);

diff --git a/src/ApiService/ApiService/onefuzzlib/NotificationOperations.cs b/src/ApiService/ApiService/onefuzzlib/NotificationOperations.cs
@@ -45,7 +45,19 @@ public async Async.Task NewFiles(Container container, string filename, bool isLa
         await foreach (var (task, containers) in GetQueueTasks()) {
             if (containers.Contains(container)) {
                 _logTracer.LogInformation("queuing input {Container} {Filename} {TaskId}", container, filename, task.TaskId);
-                var url = await _context.Containers.GetFileSasUrl(container, filename, StorageType.Corpus, BlobSasPermissions.Read | BlobSasPermissions.Delete);
+
+                var url = await _context.Containers.GetFileSasUrl(
+                    container,
+                    filename,
+                    StorageType.Corpus,
+                    BlobSasPermissions.Read | BlobSasPermissions.Delete);
+
+                if (url is null) {
+                    _logTracer.LogError("unable to generate sas url for missing container '{Container}'", container);
+                    // try again later
+                    throw new InvalidOperationException($"container '{container}' is missing");
+                }
+
                 await _context.Queue.SendMessage(task.TaskId.ToString(), url.ToString(), StorageType.Corpus);
             }
         }
@@ -93,9 +105,9 @@ public IAsyncEnumerable<Notification> GetNotifications(Container container) {
     public IAsyncEnumerable<(Task, IEnumerable<Container>)> GetQueueTasks() {
         // Nullability mismatch: We filter tuples where the containers are null
         return _context.TaskOperations.SearchStates(states: TaskStateHelper.AvailableStates)
-            .Select(task => (task, _context.TaskOperations.GetInputContainerQueues(task.Config)))
-            .Where(taskTuple => taskTuple.Item2.IsOk && taskTuple.Item2.OkV != null)
-            .Select(x => (Task: x.Item1, Containers: x.Item2.OkV))!;
+            .Select(task => (task, containers: _context.TaskOperations.GetInputContainerQueues(task.Config)))
+            .Where(x => x.containers.IsOk && x.containers.OkV is not null)
+            .Select(x => (Task: x.task, Containers: x.containers.OkV!));
     }
 
     public async Async.Task<OneFuzzResult<Notification>> Create(Container container, NotificationTemplate config, bool replaceExisting) {

diff --git a/src/ApiService/ApiService/onefuzzlib/ProxyOperations.cs b/src/ApiService/ApiService/onefuzzlib/ProxyOperations.cs
@@ -130,8 +130,16 @@ public bool IsOutdated(Proxy proxy) {
 
     public async Async.Task SaveProxyConfig(Proxy proxy) {
         var forwards = await GetForwards(proxy);
-        var url = (await _context.Containers.GetFileSasUrl(WellKnownContainers.ProxyConfigs, $"{proxy.Region}/{proxy.ProxyId}/config.json", StorageType.Config, BlobSasPermissions.Read)).EnsureNotNull("Can't generate file sas");
-        var queueSas = await _context.Queue.GetQueueSas("proxy", StorageType.Config, QueueSasPermissions.Add).EnsureNotNull("can't generate queue sas") ?? throw new Exception("Queue sas is null");
+        var url = (await _context.Containers.GetFileSasUrl(
+            WellKnownContainers.ProxyConfigs,
+            $"{proxy.Region}/{proxy.ProxyId}/config.json",
+            StorageType.Config,
+            BlobSasPermissions.Read)).EnsureNotNull("proxy configs container missing");
+
+        var queueSas = await _context.Queue.GetQueueSas(
+            "proxy",
+            StorageType.Config,
+            QueueSasPermissions.Add).EnsureNotNull("can't generate queue sas") ?? throw new Exception("Queue sas is null");
 
         var proxyConfig = new ProxyConfig(
             Url: url,

diff --git a/src/ApiService/IntegrationTests/DownloadTests.cs b/src/ApiService/IntegrationTests/DownloadTests.cs
@@ -55,6 +55,22 @@ public async Async.Task Download_WithoutFilename_IsRejected() {
         Assert.Equal(ErrorCode.INVALID_REQUEST.ToString(), err.Title);
     }
 
+    [Fact]
+    public async Async.Task Container_NotFound_Generates404() {
+        var req = TestHttpRequestData.Empty("GET");
+        // this container won't exist because we haven't explicitly created it
+        var url = new UriBuilder(req.Url) { Query = "container=xxx&filename=yyy" }.Uri;
+        req.SetUrl(url);
+
+        var func = new Download(Context);
+
+        var result = await func.Run(req);
+        Assert.Equal(HttpStatusCode.NotFound, result.StatusCode);
+
+        var err = BodyAs<ProblemDetails>(result);
+        Assert.Equal(ErrorCode.INVALID_CONTAINER.ToString(), err.Title);
+    }
+
     [Fact]
     public async Async.Task Download_RedirectsToResult_WithLocationHeader() {
         // set up a file to download