Skip to content

Potential fix for code scanning alert no. 15: Incomplete multi-character sanitization #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
KiidxAtlas wants to merge 1 commit into from
Closed

Potential fix for code scanning alert no. 15: Incomplete multi-character sanitization #11

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions extension/src/ui/contentCleaner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +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, '');

Check failure

Code scanning / CodeQL

Incomplete multi-character sanitization High

This string may still contain
<script
Loading
, which may cause an HTML element injection vulnerability.

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.

Suggested changeset 1
extension/src/ui/contentCleaner.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/extension/src/ui/contentCleaner.ts b/extension/src/ui/contentCleaner.ts
--- a/extension/src/ui/contentCleaner.ts
+++ b/extension/src/ui/contentCleaner.ts
@@ -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, ',');
EOF
@@ -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, ',');
Copilot is powered by AI and may make mistakes. Always verify output.

Check failure

Code scanning / CodeQL

Bad HTML filtering regexp High

This regular expression does not match script end tags like </script >.

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.

Suggested changeset 1
extension/src/ui/contentCleaner.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/extension/src/ui/contentCleaner.ts b/extension/src/ui/contentCleaner.ts
--- a/extension/src/ui/contentCleaner.ts
+++ b/extension/src/ui/contentCleaner.ts
@@ -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, ',');
EOF
@@ -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, ',');
Copilot is powered by AI and may make mistakes. Always verify output.
text = text.replace(/^\s*\*,?\s*$/gm, '');
text = text.replace(/\\n/g, '\n');
text = text.replace(/,\s*,/g, ',');
Expand Down
Loading