Workflow content: durable key, run-scoped references, validated digests

ANcpLua · claude · ANcpLua · commit 2cea8fa95be0 · 2026-07-28T02:45:32.000+02:00
Three defects in the captured-content path, each with a test that fails without
the fix.

The content encryption key could be derived from QYL_OTLP_PRIMARY_API_KEY. That
bound the lifetime of stored content to a rotatable ingest credential: rotating
the OTLP key left every captured payload undecryptable, surfacing later as an
AES-GCM tag mismatch that reads like corruption rather than key management.
Production now requires QYL_WORKFLOW_CONTENT_KEY and fails closed without it —
a collector that will not start is recoverable; ciphertext without its key is not.

workflow_content is deduplicated per project by digest, so the reference
existence check passed for content captured by any run in the project. Since
GetWorkflowContentAsync authorises on the reference row existing for the asking
run, minting that row was the whole exploit: reference another run's digest,
then read its payload back. A run may now reference only content it captured in
the same batch or has already referenced.

The ^sha256:[a-f0-9]{64}$ pattern on WorkflowContentRef is an OpenAPI constraint
with no runtime enforcement in the generated contract. A reference shorter than
the prefix threw ArgumentOutOfRangeException out of the digest slice — a 500 on
attacker-controlled input instead of a rejected request.

QYL_WORKFLOW_CONTENT_KEY is set on the production collector (skipDeploys, so it
lands on the next deploy). No content is stored yet, so the key can still be
rotated freely.

Co-Authored-By: Claude Opus 5 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/services/qyl.collector/Storage/DuckDbStore.Workflow.cs b/services/qyl.collector/Storage/DuckDbStore.Workflow.cs
@@ -115,6 +115,10 @@ public Task<WorkflowAppendResult> AppendWorkflowEventsAsync(
             foreach (var item in content)
                 InsertWorkflowContent(con, transaction, projectId, _workflowContentProtector.Protect(item));
 
+            var capturedInThisBatch = content
+                .Select(static item => item.ContentRef)
+                .ToHashSet(StringComparer.Ordinal);
+
             var accepted = 0;
             var duplicates = 0;
             ulong? firstJournalSequence = null;
@@ -168,6 +172,8 @@ public Task<WorkflowAppendResult> AppendWorkflowEventsAsync(
                     con,
                     transaction,
                     projectId,
+                    runId,
+                    capturedInThisBatch,
                     workflowEvent.ContentRefs);
                 latest++;
                 InsertWorkflowEvent(
@@ -757,25 +763,38 @@ FROM workflow_events
         return reader.Read() ? ReadWorkflowEvent(reader) : null;
     }
 
+    /// <summary>
+    /// A run may reference content it captured in this batch, or content it has already
+    /// referenced. It may NOT reference content merely because some other run in the project
+    /// captured it: <c>workflow_content</c> is deduplicated per project by digest, so a
+    /// project-scoped existence check let run A mint a reference row for run B's payload, and
+    /// <see cref="GetWorkflowContentAsync"/> then served it — it gates on the reference row
+    /// existing for the asking run, which A had just created.
+    /// </summary>
     private static void EnsureContentReferencesExist(
         DuckDBConnection con,
         DbTransaction transaction,
         string projectId,
+        string runId,
+        IReadOnlySet<string> capturedInThisBatch,
         IReadOnlyList<string> contentRefs)
     {
         foreach (var contentRef in contentRefs)
         {
+            if (capturedInThisBatch.Contains(contentRef))
+                continue;
+
             using var command = con.CreateCommand();
             command.Transaction = transaction;
             command.CommandText = """
                                   SELECT count(*)
-                                  FROM workflow_content
-                                  WHERE project_id = $1 AND content_ref = $2
+                                  FROM workflow_content_refs
+                                  WHERE project_id = $1 AND run_id = $2 AND content_ref = $3
                                   """;
-            AddParameters(command, projectId, contentRef);
+            AddParameters(command, projectId, runId, contentRef);
             if (Convert.ToInt32(command.ExecuteScalar(), CultureInfo.InvariantCulture) is 0)
                 throw new WorkflowEventConflictException(
-                    $"Workflow event references content '{contentRef}' that has not been captured.");
+                    $"Workflow event references content '{contentRef}' that this run has not captured.");
         }
     }
 
diff --git a/services/qyl.collector/Workflow/WorkflowContentProtector.cs b/services/qyl.collector/Workflow/WorkflowContentProtector.cs
@@ -40,26 +40,58 @@ public static WorkflowContentProtector FromConfiguration(
             return new WorkflowContentProtector(key);
         }
 
-        if (configuration["QYL_OTLP_PRIMARY_API_KEY"] is { Length: > 0 } apiKey)
-        {
-            return new WorkflowContentProtector(
-                SHA256.HashData(Encoding.UTF8.GetBytes($"qyl.workflow.content.v1\0{apiKey}")));
-        }
-
         if (environment.IsDevelopment() || environment.IsEnvironment("Testing"))
         {
             return new WorkflowContentProtector(
                 SHA256.HashData(Encoding.UTF8.GetBytes("qyl-development-workflow-content-key")));
         }
 
+        // Deriving this key from QYL_OTLP_PRIMARY_API_KEY used to be the production fallback.
+        // That silently bound the lifetime of stored content to a ROTATABLE ingest credential:
+        // rotating the OTLP key left every previously captured payload undecryptable, as an
+        // AES-GCM tag mismatch at read time rather than anything that looks like a key problem.
+        // Content encryption needs a key with its own rotation story, so refuse to start rather
+        // than accept one that is guaranteed to be rotated out from under the data.
         throw new InvalidOperationException(
-            "Workflow content capture requires QYL_WORKFLOW_CONTENT_KEY or QYL_OTLP_PRIMARY_API_KEY.");
+            "Workflow content capture requires QYL_WORKFLOW_CONTENT_KEY (base64-encoded 32 bytes). " +
+            "It must not be derived from the OTLP ingest key: that key rotates, and rotating it " +
+            "would permanently destroy access to all previously captured workflow content.");
+    }
+
+    private const string ContentRefPrefix = "sha256:";
+    private const int ContentRefLength = 71; // "sha256:" + 64 lowercase hex characters.
+
+    /// <summary>
+    /// The <c>^sha256:[a-f0-9]{64}$</c> pattern on WorkflowContentRef is an OpenAPI constraint;
+    /// the generated contract carries no runtime validation attribute, so an untrusted observer
+    /// can post any string. Without this guard a ref shorter than the prefix threw
+    /// ArgumentOutOfRangeException out of the slice below and surfaced as a 500 from the append
+    /// endpoint instead of a rejected request.
+    /// </summary>
+    internal static void RequireWellFormedContentRef(string contentRef)
+    {
+        if (contentRef.Length != ContentRefLength ||
+            !contentRef.StartsWith(ContentRefPrefix, StringComparison.Ordinal))
+        {
+            throw new InvalidDataException(
+                $"Captured content reference '{contentRef}' is not a well-formed 'sha256:' digest.");
+        }
+
+        foreach (var character in contentRef.AsSpan(ContentRefPrefix.Length))
+        {
+            if (character is not (>= '0' and <= '9') and not (>= 'a' and <= 'f'))
+            {
+                throw new InvalidDataException(
+                    $"Captured content reference '{contentRef}' is not lowercase hexadecimal.");
+            }
+        }
     }
 
     public WorkflowContentStorageRow Protect(WorkflowContentWrite content)
     {
+        RequireWellFormedContentRef(content.ContentRef);
         var plaintext = Decode(content);
-        var expected = content.ContentRef["sha256:".Length..];
+        var expected = content.ContentRef[ContentRefPrefix.Length..];
         var actual = Convert.ToHexStringLower(SHA256.HashData(plaintext));
         if (!CryptographicOperations.FixedTimeEquals(
                 Encoding.ASCII.GetBytes(expected),
diff --git a/tests/Qyl.Collector.Tests/WorkflowContentSecurityTests.cs b/tests/Qyl.Collector.Tests/WorkflowContentSecurityTests.cs
@@ -0,0 +1,153 @@
+using System.Security.Cryptography;
+using System.Text;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.FileProviders;
+using Microsoft.Extensions.Hosting;
+using Qyl.Api.Contracts.Workflow;
+using Qyl.Collector.Storage;
+using Qyl.Collector.Workflow;
+
+namespace Qyl.Collector.Tests;
+
+/// <summary>
+/// Captured workflow content is agent output — tool results, file contents, messages. These
+/// assert the three properties that decide whether storing it is safe: the key outlives the
+/// data, a run cannot read another run's payload, and a malformed reference is rejected rather
+/// than crashing the ingest path.
+/// </summary>
+public sealed class WorkflowContentSecurityTests
+{
+    private static readonly DateTimeOffset s_startedAt =
+        new(2026, 7, 28, 8, 0, 0, TimeSpan.Zero);
+
+    /// <summary>
+    /// The production fallback used to derive the content key from QYL_OTLP_PRIMARY_API_KEY.
+    /// That key rotates; the data it encrypts does not get re-encrypted, so a routine ingest-key
+    /// rotation silently destroyed every previously captured payload — surfacing later as an
+    /// AES-GCM tag mismatch, which reads like corruption rather than a key-management mistake.
+    /// Refusing to boot is the correct trade: a collector that will not start is recoverable.
+    /// </summary>
+    [Fact]
+    public void Content_key_is_never_derived_from_the_rotatable_ingest_key()
+    {
+        var configuration = new ConfigurationBuilder()
+            .AddInMemoryCollection(new Dictionary<string, string?>
+            {
+                ["QYL_OTLP_PRIMARY_API_KEY"] = "an-ingest-key-that-will-be-rotated",
+            })
+            .Build();
+
+        var failure = Assert.Throws<InvalidOperationException>(
+            () => WorkflowContentProtector.FromConfiguration(configuration, new ProductionEnvironment()));
+
+        Assert.Contains("QYL_WORKFLOW_CONTENT_KEY", failure.Message, StringComparison.Ordinal);
+    }
+
+    /// <summary>An explicit, independently rotatable key is still accepted.</summary>
+    [Fact]
+    public void An_explicit_content_key_is_accepted_in_production()
+    {
+        var configuration = new ConfigurationBuilder()
+            .AddInMemoryCollection(new Dictionary<string, string?>
+            {
+                ["QYL_WORKFLOW_CONTENT_KEY"] = Convert.ToBase64String(RandomNumberGenerator.GetBytes(32)),
+            })
+            .Build();
+
+        Assert.NotNull(WorkflowContentProtector.FromConfiguration(configuration, new ProductionEnvironment()));
+    }
+
+    /// <summary>
+    /// workflow_content is deduplicated per project by digest, so the existence check used to
+    /// pass for any content captured by ANY run in the project. Because GetWorkflowContentAsync
+    /// authorises on the reference row existing for the asking run, minting that row was the
+    /// whole exploit: reference another run's digest, then read its payload back as your own.
+    /// </summary>
+    [Fact]
+    public async Task A_run_cannot_reference_content_captured_by_another_run()
+    {
+        await using var store = new DuckDbStore(":memory:");
+        await CreateRunAsync(store, "run-victim");
+        await CreateRunAsync(store, "run-attacker");
+
+        const string secret = "AWS_SECRET_ACCESS_KEY=not-actually-a-real-key";
+        var secretRef = ContentRef(secret);
+
+        // The victim run captures the payload and references it legitimately.
+        await store.AppendWorkflowEventsAsync(
+            "project-a", "run-victim", "observer-1",
+            [Event("victim-1", 1, WorkflowJournalEventKind.AttemptStarted, "attempt-1", [secretRef])],
+            [new WorkflowContentWrite(secretRef, "text/plain", WorkflowContentEncoding.Utf8, secret)],
+            TestContext.Current.CancellationToken);
+
+        // The attacker run knows only the digest and captures nothing.
+        var reach = await Assert.ThrowsAsync<WorkflowEventConflictException>(() =>
+            store.AppendWorkflowEventsAsync(
+                "project-a", "run-attacker", "observer-2",
+                [Event("attacker-1", 1, WorkflowJournalEventKind.AttemptStarted, "attempt-1", [secretRef])],
+                [],
+                TestContext.Current.CancellationToken));
+        Assert.Contains("has not captured", reach.Message, StringComparison.Ordinal);
+
+        // And the payload stays unreadable through the attacker's run scope.
+        Assert.Null(await store.GetWorkflowContentAsync(
+            "project-a", "run-attacker", secretRef, TestContext.Current.CancellationToken));
+        Assert.NotNull(await store.GetWorkflowContentAsync(
+            "project-a", "run-victim", secretRef, TestContext.Current.CancellationToken));
+    }
+
+    /// <summary>
+    /// The ^sha256:[a-f0-9]{64}$ pattern is an OpenAPI constraint with no runtime enforcement in
+    /// the generated contract, so an observer can post anything. A ref shorter than the prefix
+    /// threw ArgumentOutOfRangeException out of the digest slice — a 500 on attacker-controlled
+    /// input rather than a rejected request.
+    /// </summary>
+    [Theory]
+    [InlineData("sha")]
+    [InlineData("")]
+    [InlineData("sha256:")]
+    [InlineData("sha256:tooshort")]
+    [InlineData("md5:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef")]
+    [InlineData("sha256:0123456789ABCDEF0123456789abcdef0123456789abcdef0123456789abcdef")]
+    public async Task A_malformed_content_reference_is_rejected_not_crashed_on(string contentRef)
+    {
+        await using var store = new DuckDbStore(":memory:");
+        await CreateRunAsync(store, "run-1");
+
+        await Assert.ThrowsAsync<InvalidDataException>(() =>
+            store.AppendWorkflowEventsAsync(
+                "project-a", "run-1", "observer-1",
+                [Event("e1", 1, WorkflowJournalEventKind.AttemptStarted, "attempt-1", [])],
+                [new WorkflowContentWrite(contentRef, "text/plain", WorkflowContentEncoding.Utf8, "payload")],
+                TestContext.Current.CancellationToken));
+    }
+
+    private sealed class ProductionEnvironment : IHostEnvironment
+    {
+        public string EnvironmentName { get; set; } = Environments.Production;
+        public string ApplicationName { get; set; } = "qyl.collector";
+        public string ContentRootPath { get; set; } = AppContext.BaseDirectory;
+        public IFileProvider ContentRootFileProvider { get; set; } =
+            new NullFileProvider();
+    }
+
+    private static string ContentRef(string plaintext) =>
+        $"sha256:{Convert.ToHexStringLower(SHA256.HashData(Encoding.UTF8.GetBytes(plaintext)))}";
+
+    private static Task<WorkflowRunStorageRow> CreateRunAsync(DuckDbStore store, string runId) =>
+        store.CreateWorkflowRunAsync(
+            new WorkflowRunStorageRow(
+                "project-a", runId, "thread-1", "Content security fixture",
+                WorkflowRunStatus.Active, s_startedAt, null, 0, null, null),
+            TestContext.Current.CancellationToken);
+
+    private static WorkflowEventWrite Event(
+        string eventId,
+        ulong sourceSequence,
+        WorkflowJournalEventKind kind,
+        string attemptId,
+        IReadOnlyList<string> contentRefs) =>
+        new(
+            eventId, sourceSequence, s_startedAt, kind, "thread-1", null, attemptId,
+            null, null, null, null, contentRefs, null);
+}
