diff --git a/GitLfsCache.Tests/Batch/BatchRewriterTests.cs b/GitLfsCache.Tests/Batch/BatchRewriterTests.cs index b21c868..f62281c 100644 --- a/GitLfsCache.Tests/Batch/BatchRewriterTests.cs +++ b/GitLfsCache.Tests/Batch/BatchRewriterTests.cs @@ -2,6 +2,7 @@ namespace ktsu.GitLfsCache.Tests.Batch; +using System.Text.Json; using System.Text.Json.Nodes; using ktsu.Essentials.EncryptionProviders.Aes; using ktsu.GitLfsCache.Batch; @@ -208,6 +209,21 @@ public void Rewrite_ActionWithNoHref_IsLeftAlone() Assert.HasCount(0, FirstAction(rewritten, "download")); } + [TestMethod] + [DataRow("""{"objects":[{"oid":1,"size":1,"actions":{"download":{"href":"https://upstream.example/x"}}}]}""")] + [DataRow("""{"objects":[{"oid":"aa","size":"123","actions":{"download":{"href":"https://upstream.example/x"}}}]}""")] + [DataRow("""{"objects":[{"oid":"aa","size":1.5,"actions":{"download":{"href":"https://upstream.example/x"}}}]}""")] + [DataRow("""{"objects":[{"oid":"aa","size":1,"actions":{"download":{"href":7}}}]}""")] + [DataRow("""{"objects":[{"oid":"aa","size":1,"actions":{"download":{"href":"https://upstream.example/x","header":{"X-Count":1}}}}]}""")] + public void Rewrite_FieldOfTheWrongType_IsRefusedAsMalformed(string json) + { + // Neither rewritable nor safe to pass through with upstream's credentials still in it. + (BatchRewriter rewriter, _) = Create(); + JsonNode input = JsonNode.Parse(json)!; + + Assert.ThrowsExactly(() => rewriter.Rewrite(input, Context())); + } + [TestMethod] public void Rewrite_DoesNotMutateTheInputNode() { diff --git a/GitLfsCache.Tests/Integration/ProxyFlowTests.cs b/GitLfsCache.Tests/Integration/ProxyFlowTests.cs index 76c30d8..4e3426b 100644 --- a/GitLfsCache.Tests/Integration/ProxyFlowTests.cs +++ b/GitLfsCache.Tests/Integration/ProxyFlowTests.cs @@ -121,6 +121,26 @@ public async Task Batch_UpstreamRefusal_IsRelayedVerbatim() Assert.Contains("Repository not found", await response.Content.ReadAsStringAsync()); } + [TestMethod] + [DataRow("Sign in to continue", "text/html")] + [DataRow("""{"objects":[{"oid":"aa","size":"123","actions":{"download":{"href":"https://upstream.example/x"}}}]}""", "application/vnd.git-lfs+json")] + [DataRow("""{"objects":[{"oid":"aa","size":1,"actions":{"download":{"href":"https://upstream.example/x","header":{"X-Count":1}}}}]}""", "application/vnd.git-lfs+json")] + public async Task Batch_UpstreamSuccessThatCannotBeUsed_IsABadGateway(string upstreamBody, string mediaType) + { + // A sign-in or SSO page served with 200, or a field of the wrong type, is upstream's failure. + // Answering 500 would point operators at the proxy instead. + await using ProxyFixture fixture = await ProxyFixture.StartAsync(); + fixture.Upstream.BatchSuccessBody = (upstreamBody, mediaType); + (byte[] content, string oid) = Object("behind a sign-in page"); + using HttpClient client = fixture.Client; + + using StringContent body = BatchRequest("download", oid, content.Length); + + using HttpResponseMessage response = await client.PostAsync($"{LfsPath}/objects/batch", body); + + Assert.AreEqual(HttpStatusCode.BadGateway, response.StatusCode); + } + [TestMethod] public async Task Download_ColdThenWarm_FetchesUpstreamOnceAndServesFromTheStore() { diff --git a/GitLfsCache.Tests/Integration/StubUpstream.cs b/GitLfsCache.Tests/Integration/StubUpstream.cs index 4bcb8e6..d236087 100644 --- a/GitLfsCache.Tests/Integration/StubUpstream.cs +++ b/GitLfsCache.Tests/Integration/StubUpstream.cs @@ -38,6 +38,12 @@ public IReadOnlyList Requests /// Gets or sets the body the batch endpoint answers with when it is not successful. public string BatchFailureBody { get; set; } = """{"message":"Repository not found"}"""; + /// + /// Gets or sets a body the batch endpoint answers a successful call with verbatim, in place of the + /// one it would build, used to make upstream hand back a 2xx the proxy cannot use. + /// + public (string Body, string MediaType)? BatchSuccessBody { get; set; } + /// Gets or sets the status an object fetch answers with. public HttpStatusCode ObjectStatus { get; set; } = HttpStatusCode.OK; @@ -288,6 +294,14 @@ private HttpResponseMessage BuildBatchResponse(string? requestBody) }; } + if (BatchSuccessBody is (string body, string mediaType)) + { + return new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(body, Encoding.UTF8, mediaType), + }; + } + JsonNode parsed = JsonNode.Parse(requestBody ?? "{}")!; string operation = parsed["operation"]?.GetValue() ?? "download"; JsonArray objects = []; diff --git a/GitLfsCache/Batch/BatchRewriter.cs b/GitLfsCache/Batch/BatchRewriter.cs index 8599d7a..e9db7df 100644 --- a/GitLfsCache/Batch/BatchRewriter.cs +++ b/GitLfsCache/Batch/BatchRewriter.cs @@ -2,8 +2,10 @@ namespace ktsu.GitLfsCache.Batch; +using System.Text.Json; using System.Text.Json.Nodes; using ktsu.GitLfsCache.Configuration; +using ktsu.GitLfsCache.Locks; using ktsu.GitLfsCache.Tokens; using Microsoft.Extensions.Options; @@ -34,6 +36,11 @@ public sealed class BatchRewriter( /// The parsed upstream response. /// The request context. /// A new node tree with rewritten hrefs. + /// + /// An object's oid or size, or an action's href or header value, has the wrong + /// JSON type. Such an entry can be neither rewritten nor safely passed through with upstream's + /// credentials still in it, so the whole response is refused. + /// public JsonNode Rewrite(JsonNode upstreamResponse, BatchRewriteContext context) { Ensure.NotNull(upstreamResponse); @@ -78,14 +85,14 @@ private void RewriteObject( return; } - string? oid = batchObject["oid"]?.GetValue(); + string? oid = ReadString(batchObject["oid"], "oid"); if (string.IsNullOrEmpty(oid)) { return; } - long size = batchObject["size"]?.GetValue() ?? 0; + long size = ReadSize(batchObject["size"]); foreach (string actionName in RewrittenActions) { @@ -106,7 +113,7 @@ private void RewriteAction( DateTimeOffset expiresAt, int expiresInSeconds) { - string? upstreamHref = action["href"]?.GetValue(); + string? upstreamHref = ReadString(action["href"], "href"); if (string.IsNullOrEmpty(upstreamHref)) { @@ -121,7 +128,7 @@ private void RewriteAction( { if (value is not null) { - headers[name] = value.GetValue(); + headers[name] = ReadString(value, $"header {name}")!; } } } @@ -149,6 +156,32 @@ private void RewriteAction( action["expires_in"] = expiresInSeconds; } + /// + /// Reads a string field, treating an absent one as null and any other type as malformed. + /// + private static string? ReadString(JsonNode? node, string field) => + node is null + ? null + : JsonValues.String(node) ?? throw new JsonException($"The batch response's {field} is not a string."); + + /// + /// Reads an object's size, treating an absent one as zero and anything but an integer as malformed. + /// + private static long ReadSize(JsonNode? node) + { + if (node is null) + { + return 0; + } + + if (node.GetValueKind() == JsonValueKind.Number && node is JsonValue value && value.TryGetValue(out long size)) + { + return size; + } + + throw new JsonException("The batch response's size is not an integer."); + } + private static string BuildProxyHref( string actionName, string oid, diff --git a/GitLfsCache/Endpoints/ObjectRouteHandler.cs b/GitLfsCache/Endpoints/ObjectRouteHandler.cs index 873d389..868a520 100644 --- a/GitLfsCache/Endpoints/ObjectRouteHandler.cs +++ b/GitLfsCache/Endpoints/ObjectRouteHandler.cs @@ -84,30 +84,42 @@ public async Task BatchAsync( } JsonNode? upstreamBody; + JsonNode rewritten; Stream batchBody = await response.Content .ReadAsStreamAsync(cancellationToken) .ConfigureAwait(false); - await using (batchBody.ConfigureAwait(false)) + // A success status with a body this proxy cannot use (a sign-in page served with 200, or a + // field of the wrong type) is upstream's failure, not the proxy's, so it is a 502 rather than + // an unhandled exception and a 500 that points operators at the wrong component. + try { - upstreamBody = await JsonNode.ParseAsync(batchBody, cancellationToken: cancellationToken) - .ConfigureAwait(false); - } + await using (batchBody.ConfigureAwait(false)) + { + upstreamBody = await JsonNode.ParseAsync(batchBody, cancellationToken: cancellationToken) + .ConfigureAwait(false); + } - if (upstreamBody is null) + if (upstreamBody is null) + { + context.Response.StatusCode = StatusCodes.Status502BadGateway; + return; + } + + rewritten = rewriter.Rewrite(upstreamBody, new BatchRewriteContext + { + Upstream = route.Upstream, + RepositoryPath = route.RepositoryPath, + PublicBaseUrl = publicUrls.Resolve(context.Request), + }); + } + catch (System.Text.Json.JsonException) { context.Response.StatusCode = StatusCodes.Status502BadGateway; return; } - JsonNode rewritten = rewriter.Rewrite(upstreamBody, new BatchRewriteContext - { - Upstream = route.Upstream, - RepositoryPath = route.RepositoryPath, - PublicBaseUrl = publicUrls.Resolve(context.Request), - }); - context.Response.StatusCode = StatusCodes.Status200OK; context.Response.ContentType = UpstreamRequests.LfsMediaType; await context.Response