Skip to content

Commit 5d3f546

Browse files
zaldihzaldih
authored
Merge pull request #204 from voidcosmos/improve-isdangerous-algorithm
The patterns used so far was rather conservative and yielded "false positives". Of course, this is better than "false negatives". The algorithm has been improved to not mark as ‘dangerous’ directories that would not have any dangerous consequences if deleted (like npm caches and so on).
2 parents 3a56179 + c731199 commit 5d3f546

2 files changed

Lines changed: 165 additions & 40 deletions

File tree

‎__tests__/files.service.test.ts‎

Lines changed: 107 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,9 @@ describe('File Service', () => {
130130
fileService.isSafeToDelete('/one/node_/ro/modules', target),
131131
).toBeFalsy();
132132
expect(fileService.isSafeToDelete('nodemodules', target)).toBeFalsy();
133+
expect(fileService.isSafeToDelete('/', target)).toBeFalsy();
134+
expect(fileService.isSafeToDelete('/home', target)).toBeFalsy();
135+
expect(fileService.isSafeToDelete('/home/user', target)).toBeFalsy();
133136
});
134137

135138
it('should get true if is safe to delete ', () => {
@@ -142,39 +145,112 @@ describe('File Service', () => {
142145
});
143146
});
144147

145-
describe('#isDangerous', () => {
146-
it('should return false for paths that are not considered dangerous', () => {
147-
expect(
148-
fileService.isDangerous('/home/apps/myapp/node_modules'),
149-
).toBeFalsy();
150-
expect(fileService.isDangerous('node_modules')).toBeFalsy();
151-
expect(
152-
fileService.isDangerous('/home/user/projects/a/node_modules'),
153-
).toBeFalsy();
154-
expect(
155-
fileService.isDangerous('/Applications/NotAnApp/node_modules'),
156-
).toBeFalsy();
157-
expect(
158-
fileService.isDangerous('C:\\Users\\User\\Documents\\node_modules'),
159-
).toBeFalsy();
148+
describe('isDangerous', () => {
149+
const originalEnv = { ...process.env };
150+
const originalCwd = process.cwd();
151+
152+
const mockCwd = (cwd: string) => {
153+
jest.spyOn(process, 'cwd').mockReturnValue(cwd);
154+
};
155+
156+
afterAll(() => {
157+
process.env = originalEnv;
160158
});
161159

162-
it('should return true for paths that are considered dangerous', () => {
163-
expect(
164-
fileService.isDangerous('/home/.config/myapp/node_modules'),
165-
).toBeTruthy();
166-
expect(fileService.isDangerous('.apps/node_modules')).toBeTruthy();
167-
expect(
168-
fileService.isDangerous('.apps/.sample/node_modules'),
169-
).toBeTruthy();
170-
expect(
171-
fileService.isDangerous('/Applications/MyApp.app/node_modules'),
172-
).toBeTruthy();
173-
expect(
174-
fileService.isDangerous(
175-
'C:\\Users\\User\\AppData\\Local\\node_modules',
176-
),
177-
).toBeTruthy();
160+
describe('POSIX paths', () => {
161+
beforeAll(() => {
162+
process.env.HOME = '/home/user';
163+
delete process.env.USERPROFILE;
164+
});
165+
166+
test('safe relative path', () => {
167+
mockCwd('/safe/path');
168+
expect(fileService.isDangerous('../project/node_modules')).toBe(false);
169+
});
170+
171+
test('hidden relative path', () => {
172+
mockCwd('/home/user/projects');
173+
expect(fileService.isDangerous('../.config/secret')).toBe(true);
174+
});
175+
176+
test('node_modules in ~/.cache', () => {
177+
expect(
178+
fileService.isDangerous('/home/user/.cache/project/node_modules'),
179+
).toBe(false);
180+
});
181+
182+
test('parent relative path (..)', () => {
183+
mockCwd('/home/user');
184+
expect(fileService.isDangerous('..')).toBe(false);
185+
});
186+
187+
test('current relative path (.)', () => {
188+
mockCwd('/home/user');
189+
expect(fileService.isDangerous('.')).toBe(false);
190+
});
191+
});
192+
193+
describe('Windows paths', () => {
194+
beforeAll(() => {
195+
process.env.USERPROFILE = 'C:\\Users\\user';
196+
process.env.HOME = '';
197+
});
198+
199+
test('safe relative path', () => {
200+
mockCwd('D:\\safe\\path');
201+
expect(fileService.isDangerous('..\\project\\node_modules')).toBe(
202+
false,
203+
);
204+
});
205+
206+
test('AppData relative path', () => {
207+
mockCwd('C:\\Users\\user\\Documents');
208+
expect(fileService.isDangerous('..\\AppData\\Roaming\\app')).toBe(true);
209+
});
210+
211+
test('Program Files (x86)', () => {
212+
expect(
213+
fileService.isDangerous('C:\\Program Files (x86)\\app\\node_modules'),
214+
).toBe(true);
215+
});
216+
217+
test('network paths', () => {
218+
expect(
219+
fileService.isDangerous('\\\\network\\share\\.hidden\\node_modules'),
220+
).toBe(true);
221+
});
222+
});
223+
224+
describe('Edge cases', () => {
225+
test('no home directory', () => {
226+
delete process.env.HOME;
227+
delete process.env.USERPROFILE;
228+
expect(fileService.isDangerous('/some/path')).toBe(false);
229+
});
230+
231+
test('whitelisted hidden segments', () => {
232+
expect(
233+
fileService.isDangerous('/tmp/.cache/project/node_modules'),
234+
).toBe(false);
235+
expect(fileService.isDangerous('/tmp/.npm/project/node_modules')).toBe(
236+
false,
237+
);
238+
});
239+
240+
test('macOS application bundle', () => {
241+
expect(
242+
fileService.isDangerous(
243+
'/Applications/MyApp.app/Contents/node_modules',
244+
),
245+
).toBe(true);
246+
});
247+
248+
test('Windows-style path on POSIX', () => {
249+
process.env.HOME = '/home/user';
250+
expect(fileService.isDangerous('/home/user/AppData/Local/.cache')).toBe(
251+
false,
252+
);
253+
});
178254
});
179255
});
180256

‎src/services/files/files.service.ts‎

Lines changed: 58 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import fs, { accessSync, readFileSync, Stats, statSync } from 'fs';
2+
import path from 'path';
23
import {
34
IFileService,
45
IFileStat,
@@ -85,16 +86,64 @@ export abstract class FileService implements IFileService {
8586
* application-specific data or configurations that should not be tampered with. Deleting node_modules
8687
* from these locations could potentially disrupt the normal operation of these applications.
8788
*/
88-
isDangerous(path: string): boolean {
89-
const hiddenFilePattern = /(^|\/)\.[^/.]/g;
90-
const macAppsPattern = /(^|\/)Applications\/[^/]+\.app\//g;
91-
const windowsAppDataPattern = /(^|\\)AppData\\/g;
92-
93-
return (
94-
hiddenFilePattern.test(path) ||
95-
macAppsPattern.test(path) ||
96-
windowsAppDataPattern.test(path)
89+
isDangerous(originalPath: string): boolean {
90+
const absolutePath = path.resolve(originalPath);
91+
const normalizedPath = absolutePath.replace(/\\/g, '/').toLowerCase();
92+
93+
const home = process.env.HOME ?? process.env.USERPROFILE ?? '';
94+
let isInHome = false;
95+
96+
if (home !== '') {
97+
const normalizedHome = path
98+
.resolve(home)
99+
.replace(/\\/g, '/')
100+
.toLowerCase();
101+
isInHome = normalizedPath.startsWith(normalizedHome);
102+
}
103+
104+
if (isInHome) {
105+
if (/\/\.config(\/|$)/.test(normalizedPath)) {
106+
return true;
107+
}
108+
if (/\/\.local\/share(\/|$)/.test(normalizedPath)) {
109+
return true;
110+
}
111+
if (/\/\.(cache|npm|pnpm)(\/|$)/.test(normalizedPath)) {
112+
return false;
113+
}
114+
}
115+
116+
if (/\/applications\/[^/]+\.app\//.test(normalizedPath)) {
117+
return true;
118+
}
119+
120+
if (normalizedPath.includes('/appdata/roaming')) {
121+
return true;
122+
}
123+
if (normalizedPath.includes('/appdata/local')) {
124+
if (/\/\.(cache|npm|pnpm)(\/|$)/.test(normalizedPath)) {
125+
return false;
126+
}
127+
return true;
128+
}
129+
if (/program files( \(x86\))?\//.test(normalizedPath)) {
130+
return true;
131+
}
132+
133+
const segments = normalizedPath.split('/');
134+
const hasUnsafeHiddenFolder = segments.some(
135+
(segment) =>
136+
segment.startsWith('.') &&
137+
segment !== '.' &&
138+
segment !== '..' &&
139+
!['.cache', '.npm', '.pnpm'].includes(segment),
97140
);
141+
142+
if (hasUnsafeHiddenFolder) {
143+
return true;
144+
}
145+
146+
return false;
98147
}
99148

100149
async getRecentModificationInDir(path: string): Promise<number> {

0 commit comments

Comments
 (0)