Skip to content

Commit 23aa0c0

Browse files
realmeylisdevrealmeylisdev
committed
🐛 Address PR cfug#2512 review: late mutation, proxy honesty, fixture 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.
1 parent 1ce3ee2 commit 23aa0c0

4 files changed

Lines changed: 303 additions & 143 deletions

File tree

‎dio/CHANGELOG.md‎

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,24 +5,23 @@ See the [Migration Guide][] for the complete breaking changes list.**
55

66
## Unreleased
77

8-
*None.*
9-
10-
## 5.10.0
11-
128
- **Security:** `IOHttpClientAdapter.validateCertificate` now fires between
13-
the TLS handshake and the first HTTP byte instead of after the response
14-
head, making it suitable for true certificate or public-key pinning
15-
(issue #2418). Previously, an attacker presenting a publicly trusted
16-
certificate for the wrong host could receive the full request body and
17-
headers before the callback aborted.
18-
- Behavioral change: validation now runs once per TCP connection rather
19-
than once per request, matching `dio_http2_adapter`'s long-standing
20-
semantics. Tests asserting "N approvals for N requests" should be
21-
updated.
22-
- The pre-emission path is active only when `createHttpClient` is not
23-
supplied. With a custom `createHttpClient`, the callback continues to
24-
run post-response (the previous 5.x behavior). Documented on
25-
`validateCertificate` and `createHttpClient`.
9+
the TLS handshake and the first HTTP byte for direct HTTPS connections
10+
(no proxy, no custom `createHttpClient`), making it suitable for true
11+
certificate or public-key pinning (issue #2418). Previously, an
12+
attacker presenting a publicly trusted certificate for the wrong host
13+
could receive the full request body and headers before the callback
14+
aborted.
15+
- The callback also continues to run **after the response head arrives**
16+
as defense in depth — both invocations receive the same leaf
17+
certificate, so for fingerprint-style pinning the second call is
18+
idempotent. Callbacks with side effects (logging, metrics) will
19+
observe one extra call per request on the direct-HTTPS path.
20+
- HTTPS routed through a proxy continues to use the post-response
21+
validation path only (HttpClient performs its own `CONNECT` tunnel and
22+
TLS handshake, bypassing the pre-emission hook). For pre-emission
23+
pinning behind a proxy, supply `createHttpClient` and configure
24+
`HttpClient.connectionFactory` yourself.
2625
- On the pre-emission path, `validateCertificate` is the sole gate for
2726
certificate trust — system / CA validation is bypassed (matching what
2827
users already configure via `badCertificateCallback: (_, _, _) => true`

‎dio/lib/src/adapters/io_adapter.dart‎

Lines changed: 65 additions & 124 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import 'dart:async';
2-
import 'dart:convert';
32
import 'dart:io';
43
import 'dart:typed_data';
54

@@ -77,34 +76,45 @@ class IOHttpClientAdapter implements HttpClientAdapter {
7776
/// Allows the user to decide if the leaf certificate of the TLS connection
7877
/// is good. If this function is missing, then the certificate is allowed.
7978
///
80-
/// When [createHttpClient] is **not** set, this callback fires **before**
81-
/// the request body is sent, immediately after the TLS handshake completes.
82-
/// Returning `false` aborts the connection without leaking any request
83-
/// data and surfaces as [DioException.badCertificate]. This makes the
84-
/// callback suitable for certificate or public-key pinning.
79+
/// For **direct HTTPS** connections (no proxy) with the default
80+
/// [createHttpClient], this callback fires **before** the request body
81+
/// is sent, immediately after the TLS handshake completes. Returning
82+
/// `false` aborts the connection without leaking any request data and
83+
/// surfaces as [DioException.badCertificate]. This makes the callback
84+
/// suitable for certificate or public-key pinning.
8585
///
86-
/// When [createHttpClient] **is** supplied, dio cannot install the
87-
/// pre-emission hook on the user-built [HttpClient]; the callback then
88-
/// runs after the response head arrives (the legacy 5.x behavior). For
89-
/// pre-emission validation in that case, set `connectionFactory` on the
90-
/// [HttpClient] you return.
86+
/// The callback also runs **after the response head arrives** for the
87+
/// same connection. The two invocations receive the same leaf
88+
/// certificate, so for fingerprint-style pinning the second call is
89+
/// idempotent. If your callback has side effects (logging, metrics),
90+
/// expect to see them twice per request on the direct-HTTPS path.
9191
///
92-
/// Validation runs once per TCP connection, not per request. Connections
93-
/// are pooled by [HttpClient], so multiple requests to the same host
94-
/// within `idleTimeout` will produce one approval call.
92+
/// **HTTPS through a proxy:** `HttpClient` performs its own `CONNECT`
93+
/// tunnel and TLS handshake, so the pre-emission hook is bypassed.
94+
/// Validation runs only post-response on this path — the request body
95+
/// has already been transmitted by the time the callback is invoked.
96+
/// For pre-emission pinning behind a proxy, use [createHttpClient] and
97+
/// install your own `connectionFactory` on the returned client.
98+
///
99+
/// **Custom [createHttpClient]:** dio cannot install the pre-emission
100+
/// hook on a user-built [HttpClient]; the callback runs post-response
101+
/// (the legacy 5.x behavior). For pre-emission validation in that case,
102+
/// set `connectionFactory` on the [HttpClient] you return.
95103
///
96104
/// On the pre-emission path, this callback is the **sole** gate for
97105
/// certificate trust: system / CA validation is bypassed (the equivalent
98106
/// of `HttpClient.badCertificateCallback: (_, _, _) => true` in the
99107
/// existing example), so the callback receives every leaf cert and is
100108
/// solely responsible for accepting or rejecting it. This matches what
101109
/// users already configure manually for pinning and lets self-signed and
102-
/// pinned-CA setups work without supplying [createHttpClient].
110+
/// pinned-CA setups work without supplying [createHttpClient]. If you
111+
/// do not intend to perform pinning, do not set this callback —
112+
/// supplying any non-null value disables stdlib chain validation.
103113
///
104114
/// Any [SecurityContext] you set on a custom [HttpClient] is **not**
105115
/// used by the pre-emission path. To combine mTLS or a custom trust store
106-
/// with pinning, supply a [createHttpClient] and pin in the legacy
107-
/// post-response path.
116+
/// with pinning, supply a [createHttpClient] and pin in the post-response
117+
/// path.
108118
ValidateCertificate? validateCertificate;
109119

110120
HttpClient? _cachedHttpClient;
@@ -232,12 +242,19 @@ class IOHttpClientAdapter implements HttpClientAdapter {
232242
}
233243
final responseStream = await future;
234244

235-
if (validateCertificate != null && createHttpClient != null) {
236-
// Legacy post-response validation: only runs when the user supplied
237-
// a custom [createHttpClient]. In that case we cannot install the
238-
// pre-emission [HttpClient.connectionFactory] without clobbering the
239-
// user's client, so the callback runs after the response head has
240-
// arrived. See the doc-comment on [validateCertificate].
245+
if (validateCertificate != null) {
246+
// Post-response validation runs unconditionally as defense in depth.
247+
// On the pre-emission path it is redundant — the same cert was
248+
// already approved by the [HttpClient.connectionFactory] hook — but
249+
// it is the only line of defense for paths where pre-emission did
250+
// not run: (a) when the user supplied a custom [createHttpClient],
251+
// (b) when [validateCertificate] was set after the [HttpClient] was
252+
// first cached, or (c) for HTTPS through a proxy (see the doc-comment
253+
// on [validateCertificate]). For pinning the callback is idempotent
254+
// so the redundancy is harmless; callbacks with side effects will
255+
// observe one extra call per request on the direct-HTTPS path.
256+
// On plain HTTP, [responseStream.certificate] is null and the user
257+
// typically rejects nulls (matching the example).
241258
final host = options.uri.host;
242259
final port = options.uri.port;
243260
final bool isCertApproved = validateCertificate!(
@@ -314,132 +331,56 @@ class IOHttpClientAdapter implements HttpClientAdapter {
314331
return onHttpClientCreate?.call(client) ?? client;
315332
}
316333

317-
/// Custom connection factory that performs TLS handshake and runs
334+
/// Custom connection factory that performs the TLS handshake and runs
318335
/// [validateCertificate] *before* yielding the socket to [HttpClient],
319336
/// so a rejected certificate cannot leak request bytes.
337+
///
338+
/// For HTTPS requests routed through a proxy, this factory cannot
339+
/// pre-emption-validate: HttpClient writes its own `CONNECT` and performs
340+
/// its own TLS handshake on top of the socket we return. In that case the
341+
/// proxy branch returns a plain socket to the proxy and validation falls
342+
/// back to the post-response check in [_fetch].
320343
Future<ConnectionTask<Socket>> _pinnedConnectionFactory(
321344
Uri url,
322345
String? proxyHost,
323346
int? proxyPort,
324347
) async {
325-
// Plain HTTP — no TLS handshake to gate.
348+
// Proxy: hand HttpClient a plain socket to the proxy and let it own the
349+
// CONNECT-tunnel and TLS upgrade. The post-response block in [_fetch]
350+
// performs validation in this path.
351+
if (proxyHost != null) {
352+
return Socket.startConnect(proxyHost, proxyPort!);
353+
}
354+
355+
// Plain HTTP, no proxy: hand HttpClient the target socket.
326356
if (url.scheme != 'https') {
327-
if (proxyHost != null) {
328-
return Socket.startConnect(proxyHost, proxyPort!);
329-
}
330357
return Socket.startConnect(url.host, url.port);
331358
}
332359

333360
// Snapshot at call time so a runtime mutation of the public field
334361
// is honored on every new connection.
335362
final validator = validateCertificate;
336363

337-
if (proxyHost != null) {
338-
return _connectHttpsViaProxy(url, proxyHost, proxyPort!, validator);
339-
}
340-
341364
final socketFuture = () async {
342365
final ss = await SecureSocket.connect(
343366
url.host,
344367
url.port,
345368
supportedProtocols: const ['http/1.1'],
346-
// When [validateCertificate] is set, defer all certificate validation
347-
// to the user's callback. This makes self-signed and pinned-CA setups
348-
// work without forcing the user to also supply [createHttpClient].
369+
// When [validateCertificate] is set, defer all certificate
370+
// validation to the user's callback. This makes self-signed and
371+
// pinned-CA setups work without forcing the user to also supply
372+
// [createHttpClient].
349373
onBadCertificate: validator == null ? null : (_) => true,
350374
);
351375
if (validator != null) {
352-
_validatePeerCertificate(ss, url.host, url.port, validator);
376+
final cert = ss.peerCertificate;
377+
if (!validator(cert, url.host, url.port)) {
378+
ss.destroy();
379+
throw _BadCertificateException(url.host, url.port, cert);
380+
}
353381
}
354-
return ss as Socket;
382+
return ss;
355383
}();
356384
return ConnectionTask.fromSocket(socketFuture, () {});
357385
}
358-
359-
Future<ConnectionTask<Socket>> _connectHttpsViaProxy(
360-
Uri target,
361-
String proxyHost,
362-
int proxyPort,
363-
ValidateCertificate? validator,
364-
) async {
365-
Socket? proxySocket;
366-
bool cancelled = false;
367-
368-
final socketFuture = () async {
369-
proxySocket = await Socket.connect(proxyHost, proxyPort);
370-
if (cancelled) {
371-
proxySocket!.destroy();
372-
throw const SocketException('connection cancelled');
373-
}
374-
375-
// HTTP/1.1 CONNECT tunnel preamble.
376-
const crlf = '\r\n';
377-
final preamble = StringBuffer()
378-
..write('CONNECT ${target.host}:${target.port} HTTP/1.1$crlf')
379-
..write('Host: ${target.host}:${target.port}$crlf')
380-
..write(crlf);
381-
proxySocket!.write(preamble.toString());
382-
383-
final completer = Completer<void>();
384-
late StreamSubscription<List<int>> subscription;
385-
subscription = proxySocket!.listen(
386-
(event) {
387-
if (completer.isCompleted) {
388-
return;
389-
}
390-
final response = ascii.decode(event);
391-
final statusLine = response.split(crlf).first;
392-
if (statusLine.contains(' 200 ')) {
393-
completer.complete();
394-
} else {
395-
completer.completeError(
396-
SocketException(
397-
'Proxy CONNECT failed: $statusLine '
398-
'(host=${target.host}, port=${target.port})',
399-
),
400-
);
401-
}
402-
},
403-
onError: (Object e, StackTrace s) {
404-
if (!completer.isCompleted) {
405-
completer.completeError(e, s);
406-
}
407-
},
408-
);
409-
try {
410-
await completer.future;
411-
} finally {
412-
await subscription.cancel();
413-
}
414-
415-
final ss = await SecureSocket.secure(
416-
proxySocket!,
417-
host: target.host,
418-
supportedProtocols: const ['http/1.1'],
419-
onBadCertificate: validator == null ? null : (_) => true,
420-
);
421-
if (validator != null) {
422-
_validatePeerCertificate(ss, target.host, target.port, validator);
423-
}
424-
return ss as Socket;
425-
}();
426-
427-
return ConnectionTask.fromSocket(socketFuture, () {
428-
cancelled = true;
429-
proxySocket?.destroy();
430-
});
431-
}
432-
433-
void _validatePeerCertificate(
434-
SecureSocket ss,
435-
String host,
436-
int port,
437-
ValidateCertificate validator,
438-
) {
439-
final cert = ss.peerCertificate;
440-
if (!validator(cert, host, port)) {
441-
ss.destroy();
442-
throw _BadCertificateException(host, port, cert);
443-
}
444-
}
445386
}

‎dio/test/_pinning/README.md‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
# Test fixtures: pinning
2+
3+
`server_cert.pem` and `server_key.pem` are a self-signed TLS keypair used by
4+
`pinning_test.dart`'s pre-emission validation tests to spin up local
5+
`HttpServer.bindSecure` instances on `localhost`.
6+
7+
These files are committed to keep the test suite hermetic (no network
8+
dependency for the load-bearing regression test that asserts request bytes
9+
do not leak when `validateCertificate` rejects).
10+
11+
The current pair has 10-year validity. To regenerate when it expires:
12+
13+
```sh
14+
openssl req -x509 -newkey rsa:2048 \
15+
-keyout server_key.pem -out server_cert.pem \
16+
-days 3650 -nodes \
17+
-subj "/CN=localhost" \
18+
-addext "subjectAltName=DNS:localhost,IP:127.0.0.1"
19+
```
20+
21+
The private key is for localhost-only testing and has no production value.

0 commit comments

Comments
 (0)