Skip to content

Commit 48136ea

Browse files
committed
Tighten the CSRF security, since Electron interceptor allows RCE
Previously the API was difficult to CSRF, with attacks that only worked in certain old browsers etc. With this change, it should be impossible to send a request to the GraphQL API that isn't from an allowed server. There is still a remaining risky avenue due to Mockttp's API (if you're clever, you could spoof a trusted origin directly), but that'll be closed the same way shortly.
1 parent 52ecbf7 commit 48136ea

File tree

6 files changed

+34
-19
lines changed

6 files changed

+34
-19
lines changed

custom-typings/cors-gate.d.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
declare module 'cors-gate' {
2+
import * as express from 'express';
3+
4+
interface Options {
5+
origin: string;
6+
strict?: boolean;
7+
allowSafe?: boolean;
8+
}
9+
10+
function corsGate(options: Options): express.RequestHandler;
11+
12+
export = corsGate;
13+
}

package-lock.json

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
"async-mutex": "^0.1.3",
3939
"chrome-remote-interface": "^0.28.0",
4040
"command-exists": "^1.2.8",
41+
"cors-gate": "^1.1.3",
4142
"env-paths": "^1.0.0",
4243
"global-agent": "^2.0.0",
4344
"global-tunnel-ng": "^2.7.1",

src/httptoolkit-server.ts

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import * as _ from 'lodash';
22
import * as os from 'os';
33
import * as events from 'events';
4+
import corsGate = require('cors-gate');
45
import { GraphQLServer } from 'graphql-yoga';
56
import * as Express from 'express';
67
import { GraphQLScalarType } from 'graphql';
@@ -171,21 +172,11 @@ export class HttpToolkitServer extends events.EventEmitter {
171172
resolvers: buildResolvers(config, interceptors, this)
172173
});
173174

174-
// TODO: This logic also exists in Mockttp - probably good to commonize it somewhere.
175-
this.graphql.use((req: Express.Request, res: Express.Response, next: () => void) => {
176-
const origin = req.headers['origin'];
177-
// This will have been set (or intentionally not set), by the CORS middleware
178-
const allowedOrigin = res.getHeader('Access-Control-Allow-Origin');
179-
180-
// If origin is set (null or an origin) but was not accepted by the CORS options
181-
// Note that if no options.cors is provided, allowedOrigin is always *.
182-
if (origin !== undefined && allowedOrigin !== '*' && allowedOrigin !== origin) {
183-
// Don't process the request: error out & skip the lot (to avoid CSRF)
184-
res.status(403).send('CORS request sent by unacceptable origin');
185-
} else {
186-
next();
187-
}
188-
});
175+
this.graphql.use(corsGate({
176+
strict: true, // MUST send an allowed origin
177+
allowSafe: false, // Even for HEAD/GET requests (should be none anyway)
178+
origin: '' // No origin - we accept *no* same-origin requests
179+
}));
189180
}
190181

191182
async start() {

src/util.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,7 @@ export const ALLOWED_ORIGINS = IS_PROD_BUILD
99
// Prod builds only allow HTTPS app.httptoolkit.tech usage. This
1010
// ensures that no other sites/apps can communicate with your server
1111
// whilst you have the app open. If they could (requires an HTTP mitm),
12-
// they would be able to start proxies & interceptors. It's not remote
13-
// execution, but it's definitely not desirable.
12+
// they would be able to start proxies & interceptors.
1413
/^https:\/\/app\.httptoolkit\.tech$/
1514
]
1615
: [

test/integration-test.spec.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,12 @@ async function setupServerPath() {
3939
return path.join(tmpDir, 'httptoolkit-server', 'bin', 'run');
4040
}
4141

42+
const buildGraphql = (url: string) => getGraphQL(url, {
43+
asJSON: true,
44+
// Pretend to be a browser on the real site:
45+
headers: { 'origin': 'https://app.httptoolkit.tech' }
46+
});
47+
4248
describe('Integration test', function () {
4349
// Timeout needs to be long, as first test runs (e.g. in CI) generate
4450
// fresh certificates, which can take a little while.
@@ -98,7 +104,7 @@ describe('Integration test', function () {
98104
});
99105

100106
it('exposes the version over HTTP', async () => {
101-
const graphql = getGraphQL('http://localhost:45457/', { asJSON: true });
107+
const graphql = buildGraphql('http://localhost:45457/');
102108

103109
const response = await graphql(`
104110
query getVersion {
@@ -110,7 +116,7 @@ describe('Integration test', function () {
110116
});
111117

112118
it('exposes interceptors over HTTP', async () => {
113-
const graphql = getGraphQL('http://localhost:45457/', { asJSON: true });
119+
const graphql = buildGraphql('http://localhost:45457/');
114120

115121
const response = await graphql(`
116122
query getInterceptors($proxyPort: Int!) {

0 commit comments

Comments
 (0)