Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(node): Guard against invalid maxSpanWaitDuration values #14632

Merged
merged 2 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion packages/node/src/sdk/initOtel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
import { GLOBAL_OBJ, SDK_VERSION, consoleSandbox, logger } from '@sentry/core';
import { SentryPropagator, SentrySampler, SentrySpanProcessor } from '@sentry/opentelemetry';
import { createAddHookMessageChannel } from 'import-in-the-middle';
import { DEBUG_BUILD } from '../debug-build';
import { getOpenTelemetryInstrumentationToPreload } from '../integrations/tracing';
import { SentryContextManager } from '../otel/contextManager';
import type { EsmLoaderHookOptions } from '../types';
Expand All @@ -18,6 +19,9 @@ import type { NodeClient } from './client';

declare const __IMPORT_META_URL_REPLACEMENT__: string;

// About 277h - this must fit into new Array(len)!
const MAX_MAX_SPAN_WAIT_DURATION = 1_000_000;

/**
* Initialize OpenTelemetry for Node.
*/
Expand Down Expand Up @@ -138,7 +142,7 @@ export function setupOtel(client: NodeClient): BasicTracerProvider {
forceFlushTimeoutMillis: 500,
spanProcessors: [
new SentrySpanProcessor({
timeout: client.getOptions().maxSpanWaitDuration,
timeout: _clampSpanProcessorTimeout(client.getOptions().maxSpanWaitDuration),
}),
],
});
Expand All @@ -152,6 +156,26 @@ export function setupOtel(client: NodeClient): BasicTracerProvider {
return provider;
}

/** Just exported for tests. */
export function _clampSpanProcessorTimeout(maxSpanWaitDuration: number | undefined): number | undefined {
if (maxSpanWaitDuration == null) {
return undefined;
}

// We guard for a max. value here, because we create an array with this length
// So if this value is too large, this would fail
if (maxSpanWaitDuration > MAX_MAX_SPAN_WAIT_DURATION) {
DEBUG_BUILD &&
logger.warn(`\`maxSpanWaitDuration\` is too high, using the maximum value of ${MAX_MAX_SPAN_WAIT_DURATION}`);
return MAX_MAX_SPAN_WAIT_DURATION;
} else if (maxSpanWaitDuration <= 0 || Number.isNaN(maxSpanWaitDuration)) {
DEBUG_BUILD && logger.warn('`maxSpanWaitDuration` must be a positive number, using default value instead.');
return undefined;
}

return maxSpanWaitDuration;
}

/**
* Setup the OTEL logger to use our own logger.
*/
Expand Down
78 changes: 78 additions & 0 deletions packages/node/test/sdk/initOtel.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import { logger } from '@sentry/core';
import { _clampSpanProcessorTimeout } from '../../src/sdk/initOtel';

describe('_clampSpanProcessorTimeout', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('works with undefined', () => {
const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
const timeout = _clampSpanProcessorTimeout(undefined);
expect(timeout).toBe(undefined);
expect(loggerWarnSpy).not.toHaveBeenCalled();
});

it('works with positive number', () => {
const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
const timeout = _clampSpanProcessorTimeout(10);
expect(timeout).toBe(10);
expect(loggerWarnSpy).not.toHaveBeenCalled();
});

it('works with 0', () => {
const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
const timeout = _clampSpanProcessorTimeout(0);
expect(timeout).toBe(undefined);
expect(loggerWarnSpy).toHaveBeenCalledTimes(1);
expect(loggerWarnSpy).toHaveBeenCalledWith(
'`maxSpanWaitDuration` must be a positive number, using default value instead.',
);
});

it('works with negative number', () => {
const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
const timeout = _clampSpanProcessorTimeout(-10);
expect(timeout).toBe(undefined);
expect(loggerWarnSpy).toHaveBeenCalledTimes(1);
expect(loggerWarnSpy).toHaveBeenCalledWith(
'`maxSpanWaitDuration` must be a positive number, using default value instead.',
);
});

it('works with -Infinity', () => {
const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
const timeout = _clampSpanProcessorTimeout(-Infinity);
expect(timeout).toBe(undefined);
expect(loggerWarnSpy).toHaveBeenCalledTimes(1);
expect(loggerWarnSpy).toHaveBeenCalledWith(
'`maxSpanWaitDuration` must be a positive number, using default value instead.',
);
});

it('works with Infinity', () => {
const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
const timeout = _clampSpanProcessorTimeout(Infinity);
expect(timeout).toBe(1_000_000);
expect(loggerWarnSpy).toHaveBeenCalledTimes(1);
expect(loggerWarnSpy).toHaveBeenCalledWith('`maxSpanWaitDuration` is too high, using the maximum value of 1000000');
});

it('works with large number', () => {
const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
const timeout = _clampSpanProcessorTimeout(1_000_000_000);
expect(timeout).toBe(1_000_000);
expect(loggerWarnSpy).toHaveBeenCalledTimes(1);
expect(loggerWarnSpy).toHaveBeenCalledWith('`maxSpanWaitDuration` is too high, using the maximum value of 1000000');
});

it('works with NaN', () => {
const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
const timeout = _clampSpanProcessorTimeout(NaN);
expect(timeout).toBe(undefined);
expect(loggerWarnSpy).toHaveBeenCalledTimes(1);
expect(loggerWarnSpy).toHaveBeenCalledWith(
'`maxSpanWaitDuration` must be a positive number, using default value instead.',
);
});
});
Loading