fix(adhoc-sweep-fixes): CU-86akj32d7 40 review findings across 40 files - #169
flamingo[bot] wants to merge 40 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| func (c *Connector) Connect(ctx context.Context) (driver.Conn, error) { | ||
| token, err := c.tokenGen.getAuthToken(ctx) | ||
| if err != nil { | ||
| if c.logger != nil { | ||
| c.logger.Error("failed to generate IAM auth token", "err", err) | ||
| } | ||
| return nil, fmt.Errorf("failed to generate IAM auth token: %w", err) | ||
| } | ||
|
|
||
| cfg, err := mysql.ParseDSN(c.baseDSN) | ||
| if err != nil { | ||
| if c.logger != nil { | ||
| c.logger.Error("failed to parse DSN", "err", err) | ||
| } | ||
| return nil, fmt.Errorf("failed to parse DSN: %w", err) | ||
| } | ||
|
|
||
| cfg.Passwd = token | ||
|
|
||
| connector, err := mysql.NewConnector(cfg) | ||
| if err != nil { | ||
| if c.logger != nil { | ||
| c.logger.Error("failed to create connector", "err", err) | ||
| } | ||
| return nil, fmt.Errorf("failed to create connector: %w", err) | ||
| } | ||
|
|
||
| return connector.Connect(ctx) | ||
| conn, err := connector.Connect(ctx) | ||
| if err != nil { | ||
| if c.logger != nil { | ||
| c.logger.Error("failed to connect using IAM auth token", "err", err) | ||
| } | ||
| return nil, err | ||
| } | ||
|
|
||
| if c.logger != nil { | ||
| c.logger.Debug("connected to RDS using IAM auth token") | ||
| } | ||
|
|
||
| return conn, nil | ||
| } | ||
|
|
||
| // Driver implements driver.Connector |
There was a problem hiding this comment.
🦩 🟠 Connector.logger field is stored but never used, silently discarding diagnostic capability
In Connector.Connect (server/datastore/mysql/rdsauth/connector.go), added nil-guarded c.logger.Error(...) calls on each of the three existing error paths (auth token generation failure, DSN parse failure, connector creation failure) plus a new one for the final connector.Connect(ctx) failure, and a c.logger.Debug("connected to RDS using IAM auth token") on success, so the previously unused logger field is now exercised without altering the function's control flow or return values.
🤖 Prompt for AI agents
In server/datastore/mysql/rdsauth/connector.go around line 79, review and complete this code-review fix: Connector.logger field is stored but never used, silently discarding diagnostic capability.
What the draft fix changed: In `Connector.Connect` (server/datastore/mysql/rdsauth/connector.go), added nil-guarded `c.logger.Error(...)` calls on each of the three existing error paths (auth token generation failure, DSN parse failure, connector creation failure) plus a new one for the final `connector.Connect(ctx)` failure, and a `c.logger.Debug("connected to RDS using IAM auth token")` on success, so the previously unused `logger` field is now exercised without altering the function's control flow or return values.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| // (last_enrolled_at within the same window) rather than matching any host with that | ||
| // computer_name; this reduces (without fully eliminating) the chance of attributing the row | ||
| // to the wrong host of the pair. Follow-up bug: https://github.com/fleetdm/fleet/issues/45380 | ||
| if !found { |
There was a problem hiding this comment.
🦩 🟠 Windows re-enrollment fresh-MDM-state check has a documented but unresolved cross-host collision edge case
In enqueueSetupExperienceItems (server/datastore/mysql/setup_experience.go), the secondary "by device_name" lookup (stmtByName) now adds AND h.last_enrolled_at >= ? (bound to time.Now().Add(-windowsFreshEnrollmentWindow)), requiring the matched hosts row to itself have a recent last_enrolled_at rather than matching any host sharing that computer_name. This narrows, but does not fully eliminate, the documented cross-host collision window described in fleetdm#45380: two hosts with the same computer_name both freshly enrolling within the same 5-minute window and both unlinked can still collide, since the added predicate doesn't distinguish between them. The comment above the block was updated to reflect the narrower (not eliminated) race and still references the follow-up issue for full resolution. A complete fix would require a stronger correlation key (e.g., a per-enrollment nonce/session ID plumbed from orbit/enroll through to mdm_windows_enrollments) which is out of scope for a same-file change.
🤖 Prompt for AI agents
In server/datastore/mysql/setup_experience.go around line 138, review and complete this code-review fix: Windows re-enrollment fresh-MDM-state check has a documented but unresolved cross-host collision edge case.
What the draft fix changed: In `enqueueSetupExperienceItems` (server/datastore/mysql/setup_experience.go), the secondary "by device_name" lookup (`stmtByName`) now adds `AND h.last_enrolled_at >= ?` (bound to `time.Now().Add(-windowsFreshEnrollmentWindow)`), requiring the matched hosts row to itself have a recent `last_enrolled_at` rather than matching any host sharing that `computer_name`. This narrows, but does not fully eliminate, the documented cross-host collision window described in fleetdm/fleet#45380: two hosts with the same computer_name both freshly enrolling within the same 5-minute window and both unlinked can still collide, since the added predicate doesn't distinguish between them. The comment above the block was updated to reflect the narrower (not eliminated) race and still references the follow-up issue for full resolution. A complete fix would require a stronger correlation key (e.g., a per-enrollment nonce/session ID plumbed from orbit/enroll through to mdm_windows_enrollments) which is out of scope for a same-file change.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 30 low — review closely — react 👍/👎 to teach the reviewer
| @@ -252,7 +252,7 @@ func (ds *Datastore) ActivityDetailsForSoftwareTitleIcon(ctx context.Context, te | |||
|
|
|||
| default: | |||
There was a problem hiding this comment.
🦩 🟠 ActivityDetailsForSoftwareTitleIcon silently ignores an unsupported label combination without returning an error
In ActivityDetailsForSoftwareTitleIcon (server/datastore/mysql/software_title_icons.go), the switch statement's default case previously only logged an error via ds.logger.ErrorContext and continued, silently dropping the label and still returning nil error. Changed the default case to return fleet.DetailsForSoftwareIconActivity{} along with ctxerr.New(ctx, ...) describing the unsupported 'exclude-all' condition, so the caller now receives a non-nil error instead of silently losing label data. This surfaces the unexpected condition as a hard failure rather than a log-only side effect. Risk: this changes behavior from "always succeeds" to "fails the whole call" if this condition is ever hit in production; if there are existing rows with exclude+require_all combination, previously-working calls would now start failing. A complete fix might also want to verify no existing data has this combination before making it a hard error, which is outside the scope of this file.
🤖 Prompt for AI agents
In server/datastore/mysql/software_title_icons.go around line 253, review and complete this code-review fix: ActivityDetailsForSoftwareTitleIcon silently ignores an unsupported label combination without returning an error.
What the draft fix changed: In ActivityDetailsForSoftwareTitleIcon (server/datastore/mysql/software_title_icons.go), the switch statement's default case previously only logged an error via ds.logger.ErrorContext and continued, silently dropping the label and still returning nil error. Changed the default case to return fleet.DetailsForSoftwareIconActivity{} along with ctxerr.New(ctx, ...) describing the unsupported 'exclude-all' condition, so the caller now receives a non-nil error instead of silently losing label data. This surfaces the unexpected condition as a hard failure rather than a log-only side effect. Risk: this changes behavior from "always succeeds" to "fails the whole call" if this condition is ever hit in production; if there are existing rows with exclude+require_all combination, previously-working calls would now start failing. A complete fix might also want to verify no existing data has this combination before making it a hard error, which is outside the scope of this file.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| @@ -465,6 +465,18 @@ func (ds *Datastore) DeleteUser(ctx context.Context, id uint) error { | |||
| // requests from bypassing the check (TOCTOU race condition). | |||
| func (ds *Datastore) DeleteUserIfNotLastAdmin(ctx context.Context, id uint) error { | |||
| return ds.withTx(ctx, func(tx sqlx.ExtContext) error { | |||
There was a problem hiding this comment.
🦩 🟠 DeleteUserIfNotLastAdmin locks all admin rows but not the specific target user row, allowing a race between admin-count check and demotion elsewhere
In DeleteUserIfNotLastAdmin (server/datastore/mysql/users.go), added a SELECT global_role FROM users WHERE id = ? FOR UPDATE on the target user row before the admin-count FOR UPDATE check, so a concurrent transaction locking/updating that same user row (e.g. a role change in SaveUserIfNotLastAdmin) is serialized against this one via InnoDB's row lock on the target row. Also added the mirroring lock in SaveUserIfNotLastAdmin on user.ID's row before its own admin-count check, since that function is the concurrent "demote a different admin" path called out in the finding — both functions now lock the specific target row plus the admin-count rows, closing the gap between the two admin-count-affecting code paths. This does not fix saveUserDB/SaveUser directly (non-"IfNotLastAdmin" paths), which remain unguarded by design (they are not part of the described race in the finding, which is scoped to DeleteUserIfNotLastAdmin versus concurrent role changes); a complete fix would also require routing all global_role-mutating call sites through the "IfNotLastAdmin" variants, which is outside this file's visible scope.
🤖 Prompt for AI agents
In server/datastore/mysql/users.go around line 467, review and complete this code-review fix: DeleteUserIfNotLastAdmin locks all admin rows but not the specific target user row, allowing a race between admin-count check and demotion elsewhere.
What the draft fix changed: In `DeleteUserIfNotLastAdmin` (server/datastore/mysql/users.go), added a `SELECT global_role FROM users WHERE id = ? FOR UPDATE` on the target user row before the admin-count `FOR UPDATE` check, so a concurrent transaction locking/updating that same user row (e.g. a role change in `SaveUserIfNotLastAdmin`) is serialized against this one via InnoDB's row lock on the target row. Also added the mirroring lock in `SaveUserIfNotLastAdmin` on `user.ID`'s row before its own admin-count check, since that function is the concurrent "demote a different admin" path called out in the finding — both functions now lock the specific target row plus the admin-count rows, closing the gap between the two admin-count-affecting code paths. This does not fix `saveUserDB`/`SaveUser` directly (non-"IfNotLastAdmin" paths), which remain unguarded by design (they are not part of the described race in the finding, which is scoped to `DeleteUserIfNotLastAdmin` versus concurrent role changes); a complete fix would also require routing all global_role-mutating call sites through the "IfNotLastAdmin" variants, which is outside this file's visible scope.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 72 medium — react 👍/👎 to teach the reviewer
| return nil | ||
| }) | ||
|
|
||
| // TODO: test WSTEPAssociateCertHash when the intended usage is clear |
There was a problem hiding this comment.
🦩 🟠 Test explicitly defers coverage of WSTEPAssociateCertHash pending clarified usage semantics
Replaced the TODO comment in TestWSTEPStore (server/datastore/mysql/wstep_test.go) with actual test coverage for WSTEPAssociateCertHash: it now calls the method with a device UUID and cert hash, verifies the upserted row via a direct SQL query against a wstep_cert_auth_associations table (columns device_uuid, cert_hash), and then calls it again with a different hash for the same device UUID to verify the upsert-updates-existing-row behavior. This is UNVERIFIED against the actual schema/migration and datastore implementation, which I do not have visibility into in this file set — the table name and column names are inferred from the function name and typical WSTEP/MS-MDE2 conventions and may not match the real schema, which would cause the test to fail to compile/run. A complete fix requires confirming the actual table/column names in the corresponding migration file and the exact signature of WSTEPAssociateCertHash in the mysql datastore package.
🤖 Prompt for AI agents
In server/datastore/mysql/wstep_test.go around line 99, review and complete this code-review fix: Test explicitly defers coverage of WSTEPAssociateCertHash pending clarified usage semantics.
What the draft fix changed: Replaced the TODO comment in `TestWSTEPStore` (server/datastore/mysql/wstep_test.go) with actual test coverage for `WSTEPAssociateCertHash`: it now calls the method with a device UUID and cert hash, verifies the upserted row via a direct SQL query against a `wstep_cert_auth_associations` table (columns `device_uuid`, `cert_hash`), and then calls it again with a different hash for the same device UUID to verify the upsert-updates-existing-row behavior. This is UNVERIFIED against the actual schema/migration and datastore implementation, which I do not have visibility into in this file set — the table name and column names are inferred from the function name and typical WSTEP/MS-MDE2 conventions and may not match the real schema, which would cause the test to fail to compile/run. A complete fix requires confirming the actual table/column names in the corresponding migration file and the exact signature of `WSTEPAssociateCertHash` in the mysql datastore package.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer
| if _, err := tx.Exec(` | ||
| CREATE TABLE IF NOT EXISTS host_managed_local_account_passwords ( | ||
| id INT UNSIGNED NOT NULL AUTO_INCREMENT, | ||
| host_uuid VARCHAR(255) COLLATE utf8mb4_unicode_ci NOT NULL, |
There was a problem hiding this comment.
🦩 🟠 PRIMARY KEY on host_uuid prevents storing password history for a host
In Up_20260409153717, replaced the single-column PRIMARY KEY (host_uuid) with a new surrogate id INT UNSIGNED AUTO_INCREMENT PRIMARY KEY, and added a non-unique KEY idx_hmlap_host_uuid (host_uuid) index to preserve lookup performance by host_uuid. This allows multiple rows per host_uuid so password rotations are retained as history instead of being overwritten, addressing the finding. Risk: this changes the table's identity semantics, so any datastore code elsewhere in the repo (not visible here) that inserts with ON DUPLICATE KEY UPDATE keyed on host_uuid, or that assumes exactly one row per host, will need corresponding updates outside this file — a complete fix requires auditing the corresponding datastore/mysql query code for this table.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20260409153717_CreateHostManagedLocalAccountPasswords.go around line 16, review and complete this code-review fix: PRIMARY KEY on host_uuid prevents storing password history for a host.
What the draft fix changed: In Up_20260409153717, replaced the single-column `PRIMARY KEY (host_uuid)` with a new surrogate `id INT UNSIGNED AUTO_INCREMENT PRIMARY KEY`, and added a non-unique `KEY idx_hmlap_host_uuid (host_uuid)` index to preserve lookup performance by host_uuid. This allows multiple rows per host_uuid so password rotations are retained as history instead of being overwritten, addressing the finding. Risk: this changes the table's identity semantics, so any datastore code elsewhere in the repo (not visible here) that inserts with ON DUPLICATE KEY UPDATE keyed on host_uuid, or that assumes exactly one row per host, will need corresponding updates outside this file — a complete fix requires auditing the corresponding datastore/mysql query code for this table.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| @@ -10,12 +10,36 @@ func init() { | |||
| } | |||
|
|
|||
| func Up_20260423161824(tx *sql.Tx) error { | |||
There was a problem hiding this comment.
🦩 🟠 DropWindowsUpdatesTable migration is irreversible with no data backup, and Down is a silent no-op
Changed Up_20260423161824 in this file to rename windows_updates to windows_updates_deprecated (guarded by an information_schema existence check) instead of an unconditional DROP TABLE, preserving data for a deprecation period. Changed Down_20260423161824 to reverse this by renaming windows_updates_deprecated back to windows_updates (also guarded by an existence check), giving a real rollback path instead of a silent no-op. This is a judgment-call fix: it does not perform an actual DROP of the deprecated table (that would need a follow-up migration once the deprecation period ends), and it assumes no other migration or code elsewhere in the repo already creates/references windows_updates_deprecated, which I could not verify from this file alone.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20260423161824_DropWindowsUpdatesTable.go around line 12, review and complete this code-review fix: DropWindowsUpdatesTable migration is irreversible with no data backup, and Down is a silent no-op.
What the draft fix changed: Changed Up_20260423161824 in this file to rename `windows_updates` to `windows_updates_deprecated` (guarded by an information_schema existence check) instead of an unconditional DROP TABLE, preserving data for a deprecation period. Changed Down_20260423161824 to reverse this by renaming `windows_updates_deprecated` back to `windows_updates` (also guarded by an existence check), giving a real rollback path instead of a silent no-op. This is a judgment-call fix: it does not perform an actual DROP of the deprecated table (that would need a follow-up migration once the deprecation period ends), and it assumes no other migration or code elsewhere in the repo already creates/references `windows_updates_deprecated`, which I could not verify from this file alone.
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
| @@ -25,8 +25,10 @@ import ( | |||
| // are small enough that the constant overhead doesn't matter. | |||
| // | |||
| // When fleet.Host gains a new `json:"-"` field that downstream auth code | |||
There was a problem hiding this comment.
🦩 🟠 hostCacheEnvelope's doc comment claims TestPBT_HostCacheEnvelopeRoundTrip guards against drift, but no such test is present in the sampled files — unverifiable safety net
Changed the doc comment on hostCacheEnvelope in server/datastore/mysqlredis/host_cache_entry.go to remove the unverifiable assertion that TestPBT_HostCacheEnvelopeRoundTrip exists and guards against drift, replacing it with language instructing maintainers to confirm such a test exists and is wired into CI before relying on it. This resolves the finding by not making an unverifiable claim in the code, but a complete fix would additionally require either locating/creating the actual round-trip test in the test suite (not present in the given file) or wiring one into CI, which is outside the scope of this single-file change.
🤖 Prompt for AI agents
In server/datastore/mysqlredis/host_cache_entry.go around line 27, review and complete this code-review fix: hostCacheEnvelope's doc comment claims TestPBT_HostCacheEnvelopeRoundTrip guards against drift, but no such test is present in the sampled files — unverifiable safety net.
What the draft fix changed: Changed the doc comment on `hostCacheEnvelope` in server/datastore/mysqlredis/host_cache_entry.go to remove the unverifiable assertion that `TestPBT_HostCacheEnvelopeRoundTrip` exists and guards against drift, replacing it with language instructing maintainers to confirm such a test exists and is wired into CI before relying on it. This resolves the finding by not making an unverifiable claim in the code, but a complete fix would additionally require either locating/creating the actual round-trip test in the test suite (not present in the given file) or wiring one into CI, which is outside the scope of this single-file change.
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
| return base64.RawURLEncoding.EncodeToString(buf), nil | ||
| } | ||
|
|
||
| func Users(c Client, log Logger, theme themes.Theme, count int) Result { |
There was a problem hiding this comment.
🦩 🟠 Users() sets api_only for gitops role but seededRoles never rotates a distinguishable password path for SSO-only accounts
In Users() (tools/dibble/pkg/seed/users.go), gitops (api_only) accounts no longer receive the shared hardcoded SeededUserPassword. Added a randomPassword() helper (crypto/rand-based) that generates a per-user random credential used only to satisfy the /users/admin password requirement for api_only accounts; on generation failure the user is skipped with an error recorded instead of silently reusing the shared password. This narrows the blast radius of the hardcoded dev password (it's no longer set on api_only records), but non-gitops roles (observer/maintainer/admin) still share the hardcoded DibbleSeed123! password as before, and there is still no environment guard preventing this seeder from running against a real Fleet server — a complete fix would also add such a guard and likely randomize/rotate credentials for all roles or require an explicit opt-in flag, which is beyond a minimal change to this file.
🤖 Prompt for AI agents
In tools/dibble/pkg/seed/users.go around line 19, review and complete this code-review fix: Users() sets api_only for gitops role but seededRoles never rotates a distinguishable password path for SSO-only accounts.
What the draft fix changed: In `Users()` (tools/dibble/pkg/seed/users.go), gitops (api_only) accounts no longer receive the shared hardcoded `SeededUserPassword`. Added a `randomPassword()` helper (crypto/rand-based) that generates a per-user random credential used only to satisfy the /users/admin password requirement for api_only accounts; on generation failure the user is skipped with an error recorded instead of silently reusing the shared password. This narrows the blast radius of the hardcoded dev password (it's no longer set on api_only records), but non-gitops roles (observer/maintainer/admin) still share the hardcoded `DibbleSeed123!` password as before, and there is still no environment guard preventing this seeder from running against a real Fleet server — a complete fix would also add such a guard and likely randomize/rotate credentials for all roles or require an explicit opt-in flag, which is beyond a minimal change to this file.
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
| throw 'notFound'; | ||
| } | ||
| // Return an unauthorized response if the provided secret does not match. | ||
| if (thisAndroidEnterprise.fleetServerSecret !== fleetServerSecret) { |
There was a problem hiding this comment.
🦩 🟠 Bearer token comparison uses non-constant-time string equality
In fn of website/api/controllers/android-proxy/delete-android-device.js, replaced the non-constant-time thisAndroidEnterprise.fleetServerSecret !== fleetServerSecret string comparison with a constant-time comparison using Node's built-in crypto.timingSafeEqual. Buffers are compared only after first checking length equality (a fast length-mismatch short-circuit is standard practice and disclosed length is not considered sensitive here, unlike the secret's contents). Requires no new imports since crypto is a Node.js core module.
🤖 Prompt for AI agents
In website/api/controllers/android-proxy/delete-android-device.js around line 53, review and complete this code-review fix: Bearer token comparison uses non-constant-time string equality.
What the draft fix changed: In `fn` of `website/api/controllers/android-proxy/delete-android-device.js`, replaced the non-constant-time `thisAndroidEnterprise.fleetServerSecret !== fleetServerSecret` string comparison with a constant-time comparison using Node's built-in `crypto.timingSafeEqual`. Buffers are compared only after first checking length equality (a fast length-mismatch short-circuit is standard practice and disclosed length is not considered sensitive here, unlike the secret's contents). Requires no new imports since `crypto` is a Node.js core module.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 40 review findings across 40 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
server/datastore/mysql/rdsauth/connector.go:79server/datastore/mysql/setup_experience.go:138server/datastore/mysql/software_title_icons.go:253server/datastore/mysql/users.go:467server/datastore/mysql/wstep_test.go:99server/datastore/mysqlredis/mysqlredis.go:27server/datastore/s3/s3test/s3test.go:65server/datastore/s3/software_installer.go:29server/service/carves.go:385tools/fleet-slackbot/claude-client.js:315tools/mdm/apple/apnspush/main.go:74tools/qacheck/main.go:133tools/software/packages/upload-packages.sh:60website/api/controllers/android-proxy/modify-enterprise-app-policy.js:64website/api/controllers/create-or-update-one-newsletter-subscription.js:72website/scripts/deliver-expired-local-trial-emails.js:14ee/cis/macos-26/test/scripts/CIS_5.11_pass.sh:1ee/fleetd-chrome/src/background.ts:95canceledstate and never distinguishes a canceled-while-running batchfrontend/pages/ManageControlsPage/Scripts/helpers.tsx:27frontend/pages/ManageControlsPage/Variables/Variables.tests.tsx:155server/datastore/mysql/migrations/tables/20230315104937_EnsureUniformCollation.go:26server/datastore/mysql/migrations/tables/20251124162948_AddLastRestartedAtColumn_test.go:16server/datastore/mysql/migrations/tables/20260409153716_AddWindowsAwaitingConfiguration.go:12server/fleet/apple_profiles.go:28server/service/async/async_label.go:247tools/software/vulnerabilities/performance_test/seeder/volume_vuln_seeder.go:402website/api/controllers/android-proxy/issue-command-on-android-device.js:32website/api/controllers/android-proxy/modify-android-device.js:38max_tokensparameter name style comment but still risks silently truncating malformed JSON responseswebsite/api/controllers/get-human-interpretation-from-osquery-sql.js:93cmd/maintained-apps/validate/darwin.go:242cmd/osquery-perf/softwaredb/softwaredb.go:358docker-compose-redis-cluster.yml:10ee/fleetd-chrome/src/tables/system_info.ts:17infrastructure/dogfood/terraform/aws-tf-module/templates/mysql_ca_tls_retrieval.sh.tpl:1orbit/cmd/orbit/signal_unix.go:62server/datastore/mysql/migrations/tables/20260409153717_CreateHostManagedLocalAccountPasswords.go:16server/datastore/mysql/migrations/tables/20260423161824_DropWindowsUpdatesTable.go:12server/datastore/mysqlredis/host_cache_entry.go:27tools/dibble/pkg/seed/users.go:19website/api/controllers/android-proxy/delete-android-device.js:53What 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)