From 560adc7dc6d1efe8d70db934b1db43fefcdb1f91 Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Thu, 26 Sep 2024 15:19:54 +0000 Subject: [PATCH] [dds/devtools] Switch to always using a relative base href in DevTools pages This avoids the server needing to know the exact URL path being used on the client, so proxies that rewrite the path (such as code-server) can still work. See https://github.com/flutter/devtools/issues/8067#issuecomment-2236419703 Change-Id: I5b131d84232c63f5495ccbb533da727cce514de5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/376682 Reviewed-by: Kenzie Davisson Commit-Queue: Ben Konyi Reviewed-by: Ben Konyi --- pkg/dds/lib/src/devtools/handler.dart | 22 +++++ .../test/devtools_server/base_href_test.dart | 54 ++++++++++++ ...evtools_server_path_strategy_dds_test.dart | 76 ++++++++++++----- .../devtools_server_path_strategy_test.dart | 83 ++++++++++++------- 4 files changed, 183 insertions(+), 52 deletions(-) create mode 100644 pkg/dds/test/devtools_server/base_href_test.dart diff --git a/pkg/dds/lib/src/devtools/handler.dart b/pkg/dds/lib/src/devtools/handler.dart index ef95f811c4c..32a058fc3f8 100644 --- a/pkg/dds/lib/src/devtools/handler.dart +++ b/pkg/dds/lib/src/devtools/handler.dart @@ -224,6 +224,15 @@ Future _serveStaticFile( if (baseHref != null) { assert(baseHref.startsWith('/')); assert(baseHref.endsWith('/')); + + // Always use a relative base href to support going through proxies that + // rewrite paths. For example if the server thinks we are hosting at + // `http://localhost/devtools/` but a frontend proxy means the client app + // is at `http://localhost/proxy/1234/devtools/` then we need the base href + // to effectively be `/proxy/1234/devtools/`, however we have no knowledge + // of `/proxy/1234` because it was stripped by the proxy on its way to us. + baseHref = computeRelativeBaseHref(baseHref, request.requestedUri); + // Replace the base href to match where the app is being served from. final baseHrefPattern = RegExp(r''); contents = contents.replaceFirst( @@ -233,3 +242,16 @@ Future _serveStaticFile( } return Response.ok(contents, headers: headers); } + +/// Computes a relative "base href" that can be used in place of +/// [absoluteBaseHref] for a request served at [requestUri]. +String computeRelativeBaseHref(String absoluteBaseHref, Uri requestUri) { + // path.relative will always treat `from` as if it's a directory, but for + // URIs that's not correct. If the request is /foo/bar then `.` is `/foo` and + // not `/foo/bar`. To handle this, trim the last segment if the request does + // not end with a slash. + final requestFolderPath = requestUri.path.endsWith('/') + ? requestUri.path + : path.posix.dirname(requestUri.path); + return path.posix.relative(absoluteBaseHref, from: requestFolderPath); +} diff --git a/pkg/dds/test/devtools_server/base_href_test.dart b/pkg/dds/test/devtools_server/base_href_test.dart new file mode 100644 index 00000000000..c5bb6b44164 --- /dev/null +++ b/pkg/dds/test/devtools_server/base_href_test.dart @@ -0,0 +1,54 @@ +// Copyright 2024 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:dds/src/devtools/handler.dart'; +import 'package:test/test.dart'; + +void main() { + /// A set of test cases to verify base hrefs for. + /// + /// The key is a suffix appended to / or /devtools/ depending on how DevTools + /// is hosted. + /// The value is the expected base href (which should always resolve back to + /// the root of where DevTools is being hosted). + final testBaseHrefs = { + '': '.', + 'inspector': '.', + 'inspector/': '..', + 'inspector/foo': '..', + 'inspector/foo/': '../..', + 'inspector/foo/bar': '../..', + 'inspector/foo/bar/baz': '../../..', + }; + + for (final MapEntry(key: suffix, value: expectedBaseHref) + in testBaseHrefs.entries) { + test('computes correct base href for /$suffix with devtools at root', + () async { + final actual = computeRelativeBaseHref( + '/', + Uri.parse('http://localhost/$suffix'), + ); + expect(actual, expectedBaseHref); + }); + + test('computes correct base href for /$suffix with devtools at /devtools/', + () async { + final actual = computeRelativeBaseHref( + '/devtools/', + Uri.parse('http://localhost/devtools/$suffix'), + ); + expect(actual, expectedBaseHref); + }); + + test('computes correct base href for /$suffix in a devtools extension', + () async { + final actual = computeRelativeBaseHref( + '/devtools/devtools_extension/foo/', + Uri.parse('http://localhost/devtools/devtools_extension/foo/$suffix'), + ); + expect(actual, expectedBaseHref); + }); + } +} diff --git a/pkg/dds/test/devtools_server/devtools_server_path_strategy_dds_test.dart b/pkg/dds/test/devtools_server/devtools_server_path_strategy_dds_test.dart index ac48d027517..2b61572de11 100644 --- a/pkg/dds/test/devtools_server/devtools_server_path_strategy_dds_test.dart +++ b/pkg/dds/test/devtools_server/devtools_server_path_strategy_dds_test.dart @@ -18,15 +18,21 @@ void main() { 'Future main() => Future.delayed(const Duration(minutes: 10));'; final tempDir = Directory.systemTemp.createTempSync('devtools_server.'); final devToolsBannerRegex = - RegExp(r'DevTools[\w\s]+at: (https?:.*\/devtools)'); + RegExp(r'DevTools[\w\s]+at: (https?:.*\/devtools/)'); + final baseHrefRegex = RegExp('(); proc.stderr .transform(utf8.decoder) @@ -51,24 +57,50 @@ void main() { } }, ); + devToolsUrl = Uri.parse(await completer.future); + }); - final devToolsUrl = Uri.parse('${await completer.future}/'); - final httpClient = HttpClient(); - late HttpClientResponse resp; - try { - final req = await httpClient.get(devToolsUrl.host, devToolsUrl.port, - '${devToolsUrl.path}/inspector'); - resp = await req.close(); + tearDownAll(() { + httpClient.close(force: true); + process?.kill(); + }); + + test('correct content for /token/devtools/', () async { + final uri = devToolsUrl; + final req = await httpClient.getUrl(uri); + final resp = await req.close(); + expect(resp.statusCode, 200); + final bodyContent = await resp.transform(utf8.decoder).join(); + expect(bodyContent, contains('Dart DevTools')); + }, timeout: const Timeout.factor(10)); + + /// A set of test cases to verify base hrefs for. + /// + /// The key is a suffix to go after /devtools/ in the URI. + /// The value is the expected base href (which should always resolve back to + /// `/devtools/` or in the case of an extension, the base of the extension). + final testBaseHrefs = { + '': '.', + 'inspector': '.', + // We can't test devtools_extensions here without having one set up, but + // their paths are also tested in `base_href_test.dart`. + }; + + for (final MapEntry(key: suffix, value: expectedBaseHref) + in testBaseHrefs.entries) { + test('with correct base href for /token/devtools/$suffix', () async { + final uri = Uri.parse('$devToolsUrl$suffix'); + final req = await httpClient.getUrl(uri); + final resp = await req.close(); expect(resp.statusCode, 200); final bodyContent = await resp.transform(utf8.decoder).join(); - expect(bodyContent, contains('Dart DevTools')); - final expectedBaseHref = htmlEscape.convert(devToolsUrl.path); - expect(bodyContent, contains('')); - } finally { - httpClient.close(); - } - } finally { - proc.kill(); + expect(bodyContent, contains(' map!['event'] == 'server.started', ))!; final host = startedEvent['params']['host']; final port = startedEvent['params']['port']; + devToolsUrl = Uri(scheme: 'http', host: host, port: port); + }); - final req = await httpClient.get(host, port, '/inspector'); - resp = await req.close(); + tearDownAll(() { + httpClient.close(force: true); + server?.kill(); + }); + + test('correct content for /inspector', () async { + final req = await httpClient.getUrl(devToolsUrl.resolve('/inspector')); + final resp = await req.close(); expect(resp.statusCode, 200); final bodyContent = await resp.transform(utf8.decoder).join(); expect(bodyContent, contains('Dart DevTools')); - final expectedBaseHref = htmlEscape.convert('/'); - expect(bodyContent, contains('')); - } finally { - httpClient.close(); - server.kill(); - } - }, timeout: const Timeout.factor(10)); - - test('serves 404 contents for requests that are not pages', () async { - final server = await DevToolsServerDriver.create(); - final httpClient = HttpClient(); - late HttpClientResponse resp; - try { - final startedEvent = (await server.stdout.firstWhere( - (map) => map!['event'] == 'server.started', - ))!; - final host = startedEvent['params']['host']; - final port = startedEvent['params']['port']; + }, timeout: const Timeout.factor(10)); + test('serves 404 for requests that are not pages', () async { // The index page is only served up for extension-less requests. - final req = await httpClient.get(host, port, '/inspector.html'); - resp = await req.close(); + final req = + await httpClient.getUrl(devToolsUrl.resolve('/inspector.html')); + final resp = await req.close(); expect(resp.statusCode, 404); - } finally { - httpClient.close(); await resp.drain(); - server.kill(); + }, timeout: const Timeout.factor(10)); + + /// A set of test cases to verify base hrefs for. + /// + /// The key is a suffix to go after /devtools/ in the URI. + /// The value is the expected base href (which should always resolve back to + /// `/devtools/` or in the case of an extension, the base of the extension). + final testBaseHrefs = { + '': '.', + 'inspector': '.', + // TODO(dantup): Is there a way we could verify extension URLs here? + // 'devtools_extensions/foo/': '.', + // 'devtools_extensions/foo/bar': '.', + // 'devtools_extensions/foo/bar/': '..', + // 'devtools_extensions/foo/bar/baz': '../..', + }; + + for (final MapEntry(key: suffix, value: expectedBaseHref) + in testBaseHrefs.entries) { + test('with correct base href for /$suffix', () async { + final req = await httpClient.getUrl(devToolsUrl.resolve('/inspector')); + final resp = await req.close(); + expect(resp.statusCode, 200); + final bodyContent = await resp.transform(utf8.decoder).join(); + + // Extract the base href so if the test failures, we get a simpler error + // than just the entire content. + final actualBaseHref = baseHrefRegex.firstMatch(bodyContent)!.group(1); + expect(actualBaseHref, htmlEscape.convert(expectedBaseHref)); + }, timeout: const Timeout.factor(10)); } }, timeout: const Timeout.factor(10)); }