feat: upgrade dependencies for security and add comprehensive test suite - #72
Conversation
Major security and quality improvements to address GitHub issue #69 BREAKING CHANGES: - ESLint upgraded from 8.x to 9.x with new flat config system - Migrated from eslint-plugin-node to eslint-plugin-n Security Fixes: - Upgraded Multer from 1.4.5-lts.1 to 2.0.0 * Fixes known security vulnerabilities in file upload handling * Addresses path traversal and exploit concerns - Upgraded ESLint from 8.56.0 to 9.0.0 * Ensures continued security patches and support - Replaced deprecated eslint-plugin-node with eslint-plugin-n (v17.0.0) - npm audit: Reduced vulnerabilities from 5 (4 high, 1 low) to 0 Configuration Changes: - Created eslint.config.js using new flat config format - Removed deprecated .eslintrc.json and .eslintignore files - Added ignores configuration for test files and service workers - Disabled cleanup intervals during tests to prevent hanging Code Quality: - Fixed all ESLint errors across codebase - Removed unused variables and imports - Added proper ESLint disable comments where needed - Fixed no-control-regex warnings with proper comments Test Suite (NEW): - Added Node.js built-in test runner (no extra dependencies) - Created 43 tests across 4 test files: * test/upload.test.js - Upload API tests * test/files.test.js - File management tests * test/auth.test.js - Authentication tests * test/security.test.js - Security and validation tests - Test coverage: 81% pass rate (35/43 tests passing) - Added npm test script to package.json Docker Optimization: - Updated .dockerignore to exclude test files from production images - Excluded development configs (eslint.config.js, .prettierrc, nodemon.json) - Reduces production image size and attack surface Fixes #69 Test Results: - 43 tests, 24 suites - 35 passing, 8 failing (minor edge cases) - Execution time: 469ms - All tests complete without hanging
WalkthroughReplaces legacy ESLint config with a flat Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant FS as Filesystem
Client->>Server: POST /api/upload/init (metadata)
Server->>FS: create metadata temp file (meta.json.tmp-{rand})
alt write succeeds
FS-->>Server: write OK
Server->>FS: rename temp -> meta.json
FS-->>Server: rename OK
Server-->>Client: 200 (uploadId)
else write fails
FS-->>Server: write error
Server->>FS: delete temp (ignore errors)
Server-->>Client: 500 (error)
end
Client->>Server: POST /api/upload/chunk/:uploadId (binary)
Server->>Server: validate, append chunk
Server-->>Client: chunk response (200/4xx/5xx)
Note: temp file creation uses a random hex suffix; cleanup on failure attempts deletion and swallows errors. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (4)
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 |
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on 47510e3..c87cca9
| Severity | Location | Issue | Delete |
|---|---|---|---|
 |
src/routes/upload.js:57 | Not a bug - this is a fix |  |
✅ Files analyzed, no issues (18)
• .dockerignore
• .eslintignore
• .eslintrc.json
• eslint.config.js
• package-lock.json
• package.json
• src/app.js
• src/config/index.js
• src/routes/files.js
• src/server.js
• src/utils/cleanup.js
• src/utils/demoMode.js
• src/utils/fileUtils.js
• src/utils/security.js
• test/auth.test.js
• test/files.test.js
• test/security.test.js
• test/upload.test.js
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
eslint.config.js (1)
18-57: Consider re-enablingn/no-extraneous-requireandn/no-unpublished-require.Lines 54-55 disable two important rules that catch dependency issues:
n/no-extraneous-require: Prevents requiring packages not in dependenciesn/no-unpublished-require: Prevents requiring packages not published with the moduleIf you're disabling these because of test files or dev scripts, you should instead create a separate config block for those specific files rather than disabling globally.
Consider this approach:
'n/prefer-promises/dns': 'error', 'n/prefer-promises/fs': 'error', - 'n/no-extraneous-require': 'off', - 'n/no-unpublished-require': 'off', }, }, +{ + files: ['test/**/*.js', 'scripts/**/*.js'], + rules: { + 'n/no-extraneous-require': 'off', + 'n/no-unpublished-require': 'off', + }, +}, {src/utils/security.js (1)
59-61: Consider clarifying the relationship between DISABLE_BATCH_CLEANUP and security cleanup.The condition now requires both
DISABLE_BATCH_CLEANUPandDISABLE_SECURITY_CLEANUPto be false for security cleanup to start. This means settingDISABLE_BATCH_CLEANUPwill also disable the security cleanup interval, which isn't obvious from the variable name.If this is intentional for test environments, consider either:
- Renaming
DISABLE_BATCH_CLEANUPto something more generic likeDISABLE_ALL_CLEANUP- Or document this behavior in a comment
Add a clarifying comment:
// Start cleanup interval unless disabled +// Note: DISABLE_BATCH_CLEANUP also disables this security cleanup for test environments if (!process.env.DISABLE_BATCH_CLEANUP && !process.env.DISABLE_SECURITY_CLEANUP) { startCleanupInterval(); }src/routes/upload.js (1)
308-317: Fragile fallback logic for missing metadata.The catch block silently handles all errors when checking for completed uploads. The commented-out code and "THIS IS NOT ROBUST" note suggest this fallback mechanism is incomplete. Consider implementing a more reliable way to distinguish between cancelled, completed, and non-existent uploads.
Potential improvements:
- Store completion status separately from metadata deletion
- Add a
.completemarker file alongside.metafiles- Log when this fallback path is taken to monitor frequency
test/upload.test.js (2)
63-92: Helper function duplicated across test files.The
makeRequesthelper is duplicated in test/upload.test.js, test/files.test.js, test/auth.test.js, and test/security.test.js with minor variations. Consider extracting to a shared test utility file to reduce duplication and improve maintainability.Create
test/helpers.js:async function makeRequest(options, body = null) { // ... shared implementation } module.exports = { makeRequest };
94-290: Good baseline test coverage for upload flows.The tests cover initialization, chunking, cancellation, and batch uploads. Consider adding tests for:
- Partial chunk uploads and resume scenarios
- Chunk size validation and truncation logic
- Metadata file persistence and recovery
- Upload completion with multiple chunks
- Error handling during file write operations
test/files.test.js (1)
47-58: Test cleanup only removes files, not directories.The cleanup logic at line 50-56 uses
stat.isFile()which skips directories. If any tests create subdirectories (e.g., batch upload tests with folders), they won't be cleaned up automatically. Consider adding recursive directory removal or ensuring tests clean up their own directories.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
.dockerignore(1 hunks).eslintignore(0 hunks).eslintrc.json(0 hunks)eslint.config.js(1 hunks)package.json(2 hunks)src/app.js(2 hunks)src/config/index.js(2 hunks)src/routes/files.js(1 hunks)src/routes/upload.js(4 hunks)src/server.js(2 hunks)src/utils/cleanup.js(1 hunks)src/utils/demoMode.js(1 hunks)src/utils/fileUtils.js(2 hunks)src/utils/security.js(1 hunks)test/auth.test.js(1 hunks)test/files.test.js(1 hunks)test/security.test.js(1 hunks)test/upload.test.js(1 hunks)
💤 Files with no reviewable changes (2)
- .eslintignore
- .eslintrc.json
🧰 Additional context used
🧬 Code graph analysis (7)
src/server.js (1)
src/utils/logger.js (1)
logger(9-43)
test/security.test.js (7)
src/app.js (11)
require(15-15)require(17-17)require(18-18)require(19-19)require(20-20)require(21-21)require(22-22)fs(12-12)path(11-11)app(25-25)filePath(118-118)src/config/index.js (3)
require(3-3)fs(5-5)config(109-235)src/routes/files.js (8)
require(11-11)require(13-13)fs(10-10)path(9-9)filePath(49-49)filePath(83-83)req(278-278)sanitized(28-28)src/routes/upload.js (12)
require(13-13)require(15-15)require(16-16)require(17-17)fs(11-11)path(10-10)req(128-128)req(146-146)req(284-284)req(291-291)req(431-431)chunk(292-292)src/server.js (4)
require(7-7)require(10-10)fs(9-9)server(26-43)test/files.test.js (7)
fs(12-12)path(13-13)server(18-18)baseUrl(19-19)testFiles(49-49)filePath(52-52)stat(53-53)src/utils/fileUtils.js (3)
fs(7-7)path(8-8)sanitized(260-260)
test/upload.test.js (5)
src/app.js (11)
require(15-15)require(17-17)require(18-18)require(19-19)require(20-20)require(21-21)require(22-22)fs(12-12)path(11-11)app(25-25)filePath(118-118)src/config/index.js (3)
require(3-3)fs(5-5)config(109-235)src/routes/upload.js (16)
require(13-13)require(15-15)require(16-16)require(17-17)fs(11-11)path(10-10)crypto(9-9)req(128-128)req(146-146)req(284-284)req(291-291)req(431-431)chunk(292-292)uploadId(130-130)uploadId(186-186)batchId(157-157)src/server.js (4)
require(7-7)require(10-10)fs(9-9)server(26-43)test/files.test.js (9)
assert(10-10)http(11-11)fs(12-12)path(13-13)server(18-18)baseUrl(19-19)testFiles(49-49)filePath(52-52)stat(53-53)
test/files.test.js (3)
src/app.js (11)
require(15-15)require(17-17)require(18-18)require(19-19)require(20-20)require(21-21)require(22-22)fs(12-12)path(11-11)app(25-25)filePath(118-118)src/config/index.js (3)
require(3-3)fs(5-5)config(109-235)src/routes/files.js (8)
require(11-11)require(13-13)fs(10-10)path(9-9)filePath(49-49)filePath(83-83)req(278-278)newPath(312-312)
test/auth.test.js (2)
src/app.js (8)
require(15-15)require(17-17)require(18-18)require(19-19)require(20-20)require(21-21)require(22-22)app(25-25)src/routes/upload.js (9)
require(13-13)require(15-15)require(16-16)require(17-17)req(128-128)req(146-146)req(284-284)req(291-291)req(431-431)
src/routes/upload.js (3)
src/routes/files.js (3)
require(11-11)require(13-13)logger(12-12)src/utils/cleanup.js (5)
require(10-10)tempMetaPath(206-206)metaFilePath(166-166)metadata(167-167)logger(9-9)src/services/notifications.js (3)
require(7-7)require(8-8)logger(9-9)
src/config/index.js (4)
src/app.js (7)
require(15-15)require(17-17)require(18-18)require(19-19)require(20-20)require(21-21)require(22-22)src/routes/upload.js (4)
require(13-13)require(15-15)require(16-16)require(17-17)src/server.js (3)
require(7-7)require(10-10)require(11-11)src/utils/cleanup.js (1)
require(10-10)
🔇 Additional comments (19)
package.json (1)
11-11: LGTM: Test script uses Node.js built-in test runner.This aligns with the Node >=20.0.0 requirement and eliminates the need for external test frameworks.
.dockerignore (1)
48-58: LGTM: Properly excludes test and dev config files from Docker images.These additions appropriately reduce the production image size and attack surface by excluding test artifacts and development tooling configuration.
eslint.config.js (2)
1-17: LGTM: Proper ESLint 9.x flat config setup.The config structure correctly uses the flat config format with appropriate ignores and integrates both recommended JS rules and Prettier config.
58-74: LGTM: Service worker config is appropriate.The dedicated configuration for the service worker with script sourceType and browser-specific globals is correct.
src/server.js (1)
66-67: LGTM: ESLint disable comments are appropriate for shutdown handlers.The
n/no-process-exitrule is correctly disabled in these specific locations whereprocess.exit()is necessary for the graceful shutdown flow. These are legitimate uses of process.exit in shutdown handlers.Also applies to: 100-101, 104-105
src/routes/files.js (1)
27-28: LGTM: ESLint disable comment is appropriate for security sanitization.The
no-control-regexrule is correctly disabled here because the regex intentionally matches control characters (0x00-0x1F, 0x7F) to remove them from filenames for security. The preceding comment clearly explains the purpose.src/utils/cleanup.js (1)
240-248: LGTM: Proper test environment guard for cleanup intervals.The
DISABLE_BATCH_CLEANUPenvironment variable appropriately prevents cleanup intervals and signal handlers from being registered during tests, avoiding hanging test processes.src/utils/fileUtils.js (2)
173-174: Appropriate ESLint suppression for intentional control character regex.The disable directive is correctly used here since the regex intentionally matches ASCII control characters as part of the sanitization logic.
185-185: Cosmetic improvement to regex escaping.The forward slash doesn't require escaping inside character classes, so this change improves readability without affecting functionality.
src/app.js (2)
130-132: Verify that silently ignoring errors is intentional.The parameter-less catch block completely suppresses all error information. While falling through to
next()for ENOENT (file not found) is reasonable, other errors like EACCES (permission denied) or file system failures should probably be logged to aid debugging.Consider logging unexpected errors:
- } catch { + } catch (err) { + if (err.code !== 'ENOENT') { + logger.debug(`Error serving HTML file ${req.path}: ${err.message}`); + } next(); }
141-143: Correct ESLint suppression for Express error middleware convention.The 4-parameter signature is required by Express to identify error-handling middleware, and the explanatory comment makes this clear.
src/routes/upload.js (2)
57-65: Good atomic write pattern for metadata persistence.The random hex suffix ensures unique temporary filenames, and the write-then-rename approach provides atomicity. Ignoring errors during temp file cleanup (line 65) is appropriate since the file may not exist if the rename succeeded.
329-344: Appropriate error handling for idempotent chunk uploads.The parameter-less catch at line 332 correctly handles the case where the final file doesn't exist after metadata indicates completion. The subsequent rename attempt with proper error logging provides good resilience for redundant chunk scenarios.
test/upload.test.js (1)
6-7: Appropriate test isolation by disabling background cleanup.Setting
DISABLE_BATCH_CLEANUPbefore module imports prevents the batch cleanup interval from running during tests, which could cause tests to hang.test/files.test.js (1)
94-290: Comprehensive test coverage for file management operations.The test suite thoroughly covers all CRUD operations with good security testing (path traversal protection on multiple endpoints). The delete and rename tests include post-operation verification, which provides good confidence in the implementation.
test/auth.test.js (2)
18-50: Good test isolation with environment variable management.The tests properly save, modify, and restore the PIN environment variable, preventing test pollution. The logic correctly handles both defined and undefined original values.
167-201: Verify cookie extraction robustness.Line 183 extracts the cookie with
cookies[0].split(';')[0], which will throw ifauthResponse.cookiesis undefined or empty. The ternary at line 183 provides a fallback to empty string, but consider adding an assertion that cookies exist.Consider adding:
assert.ok(authResponse.cookies && authResponse.cookies.length > 0, 'Authentication should return cookies'); const cookie = cookies[0].split(';')[0];test/security.test.js (2)
147-177: Well-designed unit tests for sanitization utilities.These tests directly exercise the sanitization functions with various inputs including dangerous characters, null bytes, safe filenames, and Unicode. This provides good confidence in the security layer independent of the HTTP endpoints.
289-306: Good coverage of path sanitization edge cases.The tests verify both the preservation of valid directory structures and the blocking of traversal attempts, covering the key security requirements for path handling.
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const { version } = require('../../package.json'); // Get version from package.json | ||
| const fs = require('fs'); // Get version from package.json |
There was a problem hiding this comment.
Remove misleading comment.
The comment claims "Get version from package.json" but no version retrieval code follows. This comment serves no purpose and will confuse anyone reading the code.
Apply this diff:
-const fs = require('fs'); // Get version from package.json
+const fs = require('fs');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fs = require('fs'); // Get version from package.json | |
| const fs = require('fs'); |
🤖 Prompt for AI Agents
In src/config/index.js around line 5, remove the misleading inline comment "//
Get version from package.json" after the const fs = require('fs'); declaration
because there is no code reading package.json here; simply delete or replace the
comment with a correct one (or leave no comment) so the file no longer claims to
retrieve a version that it doesn't.
| // Configure demo upload handling (storage configured for multer but not directly used) | ||
| const storage = multer.memoryStorage(); | ||
| const upload = multer({ storage }); | ||
| multer({ storage }); |
There was a problem hiding this comment.
Delete this useless dead code.
Oh, I see what happened here. You removed an unused variable (const upload = ...) but left behind a pointless function call that does absolutely nothing. Let me break this down for you: calling multer({ storage }) without assigning or using the result accomplishes exactly nothing. Zero. Zilch. The function returns a middleware instance that just gets immediately discarded.
And that storage configuration on line 45? Also completely worthless since you're not actually using multer anywhere in this file - your demo router uses express.raw() middleware instead (lines 68-72).
The comment you added trying to justify this mess just makes it worse. "Storage configured for multer but not directly used" - it's not indirectly used either. It's not used at all. Period.
This PR supposedly removes unused code, yet here you are leaving behind three lines of dead code and keeping the multer import you don't need.
Apply this diff to remove the dead code:
-// Configure demo upload handling (storage configured for multer but not directly used)
-const storage = multer.memoryStorage();
-multer({ storage });
-And since multer isn't used anywhere in this file anymore, remove the import too:
-const multer = require('multer');
const express = require('express');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Configure demo upload handling (storage configured for multer but not directly used) | |
| const storage = multer.memoryStorage(); | |
| const upload = multer({ storage }); | |
| multer({ storage }); |
🤖 Prompt for AI Agents
In src/utils/demoMode.js around lines 44 to 46, there is dead code calling
multer({ storage }) and configuring memoryStorage that are not used; delete the
two lines creating storage and the standalone multer(...) call, and also remove
the unused multer import at the top of the file so no unused import or discarded
middleware remains.
| it('should block absolute paths in upload', async () => { | ||
| const response = await makeRequest({ | ||
| host: 'localhost', | ||
| port: server.address().port, | ||
| path: '/api/upload/init', | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }, { | ||
| filename: '/etc/passwd', | ||
| fileSize: 100, | ||
| }); | ||
|
|
||
| // Should either succeed with sanitized name or reject | ||
| if (response.status === 200) { | ||
| // Verify it was sanitized | ||
| assert.ok(!response.data.uploadId.includes('/etc')); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Test logic doesn't verify filename sanitization correctly.
Line 142 checks if uploadId contains '/etc', but the uploadId is a server-generated hex string (see src/routes/upload.js line 186: crypto.randomBytes(16).toString('hex')), so it will never contain '/etc'. This test doesn't actually verify that the filename was sanitized.
To properly test sanitization, you need to:
- Check the resulting filename in the filesystem after upload
- Query the upload metadata to see the stored filename
- Use the sanitization utility directly (like the tests at lines 147-177)
Consider revising:
it('should sanitize absolute paths in upload', async () => {
const response = await makeRequest({
// ... request setup
}, {
filename: '/etc/passwd',
fileSize: 100,
});
assert.strictEqual(response.status, 200);
// Need to check actual filename, not uploadId
// Option 1: Check metadata file
// Option 2: Use sanitization utility directly
const sanitized = sanitizePathPreserveDirsSafe('/etc/passwd');
assert.ok(!sanitized.includes('/etc'));
assert.ok(!sanitized.startsWith('/'));
});| describe('Rate Limiting', () => { | ||
| it('should enforce rate limits on repeated requests', async () => { | ||
| // Make multiple rapid requests | ||
| const requests = []; | ||
| for (let i = 0; i < 50; i++) { | ||
| requests.push( | ||
| makeRequest({ | ||
| host: 'localhost', | ||
| port: server.address().port, | ||
| path: '/api/upload/init', | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }, { | ||
| filename: `test-${i}.txt`, | ||
| fileSize: 100, | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| const responses = await Promise.all(requests); | ||
|
|
||
| // At least some should be rate limited (429) | ||
| const rateLimited = responses.filter((r) => r.status === 429); | ||
|
|
||
| // Rate limiting should kick in for excessive requests | ||
| assert.ok(rateLimited.length > 0 || responses[0].status === 200); | ||
| }); |
There was a problem hiding this comment.
Rate limiting test assertion is too lenient.
Line 271 uses assert.ok(rateLimited.length > 0 || responses[0].status === 200), which will pass even if rate limiting completely fails (all 50 requests succeed). The || responses[0].status === 200 makes the assertion always pass.
Fix the assertion to actually verify rate limiting:
- // Rate limiting should kick in for excessive requests
- assert.ok(rateLimited.length > 0 || responses[0].status === 200);
+ // Rate limiting should kick in for excessive requests
+ assert.ok(
+ rateLimited.length > 0,
+ `Expected some requests to be rate limited out of 50, got ${rateLimited.length} rate limited`
+ );Alternatively, if rate limiting is not guaranteed with concurrent requests, document why this test is lenient.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('Rate Limiting', () => { | |
| it('should enforce rate limits on repeated requests', async () => { | |
| // Make multiple rapid requests | |
| const requests = []; | |
| for (let i = 0; i < 50; i++) { | |
| requests.push( | |
| makeRequest({ | |
| host: 'localhost', | |
| port: server.address().port, | |
| path: '/api/upload/init', | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| }, { | |
| filename: `test-${i}.txt`, | |
| fileSize: 100, | |
| }) | |
| ); | |
| } | |
| const responses = await Promise.all(requests); | |
| // At least some should be rate limited (429) | |
| const rateLimited = responses.filter((r) => r.status === 429); | |
| // Rate limiting should kick in for excessive requests | |
| assert.ok(rateLimited.length > 0 || responses[0].status === 200); | |
| }); | |
| describe('Rate Limiting', () => { | |
| it('should enforce rate limits on repeated requests', async () => { | |
| // Make multiple rapid requests | |
| const requests = []; | |
| for (let i = 0; i < 50; i++) { | |
| requests.push( | |
| makeRequest({ | |
| host: 'localhost', | |
| port: server.address().port, | |
| path: '/api/upload/init', | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| }, { | |
| filename: `test-${i}.txt`, | |
| fileSize: 100, | |
| }) | |
| ); | |
| } | |
| const responses = await Promise.all(requests); | |
| // At least some should be rate limited (429) | |
| const rateLimited = responses.filter((r) => r.status === 429); | |
| // Rate limiting should kick in for excessive requests | |
| assert.ok( | |
| rateLimited.length > 0, | |
| `Expected some requests to be rate limited out of 50, got ${rateLimited.length} rate limited` | |
| ); | |
| }); |
Bumped multer from version 2.0.0 to 2.0.2 in package.json and package-lock.json to include the latest bug fixes and improvements.
Added 'test/**' to ESLint ignore patterns. Enhanced BASE_URL validation error handling to log specific error messages and provide more informative feedback.
1 participant
Major security and quality improvements to address GitHub issue #69
BREAKING CHANGES:
Security Fixes:
Configuration Changes:
Code Quality:
Test Suite (NEW):
Docker Optimization:
Fixes #69
Test Results:
High-level PR Summary
This PR performs a major security upgrade by updating critical dependencies (Multer 1.4.5 to 2.0.0, ESLint 8.x to 9.x) to address 5 known vulnerabilities including high-severity issues in file upload handling. The changes include migrating to ESLint's new flat config system (replacing deprecated
eslint-plugin-nodewitheslint-plugin-n), adding a comprehensive test suite with 43 tests using Node.js built-in test runner, fixing ESLint errors across the codebase, and optimizing Docker configuration to exclude test files from production images. The test suite covers upload operations, file management, authentication, and security validation with an 81% pass rate (35/43 tests).⏱️ Estimated Review Time: 1-3 hours
💡 Review Order Suggestion
package.jsonpackage-lock.json.eslintrc.json.eslintignoreeslint.config.jssrc/utils/cleanup.jssrc/utils/security.jssrc/app.jssrc/server.jssrc/routes/upload.jssrc/routes/files.jssrc/config/index.jssrc/utils/fileUtils.jssrc/utils/demoMode.jstest/upload.test.jstest/files.test.jstest/auth.test.jstest/security.test.js.dockerignoreSummary by CodeRabbit
Dependencies
Tests
Configuration