fix: misc security and correctness fixes (#65)

Author: deer

serve-auth/src/main/java/build/serve/auth/AuthStrategy.java

@@ -21,7 +21,9 @@
 
 import build.serve.foundation.Request;
 
+import java.util.Arrays;
 import java.util.Optional;
+import java.util.stream.Collectors;
 
 /**
  * A strategy for authenticating an inbound {@link Request}.
@@ -69,14 +71,25 @@ default String challenge() {
      * @return a composite {@link AuthStrategy}
      */
     static AuthStrategy anyOf(final AuthStrategy... strategies) {
-        return request -> {
-            for (final var strategy : strategies) {
-                final var result = strategy.authenticate(request);
-                if (result.isPresent()) {
-                    return result;
+        final var compositeChallenge = Arrays.stream(strategies)
+            .map(AuthStrategy::challenge)
+            .collect(Collectors.joining(", "));
+        return new AuthStrategy() {
+            @Override
+            public Optional<Principal> authenticate(final Request request) {
+                for (final var strategy : strategies) {
+                    final var result = strategy.authenticate(request);
+                    if (result.isPresent()) {
+                        return result;
+                    }
                 }
+                return Optional.empty();
+            }
+
+            @Override
+            public String challenge() {
+                return compositeChallenge;
             }
-            return Optional.empty();
         };
     }
 }

serve-auth/src/main/java/build/serve/auth/BasicAuthStrategy.java

@@ -46,6 +46,9 @@ public final class BasicAuthStrategy implements AuthStrategy {
 
     private BasicAuthStrategy(final String realm,
                               final BiFunction<String, String, Optional<Principal>> validator) {
+        if (realm.contains("\"") || realm.contains("\r") || realm.contains("\n")) {
+            throw new IllegalArgumentException("realm must not contain quotes or CRLF");
+        }
         this.realm = realm;
         this.validator = validator;
     }

serve-auth/src/test/java/build/serve/auth/AuthMiddlewareTests.java

@@ -39,6 +39,7 @@
 import java.util.concurrent.atomic.AtomicReference;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 class AuthMiddlewareTests {
 
@@ -187,6 +188,18 @@ void shouldIncludeRealmInChallenge() {
             .isEqualTo("Basic realm=\"myapp\"");
     }
 
+    @Test
+    void shouldRejectRealmWithQuote() {
+        assertThatThrownBy(() -> BasicAuthStrategy.of("my\"app", (u, p) -> Optional.empty()))
+            .isInstanceOf(IllegalArgumentException.class);
+    }
+
+    @Test
+    void shouldRejectRealmWithCrlf() {
+        assertThatThrownBy(() -> BasicAuthStrategy.of("app\r\nX-Injected: val", (u, p) -> Optional.empty()))
+            .isInstanceOf(IllegalArgumentException.class);
+    }
+
     // --- ApiKeyStrategy ---
 
     @Test
@@ -233,6 +246,16 @@ void shouldReturnEmptyWhenAllStrategiesFail() {
         assertThat(strategy.authenticate(request("X-None", ""))).isEmpty();
     }
 
+    @Test
+    void shouldJoinDelegateChallengesInAnyOf() {
+        var basic = BasicAuthStrategy.of("myapp", (u, p) -> Optional.empty());
+        var bearer = BearerTokenStrategy.of(t -> Optional.empty());
+        var strategy = AuthStrategy.anyOf(basic, bearer);
+
+        assertThat(strategy.challenge())
+            .isEqualTo("Basic realm=\"myapp\", Bearer");
+    }
+
     // --- helpers ---
 
     private static Exchange exchange(final String headerName, final StubResponse response) {

serve-devtools/src/main/java/build/serve/devtools/ChromeDevToolsHandler.java

@@ -19,6 +19,7 @@
  */
 package build.serve.devtools;
 
+import build.base.logging.Logger;
 import build.serve.foundation.Handler;
 import build.serve.foundation.util.JsonStrings;
 
@@ -57,6 +58,7 @@ public final class ChromeDevToolsHandler {
 
     private static final String CONTENT_TYPE = "Content-Type";
     private static final String APPLICATION_JSON = "application/json";
+    private static final Logger LOGGER = Logger.get(ChromeDevToolsHandler.class);
 
     private ChromeDevToolsHandler() {
     }
@@ -85,6 +87,7 @@ public static Handler forWorkspace(final Path root,
                                        final UUID uuid) {
         Objects.requireNonNull(root, "root");
         Objects.requireNonNull(uuid, "uuid");
+        LOGGER.warn("ChromeDevToolsHandler exposes the absolute filesystem path — do not register in production");
         final var json = buildJson(root.toAbsolutePath().normalize().toString(), uuid);
         return exchange -> exchange.response()
             .status(200)

serve-foundation/src/main/java/build/serve/foundation/Response.java

@@ -55,6 +55,14 @@ public interface Response {
      */
     Response header(String name, String value);
 
+    /**
+     * Removes a response header so it is not sent to the client.
+     *
+     * @param name the header name to remove
+     */
+    default void removeHeader(final String name) {
+    }
+
     /**
      * Sends a plain text body and closes the response.
      *

serve-graphql/src/main/java/build/serve/graphql/GraphiQlHandler.java

@@ -71,6 +71,13 @@ public final class GraphiQlHandler {
     private GraphiQlHandler() {
     }
 
+    private static String escapeJs(final String s) {
+        return s.replace("\\", "\\\\")
+            .replace("'", "\\'")
+            .replace("\r", "\\r")
+            .replace("\n", "\\n");
+    }
+
     /**
      * Creates a {@link Handler} that serves the GraphiQL IDE, configured to send
      * queries to the given GraphQL endpoint.
@@ -81,7 +88,7 @@ private GraphiQlHandler() {
     public static Handler graphiql(final String endpoint) {
         Objects.requireNonNull(endpoint, "endpoint must not be null");
 
-        final var html = TEMPLATE.replace("{{ENDPOINT}}", endpoint)
+        final var html = TEMPLATE.replace("{{ENDPOINT}}", escapeJs(endpoint))
             .getBytes(StandardCharsets.UTF_8);
 
         return exchange -> {

serve-graphql/src/test/java/build/serve/graphql/GraphiQlHandlerTests.java

@@ -74,4 +74,19 @@ void bodyContainsGraphiQlMarkers() throws Exception {
             .contains("graphiql")
             .contains("GraphiQL");
     }
+
+    @Test
+    void shouldEscapeEndpointInJsStringLiteral() throws Exception {
+        final var dangerousServer = TestServer.of(GraphiQlHandler.graphiql("/gql?a=1&b='x'\\y"));
+
+        try {
+            final var body = dangerousServer.get("/").send().assertStatus(200).body();
+            assertThat(body)
+                .doesNotContain("'x'")
+                .contains("\\'x\\'")
+                .contains("\\\\y");
+        } finally {
+            dangerousServer.close();
+        }
+    }
 }

serve-lsp/src/main/java/build/serve/lsp/LspTransport.java

@@ -32,6 +32,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.net.SocketException;
 import java.nio.charset.StandardCharsets;
 import java.util.List;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -70,18 +71,22 @@ public static void stdio(final LspServer server) throws Exception {
     public static void tcp(final LspServer server, final int port) throws IOException {
         try (var serverSocket = new java.net.ServerSocket(port)) {
             while (true) {
-                final var socket = serverSocket.accept();
-                Thread.ofVirtual().start(() -> {
-                    try {
-                        run(server, socket.getInputStream(), socket.getOutputStream());
-                    } catch (final Exception ignored) {
-                    } finally {
+                try {
+                    final var socket = serverSocket.accept();
+                    Thread.ofVirtual().start(() -> {
                         try {
-                            socket.close();
-                        } catch (final IOException ignored) {
+                            run(server, socket.getInputStream(), socket.getOutputStream());
+                        } catch (final Exception ignored) {
+                        } finally {
+                            try {
+                                socket.close();
+                            } catch (final IOException ignored) {
+                            }
                         }
-                    }
-                });
+                    });
+                } catch (final SocketException e) {
+                    break;
+                }
             }
         }
     }

serve-security/src/main/java/build/serve/security/SecurityHeadersMiddleware.java

@@ -101,7 +101,7 @@ public Handler apply(final Handler next) {
             setIfNonNull(response, "Cross-Origin-Resource-Policy", crossOriginResourcePolicy);
 
             for (final var header : removeHeaders) {
-                response.header(header, "");
+                response.removeHeader(header);
             }
         };
     }

serve-security/src/test/java/build/serve/security/SecurityHeadersMiddlewareTests.java

@@ -113,8 +113,8 @@ void removeHeaderStripsHeaders() throws Exception {
         middleware.apply(ex -> {
         }).handle(exchange);
 
-        assertThat(response.headers).containsEntry("X-Powered-By", "");
-        assertThat(response.headers).containsEntry("Server", "");
+        assertThat(response.headers).doesNotContainKey("X-Powered-By");
+        assertThat(response.headers).doesNotContainKey("Server");
     }
 
     @Test
@@ -217,6 +217,11 @@ public Response header(final String name, final String value) {
             return this;
         }
 
+        @Override
+        public void removeHeader(final String name) {
+            headers.remove(name);
+        }
+
         @Override
         public void send(final String body) {
         }