Skip to content
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.

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

bug: TypeError on Setting 'innerHTML': 'TrustedHTML' Assignment Required #5206

Closed
TheodoreGC opened this issue Dec 22, 2023 · 9 comments · Fixed by #5207
Closed

bug: TypeError on Setting 'innerHTML': 'TrustedHTML' Assignment Required #5206

TheodoreGC opened this issue Dec 22, 2023 · 9 comments · Fixed by #5207
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil

Comments

@TheodoreGC
Copy link
Contributor

TheodoreGC commented Dec 22, 2023

Stencil version:

 @stencil/core@4.9.0

Current behavior:
When attempting to inject a web component, built using Stencil, into a webpage via a browser extension's content script, an error is encountered:

TypeError: Failed to set the 'innerHTML' property on 'Element': This document requires 'TrustedHTML' assignment.

Expected behavior:
The process of injecting a Stencil-generated web component into a webpage through a browser extension's content script should occur seamlessly, without triggering any errors related to content security policies or HTML trust requirements.

Other information:
This issue arises due to Stencil's bootstrap process for components, which relies on the innerHTML method. This method can be considered unsafe in the context of Chrome extensions, leading to stricter security checks and the resulting error.

dataStyles.innerHTML += SLOT_FB_CSS;

dataStyles.innerHTML += cmpTags + HYDRATED_CSS;

The error encountered indicates a conflict with the Content Security Policy (CSP) typically enforced in browser extensions. CSP aims to mitigate risks associated with cross-site scripting (XSS) and data injection attacks, which are particularly sensitive in the context of extensions that have elevated access to the user's browser.

As a proposed solution, replacing innerHTML with textContent in these contexts could mitigate the issue. textContent is generally considered a safer alternative and aligns well with the security requirements of browser extensions. An example substitution could be as follows:

// Add styles for `slot-fb` elements if any of our components are using slots outside the Shadow DOM
  if (hasSlotRelocation) {
    dataStyles.textContent += SLOT_FB_CSS;
  }

  // Add hydration styles
  if (BUILD.invisiblePrehydration && (BUILD.hydratedClass || BUILD.hydratedAttribute)) {
    dataStyles.textContent += cmpTags + HYDRATED_CSS;
  }

Rationale for prefering textContent over innerHTML here:

  • Security: The most compelling reason to prefer textContent is its inherent security. Unlike innerHTML, textContent does not parse HTML tags and therefore, cannot execute potentially malicious scripts. This makes it a safer choice for dynamic content insertion, aligning with the strict security requirements of browser extensions.
  • Performance: For simple text manipulations, textContent is generally faster than innerHTML because it does not involve HTML parsing and rendering overhead.
  • Impact on Web Applications Using <style> Elements: In the case of appending CSS to <style> elements, the switch from innerHTML to textContent carries no functional drawbacks for classical web applications. Since the content being manipulated is CSS (a form of text), and not HTML, there are no functional disadvantages to using textContent over innerHTML. Both methods effectively achieve the same result when used to update the content of <style> elements.

In the specific scenario of appending CSS to <style> elements within Stencil's component initialisation process, the adoption of textContent is not only a security enhancement but also aligns with best practices for performance and code clarity. This change would positively impact both browser extensions and traditional web applications, without any loss of functionality or capability.

System Info:

System: node 18.17.1
 Platform: darwin (23.1.0)
CPU Model: Apple M1 (8 cpus)
Compiler: /Users/theo/.nvm/versions/node/v18.17.1/lib/node_modules/@stencil/core/compiler/stencil.js
Build: 1702922611
Stencil: 4.9.0 🐏
TypeScript: 5.2.2
Rollup: 2.42.3
Parse5: 7.1.2
jQuery: 4.0.0-pre
Terser: 5.26.0

Code Reproduction URL:

Stencil Bug Reproduction Repository

Steps to Reproduce:

The steps to reproduce the issue can be found here

@ionitron-bot ionitron-bot bot added the holiday triage The Stencil team may not respond for an extended period of time label Dec 22, 2023
Copy link

ionitron-bot bot commented Dec 22, 2023

Thanks for the issue! This issue has been labeled as holiday triage. With the winter holidays quickly approaching, much of the Stencil Team will soon be taking time off. During this time, issue triaging and PR review will be delayed until the team begins to return. After this period, we will work to ensure that all new issues & PRs are properly triaged.

In the meantime, please read our 2023 Winter Holiday Triage Guide for information on how to ensure that your issue is triaged correctly.

Thank you!

TheodoreGC added a commit to TheodoreGC/stencil that referenced this issue Dec 22, 2023
This commit addresses security and performance concerns associated with using `innerHTML` for injecting CSS into `<style>` elements in `bootstrap-lazy.ts`.

By switching to `textContent`, the risk of executing malicious scripts is mitigated and performance is improved due to the avoidance of HTML parsing. This change enhances the security and efficiency of Stencil's component initialisation process, particularly in environments with strict security policies like browser extensions, without impacting functionality in standard web applications.

fixes: ionic-team#5206

Signed-off-by: Theodore GARSON <theodore.corbeaux@gmail.com>
@rwaskiewicz rwaskiewicz self-assigned this Jan 2, 2024
@rwaskiewicz
Copy link
Member

Hey @TheodoreGC 👋

Thanks for the report! It looks like this was created using an older version of our bug report template. Can you please add the following information to the issue summary for me? This helps us triage/understand issues much more efficiently.

  1. System Info - Output of npx stencil info, browser (e.g. does this only apply to Chrome?), versions of affected browser, and any other environment information you think is important here
  2. Code reproduction URL - a link to a minimal reproduction case for the team to look at
  3. Steps to reproduce - the steps to reproduce this issue with the code reproduction case

Thanks!

@rwaskiewicz rwaskiewicz added ionitron: needs reproduction This PR or Issue does not have a reproduction case URL and removed holiday triage The Stencil team may not respond for an extended period of time labels Jan 2, 2024
@rwaskiewicz rwaskiewicz removed their assignment Jan 2, 2024
Copy link

ionitron-bot bot commented Jan 2, 2024

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please reproduce this issue in an Stencil starter component library and provide a way for us to access it (GitHub repo, StackBlitz, etc). Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

If you have already provided a code snippet and are seeing this message, it is likely that the code snippet was not enough for our team to reproduce the issue.

For a guide on how to create a good reproduction, see our Contributing Guide.

@TheodoreGC
Copy link
Contributor Author

Hey @rwaskiewicz 👋,

Thank you for the guidance. I've updated the bug report with the additional information you requested. Here's a quick summary:

  • System Info: I've added the output of npx stencil info which details my development environment.
  • Code Reproduction URL: I've provided a link to the repository containing a minimal code reproduction case. You can find it here: Stencil Bug Reproduction. This should offer a clear view of the problem in a controlled setting.
  • Steps to Reproduce: I've detailed the specific steps to replicate the issue using the code in the provided repository.

Thanks again for your assistance!

@rwaskiewicz rwaskiewicz self-assigned this Jan 8, 2024
@rwaskiewicz rwaskiewicz added triage and removed ionitron: needs reproduction This PR or Issue does not have a reproduction case URL labels Jan 8, 2024
@rwaskiewicz
Copy link
Member

Hey @TheodoreGC - can you please provide some additional information for us?

  1. Can you include the original Stencil source code in your reproduction case? It appears that only the compiled code is in that GitHub repo.
  2. Can you provide the browser(s) and version(s) of those browsers this is occurring in?

Thanks!

@rwaskiewicz rwaskiewicz added Awaiting Reply This PR or Issue needs a reply from the original reporter. and removed triage labels Jan 9, 2024
@rwaskiewicz rwaskiewicz removed their assignment Jan 9, 2024
@TheodoreGC
Copy link
Contributor Author

Hey @rwaskiewicz,

Thanks for reaching out. I've updated the GitHub repository to include the full project of the web component. This should give a clearer picture.

Additionally, I've revised the README to provide detailed instructions on how to install the component. This will help in setting up the environment for testing and reproducing the issue.

Regarding the browsers, I've tested this behaviour on the following:

  • Brave v1.61.114 (Chromium: 120.0.6099.199) (Official Build) (arm64)
  • Chrome v120.0.6099.216 (Official Build) (arm64)

I hope this information helps. Let me know if you need anything else.

Thanks!

@ionitron-bot ionitron-bot bot removed the Awaiting Reply This PR or Issue needs a reply from the original reporter. label Jan 10, 2024
@rwaskiewicz rwaskiewicz added the Bug: Validated This PR or Issue is verified to be a bug within Stencil label Jan 10, 2024
@rwaskiewicz
Copy link
Member

Thanks! I've validated that this is a bug, and I've ingested this into our backlog. I'll see if we can get the PR reviewed soon - there's a couple mixed usages of innerHTML and textContent that I want us (the team) to verify/validate everything works as expected. Thanks again!

@alicewriteswrongs
Copy link
Contributor

Hey @TheodoreGC, just set your PR to merge, so it will be included in the next release of Stencil. We'll be sure to ping here when that gets released!

github-merge-queue bot pushed a commit that referenced this issue Feb 12, 2024
#5207)

* fix(runtime): replace `innerHTML` with `textContent` for CSS injection

This commit addresses security and performance concerns associated with using `innerHTML` for injecting CSS into `<style>` elements in `bootstrap-lazy.ts`.

By switching to `textContent`, the risk of executing malicious scripts is mitigated and performance is improved due to the avoidance of HTML parsing. This change enhances the security and efficiency of Stencil's component initialisation process, particularly in environments with strict security policies like browser extensions, without impacting functionality in standard web applications.

fixes: #5206

Signed-off-by: Theodore GARSON <theodore.corbeaux@gmail.com>

* Update bootstrap-lazy.ts

---------

Signed-off-by: Theodore GARSON <theodore.corbeaux@gmail.com>
Co-authored-by: Alice Pote <alice.writes.wrongs@gmail.com>
Co-authored-by: Alice Pote <alice.octavia.pote@gmail.com>
@christian-bromann
Copy link
Member

A fix for this has been published in v4.12.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants