Security: Replace NoScripts blacklist with allowlist HTML sanitization - #780
Merged
Merged
Conversation
KrzysztofPajak
force-pushed
the
feature/html-sanitization-allowlist
branch
from
August 13, 2026 15:22
8129579 to
ce7431b
Compare
- Add HtmlSanitizer (Ganss.Xss) package for robust sanitization - Implement IHtmlSanitizationService with allowlist-based sanitization - Remove regex blacklist NoScriptsAttribute from validation layer - Add [SanitizeHtml] / [NoHtml] marker attributes and global HtmlSanitizationFilter to sanitize on write, covering both MVC and API models on same code path - Apply sanitization markers to all rich-text models (Blog, News, Page, Course, Document, Knowledgebase, Vendor, etc.) previously unprotected - Sanitize on render via Html.RawSanitized helper for Razor views and both Editor.cshtml copies (shared, vendor panel); fixes the critical sink where vendor FullDescription executes in admin session with live antiforgery token - Add SanitizerAllowedIframeHosts config (youtube/vimeo/google by default); relative URLs permitted for self-hosted video from file manager - Fix 6 unencoded JS-string interpolations with JavaScriptEncoder, 1 attribute- context Html.Raw with encoding - Add 52 unit tests covering all 24 verified NoScripts bypasses, filter behavior, and config overrides Why: The prior blacklist regex was bypassable (newlines before =, onfocus, entity-encoded javascript:, unclosed <script src>), covered only 5 catalog models, was skipped by API DTOs, and enforcement relied on each controller remembering ModelState.IsValid. Vendor could store XSS in FullDescription and it would execute in admin session on a page minting an antiforgery token.
KrzysztofPajak
force-pushed
the
feature/html-sanitization-allowlist
branch
from
August 13, 2026 15:24
ce7431b to
9885656
Compare
Decided to rely solely on write-side [SanitizeHtml]/[NoHtml] enforcement via HtmlSanitizationFilter. Content stored before this change ships remains unsanitized until re-saved; this is an accepted residual risk for legacy data, not addressed by a migration or render-side defense-in-depth.
…filter - Remove HtmlSanitizationFilter and its DI/pipeline registration; ASP.NET Core already invokes DataAnnotations attributes on model bind, so no custom reflection-based filter is needed - SanitizeHtmlAttribute / NoHtmlAttribute now inherit ValidationAttribute and reject (rather than silently rewrite) values containing disallowed markup, resolving IHtmlSanitizationService via ValidationContext.GetService - the supported way for a DataAnnotations attribute to reach a DI service, since ASP.NET Core constructs ValidationContext with HttpContext.RequestServices - IHtmlSanitizationService reworked from rewrite (SanitizeRichText/StripHtml) to detection (ContainsDisallowedRichText/ContainsMarkup): runs the same allowlist sanitizer and reports whether anything would be removed, using a [ThreadStatic] flag toggled by the library's Removing*/FilterUrl events instead of comparing sanitized output strings (which would false-positive on pure reformatting, e.g. an implied <tbody> or re-spaced CSS) - Fixed a real detection gap found while switching: a literal <body>/<html>/ <head> tag in the input merges into AngleSharp's document root and its own attributes never reach RemovingAttribute, so <body onload=alert(1)> passed through undetected. Closed by explicitly checking those three root elements for any attribute of their own after SanitizeDom - verified against the library directly before and after the fix - This gap mattered more under the validation-attribute model than it would have under the filter: validation-only means a value that passes is stored byte-for-byte unchanged, so an undetected payload is not just unflagged but written verbatim - Rewrote HtmlSanitizationServiceTests for the new boolean detection API; new SanitizeHtmlAttributeTests exercises both attributes through Validator.TryValidateObject with a real DI-backed ValidationContext, matching how ASP.NET Core model validation invokes them - All 146 Grand.Infrastructure.Tests pass; full solution builds clean
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Html.Raw(spec.ValueRaw) was correct there - ValueRaw is already WebUtility.HtmlEncode-d for the Option-type spec branch that renders this color-square title (GetProductSpecificationHandler.cs:51). Auto-encoding it again would have double-encoded the value; not a real vulnerability.
- IsAllowedIframeUrl treated "//host/path" as safe same-origin (Uri.IsAbsoluteUri is false for it in .NET), but a browser resolves it as an absolute external URL, bypassing SanitizerAllowedIframeHosts entirely. Reject any src starting with "//" before the relative-url fast path. Added a regression test. - Replaced [ThreadStatic] static bool _disallowedContentSeen with a plain instance field guarded by a lock, addressing the 6 github-code-quality 'static field written by instance method' review comments on PR #780. Sanitization runs on form submission, not a hot path, so serializing it is simpler to verify than relying on Sanitize() never yielding across threads. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…zer instances The singleton no longer holds any mutable shared state. Allowlists (AllowedTags, AllowedAttributes, UriAttributes, AllowedCssProperties, ...) are built once into a read-only HtmlSanitizerOptions and never mutated afterwards, so they are safe to read from any number of threads. Each detection call now constructs its own short-lived HtmlSanitizer from those shared options, with event handlers that close over a local 'was anything removed' flag - concurrent calls can no longer observe or overwrite each other's result, so the lock from the previous commit is no longer needed. Caught along the way: HtmlSanitizerOptions only fills in what it's given - leaving UriAttributes/AllowedCssProperties/AllowedAtRules unset produces empty sets, not the library defaults (verified by reflection against the library). Missing that would have silently dropped URL-scheme filtering (javascript:, data:, iframe host allowlist) and CSS property filtering from every per-call sanitizer. Fixed by copying every relevant default off a throwaway HtmlSanitizer into the shared options, not just the ones this class customizes. Added a concurrency stress test (2000 parallel alternating calls) proving no cross-call contamination. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confirmed vectors (lower-trust role -> raw/v-html render to a customer or another staff member's session), all plain textarea/input fields (no rich-text editor), so [NoHtml] - reject any markup - matches the existing convention (AddShipmentModel.AdminComment): - OrderModel.AddOrderNoteMessage - Store manager -> Html.Raw(item.Note) on Order/Details.cshtml (customer-facing) - MerchandiseReturnModel.AddMerchandiseReturnNoteMessage - Vendor and Store manager -> Html.Raw(item.Note) on MerchandiseReturnDetails.cshtml - ShippingMethodModel.Description (+ localized) - Store manager -> v-html="shippingMethod.Description" at checkout - PickupPointModel.Description / AdminComment - Store manager, same pattern - Vendor/AdminShared ShipmentModel.AddShipmentNoteMessage - the shipment-note counterpart of the above, missed by the original migration entirely (same Html.Raw sink already fixed for OrderNote) Admin-only authored, same shape, added for consistency/defense-in-depth: - GiftVoucherModel.Message, CustomerModel.AddLoyaltyPointsMessage, CustomerModel.SendEmailModel.Body Deliberately NOT touched: MessageTemplateModel.Body and CampaignModel.Body. Both are edited via a Codemirror raw-source field (not the rich-text Editor template) because they legitimately contain full HTML/DotLiquid email markup (<style> blocks, layout tables). [NoHtml] would reject every template outright and [SanitizeHtml]'s allowlist would strip <style> and other structural markup from every legitimate template. Store manager CAN edit MessageTemplateModel (Grand.Web.Store/Controllers/MessageTemplateController.cs), so this is a real, still-open gap - it needs a design decision, not a blind attribute. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds a config-driven escape hatch for [SanitizeHtml]/[NoHtml]: when appsettings.json sets EnableHtmlSanitization to false, HtmlSanitizationService reports 'nothing disallowed' unconditionally, so both attributes accept input as-is. Intended only as a temporary mitigation if the allowlist is found to reject legitimate content in production, not as a normal operating mode - disabling it means every field these attributes guard goes back to accepting raw, unsanitized HTML from Vendor/Store-manager input. Implemented once, in the service (not duplicated across the two attributes): the flag is read in the constructor and short-circuits both ContainsDisallowedRichText and ContainsMarkup before any sanitization runs. Defaults to true (fails closed) if SecurityConfig is null or the key is absent from appsettings.json, matching how the rest of SecurityConfig's boolean options behave when unset. Added to all four web projects' appsettings.json (Web/Admin/Store/Vendor), next to the existing SanitizerAllowedIframeHosts entry, with a comment explaining it is not a security setting to leave off. 5 new tests: disabled service accepts a <script> payload and an iframe from an unlisted host through both ContainsDisallowedRichText and ContainsMarkup, and a default-SecurityConfig control case confirming detection is unaffected when the key is never set. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
Replaces the bypassable
NoScriptsAttributeregex blacklist with an allowlist-based sanitization architecture using HtmlSanitizer (Ganss.Xss).Security Issue
The prior blacklist had 24 verified bypasses including:
Coverage was narrow (5 catalog models only), API DTOs were unprotected, and enforcement relied on each controller checking ModelState.IsValid.
Exploit path: Vendor → Product.FullDescription → stored XSS → Editor.cshtml:16 Html.Raw → admin session. The same page mints an antiforgery token for ElFinder file manager.
Solution
Input-side sanitization
Output-side sanitization
Legacy data
Testing
Files Changed
🤖 Generated with Claude Code