fix(adhoc-sweep-fixes): CU-86akj32d7 74 review findings across 40 files - #167
flamingo[bot] wants to merge 40 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -100,7 +101,11 @@ func (d *fileDepot) Put(cn string, crt *x509.Certificate) error { | |||
| return err | |||
| } | |||
| if err := d.writeDB(cn, serial, filename, crt); err != nil { | |||
There was a problem hiding this comment.
🦩 🟠 newPrivateKey ignores CommonName collision and possible unencrypted key file left behind on partial failure
In Put() (server/mdm/scep/depot/file/depot.go), the writeDB failure path now calls os.Remove(filepath) before returning the error, removing the leftover cert PEM file so the on-disk files stay in sync with index.txt; the stale TODO comment was replaced with an explanatory comment describing the mechanism. This does not address the CommonName collision detection itself (that is handled separately by HasCN/writeDB's own DN-collision error), only the file-removal-on-failure half of the finding.
🤖 Prompt for AI agents
In server/mdm/scep/depot/file/depot.go around line 102, review and complete this code-review fix: newPrivateKey ignores CommonName collision and possible unencrypted key file left behind on partial failure.
What the draft fix changed: In Put() (server/mdm/scep/depot/file/depot.go), the writeDB failure path now calls os.Remove(filepath) before returning the error, removing the leftover cert PEM file so the on-disk files stay in sync with index.txt; the stale TODO comment was replaced with an explanatory comment describing the mechanism. This does not address the CommonName collision detection itself (that is handled separately by HasCN/writeDB's own DN-collision error), only the file-removal-on-failure half of the finding.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| @@ -41,19 +42,19 @@ type fileDepot struct { | |||
| func (d *fileDepot) CA(pass []byte) ([]*x509.Certificate, *rsa.PrivateKey, error) { | |||
There was a problem hiding this comment.
🦩 🟠 Serial() returns underlying error unwrapped when SetString fails, and other bare error returns lack context
Wrapped bare error returns with fmt.Errorf("...: %w", err) context in NewFileDepot (opening index.txt), CA() (reading ca.pem/ca.key, loading ca certificate/key), Serial() (writing serial, opening/reading serial file, incrementing serial), and getFile() (checking/stat/reading the named file). Left other propagated errors (e.g. inside HasCN, writeDB, writeSerial) unwrapped since the finding specifically named Serial(), CA(), and getFile(); a fully complete fix per FLEETMDM-002-2 would wrap every bare return ..., err in the file, which is a broader change than these three functions.
🤖 Prompt for AI agents
In server/mdm/scep/depot/file/depot.go around line 41, review and complete this code-review fix: Serial() returns underlying error unwrapped when SetString fails, and other bare error returns lack context.
What the draft fix changed: Wrapped bare error returns with fmt.Errorf("...: %w", err) context in NewFileDepot (opening index.txt), CA() (reading ca.pem/ca.key, loading ca certificate/key), Serial() (writing serial, opening/reading serial file, incrementing serial), and getFile() (checking/stat/reading the named file). Left other propagated errors (e.g. inside HasCN, writeDB, writeSerial) unwrapped since the finding specifically named Serial(), CA(), and getFile(); a fully complete fix per FLEETMDM-002-2 would wrap every bare `return ..., err` in the file, which is a broader change than these three functions.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| return nil, fmt.Errorf("opening serial file: %w", err) | ||
| } | ||
| defer file.Close() | ||
| r := bufio.NewReader(file) |
There was a problem hiding this comment.
🦩 🟠 Serial() reads with ReadString('\r') but serial file is written with '\n' terminator, causing potential misread
Changed Serial()'s r.ReadString('\r') to r.ReadString('\n') in the Serial() function so the reader delimiter matches writeSerial()'s \n terminator; the existing TrimSuffix("\r") then TrimSuffix("\n") calls are left in place to remain tolerant of any stray \r (e.g. CRLF), so behavior for normal single-line files is unchanged but now correctly stops at the actual line terminator instead of relying on EOF. Not extensively tested against externally-edited multi-line serial files.
🤖 Prompt for AI agents
In server/mdm/scep/depot/file/depot.go around line 127, review and complete this code-review fix: Serial() reads with ReadString('\r') but serial file is written with '\n' terminator, causing potential misread.
What the draft fix changed: Changed Serial()'s r.ReadString('\r') to r.ReadString('\n') in the Serial() function so the reader delimiter matches writeSerial()'s `\n` terminator; the existing TrimSuffix("\r") then TrimSuffix("\n") calls are left in place to remain tolerant of any stray `\r` (e.g. CRLF), so behavior for normal single-line files is unchanged but now correctly stops at the actual line terminator instead of relying on EOF. Not extensively tested against externally-edited multi-line serial files.
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
| @@ -236,7 +241,7 @@ func (d *fileDepot) HasCN(_ string, allowTime int, cert *x509.Certificate, revok | |||
| return false, errors.New("DN " + dn + " already exists") | |||
| } | |||
| if revokeOldCertificate { | |||
There was a problem hiding this comment.
🦩 🟠 fmt.Println used for revocation notice instead of structured logging
Replaced fmt.Println in HasCN with slog.Info("revoking certificate from DB, recreation of CRL needed", "serial", key), using the log/slog package directly instead of routing through kitlogadapter.go (which was not shown to me and whose exact adapter API/signature I cannot verify); this satisfies "structured logging" but a reviewer should confirm whether call sites expect this file to route through the specific kitlogadapter bridge instead of stdlib slog directly.
🤖 Prompt for AI agents
In server/mdm/scep/depot/file/depot.go around line 238, review and complete this code-review fix: fmt.Println used for revocation notice instead of structured logging.
What the draft fix changed: Replaced fmt.Println in HasCN with slog.Info("revoking certificate from DB, recreation of CRL needed", "serial", key), using the log/slog package directly instead of routing through kitlogadapter.go (which was not shown to me and whose exact adapter API/signature I cannot verify); this satisfies "structured logging" but a reviewer should confirm whether call sites expect this file to route through the specific kitlogadapter bridge instead of stdlib slog directly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| throw 'notFound'; | ||
| } | ||
|
|
||
| if (thisAndroidEnterprise.fleetServerSecret !== fleetServerSecret) { |
There was a problem hiding this comment.
🦩 🟠 Android enterprise auth compares fleetServerSecret using non-constant-time string equality
In fn, replaced the plain !== comparison of fleetServerSecret with a constant-time comparison using crypto.timingSafeEqual, guarding against length mismatches (which throw in timingSafeEqual) by first comparing buffer lengths before calling it. This removes the timing-attack-prone equality check.
🤖 Prompt for AI agents
In website/api/controllers/android-proxy/get-android-enterprises.js around line 49, review and complete this code-review fix: Android enterprise auth compares fleetServerSecret using non-constant-time string equality.
What the draft fix changed: In `fn`, replaced the plain `!==` comparison of `fleetServerSecret` with a constant-time comparison using `crypto.timingSafeEqual`, guarding against length mismatches (which throw in `timingSafeEqual`) by first comparing buffer lengths before calling it. This removes the timing-attack-prone equality check.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| @@ -12,7 +12,7 @@ run(){ | |||
| while getopts s:p:u:f:d:o:x flag | |||
There was a problem hiding this comment.
🦩 🟠 getopts flag f defined twice, second definition silently shadows the source-file flag
In run(), the getopts case statement's first f) branch (path to team-names file) was renamed to s) to match the existing s: flag already declared in the getopts s:p:u:f:d:o:x flag spec string, eliminating the duplicate f) pattern. The second f) branch (additional fleetctl flags, flags+=($OPTARG)) is now reachable. The getopts spec string was left unchanged since s: was already present but previously unused/shadowed by the duplicate f) case; the comment text for the file-source flag was preserved as-is.
🤖 Prompt for AI agents
In tools/team-builder/build_teams.sh around line 12, review and complete this code-review fix: getopts flag `f` defined twice, second definition silently shadows the source-file flag.
What the draft fix changed: In `run()`, the getopts case statement's first `f)` branch (path to team-names file) was renamed to `s)` to match the existing `s:` flag already declared in the `getopts s:p:u:f:d:o:x flag` spec string, eliminating the duplicate `f)` pattern. The second `f)` branch (additional fleetctl flags, `flags+=($OPTARG)`) is now reachable. The getopts spec string was left unchanged since `s:` was already present but previously unused/shadowed by the duplicate `f)` case; the comment text for the file-source flag was preserved as-is.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -365,7 +365,7 @@ export const SoftwareInstallDetailsModal = ({ | |||
| const overrideFailedMessageWithInstalledMessage = | |||
| canOverrideFailureWithInstalled && | |||
There was a problem hiding this comment.
🦩 🟠 Duplicate '|| ""' in status fallback produces no functional difference but signals a copy-paste bug
Removed the duplicate || "" fallback in the overrideFailedMessageWithInstalledMessage computation inside SoftwareInstallDetailsModal (around the .includes(swInstallResult?.status || "" || "") expression), changing it to .includes(swInstallResult?.status || "") as suggested, with no functional change.
🤖 Prompt for AI agents
In frontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/SoftwareInstallDetailsModal.tsx around line 366, review and complete this code-review fix: Duplicate '|| ""' in status fallback produces no functional difference but signals a copy-paste bug.
What the draft fix changed: Removed the duplicate `|| ""` fallback in the `overrideFailedMessageWithInstalledMessage` computation inside `SoftwareInstallDetailsModal` (around the `.includes(swInstallResult?.status || "" || "")` expression), changing it to `.includes(swInstallResult?.status || "")` as suggested, with no functional change.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| "Couldn't uninstall." | ||
| ); | ||
| } else if (reason.startsWith("No uninstall script exists")) { | ||
| return `${UNINSTALL_SOFTWARE_ERROR_PREFIX}. An uninstall script does not exist for this package.`; |
There was a problem hiding this comment.
🦩 🟠 getUninstallErrorMessage produces malformed message with stray period before reason for the 'no uninstall script' branch
In getUninstallErrorMessage, the "No uninstall script exists" branch template literal changed from ${UNINSTALL_SOFTWARE_ERROR_PREFIX}. An uninstall script... to ${UNINSTALL_SOFTWARE_ERROR_PREFIX} An uninstall script..., removing the stray period that doubled up with the prefix's own trailing period, matching the suggested fix exactly.
🤖 Prompt for AI agents
In frontend/pages/hosts/details/cards/HostSoftwareLibrary/helpers.tsx around line 94, review and complete this code-review fix: getUninstallErrorMessage produces malformed message with stray period before reason for the 'no uninstall script' branch.
What the draft fix changed: In getUninstallErrorMessage, the "No uninstall script exists" branch template literal changed from `${UNINSTALL_SOFTWARE_ERROR_PREFIX}. An uninstall script...` to `${UNINSTALL_SOFTWARE_ERROR_PREFIX} An uninstall script...`, removing the stray period that doubled up with the prefix's own trailing period, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| -fleet-url "${MDMPROXY_FLEET_URL:?}" \ | ||
| -server-address "${MDMPROXY_SERVER_ADDRESS:?}" | ||
|
|
||
| if [ -n "${MDMPROXY_AUTH_TOKEN}" ]; then |
There was a problem hiding this comment.
🦩 🟠 mdmproxy entrypoint.sh uses eval on env-var-derived arguments, risking shell injection via unquoted expansion
Replaced the eval-based argument assembly in tools/mdm/migration/mdmproxy/entrypoint.sh with a positional-parameter (set --) array built incrementally, then executed via exec /usr/bin/mdmproxy "$@" without eval. This removes the string-concatenation-then-eval anti-pattern entirely, so env vars like MDMPROXY_AUTH_TOKEN, MDMPROXY_MIGRATE_PERCENTAGE, and MDMPROXY_MIGRATE_UDIDS are passed as single quoted positional args and cannot be re-parsed by the shell, eliminating the injection vector regardless of embedded shell metacharacters.
🤖 Prompt for AI agents
In tools/mdm/migration/mdmproxy/entrypoint.sh around line 12, review and complete this code-review fix: mdmproxy entrypoint.sh uses eval on env-var-derived arguments, risking shell injection via unquoted expansion.
What the draft fix changed: Replaced the eval-based argument assembly in `tools/mdm/migration/mdmproxy/entrypoint.sh` with a positional-parameter (`set --`) array built incrementally, then executed via `exec /usr/bin/mdmproxy "$@"` without eval. This removes the string-concatenation-then-eval anti-pattern entirely, so env vars like MDMPROXY_AUTH_TOKEN, MDMPROXY_MIGRATE_PERCENTAGE, and MDMPROXY_MIGRATE_UDIDS are passed as single quoted positional args and cannot be re-parsed by the shell, eliminating the injection vector regardless of embedded shell metacharacters.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| @@ -175,7 +175,7 @@ def req(method: :get, path: '', body: nil, headers: {}, cached: false, environme | |||
| end | |||
| end | |||
| rescue => e | |||
There was a problem hiding this comment.
🦩 🟠 FleetClient#req swallows exceptions into a string, losing exception type/backtrace
In FleetClient#req's rescue clause, changed out['error'] = e to out['error'] = e.message so the stored error is a String consistent with parse_response's error messages, avoiding downstream type errors from an unserialized Exception object.
🤖 Prompt for AI agents
In ee/tools/puppet/fleetdm/lib/puppet/util/fleet_client.rb around line 177, review and complete this code-review fix: FleetClient#req swallows exceptions into a string, losing exception type/backtrace.
What the draft fix changed: In FleetClient#req's rescue clause, changed `out['error'] = e` to `out['error'] = e.message` so the stored error is a String consistent with parse_response's error messages, avoiding downstream type errors from an unserialized Exception object.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 74 review findings across 40 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
server/mdm/scep/depot/file/depot.go:102server/mdm/scep/depot/file/depot.go:41server/mdm/scep/depot/file/depot.go:127server/mdm/scep/depot/file/depot.go:238website/api/controllers/android-proxy/get-android-enterprises.js:49website/api/controllers/android-proxy/get-android-enterprises.js:93website/api/controllers/android-proxy/get-android-enterprises.js:56PROCESSED_PACKAGEDwrites to a different variable than the one that is read/reportedtools/gitops-migrate/migrate.sh:193tools/gitops-migrate/migrate.sh:76server/logging/pubsub.go:96server/logging/pubsub.go:80server/mdm/nanomdm/http/mdm/mdm.go:51server/mdm/nanomdm/http/mdm/mdm.go:80server/service/externalsvc/jira.go:113server/service/externalsvc/jira.go:126it(...)blocks execute at describe-time, not as part of a testfrontend/pages/admin/IntegrationsPage/cards/IdentityProviders/components/EndUserAuthSection/helpers.tests.ts:82it()blockfrontend/pages/admin/IntegrationsPage/cards/IdentityProviders/components/EndUserAuthSection/helpers.tests.ts:216tools/github-manage/pkg/ghapi/issues.go:372tools/github-manage/pkg/ghapi/issues.go:137ee/maintained-apps/inputs/homebrew/scripts/expressvpn-install.sh:18ee/maintained-apps/inputs/homebrew/scripts/expressvpn-install.sh:4frontend/pages/hosts/details/components/InventoryVersions/InventoryVersions.tsx:109frontend/pages/hosts/details/components/InventoryVersions/InventoryVersions.tsx:173frontend/pages/hosts/ManageHostsPage/components/CustomValueContainer/CustomValueContainer.tsx:1frontend/pages/hosts/ManageHostsPage/components/CustomValueContainer/CustomValueContainer.tsx:8server/service/async/collect.go:178server/service/async/collect.go:109server/service/embedded_scripts/delete-duplicate-scep-certificates.sh:75server/service/embedded_scripts/delete-duplicate-scep-certificates.sh:60tools/terraform/provider/teams_resource.go:223tools/terraform/provider/teams_resource.go:129website/assets/js/pages/docs/vital-details.page.js:126website/assets/js/pages/docs/vital-details.page.js:86server/datastore/mysql/secret_variables.go:361server/datastore/mysql/secret_variables.go:374orbit/pkg/execuser/execuser_windows.go:164orbit/pkg/execuser/execuser_windows.go:209server/datastore/mysql/migrations/tables/20250902112642_MigratePrimoFailingPoliciesAutomations.go:19server/datastore/mysql/migrations/tables/20250902112642_MigratePrimoFailingPoliciesAutomations.go:71server/vulnerabilities/io/github.go:124server/vulnerabilities/io/github.go:97server/vulnerabilities/osv/sync.go:43server/vulnerabilities/osv/sync.go:55cmd/osquery-perf/software-library/tools/import-data/main.go:49cmd/osquery-perf/software-library/tools/import-data/main.go:246frontend/pages/admin/OrgSettingsPage/cards/Info/Info.tsx:319frontend/pages/admin/OrgSettingsPage/cards/Info/Info.tsx:185server/goose/migrate.go:28server/goose/migrate.go:165website/api/controllers/articles/view-basic-webinar.js:1website/api/controllers/articles/view-basic-webinar.js:47website/assets/resources/install-fleetctl.sh:53website/assets/resources/install-fleetctl.sh:19cmd/maintained-apps/validate/windows.go:54cmd/maintained-apps/validate/windows.go:306frontend/hooks/useTeamIdParam.ts:79frontend/hooks/useTeamIdParam.ts:191frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tsx:162frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tsx:104tools/mdm/migration/simplemdm/main.go:20tools/mdm/migration/simplemdm/main.go:123frontend/pages/ManageControlsPage/SetupExperience/cards/BootstrapPackage/BootstrapPackage.tsx:141frontend/pages/ManageControlsPage/SetupExperience/cards/BootstrapPackage/BootstrapPackage.tsx:141server/service/software_installers.go:44server/service/software_installers.go:67server/datastore/mysql/migrations/tables/20220831100151_AddWindowsUpdatesTable.go:24frontend/pages/DashboardPage/cards/Software/Software.tsx:95website/api/helpers/microsoft-proxy/get-access-token-and-api-urls.js:29server/service/jitter_test.go:37fdefined twice, second definition silently shadows the source-file flagtools/team-builder/build_teams.sh:12frontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/SoftwareInstallDetailsModal.tsx:366frontend/pages/hosts/details/cards/HostSoftwareLibrary/helpers.tsx:94tools/mdm/migration/mdmproxy/entrypoint.sh:12ee/tools/puppet/fleetdm/lib/puppet/util/fleet_client.rb:177What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
1446a072-096e-4294-8082-c7cadffe76deMerging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akj32d7 FleetMDM bulk review findings sweep (12 PRs)