fix(adhoc-plugins-openframe-js): 3 review findings in openframe.js - #137
fix(adhoc-plugins-openframe-js): 3 review findings in openframe.js#137flamingo[bot] wants to merge 1 commit into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -8,8 +8,13 @@ const MESH_DEVICE_GROUP = process.env.MESH_DEVICE_GROUP || ''; | |||
|
|
|||
| // --- Helpers --- | |||
There was a problem hiding this comment.
🦩 🔴 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
|
|
||
| // 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'); |
There was a problem hiding this comment.
🦩 🔴 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
|
|
||
| if (!checkAuth(req, res)) return; | ||
|
|
||
| var nodeId = req.query.id; |
There was a problem hiding this comment.
🦩 🔴 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
Closes 3 review findings in
plugins/openframe.js.Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
Note
1 of these finding(s) already have a fix PR (#93); this PR covers the remainder, and their tracking stays on the original.
plugins/openframe.js:9plugins/openframe.js:64plugins/openframe.js:100What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
bd83e60f-661c-461b-b0a4-217f424f321cMerging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.