Add RemoteOrchestrationTypeBinder for stricter type handling#1344
Add RemoteOrchestrationTypeBinder for stricter type handling#1344
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a strict JSON.NET serialization binder for the Azure Service Fabric remote orchestration client to constrain which $type values can be resolved, reducing risk from unsafe type handling in remote RPC payload serialization settings.
Changes:
- Added
RemoteOrchestrationTypeBinder(allowlist-basedISerializationBinder) for DurableTask remote RPC payloads. - Updated
RemoteOrchestrationServiceClient.PutJsonAsyncto useTypeNameHandling.Autoand the new strict binder. - Added unit tests intended to validate round-trip behavior and rejection of non-allowlisted types.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/DurableTask.AzureServiceFabric/Remote/RemoteOrchestrationTypeBinder.cs | New strict binder that only permits types from DurableTask.Core, DurableTask.AzureServiceFabric, and Dictionary<string,string>. |
| src/DurableTask.AzureServiceFabric/Remote/RemoteOrchestrationServiceClient.cs | Switches JSON formatter to TypeNameHandling.Auto and applies the strict binder. |
| Test/DurableTask.AzureServiceFabric.Tests/RemoteOrchestrationTypeBinderTests.cs | Adds tests for binder allowlist and rejection behavior (but currently placed outside the solution’s active test projects). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [TestClass] | ||
| public class RemoteOrchestrationTypeBinderTests | ||
| { | ||
| // Mirrors the production formatter settings configured in RemoteOrchestrationServiceClient.PutJsonAsync. | ||
| static readonly JsonSerializerSettings RoundTripSettings = new JsonSerializerSettings | ||
| { | ||
| TypeNameHandling = TypeNameHandling.Auto, | ||
| SerializationBinder = new RemoteOrchestrationTypeBinder() | ||
| }; |
There was a problem hiding this comment.
This test file is under Test/ (capital T), but the solution’s MSTest projects live under test/ (lowercase) and DurableTask.AzureServiceFabric.Tests.csproj is at test/DurableTask.AzureServiceFabric.Tests/…. As-is, this file won’t be compiled or executed by CI. Move it into test/DurableTask.AzureServiceFabric.Tests/ (or update the test project to include it) so the new binder behavior is actually validated.
| public void RejectsNonAllowlistedRootType() | ||
| { | ||
| string json = "{\"$type\":\"System.IO.FileInfo, System.Private.CoreLib\",\"OriginalPath\":\"c:\\\\evil\"}"; | ||
| Assert.ThrowsException<JsonSerializationException>( | ||
| () => JsonConvert.DeserializeObject<object>(json, RoundTripSettings)); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void RejectsNonAllowlistedNestedType() | ||
| { | ||
| string json = "{\"$type\":\"DurableTask.Core.History.ExecutionStartedEvent, DurableTask.Core\"," | ||
| + "\"Tags\":{\"$type\":\"System.Collections.Generic.SortedDictionary`2[[System.String, System.Private.CoreLib],[System.String, System.Private.CoreLib]], System.Collections\"}}"; | ||
| Assert.ThrowsException<JsonSerializationException>( | ||
| () => JsonConvert.DeserializeObject<HistoryEvent>(json, RoundTripSettings)); |
There was a problem hiding this comment.
These payloads hard-code .NET (Core) assembly names (System.Private.CoreLib, System.Collections). The DurableTask.AzureServiceFabric.Tests project targets net48, where these types live in different assemblies, so the test will fail once it’s included in the test project. Consider constructing the $type strings using typeof(FileInfo).Assembly.GetName().Name / typeof(SortedDictionary<,>).Assembly.GetName().Name (or otherwise avoiding framework-specific assembly names).
Resolves https://msazure.visualstudio.com/Antares/_workitems/edit/37181654