Skip to content

feat(node): Capture SystemError context and remove paths from message #17331

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

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import * as Sentry from '@sentry/node-core';
import { loggingTransport } from '@sentry-internal/node-integration-tests';
import { readFileSync } from 'fs';

Sentry.init({
dsn: 'https://[email protected]/1337',
transport: loggingTransport,
sendDefaultPii: true,
});

readFileSync('non-existent-file.txt');
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import * as Sentry from '@sentry/node-core';
import { loggingTransport } from '@sentry-internal/node-integration-tests';
import { readFileSync } from 'fs';

Sentry.init({
dsn: 'https://[email protected]/1337',
transport: loggingTransport,
});

readFileSync('non-existent-file.txt');
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import { afterAll, describe, test } from 'vitest';
import { cleanupChildProcesses, createRunner } from '../../utils/runner';

afterAll(() => {
cleanupChildProcesses();
});

describe('SystemError integration', () => {
test('sendDefaultPii: false', async () => {
await createRunner(__dirname, 'basic.mjs')
.expect({
event: {
contexts: {
node_system_error: {
errno: -2,
code: 'ENOENT',
syscall: 'open',
},
},
exception: {
values: [
{
type: 'Error',
value: 'ENOENT: no such file or directory, open',
},
],
},
},
})
.start()
.completed();
});

test('sendDefaultPii: true', async () => {
await createRunner(__dirname, 'basic-pii.mjs')
.expect({
event: {
contexts: {
node_system_error: {
errno: -2,
code: 'ENOENT',
syscall: 'open',
path: 'non-existent-file.txt',
},
},
exception: {
values: [
{
type: 'Error',
value: 'ENOENT: no such file or directory, open',
},
],
},
},
})
.start()
.completed();
});
});
1 change: 1 addition & 0 deletions packages/node-core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export { onUnhandledRejectionIntegration } from './integrations/onunhandledrejec
export { anrIntegration, disableAnrDetectionForCallback } from './integrations/anr';

export { spotlightIntegration } from './integrations/spotlight';
export { systemErrorIntegration } from './integrations/systemError';
export { childProcessIntegration } from './integrations/childProcess';
export { createSentryWinstonTransport } from './integrations/winston';

Expand Down
69 changes: 69 additions & 0 deletions packages/node-core/src/integrations/systemError.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { defineIntegration } from '@sentry/core';
import { getSystemErrorMap } from 'util';

const INTEGRATION_NAME = 'NodeSystemError';

type SystemErrorContext = {
dest?: string; // If present, the file path destination when reporting a file system error
errno: number; // The system-provided error number
path?: string; // If present, the file path when reporting a file system error
};

type SystemError = Error & SystemErrorContext;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Incomplete Error Context Causes Type Mismatch

The NodeSystemError integration's SystemErrorContext type is incomplete, missing code and syscall properties found on Node.js SystemError objects. The errorContext is populated using a spread operator (...error), which copies all enumerable properties from the original Error object. This creates a type mismatch between the declared type and the actual captured data. It also captures unintended properties (e.g., message, stack, name, code, syscall), potentially exposing sensitive data beyond what sendDefaultPii is designed to filter (path, dest).

Additional Locations (1)
Fix in Cursor Fix in Web

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The others dont add anything to the type because we're not using them


function isSystemError(error: unknown): error is SystemError {
if (!(error instanceof Error)) {
return false;
}

if (!('errno' in error) || typeof error.errno !== 'number') {
return false;
}

// Appears this is the recommended way to check for Node.js SystemError
// https://github.com/nodejs/node/issues/46869
return getSystemErrorMap().has(error.errno);
}

/**
* Captures context for Node.js SystemError errors.
*/
export const systemErrorIntegration = defineIntegration(() => {
return {
name: INTEGRATION_NAME,
processEvent: (event, hint, client) => {
if (!isSystemError(hint.originalException)) {
return event;
}

const error = hint.originalException;

const errorContext: SystemErrorContext = {
...error,
};

if (!client.getOptions().sendDefaultPii) {
delete errorContext.path;
delete errorContext.dest;
}

event.contexts = {
...event.contexts,
node_system_error: errorContext,
};

for (const exception of event.exception?.values || []) {
if (exception.value) {
if (error.path && exception.value.includes(error.path)) {
exception.value = exception.value.replace(`'${error.path}'`, '').trim();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should expose an integration option to control this replacement as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally I would have used util.getSystemErrorMessage(err) to get the plain message without the paths but it was only added in Node v22

}
if (error.dest && exception.value.includes(error.dest)) {
exception.value = exception.value.replace(`'${error.dest}'`, '').trim();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Error Message Parsing Logic Mismatch

Inconsistent logic for stripping path and dest values from exception messages. The includes check for error.path (or error.dest) looks for the value without quotes, but the subsequent replace operation attempts to remove the value wrapped in single quotes. This mismatch prevents the path/dest from being removed if it appears unquoted in the error message, impacting error fingerprinting.

Fix in Cursor Fix in Web

}
}

return event;
},
};
});
2 changes: 2 additions & 0 deletions packages/node-core/src/sdk/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { onUncaughtExceptionIntegration } from '../integrations/onuncaughtexcept
import { onUnhandledRejectionIntegration } from '../integrations/onunhandledrejection';
import { processSessionIntegration } from '../integrations/processSession';
import { INTEGRATION_NAME as SPOTLIGHT_INTEGRATION_NAME, spotlightIntegration } from '../integrations/spotlight';
import { systemErrorIntegration } from '../integrations/systemError';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll have to export this from all of our node packages as well

import { makeNodeTransport } from '../transports';
import type { NodeClientOptions, NodeOptions } from '../types';
import { isCjs } from '../utils/commonjs';
Expand All @@ -52,6 +53,7 @@ export function getDefaultIntegrations(): Integration[] {
functionToStringIntegration(),
linkedErrorsIntegration(),
requestDataIntegration(),
systemErrorIntegration(),
// Native Wrappers
consoleIntegration(),
httpIntegration(),
Expand Down
Loading