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
7885a78
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
643ce27
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
9de6d67
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
6b7093c
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
8204986
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
47a7977
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
1dcd43b
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
c193312
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
8f0fe5b
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
b9489b4
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
fbd958f
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
30ba620
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
3d75822
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
4af6f32
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
3e24899
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
2d92f05
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
8208c70
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
e53da01
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
68ab133
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
bd8417c
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
9dd0d13
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
04ba74d
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
b68403f
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
1973c42
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
4b28db0
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
8476300
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
c1726e0
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
fb5e40d
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
ac2d9da
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
d634122
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
e482d5e
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
8474427
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
74d34b6
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
30e71a2
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
6cd322b
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
b36579c
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
da1f4d9
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
4f7d21b
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
a09abc3
fix(adhoc-sweep-fixes): 40 review findings across 40 files
flamingo[bot] Sep 14, 2026
b3346c9
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
144 changes: 83 additions & 61 deletions cmd/maintained-apps/validate/darwin.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,87 @@ func checkVersionMatch(expectedVersion, foundVersion, foundBundledVersion string
return false
}

// appVersionMatcher is a strategy for determining whether a found app result
// satisfies the version requirement for a specific bundle identifier. Each
// matcher encapsulates one vendor-specific quirk so that new quirks can be
// added, tested, and reasoned about independently of appExists' main loop.
type appVersionMatcher func(ctx context.Context, logger *slog.Logger, appVersion string, result AppResult) bool

// appVersionMatchers maps bundle identifiers to their special-case version
// matching strategy. Bundle identifiers not present here fall back to the
// default checkVersionMatch behavior in appExists.
var appVersionMatchers = map[string]appVersionMatcher{
// OneDrive auto-updates immediately after installation, so the installed version
// might be newer than the installer version. For OneDrive, we only verify that
// the app exists rather than checking the version.
"com.microsoft.OneDrive": func(ctx context.Context, logger *slog.Logger, appVersion string, result AppResult) bool {
logger.InfoContext(ctx, "OneDrive detected - skipping version check due to auto-update behavior")
return true
},

// GPG Suite's installer version (e.g., "2023.3") doesn't match the app bundle version
// (e.g., "1.12" with bundled version "1800"). We only verify that the app exists
// rather than checking the version.
"org.gpgtools.gpgkeychain": func(ctx context.Context, logger *slog.Logger, appVersion string, result AppResult) bool {
logger.InfoContext(ctx, "GPG Suite detected - skipping version check due to version mismatch between installer and app bundle")
return true
},

// Adobe DNG Converter's version format includes build number in parentheses
// (e.g., "18.0 (2389)") which doesn't match the installer version (e.g., "18.0")
// Check if the version starts with the expected version to handle this case
"com.adobe.DNGConverter": func(ctx context.Context, logger *slog.Logger, appVersion string, result AppResult) bool {
if strings.HasPrefix(result.Version, appVersion+" ") || strings.HasPrefix(result.Version, appVersion+"(") {
logger.InfoContext(ctx, "Adobe DNG Converter detected - version matches with build number")
return true
}
return false
},

// Ableton Live's version format includes a build identifier in parentheses
// (e.g., "12.4.1 (2026-05-20_fbe5fe99c9)") which doesn't match the installer
// version (e.g., "12.4.1"). Check if the version starts with the expected
// version to handle this case.
"com.ableton.live": func(ctx context.Context, logger *slog.Logger, appVersion string, result AppResult) bool {
if strings.HasPrefix(result.Version, appVersion+" ") || strings.HasPrefix(result.Version, appVersion+"(") {
logger.InfoContext(ctx, "Ableton Live detected - version matches with build identifier")
return true
}
return false
},

// WhatsApp: Homebrew sometimes reports a newer version than what's actually available.
// If version doesn't match but app is installed, fall back to existence-only validation.
"net.whatsapp.WhatsApp": func(ctx context.Context, logger *slog.Logger, appVersion string, result AppResult) bool {
if !checkVersionMatch(appVersion, result.Version, result.BundledVersion) {
logger.InfoContext(ctx, "WhatsApp detected - version mismatch but app is installed, falling back to existence-only validation")
return true
}
return false
},

// Logi Tune: the installer URL always serves the latest release, while the Homebrew
// cask version lags behind (its livecheck scrapes a Logitech support article that is
// updated less often than the download). The installed version is therefore newer
// than the manifest version. If version doesn't match but app is installed, fall
// back to existence-only validation.
"com.logitech.logitune": func(ctx context.Context, logger *slog.Logger, appVersion string, result AppResult) bool {
if !checkVersionMatch(appVersion, result.Version, result.BundledVersion) {
logger.InfoContext(ctx, "Logi Tune detected - version mismatch but app is installed, falling back to existence-only validation")
return true
}
return false
},
}

// AppResult represents a single row returned by the osquery apps query in appExists.
type AppResult struct {
Name string `json:"name"`
Path string `json:"path"`
Version string `json:"bundle_short_version"`
BundledVersion string `json:"bundle_version"`
}

func appExists(ctx context.Context, logger *slog.Logger, appName, uniqueAppIdentifier, appVersion, appPath string) (bool, error) {
execTimeout, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()
Expand Down Expand Up @@ -211,18 +292,13 @@ func appExists(ctx context.Context, logger *slog.Logger, appName, uniqueAppIdent
return false, fmt.Errorf("executing osquery command: %w", err)
}

type AppResult struct {
Name string `json:"name"`
Path string `json:"path"`
Version string `json:"bundle_short_version"`
BundledVersion string `json:"bundle_version"`
}
var results []AppResult
if err := json.Unmarshal(output, &results); err != nil {
return false, fmt.Errorf("parsing osquery JSON output: %w", err)
}

if len(results) > 0 {
matcher := appVersionMatchers[uniqueAppIdentifier]
for _, result := range results {
software := &fleet.Software{
Name: result.Name,
Expand All @@ -236,64 +312,10 @@ func appExists(ctx context.Context, logger *slog.Logger, appName, uniqueAppIdent

logger.InfoContext(ctx, fmt.Sprintf("Found app: '%s' at %s, Version: %s, Bundled Version: %s", result.Name, result.Path, result.Version, result.BundledVersion))

// OneDrive auto-updates immediately after installation, so the installed version
// might be newer than the installer version. For OneDrive, we only verify that
// the app exists rather than checking the version.
if uniqueAppIdentifier == "com.microsoft.OneDrive" {

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.

🦩 🟠 Growing list of per-app special-cases hardcoded inside appExists violates single-responsibility and will keep growing unbounded

Extracted the five hardcoded bundle-identifier special cases (OneDrive, GPG Suite, Adobe DNG Converter, Ableton Live, WhatsApp, Logi Tune) out of the inline if uniqueAppIdentifier == "..." chain inside appExists's main loop into a new appVersionMatcher function type and an appVersionMatchers map (keyed by bundle identifier) defined at package scope. appExists now looks up appVersionMatchers[uniqueAppIdentifier] once before the loop and, per result, calls the matcher (if any) before falling back to the shared checkVersionMatch. The local AppResult struct was hoisted out of appExists to package scope so the matcher function signature could reference it. Each quirk is now a self-contained, independently referenceable/testable closure in the map rather than inlined control flow; behavior and log messages are preserved exactly. Risk: since matchers are unexported closures in a map, they can't be unit-tested individually from outside the package without also exporting the map or adding a test in this package β€” a complete resolution of the "independently testable" goal would additionally add table-driven tests against appVersionMatchers, which is out of scope for this single-file fix.

πŸ€– Prompt for AI agents
In cmd/maintained-apps/validate/darwin.go around line 242, review and complete this code-review fix: Growing list of per-app special-cases hardcoded inside appExists violates single-responsibility and will keep growing unbounded.
What the draft fix changed: Extracted the five hardcoded bundle-identifier special cases (OneDrive, GPG Suite, Adobe DNG Converter, Ableton Live, WhatsApp, Logi Tune) out of the inline `if uniqueAppIdentifier == "..."` chain inside `appExists`'s main loop into a new `appVersionMatcher` function type and an `appVersionMatchers` map (keyed by bundle identifier) defined at package scope. `appExists` now looks up `appVersionMatchers[uniqueAppIdentifier]` once before the loop and, per result, calls the matcher (if any) before falling back to the shared `checkVersionMatch`. The local `AppResult` struct was hoisted out of `appExists` to package scope so the matcher function signature could reference it. Each quirk is now a self-contained, independently referenceable/testable closure in the map rather than inlined control flow; behavior and log messages are preserved exactly. Risk: since matchers are unexported closures in a map, they can't be unit-tested individually from outside the package without also exporting the map or adding a test in this package β€” a complete resolution of the "independently testable" goal would additionally add table-driven tests against `appVersionMatchers`, which is out of scope for this single-file fix.
Verify the change is correct and complete; do not refactor unrelated code.

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

logger.InfoContext(ctx, "OneDrive detected - skipping version check due to auto-update behavior")
if matcher != nil && matcher(ctx, logger, appVersion, result) {
return true, nil
}

// GPG Suite's installer version (e.g., "2023.3") doesn't match the app bundle version
// (e.g., "1.12" with bundled version "1800"). We only verify that the app exists
// rather than checking the version.
if uniqueAppIdentifier == "org.gpgtools.gpgkeychain" {
logger.InfoContext(ctx, "GPG Suite detected - skipping version check due to version mismatch between installer and app bundle")
return true, nil
}

// Adobe DNG Converter's version format includes build number in parentheses
// (e.g., "18.0 (2389)") which doesn't match the installer version (e.g., "18.0")
// Check if the version starts with the expected version to handle this case
if uniqueAppIdentifier == "com.adobe.DNGConverter" {
if strings.HasPrefix(result.Version, appVersion+" ") || strings.HasPrefix(result.Version, appVersion+"(") {
logger.InfoContext(ctx, "Adobe DNG Converter detected - version matches with build number")
return true, nil
}
}

// Ableton Live's version format includes a build identifier in parentheses
// (e.g., "12.4.1 (2026-05-20_fbe5fe99c9)") which doesn't match the installer
// version (e.g., "12.4.1"). Check if the version starts with the expected
// version to handle this case.
if uniqueAppIdentifier == "com.ableton.live" {
if strings.HasPrefix(result.Version, appVersion+" ") || strings.HasPrefix(result.Version, appVersion+"(") {
logger.InfoContext(ctx, "Ableton Live detected - version matches with build identifier")
return true, nil
}
}

// WhatsApp: Homebrew sometimes reports a newer version than what's actually available.
// If version doesn't match but app is installed, fall back to existence-only validation.
if uniqueAppIdentifier == "net.whatsapp.WhatsApp" {
if !checkVersionMatch(appVersion, result.Version, result.BundledVersion) {
logger.InfoContext(ctx, "WhatsApp detected - version mismatch but app is installed, falling back to existence-only validation")
return true, nil
}
}

// Logi Tune: the installer URL always serves the latest release, while the Homebrew
// cask version lags behind (its livecheck scrapes a Logitech support article that is
// updated less often than the download). The installed version is therefore newer
// than the manifest version. If version doesn't match but app is installed, fall
// back to existence-only validation.
if uniqueAppIdentifier == "com.logitech.logitune" {
if !checkVersionMatch(appVersion, result.Version, result.BundledVersion) {
logger.InfoContext(ctx, "Logi Tune detected - version mismatch but app is installed, falling back to existence-only validation")
return true, nil
}
}

// Check various version matching strategies
if checkVersionMatch(appVersion, result.Version, result.BundledVersion) {
return true, nil
Expand Down
6 changes: 5 additions & 1 deletion cmd/osquery-perf/softwaredb/softwaredb.go
Original file line number Diff line number Diff line change
Expand Up @@ -369,14 +369,18 @@ func generateDatabaseFromSQL(dbPath, sqlPath string) error {
if err != nil {
return fmt.Errorf("creating database: %w", err)
}
defer db.Close()

// Execute the SQL file
if _, err := db.Exec(string(sqlContent)); err != nil {
db.Close() // Close the handle before removing the file so cleanup is reliable on all platforms (notably Windows)
os.Remove(dbPath) // Clean up partial database
return fmt.Errorf("executing SQL file: %w", err)
}

if err := db.Close(); err != nil {
return fmt.Errorf("closing database: %w", err)
}

log.Printf("βœ… Successfully created database from %s", sqlPath)
return nil
}
Comment on lines 369 to 386

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.

🦩 🟠 generateDatabaseFromSQL does not roll back the file on Exec failure inside a deferred-close db handle, risking partial state under concurrent access

In generateDatabaseFromSQL, removed the defer db.Close() and instead explicitly call db.Close() before os.Remove(dbPath) on the Exec failure path, ensuring the file handle is released before deletion is attempted (fixing unreliable cleanup on Windows). On the success path, db.Close() is now called explicitly (with its error checked) before returning nil, preserving the original close-on-success behavior without relying on a deferred close that could race with the removal on the error path.

πŸ€– Prompt for AI agents
In cmd/osquery-perf/softwaredb/softwaredb.go around line 358, review and complete this code-review fix: generateDatabaseFromSQL does not roll back the file on Exec failure inside a deferred-close db handle, risking partial state under concurrent access.
What the draft fix changed: In `generateDatabaseFromSQL`, removed the `defer db.Close()` and instead explicitly call `db.Close()` before `os.Remove(dbPath)` on the `Exec` failure path, ensuring the file handle is released before deletion is attempted (fixing unreliable cleanup on Windows). On the success path, `db.Close()` is now called explicitly (with its error checked) before returning nil, preserving the original close-on-success behavior without relying on a deferred close that could race with the removal on the error path.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down
48 changes: 42 additions & 6 deletions docker-compose-redis-cluster.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,18 @@ services:
cluster_network:
ipv4_address: 172.20.0.30
depends_on:

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.

🦩 🟠 redis-cluster-setup uses depends_on without health-check condition, allowing cluster-create to race container startup

In docker-compose-redis-cluster.yml, added a healthcheck (redis-cli ping on the service's own port) to each of the six redis-cluster-N services, and changed redis-cluster-setup's depends_on from a plain list to the long form with condition: service_healthy for each of the six dependencies, so redis-cli --cluster create waits for redis-server to actually accept connections before running. Unverified: exact healthcheck timing (interval/timeout/retries values) is a reasonable default but not tuned against actual CI boot times; compose v2 (version: "2") file format does support the long-form depends_on with condition, but confirm the docker-compose version in CI honors it (older v2.x behavior varies) β€” a complete fix might need bumping version if the CI's compose binary requires 2.1+/3.x semantics.

πŸ€– Prompt for AI agents
In docker-compose-redis-cluster.yml around line 10, review and complete this code-review fix: redis-cluster-setup uses depends_on without health-check condition, allowing cluster-create to race container startup.
What the draft fix changed: In `docker-compose-redis-cluster.yml`, added a `healthcheck` (redis-cli ping on the service's own port) to each of the six `redis-cluster-N` services, and changed `redis-cluster-setup`'s `depends_on` from a plain list to the long form with `condition: service_healthy` for each of the six dependencies, so `redis-cli --cluster create` waits for redis-server to actually accept connections before running. Unverified: exact healthcheck timing (interval/timeout/retries values) is a reasonable default but not tuned against actual CI boot times; compose v2 (`version: "2"`) file format does support the long-form `depends_on` with `condition`, but confirm the docker-compose version in CI honors it (older v2.x behavior varies) β€” a complete fix might need bumping `version` if the CI's compose binary requires 2.1+/3.x semantics.
Verify the change is correct and complete; do not refactor unrelated code.

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

- redis-cluster-1
- redis-cluster-2
- redis-cluster-3
- redis-cluster-4
- redis-cluster-5
- redis-cluster-6
redis-cluster-1:
condition: service_healthy
redis-cluster-2:
condition: service_healthy
redis-cluster-3:
condition: service_healthy
redis-cluster-4:
condition: service_healthy
redis-cluster-5:
condition: service_healthy
redis-cluster-6:
condition: service_healthy

redis-cluster-1:
image: ${FLEET_REDIS_IMAGE:-redis:6.2}
Expand All @@ -25,6 +31,11 @@ services:
networks:
cluster_network:
ipv4_address: 172.20.0.31
healthcheck:
test: ["CMD", "redis-cli", "-p", "7001", "ping"]
interval: 2s
timeout: 2s
retries: 15

redis-cluster-2:
image: ${FLEET_REDIS_IMAGE:-redis:6.2}
Expand All @@ -36,6 +47,11 @@ services:
networks:
cluster_network:
ipv4_address: 172.20.0.32
healthcheck:
test: ["CMD", "redis-cli", "-p", "7002", "ping"]
interval: 2s
timeout: 2s
retries: 15

redis-cluster-3:
image: ${FLEET_REDIS_IMAGE:-redis:6.2}
Expand All @@ -47,6 +63,11 @@ services:
networks:
cluster_network:
ipv4_address: 172.20.0.33
healthcheck:
test: ["CMD", "redis-cli", "-p", "7003", "ping"]
interval: 2s
timeout: 2s
retries: 15

redis-cluster-4:
image: ${FLEET_REDIS_IMAGE:-redis:6.2}
Expand All @@ -58,6 +79,11 @@ services:
networks:
cluster_network:
ipv4_address: 172.20.0.34
healthcheck:
test: ["CMD", "redis-cli", "-p", "7004", "ping"]
interval: 2s
timeout: 2s
retries: 15

redis-cluster-5:
image: ${FLEET_REDIS_IMAGE:-redis:6.2}
Expand All @@ -69,6 +95,11 @@ services:
networks:
cluster_network:
ipv4_address: 172.20.0.35
healthcheck:
test: ["CMD", "redis-cli", "-p", "7005", "ping"]
interval: 2s
timeout: 2s
retries: 15

redis-cluster-6:
image: ${FLEET_REDIS_IMAGE:-redis:6.2}
Expand All @@ -80,6 +111,11 @@ services:
networks:
cluster_network:
ipv4_address: 172.20.0.36
healthcheck:
test: ["CMD", "redis-cli", "-p", "7006", "ping"]
interval: 2s
timeout: 2s
retries: 15

networks:
cluster_network:
Expand Down
16 changes: 15 additions & 1 deletion ee/cis/macos-26/test/scripts/CIS_5.11_pass.sh
Original file line number Diff line number Diff line change
@@ -1,5 +1,19 @@
#!/bin/bash

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 5.11 pass script writes root-owned sudoers policy without validating syntax before install

In CIS_5.11_pass.sh, replaced the direct tee-into-/etc/sudoers.d/ write with a flow that first writes the Defaults log_allowed line to a temp file created via mktemp, validates it with sudo visudo -c -f "$TMPFILE" and aborts (removing the temp file, exit 1) on failure. On success it copies the temp file into place with sudo cp, removes the temp file, chmods it 0440 as before, then re-validates the installed file in place with visudo -c -f and removes it if that final check fails. This ensures no unvalidated sudoers content is ever active. Risk: assumes /usr/sbin/visudo exists at that path on macOS 26 (standard location) and that mktemp/cp/rm are available at the referenced paths; not executed in a live environment to confirm exact visudo output/exit codes on this OS version.

πŸ€– Prompt for AI agents
In ee/cis/macos-26/test/scripts/CIS_5.11_pass.sh around line 1, review and complete this code-review fix: CIS 5.11 pass script writes root-owned sudoers policy without validating syntax before install.
What the draft fix changed: In CIS_5.11_pass.sh, replaced the direct `tee`-into-/etc/sudoers.d/ write with a flow that first writes the `Defaults log_allowed` line to a temp file created via `mktemp`, validates it with `sudo visudo -c -f "$TMPFILE"` and aborts (removing the temp file, exit 1) on failure. On success it copies the temp file into place with `sudo cp`, removes the temp file, chmods it 0440 as before, then re-validates the installed file in place with `visudo -c -f` and removes it if that final check fails. This ensures no unvalidated sudoers content is ever active. Risk: assumes `/usr/sbin/visudo` exists at that path on macOS 26 (standard location) and that `mktemp`/`cp`/`rm` are available at the referenced paths; not executed in a live environment to confirm exact visudo output/exit codes on this OS version.
Verify the change is correct and complete; do not refactor unrelated code.

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

# CIS 5.11 - Ensure Logging Is Enabled for Sudo
# Adds Defaults log_allowed to a sudoers.d file.
echo 'Defaults log_allowed' | /usr/bin/sudo /usr/bin/tee /etc/sudoers.d/CIS_5_11_sudoconfiguration > /dev/null
TMPFILE=$(/usr/bin/mktemp)
echo 'Defaults log_allowed' > "$TMPFILE"
if ! /usr/bin/sudo /usr/sbin/visudo -c -f "$TMPFILE" > /dev/null; then
echo "Error: sudoers syntax validation failed for CIS_5_11_sudoconfiguration" >&2
/bin/rm -f "$TMPFILE"
exit 1
fi
/usr/bin/sudo /bin/cp "$TMPFILE" /etc/sudoers.d/CIS_5_11_sudoconfiguration
/bin/rm -f "$TMPFILE"
/usr/bin/sudo /bin/chmod 0440 /etc/sudoers.d/CIS_5_11_sudoconfiguration
if ! /usr/bin/sudo /usr/sbin/visudo -c -f /etc/sudoers.d/CIS_5_11_sudoconfiguration > /dev/null; then
echo "Error: installed sudoers file failed validation, removing" >&2
/usr/bin/sudo /bin/rm -f /etc/sudoers.d/CIS_5_11_sudoconfiguration
exit 1
fi

5 changes: 5 additions & 0 deletions ee/fleetd-chrome/src/background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ const enroll = async () => {
enroll_secret: FLEET_ENROLL_SECRET,
});

if (!enroll_secret) {
throw new Error("enroll_secret is empty, refusing to enroll");
}

let host_identifier = host_details.system_info.hardware_serial;
if (!host_identifier) {
host_identifier = host_details.system_info.uuid;
Comment on lines 104 to 113

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.

🦩 🟠 Chrome extension enroll secret and node key handling has no reenroll loop guard beyond a single retry flag but node_key returned empty check happens after request completes without validating enroll_secret is non-empty

In enroll() in ee/fleetd-chrome/src/background.ts, added a validation check immediately after destructuring enroll_secret from chrome.storage.managed.get(...) that throws an Error("enroll_secret is empty, refusing to enroll") when enroll_secret is falsy (empty string, undefined, etc.), preventing the network request to /api/v1/osquery/enroll from ever being made with an empty secret. This surfaces as a thrown error that propagates up through authenticatedRequest's reenroll call and main's mainLoop error handling (already logged via console.error(err)), so no infinite retry loop or silent enroll spam occurs.

πŸ€– Prompt for AI agents
In ee/fleetd-chrome/src/background.ts around line 95, review and complete this code-review fix: Chrome extension enroll secret and node key handling has no reenroll loop guard beyond a single retry flag but node_key returned empty check happens after request completes without validating enroll_secret is non-empty.
What the draft fix changed: In `enroll()` in ee/fleetd-chrome/src/background.ts, added a validation check immediately after destructuring `enroll_secret` from `chrome.storage.managed.get(...)` that throws an `Error("enroll_secret is empty, refusing to enroll")` when `enroll_secret` is falsy (empty string, undefined, etc.), preventing the network request to `/api/v1/osquery/enroll` from ever being made with an empty secret. This surfaces as a thrown error that propagates up through `authenticatedRequest`'s reenroll call and `main`'s `mainLoop` error handling (already logged via `console.error(err)`), so no infinite retry loop or silent enroll spam occurs.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down Expand Up @@ -297,3 +301,4 @@ chrome.alarms.onAlarm.addListener(async ({ name }) => {
console.error(`unknown alarm ${name}`);
}
});

4 changes: 2 additions & 2 deletions ee/fleetd-chrome/src/tables/system_info.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ export default class TableSystemInfo extends Table {
getComputerName(hostname: string, hwSerial: string): string {

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.

🦩 🟠 getComputerName truthy-length checks silently misbehave for whitespace-only strings

In getComputerName (TableSystemInfo, ee/fleetd-chrome/src/tables/system_info.ts), changed !!hostname?.length to !!hostname?.trim().length and !!hwSerial?.length to !!hwSerial?.trim().length, so whitespace-only strings are treated as empty and correctly fall through to the hardware serial number or default "Chromebook" prefix.

πŸ€– Prompt for AI agents
In ee/fleetd-chrome/src/tables/system_info.ts around line 17, review and complete this code-review fix: getComputerName truthy-length checks silently misbehave for whitespace-only strings.
What the draft fix changed: In `getComputerName` (TableSystemInfo, `ee/fleetd-chrome/src/tables/system_info.ts`), changed `!!hostname?.length` to `!!hostname?.trim().length` and `!!hwSerial?.length` to `!!hwSerial?.trim().length`, so whitespace-only strings are treated as empty and correctly fall through to the hardware serial number or default "Chromebook" prefix.
Verify the change is correct and complete; do not refactor unrelated code.

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

const prefix = "Chromebook";

if (!!hostname?.length) {
if (!!hostname?.trim().length) {
return hostname;
}

if (!!hwSerial?.length) {
if (!!hwSerial?.trim().length) {
return `${prefix} ${hwSerial}`;
}

Expand Down
9 changes: 7 additions & 2 deletions frontend/pages/ManageControlsPage/Scripts/helpers.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,12 @@ export const getWhen = (summary: IScriptBatchSummaryV2) => {
}
return (
<>
<Icon name="pending-outline" color="ui-fleet-black-50" size="small" />
Started{" "}
<Icon
name={canceled ? "close-filled" : "pending-outline"}
color="ui-fleet-black-50"
size="small"
/>
{canceled ? "Canceled" : "Started"}{" "}
<HumanTimeDiffWithFleetLaunchCutoff
timeString={started_at}
tooltipPosition="right"
Comment on lines 34 to 45

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.

🦩 🟠 getWhen 'started' case ignores canceled state and never distinguishes a canceled-while-running batch

In getWhen's started case (frontend/pages/ManageControlsPage/Scripts/helpers.tsx), the rendered icon and label now branch on canceled: shows "close-filled" icon and "Canceled" text when canceled is true, otherwise keeps the existing "pending-outline" icon and "Started" text, mirroring the pattern already used in the finished case. The started_at validation and timestamp display are unchanged. Risk: I did not change any type definitions or verify that canceled is a valid field on the started-status branch of IScriptBatchSummaryV2 (it's destructured from summary regardless of status in the original code, so this should type-check), and there may be a design preference for a distinct label like "Canceled while running" instead of reusing "Canceled" β€” a reviewer should confirm the desired copy.

πŸ€– Prompt for AI agents
In frontend/pages/ManageControlsPage/Scripts/helpers.tsx around line 27, review and complete this code-review fix: getWhen 'started' case ignores `canceled` state and never distinguishes a canceled-while-running batch.
What the draft fix changed: In `getWhen`'s `started` case (frontend/pages/ManageControlsPage/Scripts/helpers.tsx), the rendered icon and label now branch on `canceled`: shows "close-filled" icon and "Canceled" text when `canceled` is true, otherwise keeps the existing "pending-outline" icon and "Started" text, mirroring the pattern already used in the `finished` case. The `started_at` validation and timestamp display are unchanged. Risk: I did not change any type definitions or verify that `canceled` is a valid field on the `started`-status branch of `IScriptBatchSummaryV2` (it's destructured from `summary` regardless of status in the original code, so this should type-check), and there may be a design preference for a distinct label like "Canceled while running" instead of reusing "Canceled" β€” a reviewer should confirm the desired copy.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down Expand Up @@ -84,3 +88,4 @@ export const getWhen = (summary: IScriptBatchSummaryV2) => {
return null;
}
};

Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,8 @@ describe("Custom variables", () => {
);
const deleteVariableHandler = http.delete(

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.

🦩 🟠 deleteVariableHandler parses id via string.split('/').pop() which is fragile against query strings

In the deleteVariableHandler (MSW http.delete handler for baseUrl("/custom_variables/:id")), replaced the fragile request.url.split("/").pop() parsing with the matched route param: destructured params from the handler callback and used params.id as string instead of manually parsing the URL. This avoids breakage from trailing slashes or query strings, resolving the finding directly.

πŸ€– Prompt for AI agents
In frontend/pages/ManageControlsPage/Variables/Variables.tests.tsx around line 155, review and complete this code-review fix: deleteVariableHandler parses id via string.split('/').pop() which is fragile against query strings.
What the draft fix changed: In the `deleteVariableHandler` (MSW `http.delete` handler for `baseUrl("/custom_variables/:id")`), replaced the fragile `request.url.split("/").pop()` parsing with the matched route param: destructured `params` from the handler callback and used `params.id as string` instead of manually parsing the URL. This avoids breakage from trailing slashes or query strings, resolving the finding directly.
Verify the change is correct and complete; do not refactor unrelated code.

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

baseUrl("/custom_variables/:id"),
async ({ request }) => {
const id = request.url.split("/").pop();
async ({ params }) => {
const id = params.id as string;
if (!id) {
throw new Error("Variable ID not found in request URL");
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
#!/bin/bash

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.

🦩 🟠 MySQL CA certificate retrieval script has no verification of downloaded bundle integrity

Added set -euo pipefail after the shebang so the script aborts on any command failure, unset variable use, or pipeline error, and changed wget --quiet to wget --quiet --fail so a failed download (e.g., HTTP error status) causes wget to exit non-zero, which combined with set -e now halts the script immediately instead of proceeding to csplit an empty/partial bundle file. Checksum/signature verification of the bundle itself was not added since no trusted checksum source was provided in the finding or repo, but fail-fast behavior on download failure is now enforced.

πŸ€– Prompt for AI agents
In infrastructure/dogfood/terraform/aws-tf-module/templates/mysql_ca_tls_retrieval.sh.tpl around line 1, review and complete this code-review fix: MySQL CA certificate retrieval script has no verification of downloaded bundle integrity.
What the draft fix changed: Added `set -euo pipefail` after the shebang so the script aborts on any command failure, unset variable use, or pipeline error, and changed `wget --quiet` to `wget --quiet --fail` so a failed download (e.g., HTTP error status) causes wget to exit non-zero, which combined with `set -e` now halts the script immediately instead of proceeding to csplit an empty/partial bundle file. Checksum/signature verification of the bundle itself was not added since no trusted checksum source was provided in the finding or repo, but fail-fast behavior on download failure is now enforced.
Verify the change is correct and complete; do not refactor unrelated code.

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

set -euo pipefail
apk add coreutils openssl

wget --quiet https://truststore.pki.rds.amazonaws.com/${aws_region}/${aws_region}-bundle.pem -O ${aws_region}-bundle.dl.pem
wget --quiet --fail https://truststore.pki.rds.amazonaws.com/${aws_region}/${aws_region}-bundle.pem -O ${aws_region}-bundle.dl.pem
csplit -z -k -f cert. -b '%02d.pem' ${aws_region}-bundle.dl.pem '/-----BEGIN CERTIFICATE-----/' '{*}'

for filename in cert.*;
Expand All @@ -12,3 +13,4 @@ do
mv $${filename} ${container_path}/${aws_region}.pem
fi
done

Loading