Add HistoryEventSerializationBinder to constrain HistoryEvent deserialization#1343
Add HistoryEventSerializationBinder to constrain HistoryEvent deserialization#1343
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a restricted JSON serialization binder for Azure Table–backed orchestration history event deserialization in the ServiceBus backend to reduce unsafe $type-based deserialization exposure.
Changes:
- Introduced
HistoryEventSerializationBinderto allowlist deserializable types for history event payloads. - Updated Azure Table history entity deserialization to use the new binder.
- Added MSTest coverage validating round-trip behavior and rejection of non-allowlisted
$typevalues.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/DurableTask.ServiceBus/Tracking/HistoryEventSerializationBinder.cs | New restrictive binder (extends PackageUpgradeSerializationBinder) that rejects non-allowlisted resolved types. |
| src/DurableTask.ServiceBus/Tracking/AzureTableOrchestrationHistoryEventEntity.cs | Switches history event read settings to use the new restrictive binder. |
| Test/DurableTask.ServiceBus.Tests/HistoryEventSerializationBinderTests.cs | Adds tests for round-trip tags and $type rejection/allow scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| string json = "{\"$type\":\"System.IO.FileInfo, System.Private.CoreLib\",\"OriginalPath\":\"c:\\\\evil\"}"; | ||
| Assert.ThrowsException<JsonSerializationException>( | ||
| () => JsonConvert.DeserializeObject<HistoryEvent>(json, ReadJsonSettings)); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void RejectsNonAllowlistedNestedType() | ||
| { | ||
| // Embed a malicious $type inside an otherwise valid ExecutionStartedEvent's Tags | ||
| // member. The Tags member is IDictionary<string,string>, so the $type token is | ||
| // honored by Newtonsoft.Json when reading. Anything other than | ||
| // Dictionary<string,string> must be rejected. | ||
| 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>( |
There was a problem hiding this comment.
These $type payloads hard-code BCL assembly names (e.g., System.Private.CoreLib, System.Collections). Because this test project targets both net8.0 and net48, those assembly names won’t consistently resolve across TFMs (and can cause the test to pass due to type-resolution failure rather than the binder’s allowlist). Build the $type strings using typeof(FileInfo).Assembly.GetName().Name / typeof(SortedDictionary<string,string>).Assembly.GetName().Name (and similarly for string) so the types resolve on each target framework and the binder is what triggers the rejection.
AnatoliB
force-pushed
the
anatolib/codeql-fix-37181649
branch
from
25d2db1 to
b168f20
Compare
Resolves https://msazure.visualstudio.com/Antares/_workitems/edit/37181649