fix(injectBaseStylesheet): address style injection issue #605

luqven · 2021-02-22T18:52:03Z

Description

Fix a bug where getElementByTagName() was attempting to access an attribute, .innerHTML, that was not defined.
Instead updates stylesheet by using the .item() method for HTMLCollections to then access .innerHTML.

Checklist

bug fix

Bug Fix

All existing unit tests are still passing (if applicable)
Add new passing unit tests to cover the code introduced by your PR
Update the readme
Update or add any necessary API documentation
Add some steps so we can test your bug fix
The PR title is in the conventional commit format: e.g. fix(<area>): fixed bug #issue-number
Add your info to the contributors array in package.json!

Steps to Test

Clone the repo, npm run build, and open /index.html in your default browser.

Related issue: [e.g. #603 ]

Code:

const allHeadElements = head.getElementsByTagName("*");
-  allHeadElements.innerHTML = styleEl + allHeadElements.innerHTML;
+  const styleTag = headElements.item(headElements.length - 1);
+  styleTag.innerHTML = styleEl.innerHTML + styleTag.innerHTML;

Discussion

In future, Cypress testing (or the like) integration is recommended to catch this type of bugs. Hard to detect without browser testing.

commit-lint · 2021-02-22T18:52:08Z

Bug Fixes

injectBaseStylesheet: address style injection issue (34a0d61)

Chore

prettier format file and remove unused hash (0c47e9a)

Contributors

luqven

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

@Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
@Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
@Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
@Commit-Lint merge disable will desactivate merge dependabot PR
@Commit-Lint review will approve dependabot PR
@Commit-Lint stop review will stop approve dependabot PR

sherwinski · 2021-02-23T00:48:54Z

src/js/injectBaseStylesheet.js

+  const styleTag = allHeadElements.item(allHeadElements.length - 1);
+  styleTag.innerHTML = styleEl.innerHTML + styleTag.innerHTML;


This will definitely work in the context of the local demo since the last child element happens to be a <style> element, but I don't think we can assume that will be the case for all of our users. We should prepend a completely separate <style> element into the list of allHeadElements to ensure we don't accidentally cause any errors.

Suggested change

const styleTag = allHeadElements.item(allHeadElements.length - 1);

styleTag.innerHTML = styleEl.innerHTML + styleTag.innerHTML;

const styleTag = allHeadElements.item(allHeadElements.length - 1);

styleTag.outerHTML = styleEl.outerHTML + styleTag.outerHTML;

I don't know if this is the correct way to do this... but it works 🤷

frederickfogerty

I think we need to clear up that this works well for other configurations and resolve the comments before merging, but otherwise LGTM.

frederickfogerty · 2021-02-23T11:45:45Z

src/js/injectBaseStylesheet.js

@@ -63,5 +63,6 @@ export default function injectBaseStylesheet() {
  const head = document.head;

  const allHeadElements = head.getElementsByTagName("*");
-  allHeadElements.innerHTML = styleEl + allHeadElements.innerHTML;
+  const styleTag = allHeadElements.item(allHeadElements.length - 1);


Suggested change

const styleTag = allHeadElements.item(allHeadElements.length - 1);

const styleTag = allHeadElements.item(0);

We want to select the first element, not the last one (afaik)

luqven · 2021-02-23T21:27:22Z

I think we need to clear up that this works well for other configurations and resolve the comments before merging, but otherwise LGTM.

Made use of the Node.insertBefore API to achieve the desired effect. Also removed references to deprecated type parameter for Style nodes.

Tested locally on index.html and confirmed the new Style tag gets added as the first child to the document head element.

sherwinski · 2021-02-23T22:26:06Z

Hey @luqven we actually can't use insertBefore here because it can trigger a CSP violation. Here is the PR where we removed it (#598). I think you'll need to use some combination of the two recommendations @frederickfogerty mentioned to do this without insertBefore.

luqven · 2021-02-23T22:29:27Z

Hey @luqven we actually can't use insertBefore here because it can trigger a CSP violation. Here is the PR where we removed it (#598). I think you'll need to use some combination of the two recommendations @frederickfogerty mentioned to do this without insertBefore.

🤦🏼 right my bad. Ugh really wanted to avoid magic numbers or .innerHTML... will think on this.

luqven · 2021-02-24T14:49:21Z

index.html

+	<!-- If CSP style policies, ensure SHA, nonce, or unsafe-inline are enabled -->
 	<meta http-equiv="Accept-CH" content="DPR, Width, Viewport-Width">
+	    <meta
+      http-equiv="Content-Security-Policy"
+      content="style-src 'self' 'sha256-W+PadZMOYbQi3EzR/NC+KcPx3bNLCz0OtfWwp+LK9b4=' 'sha256-OdI56ZW769BKGaLghLr3uZVaaWfqyRwyccMiu8gXf0w=' 'sha256-W+PadZMOYbQi3EzR/NC+KcPx3bNLCz0OtfWwp+LK9b4=' 'sha256-cG/+JJX3ZZs6snnMipxMnaYWATXBcyQw3vFWJF1zmAM='"
+    />


Unfortunately this is the only way I successfully got around the CSP violation issues. We had to generate a SHA value for the style tags that violated the policy and add them to the csp. We also had to add self as an src to avoid the library's inline styles from getting blocked as well.

This will have to be done by an end user as well. Ie, they'll have to implement some combination of the recommended options for style-src that allows our script and or styles to be set.

We could host the stylesheets in question on a CDN, but that seems like overkill when the user can do the above.

luqven · 2021-02-26T18:40:59Z

After looking into this some more, I don't think there's a better way to solve the CSP issues. The user would have to modify their CSPs to accommodate for Drift's behavior.

edit: to be clear, this would mean some combination of creating a hash of the script and or style tags, allowing unsafe-inline, or using some other style-src CSP that allows for this type of stylesheet modification to take place on their site.

Fix a bug where getElementByTagName() was attempting to access an attr, .innerHTML, that was not defined. Instead, create and inject a new style tag as a child to the head.

Add styles hash to csp to avoid it getting blocked by browser

luqven · 2021-03-01T14:50:29Z

Apols for all the force pushes, forgot there was 1 outstanding PR before this one 😬

frederickfogerty · 2021-03-01T15:47:37Z

@all-contributors add @luqven for maintenance

allcontributors · 2021-03-01T15:47:47Z

@frederickfogerty

I've put up a pull request to add @luqven! 🎉

frederickfogerty

All good if the discussion is resolved.

frederickfogerty · 2021-03-01T15:51:58Z

src/js/injectBaseStylesheet.js

+  // prepend them to the document head's first child
+  const head = document.head;
  const allHeadElements = head.getElementsByTagName("*");
-  allHeadElements.innerHTML = styleEl + allHeadElements.innerHTML;
+  const firstItem = allHeadElements.item(0);
+  firstItem.outerHTML = styleEl.outerHTML + firstItem.outerHTML;


If we can't get around the CSP violation anyway, is it cleaner to go back to using .insertBefore() rather than this method? i.e. reverting the changes introduced here?

That's a good point. I do think this read easier that way..

It might then be better to close this PR altogether and create two new ones:

rolling back that change and then

adding the CSP to index.html as an example / check for handling those issues.

I'll discuss w/ @sherwinski offline and circle back.

luqven · 2021-03-01T20:52:09Z

After chatting I think it's best to close and split this PR into two. Will make sure to reference this discussion so context isn't lost.

luqven added the Type: Bug label Feb 22, 2021

luqven requested a review from a team February 22, 2021 18:52

luqven self-assigned this Feb 22, 2021

luqven force-pushed the luis/styleBugfix branch from 05e3d9b to 8869e0c Compare February 22, 2021 18:55

luqven changed the title ~~bug(injectBaseStylesheet): fix style injection issue~~ fix(injectBaseStylesheet): address style injection issue Feb 22, 2021

sherwinski suggested changes Feb 23, 2021

View reviewed changes

frederickfogerty approved these changes Feb 23, 2021

View reviewed changes

luqven force-pushed the luis/styleBugfix branch from 8869e0c to 9bbb838 Compare February 23, 2021 21:25

luqven requested a review from sherwinski February 23, 2021 21:28

luqven force-pushed the luis/styleBugfix branch from 9bbb838 to b9360fc Compare February 24, 2021 14:44

luqven commented Feb 24, 2021

View reviewed changes

luqven force-pushed the luis/styleBugfix branch 2 times, most recently from 144e71f to 08b5538 Compare March 1, 2021 14:38

luqven added 3 commits March 1, 2021 09:48

fix(injectBaseStylesheet): address style injection issue

34a0d61

Fix a bug where getElementByTagName() was attempting to access an attr, .innerHTML, that was not defined. Instead, create and inject a new style tag as a child to the head.

fix(index html): add csp hash

ac55fb3

Add styles hash to csp to avoid it getting blocked by browser

chore: prettier format file and remove unused hash

0c47e9a

luqven force-pushed the luis/styleBugfix branch from 08b5538 to 0c47e9a Compare March 1, 2021 14:49

allcontributors bot mentioned this pull request Mar 1, 2021

docs: add luqven as a contributor #613

Merged

frederickfogerty approved these changes Mar 1, 2021

View reviewed changes

luqven closed this Mar 1, 2021

luqven mentioned this pull request Mar 2, 2021

fix: revert directly modifying DOM over setAttribute #614

Merged

luqven mentioned this pull request Mar 2, 2021

chore: add content security policy (csp) to example page #615

Merged

luqven deleted the luis/styleBugfix branch August 2, 2021 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(injectBaseStylesheet): address style injection issue #605

fix(injectBaseStylesheet): address style injection issue #605

luqven commented Feb 22, 2021

commit-lint bot commented Feb 22, 2021 •

edited

Loading

sherwinski Feb 23, 2021

frederickfogerty Feb 23, 2021

frederickfogerty left a comment

frederickfogerty Feb 23, 2021

luqven commented Feb 23, 2021

sherwinski commented Feb 23, 2021

luqven commented Feb 23, 2021

luqven Feb 24, 2021 •

edited

Loading

luqven commented Feb 26, 2021 •

edited

Loading

luqven commented Mar 1, 2021

frederickfogerty commented Mar 1, 2021

allcontributors bot commented Mar 1, 2021

frederickfogerty left a comment

frederickfogerty Mar 1, 2021

luqven Mar 1, 2021 •

edited

Loading

luqven commented Mar 1, 2021

		const styleTag = allHeadElements.item(allHeadElements.length - 1);
		styleTag.innerHTML = styleEl.innerHTML + styleTag.innerHTML;

	const styleTag = allHeadElements.item(allHeadElements.length - 1);
	const styleTag = allHeadElements.item(0);

fix(injectBaseStylesheet): address style injection issue #605

fix(injectBaseStylesheet): address style injection issue #605

Conversation

luqven commented Feb 22, 2021

Description

Checklist

Bug Fix

Steps to Test

Discussion

commit-lint bot commented Feb 22, 2021 • edited Loading

Bug Fixes

Chore

Contributors

sherwinski Feb 23, 2021

Choose a reason for hiding this comment

frederickfogerty Feb 23, 2021

Choose a reason for hiding this comment

frederickfogerty left a comment

Choose a reason for hiding this comment

frederickfogerty Feb 23, 2021

Choose a reason for hiding this comment

luqven commented Feb 23, 2021

sherwinski commented Feb 23, 2021

luqven commented Feb 23, 2021

luqven Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

luqven commented Feb 26, 2021 • edited Loading

luqven commented Mar 1, 2021

frederickfogerty commented Mar 1, 2021

allcontributors bot commented Mar 1, 2021

frederickfogerty left a comment

Choose a reason for hiding this comment

frederickfogerty Mar 1, 2021

Choose a reason for hiding this comment

luqven Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

luqven commented Mar 1, 2021

commit-lint bot commented Feb 22, 2021 •

edited

Loading

luqven Feb 24, 2021 •

edited

Loading

luqven commented Feb 26, 2021 •

edited

Loading

luqven Mar 1, 2021 •

edited

Loading