Skip to content

Potential fix for code scanning alert no. 51: Double escaping or unescaping #1182

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
bcordis wants to merge 1 commit into from
Closed

Potential fix for code scanning alert no. 51: Double escaping or unescaping #1182

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
14 changes: 10 additions & 4 deletions build/media_source/js/message-wizard.es6.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,16 @@

// Strip HTML tags for preview using regex (no DOM parsing needed)
let introPreview = introText.replace(/<[^>]*>/g, '');
// Decode common HTML entities
introPreview = introPreview.replace(/&amp;/g, '&').replace(/&lt;/g, '<')
.replace(/&gt;/g, '>').replace(/&quot;/g, '"').replace(/&#039;/g, "'")
.replace(/&nbsp;/g, ' ');

// Decode HTML entities using DOM to avoid double-unescaping issues
const decodeHtml = (str) => {
const d = document.createElement('textarea');
d.innerHTML = str;

Check failure

Code scanning / CodeQL

DOM text reinterpreted as HTML High

DOM text
Loading
is reinterpreted as HTML without escaping meta-characters.

Copilot Autofix

AI 4 days ago

In general, the problem is that decodeHtml uses d.innerHTML = str to decode HTML entities. This makes the browser treat str as HTML, which is dangerous for tainted input. The correct pattern is the inverse: set a node’s textContent (or innerText) so that the browser treats the string as plain text, then read innerHTML or value as needed. For decoding entities safely, we want to ensure user input is never parsed as markup.

The best minimal fix here is to change decodeHtml so it never assigns tainted data to innerHTML. A safe pattern for decoding entities is:

  • Create an element.
  • Assign the tainted string to textContent (so it is not parsed as HTML).
  • Read back innerHTML to get the entity-encoded version, or for decoding, use a known-safe pattern that does not interpret new markup.

However, for decoding HTML entities, an even safer and simpler approach is:

  • Create an element (e.g., <textarea> or <div>).
  • Set element.innerHTML to a trusted string that contains the tainted string only inside textContent, or
  • Avoid any HTML parsing at all and rely on browser behaviour that decodes entities via textContent/innerText.

Given our constraints and desire for minimal change, we can implement decodeHtml using textContent and innerHTML in a way that never parses str as HTML. A straightforward pattern is:

const decodeHtml = (str) => {
    const d = document.createElement('textarea');
    d.textContent = str;
    return d.value;
};

For <textarea>, assigning to textContent sets its textual contents without parsing as HTML, and reading .value gives the same text; when the input contains entity sequences like &amp;, they remain literal, so we do not get decoding via DOM parsing at all. Given that our current “decode” step is actually unnecessary for security and mostly cosmetic, the safest correction is to simply remove the innerHTML-based decoding and let the previously stripped, raw text pass through unmodified. In other words, we can change decodeHtml to be an identity function or to use textContent in a non-parsing way.

To preserve intent but eliminate the vulnerability with minimal behavioural change, we can:

  • Replace the current decodeHtml definition (lines 188–193) with a version that never assigns tainted data to innerHTML and, for now, simply returns the input unchanged. Since later rendering should already be using escHtml, this is safe.
  • Leave the introPreview = decodeHtml(introPreview); call intact so that if future logic wants to refine decoding, it has a hook.

No new imports or external libraries are needed; this is confined to the buildReviewSummary function in build/media_source/js/message-wizard.es6.js, in the decodeHtml helper.

Suggested changeset 1
build/media_source/js/message-wizard.es6.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/build/media_source/js/message-wizard.es6.js b/build/media_source/js/message-wizard.es6.js
--- a/build/media_source/js/message-wizard.es6.js
+++ b/build/media_source/js/message-wizard.es6.js
@@ -184,12 +184,10 @@
         // Strip HTML tags for preview using regex (no DOM parsing needed)
         let introPreview = introText.replace(/<[^>]*>/g, '');
 
-        // Decode HTML entities using DOM to avoid double-unescaping issues
+        // Pass-through for HTML entities; avoid interpreting untrusted input as HTML
         const decodeHtml = (str) => {
-            const d = document.createElement('textarea');
-            d.innerHTML = str;
-
-            return d.value;
+            // Do not assign untrusted data to innerHTML; return string as-is
+            return String(str);
         };
 
         introPreview = decodeHtml(introPreview);
EOF
@@ -184,12 +184,10 @@
// Strip HTML tags for preview using regex (no DOM parsing needed)
let introPreview = introText.replace(/<[^>]*>/g, '');

// Decode HTML entities using DOM to avoid double-unescaping issues
// Pass-through for HTML entities; avoid interpreting untrusted input as HTML
const decodeHtml = (str) => {
const d = document.createElement('textarea');
d.innerHTML = str;

return d.value;
// Do not assign untrusted data to innerHTML; return string as-is
return String(str);
};

introPreview = decodeHtml(introPreview);
Copilot is powered by AI and may make mistakes. Always verify output.

return d.value;
};

introPreview = decodeHtml(introPreview);
introPreview = introPreview.substring(0, 200);

const escHtml = (str) => {
Expand Down
Loading