Skip to content

Enforce safe JSON deserialization in ServiceFabric - #1386

Merged
Naiyuan Tian (nytian) merged 2 commits into
mainfrom
nytian/fabric-security
Aug 20, 2026
Merged

Enforce safe JSON deserialization in ServiceFabric#1386
Naiyuan Tian (nytian) merged 2 commits into
mainfrom
nytian/fabric-security

Conversation

@nytian

Copy link
Copy Markdown
Contributor

Ensure the Service Fabric JSON formatter always uses the allowed-types binder. This prevents untrusted requests from deserializing arbitrary .NET types through type metadata. Tests are added.

CopilotAI lite review requested due to automatic review settings August 14, 2026 15:46

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the Azure Service Fabric proxy’s JSON deserialization by ensuring a serialization binder is always applied when TypeNameHandling.All is enabled, preventing untrusted requests from deserializing arbitrary .NET types via $type metadata.

Changes:

  • Default Startup to an AllowedTypesSerializationBinder when no binder is provided, and always assign a binder to the JSON formatter settings.
  • Change FabricOrchestrationProviderSettings.JsonSerializationBinder so null restores the default allowed-types binder instead of disabling restrictions.
  • Update the corresponding binder-setting unit test expectation (but currently in a non-build/test project path).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

FileDescription
Test/DurableTask.AzureServiceFabric.Tests/AllowedTypesSerializationBinderTests.csUpdates test to expect null binder assignment to restore the allowed-types binder (currently under Test/, not the active test/ project).
src/DurableTask.AzureServiceFabric/Service/Startup.csEnsures a non-null binder is always applied to JsonFormatter.SerializerSettings.SerializationBinder.
src/DurableTask.AzureServiceFabric/FabricOrchestrationProviderSettings.csMakes JsonSerializationBinder non-null by default and treats null as “reset to default binder”.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

CopilotAI review requested due to automatic review settings August 20, 2026 16:32

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Test/DurableTask.AzureServiceFabric.Tests/AllowedTypesSerializationBinderTests.cs:230

  • This test change is under Test/DurableTask.AzureServiceFabric.Tests, but the active test project referenced by DurableTask.sln is test/DurableTask.AzureServiceFabric.Tests (lowercase). There is no .csproj under Test/, so this test won’t run in CI and the new binder behavior won’t actually be covered.
 [TestMethod]
public void Settings_NullBinderRestoresAllowedTypesBinder()
{
var providerSettings = new FabricOrchestrationProviderSettings();
providerSettings.JsonSerializationBinder = null;
Assert.IsInstanceOfType(providerSettings.JsonSerializationBinder, typeof(AllowedTypesSerializationBinder));

src/DurableTask.AzureServiceFabric/FabricOrchestrationProviderSettings.cs:78

  • The public behavior of JsonSerializationBinder = null is now “restore default binder”, but the Service Fabric provider docs still state that setting it to null disables type restrictions. This will mislead users and may cause confusion during upgrades.
 public ISerializationBinder JsonSerializationBinder
{
get => this.jsonSerializationBinder;
set => this.jsonSerializationBinder = value ?? new AllowedTypesSerializationBinder();
}

@bachuvVarshitha Bachu (bachuv) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but please add detailed notes about this behavior change in the release notes. Thanks!

@nytian
Naiyuan Tian (nytian) merged commit 65e39af into mainAug 20, 2026
48 checks passed
@nytian
Naiyuan Tian (nytian) deleted the nytian/fabric-security branch August 20, 2026 21:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@nytian@bachuv