Throw error when incoming connection uses TLS, fix merge conflict

- Make remoteAddress and domain parameters optional since we don't have them
  available

Author: fhanau

src/workerd/api/global-scope.c++

@@ -140,21 +140,15 @@ kj::Promise<void> ServiceWorkerGlobalScope::connect(kj::String host,
     auto& ioContext = IoContext::current();
     jsg::Lock& js = lock;
 
-    auto input = kj::str("fake://", host);
-    auto url = JSG_REQUIRE_NONNULL(
-        jsg::Url::tryParse(input.asPtr()), TypeError, "Specified address could not be parsed.");
-    auto hostName = url.getHostname();
-    auto port = url.getPort();
-    JSG_REQUIRE(hostName != ""_kj, TypeError, "Specified address is missing hostname.");
-    JSG_REQUIRE(port != ""_kj, TypeError, "Specified address is missing port.");
-
-    // TLS support is not implemented so far.
+    // TLS support is not implemented so far. Note that setupSocket() expects the domain parameter
+    // to be set to the expected host name using startTLS, so that it can be provided to the TLS
+    // callback, so we'd need to change that or figure out a way to get the host domain.
     auto nullTlsStarter = kj::heap<kj::TlsStarterCallback>();
     // We set isDefaultFetchPort to false here – sockets.c++ sets it for ports 443 and 8080 to
     // provide a more descriptive error message for HTTP, but this is not relevant on the TCP server
     // side.
-    jsg::Ref<Socket> jsSocket = setupSocket(js, kj::mv(ownConnection), kj::mv(host), kj::none,
-        kj::mv(nullTlsStarter), SecureTransportKind::OFF, kj::str(hostName), false, kj::none);
+    jsg::Ref<Socket> jsSocket = setupSocket(js, kj::mv(ownConnection), kj::none, kj::none,
+        kj::mv(nullTlsStarter), SecureTransportKind::OFF, kj::none, false, kj::none);
     // handleProxyStatus() is required to indicate that the socket was opened properly. Since the
     // connection is already open at this point, exception handling is not required.
     jsSocket->handleProxyStatus(js, kj::Promise<kj::Maybe<kj::Exception>>(kj::none));

src/workerd/api/sockets.c++

@@ -84,11 +84,11 @@ class StreamWorkerInterface;
 
 jsg::Ref<Socket> setupSocket(jsg::Lock& js,
     kj::Own<kj::AsyncIoStream> connection,
-    kj::String remoteAddress,
+    kj::Maybe<kj::String> remoteAddress,
     jsg::Optional<SocketOptions> options,
     kj::Own<kj::TlsStarterCallback> tlsStarter,
     SecureTransportKind secureTransport,
-    kj::String domain,
+    kj::Maybe<kj::String> domain,
     bool isDefaultFetchPort,
     kj::Maybe<jsg::PromiseResolverPair<SocketInfo>> maybeOpenedPrPair) {
   auto& ioContext = IoContext::current();
@@ -323,10 +323,10 @@ jsg::Ref<Socket> Socket::startTls(jsg::Lock& js, jsg::Optional<TlsOptions> tlsOp
       secureTransport != SecureTransportKind::ON, TypeError, "Cannot startTls on a TLS socket.");
   JSG_REQUIRE(connectionData != kj::none, TypeError,
       "The connection was closed before startTls could be started.");
-  JSG_REQUIRE(domain != nullptr, TypeError, "startTls can only be called once.");
   auto invalidOptKindMsg =
       "The `secureTransport` socket option must be set to 'starttls' for startTls to be used.";
   JSG_REQUIRE(secureTransport == SecureTransportKind::STARTTLS, TypeError, invalidOptKindMsg);
+  JSG_REQUIRE(domain != kj::none, TypeError, "startTls can only be called once.");
 
   // The current socket's writable buffers need to be flushed. The socket's WritableStream is backed
   // by an AsyncIoStream which doesn't implement any buffering, so we don't need to worry about
@@ -346,10 +346,10 @@ jsg::Ref<Socket> Socket::startTls(jsg::Lock& js, jsg::Optional<TlsOptions> tlsOp
           // flush to complete. While it is unlikely to be GC'd while we are waiting because
           // the user code *likely* is holding a active reference to it at this point, we
           // don't want to take any chances. This prevents a possible UAF.
-          JSG_VISITABLE_LAMBDA(
-              (self = JSG_THIS, domain = kj::heapString(domain), tlsOptions = kj::mv(tlsOptions),
-                  openedResolver = openedPrPair.resolver.addRef(js),
-                  remoteAddress = kj::str(remoteAddress)),
+          JSG_VISITABLE_LAMBDA((self = JSG_THIS, domain = kj::heapString(KJ_ASSERT_NONNULL(domain)),
+                                   tlsOptions = kj::mv(tlsOptions),
+                                   openedResolver = openedPrPair.resolver.addRef(js),
+                                   remoteAddress = mapCopyString(remoteAddress)),
               (self, openedResolver), (jsg::Lock & js) mutable {
                 auto& context = IoContext::current();
 
@@ -381,7 +381,7 @@ jsg::Ref<Socket> Socket::startTls(jsg::Lock& js, jsg::Optional<TlsOptions> tlsOp
 
                 // Fork the starter promise because we need to create two separate things waiting
                 // on it below. The first is resolving the openedResolver with a JS promise that
-                // wraps one branch, the secnod is the kj::Promise that we use to resolve the
+                // wraps one branch, the second is the kj::Promise that we use to resolve the
                 // secureStream for the promised stream. This keeps us from having to bounce in and
                 // out of the JS isolate lock.
                 auto forkedPromise = KJ_ASSERT_NONNULL(*tlsStarter)(acceptedHostname).fork();
@@ -410,9 +410,9 @@ jsg::Ref<Socket> Socket::startTls(jsg::Lock& js, jsg::Optional<TlsOptions> tlsOp
   // The existing tlsStarter gets consumed and we won't need it again. Pass in an empty tlsStarter
   // to `setupSocket`.
   auto newTlsStarter = kj::heap<kj::TlsStarterCallback>();
-  return setupSocket(js, kj::newPromisedStream(kj::mv(secureStreamPromise)), kj::str(remoteAddress),
-      kj::mv(options), kj::mv(newTlsStarter), SecureTransportKind::ON, kj::mv(domain),
-      isDefaultFetchPort, kj::mv(openedPrPair));
+  return setupSocket(js, kj::newPromisedStream(kj::mv(secureStreamPromise)),
+      mapCopyString(remoteAddress), kj::mv(options), kj::mv(newTlsStarter), SecureTransportKind::ON,
+      kj::mv(domain), isDefaultFetchPort, kj::mv(openedPrPair));
 }
 
 void Socket::handleProxyStatus(
@@ -436,7 +436,7 @@ void Socket::handleProxyStatus(
       if (isDefaultFetchPort) {
         msg = kj::str(msg, ". It looks like you might be trying to connect to a HTTP-based service",
             " — consider using fetch instead");
-      } else if (remoteAddress.contains(".hyperdrive.local"_kj)) {
+      } else if (remoteAddress.orDefault(kj::String()).contains(".hyperdrive.local"_kj)) {
         // No attempts to connect to Hyperdrive should end up here, since they go through the other
         // version of handleProxyStatus. If they end up here somehow, log about it to get some
         // context that can aid in debugging.
@@ -450,7 +450,7 @@ void Socket::handleProxyStatus(
       // because there's no useful value we can provide.
       openedResolver.resolve(js,
           SocketInfo{
-            .remoteAddress = kj::str(remoteAddress),
+            .remoteAddress = mapCopyString(remoteAddress),
             .localAddress = kj::none,
           });
     }
@@ -478,7 +478,7 @@ void Socket::handleProxyStatus(jsg::Lock& js, kj::Promise<kj::Maybe<kj::Exceptio
       // because there's no useful value we can provide.
       openedResolver.resolve(js,
           SocketInfo{
-            .remoteAddress = kj::str(remoteAddress),
+            .remoteAddress = mapCopyString(remoteAddress),
             .localAddress = kj::none,
           });
     }

src/workerd/api/sockets.h

@@ -60,15 +60,15 @@ class Socket: public jsg::Object {
   Socket(jsg::Lock& js,
       IoContext& context,
       kj::Own<kj::RefcountedWrapper<kj::Own<kj::AsyncIoStream>>> connectionStream,
-      kj::String remoteAddress,
+      kj::Maybe<kj::String> remoteAddress,
       jsg::Ref<ReadableStream> readableParam,
       jsg::Ref<WritableStream> writable,
       jsg::PromiseResolverPair<void> closedPrPair,
       kj::Promise<void> watchForDisconnectTask,
       jsg::Optional<SocketOptions> options,
       kj::Own<kj::TlsStarterCallback> tlsStarter,
       SecureTransportKind secureTransport,
-      kj::String domain,
+      kj::Maybe<kj::String> domain,
       bool isDefaultFetchPort,
       jsg::PromiseResolverPair<SocketInfo> openedPrPair)
       : connectionData(context.addObject(kj::heap<ConnectionData>(
@@ -200,12 +200,12 @@ class Socket: public jsg::Object {
   // Memoized copy that is returned by the `closed` attribute.
   jsg::MemoizedIdentity<jsg::Promise<void>> closedPromise;
   jsg::Optional<SocketOptions> options;
-  kj::String remoteAddress;
+  kj::Maybe<kj::String> remoteAddress;
   // Set to true when the socket is upgraded to a secure one.
   bool upgraded = false;
   SecureTransportKind secureTransport;
   // The domain/ip this socket is connected to. Used for startTls.
-  kj::String domain;
+  kj::Maybe<kj::String> domain;
   // Whether the port this socket connected to is 80/443. Used for nicer errors.
   bool isDefaultFetchPort;
   // This fulfiller is used to resolve the `openedPromise` below.
@@ -245,11 +245,11 @@ class Socket: public jsg::Object {
 
 jsg::Ref<Socket> setupSocket(jsg::Lock& js,
     kj::Own<kj::AsyncIoStream> connection,
-    kj::String remoteAddress,
+    kj::Maybe<kj::String> remoteAddress,
     jsg::Optional<SocketOptions> options,
     kj::Own<kj::TlsStarterCallback> tlsStarter,
     SecureTransportKind secureTransport,
-    kj::String domain,
+    kj::Maybe<kj::String> domain,
     bool isDefaultFetchPort,
     kj::Maybe<jsg::PromiseResolverPair<SocketInfo>> maybeOpenedPrPair);
 

src/workerd/io/worker-entrypoint.c++

@@ -565,6 +565,8 @@ kj::Promise<void> WorkerEntrypoint::connect(kj::StringPtr host,
     JSG_FAIL_REQUIRE(TypeError, "Incoming CONNECT on a worker not supported");
   }
 
+  // TODO(soon): Implement basic TLS support for connect handler.
+  JSG_REQUIRE(!settings.useTls, Error, "Incoming CONNECT with TLS not supported");
   // Capture workerTracer, see request() for rationale.
   kj::Maybe<BaseTracer&> workerTracer;
 

src/workerd/io/worker-interface.capnp

@@ -97,7 +97,7 @@ struct Trace @0x8e8d911203762d34 {
     email @16 :EmailEventInfo;
     trace @18 :TraceEventInfo;
     hibernatableWebSocket @20 :HibernatableWebSocketEventInfo;
-    connect @28 :ConnectEventInfo;
+    connect @29 :ConnectEventInfo;
   }
   struct FetchEventInfo {
     method @0 :HttpMethod;