From 272497f3b668fe69eb6174ebe9ba36e82429bcc6 Mon Sep 17 00:00:00 2001
From: Dominik Picheta <dominik@cloudflare.com>
Date: Wed, 7 Jun 2023 17:58:03 +0000
Subject: [PATCH] Improves TCP Socket connection failure errors.

---
 src/workerd/api/sockets.c++ | 20 +++++++++++++++-----
 src/workerd/api/sockets.h   |  7 +++++--
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/src/workerd/api/sockets.c++ b/src/workerd/api/sockets.c++
index 29102bf8afb..1ee63fbb3ce 100644
--- a/src/workerd/api/sockets.c++
+++ b/src/workerd/api/sockets.c++
@@ -68,7 +68,7 @@ bool getAllowHalfOpen(jsg::Optional<SocketOptions>& opts) {
 jsg::Ref<Socket> setupSocket(
     jsg::Lock& js, kj::Own<kj::AsyncIoStream> connection,
     jsg::Optional<SocketOptions> options, kj::Own<kj::TlsStarterCallback> tlsStarter,
-    bool isSecureSocket, kj::String domain) {
+    bool isSecureSocket, kj::String domain, int port) {
   auto& ioContext = IoContext::current();
   auto connDisconnPromise = connection->whenWriteDisconnected();
 
@@ -96,7 +96,8 @@ jsg::Ref<Socket> setupSocket(
       kj::mv(options),
       kj::mv(tlsStarter),
       isSecureSocket,
-      kj::mv(domain));
+      kj::mv(domain),
+      port);
   KJ_IF_MAYBE(p, eofPromise) {
     result->handleReadableEof(js, kj::mv(*p));
   }
@@ -111,6 +112,7 @@ jsg::Ref<Socket> connectImplNoOutputLock(
 
   // Extract the domain/ip we are connecting to from the address.
   kj::String domain;
+  int port;
   KJ_SWITCH_ONEOF(address) {
     KJ_CASE_ONEOF(str, kj::String) {
       // We need just the hostname part of the address, i.e. we want to strip out the port.
@@ -119,10 +121,13 @@ jsg::Ref<Socket> connectImplNoOutputLock(
           TypeError, "Specified address could not be parsed.");
       auto& host = JSG_REQUIRE_NONNULL(record.host, TypeError,
           "Specified address is missing hostname.");
+      port = JSG_REQUIRE_NONNULL(record.port, TypeError,
+          "Specified address is missing a port.");
       domain = host.toStr();
     }
     KJ_CASE_ONEOF(record, SocketAddress) {
       domain = kj::heapString(record.hostname);
+      port = record.port;
     }
   }
 
@@ -163,7 +168,7 @@ jsg::Ref<Socket> connectImplNoOutputLock(
 
   auto result = setupSocket(
       js, kj::mv(request.connection), kj::mv(options), kj::mv(tlsStarter),
-      httpConnectSettings.useTls, kj::mv(domain));
+      httpConnectSettings.useTls, kj::mv(domain), port);
   // `handleProxyStatus` needs an initialised refcount to use `JSG_THIS`, hence it cannot be
   // called in Socket's constructor. Also it's only necessary when creating a Socket as a result of
   // a `connect`.
@@ -244,7 +249,7 @@ jsg::Ref<Socket> Socket::startTls(jsg::Lock& js, jsg::Optional<TlsOptions> tlsOp
   // to `setupSocket`.
   auto newTlsStarter = kj::heap<kj::TlsStarterCallback>();
   return setupSocket(js, kj::newPromisedStream(kj::mv(secureStreamPromise)), kj::mv(options),
-      kj::mv(newTlsStarter), true, kj::mv(domain));
+      kj::mv(newTlsStarter), true, kj::mv(domain), kj::mv(port));
 }
 
 void Socket::handleProxyStatus(
@@ -255,8 +260,13 @@ void Socket::handleProxyStatus(
     if (status.statusCode < 200 || status.statusCode >= 300) {
       // If the status indicates an unsucessful connection we need to reject the `closeFulfiller`
       // with an exception. This will reject the socket's `closed` promise.
+      auto msg = kj::str(": proxy request failed, cannot connect to the specified address");
+      if (port == 443 || port == 80) {
+        msg = kj::str(msg, ". It looks like you might be trying to connect to a HTTP-based service",
+            " — consider using fetch instead");
+      }
       auto exc = kj::Exception(kj::Exception::Type::FAILED, __FILE__, __LINE__,
-        kj::str(JSG_EXCEPTION(Error) ": proxy request failed"));
+        kj::str(JSG_EXCEPTION(Error), msg));
       resolveFulfiller(js, exc);
       readable->getController().cancel(js, nullptr).markAsHandled();
       writable->getController().abort(js, nullptr).markAsHandled();
diff --git a/src/workerd/api/sockets.h b/src/workerd/api/sockets.h
index 451c0634a4b..4325d925ef0 100644
--- a/src/workerd/api/sockets.h
+++ b/src/workerd/api/sockets.h
@@ -43,7 +43,7 @@ class Socket: public jsg::Object {
       jsg::Ref<ReadableStream> readableParam, jsg::Ref<WritableStream> writable,
       jsg::PromiseResolverPair<void> close, kj::Promise<void> connDisconnPromise,
       jsg::Optional<SocketOptions> options, kj::Own<kj::TlsStarterCallback> tlsStarter,
-      bool isSecureSocket, kj::String domain)
+      bool isSecureSocket, kj::String domain, int port)
       : connectionStream(IoContext::current().addObject(kj::mv(connectionStream))),
         readable(kj::mv(readableParam)), writable(kj::mv(writable)),
         closeFulfiller(kj::mv(close)),
@@ -56,7 +56,8 @@ class Socket: public jsg::Object {
         options(kj::mv(options)),
         tlsStarter(IoContext::current().addObject(kj::mv(tlsStarter))),
         isSecureSocket(isSecureSocket),
-        domain(kj::mv(domain)) { };
+        domain(kj::mv(domain)),
+        port(port) { };
 
   jsg::Ref<ReadableStream> getReadable() { return readable.addRef(); }
   jsg::Ref<WritableStream> getWritable() { return writable.addRef(); }
@@ -103,6 +104,8 @@ class Socket: public jsg::Object {
   // `startTls`.
   kj::String domain;
   // The domain/ip this socket is connected to. Used for startTls.
+  int port;
+  // The port this socket is connected to. Used for nicer errors.
 
   kj::Promise<kj::Own<kj::AsyncIoStream>> processConnection();
   jsg::Promise<void> maybeCloseWriteSide(jsg::Lock& js);