Reject control chars in request headers

src/IceRpc/Internal/IceProtocolConnection.cs

@@ -689,6 +689,18 @@ private static (int RequestId, IceRequestHeader Header, PipeReader? ContextReade
         var requestHeader = new IceRequestHeader(ref decoder);
         requestHeader.Facet.CheckFacetCount();
 
+        // Reject operation names that contain characters outside the printable ASCII range. The path is built from
+        // requestHeader.Identity via IceIdentity.ToPath, which percent-escapes any unsafe characters, so it does not
+        // need an additional check here.
+        try
+        {
+            ServiceAddress.CheckOperation(requestHeader.Operation);
+        }
+        catch (FormatException exception)
+        {
+            throw new InvalidDataException(exception.Message, exception);
+        }
+
         Pipe? contextPipe = null;
         long pos = decoder.Consumed;
         int count = decoder.DecodeSize();

src/IceRpc/Internal/IceRpcProtocolConnection.cs

@@ -1227,6 +1227,20 @@ async Task<OutgoingResponse> PerformDispatchRequestAsync(
         {
             var decoder = new SliceDecoder(buffer);
             var header = new IceRpcRequestHeader(ref decoder);
+
+            // Reject paths and operation names that contain characters outside the printable ASCII range. This
+            // matches the constraints already enforced by ServiceAddress and by the Slice identifier grammar, and
+            // prevents control characters (notably CR/LF/NUL) from flowing into loggers and other downstream sinks.
+            try
+            {
+                ServiceAddress.CheckPath(header.Path);
+                ServiceAddress.CheckOperation(header.Operation);
+            }
+            catch (FormatException exception)
+            {
+                throw new InvalidDataException(exception.Message, exception);
+            }
+
             (IDictionary<RequestFieldKey, ReadOnlySequence<byte>> fields, PipeReader? pipeReader) =
                 DecodeFieldDictionary(
                     ref decoder,

src/IceRpc/ServiceAddress.cs

@@ -490,7 +490,7 @@ internal static void CheckPath(string path)
         if (path.Length == 0 || path[0] != '/' || !IsValid(path, _notValidInPath))
         {
             throw new FormatException(
-                $"Invalid path '{path}'; a valid path starts with '/' and contains only unreserved characters, '%', and reserved characters other than '?' and '#'.");
+                "Invalid path; a valid path starts with '/' and contains only unreserved characters, '%', and reserved characters other than '?' and '#'.");
         }
     }
 
@@ -501,6 +501,23 @@ internal static void CheckPath(string path)
     /// <see langword="false" />.</returns>
     internal static bool IsValidParamValue(string value) => IsValid(value, _notValidInParamValue);
 
+    /// <summary>Checks if <paramref name="operation" /> is a valid operation name. A valid operation name contains
+    /// only printable ASCII characters (in the inclusive range <c>0x21</c> to <c>0x7E</c>) and may be empty.
+    /// </summary>
+    /// <param name="operation">The operation name to check.</param>
+    /// <exception cref="FormatException">Thrown if the operation name is not valid.</exception>
+    /// <remarks>This range covers every legal Slice identifier and rejects all C0 control characters as well as DEL
+    /// and any non-ASCII content. Empty operation names are accepted because <see cref="OutgoingRequest.Operation" />
+    /// defaults to the empty string.</remarks>
+    internal static void CheckOperation(string operation)
+    {
+        if (operation.AsSpan().IndexOfAnyExceptInRange(FirstValidChar, LastValidChar) != -1)
+        {
+            throw new FormatException(
+                "Invalid operation name; an operation name contains only printable ASCII characters.");
+        }
+    }
+
     /// <summary>"unchecked" constructor used by the Ice decoder when decoding a service address.
     /// </summary>
     internal ServiceAddress(

tests/IceRpc.Tests/IceProtocolConnectionTests.cs

@@ -175,6 +175,49 @@ public async Task Thrown_dispatcher_exception_is_classified(string exceptionKind
         Assert.That(response.StatusCode, Is.EqualTo(expectedStatusCode));
     }
 
+    /// <summary>Verifies that a request whose operation name contains characters outside the printable ASCII range
+    /// is rejected at decode time so peer-controlled control characters cannot reach loggers and other downstream
+    /// sinks. The dispatcher must not be invoked, and because ice cannot recover from a decode failure mid-stream,
+    /// the connection aborts and the pending invocation fails with
+    /// <see cref="IceRpcError.ConnectionAborted" />.</summary>
+    [TestCase("op\r\nINJECTED")] // CR/LF in operation
+    [TestCase("op\0")]           // NUL in operation
+    public async Task Request_with_invalid_operation_aborts_server_connection(string operation)
+    {
+        // Arrange
+        var dispatcher = new InlineDispatcher((request, cancellationToken) =>
+        {
+            Assert.Fail("The dispatcher must not be invoked for a request with an invalid operation name.");
+            return new(new OutgoingResponse(request));
+        });
+
+        await using ServiceProvider provider = new ServiceCollection()
+            .AddProtocolTest(Protocol.Ice, dispatcher)
+            .BuildServiceProvider(validateScopes: true);
+
+        ClientServerProtocolConnection sut = provider.GetRequiredService<ClientServerProtocolConnection>();
+        (_, Task serverShutdownRequested) = await sut.ConnectAsync();
+
+        using var request = new OutgoingRequest(new ServiceAddress(Protocol.Ice) { Path = "/foo" })
+        {
+            Operation = operation
+        };
+        Task<IncomingResponse> invokeTask = sut.Client.InvokeAsync(request);
+
+        // Wait until the server's read loop has rejected the invalid header and signaled shutdown, so the
+        // disposal below cannot race with request decoding.
+        await serverShutdownRequested;
+
+        // Act: closing the server surfaces the abort to the client's pending invocation. ReadFailed signals
+        // shutdown but doesn't close the duplex connection on its own.
+        await sut.Server.DisposeAsync();
+
+        // Assert
+        Assert.That(
+            async () => await invokeTask,
+            Throws.InstanceOf<IceRpcException>().With.Property("IceRpcError").EqualTo(IceRpcError.ConnectionAborted));
+    }
+
     /// <summary>Verifies that a StatusCode dispatched by the server is encoded as a ReplyStatus and decoded back to
     /// the expected StatusCode by the client.</summary>
     [Test, TestCaseSource(nameof(StatusCodeRoundTripSource))]

tests/IceRpc.Tests/IceRpcProtocolConnectionTests.cs

@@ -446,6 +446,54 @@ public async Task Invalid_request_refused(byte[] invalidRequestBytes, bool succe
             Throws.Nothing);
     }
 
+    /// <summary>Verifies that a request whose path or operation name contains characters outside the printable
+    /// ASCII range is rejected at decode time, so peer-controlled control characters cannot reach loggers and
+    /// other downstream sinks.</summary>
+    [TestCase("/foo\r\nINJECTED", "op")]   // CR/LF in path
+    [TestCase("/foo", "op\r\nINJECTED")]   // CR/LF in operation
+    [TestCase("/foo\0", "op")]             // NUL in path
+    [TestCase("/foo", "op\0")]             // NUL in operation
+    [TestCase("foo", "op")]                // path without leading '/'
+    public async Task Request_with_invalid_path_or_operation_is_rejected(string path, string operation)
+    {
+        // Arrange
+        var taskExceptionObserver = new TestTaskExceptionObserver();
+
+        await using ServiceProvider provider = new ServiceCollection()
+            .AddProtocolTest(Protocol.IceRpc)
+            .AddTestMultiplexedTransportDecorator()
+            .AddSingleton<ITaskExceptionObserver>(taskExceptionObserver)
+            .BuildServiceProvider(validateScopes: true);
+
+        ClientServerProtocolConnection sut = provider.GetRequiredService<ClientServerProtocolConnection>();
+        await sut.ConnectAsync();
+
+        var clientTransport = provider.GetRequiredService<TestMultiplexedClientTransportDecorator>();
+        IMultiplexedConnection clientTransportConnection = clientTransport.LastCreatedConnection;
+        IMultiplexedStream stream = await clientTransportConnection.CreateStreamAsync(bidirectional: true, default);
+
+        var writer = new MemoryBufferWriter(new byte[256]);
+        var encoder = new SliceEncoder(writer);
+        Span<byte> sizePlaceholder = encoder.GetPlaceholderSpan(4);
+        int startPos = encoder.EncodedByteCount;
+        encoder.EncodeString(path);
+        encoder.EncodeString(operation);
+        encoder.EncodeVarUInt62(0); // empty field dictionary
+        SliceEncoder.EncodeVarUInt62((ulong)(encoder.EncodedByteCount - startPos), sizePlaceholder);
+
+        // Act
+        stream.Output.Write(writer.WrittenMemory.Span);
+        await stream.Output.FlushAsync();
+        stream.Output.CompleteOutput(success: true);
+
+        // Assert
+        Assert.That(
+            async () => await taskExceptionObserver.DispatchRefusedException,
+            Is.InstanceOf<IceRpcException>()
+                .With.Property("IceRpcError").EqualTo(IceRpcError.IceRpcError)
+                .And.InnerException.InstanceOf<InvalidDataException>());
+    }
+
     /// <summary>Verifies that a request with a field count large enough that count * 2 would overflow int arithmetic
     /// is correctly rejected.</summary>
     [Test]