Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
18dd7cc
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
fdf04c6
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
3abd25e
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
56e8828
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
fd1b0ed
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
e2854c8
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
6cf2522
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
da9c980
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
9e87702
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
05efcc5
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
92bad79
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
f50f0f5
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
a0f7b9f
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
e267939
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
a9b0f14
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
8774c33
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
1f30b5c
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
a71c6ef
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
da6b25f
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
386ef02
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
f68b0a5
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
d1437cc
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
382e429
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
55a4a6d
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
76a22bc
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
609d4bd
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
55b33dd
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
5d9a0ac
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
9cd73c1
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
7cb64a8
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
bbef622
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
eb6e61a
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
86f78c4
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
4f6536c
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
d4122a7
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
a3dfd57
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
32f7aab
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
4101fef
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
3197acf
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
319688b
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
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
7 changes: 6 additions & 1 deletion cmd/osquery-perf/osquery_perf/stats.go
Original file line number Diff line number Diff line change
Expand Up @@ -273,10 +273,15 @@ func (s *Stats) Log() {
s.l.Lock()
defer s.l.Unlock()

var errorRate float64
if s.osqueryEnrollments > 0 {
errorRate = float64(s.errors) / float64(s.osqueryEnrollments)
}

log.Printf(

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.

🦩 🟠 Stats.Log() computes error rate with a divide-by-zero risk before any enrollments occur

In Stats.Log() (cmd/osquery-perf/osquery_perf/stats.go), replaced the unconditional float64(s.errors)/float64(s.osqueryEnrollments) division with a guarded computation: a local errorRate variable is declared and only computed as the division when s.osqueryEnrollments > 0, otherwise it stays at its zero value (0.0). The log.Printf call now passes errorRate instead of the raw division expression, eliminating the NaN/+Inf output before any enrollments occur.

πŸ€– Prompt for AI agents
In cmd/osquery-perf/osquery_perf/stats.go around line 276, review and complete this code-review fix: Stats.Log() computes error rate with a divide-by-zero risk before any enrollments occur.
What the draft fix changed: In Stats.Log() (cmd/osquery-perf/osquery_perf/stats.go), replaced the unconditional `float64(s.errors)/float64(s.osqueryEnrollments)` division with a guarded computation: a local `errorRate` variable is declared and only computed as the division when `s.osqueryEnrollments > 0`, otherwise it stays at its zero value (0.0). The `log.Printf` call now passes `errorRate` instead of the raw division expression, eliminating the NaN/+Inf output before any enrollments occur.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟒 97 high β€” react πŸ‘/πŸ‘Ž to teach the reviewer

"uptime: %s, error rate: %.2f, osquery enrolls: %d, orbit enrolls: %d, mdm enrolls: %d, distributed/reads: %d, distributed/writes: %d, config requests: %d, result log requests: %d, mdm sessions initiated: %d, mdm on-demand syncs: %d, mdm commands received: %d, config errors: %d, distributed/read errors: %d, distributed/write errors: %d, log result errors: %d, orbit errors: %d, desktop errors: %d, mdm errors: %d, mdm scep requests: %d, mdm scep success: %d, mdm scep errors: %d, ddm tokens success: %d, ddm tokens errors: %d, ddm declaration items success: %d, ddm declaration items errors: %d, ddm activation success: %d, ddm activation errors: %d, ddm configuration success: %d, ddm configuration errors: %d, ddm status success: %d, ddm status errors: %d, buffered logs: %d, script execs (errs): %d (%d), software installs (errs): %d (%d)",
time.Since(s.StartTime).Round(time.Second),
float64(s.errors)/float64(s.osqueryEnrollments),
errorRate,
s.osqueryEnrollments,
s.orbitEnrollments,
s.mdmEnrollments,
Expand Down
14 changes: 5 additions & 9 deletions ee/cis/macos-14/test/scripts/CIS_2.6.2.sh
Original file line number Diff line number Diff line change
@@ -1,21 +1,17 @@
#!/bin/bash

sudo /usr/bin/defaults write /Library/Application\

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.

🦩 🟠 CIS_2.6.2.sh path continuation via unescaped line break inside a string literal is fragile and likely broken

Replaced all four fragile mid-path backslash-newline continuations (/Library/Application\ followed by a bare newline then Support/...) in ee/cis/macos-14/test/scripts/CIS_2.6.2.sh with single-line commands using a quoted path "/Library/Application Support/CrashReporter/DiagnosticMessagesHistory.plist", for the defaults write ... AutoSubmit, defaults write ... ThirdPartyDataSubmit, chmod 644, and chgrp admin invocations. This removes the reliance on backslash-newline continuation entirely and matches the suggested fix; the unrelated <username> example block below was left untouched since it was not part of this finding.

πŸ€– Prompt for AI agents
In ee/cis/macos-14/test/scripts/CIS_2.6.2.sh around line 3, review and complete this code-review fix: CIS_2.6.2.sh path continuation via unescaped line break inside a string literal is fragile and likely broken.
What the draft fix changed: Replaced all four fragile mid-path backslash-newline continuations (`/Library/Application\` followed by a bare newline then `Support/...`) in `ee/cis/macos-14/test/scripts/CIS_2.6.2.sh` with single-line commands using a quoted path `"/Library/Application Support/CrashReporter/DiagnosticMessagesHistory.plist"`, for the `defaults write ... AutoSubmit`, `defaults write ... ThirdPartyDataSubmit`, `chmod 644`, and `chgrp admin` invocations. This removes the reliance on backslash-newline continuation entirely and matches the suggested fix; the unrelated `<username>` example block below was left untouched since it was not part of this finding.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟒 95 high β€” react πŸ‘/πŸ‘Ž to teach the reviewer

Support/CrashReporter/DiagnosticMessagesHistory.plist AutoSubmit -bool false
sudo /usr/bin/defaults write "/Library/Application Support/CrashReporter/DiagnosticMessagesHistory.plist" AutoSubmit -bool false

sudo /usr/bin/defaults write /Library/Application\
Support/CrashReporter/DiagnosticMessagesHistory.plist ThirdPartyDataSubmit -bool false
sudo /usr/bin/defaults write "/Library/Application Support/CrashReporter/DiagnosticMessagesHistory.plist" ThirdPartyDataSubmit -bool false

sudo /bin/chmod 644 /Library/Application\
Support/CrashReporter/DiagnosticMessagesHistory.plist
sudo /bin/chmod 644 "/Library/Application Support/CrashReporter/DiagnosticMessagesHistory.plist"

sudo /usr/sbin/chgrp admin /Library/Application\
Support/CrashReporter/DiagnosticMessagesHistory.plist
sudo /usr/sbin/chgrp admin "/Library/Application Support/CrashReporter/DiagnosticMessagesHistory.plist"


echo "This needs modification"
sudo -u <username> /usr/bin/defaults write
/Users/<username>/Library/Preferences/com.apple.assistant.support "Siri DataSharing Opt-In Status" -int 2

# Example:
# sudo -u sharonkatz /usr/bin/defaults write /Users/sharonkatz/Library/Preferences/com.apple.assistant.support "Siri Data Sharing Opt-In Status" -int 2
# sudo -u sharonkatz /usr/bin/defaults write /Users/sharonkatz/Library/Preferences/com.apple.assistant.support "Siri Data Sharing Opt-In Status" -int 2
4 changes: 0 additions & 4 deletions ee/fleetd-chrome/src/tables/os_version.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,6 @@ export default class TableOSVersion extends Table {
column: "codename",
error_message: err.message.toString(),
});
warningsArray.push({

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.

🦩 🟠 os_version.ts silently drops columns declared but never populated (platform_like warning pushed but not part of returned row shape mismatch)

Removed the misleading platform_like warning push in the catch block of generate() (previously located right after the codename warning push). Since platform_like is always hardcoded to "chrome" in the returned row and never depends on the getHighEntropyValues call, pushing a warning for that column when the call fails was incorrect/misleading. The column remains declared in columns and continues to be populated unconditionally with "chrome", so no column is now silently dropped and no spurious warning is emitted for a value that never actually failed to be derived.

πŸ€– Prompt for AI agents
In ee/fleetd-chrome/src/tables/os_version.ts around line 91, review and complete this code-review fix: os_version.ts silently drops columns declared but never populated (`platform_like` warning pushed but not part of returned row shape mismatch).
What the draft fix changed: Removed the misleading `platform_like` warning push in the `catch` block of `generate()` (previously located right after the `codename` warning push). Since `platform_like` is always hardcoded to `"chrome"` in the returned row and never depends on the `getHighEntropyValues` call, pushing a warning for that column when the call fails was incorrect/misleading. The column remains declared in `columns` and continues to be populated unconditionally with `"chrome"`, so no column is now silently dropped and no spurious warning is emitted for a value that never actually failed to be derived.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 85 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

column: "platform_like",
error_message: err.message.toString(),
});
}

let arch;
Expand Down
13 changes: 8 additions & 5 deletions ee/maintained-apps/ingesters/winget/external_refs/main.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package externalrefs

import (
"fmt"
"log"

maintained_apps "github.com/fleetdm/fleet/v4/ee/maintained-apps"
)
Expand All @@ -14,15 +14,18 @@ var Funcs = map[string][]func(*maintained_apps.FMAManifestApp) (*maintained_apps

// EnrichManifest applies all registered enrichment functions for the given app.
// Enrichers are looked up by app.Slug and run sequentially.
// Errors are logged but do not stop the enrichment pipeline.
// Errors are logged and stop the enrichment pipeline for that app, preserving
// the last known-good app value.
func EnrichManifest(app *maintained_apps.FMAManifestApp) {

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.

🦩 🟠 Enrichment errors are silently logged with fmt.Printf and swallowed, discarding partial-state app pointer

Changed EnrichManifest in ee/maintained-apps/ingesters/winget/external_refs/main.go so the enricher's return value is assigned to a local enriched variable first; on error, the pre-error app pointer is preserved and the loop breaks immediately via break, preventing subsequent enrichers from running on a nil/partial app and preventing a later nil-pointer panic on app.UniqueIdentifier. Also replaced fmt.Printf with log.Printf (removing the now-unused fmt import) since no structured logger was visible in this file to match; this is a minimal improvement over ad-hoc stdout printing but may not match a project-specific structured logging convention used elsewhere that I cannot see from this file alone.

πŸ€– Prompt for AI agents
In ee/maintained-apps/ingesters/winget/external_refs/main.go around line 18, review and complete this code-review fix: Enrichment errors are silently logged with fmt.Printf and swallowed, discarding partial-state app pointer.
What the draft fix changed: Changed `EnrichManifest` in ee/maintained-apps/ingesters/winget/external_refs/main.go so the enricher's return value is assigned to a local `enriched` variable first; on error, the pre-error `app` pointer is preserved and the loop breaks immediately via `break`, preventing subsequent enrichers from running on a nil/partial `app` and preventing a later nil-pointer panic on `app.UniqueIdentifier`. Also replaced `fmt.Printf` with `log.Printf` (removing the now-unused `fmt` import) since no structured logger was visible in this file to match; this is a minimal improvement over ad-hoc stdout printing but may not match a project-specific structured logging convention used elsewhere that I cannot see from this file alone.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 85 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

if enrichers, ok := Funcs[app.Slug]; ok {
for _, enricher := range enrichers {
var err error
app, err = enricher(app)
enriched, err := enricher(app)
if err != nil {
fmt.Printf("Error enriching app %s: %v\n", app.UniqueIdentifier, err)
log.Printf("Error enriching app %s: %v\n", app.UniqueIdentifier, err)
break
}
app = enriched
}
}
}

21 changes: 13 additions & 8 deletions ee/maintained-apps/inputs/homebrew/scripts/cleanmymac-uninstall.sh
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,18 @@ trash() {
fi

local trash="/Users/$logged_in_user/.Trash"
local file_name="$(basename "${target_file}")"

if [[ -e "$target_file" ]]; then
echo "removing $target_file."
mv -f "$target_file" "$trash/${file_name}_${timestamp}_${rand}"
else
echo "$target_file doesn't exist."
fi
local file_name

# Glob-expand target_file (compgen preserves spaces in the path; [[ -e "$x" ]] does not expand *)
while IFS= read -r expanded_file; do
if [[ -e "$expanded_file" ]]; then
file_name="$(basename "${expanded_file}")"
echo "removing $expanded_file."
mv -f "$expanded_file" "$trash/${file_name}_${timestamp}_${rand}"
else
echo "$expanded_file doesn't exist."
fi
done < <(compgen -G "$target_file")
}

remove_launchctl_service 'com.macpaw.CleanMyMac5.HealthMonitor'
Comment on lines 100 to 117

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.

🦩 🟠 cleanmymac-uninstall.sh trash() lacks the glob-expansion fix present in the sibling camtasia_uninstall.sh trash()

Changed the trash() function in cleanmymac-uninstall.sh to loop over compgen -G "$target_file" glob expansion (matching the sibling camtasia_uninstall.sh fix), replacing the direct [[ -e "$target_file" ]] check on the un-expanded string. This ensures wildcard patterns are correctly expanded even with spaces in the path, per the same comment used in the sibling script. Risk: not executed/tested against actual filesystem paths in this environment; behavior for non-glob literal paths (the majority of call sites here) should be unchanged since compgen -G on a literal path returns that path itself, but this assumption is based on the sibling implementation pattern and standard bash glob semantics, not a live test run.

πŸ€– Prompt for AI agents
In ee/maintained-apps/inputs/homebrew/scripts/cleanmymac-uninstall.sh around line 91, review and complete this code-review fix: cleanmymac-uninstall.sh trash() lacks the glob-expansion fix present in the sibling camtasia_uninstall.sh trash().
What the draft fix changed: Changed the `trash()` function in `cleanmymac-uninstall.sh` to loop over `compgen -G "$target_file"` glob expansion (matching the sibling `camtasia_uninstall.sh` fix), replacing the direct `[[ -e "$target_file" ]]` check on the un-expanded string. This ensures wildcard patterns are correctly expanded even with spaces in the path, per the same comment used in the sibling script. Risk: not executed/tested against actual filesystem paths in this environment; behavior for non-glob literal paths (the majority of call sites here) should be unchanged since `compgen -G` on a literal path returns that path itself, but this assumption is based on the sibling implementation pattern and standard bash glob semantics, not a live test run.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 75 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

Expand All @@ -130,3 +134,4 @@ trash $LOGGED_IN_USER '~/Library/LaunchAgents/com.macpaw.CleanMyMac5.Updater.pli
trash $LOGGED_IN_USER '~/Library/Logs/com.macpaw.CleanMyMac5'
trash $LOGGED_IN_USER '~/Library/Preferences/com.macpaw.CleanMyMac5.plist'
trash $LOGGED_IN_USER '~/Library/Saved Application State/com.macpaw.CleanMyMac5.savedState'

Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ set -euo pipefail

# variables
APPDIR="/Applications/"

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.

🦩 🟠 docker_desktop_install.sh: TMPDIR path derived from realpath of INSTALLER_PATH is not necessarily writable/removable, and Docker.app copy leaves TMPDIR unmanaged

Changed TMPDIR=$(dirname "$(realpath "$INSTALLER_PATH")") to TMPDIR=$(mktemp -d) in the variable declarations at the top of the file, matching the pattern used by expressvpn-install.sh in the same directory, so the script now uses a scoped, writable temp directory instead of the installer's own (possibly shared/unwritable) directory. Also extended the existing cleanup() trap function to rm -rf "$TMPDIR" on exit, ensuring the Docker.app copy and Docker.app.bkp backup deposited into TMPDIR during install are removed afterward instead of left behind permanently. This addresses both parts of the finding (non-writable TMPDIR and unmanaged cleanup) with minimal change to control flow; the rest of the script's use of TMPDIR (cp/mv operations) is unchanged since mktemp -d produces a directory with the same usage semantics.

πŸ€– Prompt for AI agents
In ee/maintained-apps/inputs/homebrew/scripts/docker_desktop_install.sh around line 6, review and complete this code-review fix: docker_desktop_install.sh: TMPDIR path derived from realpath of INSTALLER_PATH is not necessarily writable/removable, and Docker.app copy leaves TMPDIR unmanaged.
What the draft fix changed: Changed `TMPDIR=$(dirname "$(realpath "$INSTALLER_PATH")")` to `TMPDIR=$(mktemp -d)` in the variable declarations at the top of the file, matching the pattern used by expressvpn-install.sh in the same directory, so the script now uses a scoped, writable temp directory instead of the installer's own (possibly shared/unwritable) directory. Also extended the existing `cleanup()` trap function to `rm -rf "$TMPDIR"` on exit, ensuring the Docker.app copy and Docker.app.bkp backup deposited into TMPDIR during install are removed afterward instead of left behind permanently. This addresses both parts of the finding (non-writable TMPDIR and unmanaged cleanup) with minimal change to control flow; the rest of the script's use of TMPDIR (cp/mv operations) is unchanged since mktemp -d produces a directory with the same usage semantics.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 75 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

TMPDIR=$(dirname "$(realpath "$INSTALLER_PATH")")
TMPDIR=$(mktemp -d)
MOUNT_POINT=""

cleanup() {
Expand All @@ -15,6 +15,9 @@ cleanup() {
fi
rmdir "$mp" >/dev/null 2>&1 || true
fi
if [[ -n "${TMPDIR:-}" ]]; then
rm -rf "$TMPDIR" >/dev/null 2>&1 || true
fi
}
trap cleanup EXIT

Expand Down Expand Up @@ -142,3 +145,4 @@ mkdir -p /usr/local/bin
/bin/ln -h -f -s -- "$APPDIR/Docker.app/Contents/Resources/bin/docker-credential-desktop" "/usr/local/bin/docker-credential-desktop"
/bin/ln -h -f -s -- "$APPDIR/Docker.app/Contents/Resources/bin/docker-credential-ecr-login" "/usr/local/bin/docker-credential-ecr-login"
/bin/ln -h -f -s -- "$APPDIR/Docker.app/Contents/Resources/bin/docker-credential-osxkeychain" "/usr/local/bin/docker-credential-osxkeychain"

45 changes: 29 additions & 16 deletions ee/vulnerability-dashboard/api/helpers/get-vulnerabilities.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,28 +139,36 @@ module.exports = {
} else {// If we're filtering by a specific team and not getting the results for a single vulnerability, we'll build a list of vulnerabilities that affect that team.

let hostIdsToFind = _.pluck(await Host.find({teamApid: teamApid}).select(['id']), 'id');

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.

🦩 🟠 Unparameterized SQL query building with raw string concatenation of host IDs in vulnerability report helper

In fn, replaced raw string concatenation of hostIdsToFind into firstNativeQueryToSend with parameterized placeholders ($1,$2,... for postgres, ?,?,... for mysql) passed via a bindings array to sails.sendNativeQuery(sql, bindings). Also replaced the vulnerabilityInstallNativeQuery/vulnerabilityInstallWhereClause construction (previously using _.pluck(vulnerabilities,'id').join(',')) with placeholders built from vulnerabilityIdsToFind, passed as bindings to the second sails.sendNativeQuery call. Risk: I could not verify the exact call signature Waterline/Sails sendNativeQuery expects for this project's adapter version (some versions expect sendNativeQuery(sql, valuesArray), others expect a driver-specific bindings format, and MySQL's mysql/mysql2 driver uses ? placeholders positionally while node-postgres uses $n), so this needs to be verified against the actual sails-postgresql/sails-mysql adapter versions in use before merging; if the signature differs, the query will silently fail to substitute bindings correctly.

πŸ€– Prompt for AI agents
In ee/vulnerability-dashboard/api/helpers/get-vulnerabilities.js around line 141, review and complete this code-review fix: Unparameterized SQL query building with raw string concatenation of host IDs in vulnerability report helper.
What the draft fix changed: In `fn`, replaced raw string concatenation of `hostIdsToFind` into `firstNativeQueryToSend` with parameterized placeholders (`$1,$2,...` for postgres, `?,?,...` for mysql) passed via a `bindings` array to `sails.sendNativeQuery(sql, bindings)`. Also replaced the `vulnerabilityInstallNativeQuery`/`vulnerabilityInstallWhereClause` construction (previously using `_.pluck(vulnerabilities,'id').join(',')`) with placeholders built from `vulnerabilityIdsToFind`, passed as bindings to the second `sails.sendNativeQuery` call. Risk: I could not verify the exact call signature Waterline/Sails `sendNativeQuery` expects for this project's adapter version (some versions expect `sendNativeQuery(sql, valuesArray)`, others expect a driver-specific bindings format, and MySQL's `mysql`/`mysql2` driver uses `?` placeholders positionally while node-postgres uses `$n`), so this needs to be verified against the actual sails-postgresql/sails-mysql adapter versions in use before merging; if the signature differs, the query will silently fail to substitute bindings correctly.
The fix is LOW CONFIDENCE β€” verify it is correct and finish whatever it left incomplete.

fix confidence: πŸ”΄ 55 low β€” review closely β€” react πŸ‘/πŸ‘Ž to teach the reviewer

// TODO use parameterization
let firstNativeQueryToSend;
let hostIdsPlaceholders = hostIdsToFind.map((id, i)=>{
return sails.config.datastores.default.adapter === 'sails-postgresql' ? `$${i + 1}` : '?';
}).join(',');

if(sails.config.datastores.default.adapter === 'sails-postgresql') {
firstNativeQueryToSend = `
firstNativeQueryToSend = {
sql: `
SELECT * FROM "vulnerability"
WHERE "id" IN (
SELECT "vulnerability" FROM "vulnerabilityinstall"
WHERE "host" IN (${hostIdsToFind.join(',')})
);`;
WHERE "host" IN (${hostIdsPlaceholders})
);`,
bindings: hostIdsToFind
};
} else if(sails.config.datastores.default.adapter === 'sails-mysql') {
firstNativeQueryToSend = 'SELECT * FROM `vulnerability` '+
'WHERE `id` IN ('+
'SELECT `vulnerability` FROM `vulnerabilityinstall` '+
'WHERE `host` IN ('+
hostIdsToFind.join(',')+
')'+
');';
firstNativeQueryToSend = {
sql: 'SELECT * FROM `vulnerability` '+
'WHERE `id` IN ('+
'SELECT `vulnerability` FROM `vulnerabilityinstall` '+
'WHERE `host` IN ('+
hostIdsPlaceholders+
')'+
');',
bindings: hostIdsToFind
};
}


let rawResultFromDatabase = await sails.sendNativeQuery(firstNativeQueryToSend);
let rawResultFromDatabase = await sails.sendNativeQuery(firstNativeQueryToSend.sql, firstNativeQueryToSend.bindings);

for(let row of rawResultFromDatabase.rows){
row.createdAt = Number(row.createdAt);
Expand Down Expand Up @@ -200,17 +208,21 @@ module.exports = {

// Build a where clause for the native query we will be sending.
let vulnerabilityInstallWhereClause;
let vulnerabilityIdsToFind = _.pluck(vulnerabilities, 'id');
let vulnerabilityIdsPlaceholders = vulnerabilityIdsToFind.map((id, i)=>{
return sails.config.datastores.default.adapter === 'sails-postgresql' ? `$${i + 1}` : '?';
}).join(',');

// If we're including resolved install information in this report for a CSV export, we'll send a query without the uninstalledAt condition.
if(includeResolvedInstalls){
vulnerabilityInstallWhereClause = `WHERE vulnerability IN (${_.pluck(vulnerabilities,'id').join(',')})`;// Note this where clause will work with Postgres and MySQL datastores.
vulnerabilityInstallWhereClause = `WHERE vulnerability IN (${vulnerabilityIdsPlaceholders})`;// Note this where clause will work with Postgres and MySQL datastores.
} else { // Otherwise, we'll send queries to only get unresolved installs.

if(sails.config.datastores.default.adapter === 'sails-postgresql') {
// If this app is configured to use a Postgres datastore, we'll need to put double quotes around the uninstalledAt column name.
vulnerabilityInstallWhereClause = `WHERE "uninstalledAt" = 0 AND vulnerability IN (${_.pluck(vulnerabilities,'id').join(',')})`;
vulnerabilityInstallWhereClause = `WHERE "uninstalledAt" = 0 AND vulnerability IN (${vulnerabilityIdsPlaceholders})`;
} else if(sails.config.datastores.default.adapter === 'sails-mysql') {
vulnerabilityInstallWhereClause = `WHERE uninstalledAt = 0 AND vulnerability IN (${_.pluck(vulnerabilities,'id').join(',')})`;
vulnerabilityInstallWhereClause = `WHERE uninstalledAt = 0 AND vulnerability IN (${vulnerabilityIdsPlaceholders})`;
}
}

Expand All @@ -219,7 +231,7 @@ module.exports = {
SELECT * FROM vulnerabilityinstall
${vulnerabilityInstallWhereClause}`;

let selectedInstallsFromEntireOrg = await sails.sendNativeQuery(vulnerabilityInstallNativeQuery);
let selectedInstallsFromEntireOrg = await sails.sendNativeQuery(vulnerabilityInstallNativeQuery, vulnerabilityIdsToFind);

let hostsFromEntireOrg = await Host.find();

Expand Down Expand Up @@ -305,3 +317,4 @@ module.exports = {

};


Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const baseClass = "software-script-details-modal";

export type IPackageInstallDetails = {

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.

🦩 🟠 IPackageInstallDetails.install_uuid marked optional but code assumes always present

In IPackageInstallDetails (top of file), changed install_uuid?: string; // not actually optional to install_uuid: string;, removing the misleading optional marker so the type now matches actual usage. In SoftwareScriptDetailsModal, removed the ?? "" fallback and the stale "will always be present" comment, using detailsFromProps.install_uuid directly as installUUID. Added enabled: !!installUUID to the useQuery options so the query doesn't fire with an empty/undefined UUID, and added an explicit guard at the top of renderContent that renders a DataError ("Couldn't get script details. Missing install UUID.") when installUUID is falsy, instead of silently querying the API with an empty string. This surfaces the contract violation as an error state rather than masking it. Risk: callers that pass details objects without install_uuid will now fail TypeScript compilation rather than silently defaulting; those call sites (not visible in this file) may need updating, so this should be verified against all usages of SoftwareScriptDetailsModal/IPackageInstallDetails elsewhere in the repo.

πŸ€– Prompt for AI agents
In frontend/components/ActivityDetails/InstallDetails/SoftwareScriptDetailsModal/SoftwareScriptDetailsModal.tsx around line 45, review and complete this code-review fix: IPackageInstallDetails.install_uuid marked optional but code assumes always present.
What the draft fix changed: In `IPackageInstallDetails` (top of file), changed `install_uuid?: string; // not actually optional` to `install_uuid: string;`, removing the misleading optional marker so the type now matches actual usage. In `SoftwareScriptDetailsModal`, removed the `?? ""` fallback and the stale "will always be present" comment, using `detailsFromProps.install_uuid` directly as `installUUID`. Added `enabled: !!installUUID` to the `useQuery` options so the query doesn't fire with an empty/undefined UUID, and added an explicit guard at the top of `renderContent` that renders a `DataError` ("Couldn't get script details. Missing install UUID.") when `installUUID` is falsy, instead of silently querying the API with an empty string. This surfaces the contract violation as an error state rather than masking it. Risk: callers that pass `details` objects without `install_uuid` will now fail TypeScript compilation rather than silently defaulting; those call sites (not visible in this file) may need updating, so this should be verified against all usages of `SoftwareScriptDetailsModal`/`IPackageInstallDetails` elsewhere in the repo.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 70 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

host_display_name?: string;
install_uuid?: string; // not actually optional
install_uuid: string;
};

export const renderContactOption = (url?: string) => (
Expand Down Expand Up @@ -201,8 +201,7 @@ export const SoftwareScriptDetailsModal = ({
onRerun,
contactUrl,
}: ISoftwareInstallDetailsProps) => {
// will always be present
const installUUID = detailsFromProps.install_uuid ?? "";
const installUUID = detailsFromProps.install_uuid;

const [showInstallDetails, setShowInstallDetails] = useState(false);
const toggleInstallDetails = () => {
Expand All @@ -223,6 +222,7 @@ export const SoftwareScriptDetailsModal = ({
{
...DEFAULT_USE_QUERY_OPTIONS,
staleTime: 3000,
enabled: !!installUUID,
select: (data) => data.results as ISoftwareScriptResult,
}
);
Expand Down Expand Up @@ -263,6 +263,15 @@ export const SoftwareScriptDetailsModal = ({
: undefined;

const renderContent = () => {
if (!installUUID) {
return (
<DataError
description="Couldn't get script details. Missing install UUID."
excludeIssueLink
/>
);
}

if (isLoading) {
return <Spinner />;
}
Expand Down
6 changes: 0 additions & 6 deletions frontend/hooks/useSoftwareInstallerMeta.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,6 @@ export const useSoftwareInstaller = (
0
);

const fmaVersions =
isFleetMaintainedApp && "fleet_maintained_versions" in softwareInstaller
? softwareInstaller.fleet_maintained_versions
: [];

const isCustomPackage =
installerType === "package" && !isFleetMaintainedApp;

Expand Down Expand Up @@ -141,7 +136,6 @@ export const useSoftwareInstaller = (
isAndroidPlayStoreWebApp,
isFleetMaintainedApp,
isLatestFmaVersion,
fmaVersions,
isCustomPackage,
isIosOrIpadosApp,
sha256,
Comment on lines 136 to 141

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.

🦩 🟠 isLatestFmaVersion computed but fmaVersions never used consistently / potential undefined destructure order bug

In useSoftwareInstaller (frontend/hooks/useSoftwareInstallerMeta.ts), removed the unused fmaVersions local variable computation and dropped it from the returned meta object, since it was never part of the SoftwareInstallerMeta interface and was dead code causing drift between the interface and the actual returned object. isLatestFmaVersion remains unchanged and is still computed and returned as before.

πŸ€– Prompt for AI agents
In frontend/hooks/useSoftwareInstallerMeta.ts around line 136, review and complete this code-review fix: isLatestFmaVersion computed but fmaVersions never used consistently / potential undefined destructure order bug.
What the draft fix changed: In `useSoftwareInstaller` (frontend/hooks/useSoftwareInstallerMeta.ts), removed the unused `fmaVersions` local variable computation and dropped it from the returned `meta` object, since it was never part of the `SoftwareInstallerMeta` interface and was dead code causing drift between the interface and the actual returned object. `isLatestFmaVersion` remains unchanged and is still computed and returned as before.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 85 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

Expand Down
5 changes: 3 additions & 2 deletions frontend/interfaces/mdm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,8 @@ export const isAndroidBYO = (enrollmentStatus: MdmEnrollmentStatus | null) => {
return enrollmentStatus === "On (personal)";
};

/** Android COBO (company-owned, fully managed) enrollment. */
/** Android COBO (company-owned, fully managed) enrollment. Shares the same
* current/legacy enrollment status semantics as isAutomaticDeviceEnrollment. */
export const isAndroidCOBO = (enrollmentStatus: MdmEnrollmentStatus | null) => {
return enrollmentStatus === "On (automatic)";
return isAutomaticDeviceEnrollment(enrollmentStatus);
};
Comment on lines 338 to 345

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.

🦩 🟠 isAndroidCOBO duplicates the semantics of isAutomaticDeviceEnrollment without reusing it, risking drift

Changed isAndroidCOBO in frontend/interfaces/mdm.ts to call isAutomaticDeviceEnrollment(enrollmentStatus) instead of re-implementing the enrollmentStatus === "On (automatic)" check inline. This makes isAndroidCOBO reuse the shared predicate (which already covers both the legacy "On (automatic)" and renamed "On (company-owned)" values), eliminating the duplication/drift risk and fixing the misclassification of company-owned Android devices enrolled under the newer status string.

πŸ€– Prompt for AI agents
In frontend/interfaces/mdm.ts around line 336, review and complete this code-review fix: isAndroidCOBO duplicates the semantics of isAutomaticDeviceEnrollment without reusing it, risking drift.
What the draft fix changed: Changed `isAndroidCOBO` in frontend/interfaces/mdm.ts to call `isAutomaticDeviceEnrollment(enrollmentStatus)` instead of re-implementing the `enrollmentStatus === "On (automatic)"` check inline. This makes `isAndroidCOBO` reuse the shared predicate (which already covers both the legacy `"On (automatic)"` and renamed `"On (company-owned)"` values), eliminating the duplication/drift risk and fixing the misclassification of company-owned Android devices enrolled under the newer status string.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟒 95 high β€” react πŸ‘/πŸ‘Ž to teach the reviewer

Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const EmptyOS = (platform: PlatformValueOptions): JSX.Element => (
<EmptyState
className={`${baseClass}__os-empty-table`}
header={`No${
` ${PLATFORM_DISPLAY_NAMES[platform]}` || ""
PLATFORM_DISPLAY_NAMES[platform] ? ` ${PLATFORM_DISPLAY_NAMES[platform]}` : ""
} operating systems detected`}
info="This report is updated every hour to protect the performance of your
devices."
Comment on lines 19 to 25

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.

🦩 🟠 EmptyOS header text lacks a space before 'operating systems' due to bad template logic

Changed the header template literal inside EmptyOS (OSTable.tsx) to use a proper conditional expression (PLATFORM_DISPLAY_NAMES[platform] ? ${PLATFORM_DISPLAY_NAMES[platform]} : "") instead of the broken `${PLATFORM_DISPLAY_NAMES[platform]}` || "" pattern, so the leading space and platform name are only included when the lookup returns a truthy value, correctly falling back to an empty string otherwise.

πŸ€– Prompt for AI agents
In frontend/pages/DashboardPage/cards/OperatingSystems/OSTable.tsx around line 18, review and complete this code-review fix: EmptyOS header text lacks a space before 'operating systems' due to bad template logic.
What the draft fix changed: Changed the `header` template literal inside `EmptyOS` (OSTable.tsx) to use a proper conditional expression (`PLATFORM_DISPLAY_NAMES[platform] ? ` ${PLATFORM_DISPLAY_NAMES[platform]}` : ""`) instead of the broken `` `${PLATFORM_DISPLAY_NAMES[platform]}` || "" `` pattern, so the leading space and platform name are only included when the lookup returns a truthy value, correctly falling back to an empty string otherwise.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟒 92 high β€” react πŸ‘/πŸ‘Ž to teach the reviewer

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ const WelcomeHost = ({
if (p.response) {
return (
<Button
key={p.id}
variant="unstyled"
onClick={() => handlePolicyModal(p.id)}
>
Comment on lines 229 to 235

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.

🦩 🟠 WelcomeHost Button missing key prop and onClick handler capturing stale id via closure inside .map without React key

Added key={p.id} prop to the mapped Button element inside host.policies?.slice(0, 3).map((p) => {...}) in the WelcomeHost component's render output. This gives React a stable identity for each list item based on the policy's unique id, resolving the missing-key warning and preventing incorrect DOM/state reuse across re-renders when the policy list changes. The onClick={() => handlePolicyModal(p.id)} closure was left unchanged since it already correctly captures p.id per render and is not stale.

πŸ€– Prompt for AI agents
In frontend/pages/DashboardPage/cards/WelcomeHost/WelcomeHost.tsx around line 228, review and complete this code-review fix: WelcomeHost Button missing key prop and onClick handler capturing stale id via closure inside .map without React key.
What the draft fix changed: Added `key={p.id}` prop to the mapped `Button` element inside `host.policies?.slice(0, 3).map((p) => {...})` in the `WelcomeHost` component's render output. This gives React a stable identity for each list item based on the policy's unique `id`, resolving the missing-key warning and preventing incorrect DOM/state reuse across re-renders when the policy list changes. The `onClick={() => handlePolicyModal(p.id)}` closure was left unchanged since it already correctly captures `p.id` per render and is not stale.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟒 95 high β€” react πŸ‘/πŸ‘Ž to teach the reviewer

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { ITeamConfig } from "interfaces/team";
import { getErrorReason } from "interfaces/errors";

import { LEARN_MORE_ABOUT_BASE_LINK } from "utilities/constants";
import { API_NO_TEAM_ID } from "interfaces/team";

import diskEncryptionAPI from "services/entities/disk_encryption";
import teamsAPI, { ILoadTeamResponse } from "services/entities/teams";
Expand Down Expand Up @@ -93,7 +94,7 @@ const DiskEncryption = ({
{

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.

🦩 🟠 currentTeamId === 0 misused as sentinel for 'no team' throughout DiskEncryption and Passwords

Replaced all three hardcoded 0 sentinel checks for "no team" with the named constant API_NO_TEAM_ID, imported from interfaces/team (as done in Passwords.tsx): the enabled: currentTeamId !== 0 option in the useQuery call, the if (currentTeamId === 0) check in onUpdateDiskEncryption, and the if (currentTeamId === 0 && isLoadingTeam) check in the render body. This assumes API_NO_TEAM_ID is exported from interfaces/team (as referenced by the finding's description of Passwords.tsx); if it is instead exported from a different module (e.g. interfaces/team.ts under a different name or from pages/ManageControlsPage/... constants), the import path would need adjustment β€” I could not view Passwords.tsx or interfaces/team.ts directly to confirm the exact export location.

πŸ€– Prompt for AI agents
In frontend/pages/ManageControlsPage/OSSettings/cards/DiskEncryption/DiskEncryption.tsx around line 93, review and complete this code-review fix: currentTeamId === 0 misused as sentinel for 'no team' throughout DiskEncryption and Passwords.
What the draft fix changed: Replaced all three hardcoded `0` sentinel checks for "no team" with the named constant `API_NO_TEAM_ID`, imported from `interfaces/team` (as done in Passwords.tsx): the `enabled: currentTeamId !== 0` option in the `useQuery` call, the `if (currentTeamId === 0)` check in `onUpdateDiskEncryption`, and the `if (currentTeamId === 0 && isLoadingTeam)` check in the render body. This assumes `API_NO_TEAM_ID` is exported from `interfaces/team` (as referenced by the finding's description of Passwords.tsx); if it is instead exported from a different module (e.g. `interfaces/team.ts` under a different name or from `pages/ManageControlsPage/...` constants), the import path would need adjustment β€” I could not view Passwords.tsx or interfaces/team.ts directly to confirm the exact export location.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 70 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

refetchOnWindowFocus: false,
retry: false,
enabled: currentTeamId !== 0,
enabled: currentTeamId !== API_NO_TEAM_ID,
select: (res) => res.fleet,
onSuccess: (res) => {
const enableDiskEncryption = res.mdm?.enable_disk_encryption ?? false;
Expand All @@ -119,7 +120,7 @@ const DiskEncryption = ({
);
onMutation();
setShowAggregate(diskEncryptionEnabled);
if (currentTeamId === 0) {
if (currentTeamId === API_NO_TEAM_ID) {
getUpdatedAppConfig();
}
} catch (e) {
Expand Down Expand Up @@ -148,7 +149,7 @@ const DiskEncryption = ({
}
};

if (currentTeamId === 0 && isLoadingTeam) {
if (currentTeamId === API_NO_TEAM_ID && isLoadingTeam) {
setIsLoadingTeam(false);
}

Expand Down
Loading