Repository navigation
Potential fix for code scanning alert no. 15: Incomplete multi-character sanitization - #11
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 |
| text = stripAnnotatedWrappers(text); | ||
| text = text.replace(/Doc\(['"][\s\S]*?['"]\)/g, ''); | ||
| text = text.replace(/<\w[\w.]*\s+object\s+at\s+0x[0-9a-f]+>/gi, ''); | ||
| text = text.replace(/<script\b[^>]*>[\s\S]*?<\/script>/gi, ''); |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
In general, to fix incomplete multi-character sanitization, you either (a) apply the multi-character regex replacement repeatedly until no further changes occur, or (b) switch to a safer approach (e.g., a well-tested sanitization library or simpler single-character removal). Here, we want to keep existing behavior (remove script blocks but otherwise preserve content) and avoid introducing new dependencies if possible.
The best targeted fix is to wrap the <script>...</script> removal in a small loop that keeps running the current regex until the string stops changing. This ensures that any script blocks that “reappear” as a side-effect of earlier replacements are also removed, addressing overlapping or nested constructs. We only need to modify cleanContentAnnotations in extension/src/ui/contentCleaner.ts, specifically the line:
text = text.replace(/<script\b[^>]*>[\s\S]*?<\/script>/gi, '');We’ll replace this with a do { ... } while loop using the same regex, placed at exactly the same point in the function. No new imports or helper functions are required; we can implement the loop inline.
| @@ -144,7 +144,12 @@ | ||
| text = stripAnnotatedWrappers(text); | ||
| text = text.replace(/Doc\(['"][\s\S]*?['"]\)/g, ''); | ||
| text = text.replace(/<\w[\w.]*\s+object\s+at\s+0x[0-9a-f]+>/gi, ''); | ||
| text = text.replace(/<script\b[^>]*>[\s\S]*?<\/script>/gi, ''); | ||
| // Repeatedly strip script blocks to avoid incomplete multi-character sanitization | ||
| let previous: string; | ||
| do { | ||
| previous = text; | ||
| text = text.replace(/<script\b[^>]*>[\s\S]*?<\/script>/gi, ''); | ||
| } while (text !== previous); | ||
| text = text.replace(/^\s*\*,?\s*$/gm, ''); | ||
| text = text.replace(/\\n/g, '\n'); | ||
| text = text.replace(/,\s*,/g, ','); |
| text = stripAnnotatedWrappers(text); | ||
| text = text.replace(/Doc\(['"][\s\S]*?['"]\)/g, ''); | ||
| text = text.replace(/<\w[\w.]*\s+object\s+at\s+0x[0-9a-f]+>/gi, ''); | ||
| text = text.replace(/<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 right fix is to avoid bespoke HTML parsing with regex altogether and instead use a well-tested HTML sanitizer or parser that correctly handles script tags, including malformed end tags. Since we must keep behavior changes minimal and only touch the shown snippet, the best targeted fix is to strengthen the existing script-stripping regex so that it also matches end tags with trailing whitespace and attributes (for example </script foo="bar"> and </script >), while preserving the original functionality.
Concretely, in extension/src/ui/contentCleaner.ts within cleanContentAnnotations, we should replace the current pattern on line 147:
text = text.replace(/<script\b[^>]*>[\s\S]*?<\/script>/gi, '');with a pattern that tolerates optional whitespace and attributes on the closing tag:
text = text.replace(/<script\b[^>]*>[\s\S]*?<\/script\b[^>]*>/gi, '');This keeps the same overall matching strategy (a non-greedy span from the opening <script...> to the first acceptable closing </script...>), but changes the end-tag portion from </script> to </script\b[^>]*>. That allows any of:
</script></script ></script foo="bar"></script data-x=1 >
with any capitalization due to the i flag. No new imports or helpers are required. Only line 147 needs to be updated as described.
| @@ -144,7 +144,7 @@ | ||
| text = stripAnnotatedWrappers(text); | ||
| text = text.replace(/Doc\(['"][\s\S]*?['"]\)/g, ''); | ||
| text = text.replace(/<\w[\w.]*\s+object\s+at\s+0x[0-9a-f]+>/gi, ''); | ||
| text = text.replace(/<script\b[^>]*>[\s\S]*?<\/script>/gi, ''); | ||
| text = text.replace(/<script\b[^>]*>[\s\S]*?<\/script\b[^>]*>/gi, ''); | ||
| text = text.replace(/^\s*\*,?\s*$/gm, ''); | ||
| text = text.replace(/\\n/g, '\n'); | ||
| text = text.replace(/,\s*,/g, ','); |
Potential fix for https://github.com/KiidxAtlas/python-hover/security/code-scanning/15
In general, to fix incomplete multi-character sanitization you either (a) delegate sanitization to a robust library, or (b) avoid relying on a single fragile multi-character regex by also handling unsafe characters or repeating replacements until stabilization. Here, the simplest and safest hardening—without changing existing behavior of the other replacements—is to explicitly strip potentially dangerous HTML tags such as
<script>after the existing cleanups.Concretely, in
cleanContentAnnotationsinextension/src/ui/contentCleaner.ts, right after the existing line that removes Python object repr artifacts:add an additional replacement that removes all
<script ...>...</script>blocks (case-insensitive, single-line only) to ensure that even if a<scriptsequence appears in the text, it is removed before returning. This keeps the rest of the pipeline intact and does not require new imports. For example:is a common, conservative pattern for stripping script tags. Since this operates after the object-repr cleanup, it does not interfere with the earlier sanitization logic. No new methods or imports are required; we only add this one line in
cleanContentAnnotations.Suggested fixes powered by Copilot Autofix. Review carefully before merging.