From d17e48f93a11850e4fc8f24bb326aa4461772f5b Mon Sep 17 00:00:00 2001 From: Antoine Date: Sat, 5 Sep 2026 23:01:26 +0200 Subject: [PATCH] fix: bind the HTTP server to the loopback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server was started with listen(int), whose default host is 0.0.0.0. An API with no authentication was therefore reachable from every machine on the same network segment, which could read, write and delete in the user's library while STUdio ran. The CORS filter does not cover that: CORS is enforced by browsers on requests issued by a page, and says nothing to curl or a script on another machine. Nothing is lost, because remote use was never possible. The web UI is served by this same server and addresses it as http://localhost:8080, hardcoded throughout the frontend, so a browser on another machine would receive the page and then send every request to its own loopback. The wider binding exposed the API without ever making the application usable from elsewhere. The address is fixed rather than configurable. An override would keep the exposure reachable to buy back a capability that does not work, and the day remote access is genuinely wanted it will mean changing the frontend's addresses too. HttpServerBindingTest asserts the consequence rather than the value: it starts a real server on the address production uses, then opens real sockets. The loopback must complete a request and answer 200 — an open port proves only that a socket was accepted — and a routable address of this same machine must be refused. That case skips where the machine has no such address, since a machine that cannot set up the situation establishes nothing about it. Verified red twice, the second time against the final test: the connection really was established from this machine's LAN address. The address is a method, not a static final String. A compile-time constant is inlined into whatever reads it, so a test holding an inlined copy would keep binding to the address it was compiled against and stay green while production had been reopened. The first red proof passed only because the test happened to have been compiled against the old value. Two side effects of the new test are handled here rather than left behind: the request-and-answer exchange lets the server finish with the connection before teardown closes Vert.x, which removes a logged Netty stack trace; and *.log is ignored, since no test initialised log4j2 before and a run now left untracked files in the tree. Test counts move to 284 Java, 57 JavaScript unchanged. This narrows the exposure of CVE-2023-24815 in vertx-web 3.9 to the local machine; it does not fix it, and that check is still outstanding. Co-Authored-By: Claude Opus 5 --- .gitignore | 5 + README.md | 2 +- README_fr.md | 2 +- TESTING.md | 3 +- .../main/java/studio/webui/MainVerticle.java | 33 +++- .../studio/webui/HttpServerBindingTest.java | 156 ++++++++++++++++++ 6 files changed, 197 insertions(+), 4 deletions(-) create mode 100644 web-ui/src/test/java/studio/webui/HttpServerBindingTest.java diff --git a/.gitignore b/.gitignore index 8e837c50d..de151679c 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,8 @@ *target web-ui/javascript/build web-ui/javascript/report.*.json +# log4j2 writes studio-latest.log and a dated roll-over beside the working directory. The +# application has always done this when run from a checkout; a test that starts the HTTP server +# now does it too, so a test run no longer leaves untracked files behind. +studio-latest.log +studio-*.log diff --git a/README.md b/README.md index 8af0671b6..5d0781fe6 100644 --- a/README.md +++ b/README.md @@ -91,7 +91,7 @@ Use the operational protocol in `FIELD-VALIDATION.md` if you are writing to a re ## Testing -At the time of writing: **282 Java tests** in the standard suite and **57 JavaScript tests**, all +At the time of writing: **284 Java tests** in the standard suite and **57 JavaScript tests**, all green, on Linux and on Windows. `TESTING.md` holds the current counts, the opt-in FAT32 figure and its caveats, and is the authority — the numbers here will go stale before it does. diff --git a/README_fr.md b/README_fr.md index c83ee6110..260f3acb4 100644 --- a/README_fr.md +++ b/README_fr.md @@ -102,7 +102,7 @@ Utilisez le protocole opératoire de `FIELD-VALIDATION.md` si vous écrivez sur ## Tests -À l'heure où ces lignes sont écrites : **282 tests Java** dans la suite standard et **57 tests +À l'heure où ces lignes sont écrites : **284 tests Java** dans la suite standard et **57 tests JavaScript**, tous verts, sur Linux comme sur Windows. `TESTING.md` porte les comptes à jour, le chiffre de la suite FAT32 optionnelle et ses réserves, et fait autorité — les nombres cités ici se périmeront avant lui. diff --git a/TESTING.md b/TESTING.md index c7a512ccd..5f94946b3 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 | **284**, 14 skipped — the opt-in FAT32 classes, and two link cases each of which only one platform can set up. A third conditional case, the non-loopback binding check, skips only on a machine with no routable address | | 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 | +| Where the HTTP server can be reached from: the loopback answers, another address of this machine does not | `HttpServerBindingTest` | `web-ui` module; **specifications** — opens real sockets against a real server rather than reading the constant back. The refusal case needs a non-loopback address to aim at and skips where there is none | Web UI (`web-ui/javascript`, run by yarn): diff --git a/web-ui/src/main/java/studio/webui/MainVerticle.java b/web-ui/src/main/java/studio/webui/MainVerticle.java index 2a65d461a..4fa1e8e3f 100644 --- a/web-ui/src/main/java/studio/webui/MainVerticle.java +++ b/web-ui/src/main/java/studio/webui/MainVerticle.java @@ -33,6 +33,37 @@ public class MainVerticle extends AbstractVerticle { + /** + * The address the HTTP server binds to: the loopback, and only the loopback. + * + *

Named rather than left to {@code listen(int)}, whose default is {@code 0.0.0.0} — every + * interface. That default put an API with no authentication on the local network, where anything + * on the same segment could read, write and delete in the user's library while STUdio ran. The + * CORS filter below does not cover it: CORS is enforced by browsers, on requests issued by a + * page, and says nothing to {@code curl} or a script on another machine. + * + *

Nothing is lost by restricting it, because remote use was never possible. The web UI is + * served by this same server and addresses it as {@code http://localhost:8080}, hardcoded + * throughout the frontend, so a browser on another machine would receive the page and then send + * every request to its own loopback. The wider binding exposed the API without ever making the + * application usable from elsewhere. + * + *

Deliberately fixed and not a setting. An override would keep the exposure reachable to buy + * back a capability that does not work, and the day remote access is genuinely wanted it will + * mean changing the frontend's addresses too — which is when this decision should be revisited, + * not before. + * + *

A method rather than a {@code static final String}, so that the test which asserts this can + * read it. A compile-time constant is inlined into whatever reads it, and a test holding an + * inlined copy would go on binding to the address it was compiled against — passing while + * production had been changed under it. That is precisely the regression this must catch. + */ + static String listenHost() { + return "127.0.0.1"; + } + + static final int LISTEN_PORT = 8080; + private final Logger LOGGER = LoggerFactory.getLogger(MainVerticle.class); private DatabaseMetadataService databaseMetadataService; @@ -98,7 +129,7 @@ public void start() { }); // Start HTTP server - vertx.createHttpServer().requestHandler(router).listen(8080); + vertx.createHttpServer().requestHandler(router).listen(LISTEN_PORT, listenHost()); // Automatically open URL in browser, unless instructed otherwise String openBrowser = System.getProperty("studio.open", "true"); diff --git a/web-ui/src/test/java/studio/webui/HttpServerBindingTest.java b/web-ui/src/test/java/studio/webui/HttpServerBindingTest.java new file mode 100644 index 000000000..b3533c5be --- /dev/null +++ b/web-ui/src/test/java/studio/webui/HttpServerBindingTest.java @@ -0,0 +1,156 @@ +/* + * 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 io.vertx.core.http.HttpServer; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Assumptions; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStreamReader; +import java.net.Inet4Address; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.NetworkInterface; +import java.net.Socket; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +/** + * Where the HTTP server can be reached from. + * + *

The server used to be started with {@code listen(int)}, whose default host is {@code 0.0.0.0}: + * every machine on the same network segment could reach an API that has no authentication, and read, + * write or delete in the user's library while STUdio was running. The launcher opens a browser on + * {@code localhost}, so nothing told the user anything else was listening. + * + *

This asserts the consequence rather than the constant. It starts a real server on the address + * production uses, then opens real sockets: the loopback must answer, and a non-loopback address of + * this same machine must not. Reading {@link MainVerticle#listenHost()} back would only restate the + * source; connecting proves what the operating system actually did with it. + * + *

The refusal case needs a second local address to aim at, and a container often has none. When + * there is no non-loopback IPv4 address the case is skipped rather than passed — a machine that + * cannot set up the situation has not established anything about it. The nominal case runs + * everywhere, and carries equal weight: a server bound to nothing at all would pass every refusal. + */ +class HttpServerBindingTest { + + /** Long enough for a local connection to complete, short enough that a refusal is not a wait. */ + private static final int CONNECT_TIMEOUT_MS = 2_000; + + private Vertx vertx; + private HttpServer server; + private int port; + + @BeforeEach + void startServerOnTheProductionAddress() throws Exception { + vertx = Vertx.vertx(); + CompletableFuture started = new CompletableFuture<>(); + // Port 0, so the test never collides with a running STUdio or with a parallel run. The + // address is production's, which is the whole point. + vertx.createHttpServer() + .requestHandler(request -> request.response().end("ok")) + .listen(0, MainVerticle.listenHost(), ar -> { + if (ar.succeeded()) { + started.complete(ar.result()); + } else { + started.completeExceptionally(ar.cause()); + } + }); + server = started.get(10, TimeUnit.SECONDS); + port = server.actualPort(); + } + + @AfterEach + void stopServer() throws Exception { + if (vertx != null) { + CompletableFuture closed = new CompletableFuture<>(); + vertx.close(ar -> closed.complete(null)); + closed.get(10, TimeUnit.SECONDS); + } + } + + @Test + @DisplayName("the web UI can still reach the server on the loopback") + void loopbackIsReachable() { + try (Socket socket = new Socket()) { + socket.connect(new InetSocketAddress(InetAddress.getLoopbackAddress(), port), + CONNECT_TIMEOUT_MS); + socket.setSoTimeout(CONNECT_TIMEOUT_MS); + // A whole request and answer, not just a completed handshake: an open port proves the + // socket was accepted, this proves the application behind it still serves. It also lets + // the server finish with the connection before the teardown closes Vert.x under it. + socket.getOutputStream().write("GET / HTTP/1.0\r\n\r\n".getBytes(StandardCharsets.UTF_8)); + socket.getOutputStream().flush(); + String status = new BufferedReader( + new InputStreamReader(socket.getInputStream(), StandardCharsets.UTF_8)).readLine(); + assertNotNull(status, "the server accepted the connection and then answered nothing"); + assertTrue(status.contains("200"), "expected a served response, got: " + status); + } catch (IOException e) { + fail("the application must still be reachable where it serves its own UI: " + e, e); + } + } + + @Test + @DisplayName("no other machine on the network can reach it") + void nonLoopbackIsRefused() { + Optional routable = aNonLoopbackAddressOfThisMachine(); + Assumptions.assumeTrue(routable.isPresent(), + "this machine has no non-loopback IPv4 address, so it cannot set up the situation"); + + InetSocketAddress target = new InetSocketAddress(routable.get(), port); + assertThrows(IOException.class, () -> { + try (Socket socket = new Socket()) { + socket.connect(target, CONNECT_TIMEOUT_MS); + } + }, "the server answered on " + target + ", so it is listening beyond the loopback and the " + + "unauthenticated API is reachable from the local network"); + } + + /** + * An IPv4 address of this machine that another machine could route to, if there is one. + * + *

Loopback, down interfaces and link-local addresses are all skipped: none of them is a + * credible stand-in for "somebody else on the network". + */ + private Optional aNonLoopbackAddressOfThisMachine() { + List candidates = new ArrayList<>(); + try { + for (NetworkInterface itf : Collections.list(NetworkInterface.getNetworkInterfaces())) { + if (!itf.isUp() || itf.isLoopback()) { + continue; + } + for (InetAddress address : Collections.list(itf.getInetAddresses())) { + if (address instanceof Inet4Address + && !address.isLoopbackAddress() + && !address.isLinkLocalAddress()) { + candidates.add(address); + } + } + } + } catch (Exception e) { + return Optional.empty(); + } + return candidates.stream().findFirst(); + } +}