Skip to content

fix(adhoc-plugins-openframe-js): 3 review findings in openframe.js #137

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.

Sign up for GitHub

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

Draft
flamingo wants to merge 1 commit into
base: master
Choose a base branch
from
+30 −5
Draft

fix(adhoc-plugins-openframe-js): 3 review findings in openframe.js #137

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 30 additions & 5 deletions plugins/openframe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,13 @@ const MESH_DEVICE_GROUP = process.env.MESH_DEVICE_GROUP || '';

// --- Helpers ---

@flamingo flamingo Bot Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 CORS wildcard origin on all plugin API routes enables cross-origin data exfiltration

Changed corsHeaders(res) to corsHeaders(req, res) everywhere (definition and all three call sites: OPTIONS preflight, /generate-msh, /api/deviceStatus). The new implementation reads req.headers.origin and only echoes it back in Access-Control-Allow-Origin when it appears in the ALLOWED_ORIGINS allowlist (populated from process.env.CORS_ALLOWED_ORIGINS, a comma-separated list). A Vary: Origin header is added whenever a specific origin is reflected. If CORS_ALLOWED_ORIGINS is empty or the request origin is not listed, no Access-Control-Allow-Origin header is emitted, so browsers will block cross-origin reads. Risk: operators must set CORS_ALLOWED_ORIGINS in their environment or all browser-initiated cross-origin requests will be blocked (which is the safe default). Server-to-server callers (no Origin header) are unaffected.

🤖 Prompt for AI agents 
In plugins/openframe.js around line 9, review and complete this code-review fix: CORS wildcard origin on all plugin API routes enables cross-origin data exfiltration.
What the draft fix changed: Changed `corsHeaders(res)` to `corsHeaders(req, res)` everywhere (definition and all three call sites: OPTIONS preflight, `/generate-msh`, `/api/deviceStatus`). The new implementation reads `req.headers.origin` and only echoes it back in `Access-Control-Allow-Origin` when it appears in the `ALLOWED_ORIGINS` allowlist (populated from `process.env.CORS_ALLOWED_ORIGINS`, a comma-separated list). A `Vary: Origin` header is added whenever a specific origin is reflected. If `CORS_ALLOWED_ORIGINS` is empty or the request origin is not listed, no `Access-Control-Allow-Origin` header is emitted, so browsers will block cross-origin reads. Risk: operators must set `CORS_ALLOWED_ORIGINS` in their environment or all browser-initiated cross-origin requests will be blocked (which is the safe default). Server-to-server callers (no `Origin` header) are unaffected.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer


function corsHeaders(res) {
res.set('Access-Control-Allow-Origin', '*');
const ALLOWED_ORIGINS = (process.env.CORS_ALLOWED_ORIGINS || '').split(',').filter(Boolean);
function corsHeaders(req, res) {
var origin = req.headers.origin;
if (origin && ALLOWED_ORIGINS.includes(origin)) {
res.set('Access-Control-Allow-Origin', origin);
res.set('Vary', 'Origin');
}
res.set('Access-Control-Allow-Methods', 'GET, POST, OPTIONS');
res.set('Access-Control-Allow-Headers', 'Content-Type, X-MeshAuth');
}
Expand All @@ -32,6 +37,22 @@ function deriveTenantDomain(domains) {
return '';
}

// Validate the X-MeshAuth shared secret. Returns true if the request is authenticated.
var MESH_AUTH_SECRET = process.env.MESH_AUTH_SECRET || '';
function checkAuth(req, res) {
if (!MESH_AUTH_SECRET) {
// No secret configured — deny all to avoid accidentally open endpoints
sendError(res, 403, 'Authentication not configured');
return false;
}
var provided = req.headers['x-meshauth'] || '';
if (provided !== MESH_AUTH_SECRET) {
sendError(res, 401, 'Unauthorized');
return false;
}
return true;
}

// --- Plugin ---

module.exports.openframe = function (pluginHandler) {
Expand All @@ -51,13 +72,15 @@ module.exports.openframe = function (pluginHandler) {

// CORS preflight
app.options(['/generate-msh', '/api/*'], function (req, res) {
corsHeaders(res);
corsHeaders(req, res);
res.sendStatus(204);
});

// Route 1: GET /generate-msh?host=X - Generate custom MSH agent config
app.get('/generate-msh', function (req, res) {
corsHeaders(res);
corsHeaders(req, res);

if (!checkAuth(req, res)) return;

var host = req.query.host;
if (!host) return sendError(res, 400, 'Missing required parameter: host');
Comment on lines 72 to 86

@flamingo flamingo Bot Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 No authentication check on /generate-msh — any caller can obtain mesh credentials

Added a checkAuth helper (lines 43–52) that validates the X-MeshAuth request header against process.env.MESH_AUTH_SECRET. The /generate-msh handler now calls if (!checkAuth(req, res)) return; immediately after setting CORS headers, before any file I/O or response. If MESH_AUTH_SECRET is not set in the environment the helper rejects all requests with 403 to avoid accidentally open endpoints. Risk: this is a shared-secret scheme rather than a full MeshCentral session check; a complete fix would additionally validate a MeshCentral session cookie via parent.webserver.validateCookie or equivalent, which requires knowledge of the MeshCentral internal API not visible in this file. The shared-secret approach is a real, deployable improvement that closes the unauthenticated-access finding.

🤖 Prompt for AI agents 
In plugins/openframe.js around line 64, review and complete this code-review fix: No authentication check on /generate-msh — any caller can obtain mesh credentials.
What the draft fix changed: Added a `checkAuth` helper (lines 43–52) that validates the `X-MeshAuth` request header against `process.env.MESH_AUTH_SECRET`. The `/generate-msh` handler now calls `if (!checkAuth(req, res)) return;` immediately after setting CORS headers, before any file I/O or response. If `MESH_AUTH_SECRET` is not set in the environment the helper rejects all requests with 403 to avoid accidentally open endpoints. Risk: this is a shared-secret scheme rather than a full MeshCentral session check; a complete fix would additionally validate a MeshCentral session cookie via `parent.webserver.validateCookie` or equivalent, which requires knowledge of the MeshCentral internal API not visible in this file. The shared-secret approach is a real, deployable improvement that closes the unauthenticated-access finding.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -95,7 +118,9 @@ module.exports.openframe = function (pluginHandler) {
// Route 2: GET /api/deviceStatus?id=node/<domain>/<hash> - Get device status
// Uses MeshCentral core: GetConnectivityState() (in-memory) + db 'lc' record
app.get('/api/deviceStatus', function (req, res) {
corsHeaders(res);
corsHeaders(req, res);

if (!checkAuth(req, res)) return;

var nodeId = req.query.id;

@flamingo flamingo Bot Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 No authentication on /api/deviceStatus — unauthenticated callers can probe device existence and connectivity

Added the same if (!checkAuth(req, res)) return; guard to /api/deviceStatus immediately after corsHeaders, before any DB access or data is returned. Same mechanism and same risk/caveat as finding 2: shared-secret via X-MeshAuth / MESH_AUTH_SECRET env var. A full session-based check would require MeshCentral internals not visible here.

🤖 Prompt for AI agents 
In plugins/openframe.js around line 100, review and complete this code-review fix: No authentication on /api/deviceStatus — unauthenticated callers can probe device existence and connectivity.
What the draft fix changed: Added the same `if (!checkAuth(req, res)) return;` guard to `/api/deviceStatus` immediately after `corsHeaders`, before any DB access or data is returned. Same mechanism and same risk/caveat as finding 2: shared-secret via `X-MeshAuth` / `MESH_AUTH_SECRET` env var. A full session-based check would require MeshCentral internals not visible here.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer

if (!nodeId) return sendError(res, 400, 'Missing required parameter: id');
Expand Down