diff --git a/GitLfsCache.Tests/Batch/BatchRewriterTests.cs b/GitLfsCache.Tests/Batch/BatchRewriterTests.cs index b21c868..60b1568 100644 --- a/GitLfsCache.Tests/Batch/BatchRewriterTests.cs +++ b/GitLfsCache.Tests/Batch/BatchRewriterTests.cs @@ -17,12 +17,18 @@ public class BatchRewriterTests private static readonly DateTimeOffset Now = new(2026, 8, 18, 12, 0, 0, TimeSpan.Zero); private static (BatchRewriter Rewriter, HrefTokenCodec Codec) Create() + { + (BatchRewriter rewriter, HrefTokenCodec codec, _) = CreateWithClock(); + return (rewriter, codec); + } + + private static (BatchRewriter Rewriter, HrefTokenCodec Codec, FakeTimeProvider Time) CreateWithClock() { GitLfsCacheOptions options = new() { TokenLifetime = TimeSpan.FromHours(1) }; options.TokenKeys.Add(Convert.ToBase64String(new byte[32])); FakeTimeProvider time = new(Now); HrefTokenCodec codec = new(new AesEncryptionProvider(), Options.Create(options), time); - return (new BatchRewriter(codec, Options.Create(options), time), codec); + return (new BatchRewriter(codec, Options.Create(options), time), codec, time); } private static BatchRewriteContext Context() => new() @@ -75,7 +81,9 @@ public void Rewrite_DownloadToken_CarriesTheUpstreamAction() Assert.AreEqual(TokenAction.Download, token.Action); Assert.AreEqual("github", token.Upstream); Assert.AreEqual(500L, token.Size); - Assert.AreEqual(Now.AddHours(1), token.ExpiresAt); + + // The fixture's expires_at is exactly the proxy lifetime away, so upstream's, less the margin, wins. + Assert.AreEqual(Now.AddHours(1) - BatchRewriter.UpstreamExpiryMargin, token.ExpiresAt); } [TestMethod] @@ -89,17 +97,90 @@ public void Rewrite_RemovesTheUpstreamCredentialFromTheResponse() Assert.DoesNotContain("ado-secret-token", rewritten.ToJsonString()); } + /// A one-object download batch whose action carries after its href. + private static JsonNode DownloadBatch(string expiry) => JsonNode.Parse( + """{"objects":[{"oid":"aa","size":1,"actions":{"download":{"href":"https://upstream.example/aa" """ + + expiry + + "}}}]}")!; + [TestMethod] - public void Rewrite_SetsExpiresInFromTokenLifetimeAndDropsExpiresAt() + public void Rewrite_ActionWithNoUpstreamExpiry_KeepsTheTokenLifetime() + { + (BatchRewriter rewriter, HrefTokenCodec codec) = Create(); + + JsonObject action = FirstAction(rewriter.Rewrite(DownloadBatch(string.Empty), Context()), "download"); + + Assert.AreEqual(3600, action["expires_in"]!.GetValue()); + Assert.AreEqual(Now.AddHours(1), Decode(codec, action).ExpiresAt); + } + + [TestMethod] + public void Rewrite_DropsExpiresAt() { (BatchRewriter rewriter, _) = Create(); JsonObject action = FirstAction(rewriter.Rewrite(Load("ado-download-batch.json"), Context()), "download"); - Assert.AreEqual(3600, action["expires_in"]!.GetValue()); Assert.IsFalse(action.ContainsKey("expires_at")); } + [TestMethod] + [DataRow(",\"expires_in\":600", DisplayName = "expires_in")] + [DataRow(",\"expires_at\":\"2026-08-18T12:10:00Z\"", DisplayName = "expires_at")] + [DataRow(",\"expires_in\":600,\"expires_at\":\"2026-08-18T13:30:00Z\"", DisplayName = "expires_in sooner than expires_at")] + [DataRow(",\"expires_in\":5400,\"expires_at\":\"2026-08-18T12:10:00Z\"", DisplayName = "expires_at sooner than expires_in")] + public void Rewrite_UpstreamExpiresSooner_TokenAndExpiresInFollowUpstream(string expiry) + { + (BatchRewriter rewriter, HrefTokenCodec codec, FakeTimeProvider time) = CreateWithClock(); + int expected = 600 - (int)BatchRewriter.UpstreamExpiryMargin.TotalSeconds; + + JsonObject action = FirstAction(rewriter.Rewrite(DownloadBatch(expiry), Context()), "download"); + string encoded = new Uri(action["href"]!.GetValue()).Query.Replace("?t=", string.Empty, StringComparison.Ordinal); + + Assert.AreEqual(expected, action["expires_in"]!.GetValue()); + Assert.IsFalse(action.ContainsKey("expires_at")); + Assert.AreEqual(Now.AddSeconds(expected), Decode(codec, action).ExpiresAt); + + // Upstream's href is dead at 600 s, so the proxy token must already be refused by then. + time.Advance(TimeSpan.FromSeconds(600)); + Assert.IsFalse(codec.TryDecode(encoded, out _, out _)); + } + + [TestMethod] + public void Rewrite_UpstreamExpiresLater_KeepsTheTokenLifetime() + { + (BatchRewriter rewriter, HrefTokenCodec codec) = Create(); + + JsonObject action = FirstAction(rewriter.Rewrite(DownloadBatch(",\"expires_in\":86400"), Context()), "download"); + + Assert.AreEqual(3600, action["expires_in"]!.GetValue()); + Assert.AreEqual(Now.AddHours(1), Decode(codec, action).ExpiresAt); + } + + [TestMethod] + [DataRow(",\"expires_in\":\"600\"", DisplayName = "string expires_in")] + [DataRow(",\"expires_at\":600", DisplayName = "numeric expires_at")] + [DataRow(",\"expires_at\":\"not a date\"", DisplayName = "unparseable expires_at")] + [DataRow(",\"expires_in\":9223372036854775807", DisplayName = "huge expires_in")] + public void Rewrite_UnusableUpstreamExpiry_IsIgnored(string expiry) + { + (BatchRewriter rewriter, _) = Create(); + + JsonObject action = FirstAction(rewriter.Rewrite(DownloadBatch(expiry), Context()), "download"); + + Assert.AreEqual(3600, action["expires_in"]!.GetValue()); + } + + [TestMethod] + public void Rewrite_UpstreamAlreadyExpired_AdvertisesZero() + { + (BatchRewriter rewriter, _) = Create(); + + JsonObject action = FirstAction(rewriter.Rewrite(DownloadBatch(",\"expires_in\":10"), Context()), "download"); + + Assert.AreEqual(0, action["expires_in"]!.GetValue()); + } + [TestMethod] public void Rewrite_PreservesUnknownProperties() { diff --git a/GitLfsCache/Batch/BatchRewriter.cs b/GitLfsCache/Batch/BatchRewriter.cs index 8599d7a..163749c 100644 --- a/GitLfsCache/Batch/BatchRewriter.cs +++ b/GitLfsCache/Batch/BatchRewriter.cs @@ -2,6 +2,7 @@ namespace ktsu.GitLfsCache.Batch; +using System.Text.Json; using System.Text.Json.Nodes; using ktsu.GitLfsCache.Configuration; using ktsu.GitLfsCache.Tokens; @@ -28,6 +29,9 @@ public sealed class BatchRewriter( private static readonly string[] RewrittenActions = [TokenAction.Download, TokenAction.Upload, TokenAction.Verify]; + /// How far ahead of upstream's own expiry a proxy token stops being valid. + internal static readonly TimeSpan UpstreamExpiryMargin = TimeSpan.FromSeconds(30); + /// /// Rewrites a batch response, leaving the input untouched. /// @@ -126,6 +130,10 @@ private void RewriteAction( } } + // The token carries upstream's href and header, so it can be good for no longer than they are. + DateTimeOffset now = timeProvider.GetUtcNow(); + DateTimeOffset actionExpiresAt = ActionExpiry(action, now, expiresAt); + string token = codec.Encode(new HrefToken { Oid = oid, @@ -134,7 +142,7 @@ private void RewriteAction( Action = actionName, UpstreamHref = upstreamHref, UpstreamHeaders = headers, - ExpiresAt = expiresAt, + ExpiresAt = actionExpiresAt, }); action["href"] = BuildProxyHref(actionName, oid, token, context); @@ -143,10 +151,13 @@ private void RewriteAction( // upstream's bearer token, which is most of the point of terminating the transfer locally. action.Remove("header"); - // An upstream expires_at would contradict this proxy's own token lifetime, so it is replaced - // rather than left to disagree. + // The advertised expiry is the token's, so the client re-batches before the proxy would have to + // use an upstream credential that has already expired. expires_in alone carries it, rather than + // leaving upstream's expires_at to disagree with it. action.Remove("expires_at"); - action["expires_in"] = expiresInSeconds; + action["expires_in"] = actionExpiresAt < expiresAt + ? (int)Math.Max(0, Math.Floor((actionExpiresAt - now).TotalSeconds)) + : expiresInSeconds; } private static string BuildProxyHref( @@ -161,4 +172,50 @@ private static string BuildProxyHref( return $"{basePath}/{context.Upstream}/{context.RepositoryPath}/objects/{oid}{suffix}?t={token}"; } + + /// + /// Works out when an action's proxy token expires: at the proxy's own lifetime, or earlier when + /// upstream says its href expires sooner. + /// + /// + /// Upstream's expiry is brought forward by , so a transfer that + /// starts just inside the advertised window still reaches upstream before its credential lapses. + /// An expires_at or expires_in of the wrong type is treated as absent. + /// + /// The upstream action. + /// The current time. + /// When the proxy's own token lifetime runs out. + /// The earliest of the proxy expiry and upstream's, less the margin. + private static DateTimeOffset ActionExpiry(JsonObject action, DateTimeOffset now, DateTimeOffset proxyExpiresAt) + { + DateTimeOffset expiry = proxyExpiresAt; + + // System.Text.Json reads an ISO 8601 string as a DateTimeOffset, which is the form the Git LFS + // specification gives expires_at. + if (action["expires_at"] is JsonValue expiresAtNode + && expiresAtNode.GetValueKind() == JsonValueKind.String + && expiresAtNode.TryGetValue(out DateTimeOffset upstreamExpiresAt)) + { + expiry = Earliest(expiry, LessMargin(upstreamExpiresAt)); + } + + if (action["expires_in"] is JsonValue expiresInNode + && expiresInNode.GetValueKind() == JsonValueKind.Number + && expiresInNode.TryGetValue(out long expiresInSeconds)) + { + // Clamped so an absurd value cannot overflow the date arithmetic; anything this far out is + // later than the proxy lifetime anyway. + expiry = Earliest(expiry, LessMargin(now.AddSeconds(Math.Clamp(expiresInSeconds, int.MinValue, int.MaxValue)))); + } + + return expiry; + } + + private static DateTimeOffset LessMargin(DateTimeOffset upstreamExpiry) => + upstreamExpiry - DateTimeOffset.MinValue > UpstreamExpiryMargin + ? upstreamExpiry - UpstreamExpiryMargin + : DateTimeOffset.MinValue; + + private static DateTimeOffset Earliest(DateTimeOffset first, DateTimeOffset second) => + first <= second ? first : second; } diff --git a/README.md b/README.md index 8727a0e..4da5d3b 100644 --- a/README.md +++ b/README.md @@ -129,7 +129,7 @@ The flags are a convenience over the same settings and win over all three, so `- |---|---| | `PublicBaseUrl` | The URL clients actually use. Optional: when unset, each transfer URL is derived from the incoming request, which requires the ingress to send `X-Forwarded-Proto` and `X-Forwarded-Host`. Setting it explicitly is safer, because only the operator knows for certain what clients addressed. | | `TokenKeys` | Base64 encoded 32 byte keys protecting rewritten transfer URLs. A list, so a key can be rotated without breaking transfers in flight: put the new key first and keep the old one until the token lifetime has elapsed. | -| `TokenLifetime` | How long a rewritten transfer URL stays valid. | +| `TokenLifetime` | How long a rewritten transfer URL stays valid, at most. When upstream says its own URL expires sooner, the rewritten one expires 30 seconds before it. | | `Store:MaxSize` | Byte budget, accepting decimal (`500GB`) and binary (`500Gi`) suffixes. | | `Store:LowWaterMark` | The fraction of the budget a sweep reduces the store to. | | `Store:StagingMaxAge` | How long an orphaned staging file from a crashed write survives. | diff --git a/docs/superpowers/specs/2026-08-18-gitlfscache-design.md b/docs/superpowers/specs/2026-08-18-gitlfscache-design.md index 6248d92..695c026 100644 --- a/docs/superpowers/specs/2026-08-18-gitlfscache-design.md +++ b/docs/superpowers/specs/2026-08-18-gitlfscache-design.md @@ -44,7 +44,7 @@ Clients configure `lfs.url` as `https://cache.example///inf 1. Resolve the upstream key to a base URL. An unknown key is a 404 before anything else happens. 2. Forward the request body and the client's `Authorization` header to upstream unchanged. 3. Relay any non-success response verbatim, including status and body, so the client sees upstream's real answer rather than a proxy interpretation. -4. For each object in a success response, rewrite each action's `href` to a proxy URL and drop the action's `header` map, since the credentials it carries now live inside the token. Set `expires_in` from the token lifetime. +4. For each object in a success response, rewrite each action's `href` to a proxy URL and drop the action's `header` map, since the credentials it carries now live inside the token. Set `expires_in` from the token lifetime, never later than upstream's own `expires_at` or `expires_in` for that action (less a 30-second margin), since the token carries upstream's credentials and is good for no longer than they are. 5. Objects that upstream returned with an `error` and no actions pass through untouched. The store is never consulted during a Batch call, and a Batch call is never served from cache even when every object is already local. Short-circuiting it would move the authorization decision from upstream into the proxy, which is the one thing this design refuses to do.