diff --git a/serve-application/src/main/java/build/serve/application/ServerApplication.java b/serve-application/src/main/java/build/serve/application/ServerApplication.java index 9dda22f..eec6bcc 100644 --- a/serve-application/src/main/java/build/serve/application/ServerApplication.java +++ b/serve-application/src/main/java/build/serve/application/ServerApplication.java @@ -26,11 +26,14 @@ import build.serve.foundation.error.DefaultErrorHandler; import build.serve.foundation.error.ErrorHandler; import build.serve.foundation.option.ListenAddress; +import build.serve.foundation.option.MaxRequestSize; +import build.serve.foundation.option.RequestTimeout; import build.serve.foundation.option.ShutdownTimeout; import build.serve.foundation.option.TlsConfig; import build.serve.foundation.routing.Router; import build.serve.security.SecurityHeadersMiddleware; import build.serve.transport.http.HttpTransport; +import build.serve.transport.http.TlsOptions; import build.spawn.application.Addressable; import build.spawn.application.Lifecycle; @@ -164,6 +167,17 @@ void recorder(final TelemetryRecorder recorder) { this.recorder = Objects.requireNonNull(recorder, "recorder must not be null"); } + /** + * Obtains the {@link TelemetryRecorder} bound to this server, for use by + * {@link #configure()} when wiring components (e.g. {@code GraphQlSchema}) that need + * somewhere to report their own errors instead of defaulting to stdout/stderr. + * + * @return the {@link TelemetryRecorder} + */ + protected TelemetryRecorder recorder() { + return recorder; + } + /** * Configures and returns the {@link Router} for this server. *

@@ -231,7 +245,8 @@ public void start(final ListenAddress listenAddress, final var address = new InetSocketAddress(listenAddress.value(), listenPort.get()); - this.transport = new HttpTransport(address, 0, applySecurityHeaders(router), errorHandler(), recorder); + this.transport = new HttpTransport(address, 0, applySecurityHeaders(router), errorHandler(), + MaxRequestSize.DEFAULT, RequestTimeout.DEFAULT, recorder); this.transport.start(); this.shutdownHook = new Thread(this::stop, "serve-shutdown"); @@ -275,7 +290,7 @@ public void start(final ListenAddress listenAddress, final var address = new InetSocketAddress(listenAddress.value(), listenPort.get()); this.transport = HttpTransport.https(address, 0, applySecurityHeaders(router), errorHandler(), - tls.sslContext(), recorder); + MaxRequestSize.DEFAULT, tls.sslContext(), TlsOptions.defaults(), RequestTimeout.DEFAULT, recorder); this.transport.start(); this.shutdownHook = new Thread(this::stop, "serve-shutdown"); diff --git a/serve-example/pom.xml b/serve-example/pom.xml index 1cda20b..e6ba855 100644 --- a/serve-example/pom.xml +++ b/serve-example/pom.xml @@ -100,6 +100,16 @@ base-json + + build.base + base-telemetry + + + + build.base + base-telemetry-foundation + + @@ -175,6 +185,11 @@ build.base:base-template-processor + + + build.base:base-telemetry-foundation + diff --git a/serve-example/src/main/java/build/serve/example/ExampleApp.java b/serve-example/src/main/java/build/serve/example/ExampleApp.java index 83f6d58..2aa8b64 100644 --- a/serve-example/src/main/java/build/serve/example/ExampleApp.java +++ b/serve-example/src/main/java/build/serve/example/ExampleApp.java @@ -95,7 +95,7 @@ protected Router configure() { final var service = new TaskService(); final var broadcaster = new TaskBroadcaster(service); final var api = new TaskApiHandler(service); - final var graphql = new TaskGraphQlHandler(service); + final var graphql = new TaskGraphQlHandler(service, recorder()); final var web = new TaskWebHandler(service); final var apiAuth = AuthMiddleware.builder() diff --git a/serve-example/src/main/java/build/serve/example/graphql/TaskGraphQlHandler.java b/serve-example/src/main/java/build/serve/example/graphql/TaskGraphQlHandler.java index 6611b6d..26574b3 100644 --- a/serve-example/src/main/java/build/serve/example/graphql/TaskGraphQlHandler.java +++ b/serve-example/src/main/java/build/serve/example/graphql/TaskGraphQlHandler.java @@ -19,6 +19,7 @@ */ package build.serve.example.graphql; +import build.base.telemetry.TelemetryRecorder; import build.serve.example.domain.TaskService; import build.serve.foundation.Handler; import build.serve.graphql.GraphQlHandler; @@ -73,8 +74,9 @@ public final class TaskGraphQlHandler { /** * Constructs a {@link TaskGraphQlHandler} backed by the given service. */ - public TaskGraphQlHandler(final TaskService service) { + public TaskGraphQlHandler(final TaskService service, final TelemetryRecorder recorder) { final var schema = GraphQlSchema.builder(SDL) + .recorder(recorder) .fetcher("Query", "tasks", env -> service.list()) .fetcher("Query", "task", env -> { final var id = Long.parseLong(env.getArgument("id")); diff --git a/serve-example/src/main/java/module-info.java b/serve-example/src/main/java/module-info.java index 6c0d0f3..a57f6a0 100644 --- a/serve-example/src/main/java/module-info.java +++ b/serve-example/src/main/java/module-info.java @@ -42,6 +42,8 @@ requires build.base.network; requires build.base.template; requires build.base.json; + requires build.base.telemetry; + requires build.base.telemetry.foundation; requires static build.base.template.processor; diff --git a/serve-example/src/test/java/build/serve/example/TaskGraphQlTests.java b/serve-example/src/test/java/build/serve/example/TaskGraphQlTests.java index c61457c..3faef66 100644 --- a/serve-example/src/test/java/build/serve/example/TaskGraphQlTests.java +++ b/serve-example/src/test/java/build/serve/example/TaskGraphQlTests.java @@ -22,6 +22,7 @@ import build.base.json.Json; import build.base.json.JsonArray; import build.base.json.JsonNull; +import build.base.telemetry.foundation.NoOpTelemetryRecorder; import build.serve.example.domain.TaskService; import build.serve.example.graphql.TaskGraphQlHandler; import build.serve.foundation.routing.RouterBuilder; @@ -34,7 +35,7 @@ class TaskGraphQlTests { private static TestServer server() { final var service = new TaskService(); - final var graphql = new TaskGraphQlHandler(service); + final var graphql = new TaskGraphQlHandler(service, NoOpTelemetryRecorder.create()); return TestServer.of(RouterBuilder.create() .post("/graphql", graphql.graphqlHandler()) .build()); diff --git a/serve-foundation/src/main/java/build/serve/foundation/option/RequestTimeout.java b/serve-foundation/src/main/java/build/serve/foundation/option/RequestTimeout.java index f286503..2796cd6 100644 --- a/serve-foundation/src/main/java/build/serve/foundation/option/RequestTimeout.java +++ b/serve-foundation/src/main/java/build/serve/foundation/option/RequestTimeout.java @@ -41,6 +41,13 @@ public record RequestTimeout(Duration duration) */ public static final RequestTimeout NONE = new RequestTimeout(Duration.ZERO); + /** + * A sane default request timeout (60 seconds), used by {@code ServerApplication} so a default + * server isn't exposed to Slowloris-style slow-request attacks. Pass {@link #NONE} explicitly + * to opt out. + */ + public static final RequestTimeout DEFAULT = new RequestTimeout(Duration.ofSeconds(60)); + /** * Creates a {@link RequestTimeout} for the specified duration. * diff --git a/serve-graphql/pom.xml b/serve-graphql/pom.xml index 266a013..2a613a5 100644 --- a/serve-graphql/pom.xml +++ b/serve-graphql/pom.xml @@ -32,6 +32,16 @@ base-json + + build.base + base-telemetry + + + + build.base + base-telemetry-foundation + + build.serve diff --git a/serve-graphql/src/main/java/build/serve/graphql/GraphQlSchema.java b/serve-graphql/src/main/java/build/serve/graphql/GraphQlSchema.java index 366b676..534c639 100644 --- a/serve-graphql/src/main/java/build/serve/graphql/GraphQlSchema.java +++ b/serve-graphql/src/main/java/build/serve/graphql/GraphQlSchema.java @@ -19,9 +19,13 @@ */ package build.serve.graphql; +import build.base.telemetry.TelemetryRecorder; +import build.base.telemetry.foundation.PrintStreamTelemetryRecorder; +import graphql.ExceptionWhileDataFetching; import graphql.ExecutionInput; import graphql.ExecutionResult; import graphql.GraphQL; +import graphql.GraphQLError; import graphql.analysis.MaxQueryComplexityInstrumentation; import graphql.analysis.MaxQueryDepthInstrumentation; import graphql.execution.instrumentation.ChainedInstrumentation; @@ -31,6 +35,7 @@ import graphql.schema.idl.SchemaParser; import graphql.schema.idl.TypeDefinitionRegistry; +import java.net.URI; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -47,10 +52,15 @@ */ public final class GraphQlSchema { + private static final TelemetryRecorder DEFAULT_RECORDER = + PrintStreamTelemetryRecorder.of(URI.create("serve://graphql"), System.out, System.err); + private final GraphQL graphQL; + private final TelemetryRecorder recorder; - private GraphQlSchema(final GraphQL graphQL) { + private GraphQlSchema(final GraphQL graphQL, final TelemetryRecorder recorder) { this.graphQL = graphQL; + this.recorder = recorder; } /** @@ -76,25 +86,44 @@ public GraphQlResult execute(final GraphQlRequest request) { final ExecutionResult result = graphQL.execute(inputBuilder.build()); final List errors = result.getErrors().stream() - .map(e -> new GraphQlError( - sanitizeMessage(e.getMessage()), - e.getPath() != null - ? e.getPath().stream().map(Object::toString).toList() - : null)) + .map(this::toGraphQlError) .toList(); return new GraphQlResult(result.getData(), errors); } /** - * Strips control characters from a data-fetcher exception message before it reaches the - * client. graphql-java's default exception handler forwards a thrown exception's - * {@code getMessage()} verbatim into the error response; this matches the same - * control-character stripping {@code DefaultErrorHandler} and {@code McpServer} apply to - * exception messages at the HTTP and MCP layers, defending against CRLF/control-character - * injection via a data fetcher's exception message. + * Converts a graphql-java {@link GraphQLError} into a client-facing {@link GraphQlError}. + *

+ * A {@link ExceptionWhileDataFetching} means a data fetcher threw — its message is an + * exception message, not a spec-defined client-facing error, and may contain internal details + * (stack state, SQL, file paths). It is logged server-side via {@link #recorder} and replaced + * with a generic message, matching the pattern {@code DefaultErrorHandler} applies to unhandled + * HTTP exceptions. Other error types (validation errors, syntax errors) are part of the GraphQL + * spec's client-facing contract and are passed through, only stripped of control characters to + * defend against CRLF/control-character injection. + * + * @param error the {@link GraphQLError} returned by the execution engine + * @return the client-facing {@link GraphQlError} + */ + private GraphQlError toGraphQlError(final GraphQLError error) { + final var path = error.getPath() != null + ? error.getPath().stream().map(Object::toString).toList() + : null; + + if (error instanceof ExceptionWhileDataFetching dataFetchingError) { + recorder.error(dataFetchingError.getException(), "Unhandled exception in GraphQL data fetcher"); + return new GraphQlError("Internal Server Error", path); + } + + return new GraphQlError(sanitizeMessage(error.getMessage()), path); + } + + /** + * Strips control characters from a client-facing error message, defending against + * CRLF/control-character injection. * - * @param message the raw exception message, or {@code null} + * @param message the raw message, or {@code null} * @return the sanitized message, or {@code null} if the input was {@code null} */ private static String sanitizeMessage(final String message) { @@ -127,7 +156,8 @@ GraphQlSchema withOptions(final GraphQlOptions options) { return new GraphQlSchema( GraphQL.newGraphQL(graphQL.getGraphQLSchema()) .instrumentation(new ChainedInstrumentation(instrumentations)) - .build() + .build(), + recorder ); } @@ -151,12 +181,25 @@ public static final class Builder { private final String sdl; private final Map>> fetchers; + private TelemetryRecorder recorder; private Builder(final String sdl) { this.sdl = Objects.requireNonNull(sdl, "sdl must not be null"); this.fetchers = new HashMap<>(); } + /** + * Sets the {@link TelemetryRecorder} used to record unhandled data-fetcher exceptions. + * Defaults to a {@code System.out}/{@code System.err} recorder if not set. + * + * @param recorder the {@link TelemetryRecorder} to use + * @return this builder + */ + public Builder recorder(final TelemetryRecorder recorder) { + this.recorder = Objects.requireNonNull(recorder, "recorder must not be null"); + return this; + } + /** * Registers a {@link DataFetcher} for a specific type and field. * @@ -197,7 +240,7 @@ public GraphQlSchema build() { final var schema = new SchemaGenerator() .makeExecutableSchema(registry, wiringBuilder.build()); - return new GraphQlSchema(GraphQL.newGraphQL(schema).build()); + return new GraphQlSchema(GraphQL.newGraphQL(schema).build(), recorder != null ? recorder : DEFAULT_RECORDER); } private static graphql.schema.DataFetcher adaptFetcher(final DataFetcher fetcher) { diff --git a/serve-graphql/src/main/java/module-info.java b/serve-graphql/src/main/java/module-info.java index a488ca9..44d3154 100644 --- a/serve-graphql/src/main/java/module-info.java +++ b/serve-graphql/src/main/java/module-info.java @@ -26,6 +26,7 @@ module build.serve.graphql { requires transitive build.serve.foundation; requires build.base.json; + requires build.base.telemetry.foundation; requires com.graphqljava; exports build.serve.graphql; diff --git a/serve-graphql/src/test/java/build/serve/graphql/GraphQlHandlerTests.java b/serve-graphql/src/test/java/build/serve/graphql/GraphQlHandlerTests.java index ca8e992..6b667be 100644 --- a/serve-graphql/src/test/java/build/serve/graphql/GraphQlHandlerTests.java +++ b/serve-graphql/src/test/java/build/serve/graphql/GraphQlHandlerTests.java @@ -139,14 +139,14 @@ void shouldReturnErrorsForInvalidQuery() throws Exception { } @Test - void shouldStripControlCharactersFromDataFetcherExceptionMessage() throws Exception { + void shouldNotLeakDataFetcherExceptionMessageToClient() throws Exception { final var throwingSchema = GraphQlSchema.builder(""" type Query { boom: String } """) .fetcher("Query", "boom", env -> { - throw new RuntimeException("line one\r\nline two\tinjected"); + throw new RuntimeException("line one\r\nline two\tinjected secret internal details"); }) .build(); @@ -165,11 +165,25 @@ void shouldStripControlCharactersFromDataFetcherExceptionMessage() throws Except final var errors = (JsonArray) json.get("errors"); final var message = errors.values().get(0).asObject().getString("message"); - assertThat(message).doesNotContain("\r").doesNotContain("\n").doesNotContain("\t"); - assertThat(message).contains("line one line two injected"); + assertThat(message).isEqualTo("Internal Server Error"); } } + @Test + void shouldStripControlCharactersFromSyntaxErrorMessage() throws Exception { + final var response = server.post("/graphql") + .header("Content-Type", "application/json") + .body("{\"query\":\"{ \\u0001 }\"}") + .send() + .assertStatus(200); + + final var json = Json.parse(response.body()).asObject(); + final var errors = (JsonArray) json.get("errors"); + final var message = errors.values().get(0).asObject().getString("message"); + + assertThat(message).doesNotContain("\r").doesNotContain("\n").doesNotContain("\t"); + } + @Test void shouldBlockIntrospectionWhenDisabled() throws Exception { var schema = GraphQlSchema.builder(""" diff --git a/serve-lsp/src/main/java/build/serve/lsp/LspTransport.java b/serve-lsp/src/main/java/build/serve/lsp/LspTransport.java index 8e08b8a..4530ef7 100644 --- a/serve-lsp/src/main/java/build/serve/lsp/LspTransport.java +++ b/serve-lsp/src/main/java/build/serve/lsp/LspTransport.java @@ -87,10 +87,12 @@ public static void stdio(final LspServer server) throws Exception { * given port and handles each connection in a virtual thread. Blocks the calling thread * until the server socket is closed or an error occurs. * - *

LSP has no authentication of its own — the {@code exit} notification unconditionally - * halts the JVM ({@link Runtime#halt}) — so binding to all interfaces would let any network - * peer that can reach the port hard-kill the process. Use {@link #tcp(LspServer, int, InetAddress)} - * to bind elsewhere only if the server is genuinely meant to be reachable remotely. + *

LSP has no authentication of its own — the {@code exit} notification would otherwise let + * any peer that can reach the port hard-kill the process. Binding loopback-only means only + * same-host peers can reach it at all, so the {@code exit} notification is allowed to halt the + * JVM here. Use {@link #tcp(LspServer, int, InetAddress)} to bind elsewhere only if the server + * is genuinely meant to be reachable remotely — that overload does not grant remote peers the + * same hard-kill. * * @param server the LSP server * @param port the port to listen on @@ -105,12 +107,18 @@ public static void tcp(final LspServer server, final int port) throws IOExceptio * handling each connection in a virtual thread. Blocks the calling thread until the server * socket is closed or an error occurs. * + *

The {@code exit} notification is only allowed to halt the JVM ({@link Runtime#halt}) when + * {@code bindAddress} is a loopback address — LSP has no authentication of its own, so an + * unauthenticated remote peer must not be able to kill the process for every other connection. + * When bound elsewhere, a connection's {@code exit} notification simply closes that connection. + * * @param server the LSP server * @param port the port to listen on * @param bindAddress the address to bind to * @throws IOException if an error occurs opening or accepting on the server socket */ public static void tcp(final LspServer server, final int port, final InetAddress bindAddress) throws IOException { + final var haltOnExit = bindAddress.isLoopbackAddress(); try (var serverSocket = new java.net.ServerSocket()) { serverSocket.bind(new InetSocketAddress(bindAddress, port)); while (true) { @@ -118,7 +126,7 @@ public static void tcp(final LspServer server, final int port, final InetAddress final var socket = serverSocket.accept(); Thread.ofVirtual().start(() -> { try { - run(server, socket.getInputStream(), socket.getOutputStream()); + run(server, socket.getInputStream(), socket.getOutputStream(), haltOnExit); } catch (final Exception ignored) { } finally { try { @@ -135,7 +143,8 @@ public static void tcp(final LspServer server, final int port, final InetAddress } /** - * Creates a transport from custom streams (useful for testing). + * Creates a transport from custom streams (useful for testing). The {@code exit} notification + * halts the JVM, as for {@link #stdio(LspServer)}. * * @param server the LSP server * @param in the input stream @@ -143,6 +152,11 @@ public static void tcp(final LspServer server, final int port, final InetAddress * @throws Exception if an error occurs */ public static void run(final LspServer server, final InputStream in, final OutputStream out) throws Exception { + run(server, in, out, true); + } + + private static void run(final LspServer server, final InputStream in, final OutputStream out, + final boolean haltOnExit) throws Exception { final var shutdownRequested = new AtomicBoolean(false); final var outboundId = new AtomicInteger(0); final Map> pendingRequests = new ConcurrentHashMap<>(); @@ -260,7 +274,7 @@ private JsonObject showMessageNode(final ShowMessageParams params) { }; try { - runLoop(server, in, ctx, shutdownRequested, writer, pendingRequests, activeRequests); + runLoop(server, in, ctx, shutdownRequested, writer, pendingRequests, activeRequests, haltOnExit); } finally { failPendingRequests(pendingRequests); writer.shutdown(); @@ -278,7 +292,8 @@ private static void runLoop(final LspServer server, final AtomicBoolean shutdownRequested, final LspWriter writer, final Map> pendingRequests, - final Map activeRequests) throws IOException { + final Map activeRequests, + final boolean haltOnExit) throws IOException { while (true) { final var contentLength = readContentLength(in); if (contentLength < 0 || contentLength > MAX_CONTENT_LENGTH) { @@ -297,7 +312,9 @@ private static void runLoop(final LspServer server, // Transport lifecycle — handled before typed dispatch if ("exit".equals(methodStr)) { - Runtime.getRuntime().halt(shutdownRequested.get() ? 0 : 1); + if (haltOnExit) { + Runtime.getRuntime().halt(shutdownRequested.get() ? 0 : 1); + } return; } if ("shutdown".equals(methodStr)) { diff --git a/serve-lsp/src/test/java/build/serve/lsp/LspTransportTests.java b/serve-lsp/src/test/java/build/serve/lsp/LspTransportTests.java index 2af4a1e..aa65f62 100644 --- a/serve-lsp/src/test/java/build/serve/lsp/LspTransportTests.java +++ b/serve-lsp/src/test/java/build/serve/lsp/LspTransportTests.java @@ -438,6 +438,38 @@ void shouldAcceptTcpConnectionAndHandleRequest() throws Exception { serverThread.interrupt(); } + @Test + void shouldCloseConnectionWithoutHaltingJvmWhenExitReceivedOnNonLoopbackBindAddress() throws Exception { + final var server = LspServer.builder().build(); + + final int port; + try (var s = new ServerSocket(0)) { + port = s.getLocalPort(); + } + + final var bindAddress = java.net.InetAddress.getByName("0.0.0.0"); + final var serverThread = Thread.ofVirtual().start(() -> { + try { + LspTransport.tcp(server, port, bindAddress); + } catch (final IOException ignored) { + } + }); + + Thread.sleep(100); + + try (var socket = new Socket("localhost", port)) { + final var client = new LspTestClient(socket); + client.writeMessage("{\"jsonrpc\":\"2.0\",\"method\":\"exit\"}"); + + // If haltOnExit were mistakenly true here, this call would kill the test JVM instead + // of returning. Reading past the connection the server closes on exit confirms the + // server only tore down this connection. + assertThat(client.clientReader.readLine()).isNull(); + } + + serverThread.interrupt(); + } + @Test void shouldHandleMultipleConcurrentTcpConnections() throws Exception { final var server = LspServer.builder()