From 6f07dfd65db8699d279ac6aaea03e48673b06a48 Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Wed, 20 Nov 2024 06:21:26 -0800 Subject: [PATCH] dev-middleware: Only rewrite hostnames if they match device connection hosts (#47685) Summary: Currently, we assume any URL with a hostname of `10.0.2.2` or `10.0.3.2` (device-relative) is eligible for rewriting to `localhost` (frontend-relative), because we assume the device is an Android emulator. We rewrite these URLs between device and dev machine so that the rewritten URLs are reachable from the dev machine. This diff narrows this logic so that we'll only rewrite URLs where the hostname matches the pre-existing list *and* this matches the host the device is actually connected on, according to its headers from the original connection. The main motivation for this change is to unblock removing assumptions about device-reachable vs server-reachable hosts. Later in the stack we'll drop the hardcoded listing of `10.0.2.2` etc in favour of identifying URLs that target the dev server, from whatever network. There's also an edge case fix here that `10.0.2.2` etc might actually refer to a remote LAN server, and not be an Android emulator's alias for for an emulator host. Changelog: [General][Fixed] RN DevTools: Don't assume 10.0.2.2 is an alias for localhost unless it's used to establish a connection to the server Reviewed By: huntie Differential Revision: D66058704 --- .../src/__tests__/FetchUtils.js | 25 ++-- .../src/__tests__/InspectorDeviceUtils.js | 12 +- .../src/__tests__/InspectorProtocolUtils.js | 9 +- .../InspectorProxyCdpRewritingHacks-test.js | 115 +++++++++++++++++- .../src/inspector-proxy/Device.js | 82 +++++++++---- .../src/inspector-proxy/InspectorProxy.js | 7 +- 6 files changed, 203 insertions(+), 47 deletions(-) diff --git a/packages/dev-middleware/src/__tests__/FetchUtils.js b/packages/dev-middleware/src/__tests__/FetchUtils.js index 650ce613943..794c669cfeb 100644 --- a/packages/dev-middleware/src/__tests__/FetchUtils.js +++ b/packages/dev-middleware/src/__tests__/FetchUtils.js @@ -14,8 +14,6 @@ import type {RequestOptions} from 'undici'; import {Agent, request} from 'undici'; -declare var globalThis: $FlowFixMe; - /** * A version of `fetch` that is usable with the HTTPS server created in * ServerUtils (which uses a self-signed certificate). @@ -66,27 +64,26 @@ export async function fetchJson(url: string): Promise { * Change the global fetch dispatcher to allow self-signed certificates. * This runs with Jest's `beforeAll` and `afterAll`, and restores the original dispatcher. */ -export function withFetchSelfSignedCertsForAllTests() { - const fetchOriginal = globalThis.fetch; +export function withFetchSelfSignedCertsForAllTests( + fetchSpy: JestMockFn, ReturnType>, + fetchOriginal: typeof fetch, +) { const selfSignedCertDispatcher = new Agent({ connect: { rejectUnauthorized: false, }, }); - let fetchSpy; - beforeAll(() => { // For some reason, setting the `selfSignedCertDispatcher` with `setGlobalDispatcher` doesn't work. // Instead of using `setGlobalDispatcher`, we'll use a spy to intercept the fetch calls and add the dispatcher. - fetchSpy = jest - .spyOn(globalThis, 'fetch') - .mockImplementation((url, options) => - fetchOriginal(url, { - ...options, - dispatcher: options?.dispatcher ?? selfSignedCertDispatcher, - }), - ); + fetchSpy.mockImplementation((url, options) => + fetchOriginal(url, { + ...options, + // $FlowFixMe[prop-missing]: dispatcher + dispatcher: options?.dispatcher ?? selfSignedCertDispatcher, + }), + ); }); afterAll(() => { diff --git a/packages/dev-middleware/src/__tests__/InspectorDeviceUtils.js b/packages/dev-middleware/src/__tests__/InspectorDeviceUtils.js index 2f4d5d69d8a..92cd8793e5d 100644 --- a/packages/dev-middleware/src/__tests__/InspectorDeviceUtils.js +++ b/packages/dev-middleware/src/__tests__/InspectorDeviceUtils.js @@ -27,10 +27,17 @@ export class DeviceAgent { #ws: ?WebSocket; #readyPromise: Promise; - constructor(url: string, signal?: AbortSignal) { + constructor(url: string, signal?: AbortSignal, host?: ?string) { const ws = new WebSocket(url, { // The mock server uses a self-signed certificate. rejectUnauthorized: false, + ...(host != null + ? { + headers: { + Host: host, + }, + } + : {}), }); this.#ws = ws; ws.on('message', data => { @@ -160,8 +167,9 @@ export class DeviceMock extends DeviceAgent { export async function createDeviceMock( url: string, signal: AbortSignal, + host?: ?string, ): Promise { - const device = new DeviceMock(url, signal); + const device = new DeviceMock(url, signal, host); await device.ready(); return device; } diff --git a/packages/dev-middleware/src/__tests__/InspectorProtocolUtils.js b/packages/dev-middleware/src/__tests__/InspectorProtocolUtils.js index f3ab1e3bfcc..69040c32c49 100644 --- a/packages/dev-middleware/src/__tests__/InspectorProtocolUtils.js +++ b/packages/dev-middleware/src/__tests__/InspectorProtocolUtils.js @@ -126,7 +126,13 @@ export async function createAndConnectTarget( }>, signal: AbortSignal, page: PageFromDevice, - deviceId: ?string = null, + { + deviceId = null, + host = null, + }: $ReadOnly<{ + deviceId?: ?string, + host?: ?string, + }> = {}, ): Promise<{device: DeviceMock, debugger_: DebuggerMock}> { let device; let debugger_; @@ -136,6 +142,7 @@ export async function createAndConnectTarget( deviceId ?? 'device' + Date.now() }&name=foo&app=bar`, signal, + host, ); device.getPages.mockImplementation(() => [page]); diff --git a/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js b/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js index d0cff55f83e..8c8a6fc345f 100644 --- a/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js +++ b/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js @@ -33,12 +33,18 @@ jest.useRealTimers(); jest.setTimeout(10000); +const fetchOriginal = fetch; +const fetchSpy: JestMockFn< + Parameters, + ReturnType, +> = jest.spyOn(globalThis, 'fetch'); + describe.each(['HTTP', 'HTTPS'])( 'inspector proxy CDP rewriting hacks over %s', protocol => { // Inspector proxy tests are using a self-signed certificate for HTTPS tests. if (protocol === 'HTTPS') { - withFetchSelfSignedCertsForAllTests(); + withFetchSelfSignedCertsForAllTests(fetchSpy, fetchOriginal); } const serverRef = withServerForEachTest({ @@ -46,6 +52,7 @@ describe.each(['HTTP', 'HTTPS'])( projectRoot: __dirname, secure: protocol === 'HTTPS', }); + const autoCleanup = withAbortSignalForEachTest(); afterEach(() => { jest.clearAllMocks(); @@ -188,6 +195,96 @@ describe.each(['HTTP', 'HTTPS'])( } }); + test("does not rewrite urls in Debugger.scriptParsed that don't match the device connection host", async () => { + serverRef.app.use('/source-map', serveStaticJson({version: 3})); + const {device, debugger_} = await createAndConnectTarget( + serverRef, + autoCleanup.signal, + { + app: 'bar-app', + id: 'page1', + title: 'bar-title', + vm: 'bar-vm', + }, + { + host: '192.168.0.123:' + serverRef.port, + }, + ); + try { + let fetchCalledWithURL; + fetchSpy.mockImplementationOnce(async url => { + fetchCalledWithURL = url instanceof URL ? url : null; + throw new Error('Unreachable'); + }); + const sourceMapURL = `${protocol.toLowerCase()}://127.0.0.1:${ + serverRef.port + }/source-map`; + const scriptParsedMessage = await sendFromTargetToDebugger( + device, + debugger_, + 'page1', + { + method: 'Debugger.scriptParsed', + params: { + sourceMapURL, + }, + }, + ); + expect(fetchCalledWithURL?.href).toEqual(sourceMapURL); + expect(scriptParsedMessage.params.sourceMapURL).toEqual( + `${protocol.toLowerCase()}://127.0.0.1:${serverRef.port}/source-map`, + ); + } finally { + device.close(); + debugger_.close(); + } + }); + + test('does not rewrite urls in Debugger.scriptParsed that match the device connection host but are not allowlisted for rewriting', async () => { + serverRef.app.use('/source-map', serveStaticJson({version: 3})); + const {device, debugger_} = await createAndConnectTarget( + serverRef, + autoCleanup.signal, + { + app: 'bar-app', + id: 'page1', + title: 'bar-title', + vm: 'bar-vm', + }, + { + host: '192.168.0.123:' + serverRef.port, + }, + ); + try { + let fetchCalledWithURL; + fetchSpy.mockImplementationOnce(url => { + fetchCalledWithURL = url instanceof URL ? url : null; + throw new Error('Unreachable'); + }); + const sourceMapURL = `${protocol.toLowerCase()}://192.168.0.123:${ + serverRef.port + }/source-map`; + const scriptParsedMessage = await sendFromTargetToDebugger( + device, + debugger_, + 'page1', + { + method: 'Debugger.scriptParsed', + params: { + sourceMapURL, + }, + }, + ); + expect(fetchCalledWithURL?.href).toEqual(sourceMapURL); + expect(scriptParsedMessage.params.sourceMapURL).toEqual( + `${protocol.toLowerCase()}://192.168.0.123:${serverRef.port}/source-map`, + ); + } finally { + device.close(); + debugger_.close(); + } + }); + describe.each(['10.0.2.2', '10.0.3.2', '127.0.0.1'])( '%s aliasing to and from localhost', sourceHost => { @@ -202,6 +299,9 @@ describe.each(['HTTP', 'HTTPS'])( title: 'bar-title', vm: 'bar-vm', }, + { + host: sourceHost + ':' + serverRef.port, + }, ); try { const scriptParsedMessage = await sendFromTargetToDebugger( @@ -236,6 +336,9 @@ describe.each(['HTTP', 'HTTPS'])( title: 'bar-title', vm: 'bar-vm', }, + { + host: sourceHost + ':' + serverRef.port, + }, ); try { const scriptParsedMessage = await sendFromTargetToDebugger( @@ -284,11 +387,11 @@ describe.each(['HTTP', 'HTTPS'])( method: 'Debugger.setBreakpointByUrl', params: { lineNumber: 1, - urlRegex: 'localhost:1000|localhost:2000', + urlRegex: `localhost:${serverRef.port}|example.com:2000`, }, }); expect(setBreakpointByUrlRegexMessage.params.urlRegex).toEqual( - `${sourceHost}:1000|${sourceHost}:2000`, + `${sourceHost}:${serverRef.port}|example.com:2000`, ); } finally { device.close(); @@ -307,6 +410,9 @@ describe.each(['HTTP', 'HTTPS'])( title: 'bar-title', vm: 'bar-vm', }, + { + host: sourceHost + ':' + serverRef.port, + }, ); try { const response = await debugger_.sendAndGetResponse({ @@ -344,6 +450,9 @@ describe.each(['HTTP', 'HTTPS'])( title: 'bar-title', vm: 'bar-vm', }, + { + host: '127.0.0.1:' + serverRef.port, + }, ); try { const scriptParsedMessage = await sendFromTargetToDebugger( diff --git a/packages/dev-middleware/src/inspector-proxy/Device.js b/packages/dev-middleware/src/inspector-proxy/Device.js index 5ba6f2cd01e..977519a17b0 100644 --- a/packages/dev-middleware/src/inspector-proxy/Device.js +++ b/packages/dev-middleware/src/inspector-proxy/Device.js @@ -40,7 +40,7 @@ const PAGES_POLLING_INTERVAL = 1000; // Replace hosts appearing in the `url` and `sourceMapURL` fields of // `Debugger.scriptParsed`, and back again in messages from the debugger, // to account for device/debugger/proxy running on different networks. -const REWRITE_HOSTS_TO_LOCALHOST: Array = [ +const REWRITE_HOSTS_TO_LOCALHOST: $ReadOnlySet = new Set([ // A device may retrieve a bundle through 127.0.0.1 via a (SSH) tunnel, but // the (remote) Metro server may be on a host without an IPv4 loopback, so // 127.0.0.1 may not be addressible locally for (e.g., for source map @@ -51,7 +51,7 @@ const REWRITE_HOSTS_TO_LOCALHOST: Array = [ // standard localhost alias. '10.0.2.2', '10.0.3.2', -]; +]); // Prefix for script URLs that are alphanumeric IDs. See comment in #processMessageFromDeviceLegacy method for // more details. @@ -79,6 +79,7 @@ export type DeviceOptions = $ReadOnly<{ projectRoot: string, eventReporter: ?EventReporter, createMessageMiddleware: ?CreateCustomMessageHandlerFn, + deviceRelativeBaseUrl: URL, serverRelativeBaseUrl: URL, }>; @@ -136,6 +137,10 @@ export default class Device { #connectedPageIds: Set = new Set(); + // A base HTTP(S) URL to this server, reachable from the device. Derived from + // the http request that created the connection. + #deviceRelativeBaseUrl: URL; + // A base HTTP(S) URL to the server, relative to this server. #serverRelativeBaseUrl: URL; @@ -152,6 +157,7 @@ export default class Device { eventReporter, createMessageMiddleware, serverRelativeBaseUrl, + deviceRelativeBaseUrl, }: DeviceOptions) { this.#id = id; this.#name = name; @@ -159,6 +165,7 @@ export default class Device { this.#deviceSocket = socket; this.#projectRoot = projectRoot; this.#serverRelativeBaseUrl = serverRelativeBaseUrl; + this.#deviceRelativeBaseUrl = deviceRelativeBaseUrl; this.#deviceEventReporter = eventReporter ? new DeviceEventReporter(eventReporter, { deviceId: id, @@ -676,19 +683,33 @@ export default class Device { const params = payload.params; if ('sourceMapURL' in params) { const sourceMapURL = this.#tryParseHTTPURL(params.sourceMapURL); + if (sourceMapURL) { + // This URL will be used to fetch from the server, and will be + // mutated if necessary from device-relative to server-relative. + // This is not exposed to the debugger. const serverRelativeUrl = new URL(sourceMapURL.href); - for (const hostToRewrite of REWRITE_HOSTS_TO_LOCALHOST) { - if (params.sourceMapURL.includes(hostToRewrite)) { - payload.params.sourceMapURL = params.sourceMapURL.replace( - hostToRewrite, - 'localhost', - ); - debuggerInfo.originalSourceURLAddress = hostToRewrite; - serverRelativeUrl.host = this.#serverRelativeBaseUrl.host; - serverRelativeUrl.protocol = this.#serverRelativeBaseUrl.protocol; - } + // Rewrite device-relative URLs to localhost-relative URLs for the + // debugger. + // TODO: Fix the assumption that localhost:[same port] is correct. + if ( + // sourceMapURL is a device-relative url to the server. + // May or may not be reachable from the frontend. + sourceMapURL.origin === this.#deviceRelativeBaseUrl.origin && + // For a specific set of IPs (eg 10.0.2.2) it's relatively safe to + // assume the frontend can reach the server on localhost. + // TODO: Fix the assumption that localhost:[same port] is correct + // and remove this check. + REWRITE_HOSTS_TO_LOCALHOST.has(this.#deviceRelativeBaseUrl.hostname) + ) { + const debuggerRelativeURL = new URL(sourceMapURL.href); + debuggerRelativeURL.hostname = 'localhost'; + serverRelativeUrl.host = this.#serverRelativeBaseUrl.host; + serverRelativeUrl.protocol = this.#serverRelativeBaseUrl.protocol; + debuggerInfo.originalSourceURLAddress = + this.#deviceRelativeBaseUrl.hostname; + payload.params.sourceMapURL = debuggerRelativeURL.href; } // Some debug clients do not support fetching HTTP URLs. If the @@ -712,21 +733,30 @@ export default class Device { const originalParamsUrl = params.url; let serverRelativeUrl = originalParamsUrl; const parsedUrl = this.#tryParseHTTPURL(originalParamsUrl); - if (parsedUrl) { - for (const hostToRewrite of REWRITE_HOSTS_TO_LOCALHOST) { - if (parsedUrl.hostname === hostToRewrite) { - // URL is device-relative and points to the host - rewrite it to - // use localhost. - parsedUrl.hostname = 'localhost'; - payload.params.url = parsedUrl.href; - debuggerInfo.originalSourceURLAddress = hostToRewrite; + // Rewrite device-relative URLs pointing to the server so that they're + // reachable from the frontend. + if ( + parsedUrl && + // url is a device-relative url to the server. + // May or may not be reachable from the frontend. + parsedUrl.origin === this.#deviceRelativeBaseUrl.origin && + // For a specific set of IPs (eg 10.0.2.2) it's relatively safe to + // assume the frontend can reach the server on localhost. + // TODO: Fix the assumption that localhost:[same port] is correct and + // remove this check. + REWRITE_HOSTS_TO_LOCALHOST.has(this.#deviceRelativeBaseUrl.hostname) + ) { + // URL is device-relative and points to the host - rewrite it to + // use localhost. + parsedUrl.hostname = 'localhost'; + payload.params.url = parsedUrl.href; + debuggerInfo.originalSourceURLAddress = + this.#deviceRelativeBaseUrl.hostname; - // Determine the server-relative URL. - parsedUrl.host = this.#serverRelativeBaseUrl.host; - parsedUrl.protocol = this.#serverRelativeBaseUrl.protocol; - serverRelativeUrl = parsedUrl.href; - } - } + // Determine the server-relative URL. + parsedUrl.host = this.#serverRelativeBaseUrl.host; + parsedUrl.protocol = this.#serverRelativeBaseUrl.protocol; + serverRelativeUrl = parsedUrl.href; } // Chrome doesn't download source maps if URL param is not a valid diff --git a/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js b/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js index 119357db78c..23a9f7b2d1f 100644 --- a/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js +++ b/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js @@ -226,7 +226,11 @@ export default class InspectorProxy implements InspectorProxyQueries { const deviceName = query.name || 'Unknown'; const appName = query.app || 'Unknown'; + const deviceRelativeBaseUrl = + getBaseUrlFromRequest(req) ?? this.#serverBaseUrl; + const oldDevice = this.#devices.get(deviceId); + let newDevice; const deviceOptions: DeviceOptions = { id: deviceId, @@ -236,6 +240,7 @@ export default class InspectorProxy implements InspectorProxyQueries { projectRoot: this.#projectRoot, eventReporter: this.#eventReporter, createMessageMiddleware: this.#customMessageHandler, + deviceRelativeBaseUrl, serverRelativeBaseUrl: this.#serverBaseUrl, }; @@ -249,7 +254,7 @@ export default class InspectorProxy implements InspectorProxyQueries { this.#devices.set(deviceId, newDevice); debug( - `Got new connection: name=${deviceName}, app=${appName}, device=${deviceId}`, + `Got new connection: name=${deviceName}, app=${appName}, device=${deviceId}, via=${deviceRelativeBaseUrl.origin}`, ); socket.on('close', () => {