-
Notifications
You must be signed in to change notification settings - Fork 67
feat(common): dual-server db validation for byod config #371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
+726
to
+750
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Eager
🤖 Prompt for AI Agents |
||
| } 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
🛡️ 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 |
||
| } 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Consider running local/remote checks concurrently where possible, or wrapping the whole verification in an overall timeout (e.g., 🤖 Prompt for AI Agents |
||
|
|
||
| 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 }); | ||
|
|
||
| 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" }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Every response in this file omits As per coding guidelines, "All API endpoints must return 🔧 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 AgentsSource: 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Recommend duplicating/sharing the restricted-host validation (e.g., move 🤖 Prompt for AI Agents |
||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win Bound the public API request. The new Add an abort timeout (and check 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| catch (e) { console.error("Failed to fetch server IP", e); } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
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:
Repository: geturbackend/urBackend
Length of output: 8065
🏁 Script executed:
Repository: geturbackend/urBackend
Length of output: 5480
🏁 Script executed:
Repository: geturbackend/urBackend
Length of output: 2669
Surface or enforce missing Public API DB verification.
When
PUBLIC_API_URLorINTERNAL_SECRETis absent,updateExternalConfiglogs a warning and then sendssuccess: truefor 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