From ee01adcea22703a1b06814fbd4469af7a65b7bb3 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 25 Mar 2022 11:02:31 +0000 Subject: [PATCH 01/19] initial draft for v7 node transports --- packages/node/src/transports/index.ts | 1 + packages/node/src/transports/new.ts | 151 ++++++++++++++++++++++++++ 2 files changed, 152 insertions(+) create mode 100644 packages/node/src/transports/new.ts diff --git a/packages/node/src/transports/index.ts b/packages/node/src/transports/index.ts index f48517061c2f..267fadd41e74 100644 --- a/packages/node/src/transports/index.ts +++ b/packages/node/src/transports/index.ts @@ -1,3 +1,4 @@ export { BaseTransport } from './base'; export { HTTPTransport } from './http'; export { HTTPSTransport } from './https'; +export { makeNewHttpTransport, makeNewHttpsTransport } from './new'; diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts new file mode 100644 index 000000000000..00fa19931b83 --- /dev/null +++ b/packages/node/src/transports/new.ts @@ -0,0 +1,151 @@ +import { + BaseTransportOptions, + createTransport, + NewTransport, + TransportMakeRequestResponse, + TransportRequest, +} from '@sentry/core'; +import { TransportRequestExecutor } from '@sentry/core/dist/transports/base'; +import { eventStatusFromHttpCode } from '@sentry/utils'; +import * as fs from 'fs'; +import * as http from 'http'; +import * as https from 'https'; + +import { HTTPModule, HTTPModuleClientRequest } from './base/http-module'; + +interface HttpTransportOptions extends BaseTransportOptions { + // Todo: doc + headers?: Record; + // TODO: doc Set a HTTP proxy that should be used for outbound requests. + proxy?: string; + // TODO: doc HTTPS proxy certificates path + caCerts?: string; + // Todo: doc + httpModule?: HTTPModule; +} + +// TODO(v7): +// - Rename this file "transports.ts" +// - Move this file one folder upwards +// - Delete "transports" folder + +/** + * TODO Doc + */ +export function makeNewHttpTransport(options: HttpTransportOptions): NewTransport { + // Proxy prioritization: http => `options.proxy` | `process.env.http_proxy` + const proxy = filterNoProxy(options.url, options.proxy || process.env.http_proxy); + + const httpModule = options.httpModule ?? http; + + const agent = proxy + ? (new (require('https-proxy-agent'))(proxy) as http.Agent) + : new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 }); + + const requestExecutor = createRequestExecutor(options, httpModule, agent); + return createTransport({ bufferSize: options.bufferSize }, requestExecutor); +} + +/** + * TODO Doc + */ +export function makeNewHttpsTransport(options: HttpTransportOptions): NewTransport { + // Proxy prioritization: https => `options.proxy` | `process.env.https_proxy` | `process.env.http_proxy` + const proxy = filterNoProxy(options.url, options.proxy || process.env.https_proxy || process.env.http_proxy); + + const httpsModule = options.httpModule ?? https; + + const agent = proxy + ? (new (require('https-proxy-agent'))(proxy) as https.Agent) + : new https.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 }); + + const requestExecutor = createRequestExecutor(options, httpsModule, agent); + return createTransport({ bufferSize: options.bufferSize }, requestExecutor); +} + +/** + * Honors the `no_proxy` env variable with the highest priority to allow for hosts exclusion. + * + * @param transportUrl The URL the transport intends to send events to. + * @param proxy The client configured proxy. + * @returns A proxy the transport should use. + */ +function filterNoProxy(transportUrl: string, proxy: string | undefined): string | undefined { + const { no_proxy } = process.env; + + const urlIsExemptFromProxy = no_proxy && no_proxy.split(',').some(exemption => transportUrl.endsWith(exemption)); + + if (urlIsExemptFromProxy) { + return undefined; + } else { + return proxy; + } +} + +/** + * TODO Doc + */ +function createRequestExecutor( + options: HttpTransportOptions, + httpModule: HTTPModule, + agent: http.Agent, +): TransportRequestExecutor { + const { hostname, pathname, port, protocol } = new URL(options.url); + + // This function is extracted because we want to keep the actual `makeRequest` function as light-weight as possible + function performHttpRequest( + callback: (transportMakeRequestResponse: TransportMakeRequestResponse) => void, + ): HTTPModuleClientRequest { + return httpModule.request( + { + method: 'POST', + agent, + headers: options.headers, + hostname, + pathname, + port, + protocol, + ca: options.caCerts ? fs.readFileSync(options.caCerts) : undefined, + }, + res => { + res.on('data', () => { + // Drain socket + }); + + res.on('end', () => { + // Drain socket + }); + + const statusCode = res.statusCode ?? 500; + const status = eventStatusFromHttpCode(statusCode); + + res.setEncoding('utf8'); + + /** + * "Key-value pairs of header names and values. Header names are lower-cased." + * https://nodejs.org/api/http.html#http_message_headers + */ + const retryAfterHeader = res.headers['retry-after'] ?? null; + let rateLimitsHeader = res.headers['x-sentry-rate-limits'] ?? null; + rateLimitsHeader = Array.isArray(rateLimitsHeader) ? rateLimitsHeader[0] : rateLimitsHeader; + + callback({ + headers: { + 'x-sentry-rate-limits': rateLimitsHeader, + 'retry-after': retryAfterHeader, + }, + reason: status, + statusCode: statusCode, + }); + }, + ); + } + + return function makeRequest(request: TransportRequest): Promise { + return new Promise((resolve, reject) => { + const req = performHttpRequest(resolve); + req.on('error', reject); + req.end(request.body); + }); + }; +} From 7bc69757fb4e0a0b720191861756fe6f37f73734 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 25 Mar 2022 14:19:33 +0000 Subject: [PATCH 02/19] move rate limits header --- packages/node/src/transports/new.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index 00fa19931b83..70986f2fb72b 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -126,13 +126,12 @@ function createRequestExecutor( * https://nodejs.org/api/http.html#http_message_headers */ const retryAfterHeader = res.headers['retry-after'] ?? null; - let rateLimitsHeader = res.headers['x-sentry-rate-limits'] ?? null; - rateLimitsHeader = Array.isArray(rateLimitsHeader) ? rateLimitsHeader[0] : rateLimitsHeader; + const rateLimitsHeader = res.headers['x-sentry-rate-limits'] ?? null; callback({ headers: { - 'x-sentry-rate-limits': rateLimitsHeader, 'retry-after': retryAfterHeader, + 'x-sentry-rate-limits': Array.isArray(rateLimitsHeader) ? rateLimitsHeader[0] : rateLimitsHeader, }, reason: status, statusCode: statusCode, From 40d37d732e5eb8216b27d17d0224483f79ef55f4 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 25 Mar 2022 14:20:37 +0000 Subject: [PATCH 03/19] Update todo --- packages/node/src/transports/new.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index 70986f2fb72b..76359c391151 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -28,6 +28,8 @@ interface HttpTransportOptions extends BaseTransportOptions { // - Rename this file "transports.ts" // - Move this file one folder upwards // - Delete "transports" folder +// OR +// - Split this file up and leave it in the transports folder /** * TODO Doc From 6019f60b0ee56f926c761fc14adc33416de97773 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 25 Mar 2022 14:24:39 +0000 Subject: [PATCH 04/19] Simplify request --- packages/node/src/transports/new.ts | 90 +++++++++++++---------------- 1 file changed, 41 insertions(+), 49 deletions(-) diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index 76359c391151..ff3a788f4d78 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -94,57 +94,49 @@ function createRequestExecutor( ): TransportRequestExecutor { const { hostname, pathname, port, protocol } = new URL(options.url); - // This function is extracted because we want to keep the actual `makeRequest` function as light-weight as possible - function performHttpRequest( - callback: (transportMakeRequestResponse: TransportMakeRequestResponse) => void, - ): HTTPModuleClientRequest { - return httpModule.request( - { - method: 'POST', - agent, - headers: options.headers, - hostname, - pathname, - port, - protocol, - ca: options.caCerts ? fs.readFileSync(options.caCerts) : undefined, - }, - res => { - res.on('data', () => { - // Drain socket - }); - - res.on('end', () => { - // Drain socket - }); - - const statusCode = res.statusCode ?? 500; - const status = eventStatusFromHttpCode(statusCode); - - res.setEncoding('utf8'); - - /** - * "Key-value pairs of header names and values. Header names are lower-cased." - * https://nodejs.org/api/http.html#http_message_headers - */ - const retryAfterHeader = res.headers['retry-after'] ?? null; - const rateLimitsHeader = res.headers['x-sentry-rate-limits'] ?? null; - - callback({ - headers: { - 'retry-after': retryAfterHeader, - 'x-sentry-rate-limits': Array.isArray(rateLimitsHeader) ? rateLimitsHeader[0] : rateLimitsHeader, - }, - reason: status, - statusCode: statusCode, - }); - }, - ); - } - return function makeRequest(request: TransportRequest): Promise { return new Promise((resolve, reject) => { - const req = performHttpRequest(resolve); + const req = httpModule.request( + { + method: 'POST', + agent, + headers: options.headers, + hostname, + pathname, + port, + protocol, + ca: options.caCerts ? fs.readFileSync(options.caCerts) : undefined, + }, + res => { + res.on('data', () => { + // Drain socket + }); + + res.on('end', () => { + // Drain socket + }); + + const statusCode = res.statusCode ?? 500; + const status = eventStatusFromHttpCode(statusCode); + + res.setEncoding('utf8'); + + // "Key-value pairs of header names and values. Header names are lower-cased." + // https://nodejs.org/api/http.html#http_message_headers + const retryAfterHeader = res.headers['retry-after'] ?? null; + const rateLimitsHeader = res.headers['x-sentry-rate-limits'] ?? null; + + resolve({ + headers: { + 'retry-after': retryAfterHeader, + 'x-sentry-rate-limits': Array.isArray(rateLimitsHeader) ? rateLimitsHeader[0] : rateLimitsHeader, + }, + reason: status, + statusCode: statusCode, + }); + }, + ); + req.on('error', reject); req.end(request.body); }); From b1d6a6000d0855d31853971455e2eb7761ab3a75 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 25 Mar 2022 14:54:02 +0000 Subject: [PATCH 05/19] Add first few test cases --- packages/node/src/transports/new.ts | 2 +- .../node/test/transports/new/http.test.ts | 235 ++++++++++++++++++ 2 files changed, 236 insertions(+), 1 deletion(-) create mode 100644 packages/node/test/transports/new/http.test.ts diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index ff3a788f4d78..359683f20c2e 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -11,7 +11,7 @@ import * as fs from 'fs'; import * as http from 'http'; import * as https from 'https'; -import { HTTPModule, HTTPModuleClientRequest } from './base/http-module'; +import { HTTPModule } from './base/http-module'; interface HttpTransportOptions extends BaseTransportOptions { // Todo: doc diff --git a/packages/node/test/transports/new/http.test.ts b/packages/node/test/transports/new/http.test.ts new file mode 100644 index 000000000000..10ce3201f061 --- /dev/null +++ b/packages/node/test/transports/new/http.test.ts @@ -0,0 +1,235 @@ +import { createTransport } from '@sentry/core'; +import { EventEnvelope, EventItem } from '@sentry/types'; +import { createEnvelope, serializeEnvelope } from '@sentry/utils'; +import * as http from 'http'; + +import { makeNewHttpTransport } from '../../../src/transports/new'; + +jest.mock('@sentry/core', () => { + const actualCore = jest.requireActual('@sentry/core'); + return { + ...actualCore, + createTransport: jest.fn().mockImplementation(actualCore.createTransport), + }; +}); + +const SUCCESS = 200; +const RATE_LIMIT = 429; +const INVALID = 400; +const FAILED = 500; + +interface TestServerOptions { + statusCode: number; + responseHeaders: Record; +} + +let testServer: http.Server | undefined; + +function setupTestServer( + options: TestServerOptions, + requestInspector?: (req: http.IncomingMessage, body: string) => void, +) { + testServer = http.createServer((req, res) => { + let body = ''; + + req.on('data', data => { + body += data; + }); + + req.on('end', () => { + requestInspector?.(req, body); + }); + + res.writeHead(options.statusCode, options.responseHeaders); + res.end(); + }); + + testServer.listen(12345); + + return new Promise(resolve => { + testServer?.on('listening', resolve); + }); +} + +const testServerUrl = 'http://localhost:12345'; + +const EVENT_ENVELOPE = createEnvelope({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }, [ + [{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem, +]); + +const SERIALIZED_EVENT_ENVELOPE = serializeEnvelope(EVENT_ENVELOPE); + +describe('makeNewHttpTransport()', () => { + afterEach(() => { + jest.clearAllMocks(); + + if (testServer) { + testServer.close(); + } + }); + + describe('.send()', () => { + it('should correctly return successful server response', async () => { + await setupTestServer({ statusCode: SUCCESS, responseHeaders: {} }); + + const transport = makeNewHttpTransport({ url: testServerUrl }); + const transportResponse = await transport.send(EVENT_ENVELOPE); + + expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); + }); + + it('should correctly send envelope to server', async () => { + await setupTestServer({ statusCode: SUCCESS, responseHeaders: {} }, (req, body) => { + expect(req.method).toBe('POST'); + expect(body).toBe(SERIALIZED_EVENT_ENVELOPE); + }); + + const transport = makeNewHttpTransport({ url: testServerUrl }); + await transport.send(EVENT_ENVELOPE); + }); + + it('should correctly send user-provided headers to server', async () => { + await setupTestServer({ statusCode: SUCCESS, responseHeaders: {} }, req => { + expect(req.headers).toEqual( + expect.objectContaining({ + // node http module lower-cases incoming headers + 'x-some-custom-header-1': 'value1', + 'x-some-custom-header-2': 'value2', + }), + ); + }); + + const transport = makeNewHttpTransport({ + url: testServerUrl, + headers: { + 'X-Some-Custom-Header-1': 'value1', + 'X-Some-Custom-Header-2': 'value2', + }, + }); + + await transport.send(EVENT_ENVELOPE); + }); + + it.each([ + [RATE_LIMIT, 'rate_limit'], + [INVALID, 'invalid'], + [FAILED, 'failed'], + ])('should correctly reject bad server response (status %i)', async (serverStatusCode, expectedStatus) => { + await setupTestServer({ statusCode: serverStatusCode, responseHeaders: {} }); + + const transport = makeNewHttpTransport({ url: testServerUrl }); + await expect(transport.send(EVENT_ENVELOPE)).rejects.toEqual(expect.objectContaining({ status: expectedStatus })); + }); + + it('should resolve when server responds with rate limit header and status code 200', async () => { + await setupTestServer({ + statusCode: SUCCESS, + responseHeaders: { + 'Retry-After': '2700', + 'X-Sentry-Rate-Limits': '60::organization, 2700::organization', + }, + }); + + const transport = makeNewHttpTransport({ url: testServerUrl }); + const transportResponse = await transport.send(EVENT_ENVELOPE); + + expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); + }); + + it('should resolve when server responds with rate limit header and status code 200', async () => { + await setupTestServer({ + statusCode: SUCCESS, + responseHeaders: { + 'Retry-After': '2700', + 'X-Sentry-Rate-Limits': '60::organization, 2700::organization', + }, + }); + + const transport = makeNewHttpTransport({ url: testServerUrl }); + const transportResponse = await transport.send(EVENT_ENVELOPE); + + expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); + }); + }); + + it('should register TransportRequestExecutor that returns the correct object from server response (rate limit)', async () => { + await setupTestServer({ + statusCode: RATE_LIMIT, + responseHeaders: { + 'Retry-After': '2700', + 'X-Sentry-Rate-Limits': '60::organization, 2700::organization', + }, + }); + + makeNewHttpTransport({ url: testServerUrl }); + const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; + + const executorResult = registeredRequestExecutor({ + body: serializeEnvelope(EVENT_ENVELOPE), + category: 'error', + }); + + await expect(executorResult).resolves.toEqual( + expect.objectContaining({ + headers: { + 'retry-after': '2700', + 'x-sentry-rate-limits': '60::organization, 2700::organization', + }, + statusCode: RATE_LIMIT, + }), + ); + }); + + it('should register TransportRequestExecutor that returns the correct object from server response (success)', async () => { + await setupTestServer({ + statusCode: SUCCESS, + responseHeaders: {}, + }); + + makeNewHttpTransport({ url: testServerUrl }); + const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; + + const executorResult = registeredRequestExecutor({ + body: serializeEnvelope(EVENT_ENVELOPE), + category: 'error', + }); + + await expect(executorResult).resolves.toEqual( + expect.objectContaining({ + headers: { + 'retry-after': null, + 'x-sentry-rate-limits': null, + }, + statusCode: SUCCESS, + }), + ); + }); + + it('should register TransportRequestExecutor that returns the correct object from server response (success but rate-limit)', async () => { + await setupTestServer({ + statusCode: SUCCESS, + responseHeaders: { + 'Retry-After': '2700', + 'X-Sentry-Rate-Limits': '60::organization, 2700::organization', + }, + }); + + makeNewHttpTransport({ url: testServerUrl }); + const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; + + const executorResult = registeredRequestExecutor({ + body: serializeEnvelope(EVENT_ENVELOPE), + category: 'error', + }); + + await expect(executorResult).resolves.toEqual( + expect.objectContaining({ + headers: { + 'retry-after': '2700', + 'x-sentry-rate-limits': '60::organization, 2700::organization', + }, + statusCode: SUCCESS, + }), + ); + }); +}); From 5b9cfc67c4e266a34f8feff800946f6776868b11 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 28 Mar 2022 08:11:45 +0000 Subject: [PATCH 06/19] Fix behaviour of no_proxy env var --- packages/node/src/transports/new.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index 359683f20c2e..de807b79b760 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -36,7 +36,7 @@ interface HttpTransportOptions extends BaseTransportOptions { */ export function makeNewHttpTransport(options: HttpTransportOptions): NewTransport { // Proxy prioritization: http => `options.proxy` | `process.env.http_proxy` - const proxy = filterNoProxy(options.url, options.proxy || process.env.http_proxy); + const proxy = applyNoProxyOption(options.url, options.proxy || process.env.http_proxy); const httpModule = options.httpModule ?? http; @@ -53,7 +53,7 @@ export function makeNewHttpTransport(options: HttpTransportOptions): NewTranspor */ export function makeNewHttpsTransport(options: HttpTransportOptions): NewTransport { // Proxy prioritization: https => `options.proxy` | `process.env.https_proxy` | `process.env.http_proxy` - const proxy = filterNoProxy(options.url, options.proxy || process.env.https_proxy || process.env.http_proxy); + const proxy = applyNoProxyOption(options.url, options.proxy || process.env.https_proxy || process.env.http_proxy); const httpsModule = options.httpModule ?? https; @@ -72,10 +72,16 @@ export function makeNewHttpsTransport(options: HttpTransportOptions): NewTranspo * @param proxy The client configured proxy. * @returns A proxy the transport should use. */ -function filterNoProxy(transportUrl: string, proxy: string | undefined): string | undefined { +function applyNoProxyOption(transportUrl: string, proxy: string | undefined): string | undefined { const { no_proxy } = process.env; - const urlIsExemptFromProxy = no_proxy && no_proxy.split(',').some(exemption => transportUrl.endsWith(exemption)); + const { host: transportUrlHost, hostname: transportUrlHostname } = new URL(transportUrl); + + const urlIsExemptFromProxy = + no_proxy && + no_proxy + .split(',') + .some(exemption => transportUrlHost.endsWith(exemption) || transportUrlHostname.endsWith(exemption)); if (urlIsExemptFromProxy) { return undefined; From 0f312ceb7f4ad23c6079ba8ad22feedf71569c6f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 28 Mar 2022 09:10:30 +0000 Subject: [PATCH 07/19] Finalize tests --- .../node/test/transports/new/http.test.ts | 146 ++++++- .../node/test/transports/new/https.test.ts | 379 ++++++++++++++++++ .../test/transports/new/test-server-certs.ts | 48 +++ 3 files changed, 554 insertions(+), 19 deletions(-) create mode 100644 packages/node/test/transports/new/https.test.ts create mode 100644 packages/node/test/transports/new/test-server-certs.ts diff --git a/packages/node/test/transports/new/http.test.ts b/packages/node/test/transports/new/http.test.ts index 10ce3201f061..8c88ceb456cf 100644 --- a/packages/node/test/transports/new/http.test.ts +++ b/packages/node/test/transports/new/http.test.ts @@ -13,6 +13,12 @@ jest.mock('@sentry/core', () => { }; }); +// eslint-disable-next-line @typescript-eslint/no-var-requires +const httpProxyAgent = require('https-proxy-agent'); +jest.mock('https-proxy-agent', () => { + return jest.fn().mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 })); +}); + const SUCCESS = 200; const RATE_LIMIT = 429; const INVALID = 400; @@ -20,7 +26,7 @@ const FAILED = 500; interface TestServerOptions { statusCode: number; - responseHeaders: Record; + responseHeaders?: Record; } let testServer: http.Server | undefined; @@ -44,14 +50,14 @@ function setupTestServer( res.end(); }); - testServer.listen(12345); + testServer.listen(8099); return new Promise(resolve => { testServer?.on('listening', resolve); }); } -const testServerUrl = 'http://localhost:12345'; +const TEST_SERVER_URL = 'http://localhost:8099'; const EVENT_ENVELOPE = createEnvelope({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }, [ [{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem, @@ -70,26 +76,26 @@ describe('makeNewHttpTransport()', () => { describe('.send()', () => { it('should correctly return successful server response', async () => { - await setupTestServer({ statusCode: SUCCESS, responseHeaders: {} }); + await setupTestServer({ statusCode: SUCCESS }); - const transport = makeNewHttpTransport({ url: testServerUrl }); + const transport = makeNewHttpTransport({ url: TEST_SERVER_URL }); const transportResponse = await transport.send(EVENT_ENVELOPE); expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); }); it('should correctly send envelope to server', async () => { - await setupTestServer({ statusCode: SUCCESS, responseHeaders: {} }, (req, body) => { + await setupTestServer({ statusCode: SUCCESS }, (req, body) => { expect(req.method).toBe('POST'); expect(body).toBe(SERIALIZED_EVENT_ENVELOPE); }); - const transport = makeNewHttpTransport({ url: testServerUrl }); + const transport = makeNewHttpTransport({ url: TEST_SERVER_URL }); await transport.send(EVENT_ENVELOPE); }); it('should correctly send user-provided headers to server', async () => { - await setupTestServer({ statusCode: SUCCESS, responseHeaders: {} }, req => { + await setupTestServer({ statusCode: SUCCESS }, req => { expect(req.headers).toEqual( expect.objectContaining({ // node http module lower-cases incoming headers @@ -100,7 +106,7 @@ describe('makeNewHttpTransport()', () => { }); const transport = makeNewHttpTransport({ - url: testServerUrl, + url: TEST_SERVER_URL, headers: { 'X-Some-Custom-Header-1': 'value1', 'X-Some-Custom-Header-2': 'value2', @@ -115,9 +121,9 @@ describe('makeNewHttpTransport()', () => { [INVALID, 'invalid'], [FAILED, 'failed'], ])('should correctly reject bad server response (status %i)', async (serverStatusCode, expectedStatus) => { - await setupTestServer({ statusCode: serverStatusCode, responseHeaders: {} }); + await setupTestServer({ statusCode: serverStatusCode }); - const transport = makeNewHttpTransport({ url: testServerUrl }); + const transport = makeNewHttpTransport({ url: TEST_SERVER_URL }); await expect(transport.send(EVENT_ENVELOPE)).rejects.toEqual(expect.objectContaining({ status: expectedStatus })); }); @@ -130,7 +136,7 @@ describe('makeNewHttpTransport()', () => { }, }); - const transport = makeNewHttpTransport({ url: testServerUrl }); + const transport = makeNewHttpTransport({ url: TEST_SERVER_URL }); const transportResponse = await transport.send(EVENT_ENVELOPE); expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); @@ -145,13 +151,88 @@ describe('makeNewHttpTransport()', () => { }, }); - const transport = makeNewHttpTransport({ url: testServerUrl }); + const transport = makeNewHttpTransport({ url: TEST_SERVER_URL }); const transportResponse = await transport.send(EVENT_ENVELOPE); expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); }); }); + describe('proxy', () => { + it('can be configured through option', () => { + makeNewHttpTransport({ + url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + proxy: 'http://example.com', + }); + + expect(httpProxyAgent).toHaveBeenCalledTimes(1); + expect(httpProxyAgent).toHaveBeenCalledWith('http://example.com'); + }); + + it('can be configured through env variables option', () => { + process.env.http_proxy = 'http://example.com'; + makeNewHttpTransport({ + url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + }); + + expect(httpProxyAgent).toHaveBeenCalledTimes(1); + expect(httpProxyAgent).toHaveBeenCalledWith('http://example.com'); + delete process.env.http_proxy; + }); + + it('client options have priority over env variables', () => { + process.env.http_proxy = 'http://foo.com'; + makeNewHttpTransport({ + url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + proxy: 'http://bar.com', + }); + + expect(httpProxyAgent).toHaveBeenCalledTimes(1); + expect(httpProxyAgent).toHaveBeenCalledWith('http://bar.com'); + delete process.env.http_proxy; + }); + + it('no_proxy allows for skipping specific hosts', () => { + process.env.no_proxy = 'sentry.io'; + makeNewHttpTransport({ + url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + proxy: 'http://example.com', + }); + + expect(httpProxyAgent).not.toHaveBeenCalled(); + + delete process.env.no_proxy; + }); + + it('no_proxy works with a port', () => { + process.env.http_proxy = 'http://example.com:8080'; + process.env.no_proxy = 'sentry.io:8989'; + + makeNewHttpTransport({ + url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + }); + + expect(httpProxyAgent).not.toHaveBeenCalled(); + + delete process.env.no_proxy; + delete process.env.http_proxy; + }); + + it('no_proxy works with multiple comma-separated hosts', () => { + process.env.http_proxy = 'http://example.com:8080'; + process.env.no_proxy = 'example.com,sentry.io,wat.com:1337'; + + makeNewHttpTransport({ + url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + }); + + expect(httpProxyAgent).not.toHaveBeenCalled(); + + delete process.env.no_proxy; + delete process.env.http_proxy; + }); + }); + it('should register TransportRequestExecutor that returns the correct object from server response (rate limit)', async () => { await setupTestServer({ statusCode: RATE_LIMIT, @@ -161,7 +242,7 @@ describe('makeNewHttpTransport()', () => { }, }); - makeNewHttpTransport({ url: testServerUrl }); + makeNewHttpTransport({ url: TEST_SERVER_URL }); const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; const executorResult = registeredRequestExecutor({ @@ -180,13 +261,12 @@ describe('makeNewHttpTransport()', () => { ); }); - it('should register TransportRequestExecutor that returns the correct object from server response (success)', async () => { + it('should register TransportRequestExecutor that returns the correct object from server response (OK)', async () => { await setupTestServer({ statusCode: SUCCESS, - responseHeaders: {}, }); - makeNewHttpTransport({ url: testServerUrl }); + makeNewHttpTransport({ url: TEST_SERVER_URL }); const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; const executorResult = registeredRequestExecutor({ @@ -205,7 +285,7 @@ describe('makeNewHttpTransport()', () => { ); }); - it('should register TransportRequestExecutor that returns the correct object from server response (success but rate-limit)', async () => { + it('should register TransportRequestExecutor that returns the correct object from server response (OK with rate-limit headers)', async () => { await setupTestServer({ statusCode: SUCCESS, responseHeaders: { @@ -214,7 +294,7 @@ describe('makeNewHttpTransport()', () => { }, }); - makeNewHttpTransport({ url: testServerUrl }); + makeNewHttpTransport({ url: TEST_SERVER_URL }); const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; const executorResult = registeredRequestExecutor({ @@ -232,4 +312,32 @@ describe('makeNewHttpTransport()', () => { }), ); }); + + it('should register TransportRequestExecutor that returns the correct object from server response (NOK with rate-limit headers)', async () => { + await setupTestServer({ + statusCode: RATE_LIMIT, + responseHeaders: { + 'Retry-After': '2700', + 'X-Sentry-Rate-Limits': '60::organization, 2700::organization', + }, + }); + + makeNewHttpTransport({ url: TEST_SERVER_URL }); + const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; + + const executorResult = registeredRequestExecutor({ + body: serializeEnvelope(EVENT_ENVELOPE), + category: 'error', + }); + + await expect(executorResult).resolves.toEqual( + expect.objectContaining({ + headers: { + 'retry-after': '2700', + 'x-sentry-rate-limits': '60::organization, 2700::organization', + }, + statusCode: RATE_LIMIT, + }), + ); + }); }); diff --git a/packages/node/test/transports/new/https.test.ts b/packages/node/test/transports/new/https.test.ts new file mode 100644 index 000000000000..4ffe62140aef --- /dev/null +++ b/packages/node/test/transports/new/https.test.ts @@ -0,0 +1,379 @@ +import { createTransport } from '@sentry/core'; +import { EventEnvelope, EventItem } from '@sentry/types'; +import { createEnvelope, serializeEnvelope } from '@sentry/utils'; +import * as http from 'http'; +import * as https from 'https'; + +import { + HTTPModule, + HTTPModuleRequestIncomingMessage, + HTTPModuleRequestOptions, +} from '../../../src/transports/base/http-module'; +import { makeNewHttpsTransport } from '../../../src/transports/new'; +import testServerCerts from './test-server-certs'; + +jest.mock('@sentry/core', () => { + const actualCore = jest.requireActual('@sentry/core'); + return { + ...actualCore, + createTransport: jest.fn().mockImplementation(actualCore.createTransport), + }; +}); + +// eslint-disable-next-line @typescript-eslint/no-var-requires +const httpProxyAgent = require('https-proxy-agent'); +jest.mock('https-proxy-agent', () => { + return jest.fn().mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 })); +}); + +const SUCCESS = 200; +const RATE_LIMIT = 429; +const INVALID = 400; +const FAILED = 500; + +interface TestServerOptions { + statusCode: number; + responseHeaders?: Record; +} + +let testServer: http.Server | undefined; + +function setupTestServer( + options: TestServerOptions, + requestInspector?: (req: http.IncomingMessage, body: string) => void, +) { + testServer = https.createServer(testServerCerts, (req, res) => { + let body = ''; + + req.on('data', data => { + body += data; + }); + + req.on('end', () => { + requestInspector?.(req, body); + }); + + res.writeHead(options.statusCode, options.responseHeaders); + res.end(); + }); + + testServer.listen(8099); + + return new Promise(resolve => { + testServer?.on('listening', resolve); + }); +} + +const TEST_SERVER_URL = 'https://localhost:8099'; + +const EVENT_ENVELOPE = createEnvelope({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }, [ + [{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem, +]); + +const SERIALIZED_EVENT_ENVELOPE = serializeEnvelope(EVENT_ENVELOPE); + +const unsafeHttpsModule: HTTPModule = { + request: jest + .fn() + .mockImplementation( + (options: HTTPModuleRequestOptions, callback?: (res: HTTPModuleRequestIncomingMessage) => void) => { + return https.request({ ...options, rejectUnauthorized: false }, callback); + }, + ), +}; + +describe('makeNewHttpsTransport()', () => { + afterEach(() => { + jest.clearAllMocks(); + + if (testServer) { + testServer.close(); + } + }); + + describe('.send()', () => { + it('should correctly return successful server response', async () => { + await setupTestServer({ statusCode: SUCCESS }); + + const transport = makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + const transportResponse = await transport.send(EVENT_ENVELOPE); + + expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); + }); + + it('should correctly send envelope to server', async () => { + await setupTestServer({ statusCode: SUCCESS }, (req, body) => { + expect(req.method).toBe('POST'); + expect(body).toBe(SERIALIZED_EVENT_ENVELOPE); + }); + + const transport = makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + await transport.send(EVENT_ENVELOPE); + }); + + it('should correctly send user-provided headers to server', async () => { + await setupTestServer({ statusCode: SUCCESS }, req => { + expect(req.headers).toEqual( + expect.objectContaining({ + // node http module lower-cases incoming headers + 'x-some-custom-header-1': 'value1', + 'x-some-custom-header-2': 'value2', + }), + ); + }); + + const transport = makeNewHttpsTransport({ + httpModule: unsafeHttpsModule, + url: TEST_SERVER_URL, + headers: { + 'X-Some-Custom-Header-1': 'value1', + 'X-Some-Custom-Header-2': 'value2', + }, + }); + + await transport.send(EVENT_ENVELOPE); + }); + + it.each([ + [RATE_LIMIT, 'rate_limit'], + [INVALID, 'invalid'], + [FAILED, 'failed'], + ])('should correctly reject bad server response (status %i)', async (serverStatusCode, expectedStatus) => { + await setupTestServer({ statusCode: serverStatusCode }); + + const transport = makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + await expect(transport.send(EVENT_ENVELOPE)).rejects.toEqual(expect.objectContaining({ status: expectedStatus })); + }); + + it('should resolve when server responds with rate limit header and status code 200', async () => { + await setupTestServer({ + statusCode: SUCCESS, + responseHeaders: { + 'Retry-After': '2700', + 'X-Sentry-Rate-Limits': '60::organization, 2700::organization', + }, + }); + + const transport = makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + const transportResponse = await transport.send(EVENT_ENVELOPE); + + expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); + }); + + it('should resolve when server responds with rate limit header and status code 200', async () => { + await setupTestServer({ + statusCode: SUCCESS, + responseHeaders: { + 'Retry-After': '2700', + 'X-Sentry-Rate-Limits': '60::organization, 2700::organization', + }, + }); + + const transport = makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + const transportResponse = await transport.send(EVENT_ENVELOPE); + + expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); + }); + }); + + describe('proxy', () => { + it('can be configured through option', () => { + makeNewHttpsTransport({ + httpModule: unsafeHttpsModule, + url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + proxy: 'http://example.com', + }); + + expect(httpProxyAgent).toHaveBeenCalledTimes(1); + expect(httpProxyAgent).toHaveBeenCalledWith('http://example.com'); + }); + + it('can be configured through env variables option (http)', () => { + process.env.http_proxy = 'http://example.com'; + makeNewHttpsTransport({ + httpModule: unsafeHttpsModule, + url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + }); + + expect(httpProxyAgent).toHaveBeenCalledTimes(1); + expect(httpProxyAgent).toHaveBeenCalledWith('http://example.com'); + delete process.env.http_proxy; + }); + + it('can be configured through env variables option (https)', () => { + process.env.https_proxy = 'https://example.com'; + makeNewHttpsTransport({ + httpModule: unsafeHttpsModule, + url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + }); + + expect(httpProxyAgent).toHaveBeenCalledTimes(1); + expect(httpProxyAgent).toHaveBeenCalledWith('https://example.com'); + delete process.env.https_proxy; + }); + + it('client options have priority over env variables', () => { + process.env.https_proxy = 'http://foo.com'; + makeNewHttpsTransport({ + httpModule: unsafeHttpsModule, + url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + proxy: 'http://bar.com', + }); + + expect(httpProxyAgent).toHaveBeenCalledTimes(1); + expect(httpProxyAgent).toHaveBeenCalledWith('http://bar.com'); + delete process.env.https_proxy; + }); + + it('no_proxy allows for skipping specific hosts', () => { + process.env.no_proxy = 'sentry.io'; + makeNewHttpsTransport({ + httpModule: unsafeHttpsModule, + url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + proxy: 'http://example.com', + }); + + expect(httpProxyAgent).not.toHaveBeenCalled(); + + delete process.env.no_proxy; + }); + + it('no_proxy works with a port', () => { + process.env.http_proxy = 'http://example.com:8080'; + process.env.no_proxy = 'sentry.io:8989'; + + makeNewHttpsTransport({ + httpModule: unsafeHttpsModule, + url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + }); + + expect(httpProxyAgent).not.toHaveBeenCalled(); + + delete process.env.no_proxy; + delete process.env.http_proxy; + }); + + it('no_proxy works with multiple comma-separated hosts', () => { + process.env.http_proxy = 'http://example.com:8080'; + process.env.no_proxy = 'example.com,sentry.io,wat.com:1337'; + + makeNewHttpsTransport({ + httpModule: unsafeHttpsModule, + url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + }); + + expect(httpProxyAgent).not.toHaveBeenCalled(); + + delete process.env.no_proxy; + delete process.env.http_proxy; + }); + }); + + it('should register TransportRequestExecutor that returns the correct object from server response (rate limit)', async () => { + await setupTestServer({ + statusCode: RATE_LIMIT, + responseHeaders: { + 'Retry-After': '2700', + 'X-Sentry-Rate-Limits': '60::organization, 2700::organization', + }, + }); + + makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; + + const executorResult = registeredRequestExecutor({ + body: serializeEnvelope(EVENT_ENVELOPE), + category: 'error', + }); + + await expect(executorResult).resolves.toEqual( + expect.objectContaining({ + headers: { + 'retry-after': '2700', + 'x-sentry-rate-limits': '60::organization, 2700::organization', + }, + statusCode: RATE_LIMIT, + }), + ); + }); + + it('should register TransportRequestExecutor that returns the correct object from server response (OK)', async () => { + await setupTestServer({ + statusCode: SUCCESS, + }); + + makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; + + const executorResult = registeredRequestExecutor({ + body: serializeEnvelope(EVENT_ENVELOPE), + category: 'error', + }); + + await expect(executorResult).resolves.toEqual( + expect.objectContaining({ + headers: { + 'retry-after': null, + 'x-sentry-rate-limits': null, + }, + statusCode: SUCCESS, + }), + ); + }); + + it('should register TransportRequestExecutor that returns the correct object from server response (OK with rate-limit headers)', async () => { + await setupTestServer({ + statusCode: SUCCESS, + responseHeaders: { + 'Retry-After': '2700', + 'X-Sentry-Rate-Limits': '60::organization, 2700::organization', + }, + }); + + makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; + + const executorResult = registeredRequestExecutor({ + body: serializeEnvelope(EVENT_ENVELOPE), + category: 'error', + }); + + await expect(executorResult).resolves.toEqual( + expect.objectContaining({ + headers: { + 'retry-after': '2700', + 'x-sentry-rate-limits': '60::organization, 2700::organization', + }, + statusCode: SUCCESS, + }), + ); + }); + + it('should register TransportRequestExecutor that returns the correct object from server response (NOK with rate-limit headers)', async () => { + await setupTestServer({ + statusCode: RATE_LIMIT, + responseHeaders: { + 'Retry-After': '2700', + 'X-Sentry-Rate-Limits': '60::organization, 2700::organization', + }, + }); + + makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; + + const executorResult = registeredRequestExecutor({ + body: serializeEnvelope(EVENT_ENVELOPE), + category: 'error', + }); + + await expect(executorResult).resolves.toEqual( + expect.objectContaining({ + headers: { + 'retry-after': '2700', + 'x-sentry-rate-limits': '60::organization, 2700::organization', + }, + statusCode: RATE_LIMIT, + }), + ); + }); +}); diff --git a/packages/node/test/transports/new/test-server-certs.ts b/packages/node/test/transports/new/test-server-certs.ts new file mode 100644 index 000000000000..a5ce436c4234 --- /dev/null +++ b/packages/node/test/transports/new/test-server-certs.ts @@ -0,0 +1,48 @@ +export default { + key: `-----BEGIN RSA PRIVATE KEY----- +MIIEowIBAAKCAQEAuMunjXC2tu2d4x8vKuPQbHwPjYG6pVvAUs7wzpDnMEGo3o2A +bZpL7vUAkQWZ86M84rX9b65cVvT35uqM9uxnJKQhSdGARxEcrz9yxjc9RaIO9xM4 +6WdFd6pcVHW9MF6njnc19jyIoSGXRADJjreNZHyMobAHyL2ZbFiptknUWFW3YT4t +q9bQD5yfhZ94fRt1IbdBAn5Bmz6x61BYudWU2KA3G1akPUmzj0OwZwaIrnGbfLUH +M5F50dNUYfCdmxtE8YRBPyWwcg+KOWa/P8C84p1UQ+/0GHNqUTa4wXBgKeUXNjth +AhV/4JgDDdec+/W0Z1UdEqxZvKfAYnjveFpxEwIDAQABAoIBADLsjEPB59gJKxVH +pqvfE7SRi4enVFP1MM6hEGMcM1ls/qg1vkp11q8G/Rz5ui8VsNWY6To5hmDAKQCN +akMxaksCn9nDzeHHqWvxxCMzXcMuoYkc1vYa613KqJ7twzDtJKdx2oD8tXoR06l9 +vg2CL4idefOkmsCK3xioZjxBpC6jF6ybvlY241MGhaAGRHmP6ik1uFJ+6Y8smh6R +AQKO0u0oQPy6bka9F6DTP6BMUeZ+OA/oOrrb5FxTHu8AHcyCSk2wHnCkB9EF/Ou2 +xSWrnu0O0/0Px6OO9oEsNSq2/fKNV9iuEU8LeAoDVm4ysyMrPce2c4ZsB4U244bj +yQpQZ6ECgYEA9KwA7Lmyf+eeZHxEM4MNSqyeXBtSKu4Zyk0RRY1j69ConjHKet3Q +ylVedXQ0/FJAHHKEm4zFGZtnaaxrzCIcQSKJBCoaA+cN44MM3D1nKmHjgPy8R/yE +BNgIVwJB1MmVSGa+NYnQgUomcCIEr/guNMIxV7p2iybqoxaEHKLfGFUCgYEAwVn1 +8LARsZihLUdxxbAc9+v/pBeMTrkTw1eN1ki9VWYoRam2MLozehEzabt677cU4h7+ +bjdKCKo1x2liY9zmbIiVHssv9Jf3E9XhcajsXB42m1+kjUYVPh8o9lDXcatV9EKt +DZK8wfRY9boyDKB2zRyo6bvIEK3qWbas31W3a8cCgYA6w0TFliPkzEAiaiYHKSZ8 +FNFD1dv6K41OJQxM5BRngom81MCImdWXgsFY/DvtjeOP8YEfysNbzxMbMioBsP+Q +NTcrJOFypn+TcNoZ2zV33GLDi++8ak1azHfUTdp5vKB57xMn0J2fL6vjqoftq3GN +gkZPh50I9qPL35CDQCrMsQKBgC6tFfc1uf/Cld5FagzMOCINodguKxvyB/hXUZFS +XAqar8wpbScUPEsSjfPPY50s+GiiDM/0nvW6iWMLaMos0J+Q1VbqvDfy2525O0Ri +ADU4wfv+Oc41BfnKMexMlcYGE6j006v8KX81Cqi/e0ebETLw4UITp/eG1JU1yUPd +AHuPAoGBAL25v4/onoH0FBLdEwb2BAENxc+0g4In1T+83jfHbfD0gOF3XTbgH4FF +MduIG8qBoZC5whiZ3qH7YJK7sydaM1bDwiesqIik+gEUE65T7S2ZF84y5GC5JjTf +z6v6i+DMCIJXDY5/gjzOED6UllV2Jrn2pDoV++zVyR6KAwXpCmK6 +-----END RSA PRIVATE KEY-----`, + cert: `-----BEGIN CERTIFICATE----- +MIIDETCCAfkCFCMI53aBdS2kWTrw39Kkv93ErG3iMA0GCSqGSIb3DQEBCwUAMEUx +CzAJBgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRl +cm5ldCBXaWRnaXRzIFB0eSBMdGQwHhcNMjIwMzI4MDgzODQwWhcNNDkwODEyMDgz +ODQwWjBFMQswCQYDVQQGEwJBVTETMBEGA1UECAwKU29tZS1TdGF0ZTEhMB8GA1UE +CgwYSW50ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMIIBIjANBgkqhkiG9w0BAQEFAAOC +AQ8AMIIBCgKCAQEAuMunjXC2tu2d4x8vKuPQbHwPjYG6pVvAUs7wzpDnMEGo3o2A +bZpL7vUAkQWZ86M84rX9b65cVvT35uqM9uxnJKQhSdGARxEcrz9yxjc9RaIO9xM4 +6WdFd6pcVHW9MF6njnc19jyIoSGXRADJjreNZHyMobAHyL2ZbFiptknUWFW3YT4t +q9bQD5yfhZ94fRt1IbdBAn5Bmz6x61BYudWU2KA3G1akPUmzj0OwZwaIrnGbfLUH +M5F50dNUYfCdmxtE8YRBPyWwcg+KOWa/P8C84p1UQ+/0GHNqUTa4wXBgKeUXNjth +AhV/4JgDDdec+/W0Z1UdEqxZvKfAYnjveFpxEwIDAQABMA0GCSqGSIb3DQEBCwUA +A4IBAQBh4BKiByhyvAc5uHj5bkSqspY2xZWW8xiEGaCaQWDMlyjP9mVVWFHfE3XL +lzsJdZVnHDZUliuA5L+qTEpLJ5GmgDWqnKp3HdhtkL16mPbPyJLPY0X+m7wvoZRt +RwLfFCx1E13m0ktYWWgmSCnBl+rI7pyagDhZ2feyxsMrecCazyG/llFBuyWSOnIi +OHxjdHV7be5c8uOOp1iNB9j++LW1pRVrSCWOKRLcsUBal73FW+UvhM5+1If/F9pF +GNQrMhVRA8aHD0JAu3tpjYRKRuOpAbbqtiAUSbDPsJBQy/K9no2K83G7+AV+aGai +HXfQqFFJS6xGKU79azH51wLVEGXq +-----END CERTIFICATE-----`, +}; From 9194e855ba6f252cd75b69b433c5a30eba1820b6 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 28 Mar 2022 09:36:17 +0000 Subject: [PATCH 08/19] Add documentation --- packages/node/src/transports/new.ts | 31 ++++++++--------- .../node/test/transports/new/https.test.ts | 34 +++++++++++++------ 2 files changed, 39 insertions(+), 26 deletions(-) diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index de807b79b760..2fb5e4d71298 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -7,23 +7,11 @@ import { } from '@sentry/core'; import { TransportRequestExecutor } from '@sentry/core/dist/transports/base'; import { eventStatusFromHttpCode } from '@sentry/utils'; -import * as fs from 'fs'; import * as http from 'http'; import * as https from 'https'; import { HTTPModule } from './base/http-module'; -interface HttpTransportOptions extends BaseTransportOptions { - // Todo: doc - headers?: Record; - // TODO: doc Set a HTTP proxy that should be used for outbound requests. - proxy?: string; - // TODO: doc HTTPS proxy certificates path - caCerts?: string; - // Todo: doc - httpModule?: HTTPModule; -} - // TODO(v7): // - Rename this file "transports.ts" // - Move this file one folder upwards @@ -31,8 +19,19 @@ interface HttpTransportOptions extends BaseTransportOptions { // OR // - Split this file up and leave it in the transports folder +export interface HttpTransportOptions extends BaseTransportOptions { + /** Define custom headers */ + headers?: Record; + /** Set a proxy that should be used for outbound requests. */ + proxy?: string; + /** HTTPS proxy CA certificates */ + caCerts?: string | Buffer | Array; + /** Custom HTTP module */ + httpModule?: HTTPModule; +} + /** - * TODO Doc + * Creates a Transport that uses http to send events to Sentry. */ export function makeNewHttpTransport(options: HttpTransportOptions): NewTransport { // Proxy prioritization: http => `options.proxy` | `process.env.http_proxy` @@ -49,7 +48,7 @@ export function makeNewHttpTransport(options: HttpTransportOptions): NewTranspor } /** - * TODO Doc + * Creates a Transport that uses https to send events to Sentry. */ export function makeNewHttpsTransport(options: HttpTransportOptions): NewTransport { // Proxy prioritization: https => `options.proxy` | `process.env.https_proxy` | `process.env.http_proxy` @@ -91,7 +90,7 @@ function applyNoProxyOption(transportUrl: string, proxy: string | undefined): st } /** - * TODO Doc + * Creates a RequestExecutor to be used with `createTransport`. */ function createRequestExecutor( options: HttpTransportOptions, @@ -111,7 +110,7 @@ function createRequestExecutor( pathname, port, protocol, - ca: options.caCerts ? fs.readFileSync(options.caCerts) : undefined, + ca: options.caCerts, }, res => { res.on('data', () => { diff --git a/packages/node/test/transports/new/https.test.ts b/packages/node/test/transports/new/https.test.ts index 4ffe62140aef..c34fdd2680ca 100644 --- a/packages/node/test/transports/new/https.test.ts +++ b/packages/node/test/transports/new/https.test.ts @@ -4,11 +4,7 @@ import { createEnvelope, serializeEnvelope } from '@sentry/utils'; import * as http from 'http'; import * as https from 'https'; -import { - HTTPModule, - HTTPModuleRequestIncomingMessage, - HTTPModuleRequestOptions, -} from '../../../src/transports/base/http-module'; +import { HTTPModule, HTTPModuleRequestIncomingMessage } from '../../../src/transports/base/http-module'; import { makeNewHttpsTransport } from '../../../src/transports/new'; import testServerCerts from './test-server-certs'; @@ -75,11 +71,9 @@ const SERIALIZED_EVENT_ENVELOPE = serializeEnvelope(EVENT_ENVELOPE); const unsafeHttpsModule: HTTPModule = { request: jest .fn() - .mockImplementation( - (options: HTTPModuleRequestOptions, callback?: (res: HTTPModuleRequestIncomingMessage) => void) => { - return https.request({ ...options, rejectUnauthorized: false }, callback); - }, - ), + .mockImplementation((options: https.RequestOptions, callback?: (res: HTTPModuleRequestIncomingMessage) => void) => { + return https.request({ ...options, rejectUnauthorized: false }, callback); + }), }; describe('makeNewHttpsTransport()', () => { @@ -174,6 +168,26 @@ describe('makeNewHttpsTransport()', () => { expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); }); + + it('should use `caCerts` option', async () => { + await setupTestServer({ statusCode: SUCCESS }); + + const transport = makeNewHttpsTransport({ + httpModule: unsafeHttpsModule, + url: TEST_SERVER_URL, + caCerts: 'some cert', + }); + + await transport.send(EVENT_ENVELOPE); + + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(unsafeHttpsModule.request).toHaveBeenCalledWith( + expect.objectContaining({ + ca: 'some cert', + }), + expect.anything(), + ); + }); }); describe('proxy', () => { From a159aaa8d6b0614e3626c238fdf0d6fe5a090000 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 28 Mar 2022 09:43:21 +0000 Subject: [PATCH 09/19] Rearrange imported type --- packages/core/src/index.ts | 1 + packages/node/src/transports/new.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 5f132aaefa1f..f09d804d3bed 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -35,6 +35,7 @@ export { NewTransport, TransportMakeRequestResponse, TransportRequest, + TransportRequestExecutor, } from './transports/base'; export { SDK_VERSION } from './version'; diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index 2fb5e4d71298..5aa3f9933a62 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -4,8 +4,8 @@ import { NewTransport, TransportMakeRequestResponse, TransportRequest, + TransportRequestExecutor, } from '@sentry/core'; -import { TransportRequestExecutor } from '@sentry/core/dist/transports/base'; import { eventStatusFromHttpCode } from '@sentry/utils'; import * as http from 'http'; import * as https from 'https'; From f166ea01a751fdd3b2d60ba715573ac1c29d97b5 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 28 Mar 2022 10:22:03 +0000 Subject: [PATCH 10/19] Import url to fix older node versions --- packages/node/src/transports/new.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index 5aa3f9933a62..72c7744bf9df 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -9,6 +9,7 @@ import { import { eventStatusFromHttpCode } from '@sentry/utils'; import * as http from 'http'; import * as https from 'https'; +import { URL } from 'url'; import { HTTPModule } from './base/http-module'; @@ -107,7 +108,7 @@ function createRequestExecutor( agent, headers: options.headers, hostname, - pathname, + path: `${pathname}`, port, protocol, ca: options.caCerts, From a4160c7d94cedf4af4f9e0e0f90cbd979d84ceb1 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 28 Mar 2022 12:36:54 +0000 Subject: [PATCH 11/19] Rename options type of node transport --- packages/core/src/transports/base.ts | 11 ----------- packages/node/src/transports/index.ts | 2 +- packages/node/src/transports/new.ts | 8 ++++---- 3 files changed, 5 insertions(+), 16 deletions(-) diff --git a/packages/core/src/transports/base.ts b/packages/core/src/transports/base.ts index 6290fbfec36e..663a4c3c686f 100644 --- a/packages/core/src/transports/base.ts +++ b/packages/core/src/transports/base.ts @@ -67,17 +67,6 @@ export interface BrowserTransportOptions extends BaseTransportOptions { sendClientReports?: boolean; } -// TODO: Move into Node transport -export interface NodeTransportOptions extends BaseTransportOptions { - headers?: Record; - // Set a HTTP proxy that should be used for outbound requests. - httpProxy?: string; - // Set a HTTPS proxy that should be used for outbound requests. - httpsProxy?: string; - // HTTPS proxy certificates path - caCerts?: string; -} - export interface NewTransport { send(request: Envelope): PromiseLike; flush(timeout?: number): PromiseLike; diff --git a/packages/node/src/transports/index.ts b/packages/node/src/transports/index.ts index 267fadd41e74..440005e33f2c 100644 --- a/packages/node/src/transports/index.ts +++ b/packages/node/src/transports/index.ts @@ -1,4 +1,4 @@ export { BaseTransport } from './base'; export { HTTPTransport } from './http'; export { HTTPSTransport } from './https'; -export { makeNewHttpTransport, makeNewHttpsTransport } from './new'; +export { makeNewHttpTransport, makeNewHttpsTransport, NodeTransportOptions } from './new'; diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index 72c7744bf9df..0309d1b5f522 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -20,7 +20,7 @@ import { HTTPModule } from './base/http-module'; // OR // - Split this file up and leave it in the transports folder -export interface HttpTransportOptions extends BaseTransportOptions { +export interface NodeTransportOptions extends BaseTransportOptions { /** Define custom headers */ headers?: Record; /** Set a proxy that should be used for outbound requests. */ @@ -34,7 +34,7 @@ export interface HttpTransportOptions extends BaseTransportOptions { /** * Creates a Transport that uses http to send events to Sentry. */ -export function makeNewHttpTransport(options: HttpTransportOptions): NewTransport { +export function makeNewHttpTransport(options: NodeTransportOptions): NewTransport { // Proxy prioritization: http => `options.proxy` | `process.env.http_proxy` const proxy = applyNoProxyOption(options.url, options.proxy || process.env.http_proxy); @@ -51,7 +51,7 @@ export function makeNewHttpTransport(options: HttpTransportOptions): NewTranspor /** * Creates a Transport that uses https to send events to Sentry. */ -export function makeNewHttpsTransport(options: HttpTransportOptions): NewTransport { +export function makeNewHttpsTransport(options: NodeTransportOptions): NewTransport { // Proxy prioritization: https => `options.proxy` | `process.env.https_proxy` | `process.env.http_proxy` const proxy = applyNoProxyOption(options.url, options.proxy || process.env.https_proxy || process.env.http_proxy); @@ -94,7 +94,7 @@ function applyNoProxyOption(transportUrl: string, proxy: string | undefined): st * Creates a RequestExecutor to be used with `createTransport`. */ function createRequestExecutor( - options: HttpTransportOptions, + options: NodeTransportOptions, httpModule: HTTPModule, agent: http.Agent, ): TransportRequestExecutor { From 97f8aadffb4fd926d25e2632ca91b93cdf27f2f2 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 28 Mar 2022 12:37:02 +0000 Subject: [PATCH 12/19] Update node backend to use new transport --- packages/node/src/backend.ts | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index 61cbfad9c819..0a6e90012058 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -1,9 +1,15 @@ -import { BaseBackend } from '@sentry/core'; +import { BaseBackend, getEnvelopeEndpointWithUrlEncodedAuth, initAPIDetails } from '@sentry/core'; import { Event, EventHint, Severity, Transport, TransportOptions } from '@sentry/types'; import { makeDsn, resolvedSyncPromise } from '@sentry/utils'; import { eventFromMessage, eventFromUnknownInput } from './eventbuilder'; -import { HTTPSTransport, HTTPTransport } from './transports'; +import { + HTTPSTransport, + HTTPTransport, + makeNewHttpsTransport, + makeNewHttpTransport, + NodeTransportOptions, +} from './transports'; import { NodeOptions } from './types'; /** @@ -50,9 +56,22 @@ export class NodeBackend extends BaseBackend { if (this._options.transport) { return new this._options.transport(transportOptions); } + + const api = initAPIDetails(transportOptions.dsn, transportOptions._metadata, transportOptions.tunnel); + const url = getEnvelopeEndpointWithUrlEncodedAuth(api.dsn, api.tunnel); + + const newTransportOptions: NodeTransportOptions = { + url, + headers: transportOptions.headers, + proxy: transportOptions.httpProxy, + caCerts: transportOptions.caCerts, + }; + if (dsn.protocol === 'http') { + this._newTransport = makeNewHttpTransport(newTransportOptions); return new HTTPTransport(transportOptions); } + this._newTransport = makeNewHttpsTransport(newTransportOptions); return new HTTPSTransport(transportOptions); } } From 47caf0b4b9be925dd2e8534a0c26a316bff66b70 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 28 Mar 2022 18:00:37 +0000 Subject: [PATCH 13/19] Fix url to include query params --- packages/node/src/transports/new.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index 0309d1b5f522..d2b921c3b06b 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -98,7 +98,7 @@ function createRequestExecutor( httpModule: HTTPModule, agent: http.Agent, ): TransportRequestExecutor { - const { hostname, pathname, port, protocol } = new URL(options.url); + const { hostname, pathname, port, protocol, search } = new URL(options.url); return function makeRequest(request: TransportRequest): Promise { return new Promise((resolve, reject) => { @@ -108,7 +108,7 @@ function createRequestExecutor( agent, headers: options.headers, hostname, - path: `${pathname}`, + path: `${pathname}${search}`, port, protocol, ca: options.caCerts, From 44f54b8bf31a7c4aeb5766224b3f5218f1b288a3 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 28 Mar 2022 19:57:08 +0000 Subject: [PATCH 14/19] Unify http and https transport --- packages/node/src/backend.ts | 12 +-- packages/node/src/transports/index.ts | 2 +- packages/node/src/transports/new.ts | 47 +++++------- .../node/test/transports/new/http.test.ts | 41 +++++----- .../node/test/transports/new/https.test.ts | 75 ++++++++++--------- 5 files changed, 83 insertions(+), 94 deletions(-) diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index 0a6e90012058..a3827fe5e226 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -3,13 +3,7 @@ import { Event, EventHint, Severity, Transport, TransportOptions } from '@sentry import { makeDsn, resolvedSyncPromise } from '@sentry/utils'; import { eventFromMessage, eventFromUnknownInput } from './eventbuilder'; -import { - HTTPSTransport, - HTTPTransport, - makeNewHttpsTransport, - makeNewHttpTransport, - NodeTransportOptions, -} from './transports'; +import { HTTPSTransport, HTTPTransport, makeNodeTransport, NodeTransportOptions } from './transports'; import { NodeOptions } from './types'; /** @@ -67,11 +61,11 @@ export class NodeBackend extends BaseBackend { caCerts: transportOptions.caCerts, }; + this._newTransport = makeNodeTransport(newTransportOptions); + if (dsn.protocol === 'http') { - this._newTransport = makeNewHttpTransport(newTransportOptions); return new HTTPTransport(transportOptions); } - this._newTransport = makeNewHttpsTransport(newTransportOptions); return new HTTPSTransport(transportOptions); } } diff --git a/packages/node/src/transports/index.ts b/packages/node/src/transports/index.ts index 440005e33f2c..2cabeee08d5f 100644 --- a/packages/node/src/transports/index.ts +++ b/packages/node/src/transports/index.ts @@ -1,4 +1,4 @@ export { BaseTransport } from './base'; export { HTTPTransport } from './http'; export { HTTPSTransport } from './https'; -export { makeNewHttpTransport, makeNewHttpsTransport, NodeTransportOptions } from './new'; +export { makeNodeTransport, NodeTransportOptions } from './new'; diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index d2b921c3b06b..2b3df25c4e01 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -14,7 +14,7 @@ import { URL } from 'url'; import { HTTPModule } from './base/http-module'; // TODO(v7): -// - Rename this file "transports.ts" +// - Rename this file "transport.ts" // - Move this file one folder upwards // - Delete "transports" folder // OR @@ -27,41 +27,30 @@ export interface NodeTransportOptions extends BaseTransportOptions { proxy?: string; /** HTTPS proxy CA certificates */ caCerts?: string | Buffer | Array; - /** Custom HTTP module */ + /** Custom HTTP module. Defaults to the native 'http' and 'https' modules. */ httpModule?: HTTPModule; } /** - * Creates a Transport that uses http to send events to Sentry. + * Creates a Transport that uses native the native 'http' and 'https' modules to send events to Sentry. */ -export function makeNewHttpTransport(options: NodeTransportOptions): NewTransport { - // Proxy prioritization: http => `options.proxy` | `process.env.http_proxy` - const proxy = applyNoProxyOption(options.url, options.proxy || process.env.http_proxy); +export function makeNodeTransport(options: NodeTransportOptions): NewTransport { + const urlSegments = new URL(options.url); + const isHttps = urlSegments.protocol === 'https:'; - const httpModule = options.httpModule ?? http; - - const agent = proxy - ? (new (require('https-proxy-agent'))(proxy) as http.Agent) - : new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 }); - - const requestExecutor = createRequestExecutor(options, httpModule, agent); - return createTransport({ bufferSize: options.bufferSize }, requestExecutor); -} - -/** - * Creates a Transport that uses https to send events to Sentry. - */ -export function makeNewHttpsTransport(options: NodeTransportOptions): NewTransport { // Proxy prioritization: https => `options.proxy` | `process.env.https_proxy` | `process.env.http_proxy` - const proxy = applyNoProxyOption(options.url, options.proxy || process.env.https_proxy || process.env.http_proxy); + const proxy = applyNoProxyOption( + urlSegments, + options.proxy || (isHttps ? process.env.https_proxy : undefined) || process.env.http_proxy, + ); - const httpsModule = options.httpModule ?? https; + const nativeHttpModule = isHttps ? https : http; const agent = proxy - ? (new (require('https-proxy-agent'))(proxy) as https.Agent) - : new https.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 }); + ? (new (require('https-proxy-agent'))(proxy) as http.Agent) + : new nativeHttpModule.Agent({ keepAlive: true, maxSockets: 30, timeout: 2000 }); - const requestExecutor = createRequestExecutor(options, httpsModule, agent); + const requestExecutor = createRequestExecutor(options, options.httpModule ?? nativeHttpModule, agent); return createTransport({ bufferSize: options.bufferSize }, requestExecutor); } @@ -72,16 +61,16 @@ export function makeNewHttpsTransport(options: NodeTransportOptions): NewTranspo * @param proxy The client configured proxy. * @returns A proxy the transport should use. */ -function applyNoProxyOption(transportUrl: string, proxy: string | undefined): string | undefined { +function applyNoProxyOption(transportUrlSegments: URL, proxy: string | undefined): string | undefined { const { no_proxy } = process.env; - const { host: transportUrlHost, hostname: transportUrlHostname } = new URL(transportUrl); - const urlIsExemptFromProxy = no_proxy && no_proxy .split(',') - .some(exemption => transportUrlHost.endsWith(exemption) || transportUrlHostname.endsWith(exemption)); + .some( + exemption => transportUrlSegments.host.endsWith(exemption) || transportUrlSegments.hostname.endsWith(exemption), + ); if (urlIsExemptFromProxy) { return undefined; diff --git a/packages/node/test/transports/new/http.test.ts b/packages/node/test/transports/new/http.test.ts index 8c88ceb456cf..8c39b868a24e 100644 --- a/packages/node/test/transports/new/http.test.ts +++ b/packages/node/test/transports/new/http.test.ts @@ -3,7 +3,7 @@ import { EventEnvelope, EventItem } from '@sentry/types'; import { createEnvelope, serializeEnvelope } from '@sentry/utils'; import * as http from 'http'; -import { makeNewHttpTransport } from '../../../src/transports/new'; +import { makeNodeTransport } from '../../../src/transports/new'; jest.mock('@sentry/core', () => { const actualCore = jest.requireActual('@sentry/core'); @@ -48,16 +48,19 @@ function setupTestServer( res.writeHead(options.statusCode, options.responseHeaders); res.end(); + + // also terminate socket because keepalive hangs connection a bit + res.connection.end(); }); - testServer.listen(8099); + testServer.listen(18099); return new Promise(resolve => { testServer?.on('listening', resolve); }); } -const TEST_SERVER_URL = 'http://localhost:8099'; +const TEST_SERVER_URL = 'http://localhost:18099'; const EVENT_ENVELOPE = createEnvelope({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }, [ [{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem, @@ -78,7 +81,7 @@ describe('makeNewHttpTransport()', () => { it('should correctly return successful server response', async () => { await setupTestServer({ statusCode: SUCCESS }); - const transport = makeNewHttpTransport({ url: TEST_SERVER_URL }); + const transport = makeNodeTransport({ url: TEST_SERVER_URL }); const transportResponse = await transport.send(EVENT_ENVELOPE); expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); @@ -90,7 +93,7 @@ describe('makeNewHttpTransport()', () => { expect(body).toBe(SERIALIZED_EVENT_ENVELOPE); }); - const transport = makeNewHttpTransport({ url: TEST_SERVER_URL }); + const transport = makeNodeTransport({ url: TEST_SERVER_URL }); await transport.send(EVENT_ENVELOPE); }); @@ -105,7 +108,7 @@ describe('makeNewHttpTransport()', () => { ); }); - const transport = makeNewHttpTransport({ + const transport = makeNodeTransport({ url: TEST_SERVER_URL, headers: { 'X-Some-Custom-Header-1': 'value1', @@ -123,7 +126,7 @@ describe('makeNewHttpTransport()', () => { ])('should correctly reject bad server response (status %i)', async (serverStatusCode, expectedStatus) => { await setupTestServer({ statusCode: serverStatusCode }); - const transport = makeNewHttpTransport({ url: TEST_SERVER_URL }); + const transport = makeNodeTransport({ url: TEST_SERVER_URL }); await expect(transport.send(EVENT_ENVELOPE)).rejects.toEqual(expect.objectContaining({ status: expectedStatus })); }); @@ -136,7 +139,7 @@ describe('makeNewHttpTransport()', () => { }, }); - const transport = makeNewHttpTransport({ url: TEST_SERVER_URL }); + const transport = makeNodeTransport({ url: TEST_SERVER_URL }); const transportResponse = await transport.send(EVENT_ENVELOPE); expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); @@ -151,7 +154,7 @@ describe('makeNewHttpTransport()', () => { }, }); - const transport = makeNewHttpTransport({ url: TEST_SERVER_URL }); + const transport = makeNodeTransport({ url: TEST_SERVER_URL }); const transportResponse = await transport.send(EVENT_ENVELOPE); expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); @@ -160,7 +163,7 @@ describe('makeNewHttpTransport()', () => { describe('proxy', () => { it('can be configured through option', () => { - makeNewHttpTransport({ + makeNodeTransport({ url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', proxy: 'http://example.com', }); @@ -171,7 +174,7 @@ describe('makeNewHttpTransport()', () => { it('can be configured through env variables option', () => { process.env.http_proxy = 'http://example.com'; - makeNewHttpTransport({ + makeNodeTransport({ url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); @@ -182,7 +185,7 @@ describe('makeNewHttpTransport()', () => { it('client options have priority over env variables', () => { process.env.http_proxy = 'http://foo.com'; - makeNewHttpTransport({ + makeNodeTransport({ url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', proxy: 'http://bar.com', }); @@ -194,7 +197,7 @@ describe('makeNewHttpTransport()', () => { it('no_proxy allows for skipping specific hosts', () => { process.env.no_proxy = 'sentry.io'; - makeNewHttpTransport({ + makeNodeTransport({ url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', proxy: 'http://example.com', }); @@ -208,7 +211,7 @@ describe('makeNewHttpTransport()', () => { process.env.http_proxy = 'http://example.com:8080'; process.env.no_proxy = 'sentry.io:8989'; - makeNewHttpTransport({ + makeNodeTransport({ url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); @@ -222,7 +225,7 @@ describe('makeNewHttpTransport()', () => { process.env.http_proxy = 'http://example.com:8080'; process.env.no_proxy = 'example.com,sentry.io,wat.com:1337'; - makeNewHttpTransport({ + makeNodeTransport({ url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); @@ -242,7 +245,7 @@ describe('makeNewHttpTransport()', () => { }, }); - makeNewHttpTransport({ url: TEST_SERVER_URL }); + makeNodeTransport({ url: TEST_SERVER_URL }); const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; const executorResult = registeredRequestExecutor({ @@ -266,7 +269,7 @@ describe('makeNewHttpTransport()', () => { statusCode: SUCCESS, }); - makeNewHttpTransport({ url: TEST_SERVER_URL }); + makeNodeTransport({ url: TEST_SERVER_URL }); const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; const executorResult = registeredRequestExecutor({ @@ -294,7 +297,7 @@ describe('makeNewHttpTransport()', () => { }, }); - makeNewHttpTransport({ url: TEST_SERVER_URL }); + makeNodeTransport({ url: TEST_SERVER_URL }); const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; const executorResult = registeredRequestExecutor({ @@ -322,7 +325,7 @@ describe('makeNewHttpTransport()', () => { }, }); - makeNewHttpTransport({ url: TEST_SERVER_URL }); + makeNodeTransport({ url: TEST_SERVER_URL }); const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; const executorResult = registeredRequestExecutor({ diff --git a/packages/node/test/transports/new/https.test.ts b/packages/node/test/transports/new/https.test.ts index c34fdd2680ca..2c7af5c453d3 100644 --- a/packages/node/test/transports/new/https.test.ts +++ b/packages/node/test/transports/new/https.test.ts @@ -5,7 +5,7 @@ import * as http from 'http'; import * as https from 'https'; import { HTTPModule, HTTPModuleRequestIncomingMessage } from '../../../src/transports/base/http-module'; -import { makeNewHttpsTransport } from '../../../src/transports/new'; +import { makeNodeTransport } from '../../../src/transports/new'; import testServerCerts from './test-server-certs'; jest.mock('@sentry/core', () => { @@ -51,6 +51,9 @@ function setupTestServer( res.writeHead(options.statusCode, options.responseHeaders); res.end(); + + // also terminate socket because keepalive hangs connection a bit + res.connection.end(); }); testServer.listen(8099); @@ -89,7 +92,7 @@ describe('makeNewHttpsTransport()', () => { it('should correctly return successful server response', async () => { await setupTestServer({ statusCode: SUCCESS }); - const transport = makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + const transport = makeNodeTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); const transportResponse = await transport.send(EVENT_ENVELOPE); expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); @@ -101,7 +104,7 @@ describe('makeNewHttpsTransport()', () => { expect(body).toBe(SERIALIZED_EVENT_ENVELOPE); }); - const transport = makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + const transport = makeNodeTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); await transport.send(EVENT_ENVELOPE); }); @@ -116,7 +119,7 @@ describe('makeNewHttpsTransport()', () => { ); }); - const transport = makeNewHttpsTransport({ + const transport = makeNodeTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL, headers: { @@ -135,7 +138,7 @@ describe('makeNewHttpsTransport()', () => { ])('should correctly reject bad server response (status %i)', async (serverStatusCode, expectedStatus) => { await setupTestServer({ statusCode: serverStatusCode }); - const transport = makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + const transport = makeNodeTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); await expect(transport.send(EVENT_ENVELOPE)).rejects.toEqual(expect.objectContaining({ status: expectedStatus })); }); @@ -148,7 +151,7 @@ describe('makeNewHttpsTransport()', () => { }, }); - const transport = makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + const transport = makeNodeTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); const transportResponse = await transport.send(EVENT_ENVELOPE); expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); @@ -163,7 +166,7 @@ describe('makeNewHttpsTransport()', () => { }, }); - const transport = makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + const transport = makeNodeTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); const transportResponse = await transport.send(EVENT_ENVELOPE); expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' })); @@ -172,7 +175,7 @@ describe('makeNewHttpsTransport()', () => { it('should use `caCerts` option', async () => { await setupTestServer({ statusCode: SUCCESS }); - const transport = makeNewHttpsTransport({ + const transport = makeNodeTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL, caCerts: 'some cert', @@ -192,33 +195,33 @@ describe('makeNewHttpsTransport()', () => { describe('proxy', () => { it('can be configured through option', () => { - makeNewHttpsTransport({ + makeNodeTransport({ httpModule: unsafeHttpsModule, - url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', - proxy: 'http://example.com', + url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + proxy: 'https://example.com', }); expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('http://example.com'); + expect(httpProxyAgent).toHaveBeenCalledWith('https://example.com'); }); it('can be configured through env variables option (http)', () => { - process.env.http_proxy = 'http://example.com'; - makeNewHttpsTransport({ + process.env.http_proxy = 'https://example.com'; + makeNodeTransport({ httpModule: unsafeHttpsModule, - url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('http://example.com'); + expect(httpProxyAgent).toHaveBeenCalledWith('https://example.com'); delete process.env.http_proxy; }); it('can be configured through env variables option (https)', () => { process.env.https_proxy = 'https://example.com'; - makeNewHttpsTransport({ + makeNodeTransport({ httpModule: unsafeHttpsModule, - url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); expect(httpProxyAgent).toHaveBeenCalledTimes(1); @@ -227,24 +230,24 @@ describe('makeNewHttpsTransport()', () => { }); it('client options have priority over env variables', () => { - process.env.https_proxy = 'http://foo.com'; - makeNewHttpsTransport({ + process.env.https_proxy = 'https://foo.com'; + makeNodeTransport({ httpModule: unsafeHttpsModule, - url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', - proxy: 'http://bar.com', + url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + proxy: 'https://bar.com', }); expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('http://bar.com'); + expect(httpProxyAgent).toHaveBeenCalledWith('https://bar.com'); delete process.env.https_proxy; }); it('no_proxy allows for skipping specific hosts', () => { process.env.no_proxy = 'sentry.io'; - makeNewHttpsTransport({ + makeNodeTransport({ httpModule: unsafeHttpsModule, - url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', - proxy: 'http://example.com', + url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + proxy: 'https://example.com', }); expect(httpProxyAgent).not.toHaveBeenCalled(); @@ -253,12 +256,12 @@ describe('makeNewHttpsTransport()', () => { }); it('no_proxy works with a port', () => { - process.env.http_proxy = 'http://example.com:8080'; + process.env.http_proxy = 'https://example.com:8080'; process.env.no_proxy = 'sentry.io:8989'; - makeNewHttpsTransport({ + makeNodeTransport({ httpModule: unsafeHttpsModule, - url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); expect(httpProxyAgent).not.toHaveBeenCalled(); @@ -268,12 +271,12 @@ describe('makeNewHttpsTransport()', () => { }); it('no_proxy works with multiple comma-separated hosts', () => { - process.env.http_proxy = 'http://example.com:8080'; + process.env.http_proxy = 'https://example.com:8080'; process.env.no_proxy = 'example.com,sentry.io,wat.com:1337'; - makeNewHttpsTransport({ + makeNodeTransport({ httpModule: unsafeHttpsModule, - url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', + url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); expect(httpProxyAgent).not.toHaveBeenCalled(); @@ -292,7 +295,7 @@ describe('makeNewHttpsTransport()', () => { }, }); - makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + makeNodeTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; const executorResult = registeredRequestExecutor({ @@ -316,7 +319,7 @@ describe('makeNewHttpsTransport()', () => { statusCode: SUCCESS, }); - makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + makeNodeTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; const executorResult = registeredRequestExecutor({ @@ -344,7 +347,7 @@ describe('makeNewHttpsTransport()', () => { }, }); - makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + makeNodeTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; const executorResult = registeredRequestExecutor({ @@ -372,7 +375,7 @@ describe('makeNewHttpsTransport()', () => { }, }); - makeNewHttpsTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); + makeNodeTransport({ httpModule: unsafeHttpsModule, url: TEST_SERVER_URL }); const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1]; const executorResult = registeredRequestExecutor({ From 3249d424ea228badab350c3e33f8d04c7dd4104b Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 29 Mar 2022 07:20:06 +0000 Subject: [PATCH 15/19] Add todo for v7 to rename imports --- packages/node/test/transports/new/http.test.ts | 1 + packages/node/test/transports/new/https.test.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/packages/node/test/transports/new/http.test.ts b/packages/node/test/transports/new/http.test.ts index 8c39b868a24e..b3ce46d5a542 100644 --- a/packages/node/test/transports/new/http.test.ts +++ b/packages/node/test/transports/new/http.test.ts @@ -3,6 +3,7 @@ import { EventEnvelope, EventItem } from '@sentry/types'; import { createEnvelope, serializeEnvelope } from '@sentry/utils'; import * as http from 'http'; +// TODO(v7): We're renaming the imported file so this needs to be changed as well import { makeNodeTransport } from '../../../src/transports/new'; jest.mock('@sentry/core', () => { diff --git a/packages/node/test/transports/new/https.test.ts b/packages/node/test/transports/new/https.test.ts index 2c7af5c453d3..7784e16c65df 100644 --- a/packages/node/test/transports/new/https.test.ts +++ b/packages/node/test/transports/new/https.test.ts @@ -5,6 +5,7 @@ import * as http from 'http'; import * as https from 'https'; import { HTTPModule, HTTPModuleRequestIncomingMessage } from '../../../src/transports/base/http-module'; +// TODO(v7): We're renaming the imported file so this needs to be changed as well import { makeNodeTransport } from '../../../src/transports/new'; import testServerCerts from './test-server-certs'; From 3ecd702a83f73e3ce45b0d5c954f62a42b2a3332 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 29 Mar 2022 07:51:29 +0000 Subject: [PATCH 16/19] Set keepAlive to false and add comment to true it in v7 --- packages/node/src/transports/new.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index 2b3df25c4e01..1a202df24bac 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -48,7 +48,8 @@ export function makeNodeTransport(options: NodeTransportOptions): NewTransport { const agent = proxy ? (new (require('https-proxy-agent'))(proxy) as http.Agent) - : new nativeHttpModule.Agent({ keepAlive: true, maxSockets: 30, timeout: 2000 }); + : // TODO(v7): Set keepAlive to true. Older versions of node had memory leaks when using it: #2555 + new nativeHttpModule.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 }); const requestExecutor = createRequestExecutor(options, options.httpModule ?? nativeHttpModule, agent); return createTransport({ bufferSize: options.bufferSize }, requestExecutor); From 53e0fc0fe8b85fc2026ee1eb9beeec2d0e64a7cf Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 29 Mar 2022 10:10:01 +0000 Subject: [PATCH 17/19] Update keepAlive todo --- packages/node/src/transports/new.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index 1a202df24bac..676f64ce35fe 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -46,10 +46,11 @@ export function makeNodeTransport(options: NodeTransportOptions): NewTransport { const nativeHttpModule = isHttps ? https : http; + // TODO(v7): Evaluate if we can set keepAlive to true. This would involve testing for memory leaks in older node + // versions(>= 8) as they had memory leaks when using it: #2555 const agent = proxy ? (new (require('https-proxy-agent'))(proxy) as http.Agent) - : // TODO(v7): Set keepAlive to true. Older versions of node had memory leaks when using it: #2555 - new nativeHttpModule.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 }); + : new nativeHttpModule.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 }); const requestExecutor = createRequestExecutor(options, options.httpModule ?? nativeHttpModule, agent); return createTransport({ bufferSize: options.bufferSize }, requestExecutor); From dd939fffc72a6502f989593e75e7b71df0eeb393 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 29 Mar 2022 16:13:44 +0000 Subject: [PATCH 18/19] Directly pass object into makeNodeTransport --- packages/node/src/backend.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index a3827fe5e226..f319673ebc18 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -3,7 +3,7 @@ import { Event, EventHint, Severity, Transport, TransportOptions } from '@sentry import { makeDsn, resolvedSyncPromise } from '@sentry/utils'; import { eventFromMessage, eventFromUnknownInput } from './eventbuilder'; -import { HTTPSTransport, HTTPTransport, makeNodeTransport, NodeTransportOptions } from './transports'; +import { HTTPSTransport, HTTPTransport, makeNodeTransport } from './transports'; import { NodeOptions } from './types'; /** @@ -54,14 +54,12 @@ export class NodeBackend extends BaseBackend { const api = initAPIDetails(transportOptions.dsn, transportOptions._metadata, transportOptions.tunnel); const url = getEnvelopeEndpointWithUrlEncodedAuth(api.dsn, api.tunnel); - const newTransportOptions: NodeTransportOptions = { + this._newTransport = makeNodeTransport({ url, headers: transportOptions.headers, proxy: transportOptions.httpProxy, caCerts: transportOptions.caCerts, - }; - - this._newTransport = makeNodeTransport(newTransportOptions); + }); if (dsn.protocol === 'http') { return new HTTPTransport(transportOptions); From 339062cf8d8c39ed1ac7e0d33a84113f5e7cf9dc Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 29 Mar 2022 16:16:16 +0000 Subject: [PATCH 19/19] Clarify proxy order for http --- packages/node/src/transports/new.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/node/src/transports/new.ts b/packages/node/src/transports/new.ts index 676f64ce35fe..b30dc3ffe36f 100644 --- a/packages/node/src/transports/new.ts +++ b/packages/node/src/transports/new.ts @@ -38,6 +38,7 @@ export function makeNodeTransport(options: NodeTransportOptions): NewTransport { const urlSegments = new URL(options.url); const isHttps = urlSegments.protocol === 'https:'; + // Proxy prioritization: http => `options.proxy` | `process.env.http_proxy` // Proxy prioritization: https => `options.proxy` | `process.env.https_proxy` | `process.env.http_proxy` const proxy = applyNoProxyOption( urlSegments,