Archives
/
gristlabs_grist-core
mirror of https://github.com/gristlabs/grist-core
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Comm: fix customHost origin handling

Browse Source
pull/859/head
Jonathan Perret 3 months ago
parent adcdd5f3f3
commit 80a1b3afbc
4 changed files with 57 additions and 24 deletions
Show Stats Download Patch File Download Diff File

19
app/server/lib/Comm.ts
Unescape View File

@ -202,13 +202,6 @@ export class Comm extends EventEmitter {
* Processes a new websocket connection, and associates the websocket and a Client object.
*/
private async _onWebSocketConnection(websocket: GristServerSocket, req: http.IncomingMessage) {
if (this._options.hosts) {
// DocWorker ID (/dw/) and version tag (/v/) may be present in this request but are not
// needed. addOrgInfo assumes req.url starts with /o/ if present.
req.url = parseFirstUrlPart('dw', req.url!).path;
req.url = parseFirstUrlPart('v', req.url).path;
await this._options.hosts.addOrgInfo(req);
}
// Parse the cookie in the request to get the sessionId.
const sessionId = this.sessions.getSessionIdFromRequest(req);
@ -253,7 +246,17 @@ export class Comm extends EventEmitter {
const wss = [];
for (const server of servers) {
const wssi = new GristSocketServer(server, {
verifyClient: (req) => trustOrigin(req)
verifyClient: async (req: http.IncomingMessage) => {
if (this._options.hosts) {
// DocWorker ID (/dw/) and version tag (/v/) may be present in this request but are not
// needed. addOrgInfo assumes req.url starts with /o/ if present.
req.url = parseFirstUrlPart('dw', req.url!).path;
req.url = parseFirstUrlPart('v', req.url).path;
await this._options.hosts.addOrgInfo(req);
}
return trustOrigin(req);
}
});
wssi.onconnection = async (websocket: GristServerSocket, req) => {

10
app/server/lib/GristSocketServer.ts
Unescape View File

@ -7,7 +7,7 @@ import * as net from 'net';
const MAX_PAYLOAD = 100e6;
export interface GristSocketServerOptions {
verifyClient?: (request: http.IncomingMessage) => boolean;
verifyClient?: (request: http.IncomingMessage) => Promise<boolean>;
}
export class GristSocketServer {
@ -64,8 +64,8 @@ export class GristSocketServer {
private _attach(server: http.Server) {
// Forward all WebSocket upgrade requests to WS
server.on('upgrade', (request, socket, head) => {
if (this._options?.verifyClient && !this._options.verifyClient(request)) {
server.on('upgrade', async (request, socket, head) => {
if (this._options?.verifyClient && !await this._options.verifyClient(request)) {
// Because we are handling an "upgrade" event, we don't have access to
// a "response" object, just the raw socket. We can still construct
// a well-formed HTTP error response.
@ -83,10 +83,10 @@ export class GristSocketServer {
// requests that are meant for the Engine.IO polling implementation.
const listeners = [...server.listeners("request")];
server.removeAllListeners("request");
server.on("request", (req, res) => {
server.on("request", async (req, res) => {
// Intercept requests that have transport=polling in their querystring
if (/[&?]transport=polling(&|$)/.test(req.url ?? '')) {
if (this._options?.verifyClient && !this._options.verifyClient(req)) {
if (this._options?.verifyClient && !await this._options.verifyClient(req)) {
res.writeHead(403).end();
return;
}

5
app/server/lib/requestUtils.ts
Unescape View File

@ -100,11 +100,10 @@ export function trustOrigin(req: IncomingMessage, resp?: Response): boolean {
// Returns whether req satisfies the given allowedHost. Unless req is to a custom domain, it is
// enough if only the base domains match. Differing ports are allowed, which helps in dev/testing.
export function allowHost(req: IncomingMessage, allowedHost: string|URL) {
const mreq = req as RequestWithOrg;
const proto = getEndUserProtocol(req);
const actualUrl = new URL(getOriginUrl(req));
const allowedUrl = (typeof allowedHost === 'string') ? new URL(`${proto}://${allowedHost}`) : allowedHost;
if (mreq.isCustomHost) {
if ((req as RequestWithOrg).isCustomHost) {
// For a request to a custom domain, the full hostname must match.
return actualUrl.hostname === allowedUrl.hostname;
} else {
@ -349,6 +348,8 @@ export function getEndUserProtocol(req: IncomingMessage) {
if (process.env.APP_HOME_URL) {
return new URL(process.env.APP_HOME_URL).protocol.replace(':', '');
}
// TODO we shouldn't blindly trust X-Forwarded-Proto. See the Express approach:
// https://expressjs.com/en/5x/api.html#trust.proxy.options.table
return req.headers["x-forwarded-proto"] || ((req.socket as TLSSocket).encrypted ? 'https' : 'http');
}

47
test/server/Comm.ts
Unescape View File

@ -21,10 +21,28 @@ import {Sessions} from 'app/server/lib/Sessions';
import {TcpForwarder} from 'test/server/tcpForwarder';
import * as testUtils from 'test/server/testUtils';
import * as session from '@gristlabs/express-session';
import { Hosts, RequestOrgInfo } from 'app/server/lib/extractOrg';
const SQLiteStore = require('@gristlabs/connect-sqlite3')(session);
promisifyAll(SQLiteStore.prototype);
// Just enough implementation of Hosts to be able to fake using a custom host.
class FakeHosts {
public isCustomHost = false;
public get asHosts() { return this as unknown as Hosts; }
public async addOrgInfo<T extends http.IncomingMessage>(req: T): Promise<T & RequestOrgInfo> {
return Object.assign(req, {
isCustomHost: this.isCustomHost,
org: "example",
url: req.url!
});
}
}
describe('Comm', function() {
testUtils.setTmpLogLevel(process.env.VERBOSE ? 'debug' : 'warn');
@ -34,6 +52,7 @@ describe('Comm', function() {
let server: http.Server;
let sessions: Sessions;
let fakeHosts: FakeHosts;
let comm: Comm|null = null;
const sandbox = sinon.createSandbox();
@ -51,7 +70,8 @@ describe('Comm', function() {
function startComm(methods: {[name: string]: ClientMethod}) {
server = http.createServer();
comm = new Comm(server, {sessions});
fakeHosts = new FakeHosts();
comm = new Comm(server, {sessions, hosts: fakeHosts.asHosts});
comm.registerMethods(methods);
return listenPromise(server.listen(0, 'localhost'));
}
@ -517,16 +537,16 @@ describe('Comm', function() {
await stopComm();
});
it('should allow only example.com', async () => {
async function checkOrigin(headers: { origin: string, host: string }, allowed: boolean) {
const promise = connect({ headers });
if (allowed) {
await assert.isFulfilled(promise, `${headers.host} should allow ${headers.origin}`);
} else {
await assert.isRejected(promise, /.*/, `${headers.host} should reject ${headers.origin}`);
}
async function checkOrigin(headers: { origin: string, host: string }, allowed: boolean) {
const promise = connect({ headers });
if (allowed) {
await assert.isFulfilled(promise, `${headers.host} should allow ${headers.origin}`);
} else {
await assert.isRejected(promise, /.*/, `${headers.host} should reject ${headers.origin}`);
}
}
it('origin should match base domain of host', async () => {
await checkOrigin({origin: "https://www.toto.com", host: "worker.example.com"}, false);
await checkOrigin({origin: "https://badexample.com", host: "worker.example.com"}, false);
await checkOrigin({origin: "https://bad.com/example.com", host: "worker.example.com"}, false);
@ -534,6 +554,15 @@ describe('Comm', function() {
await checkOrigin({origin: "https://front.example.com:3000", host: "worker.example.com"}, true);
await checkOrigin({origin: "https://example.com", host: "example.com"}, true);
});
it('with custom domains, origin should match the full hostname', async () => {
fakeHosts.isCustomHost = true;
// For a request to a custom domain, the full hostname must match.
await checkOrigin({origin: "https://front.example.com", host: "worker.example.com"}, false);
await checkOrigin({origin: "https://front.example.com", host: "front.example.com"}, true);
await checkOrigin({origin: "https://front.example.com:3000", host: "front.example.com"}, true);
});
});
});

Write Preview
Loading…
Cancel
Save