Skip to content

Commit adf1f60

Browse files
dschomdschom
authored
Merge pull request #20090 from mozilla/FXA-13177
bug(auth): Fix polluted sentry captures
2 parents 2b04bd0 + 8185551 commit adf1f60

12 files changed

Lines changed: 221 additions & 315 deletions

File tree

‎libs/shared/otel/src/lib/config.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ export type TracingOpts = {
2525
url: string;
2626
concurrencyLimit: number;
2727
};
28+
sentry?: {
29+
enabled: boolean;
30+
};
2831
};
2932

3033
/** Default convict config for node tracing */

‎libs/shared/otel/src/lib/node-tracing.ts‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
checkServiceName,
1212
logType,
1313
} from './config';
14+
import { SentryContextManager } from '@sentry/node';
1415
import {
1516
NodeTracerProvider,
1617
TraceIdRatioBasedSampler,
@@ -109,10 +110,26 @@ export class NodeTracingInitializer {
109110
'@opentelemetry/instrumentation-aws-lambda': {
110111
enabled: false,
111112
},
113+
'@opentelemetry/instrumentation-http': {
114+
// Important! If Sentry is enabled, we want to rely on Sentry's http instrumentation!
115+
enabled: this.opts.sentry?.enabled ? false : true,
116+
},
112117
}),
113118
],
114119
});
115-
this.provider.register();
120+
// Use SentryContextManager so that Sentry's per-request isolation scopes
121+
// are properly forked for every api.context.with() call. Without this,
122+
// NodeTracerProvider.register() would install a plain
123+
// AsyncLocalStorageContextManager that doesn't understand Sentry's scope
124+
// forking, and Sentry.init()'s own setupOtel() would overwrite our
125+
// NodeTracerProvider (discarding the custom exporters).
126+
if (this.opts.sentry?.enabled) {
127+
this.provider.register({
128+
contextManager: new SentryContextManager(),
129+
});
130+
} else {
131+
this.provider.register();
132+
}
116133
}
117134

118135
public startSpan(name: string, action: () => void) {

‎libs/shared/sentry/src/lib/models/SentryConfigOpts.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ export type SentryConfigOpts = {
3434
* When using strings, partial matches will be filtered out. If you need to filter by exact match, use regex patterns instead */
3535
ignoreErrors?: (string | RegExp)[];
3636

37+
/** Let Sentry know that otel setup should be handled by the app. */
38+
skipOpenTelemetrySetup?: boolean;
39+
3740
/** When set to true, building a configuration will throw an error critical fields are missing. */
3841
strict?: boolean;
3942

‎packages/fxa-auth-server/lib/monitoring.js‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,11 @@ initMonitoring({
2121
...config.getProperties(),
2222
release: version,
2323
eventFilters: [filterSentryEvent],
24-
integrations: [Sentry.linkedErrorsIntegration({ key: 'jse_cause' })],
24+
integrations: [
25+
// Important! This fixes a ton of problems with our previous integration.
26+
Sentry.hapiIntegration(),
27+
Sentry.linkedErrorsIntegration({ key: 'jse_cause' }),
28+
],
2529
},
2630
});
2731

‎packages/fxa-auth-server/lib/routes/util.js‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@
44

55
'use strict';
66

7+
const Sentry = require('@sentry/node');
78
const isA = require('joi');
89
const random = require('../crypto/random');
910
const validators = require('./validators');
1011
const UTIL_DOCS = require('../../docs/swagger/util-api').default;
12+
const { AppError } = require('../../../../libs/accounts/errors/src');
1113

1214
const HEX_STRING = validators.HEX_STRING;
1315

@@ -67,5 +69,39 @@ module.exports = (log, config, redirectDomain) => {
6769
return h.redirect(config.contentServer.url + request.raw.req.url);
6870
},
6971
},
72+
{
73+
method: 'GET',
74+
path: '/boom',
75+
options: {},
76+
handler: async function (request, h) {
77+
if (config.sentry.env === 'local') {
78+
const traceId = Sentry.getActiveSpan().spanContext().traceId;
79+
console.log(`${traceId}: crumb ${new Date().toString()}`);
80+
await (async () => {
81+
await new Promise((r) => {
82+
setTimeout(() => {
83+
console.log(`${traceId}: crumb ${new Date().toString()}`);
84+
r();
85+
}, 100);
86+
});
87+
88+
// Throw fabricated error
89+
console.log(`${traceId}: crumb ${new Date().toString()}`);
90+
throw new AppError(
91+
{
92+
code: 500,
93+
error: 'BOOM',
94+
errno: AppError.ERRNO.UNEXPECTED_ERROR,
95+
message: 'Testing error capture',
96+
},
97+
{
98+
op: { foo: 'bar' },
99+
data: { foo: 'baz' },
100+
}
101+
);
102+
})();
103+
}
104+
},
105+
},
70106
];
71107
};

‎packages/fxa-auth-server/lib/sentry.js‎

Lines changed: 60 additions & 120 deletions
Original file line numberDiff line numberDiff line change
@@ -4,162 +4,102 @@
44

55
'use strict';
66

7-
const Hoek = require('@hapi/hoek');
87
const Sentry = require('@sentry/node');
9-
const verror = require('verror');
10-
const { ignoreErrors } = require('@fxa/accounts/errors');
118

129
const {
1310
formatMetadataValidationErrorMessage,
1411
reportValidationError,
1512
} = require('fxa-shared/sentry/report-validation-error');
1613

17-
function reportSentryMessage(message, captureContext) {
18-
Sentry.withScope((scope) => {
19-
scope.setExtra('report', true);
20-
21-
if (captureContext && typeof captureContext === 'object') {
22-
Hoek.merge(scope, captureContext);
23-
}
24-
25-
Sentry.captureMessage(message, captureContext);
26-
});
27-
}
28-
29-
function reportSentryError(err, request) {
30-
let exception = '';
31-
if (err && err.stack) {
32-
try {
33-
exception = err.stack.split('\n')[0];
34-
} catch (e) {
35-
// ignore bad stack frames
36-
}
37-
}
38-
39-
if (ignoreErrors(err)) {
40-
return;
41-
}
14+
// Anything with these keys containing these strings will be redacted.
15+
const SENSITIVE_KEY_TERMS = new Set(['auth', 'pw', 'kb', 'key']);
4216

43-
Sentry.withScope((scope) => {
44-
if (request) {
45-
scope.addEventProcessor((sentryEvent) => {
46-
// As of sentry v9, this should automatically happen by adding, Sentry.requestDataIntegration()
47-
// Leaving note here for historical context.
48-
// event.request = Sentry.extractRequestData(request);
49-
sentryEvent.level = 'error';
50-
return sentryEvent;
51-
});
52-
}
53-
54-
// Important! Set a flag so that we know this is an error captured
55-
// and reported by our code. Once we added tracing, we started seeing errors
56-
// propagate in other ways. By setting a breakpoint or adding a console.trace
57-
// in beforeSend, we can see that Sentry's internal libraries are picking up
58-
// and reporting errors too. This causes problems because:
59-
// - It means errors are double captured
60-
// - It means that extra error info added below won't be there are errors
61-
// where captured by this function.
62-
// - It means the duplicate error might have a different shape, and fool
63-
// our ignoreErrors() check.
64-
//
65-
// See the filterSentryEvent function to see how this flag is used.
66-
//
67-
scope.setExtra('report', true);
68-
scope.setExtra('exception', exception);
69-
// If additional data was added to the error, extract it.
70-
if (err.output && typeof err.output.payload === 'object') {
71-
const payload = err.output.payload;
72-
if (typeof payload.data === 'object') {
73-
scope.setContext('payload.data', payload.data);
74-
delete payload.data;
17+
function filterExtras(obj, depth = 0) {
18+
return Object.fromEntries(
19+
Object.entries(obj).map(([k, v]) => {
20+
const lower = k.toLowerCase();
21+
if ([...SENSITIVE_KEY_TERMS].some((term) => lower.includes(term))) {
22+
return [k, '[Filtered]'];
7523
}
76-
scope.setContext('payload', payload);
77-
}
78-
const cause = verror.cause(err);
79-
if (cause && cause.message) {
80-
const causeContext = {
81-
errorName: cause.name,
82-
reason: cause.reason,
83-
errorMessage: cause.message,
84-
};
85-
86-
// Poolee EndpointError's have a few other things and oddly don't include
87-
// a stack at all. We try and extract a bit more to reflect what actually
88-
// happened as 'socket hang up' is somewhat inaccurate when the remote server
89-
// throws a 500.
90-
const output = cause.output;
91-
if (output && output.payload) {
92-
for (const key of ['error', 'message', 'statusCode']) {
93-
causeContext[key] = output.payload[key];
24+
if (v && typeof v === 'object' && !Array.isArray(v)) {
25+
if (depth >= 4) {
26+
return [k, '[Filtered]'];
9427
}
28+
return [k, filterExtras(v, depth + 1)];
9529
}
96-
const attempt = cause.attempt;
97-
if (attempt) {
98-
causeContext.method = attempt.method;
99-
causeContext.path = attempt.path;
100-
}
101-
scope.setContext('cause', causeContext);
102-
}
30+
return [k, v];
31+
})
32+
);
33+
}
10334

104-
if (request) {
105-
// Merge the request scope into the temp scope
106-
Hoek.merge(scope, request.sentryScope);
107-
}
108-
Sentry.captureException(err);
109-
});
35+
function reportSentryMessage(message, captureContext) {
36+
Sentry.captureMessage(message, captureContext);
37+
}
38+
39+
function reportSentryError(err, request) {
40+
Sentry.captureException(err);
11041
}
11142

11243
async function configureSentry(server, config, processName = 'key_server') {
11344
if (config.sentry.dsn) {
114-
Sentry.getCurrentScope().setTag('process', processName);
45+
Sentry.getGlobalScope().setTag('process', processName);
11546

11647
if (!server) {
11748
return;
11849
}
119-
120-
// Attach a new Sentry scope to the request for breadcrumbs/tags/extras
12150
server.ext({
122-
type: 'onRequest',
51+
type: 'onPreHandler',
12352
method(request, h) {
124-
request.sentryScope = new Sentry.Scope();
125-
126-
/**
127-
// Make a transaction per request so we can get performance monitoring. There are
128-
// some limitations to this approach, and distributed tracing will be off due to
129-
// hapi's architecture.
130-
//
131-
// See https://github.com/getsentry/sentry-javascript/issues/2172 for more into. It
132-
// looks like there might be some other solutions that are more complex, but would work
133-
// with hapi and distributed tracing.
134-
//
135-
const transaction = Sentry.startInactiveSpan({
136-
op: 'auth-server',
137-
name: `${request.method.toUpperCase()} ${request.path}`,
138-
forceTransaction: true,
139-
// As of sentry v9, this should automatically happen by adding, Sentry.requestDataIntegration()
140-
// Leaving note here for historical context.
141-
// request: Sentry.extractRequestData(request.raw.req),
53+
// hapiIntegration() manages per-request isolation scopes via async context.
54+
// Set tags/extras directly on the current scope — no withIsolationScope
55+
// wrapper here, which would create a synchronous child scope that is
56+
// discarded before the async handler runs, causing breadcrumbs to leak
57+
// onto the global scope and accumulate across requests.
58+
const UNKNOWN = 'Unknown';
59+
Sentry.setTag('route', request.route.path);
60+
Sentry.setTag('method', request.method);
61+
Sentry.setUser({
62+
email:
63+
request.credentials?.email ||
64+
request.payload?.email ||
65+
request.params?.email ||
66+
UNKNOWN,
67+
geo: {
68+
city: request.app?.geo?.location?.city || UNKNOWN,
69+
country_code:
70+
request.app?.geo?.location?.countryCode ||
71+
request.app?.geo?.location?.country ||
72+
UNKNOWN,
73+
region:
74+
request.app?.geo?.location?.stateCode ||
75+
request.app?.geo?.location?.state ||
76+
UNKNOWN,
77+
},
78+
id: request.auth?.credentials?.uid || UNKNOWN,
79+
ip_address: request.app?.clientAddress || UNKNOWN,
14280
});
143-
144-
request.app.sentry = {
145-
transaction,
146-
};
147-
//*/
148-
81+
Sentry.setExtra('acceptLanguage', request.app?.acceptLanguage || {});
82+
Sentry.setExtra('request_payload', filterExtras(request.payload || {}));
83+
Sentry.setExtra('request_headers', filterExtras(request.headers || {}));
84+
Sentry.setExtra('request_params', filterExtras(request.params || {}));
14985
return h.continue;
15086
},
15187
});
15288

15389
server.events.on('request', (request, event, tags) => {
15490
if (event?.error && tags?.handler && tags?.error) {
155-
reportSentryError(event.error, request);
91+
Sentry.withScope((scope) => {
92+
scope.setExtra('hapi_event', event);
93+
Sentry.captureException(event.error);
94+
});
15695
}
15796
});
15897
}
15998
}
16099

161100
module.exports = {
162101
configureSentry,
102+
filterExtras,
163103
reportSentryMessage,
164104
reportSentryError,
165105
reportValidationError,

‎packages/fxa-auth-server/test/local/payments/stripe.js‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1683,7 +1683,6 @@ describe('#integration - StripeHelper', () => {
16831683
);
16841684

16851685
assert.equal(actual, expected);
1686-
sinon.assert.calledTwice(Sentry.withScope);
16871686
sinon.assert.calledOnceWithExactly(
16881687
sentryScope.setContext,
16891688
'validateCouponDurationForPlan',
@@ -1728,7 +1727,6 @@ describe('#integration - StripeHelper', () => {
17281727
);
17291728

17301729
assert.equal(actual, expected);
1731-
sinon.assert.calledTwice(Sentry.withScope);
17321730
sinon.assert.calledOnceWithExactly(
17331731
sentryScope.setContext,
17341732
'validateCouponDurationForPlan',
@@ -2052,7 +2050,6 @@ describe('#integration - StripeHelper', () => {
20522050
promotionCode: 'promo',
20532051
});
20542052
assert.deepEqual(actual, expected);
2055-
sinon.assert.calledTwice(Sentry.withScope);
20562053
sinon.assert.calledWithExactly(
20572054
sentryScope.setContext.getCall(0),
20582055
'retrieveCouponDetails',
@@ -2089,7 +2086,6 @@ describe('#integration - StripeHelper', () => {
20892086
promotionCode: 'promo',
20902087
});
20912088
assert.deepEqual(actual, expected);
2092-
sinon.assert.calledTwice(Sentry.withScope);
20932089
sinon.assert.calledOnceWithExactly(
20942090
sentryScope.setContext,
20952091
'retrieveCouponDetails',
@@ -7627,8 +7623,7 @@ describe('#integration - StripeHelper', () => {
76277623
'GD'
76287624
);
76297625
sinon.assert.notCalled(stripeHelper.updateCustomerBillingAddress);
7630-
sinon.assert.calledTwice(Sentry.withScope);
7631-
sinon.assert.calledOnceWithExactly(
7626+
sinon.assert.calledWithExactly(
76327627
sentryScope.setContext,
76337628
'setCustomerLocation',
76347629
{
@@ -7657,7 +7652,6 @@ describe('#integration - StripeHelper', () => {
76577652
stripeHelper.updateCustomerBillingAddress,
76587653
{ customerId: customer1.id, options: expectedAddressArg }
76597654
);
7660-
sinon.assert.calledTwice(Sentry.withScope);
76617655
sinon.assert.calledOnceWithExactly(
76627656
sentryScope.setContext,
76637657
'setCustomerLocation',

0 commit comments

Comments
 (0)