From 77f98cbfab158a18a1cac8115c11b7b72f1fbcdd Mon Sep 17 00:00:00 2001
From: "Stephen M. Coakley" <me@stephencoakley.com>
Date: Tue, 4 Feb 2020 23:45:52 -0600
Subject: [PATCH] Make default VersionNegotiation more conservative

The `Upgrade` header that can be used for insecure upgrade from HTTP/1.x to HTTP/2 is not handled properly by many in-production HTTP servers and can result in an error when the request would otherwise succeed without the `Upgrade` header.

Since there's not really any way for us to know if an error response was caused by our `Upgrade` header or not (and what the server state is for non-idempotent methods), `VersionNegotiation::latest_compatible` will no longer send this header, and instead rely exclusively on the server to explicitly _declare_ higher HTTP versions that it supports, either via ALPN or something else.

Fixes #159.
---
 src/config/mod.rs | 22 +++++++++++++++-------
 tests/version.rs  | 24 ------------------------
 2 files changed, 15 insertions(+), 31 deletions(-)
 delete mode 100644 tests/version.rs

diff --git a/src/config/mod.rs b/src/config/mod.rs
index 4abe8dff..8a01c58e 100644
--- a/src/config/mod.rs
+++ b/src/config/mod.rs
@@ -510,17 +510,25 @@ impl Default for VersionNegotiation {
 }
 
 impl VersionNegotiation {
-    /// Always prefer the latest supported version with a preference for old
-    /// versions if necessary in order to connect. This is the default.
-    ///
-    /// Typically negotiation will begin with an HTTP/1.1 request, upgrading to
-    /// HTTP/2 if possible, then to HTTP/3 if possible, etc.
+    /// Always prefer the latest supported version announced by the server,
+    /// falling back to older versions if not explicitly listed as supported.
+    /// This is the default.
+    ///
+    /// Secure connections will begin with a TLS handshake, after which the
+    /// highest supported HTTP version listed by the server via ALPN will be
+    /// used. Once connected, additional upgrades to newer versions may also
+    /// occur if the server lists support for it. In the future, headers such as
+    /// `Alt-Svc` will be used.
+    ///
+    /// Insecure connections always use HTTP/1.x since there is no standard
+    /// mechanism for a server to declare support for insecure HTTP versions,
+    /// and only HTTP/1.x and HTTP/2 support insecure transfers.
     pub const fn latest_compatible() -> Self {
         Self {
             // In curl land, this basically the most lenient option. Alt-Svc is
             // used to upgrade to newer versions, and old versions are used if
-            // the server doesn't respond to the HTTP/1.1 -> HTTP/2 upgrade.
-            flag: curl::easy::HttpVersion::V2,
+            // the server doesn't list HTTP/2 via ALPN.
+            flag: curl::easy::HttpVersion::V2TLS,
             strict: false,
         }
     }
diff --git a/tests/version.rs b/tests/version.rs
deleted file mode 100644
index 689cc6a6..00000000
--- a/tests/version.rs
+++ /dev/null
@@ -1,24 +0,0 @@
-use isahc::config::VersionNegotiation;
-use isahc::prelude::*;
-use mockito::{mock, server_url};
-
-speculate::speculate! {
-    before {
-        env_logger::try_init().ok();
-    }
-
-    test "latest compatible negotiation politely asks for HTTP/2" {
-        let m = mock("GET", "/")
-            .match_header("upgrade", "h2c")
-            .create();
-
-        Request::get(server_url())
-            .version_negotiation(VersionNegotiation::latest_compatible())
-            .body(())
-            .unwrap()
-            .send()
-            .unwrap();
-
-        m.assert();
-    }
-}