-
Notifications
You must be signed in to change notification settings - Fork 49.9k
[DevTools] Fix developer tools not working in tabs restricted by CSP #35208
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
base: main
Are you sure you want to change the base?
Conversation
eps1lon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Overall approach looks sound.
Can you make sure all new files have the license header and @flow marker like the other files?
We should also make sure the script ID in fallbackEvalInInspectedWindow is typed to a literal to avoid typos while also adding runtime type-checking.
I'm leaning towards consolidating the source-as-string and source-as-js in a single file and let the bundler do the rest. That way it's harder to have both drift apart unintentionally. Does that make sense to you?
| if (callback) { | ||
| callback(response); | ||
| evalRequestCallbacks.delete(requestId); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this error if there's no callback registered with that request ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no callback is provided, do nothing. Logging an error here would show up in the user's page console and could cause unnecessary confusion. This is a rare edge case that shouldn't occur in normal operation, so this is sufficient, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also log errors in the user console for other backend failures so that's not a concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Honestly, I still have concerns about logging this to the user's page console, but I respect your opinion and will add the error-logging.
| callback(response); | ||
| evalRequestCallbacks.delete(requestId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe wrap the callback(response) in try-finally so that we don't leak callbacks if they error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, this is definitely a problem. I’ll fix it. Thank you.
|
Thank you. That certainly seems like a good improvement. |
Summary
fix #34268, close #29647 (duplicated)
The current implementation uses
chrome.devtools.inspectedWindow.eval(), which can be blocked by a page's Content Security Policy (CSP). To handle this, I implemented a fallback mechanism that, if eval fails, attempts to evaluate the code in a Content Script.Because Content Scripts do not have access to the
Developer Tools Console API(e.g., inspect(), $0), some features (e.g. focusing a specified DOM node in the Elements panel) will remain unavailable.However, this change still improves a situation where developer tools were completely unusable and will make many features available.
How to test
Please follow the reproduction steps from the original issue (#34268) to test behavior.
Note: when you reload the extension, tabs that were already open may not behave correctly. After loading the extension, open a new tab to test.
Detailed flow of operations
sequenceDiagram participant DevTools as DevTools Page<br>(main/index.js) participant Background as Background Script<br>(background/messageHandlers.js) participant CS as Content Script<br>[ExecutionWorld.ISOLATE]<br>(contentScripts/proxy.js) participant Page as Content Script<br>[ExecutionWorld.MAIN]<br>(contentScripts/fallbackEvalContext.js) alt First, attempt the regular processing DevTools->>DevTools: chrome.devtools.inspectedWindow.eval() else If it fails due to an error, run the fallback process below. (e.g. CSP Blocked) Note right of DevTools: Message:<br>{ source: 'devtools-page', payload: {<br>type: 'eval-in-inspected-window',<br>tabId, requestId, scriptId, args, } } DevTools->>Background: chrome.runtime.sendMessage() Note right of Background: Message:<br>{source: 'devtools-page-eval',<br>payload: { scriptId, args, } } Background->>CS: chrome.tabs.sendMessage() Note right of CS: Message:<br>{ source: 'react-devtools-<br>content-script-eval',<br>payload: { requestId, scriptId, args, } } CS->>Page: window.postMessage() Note over Page: Eval in Content Script<br>evalScripts[scriptId].apply(null, args); Note right of CS: Message:<br>{ source: 'react-devtools-<br>content-script-eval-response',<br>payload: { requestId, response, } } Page-->>CS: window.postMessage(Response) Note right of Background: Message:<br> response ( {result, error} ) CS-->>Background: sendResponse() Note right of DevTools: { source: 'react-devtools-background',<br>payload: { type: 'eval-in-inspected-<br>window-response',<br>requestId, result, error, } } Background-->>DevTools: chrome.runtime.sendMessage() end