Fixes #87
---
 lib/client.dart            | 18 +++++++++++++
 lib/client_controller.dart |  3 ++-
 lib/database.dart          | 10 +++++--
 lib/main.dart              |  2 +-
 lib/models.dart            |  7 +++--
 lib/page/connect.dart      | 54 ++++++++++++++++++++++++++++++++++++--
 lib/prefs.dart             |  6 +++++
 lib/push.dart              |  3 ++-
 pubspec.lock               |  8 ++++++
 pubspec.yaml               |  1 +
 10 files changed, 103 insertions(+), 9 deletions(-)

diff --git a/lib/client.dart b/lib/client.dart
index 6dd8733..97f6590 100644
--- a/lib/client.dart
+++ b/lib/client.dart
@@ -4,6 +4,7 @@ import 'dart:convert';
 import 'dart:io';
 
 import 'package:flutter/foundation.dart';
+import 'package:hex/hex.dart';
 
 import 'irc.dart';
 import 'logging.dart';
@@ -25,6 +26,7 @@ class ConnectParams {
 	final SaslPlainCredentials? saslPlain;
 	final String? bouncerNetId;
 	final String? away;
+	final String? pinnedCertSHA1;
 
 	const ConnectParams({
 		required this.host,
@@ -36,6 +38,7 @@ class ConnectParams {
 		this.saslPlain,
 		this.bouncerNetId,
 		this.away,
+		this.pinnedCertSHA1,
 	}) : realname = realname ?? nick;
 
 	ConnectParams apply({
@@ -55,10 +58,16 @@ class ConnectParams {
 			saslPlain: saslPlain ?? this.saslPlain,
 			bouncerNetId: bouncerNetId ?? this.bouncerNetId,
 			away: away ?? this.away,
+			pinnedCertSHA1: this.pinnedCertSHA1,
 		);
 	}
 }
 
+class BadCertException implements Exception {
+	final X509Certificate badCert;
+	BadCertException(this.badCert);
+}
+
 Set<String> _getDefaultCaps(ConnectParams params) {
 	var caps = {
 		'away-notify',
@@ -107,6 +116,7 @@ class Client {
 	Socket? _socket;
 	String _nick;
 	String _realname;
+	String? _pinnedCertSHA1;
 	IrcSource? _serverSource;
 	ClientState _state = ClientState.disconnected;
 	bool _registered = false;
@@ -128,6 +138,7 @@ class Client {
 	String get nick => _nick;
 	String get realname => _realname;
 	IrcSource? get serverSource => _serverSource;
+	String? get pinnedCertSHA1 => _pinnedCertSHA1;
 	ClientState get state => _state;
 	bool get registered => _registered;
 	Stream<ClientMessage> get messages => _messagesController.stream;
@@ -145,6 +156,7 @@ class Client {
 		_requestCaps = requestCaps ?? _getDefaultCaps(params),
 		_nick = params.nick,
 		_realname = params.realname,
+		_pinnedCertSHA1 = params.pinnedCertSHA1,
 		_autoReconnect = autoReconnect,
 		isupport = isupport ?? IrcIsupportRegistry();
 
@@ -174,6 +186,12 @@ class Client {
 			connectionTaskFuture = SecureSocket.startConnect(
 				params.host,
 				params.port,
+				onBadCertificate: (X509Certificate cert) {
+					if (params?.pinnedCertSHA1 == HEX.encode(cert.sha1)) {
+						return true;
+					}
+					throw BadCertException(cert);
+				},
 				supportedProtocols: ['irc'],
 			);
 		} else {
diff --git a/lib/client_controller.dart b/lib/client_controller.dart
index e82ae54..18d1557 100644
--- a/lib/client_controller.dart
+++ b/lib/client_controller.dart
@@ -33,6 +33,7 @@ ConnectParams connectParamsFromServerEntry(ServerEntry entry, Prefs prefs) {
 		realname: prefs.realname,
 		pass: entry.pass,
 		saslPlain: saslPlain,
+		pinnedCertSHA1: entry.pinnedCertSHA1
 	);
 }
 
@@ -921,7 +922,7 @@ class ClientController {
 		);
 		networkEntry = await _db.storeNetwork(networkEntry);
 		var childClient = Client(client.params.apply(bouncerNetId: bouncerNetId));
-		childNetwork = NetworkModel(network.serverEntry, networkEntry, childClient.nick, childClient.realname);
+		childNetwork = NetworkModel(network.serverEntry, networkEntry, childClient.nick, childClient.realname, childClient.pinnedCertSHA1);
 		_networkList.add(childNetwork);
 		_provider.add(childClient, childNetwork);
 		childClient.connect().ignore();
diff --git a/lib/database.dart b/lib/database.dart
index dc2b7c9..2722dc1 100644
--- a/lib/database.dart
+++ b/lib/database.dart
@@ -19,6 +19,7 @@ class ServerEntry {
 	String? pass;
 	String? saslPlainUsername;
 	String? saslPlainPassword;
+	String? pinnedCertSHA1;
 
 	Map<String, Object?> toMap() {
 		return <String, Object?>{
@@ -30,6 +31,7 @@ class ServerEntry {
 			'pass': pass,
 			'sasl_plain_username': saslPlainUsername,
 			'sasl_plain_password': saslPlainPassword,
+			'pinned_cert_sha1': pinnedCertSHA1,
 		};
 	}
 
@@ -41,6 +43,7 @@ class ServerEntry {
 		this.pass,
 		this.saslPlainUsername,
 		this.saslPlainPassword,
+		this.pinnedCertSHA1,
 	});
 
 	ServerEntry.fromMap(Map<String, dynamic> m) :
@@ -51,7 +54,8 @@ class ServerEntry {
 		nick = m['nick'] as String?,
 		pass = m['pass'] as String?,
 		saslPlainUsername = m['sasl_plain_username'] as String?,
-		saslPlainPassword = m['sasl_plain_password'] as String?;
+		saslPlainPassword = m['sasl_plain_password'] as String?,
+		pinnedCertSHA1 = m['pinned_cert_sha1'] as String?;
 }
 
 class NetworkEntry {
@@ -340,7 +344,8 @@ const _schema = [
 			nick TEXT,
 			pass TEXT,
 			sasl_plain_username TEXT,
-			sasl_plain_password TEXT
+			sasl_plain_password TEXT,
+			pinned_cert_sha1 TEXT
 		)
 	''',
 	'''
@@ -459,6 +464,7 @@ const _migrations = [
 	'CREATE INDEX index_message_network_msgid on Message(network_msgid)',
 	'ALTER TABLE LinkPreview ADD COLUMN image_url TEXT',
 	'ALTER TABLE Network ADD COLUMN last_delivered_time TEXT',
+	'ALTER TABLE Server ADD COLUMN pinned_cert_sha1 TEXT',
 ];
 
 class DB {
diff --git a/lib/main.dart b/lib/main.dart
index 31fadc9..75ca1ed 100644
--- a/lib/main.dart
+++ b/lib/main.dart
@@ -201,7 +201,7 @@ Future<void> _initModels({
 			clientParams = clientParams.apply(bouncerNetId: networkEntry.bouncerId);
 		}
 
-		var network = NetworkModel(serverEntry, networkEntry, clientParams.nick, clientParams.realname);
+		var network = NetworkModel(serverEntry, networkEntry, clientParams.nick, clientParams.realname, clientParams.pinnedCertSHA1);
 		networkList.add(network);
 
 		var client = Client(clientParams, isupport: networkEntry.isupport);
diff --git a/lib/models.dart b/lib/models.dart
index 9f1d309..d3c1968 100644
--- a/lib/models.dart
+++ b/lib/models.dart
@@ -55,10 +55,12 @@ class NetworkModel extends ChangeNotifier {
 	String _realname;
 	String? _account;
 	String? _connectError;
+	String? _pinnedCertSHA1;
 
-	NetworkModel(this.serverEntry, this.networkEntry, String nickname, String realname) :
+	NetworkModel(this.serverEntry, this.networkEntry, String nickname, String realname, String? pinnedCertSHA1) :
 			_nickname = nickname,
-			_realname = realname {
+			_realname = realname,
+			_pinnedCertSHA1 = pinnedCertSHA1 {
 		assert(serverEntry.id != null);
 		assert(networkEntry.id != null);
 		_upstreamName = networkEntry.isupport.network;
@@ -74,6 +76,7 @@ class NetworkModel extends ChangeNotifier {
 	String get realname => _realname;
 	String? get account => _account;
 	String? get connectError => _connectError;
+	String? get pinnedCertSHA1 => _pinnedCertSHA1;
 
 	String get displayName {
 		// If the user has set a custom bouncer network name, use that
diff --git a/lib/page/connect.dart b/lib/page/connect.dart
index d661844..9889231 100644
--- a/lib/page/connect.dart
+++ b/lib/page/connect.dart
@@ -1,6 +1,8 @@
 import 'dart:async';
+import 'dart:io';
 
 import 'package:flutter/material.dart';
+import 'package:hex/hex.dart';
 import 'package:provider/provider.dart';
 
 import '../client.dart';
@@ -29,6 +31,7 @@ class _ConnectPageState extends State<ConnectPage> {
 	bool _passwordRequired = false;
 	bool _passwordUnsupported = false;
 	Client? _fetchCapsClient;
+	String? _pinnedCertSHA1;
 
 	final formKey = GlobalKey<FormState>();
 	final serverController = TextEditingController();
@@ -42,6 +45,7 @@ class _ConnectPageState extends State<ConnectPage> {
 		if (widget.initialUri != null) {
 			_populateFromUri(widget.initialUri!);
 		}
+		_pinnedCertSHA1 = null;
 	}
 
 	void _populateFromUri(IrcUri uri) {
@@ -62,12 +66,14 @@ class _ConnectPageState extends State<ConnectPage> {
 	ServerEntry _generateServerEntry() {
 		Uri uri = parseServerUri(serverController.text);
 		var useSaslPlain = !passwordController.text.isEmpty;
+		var prefs = context.read<Prefs>();
 		return ServerEntry(
 			host: uri.host,
 			port: uri.hasPort ? uri.port : null,
 			tls: uri.scheme != 'irc+insecure',
 			saslPlainUsername: useSaslPlain ? nicknameController.text : null,
 			saslPlainPassword: useSaslPlain ? passwordController.text : null,
+			pinnedCertSHA1: _pinnedCertSHA1,
 		);
 	}
 
@@ -91,6 +97,7 @@ class _ConnectPageState extends State<ConnectPage> {
 		var clientProvider = context.read<ClientProvider>();
 
 		prefs.nickname = nicknameController.text;
+		prefs.pinnedCertSHA1 = _pinnedCertSHA1;
 
 		// TODO: only connect once (but be careful not to loose messages
 		// sent immediately after RPL_WELCOME)
@@ -100,7 +107,6 @@ class _ConnectPageState extends State<ConnectPage> {
 		try {
 			await client.connect();
 			client.dispose();
-
 			await db.storeServer(serverEntry);
 			networkEntry = await db.storeNetwork(NetworkEntry(server: serverEntry.id!));
 		} on Exception catch (err) {
@@ -121,7 +127,7 @@ class _ConnectPageState extends State<ConnectPage> {
 		}
 
 		client = Client(clientParams);
-		var network = NetworkModel(serverEntry, networkEntry, client.nick, client.realname);
+		var network = NetworkModel(serverEntry, networkEntry, client.nick, client.realname, client.pinnedCertSHA1);
 		networkList.add(network);
 		clientProvider.add(client, network);
 		client.connect().ignore();
@@ -149,6 +155,12 @@ class _ConnectPageState extends State<ConnectPage> {
 			setState(() {
 				_error = err;
 			});
+
+			if (err is BadCertException) {
+				final certErr = _error as BadCertException;
+				askBadCertficate(context, certErr.badCert);
+			}
+
 			return;
 		}
 
@@ -235,6 +247,8 @@ class _ConnectPageState extends State<ConnectPage> {
 				serverErr = ircErr.toString();
 				break;
 			}
+		} else if (_error is BadCertException) {
+			serverErr = 'Bad server certificate';
 		} else {
 			serverErr = _error?.toString();
 		}
@@ -261,6 +275,7 @@ class _ConnectPageState extends State<ConnectPage> {
 							setState(() {
 								_passwordUnsupported = false;
 								_passwordRequired = false;
+								_pinnedCertSHA1 = null;
 							});
 						},
 						validator: (value) {
@@ -313,4 +328,39 @@ class _ConnectPageState extends State<ConnectPage> {
 			),
 		);
 	}
+
+	void askBadCertficate(BuildContext context, X509Certificate cert) {
+		showDialog<void>(
+			context: context,
+			builder: (BuildContext context) {
+				Widget noButton = TextButton(
+					child: const Text('Reject'),
+					onPressed:  () { Navigator.pop(context); },
+				);
+				Widget yesButton = TextButton(
+					child: const Text('Accept Always'),
+					onPressed:  () {
+						Navigator.pop(context);
+						setState(() => _pinnedCertSHA1 = HEX.encode(cert.sha1));
+						_handleServerFocusChange(false);
+					},
+				);
+				AlertDialog alert = AlertDialog(
+					title: const Text('Bad Certificate'),
+					content: SingleChildScrollView(
+						child: Text(
+							'Untrusted server certificate. '
+							+ 'Only accept this certificate if you know what you\'re doing.\n\n'
+							+ 'Issuer: ' + cert.issuer + '\n'
+							+ 'SHA1 Fingerprint: ' + HEX.encode(cert.sha1) + '\n'
+							+ 'From: ' + cert.startValidity.toString() + '\n'
+							+ 'To: ' + cert.endValidity.toString()
+						)
+					),
+					actions: [ noButton, yesButton ],
+				);
+				return alert;
+			},
+		);
+	}
 }
diff --git a/lib/prefs.dart b/lib/prefs.dart
index 211eeba..2aef755 100644
--- a/lib/prefs.dart
+++ b/lib/prefs.dart
@@ -9,6 +9,7 @@ const _nicknameKey = 'nickname';
 const _realnameKey = 'realname';
 const _pushProviderKey = 'push_provider';
 const _linkPreviewKey = 'link_preview';
+const _pinnedCertSHA1Key = 'pinnedCertSHA1';
 
 class Prefs {
 	final SharedPreferences _prefs;
@@ -30,6 +31,7 @@ class Prefs {
 	String? get realname => _prefs.getString(_realnameKey);
 	String? get pushProvider => _prefs.getString(_pushProviderKey);
 	bool get linkPreview => _prefs.getBool(_linkPreviewKey) ?? false;
+	String? get pinnedCertSHA1 => _prefs.getString(_pinnedCertSHA1Key);
 
 	set bufferCompact(bool enabled) {
 		_prefs.setBool(_bufferCompactKey, enabled);
@@ -62,4 +64,8 @@ class Prefs {
 	set linkPreview(bool enabled) {
 		_prefs.setBool(_linkPreviewKey, enabled);
 	}
+
+	set pinnedCertSHA1(String? pinnedCertSHA1) {
+		_setOptionalString(_pinnedCertSHA1Key, pinnedCertSHA1);
+	}
 }
diff --git a/lib/push.dart b/lib/push.dart
index 48e1b68..2eb925f 100644
--- a/lib/push.dart
+++ b/lib/push.dart
@@ -60,7 +60,8 @@ Future<void> handlePushMessage(DB db, WebPushSubscriptionEntry sub, List<int> ci
 
 	var nickname = serverEntry.nick ?? prefs.nickname;
 	var realname = prefs.realname ?? nickname;
-	var network = NetworkModel(serverEntry, networkEntry, nickname, realname);
+	var pinnedCertSHA1 = prefs.pinnedCertSHA1;
+	var network = NetworkModel(serverEntry, networkEntry, nickname, realname, pinnedCertSHA1);
 
 	var notifController = await NotificationController.init();
 
diff --git a/pubspec.lock b/pubspec.lock
index 298f6f3..2332764 100644
--- a/pubspec.lock
+++ b/pubspec.lock
@@ -456,6 +456,14 @@ packages:
       url: "https://pub.dev"
     source: hosted
     version: "2.1.0"
+  hex:
+    dependency: "direct main"
+    description:
+      name: hex
+      sha256: "4e7cd54e4b59ba026432a6be2dd9d96e4c5205725194997193bf871703b82c4a"
+      url: "https://pub.dev"
+    source: hosted
+    version: "0.2.0"
   html:
     dependency: "direct main"
     description:
diff --git a/pubspec.yaml b/pubspec.yaml
index 5eeae9b..4defa83 100644
--- a/pubspec.yaml
+++ b/pubspec.yaml
@@ -46,6 +46,7 @@ dependencies:
   flutter_apns_only: ^1.6.0
   share_handler: ^0.0.21
   dynamic_color: ^1.7.0
+  hex: ^0.2.0
 
 dependency_overrides:
   # TODO: drop once this is released:
-- 
2.46.0

Thanks for the patch! Overall it looks pretty good, below are a few comments.

On Sunday, August 25th, 2024 at 21:54, Matthew Hague <matthewhague@zoho.com> wrote:

> +class BadCertException implements Exception {
> +	final X509Certificate badCert;
> +	BadCertException(this.badCert);
> +}

Should we override the toString() method to provide a human-readable message?
This would typically happen if the user connects to a server, and the later on
the server certificate changes.

> diff --git a/lib/main.dart b/lib/main.dart
> index 31fadc9..75ca1ed 100644
> --- a/lib/main.dart
> +++ b/lib/main.dart
> @@ -201,7 +201,7 @@ Future<void> _initModels({
>  			clientParams = clientParams.apply(bouncerNetId: networkEntry.bouncerId);
>  		}
> 
> -		var network = NetworkModel(serverEntry, networkEntry, clientParams.nick, clientParams.realname);
> +		var network = NetworkModel(serverEntry, networkEntry, clientParams.nick, clientParams.realname, clientParams.pinnedCertSHA1);
>  		networkList.add(network);
> 
>  		var client = Client(clientParams, isupport: networkEntry.isupport);
> diff --git a/lib/models.dart b/lib/models.dart
> index 9f1d309..d3c1968 100644
> --- a/lib/models.dart
> +++ b/lib/models.dart
> @@ -55,10 +55,12 @@ class NetworkModel extends ChangeNotifier {
>  	String _realname;
>  	String? _account;
>  	String? _connectError;
> +	String? _pinnedCertSHA1;
> 
> -	NetworkModel(this.serverEntry, this.networkEntry, String nickname, String realname) :
> +	NetworkModel(this.serverEntry, this.networkEntry, String nickname, String realname, String? pinnedCertSHA1) :

Do we need to pass the pinnedCertSHA1 as a separate argument here? Isn't it
already passed in serverEntry?

>  			_nickname = nickname,
> -			_realname = realname {
> +			_realname = realname,
> +			_pinnedCertSHA1 = pinnedCertSHA1 {
>  		assert(serverEntry.id != null);
>  		assert(networkEntry.id != null);
>  		_upstreamName = networkEntry.isupport.network;
> @@ -74,6 +76,7 @@ class NetworkModel extends ChangeNotifier {
>  	String get realname => _realname;
>  	String? get account => _account;
>  	String? get connectError => _connectError;
> +	String? get pinnedCertSHA1 => _pinnedCertSHA1;

Callers could use serverEntry.pinnedCertSHA1 instead of this getter.

>  	String get displayName {
>  		// If the user has set a custom bouncer network name, use that
> diff --git a/lib/page/connect.dart b/lib/page/connect.dart
> index d661844..9889231 100644
> --- a/lib/page/connect.dart
> +++ b/lib/page/connect.dart
> @@ -1,6 +1,8 @@
>  import 'dart:async';
> +import 'dart:io';
> 
>  import 'package:flutter/material.dart';
> +import 'package:hex/hex.dart';
>  import 'package:provider/provider.dart';
> 
>  import '../client.dart';
> @@ -29,6 +31,7 @@ class _ConnectPageState extends State<ConnectPage> {
>  	bool _passwordRequired = false;
>  	bool _passwordUnsupported = false;
>  	Client? _fetchCapsClient;
> +	String? _pinnedCertSHA1;
> 
>  	final formKey = GlobalKey<FormState>();
>  	final serverController = TextEditingController();
> @@ -42,6 +45,7 @@ class _ConnectPageState extends State<ConnectPage> {
>  		if (widget.initialUri != null) {
>  			_populateFromUri(widget.initialUri!);
>  		}
> +		_pinnedCertSHA1 = null;

null should be the default value when the state is created, so this shouldn't
be necessary (initState is called at most once per instance).

>  	}
> 
>  	void _populateFromUri(IrcUri uri) {
> @@ -62,12 +66,14 @@ class _ConnectPageState extends State<ConnectPage> {
>  	ServerEntry _generateServerEntry() {
>  		Uri uri = parseServerUri(serverController.text);
>  		var useSaslPlain = !passwordController.text.isEmpty;
> +		var prefs = context.read<Prefs>();

Sounds like this is unused?

>  		return ServerEntry(
>  			host: uri.host,
>  			port: uri.hasPort ? uri.port : null,
>  			tls: uri.scheme != 'irc+insecure',
>  			saslPlainUsername: useSaslPlain ? nicknameController.text : null,
>  			saslPlainPassword: useSaslPlain ? passwordController.text : null,
> +			pinnedCertSHA1: _pinnedCertSHA1,
>  		);
>  	}
> 
> @@ -91,6 +97,7 @@ class _ConnectPageState extends State<ConnectPage> {
>  		var clientProvider = context.read<ClientProvider>();
> 
>  		prefs.nickname = nicknameController.text;
> +		prefs.pinnedCertSHA1 = _pinnedCertSHA1;

Hm, why do we need to store the pinned cert hash in the global preferences?

The nickname is a bit of a special case: in the settings view, it's a global
preference which applies to all servers. I don't think we need to replicate
this for the pinned cert hash.

> 
>  		// TODO: only connect once (but be careful not to loose messages
>  		// sent immediately after RPL_WELCOME)
> @@ -100,7 +107,6 @@ class _ConnectPageState extends State<ConnectPage> {
>  		try {
>  			await client.connect();
>  			client.dispose();
> -
>  			await db.storeServer(serverEntry);
>  			networkEntry = await db.storeNetwork(NetworkEntry(server: serverEntry.id!));
>  		} on Exception catch (err) {
> @@ -121,7 +127,7 @@ class _ConnectPageState extends State<ConnectPage> {
>  		}
> 
>  		client = Client(clientParams);
> -		var network = NetworkModel(serverEntry, networkEntry, client.nick, client.realname);
> +		var network = NetworkModel(serverEntry, networkEntry, client.nick, client.realname, client.pinnedCertSHA1);
>  		networkList.add(network);
>  		clientProvider.add(client, network);
>  		client.connect().ignore();
> @@ -149,6 +155,12 @@ class _ConnectPageState extends State<ConnectPage> {
>  			setState(() {
>  				_error = err;
>  			});
> +
> +			if (err is BadCertException) {
> +				final certErr = _error as BadCertException;

Nit: would be a bit more consistent with the line above to use "err" instead of
"_error" here.

> +	void askBadCertficate(BuildContext context, X509Certificate cert) {
> +		showDialog<void>(
> +			context: context,
> +			builder: (BuildContext context) {
> +				Widget noButton = TextButton(
> +					child: const Text('Reject'),
> +					onPressed:  () { Navigator.pop(context); },
> +				);
> +				Widget yesButton = TextButton(
> +					child: const Text('Accept Always'),
> +					onPressed:  () {
> +						Navigator.pop(context);
> +						setState(() => _pinnedCertSHA1 = HEX.encode(cert.sha1));
> +						_handleServerFocusChange(false);
> +					},
> +				);
> +				AlertDialog alert = AlertDialog(
> +					title: const Text('Bad Certificate'),
> +					content: SingleChildScrollView(
> +						child: Text(
> +							'Untrusted server certificate. '
> +							+ 'Only accept this certificate if you know what you\'re doing.\n\n'
> +							+ 'Issuer: ' + cert.issuer + '\n'
> +							+ 'SHA1 Fingerprint: ' + HEX.encode(cert.sha1) + '\n'
> +							+ 'From: ' + cert.startValidity.toString() + '\n'
> +							+ 'To: ' + cert.endValidity.toString()
> +						)
> +					),
> +					actions: [ noButton, yesButton ],
> +				);
> +				return alert;

Nit: the "alert" variable is unnecessary here, the AlertDialog can be directly
returned.

>below are a few comments..

Thanks for this -- i'll submit a rework when i can.

Best,

Matt