Merge pull request #151 from Astn/maintainer-decisions

Implement the six maintainer decisions for 2.0

Author: Astn

AustinHarris.JsonRpc.AspNetCore/AustinHarris.JsonRpc.AspNetCore.csproj

@@ -6,6 +6,7 @@
         <Product>Json-Rpc.Net ASP.NET Core host</Product>
         <Description>ASP.NET Core / Kestrel hosting for JSON-RPC.Net: MapJsonRpc endpoint (PipeReader in, BodyWriter out, no strings), a raw Kestrel ConnectionHandler for JSON-RPC over TCP, and DI registration of services.</Description>
         <VersionPrefix>2.0.0</VersionPrefix>
+        <PackageReleaseNotes>https://github.com/Astn/JSON-RPC.NET/blob/master/CHANGELOG.md</PackageReleaseNotes>
         <VersionSuffix>$(VersionSuffix)</VersionSuffix>
         <Copyright>Austin Harris</Copyright>
         <PackageProjectUrl>https://github.com/Astn/JSON-RPC.NET</PackageProjectUrl>

AustinHarris.JsonRpc.AspNetCore/JsonRpcServiceCollectionExtensions.cs

@@ -88,17 +88,14 @@ public JsonRpcBinderHostedService(IServiceProvider provider, IEnumerable<JsonRpc
 
             public Task StartAsync(CancellationToken cancellationToken)
             {
-                string defaultSession = Handler.DefaultSessionId();
                 foreach (var r in _registrations)
                 {
                     var instance = _provider.GetRequiredService(r.Type);
                     // The effective session: the registration's own, then JsonRpcOptions.SessionId, then the default.
-                    var session = r.SessionId ?? _options.SessionId ?? defaultSession;
-                    // A JsonRpcService subclass already bound itself in its constructor, to the default session
-                    // (parameterless base constructor). Bind it here whenever the effective session is a different
-                    // one, otherwise the configured session would answer -32601 for it; rebinding the same
-                    // instance to the same session is harmless (the method table entry is replaced).
-                    if (instance is JsonRpcService && session == defaultSession) continue;
+                    var session = r.SessionId ?? _options.SessionId ?? Handler.DefaultSessionId();
+                    // Always bind, whatever the type: attribute binding replaces entries by name, so binding a
+                    // JsonRpcService subclass that already bound itself to the default session is harmless, and a
+                    // subclass constructed with base(false) is bound nowhere else.
                     ServiceBinder.BindService(session, instance);
                 }
                 return Task.CompletedTask;

AustinHarris.JsonRpc.AspNetCore/README.md

@@ -60,8 +60,8 @@ app.MapJsonRpc("/rpc").RequireAuthorization("api");
 
 `MapJsonRpc` adds no authorization, TLS requirement, rate limit or request deadline by itself; apply those
 policies explicitly. `MaxRequestBytes` limits the HTTP body, but there is no batch-count or response-size limit.
-Keep `Config.IncludeExceptionDetails` off for untrusted clients; the default still sends an unhandled exception's
-CLR type name and message, see [Exception disclosure](https://github.com/Astn/JSON-RPC.NET#exception-disclosure)
+Keep `Config.IncludeExceptionDetails` off for untrusted clients: by default an unhandled exception is answered as
+`-32603` with `data: null`, see [Exception disclosure](https://github.com/Astn/JSON-RPC.NET#exception-disclosure)
 in the main README.
 
 `MapJsonRpc(pattern = "/jsonrpc", options = null)` uses the options from `AddJsonRpc` unless you pass your own,
@@ -87,9 +87,10 @@ per-request data from the context instead.
 declares a `[JsonRpcMethod]`. Private methods count, so the attribute is the whole access list, and an MVC
 controller that carries it becomes a singleton too.
 
-A class deriving from `JsonRpcService` binds itself to the default session in its constructor. Registering it here
-as well is harmless when the effective session is the default. With `SessionId` set, the host binds it to that
-session in addition, so it stays reachable on the default session too.
+The host binds every registered service to its effective session: the session given to `AddJsonRpcService`, else
+`JsonRpcOptions.SessionId`, else the default. A class deriving from `JsonRpcService` also binds itself to the
+default session in its parameterless constructor, so with `SessionId` set it is reachable in both; write
+`: base(false)` in the subclass to leave that to the host.
 
 ## Raw connection (TCP, Unix socket, named pipe)
 
@@ -136,7 +137,7 @@ in the main README.
 |---|---|---|---|
 | `EnableAsyncMethods` | false | HTTP and raw | use `ProcessAsync` for `Task`/`ValueTask` methods, with host cancellation |
 | `SessionId` | default session | HTTP and raw | which session's methods answer |
-| `SessionSelector` | null | HTTP | pick the session per request from the `HttpContext`; it must map to a fixed set of ids, because an unknown id creates a session that persists |
+| `SessionSelector` | null | HTTP | pick the session per request from the `HttpContext`; an id that was never registered answers `-32601` and creates nothing |
 | `Serializer` | session, then `Config.Serializer` | HTTP and raw | serializer for this host |
 | `ContextFactory` | `HttpContext` | HTTP | what `JsonRpcContext.Current()` returns |
 | `MaxRequestBytes` | 4 MB | HTTP body, or one raw document | larger bodies get 413; a larger raw document aborts the connection |

AustinHarris.JsonRpc.Newtonsoft/AustinHarris.JsonRpc.Newtonsoft.csproj

@@ -6,6 +6,7 @@
         <Product>Json-Rpc.Net Json.NET serializer</Product>
         <Description>Json.NET (Newtonsoft.Json) serializer for JSON-RPC.Net. Lenient parsing and full Json.NET conversion semantics; pass JsonSerializerSettings to control it.</Description>
         <VersionPrefix>2.0.0</VersionPrefix>
+        <PackageReleaseNotes>https://github.com/Astn/JSON-RPC.NET/blob/master/CHANGELOG.md</PackageReleaseNotes>
         <VersionSuffix>$(VersionSuffix)</VersionSuffix>
         <Copyright>Austin Harris</Copyright>
         <PackageProjectUrl>https://github.com/Astn/JSON-RPC.NET</PackageProjectUrl>

AustinHarris.JsonRpc.SystemTextJson/AustinHarris.JsonRpc.SystemTextJson.csproj

@@ -6,6 +6,7 @@
         <Product>Json-Rpc.Net System.Text.Json serializer</Product>
         <Description>System.Text.Json serializer for JSON-RPC.Net. Utf8JsonReader/Utf8JsonWriter end to end; pass JsonSerializerOptions to control it.</Description>
         <VersionPrefix>2.0.0</VersionPrefix>
+        <PackageReleaseNotes>https://github.com/Astn/JSON-RPC.NET/blob/master/CHANGELOG.md</PackageReleaseNotes>
         <VersionSuffix>$(VersionSuffix)</VersionSuffix>
         <Copyright>Austin Harris</Copyright>
         <PackageProjectUrl>https://github.com/Astn/JSON-RPC.NET</PackageProjectUrl>

AustinHarris.JsonRpcTestN/AspNetCoreTests.cs

@@ -194,7 +194,7 @@ public async Task Tcp_DocumentSplitAcrossWrites_IsReassembled()
 
         // ------------------------------------------------------------------ DI binding honours JsonRpcOptions.SessionId
 
-        /// <summary>A JsonRpcService subclass built by DI: its base constructor binds it to the default session.</summary>
+        /// <summary>A JsonRpcService subclass built by DI: its base constructor binds it to the default session, and the host binds it to the configured one as well.</summary>
         public class TenantAutoService : JsonRpcService
         {
             [JsonRpcMethod("tenant.ping")]
@@ -251,6 +251,46 @@ public async Task Http_JsonRpcServiceSubclass_IsBoundToTheConfiguredSession()
             }
         }
 
+        /// <summary>A JsonRpcService subclass that does not bind itself: the host is its only binder.</summary>
+        public class TenantUnboundService : JsonRpcService
+        {
+            public TenantUnboundService() : base(false) { }
+
+            [JsonRpcMethod("tenant.unbound")]
+            public int Unbound() => 9;
+        }
+
+        [Test]
+        public async Task Http_JsonRpcServiceSubclass_WithoutAutoBind_IsBoundOnlyToTheConfiguredSession()
+        {
+            const string session = "aspnetcore-tenant-unbound";
+            var builder = WebApplication.CreateBuilder();
+            builder.Logging.ClearProviders();
+            builder.WebHost.ConfigureKestrel(k => k.Listen(IPAddress.Loopback, 0));
+            builder.Services.AddJsonRpc(o => o.SessionId = session);
+            builder.Services.AddJsonRpcService<TenantUnboundService>();
+
+            var app = builder.Build();
+            app.MapJsonRpc("/rpc");
+            await app.StartAsync();
+            try
+            {
+                var address = app.Services.GetRequiredService<IServer>().Features.Get<IServerAddressesFeature>().Addresses.First();
+                using var http = new HttpClient { BaseAddress = new Uri(address) };
+                var response = await http.PostAsync("/rpc", new StringContent(@"{""jsonrpc"":""2.0"",""method"":""tenant.unbound"",""id"":1}", Encoding.UTF8, "application/json"));
+                Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"result\":9,\"id\":1}", await response.Content.ReadAsStringAsync());
+
+                Assert.IsTrue(Handler.GetSessionHandler(session).MetaData.Services.ContainsKey("tenant.unbound"));
+                Assert.IsFalse(Handler.DefaultHandler.MetaData.Services.ContainsKey("tenant.unbound"), "base(false) keeps it off the default session");
+            }
+            finally
+            {
+                await app.StopAsync();
+                await app.DisposeAsync();
+                Handler.DestroySession(session);
+            }
+        }
+
         private static async Task<string> ReadUntilAsync(NetworkStream stream, string terminator)
         {
             var sb = new StringBuilder();

AustinHarris.JsonRpcTestN/AsyncInvocationTests.cs

@@ -413,9 +413,20 @@ public async Task Cancellation_DiscardsDocument_AndWaitsForTerminalState(int pha
         public async Task MethodOwnedCancellation_IsAnOrdinaryError()
         {
             Bind("run", new Func<Task<int>>(async () => { await Task.Yield(); throw new OperationCanceledException("method-owned"); }));
-            var result = await Run(Request("run"));
-            Error(result, -32603);
-            StringAssert.Contains("method-owned", result);
+            Exception seen = null;
+            Handler.GetSessionHandler(_session).SetErrorHandler((request, error) => { seen = error.data as Exception; return error; });
+            try
+            {
+                var result = await Run(Request("run"));
+                Error(result, -32603);
+                StringAssert.Contains("\"data\":null", result, "an unhandled exception is redacted on the wire");
+                Assert.IsInstanceOf<OperationCanceledException>(seen, "but it is an ordinary error, not a cancellation of the processing");
+                Assert.AreEqual("method-owned", seen.Message);
+            }
+            finally
+            {
+                Handler.GetSessionHandler(_session).SetErrorHandler(null);
+            }
         }
 
         [TestCase(false)] [TestCase(true)]

AustinHarris.JsonRpcTestN/DispatchHardeningTests.cs

@@ -39,6 +39,9 @@ private class HardeningService
             [JsonRpcMethod("echo")]
             public string Echo(string s) => s;
 
+            [JsonRpcMethod("unserializable")]
+            public Unserializable Unserializable() => new Unserializable();
+
             [JsonRpcMethod("accept")]
             public int Accept(object o) => 7;
 
@@ -103,6 +106,12 @@ private class SessionProbe
             public string WhichSession() => _name;
         }
 
+        /// <summary>A result every serializer fails to write: the getter throws.</summary>
+        public class Unserializable
+        {
+            public string Secret => throw new InvalidOperationException("private database /server/secret");
+        }
+
         private class TaskReturningService
         {
             [JsonRpcMethod("asyncTask")]
@@ -261,8 +270,18 @@ public void PreProcessHandler_Throwing_IsAnInternalError(string name)
                 handler.SetPreProcessHandler((request, context) => throw new InvalidOperationException("hook failed"));
                 var response = Parse(Run("{\"method\":\"ping\",\"id\":1}", null, serializer));
                 Assert.AreEqual(-32603, (int)response["error"]["code"]);
-                Assert.AreEqual("hook failed", (string)response["error"]["data"]["Message"]);
+                Assert.AreEqual(JTokenType.Null, response["error"]["data"].Type, "a hook failure is an unhandled exception: redacted like any other");
                 Assert.AreEqual(1, (int)response["id"]);
+                Config.IncludeExceptionDetails = true;
+                try
+                {
+                    response = Parse(Run("{\"method\":\"ping\",\"id\":1}", null, serializer));
+                    Assert.AreEqual("hook failed", (string)response["error"]["data"]["Message"]);
+                }
+                finally
+                {
+                    Config.IncludeExceptionDetails = false;
+                }
             }
             finally
             {
@@ -315,23 +334,45 @@ public void ExceptionDetails_AreRedactedByDefault(string name)
             var serializer = SerializerCatalog.Create(name);
             Assert.IsFalse(Config.IncludeExceptionDetails, "the default is off");
 
-            var response = Parse(Run("{\"method\":\"throws\",\"id\":1}", null, serializer));
-            Assert.AreEqual(-32603, (int)response["error"]["code"]);
-            var data = (JObject)response["error"]["data"];
-            Assert.AreEqual("System.InvalidOperationException", (string)data["ClassName"]);
-            Assert.AreEqual("private database /server/secret", (string)data["Message"]);
-            Assert.AreEqual(JTokenType.Null, data["Source"].Type);
-            Assert.AreEqual(JTokenType.Null, data["StackTraceString"].Type);
-            Assert.AreEqual(0, (int)data["HResult"]);
-            Assert.AreEqual(JTokenType.Null, data["InnerException"].Type);
-
-            response = Parse(Run("{\"method\":\"throwsInner\",\"id\":1}", null, serializer));
-            data = (JObject)response["error"]["data"];
-            Assert.AreEqual("System.ArgumentException", (string)data["ClassName"], "a wrapped exception is reported through its inner exception (unchanged legacy behaviour)");
-            Assert.AreEqual("inner message", (string)data["Message"]);
-            Assert.AreEqual(JTokenType.Null, data["StackTraceString"].Type);
-            Assert.AreEqual(JTokenType.Null, data["InnerException"].Type, "the inner chain is not sent");
-            StringAssert.DoesNotContain("innermost message", response.ToString());
+            var raw = Run("{\"method\":\"throws\",\"id\":1}", null, serializer);
+            Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32603,\"message\":\"Internal Error\",\"data\":null},\"id\":1}", raw,
+                "nothing about the exception leaves the process: no type name, no message");
+
+            raw = Run("{\"method\":\"throwsInner\",\"id\":1}", null, serializer);
+            Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32603,\"message\":\"Internal Error\",\"data\":null},\"id\":1}", raw);
+        }
+
+        [TestCaseSource(nameof(Serializers))]
+        public void ExceptionDetails_ErrorHandlerStillSeesTheException(string name)
+        {
+            var serializer = SerializerCatalog.Create(name);
+            var handler = Handler.GetSessionHandler(Session);
+            Exception seen = null;
+            try
+            {
+                handler.SetErrorHandler((request, error) => { seen = error.data as Exception; return error; });
+                var response = Parse(Run("{\"method\":\"throws\",\"id\":1}", null, serializer));
+                Assert.IsInstanceOf<InvalidOperationException>(seen, "the handler gets the exception itself, redaction happens when the response is written");
+                Assert.AreEqual(JTokenType.Null, response["error"]["data"].Type);
+
+                // a handler that replaces the error with its own data is not redacted: that data is authored
+                handler.SetErrorHandler((request, error) => new JsonRpcException(-32000, "Server error", "ticket 42"));
+                response = Parse(Run("{\"method\":\"throws\",\"id\":1}", null, serializer));
+                Assert.AreEqual("ticket 42", (string)response["error"]["data"]);
+            }
+            finally
+            {
+                handler.SetErrorHandler(null);
+            }
+        }
+
+        [TestCaseSource(nameof(Serializers))]
+        public void ExceptionDetails_ResultSerializationFailure_IsRedactedToo(string name)
+        {
+            var serializer = SerializerCatalog.Create(name);
+            var raw = Run("{\"method\":\"unserializable\",\"id\":1}", null, serializer);
+            Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32603,\"message\":\"Internal Error\",\"data\":null},\"id\":1}", raw,
+                "an exception thrown while writing the result is an unhandled exception as well");
         }
 
         [TestCaseSource(nameof(Serializers))]

AustinHarris.JsonRpcTestN/ErrorDataTests.cs

@@ -154,11 +154,25 @@ public void TheSameExceptionFromInsideTheMethod_StaysInternal(string name)
             var s = SerializerCatalog.Create(name);
             var response = Run("{\"method\":\"ed.parses\",\"params\":[\"abc\"],\"id\":1}", s);
             Assert.AreEqual(-32603, (int)response["error"]["code"], "a FormatException thrown by the method is not a binding failure");
-            Assert.AreEqual("System.FormatException", (string)response["error"]["data"]["ClassName"]);
+            Assert.AreEqual(JTokenType.Null, response["error"]["data"].Type, "with details off an unhandled exception is data:null");
 
             response = Run("{\"method\":\"ed.throwsFormat\",\"id\":1}", s);
             Assert.AreEqual(-32603, (int)response["error"]["code"]);
-            Assert.AreEqual("from the method", (string)response["error"]["data"]["Message"]);
+            Assert.AreEqual(JTokenType.Null, response["error"]["data"].Type);
+            StringAssert.DoesNotContain("from the method", response.ToString());
+            StringAssert.DoesNotContain("FormatException", response.ToString());
+
+            try
+            {
+                Config.IncludeExceptionDetails = true;
+                response = Run("{\"method\":\"ed.throwsFormat\",\"id\":1}", s);
+                Assert.AreEqual("System.FormatException", (string)response["error"]["data"]["ClassName"]);
+                Assert.AreEqual("from the method", (string)response["error"]["data"]["Message"]);
+            }
+            finally
+            {
+                Config.IncludeExceptionDetails = false;
+            }
         }
 
         [TestCaseSource(nameof(Serializers))]
@@ -271,9 +285,12 @@ public void UnsupportedType_StaysInternal()
             try
             {
                 ServiceBinder.BindMethod(session, "takesDelegate", new Func<Action, int>(a => 1));
+                Exception seen = null;
+                Config.SetErrorHandler(session, (request, error) => { seen = error.data as Exception; return error; });
                 var response = JObject.Parse(JsonRpcProcessor.ProcessSync(session, "{\"method\":\"takesDelegate\",\"params\":[{}],\"id\":1}", null, SerializerCatalog.Create("jsmn")));
                 Assert.AreEqual(-32603, (int)response["error"]["code"], response.ToString());
-                Assert.AreEqual("System.NotSupportedException", (string)response["error"]["data"]["ClassName"]);
+                Assert.AreEqual(JTokenType.Null, response["error"]["data"].Type, "redacted on the wire");
+                Assert.IsInstanceOf<NotSupportedException>(seen, "the handler sees why");
             }
             finally
             {