[server] verifyClient: Remove dead code to parse workspace URLs (#16845)

geropl · web-flow · commit 8744ad02fcff · 2023-03-15T11:49:07.000+01:00
diff --git a/components/server/src/express-util.spec.ts b/components/server/src/express-util.spec.ts
@@ -15,7 +15,6 @@ describe("express-util", function () {
             const result = isAllowedWebsocketDomain(
                 "http://3000-moccasin-ferret-155799b3.ws-eu.gpl-2732-ws-csrf.staging.gitpod.io",
                 HOSTURL_HOSTNAME,
-                false,
             );
             expect(result).to.be.false;
         });
@@ -24,24 +23,18 @@ describe("express-util", function () {
             const result = isAllowedWebsocketDomain(
                 "http://moccasin-ferret-155799b3.ws-eu.gpl-2732-ws-csrf.staging.gitpod.io",
                 HOSTURL_HOSTNAME,
-                false,
             );
             expect(result).to.be.true;
         });
 
         it("should return true for dashboard", function () {
-            const result = isAllowedWebsocketDomain(
-                "http://gpl-2732-ws-csrf.staging.gitpod.io",
-                HOSTURL_HOSTNAME,
-                false,
-            );
+            const result = isAllowedWebsocketDomain("http://gpl-2732-ws-csrf.staging.gitpod.io", HOSTURL_HOSTNAME);
             expect(result).to.be.true;
         });
         it("should return false for workspace-port locations (strict)", function () {
             const result = isAllowedWebsocketDomain(
                 "http://3000-moccasin-ferret-155799b3.ws-eu.gpl-2732-ws-csrf.staging.gitpod.io",
                 HOSTURL_HOSTNAME,
-                true,
             );
             expect(result).to.be.false;
         });
@@ -50,17 +43,12 @@ describe("express-util", function () {
             const result = isAllowedWebsocketDomain(
                 "http://moccasin-ferret-155799b3.ws-eu.gpl-2732-ws-csrf.staging.gitpod.io",
                 HOSTURL_HOSTNAME,
-                true,
             );
             expect(result).to.be.false;
         });
 
         it("should return true for dashboard (strict)", function () {
-            const result = isAllowedWebsocketDomain(
-                "http://gpl-2732-ws-csrf.staging.gitpod.io",
-                HOSTURL_HOSTNAME,
-                true,
-            );
+            const result = isAllowedWebsocketDomain("http://gpl-2732-ws-csrf.staging.gitpod.io", HOSTURL_HOSTNAME);
             expect(result).to.be.true;
         });
     });
@@ -70,34 +58,24 @@ describe("express-util", function () {
             const result = isAllowedWebsocketDomain(
                 "https://8000-black-capybara-dy6e3fgz.ws-eu08.gitpod.io",
                 HOSTURL_HOSTNAME,
-                false,
             );
             expect(result).to.be.false;
         });
 
         it("should return true for workspace locations", function () {
-            const result = isAllowedWebsocketDomain(
-                "https://bronze-bird-p2q226d8.ws-eu08.gitpod.io",
-                HOSTURL_HOSTNAME,
-                false,
-            );
+            const result = isAllowedWebsocketDomain("https://bronze-bird-p2q226d8.ws-eu08.gitpod.io", HOSTURL_HOSTNAME);
             expect(result).to.be.true;
         });
         it("should return false for workspace-port locations (strict)", function () {
             const result = isAllowedWebsocketDomain(
                 "https://8000-black-capybara-dy6e3fgz.ws-eu08.gitpod.io",
                 HOSTURL_HOSTNAME,
-                true,
             );
             expect(result).to.be.false;
         });
 
         it("should return false for workspace locations (strict)", function () {
-            const result = isAllowedWebsocketDomain(
-                "https://bronze-bird-p2q226d8.ws-eu08.gitpod.io",
-                HOSTURL_HOSTNAME,
-                true,
-            );
+            const result = isAllowedWebsocketDomain("https://bronze-bird-p2q226d8.ws-eu08.gitpod.io", HOSTURL_HOSTNAME);
             expect(result).to.be.true;
         });
     });
diff --git a/components/server/src/express-util.ts b/components/server/src/express-util.ts
@@ -8,8 +8,6 @@ import { URL } from "url";
 import * as express from "express";
 import * as crypto from "crypto";
 import * as session from "express-session";
-import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
-import { GitpodHostUrl } from "@gitpod/gitpod-protocol/lib/util/gitpod-host-url";
 
 export const query = (...tuples: [string, string][]) => {
     if (tuples.length === 0) {
@@ -21,7 +19,7 @@ export const query = (...tuples: [string, string][]) => {
 // Strict: We only allow connections from the base domain, so disallow connections from all other Origins
 //      Only (current) exception: If no Origin header is set, skip the check!
 // Non-Strict: "rely" on subdomain parsing (do we still need this?)
-export const isAllowedWebsocketDomain = (originHeader: string, gitpodHostName: string, strict: boolean): boolean => {
+export const isAllowedWebsocketDomain = (originHeader: string, gitpodHostName: string): boolean => {
     if (!originHeader) {
         // Origin header check not applied because of empty Origin header
         return true;
@@ -30,43 +28,12 @@ export const isAllowedWebsocketDomain = (originHeader: string, gitpodHostName: s
     try {
         const originUrl = new URL(originHeader);
         const originHostname = originUrl.hostname;
-        if (originHostname === gitpodHostName) {
-            return true;
-        }
-
-        if (strict) {
-            return false;
-        }
-        // TODO(gpl) This is only used for Bearer-Token authentication.
-        // Does this restriction help at all, or can we remove it entirely?
-        log.warn("Origin header check based on looksLikeWorkspaceHostname");
-        if (looksLikeWorkspaceHostname(originUrl, gitpodHostName)) {
-            return true;
-        } else {
-            return false;
-        }
+        return originHostname === gitpodHostName;
     } catch (err) {
         return false;
     }
 };
 
-const looksLikeWorkspaceHostname = (originHostname: URL, gitpodHostName: string): boolean => {
-    // Is prefix a valid (looking) workspace ID?
-    const found = originHostname.toString().lastIndexOf(gitpodHostName);
-    if (found === -1) {
-        return false;
-    }
-    const url = new GitpodHostUrl(originHostname);
-    const workspaceId = url.workspaceId;
-    if (workspaceId) {
-        const hostname = url.url.hostname as string;
-        if (hostname.startsWith(workspaceId)) {
-            return true;
-        }
-    }
-    return false;
-};
-
 export function saveSession(session: session.Session): Promise<void> {
     return new Promise<void>((resolve, reject) => {
         session.save((err: any) => {
diff --git a/components/server/src/server.ts b/components/server/src/server.ts
@@ -153,13 +153,12 @@ export class Server<C extends GitpodClient, S extends GitpodServer> {
         const websocketConnectionHandler = this.websocketConnectionHandler;
         this.eventEmitter.on(Server.EVENT_ON_START, (httpServer) => {
             // CSRF protection: check "Origin" header:
-            //  - for cookie/session auth: MUST be gitpod.io (hostUrl.hostname)
-            //  - for Bearer auth: MUST be sth with the same base domain (*.gitpod.io) (is this required?)
-            //  - edge case: empty "Origin" is always permitted (can this be removed?)
+            //  - for cookie/session AND Bearer auth: MUST be hostUrl.hostname (gitpod.io)
+            //  - edge case: empty "Origin" is always permitted
             // We rely on the origin header being set correctly (needed by regular clients to use Gitpod:
             // CORS allows subdomains to access gitpod.io)
-            const verifyOrigin = (origin: string, strict: boolean) => {
-                let allowedRequest = isAllowedWebsocketDomain(origin, this.config.hostUrl.url.hostname, strict);
+            const verifyOrigin = (origin: string) => {
+                let allowedRequest = isAllowedWebsocketDomain(origin, this.config.hostUrl.url.hostname);
                 if (!allowedRequest && this.config.insecureNoDomain) {
                     log.warn("Websocket connection CSRF guard disabled");
                     allowedRequest = true;
@@ -174,7 +173,7 @@ export class Server<C extends GitpodClient, S extends GitpodServer> {
                 let authenticatedUsingBearerToken = false;
                 if (info.req.url === "/v1") {
                     // Connection attempt with Bearer-Token: be less strict for now
-                    if (!verifyOrigin(info.origin, false)) {
+                    if (!verifyOrigin(info.origin)) {
                         log.debug("Websocket connection attempt with non-matching Origin header.", {
                             origin: info.origin,
                         });
@@ -196,7 +195,7 @@ export class Server<C extends GitpodClient, S extends GitpodServer> {
 
                 if (!authenticatedUsingBearerToken) {
                     // Connection attempt with cookie/session based authentication: be strict about where we accept connections from!
-                    if (!verifyOrigin(info.origin, true)) {
+                    if (!verifyOrigin(info.origin)) {
                         log.debug("Websocket connection attempt with non-matching Origin header: " + info.origin);
                         return callback(false, 403);
                     }
