Skip to content

Commit bdd7fde

Browse files
fix(remix): Use domains to prevent scope bleed (getsentry#5570)
This patch wraps both the express and built-in request handlers with domains, preventing scope bleeding from happening. It also updates our express request handler to match that of the `withSentry` handler from NextJS. In a follow-up patch, we'll consolidate the implementations of both these handlers. To test this, we've added an integration test. Co-authored-by: Onur Temizkan <[email protected]>
1 parent 4e36d26 commit bdd7fde

File tree

4 files changed

+161
-30
lines changed

4 files changed

+161
-30
lines changed

packages/remix/src/utils/instrumentServer.ts

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
/* eslint-disable max-lines */
2-
import { captureException, getCurrentHub } from '@sentry/node';
3-
import { getActiveTransaction, hasTracingEnabled, Span } from '@sentry/tracing';
4-
import { WrappedFunction } from '@sentry/types';
2+
import { captureException, getCurrentHub, Hub } from '@sentry/node';
3+
import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing';
4+
import { Transaction, WrappedFunction } from '@sentry/types';
55
import { addExceptionMechanism, fill, isNodeEnv, loadModule, logger, serializeBaggage } from '@sentry/utils';
6+
import * as domain from 'domain';
67

78
import {
89
AppData,
@@ -291,9 +292,9 @@ export function startRequestHandlerTransaction(
291292
url: URL,
292293
method: string,
293294
routes: ServerRoute[],
295+
hub: Hub,
294296
pkg?: ReactRouterDomPkg,
295-
): Span | undefined {
296-
const hub = getCurrentHub();
297+
): Transaction {
297298
const currentScope = hub.getScope();
298299
const matches = matchServerRoutes(routes, url.pathname, pkg);
299300

@@ -311,24 +312,28 @@ export function startRequestHandlerTransaction(
311312
},
312313
});
313314

314-
if (transaction) {
315-
currentScope?.setSpan(transaction);
316-
}
315+
currentScope?.setSpan(transaction);
317316
return transaction;
318317
}
319318

320319
function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBuild): RequestHandler {
321320
const routes = createRoutes(build.routes);
322321
const pkg = loadModule<ReactRouterDomPkg>('react-router-dom');
323322
return async function (this: unknown, request: Request, loadContext?: unknown): Promise<Response> {
324-
const url = new URL(request.url);
323+
const hub = getCurrentHub();
324+
const options = hub.getClient()?.getOptions();
325325

326-
const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg);
326+
if (!options || !hasTracingEnabled(options)) {
327+
return origRequestHandler.call(this, request, loadContext);
328+
}
329+
330+
const url = new URL(request.url);
331+
const transaction = startRequestHandlerTransaction(url, request.method, routes, hub, pkg);
327332

328333
const res = (await origRequestHandler.call(this, request, loadContext)) as Response;
329334

330-
transaction?.setHttpStatus(res.status);
331-
transaction?.finish();
335+
transaction.setHttpStatus(res.status);
336+
transaction.finish();
332337

333338
return res;
334339
};
@@ -416,7 +421,8 @@ function makeWrappedCreateRequestHandler(
416421
const newBuild = instrumentBuild(build);
417422
const requestHandler = origCreateRequestHandler.call(this, newBuild, ...args);
418423

419-
return wrapRequestHandler(requestHandler, newBuild);
424+
const local = domain.create();
425+
return local.bind(() => wrapRequestHandler(requestHandler, newBuild))();
420426
};
421427
}
422428

packages/remix/src/utils/serverAdapters/express.ts

Lines changed: 84 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,9 @@
1-
import { extractRequestData, loadModule } from '@sentry/utils';
1+
import { getCurrentHub } from '@sentry/hub';
2+
import { flush } from '@sentry/node';
3+
import { hasTracingEnabled } from '@sentry/tracing';
4+
import { Transaction } from '@sentry/types';
5+
import { extractRequestData, loadModule, logger } from '@sentry/utils';
6+
import * as domain from 'domain';
27

38
import {
49
createRoutes,
@@ -35,20 +40,26 @@ function wrapExpressRequestHandler(
3540
res: ExpressResponse,
3641
next: ExpressNextFunction,
3742
): Promise<void> {
38-
const request = extractRequestData(req);
43+
// eslint-disable-next-line @typescript-eslint/unbound-method
44+
res.end = wrapEndMethod(res.end);
3945

40-
if (!request.url || !request.method) {
41-
return origRequestHandler.call(this, req, res, next);
42-
}
46+
const local = domain.create();
47+
local.add(req);
48+
local.add(res);
4349

44-
const url = new URL(request.url);
50+
local.run(async () => {
51+
const request = extractRequestData(req);
52+
const hub = getCurrentHub();
53+
const options = hub.getClient()?.getOptions();
4554

46-
const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg);
55+
if (!options || !hasTracingEnabled(options) || !request.url || !request.method) {
56+
return origRequestHandler.call(this, req, res, next);
57+
}
4758

48-
await origRequestHandler.call(this, req, res, next);
49-
50-
transaction?.setHttpStatus(res.statusCode);
51-
transaction?.finish();
59+
const url = new URL(request.url);
60+
startRequestHandlerTransaction(url, request.method, routes, hub, pkg);
61+
await origRequestHandler.call(this, req, res, next);
62+
});
5263
};
5364
}
5465

@@ -57,11 +68,73 @@ function wrapExpressRequestHandler(
5768
*/
5869
export function wrapExpressCreateRequestHandler(
5970
origCreateRequestHandler: ExpressCreateRequestHandler,
71+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
6072
): (options: any) => ExpressRequestHandler {
73+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
6174
return function (this: unknown, options: any): ExpressRequestHandler {
6275
const newBuild = instrumentBuild((options as ExpressCreateRequestHandlerOptions).build);
6376
const requestHandler = origCreateRequestHandler.call(this, { ...options, build: newBuild });
6477

6578
return wrapExpressRequestHandler(requestHandler, newBuild);
6679
};
6780
}
81+
82+
export type AugmentedExpressResponse = ExpressResponse & {
83+
__sentryTransaction?: Transaction;
84+
};
85+
86+
type ResponseEndMethod = AugmentedExpressResponse['end'];
87+
type WrappedResponseEndMethod = AugmentedExpressResponse['end'];
88+
89+
/**
90+
* Wrap `res.end()` so that it closes the transaction and flushes events before letting the request finish.
91+
*
92+
* Note: This wraps a sync method with an async method. While in general that's not a great idea in terms of keeping
93+
* things in the right order, in this case it's safe, because the native `.end()` actually *is* async, and its run
94+
* actually *is* awaited, just manually so (which reflects the fact that the core of the request/response code in Node
95+
* by far predates the introduction of `async`/`await`). When `.end()` is done, it emits the `prefinish` event, and
96+
* only once that fires does request processing continue. See
97+
* https://github.com/nodejs/node/commit/7c9b607048f13741173d397795bac37707405ba7.
98+
*
99+
* @param origEnd The original `res.end()` method
100+
* @returns The wrapped version
101+
*/
102+
function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod {
103+
return async function newEnd(this: AugmentedExpressResponse, ...args: unknown[]) {
104+
await finishSentryProcessing(this);
105+
106+
return origEnd.call(this, ...args);
107+
};
108+
}
109+
110+
/**
111+
* Close the open transaction (if any) and flush events to Sentry.
112+
*
113+
* @param res The outgoing response for this request, on which the transaction is stored
114+
*/
115+
async function finishSentryProcessing(res: AugmentedExpressResponse): Promise<void> {
116+
const { __sentryTransaction: transaction } = res;
117+
118+
if (transaction) {
119+
transaction.setHttpStatus(res.statusCode);
120+
121+
// Push `transaction.finish` to the next event loop so open spans have a better chance of finishing before the
122+
// transaction closes, and make sure to wait until that's done before flushing events
123+
await new Promise(resolve => {
124+
setImmediate(() => {
125+
transaction.finish();
126+
resolve();
127+
});
128+
});
129+
}
130+
131+
// Flush the event queue to ensure that events get sent to Sentry before the response is finished and the lambda
132+
// ends. If there was an error, rethrow it so that the normal exception-handling mechanisms can apply.
133+
try {
134+
__DEBUG_BUILD__ && logger.log('Flushing events...');
135+
await flush(2000);
136+
__DEBUG_BUILD__ && logger.log('Done flushing events');
137+
} catch (e) {
138+
__DEBUG_BUILD__ && logger.log('Error while flushing events:\n', e);
139+
}
140+
}
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import { json, LoaderFunction } from '@remix-run/node';
2+
3+
import * as Sentry from '@sentry/remix';
4+
5+
export const loader: LoaderFunction = async ({ params: { id } }) => {
6+
const timeTil = parseInt(id || '', 10) * 1000;
7+
await new Promise(resolve => setTimeout(resolve, 3000 - timeTil));
8+
Sentry.setTag(`tag${id}`, id);
9+
return json({ test: 'test' });
10+
};
11+
12+
export default function ScopeBleed() {
13+
return (
14+
<div>
15+
<h1>Hello</h1>
16+
</div>
17+
);
18+
}

packages/remix/test/integration/test/server/loader.test.ts

Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,12 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
1414
const config = await runServer(adapter);
1515
const url = `${config.url}/loader-json-response/-2`;
1616

17-
let [transaction, event] = await getMultipleEnvelopeRequest({ ...config, url }, { count: 2 });
17+
const envelopes = await getMultipleEnvelopeRequest({ ...config, url }, { count: 2 });
1818

19-
// The event envelope is returned before the transaction envelope when using express adapter.
20-
// We can update this when we merge the envelope filtering utility.
21-
adapter === 'express' && ([event, transaction] = [transaction, event]);
19+
const event = envelopes[0][2].type === 'transaction' ? envelopes[1][2] : envelopes[0][2];
20+
const transaction = envelopes[0][2].type === 'transaction' ? envelopes[0][2] : envelopes[1][2];
2221

23-
assertSentryTransaction(transaction[2], {
22+
assertSentryTransaction(transaction, {
2423
contexts: {
2524
trace: {
2625
status: 'internal_error',
@@ -31,7 +30,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
3130
},
3231
});
3332

34-
assertSentryEvent(event[2], {
33+
assertSentryEvent(event, {
3534
exception: {
3635
values: [
3736
{
@@ -136,4 +135,39 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
136135
},
137136
});
138137
});
138+
139+
it('makes sure scope does not bleed between requests', async () => {
140+
const { url, server, scope } = await runServer(adapter);
141+
142+
const envelopes = await Promise.all([
143+
getEnvelopeRequest({ url: `${url}/scope-bleed/1`, server, scope }, { endServer: false }),
144+
getEnvelopeRequest({ url: `${url}/scope-bleed/2`, server, scope }, { endServer: false }),
145+
getEnvelopeRequest({ url: `${url}/scope-bleed/3`, server, scope }, { endServer: false }),
146+
getEnvelopeRequest({ url: `${url}/scope-bleed/4`, server, scope }, { endServer: false }),
147+
]);
148+
149+
scope.persist(false);
150+
await new Promise(resolve => server.close(resolve));
151+
152+
assertSentryTransaction(envelopes[0][2], {
153+
tags: {
154+
tag4: '4',
155+
},
156+
});
157+
assertSentryTransaction(envelopes[1][2], {
158+
tags: {
159+
tag3: '3',
160+
},
161+
});
162+
assertSentryTransaction(envelopes[2][2], {
163+
tags: {
164+
tag2: '2',
165+
},
166+
});
167+
assertSentryTransaction(envelopes[3][2], {
168+
tags: {
169+
tag1: '1',
170+
},
171+
});
172+
});
139173
});

0 commit comments

Comments
 (0)