feat(media): unify file type classification - #1245
Merged
Aaronontheweb merged 16 commits intoMay 31, 2026
Merged
Conversation
Aaronontheweb
added
.NET
The scan -> require-verified-MIME -> re-gate-on-verified-category -> inbox-write flow was copy-pasted verbatim across Slack, Discord, and Mattermost binding actors. Only the trivial formatting tail had been shared; the security-sensitive sequence was triplicated. Extract it into AttachmentIngressPipeline.IngestAsync in Netclaw.Channels. Each actor now supplies only what differs: the file descriptor, the download closure (Slack authed vs Discord/Mattermost streaming), and the platform pre-download trust gate. Removes the three private AttachmentIngestResult records, TryDeleteTemp, and FormatBytes copies in favor of the shared AttachmentIngestOutcome. Net -527 lines across the three actors collapsed into one 250-line source of truth. Normalizes Mattermost rejection copy to the em-dash form used by the other channels (no test asserts on that segment).
…paths Delete members with zero production callers: - ContentScanResult.Allowed() (no-arg) — a latent trap: produces an allowed result with no VerifiedMimeType, which every ingest path now rejects. - DeclaredMimeType.Canonical, MimeType.IsImage/IsText instance props. - AttachmentCategories shim (production uses MimeTypeCatalog.GetCategory). - ChatMessageConverter.MimeToModality / MimeToExtension (test-only wrappers). Re-point the orphaned tests at the real production entry points (MimeTypeCatalog.GetCategory / .ExtensionFor, SessionMediaStore.TryGetMediaModality) so the suite exercises shipping code instead of test-only seams.
…y gate The thread-history cache-hit path (TryGetExistingFile) returned the attachment using the UNVERIFIED declared MIME and provisional category, with no content scan — diverging from the fresh-download path that every other ingress now enforces. A PNG that the transport mislabeled as application/octet-stream was served to the model as octet-stream on replay (GetMediaKind=Unknown → not an image, and a hard throw if it reached the OpenAI-compatible provider). Extract the scan -> verified-MIME -> verified-category gate into a shared HistoricalAttachmentIngress.ScanAndVerifyAsync and run BOTH the cache-hit and download paths through it, so a cache hit re-scans the on-disk file and serves the verified MIME. This removes ~120 lines of triplicated scan/verify/gate logic across the Slack/Discord/Mattermost fetchers and the duplicated reject-note format; each channel keeps only its genuinely-different download + URL-trust. Adds HistoricalAttachmentIngressTests covering octet-stream PNG -> verified image/png, executable-spoofed-as-png rejection, and verified-category gating.
…e html web_fetch's IsBinaryContentType was narrowed from prefix matching to catalog-only lookups, so image/audio/video subtypes absent from the catalog (image/avif, image/heic, image/svg+xml, video/x-flv, audio/aac) were decoded as UTF-8 text and the saved file corrupted. Restore the media-family prefix check on top of the catalog lookup; truly-unknown application/* types still fall through to text as before. Add (.html|.htm, text/plain) -> text/html to the extension-override table so an HTML file the transport mislabels as text/plain is promoted to a supported type instead of being rejected with MimeTypeMismatch (its .html extension is not in the text/plain rule's extension set). Regression tests added for both.
MagicByteValidator kept its own per-MIME extension table (Exts(...)) that duplicated MimeTypeCatalog's extension data — two sources of truth that could silently drift (the class of channel/component-specific bug we're eliminating). Collapse the rule table to MIME -> byte-signature matcher only (the one piece the security layer legitimately owns; the matchers can't live in the dependency-free Netclaw.Media). Extension membership now comes from MimeTypeCatalog.ExtensionMatches, so extensions are defined once. Expose MagicByteValidator.SupportedMimeTypes and assert it equals MimeTypeCatalog.GetNativeSignatureValidatedMimeTypes() so a matcher added without a catalog entry (or vice versa) fails CI.
default(MimeType) / default(DeclaredMimeType) / default(VerifiedMimeType) / default(FileExtension) bypass the constructor, leaving .Value null even though it is typed non-null string. Passing such a value into a catalog method throws ArgumentNullException inside the FrozenDictionary's OrdinalIgnoreCase lookup — a footgun hidden by the type signature. Back each value with a nullable field and a computed getter that falls back to the canonical default, so the value objects can never silently carry null.
…igibility Per product decision: image/bmp and image/tiff are legitimate images that the old prefix classifier accepted but the catalog rejected as Other (blocked pre-download, even in DMs). Add them to the catalog as Image with native byte-signature matchers (IsBmp validates the DIB header size; IsTiff validates the LE/BE byte-order mark). They are NOT model-input-eligible (providers ingest png/jpeg/gif/webp), which exposed a latent mismatch: AttachmentInlineDecision inlined on coarse category (Image) while the OpenAI-compatible provider only accepts IsModelInputSupported types. Re-key the inline decision on model-input eligibility so non-ingestible images (bmp/tiff) are accepted but delivered path-only and never reach the image-only serialization guardrail. Tests: catalog category + model-input flags for bmp/tiff, validator acceptance, and inline-decision behavior (model-input images inline; bmp/tiff path-only).
The scan -> require-verified-MIME -> re-gate-on-verified-category decision was duplicated between the live pipeline (AttachmentIngressPipeline) and the historical gate (HistoricalAttachmentIngress.ScanAndVerifyAsync) — the same class of divergence (one level up from the per-channel copies) that let the cache-hit path skip scanning entirely. Introduce ContentVerification.ResolveAsync as the single decision point, returning a structured ContentVerificationResult (Verified / ScanThrew / ScanBlocked / MissingVerifiedMime / CategoryNotAllowed). It makes the decision only; each caller still projects the result into its own logging, rejection wording, and staging cleanup — a live upload replies to the user, a backfilled attachment annotates rehydrated context. Behavior and copy are unchanged (existing live integration + historical fetcher tests pass verbatim). Adds ContentVerificationTests covering each outcome, including the allow-without-verified-MIME case that must reject rather than accept.
The binding actors already enrich their ILoggingAdapter with
WithContext("Adapter", "slack"|"discord"|"mattermost"), so prefixing the log
template with the channel name (slack_attachment_rejected, ...) double-stamped
information the log context already carries. It also forced the shared
AttachmentIngressPipeline to take a channelName parameter purely for logging.
Drop the channelName parameter and emit plain semantic events
(attachment_rejected / attachment_accepted); the channel is preserved via the
existing Adapter context. Also de-prefix the URL-trust gate logs in the Discord
and Mattermost wrappers, which run on the same enriched logger.
Capture the WithContext-over-template-prefix rule that the attachment-ingress cleanup followed, so shared abstractions keep relying on the caller's enriched logger context instead of threading channel-name strings for logging.
Apply the same structured-logging convention to the historical (thread-backfill)
path. These fetchers log via ILogger<T>, whose category already identifies the
channel, so the explicit channelLabel parameter on
HistoricalAttachmentIngress.ScanAndVerifyAsync and the literal "Slack"/"Discord"/
"Mattermost" words in the per-fetcher log templates were redundant.
Remove the channelLabel parameter (and its call-site/test arguments) and strip
the channel word from the message templates ("Historical attachment ..."); the
ILogger<T> category disambiguates the source. User-facing rejection notes are
unchanged.
…handling Review follow-ups on the shared ContentVerification gate: - ContentVerification.ResolveAsync caught all exceptions, including OperationCanceledException from the outer token, masking host/session shutdown as a scan failure (a user-facing "couldn't scan" rejection) and defeating the historical fetchers' `catch ... when (ex is not OperationCanceledException)` propagation intent. Now rethrow OCE when the outer token is cancelled; a scan-timeout from the linked CTS still maps to ScanThrew. - Both result switches used a `default:` arm that hard-cast to CategoryNotAllowed. Since ContentVerificationResult is abstract (not sealed), a future subtype would compile but throw InvalidCastException at runtime. Make CategoryNotAllowed an explicit case and let `default:` throw loudly. - Live ScanBlocked rejection now falls back to "unknown error" when a scanner reports neither error nor message (matching the historical path), instead of emitting "Content scanner rejected `x`: .". Adds cancellation-propagation tests to ContentVerificationTests.
The shared scan -> verified-MIME -> policy gate is a security decision, but it lived in Netclaw.Channels. Move ContentVerification/ContentVerificationResult to Netclaw.Security, alongside IContentScanner / ContentPolicy / ContentScanResult, so any ingress surface (channels today, MCP/REST tomorrow) can reuse the gate without taking a dependency on the channel layer. Netclaw.Security already references Configuration and Media, so no new dependencies or cycles; the channel callers already import Netclaw.Security. Doc-comment crefs to the channel pipelines were reworded (Security can't reference Channels).
…chers The provisional category gate + size gate before download was copy-pasted verbatim in all three thread-history fetchers (Slack/Discord/Mattermost). Extract it into HistoricalAttachmentIngress.CheckPreDownload, which classifies the declared MIME (corrected by extension), rejects on disallowed category or oversize, logs, and returns the rejection note (or null to proceed). Each fetcher now calls it in one line. Behavior and copy are unchanged.
Aaronontheweb
marked this pull request as ready for review
May 31, 2026 18:52
Aaronontheweb
enabled auto-merge (squash)
May 31, 2026 19:00
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
Post-review refinements
Follow-up commits from code review, consolidating attachment handling and fixing bugs it surfaced. All behavior-preserving unless noted; full suite green throughout.
Unification / less duplication (across all channels + tool calls)
AttachmentIngressPipeline; Slack/Discord/Mattermost binding actors now supply only their download closure + URL-trust gateHistoricalAttachmentIngress, and extract the pre-download category/size gate (CheckPreDownload) that was copy-pasted across the three fetchersContentVerification.ResolveAsync(returns a structuredContentVerificationResult); homed inNetclaw.Securitysince it's a security decisionMagicByteValidator's extension checks fromMimeTypeCataloginstead of a duplicate per-MIME extension table (single source of truth; drift-guard test added)Correctness fixes surfaced by review
web_fetchbinary detection was narrowed to catalog-only, decoding unknown media subtypes (image/avif, image/heic, video/x-flv, …) as UTF-8 text and corrupting the saved file — restored media-family detectionContentVerificationno longer swallowsOperationCanceledExceptionfrom the outer token (host/session shutdown was masked as a scan failure); a scan timeout still maps to a rejectiondefault:cast) so a future result type fails loudly, not as anInvalidCastExceptiontext/plainis promoted totext/htmlinstead of being rejected as a MIME mismatchStrong typing / less code
MimeType,DeclaredMimeType,VerifiedMimeType,FileExtension) null-safe by constructionWithContext("Adapter", …)/ILogger<T>category instead of prefixing it into log templates; convention recorded inTOOLING.mdBehavior note: outer-token cancellation during a scan now propagates as cancellation (correct semantics) rather than surfacing as a "couldn't scan" reply.
Verification
Follow-ups
WithContextenrichment /ILogger<T>category instead of templated identity prefixes (this PR applied that convention to the attachment-ingress path and documented it inTOOLING.md; the broader sweep is intentionally out of scope here).