fix(sveltekit): Fix file system race condition in source map cleaning#19714
fix(sveltekit): Fix file system race condition in source map cleaning#19714
Conversation
…leaning Replace `existsSync` guard with try/catch around read+write operations to eliminate the time-of-check to time-of-use race condition (CWE-367) flagged by CodeQL (code-scanning alert #439). Co-Authored-By: Claude Opus 4.6 <[email protected]>
size-limit report 📦
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Overly broad catch silently swallows non-ENOENT errors
- Changed catch block to check for ENOENT errors specifically and log unexpected errors in debug mode, following the existing pattern in the same file.
- ✅ Fixed: Fix PR missing regression test for the change
- Added two regression tests that verify ENOENT errors are handled gracefully and unexpected errors are logged when debug is enabled.
Or push these changes by commenting:
@cursor push 96a66011b4
Preview (96a66011b4)
diff --git a/packages/sveltekit/src/vite/sourceMaps.ts b/packages/sveltekit/src/vite/sourceMaps.ts
--- a/packages/sveltekit/src/vite/sourceMaps.ts
+++ b/packages/sveltekit/src/vite/sourceMaps.ts
@@ -221,8 +221,12 @@
'',
);
await fs.promises.writeFile(mapFile, cleanedMapContent);
- } catch {
- // Map file doesn't exist, nothing to clean
+ } catch (e) {
+ const isKnownError = e instanceof Error && e.message.includes('ENOENT: no such file or directory, open');
+ if (debug && !isKnownError) {
+ // eslint-disable-next-line no-console
+ console.error('[Source Maps Plugin] error while cleaning source map', mapFile, e);
+ }
}
}
diff --git a/packages/sveltekit/test/vite/sourceMaps.test.ts b/packages/sveltekit/test/vite/sourceMaps.test.ts
--- a/packages/sveltekit/test/vite/sourceMaps.test.ts
+++ b/packages/sveltekit/test/vite/sourceMaps.test.ts
@@ -1,4 +1,5 @@
import { sentryVitePlugin } from '@sentry/vite-plugin';
+import * as fs from 'fs';
import type { Plugin } from 'vite';
import * as vite from 'vite';
import { beforeEach, describe, expect, it, vi } from 'vitest';
@@ -37,6 +38,20 @@
};
});
+vi.mock('fs', async () => {
+ const actual = (await vi.importActual('fs')) as typeof fs;
+ return {
+ ...actual,
+ existsSync: vi.fn(actual.existsSync),
+ readdirSync: vi.fn(actual.readdirSync),
+ promises: {
+ ...actual.promises,
+ readFile: vi.fn(actual.promises.readFile),
+ writeFile: vi.fn(actual.promises.writeFile),
+ },
+ };
+});
+
beforeEach(() => {
vi.clearAllMocks();
});
@@ -212,6 +227,57 @@
expect(consoleWarnSpy).toHaveBeenCalledWith(expect.stringContaining('Failed to upload source maps'));
expect(consoleLogSpy).toHaveBeenCalled();
});
+
+ it('handles missing map files gracefully during source map cleaning', async () => {
+ const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
+
+ vi.mocked(fs.existsSync).mockReturnValueOnce(true);
+ vi.mocked(fs.readdirSync).mockReturnValueOnce([{ name: 'test.js', isDirectory: () => false } as any]);
+ vi.mocked(fs.promises.readFile).mockRejectedValueOnce(
+ Object.assign(new Error('ENOENT: no such file or directory, open'), { code: 'ENOENT' }),
+ );
+
+ const plugin = await getSentryViteSubPlugin('sentry-sveltekit-debug-id-upload-plugin');
+
+ // @ts-expect-error this function exists!
+ plugin.configResolved({ build: { ssr: true } });
+ // @ts-expect-error this function exists!
+ await plugin.closeBundle();
+
+ expect(consoleErrorSpy).not.toHaveBeenCalled();
+ });
+
+ it('logs unexpected errors during source map cleaning when debug is enabled', async () => {
+ const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
+
+ vi.mocked(fs.existsSync).mockReturnValueOnce(true);
+ vi.mocked(fs.readdirSync).mockReturnValueOnce([{ name: 'test.js', isDirectory: () => false } as any]);
+ vi.mocked(fs.promises.readFile).mockRejectedValueOnce(new Error('Permission denied'));
+
+ const plugins = await makeCustomSentryVitePlugins(
+ {
+ authToken: 'token',
+ org: 'org',
+ project: 'project',
+ adapter: 'other',
+ debug: true,
+ },
+ { kit: {} },
+ );
+
+ const plugin = plugins.find(p => p.name === 'sentry-sveltekit-debug-id-upload-plugin');
+
+ // @ts-expect-error this function exists!
+ plugin.configResolved({ build: { ssr: true } });
+ // @ts-expect-error this function exists!
+ await plugin.closeBundle();
+
+ expect(consoleErrorSpy).toHaveBeenCalledWith(
+ expect.stringContaining('[Source Maps Plugin] error while cleaning source map'),
+ expect.any(String),
+ expect.any(Error),
+ );
+ });
});
describe('_getUpdatedSourceMapSettings', () => {| ); | ||
| await fs.promises.writeFile(mapFile, cleanedMapContent); | ||
| } catch { | ||
| // Map file doesn't exist, nothing to clean |
There was a problem hiding this comment.
Overly broad catch silently swallows non-ENOENT errors
Medium Severity
The new catch block silently swallows all errors, not just ENOENT (file not found). If readFile or writeFile fails due to permission errors, disk full, or other I/O issues, the failure is silently ignored. The try/catch block just above (lines 202–211) demonstrates the better pattern already used in this file: it checks whether the error is a known ENOENT and logs unexpected errors when debug is enabled. The same approach would be appropriate here.
Additional Locations (1)
| ); | ||
| await fs.promises.writeFile(mapFile, cleanedMapContent); | ||
| } catch { | ||
| // Map file doesn't exist, nothing to clean |
There was a problem hiding this comment.
Fix PR missing regression test for the change
Low Severity
This is a fix PR but no regression test was added to verify the race condition fix. The existing test file sourceMaps.test.ts has no tests covering the source map file cleaning logic (the readFile/writeFile path for .map files). A test verifying behavior when the map file doesn't exist — and ideally when other I/O errors occur — would help prevent regressions.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Lms24
left a comment
There was a problem hiding this comment.
good catch! Looks reasonable and pragmatic to me, thanks for fixing!



Replace
existsSyncguard with try/catch around read+write operations to eliminate the time-of-check to time-of-use race condition (CWE-367) flagged by CodeQL (code-scanning alert #439).closes https://github.com/getsentry/sentry-javascript/security/code-scanning/439