Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions lib/core/utils/plaintext_destination_guard.dart
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ class PlaintextDestinationGuard {
/// promise is about the app's traffic to a user-supplied address and not
/// about one call site. Anything given this client is covered, including
/// call sites that do not exist yet.
///
/// **Redirects are not followed.** They would be followed below this layer,
/// where the guard never sees them, so a 30x comes back to the caller
/// unfollowed rather than becoming an unchecked second destination.
class GuardedPlaintextClient extends http.BaseClient {
final http.Client _inner;
final PlaintextDestinationGuard _guard;
Expand All @@ -148,8 +152,22 @@ class GuardedPlaintextClient extends http.BaseClient {
@override
Future<http.StreamedResponse> send(http.BaseRequest request) async {
final approved = await _guard.approve(request.url);
if (approved == request.url) return _inner.send(request);
return _inner.send(_reboundTo(approved, request));
final outgoing = approved == request.url
? request
: _reboundTo(approved, request);

Comment on lines 153 to +158
// Redirects are followed inside `dart:io`, below `BaseClient.send`, so a
// hop never comes back through here and never meets [approve]. Left on,
// a server answering 30x with a public `http://` target would have that
// connection made — from a check that reported the destination private.
// The guard cannot vouch for a hop it never sees, so it does not let one
// happen: a redirect is returned to the caller as the 30x it is.
//
// This holds for `https://` too, which [approve] waves through: an
// encrypted first hop says nothing about where a `Location` points.
if (outgoing.followRedirects) outgoing.followRedirects = false;

return _inner.send(outgoing);
}

/// Rebuilds the request against the approved address, keeping the original
Expand All @@ -166,7 +184,6 @@ class GuardedPlaintextClient extends http.BaseClient {
..headers.addAll(request.headers)
..headers['host'] = request.url.host
..bodyBytes = request.bodyBytes
..followRedirects = request.followRedirects
..maxRedirects = request.maxRedirects
..persistentConnection = request.persistentConnection;
}
Expand Down
81 changes: 80 additions & 1 deletion test/unit_test/plaintext_destination_guard_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,25 @@ import 'package:http/http.dart' as http;
import 'package:opennutritracker/core/utils/plaintext_destination_guard.dart';

/// Records the request that reached the socket, or the fact that none did.
///
/// [sentCount] matters for the redirect tests: following a 30x would show up
/// here as a second send, which is exactly what must not happen.
class _RecordingClient extends http.BaseClient {
http.BaseRequest? sent;
int sentCount = 0;

/// Returned instead of a bare 200 when a test needs a specific status —
/// a 30x, for the redirect cases.
final http.StreamedResponse? response;

_RecordingClient({this.response});

@override
Future<http.StreamedResponse> send(http.BaseRequest request) async {
sent = request;
return http.StreamedResponse(const Stream.empty(), 200, request: request);
sentCount++;
return response ??
http.StreamedResponse(const Stream.empty(), 200, request: request);
}
}

Expand Down Expand Up @@ -218,6 +230,73 @@ void main() {
expect(inner.sent, isNull);
});

test('redirects are not followed, on the rebound path', () async {
// The gap this closes: `dart:io` follows a 30x below `BaseClient.send`,
// so the hop never re-enters the guard. A private server answering with
// a public `http://` Location would have had that connection made, out
// of a check that reported the destination private.
final inner = _RecordingClient();
final client = GuardedPlaintextClient(
inner,
guard: PlaintextDestinationGuard(
lookup: (_) async => [_v4('192.168.1.5')],
),
);

await client.post(
Uri.parse('http://ollama.lan:11434/v1/chat/completions'),
body: 'payload',
);

expect(inner.sent!.followRedirects, isFalse);
});

test('redirects are not followed on the https pass-through either', () async {
// `approve` waves https straight through, so the pass-through branch
// needs its own guarantee: an encrypted first hop says nothing about
// where a `Location` header points.
final inner = _RecordingClient();
final client = GuardedPlaintextClient(inner);

await client.post(
Uri.parse('https://ollama.example.com/v1/chat/completions'),
body: 'payload',
);

expect(inner.sent!.followRedirects, isFalse);
});

test('a 30x comes back to the caller as a response, not a second hop', () async {
// Contract, not regression: the following happens inside `dart:io`,
// below this fake, so this test passes with or without the fix above.
// It pins what a caller sees — a 30x surfacing rather than being
// swallowed — while the two tests above are the ones that fail if
// `followRedirects` is ever turned back on.
final inner = _RecordingClient(
response: http.StreamedResponse(
const Stream.empty(),
302,
headers: {'location': 'http://93.184.216.34/v1/chat/completions'},
),
);
final client = GuardedPlaintextClient(
inner,
guard: PlaintextDestinationGuard(
lookup: (_) async => [_v4('192.168.1.5')],
),
);

final response = await client.post(
Uri.parse('http://ollama.lan:11434/v1/chat/completions'),
body: 'payload',
);

// Surfaced rather than chased. The caller sees a failed request; what
// it does not see is a payload delivered to 93.184.216.34.
expect(response.statusCode, 302);
expect(inner.sentCount, 1);
});

test('the exception names the host and nothing else', () async {
// Raised on a request that may be carrying a photograph of somebody's
// dinner. The path and body must not reach a log through it.
Expand Down
Loading