Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 44 additions & 4 deletions apps/dashboard-api/src/controllers/project.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -723,22 +723,62 @@ module.exports.updateExternalConfig = async (req, res) => {
updateData["resources.db.isExternal"] = true;

console.log("Verifying connection to:", projectId);

let dashboardIp = null;
let publicApiIp = null;

try {
dashboardIp = await getPublicIp();

// 1. Test local connection (Dashboard API)
const tempConn = mongoose.createConnection(dbUri, {
serverSelectionTimeoutMS: 5000,
});
await tempConn.asPromise();
await tempConn.close();

// 2. Test remote connection (Public API) if PUBLIC_API_URL is configured
if (process.env.PUBLIC_API_URL && process.env.INTERNAL_SECRET) {
const publicApiRes = await axios.post(`${process.env.PUBLIC_API_URL}/api/internal/test-db`,
{ dbUri },
{ headers: { 'x-internal-secret': process.env.INTERNAL_SECRET }, timeout: 8000 }
);
// If it didn't throw, it succeeded.
} else {
console.warn("Skipping Public API DB verification because PUBLIC_API_URL or INTERNAL_SECRET is missing.");
}
Comment on lines +741 to +749

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate file =="
git ls-files | grep -F 'apps/dashboard-api/src/controllers/project.controller.js' || true

echo "== file size == "
wc -l apps/dashboard-api/src/controllers/project.controller.js

echo "== outline relevant sections =="
ast-grep outline apps/dashboard-api/src/controllers/project.controller.js --match saveProject --view expanded || true
ast-grep outline apps/dashboard-api/src/controllers/project.controller.js --match validateProjectConnection --view expanded || true

echo "== lines 700-770 =="
sed -n '700,770p' apps/dashboard-api/src/controllers/project.controller.js | cat -n

echo "== relevant usages/test DB endpoint =="
rg -n "PUBLIC_API_URL|INTERNAL_SECRET|/api/internal/test-db|saveProject|validateProjectConnection|test DB|verification|warnings" apps/dashboard-api/src -S

Repository: geturbackend/urBackend

Length of output: 8065


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== project.controller.js snippet around rest of updateExternalConfig =="
sed -n '770,835p' apps/dashboard-api/src/controllers/project.controller.js | cat -n

echo "== project.controller.js snippet around tests endpoint =="
rg -n "/api/internal/test-db|test-db" apps -S

echo "== project controller routes for updateExternalConfig =="
rg -n "updateExternalConfig|routes\.projects|/projects/" apps/dashboard-api/src -S

Repository: geturbackend/urBackend

Length of output: 5480


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== test-db endpoint =="
sed -n '1,80p' apps/public-api/src/routes/internal.js | cat -n

echo "== response shape checks =="
node - <<'JS'
const fs = require('fs');
const path = 'apps/public-api/src/routes/internal.js';
const src = fs.readFileSync(path, 'utf8');
const endpoint = src.split("router.post('/test-db'")[1].split("router.")[0];
console.log('endpoint contains success: true:', endpoint.includes("success: true"));
console.log('endpoint contains success: false:', endpoint.includes("success: false"));
console.log('endpoint contains message:', endpoint.includes("message"));
JS

echo "== dashboard success/warnings paths =="
python3 - <<'PY'
from pathlib import Path
import re
p = Path('apps/dashboard-api/src/controllers/project.controller.js')
text = p.read_text()
for m in re.finditer(r"res\.status\(\d+\)\.json\(\{ success: (true|false), data: }, message: \"[^\"]+\"", text):
    print(text[m.start()-50:m.end()+100])
PY

Repository: geturbackend/urBackend

Length of output: 2669


Surface or enforce missing Public API DB verification.

When PUBLIC_API_URL or INTERNAL_SECRET is absent, updateExternalConfig logs a warning and then sends success: true for an externalDB confirmation that never ran. This defeats the dual-server validation guarantee; either require both values for external DB updates, or make the call fail closed unless the caller explicitly opts out.

🤖 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 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.


Comment on lines +726 to +750

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Eager getPublicIp() call risks misattributing unrelated failures as DB connection errors.

dashboardIp is fetched unconditionally before the connection test even starts, inside the same try block. If getPublicIp() itself fails/throws, the catch block will report it as "Could not connect to the provided MongoDB URI," which is misleading. It's also wasted work on the happy path since dashboardIp is only used in the catch block. Consider fetching it lazily inside catch, mirroring how publicApiIp is already fetched lazily (L769-777).

🤖 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 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.

} catch (connErr) {
console.error("Verification Connection Failed:", connErr.message);
let errorMsg = "Could not connect to the provided MongoDB URI.";

if (

// If the error came from Axios (Public API verification failed)
if (connErr.response && connErr.response.data && !connErr.response.data.success) {
errorMsg = connErr.response.data.message;
if (connErr.response.data.serverIp) {
publicApiIp = connErr.response.data.serverIp;
}
Comment on lines +755 to +760

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Internal API error messages are forwarded to end users verbatim.

errorMsg = connErr.response.data.message trusts whatever internal.js returns. If that call fails for infrastructure reasons (e.g., INTERNAL_SECRET mismatch → "Forbidden: Invalid internal secret", or missing config → "Server misconfiguration"), those internal-only strings get shown directly to the customer configuring their DB, leaking internal auth/config details and confusing users about the actual (their-DB-unrelated) failure cause.

🛡️ Only trust dbUri-specific failure messages
-        if (connErr.response && connErr.response.data && !connErr.response.data.success) {
-            errorMsg = connErr.response.data.message;
+        if (connErr.response && connErr.response.status === 400 && connErr.response.data && !connErr.response.data.success) {
+            errorMsg = connErr.response.data.message;
             if (connErr.response.data.serverIp) {
                 publicApiIp = connErr.response.data.serverIp;
             }
+        } else if (connErr.response) {
+            // 403/500 from the internal route indicate our own infra issue, not the user's DB URI
+            console.error("Internal verification service error:", connErr.response.status, connErr.response.data);
         } else if (
🤖 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 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.

} else if (
connErr.message.includes("Server selection timed out") ||
connErr.message.includes("Could not connect")
) {
const serverIp = await getPublicIp();
errorMsg = `Access Denied: Please whitelist Server IP [${serverIp}] in MongoDB Atlas.`;
// Local failure
errorMsg = `Access Denied: Please whitelist Server IPs in MongoDB Atlas.`;
}

// Fetch Public API IP if we don't have it yet, to show both in the error message
if (!publicApiIp && process.env.PUBLIC_API_URL) {
try {
const ipRes = await axios.get(`${process.env.PUBLIC_API_URL}/api/server-ip`, { timeout: 3000 });
if (ipRes.data && ipRes.data.ip) publicApiIp = ipRes.data.ip;
} catch (e) {
console.error("Failed to fetch public API IP for error message", e.message);
}
}
Comment on lines 730 to +777

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

Sequential verification steps can stack up to ~16s of latency on one request.

Local test (≤5s serverSelectionTimeoutMS) → remote test (≤8s axios timeout) → extra public-IP lookup on failure (≤3s) all run sequentially within a single HTTP request handler, with no overall deadline. This risks gateway/proxy timeouts and a poor UX for a config-save action.

Consider running local/remote checks concurrently where possible, or wrapping the whole verification in an overall timeout (e.g., Promise.race with a bounded deadline) independent of the individual per-call timeouts.

🤖 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 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.


if (dashboardIp || publicApiIp) {
const ips = [dashboardIp, publicApiIp].filter(Boolean).join(", ");
errorMsg = `Access Denied: Please whitelist Server IPs [${ips}] in MongoDB Atlas.`;
}

return res.status(400).json({ success: false, data: {}, message: errorMsg });
Expand Down
3 changes: 3 additions & 0 deletions apps/public-api/src/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ app.use('/api/storage', limiter, logger, storageRoute);
app.use('/api/mail', limiter, logger, mailRoute);
app.use('/api/health', limiter, logger, healthRoute);

const internalRoute = require('./routes/internal');
app.use('/api/internal', internalRoute);

app.get('/api/server-ip', async (req, res) => {
const ip = await getPublicIp();
res.json({ ip });
Expand Down
53 changes: 53 additions & 0 deletions apps/public-api/src/routes/internal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
const express = require('express');
const mongoose = require('mongoose');
const { getPublicIp } = require('@urbackend/common');
const router = express.Router();

// Middleware to protect internal routes
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" });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Responses don't conform to the mandated { success, data, message } shape.

Every response in this file omits data: {}, and the failure response also adds a non-standard top-level serverIp field instead of nesting it under data.

As per coding guidelines, "All API endpoints must return { success: bool, data: {}, message: "" }."

🔧 Normalize response shape
-        return res.status(500).json({ success: false, message: "Server misconfiguration" });
+        return res.status(500).json({ success: false, data: {}, message: "Server misconfiguration" });
...
-        return res.status(403).json({ success: false, message: "Forbidden: Invalid internal secret" });
+        return res.status(403).json({ success: false, data: {}, message: "Forbidden: Invalid internal secret" });
...
-        return res.status(400).json({ success: false, message: "dbUri is required" });
+        return res.status(400).json({ success: false, data: {}, message: "dbUri is required" });
...
-        return res.status(200).json({ success: true, message: "Connection verified" });
+        return res.status(200).json({ success: true, data: {}, message: "Connection verified" });
...
-        return res.status(400).json({ success: false, message: errorMsg, serverIp });
+        return res.status(400).json({ success: false, data: { serverIp }, message: errorMsg });

Also applies to: 16-16, 25-25, 35-35, 49-49

🤖 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` at 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.

Source: Coding guidelines

}

const providedSecret = req.headers['x-internal-secret'];
if (!providedSecret || providedSecret !== secret) {
return res.status(403).json({ success: false, message: "Forbidden: Invalid internal secret" });
}

next();
};

router.post('/test-db', internalAuth, async (req, res) => {
const { dbUri } = req.body;
if (!dbUri) {
return res.status(400).json({ success: false, message: "dbUri is required" });
}

console.log("[Internal] Verifying connection for external DB...");
try {
const tempConn = mongoose.createConnection(dbUri, {
serverSelectionTimeoutMS: 5000,
});
await tempConn.asPromise();
await tempConn.close();
Comment on lines +22 to +34

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

No independent SSRF/host validation before opening the connection.

dbUri is trusted as-is here and passed straight to mongoose.createConnection. The only restricted-host/loopback/internal-network check (isSafeUri) lives in apps/dashboard-api/src/controllers/project.controller.js and is never applied on this side. This endpoint effectively becomes an open connectivity prober for any host if reached with an unvalidated URI (secret leak, misconfigured caller, future direct integration), since it will attempt outbound connections to whatever dbUri is supplied.

Recommend duplicating/sharing the restricted-host validation (e.g., move isSafeUri into @urbackend/common so both services enforce it) before calling mongoose.createConnection here.

🤖 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 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.

return res.status(200).json({ success: true, message: "Connection verified" });
} catch (connErr) {
console.error("[Internal] Verification Connection Failed:", connErr.message);
let errorMsg = "Could not connect to the provided MongoDB URI.";
let serverIp = null;

if (
connErr.message.includes("Server selection timed out") ||
connErr.message.includes("Could not connect")
) {
serverIp = await getPublicIp();
errorMsg = `Access Denied: Please whitelist Server IP [${serverIp}] in MongoDB Atlas.`;
}

return res.status(400).json({ success: false, message: errorMsg, serverIp });
}
});

module.exports = router;
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,17 @@ export default function DatabaseConfigForm({ project, projectId, onProjectUpdate
Promise.resolve().then(() => setShowForm(!(project?.resources?.db?.isExternal || false)));
const fetchIp = async () => {
try {
const res = await api.get(`/api/server-ip`);
if (isMounted) setServerIp(res.data.ip);
const { PUBLIC_API_URL } = await import('../../config');
const [res1, res2] = await Promise.allSettled([
api.get(`/api/server-ip`),
fetch(`${PUBLIC_API_URL}/api/server-ip`).then(r => r.json())
]);

const ips = [];
if (res1.status === 'fulfilled' && res1.value.data.ip) ips.push(res1.value.data.ip);
if (res2.status === 'fulfilled' && res2.value.ip) ips.push(res2.value.ip);

if (isMounted) setServerIp(ips.join(', '));
Comment on lines +24 to +33

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Bound the public API request.

The new fetch has no timeout. Because Promise.allSettled waits for every promise, a hung public API prevents serverIp from being updated—even when the dashboard API already returned its IP—leaving the whitelist guidance stuck on ....

Add an abort timeout (and check response.ok) for the direct request, or update the state as each successful request completes.

Proposed timeout handling
+                const controller = new AbortController();
+                const timeoutId = setTimeout(() => controller.abort(), 5000);
                 const [res1, res2] = await Promise.allSettled([
                     api.get(`/api/server-ip`),
-                    fetch(`${PUBLIC_API_URL}/api/server-ip`).then(r => r.json())
+                    fetch(`${PUBLIC_API_URL}/api/server-ip`, { signal: controller.signal })
+                        .then(r => {
+                            if (!r.ok) throw new Error(`HTTP ${r.status}`);
+                            return r.json();
+                        })
+                        .finally(() => clearTimeout(timeoutId))
                 ]);
📝 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.

Suggested change
const [res1, res2] = await Promise.allSettled([
api.get(`/api/server-ip`),
fetch(`${PUBLIC_API_URL}/api/server-ip`).then(r => r.json())
]);
const ips = [];
if (res1.status === 'fulfilled' && res1.value.data.ip) ips.push(res1.value.data.ip);
if (res2.status === 'fulfilled' && res2.value.ip) ips.push(res2.value.ip);
if (isMounted) setServerIp(ips.join(', '));
const controller = new AbortController();
const timeoutId = setTimeout(() => controller.abort(), 5000);
const [res1, res2] = await Promise.allSettled([
api.get(`/api/server-ip`),
fetch(`${PUBLIC_API_URL}/api/server-ip`, { signal: controller.signal })
.then(r => {
if (!r.ok) throw new Error(`HTTP ${r.status}`);
return r.json();
})
.finally(() => clearTimeout(timeoutId))
]);
const ips = [];
if (res1.status === 'fulfilled' && res1.value.data.ip) ips.push(res1.value.data.ip);
if (res2.status === 'fulfilled' && res2.value.ip) ips.push(res2.value.ip);
if (isMounted) setServerIp(ips.join(', '));
🤖 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/web-dashboard/src/components/Settings/DatabaseConfigForm.jsx` around
lines 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.

}
catch (e) { console.error("Failed to fetch server IP", e); }
};
Expand Down
Loading