Repository navigation
feat(common): dual-server db validation for byod config - #371
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds shared MongoDB URI safety validation, an authenticated Public API database check, bounded local and remote verification, differentiated errors, and dashboard collection of local and public server IPs. ChangesExternal database verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DashboardForm
participant DashboardAPI
participant PublicAPI
participant MongoDB
DashboardForm->>DashboardAPI: Submit external database configuration
DashboardAPI->>DashboardAPI: Validate dbUri with isSafeUri
DashboardAPI->>MongoDB: Verify local dbUri
DashboardAPI->>PublicAPI: Verify dbUri with x-internal-secret
PublicAPI->>PublicAPI: Validate dbUri with isSafeUri
PublicAPI->>MongoDB: Verify temporary connection
PublicAPI-->>DashboardAPI: Return verification result and serverIp
DashboardAPI-->>DashboardForm: Return verification result and IP guidance
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
apps/public-api/src/routes/internal.js (2)
9-20: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTiming-unsafe secret comparison.
providedSecret !== secretis a variable-time string comparison, which is a known side-channel weakness for secret verification.🔒 Use constant-time comparison
+const crypto = require('crypto'); + const internalAuth = (req, res, next) => { const secret = process.env.INTERNAL_SECRET; if (!secret) { console.error("INTERNAL_SECRET is not configured on Public API"); return res.status(500).json({ success: false, message: "Server misconfiguration" }); } const providedSecret = req.headers['x-internal-secret']; - if (!providedSecret || providedSecret !== secret) { + const provided = Buffer.from(String(providedSecret || '')); + const expected = Buffer.from(secret); + if (provided.length !== expected.length || !crypto.timingSafeEqual(provided, expected)) { return res.status(403).json({ success: false, message: "Forbidden: Invalid internal secret" }); } next(); };🤖 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 `@apps/public-api/src/routes/internal.js` around lines 9 - 20, Replace the variable-time providedSecret !== secret check in the internal-secret middleware with a constant-time comparison using the appropriate crypto API, ensuring both values are safely normalized to equal-length byte representations before comparison. Preserve the existing 403 response for missing or invalid secrets and the next() path for valid credentials.
30-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTemp MongoDB connections aren't closed on the failure path in either service. Both files create a
mongoose.createConnection(...)for verification but only call.close()on success; the same cleanup gap exists in both.
apps/public-api/src/routes/internal.js#L30-L51: closetempConnin afinallyblock covering both the success and catch paths.apps/dashboard-api/src/controllers/project.controller.js#L734-L738: apply the samefinally-based cleanup to its localtempConn.🤖 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 `@apps/public-api/src/routes/internal.js` around lines 30 - 51, Ensure both verification flows close their temporary MongoDB connections on success and failure. In apps/public-api/src/routes/internal.js lines 30-51, move tempConn cleanup into a finally block surrounding the try/catch and remove the success-only close; apply the same finally-based cleanup to the local tempConn in apps/dashboard-api/src/controllers/project.controller.js lines 734-738.apps/public-api/src/app.js (1)
82-84: 🔒 Security & Privacy | 🔵 TrivialConsider network-level restriction as defense in depth.
/api/internalrelies solely on a shared secret header for authorization. If feasible, also restrict this path at the infrastructure layer (VPC/security group, ingress rule) so it's unreachable from the public internet, complementing the secret check.🤖 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 `@apps/public-api/src/app.js` around lines 82 - 84, Restrict the /api/internal route exposed by app.use and the internalRoute registration to trusted internal network sources at the infrastructure layer, such as VPC, security-group, or ingress rules, while retaining the existing shared-secret authorization. Ensure the path is unreachable from the public internet without changing other routes.
🤖 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 `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 726-750: Move the getPublicIp() call out of the main try block and
into the catch block, where dashboardIp is only needed for failure reporting.
Keep the database connection and Public API verification flow unchanged, and
preserve lazy handling consistent with publicApiIp.
- Around line 730-777: Bound the verification flow in the surrounding controller
handler with an overall timeout so local MongoDB verification, remote
`/api/internal/test-db` verification, and fallback `/api/server-ip` lookup
cannot cumulatively exceed the request deadline. Preserve the existing error
classification and IP propagation while ensuring the timeout rejects promptly
and is handled by the existing connection-failure path.
- Around line 741-749: Update updateExternalConfig around the PUBLIC_API_URL and
INTERNAL_SECRET check so external DB confirmation cannot return success when
verification is skipped. Require both configuration values for external DB
updates and fail the operation when either is missing, unless an existing
explicit opt-out is provided; preserve the current axios verification path when
both values are present.
- Around line 755-760: Update the Axios error handling in the project connection
flow around connErr.response.data so only database-URI-specific verification
failures use the returned message; replace internal authentication,
configuration, and other infrastructure errors with a safe user-facing fallback
indicating public API verification failed. Preserve the existing serverIp
extraction behavior.
In `@apps/public-api/src/routes/internal.js`:
- Line 11: Normalize every response in the internal route handlers, including
the branches near the referenced lines, to the mandated { success, data, message
} shape by always including data as an object. Move the failure response’s
serverIp value into data.serverIp, and preserve the existing success status,
message, and payload semantics within data.
- Around line 22-34: Validate dbUri with the existing isSafeUri
restricted-host/internal-network check before mongoose.createConnection in the
/test-db route. Share or reuse that validator from a common module so this
endpoint independently rejects unsafe URIs with an appropriate client error,
while preserving the current connection test for safe URIs.
In `@apps/web-dashboard/src/components/Settings/DatabaseConfigForm.jsx`:
- Around line 24-33: Update the direct public API request in the server-IP
loading flow around Promise.allSettled to use an AbortController timeout and
validate response.ok before parsing JSON. Ensure a hung request is rejected
within the timeout so the fulfilled dashboard API result can still update
serverIp, while preserving the existing mounted-state and IP aggregation
behavior.
---
Nitpick comments:
In `@apps/public-api/src/app.js`:
- Around line 82-84: Restrict the /api/internal route exposed by app.use and the
internalRoute registration to trusted internal network sources at the
infrastructure layer, such as VPC, security-group, or ingress rules, while
retaining the existing shared-secret authorization. Ensure the path is
unreachable from the public internet without changing other routes.
In `@apps/public-api/src/routes/internal.js`:
- Around line 9-20: Replace the variable-time providedSecret !== secret check in
the internal-secret middleware with a constant-time comparison using the
appropriate crypto API, ensuring both values are safely normalized to
equal-length byte representations before comparison. Preserve the existing 403
response for missing or invalid secrets and the next() path for valid
credentials.
- Around line 30-51: Ensure both verification flows close their temporary
MongoDB connections on success and failure. In
apps/public-api/src/routes/internal.js lines 30-51, move tempConn cleanup into a
finally block surrounding the try/catch and remove the success-only close; apply
the same finally-based cleanup to the local tempConn in
apps/dashboard-api/src/controllers/project.controller.js lines 734-738.
🪄 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: 0a0085f2-567e-4508-a558-433bf643fa84
📒 Files selected for processing (4)
apps/dashboard-api/src/controllers/project.controller.jsapps/public-api/src/app.jsapps/public-api/src/routes/internal.jsapps/web-dashboard/src/components/Settings/DatabaseConfigForm.jsx
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds “dual-server” MongoDB BYOD connectivity validation so external DB connection strings are verified from both the Dashboard API and the Public API, and the UI surfaces both service IPs to whitelist when access is denied.
Changes:
- Introduces
isSafeUriin@urbackend/commonto validate DB URIs against restricted/internal IP ranges (SSRF prevention). - Adds a new Public API internal endpoint (
POST /api/internal/test-db) to verify DB connectivity from the Public API service. - Updates the web dashboard DB settings UI to fetch/display both Dashboard API and Public API server IPs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/common/src/utils/network.js | Adds SSRF-oriented URI safety validation (isSafeUri) and exports it. |
| packages/common/src/index.js | Re-exports isSafeUri from common package entrypoint. |
| apps/web-dashboard/src/components/Settings/DatabaseConfigForm.jsx | Fetches server IPs from both services and displays them together for whitelisting. |
| apps/public-api/src/routes/internal.js | Adds protected internal route to test DB connectivity from Public API. |
| apps/public-api/src/app.js | Mounts the new /api/internal route. |
| apps/dashboard-api/src/controllers/project.controller.js | Updates BYOD DB verification to validate both local and Public API connectivity and improve whitelist messaging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
|
whats in dual server validation? @yash-pouranik |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
apps/dashboard-api/src/controllers/project.controller.js (1)
616-626: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
AppErrorinstead of rawthrow new Error(...).Both the overall-timeout rejection and the missing-configuration rejection use plain
Errorobjects for internal control flow. The coding guidelines for this path requireAppErrorfor errors instead of rawthrow. Both throws are currently caught internally and mapped to compliant{success, data, message}responses, so there is no client-facing defect today, but aligning withAppErrorkeeps error handling consistent with the rest of the API surface.As per coding guidelines, "use AppError for errors, never raw throw."
🤖 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 `@apps/dashboard-api/src/controllers/project.controller.js` around lines 616 - 626, Replace both raw Error instances in the verification Promise flow— the overall timeout rejection and the missing PUBLIC_API_URL/INTERNAL_SECRET validation—with AppError instances. Preserve the existing timeout behavior and compliant success/data/message response mapping.Source: Coding guidelines
🤖 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 `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 645-668: Update the error classification around the Axios response
handling to explicitly detect Axios failures with no response, using
connErr.isAxiosError and !connErr.response (or the relevant network
timeout/refusal codes), and assign the same "Public API verification failed."
infrastructure message used by the existing response-error branch. Ensure these
failures do not fall through to the default MongoDB URI error.
- Line 636: Update the connection-failure logging around connErr in the project
controller to avoid writing connErr.message directly, since it may contain dbUri
credentials. Log only a safe failure classification, or sanitize the message to
redact connection credentials before passing it to console.error.
In `@packages/common/src/utils/network.js`:
- Around line 38-64: Extend isRestrictedIPv4 to return true for carrier-grade
NAT, documentation/test, multicast, reserved, and broadcast ranges listed in the
review comment. Add the checks alongside the existing IPv4 range checks while
preserving current behavior for all already-restricted ranges.
- Around line 109-148: Update isSafeUri and the downstream
mongoose.createConnection paths to resolve hostnames once, validate every A/AAAA
result, and return or propagate the validated address set for connection use
instead of resolving the original hostname again. Ensure Dashboard API, Public
API /api/internal/test-db, and connection.manager.js pass the pinned safe IP
through MongoDB/Mongoose lookup while preserving rejection of restricted or
unresolved targets.
- Around line 66-87: Update isRestrictedIPv6 to recognize IPv4-mapped addresses
in both dotted-decimal and hexadecimal last-32-bit forms. Parse the mapped IPv6
suffix into its numeric 32-bit value, convert or compare it against the
corresponding IPv4 restriction logic, and preserve existing handling for
loopback, unspecified, link-local, and ULA addresses.
- Around line 109-125: Update isSafeUri to parse MongoDB connection strings with
the project’s MongoDB connection-string parser instead of relying on new URL().
For mongodb:// URIs, iterate through every host in the parsed .hosts collection
and apply the existing hostname, blocked-host, and restricted-IP validation to
each host, while preserving rejection of mongodb+srv:// URIs; continue using URL
parsing for non-MongoDB schemes.
---
Nitpick comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 616-626: Replace both raw Error instances in the verification
Promise flow— the overall timeout rejection and the missing
PUBLIC_API_URL/INTERNAL_SECRET validation—with AppError instances. Preserve the
existing timeout behavior and compliant success/data/message response mapping.
🪄 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: b08990de-84bb-424e-a271-0d8dca0e852f
📒 Files selected for processing (6)
apps/dashboard-api/src/controllers/project.controller.jsapps/public-api/src/app.jsapps/public-api/src/routes/internal.jsapps/web-dashboard/src/components/Settings/DatabaseConfigForm.jsxpackages/common/src/index.jspackages/common/src/utils/network.js
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web-dashboard/src/components/Settings/DatabaseConfigForm.jsx
- apps/public-api/src/app.js
- apps/public-api/src/routes/internal.js
Member
Author
we have 2 servers So both servers IP address should be whitlisted in users mongo atlas setting. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/public-api/src/app.js (1)
86-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
/api/internalobservable or remove the no-op logger.
loggerexcludes/api/internalfrom its completion handler. The new database verification requests therefore produce no request logs or API analytics through this middleware. Add dedicated internal audit logging, or removeloggerfrom this mount if these requests do not require observability.🤖 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 `@apps/public-api/src/app.js` at line 86, Update the `/api/internal` mount involving `logger` so these requests remain observable: either add dedicated audit logging that records their completion and analytics, or remove `logger` from the mount if observability is intentionally unnecessary. Do not leave the current no-op logger middleware in place.
🤖 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.
Nitpick comments:
In `@apps/public-api/src/app.js`:
- Line 86: Update the `/api/internal` mount involving `logger` so these requests
remain observable: either add dedicated audit logging that records their
completion and analytics, or remove `logger` from the mount if observability is
intentionally unnecessary. Do not leave the current no-op logger middleware in
place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a0700f2-21f8-4470-8ab5-eaab40b36b4d
📒 Files selected for processing (4)
apps/dashboard-api/src/controllers/project.controller.jsapps/public-api/src/app.jsapps/public-api/src/routes/internal.jspackages/common/src/utils/network.js
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/public-api/src/routes/internal.js
- packages/common/src/utils/network.js
- apps/dashboard-api/src/controllers/project.controller.js
…on string parsing with custom lookup
Contributor
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 `@packages/common/src/utils/network.js`:
- Around line 169-197: Update the MongoDB connection safety flow around
isSafeUri and createSafeLookup so every topology-discovered replica-set or SRV
endpoint is validated against the existing private/restricted-IP allowlist
before socket creation. Ensure hostname targets use the same constrained DNS
resolution and IP literals cannot bypass validation; preserve rejection of
unsafe destinations rather than allowing unrestricted dynamic lookup.
- Around line 249-258: Update createSafeLookup to honor options.family by
filtering resolvedIps[lowerHost] to IPv4 or IPv6 addresses when requested, while
preserving all validated addresses when no family is specified. When options.all
is true, invoke cb with an array of address records containing address and
family; otherwise return the first matching address with its family as the
scalar lookup result.
🪄 Autofix
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: 5fe35655-bebe-4814-bed3-f689251156a8
📒 Files selected for processing (7)
apps/dashboard-api/src/__tests__/project.controller.byod.test.jsapps/dashboard-api/src/controllers/project.controller.jsapps/public-api/src/__tests__/internal.test.jsapps/public-api/src/routes/internal.jspackages/common/src/index.jspackages/common/src/utils/connection.manager.jspackages/common/src/utils/network.js
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/public-api/src/routes/internal.js
- apps/dashboard-api/src/controllers/project.controller.js
…Coderabbit network comments
yash-pouranik
temporarily deployed
to
feature/dual-server-db-validation - urBackend-frankfrut PR #371
August 4, 2026 20:38 — with 
Render
Destroyed
Render
Destroyed
…i/internal/test-db
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/public-api/src/__tests__/internal.test.js (1)
88-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert temporary connection cleanup.
Each case creates a temporary Mongoose connection and supplies
closeas a mock. Assert thatcloseruns after successful verification, local failure, remote failure, and timeout.
apps/public-api/src/__tests__/internal.test.js#L88-L101: AssertmockConn.closeafter the successful response.apps/dashboard-api/src/__tests__/project.controller.byod.test.js#L105-L123: AssertmockConn.closeafter local connection rejection.apps/dashboard-api/src/__tests__/project.controller.byod.test.js#L125-L151: AssertmockConn.closeafter remote verification failure.apps/dashboard-api/src/__tests__/project.controller.byod.test.js#L153-L172: AssertmockConn.closeafter timeout handling.🤖 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 `@apps/public-api/src/__tests__/internal.test.js` around lines 88 - 101, Assert that the temporary Mongoose connection’s close mock runs in every verification outcome: after successful verification in apps/public-api/src/__tests__/internal.test.js lines 88-101, and after local rejection, remote verification failure, and timeout handling in apps/dashboard-api/src/__tests__/project.controller.byod.test.js lines 105-123, 125-151, and 153-172. Add the close assertions after each response or error-handling flow using each test’s mockConn.close.
🤖 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 `@apps/dashboard-api/src/__tests__/project.controller.byod.test.js`:
- Around line 159-166: Update the test around updateExternalConfig so fake time
advances asynchronously with await jest.advanceTimersByTimeAsync(10000),
allowing the controller to schedule its verification timeout before advancing.
Wrap the fake-timer setup and updatePromise execution in try/finally to always
call jest.useRealTimers(), including when the test fails.
In `@apps/public-api/src/__tests__/internal.test.js`:
- Around line 98-100: Strengthen the response-envelope assertions in
apps/public-api/src/__tests__/internal.test.js lines 98-100 by asserting that
res.body.data is an object. In
apps/dashboard-api/src/__tests__/project.controller.byod.test.js lines 116-122
and 144-150, assert success is false; at lines 168-171, assert success is false
and data is an object. Keep the existing message and status assertions.
---
Nitpick comments:
In `@apps/public-api/src/__tests__/internal.test.js`:
- Around line 88-101: Assert that the temporary Mongoose connection’s close mock
runs in every verification outcome: after successful verification in
apps/public-api/src/__tests__/internal.test.js lines 88-101, and after local
rejection, remote verification failure, and timeout handling in
apps/dashboard-api/src/__tests__/project.controller.byod.test.js lines 105-123,
125-151, and 153-172. Add the close assertions after each response or
error-handling flow using each test’s mockConn.close.
🪄 Autofix
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: c0179f1f-16c8-4a5a-a7b5-e73207945619
📒 Files selected for processing (3)
apps/dashboard-api/src/__tests__/project.controller.byod.test.jsapps/public-api/src/__tests__/internal.test.jspackages/common/src/utils/network.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/common/src/utils/network.js
yash-pouranik
temporarily deployed
to
feature/dual-server-db-validation - urBackend-frankfrut PR #371
August 4, 2026 21:17 — with
Render
Destroyed
Render
Destroyed
This was referenced Aug 7, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While adding mongo db connection string in settings, user have to whitelist both the IPs of the microservices.
Summary by CodeRabbit
New Features
Bug Fixes