Skip to content

Commit a442194

Browse files
sjsyrekclaude
andcommitted
fix(translate): reject symlinks to prevent directory traversal (Issue #6)
TDD Cycle: RED-GREEN-REFACTOR for symlink path validation RED Phase: - Added 5 failing tests for symlink validation - Tests verify symlink rejection for files and directories - Tests verify regular files/directories still work correctly - Tests verify clear error messages GREEN Phase: - Added lstatSync check at translate() entry point before processing - lstatSync doesn't follow symlinks, allowing detection before statSync - Throws error with clear message: "Symlinks are not supported for security reasons: <path>" - Re-throws symlink errors while catching other filesystem errors - All 1454 tests passing Security Impact: - Prevents directory traversal attacks via symlinks - Blocks access to sensitive files like /etc/passwd - Prevents reading files outside intended directory scope - Example attack: ln -s /etc/passwd ~/myfile.txt && deepl translate ~/myfile.txt Implementation: - Uses fs.lstatSync() to detect symlinks (doesn't follow them) - Applies to both file and directory translation paths - Clear, actionable error messages for users Updated 2 existing tests to mock lstatSync for new validation. CHANGELOG.md updated with security enhancement details. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 4f965f2 commit a442194

3 files changed

Lines changed: 141 additions & 3 deletions

File tree

CHANGELOG.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
5252
- **Performance Benefit**: Avoids expensive database aggregation on every cache write
5353
- Location: `src/storage/cache.ts` (currentSize field, initialize, set, clear, cleanupExpired, evictIfNeeded)
5454

55+
### Security
56+
- **Symlink Path Validation** - Prevents directory traversal attacks (Issue #6)
57+
- Rejects symlinks at translate() entry point before processing files
58+
- Uses `fs.lstatSync()` to detect symlinks (doesn't follow symlinks)
59+
- Prevents attackers from using symlinks to access sensitive files outside intended scope
60+
- Clear error message: "Symlinks are not supported for security reasons: <path>"
61+
- Applies to both file and directory paths
62+
- Added 5 comprehensive tests covering symlink rejection and regular path acceptance
63+
- **Impact**: Closes security vulnerability allowing directory traversal via symlinks
64+
- **Example Attack Prevented**: User creating symlink to `/etc/passwd` and attempting to translate it
65+
- Location: `src/cli/commands/translate.ts:118-149` (translate method with lstatSync check)
66+
5567
### Fixed
5668
- **Critical: Duplicate text handling in batch translation** - Fixed data loss bug for duplicate inputs
5769
- When input array contained duplicate texts (e.g., `["Hello", "Hello", "World"]`), only the last occurrence received translation

src/cli/commands/translate.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,17 +113,30 @@ export class TranslateCommand {
113113
/**
114114
* Translate text, file, or directory
115115
* Fix for Issue #8: Cache stats to avoid redundant filesystem calls
116+
* Fix for Issue #6: Reject symlinks for security (prevent directory traversal attacks)
116117
*/
117118
async translate(textOrPath: string, options: TranslateOptions): Promise<string> {
118119
// Check if input is a file/directory with a single stat() call
119120
// Cache the stats result to avoid duplicate syscalls later
120121
let stats: fs.Stats | null = null;
121122
try {
123+
// First check if the path is a symlink using lstatSync (doesn't follow symlinks)
124+
// This prevents directory traversal attacks and accessing sensitive files
125+
const lstat = fs.lstatSync(textOrPath);
126+
if (lstat.isSymbolicLink()) {
127+
throw new Error(`Symlinks are not supported for security reasons: ${textOrPath}`);
128+
}
129+
130+
// Now safe to use statSync (follows symlinks if any, but we already checked)
122131
stats = fs.statSync(textOrPath);
123132
if (stats.isDirectory()) {
124133
return this.translateDirectory(textOrPath, options);
125134
}
126-
} catch {
135+
} catch (error) {
136+
// Re-throw symlink errors
137+
if (error instanceof Error && error.message.includes('Symlinks are not supported')) {
138+
throw error;
139+
}
127140
// Not a file/directory, treat as text
128141
}
129142

tests/unit/translate-command.test.ts

Lines changed: 115 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -704,8 +704,9 @@ describe('TranslateCommand', () => {
704704

705705
describe('translate() - file/directory detection', () => {
706706
it('should detect and route to translateDirectory() for directory paths', async () => {
707-
// Mock fs to indicate directory
707+
// Mock fs to indicate directory (Issue #6: must check lstatSync for symlinks)
708708
const fs = jest.requireActual('fs');
709+
jest.spyOn(fs, 'lstatSync').mockReturnValue({ isSymbolicLink: () => false, isDirectory: () => true } as any);
709710
jest.spyOn(fs, 'existsSync').mockReturnValue(true);
710711
jest.spyOn(fs, 'statSync').mockReturnValue({ isDirectory: () => true } as any);
711712

@@ -725,9 +726,10 @@ describe('TranslateCommand', () => {
725726
});
726727

727728
it('should detect and route to translateFile() for file paths', async () => {
728-
// Mock fs to indicate file (not directory)
729+
// Mock fs to indicate file (not directory) (Issue #6: must check lstatSync for symlinks)
729730
const fs = jest.requireActual('fs');
730731
const mockStats = { isDirectory: () => false, isFile: () => true };
732+
jest.spyOn(fs, 'lstatSync').mockReturnValue({ isSymbolicLink: () => false, isFile: () => true } as any);
731733
jest.spyOn(fs, 'existsSync').mockReturnValue(true);
732734
jest.spyOn(fs, 'statSync').mockReturnValue(mockStats as any);
733735

@@ -1587,4 +1589,115 @@ describe('TranslateCommand', () => {
15871589
).rejects.toThrow('Invalid target language code: "invalid"');
15881590
});
15891591
});
1592+
1593+
describe('symlink path validation (Issue #6)', () => {
1594+
it('should reject symlink file paths', async () => {
1595+
const fs = jest.requireActual('fs');
1596+
// Mock lstatSync to indicate it's a symlink
1597+
jest.spyOn(fs, 'lstatSync').mockReturnValue({
1598+
isSymbolicLink: () => true,
1599+
isDirectory: () => false,
1600+
isFile: () => false,
1601+
} as any);
1602+
1603+
await expect(
1604+
translateCommand.translate('/path/to/symlink.txt', {
1605+
to: 'es',
1606+
output: '/out.txt',
1607+
})
1608+
).rejects.toThrow('Symlinks are not supported for security reasons');
1609+
});
1610+
1611+
it('should reject symlink directory paths', async () => {
1612+
const fs = jest.requireActual('fs');
1613+
// Mock lstatSync to indicate it's a symlink directory
1614+
jest.spyOn(fs, 'lstatSync').mockReturnValue({
1615+
isSymbolicLink: () => true,
1616+
isDirectory: () => true,
1617+
isFile: () => false,
1618+
} as any);
1619+
1620+
await expect(
1621+
translateCommand.translate('/path/to/symlink-dir', {
1622+
to: 'es',
1623+
output: '/out',
1624+
})
1625+
).rejects.toThrow('Symlinks are not supported for security reasons');
1626+
});
1627+
1628+
it('should accept regular file paths', async () => {
1629+
const fs = jest.requireActual('fs');
1630+
// Mock lstatSync to indicate it's a regular file (not a symlink)
1631+
jest.spyOn(fs, 'lstatSync').mockReturnValue({
1632+
isSymbolicLink: () => false,
1633+
isDirectory: () => false,
1634+
isFile: () => true,
1635+
size: 1024,
1636+
} as any);
1637+
jest.spyOn(fs, 'statSync').mockReturnValue({
1638+
isDirectory: () => false,
1639+
isFile: () => true,
1640+
size: 1024,
1641+
} as any);
1642+
jest.spyOn(fs, 'readFileSync').mockReturnValue('Hello world');
1643+
jest.spyOn(fs, 'writeFileSync').mockImplementation(() => undefined);
1644+
jest.spyOn(fs, 'existsSync').mockReturnValue(true);
1645+
1646+
(mockDocumentTranslationService.isDocumentSupported as jest.Mock).mockReturnValue(true);
1647+
(mockTranslationService.translate as jest.Mock).mockResolvedValueOnce({
1648+
text: 'Hola mundo',
1649+
});
1650+
1651+
const result = await translateCommand.translate('/path/to/regular-file.txt', {
1652+
to: 'es',
1653+
output: '/out.txt',
1654+
});
1655+
1656+
expect(result).toContain('Translated');
1657+
expect(result).toContain('/out.txt');
1658+
});
1659+
1660+
it('should accept regular directory paths', async () => {
1661+
const fs = jest.requireActual('fs');
1662+
// Mock lstatSync to indicate it's a regular directory (not a symlink)
1663+
jest.spyOn(fs, 'lstatSync').mockReturnValue({
1664+
isSymbolicLink: () => false,
1665+
isDirectory: () => true,
1666+
isFile: () => false,
1667+
} as any);
1668+
jest.spyOn(fs, 'statSync').mockReturnValue({
1669+
isDirectory: () => true,
1670+
isFile: () => false,
1671+
} as any);
1672+
1673+
// Mock translateDirectory to return success
1674+
const spy = jest.spyOn(translateCommand as any, 'translateDirectory')
1675+
.mockResolvedValue('Directory translation complete');
1676+
1677+
const result = await translateCommand.translate('/path/to/regular-dir', {
1678+
to: 'es',
1679+
output: '/out',
1680+
});
1681+
1682+
expect(result).toBe('Directory translation complete');
1683+
expect(spy).toHaveBeenCalled();
1684+
spy.mockRestore();
1685+
});
1686+
1687+
it('should provide clear error message for symlinks', async () => {
1688+
const fs = jest.requireActual('fs');
1689+
jest.spyOn(fs, 'lstatSync').mockReturnValue({
1690+
isSymbolicLink: () => true,
1691+
isDirectory: () => false,
1692+
isFile: () => false,
1693+
} as any);
1694+
1695+
await expect(
1696+
translateCommand.translate('/path/to/symlink.txt', {
1697+
to: 'es',
1698+
output: '/out.txt',
1699+
})
1700+
).rejects.toThrow('Symlinks are not supported for security reasons: /path/to/symlink.txt');
1701+
});
1702+
});
15901703
});

0 commit comments

Comments
 (0)