diff --git a/README.md b/README.md index 8af0671b6..16cd97381 100644 --- a/README.md +++ b/README.md @@ -16,6 +16,14 @@ deciding whether it fits your use. Grouped by area rather than listed change by change. The detail is in `TESTING.md` and the commit history. +**Web server** + +- The web server binds the loopback interface by default. It used to listen on every interface, so + any machine on the same network reached an API that has no authentication and that lists, reads, + writes and deletes in the library. `-Dstudio.host` and `-Dstudio.port` override the address, and + the frontend no longer names a port anywhere: it uses whatever origin served the page. +- A port already in use is reported instead of leaving a running process that serves nothing. + **Device and transport** - The partition search waits for the OS to mount the device instead of giving up after ten seconds, @@ -137,6 +145,20 @@ this exists on top of. Licence, attribution and disclaimers are unchanged and re fork can be rebased on upstream if it becomes active again; until then the changes above are maintained here. +## Code from other forks + +Parts of this fork come from other people's forks rather than from upstream. They are listed here +because the licence alone does not say who did the work. + +**Configurable listen host and port** — from [@kairoh](https://github.com/kairoh)'s fork, commit +[`74f53cc`](https://github.com/kairoh/studio/commit/74f53cc57b70734e015f0cd31f036ca57ff3ea47) +("Configurable listen host and port", 2 April 2022), which predates that fork's move to Quarkus and +so applied to the same Vert.x code this fork still runs. Taken from it: reading the host and port +from configuration instead of hard-coding them, deriving the CORS pattern and the browser URL from +the host, and serving the whole web UI from relative URLs so the frontend stops naming a port. Not +taken from it: binding to the loopback by default, and reporting a failed bind — that commit keeps +`listen(port)`, which still accepts connections on every interface. Both projects are MPL-2.0. + --- The rest of this file is the upstream README, kept as it was except where it would say something diff --git a/TESTING.md b/TESTING.md index c7a512ccd..de11fd77d 100644 --- a/TESTING.md +++ b/TESTING.md @@ -75,7 +75,7 @@ Nothing here touches a device. Fixtures are synthesised in code; no device data | Suite | Tests | | --- | --- | -| Java, standard | **282**, 14 skipped — the opt-in FAT32 classes, and two link cases each of which only one platform can set up | +| Java, standard | **287**, 14 skipped — the opt-in FAT32 classes, and two link cases each of which only one platform can set up | | Java, with `-Dstudio.test.fat32.root=` | last measured at **172** before the C6d-5 additions; not re-measured since, because it needs the volume mounted | | JavaScript | **57** | @@ -121,6 +121,7 @@ counts. | When a conversion is proven to match its source, and every reason it is not | `ConversionVerificationTest` | `web-ui` module; **specifications** — only MATCH removes a confirmation; a path outside the library is refused rather than answered | | Where a library operation may reach: direct children only, links refused, nominal cases intact | `LibraryPathConfinementTest` | `web-ui` module; **specifications** — converted from characterization once the confinement existed. The symbolic-link case is Linux-only and the junction case Windows-only | | A conversion releases its source and its temporary even when the reader or writer throws | `ConversionStreamLifecycleTest` | `web-ui` module; **specifications** — asserts the consequence by deleting the work folder, so the three failure cases are Windows-only | +| Which interfaces the web server accepts connections on, and what an occupied port does | `ServerBindingTest` | `web-ui` module; **specifications** — deploys the real `MainVerticle` with `env=dev` and every path pointed at a temporary directory, so no device and no network are touched. The two cases needing a non-loopback address are skipped on a machine that has none. Deliberately silent on the log message and on the browser, which have no seam | Web UI (`web-ui/javascript`, run by yarn): diff --git a/web-ui/javascript/.env.development b/web-ui/javascript/.env.development new file mode 100644 index 000000000..14fdd2658 --- /dev/null +++ b/web-ui/javascript/.env.development @@ -0,0 +1,5 @@ +# `yarn start` serves the app from :3000 while the Java backend listens on :8080. The `proxy` entry +# in package.json forwards /api and /locales, but the CRA dev server does not proxy the SockJS +# connection, so the event bus is pointed straight at the backend here. In a packaged build this +# variable is unset and the origin the page was served from is used instead. +REACT_APP_EVENTBUS_ORIGIN=http://localhost:8080 diff --git a/web-ui/javascript/package.json b/web-ui/javascript/package.json index 610ecb3e1..1ba8138d6 100644 --- a/web-ui/javascript/package.json +++ b/web-ui/javascript/package.json @@ -6,6 +6,7 @@ "author": "Marian MULLER REBEYROL", "license": "MPL-2.0", "private": true, + "proxy": "http://localhost:8080", "dependencies": { "@emotion/core": "^10.0.22", "@emotion/styled": "^10.0.23", diff --git a/web-ui/javascript/public/index.html b/web-ui/javascript/public/index.html index 75b07fcec..cf4d55a3f 100644 --- a/web-ui/javascript/public/index.html +++ b/web-ui/javascript/public/index.html @@ -9,7 +9,7 @@ STUdio - Story Teller Unleashed - + diff --git a/web-ui/javascript/src/App.js b/web-ui/javascript/src/App.js index 72c311e3e..a97a931bf 100644 --- a/web-ui/javascript/src/App.js +++ b/web-ui/javascript/src/App.js @@ -65,8 +65,14 @@ class App extends React.Component { // from inside a setState callback, one React tick later; the underlying client fires onopen // at most once and only if it is already assigned, so a socket that opened during that tick // left the application with no device handlers at all for the lifetime of the page. + // + // Derived from the page's own origin rather than relative, unlike the `fetch` calls in + // `services/`: sockjs-client rejects a URL with no host and no protocol (sockjs.js, "The + // URL '...' is invalid"), so '/eventbus' would throw at construction. The origin still + // follows whatever host and port served the page, which is the point. console.log("Setting up vert.x event bus channel..."); - const channel = createEventBusChannel('http://localhost:8080/eventbus', { + const eventBusOrigin = process.env.REACT_APP_EVENTBUS_ORIGIN || window.location.origin; + const channel = createEventBusChannel(eventBusOrigin + '/eventbus', { onStateChange: state => this.onChannelStateChange(state) }); diff --git a/web-ui/javascript/src/i18n.js b/web-ui/javascript/src/i18n.js index 9f1dc5944..6dfbf3e20 100644 --- a/web-ui/javascript/src/i18n.js +++ b/web-ui/javascript/src/i18n.js @@ -20,7 +20,7 @@ i18n .init({ fallbackLng: 'en', backend: { - loadPath: 'http://localhost:8080/locales/{{lng}}/{{ns}}.json', + loadPath: '/locales/{{lng}}/{{ns}}.json', }, interpolation: { escapeValue: false, // not needed for react as it escapes by default diff --git a/web-ui/javascript/src/services/device.js b/web-ui/javascript/src/services/device.js index f773519cd..30cf16ab9 100644 --- a/web-ui/javascript/src/services/device.js +++ b/web-ui/javascript/src/services/device.js @@ -7,17 +7,17 @@ import {handleJsonOrError} from "../utils/fetch"; export const fetchDeviceInfos = () => { - return fetch('http://localhost:8080/api/device/infos') + return fetch('/api/device/infos') .then(handleJsonOrError); }; export const fetchDevicePacks = () => { - return fetch('http://localhost:8080/api/device/packs') + return fetch('/api/device/packs') .then(handleJsonOrError); }; export const addFromLibrary = (uuid, path) => { - return fetch('http://localhost:8080/api/device/addFromLibrary', { + return fetch('/api/device/addFromLibrary', { method: "POST", headers: { "Content-Type" : "application/json" }, body: JSON.stringify({uuid, path}) @@ -26,7 +26,7 @@ export const addFromLibrary = (uuid, path) => { }; export const removeFromDevice = (uuid) => { - return fetch('http://localhost:8080/api/device/removeFromDevice', { + return fetch('/api/device/removeFromDevice', { method: "POST", headers: { "Content-Type" : "application/json" }, body: JSON.stringify({uuid}) @@ -35,7 +35,7 @@ export const removeFromDevice = (uuid) => { }; export const reorderPacks = (uuids) => { - return fetch('http://localhost:8080/api/device/reorder', { + return fetch('/api/device/reorder', { method: "POST", headers: { "Content-Type" : "application/json" }, body: JSON.stringify({uuids}) @@ -44,7 +44,7 @@ export const reorderPacks = (uuids) => { }; export const addToLibrary = (uuid, driver) => { - return fetch('http://localhost:8080/api/device/addToLibrary', { + return fetch('/api/device/addToLibrary', { method: "POST", headers: { "Content-Type" : "application/json" }, body: JSON.stringify({uuid, driver}) diff --git a/web-ui/javascript/src/services/evergreen.js b/web-ui/javascript/src/services/evergreen.js index dd28233b5..fce17e1da 100644 --- a/web-ui/javascript/src/services/evergreen.js +++ b/web-ui/javascript/src/services/evergreen.js @@ -7,16 +7,16 @@ import {handleJsonOrError} from "../utils/fetch"; export const fetchEvergreenInfos = () => { - return fetch('http://localhost:8080/api/evergreen/infos') + return fetch('/api/evergreen/infos') .then(handleJsonOrError); }; export const fetchEvergreenLatestRelease = () => { - return fetch('http://localhost:8080/api/evergreen/latest') + return fetch('/api/evergreen/latest') .then(handleJsonOrError); }; export const fetchEvergreenAnnounce = () => { - return fetch('http://localhost:8080/api/evergreen/announce') + return fetch('/api/evergreen/announce') .then(handleJsonOrError); }; diff --git a/web-ui/javascript/src/services/library.js b/web-ui/javascript/src/services/library.js index b2be01537..5f555935d 100644 --- a/web-ui/javascript/src/services/library.js +++ b/web-ui/javascript/src/services/library.js @@ -7,17 +7,17 @@ import {handleJsonOrError} from "../utils/fetch"; export const fetchLibraryInfos = () => { - return fetch('http://localhost:8080/api/library/infos') + return fetch('/api/library/infos') .then(handleJsonOrError); }; export const fetchLibraryPacks = () => { - return fetch('http://localhost:8080/api/library/packs') + return fetch('/api/library/packs') .then(handleJsonOrError); }; export const downloadFromLibrary = async (uuid, path) => { - return await fetch('http://localhost:8080/api/library/download', { + return await fetch('/api/library/download', { method: "POST", headers: {"Content-Type": "application/json"}, body: JSON.stringify({uuid, path}) @@ -34,7 +34,7 @@ export const uploadToLibrary = async (uuid, path, packData, progressHandler) => console.log('xhr upload complete: ' + JSON.parse(xhr.responseText)); resolve(JSON.parse(xhr.responseText)); }; - xhr.open('post', 'http://localhost:8080/api/library/upload', true); + xhr.open('post', '/api/library/upload', true); let formData = new FormData(); formData.append("uuid", uuid); formData.append("path", path); @@ -44,7 +44,7 @@ export const uploadToLibrary = async (uuid, path, packData, progressHandler) => }; export const convertInLibrary = async (uuid, path, format, allowEnriched) => { - return await fetch('http://localhost:8080/api/library/convert', { + return await fetch('/api/library/convert', { method: "POST", headers: {"Content-Type": "application/json"}, body: JSON.stringify({uuid, path, format, allowEnriched}) @@ -53,7 +53,7 @@ export const convertInLibrary = async (uuid, path, format, allowEnriched) => { }; export const removeFromLibrary = (path) => { - return fetch('http://localhost:8080/api/library/remove', { + return fetch('/api/library/remove', { method: "POST", headers: { "Content-Type" : "application/json" }, body: JSON.stringify({path}) @@ -69,7 +69,7 @@ export const removeFromLibrary = (path) => { * this side has nothing to decide and nothing to get wrong. */ export const verifyConversion = async (sourcePath, convertedPath) => { - return await fetch('http://localhost:8080/api/library/verify-conversion', { + return await fetch('/api/library/verify-conversion', { method: "POST", headers: {"Content-Type": "application/json"}, body: JSON.stringify({sourcePath, convertedPath}) diff --git a/web-ui/src/main/java/studio/webui/MainVerticle.java b/web-ui/src/main/java/studio/webui/MainVerticle.java index 2a65d461a..1de774c06 100644 --- a/web-ui/src/main/java/studio/webui/MainVerticle.java +++ b/web-ui/src/main/java/studio/webui/MainVerticle.java @@ -61,10 +61,22 @@ public void start() { } + // Making the host and port configurable, and deriving the CORS pattern and the browser URL + // from them, comes from kairoh's fork — commit 74f53cc, "Configurable listen host and + // port" (2022-04-02), MPL-2.0 like this file. See "Code from other forks" in the README. + // + // Listen address. The loopback default is deliberate: this is a desktop application whose + // UI is served to a browser on the same machine, and the API below is unauthenticated — + // it lists, reads, writes and deletes in the user's library, and drives the device. Bound + // to every interface, as `listen(8080)` did, any host on the same network segment reached + // it. Overriding the host is possible, but it is now an explicit choice. + String host = System.getProperty("studio.host", "127.0.0.1"); + int port = Integer.parseInt(System.getProperty("studio.port", "8080")); + Router router = Router.router(vertx); // Handle cross-origin calls - router.route().handler(CorsHandler.create("http://localhost:.*") + router.route().handler(CorsHandler.create("http://" + host + ":.*") .allowedMethods(Set.of( HttpMethod.GET, HttpMethod.POST @@ -97,16 +109,30 @@ public void start() { errorHandler.handle(ctx); }); - // Start HTTP server - vertx.createHttpServer().requestHandler(router).listen(8080); + // Start HTTP server. The handler is not optional: `listen()` without one discards the + // failure, and a port already in use then leaves a running process that serves nothing + // and says nothing. The browser is opened only once the socket is actually bound. + String url = "http://" + host + ":" + port; + vertx.createHttpServer().requestHandler(router).listen(port, host, ar -> { + if (ar.failed()) { + LOGGER.error("Failed to listen on " + host + ":" + port + + " - the port may already be in use. Set -Dstudio.port to another one.", + ar.cause()); + return; + } + LOGGER.info("Listening on " + url); + openInBrowser(url); + }); + } + private void openInBrowser(String url) { // Automatically open URL in browser, unless instructed otherwise String openBrowser = System.getProperty("studio.open", "true"); if (Boolean.valueOf(openBrowser)) { LOGGER.info("Opening URL in default browser..."); if (Desktop.isDesktopSupported()) { try { - Desktop.getDesktop().browse(new URI("http://localhost:8080")); + Desktop.getDesktop().browse(new URI(url)); } catch (Exception e) { LOGGER.error("Failed to open URL in default browser", e); } diff --git a/web-ui/src/test/java/studio/webui/ServerBindingTest.java b/web-ui/src/test/java/studio/webui/ServerBindingTest.java new file mode 100644 index 000000000..6ac9e54c4 --- /dev/null +++ b/web-ui/src/test/java/studio/webui/ServerBindingTest.java @@ -0,0 +1,286 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. + */ + +package studio.webui; + +import io.vertx.core.Vertx; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.NetworkInterface; +import java.net.ServerSocket; +import java.net.Socket; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +/** + * Which interfaces the web server accepts connections on. + * + *

These are specifications, not characterization. The behaviour they describe + * replaced an earlier one: {@code listen(8080)} uses Vert.x's default host, {@code 0.0.0.0}, so + * every interface accepted connections to an API that has no authentication and that lists, + * downloads, uploads, converts and deletes in the library, and drives a connected device. Anyone on + * the same network segment reached it, and since the launcher opens a browser on {@code localhost} + * the user had no reason to suspect otherwise. + * + *

The rule is one sentence: the server listens on the loopback unless it is told + * otherwise. Widening it stays possible, because someone may genuinely want to reach their + * own library from a tablet, but it is now a decision someone made rather than the default. + * + *

What these tests deliberately do not assert: + * + *

+ */ +@DisplayName("Where the web server accepts connections") +class ServerBindingTest { + + /** Every system property these tests write, saved and restored around each one. */ + private static final List TOUCHED_PROPERTIES = List.of( + "studio.host", "studio.port", "studio.open", "env", + "studio.library", "studio.tmpdir", "studio.db.official", "studio.db.unofficial"); + + private final Map savedProperties = new HashMap<>(); + + @TempDir + Path studioHome; + + private Vertx vertx; + + @BeforeEach + void setUp() throws IOException { + for (String property : TOUCHED_PROPERTIES) { + savedProperties.put(property, System.getProperty(property)); + } + + // Point every path at the temporary directory. The official database in particular: when + // the file is missing the service falls back to fetching it over the network, and a test + // that quietly downloads a database is a test that fails on a train. + Path officialDb = studioHome.resolve("official.json"); + Files.write(officialDb, "{}".getBytes(StandardCharsets.UTF_8)); + System.setProperty("studio.db.official", officialDb.toString()); + System.setProperty("studio.db.unofficial", studioHome.resolve("unofficial.json").toString()); + System.setProperty("studio.library", + Files.createDirectories(studioHome.resolve("library")) + "/"); + System.setProperty("studio.tmpdir", + Files.createDirectories(studioHome.resolve("tmp")) + "/"); + + System.setProperty("env", "dev"); // mock story teller, no libusb + System.setProperty("studio.open", "false"); // no browser + + vertx = Vertx.vertx(); + } + + @AfterEach + void tearDown() throws InterruptedException { + if (vertx != null) { + CountDownLatch closed = new CountDownLatch(1); + vertx.close(ar -> closed.countDown()); + closed.await(30, TimeUnit.SECONDS); + } + savedProperties.forEach((property, value) -> { + if (value == null) { + System.clearProperty(property); + } else { + System.setProperty(property, value); + } + }); + } + + @Nested + @DisplayName("By default") + class ByDefault { + + @Test + @DisplayName("it accepts connections on the loopback") + void acceptsLoopback() throws Exception { + int port = freePort(); + System.setProperty("studio.port", Integer.toString(port)); + + deployMainVerticle(); + + assertTrue(waitForConnection("127.0.0.1", port), + "the server should be reachable on the loopback"); + } + + @Test + @DisplayName("it refuses connections on every other interface") + void refusesNonLoopback() throws Exception { + String externalAddress = nonLoopbackAddress().orElse(null); + assumeTrue(externalAddress != null, + "no non-loopback IPv4 address on this machine, nothing to refuse"); + + int port = freePort(); + System.setProperty("studio.port", Integer.toString(port)); + + deployMainVerticle(); + + // The loopback check first: once it answers, the socket is bound and a refusal on the + // other address is a real refusal rather than a server that has not started yet. + assertTrue(waitForConnection("127.0.0.1", port), "the server should have started"); + assertFalse(connectsOnce(externalAddress, port), + "the server should not be reachable on " + externalAddress); + } + } + + @Nested + @DisplayName("When told otherwise") + class WhenTold { + + @Test + @DisplayName("studio.port chooses the port") + void portIsHonoured() throws Exception { + int port = freePort(); + System.setProperty("studio.port", Integer.toString(port)); + + deployMainVerticle(); + + assertTrue(waitForConnection("127.0.0.1", port), + "the server should listen on the port it was given"); + } + + @Test + @DisplayName("studio.host widens the binding, which is what makes the default a choice") + void hostCanWidenTheBinding() throws Exception { + String externalAddress = nonLoopbackAddress().orElse(null); + assumeTrue(externalAddress != null, + "no non-loopback IPv4 address on this machine, nothing to widen onto"); + + int port = freePort(); + System.setProperty("studio.port", Integer.toString(port)); + System.setProperty("studio.host", "0.0.0.0"); + + deployMainVerticle(); + + assertTrue(waitForConnection(externalAddress, port), + "an explicit 0.0.0.0 should accept connections on " + externalAddress); + } + } + + @Nested + @DisplayName("When the port is already in use") + class PortInUse { + + @Test + @DisplayName("the occupant keeps it, and the application stays up") + void occupiedPortIsSurvived() throws Exception { + try (ServerSocket occupant = new ServerSocket()) { + occupant.bind(new InetSocketAddress("127.0.0.1", 0)); + int port = occupant.getLocalPort(); + System.setProperty("studio.port", Integer.toString(port)); + + // Deployment itself must not fail: `listen` is asynchronous, and the point of the + // handler added alongside these tests is that its failure is dealt with rather + // than discarded. Before that handler existed this test still passed — what it + // guards is that the failure never becomes an exception nobody catches. + deployMainVerticle(); + + assertTrue(occupant.isBound() && !occupant.isClosed(), + "the socket that already held the port should still hold it"); + } + } + } + + // ---------------------------------------------------------------- helpers + + private void deployMainVerticle() throws InterruptedException { + CountDownLatch deployed = new CountDownLatch(1); + AtomicReference failure = new AtomicReference<>(); + vertx.deployVerticle(new MainVerticle(), ar -> { + if (ar.failed()) { + failure.set(ar.cause()); + } + deployed.countDown(); + }); + assertTrue(deployed.await(60, TimeUnit.SECONDS), "deploying MainVerticle timed out"); + if (failure.get() != null) { + fail("deploying MainVerticle failed", failure.get()); + } + } + + /** + * A port nothing is listening on. Racy by nature — anything may take it between the close and + * the bind under test — but the window is small and the alternative, a fixed port, collides + * with whatever else the machine is running. + */ + private static int freePort() throws IOException { + try (ServerSocket socket = new ServerSocket(0)) { + return socket.getLocalPort(); + } + } + + /** The server binds asynchronously, so give it a moment to appear before concluding. */ + private static boolean waitForConnection(String host, int port) throws InterruptedException { + long deadline = System.currentTimeMillis() + 20_000; + do { + if (connectsOnce(host, port)) { + return true; + } + Thread.sleep(50); + } while (System.currentTimeMillis() < deadline); + return false; + } + + private static boolean connectsOnce(String host, int port) { + try (Socket socket = new Socket()) { + socket.connect(new InetSocketAddress(host, port), 1_000); + return true; + } catch (IOException refusedOrUnreachable) { + return false; + } + } + + /** + * An IPv4 address of this machine that is not the loopback, if it has one. A CI runner + * normally does; a machine with no network does not, and the cases needing one are skipped + * rather than failed. + */ + private static Optional nonLoopbackAddress() throws Exception { + List interfaces = + new ArrayList<>(Collections.list(NetworkInterface.getNetworkInterfaces())); + for (NetworkInterface networkInterface : interfaces) { + if (!networkInterface.isUp() || networkInterface.isLoopback()) { + continue; + } + for (InetAddress address : Collections.list(networkInterface.getInetAddresses())) { + if (address.getAddress().length == 4 && !address.isLoopbackAddress()) { + return Optional.of(address.getHostAddress()); + } + } + } + return Optional.empty(); + } +}