fix: path traversal vulnerability in URL-to-folder mapping #1806
Conversation
- Add validatePathWithinBase utility to prevent path traversal attacks - Update getMapBaseUrlToFolderResolvers to validate paths before file operations - Add comprehensive tests for path traversal prevention (7 test cases) - Improve error messages with context and security violation details - Handle both Unix and Windows path separators - Maintain backward compatibility for legitimate use cases Security: Fixes critical path traversal vulnerability that allowed accessing files outside the specified base directory through malicious URLs. Tests: Added test suite covering: - Normal file access within base directory - Path traversal attempts with ../ and ..\ - Absolute paths outside base directory - Multiple traversal sequences - Clear error message validation
gkganesh12
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
|
What reviewer looks at during PR reviewThe following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.
|
📝 WalkthroughWalkthroughAdds a path-validation utility and applies it to parser read operations to block directory traversal, decodes URI paths, improves error messages, and adds integration tests covering traversal attempts, absolute/outside paths, Windows cases, and base/root edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/generator/lib/utils.js (2)
209-233: Add input validation for parameters.The function should validate that
filePathandbaseDirare non-empty strings before proceeding. Without validation, passing empty strings, null, or undefined could lead to confusing behavior or errors.🔎 Proposed input validation
utils.validatePathWithinBase = (filePath, baseDir, operation = 'access') => { + if (!filePath || typeof filePath !== 'string') { + throw new Error('filePath must be a non-empty string'); + } + if (!baseDir || typeof baseDir !== 'string') { + throw new Error('baseDir must be a non-empty string'); + } + // Resolve and normalize both paths const resolvedBaseDir = path.resolve(baseDir);
200-208: Consider documenting symlink behavior.The function validates paths but doesn't resolve symlinks. If a symlink inside the base directory points outside, it won't be detected. While this is acceptable (it's a different vulnerability class requiring prior write access), consider adding a note in the JSDoc for clarity.
📝 Suggested documentation enhancement
/** * Validates that a file path stays within a base directory to prevent path traversal attacks. + * Note: This function does not resolve symbolic links. If a symlink inside the base directory + * points to a location outside it, this will not be detected. * * @param {string} filePath - The file path to validate (can be relative or absolute)apps/generator/test/parser.test.js (1)
298-330: Consider simplifying the test logic.The conditional logic in this test (lines 320-329) makes it harder to understand and maintain. The test would be clearer with a straightforward malicious path construction.
🔎 Suggested simplification
- it('should reject absolute paths outside base directory', async () => { + it('should reject paths resolving outside base directory', async () => { const generator = { mapBaseUrlToFolder: { url: 'https://schema.example.com/crm/', folder: testBaseDir } }; const oldOptions = {}; const newOptions = convertOldOptionsToNew(oldOptions, generator); const resolver = newOptions.__unstable.resolver.resolvers[0]; - // Create a file outside the base directory - const outsideFile = path.join(tempDir, 'outside-file.json'); - await writeFile(outsideFile, JSON.stringify({ secret: 'data' }), 'utf8'); - - // Attempt to access it using absolute path manipulation + // Attempt to access parent directory const maliciousUri = { - toString: () => `https://schema.example.com/crm/${path.relative(testBaseDir, outsideFile)}`, - valueOf: () => `https://schema.example.com/crm/${path.relative(testBaseDir, outsideFile)}` + toString: () => 'https://schema.example.com/crm/../../sensitive-file.json', + valueOf: () => 'https://schema.example.com/crm/../../sensitive-file.json' }; - // If the relative path doesn't work, try direct absolute path - if (!maliciousUri.toString().includes('..')) { - // Try with a path that would resolve outside - const anotherMaliciousUri = { - toString: () => `https://schema.example.com/crm/../../${path.basename(outsideFile)}`, - valueOf: () => `https://schema.example.com/crm/../../${path.basename(outsideFile)}` - }; - await expect(resolver.read(anotherMaliciousUri)).rejects.toThrow('Path traversal detected'); - } else { - await expect(resolver.read(maliciousUri)).rejects.toThrow('Path traversal detected'); - } + await expect(resolver.read(maliciousUri)).rejects.toThrow('Path traversal detected'); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/generator/lib/parser.js(2 hunks)apps/generator/lib/utils.js(1 hunks)apps/generator/test/parser.test.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/generator/test/parser.test.js (1)
apps/generator/lib/parser.js (6)
require(3-3)require(4-4)require(5-5)fs(1-1)path(2-2)resolver(102-127)
apps/generator/lib/parser.js (1)
apps/generator/lib/utils.js (2)
path(3-3)fs(1-1)
🔇 Additional comments (5)
apps/generator/lib/utils.js (1)
211-222: Excellent path traversal prevention logic!The validation correctly:
- Resolves and normalizes both paths to handle
..sequences and various path formats- Uses
normalizedBase + path.septo prevent false positives (e.g.,/base-evilwhen base is/base)- Allows the base directory itself via equality check
This approach effectively prevents path traversal attacks.
apps/generator/lib/parser.js (2)
2-5: Good integration of path validation!The imports are correct, and renaming
pathtouriPathproperly avoids naming conflicts with thepathmodule. The string replacement to derivelocalpathis straightforward and appropriate for this use case.Also applies to: 107-108
110-124: Security validation correctly implemented!The integration properly:
- Validates the path before any filesystem access
- Uses the
validatedPathconsistently for both file reading and error messages- Catches and propagates validation errors appropriately
- Cannot be bypassed due to the try-catch structure
This effectively prevents path traversal attacks in the resolver.
apps/generator/test/parser.test.js (2)
208-256: Excellent test infrastructure and positive test case!The test setup properly:
- Creates an isolated temporary directory for each test
- Cleans up resources in afterEach
- Includes a positive test case to ensure legitimate file access still works
This provides a solid foundation for testing the security fix.
332-398: Comprehensive test coverage for edge cases and error messages!The tests effectively verify:
- Edge case handling for the base directory itself
- Detailed error messages include security-relevant keywords
- Multiple traversal sequences are caught
The use of try-catch in the error message test (lines 353-378) allows for more detailed assertions on the error content, which is valuable for security-related errors.
gkganesh12
changed the title
gkganesh12
changed the title
- Add input validation to validatePathWithinBase for non-empty strings - Document symlink behavior in JSDoc (does not resolve symlinks) - Simplify test case by removing conditional logic - Ensure consistent error messages for security violations
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/generator/lib/utils.jsapps/generator/test/parser.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/generator/test/parser.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
apps/generator/lib/utils.js (1)
apps/generator/lib/parser.js (1)
path(2-2)
🔇 Additional comments (2)
apps/generator/lib/utils.js (2)
216-222: LGTM: Input validation is thorough.The input validation correctly ensures both
filePathandbaseDirare non-empty strings and throws appropriateTypeErrorfor invalid inputs.
200-213: Excellent documentation.The JSDoc is comprehensive and clearly documents the function's behavior, parameters, return value, exceptions, and the important note about symlink handling.
- Add case-insensitive path comparison on Windows to match filesystem behavior - Fix root directory edge case by checking for existing trailing separator - Use fs.realpathSync() for base directory to resolve symlinks and get canonical paths - Add tests for Windows case-insensitivity and root directory handling - Update JSDoc to document Windows case-insensitivity and root directory handling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/generator/test/parser.test.js (6)
3-4: Inconsistent use offs.promisesAPI.Line 4 destructures
mkdir,writeFile,rmfromrequire('fs').promises, but line 216 usesfs.promises.mkdtempdirectly. Consider using a consistent approach.Suggested fix
const os = require('os'); -const { mkdir, writeFile, rm } = require('fs').promises; +const { mkdir, writeFile, rm, mkdtemp } = require('fs').promises;Then on line 216:
- tempDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'asyncapi-test-')); + tempDir = await mkdtemp(path.join(os.tmpdir(), 'asyncapi-test-'));
298-320: Test name is misleading.The test is named "should reject absolute paths outside base directory" but actually tests relative traversal (
../../) rather than absolute paths. Consider either renaming the test to match its behavior or adding a separate test for true absolute paths (e.g.,/etc/passwdorC:\Windows\System32).Suggested rename
- it('should reject absolute paths outside base directory', async () => { + it('should reject relative traversal to files outside base directory', async () => {Or add a separate test for absolute paths:
it('should reject absolute paths outside base directory', async () => { // ... setup ... const absolutePathUri = { toString: () => 'https://schema.example.com/crm//etc/passwd', valueOf: () => 'https://schema.example.com/crm//etc/passwd' }; await expect(resolver.read(absolutePathUri)).rejects.toThrow(); });
339-341: Vague assertion - clarify expected behavior.The assertion
.rejects.toThrow()without a specific message accepts any error. The comment on line 333 says "should be allowed or handled gracefully" but the test expects a throw. Consider clarifying the expected behavior:
- If it should fail because directories can't be read as files, expect an
EISDIRor similar error.- If it should be allowed, the test logic needs to change.
Suggested improvement
// This should either succeed (if baseDir is a file) or fail with a clear error - await expect(resolver.read(baseDirUri)).rejects.toThrow(); + // Reading a directory should fail with an appropriate filesystem error + await expect(resolver.read(baseDirUri)).rejects.toThrow(/EISDIR|ENOENT|Error opening file/);
359-368: Replace anti-patternexpect(true).toBe(false)with idiomatic Jest assertion.Line 362 uses
expect(true).toBe(false)which produces a confusing failure message. Useexpect.assertions()at the start of the test to ensure the catch block runs.Suggested fix
+ expect.assertions(3); try { await resolver.read(maliciousUri); - // If we reach here, the test should fail - expect(true).toBe(false); // Force test failure + // Unreachable if validation works correctly } catch (error) { expect(error.message).toContain('Path traversal detected'); expect(error.message).toContain('security violation'); expect(error.message).toContain('blocked'); }Alternatively, if only one assertion is needed:
await expect(resolver.read(maliciousUri)).rejects.toThrow(/Path traversal detected.*security violation.*blocked/s);
390-393: Silent test skip obscures test coverage.Using
returnto skip tests on non-Windows platforms makes it appear as if the test passed when it was actually skipped. Consider using Jest's conditional skip pattern for clearer test reporting.Suggested improvement
- it('should handle case-insensitive path comparison on Windows', async () => { - if (process.platform !== 'win32') { - return; // Skip on non-Windows platforms - } + (process.platform === 'win32' ? it : it.skip)('should handle case-insensitive path comparison on Windows', async () => {Or use
describe.skipfor the entire block:(process.platform === 'win32' ? describe : describe.skip)('Windows-specific tests', () => { it('should handle case-insensitive path comparison', async () => { // ... }); });
417-422: Silent test skip and potentially misleading test name.Same issue as the Windows test - the early
returnsilently skips without indicating in test output. Additionally, the test name suggests testing the root directory (/) as a base, but it actually tests a subdirectory oftmpdir(). Consider clarifying the test's intent.Suggested improvements
- it('should handle root directory edge case', async () => { - // This test verifies that root directories (/, C:\) are handled correctly - // Skip on Windows as we can't easily test C:\ root - if (process.platform === 'win32') { - return; - } + (process.platform !== 'win32' ? it : it.skip)('should handle URL mapping to top-level directory', async () => { + // This test verifies that URL-to-folder mapping works when the URL path is at the root level
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/generator/lib/utils.jsapps/generator/test/parser.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/generator/lib/utils.js
🧰 Additional context used
🧬 Code graph analysis (1)
apps/generator/test/parser.test.js (1)
apps/generator/lib/parser.js (6)
require(3-3)require(4-4)require(5-5)fs(1-1)path(2-2)resolver(102-127)
🔇 Additional comments (5)
apps/generator/test/parser.test.js (5)
214-230: LGTM!The
beforeEach/afterEachsetup correctly creates an isolated temporary directory structure and cleans it up after each test, ensuring test isolation.
248-255: LGTM!The test correctly validates that files within the base directory can be read successfully. The mock URI object pattern is clear.
258-276: LGTM!This test correctly validates that Unix-style path traversal attempts (
../) are blocked with an appropriate error message.
278-296: LGTM!Good coverage for Windows-style backslash traversal sequences (
..\\), ensuring cross-platform security.
370-388: LGTM!Good test ensuring that multiple consecutive traversal sequences are blocked.
- Add decodeURIComponent() to decode URL-encoded characters before path validation - Prevents bypass using encoded sequences like %2e%2e%2f (../) or %2e%2e%5c (..\) - Add proper error handling for invalid URL encoding with descriptive error messages - Add comprehensive tests for URL-encoded traversal attempts - Use descriptive error variable name (decodeError) for better code clarity
|
|
Please do not open PRs until the issue is reviewed by maintainers and opened for contributions. Additionally, please:
|
3 participants
Description
getMapBaseUrlToFolderResolversfunction that allowed accessing files outside the base directoryvalidatePathWithinBaseutility function with input validation for non-empty string parameterspath.resolve()andpath.normalize()Related issue(s)
Resolves #1805
Summary by CodeRabbit
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.