Skip to content

Commit

Permalink
dev-middleware: Only rewrite hostnames if they match device connectio…
Browse files Browse the repository at this point in the history 
…n hosts (#47685)

Summary:
Pull Request resolved: #47685

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

fbshipit-source-id: bad28717b0c9b1ca43e2ea3391cef13f87892e6c
  • Loading branch information
@robhogan @facebook-github-bot
robhogan authored and facebook-github-bot committed Nov 20, 2024
1 parent d658ff5 commit 69400be
Show file tree
Hide file tree
Showing 6 changed files with 204 additions and 48 deletions.
25 changes: 11 additions & 14 deletions packages/dev-middleware/src/__tests__/FetchUtils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down Expand Up @@ -66,27 +64,26 @@ export async function fetchJson<T: JSONSerializable>(url: string): Promise<T> {
* 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<Parameters<typeof fetch>, ReturnType<typeof fetch>>,
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(() => {
Expand Down
12 changes: 10 additions & 2 deletions packages/dev-middleware/src/__tests__/InspectorDeviceUtils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,17 @@ export class DeviceAgent {
#ws: ?WebSocket;
#readyPromise: Promise<void>;

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 => {
Expand Down Expand Up @@ -160,8 +167,9 @@ export class DeviceMock extends DeviceAgent {
export async function createDeviceMock(
url: string,
signal: AbortSignal,
host?: ?string,
): Promise<DeviceMock> {
const device = new DeviceMock(url, signal);
const device = new DeviceMock(url, signal, host);
await device.ready();
return device;
}
9 changes: 8 additions & 1 deletion packages/dev-middleware/src/__tests__/InspectorProtocolUtils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand All @@ -136,6 +142,7 @@ export async function createAndConnectTarget(
deviceId ?? 'device' + Date.now()
}&name=foo&app=bar`,
signal,
host,
);
device.getPages.mockImplementation(() => [page]);

Expand Down
115 changes: 112 additions & 3 deletions packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,26 @@ jest.useRealTimers();

jest.setTimeout(10000);

const fetchOriginal = fetch;
const fetchSpy: JestMockFn<
Parameters<typeof fetch>,
ReturnType<typeof fetch>,
> = 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({
logger: undefined,
projectRoot: __dirname,
secure: protocol === 'HTTPS',
});

const autoCleanup = withAbortSignalForEachTest();
afterEach(() => {
jest.clearAllMocks();
Expand Down Expand Up @@ -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 => {
Expand All @@ -202,6 +299,9 @@ describe.each(['HTTP', 'HTTPS'])(
title: 'bar-title',
vm: 'bar-vm',
},
{
host: sourceHost + ':' + serverRef.port,
},
);
try {
const scriptParsedMessage = await sendFromTargetToDebugger(
Expand Down Expand Up @@ -236,6 +336,9 @@ describe.each(['HTTP', 'HTTPS'])(
title: 'bar-title',
vm: 'bar-vm',
},
{
host: sourceHost + ':' + serverRef.port,
},
);
try {
const scriptParsedMessage = await sendFromTargetToDebugger(
Expand Down Expand Up @@ -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();
Expand All @@ -307,6 +410,9 @@ describe.each(['HTTP', 'HTTPS'])(
title: 'bar-title',
vm: 'bar-vm',
},
{
host: sourceHost + ':' + serverRef.port,
},
);
try {
const response = await debugger_.sendAndGetResponse({
Expand Down Expand Up @@ -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(
Expand Down
Loading

0 comments on commit 69400be

Please sign in to comment.