fix(config): gate production hardening on isProd predicate (env-gate defect class)#3874
Conversation
Capture NODE_ENV in beforeEach/afterEach (safe restore even if a sibling test mutates it before the file loads). Document the inline DEV_ENVS set in express.docsEnvGate must stay in sync with lib/helpers/config.js. Add local-env positive cases to docsEnvGate (mounts docs) and debugGate (debug true) — mirrors the existing development cases.
|
Warning Review limit reached
More reviews will be available in 50 minutes and 49 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 (3)
WalkthroughIntroduces ChangesProduction env-gate hardening
Sequence Diagram(s)sequenceDiagram
participant Client
participant expressService as Express (initErrorRoutes)
participant configHelper as configHelper.isProd()
participant responsesHelper as responses.error
Client->>expressService: HTTP request triggers error
expressService->>configHelper: isProd()
configHelper-->>expressService: true (non-dev env) / false (dev env)
alt isProd() === true
expressService-->>Client: { message: "Internal Server Error" }
else isProd() === false
expressService->>responsesHelper: include serialized error detail
responsesHelper-->>Client: { message, error: "<serialized details>" }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 #3874 +/- ##
=======================================
Coverage 92.49% 92.50%
=======================================
Files 165 165
Lines 5397 5400 +3
Branches 1737 1740 +3
=======================================
+ Hits 4992 4995 +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:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a production-hardening env-gating defect by introducing shared environment predicates (isDevEnv / isProd) and using them to correctly apply “production-grade” behavior when downstream deployments use NODE_ENV=<project> instead of the literal production.
Changes:
- Add
isDevEnv/isProdpredicates inlib/helpers/config.jsand switch production hardening checks to use them. - Secure-by-default gates: disable swagger docs mounting and mongoose query debug logging outside dev-grade envs; prevent detailed Express error responses outside dev-grade envs.
- Ensure rate-limiter profiles (
api,billingPlans) exist in the always-merged base config layer, with production overrides, and add regression tests for all gates.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/organizations/config/organizations.development.config.js | Adds base-layer rateLimit.api profile so limiter is present under all envs. |
| modules/billing/config/billing.development.config.js | Adds base-layer rateLimit.billingPlans profile so limiter is present under all envs. |
| lib/services/mongoose.js | Gates mongoose debug via isDevEnv() and exports resolveDebug. |
| lib/services/express.js | Gates swagger mounting by env predicate and hardens terminal error handler for non-dev envs. |
| lib/helpers/config.js | Introduces and exports isDevEnv / isProd predicates; reuses predicate for JWT-secret validation. |
| lib/helpers/responses.js | Gates raw serialized error-object exposure using the env predicate. |
| config/defaults/production.config.js | Explicitly disables swagger via production defaults. |
| lib/services/tests/mongoose.debugGate.unit.tests.js | Adds unit coverage for mongoose debug env gating behavior. |
| lib/services/tests/express.errorroutes.unit.tests.js | Adds regression test ensuring non-dev project envs don’t leak error details. |
| lib/services/tests/express.docsEnvGate.unit.tests.js | Adds tests ensuring docs routes mount only in dev-grade envs. |
| lib/services/tests/express.docs.unit.tests.js | Updates existing swagger tests to mock new env predicate dependency. |
| lib/middlewares/tests/rateLimiter.baseLayer.unit.tests.js | Adds regression guard asserting base-layer limiter profile presence/shape. |
| lib/helpers/tests/responses.errorLeak.unit.tests.js | Adds tests for error-object exposure gating outside dev envs. |
| lib/helpers/tests/config.envPredicate.unit.tests.js | Adds unit tests for isDevEnv / isProd semantics and defaults. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@lib/middlewares/tests/rateLimiter.baseLayer.unit.tests.js`:
- Around line 28-32: The JSDoc comment for the expectUsableProfile helper
function is missing the required `@returns` tag. Add `@returns` {void} to the JSDoc
block above the expectUsableProfile function declaration to satisfy the JS
function JSDoc rule that requires both `@param` and `@returns` documentation for all
named helper functions in test files.
In `@lib/services/tests/express.docsEnvGate.unit.tests.js`:
- Around line 30-33: The buildMockApp helper function is missing the required
JSDoc header. Add a JSDoc comment above the buildMockApp function definition
that includes a brief description of what the function does and a `@returns` tag
documenting that it returns a mock app object with a get method and _routes
property. Since the function takes no parameters, no `@param` tags are needed.
🪄 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: 98f6925a-3aff-42bb-969f-c6db49dd61e5
📒 Files selected for processing (14)
config/defaults/production.config.jslib/helpers/config.jslib/helpers/responses.jslib/helpers/tests/config.envPredicate.unit.tests.jslib/helpers/tests/responses.errorLeak.unit.tests.jslib/middlewares/tests/rateLimiter.baseLayer.unit.tests.jslib/services/express.jslib/services/mongoose.jslib/services/tests/express.docs.unit.tests.jslib/services/tests/express.docsEnvGate.unit.tests.jslib/services/tests/express.errorroutes.unit.tests.jslib/services/tests/mongoose.debugGate.unit.tests.jsmodules/billing/config/billing.development.config.jsmodules/organizations/config/organizations.development.config.js
… pass-1) - Clarify that isDevEnv/isProd default to 'development' when NODE_ENV is unset - Add @returns {void} to expectUsableProfile in rateLimiter.baseLayer tests - Add JSDoc to buildMockApp helper in express.docsEnvGate tests
Summary
isProd/isDevEnvpredicates; gate error-object leak, rate-limiter defaults, swagger docs, and mongoose debug logging off these predicates instead of a literalNODE_ENV==='production'check.NODE_ENV === 'production', but downstream projects runNODE_ENV={project}(e.g.NODE_ENV=trawl), soproduction.config.jsnever loads for them. The practical effect: full error objects were leaked to API consumers in production, rate-limiter profiles forapiandbillingPlansroutes were silently no-ops, mongoose query logging was on, and/api/docs+/api/spec.jsonwere exposed.Scope
lib/helpers/config.js,lib/helpers/responses.js,lib/services/express.js,lib/services/mongoose.js,config/defaults/production.config.js,lib/middlewares/,modules/billing/config/,modules/organizations/config/yes— sharedlib/predicates consumed by express, mongoose, responses; rate-limiter config consumed by every route that mountsapi/billingPlansprofilesmedium— changes observable defaults for any env that is notdevelopment/test/localhostValidation
npm run lintnpm test— 2017 unit tests pass (--runInBand)Guardrails check
.env*,secrets/**, keys, tokens)Infra/Stack alignment details
Before vs After (key changes only)
NODE_ENV==='production'guard!isProd()viaDEV_ENVSsetapi+billingPlansrate limitersproduction.config.js→ inactive downstreamauthalready used/api/docs+/api/spec.jsonisProd/isDevEnvlib/helpers/config.jsDEV_ENVS = new Set(['development','test','localhost'])Part breakdown
(A)
isProd/isDevEnvpredicates (lib/helpers/config.js):isDevEnv()returnstruefordevelopment,test,localhost;isProd()=!isDevEnv(). Both exported.(B) Error-object leak (
lib/helpers/responses.js+ express terminal handler): stack traces and raw error objects are now stripped for any env that is not dev/test/localhost — i.e.!isDevEnv()— matching what downstream projects need.(C) Rate-limiter base layer (
lib/middlewares/,modules/billing/config/,modules/organizations/config/):apiandbillingPlansprofiles moved into the dev-config layer with permissive base caps (presence-driven via a Proxy);production.config.jsstill overrides with strict caps. This alignsapi/billingPlanswith howauthalready behaved.(D) Swagger + mongoose debug (
lib/services/express.js,lib/services/mongoose.js):/api/docsand/api/spec.jsonare now only served whenisDevEnv()is true; mongoose query debug is only enabled in dev envs.Deliberate tradeoffs
Swagger opt-out-by-default: In any non-dev env (including downstream staging/prod),
/api/docsand/api/spec.jsonare no longer served. A downstream that intentionally publishes production API docs has no escape hatch yet. A futureconfig.swagger.enableInProdflag would allow opting back in — not added here to keep scope tight.api/billingPlansrate limiters now active at base caps downstream: These were previously silently inactive for all non-productionenvs. They now apply at lenient base-layer caps to every downstream env, with strict caps binding only underNODE_ENV=production. This is the correct security posture — consistent with howauthalready behaved — but downstream configs can tighten the base caps via their own dev-config overrides if needed.Notes for reviewers
/update-stackneed to ensure their dev-config layers defineapi+billingPlansrate-limiter entries if they want project-specific caps (the base layer defaults are permissive, so it is safe to not define them).swagger.enableInProdescape-hatch flag (🔒 Production env-gate: error leak + swagger/debug/rate-limiter defaults #3852 tracks).Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Tests