Security: Replace NoScripts blacklist with allowlist HTML sanitization - #780
Closed
KrzysztofPajak wants to merge 4 commits into
Closed
Security: Replace NoScripts blacklist with allowlist HTML sanitization#780KrzysztofPajak wants to merge 4 commits into
KrzysztofPajak wants to merge 4 commits into
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}
| { | ||
| if (string.IsNullOrWhiteSpace(html)) return false; | ||
|
|
||
| _disallowedContentSeen = false; |
| //attributes on that wrapper, so any attribute there means the raw input smuggled one in. | ||
| if (HasOwnAttributes(document.DocumentElement) || HasOwnAttributes(document.Head) || | ||
| HasOwnAttributes(document.Body)) | ||
| _disallowedContentSeen = true; |
| { | ||
| if (string.IsNullOrWhiteSpace(text)) return false; | ||
|
|
||
| _disallowedContentSeen = false; |
| //into the document root and its own attributes never reach RemovingAttribute | ||
| if (HasOwnAttributes(document.DocumentElement) || HasOwnAttributes(document.Head) || | ||
| HasOwnAttributes(document.Body)) | ||
| _disallowedContentSeen = true; |
| if (!isInlineImage) | ||
| { | ||
| e.SanitizedUrl = null; | ||
| _disallowedContentSeen = true; |
| if (string.Equals(tagName, "IFRAME", StringComparison.OrdinalIgnoreCase) && !IsAllowedIframeUrl(url)) | ||
| { | ||
| e.SanitizedUrl = null; | ||
| _disallowedContentSeen = true; |
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.
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