Repository navigation
Potential fix for code scanning alert no. 14: Incomplete multi-character sanitization - #12
KiidxAtlas wants to merge 1 commit into
Conversation
…ter sanitization Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
| // As a final safety step, strip any script tags that might have been | ||
| // reintroduced by HTML entity decoding above. | ||
| let prev: string; | ||
| const scriptTagPattern = /<script\b[^>]*>[\s\S]*?<\/script>/gi; |
Check failure
Code scanning / CodeQL
Bad HTML filtering regexp High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
In general, the robust way to fix this is to avoid hand-written HTML-stripping regexes entirely and instead rely on a well-tested HTML parser or sanitization library that understands browser quirks (for example, ignoring attributes on closing tags, handling malformed tags, etc.). For this specific case, we want to keep existing behavior and minimize changes, while making the "final safety step" actually remove any remaining script content that a browser might interpret as a script, even if the closing tag is malformed.
The single best minimal change here is to stop trying to match well-formed <script>...</script> blocks with a regex that insists on a precise </script> end tag. Instead, we can: (1) remove any remaining <script start tags in a loop, regardless of how they are closed, and (2) optionally also remove standalone </script-like closing tags. Since this function already strips all other tags, losing only the script tag text (or even the script contents) is acceptable and aligns with the comment that this is a "final safety step". Concretely, within htmlToMarkdown near lines 1011–1018 in docs-engine/src/scraping/sphinxScraper.ts, replace the current scriptTagPattern and do/while loop with a simpler pattern that matches any <script tag (opening or malformed), such as /<\/?script\b[^>]*>/gi, and run it until no more matches remain. That avoids reliance on a specific </script> shape, and covers odd cases like </script foo="bar">. No additional imports or helper functions are required.
| @@ -1009,9 +1009,11 @@ | ||
| md = md.replace(/\n{3,}/g, '\n\n'); | ||
|
|
||
| // As a final safety step, strip any script tags that might have been | ||
| // reintroduced by HTML entity decoding above. | ||
| // reintroduced by HTML entity decoding above. We deliberately match | ||
| // both opening and closing tags with optional attributes to handle | ||
| // malformed but browser-accepted variants like "</script foo="bar">". | ||
| let prev: string; | ||
| const scriptTagPattern = /<script\b[^>]*>[\s\S]*?<\/script>/gi; | ||
| const scriptTagPattern = /<\/?script\b[^>]*>/gi; | ||
| do { | ||
| prev = md; | ||
| md = md.replace(scriptTagPattern, ''); |
Potential fix for https://github.com/KiidxAtlas/python-hover/security/code-scanning/14
In general, the problem is that the sanitization first removes tags with a regex, then decodes HTML entities, which can recreate
<and>characters and thus resurrect tags such as<script>. To fix this without changing existing functionality too much, we should ensure that after all entity decoding and whitespace normalization, the resulting string no longer contains dangerous HTML constructs, especially script blocks. Two straightforward strategies, both compatible with the existing pipeline, are: (1) escape any<characters that could start an HTML tag, or (2) remove or neutralize<script>tags specifically after decoding. Since this output is intended as Markdown, preserving<and>as plain text is often desirable (for code samples etc.), so the best minimal fix is to strip any<script ...>...</script>blocks that might have been reintroduced by entity decoding.Concretely, we should add a final sanitization step after the existing entity decoding and whitespace normalization, just before
return md.trim();. This step will repeatedly remove any<script>blocks using a robust regex that handles multiline content and nested</>inside the body. To avoid the incomplete multi-character sanitization problem, we will apply the replacement in a loop until no changes occur. All changes will be restricted todocs-engine/src/scraping/sphinxScraper.tsinside the shown snippet, and no new imports are needed.Suggested fixes powered by Copilot Autofix. Review carefully before merging.