Skip to content

[server] verifyClient: Remove dead code #16845

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 15, 2023
Merged

[server] verifyClient: Remove dead code #16845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 4 additions & 26 deletions components/server/src/express-util.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
Expand All @@ -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;
});
Expand All @@ -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;
});
});
Expand All @@ -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;
});
});
Expand Down
37 changes: 2 additions & 35 deletions components/server/src/express-util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
Expand All @@ -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) => {
Expand Down
13 changes: 6 additions & 7 deletions components/server/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
});
Expand All @@ -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);
}
Expand Down