fix(security): prevent MagicByteValidator type poisoning from dropping Slack images#345
Merged
Merged
Conversation
…y dropping Slack images MagicByteValidator's Lazy<IContentInspector> static field captured a delegate referencing MimeDetective types. If MimeDetective failed to load during the static constructor (transient resource contention, assembly load timing), .NET cached the TypeInitializationException permanently, making ALL image validation fail for the process lifetime. The MagicByteContentScanner then converted these crashes into "file rejected" results, and SlackThreadBindingActor silently dropped the images — users sent images that the LLM never received. Changes: - Move MimeDetective inspector out of static field initialization into a lazily-initialized method (GetOrCreateInspector) with try-catch, so a MimeDetective failure can't poison the type. Core magic byte validation (PNG/JPEG/GIF/WebP headers, executable detection) uses direct byte comparison and doesn't need MimeDetective at all. - Distinguish scanner failures from policy rejections in SlackThreadBindingActor: ScanFailure errors now allow the file through with an error log, instead of silently dropping valid images. - Add integration test with production MagicByteContentScanner to catch scanner-related regressions. - Add integration test verifying broken scanners don't silently drop images. - Add unit test verifying core validation works even when MimeDetective is unavailable.
…Validator Assign null to s_inspector in the catch block and catch Exception explicitly to satisfy slopwatch's empty catch block detection.
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
1 participant
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
MagicByteValidator'sLazy<IContentInspector>static field referenced MimeDetective types. A transient loading failure during the static constructor permanently poisoned the type via cachedTypeInitializationException, causing ALL image content scans to fail silently for the process lifetime.SlackThreadBindingActornow distinguishes scanner crashes (ContentScanError.ScanFailure) from policy rejections — scanner failures allow the file through with an error log instead of silently dropping valid images.MagicByteContentScannerand aFailingContentScannerto prevent regressions. All 216 tests pass.Test plan
dotnet test src/Netclaw.Security.Tests/— 49 tests pass (including new resilience test)dotnet test src/Netclaw.Actors.Tests/ --filter SlackFileFlow— 12 tests pass (including 2 new integration tests)dotnet test— full suite 216/216 greendotnet slopwatch analyze— 0 violations