From 79478eb89f30752d96ec8726dae8d359e13d369b Mon Sep 17 00:00:00 2001
From: Asher <ash@coder.com>
Date: Tue, 10 Nov 2020 18:33:16 -0600
Subject: [PATCH] Clarify some points around the cookie domain

Also add a check that the domain has a dot. This covers the localhost
case as well, so remove that.
---
 src/node/http.ts | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/src/node/http.ts b/src/node/http.ts
index f259d1037..1aa7adb51 100644
--- a/src/node/http.ts
+++ b/src/node/http.ts
@@ -98,27 +98,36 @@ export const redirect = (
 
 /**
  * Get the value that should be used for setting a cookie domain. This will
- * allow the user to authenticate only once. This will use the highest level
- * domain (e.g. `coder.com` over `test.coder.com` if both are specified).
+ * allow the user to authenticate once no matter what sub-domain they use to log
+ * in. This will use the highest level proxy domain (e.g. `coder.com` over
+ * `test.coder.com` if both are specified).
  */
 export const getCookieDomain = (host: string, proxyDomains: string[]): string | undefined => {
   const idx = host.lastIndexOf(":")
   host = idx !== -1 ? host.substring(0, idx) : host
+  // If any of these are true we will still set cookies but without an explicit
+  // `Domain` attribute on the cookie.
   if (
-    // Might be blank/missing, so there's nothing more to do.
+    // The host can be be blank or missing so there's nothing we can set.
     !host ||
     // IP addresses can't have subdomains so there's no value in setting the
-    // domain for them. Assume anything with a : is ipv6 (valid domain name
-    // characters are alphanumeric or dashes).
+    // domain for them. Assume that anything with a : is ipv6 (valid domain name
+    // characters are alphanumeric or dashes)...
     host.includes(":") ||
-    // Assume anything entirely numbers and dots is ipv4 (currently tlds
+    // ...and that anything entirely numbers and dots is ipv4 (currently tlds
     // cannot be entirely numbers).
     !/[^0-9.]/.test(host) ||
-    // localhost subdomains don't seem to work at all (browser bug?).
+    // localhost subdomains don't seem to work at all (browser bug?). A cookie
+    // set at dev.localhost cannot be read by 8080.dev.localhost.
     host.endsWith(".localhost") ||
-    // It might be localhost (or an IP, see above) if it's a proxy and it
-    // isn't setting the host header to match the access domain.
-    host === "localhost"
+    // Domains without at least one dot (technically two since domain.tld will
+    // become .domain.tld) are considered invalid according to the spec so don't
+    // set the domain for them. In my testing though localhost is the only
+    // problem (the browser just doesn't store the cookie at all). localhost has
+    // an additional problem which is that a reverse proxy might give
+    // code-server localhost even though the domain is really domain.tld (by
+    // default NGINX does this).
+    !host.includes(".")
   ) {
     logger.debug("no valid cookie doman", field("host", host))
     return undefined