Patch: Better Security - #4
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds startup and deployment security controls, strengthens authentication and OTP handling, introduces request validation and sanitization utilities, protects AI, master, and payment routes, and updates documentation, tests, and Postman requests. ChangesSecurity hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthRoute
participant RefreshController
participant JWT
participant AuthService
participant UserDatabase
Client->>AuthRoute: POST /api/auth/refresh
AuthRoute->>RefreshController: Apply rate limiter and invoke handler
RefreshController->>JWT: Verify refresh token
JWT-->>RefreshController: Return token payload
RefreshController->>AuthService: Refresh user token
AuthService->>UserDatabase: Fetch user and token version
UserDatabase-->>AuthService: Return user state
AuthService-->>Client: Return rotated token pair
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (5)
__tests__/app.test.js (1)
95-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetain coverage for authenticated message validation.
These tests correctly verify that authentication runs first, but removing the old cases leaves the controller’s
400paths for missing and emptymessageuntested. Add equivalent requests with a valid JWT and continue asserting400.🤖 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 `@__tests__/app.test.js` around lines 95 - 108, Add tests in the authenticated /api/ai/chat suite using a valid JWT to cover requests with a missing message and an empty message, asserting status 400 for both. Preserve the existing unauthenticated and invalid-token tests, and reuse the test’s established JWT-generation symbol.src/utils/sanitize.js (1)
10-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrefer a vetted HTML-escaping library over manual character replacement.
The escaping order (
&first) is correct, avoiding double-encoding, but static analysis flags manual escaping as a CWE-79 risk pattern; a maintained library reduces the chance of missed edge cases (e.g., encoding contexts other than HTML body text, such as attributes/URLs).As per static analysis hints flagging lines 11-17 for "Avoid hand-rolled HTML escaping... use a vetted encoder/sanitizer."
🤖 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 `@src/utils/sanitize.js` around lines 10 - 19, Replace the manual character replacements in escapeHtml with a vetted, maintained HTML-escaping library, preserving the existing non-string passthrough behavior. Use the library’s appropriate HTML text-context encoder and update the dependency/import configuration as needed; do not retain the hand-rolled replacement chain.Source: Linters/SAST tools
src/schemas/user.schema.js (1)
6-9: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCap password inputs to fit bcrypt’s 72-byte effective limit.
registerSchema.passwordandresetPasswordSchema.newPasswordboth allow.max(128), but bcrypt only uses the first 72 bytes of each password (auth.service.jsregisters withbcrypt.hash(password, 10)and reset-password usesbcrypt.hash(newPassword, 10)). Characters/bytes beyond that are ignored, so two passwords with identical first 72 bytes hash identically.🔒️ Proposed fix
password: z .string() .min(6, 'Password must be at least 6 characters') - .max(128, 'Password must be at most 128 characters'), + .max(72, 'Password must be at most 72 characters'),Also apply the same change to
resetPasswordSchema.newPassword.🤖 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 `@src/schemas/user.schema.js` around lines 6 - 9, Update the password constraints in registerSchema and resetPasswordSchema.newPassword to cap inputs at bcrypt’s 72-byte effective limit instead of 128 characters, preserving the existing minimum validation and messages where appropriate.src/middlewares/rateLimiter.js (1)
69-74: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWebhook rate limit may drop legitimate Midtrans notifications.
10 requests/minute, keyed by IP, is aggressive for a payment webhook that likely originates from a small pool of Midtrans source IPs. A burst of simultaneous transactions could exceed this cap, and rejected webhooks are simply dropped (no automatic Midtrans-side guarantee beyond their own retry policy). The
POST /:id/syncroute is a compensating control, but relies on the user manually triggering it. Consider raising the threshold, exempting Midtrans's known IP ranges, or confirming expected payment volume against this limit.🤖 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 `@src/middlewares/rateLimiter.js` around lines 69 - 74, Adjust the webhook limiter configuration in createRateLimiter’s webhook entry to avoid rejecting legitimate bursts of Midtrans notifications: validate expected webhook volume and either raise max appropriately or exempt trusted Midtrans source IP ranges, while preserving rate limiting for untrusted callers.src/middlewares/masterAuth.js (1)
10-13: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTransmit
master_keyvia an authenticated header instead ofreq.body.
masterAuthreads the admin secret from the JSON body, and the Postman collection/docs currently sendmaster_keyin the request payload. Webhook/signature secrets are already sent via headers in this codebase, so move this to a protected header such asx-master-keyand update the Postman collection/docs/client contract accordingly.🤖 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 `@src/middlewares/masterAuth.js` around lines 10 - 13, Update masterAuth to read the admin secret from the authenticated x-master-key request header instead of req.body.master_key, while preserving the existing configured-key validation flow. Update all related Postman collection, documentation, and client contract references to send x-master-key and remove master_key from request payloads.
🤖 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 `@docker-compose.yml`:
- Around line 9-11: Update the PostgreSQL environment variables in the compose
service to require POSTGRES_USER, POSTGRES_PASSWORD, and POSTGRES_DB instead of
using the postgres/taskmanager fallback values. Preserve the existing variable
names and compose configuration while removing all silent credential defaults.
- Line 55: Update the DATABASE_URL configuration to avoid embedding unescaped
POSTGRES_USER and POSTGRES_PASSWORD values; either consume a pre-encoded
DATABASE_URL or require and document a credential charset that is safe for URI
interpolation, ensuring special characters cannot alter PostgreSQL URI parsing.
In `@EB-postman.json`:
- Line 753: Remove the hardcoded master credential from the Postman request
bodies at both occurrences and replace it with the uncommitted environment
variable reference {{masterKey}} expected by masterAuth. Rotate or revoke the
exposed key if it has been used.
In `@env.example`:
- Around line 4-8: Update the security notes in the configuration template to
stop warning against committing env.example; instead, prohibit committing .env
files or real secret values while retaining the guidance about strong, unique
secrets and secure key handling.
In `@README.md`:
- Around line 270-273: The README validation table overstates coverage beyond
the task, team, and user schemas changed by this PR. Narrow the Zod Schema
Validation and String Length Limits claims to the routes and fields actually
covered, and adjust the related security wording so it does not imply all
request inputs are validated; do not add unrelated route validation.
- Around line 299-300: Update the README environment-variable table to document
FRONTEND_URL as required in production, noting that it configures the production
CORS origin and prevents fallback to https://yourfrontend.com. Keep the existing
table structure and descriptions unchanged.
In `@src/config/validateEnv.js`:
- Around line 57-66: Update the hasError branch in the environment validation
flow to always terminate startup when critical variables are missing, including
when isRunningInDocker() returns true. Remove the Docker-specific continuation
and warning while preserving the existing critical error log and process.exit(1)
behavior.
- Around line 35-42: Add deterministic values for ACCESS_TOKEN_SECRET,
REFRESH_TOKEN_SECRET, MASTER_KEY, SERVER_KEY, and CLIENT_KEY in the CI/test
setup before src/app.js is imported, so validateEnv() sees all required
variables while preserving normal required-variable validation.
- Around line 10-17: Update the validation logic associated with CRITICAL_VARS
to reject documented template placeholder values, including the access-token and
master-key examples, rather than only the literal "secretkey". Enforce the
documented minimum strength requirements for ACCESS_TOKEN_SECRET and MASTER_KEY
while preserving the existing required-variable checks and clear validation
errors.
- Around line 19-24: Update isRunningInDocker to use the module’s ESM-compatible
fs import/API instead of require('fs'), while preserving the existing
existsSync('/.dockerenv') check and false fallback when filesystem access fails.
In `@src/controllers/auth.controller.js`:
- Around line 217-222: Update the ZodError handling branch in the authentication
controller to map validation messages from error.issues instead of the removed
error.errors property, preserving the existing 400 status response and
next(errorResponse) flow for refresh validation failures.
In `@src/middlewares/rateLimiter.js`:
- Around line 26-33: Sanitize the attacker-controlled req.originalUrl and req.ip
values in the rate-limit handler before interpolating them into logger.warn.
Update the handler’s [RATE LIMIT] log construction to remove or escape CR/LF and
other control characters while preserving the existing method, URL, and IP
context.
In `@src/middlewares/verifyMidtransWebhook.js`:
- Around line 44-51: Update the signature verification flow in the webhook
middleware to use Midtrans’s actual notification format: read the signature_key
value from the JSON payload when that is where delivery provides it, and compute
the expected value using the documented plain SHA-512 order_id + status_code +
gross_amount + server_key formula. Keep the timing-safe comparison and
downstream inventory/transaction handling unchanged, while removing the
conflicting header/HMAC expectation.
In `@src/services/auth.service.js`:
- Around line 482-513: Update refreshUserToken to increment and persist the
user’s tokenVersion before creating the replacement token pair, and use the new
version in the token payload. Ensure the update is atomic with the refresh
operation so concurrent reuse cannot issue multiple valid rotated tokens, while
preserving the existing revoked-token and verification checks.
- Around line 68-116: Update incrementOTPAttempts to avoid the read-modify-write
race: remove the findUnique-based calculation and use Prisma’s atomic increment
operation when updating otpAttempts. Preserve the existing five-attempt lockout
behavior and warning, ensuring otpLockedUntil is set when the atomically
resulting count reaches the threshold; apply this shared fix for both verifyOTP
and resetPasswordWithOTP callers.
In `@src/utils/sanitize.js`:
- Around line 21-29: Replace the regex implementation in stripHtml with a vetted
HTML sanitizer such as the project’s established DOMPurify or sanitize-html
dependency, configured to remove HTML while preserving plain text. Keep the
existing non-string passthrough behavior and ensure sanitizeObject continues
using stripHtml recursively.
---
Nitpick comments:
In `@__tests__/app.test.js`:
- Around line 95-108: Add tests in the authenticated /api/ai/chat suite using a
valid JWT to cover requests with a missing message and an empty message,
asserting status 400 for both. Preserve the existing unauthenticated and
invalid-token tests, and reuse the test’s established JWT-generation symbol.
In `@src/middlewares/masterAuth.js`:
- Around line 10-13: Update masterAuth to read the admin secret from the
authenticated x-master-key request header instead of req.body.master_key, while
preserving the existing configured-key validation flow. Update all related
Postman collection, documentation, and client contract references to send
x-master-key and remove master_key from request payloads.
In `@src/middlewares/rateLimiter.js`:
- Around line 69-74: Adjust the webhook limiter configuration in
createRateLimiter’s webhook entry to avoid rejecting legitimate bursts of
Midtrans notifications: validate expected webhook volume and either raise max
appropriately or exempt trusted Midtrans source IP ranges, while preserving rate
limiting for untrusted callers.
In `@src/schemas/user.schema.js`:
- Around line 6-9: Update the password constraints in registerSchema and
resetPasswordSchema.newPassword to cap inputs at bcrypt’s 72-byte effective
limit instead of 128 characters, preserving the existing minimum validation and
messages where appropriate.
In `@src/utils/sanitize.js`:
- Around line 10-19: Replace the manual character replacements in escapeHtml
with a vetted, maintained HTML-escaping library, preserving the existing
non-string passthrough behavior. Use the library’s appropriate HTML text-context
encoder and update the dependency/import configuration as needed; do not retain
the hand-rolled replacement chain.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0729521f-22ef-4207-9308-5706ff6a1a76
📒 Files selected for processing (25)
EB-postman.jsonREADME.md__tests__/app.test.jsdocker-compose.ymlenv.exampleprisma/schema.prismasrc/app.jssrc/config/validateEnv.jssrc/controllers/auth.controller.jssrc/controllers/master.controller.jssrc/middlewares/masterAuth.jssrc/middlewares/rateLimiter.jssrc/middlewares/verifyMidtransWebhook.jssrc/routes/ai.route.jssrc/routes/assignment.route.jssrc/routes/auth.route.jssrc/routes/completion.route.jssrc/routes/master.route.jssrc/routes/payment.route.jssrc/schemas/task.schema.jssrc/schemas/team.schema.jssrc/schemas/user.schema.jssrc/services/auth.service.jssrc/utils/jwt.jssrc/utils/sanitize.js
💤 Files with no reviewable changes (1)
- src/controllers/master.controller.js
| POSTGRES_USER: ${POSTGRES_USER:-postgres} | ||
| POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-postgres} | ||
| POSTGRES_DB: ${POSTGRES_DB:-taskmanager} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove known production database credentials.
The :-postgres and :-taskmanager fallbacks allow docker compose up to start with predictable PostgreSQL credentials, while port 5433 is exposed. Require these variables instead of silently defaulting them.
Proposed fix
- POSTGRES_USER: ${POSTGRES_USER:-postgres}
- POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-postgres}
- POSTGRES_DB: ${POSTGRES_DB:-taskmanager}
+ POSTGRES_USER: ${POSTGRES_USER:?POSTGRES_USER must be set}
+ POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:?POSTGRES_PASSWORD must be set}
+ POSTGRES_DB: ${POSTGRES_DB:?POSTGRES_DB must be set}Also applies to: 55-55
🤖 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 `@docker-compose.yml` around lines 9 - 11, Update the PostgreSQL environment
variables in the compose service to require POSTGRES_USER, POSTGRES_PASSWORD,
and POSTGRES_DB instead of using the postgres/taskmanager fallback values.
Preserve the existing variable names and compose configuration while removing
all silent credential defaults.
| NODE_ENV: ${NODE_ENV:-production} | ||
| PORT: 3001 | ||
| DATABASE_URL: postgresql://postgres:postgres@postgres:5432/taskmanager | ||
| DATABASE_URL: postgresql://${POSTGRES_USER:-postgres}:${POSTGRES_PASSWORD:-postgres}@postgres:5432/${POSTGRES_DB:-taskmanager} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,120p'
echo "== docker-compose relevant lines =="
if [ -f docker-compose.yml ]; then
nl -ba docker-compose.yml | sed -n '1,120p'
fi
echo "== env / compose usage =="
rg -n "POSTGRES_(USER|PASSWORD|DB)|DATABASE_URL|environment:" docker-compose.yml . -g '!node_modules' -g '!dist' -g '!build' | sed -n '1,200p'Repository: zannunakiz/TaskManagementBackend_EstelleBright
Length of output: 2528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docker-compose.yml =="
python3 - <<'PY'
from pathlib import Path
p=Path('docker-compose.yml')
for i,line in enumerate(p.read_text().splitlines(),1):
if i <= 120:
print(f"{i:4}: {line}")
PY
echo "== postgres/prisma references =="
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('*'):
if p.is_file() and str(p) not in {'.gitignore','.dockerignore'}:
try:
text=p.read_text(errors='ignore')
except Exception:
continue
if 'POSTGRES_' in text or 'DATABASE_URL' in text or 'postgresql://' in text or '`@postgresql`' in text.lower():
if p.name in {'docker-compose.yml','.env.example','env.example','package.json','Dockerfile'} or p.suffix in {'.js','.cjs','.mjs','.json'}:
print(f"--- {p}")
for n,l in enumerate(text.splitlines(),1):
if 'POSTGRES_' in l or 'DATABASE_URL' in l or 'postgresql://' in l or '`@postgresql`' in l.lower() or 'postgres://' in l.lower():
print(f"{n}: {l}")
PYRepository: zannunakiz/TaskManagementBackend_EstelleBright
Length of output: 50403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docker-compose line count =="
wc -l docker-compose.yml
echo "== docker-compose relevant lines =="
python3 - <<'PY'
from pathlib import Path
for i,line in enumerate(Path('docker-compose.yml').read_text().splitlines(),1):
if (1 <= i <= 140) or 'POSTGRES_' in line or 'DATABASE_URL' in line or 'postgresql://' in line:
print(f"{i:4}: {line}")
PY
echo "== focused config/env/postgres references =="
python3 - <<'PY'
from pathlib import Path
paths = ['docker-compose.yml','Dockerfile','env.example','package.json','src/config/prisma.js','src/config/validateEnv.js']
for p in paths:
if not Path(p).exists():
continue
print(f"--- {p}")
text = Path(p).read_text(errors='ignore')
for n,line in enumerate(text.splitlines(),1):
if any(q in line for q in ['POSTGRES_', 'DATABASE_URL', 'postgresql://', 'postgres://', 'PGPASSWORD', 'PGUSER', 'PGDATABASE', 'DRIVER']):
print(f"{n}: {line}")
PYRepository: zannunakiz/TaskManagementBackend_EstelleBright
Length of output: 4571
Do not construct DATABASE_URL from unescaped credentials.
docker-compose.yml:55 interpolates ${POSTGRES_USER} and ${POSTGRES_PASSWORD} directly into the PostgreSQL URI, so values containing @, :, /, #, or % can be parsed as host/path/query components instead of credentials. Pass a pre-encoded DATABASE_URL or document/require an allowed credential charset and URL-encode the interpolated values.
🤖 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 `@docker-compose.yml` at line 55, Update the DATABASE_URL configuration to
avoid embedding unescaped POSTGRES_USER and POSTGRES_PASSWORD values; either
consume a pre-encoded DATABASE_URL or require and document a credential charset
that is safe for URI interpolation, ensuring special characters cannot alter
PostgreSQL URI parsing.
| "body": { | ||
| "mode": "raw", | ||
| "raw": "{\n \"master_key\": \"masterkey\"\n}" | ||
| "raw": "{\n \"master_key\": \"estellebright-master-key-2024-secure\"\n}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Do not commit the master credential in the Postman collection.
masterAuth compares this value with process.env.MASTER_KEY, while these requests invoke database-destructive endpoints. Replace it with {{masterKey}} resolved from an uncommitted Postman environment, and rotate/revoke this key if it has been used.
Proposed fix
- "raw": "{\n \"master_key\": \"estellebright-master-key-2024-secure\"\n}"
+ "raw": "{\n \"master_key\": \"{{masterKey}}\"\n}"Cross-file basis: src/middlewares/masterAuth.js validates this body field, and src/routes/master.route.js protects the clear/seed operations.
Also applies to: 775-775
🤖 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 `@EB-postman.json` at line 753, Remove the hardcoded master credential from the
Postman request bodies at both occurrences and replace it with the uncommitted
environment variable reference {{masterKey}} expected by masterAuth. Rotate or
revoke the exposed key if it has been used.
| # IMPORTANT SECURITY NOTES: | ||
| # 1. NEVER commit this file or .env to version control | ||
| # 2. Use STRONG, UNIQUE secrets for JWT and MASTER_KEY | ||
| # 3. In production, use a strong random generator for secrets | ||
| # 4. Keep all API keys secure and rotate them periodically |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not warn contributors against committing env.example.
This file is the configuration template being added. The warning should prohibit committing .env files or real secret values, not the template itself.
🤖 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 `@env.example` around lines 4 - 8, Update the security notes in the
configuration template to stop warning against committing env.example; instead,
prohibit committing .env files or real secret values while retaining the
guidance about strong, unique secrets and secure key handling.
| | **Zod Schema Validation** | All request bodies, params, and queries validated at runtime | | ||
| | **String Length Limits** | Maximum character limits on all string fields (prevents resource exhaustion) | | ||
| | **HTML Tag Stripping** | XSS prevention via HTML tag removal from string inputs | | ||
| | **Type Coercion Safety** | Numeric IDs validated as positive integers | |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Narrow the validation coverage claims.
The README claims that all request bodies, parameters, queries, and string fields are validated, while this PR context only identifies validation changes for task, team, and user schemas. If other routes are not covered, this documents a false security guarantee; either narrow the wording or add validation to the remaining routes.
🤖 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 `@README.md` around lines 270 - 273, The README validation table overstates
coverage beyond the task, team, and user schemas changed by this PR. Narrow the
Zod Schema Validation and String Length Limits claims to the routes and fields
actually covered, and adjust the related security wording so it does not imply
all request inputs are validated; do not add unrelated route validation.
| message: { | ||
| success: false, | ||
| message, | ||
| }, | ||
| handler: (req, res, next, options) => { | ||
| logger.warn(`[RATE LIMIT] ${req.method} ${req.originalUrl} - IP: ${req.ip}`); | ||
| res.status(options.statusCode).json(options.message); | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Sanitize request data before logging (log injection).
req.originalUrl and req.ip are attacker-controlled and are logged unsanitized; a crafted URL with encoded CR/LF sequences can forge extra log lines (CWE-117 / log injection).
🛡️ Proposed fix
handler: (req, res, next, options) => {
- logger.warn(`[RATE LIMIT] ${req.method} ${req.originalUrl} - IP: ${req.ip}`);
+ const safeUrl = String(req.originalUrl).replace(/[\r\n]/g, '');
+ logger.warn(`[RATE LIMIT] ${req.method} ${safeUrl} - IP: ${req.ip}`);
res.status(options.statusCode).json(options.message);
},🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 30-30: Logging request-derived input unsanitized allows log forging (CRLF injection); strip newlines / encode the value before logging.
Context: logger.warn([RATE LIMIT] ${req.method} ${req.originalUrl} - IP: ${req.ip})
Note: [CWE-117] Improper Output Neutralization for Logs. OWASP A09:2021 Security Logging and Monitoring Failures.
(log-injection-javascript)
🤖 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 `@src/middlewares/rateLimiter.js` around lines 26 - 33, Sanitize the
attacker-controlled req.originalUrl and req.ip values in the rate-limit handler
before interpolating them into logger.warn. Update the handler’s [RATE LIMIT]
log construction to remove or escape CR/LF and other control characters while
preserving the existing method, URL, and IP context.
Source: Linters/SAST tools
| // Build the string to hash: order_id + status_code + gross_amount + server_key | ||
| const hashString = order_id + status_code + gross_amount + serverKey; | ||
| const computedSignature = crypto.createHash('sha512').update(hashString).digest('hex'); | ||
|
|
||
| // Use timing-safe comparison to prevent timing attacks | ||
| const expectedSignature = Buffer.from(computedSignature); | ||
| const receivedSignature = Buffer.from(signature); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Midtrans send the X-Signature-Key header in lowercase hex format?
💡 Result:
Midtrans does not use an X-Signature-Key header. Instead, its authentication and integrity verification mechanisms utilize different header names and formats depending on the specific API being used [1][2]. 1. Transactional APIs use the X-SIGNATURE header. This header is calculated using HMAC_SHA512 based on a string that includes a lowercase hex-encoded SHA-256 hash of the request body (formatted as Lowercase(HexEncode(SHA-256(minify(RequestBody))))), but the final X-SIGNATURE header itself is not explicitly defined as being in lowercase hex format in the same way [1][2]. 2. Notification webhooks include a signature_key field in the JSON request body (not a header), which is generated using the SHA-512 algorithm on a string composed of order_id, status_code, gross_amount, and the merchant's server key [3][4][5]. The resulting hash is typically represented in lowercase hex format [4]. It is important to distinguish between the X-SIGNATURE header used for transactional requests and the signature_key found in notification payloads, as they use different algorithms (HMAC_SHA512 vs. SHA-512) and are provided in different parts of the request [1][3][2][4].
Citations:
- 1: https://docs.midtrans.com/reference/signature-generation
- 2: https://docs.midtrans.com/reference/security-specification
- 3: https://docs.midtrans.com/reference/receiving-notifications
- 4: https://docs.midtrans.com/docs/https-notification-webhooks
- 5: https://docs.midtrans.com/reference/handle-notifications
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching verifyMidtransWebhook.js:"
fd -a 'verifyMidtransWebhook\.js$' . || true
file="$(fd 'verifyMidtransWebhook\.js$' . | head -n 1)"
if [ -n "${file:-}" ]; then
echo "--- $file size: $(wc -l < "$file")"
sed -n '1,120p' "$file" | cat -n
fi
echo
echo "All references to signature_key / X-Signature-Key / signature in this middleware context:"
rg -n "signature_key|x-signature|x-Signature-Key|signature" README.md src package.json 2>/dev/null || trueRepository: zannunakiz/TaskManagementBackend_EstelleBright
Length of output: 4848
Verify Midtrans signature delivery against the actual notification format.
This middleware reads X-Signature-Key in the request headers and describes the signature as HMAC-SHA512, while the computed value is a plain SHA-512 digest from order_id + status_code + gross_amount + server_key. If Midtrans sends signature_key in the JSON payload instead, this check may reject valid webhooks before updating inventory/transaction state.
🤖 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 `@src/middlewares/verifyMidtransWebhook.js` around lines 44 - 51, Update the
signature verification flow in the webhook middleware to use Midtrans’s actual
notification format: read the signature_key value from the JSON payload when
that is where delivery provides it, and compute the expected value using the
documented plain SHA-512 order_id + status_code + gross_amount + server_key
formula. Keep the timing-safe comparison and downstream inventory/transaction
handling unchanged, while removing the conflicting header/HMAC expectation.
| /** | ||
| * Check if user is locked out due to too many OTP attempts | ||
| */ | ||
| const checkOTPLockout = (user) => { | ||
| if (user.otpLockedUntil && user.otpLockedUntil > new Date()) { | ||
| const remainingMinutes = Math.ceil((user.otpLockedUntil.getTime() - Date.now()) / 60000); | ||
| const error = new Error( | ||
| `Too many OTP attempts. Please try again in ${remainingMinutes} minute(s).` | ||
| ); | ||
| error.statusCode = 429; | ||
| throw error; | ||
| } | ||
| }; | ||
|
|
||
| /** | ||
| * Increment OTP attempt counter and lock if exceeded | ||
| */ | ||
| const incrementOTPAttempts = async (userId) => { | ||
| const user = await prisma.user.findUnique({ where: { id: userId } }); | ||
|
|
||
| const newAttempts = (user.otpAttempts || 0) + 1; | ||
|
|
||
| const updateData = { otpAttempts: newAttempts }; | ||
|
|
||
| // Lock after 5 failed attempts for 15 minutes | ||
| if (newAttempts >= 5) { | ||
| updateData.otpLockedUntil = new Date(Date.now() + 15 * 60 * 1000); | ||
| logger.warn(`[AUTH] User ${userId} locked out due to ${newAttempts} failed OTP attempts`); | ||
| } | ||
|
|
||
| await prisma.user.update({ | ||
| where: { id: userId }, | ||
| data: updateData, | ||
| }); | ||
| }; | ||
|
|
||
| /** | ||
| * Reset OTP attempt counter on successful verification | ||
| */ | ||
| const resetOTPAttempts = async (userId) => { | ||
| await prisma.user.update({ | ||
| where: { id: userId }, | ||
| data: { | ||
| otpAttempts: 0, | ||
| otpLockedUntil: null, | ||
| }, | ||
| }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Race condition on OTP attempt counter (TOCTOU).
incrementOTPAttempts reads otpAttempts via findUnique, computes newAttempts in application code, then writes it back. Concurrent failed-verification requests (e.g. parallel /verify-otp calls) can all read the same stale count and each write the same incremented value, losing updates and letting an attacker exceed the intended 5-attempt lockout threshold via concurrency.
🔒️ Proposed fix using Prisma's atomic increment
const incrementOTPAttempts = async (userId) => {
- const user = await prisma.user.findUnique({ where: { id: userId } });
-
- const newAttempts = (user.otpAttempts || 0) + 1;
-
- const updateData = { otpAttempts: newAttempts };
-
- // Lock after 5 failed attempts for 15 minutes
- if (newAttempts >= 5) {
- updateData.otpLockedUntil = new Date(Date.now() + 15 * 60 * 1000);
- logger.warn(`[AUTH] User ${userId} locked out due to ${newAttempts} failed OTP attempts`);
- }
-
- await prisma.user.update({
- where: { id: userId },
- data: updateData,
- });
+ const { otpAttempts } = await prisma.user.update({
+ where: { id: userId },
+ data: { otpAttempts: { increment: 1 } },
+ select: { otpAttempts: true },
+ });
+
+ // Lock after 5 failed attempts for 15 minutes
+ if (otpAttempts >= 5) {
+ await prisma.user.update({
+ where: { id: userId },
+ data: { otpLockedUntil: new Date(Date.now() + 15 * 60 * 1000) },
+ });
+ logger.warn(`[AUTH] User ${userId} locked out due to ${otpAttempts} failed OTP attempts`);
+ }
};This affects both call sites: verifyOTP (Line 201) and resetPasswordWithOTP (Line 408).
📝 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.
| /** | |
| * Check if user is locked out due to too many OTP attempts | |
| */ | |
| const checkOTPLockout = (user) => { | |
| if (user.otpLockedUntil && user.otpLockedUntil > new Date()) { | |
| const remainingMinutes = Math.ceil((user.otpLockedUntil.getTime() - Date.now()) / 60000); | |
| const error = new Error( | |
| `Too many OTP attempts. Please try again in ${remainingMinutes} minute(s).` | |
| ); | |
| error.statusCode = 429; | |
| throw error; | |
| } | |
| }; | |
| /** | |
| * Increment OTP attempt counter and lock if exceeded | |
| */ | |
| const incrementOTPAttempts = async (userId) => { | |
| const user = await prisma.user.findUnique({ where: { id: userId } }); | |
| const newAttempts = (user.otpAttempts || 0) + 1; | |
| const updateData = { otpAttempts: newAttempts }; | |
| // Lock after 5 failed attempts for 15 minutes | |
| if (newAttempts >= 5) { | |
| updateData.otpLockedUntil = new Date(Date.now() + 15 * 60 * 1000); | |
| logger.warn(`[AUTH] User ${userId} locked out due to ${newAttempts} failed OTP attempts`); | |
| } | |
| await prisma.user.update({ | |
| where: { id: userId }, | |
| data: updateData, | |
| }); | |
| }; | |
| /** | |
| * Reset OTP attempt counter on successful verification | |
| */ | |
| const resetOTPAttempts = async (userId) => { | |
| await prisma.user.update({ | |
| where: { id: userId }, | |
| data: { | |
| otpAttempts: 0, | |
| otpLockedUntil: null, | |
| }, | |
| }); | |
| }; | |
| /** | |
| * Check if user is locked out due to too many OTP attempts | |
| */ | |
| const checkOTPLockout = (user) => { | |
| if (user.otpLockedUntil && user.otpLockedUntil > new Date()) { | |
| const remainingMinutes = Math.ceil((user.otpLockedUntil.getTime() - Date.now()) / 60000); | |
| const error = new Error( | |
| `Too many OTP attempts. Please try again in ${remainingMinutes} minute(s).` | |
| ); | |
| error.statusCode = 429; | |
| throw error; | |
| } | |
| }; | |
| /** | |
| * Increment OTP attempt counter and lock if exceeded | |
| */ | |
| const incrementOTPAttempts = async (userId) => { | |
| const { otpAttempts } = await prisma.user.update({ | |
| where: { id: userId }, | |
| data: { otpAttempts: { increment: 1 } }, | |
| select: { otpAttempts: true }, | |
| }); | |
| // Lock after 5 failed attempts for 15 minutes | |
| if (otpAttempts >= 5) { | |
| await prisma.user.update({ | |
| where: { id: userId }, | |
| data: { otpLockedUntil: new Date(Date.now() + 15 * 60 * 1000) }, | |
| }); | |
| logger.warn(`[AUTH] User ${userId} locked out due to ${otpAttempts} failed OTP attempts`); | |
| } | |
| }; | |
| /** | |
| * Reset OTP attempt counter on successful verification | |
| */ | |
| const resetOTPAttempts = async (userId) => { | |
| await prisma.user.update({ | |
| where: { id: userId }, | |
| data: { | |
| otpAttempts: 0, | |
| otpLockedUntil: null, | |
| }, | |
| }); | |
| }; |
🤖 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 `@src/services/auth.service.js` around lines 68 - 116, Update
incrementOTPAttempts to avoid the read-modify-write race: remove the
findUnique-based calculation and use Prisma’s atomic increment operation when
updating otpAttempts. Preserve the existing five-attempt lockout behavior and
warning, ensuring otpLockedUntil is set when the atomically resulting count
reaches the threshold; apply this shared fix for both verifyOTP and
resetPasswordWithOTP callers.
| export const refreshUserToken = async (payload) => { | ||
| const user = await getUserById(payload.id); | ||
|
|
||
| // Check token version | ||
| if (payload.version !== user.tokenVersion) { | ||
| const error = new Error('Token revoked. Please login again.'); | ||
| error.statusCode = 401; | ||
| throw error; | ||
| } | ||
|
|
||
| if (!user.isVerified) { | ||
| const error = new Error('Account is not verified.'); | ||
| error.statusCode = 403; | ||
| throw error; | ||
| } | ||
|
|
||
| // Generate new token pair (token rotation) | ||
| const tokens = createTokenPair({ | ||
| id: user.id, | ||
| email: user.email, | ||
| version: user.tokenVersion, | ||
| }); | ||
|
|
||
| return { | ||
| user: { | ||
| id: user.id, | ||
| name: user.name, | ||
| email: user.email, | ||
| }, | ||
| tokens, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Refresh-token "rotation" doesn't invalidate the previous refresh token.
refreshUserToken issues a new token pair but never bumps tokenVersion, so the old refresh token remains valid until its own natural expiry. This isn't single-use rotation — a stolen refresh token can keep being used indefinitely alongside the legitimate one, with no reuse/theft detection, which undercuts the "refresh-token rotation" security goal of this PR.
🔒️ Proposed fix: invalidate prior refresh token on each rotation
if (!user.isVerified) {
const error = new Error('Account is not verified.');
error.statusCode = 403;
throw error;
}
+ // Invalidate the previous refresh token so it cannot be replayed
+ const { tokenVersion } = await prisma.user.update({
+ where: { id: user.id },
+ data: { tokenVersion: { increment: 1 } },
+ select: { tokenVersion: true },
+ });
+
// Generate new token pair (token rotation)
const tokens = createTokenPair({
id: user.id,
email: user.email,
- version: user.tokenVersion,
+ version: tokenVersion,
});📝 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.
| export const refreshUserToken = async (payload) => { | |
| const user = await getUserById(payload.id); | |
| // Check token version | |
| if (payload.version !== user.tokenVersion) { | |
| const error = new Error('Token revoked. Please login again.'); | |
| error.statusCode = 401; | |
| throw error; | |
| } | |
| if (!user.isVerified) { | |
| const error = new Error('Account is not verified.'); | |
| error.statusCode = 403; | |
| throw error; | |
| } | |
| // Generate new token pair (token rotation) | |
| const tokens = createTokenPair({ | |
| id: user.id, | |
| email: user.email, | |
| version: user.tokenVersion, | |
| }); | |
| return { | |
| user: { | |
| id: user.id, | |
| name: user.name, | |
| email: user.email, | |
| }, | |
| tokens, | |
| }; | |
| }; | |
| export const refreshUserToken = async (payload) => { | |
| const user = await getUserById(payload.id); | |
| // Check token version | |
| if (payload.version !== user.tokenVersion) { | |
| const error = new Error('Token revoked. Please login again.'); | |
| error.statusCode = 401; | |
| throw error; | |
| } | |
| if (!user.isVerified) { | |
| const error = new Error('Account is not verified.'); | |
| error.statusCode = 403; | |
| throw error; | |
| } | |
| // Invalidate the previous refresh token so it cannot be replayed | |
| const { tokenVersion } = await prisma.user.update({ | |
| where: { id: user.id }, | |
| data: { tokenVersion: { increment: 1 } }, | |
| select: { tokenVersion: true }, | |
| }); | |
| // Generate new token pair (token rotation) | |
| const tokens = createTokenPair({ | |
| id: user.id, | |
| email: user.email, | |
| version: tokenVersion, | |
| }); | |
| return { | |
| user: { | |
| id: user.id, | |
| name: user.name, | |
| email: user.email, | |
| }, | |
| tokens, | |
| }; | |
| }; |
🤖 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 `@src/services/auth.service.js` around lines 482 - 513, Update refreshUserToken
to increment and persist the user’s tokenVersion before creating the replacement
token pair, and use the new version in the token payload. Ensure the update is
atomic with the refresh operation so concurrent reuse cannot issue multiple
valid rotated tokens, while preserving the existing revoked-token and
verification checks.
| /** | ||
| * Strip HTML tags from a string. | ||
| * @param {string} str - Input string | ||
| * @returns {string} String with HTML tags removed | ||
| */ | ||
| export const stripHtml = (str) => { | ||
| if (typeof str !== 'string') return str; | ||
| return str.replace(/<[^>]*>/g, ''); | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Regex-based tag stripping is bypassable — use a vetted sanitizer.
stripHtml's single-pass /<[^>]*>/g regex is a known-weak pattern: it can be defeated with nested/malformed tags (e.g. <<script>...</script>), where consuming everything up to the first > leaves a reconstituted valid tag behind after replacement. Since this utility is documented as an XSS defense (sanitizeObject applies it recursively to request bodies), a bypass here has real impact.
🛡️ Proposed fix using a vetted library
+import sanitizeHtml from 'sanitize-html';
+
export const stripHtml = (str) => {
if (typeof str !== 'string') return str;
- return str.replace(/<[^>]*>/g, '');
+ return sanitizeHtml(str, { allowedTags: [], allowedAttributes: {} });
};As per static analysis hints, "Avoid hand-rolled HTML escaping... use a vetted encoder/sanitizer such as DOMPurify or sanitize-html," noting CWE-79 risk.
📝 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.
| /** | |
| * Strip HTML tags from a string. | |
| * @param {string} str - Input string | |
| * @returns {string} String with HTML tags removed | |
| */ | |
| export const stripHtml = (str) => { | |
| if (typeof str !== 'string') return str; | |
| return str.replace(/<[^>]*>/g, ''); | |
| }; | |
| import sanitizeHtml from 'sanitize-html'; | |
| /** | |
| * Strip HTML tags from a string. | |
| * `@param` {string} str - Input string | |
| * `@returns` {string} String with HTML tags removed | |
| */ | |
| export const stripHtml = (str) => { | |
| if (typeof str !== 'string') return str; | |
| return sanitizeHtml(str, { allowedTags: [], allowedAttributes: {} }); | |
| }; |
🤖 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 `@src/utils/sanitize.js` around lines 21 - 29, Replace the regex implementation
in stripHtml with a vetted HTML sanitizer such as the project’s established
DOMPurify or sanitize-html dependency, configured to remove HTML while
preserving plain text. Keep the existing non-string passthrough behavior and
ensure sanitizeObject continues using stripHtml recursively.
Source: Linters/SAST tools
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation