Suppress false positive CodeQL warning on LocalOrchestrationService#1348
Suppress false positive CodeQL warning on LocalOrchestrationService#1348
Conversation
…alization security in in-proc testing context Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
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.CodeAnalysisand[SuppressMessage]attributes for CA2326/CA2327 on theStateJsonSettingsfield. - 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.
| // 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.")] |
There was a problem hiding this comment.
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.
| // 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 }; | ||
|
|
There was a problem hiding this comment.
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.
| // 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(), | |
| }; |
Resolves https://msazure.visualstudio.com/Antares/_workitems/edit/37181655