Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ const contentScriptsToInject = [
runAt: 'document_end',
world: chrome.scripting.ExecutionWorld.ISOLATED,
},
{
id: '@react-devtools/fallback-eval-context',
js: ['build/fallbackEvalContext.js'],
matches: ['<all_urls>'],
persistAcrossSessions: true,
runAt: 'document_start',
world: chrome.scripting.ExecutionWorld.MAIN,
},
{
id: '@react-devtools/hook',
js: ['build/installHook.js'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,58 @@ export function handleDevToolsPageMessage(message) {

break;
}

case 'eval-in-inspected-window': {
const {
payload: {tabId, requestId, scriptId, args},
} = message;

chrome.tabs
.sendMessage(tabId, {
Comment on lines +106 to +107

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

missing validation that message is from a legitimate devtools page. should verify sender is from extension context

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-extensions/src/background/messageHandlers.js
Line: 106:107

Comment:
missing validation that message is from a legitimate devtools page. should verify sender is from extension context

How can I resolve this? If you propose a fix, please make it concise.

source: 'devtools-page-eval',
payload: {
scriptId,
args,
},
})
.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',
Comment on lines +114 to +129

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

payload: {
type: 'eval-in-inspected-window-response',
requestId,
result,
error,
},
});
})
.catch(error => {
chrome.runtime.sendMessage({
source: 'react-devtools-background',
payload: {
type: 'eval-in-inspected-window-response',
requestId,
result: null,
error: error?.message || String(error),
},
});
});

break;
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

import {evalScripts} from '../evalScripts';

window.addEventListener('message', event => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

if (event.data?.source === 'react-devtools-content-script-eval') {
const {scriptId, args, requestId} = event.data.payload;
const response = {result: null, error: null};
try {
if (!evalScripts[scriptId]) {
throw new Error(`No eval script with id "${scriptId}" exists.`);
}
Comment on lines +16 to +19

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

response.result = evalScripts[scriptId].fn.apply(null, args);
} catch (err) {
response.error = err.message;
}
window.postMessage(
{
source: 'react-devtools-content-script-eval-response',
payload: {
requestId,
response,
},
},
'*',
);
}
});
46 changes: 46 additions & 0 deletions packages/react-devtools-extensions/src/contentScripts/proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,3 +117,49 @@ function connectPort() {
// $FlowFixMe[incompatible-use]
port.onDisconnect.addListener(handleDisconnect);
}

let evalRequestId = 0;
const evalRequestCallbacks = new Map<number, Function>();
Comment on lines +121 to +122

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.


chrome.runtime.onMessage.addListener((msg, sender, sendResponse) => {
switch (msg?.source) {
case 'devtools-page-eval': {
const {scriptId, args} = msg.payload;
const requestId = evalRequestId++;
window.postMessage(
{
source: 'react-devtools-content-script-eval',
payload: {
requestId,
scriptId,
args,
},
Comment on lines +133 to +136

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

},
'*',
);
evalRequestCallbacks.set(requestId, sendResponse);
return true; // Indicate we will respond asynchronously
}
}
});

window.addEventListener('message', event => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

if (event.data?.source === 'react-devtools-content-script-eval-response') {
const {requestId, response} = event.data.payload;
const callback = evalRequestCallbacks.get(requestId);
try {
if (!callback)
throw new Error(
`No eval request callback for id "${requestId}" exists.`,
);
callback(response);
} catch (e) {
console.warn(
'React DevTools Content Script eval response error occurred:',
e,
);
} finally {
evalRequestCallbacks.delete(requestId);
}
}
});
112 changes: 112 additions & 0 deletions packages/react-devtools-extensions/src/evalScripts.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

export type EvalScriptIds =
| 'checkIfReactPresentInInspectedWindow'
| 'reload'
| 'setBrowserSelectionFromReact'
| 'setReactSelectionFromBrowser'
| 'viewAttributeSource'
| 'viewElementSource';

/*
.fn for fallback in Content Script context
.code for chrome.devtools.inspectedWindow.eval()
*/
type EvalScriptEntry = {
fn: (...args: any[]) => any,
code: (...args: any[]) => string,
};

/*
Can not access `Developer Tools Console API` (e.g., inspect(), $0) in this context.
So some fallback functions are no-op or throw error.
*/
export const evalScripts: {[key: EvalScriptIds]: EvalScriptEntry} = {
checkIfReactPresentInInspectedWindow: {
fn: () =>
window.__REACT_DEVTOOLS_GLOBAL_HOOK__ &&
window.__REACT_DEVTOOLS_GLOBAL_HOOK__.renderers.size > 0,
code: () =>
'window.__REACT_DEVTOOLS_GLOBAL_HOOK__ &&' +
'window.__REACT_DEVTOOLS_GLOBAL_HOOK__.renderers.size > 0',
},
reload: {
fn: () => window.location.reload(),
code: () => 'window.location.reload();',
},
setBrowserSelectionFromReact: {
fn: () => {
throw new Error('Not supported in fallback eval context');
},
code: () =>
'(window.__REACT_DEVTOOLS_GLOBAL_HOOK__.$0 !== $0) ?' +
'(inspect(window.__REACT_DEVTOOLS_GLOBAL_HOOK__.$0), true) :' +
'false',
},
setReactSelectionFromBrowser: {
fn: () => {
throw new Error('Not supported in fallback eval context');
},
code: () =>
'(window.__REACT_DEVTOOLS_GLOBAL_HOOK__ && window.__REACT_DEVTOOLS_GLOBAL_HOOK__.$0 !== $0) ?' +
'(window.__REACT_DEVTOOLS_GLOBAL_HOOK__.$0 = $0, true) :' +
'false',
},
viewAttributeSource: {
fn: ({rendererID, elementID, path}) => {
Comment on lines +62 to +63

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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: 62:63

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.

return false; // Not supported in fallback eval context
},
code: ({rendererID, elementID, path}) =>
'{' + // The outer block is important because it means we can declare local variables.
'const renderer = window.__REACT_DEVTOOLS_GLOBAL_HOOK__.rendererInterfaces.get(' +
JSON.stringify(rendererID) +
');' +
'if (renderer) {' +
' const value = renderer.getElementAttributeByPath(' +
JSON.stringify(elementID) +
',' +
JSON.stringify(path) +
');' +
' if (value) {' +
' inspect(value);' +
' true;' +
' } else {' +
' false;' +
' }' +
'} else {' +
' false;' +
'}' +
'}',
},
viewElementSource: {
fn: ({rendererID, elementID}) => {
return false; // Not supported in fallback eval context
Comment on lines +89 to +90

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

},
code: ({rendererID, elementID}) =>
'{' + // The outer block is important because it means we can declare local variables.
'const renderer = window.__REACT_DEVTOOLS_GLOBAL_HOOK__.rendererInterfaces.get(' +
JSON.stringify(rendererID) +
');' +
'if (renderer) {' +
' const value = renderer.getElementSourceFunctionById(' +
JSON.stringify(elementID) +
');' +
' if (value) {' +
' inspect(value);' +
' true;' +
' } else {' +
' false;' +
' }' +
'} else {' +
' false;' +
'}' +
'}',
},
};
16 changes: 7 additions & 9 deletions packages/react-devtools-extensions/src/main/elementSelection.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
/* global chrome */
import {evalInInspectedWindow} from './evalInInspectedWindow';

export function setBrowserSelectionFromReact() {
// This is currently only called on demand when you press "view DOM".
// In the future, if Chrome adds an inspect() that doesn't switch tabs,
// we could make this happen automatically when you select another component.
chrome.devtools.inspectedWindow.eval(
'(window.__REACT_DEVTOOLS_GLOBAL_HOOK__.$0 !== $0) ?' +
'(inspect(window.__REACT_DEVTOOLS_GLOBAL_HOOK__.$0), true) :' +
'false',
evalInInspectedWindow(
'setBrowserSelectionFromReact',
[],
(didSelectionChange, evalError) => {
if (evalError) {
console.error(evalError);
Expand All @@ -19,10 +18,9 @@ export function setBrowserSelectionFromReact() {
export function setReactSelectionFromBrowser(bridge) {
// When the user chooses a different node in the browser Elements tab,
// copy it over to the hook object so that we can sync the selection.
chrome.devtools.inspectedWindow.eval(
'(window.__REACT_DEVTOOLS_GLOBAL_HOOK__ && window.__REACT_DEVTOOLS_GLOBAL_HOOK__.$0 !== $0) ?' +
'(window.__REACT_DEVTOOLS_GLOBAL_HOOK__.$0 = $0, true) :' +
'false',
evalInInspectedWindow(
'setReactSelectionFromBrowser',
[],
(didSelectionChange, evalError) => {
if (evalError) {
console.error(evalError);
Expand Down
Loading
Loading