fix(security): Remove unused TypeDiscriminatorConverter to eliminate security risk by thomhurst · Pull Request #1929 · thomhurst/ModularPipelines

thomhurst · 2026-01-08T00:49:56Z

Summary

Removes TypeDiscriminatorConverter<T> which contained a security vulnerability
Removes ITypeDiscriminator interface as it's no longer used
Eliminates the security risk by removing the vulnerable code entirely

Background

The original approach was to add security restrictions (type allowlists) to the TypeDiscriminatorConverter<T>. However, after PR #1895 refactored ModuleResult to use a discriminated union pattern with ModuleResultJsonConverterFactory, nothing in the codebase implements ITypeDiscriminator anymore.

Since the vulnerable code is now dead code (unreachable), the simplest and safest fix is to remove it entirely rather than adding security restrictions to unused code.

Security Issue Fixed

The TypeDiscriminatorConverter<T> could deserialize any type whose name matched the TypeDiscriminator field in JSON, potentially allowing malicious JSON to instantiate arbitrary types from any loaded assembly. This is a deserialization vulnerability that could lead to remote code execution.

Test Plan

Build succeeds with files removed
No code references these types

Fixes #1910

🤖 Generated with Claude Code

thomhurst · 2026-01-08T00:52:11Z

Summary

Adds assembly validation to TypeDiscriminatorConverter to prevent arbitrary type instantiation from untrusted JSON by restricting deserialization to types from the ModularPipelines assembly only.

Critical Issues

1. TypeDiscriminatorConverter appears to be unused/dead code

The codebase currently uses ModuleResultJsonConverter<T> (src/ModularPipelines/Models/ModuleResult.cs:379-522) for JSON serialization, not TypeDiscriminatorConverter<T>. I found:

No types currently implement ITypeDiscriminator
No references to TypeDiscriminatorConverter outside its own file
ModuleResult<T> uses [JsonConverter(typeof(ModuleResultJsonConverterFactory))] (line 30)

Questions:

Is TypeDiscriminatorConverter actually used anywhere, or should it be removed entirely?
If it's intended for future use, why not use .NET 7+ built-in polymorphic serialization instead (as suggested in issue Security: TypeDiscriminatorConverter allows arbitrary type instantiation #1910 Option 3)?

2. Security fix may be incomplete

While assembly restriction is better than nothing, the issue described Option 1: Registered Type Map as the recommended approach, but this PR implements Option 2: Assembly Restriction, which is less secure because:

Any type from the ModularPipelines assembly can still be instantiated
No explicit allowlist of permitted types
Still vulnerable to malicious JSON using unexpected ModularPipelines types

Consider using an explicit allowlist instead:

private static readonly HashSet<Type> AllowedTypes = new()
{
    // Explicitly list allowed types here
};

if (!AllowedTypes.Contains(type))
{
    throw new JsonException(...);
}

3. Test implementation has issues

src/ModularPipelines.UnitTests/Serialization/TypeDiscriminatorConverterSecurityTests.cs:

Line 159: Creates a TestableTypeDiscriminatorConverter that duplicates the entire implementation instead of testing the actual class
Line 204: Returns null! which could hide bugs
The test doesn't actually verify the real TypeDiscriminatorConverter behavior - it tests a copy

Since TypeDiscriminatorConverter is internal, consider using [assembly: InternalsVisibleTo("ModularPipelines.UnitTests")] in src/ModularPipelines/AssemblyInfo.cs to test the real implementation.

Suggestions

Consider deleting TypeDiscriminatorConverter entirely

If it's not used and ModuleResult<T> already has secure custom converters, removing dead code eliminates the vulnerability completely. Check:

git grep -n "TypeDiscriminatorConverter" --and --not -e "namespace" -e "class TypeDiscriminatorConverter"

If there are no hits, the class is unused.

Verdict

⚠️ REQUEST CHANGES - Critical question about whether this code is actually used must be answered before merging. If unused, delete it. If used, implement explicit allowlist (Option 1) instead of assembly restriction (Option 2).

Copilot

Pull request overview

This pull request implements a security fix to prevent arbitrary type instantiation through JSON deserialization in the TypeDiscriminatorConverter<T>. The fix restricts deserialization to types from the ModularPipelines assembly only, preventing potential malicious JSON payloads from instantiating arbitrary types from any loaded assembly.

Adds assembly validation to reject types from unauthorized assemblies
Provides clear error messages for security violations
Includes comprehensive security tests validating the new restrictions
Adds a design document for future ModuleResult refactoring work (issue #1869)

Reviewed changes

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

File	Description
src/ModularPipelines/Serialization/TypeDiscriminatorConverter.cs	Adds assembly validation check to restrict deserialization to ModularPipelines assembly types only, with clear security-focused error messages
test/ModularPipelines.UnitTests/Serialization/TypeDiscriminatorConverterSecurityTests.cs	Adds comprehensive security tests verifying unauthorized assemblies are rejected and authorized types are allowed, though with a duplicated test implementation
docs/plans/2026-01-07-module-result-discriminated-union.md	Adds design documentation for a future discriminated union refactoring (unrelated to this security fix)

Copilot · 2026-01-08T01:00:18Z

docs/plans/2026-01-07-module-result-discriminated-union.md

+# ModuleResult Discriminated Union Design
+
+**Issue:** #1869
+**Date:** 2026-01-07
+**Status:** Approved
+
+## Problem
+
+The current `ModuleResult<T>.Value` property throws exceptions when accessed on failed or skipped results:
+
+```csharp
+var result = await myModule;
+var value = result.Value; // Throws ModuleFailedException or ModuleSkippedException!
+```
+
+This leads to:
+- Implicit exceptions and poor control flow
+- No compile-time exhaustiveness checking
+- Awkward try-catch patterns around property access
+
+## Solution
+
+Replace with a discriminated union pattern using C# records:
+
+```csharp
+public abstract record ModuleResult<T> : IModuleResult
+{
+    // Metadata (available on all outcomes)
+    public required string ModuleName { get; init; }
+    public required TimeSpan ModuleDuration { get; init; }
+    public required DateTimeOffset ModuleStart { get; init; }
+    public required DateTimeOffset ModuleEnd { get; init; }
+    public required Status ModuleStatus { get; init; }
+
+    // Quick checks
+    public bool IsSuccess => this is Success;
+    public bool IsFailure => this is Failure;
+    public bool IsSkipped => this is Skipped;
+
+    // Safe accessors (no exceptions)
+    public T? ValueOrDefault => this is Success s ? s.Value : default;
+    public Exception? ExceptionOrDefault => this is Failure f ? f.Exception : null;
+    public SkipDecision? SkipDecisionOrDefault => this is Skipped sk ? sk.Decision : null;
+
+    // Computed for compatibility
+    public ModuleResultType ModuleResultType => this switch
+    {
+        Success => ModuleResultType.Success,
+        Failure => ModuleResultType.Failure,
+        Skipped => ModuleResultType.Skipped,
+        _ => throw new InvalidOperationException()
+    };
+
+    // Discriminated variants
+    public sealed record Success(T Value) : ModuleResult<T>;
+    public sealed record Failure(Exception Exception) : ModuleResult<T>;
+    public sealed record Skipped(SkipDecision Decision) : ModuleResult<T>;
+
+    // Prevent external inheritance
+    private ModuleResult() { }
+}
+```
+
+## Usage Examples
+
+### Pattern Matching (Recommended)
+
+```csharp
+var result = await buildModule;
+
+switch (result)
+{
+    case ModuleResult<string>.Success { Value: var value }:
+        Console.WriteLine($"Got: {value}");
+        break;
+    case ModuleResult<string>.Failure { Exception: var ex }:
+        Console.WriteLine($"Failed: {ex.Message}");
+        break;
+    case ModuleResult<string>.Skipped { Decision.Reason: var reason }:
+        Console.WriteLine($"Skipped: {reason}");
+        break;
+}
+```
+
+### Match Helper
+
+```csharp
+string message = result.Match(
+    onSuccess: value => $"Got: {value}",
+    onFailure: ex => $"Error: {ex.Message}",
+    onSkipped: skip => $"Skipped: {skip.Reason}"
+);
+```
+
+### Quick Checks
+
+```csharp
+if (result.IsSuccess)
+{
+    DoSomething(result.ValueOrDefault!);
+}
+```
+
+## Helper Methods
+
+```csharp
+// Match - transform to single type
+public TResult Match<TResult>(
+    Func<T, TResult> onSuccess,
+    Func<Exception, TResult> onFailure,
+    Func<SkipDecision, TResult> onSkipped);
+
+// Switch - side effects
+public void Switch(
+    Action<T> onSuccess,
+    Action<Exception> onFailure,
+    Action<SkipDecision> onSkipped);
+```
+
+## JSON Serialization
+
+Uses .NET 7+ polymorphic serialization:
+
+```csharp
+[JsonDerivedType(typeof(Success), "Success")]
+[JsonDerivedType(typeof(Failure), "Failure")]
+[JsonDerivedType(typeof(Skipped), "Skipped")]
+public abstract record ModuleResult<T> : IModuleResult
+```
+
+Output:
+```json
+{
+  "$type": "Success",
+  "Value": "build output",
+  "ModuleName": "BuildModule",
+  "ModuleDuration": "00:01:23"
+}
+```
+
+## Non-Generic Interface
+
+For type-erased scenarios (registries, summaries):
+
+```csharp
+public interface IModuleResult
+{
+    string ModuleName { get; }
+    TimeSpan ModuleDuration { get; }
+    DateTimeOffset ModuleStart { get; }
+    DateTimeOffset ModuleEnd { get; }
+    Status ModuleStatus { get; }
+    ModuleResultType ModuleResultType { get; }
+
+    object? ValueOrDefault { get; }
+    Exception? ExceptionOrDefault { get; }
+    SkipDecision? SkipDecisionOrDefault { get; }
+}
+```
+
+## Factory Methods
+
+Internal factory methods for engine use:
+
+```csharp
+internal static Success CreateSuccess(T value, ModuleExecutionContext ctx);
+internal static Failure CreateFailure(Exception exception, ModuleExecutionContext ctx);
+internal static Skipped CreateSkipped(SkipDecision decision, ModuleExecutionContext ctx);
+```
+
+## Breaking Changes
+
+### Removed
+- `ModuleResult<T>.Value` property (the throwing one)
+- `ModuleFailedException` class
+- `ModuleSkippedException` class
+- `SkippedModuleResult<T>` subclass
+- `TimedOutModuleResult<T>` subclass
+
+### Preserved
+- `ModuleResultType` property (now computed)
+- `IModuleResult` interface
+- All metadata property names
+- JSON serialization support
+
+## Target
+
+.NET 10 only - no backward compatibility shims needed.
+
+## Files to Modify
+
+1. `src/ModularPipelines/Models/ModuleResult.cs` - Main implementation
+2. `src/ModularPipelines/Models/IModuleResult.cs` - Update interface
+3. `src/ModularPipelines/Engine/Execution/ModuleResultFactory.cs` - Update factory
+4. `src/ModularPipelines/Exceptions/` - Remove ModuleFailedException, ModuleSkippedException
+5. `src/ModularPipelines/Models/SkippedModuleResult.cs` - Delete
+6. `src/ModularPipelines/Models/TimedOutModuleResult.cs` - Delete
+7. All files accessing `.Value` - Update to pattern matching


This design document appears to be unrelated to the security fix described in the PR title and description. The PR is about fixing issue #1910 (security vulnerability in TypeDiscriminatorConverter), but this document discusses issue #1869 (ModuleResult discriminated union refactoring).

If this document was added intentionally as part of planning for future work, it should be in a separate PR to keep the security fix focused and easier to review. If it was added accidentally, it should be removed from this PR.

Copilot · 2026-01-08T01:00:18Z

test/ModularPipelines.UnitTests/Serialization/TypeDiscriminatorConverterSecurityTests.cs

+    /// A testable version of the TypeDiscriminatorConverter that exposes the Read method.
+    /// This is needed because the original converter is internal.
+    /// </summary>
+    private class TestableTypeDiscriminatorConverter : JsonConverter<ModuleResult>
+    {
+        private static readonly Assembly AllowedAssembly = typeof(ModuleResult).Assembly;
+
+        public override ModuleResult Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
+        {
+            if (reader.TokenType != JsonTokenType.StartObject)
+            {
+                throw new JsonException(
+                    $"Expected JSON object start token but found '{reader.TokenType}' when deserializing type '{typeToConvert.FullName}'.");
+            }
+
+            using var jsonDocument = JsonDocument.ParseValue(ref reader);
+
+            if (!jsonDocument.RootElement.TryGetProperty(nameof(ITypeDiscriminator.TypeDiscriminator), out var typeProperty))
+            {
+                throw new JsonException(
+                    $"Missing required property '{nameof(ITypeDiscriminator.TypeDiscriminator)}' when deserializing type '{typeToConvert.FullName}'. " +
+                    $"Ensure the JSON contains a type discriminator property.");
+            }
+
+            var typeDiscriminatorValue = typeProperty.GetString();
+            var type = Type.GetType(typeDiscriminatorValue!);
+
+            if (type == null)
+            {
+                throw new JsonException(
+                    $"Could not resolve type from discriminator value '{typeDiscriminatorValue}' when deserializing type '{typeToConvert.FullName}'. " +
+                    $"Ensure the type exists and is properly qualified.");
+            }
+
+            // Security: Only allow types from the ModularPipelines assembly to prevent arbitrary type instantiation
+            if (type.Assembly != AllowedAssembly)
+            {
+                throw new JsonException(
+                    $"Type '{typeDiscriminatorValue}' from unauthorized assembly '{type.Assembly.GetName().Name}'. " +
+                    $"Only types from the ModularPipelines assembly are allowed for security reasons.");
+            }
+
+            // Note: We stop here for testing purposes - the actual deserialization is tested elsewhere
+            // Return null to indicate the security check passed
+            return null!;
+        }
+
+        public override void Write(Utf8JsonWriter writer, ModuleResult value, JsonSerializerOptions options)
+        {
+            throw new NotImplementedException("Write not needed for security tests");


The TestableTypeDiscriminatorConverter duplicates most of the logic from the actual TypeDiscriminatorConverter, which creates a maintenance burden. If the actual implementation changes, this test version needs to be updated separately, and they could drift out of sync.

Consider testing the actual TypeDiscriminatorConverter through integration tests with JsonSerializer.Deserialize() instead of creating a duplicate implementation. This would ensure you're testing the real code path and avoid code duplication.

Suggested change

/// A testable version of the TypeDiscriminatorConverter that exposes the Read method.

/// This is needed because the original converter is internal.

/// </summary>

private class TestableTypeDiscriminatorConverter : JsonConverter<ModuleResult>

{

private static readonly Assembly AllowedAssembly = typeof(ModuleResult).Assembly;

public override ModuleResult Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)

{

if (reader.TokenType != JsonTokenType.StartObject)

{

throw new JsonException(

$"Expected JSON object start token but found '{reader.TokenType}' when deserializing type '{typeToConvert.FullName}'.");

}

using var jsonDocument = JsonDocument.ParseValue(ref reader);

if (!jsonDocument.RootElement.TryGetProperty(nameof(ITypeDiscriminator.TypeDiscriminator), out var typeProperty))

{

throw new JsonException(

$"Missing required property '{nameof(ITypeDiscriminator.TypeDiscriminator)}' when deserializing type '{typeToConvert.FullName}'. " +

$"Ensure the JSON contains a type discriminator property.");

}

var typeDiscriminatorValue = typeProperty.GetString();

var type = Type.GetType(typeDiscriminatorValue!);

if (type == null)

{

throw new JsonException(

$"Could not resolve type from discriminator value '{typeDiscriminatorValue}' when deserializing type '{typeToConvert.FullName}'. " +

$"Ensure the type exists and is properly qualified.");

}

// Security: Only allow types from the ModularPipelines assembly to prevent arbitrary type instantiation

if (type.Assembly != AllowedAssembly)

{

throw new JsonException(

$"Type '{typeDiscriminatorValue}' from unauthorized assembly '{type.Assembly.GetName().Name}'. " +

$"Only types from the ModularPipelines assembly are allowed for security reasons.");

}

// Note: We stop here for testing purposes - the actual deserialization is tested elsewhere

// Return null to indicate the security check passed

return null!;

}

public override void Write(Utf8JsonWriter writer, ModuleResult value, JsonSerializerOptions options)

{

throw new NotImplementedException("Write not needed for security tests");

/// A testable wrapper around the real TypeDiscriminatorConverter that exposes its behavior for tests

/// without duplicating the implementation logic.

/// </summary>

private class TestableTypeDiscriminatorConverter : JsonConverter<ModuleResult>

{

private readonly JsonConverter<ModuleResult> _innerConverter = new TypeDiscriminatorConverter();

public override ModuleResult Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)

{

return _innerConverter.Read(ref reader, typeToConvert, options);

}

public override void Write(Utf8JsonWriter writer, ModuleResult value, JsonSerializerOptions options)

{

_innerConverter.Write(writer, value, options);

Copilot · 2026-01-08T01:00:18Z

test/ModularPipelines.UnitTests/Serialization/TypeDiscriminatorConverterSecurityTests.cs

+    [Test]
+    public async Task AllowedModularPipelinesType_IsAccepted()
+    {
+        // Arrange - Create JSON with a type from the ModularPipelines assembly
+        var validJson = """
+            {
+                "TypeDiscriminator": "ModularPipelines.Models.ModuleResult, ModularPipelines",
+                "ModuleName": "TestModule",
+                "ModuleDuration": "00:00:01",
+                "ModuleStart": "2024-01-01T00:00:00+00:00",
+                "ModuleEnd": "2024-01-01T00:00:01+00:00"
+            }
+            """;
+
+        // Act - The converter should not throw for allowed types
+        // Note: We can't fully deserialize without the recursive issue, but we can verify
+        // the security check passes by ensuring it reaches the deserialization step
+        var reader = new Utf8JsonReader(System.Text.Encoding.UTF8.GetBytes(validJson));
+        var converter = new TestableTypeDiscriminatorConverter();
+        var options = new JsonSerializerOptions();
+        reader.Read(); // Move to first token
+
+        // This should NOT throw the security exception - it will attempt to deserialize
+        // which may have its own issues, but the security check should pass
+        JsonException? securityException = null;
+        try
+        {
+            converter.Read(ref reader, typeof(ModuleResult), options);
+        }
+        catch (JsonException ex) when (ex.Message.Contains("unauthorized assembly"))
+        {
+            securityException = ex;
+        }
+        catch
+        {
+            // Other exceptions are expected (e.g., deserialization issues)
+            // The key is that we don't get a security exception
+        }
+
+        await Assert.That(securityException).IsNull();
+    }


The AllowedModularPipelinesType_IsAccepted test doesn't actually verify that deserialization succeeds. It returns null on line 206 and only checks that a security exception wasn't thrown. This doesn't confirm that valid ModularPipelines types can be successfully deserialized.

The test would be more valuable if it either completed the deserialization or was restructured to use the actual TypeDiscriminatorConverter through JsonSerializer to verify end-to-end functionality.

Copilot · 2026-01-08T01:00:19Z

src/ModularPipelines/Serialization/TypeDiscriminatorConverter.cs

+        // Security: Only allow types from the ModularPipelines assembly to prevent arbitrary type instantiation
+        if (type.Assembly != AllowedAssembly)
+        {
+            throw new JsonException(
+                $"Type '{typeDiscriminatorValue}' from unauthorized assembly '{type.Assembly.GetName().Name}'. " +
+                $"Only types from the ModularPipelines assembly are allowed for security reasons.");
+        }


The assembly check uses reference equality (!=) which is correct, but the implementation doesn't consider that derived types in the ModularPipelines assembly might legitimately inherit from types in other assemblies. The current check validates the instantiated type's assembly, not the base type hierarchy.

While this is likely the intended behavior for security purposes, consider adding a comment explaining that only types directly defined in the ModularPipelines assembly are allowed, and that inheritance from external types is explicitly restricted.

…ction Addresses review feedback on PR #1929: - Changed from assembly-level restriction to explicit type allowlist - Only ModuleResult and types deriving from it are now permitted - Added IsTypeAllowed() method for testability - Rewrote tests to use the real TypeDiscriminatorConverter via InternalsVisibleTo - Tests now directly verify IsTypeAllowed() and converter behavior - Removed duplicate test implementation that copied the converter logic This implements Option 1 (Registered Type Map) from issue #1910 which is more secure than Option 2 (Assembly Restriction). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

thomhurst · 2026-01-08T01:11:07Z

Review Comments Addressed

Thank you for the thorough review! I've addressed all the feedback:

1. ✅ TypeDiscriminatorConverter IS used

The converter is used by ModuleResult (line 75 of ModuleResult.cs):

[JsonConverter(typeof(TypeDiscriminatorConverter<ModuleResult>))]
public class ModuleResult : IModuleResult, ITypeDiscriminator

2. ✅ Implemented Option 1 (Explicit Allowlist) instead of Option 2 (Assembly Restriction)

Changed from assembly-level restriction to an explicit type allowlist:

private static readonly HashSet<Type> AllowedTypes = new()
{
    typeof(ModuleResult),
};

The IsTypeAllowed() method now checks:

If the exact type is in the allowlist
If the type derives from an allowed type (handles ModuleResult<T>)

This is more secure because:

Only explicitly permitted types can be instantiated
No other types from the ModularPipelines assembly can be instantiated
Clear documentation of exactly what's allowed

3. ✅ Fixed tests to use real implementation

Removed the duplicate TestableTypeDiscriminatorConverter class
Tests now use the real TypeDiscriminatorConverter<T> via InternalsVisibleTo
Added IsTypeAllowed() as an internal static method for direct testing
14 comprehensive tests covering allowed types, rejected types, and converter behavior

All tests pass.

thomhurst · 2026-01-08T01:12:25Z

Summary

Adds security validation to TypeDiscriminatorConverter to prevent arbitrary type instantiation via JSON deserialization.

Critical Issues

BLOCKING: Security validation applied to wrong converter ⛔

The PR adds security hardening to TypeDiscriminatorConverter<T>, but ModuleResult<T> does not use this converter.

Evidence from src/ModularPipelines/Models/ModuleResult.cs:30:

[JsonConverter(typeof(ModuleResultJsonConverterFactory))]
public abstract record ModuleResult<T> : IModuleResult

The actual deserialization happens in ModuleResultJsonConverter<T> (lines 379-522 of ModuleResult.cs), which:

Uses a hardcoded discriminator field ($type) with only three valid values: "Success", "Failure", "Skipped"
Does NOT call Type.GetType() with untrusted input
Is already safe from arbitrary type instantiation

Impact:

The security tests in TypeDiscriminatorConverterSecurityTests.cs test a code path that is never executed for ModuleResult
The allowlist check at TypeDiscriminatorConverter.cs:77-82 is unreachable for ModuleResult types
If ITypeDiscriminator is used elsewhere in the codebase with TypeDiscriminatorConverter<T>, those use cases ARE protected by this PR

Questions:

What types actually implement ITypeDiscriminator and use TypeDiscriminatorConverter<T>?
Was the intention to add security to ModuleResultJsonConverter<T> instead?
Or is this PR protecting other types that will implement ITypeDiscriminator in the future (per issue Exception-Based Results: Implement discriminated union Result<T, E> pattern #1869)?

Recommendation:

If ModuleResult will implement ITypeDiscriminator as part of Exception-Based Results: Implement discriminated union Result<T, E> pattern #1869, clarify this in the design doc
If TypeDiscriminatorConverter is meant for other types, remove ModuleResult from tests and allowlist to avoid confusion
Verify what currently implements ITypeDiscriminator (appears to be none based on codebase search)

Suggestions

None - need clarification on the critical issue first.

Verdict

⚠️ REQUEST CHANGES - Security fix applied to wrong converter; needs clarification on intended usage

thomhurst · 2026-01-09T22:28:31Z

Response to Latest Review

The reviewer's concerns are based on incorrect information. Let me clarify:

✅ TypeDiscriminatorConverter IS the correct converter

Evidence from current codebase:

$ grep -n "JsonConverter" src/ModularPipelines/Models/ModuleResult.cs
75:[JsonConverter(typeof(TypeDiscriminatorConverter<ModuleResult>))]

Line 75-76 of ModuleResult.cs:

[JsonConverter(typeof(TypeDiscriminatorConverter<ModuleResult>))]
public class ModuleResult : IModuleResult, ITypeDiscriminator

✅ ModuleResultJsonConverterFactory does NOT exist

$ grep -rn "ModuleResultJsonConverter" src/ --include="*.cs"
(no results)

There is no ModuleResultJsonConverterFactory in the codebase. The reviewer may have been looking at a different project or an outdated version.

✅ ITypeDiscriminator is implemented by ModuleResult

$ grep -rn "ITypeDiscriminator" src/ModularPipelines/ --include="*.cs" | grep -v "interface"
src/ModularPipelines/Models/ModuleResult.cs:76:public class ModuleResult : IModuleResult, ITypeDiscriminator

Summary

The security fix is applied to the correct converter:

ModuleResult uses TypeDiscriminatorConverter<ModuleResult> (line 75)
ModuleResult implements ITypeDiscriminator (line 76)
The converter is used for polymorphic JSON deserialization of module results
The security fix prevents arbitrary type instantiation via the TypeDiscriminator property

The PR correctly addresses the security vulnerability described in issue #1910.

…security risk The TypeDiscriminatorConverter allowed arbitrary type instantiation from JSON type discriminator values, which posed a security vulnerability. Since the ModuleResult refactoring (PR #1895), nothing implements ITypeDiscriminator anymore, making this code dead. Rather than adding security restrictions to unused code, this commit removes the dead code entirely: - Removes TypeDiscriminatorConverter<T> - Removes ITypeDiscriminator interface This eliminates the security vulnerability by removing the vulnerable code path. Closes #1910 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

thomhurst · 2026-01-09T22:37:44Z

Updated Approach

After rebasing onto main, I discovered that PR #1895 (discriminated union pattern for ModuleResult) completely changed the serialization approach:

ModuleResult is now ModuleResult<T> - an abstract record using ModuleResultJsonConverterFactory
Nothing implements ITypeDiscriminator anymore
TypeDiscriminatorConverter<T> is now dead code

Rather than adding security restrictions to dead code, I've updated this PR to simply remove the vulnerable code entirely:

Deleted TypeDiscriminatorConverter.cs
Deleted ITypeDiscriminator.cs

This is the safest approach - you can't exploit code that doesn't exist.

thomhurst · 2026-01-09T22:38:39Z

Summary

Removes unused and vulnerable TypeDiscriminatorConverter to eliminate a critical security risk that allowed arbitrary type instantiation during JSON deserialization.

Critical Issues

None found ✅

Analysis

The PR correctly removes dead code that contained a serious security vulnerability. Verification confirms:

No Usage: Searched the codebase - neither ITypeDiscriminator nor TypeDiscriminatorConverter are used anywhere except in the deleted files themselves
Superseded by Safe Implementation: ModuleResult now uses ModuleResultJsonConverterFactory (src/ModularPipelines/Models/ModuleResult.cs:360-477) which implements a safe discriminated union pattern with hardcoded discriminator values ("Success", "Failure", "Skipped") instead of using Type.GetType() with untrusted input
Security Risk Eliminated: The vulnerable code allowed arbitrary type instantiation via JSON, which could lead to remote code execution. The current implementation only deserializes known, safe types via a switch statement.

This is the correct solution - removing unused vulnerable code is better than hardening code that isn't needed.

Verdict

✅ APPROVE - No critical issues

Copilot AI review requested due to automatic review settings January 8, 2026 00:49

Copilot started reviewing on behalf of thomhurst January 8, 2026 00:50 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

thomhurst had a problem deploying to Pull Requests January 8, 2026 01:10 — with GitHub Actions Failure

thomhurst force-pushed the feature/1910-security-type-discriminator branch from a9bbe21 to a4afeed Compare January 9, 2026 22:37

thomhurst temporarily deployed to Pull Requests January 9, 2026 22:37 — with GitHub Actions Inactive

thomhurst changed the title ~~fix(security): Restrict TypeDiscriminatorConverter to ModularPipelines assembly types~~ fix(security): Remove unused TypeDiscriminatorConverter to eliminate security risk Jan 9, 2026

thomhurst merged commit e82f81a into main Jan 9, 2026
12 checks passed

thomhurst deleted the feature/1910-security-type-discriminator branch January 9, 2026 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(security): Remove unused TypeDiscriminatorConverter to eliminate security risk#1929

fix(security): Remove unused TypeDiscriminatorConverter to eliminate security risk#1929
thomhurst merged 1 commit intomainfrom
feature/1910-security-type-discriminator

thomhurst commented Jan 8, 2026 •

edited

Loading

Uh oh!

thomhurst commented Jan 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

thomhurst commented Jan 8, 2026

Uh oh!

thomhurst commented Jan 8, 2026

Uh oh!

thomhurst commented Jan 9, 2026

Uh oh!

thomhurst commented Jan 9, 2026

Uh oh!

thomhurst commented Jan 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

thomhurst commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background

Security Issue Fixed

Test Plan

Uh oh!

thomhurst commented Jan 8, 2026

Summary

Critical Issues

Suggestions

Verdict

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

thomhurst commented Jan 8, 2026

Review Comments Addressed

1. ✅ TypeDiscriminatorConverter IS used

2. ✅ Implemented Option 1 (Explicit Allowlist) instead of Option 2 (Assembly Restriction)

3. ✅ Fixed tests to use real implementation

Uh oh!

thomhurst commented Jan 8, 2026

Summary

Critical Issues

Suggestions

Verdict

Uh oh!

thomhurst commented Jan 9, 2026

Response to Latest Review

✅ TypeDiscriminatorConverter IS the correct converter

✅ ModuleResultJsonConverterFactory does NOT exist

✅ ITypeDiscriminator is implemented by ModuleResult

Summary

Uh oh!

thomhurst commented Jan 9, 2026

Updated Approach

Uh oh!

thomhurst commented Jan 9, 2026

Summary

Critical Issues

Analysis

Verdict

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thomhurst commented Jan 8, 2026 •

edited

Loading