Skip to content

Commit 4e745bd

Browse files
antonisclaude
andcommitted
fix(core): Canonicalize paths via realpath; add escaping regression test
- metroMiddleware: run realpath on both the allowed roots and each frame filename so a symlink inside an allowed root pointing outside cannot escape the containment check. Reject frames whose realpath fails. - sentryReleaseInjector: add a test asserting JSON.stringify escaping, so a future refactor cannot silently regress to unescaped interpolation. - Drop unneeded config cast now that InputConfigT exposes projectRoot and watchFolders directly. Addresses review feedback on #6044. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 30ceb95 commit 4e745bd

3 files changed

Lines changed: 114 additions & 14 deletions

File tree

packages/core/src/js/tools/metroMiddleware.ts

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import type { IncomingMessage, ServerResponse } from 'http';
33
import type { InputConfigT, Middleware } from 'metro-config';
44

55
import { addContextToFrame, debug } from '@sentry/core';
6-
import { readFile } from 'fs';
6+
import { readFile, realpath, realpathSync } from 'fs';
77
import * as path from 'path';
88
import { promisify } from 'util';
99

@@ -12,17 +12,26 @@ import { getRawBody } from '../metro/getRawBody';
1212
import { openURLMiddleware } from '../metro/openUrlMiddleware';
1313

1414
const readFileAsync = promisify(readFile);
15+
const realpathAsync = promisify(realpath);
1516

1617
/**
1718
* Accepts Sentry formatted stack frames and
1819
* adds source context to the in app frames.
1920
*
2021
* Relative filenames are resolved against the first entry in `allowedRoots`.
21-
* A resolved filename must be contained in at least one of `allowedRoots`
22-
* (project root plus any Metro `watchFolders`).
22+
* Both the resolved filename and the allowed roots are canonicalized via
23+
* `fs.realpath`, so a symlink inside an allowed root pointing outside of it
24+
* cannot escape the containment check.
2325
*/
2426
export const createStackFramesContextMiddleware = (allowedRoots: string[]): Middleware => {
25-
const normalizedRoots = allowedRoots.map(root => path.resolve(root));
27+
const canonicalRoots = allowedRoots.map(root => {
28+
const resolved = path.resolve(root);
29+
try {
30+
return realpathSync(resolved);
31+
} catch {
32+
return resolved;
33+
}
34+
});
2635

2736
return async (request: IncomingMessage, response: ServerResponse, _next: () => void): Promise<void> => {
2837
debug.log('[@sentry/react-native/metro] Received request for stack frames context.');
@@ -47,15 +56,15 @@ export const createStackFramesContextMiddleware = (allowedRoots: string[]): Midd
4756
return;
4857
}
4958

50-
const stackWithSourceContext = await Promise.all(stack.map(frame => addSourceContext(frame, normalizedRoots)));
59+
const stackWithSourceContext = await Promise.all(stack.map(frame => addSourceContext(frame, canonicalRoots)));
5160
response.setHeader('Content-Type', 'application/json');
5261
response.statusCode = 200;
5362
response.end(JSON.stringify({ stack: stackWithSourceContext }));
5463
debug.log('[@sentry/react-native/metro] Sent stack frames context.');
5564
};
5665
};
5766

58-
async function addSourceContext(frame: StackFrame, allowedRoots: string[]): Promise<StackFrame> {
67+
async function addSourceContext(frame: StackFrame, canonicalRoots: string[]): Promise<StackFrame> {
5968
if (!frame.in_app) {
6069
return frame;
6170
}
@@ -66,22 +75,30 @@ async function addSourceContext(frame: StackFrame, allowedRoots: string[]): Prom
6675
return frame;
6776
}
6877

69-
if (allowedRoots.length === 0) {
78+
if (canonicalRoots.length === 0) {
7079
debug.warn('[@sentry/react-native/metro] Skipping frame: no allowed roots configured.');
7180
return frame;
7281
}
7382

74-
const resolvedPath = path.resolve(allowedRoots[0]!, frame.filename);
75-
const isInside = allowedRoots.some(root => {
76-
const relative = path.relative(root, resolvedPath);
83+
const resolvedPath = path.resolve(canonicalRoots[0]!, frame.filename);
84+
let canonicalPath: string;
85+
try {
86+
canonicalPath = await realpathAsync(resolvedPath);
87+
} catch {
88+
debug.warn('[@sentry/react-native/metro] Skipping frame: could not canonicalize filename.');
89+
return frame;
90+
}
91+
92+
const isInside = canonicalRoots.some(root => {
93+
const relative = path.relative(root, canonicalPath);
7794
return relative !== '' && !relative.startsWith('..') && !path.isAbsolute(relative);
7895
});
7996
if (!isInside) {
8097
debug.warn('[@sentry/react-native/metro] Skipping frame whose filename is outside the allowed roots.');
8198
return frame;
8299
}
83100

84-
const source = await readFileAsync(resolvedPath, { encoding: 'utf8' });
101+
const source = await readFileAsync(canonicalPath, { encoding: 'utf8' });
85102
const lines = source.split('\n');
86103
addContextToFrame(lines, frame);
87104
} catch (error) {
@@ -127,9 +144,8 @@ export const withSentryMiddleware = (config: InputConfigT): InputConfigT => {
127144
config.server = {};
128145
}
129146

130-
const typedConfig = config as { projectRoot?: string; watchFolders?: readonly string[] };
131-
const projectRoot = typedConfig.projectRoot || process.cwd();
132-
const watchFolders = typedConfig.watchFolders || [];
147+
const projectRoot = config.projectRoot || process.cwd();
148+
const watchFolders = config.watchFolders || [];
133149
const allowedRoots = [projectRoot, ...watchFolders];
134150

135151
const originalEnhanceMiddleware = config.server.enhanceMiddleware;

packages/core/test/tools/metroMiddleware.test.ts

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ jest.mock('../../src/js/tools/metroMiddleware', () => jest.requireActual('../../
1515
jest.mock('fs', () => {
1616
return {
1717
readFile: jest.fn(),
18+
realpath: jest.fn((p: string, cb: (err: NodeJS.ErrnoException | null, resolved: string) => void) => cb(null, p)),
19+
realpathSync: jest.fn((p: string) => p),
1820
};
1921
});
2022

@@ -130,6 +132,12 @@ describe('metroMiddleware', () => {
130132
let testData: string = '';
131133

132134
beforeEach(() => {
135+
// afterEach resetAllMocks wipes the default fs impls installed via jest.mock, so reinstate.
136+
(fs.realpath as unknown as jest.Mock).mockImplementation(
137+
(p: string, cb: (err: NodeJS.ErrnoException | null, resolved: string) => void) => cb(null, p),
138+
);
139+
(fs.realpathSync as unknown as jest.Mock).mockImplementation((p: string) => p);
140+
133141
request = {
134142
setEncoding: jest.fn(),
135143
on: jest.fn((event: string, cb: (data?: string) => void) => {
@@ -371,6 +379,61 @@ describe('metroMiddleware', () => {
371379
});
372380
});
373381

382+
it('should skip frames whose realpath resolves outside the allowed roots', async () => {
383+
const realpathSpy = jest.spyOn(fs, 'realpath') as unknown as jest.Mock;
384+
const readFileSpy = jest.spyOn(fs, 'readFile');
385+
386+
// Simulate a symlink: the file lives at <project>/link/file.js inside the project,
387+
// but its realpath is /etc/shadow — outside every allowed root.
388+
realpathSpy.mockImplementationOnce(
389+
(_p: string, cb: (err: NodeJS.ErrnoException | null, resolved: string) => void) => cb(null, '/etc/shadow'),
390+
);
391+
392+
testData = JSON.stringify({
393+
stack: [
394+
{
395+
in_app: true,
396+
filename: 'link/file.js',
397+
function: 'testFunction',
398+
lineno: 1,
399+
colno: 1,
400+
},
401+
],
402+
} satisfies { stack: StackFrame[] });
403+
404+
await stackFramesContextMiddleware(request, response, next);
405+
406+
expect(readFileSpy).not.toHaveBeenCalled();
407+
expect(response.statusCode).toBe(200);
408+
});
409+
410+
it('should skip frames whose realpath cannot be resolved', async () => {
411+
const realpathSpy = jest.spyOn(fs, 'realpath') as unknown as jest.Mock;
412+
const readFileSpy = jest.spyOn(fs, 'readFile');
413+
414+
const enoent: NodeJS.ErrnoException = Object.assign(new Error('ENOENT'), { code: 'ENOENT' });
415+
realpathSpy.mockImplementationOnce(
416+
(_p: string, cb: (err: NodeJS.ErrnoException | null, resolved: string) => void) => cb(enoent, ''),
417+
);
418+
419+
testData = JSON.stringify({
420+
stack: [
421+
{
422+
in_app: true,
423+
filename: 'missing.js',
424+
function: 'testFunction',
425+
lineno: 1,
426+
colno: 1,
427+
},
428+
],
429+
} satisfies { stack: StackFrame[] });
430+
431+
await stackFramesContextMiddleware(request, response, next);
432+
433+
expect(readFileSpy).not.toHaveBeenCalled();
434+
expect(response.statusCode).toBe(200);
435+
});
436+
374437
it('should handle mixed frame types correctly', async () => {
375438
const readFileSpy = jest.spyOn(fs, 'readFile');
376439
mockReadFileOnce(readFileSpy, path.join(TEST_PROJECT_ROOT, 'app1.js'), 'line1\nline2\nline3\nline4\nline5');

packages/core/test/tools/sentryReleaseInjector.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,4 +47,25 @@ describe('Sentry Release Injector', () => {
4747
'var SENTRY_RELEASE;SENTRY_RELEASE={name: "TestApp", version: "1.0.0"};',
4848
);
4949
});
50+
51+
test('unstableReleaseConstantsPlugin escapes values that would break out of the string literal', () => {
52+
mockedExpoConfigRequire.mockReturnValue({
53+
exp: {
54+
name: 'App", evil();//',
55+
version: '1.0\\"',
56+
},
57+
});
58+
const projectRoot = '/some/project/root';
59+
const graph = {
60+
transformOptions: { platform: 'web' },
61+
} as unknown as ReadOnlyGraph<MixedOutput>;
62+
const premodules = [{ path: 'someModule.js' }] as Module[];
63+
64+
const plugin = unstableReleaseConstantsPlugin(projectRoot);
65+
const result = plugin({ graph, premodules });
66+
67+
expect(result[0]?.getSource().toString()).toEqual(
68+
'var SENTRY_RELEASE;SENTRY_RELEASE={name: "App\\", evil();//", version: "1.0\\\\\\""};',
69+
);
70+
});
5071
});

0 commit comments

Comments
 (0)