[Bug] Unexpected Serialization of Activity Results

[robcao]

What are you really trying to do?

I have a use case where I am invoking signal from start in an activity. I stumbled across a scenario where Temporal client credentials can potentially get leaked in workflow history.

Describe the bug

When executing an activity that returns a Task, if you instead return a Task<T>, the value of T is stored in workflow history. This can lead to unexpected leakage of Temporal server credentials when T is a WorkflowHandle.

Consider the following activity. The activity returns Task<WorkflowHandle>, and the workflow handle gets stored in workflow history.

internal class Activities(ITemporalClient client)
{
	/// <summary>
	/// When returning a <see cref="Task{WorkflowHandle}"/>, the workflow handle gets serialized to workflow history.
	/// </summary>
	[Activity]
	public Task StartWorkflowThatLeaksWorkflowHandleToEventHistoryAsync()
	{
		WorkflowOptions options = new WorkflowOptions(Guid.NewGuid().ToString(), "my-tq");

		return client.StartWorkflowAsync<Other>(wf => wf.RunAsync(), options);
	}

The issue here is that WorkflowHandle has a reference to the Temporal client, which leaks sensitive information like the api key or TLS certificate into the workflow history.

Minimal Reproduction

I have a code sample here: https://github.com/robcao/temporal-client-serialization

To reproduce:

Start a dev server: temporal server start-dev.

Start the worker: dotnet run.

Execute a workflow: temporal workflow start --task-queue=my-tq --type=Example.

Navigate to the temporal ui and click the Example workflow. If you look at the workflow history, you'll see that the entire workflow handle was stored.

Environment/Versions

• OS and processor: [e.g. M1 Mac, x86 Windows, Linux]
• Temporal Version: [e.g. 1.14.0?] and/or SDK version
• Are you using Docker or Kubernetes or building Temporal from source?
  .NET SDK 1.3.1

Additional context

This can be fixed by re-writing the activity like this, so it's fairly minor. However, perhaps serialization of the TemporalClient should be changed to omit the ApiKey, TlsOptions, and other potentially sensitive values?

internal class Activities(ITemporalClient client)
{
	/// <summary>
	/// No problems here because a <see cref="Task"/>> is returned.
	/// </summary>
	[Activity]
	public async Task StartWorkflowThatDoesNotLeakWorkflowHandleToEventHistoryAsync()
	{
		WorkflowOptions options = new WorkflowOptions(Guid.NewGuid().ToString(), "my-tq");

		await client.StartWorkflowAsync<Other>(wf => wf.RunAsync(), options).ConfigureAwait(false);
	}
}

[cretz]

You are returning Task<WorkflowHandle> which is then serialized.

WorkflowHandle is not meant to be serializable (just coincidence that it works with System.Text.Json). This would be the same as if you returned Task<MyDatabaseOrmEntity> that contains a property with the database that had its initial options. That the return type is Task does not mean the result is expected to automatically be removed, Task is just a supertype of Task<TResult> and return covariance applies and we do support caller specifying the result-type for upcasting. This information would leak if you just called the method regularly and not through Temporal.

You should not return the workflow handle from the activity if you do not want it to be serialized. If you cannot await, then you can do something like startTask.ContinueWith(_ => Task.CompletedTask), TaskContinuationOptions.OnlyOnRanToCompletion).