Expire a batch action's proxy token no later than upstream's href [patch]

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());
 	}
 
+	/// <summary>A one-object download batch whose action carries <paramref name="expiry"/> after its href.</summary>
+	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<int>());
+		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<int>());
 		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<string>()).Query.Replace("?t=", string.Empty, StringComparison.Ordinal);
+
+		Assert.AreEqual(expected, action["expires_in"]!.GetValue<int>());
+		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<int>());
+		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<int>());
+	}
+
+	[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<int>());
+	}
+
 	[TestMethod]
 	public void Rewrite_PreservesUnknownProperties()
 	{

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];
 
+	/// <summary>How far ahead of upstream's own expiry a proxy token stops being valid.</summary>
+	internal static readonly TimeSpan UpstreamExpiryMargin = TimeSpan.FromSeconds(30);
+
 	/// <summary>
 	/// Rewrites a batch response, leaving the input untouched.
 	/// </summary>
@@ -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}";
 	}
+
+	/// <summary>
+	/// Works out when an action's proxy token expires: at the proxy's own lifetime, or earlier when
+	/// upstream says its href expires sooner.
+	/// </summary>
+	/// <remarks>
+	/// Upstream's expiry is brought forward by <see cref="UpstreamExpiryMargin"/>, so a transfer that
+	/// starts just inside the advertised window still reaches upstream before its credential lapses.
+	/// An <c>expires_at</c> or <c>expires_in</c> of the wrong type is treated as absent.
+	/// </remarks>
+	/// <param name="action">The upstream action.</param>
+	/// <param name="now">The current time.</param>
+	/// <param name="proxyExpiresAt">When the proxy's own token lifetime runs out.</param>
+	/// <returns>The earliest of the proxy expiry and upstream's, less the margin.</returns>
+	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;
 }

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. |

docs/superpowers/specs/2026-08-18-gitlfscache-design.md

@@ -44,7 +44,7 @@ Clients configure `lfs.url` as `https://cache.example/<upstream>/<repo path>/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.