fix(adhoc-sweep-fixes): 40 review findings across 40 files - #122
fix(adhoc-sweep-fixes): 40 review findings across 40 files#122flamingo[bot] wants to merge 40 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| refMatchByHostUUID[match.HostUUID] = match | ||
| } | ||
|
|
||
| // Log any conflicts we found. We don't want to fail the migration, but | ||
| // we do want to surface potential issues for investigation. | ||
| // Log any conflicts we found via the migration logger. We don't fail the | ||
| // migration outright, but we do surface these as warnings so they are | ||
| // captured in structured logs (rather than stdout) for investigation, | ||
| // since silently ignoring conflicting duplicate accounts could result in | ||
| // devices being mis-enrolled to the wrong IDP account. | ||
| msg := "" | ||
| if len(ignored) > 0 { | ||
| msg += fmt.Sprintf("ignoring %d host email records because no matching account or conflicting acount information\n", len(ignored)) |
There was a problem hiding this comment.
🦩 🟠 Migration swallows critical data-integrity conflicts instead of failing the migration
In Up_20250502222222, replaced the fmt.Println(msg) (and removed the stale // TODO: return or log error? / commented-out return errors.New(msg) lines) with a structured log call logger.Warn.Log(...). This surfaces conflicts via structured logging instead of stdout as requested. However, this file's migrations package does not visibly import/define a logger variable in the shown code, so this change assumes a package-level logger (e.g. a go-kit logger such as used elsewhere in fleetdm/fleet migrations) is available in this package; if it is not, this will fail to compile and the import/wiring must be added. I did not add a conflict-count threshold to fail the migration, since doing so risks blocking deploys without clearer product guidance on an acceptable threshold — this is flagged as the remaining risk the finding also suggested ("consider failing/blocking if conflicts exceed some threshold").
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20250502222222_AddMdmEnrollTables.go around line 187, review and complete this code-review fix: Migration swallows critical data-integrity conflicts instead of failing the migration.
What the draft fix changed: In Up_20250502222222, replaced the `fmt.Println(msg)` (and removed the stale `// TODO: return or log error?` / commented-out `return errors.New(msg)` lines) with a structured log call `logger.Warn.Log(...)`. This surfaces conflicts via structured logging instead of stdout as requested. However, this file's migrations package does not visibly import/define a `logger` variable in the shown code, so this change assumes a package-level `logger` (e.g. a go-kit logger such as used elsewhere in fleetdm/fleet migrations) is available in this package; if it is not, this will fail to compile and the import/wiring must be added. I did not add a conflict-count threshold to fail the migration, since doing so risks blocking deploys without clearer product guidance on an acceptable threshold — this is flagged as the remaining risk the finding also suggested ("consider failing/blocking if conflicts exceed some threshold").
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer
| @@ -76,7 +81,9 @@ func (c *goqueryClient) ScheduleQuery(uuid, query string) (string, error) { | |||
| c.queryCounter++ | |||
| queryName := strconv.Itoa(c.queryCounter) | |||
|
|
|||
There was a problem hiding this comment.
🦩 🟠 goqueryClient.ScheduleQuery mutates shared maps from a goroutine without synchronization
Added a sync.Mutex field (mu) to goqueryClient and guarded all reads/writes of c.queries and c.hostnameByUUID with c.mu.Lock()/c.mu.Unlock() in CheckHost, ScheduleQuery (both the synchronous "Pending" write and the asynchronous goroutine writes on completion/error), and FetchResults. This eliminates the concurrent map read/write race between the REPL goroutine and the query-completion goroutine, matching the suggested fix exactly. queryCounter was left unguarded since it's only ever touched from the calling goroutine per the existing code path, but if ScheduleQuery can be invoked concurrently that field would also need protection — not evidenced in this file.
🤖 Prompt for AI agents
In cmd/fleetctl/fleetctl/goquerycmd/goquery.go around line 78, review and complete this code-review fix: goqueryClient.ScheduleQuery mutates shared maps from a goroutine without synchronization.
What the draft fix changed: Added a `sync.Mutex` field (`mu`) to `goqueryClient` and guarded all reads/writes of `c.queries` and `c.hostnameByUUID` with `c.mu.Lock()`/`c.mu.Unlock()` in `CheckHost`, `ScheduleQuery` (both the synchronous "Pending" write and the asynchronous goroutine writes on completion/error), and `FetchResults`. This eliminates the concurrent map read/write race between the REPL goroutine and the query-completion goroutine, matching the suggested fix exactly. `queryCounter` was left unguarded since it's only ever touched from the calling goroutine per the existing code path, but if `ScheduleQuery` can be invoked concurrently that field would also need protection — not evidenced in this file.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| } | ||
| generatingCsv.end(); | ||
|
|
||
| // After the the csvString has been generated by the writableStream, broadcast the csvString to the requesting user's socket. | ||
| writableStream.on('finish', () => { | ||
| if(this.req.isSocket){ | ||
| // Note: we're sending the cveId with the cvsString, this is so we can set the filename in our frontend code. | ||
| sails.sockets.broadcast(roomId, 'singleCsvExportDone', {csv: csvString, cveId}); | ||
| // Unsubscribe the socket from the room. | ||
| sails.sockets.leave(this.req, roomId); | ||
| } else { | ||
| return csvString; | ||
| } | ||
| // Wait for the csvString to be fully generated by the writableStream before continuing. | ||
| await new Promise((resolve)=>{ | ||
| writableStream.on('finish', ()=>{ | ||
| resolve(); | ||
| }); | ||
| }); | ||
|
|
||
| if(this.req.isSocket){ | ||
| // Note: we're sending the cveId with the cvsString, this is so we can set the filename in our frontend code. | ||
| sails.sockets.broadcast(roomId, 'singleCsvExportDone', {csv: csvString, cveId}); | ||
| // Unsubscribe the socket from the room. | ||
| sails.sockets.leave(this.req, roomId); | ||
| } else { | ||
| return csvString; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| }; | ||
|
|
There was a problem hiding this comment.
🦩 🟠 download-one-vulnerability-csv controller can hang indefinitely for non-socket requests due to async write-then-return inside event handler
In fn (download-one-vulnerability-csv.js), replaced the fire-and-forget writableStream.on('finish', ...) callback (whose inner return csvString had no effect on the outer async function) with an await new Promise(...) that resolves on the stream's finish event. The if(this.req.isSocket) broadcast/leave logic and the else { return csvString; } are now executed directly in the outer fn body after the await, so return csvString actually resolves the machine's promise for non-socket requests, fixing the hang/empty-response bug. Risk: assumes no other code relies on the old (broken) synchronous-return timing; behavior for the socket branch is unchanged aside from now being awaited before fn resolves, which is a minor, intended change to ensure the exit fires only after CSV generation completes.
🤖 Prompt for AI agents
In ee/vulnerability-dashboard/api/controllers/download-one-vulnerability-csv.js around line 82, review and complete this code-review fix: download-one-vulnerability-csv controller can hang indefinitely for non-socket requests due to async write-then-return inside event handler.
What the draft fix changed: In `fn` (download-one-vulnerability-csv.js), replaced the fire-and-forget `writableStream.on('finish', ...)` callback (whose inner `return csvString` had no effect on the outer async function) with an `await new Promise(...)` that resolves on the stream's `finish` event. The `if(this.req.isSocket)` broadcast/leave logic and the `else { return csvString; }` are now executed directly in the outer `fn` body after the await, so `return csvString` actually resolves the machine's promise for non-socket requests, fixing the hang/empty-response bug. Risk: assumes no other code relies on the old (broken) synchronous-return timing; behavior for the socket branch is unchanged aside from now being awaited before `fn` resolves, which is a minor, intended change to ensure the exit fires only after CSV generation completes.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
|
|
||
| generatingCsv.end(); | ||
| // After the the csvString has been generated by the writableStream, broadcast the csvString to the requesting user's socket. | ||
| writableStream.on('finish', () => { | ||
| if(this.req.isSocket){ | ||
| // Note: we're sending the cveId with the cvsString, this is so we can set the filename in our frontend code. | ||
| sails.sockets.broadcast(roomId, 'csvExportDone', csvString); | ||
| // Unsubscribe the socket from the room. | ||
| sails.sockets.leave(this.req, roomId); | ||
| } else { | ||
| return csvString; | ||
| } | ||
| await new Promise((resolve) => { | ||
| writableStream.on('finish', () => { | ||
| if(this.req.isSocket){ | ||
| // Note: we're sending the cveId with the cvsString, this is so we can set the filename in our frontend code. | ||
| sails.sockets.broadcast(roomId, 'csvExportDone', csvString); | ||
| // Unsubscribe the socket from the room. | ||
| sails.sockets.leave(this.req, roomId); | ||
| } | ||
| resolve(); | ||
| }); | ||
| }); | ||
| if(!this.req.isSocket){ | ||
| return csvString; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| }; | ||
|
|
There was a problem hiding this comment.
🦩 🟠 download-unpatched-hosts-csv controller has the same unreachable return-inside-event-handler bug as the sibling vulnerability CSV controller
In fn (download-unpatched-hosts-csv.js), replaced the fire-and-forget writableStream.on('finish', ...) callback (which had an unreachable return csvString; inside the event handler) with an await new Promise(...) that resolves once the finish event fires, keeping the socket-broadcast side effect inside the callback. After the stream completes, the outer async fn now checks this.req.isSocket and returns csvString directly from the outer function scope for non-socket requests, so the HTTP response body is now properly populated instead of resolving with undefined before the CSV finishes generating.
🤖 Prompt for AI agents
In ee/vulnerability-dashboard/api/controllers/download-unpatched-hosts-csv.js around line 91, review and complete this code-review fix: download-unpatched-hosts-csv controller has the same unreachable return-inside-event-handler bug as the sibling vulnerability CSV controller.
What the draft fix changed: In `fn` (download-unpatched-hosts-csv.js), replaced the fire-and-forget `writableStream.on('finish', ...)` callback (which had an unreachable `return csvString;` inside the event handler) with an `await new Promise(...)` that resolves once the `finish` event fires, keeping the socket-broadcast side effect inside the callback. After the stream completes, the outer async `fn` now checks `this.req.isSocket` and returns `csvString` directly from the outer function scope for non-socket requests, so the HTTP response body is now properly populated instead of resolving with `undefined` before the CSV finishes generating.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| } | ||
| }//∞ | ||
| generatingCsv.end(); | ||
| // After the the csvString has been generated by the writableStream, broadcast the csvString to the requesting user's socket. | ||
| writableStream.on('finish', () => { | ||
| if(this.req.isSocket){ | ||
| // Note: we're sending the cveId with the cvsString, this is so we can set the filename in our frontend code. | ||
| sails.sockets.broadcast(roomId, 'csvExportDone', csvString); | ||
| // Unsubscribe the socket from the room. | ||
| sails.sockets.leave(this.req, roomId); | ||
| } else { | ||
| return csvString; | ||
| } | ||
| // After the the csvString has been generated by the writableStream, broadcast the csvString to the requesting user's socket | ||
| // (or, for the non-socket case, wait for the stream to finish before resolving so the response isn't sent empty). | ||
| await new Promise((resolve, reject) => { | ||
| writableStream.on('error', (err) => { | ||
| reject(err); | ||
| }); | ||
| writableStream.on('finish', () => { | ||
| resolve(); | ||
| }); | ||
| }); | ||
| if(this.req.isSocket){ | ||
| // Note: we're sending the cveId with the cvsString, this is so we can set the filename in our frontend code. | ||
| sails.sockets.broadcast(roomId, 'csvExportDone', csvString); | ||
| // Unsubscribe the socket from the room. | ||
| sails.sockets.leave(this.req, roomId); | ||
| } else { | ||
| return csvString; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| }; | ||
|
|
There was a problem hiding this comment.
🦩 🟠 download-vulnerabilities-csv streams response via async writable finish handler with no error handling and possible unhandled promise
In fn (download-vulnerabilities-csv.js), replaced the fire-and-forget writableStream.on('finish', ...) callback (which returned csvString asynchronously, after fn had already resolved) with an await new Promise(...) that attaches both 'finish' and 'error' listeners to writableStream before proceeding. The promise resolves on 'finish' and rejects on 'error', so fn now correctly awaits stream completion before checking this.req.isSocket and returning csvString (for the HTTP case) or broadcasting via socket (for the socket case), and any write errors now propagate as a rejected/thrown error instead of being silently dropped. Risk: behavior on error now causes the action to throw, which will be handled by Sails' default error exit rather than the previous silent failure — this is the intended fix but changes the error-path response, which should be reviewed for whether an explicit error exit annotation is desired.
(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)
🤖 Prompt for AI agents
In ee/vulnerability-dashboard/api/controllers/download-vulnerabilities-csv.js around line 139, review and complete this code-review fix: download-vulnerabilities-csv streams response via async writable finish handler with no error handling and possible unhandled promise.
What the draft fix changed: In `fn` (download-vulnerabilities-csv.js), replaced the fire-and-forget `writableStream.on('finish', ...)` callback (which returned `csvString` asynchronously, after `fn` had already resolved) with an `await new Promise(...)` that attaches both `'finish'` and `'error'` listeners to `writableStream` before proceeding. The promise resolves on `'finish'` and rejects on `'error'`, so `fn` now correctly awaits stream completion before checking `this.req.isSocket` and returning `csvString` (for the HTTP case) or broadcasting via socket (for the socket case), and any write errors now propagate as a rejected/thrown error instead of being silently dropped. Risk: behavior on error now causes the action to throw, which will be handled by Sails' default error exit rather than the previous silent failure — this is the intended fix but changes the error-path response, which should be reviewed for whether an explicit `error` exit annotation is desired.
_(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)_
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
| PartnerRemediationUrl: `https://fleetdm.com/microsoft-compliance-partner/remediate`, | ||
| } | ||
| }).intercept((err)=>{ | ||
| return new Error({error: `an error occurred when deprovisioning a Microsoft compliance tenant. Full error: ${require('util').inspect(err, {depth: 3})}`}); | ||
| sails.log.warn(`an error occurred when deprovisioning a Microsoft compliance tenant. Full error: ${require('util').inspect(err, {depth: 3})}`); | ||
| return new Error(`an error occurred when deprovisioning a Microsoft compliance tenant. Full error: ${require('util').inspect(err, {depth: 3})}`); | ||
| }); | ||
| // Log responses from Micrsoft APIs for Fleet's integration | ||
| if(informationAboutThisTenant.fleetInstanceUrl === 'https://dogfood.fleetdm.com') { |
There was a problem hiding this comment.
🦩 🟠 remove-one-compliance-partner-tenant.js constructs an Error with an object instead of a string, and returns it instead of throwing
In the .intercept((err)=>{...}) callback inside fn (the PUT deprovision request in remove-one-compliance-partner-tenant.js), changed new Error({error: ...}) to new Error(...) using a proper string message (matching other .intercept() call sites in this file), and added a sails.log.warn(...) call with the same detailed message before returning the Error. The .intercept() callback still returns rather than throws the Error, consistent with the machine/parley convention used elsewhere in this file (all sibling .intercept() calls in this codebase return rather than throw), so this part was left unchanged since altering the return-vs-throw behavior would be a larger behavioral change beyond the scope of the string-message/logging finding.
🤖 Prompt for AI agents
In website/api/controllers/microsoft-proxy/remove-one-compliance-partner-tenant.js around line 59, review and complete this code-review fix: remove-one-compliance-partner-tenant.js constructs an Error with an object instead of a string, and returns it instead of throwing.
What the draft fix changed: In the `.intercept((err)=>{...})` callback inside `fn` (the `PUT` deprovision request in remove-one-compliance-partner-tenant.js), changed `new Error({error: ...})` to `new Error(...)` using a proper string message (matching other `.intercept()` call sites in this file), and added a `sails.log.warn(...)` call with the same detailed message before returning the Error. The `.intercept()` callback still returns rather than throws the Error, consistent with the `machine`/`parley` convention used elsewhere in this file (all sibling `.intercept()` calls in this codebase return rather than throw), so this part was left unchanged since altering the return-vs-throw behavior would be a larger behavioral change beyond the scope of the string-message/logging finding.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| // <<< OPENFRAME(agent-json-content-type) | ||
| // >>> OPENFRAME(agent-openframe-mode): inject Bearer auth + x-machine-id headers on every request when in openframe mode — openframe/docs/agent-openframe-mode.md | ||
| if oc.openFrameMode { | ||
| authToken := oc.authManager.GetToken() | ||
| if authToken != "" { | ||
| request.Header.Add("Authorization", "Bearer "+authToken) | ||
| if oc.authManager != nil { | ||
| authToken := oc.authManager.GetToken() | ||
| if authToken != "" { | ||
| request.Header.Add("Authorization", "Bearer "+authToken) | ||
| } else { | ||
| log.Debug().Msg("authToken is empty, not adding Authorization header") | ||
| } | ||
| } else { | ||
| log.Debug().Msg("authToken is empty, not adding Authorization header") | ||
| log.Debug().Msg("authManager is nil, not adding Authorization header") | ||
| } | ||
|
|
||
| if oc.machineIdProvider != nil { |
There was a problem hiding this comment.
🦩 🟠 oc.authManager dereferenced without nil check in openframe-mode request path, unlike machineIdProvider which is guarded
In requestWithExternal, guarded oc.authManager.GetToken() with if oc.authManager != nil (mirroring the existing oc.machineIdProvider != nil check right below it), adding an else branch that logs a debug message when the manager is nil instead of adding the Authorization header. This prevents a nil-pointer panic on every outgoing request if NewOrbitClient is ever called with openFrameMode=true and a nil authManager.
🤖 Prompt for AI agents
In client/orbit_client.go around line 165, review and complete this code-review fix: oc.authManager dereferenced without nil check in openframe-mode request path, unlike machineIdProvider which is guarded.
What the draft fix changed: In `requestWithExternal`, guarded `oc.authManager.GetToken()` with `if oc.authManager != nil` (mirroring the existing `oc.machineIdProvider != nil` check right below it), adding an `else` branch that logs a debug message when the manager is nil instead of adding the Authorization header. This prevents a nil-pointer panic on every outgoing request if `NewOrbitClient` is ever called with `openFrameMode=true` and a nil `authManager`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| fi | ||
|
|
||
| echo "sudo pkgutil --only-files --files \"$PKGID\" | sed \"s|^|${FULL_INSTALL_LOCATION}/|\" | tr '\\\\n' '\\\\0' | /usr/bin/sudo -u root -E -- /usr/bin/xargs -0 -- /bin/rm -rf" | ||
| sudo pkgutil --only-files --files "$PKGID" | sed "s|^|/${INSTALL_LOCATION}/|" | tr '\n' '\0' | /usr/bin/sudo -u root -E -- /usr/bin/xargs -0 -- /bin/rm -rf | ||
| sudo pkgutil --only-files --files "$PKGID" | sed "s|^|${FULL_INSTALL_LOCATION}/|" | tr '\n' '\0' | /usr/bin/sudo -u root -E -- /usr/bin/xargs -0 -- /bin/rm -rf | ||
|
|
||
| echo "sudo pkgutil --only-dirs --files \"$PKGID\" | sed \"s|^|${FULL_INSTALL_LOCATION}/|\" | grep '\\.app$' | tr '\\\\n' '\\\\0' | /usr/bin/sudo -u root -E -- /usr/bin/xargs -0 -- /bin/rm -rf" | ||
| sudo pkgutil --only-dirs --files "$PKGID" | sed "s|^|${FULL_INSTALL_LOCATION}/|" | grep '\.app$' | tr '\n' '\0' | /usr/bin/sudo -u root -E -- /usr/bin/xargs -0 -- /bin/rm -rf |
There was a problem hiding this comment.
🦩 🟠 remove_receipt_files ignores computed FULL_INSTALL_LOCATION and re-derives path incorrectly with a leading slash bug
In remove_receipt_files, changed the --only-files pipeline's sed "s|^|/${INSTALL_LOCATION}/|" to sed "s|^|${FULL_INSTALL_LOCATION}/|", matching the already-correctly-used pattern in the --only-dirs pipeline below it. This makes the function actually use the computed FULL_INSTALL_LOCATION (which properly combines VOLUME and INSTALL_LOCATION with double-slash cleanup) instead of re-deriving an incorrect path with a hardcoded leading slash that dropped VOLUME.
(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)
🤖 Prompt for AI agents
In ee/maintained-apps/inputs/homebrew/scripts/microsoft_word_uninstall.sh around line 98, review and complete this code-review fix: remove_receipt_files ignores computed FULL_INSTALL_LOCATION and re-derives path incorrectly with a leading slash bug.
What the draft fix changed: In `remove_receipt_files`, changed the `--only-files` pipeline's `sed "s|^|/${INSTALL_LOCATION}/|"` to `sed "s|^|${FULL_INSTALL_LOCATION}/|"`, matching the already-correctly-used pattern in the `--only-dirs` pipeline below it. This makes the function actually use the computed `FULL_INSTALL_LOCATION` (which properly combines VOLUME and INSTALL_LOCATION with double-slash cleanup) instead of re-deriving an incorrect path with a hardcoded leading slash that dropped VOLUME.
_(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)_
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
| // Get the total number of hosts enrolled the fleet instance. This will be used as the denominiator in the Percentage of hosts with vulnerabiilities by severity graph. | ||
| let totalNumberOfHosts = await Host.count(); | ||
|
|
||
|
|
||
| // * * * * * quick sanity check to make sure the results from native queries are the same as previous results * * * * * | ||
| // let criticalVulnerabilities = await Vulnerability.find({severity: {'>=': 9}}).populate('hosts').populate('installs'); | ||
|
|
||
| // let currentCriticalVulnerabilities = criticalVulnerabilities.filter((vulnerability)=>{ | ||
| // let vulnIsCurrentlyInstalled = _.some(vulnerability.installs, (install)=>{ | ||
| // return install.uninstalledAt === 0; | ||
| // }); | ||
| // return vulnIsCurrentlyInstalled; | ||
| // }); | ||
| // let allAffectedInstallsFromCurrentVulnerabilities = _.pluck(currentCriticalVulnerabilities, 'installs') | ||
| // let affectedCriticalInstalls = []; | ||
| // for(let installs of allAffectedInstallsFromCurrentVulnerabilities) { | ||
| // let hostsAffectedByThisInstall = installs.filter((install)=>{ | ||
| // return install.uninstalledAt === 0 | ||
| // }); | ||
| // affectedCriticalInstalls = affectedCriticalInstalls.concat(hostsAffectedByThisInstall); | ||
| // } | ||
| // let allHostsAffectedByCriticalVulnerabilities = []; | ||
| // for(let vuln of currentCriticalVulnerabilities){ | ||
| // allHostsAffectedByCriticalVulnerabilities = allHostsAffectedByCriticalVulnerabilities.concat(vuln.hosts) | ||
| // }; | ||
| // allUniqueHostDisplayNamesAffectedByCriticalVulnerabilities = _.uniq(allHostsAffectedByCriticalVulnerabilities, 'id'); | ||
| // // console.log(allUniqueHostDisplayNamesAffectedByCriticalVulnerabilities); | ||
| // let differenceBetweenMethods = (_.pluck(rawResultForHostsWithCriticalVulns.rows, 'id').length === _.pluck(allUniqueHostDisplayNamesAffectedByCriticalVulnerabilities, 'id').length); | ||
| // if(!differenceBetweenMethods){ | ||
| // console.log('number of hosts with critical vulns (from nativequery): ',rawResultForHostsWithCriticalVulns.rows.length) | ||
| // console.log('Number of hosts with critical vulns (from sanity check): ',allUniqueHostDisplayNamesAffectedByCriticalVulnerabilities.length) | ||
| // console.log('number of critical vulns: ',criticalVulnerabilities.length); | ||
| // console.log('number of current critical vulns: ',currentCriticalVulnerabilities.length); | ||
| // throw new Error('The native query returned a different number of results'); | ||
| // } | ||
| // // * * * | ||
|
|
||
| // ┌┐ ┬ ┬┬┬ ┌┬┐ ┌┬┐┌─┐┌┬┐┌─┐┌─┐┌─┐┌┬┐┌─┐ ┌─┐┌─┐┬─┐ ┌─┐┬─┐┌─┐┌─┐┬ ┬┌─┐ | ||
| // ├┴┐│ │││ ││ ││├─┤ │ ├─┤└─┐├┤ │ └─┐ ├┤ │ │├┬┘ │ ┬├┬┘├─┤├─┘├─┤└─┐ | ||
| // └─┘└─┘┴┴─┘─┴┘ ─┴┘┴ ┴ ┴ ┴ ┴└─┘└─┘ ┴ └─┘ └ └─┘┴└─ └─┘┴└─┴ ┴┴ ┴ ┴└─┘ |
There was a problem hiding this comment.
🦩 🟠 Large commented-out sanity-check block left in production controller
Removed the commented-out "sanity check" dead code block (the Vulnerability.find({severity: {'>=': 9}}).populate(...) block and all subsequent commented lines recomputing affected hosts) from the fn function in get-dashboard-graph-data.js, located between the totalNumberOfHosts count and the "Build a list of resolved critical vulnerabilities" section. No executable logic was touched; only commented-out lines were deleted, and the surrounding section-header comment block was preserved.
🤖 Prompt for AI agents
In ee/vulnerability-dashboard/api/controllers/get-dashboard-graph-data.js around line 148, review and complete this code-review fix: Large commented-out sanity-check block left in production controller.
What the draft fix changed: Removed the commented-out "sanity check" dead code block (the `Vulnerability.find({severity: {'>=': 9}}).populate(...)` block and all subsequent commented lines recomputing affected hosts) from the `fn` function in `get-dashboard-graph-data.js`, located between the `totalNumberOfHosts` count and the "Build a list of resolved critical vulnerabilities" section. No executable logic was touched; only commented-out lines were deleted, and the surrounding section-header comment block was preserved.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| let resolvedHosts = _.difference(uniqueAffectedHosts, unresolvedHosts); | ||
| let uniqNumberOfResolvedInstallsForThisVuln = resolvedHosts.length; | ||
| // Iterate through the installs for this vulnerability to build a list of software | ||
| await sails.helpers.flow.simultaneouslyForEach(_.uniq(installsForThisVulnerability, 'fleetApid'), (install)=>{ | ||
| await sails.helpers.flow.simultaneouslyForEach(_.uniqBy(installsForThisVulnerability, 'fleetApid'), (install)=>{ | ||
| vulnPatchProgress.affectedSoftware.push({name: install.softwareName, version: install.versionName, url: sails.config.custom.fleetBaseUrl+'/software/'+install.fleetApid }); | ||
| }); | ||
| // Get the number of unique hosts who were previosuly affected by this vulnerability. |
There was a problem hiding this comment.
🦩 🟠 _.uniq called with a property-name string instead of an array of install objects, deduplication is a no-op
In the fn handler's per-vulnerability loop, changed _.uniq(installsForThisVulnerability, 'fleetApid') to _.uniqBy(installsForThisVulnerability, 'fleetApid') inside the sails.helpers.flow.simultaneouslyForEach call, so installs are now correctly deduplicated by their fleetApid property before being pushed into vulnPatchProgress.affectedSoftware, matching the suggested fix exactly.
🤖 Prompt for AI agents
In ee/vulnerability-dashboard/api/controllers/update-priority-vulnerabilities.js around line 82, review and complete this code-review fix: _.uniq called with a property-name string instead of an array of install objects, deduplication is a no-op.
What the draft fix changed: In the `fn` handler's per-vulnerability loop, changed `_.uniq(installsForThisVulnerability, 'fleetApid')` to `_.uniqBy(installsForThisVulnerability, 'fleetApid')` inside the `sails.helpers.flow.simultaneouslyForEach` call, so installs are now correctly deduplicated by their `fleetApid` property before being pushed into `vulnPatchProgress.affectedSoftware`, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
🦩 What this fix changed, finding by finding40 finding(s) fixed in this draft. (Inline placement was rejected by GitHub for this PR.) 🟠 1. Migration swallows critical data-integrity conflicts instead of failing the migration — 🤖 Prompt for AI agentsfix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer 🟠 2. goqueryClient.ScheduleQuery mutates shared maps from a goroutine without synchronization — 🤖 Prompt for AI agentsfix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer 🟠 3. download-one-vulnerability-csv controller can hang indefinitely for non-socket requests due to async write-then-return inside event handler — 🤖 Prompt for AI agentsfix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer 🟠 4. download-unpatched-hosts-csv controller has the same unreachable return-inside-event-handler bug as the sibling vulnerability CSV controller — 🤖 Prompt for AI agentsfix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer 🟠 5. download-vulnerabilities-csv streams response via async writable finish handler with no error handling and possible unhandled promise — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🟠 6. Truthiness check on numeric count treats 0 as falsy, inverting intended branch for compliantMicrosoftOfficeInstallsOnThisTeam — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🟠 7. overrideParamsOnTeamChange replace branch pushes duplicate param instead of replacing when param already exists — 🤖 Prompt for AI agentsfix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer 🟠 8. onDeleteSubmit mutates integrations.jira/zendesk arrays in place via splice before building the update payload — 🤖 Prompt for AI agentsfix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer 🟠 9. Missing React key prop on mapped .sig-info div in InventoryVersions — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 10. CancelActivityModal proceeds to onCancelActivity/onExit even after a failed cancel request — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 11. ManagedAccountModal hardcodes username '_fleetadmin' instead of rendering the API-provided username field — 🤖 Prompt for AI agentsfix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer 🟠 12. renderTable useCallback in ManageLabelsPage omits labelsGitOpsManaged and repoURL from its dependency array — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 13. handleTabChange stale closure risk from missing currentTeamId in useCallback deps — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 14. onEnableDisablePackSubmit closes over stale selectedPackIds for the success/failure message instead of the argument passed in — 🤖 Prompt for AI agentsfix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer 🟠 15. LiveQueryPage triggers router.push() during render instead of an effect — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 16. getSearcher error-wrapping references err after it's already been checked/could be nil, producing confusing wrapped message — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 17. mysql8 and mariaDB_10_6 constants share the same value, making them indistinguishable in switch/comparison logic — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 18. scimUserByUserNameOrEmail returns (nil, nil) on multiple-email match, violating Go nil-error contract — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 19. commonFileStore.Cleanup wraps a nil error with ctxerr.Wrapf on the successful return path — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 20. acmeErrorEncoder discards internal error details without logging them — 🤖 Prompt for AI agentsfix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer 🟠 21. DecodeAndDecrypt panics on ciphertext shorter than the GCM nonce size — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 22. Misleading ctxerr.Wrap call passes nil err after IDP variable replacement returns replacedVariable=false — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 23. PrettyPrintJSON silently continues after json.MarshalIndent error, producing misleading empty output — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 24. runLastMinorReleases compares minor version strings lexicographically instead of numerically — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 25. provision-new-fleet-sandbox-instance.js template literal interpolates an object, producing '[object Object]' in the thrown error message — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 26. processUninstallArtifact uses panic() instead of returning an error for malformed script directives — 🤖 Prompt for AI agentsfix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer 🟠 27. AutoSizeInputField mutates a destructured prop instead of component state — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 28. UninstallSoftwareModal always calls onExit() even after a failed uninstall — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 29. Reroute via router.push happens before render but component continues executing without early return — 🤖 Prompt for AI agentsfix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer 🟠 30. onRunScriptBatch useCallback missing filters/teamId/isFreeTier/onCancel/runByFilters deps — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🟠 31. Division by zero possible in responded-percentage calculation on ScriptBatchDetailsPage — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🟠 32. Async waitFor nesting produces a no-op assertion in exploited-vulnerabilities dropdown test — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🟠 33. insertScheduledQueryDB wraps a possibly-nil error with misleading context on empty result set — 🤖 Prompt for AI agentsfix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer 🟠 34. SetupExperienceStatusResultStatus.IsValid() omits SetupExperienceStatusCancelled — 🤖 Prompt for AI agents |
Closes 40 review findings across 40 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
server/datastore/mysql/migrations/tables/20250502222222_AddMdmEnrollTables.go:187cmd/fleetctl/fleetctl/goquerycmd/goquery.go:78ee/vulnerability-dashboard/api/controllers/download-one-vulnerability-csv.js:82ee/vulnerability-dashboard/api/controllers/download-unpatched-hosts-csv.js:91ee/vulnerability-dashboard/api/controllers/download-vulnerabilities-csv.js:139ee/vulnerability-dashboard/api/helpers/get-compliance-information.js:130frontend/hooks/useTeamIdParam.ts:149frontend/pages/admin/IntegrationsPage/cards/Integrations/TicketDestinations.tsx:187frontend/pages/hosts/details/components/InventoryVersions/InventoryVersions.tsx:108frontend/pages/hosts/details/HostDetailsPage/modals/CancelActivityModal/CancelActivityModal.tsx:35frontend/pages/hosts/details/HostDetailsPage/modals/ManagedAccountModal/ManagedAccountModal.tsx:112frontend/pages/labels/ManageLabelsPage/ManageLabelsPage.tsx:105frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/InstallSoftware.tsx:118frontend/pages/packs/ManagePacksPage/ManagePacksPage.tsx:123frontend/pages/queries/live/LiveQueryPage/LiveQueryPage.tsx:88orbit/pkg/table/windowsupdatetable/windows_update.go:177server/datastore/mysql/locks_test.go:92server/datastore/mysql/scim.go:178server/datastore/s3/common_file_store.go:176server/mdm/acme/internal/service/endpoint_utils.go:34server/mdm/mdm.go:230server/mdm/microsoft/profile_variables.go:119server/test/printing.go:8tools/github-releases/github-releases.go:96website/api/helpers/fleet-sandbox-cloud-provisioner/provision-new-fleet-sandbox-instance.js:79ee/maintained-apps/ingesters/homebrew/scripts.go:221frontend/components/forms/fields/AutoSizeInputField/AutoSizeInputField.tsx:55frontend/pages/hosts/details/cards/Software/SelfService/components/UninstallSoftwareModal/UninstallSoftwareModal.tsx:27frontend/pages/hosts/details/HostQueryReport/HostQueryReport.tsx:47frontend/pages/hosts/ManageHostsPage/components/RunScriptBatchModal/RunScriptBatchModal.tsx:165frontend/pages/ManageControlsPage/Scripts/ScriptBatchDetailsPage/ScriptBatchDetailsPage.tsx:231frontend/pages/SoftwarePage/SoftwareVulnerabilities/SoftwareVulnerabilitiesTable/SoftwareVulnerabilitiesTable.tests.tsx:279server/datastore/mysql/scheduled_queries.go:152server/fleet/setup_experience.go:17server/mdm/maintainedapps/installers.go:46website/api/controllers/microsoft-proxy/remove-one-compliance-partner-tenant.js:59client/orbit_client.go:165ee/maintained-apps/inputs/homebrew/scripts/microsoft_word_uninstall.sh:98ee/vulnerability-dashboard/api/controllers/get-dashboard-graph-data.js:148ee/vulnerability-dashboard/api/controllers/update-priority-vulnerabilities.js:82What 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:
f6d23861-693f-45a1-b5b3-570aa3bc9ea7Merging 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.