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

Moved backend injection to the content script #16752

Merged
merged 8 commits into from
Sep 11, 2019
Merged

Moved backend injection to the content script #16752

merged 8 commits into from
Sep 11, 2019

Conversation

onionymous
Copy link
Contributor

Moved the backend injection logic to the content script. Previously this was done by using chrome.devtools.inspectedWindow.eval():

const source = `
  // the prototype stuff is in case document.createElement has been modified
  (function () {
    var script = document.constructor.prototype.createElement.call(document, 'script');
    script.src = "${scriptName}";
    script.charset = "utf-8";
    document.documentElement.appendChild(script);
    script.parentNode.removeChild(script);
  })()
  `;

  chrome.devtools.inspectedWindow.eval(source, function(response, error)
...

Since this code runs in the context of the inspected page, it is subject to the page's CSP restrictions and will cause a Trusted Types violation. Content scripts, however, are not subject to the page's restrictions and can manipulate the DOM. This change will prevent a TrustedScriptURL violation on clients that have Trusted Types support.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@sizebot
Copy link

sizebot commented Sep 11, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS against f09854a

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change seems safe. At least based on my own testing, I don't see anything that has regressed. I'll land it internally and see if anyone else notices something!

@@ -4,7 +4,6 @@ import {createElement} from 'react';
import {unstable_createRoot as createRoot, flushSync} from 'react-dom';
import Bridge from 'react-devtools-shared/src/bridge';
import Store from 'react-devtools-shared/src/devtools/store';
import inject from './inject';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the only use of the inject.js module, so we can delete that now.

bvaughn pushed a commit that referenced this pull request Sep 11, 2019
* Moved backend injection logic to content script

* Moved backend injection logic to content script

* Moved injection logic to content script

* Formatting changes

* remove ability to inject arbitrary scripts

* Removed done(), added some comments explaining the change

* Lint fixes

* Moved inline comment.

* Deleted inject() script since it was no longer being used
@bvaughn
Copy link
Contributor

bvaughn commented Sep 11, 2019

Merged via 8f03109

@bvaughn bvaughn closed this Sep 11, 2019
@bvaughn bvaughn merged commit f09854a into facebook:master Sep 11, 2019
bvaughn pushed a commit to bvaughn/react that referenced this pull request Sep 23, 2019
bvaughn pushed a commit to bvaughn/react that referenced this pull request Sep 23, 2019
PR facebook#16752 changed how we were injecting the backend script to be done by the content script in order to work around Trusted Type limitations with our previous approach. This may have caused a regression (see facebook#16840) so I'm backing it out to verify.
bvaughn pushed a commit that referenced this pull request Sep 23, 2019
PR #16752 changed how we were injecting the backend script to be done by the content script in order to work around Trusted Type limitations with our previous approach. This may have caused a regression (see #16840) so I'm backing it out to verify.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants