[DevTools] Enable minimal support in pages with sandbox Content-Security-Policy#207
Closed
everettbu wants to merge 4 commits into
Closed
[DevTools] Enable minimal support in pages with sandbox Content-Security-Policy#207everettbu wants to merge 4 commits into
sandbox Content-Security-Policy#207everettbu wants to merge 4 commits into
Conversation
Greptile OverviewGreptile SummaryThis PR implements a CSP fallback mechanism enabling React DevTools to function on pages with restrictive Content Security Policies by routing eval calls through content scripts when Architecture changes:
Critical security vulnerabilities found:
Logic issues:
Code quality:
Confidence Score: 1/5
Important Files ChangedFile Analysis
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| import {evalScripts} from '../evalScripts'; | ||
|
|
||
| window.addEventListener('message', event => { |
There was a problem hiding this comment.
logic: missing event.source validation - other content scripts check event.source !== window to prevent malicious messages from iframes or other origins
Suggested change
| window.addEventListener('message', event => { | |
| if (event.source !== window || event.data?.source === 'react-devtools-content-script-eval') { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-extensions/src/contentScripts/fallbackEvalContext.js
Line: 12:12
Comment:
**logic:** missing `event.source` validation - other content scripts check `event.source !== window` to prevent malicious messages from iframes or other origins
```suggestion
if (event.source !== window || event.data?.source === 'react-devtools-content-script-eval') {
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+109
to
+110
| if (callback) { | ||
| callback({result, error}); |
There was a problem hiding this comment.
logic: callback not cleaned up if called - if callback is invoked, it remains in the evalRequestCallbacks map forever causing a memory leak
Suggested change
| if (callback) { | |
| callback({result, error}); | |
| if (callback) { | |
| evalRequestCallbacks.delete(requestId); | |
| callback({result, error}); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-extensions/src/main/evalInInspectedWindow.js
Line: 109:110
Comment:
**logic:** callback not cleaned up if called - if callback is invoked, it remains in the `evalRequestCallbacks` map forever causing a memory leak
```suggestion
if (callback) {
evalRequestCallbacks.delete(requestId);
callback({result, error});
}
```
How can I resolve this? If you propose a fix, please make it concise.
everettbu
changed the title
sandbox Content-Security-Policy
|
|
||
| import {evalScripts} from '../evalScripts'; | ||
|
|
||
| window.addEventListener('message', event => { |
There was a problem hiding this comment.
Security vulnerability: missing event.source validation allows malicious iframes to trigger arbitrary script execution. Any iframe can send a message with source: 'react-devtools-content-script-eval' and execute scripts from the evalScripts registry.
Suggested change
| window.addEventListener('message', event => { | |
| window.addEventListener('message', event => { | |
| if (event.source !== window) return; | |
| if (event.data?.source === 'react-devtools-content-script-eval') { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-extensions/src/contentScripts/fallbackEvalContext.js
Line: 12:12
Comment:
**Security vulnerability**: missing `event.source` validation allows malicious iframes to trigger arbitrary script execution. Any iframe can send a message with `source: 'react-devtools-content-script-eval'` and execute scripts from the `evalScripts` registry.
```suggestion
window.addEventListener('message', event => {
if (event.source !== window) return;
if (event.data?.source === 'react-devtools-content-script-eval') {
```
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| }); | ||
|
|
||
| window.addEventListener('message', event => { |
There was a problem hiding this comment.
Security vulnerability: missing event.source validation allows malicious iframes to send fake eval responses. Should validate event.source === window.
Suggested change
| window.addEventListener('message', event => { | |
| window.addEventListener('message', event => { | |
| if (event.source !== window) return; | |
| if (event.data?.source === 'react-devtools-content-script-eval-response') { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-extensions/src/contentScripts/proxy.js
Line: 146:146
Comment:
**Security vulnerability**: missing `event.source` validation allows malicious iframes to send fake eval responses. Should validate `event.source === window`.
```suggestion
window.addEventListener('message', event => {
if (event.source !== window) return;
if (event.data?.source === 'react-devtools-content-script-eval-response') {
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+121
to
+122
| let evalRequestId = 0; | ||
| const evalRequestCallbacks = new Map<number, Function>(); |
There was a problem hiding this comment.
memory leak: callbacks never cleaned up on extension unload or when responses are never received (e.g., if page unloads before response)
Suggested change
| let evalRequestId = 0; | |
| const evalRequestCallbacks = new Map<number, Function>(); | |
| let evalRequestId = 0; | |
| const evalRequestCallbacks = new Map<number, {callback: Function, timeout: TimeoutID}>(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-extensions/src/contentScripts/proxy.js
Line: 121:122
Comment:
memory leak: callbacks never cleaned up on extension unload or when responses are never received (e.g., if page unloads before response)
```suggestion
let evalRequestId = 0;
const evalRequestCallbacks = new Map<number, {callback: Function, timeout: TimeoutID}>();
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+37
to
+38
| } | ||
| const code = evalScripts[scriptId].code.apply(null, args); |
There was a problem hiding this comment.
evalScripts[scriptId].code is being called but result isn't used here - it's computed again in the outer function. redundant computation.
Suggested change
| } | |
| const code = evalScripts[scriptId].code.apply(null, args); | |
| // code is already computed by the outer function | |
| const tabId = chrome.devtools.inspectedWindow.tabId; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-extensions/src/main/evalInInspectedWindow.js
Line: 37:38
Comment:
`evalScripts[scriptId].code` is being called but result isn't used here - it's computed again in the outer function. redundant computation.
```suggestion
// code is already computed by the outer function
const tabId = chrome.devtools.inspectedWindow.tabId;
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+67
to
+68
| if (callback) { | ||
| if (error) { |
There was a problem hiding this comment.
callbacks invoked with different parameter structure than the main eval path (callback(result, null) vs callback(null, exceptionInfo)). fallback path doesn't pass exceptionInfo with isError: false, isException: true structure for exceptions
Suggested change
| if (callback) { | |
| if (error) { | |
| if (error) { | |
| callback(null, { | |
| code, | |
| description: error, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-extensions/src/main/evalInInspectedWindow.js
Line: 67:68
Comment:
callbacks invoked with different parameter structure than the main eval path (`callback(result, null)` vs `callback(null, exceptionInfo)`). fallback path doesn't pass `exceptionInfo` with `isError: false, isException: true` structure for exceptions
```suggestion
if (error) {
callback(null, {
code,
description: error,
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+89
to
+90
| fn: ({rendererID, elementID}) => { | ||
| return false; // Not supported in fallback eval context |
There was a problem hiding this comment.
returning false silently makes it unclear whether feature is unsupported or operation failed legitimately. should throw error or return distinguishable value
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-extensions/src/evalScripts.js
Line: 89:90
Comment:
returning `false` silently makes it unclear whether feature is unsupported or operation failed legitimately. should throw error or return distinguishable value
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+133
to
+136
| requestId, | ||
| scriptId, | ||
| args, | ||
| }, |
There was a problem hiding this comment.
should set timeout to clean up callback if response never arrives (e.g., page unloads). prevents memory leak in evalRequestCallbacks
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-extensions/src/contentScripts/proxy.js
Line: 133:136
Comment:
should set timeout to clean up callback if response never arrives (e.g., page unloads). prevents memory leak in `evalRequestCallbacks`
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+91
to
+92
| const code = evalScripts[scriptId].code.apply(null, args); | ||
| chrome.devtools.inspectedWindow.eval(code, (result, exceptionInfo) => { |
There was a problem hiding this comment.
always triggers fallback on ANY exception, even non-CSP errors like syntax errors or runtime exceptions. should check exceptionInfo.code to only fallback for CSP violations
Suggested change
| const code = evalScripts[scriptId].code.apply(null, args); | |
| chrome.devtools.inspectedWindow.eval(code, (result, exceptionInfo) => { | |
| chrome.devtools.inspectedWindow.eval(code, (result, exceptionInfo) => { | |
| if (!exceptionInfo || !exceptionInfo.isException) { | |
| callback(result, exceptionInfo); | |
| return; | |
| } | |
| // Only fallback for CSP-related errors, not all exceptions | |
| if (exceptionInfo.code === 'E_PROTOCOLERROR' || | |
| (exceptionInfo.description && exceptionInfo.description.includes('Content Security Policy'))) { | |
| fallbackEvalInInspectedWindow(scriptId, args, callback); | |
| } else { | |
| // Pass through other exceptions normally | |
| callback(result, exceptionInfo); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-extensions/src/main/evalInInspectedWindow.js
Line: 91:92
Comment:
always triggers fallback on ANY exception, even non-CSP errors like syntax errors or runtime exceptions. should check `exceptionInfo.code` to only fallback for CSP violations
```suggestion
chrome.devtools.inspectedWindow.eval(code, (result, exceptionInfo) => {
if (!exceptionInfo || !exceptionInfo.isException) {
callback(result, exceptionInfo);
return;
}
// Only fallback for CSP-related errors, not all exceptions
if (exceptionInfo.code === 'E_PROTOCOLERROR' ||
(exceptionInfo.description && exceptionInfo.description.includes('Content Security Policy'))) {
fallbackEvalInInspectedWindow(scriptId, args, callback);
} else {
// Pass through other exceptions normally
callback(result, exceptionInfo);
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+114
to
+129
| .then(response => { | ||
| if (!response) { | ||
| chrome.runtime.sendMessage({ | ||
| source: 'react-devtools-background', | ||
| payload: { | ||
| type: 'eval-in-inspected-window-response', | ||
| requestId, | ||
| result: null, | ||
| error: 'No response from content script', | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
| const {result, error} = response; | ||
| chrome.runtime.sendMessage({ | ||
| source: 'react-devtools-background', |
There was a problem hiding this comment.
if sendMessage resolves with undefined (no listener on target tab), reports generic "No response" error. should distinguish between "no content script" vs actual errors
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-extensions/src/background/messageHandlers.js
Line: 114:129
Comment:
if `sendMessage` resolves with `undefined` (no listener on target tab), reports generic "No response" error. should distinguish between "no content script" vs actual errors
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+16
to
+19
| try { | ||
| if (!evalScripts[scriptId]) { | ||
| throw new Error(`No eval script with id "${scriptId}" exists.`); | ||
| } |
There was a problem hiding this comment.
caught error's stack trace is lost when only passing err.message. better to serialize full error or include stack
Suggested change
| try { | |
| if (!evalScripts[scriptId]) { | |
| throw new Error(`No eval script with id "${scriptId}" exists.`); | |
| } | |
| try { | |
| if (!evalScripts[scriptId]) { | |
| throw new Error(`No eval script with id "${scriptId}" exists.`); | |
| } | |
| response.result = evalScripts[scriptId].fn.apply(null, args); | |
| } catch (err) { | |
| response.error = err.stack || err.message || String(err); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-extensions/src/contentScripts/fallbackEvalContext.js
Line: 16:19
Comment:
caught error's stack trace is lost when only passing `err.message`. better to serialize full error or include stack
```suggestion
try {
if (!evalScripts[scriptId]) {
throw new Error(`No eval script with id "${scriptId}" exists.`);
}
response.result = evalScripts[scriptId].fn.apply(null, args);
} catch (err) {
response.error = err.stack || err.message || String(err);
```
How can I resolve this? If you propose a fix, please make it concise.
Author
|
Upstream PR was closed or merged. Code is synced via branch mirror. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mirror of facebook/react#35208
Original author: mochiya98
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