Prevent assembly loading from TaskFailureDetails.ErrorType values#735
Open
AnatoliB wants to merge 1 commit into
Open
Prevent assembly loading from TaskFailureDetails.ErrorType values#735AnatoliB wants to merge 1 commit into
AnatoliB wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens TaskFailureDetails.IsCausedBy by changing error type resolution to only search already-loaded assemblies (and by stripping assembly qualifiers / generic payloads), preventing attacker-controlled ErrorType values from triggering on-demand assembly loading.
Changes:
- Updated
TaskFailureDetails.IsCausedBy(Type)to resolveErrorTypevia already-loaded assemblies only, avoidingType.GetType(string)and stripping potentially dangerous qualifiers/payloads. - Added unit tests covering resolution semantics plus defense-in-depth scenarios ensuring
AssemblyResolveis not triggered by poisonedErrorTypevalues.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Abstractions/TaskFailureDetails.cs | Reworks exception-type resolution to avoid assembly loading; adds safe type-name extraction and a loaded-assembly scan fallback. |
| test/Abstractions.Tests/TaskFailureDetailsTests.cs | Adds coverage for IsCausedBy semantics and tests ensuring poisoned type strings don’t trigger assembly resolution. |
Comment on lines
+109
to
+113
| List<Type> matches = AppDomain.CurrentDomain.GetAssemblies() | ||
| .Select(a => a.GetType(safeTypeName, throwOnError: false)) | ||
| .Where(t => t is not null) | ||
| .Distinct() | ||
| .ToList()!; |
Comment on lines
+143
to
+147
| List<string> resolveRequests = new(); | ||
| ResolveEventHandler handler = (_, args) => | ||
| { | ||
| resolveRequests.Add(args.Name); | ||
| return null; |
Comment on lines
+185
to
+189
| List<string> resolveRequests = new(); | ||
| ResolveEventHandler handler = (_, args) => | ||
| { | ||
| resolveRequests.Add(args.Name); | ||
| return null; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What changed?
TaskFailureDetails.IsCausedByto resolveErrorTypeagainst already-loaded assemblies only, instead of usingType.GetTypewhich can trigger on-demand assembly loading.Why is this change needed?
Type.GetType(string)internally callsAssembly.Loadwhen the input contains an assembly-qualified name. SinceErrorTypeis deserialized from the durable task hub store, this means anErrorTypevalue written by a different application version or workflow could cause unintended assembly loading at the pointIsCausedByis called. Restricting resolution to already-loaded assemblies makes the behavior more predictable and avoids unexpected side effects.Issues / work items
Project checklist
MyException<string>) stored inErrorTypewill no longer resolve, causingIsCausedByto returnfalse. Non-generic exception types are unaffected.ErrorTypedirectly:details.ErrorType.StartsWith("MyNamespace.MyException\1")`.AI-assisted code disclosure (required)
Was an AI tool used? (select one)
If AI was used:
TaskFailureDetailsTests.csAI verification (required if AI was used):
Testing
Automated tests
Notes for reviewers