Skip to content

Suppress false positive CodeQL warning on LocalOrchestrationService#1348

Open
AnatoliB wants to merge 1 commit intomainfrom
anatolib/codeql-fix-37181655-2
Open

Suppress false positive CodeQL warning on LocalOrchestrationService#1348
AnatoliB wants to merge 1 commit intomainfrom
anatolib/codeql-fix-37181655-2

Conversation

@AnatoliB
Copy link
Copy Markdown
Collaborator

@AnatoliB AnatoliB commented May 1, 2026

…alization security in in-proc testing context

Co-authored-by: Copilot <copilot@github.com>
Copilot AI review requested due to automatic review settings May 1, 2026 06:24
@AnatoliB AnatoliB marked this pull request as ready for review May 1, 2026 06:24
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the DurableTask emulator’s in-proc orchestration state JSON serialization settings to suppress security analyzer warnings related to Json.NET TypeNameHandling.

Changes:

  • Adds System.Diagnostics.CodeAnalysis and [SuppressMessage] attributes for CA2326/CA2327 on the StateJsonSettings field.
  • Adds an inline comment explaining why the unsafe-deserialization warnings are considered a false positive for this emulator scenario.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +59 to +72
// CodeQL [SM02211] False positive. LocalOrchestrationService is a fully in-proc emulator intended for
// testing only. The bytes produced by SerializeOrchestrationRuntimeState are stored in the in-memory
// sessionState dictionary on this same instance and never cross a trust boundary (no disk, network,
// or cross-process surface). The polymorphic surface is bounded to HistoryEvent subclasses defined in
// DurableTask.Core; customer payload fields (Input/Output/Result/Reason/Details) are typed as string
// and are opaque to this serializer. No attacker-controlled JSON can reach DeserializeOrchestrationRuntimeState.
[SuppressMessage(
"Security",
"CA2326:Do not use TypeNameHandling values other than None",
Justification = "In-proc test-only emulator; serialized bytes never cross a trust boundary. See inline CodeQL suppression comment above.")]
[SuppressMessage(
"Security",
"CA2327:Do not use insecure deserializer settings",
Justification = "In-proc test-only emulator; serialized bytes never cross a trust boundary. See inline CodeQL suppression comment above.")]
Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

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

This change adds suppressions for CA2326/CA2327 (.NET analyzers), but it likely won’t suppress the CodeQL alert referenced in the comment/title. The repo’s CodeQL workflow runs github/codeql-action with default C# queries, which typically requires a CodeQL suppression comment (e.g., codeql[...] / lgtm[...]) or dismissing the alert in the Code Scanning UI. Consider updating this to use the correct CodeQL suppression mechanism for the specific query id that fired, otherwise the alert may remain open even after this PR merges.

Copilot uses AI. Check for mistakes.
Comment on lines +59 to 74
// CodeQL [SM02211] False positive. LocalOrchestrationService is a fully in-proc emulator intended for
// testing only. The bytes produced by SerializeOrchestrationRuntimeState are stored in the in-memory
// sessionState dictionary on this same instance and never cross a trust boundary (no disk, network,
// or cross-process surface). The polymorphic surface is bounded to HistoryEvent subclasses defined in
// DurableTask.Core; customer payload fields (Input/Output/Result/Reason/Details) are typed as string
// and are opaque to this serializer. No attacker-controlled JSON can reach DeserializeOrchestrationRuntimeState.
[SuppressMessage(
"Security",
"CA2326:Do not use TypeNameHandling values other than None",
Justification = "In-proc test-only emulator; serialized bytes never cross a trust boundary. See inline CodeQL suppression comment above.")]
[SuppressMessage(
"Security",
"CA2327:Do not use insecure deserializer settings",
Justification = "In-proc test-only emulator; serialized bytes never cross a trust boundary. See inline CodeQL suppression comment above.")]
static readonly JsonSerializerSettings StateJsonSettings = new JsonSerializerSettings { TypeNameHandling = TypeNameHandling.Auto };

Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

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

Suppressing CA2326/CA2327 here hides a real unsafe-deserialization pattern (TypeNameHandling != None without a binder). Elsewhere in this repo, Json.NET settings that use TypeNameHandling are paired with a SerializationBinder/Binder to restrict allowed types (e.g., PackageUpgradeSerializationBinder / TypeNameSerializationBinder). Consider adding a binder that whitelists the expected DurableTask.Core HistoryEvent types and then removing these suppressions; this would address both the analyzer warning and reduce risk if this emulator is ever used with externally-influenced state bytes.

Suggested change
// CodeQL [SM02211] False positive. LocalOrchestrationService is a fully in-proc emulator intended for
// testing only. The bytes produced by SerializeOrchestrationRuntimeState are stored in the in-memory
// sessionState dictionary on this same instance and never cross a trust boundary (no disk, network,
// or cross-process surface). The polymorphic surface is bounded to HistoryEvent subclasses defined in
// DurableTask.Core; customer payload fields (Input/Output/Result/Reason/Details) are typed as string
// and are opaque to this serializer. No attacker-controlled JSON can reach DeserializeOrchestrationRuntimeState.
[SuppressMessage(
"Security",
"CA2326:Do not use TypeNameHandling values other than None",
Justification = "In-proc test-only emulator; serialized bytes never cross a trust boundary. See inline CodeQL suppression comment above.")]
[SuppressMessage(
"Security",
"CA2327:Do not use insecure deserializer settings",
Justification = "In-proc test-only emulator; serialized bytes never cross a trust boundary. See inline CodeQL suppression comment above.")]
static readonly JsonSerializerSettings StateJsonSettings = new JsonSerializerSettings { TypeNameHandling = TypeNameHandling.Auto };
sealed class HistoryEventSerializationBinder : Newtonsoft.Json.Serialization.ISerializationBinder
{
static readonly Dictionary<string, Type> AllowedTypes = typeof(HistoryEvent)
.Assembly
.GetTypes()
.Where(t => typeof(HistoryEvent).IsAssignableFrom(t) && !t.IsAbstract)
.ToDictionary(t => t.FullName, t => t, StringComparer.Ordinal);
public Type BindToType(string assemblyName, string typeName)
{
if (typeName != null && AllowedTypes.TryGetValue(typeName, out Type resolvedType))
{
return resolvedType;
}
throw new JsonSerializationException($"Type '{typeName}' is not allowed for orchestration state deserialization.");
}
public void BindToName(Type serializedType, out string assemblyName, out string typeName)
{
if (serializedType == null)
{
throw new ArgumentNullException(nameof(serializedType));
}
if (!typeof(HistoryEvent).IsAssignableFrom(serializedType) || serializedType.IsAbstract || !AllowedTypes.ContainsKey(serializedType.FullName))
{
throw new JsonSerializationException($"Type '{serializedType.FullName}' is not allowed for orchestration state serialization.");
}
assemblyName = serializedType.Assembly.FullName;
typeName = serializedType.FullName;
}
}
static readonly JsonSerializerSettings StateJsonSettings = new JsonSerializerSettings
{
TypeNameHandling = TypeNameHandling.Auto,
SerializationBinder = new HistoryEventSerializationBinder(),
};

Copilot uses AI. Check for mistakes.
Sign up for free to 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.

2 participants