🐛 Fix validateCertificate to fire pre-emission for TLS pinning#2512
🐛 Fix validateCertificate to fire pre-emission for TLS pinning#2512realmeylisdev wants to merge 3 commits into
validateCertificate to fire pre-emission for TLS pinning#2512Conversation
`IOHttpClientAdapter.validateCertificate` previously ran after `HttpClientRequest.close()` had already flushed the request method, headers, and body to the wire. The callback could prevent the response from being delivered to the caller, but it could not prevent leakage of outbound request data — defeating the canonical purpose of certificate or public-key pinning. Install an `HttpClient.connectionFactory` (when `validateCertificate` is set and `createHttpClient` is not) that performs the TLS handshake, runs `validateCertificate`, and only then yields the socket to `HttpClient`. Rejection throws a private `_BadCertificateException` (a `HandshakeException` subtype) that `_fetch` translates into `DioException.badCertificate` — without a single byte of the request ever leaving the client. The legacy post-response validation block is preserved as a fallback for the `createHttpClient` escape hatch, so users who supply a custom `HttpClient` keep today's degraded-but-not-broken semantics. Proxy support is included via an HTTP/1.1 CONNECT-tunnel branch, mirroring the pattern in `dio_http2_adapter`. Behavior change: validation now runs once per TCP connection rather than once per HTTP request, matching `dio_http2_adapter`'s existing semantics. The existing `'2 requests == 2 approvals'` test is updated accordingly. A new load-bearing regression test asserts that the server receives zero bytes when `validateCertificate` returns false. Min Dart SDK is raised to 3.5.0 for `ConnectionTask.fromSocket`. The existing TODO migrating `DioMixin` to `mixin class` syntax is resolved. Fixes cfug#2418
…README Three issues surfaced during self-review of cfug#2512: H2: late mutation of `validateCertificate` silently no-oped. The gate on the legacy post-response block was `createHttpClient != null`, so a user who set `validateCertificate` *after* the `HttpClient` was first cached saw the callback never fire. Drop the gate so the post-response block runs whenever `validateCertificate` is non-null — redundant on the direct-HTTPS pre-emission path (the same cert was already approved by the connectionFactory hook, idempotent for pinning), but the only defense for late-mutation, proxy, and `createHttpClient` paths. H1+M1: the manual `CONNECT`-tunnel inside `_connectHttpsViaProxy` was broken — `HttpClient` writes its own `CONNECT` over whatever socket the factory returns, so we ended up issuing two `CONNECT` requests and the TLS handshake corrupted. (Discovered by the new proxy test failing.) Drop the manual tunnel entirely; the factory's proxy branch now returns a plain `Socket.startConnect(proxyHost, proxyPort)` and `HttpClient` handles `CONNECT` + TLS itself. Proxy + pinning therefore falls back to post-response validation only — documented as a known limitation; for pre-emission behind a proxy, users must supply `createHttpClient` and configure `HttpClient.connectionFactory` themselves. Add two tests that exercise the proxy code path through a local stub `CONNECT` proxy fixture. Add `dio/test/_pinning/README.md` documenting the self-signed cert regeneration command, since the committed pair has a 10-year validity and a future maintainer will need to rotate it. Also drop the redundant `as Socket` cast on the direct HTTPS branch and consolidate the dangling `## Unreleased *None.*` CHANGELOG header with the 5.10.0 entry — entries now live under `Unreleased` until the release script cuts them.
There was a problem hiding this comment.
Please revert since we don't match the requirement here.
There was a problem hiding this comment.
Reverted in 8a66e1e — back to abstract class DioMixin implements Dio with the original TODO(EVERYONE): Use \mixin class` when the lower bound of SDK is raised to 3.0.0.` comment.
There was a problem hiding this comment.
The fix is intentionally scoped to IOHttpClientAdapter since that's the adapter with the bug. Quick rundown of the others:
plugins/http2_adapter— already does pre-emission validation correctly atconnection_manager_imp.dart:133-150. This PR bringsIOHttpClientAdapterto parity.plugins/web_adapter— novalidateCertificatefield exists on the web adapter; TLS is browser-controlled and not introspectable from Dart. Not applicable.plugins/native_dio_adapter— delegates tocronet_http/cupertino_http; TLS pinning is platform-native (Android NSC, iOS ATS/TrustKit), not a dio-level callback. Not applicable.
Happy to add this as a one-line cross-reference comment in io_adapter.dart if you'd like that captured in source.
There was a problem hiding this comment.
Reverted in 8a66e1e — back to sdk: '>=2.18.0 <4.0.0'. Same for dio_test/pubspec.yaml. The pre-emission factory no longer needs Dart 3.5+.
| } | ||
| return ss; | ||
| }(); | ||
| return ConnectionTask.fromSocket(socketFuture, () {}); |
There was a problem hiding this comment.
The implementation of ConnectionTask is simple, try to copy it into the code base to avoid bumping the SDK version too high.
There was a problem hiding this comment.
Addressed differently in 8a66e1e — literal vendoring is blocked because Dart 3.0 made ConnectionTask a final class (any vendored class _ConnectionTask implements ConnectionTask fails to compile from 3.0 onward), and ConnectionTask.fromSocket is Dart 3.5+ only.
Switched to SecureSocket.startConnect(...) (stable pre-2.18 API) which returns a real SDK ConnectionTask<SecureSocket>. We await task.socket once to validate the leaf cert, then return task; HttpClient re-awaits the same Future and gets the resolved socket immediately. Covariance handles the upcast to ConnectionTask<Socket>. No vendoring, no SDK floor change — same observable behavior.
|
Pushed review fixes (23aa0c0):
PR description updated to reflect the corrected scope (proxy = known limitation, post-response only). |
Per @AlexV525: 1. Revert dio/pubspec.yaml + dio_test/pubspec.yaml min SDK back to `>=2.18.0 <4.0.0` (the bump to 3.5.0 is no longer needed). 2. Revert dio/lib/src/dio_mixin.dart to `abstract class DioMixin` with the original TODO comment. 3. Replace the Dart-3.5+ `ConnectionTask.fromSocket(...)` call in `_pinnedConnectionFactory` with `SecureSocket.startConnect(...)` and return the stock SDK [ConnectionTask] it produces. We await `task.socket` once to do the leaf-cert validation; HttpClient re-awaits the same `Future<SecureSocket>` and gets the resolved value immediately (Future await is idempotent). This works on every Dart >=2.18.0 because `SecureSocket.startConnect` has been in `dart:io` well before the minimum SDK, and the `ConnectionTask` generic is covariant in its type parameter so `ConnectionTask<SecureSocket>` is implicitly assignable to `ConnectionTask<Socket>` at the factory's return position. No vendoring or `implements ConnectionTask` is required — and `ConnectionTask` being `final class` from Dart 3.0 onward is no longer a problem since we never inherit from it. The maintainer's suggestion to literally "copy ConnectionTask into the code base" cannot be done across the Dart 3.0–3.4 window (final class blocks implements; fromSocket didn't exist yet), but `SecureSocket.startConnect` achieves the same end with zero new types and zero SDK floor change. 4. Drop the two CHANGELOG bullets that referenced the SDK bump and the mixin migration. The rest of the entry (the actual cfug#2418 fix) stays. No test changes — the factory's observable behavior is unchanged (approval yields a validated SecureSocket; rejection still throws `_BadCertificateException` from the same point in `_fetch`'s try block). All 222 tests pass; `dart analyze lib/` is clean.
|
Still not covering previous comments |
|
@AlexV525 thanks for the review. Pushed 8a66e1e addressing all four points: 1. 2. 3.
The fix is to use Diff for the affected branch is small:
4. "What about other adapters?" ✅ The fix is intentionally scoped to
Happy to add a one-line cross-reference comment in All 222 tests pass; |
|
Tests didn't seem passing. |
Fixes #2418.
Summary
IOHttpClientAdapter.validateCertificateruns only afterHttpClientRequest.close()has flushed the request method, headers, and body to the socket. The callback can prevent the response from being delivered to the caller, but it cannot prevent leakage of outbound request data — defeating the canonical purpose of certificate or public-key pinning.This PR makes
validateCertificatefire between the TLS handshake and the first HTTP byte for direct HTTPS connections by installing a customHttpClient.connectionFactory(Dart 3.5+ConnectionTask.fromSocket). When the callback returnsfalse, the request body never leaves the client.Reproducing the bug (on
maintoday)The bug is structural — no exploit setup needed.
_fetchindio/lib/src/adapters/io_adapter.dartonmain:Demonstrable end-to-end:
HttpServer.bindSecure('localhost', 0, ctx)with a self-signed cert. Record bytes received per request.await dio.post('https://localhost:\$port/leak', data: 'SENSITIVE');main: server recordsbytesReceived > 0(the payload reached it).bytesReceived == 0, the request handler is never invoked, and the caller getsDioException.badCertificatefrom insideopenUrl— before any byte is written.This is encoded as the load-bearing regression test in
dio/test/pinning_test.dart:Approach
Install
HttpClient.connectionFactoryin_createHttpClientwhenevervalidateCertificate != nullandcreateHttpClientis not supplied. The factory:url.scheme != 'https') — short-circuits toSocket.startConnect. Pre-emission validation is skipped (no TLS handshake to gate); the post-response block still fires with a null cert.SecureSocket.connect(...)withsupportedProtocols: ['http/1.1'], runs the user's validator, returnsConnectionTask.fromSocket(...). On rejection, throws a private_BadCertificateException(aHandshakeExceptionsubtype carrying the cert) —_fetchcatches it and converts toDioException.badCertificate(error: cert)so callers see the same exception type as before.HttpClientperform its ownCONNECTtunnel and TLS handshake (we cannot pre-emption-validate proxy connections becauseHttpClientwrites its ownCONNECTover whatever socket we return). Validation runs only via the post-response block in this case — documented as a known limitation.The post-response validation block in
_fetchis retained and runs unconditionally whenvalidateCertificate != null, providing defense in depth:createHttpClient, late-mutation ofvalidateCertificateafter the cached client was built, and HTTPS through a proxy.On the pre-emission path,
validateCertificateis the sole gate for certificate trust — system / CA validation is bypassed (equivalent toHttpClient.badCertificateCallback: (_, _, _) => truein the existing example). This lets self-signed and pinned-CA setups work without supplyingcreateHttpClient. Documented on the doc-comment.Behavioral changes
dio_http2_adapter); the post-response path is per request.CONNECT+ TLS path).ConnectionTask.fromSocket.dio_test's SDK constraint bumped to match.abstract class DioMixinmigrated toabstract mixin class DioMixin, resolving the pre-existing TODO indio/lib/src/dio_mixin.dart.example_dart/lib/certificate_pinning.dartis simplified — theSecurityContext(withTrustedRoots: false)+badCertificateCallback: (_, _, _) => trueworkaround is no longer needed.DioException.badCertificatestack-trace test indio/test/stacktrace_test.dartis updated to use thecreateHttpClientescape hatch, since the new factory path can't be exercised against the existingMockHttpClient(no stub forconnectionFactory=).dio/test/_pinning/contains a committed self-signed keypair for the localHttpServer.bindSecuretest; a README documents the regeneration command.Edge cases acknowledged
createHttpClientsuppliedvalidateCertificate(after first request)HttpClientdoesCONNECT+ TLS itself. Pre-emission is bypassed; post-response block validates. Documented.Proxy-Authorization)HttpClient.findProxy/HttpClient.addProxyCredentialscontinue to work because we delegateCONNECTtoHttpClient.SecurityContextwith mTLS +validateCertificatewithoutcreateHttpClientcreateHttpClientfor mTLS + pinning.http://; short-circuits. Same as today.validateCertificate(null, host, port)invoked. Same as 5.9.x.New Pull Request Checklist
Test plan
Known limitations / follow-ups