From 85bc305df23281ce57b833d74daf8f12915e627a Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 1 Oct 2024 21:39:32 +0200 Subject: [PATCH] Merge pull request #20750 from overleaf/jpa-issue-20743 [logger] sanitize sentry exception context GitOrigin-RevId: 7f141ad93fc8df807acad9bcf691027efe799150 --- libraries/logger/sentry-manager.js | 40 ++----- libraries/logger/serializers.js | 9 +- .../logger/test/unit/sentry-manager-tests.js | 110 ++++++++++++++++++ 3 files changed, 126 insertions(+), 33 deletions(-) diff --git a/libraries/logger/sentry-manager.js b/libraries/logger/sentry-manager.js index 07840dd9a5..c17616294e 100644 --- a/libraries/logger/sentry-manager.js +++ b/libraries/logger/sentry-manager.js @@ -1,4 +1,4 @@ -const OError = require('@overleaf/o-error') +const Serializers = require('./serializers') const RATE_LIMIT_MAX_ERRORS = 5 const RATE_LIMIT_INTERVAL_MS = 60000 @@ -42,7 +42,7 @@ class SentryManager { } // extract any error object - let error = attributes.err || attributes.error + let error = Serializers.err(attributes.err || attributes.error) // avoid reporting errors twice for (const key in attributes) { @@ -67,45 +67,21 @@ class SentryManager { const tags = {} const extra = {} for (const key in attributes) { - const value = attributes[key] + let value = attributes[key] + if (Serializers[key]) { + value = Serializers[key](value) + } if (key.match(/_id/) && typeof value === 'string') { tags[key] = value } extra[key] = value } - // capture req object if available - const { req } = attributes - if (req != null) { - extra.req = { - method: req.method, - url: req.originalUrl, - query: req.query, - headers: req.headers, - ip: req.ip, - } - } - - // recreate error objects that have been converted to a normal object - if (!(error instanceof Error) && typeof error === 'object') { - const newError = new Error(error.message) - for (const key of Object.keys(error || {})) { - const value = error[key] - newError[key] = value - } - error = newError - } - // OError integration - extra.info = OError.getFullInfo(error) + extra.info = error.info + delete error.info - // filter paths from the message to avoid duplicate errors in sentry - // (e.g. errors from `fs` methods which have a path attribute) try { - if (error.path) { - error.message = error.message.replace(` '${error.path}'`, '') - } - // send the error to sentry this.Sentry.captureException(error, { tags, extra, level }) diff --git a/libraries/logger/serializers.js b/libraries/logger/serializers.js index 341c17f4c6..d6831a0a89 100644 --- a/libraries/logger/serializers.js +++ b/libraries/logger/serializers.js @@ -4,13 +4,20 @@ function errSerializer(err) { if (!err) { return err } + let message = err.message + if (err.path) { + // filter paths from the message to avoid duplicate errors with different path in message + // (e.g. errors from `fs` methods which have a path attribute) + message = message.replace(` '${err.path}'`, '') + } return { - message: err.message, + message, name: err.name, stack: OError.getFullStack(err), info: OError.getFullInfo(err), code: err.code, signal: err.signal, + path: err.path, } } diff --git a/libraries/logger/test/unit/sentry-manager-tests.js b/libraries/logger/test/unit/sentry-manager-tests.js index 49ce978910..a53483d374 100644 --- a/libraries/logger/test/unit/sentry-manager-tests.js +++ b/libraries/logger/test/unit/sentry-manager-tests.js @@ -104,6 +104,116 @@ describe('SentryManager', function () { ) }) + it('should sanitize error', function () { + const err = { + name: 'CustomError', + message: 'hello', + _oErrorTags: [{ stack: 'here:1', info: { one: 1 } }], + stack: 'here:0', + info: { key: 'value' }, + code: 42, + signal: 9, + path: '/foo', + } + this.sentryManager.captureException({ err }, 'message', 'error') + const expectedErr = { + name: 'CustomError', + message: 'hello', + stack: 'here:0\nhere:1', + code: 42, + signal: 9, + path: '/foo', + } + expect(this.Sentry.captureException).to.have.been.calledWith( + sinon.match(expectedErr), + sinon.match({ + tags: sinon.match.any, + level: sinon.match.any, + extra: { + description: 'message', + info: sinon.match({ + one: 1, + key: 'value', + }), + }, + }) + ) + expect(this.Sentry.captureException.args[0][0]).to.deep.equal(expectedErr) + }) + it('should sanitize request', function () { + const req = { + ip: '1.2.3.4', + method: 'GET', + url: '/foo', + headers: { + referer: 'abc', + 'content-length': 1337, + 'user-agent': 'curl', + authorization: '42', + }, + } + this.sentryManager.captureException({ req }, 'message', 'error') + const expectedReq = { + remoteAddress: '1.2.3.4', + method: 'GET', + url: '/foo', + headers: { + referer: 'abc', + 'content-length': 1337, + 'user-agent': 'curl', + }, + } + expect(this.Sentry.captureException).to.have.been.calledWith( + sinon.match({ + message: 'message', + }), + sinon.match({ + tags: sinon.match.any, + level: sinon.match.any, + extra: { + info: sinon.match.any, + req: expectedReq, + }, + }) + ) + expect(this.Sentry.captureException.args[0][1].extra.req).to.deep.equal( + expectedReq + ) + }) + it('should sanitize response', function () { + const res = { + statusCode: 417, + body: Buffer.from('foo'), + getHeader(key) { + expect(key).to.be.oneOf(['content-length']) + if (key === 'content-length') return 1337 + }, + } + this.sentryManager.captureException({ res }, 'message', 'error') + const expectedRes = { + statusCode: 417, + headers: { + 'content-length': 1337, + }, + } + expect(this.Sentry.captureException).to.have.been.calledWith( + sinon.match({ + message: 'message', + }), + sinon.match({ + tags: sinon.match.any, + level: sinon.match.any, + extra: { + info: sinon.match.any, + res: expectedRes, + }, + }) + ) + expect(this.Sentry.captureException.args[0][1].extra.res).to.deep.equal( + expectedRes + ) + }) + describe('reportedToSentry', function () { it('should mark the error as reported to sentry', function () { const err = new Error()