Skip to content

fix(FLEETMDM-003): CU-86akj32d7 6 review findings across 5 files #177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

fix(FLEETMDM-003): CU-86akj32d7 6 review findings across 5 files #177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
99b177f
fix(FLEETMDM-003): 6 review findings across 5 files
flamingo[bot] Sep 14, 2026
1305122
fix(FLEETMDM-003): 6 review findings across 5 files
flamingo[bot] Sep 14, 2026
f142261
fix(FLEETMDM-003): 6 review findings across 5 files
flamingo[bot] Sep 14, 2026
31eac82
fix(FLEETMDM-003): 6 review findings across 5 files
flamingo[bot] Sep 14, 2026
22b9c5b
fix(FLEETMDM-003): 6 review findings across 5 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
2 changes: 1 addition & 1 deletion orbit/pkg/kdialog/kdialog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func execCmdWithOutput(timeout time.Duration, args ...string) ([]byte, int, erro

output, exitCode, err := execuser.RunWithOutput(kdialogProcessName, opts...)

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 execCmdWithOutput returns bare exitCode/err from RunWithOutput without wrapping

In execCmdWithOutput (orbit/pkg/kdialog/kdialog.go), wrapped the error returned from execuser.RunWithOutput with fmt.Errorf("run kdialog via execuser: %w", err) instead of returning it bare, matching the suggested fix exactly. fmt is already imported so no new imports needed.

🤖 Prompt for AI agents 
In orbit/pkg/kdialog/kdialog.go around line 96, review and complete this code-review fix: execCmdWithOutput returns bare exitCode/err from RunWithOutput without wrapping.
What the draft fix changed: In execCmdWithOutput (orbit/pkg/kdialog/kdialog.go), wrapped the error returned from execuser.RunWithOutput with fmt.Errorf("run kdialog via execuser: %w", err) instead of returning it bare, matching the suggested fix exactly. fmt is already imported so no new imports needed.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

if err != nil {
return nil, exitCode, err
return nil, exitCode, fmt.Errorf("run kdialog via execuser: %w", err)
}

return output, exitCode, nil
Expand Down
12 changes: 6 additions & 6 deletions tools/mdm/migration/kandji/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,25 +86,25 @@ func unenroll(serialNumber string) error {
client := fleethttp.NewClient()

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 unenroll() returns bare errors from http.NewRequest / client.Do without context wrapping

In unenroll() (tools/mdm/migration/kandji/main.go), wrapped every bare return err with fmt.Errorf and a descriptive message identifying which call failed: http.NewRequest for the GET device request ("creating get device request"), client.Do for the GET ("performing get device request"), io.ReadAll of the GET response ("reading get device response body"), json.Unmarshal of the device info ("unmarshalling get device response body"), http.NewRequest for the DELETE request ("creating delete device request"), and client.Do for the DELETE request ("performing delete device request"). All wraps use %w to preserve the original error for errors.Is/As.

🤖 Prompt for AI agents 
In tools/mdm/migration/kandji/main.go around line 86, review and complete this code-review fix: unenroll() returns bare errors from http.NewRequest / client.Do without context wrapping.
What the draft fix changed: In unenroll() (tools/mdm/migration/kandji/main.go), wrapped every bare `return err` with fmt.Errorf and a descriptive message identifying which call failed: http.NewRequest for the GET device request ("creating get device request"), client.Do for the GET ("performing get device request"), io.ReadAll of the GET response ("reading get device response body"), json.Unmarshal of the device info ("unmarshalling get device response body"), http.NewRequest for the DELETE request ("creating delete device request"), and client.Do for the DELETE request ("performing delete device request"). All wraps use %w to preserve the original error for errors.Is/As.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

req, err := http.NewRequest("GET", fmt.Sprintf("https://%s.api.kandji.io/api/v1/devices?serial_number=%s", *subdomainFlag, serialNumber), nil)
if err != nil {
return err
return fmt.Errorf("creating get device request: %w", err)
}
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", *apiTokenFlag))
req.Header.Set("Content-Type", "application/json")
resp, err := client.Do(req)
if err != nil {
return err
return fmt.Errorf("performing get device request: %w", err)
}
defer resp.Body.Close()
bodyText, err := io.ReadAll(resp.Body)
if err != nil {
return err
return fmt.Errorf("reading get device response body: %w", err)
}

var deviceInfo []struct {
DeviceID string `json:"device_id"`
}
if err = json.Unmarshal(bodyText, &deviceInfo); err != nil {
return err
return fmt.Errorf("unmarshalling get device response body: %w", err)
}
if len(deviceInfo) == 0 {
return fmt.Errorf("empty deviceInfo response, serial: %s", serialNumber)
Expand All @@ -114,12 +114,12 @@ func unenroll(serialNumber string) error {
// https://api-docs.kandji.io/#97deb582-d86c-444a-aa3b-3528b9a8478f
req, err = http.NewRequest("DELETE", fmt.Sprintf("https://%s.api.kandji.io/api/v1/devices/%s", *subdomainFlag, deviceInfo[0].DeviceID), nil)
if err != nil {
return err
return fmt.Errorf("creating delete device request: %w", err)
}
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", *apiTokenFlag))
resp, err = client.Do(req)
if err != nil {
return err
return fmt.Errorf("performing delete device request: %w", err)
}
fmt.Println("resp.StatusCode, serialNumber, device", resp.StatusCode, serialNumber, deviceInfo[0].DeviceID)

Expand Down
17 changes: 12 additions & 5 deletions tools/software/vulnerabilities/performance_test/tester/performance_tester.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,19 @@ import (

var (

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 Hardcoded default MySQL credentials in performance testing tools

The hardcoded mysqlAddr/mysqlUser/mysqlPass/mysqlDB var declarations were changed to call a new getEnvOrDefault(envVar, defaultValue string) helper (added in this same file), reading from PERF_TEST_MYSQL_ADDR, PERF_TEST_MYSQL_USER, PERF_TEST_MYSQL_PASS, and PERF_TEST_MYSQL_DB environment variables, falling back to the original hardcoded local-dev values ("localhost:3306", "fleet", "insecure", "fleet") when unset. This makes the credentials overridable without touching source, per the finding's "at minimum" recommendation. Flag-based overrides were not added to keep the change minimal; a more complete fix could also expose these as CLI flags. The companion file volume_vuln_seeder.go mentioned in the finding is not modified since it is a separate file outside the scope of this fix.

🤖 Prompt for AI agents 
In tools/software/vulnerabilities/performance_test/tester/performance_tester.go around line 20, review and complete this code-review fix: Hardcoded default MySQL credentials in performance testing tools.
What the draft fix changed: The hardcoded mysqlAddr/mysqlUser/mysqlPass/mysqlDB var declarations were changed to call a new getEnvOrDefault(envVar, defaultValue string) helper (added in this same file), reading from PERF_TEST_MYSQL_ADDR, PERF_TEST_MYSQL_USER, PERF_TEST_MYSQL_PASS, and PERF_TEST_MYSQL_DB environment variables, falling back to the original hardcoded local-dev values ("localhost:3306", "fleet", "insecure", "fleet") when unset. This makes the credentials overridable without touching source, per the finding's "at minimum" recommendation. Flag-based overrides were not added to keep the change minimal; a more complete fix could also expose these as CLI flags. The companion file volume_vuln_seeder.go mentioned in the finding is not modified since it is a separate file outside the scope of this fix.
Verify the change is correct and complete; do not refactor unrelated code.

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

// MySQL config
mysqlAddr = "localhost:3306"
mysqlUser = "fleet"
mysqlPass = "insecure"
mysqlDB = "fleet"
mysqlAddr = getEnvOrDefault("PERF_TEST_MYSQL_ADDR", "localhost:3306")
mysqlUser = getEnvOrDefault("PERF_TEST_MYSQL_USER", "fleet")
mysqlPass = getEnvOrDefault("PERF_TEST_MYSQL_PASS", "insecure")
mysqlDB = getEnvOrDefault("PERF_TEST_MYSQL_DB", "fleet")
)

func getEnvOrDefault(envVar, defaultValue string) string {
if v := os.Getenv(envVar); v != "" {
return v
}
return defaultValue
}

// TestFunction represents a datastore method to test
type TestFunction func(context.Context, *mysql.Datastore) error

Expand Down Expand Up @@ -221,7 +228,7 @@ func main() {
Database: mysqlDB,
}, clock.C)
if err != nil {
log.Fatal(err)
log.Fatalf("connect to mysql datastore: %v", err)
}
defer func() { _ = ds.Close() }()

Comment on lines 228 to 234

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 log.Fatal(err) swallows context in performance_tester.go connection setup

In main(), the log.Fatal(err) call after mysql.New(...) fails was changed to log.Fatalf("connect to mysql datastore: %v", err), wrapping the raw driver error with descriptive context as suggested by the finding.

🤖 Prompt for AI agents 
In tools/software/vulnerabilities/performance_test/tester/performance_tester.go around line 216, review and complete this code-review fix: log.Fatal(err) swallows context in performance_tester.go connection setup.
What the draft fix changed: In main(), the log.Fatal(err) call after mysql.New(...) fails was changed to log.Fatalf("connect to mysql datastore: %v", err), wrapping the raw driver error with descriptive context as suggested by the finding.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

Expand Down
6 changes: 3 additions & 3 deletions tools/tuf/download-artifacts/download-artifacts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,11 +265,11 @@ func downloadComponents(workflowName string, headBranch string, artifactNames ma
for {
workflow, _, err := gc.Actions.GetWorkflowByFileName(ctx, "fleetdm", "fleet", workflowName)

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 Bare error returns in downloadComponents workflow lookup calls

In downloadComponents (tools/tuf/download-artifacts/download-artifacts.go), wrapped the previously bare error returns from gc.Actions.GetWorkflowByFileName, gc.Actions.ListWorkflowRunsByID, and gc.Actions.ListWorkflowRunArtifacts with fmt.Errorf("...: %w", ..., err) calls that describe the operation and relevant identifier (workflow name or run ID), matching the suggested fix. fmt was already imported so no new imports were needed.

🤖 Prompt for AI agents 
In tools/tuf/download-artifacts/download-artifacts.go around line 266, review and complete this code-review fix: Bare error returns in downloadComponents workflow lookup calls.
What the draft fix changed: In `downloadComponents` (tools/tuf/download-artifacts/download-artifacts.go), wrapped the previously bare error returns from `gc.Actions.GetWorkflowByFileName`, `gc.Actions.ListWorkflowRunsByID`, and `gc.Actions.ListWorkflowRunArtifacts` with `fmt.Errorf("...: %w", ..., err)` calls that describe the operation and relevant identifier (workflow name or run ID), matching the suggested fix. `fmt` was already imported so no new imports were needed.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

if err != nil {
return err
return fmt.Errorf("get workflow %s: %w", workflowName, err)
}
workflowRuns, _, err := gc.Actions.ListWorkflowRunsByID(ctx, "fleetdm", "fleet", *workflow.ID, nil)
if err != nil {
return err
return fmt.Errorf("list workflow runs for %s: %w", workflowName, err)
}
for _, wr := range workflowRuns.WorkflowRuns {
if headBranch == *wr.HeadBranch {
Expand Down Expand Up @@ -306,7 +306,7 @@ func downloadComponents(workflowName string, headBranch string, artifactNames ma
for {
artifactList, _, err := gc.Actions.ListWorkflowRunArtifacts(ctx, "fleetdm", "fleet", *workflowRun.ID, nil)
if err != nil {
return err
return fmt.Errorf("list workflow run artifacts for run %d: %w", *workflowRun.ID, err)
}
urls = make(map[string]string)
for _, artifact := range artifactList.Artifacts {
Expand Down
1 change: 1 addition & 0 deletions website/api/controllers/android-proxy/modify-android-policies.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,4 @@ module.exports = {


};

Comment on lines 106 to +109

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 Duplicated Google Auth + intercept boilerplate across both Android proxy controllers should be extracted to a shared helper

The finding recommends extracting the duplicated GoogleAuth client construction, 429 interception, and error-wrapping logic (in the fn handler's sails.helpers.flow.build block) into a shared helper used by both modify-android-device.js and modify-android-policies.js. I did not perform this extraction: doing so safely would require creating a new shared helper module (e.g. sails.helpers.androidProxy.someHelper) and also modifying modify-android-device.js to use it, which is outside this file and risks behavioral drift given the two controllers already intercept different status codes (400/404 only in policies) and have different request bodies/return shapes. Extracting only partially, or inventing a helper signature/name not present in the repo, would violate the "never invent an identifier" and "every import must exist" rules more riskily than leaving the duplication in place. As a minimal, safe, in-file measure, no functional change was made to this file since a partial fix here without the corresponding shared module and device-controller update would leave the finding unresolved and could introduce an unused or half-wired import. A complete fix requires: (a) designing and adding a shared helper (e.g. api/helpers/android-proxy/call-android-management-api.js or similar) that both controllers call, (b) updating this file to use it, and (c) updating modify-android-device.js to use it too — all of which need sign-off on the shared helper's exact interface/name before it can be safely introduced.

🤖 Prompt for AI agents 
In website/api/controllers/android-proxy/modify-android-policies.js around line 67, review and complete this code-review fix: Duplicated Google Auth + intercept boilerplate across both Android proxy controllers should be extracted to a shared helper.
What the draft fix changed: The finding recommends extracting the duplicated GoogleAuth client construction, 429 interception, and error-wrapping logic (in the `fn` handler's `sails.helpers.flow.build` block) into a shared helper used by both `modify-android-device.js` and `modify-android-policies.js`. I did not perform this extraction: doing so safely would require creating a new shared helper module (e.g. `sails.helpers.androidProxy.someHelper`) and also modifying `modify-android-device.js` to use it, which is outside this file and risks behavioral drift given the two controllers already intercept different status codes (400/404 only in policies) and have different request bodies/return shapes. Extracting only partially, or inventing a helper signature/name not present in the repo, would violate the "never invent an identifier" and "every import must exist" rules more riskily than leaving the duplication in place. As a minimal, safe, in-file measure, no functional change was made to this file since a partial fix here without the corresponding shared module and device-controller update would leave the finding unresolved and could introduce an unused or half-wired import. A complete fix requires: (a) designing and adding a shared helper (e.g. `api/helpers/android-proxy/call-android-management-api.js` or similar) that both controllers call, (b) updating this file to use it, and (c) updating `modify-android-device.js` to use it too — all of which need sign-off on the shared helper's exact interface/name before it can be safely introduced.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 20 low — review closely — react 👍/👎 to teach the reviewer