Skip to content
Permalink
Browse files Browse the repository at this point in the history
[server] strict same site origin for /api/gitpod endpoint
  • Loading branch information
corneliusludmann committed Feb 23, 2023
1 parent 3c79f0c commit 1295698
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 20 deletions.
58 changes: 56 additions & 2 deletions components/server/src/express-util.spec.ts
Expand Up @@ -15,6 +15,7 @@ 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 @@ -23,12 +24,43 @@ 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);
const result = isAllowedWebsocketDomain(
"http://gpl-2732-ws-csrf.staging.gitpod.io",
HOSTURL_HOSTNAME,
false,
);
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;
});

it("should return true for workspace locations (strict)", 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,
);
expect(result).to.be.true;
});
});
Expand All @@ -38,12 +70,34 @@ 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);
const result = isAllowedWebsocketDomain(
"https://bronze-bird-p2q226d8.ws-eu08.gitpod.io",
HOSTURL_HOSTNAME,
false,
);
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,
);
expect(result).to.be.true;
});
});
Expand Down
25 changes: 16 additions & 9 deletions components/server/src/express-util.ts
Expand Up @@ -7,8 +7,9 @@
import { URL } from "url";
import * as express from "express";
import * as crypto from "crypto";
import { GitpodHostUrl } from "@gitpod/gitpod-protocol/lib/util/gitpod-host-url";
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 @@ -17,23 +18,29 @@ export const query = (...tuples: [string, string][]) => {
return "?" + tuples.map((t) => `${t[0]}=${encodeURIComponent(t[1])}`).join("&");
};

// We do not precise UUID parsing here, we just want to distinguish three cases:
// - the base domain
// - a frontend domain (workspace domain)
// - a workspace port domain
// We control all of those values and the base domain, so we don't need to much effort
export const isAllowedWebsocketDomain = (originHeader: any, gitpodHostName: string): boolean => {
// 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 => {
if (!originHeader || typeof originHeader !== "string") {
// TODO(gpl) Can we get rid of this dependency alltogether?
log.warn("Origin header check not applied because of empty Origin header!");
return false;
}

var originHostname = "";
try {
const originUrl = new URL(originHeader);
originHostname = originUrl.hostname;
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 {
Expand Down
33 changes: 24 additions & 9 deletions components/server/src/server.ts
Expand Up @@ -146,13 +146,14 @@ export class Server<C extends GitpodClient, S extends GitpodServer> {
// Websocket for client connection
const websocketConnectionHandler = this.websocketConnectionHandler;
this.eventEmitter.on(Server.EVENT_ON_START, (httpServer) => {
// CSRF protection: check "Origin" header, it must be either:
// - gitpod.io (hostUrl.hostname) or
// - a workspace location (ending of hostUrl.hostname)
// 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?)
// We rely on the origin header being set correctly (needed by regular clients to use Gitpod:
// CORS allows subdomains to access gitpod.io)
const verifyCSRF = (origin: string) => {
let allowedRequest = isAllowedWebsocketDomain(origin, this.config.hostUrl.url.hostname);
const verifyOrigin = (origin: string, strict: boolean) => {
let allowedRequest = isAllowedWebsocketDomain(origin, this.config.hostUrl.url.hostname, strict);
if (!allowedRequest && this.config.insecureNoDomain) {
log.warn("Websocket connection CSRF guard disabled");
allowedRequest = true;
Expand All @@ -164,21 +165,35 @@ export class Server<C extends GitpodClient, S extends GitpodServer> {
* Verify the web socket handshake request.
*/
const verifyClient: ws.VerifyClientCallbackAsync = async (info, callback) => {
if (!verifyCSRF(info.origin)) {
log.warn("Websocket connection attempt with non-matching Origin header: " + info.origin);
return callback(false, 403);
}
let authenticatedUsingBearerToken = false;
if (info.req.url === "/v1") {
// Connection attempt with Bearer-Token: be less strict for now
if (!verifyOrigin(info.origin, false)) {
log.warn("Websocket connection attempt with non-matching Origin header: " + info.origin);
return callback(false, 403);
}

try {
await this.bearerAuth.auth(info.req as express.Request);
authenticatedUsingBearerToken = true;
} catch (e) {
if (isBearerAuthError(e)) {
return callback(false, 401, e.message);
}
log.warn("authentication failed: ", e);
return callback(false, 500);
}
// intentional fall-through to cookie/session based authentication
}

if (!authenticatedUsingBearerToken) {
// Connection attempt with cookie/session based authentication: be strict about where we accept connections from!
if (!verifyOrigin(info.origin, true)) {
log.warn("Websocket connection attempt with non-matching Origin header: " + info.origin);
return callback(false, 403);
}
}

return callback(true);
};

Expand Down

0 comments on commit 1295698

Please sign in to comment.