tests: land the review round's reproductions, with the refuted claims turned into what is true (#197)

The 24-agent review round produced 20 test files that were never committed, because 11 of their 31
reproductions had been refuted by an adversarial pass and shipping them would have put two mutually
exclusive assertions in the same suite - one of them asserts the opposite of a test that passes
today. Leaving all 20 out cost the 77 PASSING tests in them, which is the larger number: controls,
matrices and ordinary coverage the agents wrote alongside their claims.

So the refuted 11 are converted rather than deleted. Each drove a real scenario that was already
wired up; what was wrong was the conclusion. Each now pins the behaviour that was verified correct
and records why the claim did not hold:

  resume       a ticket issued before a rotation is REFUSED after it, and the client still served.
               Ticket keys are per SSL_CTX; nginx, Apache and HAProxy retire them on reload too, and
               the test below it depends on that - a ticket outliving a rotation would carry the old
               verify verdict past the new anchors.
  truncation   the pipe reports both endings alike and the SESSION tells them apart. Faulting the
               cut one would fault every client that merely disposes its SslStream.
  identity     a CN the decoder refuses is not named, even where the rendered subject cannot tell it
               apart - PeerSubject is documented for people, PeerCommonName is what authorizes.
  mtls/quic    a DN or CN too long to record is reported as NO name, never a prefix. Fails closed on
               purpose: a prefix can belong to someone else.
  quic/fault   a throwing HandshakeCompleted is logged and the connection keeps serving, which is
               this runtime's policy for user-code faults everywhere else.
  http3        a body cut mid-frame kills the connection with H3_FRAME_ERROR and the reader signals
               an ordinary end - the contract all three body readers document identically.
  parity       omitting the host table is refused on QUIC and applied on TCP, pinned as a KNOWN
               DIVERGENCE. Neither side is a bug; TCP's is documented and pinned green, so resolving
               it is a deliberate breaking change rather than a fix, and this makes sure nobody
               resolves it by accident.
  lifetime     deleted - the file already had a passing test for the only true part.

Two were better answered by fixing the code than by rewriting the test, which both the refuting
agent and the commit auditor independently recommended: TlsService.Start now range-checks
MinProtocolVersion and HandshakeTimeoutMs. Both resolved to something plausible rather than
failing - an undefined version through a not-Tls13 ternary to the TLS 1.2 floor, and a negative
timeout to no handshake sweep at all, silently removing the only bound on a peer that connects and
says nothing. No config binder validates an enum, so neither needed a cast to arrive.

Two identity doc comments were corrected to match: null from PeerSubject/PeerCommonName is a
refusal, not "the peer offered none".

The 19 surviving reproductions stay as Pending - they report PEND and fail the run if they start
passing. 419 passed, 0 failed, 20 pending, across all six suites.

Author: MDA2AV

src/ioxide/Tls/TlsService.cs

@@ -205,6 +205,29 @@ private static byte[] Fold(string host)
     /// </param>
     public static TlsService Start(Reactor reactor, TlsOptions options, bool register = true)
     {
+        // Scalars first, because the checks below reason about COMBINATIONS and a value outside its
+        // own domain makes that reasoning meaningless. Both of these resolve to something plausible
+        // rather than failing, which is the shape worth refusing: an undefined version maps to the
+        // TLS 1.2 floor through a not-Tls13 ternary, and a negative timeout disables the handshake
+        // sweep entirely because both readers guard on "> 0" - so the one bound on a peer that
+        // connects and then says nothing is silently off. No config binder validates an enum
+        // (Enum.Parse<TlsProtocolVersion>("3") succeeds), so neither value needs a cast to arrive.
+        if (!Enum.IsDefined(options.MinProtocolVersion))
+        {
+            throw new ArgumentException(
+                $"MinProtocolVersion is {(int)options.MinProtocolVersion}, which is not one of "
+                + "Default, Tls12 or Tls13. Name the floor you want rather than leaving it to be "
+                + "resolved.", nameof(options));
+        }
+
+        if (options.HandshakeTimeoutMs < 0)
+        {
+            throw new ArgumentException(
+                $"HandshakeTimeoutMs is {options.HandshakeTimeoutMs}. Zero disables the handshake "
+                + "sweep; a negative value would disable it too, which is worth saying rather than "
+                + "arriving at by accident.", nameof(options));
+        }
+
         // RX alone cannot be programmed: the handoff shares the TCP_ULP that EnableTx installs.
         // Refuse loudly rather than silently serving the userspace path the caller opted out of.
         if (options.KernelRx && !options.KernelTx)

src/protocols/ioxide.ngtcp2/Connection/QuicEngineConnection.cs

@@ -39,6 +39,11 @@ public unsafe partial class QuicEngineConnection : QuicConnection
     /// whose organisation is <c>Acme\/CN=admin.internal</c> satisfies a
     /// <c>Contains("/CN=admin.internal")</c> check while being a different principal.
     /// <see cref="PeerCommonName"/> is the value to compare instead.
+    ///
+    /// Null also when the DN does not fit the 1024 bytes the shim records, which is a refusal
+    /// rather than an omission: a truncated DN is plausible, comparable, and can equal a DIFFERENT
+    /// principal's prefix, so no name is reported instead of a partial one. Nothing here ever
+    /// hands back a shortened identity.
     /// </remarks>
     public string? PeerSubject
     {
@@ -65,8 +70,11 @@ public string? PeerSubject
     /// with <see cref="StringComparison.Ordinal"/>.
     ///
     /// Null when there was no validated certificate, when the subject carries no CN (legitimate:
-    /// modern certificates identify by subjectAltName), or when the CN is empty or contains an
-    /// embedded NUL, which is a name built to be read differently by different consumers.
+    /// modern certificates identify by subjectAltName), when the CN is empty or contains an
+    /// embedded NUL - a name built to be read differently by different consumers - or when it
+    /// exceeds the 256 bytes recorded for it, which is four times RFC 5280's ub-common-name of 64.
+    /// Every one of those is a refusal rather than an omission: the accessor never reports a name
+    /// it had to shorten, because a prefix can belong to someone else.
     /// </summary>
     public string? PeerCommonName
     {

tests/Ioxide.Tests.E2E/Protocols/H3AlpnTests.cs

@@ -1,22 +1,211 @@
 using ioxide;
+using ioxide.nghttp3;
 using ioxide.ngtcp2;
 
 namespace Ioxide.Tests;
 
 /// <summary>
-/// ALPN as HTTP/3 requires it, and what the negotiated protocol reads back as.
+/// ALPN as HTTP/3 requires it, and what the negotiated protocol reads back as. RFC 9001 section
+/// 8.1 makes ALPN mandatory for QUIC (no mutual protocol = no_application_protocol, during the
+/// handshake); RFC 9114 section 3.1 makes "h3" the token an HTTP/3 server may serve.
 /// </summary>
 /// <remarks>
-/// Reserved for a review pass whose deliverable is a FAILING test. See tests/README.md: a defect
-/// that has been reproduced is committed as <c>runner.Pending</c> - it reports PEND while it still
-/// fails, and fails the run the moment it starts passing.
+/// Two behaviours in this area were examined and could NOT be driven from this suite, so they are
+/// recorded here rather than half-tested:
 ///
-/// Empty is a legitimate outcome. It means the area was examined and nothing was found that could
-/// be made to fail, which is worth more than a test that passes for reasons nobody established.
+/// - Server preference order with a multi-token offer (the shim's iq_on_client_hello walks the
+///   allowlist on the outside, so the server's order decides, like the TCP side's
+///   AlpnNegotiationTests pins). The shim's client entry points hand picotls exactly ONE token -
+///   a single iovec, count = 1 - so no in-tree client can offer several protocols at once.
+///
+/// - The negotiated token reading back as nothing when it exceeds the 64-byte read-back buffer
+///   (iq_conn_get_alpn returns 0 when the token does not fit the buffer
+///   QuicEngineConnection.HandshakeCompletedOnce hands it). A legal ALPN token may be 255 bytes,
+///   but the shim's client stores its offer in a char[64] via snprintf, truncating it to 63 - so
+///   the shortest token that would trip the server's cap cannot be offered from here. The 63-byte
+///   test below pins the longest reachable token instead.
 /// </remarks>
 internal static class H3AlpnTests
 {
     public static void Register(Runner runner)
     {
+        runner.Test("quic/alpn: control - a pinned engine serves an h3 offer and the handler reads back 'h3'", () =>
+        {
+            // The control for every refusal below: the same engine shape, the same handler, the
+            // one offer an HTTP/3 server may accept - and it serves. Also the only place the
+            // SERVER-side NegotiatedProtocol value is asserted: the pure-C# stack consumes it in
+            // its backstop, but nothing else pins that the shim's read-back (iq_conn_get_alpn)
+            // surfaces the very token the client offered.
+            (string certPath, string keyPath) = TestCert.Ensure();
+            using var engine = new QuicEngine(certPath, keyPath, cidLength: 8, alpn: ["h3"]);
+
+            (_, int udpPort) = TestServer.StartDatagram(
+                onDatagram: null,
+                quicFactory: engine.CreateFactory(),
+                quicHandle: (_, conn) => new Nghttp3Connection(conn).RunBufferedAsync(
+                    _ => Nghttp3Response.Text($"alpn={conn.NegotiatedProtocol ?? "(none)"}")));
+
+            using var client = new H3TestClient("127.0.0.1", udpPort);
+            client.Connect();
+            Assert.True(client.CompleteHandshake(timeoutMs: 5000), "handshake did not complete");
+
+            (int status, string body) = client.Get("/alpn", timeoutMs: 5000);
+            Assert.Equal(200, status);
+            Assert.Equal("alpn=h3", body);
+        });
+
+        runner.Test("quic/alpn: a pinned engine refuses a no-overlap offer during the handshake, with a close", () =>
+        {
+            // RFC 9001 section 8.1: no mutual protocol fails the handshake with
+            // no_application_protocol. The engine-side allowlist is the real fix for serving h3
+            // to clients that never claimed it, and every h3 test site now pins ["h3"] - but the
+            // refusal itself was one test deep and asserted only "not served". This pins the
+            // stronger half: the handshake never completes, and the refusal ARRIVES as a close.
+            // PeerClosed is the load-bearing assert - a server that silently dropped the
+            // connection would also fail CompleteHandshake, by timeout, and a hang is not a
+            // refusal (it is also not the alert RFC 9001 requires).
+            (string certPath, string keyPath) = TestCert.Ensure();
+            using var engine = new QuicEngine(certPath, keyPath, cidLength: 8, alpn: ["h3"]);
+
+            (_, int udpPort) = TestServer.StartDatagram(
+                onDatagram: null,
+                quicFactory: engine.CreateFactory(),
+                quicHandle: static (_, conn) => new Nghttp3Connection(conn).RunBufferedAsync(
+                    static _ => Nghttp3Response.Text("must never serve")));
+
+            using var client = new H3TestClient("127.0.0.1", udpPort) { Alpn = "echo" };
+            client.Connect();
+            bool done = client.CompleteHandshake(timeoutMs: 5000);
+            Assert.True(!done, "a pinned engine must not complete a handshake with no ALPN overlap");
+            Assert.True(client.PeerClosed,
+                "the refusal must arrive as a close during the handshake - a timeout is a hang, not a refusal");
+        });
+
+        runner.Test("quic/alpn: a pinned engine refuses a client that offered no ALPN at all", () =>
+        {
+            // The other half of RFC 9001 section 8.1: a client that offers NOTHING. An empty Alpn
+            // makes the shim's client omit the extension entirely (it only hands picotls a list
+            // for a non-empty token), and a pinned server must treat that as no overlap - not as
+            // "nothing to check". The permissive engine's documented default is to confirm even
+            // this; pinning is what closes it, so the pinned refusal is the behaviour to hold.
+            (string certPath, string keyPath) = TestCert.Ensure();
+            using var engine = new QuicEngine(certPath, keyPath, cidLength: 8, alpn: ["h3"]);
+
+            (_, int udpPort) = TestServer.StartDatagram(
+                onDatagram: null,
+                quicFactory: engine.CreateFactory(),
+                quicHandle: static (_, conn) => new Nghttp3Connection(conn).RunBufferedAsync(
+                    static _ => Nghttp3Response.Text("must never serve")));
+
+            using var client = new H3TestClient("127.0.0.1", udpPort) { Alpn = "" };
+            client.Connect();
+            bool done = client.CompleteHandshake(timeoutMs: 5000);
+            Assert.True(!done, "RFC 9001 8.1: a pinned engine must refuse a client that offered no ALPN");
+            Assert.True(client.PeerClosed,
+                "the refusal must arrive as a close during the handshake - a timeout is a hang, not a refusal");
+        });
+
+        runner.Test("quic/alpn: a 63-byte token - the longest the harness can offer - reads back whole", () =>
+        {
+            // The negotiated token reads back through a 64-byte buffer, and iq_conn_get_alpn
+            // answers 0 - "no protocol" - for anything that does not fit, so a shrunk buffer
+            // would not fail loudly: it would report a legal negotiated token as none at all.
+            // 63 bytes is the longest offer the harness client can make (its own char[64] +
+            // snprintf truncation - see the file remarks), which makes it the boundary this
+            // suite can hold: the whole token, not empty, not clipped.
+            //
+            // Recorded at handshake completion rather than through a served response, so this
+            // stays true even once the nghttp3 layer learns to refuse non-h3 connections.
+            string big = new string('a', 63);
+            var recorded = new TaskCompletionSource<string?>(TaskCreationOptions.RunContinuationsAsynchronously);
+
+            (string certPath, string keyPath) = TestCert.Ensure();
+            using var engine = new QuicEngine(certPath, keyPath, cidLength: 8);   // permissive: confirms the offer
+
+            (_, int udpPort) = TestServer.StartDatagram(
+                onDatagram: null,
+                quicFactory: engine.CreateFactory(),
+                quicHandle: (_, conn) =>
+                {
+                    ((QuicEngineConnection)conn).HandshakeCompleted =
+                        () => recorded.TrySetResult(conn.NegotiatedProtocol);
+                    return new Nghttp3Connection(conn).RunBufferedAsync(
+                        static _ => Nghttp3Response.Text("ok"));
+                });
+
+            using var client = new H3TestClient("127.0.0.1", udpPort) { Alpn = big };
+            client.Connect();
+            Assert.True(client.CompleteHandshake(timeoutMs: 5000), "handshake did not complete");
+
+            Assert.True(recorded.Task.Wait(5000), "the server never reported handshake completion");
+            Assert.Equal(big, recorded.Task.Result);
+        });
+
+        runner.Pending("h3/nghttp3: a connection that did not negotiate h3 is not served", () =>
+        {
+            // The mirror of Http3Tests' "a connection that did not negotiate h3 is not served",
+            // on the OTHER stack. The backstop landed only in the pure-C# layer
+            // (Http3Connection.RunCoreAsync checks NegotiatedProtocol once the control stream is
+            // up); Nghttp3Connection never reads it, so on an engine built without an allowlist -
+            // the constructor's documented default and its own doc example - it answers HTTP/3 on
+            // a connection that negotiated "echo", a protocol the client actually asked for and
+            // is entitled to believe it got.
+            (string certPath, string keyPath) = TestCert.Ensure();
+            using var engine = new QuicEngine(certPath, keyPath, cidLength: 8);   // permissive, on purpose
+
+            (_, int udpPort) = TestServer.StartDatagram(
+                onDatagram: null,
+                quicFactory: engine.CreateFactory(),
+                quicHandle: static (_, conn) => new Nghttp3Connection(conn).RunBufferedAsync(
+                    static _ => Nghttp3Response.Text("served-by-nghttp3")));
+
+            using var client = new H3TestClient("127.0.0.1", udpPort) { Alpn = "echo" };
+            client.Connect();
+            Assert.True(client.CompleteHandshake(timeoutMs: 5000),
+                "the permissive engine should still complete the handshake - that is the point");
+
+            (int status, string body) = client.Get("/nope", timeoutMs: 3000);
+            Assert.True(status != 200, $"an h3 handler must not serve a non-h3 connection, got {status} '{body}'");
+        }, "the ALPN backstop landed only in the pure-C# stack; Nghttp3Connection never reads "
+         + "NegotiatedProtocol, so a permissive engine's 'echo' connection is answered 200");
+
+        runner.Pending("quic/alpn: a non-ascii allow-list token must not admit a protocol nobody configured", () =>
+        {
+            // QuicEngine.AlpnWire encodes each configured token with Encoding.ASCII, whose
+            // fallback substitutes '?' for anything non-ascii - so ["Ũ2"] goes on the wire as the
+            // allowlist entry "?2". Two consequences: the configured token itself can never
+            // negotiate (a client offering the actual bytes of "Ũ2" finds no match), and every
+            // non-ascii token collapses onto '?', so a client offering the literal "?2" is
+            // admitted, served, and reads back NegotiatedProtocol == "?2" - a protocol nobody
+            // configured. The TCP side's BuildAlpnWire has the same shape with a worse symptom
+            // (UTF-16 units cast to bytes turn "Ũ2" into the real "h2"); the fix on either side
+            // is to refuse a non-ascii token at construction, like the >255-byte one already is,
+            // or to encode it faithfully - both make this body pass.
+            (string certPath, string keyPath) = TestCert.Ensure();
+            QuicEngine engine;
+            try
+            {
+                engine = new QuicEngine(certPath, keyPath, cidLength: 8, alpn: ["Ũ2"]);
+            }
+            catch (ArgumentException)
+            {
+                return;   // refused at configuration - the defect is gone
+            }
+
+            using (engine)
+            {
+                (_, int udpPort) = TestServer.StartDatagram(
+                    onDatagram: null,
+                    quicFactory: engine.CreateFactory(),
+                    quicHandle: static (_, conn) => new Nghttp3Connection(conn).RunBufferedAsync(
+                        static _ => Nghttp3Response.Text("must never serve")));
+
+                using var client = new H3TestClient("127.0.0.1", udpPort) { Alpn = "?2" };
+                client.Connect();
+                Assert.True(!client.CompleteHandshake(timeoutMs: 5000),
+                    "a client offering '?2' completed the handshake against an allow list of 'Ũ2'");
+            }
+        }, "AlpnWire's ASCII '?' substitution puts \"?2\" on the wire for the configured \"Ũ2\", "
+         + "and a client offering the literal \"?2\" is admitted and served");
     }
 }