diff --git a/components/server/src/express-util.spec.ts b/components/server/src/express-util.spec.ts index 4d660ce2d78cba..8604d1587ace25 100644 --- 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 index 7fe3bb6a1fff91..9a495618d17444 100644 --- 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 { return new Promise((resolve, reject) => { session.save((err: any) => { diff --git a/components/server/src/server.ts b/components/server/src/server.ts index a33defce129cb0..d4b7e374ea0724 100644 --- a/components/server/src/server.ts +++ b/components/server/src/server.ts @@ -153,13 +153,12 @@ export class Server { 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 { 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 { 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); }