🐛 Expose supportedProtocols on ConnectionManager to fix fallback for RFC-strict servers

[chrisslazzz]

fallbackAdapter silently never fires against HTTP/1.1-only servers that enforce ALPN strictly. With supportedProtocols: ['h2'] hardcoded, a compliant server has no matching protocol and — per RFC 7301 §3.2 — responds with a fatal no_application_protocol alert, aborting the TLS handshake. The resulting HandshakeException is never caught by Http2Adapter._fetch, so it propagates to the caller instead of reaching the fallback.

This PR exposes supportedProtocols on ConnectionManager (default ['h2'], no behaviour change). Setting it to ['h2', 'http/1.1'] allows the handshake to complete against any server: if the server selects http/1.1, _throwIfH2NotSelected raises DioH2NotSupportedException, which Http2Adapter already catches and routes to fallbackAdapter.

Implementation, tests, and CHANGELOG by Claude (Sonnet 4.6) via GitHub Copilot.

Fixes #2584

[chrisslazzz]

This test exercises the existing fallback path against a lenient server
(www.baidu.com), which completes the TLS handshake regardless of the ALPN
list and therefore doesn't reproduce the root cause described in the PR — a
strict RFC 7301 server aborting the handshake before Dio code runs.

Testing the actual fix would require a server that sends a fatal
no_application_protocol alert when only ['h2'] is offered, but completes
the handshake when ['h2', 'http/1.1'] is offered. Two options:

1. Local TLS server — spin up a SecureServerSocket with
   supportedProtocols: ['http/1.1'] in test setup. Needs embedded
   self-signed certificates, which the repo doesn't currently have.
2. Remote host — use a public server that strictly enforces ALPN this
   way. Happy to do this if the maintainers know of a reliable one, though
   leaning on an external host for a correctness test adds fragility to CI.

Happy to implement either if that's the direction the maintainers want to go.

[AlexV525]

Hi @chrisslazzz . Your commit email doesn't seem to be associated with your GitHub account. Please correct this so we can at least verify the user.

[chrisslazzz]

Hi @chrisslazzz . Your commit email doesn't seem to be associated with your GitHub account. Please correct this so we can at least verify the user.

sorry about that. Should be ok now

[AlexV525]

sorry about that. Should be ok now

1/2 maybe; the GPG key is still missing. 🤔

[chrisslazzz]

sorry about that. Should be ok now

1/2 maybe; the GPG key is still missing. 🤔

uff, now maybe 1/1? 👀

[CaiJingLong]

PR review — fix is correct, but the test doesn't defend the bug

I independently reproduced the issue (#2584) and confirmed the root cause. The fix approach here — exposing supportedProtocols and replacing both hardcoded ['h2'] literals — is the right minimal, backwards-compatible change.

However, the added test has a significant gap that the PR author already flagged in their own review comment:

The test does not exercise the bug

The test routes to fallbackAdapter when server selects http/1.1 via ALPN hits www.baidu.com, which is a lenient ALPN server — it completes the TLS handshake regardless of the client's offered protocols and selects nothing (selectedProtocol == null). This means:

1. The test would pass even without this PR's code change, because the existing _throwIfH2NotSelected path already handles selectedProtocol == null.
2. The test does not verify the actual fix — that the handshake now succeeds against a strict RFC 7301 server (one that sends no_application_protocol when only ['h2'] is offered).

I confirmed this by testing three TLS server implementations locally:

Server	Offers only ['h2'] → handshake	Strict ALPN?
Dart SecureServerSocket (macOS/BoringSSL)	Succeeds, selectedProtocol = null	No (lenient)
Python ssl (OpenSSL set_alpn_protos)	Succeeds, selectedProtocol = null	No (lenient)
Go crypto/tls (NextProtos: ["http/1.1"])	Fails: HandshakeException: TLSV1_ALERT_NO_APPLICATION_PROTOCOL	Yes (RFC-strict)

Only Go's crypto/tls reproduces the actual bug. The www.baidu.com test is in the same category as the first two rows — it cannot distinguish the fix from a no-op.

What would make the test effective

A test that actually defends this fix needs a server that:

• Sends no_application_protocol when the client offers only ['h2'] (reproducing the bug), and
• Completes the handshake and selects http/1.1 when the client offers ['h2', 'http/1.1'] (verifying the fix).

This requires a local TLS server with strict ALPN enforcement. Two options the PR author already mentioned:

1. Embedded Go binary — compile a small Go crypto/tls server binary into test/ and spawn it from the test. This is the most reliable option since Go's crypto/tls is one of the few common implementations that enforces ALPN strictly by default.
2. Dart SecureServerSocket with a custom ALPN callback — if Dart exposes an API to make the server ALPN-strict (returning a fatal alert on no match), this would keep the test pure-Dart. However, I'm not aware of such an API in current dart:io.

Without a test that actually fails on the old code and passes on the new code, a future regression that re-hardcodes ['h2'] would go undetected.

Minor: the fix is opt-in

The default stays ['h2'], so users hitting this bug today will continue to get HandshakeException unless they explicitly pass supportedProtocols: ['h2', 'http/1.1']. This is the correct backwards-compatible choice (changing the default could cause dual-protocol servers to prefer http/1.1), but it's worth noting in the CHANGELOG or README that this is a required configuration step for strict-ALPN server compatibility, not an automatic fix.

This comment was generated by AI agent omp (model: devin/glm-5-2).

[chrisslazzz]

@CaiJingLong I've reworked the tests to use openssl s_server as the strict-ALPN enforcement server instead of a Go binary. What do you think? Is openssl available in CI?

The setup sends no_application_protocol when the client advertises only h2 and completes the handshake with http/1.1 selected when both are offered. The tests call markTestSkipped if openssl is not found on PATH, so they degrade gracefully.

[AlexV525]

Re-review — the reworked tests now genuinely reproduce the bug ✅

Thanks for the rework, @chrisslazzz. The openssl s_server harness fixes the exact gap I flagged last time (the www.baidu.com test couldn't distinguish the fix from a no-op). Two independent verification passes this round — (a) local reproduction + (b) a fact-checking review + (c) an adversarial review — agree the fix is correct. Details below.

(a) Local verification

• Tests pass: ran the two new tests + the full test/http2_test.dart (11 pass, 1 proxy test skipped behind -P all). dart analyze and dart format --set-exit-if-changed both clean.
• The new test actually defends the fix (regression check): I temporarily re-hardcoded supportedProtocols: const ["h2"] at both call sites (keeping the new param so it still compiles). Test 2 then failed with the exact HandshakeException: TLSV1_ALERT_NO_APPLICATION_PROTOCOL(tls_record.cc:486):

  DioException [unknown]: null
  Error: HandshakeException: Handshake error in client (OS Error:
      TLSV1_ALERT_NO_APPLICATION_PROTOCOL(tls_record.cc:486))
  

  Restored → green again. So Test 2 (['h2','http/1.1'] → 200 via fallback) is a real guard, not a characterization test. (Test 1, the ['h2']-only case, is a characterization test — it passes before and after — but it documents the limitation clearly, which is fine.)
• The socket-leak fix is real: on main, _throwIfH2NotSelected throws with the just-negotiated SecureSocket still alive at both sites (connection_manager_imp.dart direct-https ~L194 and proxy SecureSocket.secure ~L271). The PR's try { … } on DioH2NotSupportedException { socket.destroy(); rethrow; } closes it. The destroy is safe — no caller holds a reference (onClientCreate gets a ClientSetting, never the socket), and on the proxy path destroying the SecureSocket cascades to the underlying proxySocket.
• Backwards-compatible: const ['h2'] default sends identical ClientHello ALPN bytes to the old hardcoded literal. The socket.destroy() change only affects the fallback path (a genuine leak fix for existing lenient-server users too).

To your question: is openssl available in CI?

Yes. .github/workflows/tests.yml runs only ubuntu-latest (no windows/macos in the matrix), and openssl ships in the ubuntu-latest base image. So the new tests will not be skipped in CI. The markTestSkipped fallback on ProcessException only matters for local Windows/Mac devs without openssl on PATH.

(b) Fact-checking pass — every external claim checks out

A read-only fact-checking review verified each claimable assertion against primary sources. Highlights:

Claim	Verdict
RFC 7301 §3.2 mandates a fatal no_application_protocol(120) alert on zero overlap	✅ https://www.rfc-editor.org/rfc/rfc7301#section-3.2 resolves and quotes verbatim "the server SHALL respond with a fatal 'no_application_protocol' alert"
openssl s_server -alpn is RFC-7301-strict	✅ OpenSSL docs + empirical reproduction above
HandshakeException is not caught by _fetch's on SocketException catch	✅ HandshakeException→TlsException and SocketException→IOException are siblings (api.dart.dev), so it bubbles out as DioException.error — exactly what Test 1 asserts
Socket "abandoned" wording in CHANGELOG	✅ literally true on main
[Http2Adapter.fallbackAdapter] dartdoc ref resolves	✅ (http2_adapter.dart:47)
AI attribution (§8.3)	✅ commit trailer + PR-body disclosure both present
Hallucinated RFC/version/CVE	✅ none found

(c) Adversarial pass — no blockers, all findings are test-harness hardening

The core fix is correct and security-neutral; every issue found is in the openssl s_server harness or docs. Ranked by impact:

1. The "openssl s_server doesn't support port 0" comment is wrong — and fixing it removes the flakiness root cause. openssl s_server … -accept 127.0.0.1:0 does bind an ephemeral port and prints ACCEPT 127.0.0.1:<port> on stdout (verified on OpenSSL 3.6.3). So:

   • The port-probe + close() + re-bind is an unnecessary TOCTOU race.
   • The fixed 300ms sleep is a guess that can lose on a loaded runner.
   • The // openssl buffers stderr … can't rely on the ACCEPT line comment is also wrong — the ACCEPT line is on stdout and is only hidden by -quiet.

   Suggested fix: spawn with -accept 127.0.0.1:0, drop -quiet (or keep it and poll), and parse the bound port from the ACCEPT host:PORT line as the deterministic readiness signal — no probe, no sleep, no race.

2. Register the process.kill() teardown inside _bindHttp11OnlyServer right after Process.start, not in each test body. Today, if the test is cancelled during the 300ms window, the openssl child is orphaned.

3. Guard the stdout listener against re-entry (var responded = false; … if (responded) return;) — a fragmented request fires the listener twice and the second stdin.write throws StateError on the closed sink. Low risk for this minimal GET, but fragile.

4. Docs/validation for misconfiguration: the dartdoc only shows the positive case. Note that supportedProtocols must contain 'h2' for HTTP/2 to be usable — [] or ['http/1.1'] silently makes the adapter always fall back. A one-line dartdoc note (or an ArgumentError in the factory) would help.

5. Missing-cert → opaque failure: if openssl is found but can't load the certs (wrong CWD), it exits non-zero and the test fails with a confusing SocketException instead of skipping. A quick exitCode check + markTestSkipped mirrors the not-on-PATH path.

6. (nit) The dartdoc sentence in connection_manager.dart ends mid-phrase (…to support servers that strictly disable h2) before the param list — worth finishing with a period.

One process note

This PR now bundles two concerns (the opt-in supportedProtocols param + the socket-leak fix). They're tightly related — enabling ['h2','http/1.1'] makes the leak more likely to trigger — so bundling is defensible, but the leak fix changes behavior for all existing fallback-path users, not just opt-in ones. Worth a maintainer ack on that; the opt-in default itself is the right call (flipping the default to ['h2','http/1.1'] would let dual-stack servers silently downgrade HTTP/2 users).

Verdict

Approve-able once items 1–2 above are addressed (deterministic openssl readiness + guaranteed teardown) — those are the CI-flake risks. The rest are nits. The fix logic itself is sound, minimal, backwards-compatible, and now properly tested.

Review, local test reproduction, and both subagent passes (fact-check + adversarial) by omp (model: glm-5.2).

[AlexV525]

Added: exclude the test fixture private key from the published package

I pushed a follow-up commit (ca3091f) to this branch. It adds two things so the self-signed server.key/server.crt introduced for the strict-ALPN tests don't end up in the published artifact or trigger dart pub publish leak detection:

1. false_secrets in plugins/http2_adapter/pubspec.yaml — tells pub not to flag the key as a leaked credential.
2. test/certificates/.pubignore — prevents both server.key and server.crt from being published at all.

Why both?

• Without either, dart pub publish --dry-run reports Potential leak of Private Key detected and would block release.
• false_secrets alone already suppresses the warning (and in the current pub implementation also excludes the file from the archive), but .pubignore makes the exclusion explicit and robust against future pub behavior changes.

Why .pubignore in the subdirectory, not the package root?

A root-level .pubignore overrides the package-root .gitignore entirely for that directory — so any existing gitignore rules (build artifacts, etc.) would stop being applied at publish time. Placing .pubignore inside test/certificates/ scopes the override to just that directory, leaving all other gitignore rules intact. (Verified empirically.)

Verification

dart pub publish --dry-run
# ✓ server.key / server.crt not in the file list
# ✓ "Potential leak" warning gone
# ✓ all lib/ and test/ source files still present

The two strict-ALPN regression tests still pass (dart test test/http2_test.dart).

---

Follow-up commit and this note by AlexV525, with verification and drafting assistance from GLM-5.2 via omp.

[github-actions]

Code Coverage Report: Only Changed Files listed

Package	Base Coverage	New Coverage	Difference
plugins/http2_adapter/lib/src/connection_manager_imp.dart	🟢 87.14%	🟢 86.11%	🔴 -1.03%
Overall Coverage	🟢 86.19%	🟢 86.12%	🔴 -0.07%

Minimum allowed coverage is 0%, this run produced 86.12%

[chrisslazzz]

@AlexV525 Thanks for the detailed review — addressed items 1–6 in 63c9571.

Items 1 & 2 (readiness + teardown): Replaced the fixed 300 ms delay with a TCP port-poll loop (40 × 50 ms). process.kill() is now registered via addTearDown immediately after Process.start, so the child is always cleaned up even on cancellation. The poll also detects early openssl exit (wrong CWD, missing certs) and calls markTestSkipped rather than hanging.

On the TOCTOU race: I kept the port-probe approach (ServerSocket.bind(0) → close → pass port to openssl). The alternative — dropping -quiet to parse the ACCEPT 127.0.0.1:PORT line from stdout — avoids the race but introduces a different problem: without -quiet, openssl's TLS session info is printed to stdout before the HTTP request bytes arrive, so "respond on first data" would fire on session output rather than the actual request. We'd then need to detect where session info ends and the HTTP request begins, which means relying on undocumented output formatting. On loopback the race window is microseconds and no other process targets specific high-numbered ephemeral ports — happy to revisit if you disagree with this or you have a cleaner idea.

Items 3 & 5 (re-entry guard + early exit): Added responded flag against fragmented requests; early-exit detection as described above.

Items 4 & 6 (dartdoc): Completed the cut-off sentence; added a note that supportedProtocols must contain 'h2' for HTTP/2 to be negotiated.

Also extracted _serveHttp11Response and _pollUntilListening as named helpers to improve readability of _bindHttp11OnlyServer.

[AlexV525]

@chrisslazzz Tests start failing after the recent change.

[chrisslazzz]

@AlexV525 Haven't figured out how the failures are connected to my changes & how to run some of these tests locally. Hopefully, I'll find some more time by the end of the week..

But could this also have been a temporary issue with badssl.com?