fix: Node low-severity security hardening bundle (timing oracle, sharp DoS, dependabot, deps, audit)#3876
Conversation
… residual reasoning
|
Warning Review limit reached
More reviews will be available in 46 minutes and 28 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThree security fixes are applied: ChangesSecurity hardening bundle
Sequence Diagram(s)Constant-time authenticate() flow sequenceDiagram
participant Client
participant AuthService as auth.service.js
participant UserRepo as getBrut
participant bcrypt as comparePassword
Client->>AuthService: authenticate(email, password)
AuthService->>UserRepo: getBrut(email)
alt user record found
UserRepo-->>AuthService: userRecord
AuthService->>bcrypt: comparePassword(password, userRecord.hash)
bcrypt-->>AuthService: true / false
else user not found
UserRepo-->>AuthService: null
AuthService->>bcrypt: comparePassword(password, DUMMY_PASSWORD_HASH)
bcrypt-->>AuthService: dummy result (discarded)
AuthService-->>Client: AppError("invalid user or password")
end
Public image request with rate limiting sequenceDiagram
participant Guest as Guest Client
participant Route as uploads.routes.js
participant Limiter as limiters.publicImage
participant Sharp as uploads.getSharp
Guest->>Route: GET /api/uploads/images/:imageName
Route->>Limiter: policy.isAllowed → limiters.publicImage
alt requests within limit
Limiter-->>Sharp: next()
Sharp-->>Guest: processed image
else rate limit exceeded
Limiter-->>Guest: 429 Too Many Requests
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3876 +/- ##
=======================================
Coverage 92.50% 92.50%
=======================================
Files 165 165
Lines 5400 5403 +3
Branches 1739 1740 +1
=======================================
+ Hits 4995 4998 +3
Misses 325 325
Partials 80 80
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/uploads/routes/uploads.routes.js (1)
10-13: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd required JSDoc tags for the modified route registrar function.
This function was modified, but the current header lacks
@paramand@returns.JSDoc compliance patch
/** - * Routes + * Register uploads routes. + * `@param` {import('express').Application} app - Express app instance + * `@returns` {void} */ export default (app) => {As per coding guidelines, “Every new or modified function must have a JSDoc header: one-line description,
@paramfor each argument,@returnsfor any non-void return value.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/uploads/routes/uploads.routes.js` around lines 10 - 13, The default export function in uploads.routes.js lacks the required JSDoc documentation. Add a `@param` tag to document the app parameter and a `@returns` tag to document the function's return value. The JSDoc header should be placed directly above the function declaration and include a brief one-line description followed by these parameter and return tags as per the coding guidelines requiring all new or modified functions to have complete JSDoc headers.Source: Coding guidelines
modules/auth/services/auth.service.js (1)
94-112:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftTiming still diverges between unknown-user and wrong-password failures.
Line 99 throws immediately after bcrypt for unknown users, but Line 111 awaits
recordFailedAttempt(user)for known users. That DB write/read path keeps a measurable timing delta, so enumeration risk remains despite the dummy compare. Equalize the full failure path (not only bcrypt work).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/auth/services/auth.service.js` around lines 94 - 112, The authentication failure paths currently have different timing characteristics: the unknown user path (line 99) throws immediately after the dummy bcrypt compare, while the wrong password path (line 111) awaits recordFailedAttempt which performs a database operation. This timing delta allows attackers to distinguish between unknown users and known users, defeating the anti-enumeration goal. Add a dummy database operation or equivalent timing-consuming operation in the unknown user path (after the comparePassword call at line 99 and before throwing the error) so that both failure scenarios (unknown user and wrong password) incur the same measurable latency from both bcrypt work and database operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/auth/tests/auth.unit.tests.js`:
- Around line 100-108: The test for the unknown-user anti-enumeration path
currently only asserts that mockBcryptCompare was called once on line 107, but
does not validate the arguments passed to it. Strengthen the regression guard by
adding an additional expectation that verifies the second argument (the
sentinel/dummy hash) passed to mockBcryptCompare.toHaveBeenCalledWith, ensuring
that a proper bcrypt sentinel hash is being used rather than allowing
regressions to non-bcrypt values.
---
Outside diff comments:
In `@modules/auth/services/auth.service.js`:
- Around line 94-112: The authentication failure paths currently have different
timing characteristics: the unknown user path (line 99) throws immediately after
the dummy bcrypt compare, while the wrong password path (line 111) awaits
recordFailedAttempt which performs a database operation. This timing delta
allows attackers to distinguish between unknown users and known users, defeating
the anti-enumeration goal. Add a dummy database operation or equivalent
timing-consuming operation in the unknown user path (after the comparePassword
call at line 99 and before throwing the error) so that both failure scenarios
(unknown user and wrong password) incur the same measurable latency from both
bcrypt work and database operations.
In `@modules/uploads/routes/uploads.routes.js`:
- Around line 10-13: The default export function in uploads.routes.js lacks the
required JSDoc documentation. Add a `@param` tag to document the app parameter and
a `@returns` tag to document the function's return value. The JSDoc header should
be placed directly above the function declaration and include a brief one-line
description followed by these parameter and return tags as per the coding
guidelines requiring all new or modified functions to have complete JSDoc
headers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 86331033-d956-41e8-8725-dd747228c8d7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
.github/workflows/dependabot.ymlERRORS.mdconfig/defaults/production.config.jsconfig/defaults/test.config.jslib/middlewares/tests/rateLimiter.baseLayer.unit.tests.jslib/middlewares/tests/rateLimiter.unit.tests.jsmodules/auth/services/auth.service.jsmodules/auth/tests/auth.unit.tests.jsmodules/uploads/config/uploads.development.config.jsmodules/uploads/routes/uploads.routes.jspackage.json
|
Addressing the two outside-diff CodeRabbit findings from the CHANGES_REQUESTED review (ecde5b9): 1. JSDoc on uploads.routes.js (fixed in ecde5b9): Added 2. Timing delta from
A follow-up issue will be filed on this repo to track the complete timing equalization improvement. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
Items
5.1 — Account-enumeration timing oracle (auth)
Equalise the login response time on the unknown-user path by running a dummy
bcrypt.compareagainst a static hash when no user record is found. Previously the handler returned immediately on a DB miss, leaking user existence via response-time difference.5.2 — Sharp CPU-exhaustion DoS (uploads)
Rate-limit the public image endpoint (
GET /uploads/:id/image) via a dedicatedpublicImagelimiter profile added to the base config layer, with a tighter production override. No dimension clamping is added — file size is already allowlisted at the storage layer.5.3 — Dependabot auto-merge scope (CI)
Restrict the Dependabot auto-merge workflow to
version-update:semver-patchupdates only. Minor and major bumps now require a human review before merge.5.4 — devDependency hygiene (deps)
Move test and build-only tooling (
cross-envintentionally kept — runtime script use;snykintentionally kept — CI script use) todevDependencies. Reduces the production install footprint.5.5 — npm audit fix (deps)
npm audit fixcleared all remaining advisories via in-range lockfile bumps. A note has been added toERRORS.mddocumenting the residual reasoning for any advisory that was explicitly accepted.Scope
modules/auth,modules/uploads,.github/workflows,package.json,package-lock.json,ERRORS.mdnonelowValidation
npm run lintnpm testGuardrails check
.env*,secrets/**, keys, tokens)Optional: Infra/Stack alignment details
Before vs After (key changes only)
publicImagelimiter profileNotes for reviewers
npm audit fixchangedpackage-lock.json— CI will runnpm cicleanly on the updated lockfile.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores