fix(adhoc-sweep-fixes): CU-86akbhhtv 66 review findings across 40 files - #143
fix(adhoc-sweep-fixes): CU-86akbhhtv 66 review findings across 40 files#143flamingo[bot] wants to merge 40 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| log.Fatalf("host count (%d) must match expected team count (%d)", len(hosts), *teamCount) | ||
| } | ||
|
|
||
| if *teamExtraCount > len(hosts) { | ||
| log.Fatalf("team_extra_count (%d) exceeds available hosts (%d)", *teamExtraCount, len(hosts)) | ||
| } | ||
|
|
||
| printfAndPrompt("1. Creating %d teams...", *teamCount) | ||
| start := time.Now() | ||
|
|
There was a problem hiding this comment.
🦩 🔴 loadtest.go uses uninitialized index t against hosts slice in loop bounded by teamExtraCount, risking index-out-of-range panic
Added a validation check if *teamExtraCount > len(hosts) { log.Fatalf(...) } immediately after the existing if len(hosts) != *teamCount check in main(), before any teams/hosts processing begins. This ensures the program fails fast with a clear error message instead of panicking later with an index-out-of-range error in the extra-teams loop (for t := 0; t < *teamExtraCount; t++ { ... hosts[t] ... }), matching the suggested fix exactly.
🤖 Prompt for AI agents
In tools/mdm/apple/loadtest/loadtest.go around line 204, review and complete this code-review fix: loadtest.go uses uninitialized index t against hosts slice in loop bounded by teamExtraCount, risking index-out-of-range panic.
What the draft fix changed: Added a validation check `if *teamExtraCount > len(hosts) { log.Fatalf(...) }` immediately after the existing `if len(hosts) != *teamCount` check in `main()`, before any teams/hosts processing begins. This ensures the program fails fast with a clear error message instead of panicking later with an index-out-of-range error in the extra-teams loop (`for t := 0; t < *teamExtraCount; t++ { ... hosts[t] ... }`), matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| for i := range 3 { | ||
| osqueryHostID := fmt.Sprintf("idp-cron-%d", i) | ||
| nodeKey := fmt.Sprintf("idp-cron-%d", i) | ||
| h, err := ds.NewHost(ctx, &fleet.Host{ |
There was a problem hiding this comment.
🦩 🔴 cron_test.go calls new(...) as if it were a helper function for creating pointers, but this is not valid Go without a locally-defined helper
In TestHostVitalsLabelMembershipCronIDP, replaced all invalid new(value) calls with standard Go pointer idioms: introduced local variables (osqueryHostID, nodeKey, active, vital, value, criteriaRawMessage) and took their addresses (&osqueryHostID, etc.) for the OsqueryHostID, NodeKey, Active, Vital, Value, and HostVitalsCriteria struct fields, so the file now compiles without relying on any nonexistent generic new[T any](v T) *T helper. Risk: since no such helper is defined anywhere in the visible file, this assumes the original intent was simple pointer-to-value construction identical to what a ptr.String/ptr.Bool-style helper would produce; the reviewer should confirm the field types (*string, *bool, *json.RawMessage) match, and check whether the project has a conventional ptr package that should be used instead for consistency with the rest of the codebase.
🤖 Prompt for AI agents
In cmd/fleet/cron_test.go around line 363, review and complete this code-review fix: cron_test.go calls new(...) as if it were a helper function for creating pointers, but this is not valid Go without a locally-defined helper.
What the draft fix changed: In `TestHostVitalsLabelMembershipCronIDP`, replaced all invalid `new(value)` calls with standard Go pointer idioms: introduced local variables (`osqueryHostID`, `nodeKey`, `active`, `vital`, `value`, `criteriaRawMessage`) and took their addresses (`&osqueryHostID`, etc.) for the `OsqueryHostID`, `NodeKey`, `Active`, `Vital`, `Value`, and `HostVitalsCriteria` struct fields, so the file now compiles without relying on any nonexistent generic `new[T any](v T) *T` helper. Risk: since no such helper is defined anywhere in the visible file, this assumes the original intent was simple pointer-to-value construction identical to what a `ptr.String`/`ptr.Bool`-style helper would produce; the reviewer should confirm the field types (`*string`, `*bool`, `*json.RawMessage`) match, and check whether the project has a conventional `ptr` package that should be used instead for consistency with the rest of the codebase.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| @@ -28,7 +28,9 @@ const DeleteEntraClientIdModal = ({ | |||
|
|
|||
| try { | |||
| const currentClientIds = config?.mdm.windows_entra_client_ids ?? []; | |||
There was a problem hiding this comment.
🦩 🔴 Entra client ID de-duplication normalizes input to lowercase but does not validate case-insensitive duplicates against the raw stored value consistently on delete
Changed the filter predicate in onDeleteClientId (DeleteEntraClientIdModal component) from strict equality id !== clientId to case-insensitive comparison id.toLowerCase() !== clientId.toLowerCase(), ensuring that a stored client ID differing only in case from the passed-in clientId prop is still matched and removed, consistent with the case-insensitive duplicate check used in AddEntraClientIDModal.
🤖 Prompt for AI agents
In frontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/DeleteEntraClientIDModal/DeleteEntraClientIDModal.tsx around line 30, review and complete this code-review fix: Entra client ID de-duplication normalizes input to lowercase but does not validate case-insensitive duplicates against the raw stored value consistently on delete.
What the draft fix changed: Changed the filter predicate in `onDeleteClientId` (DeleteEntraClientIdModal component) from strict equality `id !== clientId` to case-insensitive comparison `id.toLowerCase() !== clientId.toLowerCase()`, ensuring that a stored client ID differing only in case from the passed-in `clientId` prop is still matched and removed, consistent with the case-insensitive duplicate check used in AddEntraClientIDModal.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| @@ -30,6 +30,7 @@ const ApiOnlyUser = ({ router }: IApiOnlyUserProps): JSX.Element => { | |||
| } | |||
| } catch (response) { | |||
There was a problem hiding this comment.
🦩 🔴 console.error used to swallow fetch-current-user failure instead of surfacing to the user
In the fetchCurrentUser function's catch block inside the useEffect hook, added router.push(LOGIN) after the existing console.error(response) call, so that a fetch failure (e.g., network error or unhandled 401) now redirects the user to the LOGIN page instead of leaving them stuck on the 'Access denied' page, matching the behavior of the !user branch.
🤖 Prompt for AI agents
In frontend/pages/ApiOnlyUser/ApiOnlyUser.tsx around line 31, review and complete this code-review fix: console.error used to swallow fetch-current-user failure instead of surfacing to the user.
What the draft fix changed: In the `fetchCurrentUser` function's `catch` block inside the `useEffect` hook, added `router.push(LOGIN)` after the existing `console.error(response)` call, so that a fetch failure (e.g., network error or unhandled 401) now redirects the user to the LOGIN page instead of leaving them stuck on the 'Access denied' page, matching the behavior of the `!user` branch.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| e.preventDefault(); | ||
|
|
||
| setIsUpdating(true); | ||
| const canLockEndUserInfo = |
There was a problem hiding this comment.
🦩 🔴 UsersForm resets lockEndUserInfo to computed canLockEndUserInfo after save even when Apple MDM is not configured
In onSubmit, replaced the unconditional canLockEndUserInfo = formData.endUserAuthEnabled && formData.lockEndUserInfo computation with lockEndUserInfoToSend, which only applies that collapsing logic when isMacMdmEnabledAndConfigured is true; otherwise it passes through formData.lockEndUserInfo unchanged (preserving the backend-derived value when Apple MDM isn't configured, matching the read-only-field semantics already established in onEndUserAuthChange). The payload's lock_end_user_info field and the post-save setFormData sync now both use this same value, and since that field is only included in the payload when isMacMdmEnabledAndConfigured is true anyway, behavior for the Apple-MDM-configured path is unchanged while the non-configured path no longer silently corrupts formData.lockEndUserInfo.
🤖 Prompt for AI agents
In frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsx around line 102, review and complete this code-review fix: UsersForm resets lockEndUserInfo to computed canLockEndUserInfo after save even when Apple MDM is not configured.
What the draft fix changed: In `onSubmit`, replaced the unconditional `canLockEndUserInfo = formData.endUserAuthEnabled && formData.lockEndUserInfo` computation with `lockEndUserInfoToSend`, which only applies that collapsing logic when `isMacMdmEnabledAndConfigured` is true; otherwise it passes through `formData.lockEndUserInfo` unchanged (preserving the backend-derived value when Apple MDM isn't configured, matching the read-only-field semantics already established in `onEndUserAuthChange`). The payload's `lock_end_user_info` field and the post-save `setFormData` sync now both use this same value, and since that field is only included in the payload when `isMacMdmEnabledAndConfigured` is true anyway, behavior for the Apple-MDM-configured path is unchanged while the non-configured path no longer silently corrupts `formData.lockEndUserInfo`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| return nil | ||
| } | ||
|
|
||
| func (s *CVE) updateYearFile(ctx context.Context, year int, cves []nvdapi.CVEItem) error { |
There was a problem hiding this comment.
🦩 🟠 Legacy feed year clamping logic duplicated verbatim between updateYearFile and updateVulnCheckYearFile
Extracted the duplicated "clamp year to 2002" logic into a new small helper legacyFeedYear(year int) int, used by both updateYearFile and updateVulnCheckYearFile, so future changes to the year floor only need to be made in one place. The finding also mentions broader duplication in the read/convert/merge/store pattern between the two functions, which was NOT deduplicated (doing so would be a larger structural refactor touching both functions' distinct merge semantics — VulnCheck merge preserves existing configurations and tracks mod/add counts differently than the plain NVD merge). This is a partial fix: it resolves the specific "year < 2002" duplication cited in the evidence but leaves the larger duplicated read/merge/store pattern as-is, which the finding also flags as a risk.
🤖 Prompt for AI agents
In server/vulnerabilities/nvd/sync/cve_syncer.go around line 204, review and complete this code-review fix: Legacy feed year clamping logic duplicated verbatim between updateYearFile and updateVulnCheckYearFile.
What the draft fix changed: Extracted the duplicated "clamp year to 2002" logic into a new small helper `legacyFeedYear(year int) int`, used by both `updateYearFile` and `updateVulnCheckYearFile`, so future changes to the year floor only need to be made in one place. The finding also mentions broader duplication in the read/convert/merge/store pattern between the two functions, which was NOT deduplicated (doing so would be a larger structural refactor touching both functions' distinct merge semantics — VulnCheck merge preserves existing configurations and tracks mod/add counts differently than the plain NVD merge). This is a partial fix: it resolves the specific "year < 2002" duplication cited in the evidence but leaves the larger duplicated read/merge/store pattern as-is, which the finding also flags as a risk.
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
| } | ||
|
|
||
| // writeLastModStartDateFile writes the lastModStartDate to a file in the local DB directory. | ||
| func (s *CVE) writeLastModStartDateFile(lastModStartDate string) error { |
There was a problem hiding this comment.
🦩 🟠 writeLastModStartDateFile propagates raw error from parseAndFormatForNVD without wrapping
In writeLastModStartDateFile, changed return err to return fmt.Errorf("writeLastModStartDateFile: %w", err) when parseAndFormatForNVD fails, wrapping the error with call-site context per the cited convention, using %w to preserve error chain unwrapping.
🤖 Prompt for AI agents
In server/vulnerabilities/nvd/sync/cve_syncer.go around line 333, review and complete this code-review fix: writeLastModStartDateFile propagates raw error from parseAndFormatForNVD without wrapping.
What the draft fix changed: In `writeLastModStartDateFile`, changed `return err` to `return fmt.Errorf("writeLastModStartDateFile: %w", err)` when `parseAndFormatForNVD` fails, wrapping the error with call-site context per the cited convention, using `%w` to preserve error chain unwrapping.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -168,6 +168,10 @@ func (cf cpeFile) Sync(ctx context.Context, src SourceConfig, localdir string) e | |||
|
|
|||
| func (cf cpeFile) needsUpdate(ctx context.Context, targetURL, localdir string) (bool, error) { | |||
There was a problem hiding this comment.
🦩 🟠 needsUpdate() only checks Etag file, not the actual presence of the data file itself
In needsUpdate() (cpe.go), added an os.Stat check on filepath.Join(localdir, cf.DataFile) at the top of the function; if the data file is missing, the function now immediately returns (true, nil) to force a re-sync, instead of relying solely on the etag file comparison. This directly addresses the finding that a matching etag with a missing/deleted data file previously caused needsUpdate to wrongly report no update needed.
🤖 Prompt for AI agents
In server/vulnerabilities/nvd/tools/providers/nvd/cpe.go around line 169, review and complete this code-review fix: needsUpdate() only checks Etag file, not the actual presence of the data file itself.
What the draft fix changed: In needsUpdate() (cpe.go), added an os.Stat check on filepath.Join(localdir, cf.DataFile) at the top of the function; if the data file is missing, the function now immediately returns (true, nil) to force a re-sync, instead of relying solely on the etag file comparison. This directly addresses the finding that a matching etag with a missing/deleted data file previously caused needsUpdate to wrongly report no update needed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
|
|
||
| func (cf cpeFile) needsUpdate(ctx context.Context, targetURL, localdir string) (bool, error) { | ||
| flog.V(1).Infof("checking etag for %q", targetURL) | ||
| if _, err := os.Stat(filepath.Join(localdir, cf.DataFile)); err != nil { | ||
| flog.V(1).Infof("data file %q does not exist in %q, needs sync", cf.DataFile, localdir) | ||
| return true, nil | ||
| } | ||
| req, err := httpNewRequestContext(ctx, "HEAD", targetURL) | ||
| if err != nil { | ||
| return false, err |
There was a problem hiding this comment.
🦩 🟠 cpe.go Sync uses non-atomic rename-swap that can leave data file missing on crash between renames
No rename/recovery logic was changed in Sync(); this finding is only partially mitigated as a side effect of fix #1 (needsUpdate now detects a missing data file and forces a re-download on the next run, so the crash window no longer causes a permanent stale state). This does NOT make the rename-swap atomic and does NOT add a startup check to recover from a stray .bak file — a complete fix would require detecting/restoring the .bak file explicitly (e.g., at the start of Sync, if dataFilename is missing but bakDataFilename exists, rename it back) or using a truly atomic replace mechanism. Risk: between the two renames, the file is still briefly missing on disk, and any concurrent reader in that instant sees no file; only the next Sync invocation self-heals now, via needsUpdate's new existence check.
🤖 Prompt for AI agents
In server/vulnerabilities/nvd/tools/providers/nvd/cpe.go around line 157, review and complete this code-review fix: cpe.go Sync uses non-atomic rename-swap that can leave data file missing on crash between renames.
What the draft fix changed: No rename/recovery logic was changed in Sync(); this finding is only partially mitigated as a side effect of fix #1 (needsUpdate now detects a missing data file and forces a re-download on the next run, so the crash window no longer causes a *permanent* stale state). This does NOT make the rename-swap atomic and does NOT add a startup check to recover from a stray .bak file — a complete fix would require detecting/restoring the .bak file explicitly (e.g., at the start of Sync, if dataFilename is missing but bakDataFilename exists, rename it back) or using a truly atomic replace mechanism. Risk: between the two renames, the file is still briefly missing on disk, and any concurrent reader in that instant sees no file; only the next Sync invocation self-heals now, via needsUpdate's new existence check.
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 != nil { | ||
| return "", "", err | ||
| } | ||
| defer dataFile.Close() | ||
| _, err = io.Copy(dataFile, resp.Body) | ||
| if err != nil { | ||
| return "", "", err |
There was a problem hiding this comment.
🦩 🔵 cpe.go download() ignores non-200 error body / does not clean up temp file on later errors within Sync
In download() (cpe.go), added defer dataFile.Close() immediately after the successful ioutil.TempFile call, ensuring the file descriptor is always closed on both the success path and the early-return error path when io.Copy fails, fixing the file descriptor leak. The partially-written temp file itself is still cleaned up by the caller's existing defer os.Remove(tempDataFilename) in Sync, which was already correct and untouched.
🤖 Prompt for AI agents
In server/vulnerabilities/nvd/tools/providers/nvd/cpe.go around line 201, review and complete this code-review fix: cpe.go download() ignores non-200 error body / does not clean up temp file on later errors within Sync.
What the draft fix changed: In download() (cpe.go), added `defer dataFile.Close()` immediately after the successful ioutil.TempFile call, ensuring the file descriptor is always closed on both the success path and the early-return error path when io.Copy fails, fixing the file descriptor leak. The partially-written temp file itself is still cleaned up by the caller's existing `defer os.Remove(tempDataFilename)` in Sync, which was already correct and untouched.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| log.Fatalf("host count (%d) must match expected team count (%d)", len(hosts), *teamCount) | ||
| } | ||
|
|
||
| if *teamExtraCount > len(hosts) { | ||
| log.Fatalf("team_extra_count (%d) exceeds available hosts (%d)", *teamExtraCount, len(hosts)) | ||
| } | ||
|
|
||
| printfAndPrompt("1. Creating %d teams...", *teamCount) | ||
| start := time.Now() | ||
|
|
There was a problem hiding this comment.
🦩 🔴 loadtest.go uses uninitialized index t against hosts slice in loop bounded by teamExtraCount, risking index-out-of-range panic
Added a validation check if *teamExtraCount > len(hosts) { log.Fatalf(...) } immediately after the existing if len(hosts) != *teamCount check in main(), before any teams/hosts processing begins. This ensures the program fails fast with a clear error message instead of panicking later with an index-out-of-range error in the extra-teams loop (for t := 0; t < *teamExtraCount; t++ { ... hosts[t] ... }), matching the suggested fix exactly.
🤖 Prompt for AI agents
In tools/mdm/apple/loadtest/loadtest.go around line 204, review and complete this code-review fix: loadtest.go uses uninitialized index t against hosts slice in loop bounded by teamExtraCount, risking index-out-of-range panic.
What the draft fix changed: Added a validation check `if *teamExtraCount > len(hosts) { log.Fatalf(...) }` immediately after the existing `if len(hosts) != *teamCount` check in `main()`, before any teams/hosts processing begins. This ensures the program fails fast with a clear error message instead of panicking later with an index-out-of-range error in the extra-teams loop (`for t := 0; t < *teamExtraCount; t++ { ... hosts[t] ... }`), matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| for i := range 3 { | ||
| osqueryHostID := fmt.Sprintf("idp-cron-%d", i) | ||
| nodeKey := fmt.Sprintf("idp-cron-%d", i) | ||
| h, err := ds.NewHost(ctx, &fleet.Host{ |
There was a problem hiding this comment.
🦩 🔴 cron_test.go calls new(...) as if it were a helper function for creating pointers, but this is not valid Go without a locally-defined helper
In TestHostVitalsLabelMembershipCronIDP, replaced all invalid new(value) calls with standard Go pointer idioms: introduced local variables (osqueryHostID, nodeKey, active, vital, value, criteriaRawMessage) and took their addresses (&osqueryHostID, etc.) for the OsqueryHostID, NodeKey, Active, Vital, Value, and HostVitalsCriteria struct fields, so the file now compiles without relying on any nonexistent generic new[T any](v T) *T helper. Risk: since no such helper is defined anywhere in the visible file, this assumes the original intent was simple pointer-to-value construction identical to what a ptr.String/ptr.Bool-style helper would produce; the reviewer should confirm the field types (*string, *bool, *json.RawMessage) match, and check whether the project has a conventional ptr package that should be used instead for consistency with the rest of the codebase.
🤖 Prompt for AI agents
In cmd/fleet/cron_test.go around line 363, review and complete this code-review fix: cron_test.go calls new(...) as if it were a helper function for creating pointers, but this is not valid Go without a locally-defined helper.
What the draft fix changed: In `TestHostVitalsLabelMembershipCronIDP`, replaced all invalid `new(value)` calls with standard Go pointer idioms: introduced local variables (`osqueryHostID`, `nodeKey`, `active`, `vital`, `value`, `criteriaRawMessage`) and took their addresses (`&osqueryHostID`, etc.) for the `OsqueryHostID`, `NodeKey`, `Active`, `Vital`, `Value`, and `HostVitalsCriteria` struct fields, so the file now compiles without relying on any nonexistent generic `new[T any](v T) *T` helper. Risk: since no such helper is defined anywhere in the visible file, this assumes the original intent was simple pointer-to-value construction identical to what a `ptr.String`/`ptr.Bool`-style helper would produce; the reviewer should confirm the field types (`*string`, `*bool`, `*json.RawMessage`) match, and check whether the project has a conventional `ptr` package that should be used instead for consistency with the rest of the codebase.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| @@ -28,7 +28,9 @@ const DeleteEntraClientIdModal = ({ | |||
|
|
|||
| try { | |||
| const currentClientIds = config?.mdm.windows_entra_client_ids ?? []; | |||
There was a problem hiding this comment.
🦩 🔴 Entra client ID de-duplication normalizes input to lowercase but does not validate case-insensitive duplicates against the raw stored value consistently on delete
Changed the filter predicate in onDeleteClientId (DeleteEntraClientIdModal component) from strict equality id !== clientId to case-insensitive comparison id.toLowerCase() !== clientId.toLowerCase(), ensuring that a stored client ID differing only in case from the passed-in clientId prop is still matched and removed, consistent with the case-insensitive duplicate check used in AddEntraClientIDModal.
🤖 Prompt for AI agents
In frontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/DeleteEntraClientIDModal/DeleteEntraClientIDModal.tsx around line 30, review and complete this code-review fix: Entra client ID de-duplication normalizes input to lowercase but does not validate case-insensitive duplicates against the raw stored value consistently on delete.
What the draft fix changed: Changed the filter predicate in `onDeleteClientId` (DeleteEntraClientIdModal component) from strict equality `id !== clientId` to case-insensitive comparison `id.toLowerCase() !== clientId.toLowerCase()`, ensuring that a stored client ID differing only in case from the passed-in `clientId` prop is still matched and removed, consistent with the case-insensitive duplicate check used in AddEntraClientIDModal.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| @@ -30,6 +30,7 @@ const ApiOnlyUser = ({ router }: IApiOnlyUserProps): JSX.Element => { | |||
| } | |||
| } catch (response) { | |||
There was a problem hiding this comment.
🦩 🔴 console.error used to swallow fetch-current-user failure instead of surfacing to the user
In the fetchCurrentUser function's catch block inside the useEffect hook, added router.push(LOGIN) after the existing console.error(response) call, so that a fetch failure (e.g., network error or unhandled 401) now redirects the user to the LOGIN page instead of leaving them stuck on the 'Access denied' page, matching the behavior of the !user branch.
🤖 Prompt for AI agents
In frontend/pages/ApiOnlyUser/ApiOnlyUser.tsx around line 31, review and complete this code-review fix: console.error used to swallow fetch-current-user failure instead of surfacing to the user.
What the draft fix changed: In the `fetchCurrentUser` function's `catch` block inside the `useEffect` hook, added `router.push(LOGIN)` after the existing `console.error(response)` call, so that a fetch failure (e.g., network error or unhandled 401) now redirects the user to the LOGIN page instead of leaving them stuck on the 'Access denied' page, matching the behavior of the `!user` branch.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| e.preventDefault(); | ||
|
|
||
| setIsUpdating(true); | ||
| const canLockEndUserInfo = |
There was a problem hiding this comment.
🦩 🔴 UsersForm resets lockEndUserInfo to computed canLockEndUserInfo after save even when Apple MDM is not configured
In onSubmit, replaced the unconditional canLockEndUserInfo = formData.endUserAuthEnabled && formData.lockEndUserInfo computation with lockEndUserInfoToSend, which only applies that collapsing logic when isMacMdmEnabledAndConfigured is true; otherwise it passes through formData.lockEndUserInfo unchanged (preserving the backend-derived value when Apple MDM isn't configured, matching the read-only-field semantics already established in onEndUserAuthChange). The payload's lock_end_user_info field and the post-save setFormData sync now both use this same value, and since that field is only included in the payload when isMacMdmEnabledAndConfigured is true anyway, behavior for the Apple-MDM-configured path is unchanged while the non-configured path no longer silently corrupts formData.lockEndUserInfo.
🤖 Prompt for AI agents
In frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsx around line 102, review and complete this code-review fix: UsersForm resets lockEndUserInfo to computed canLockEndUserInfo after save even when Apple MDM is not configured.
What the draft fix changed: In `onSubmit`, replaced the unconditional `canLockEndUserInfo = formData.endUserAuthEnabled && formData.lockEndUserInfo` computation with `lockEndUserInfoToSend`, which only applies that collapsing logic when `isMacMdmEnabledAndConfigured` is true; otherwise it passes through `formData.lockEndUserInfo` unchanged (preserving the backend-derived value when Apple MDM isn't configured, matching the read-only-field semantics already established in `onEndUserAuthChange`). The payload's `lock_end_user_info` field and the post-save `setFormData` sync now both use this same value, and since that field is only included in the payload when `isMacMdmEnabledAndConfigured` is true anyway, behavior for the Apple-MDM-configured path is unchanged while the non-configured path no longer silently corrupts `formData.lockEndUserInfo`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| return nil | ||
| } | ||
|
|
||
| func (s *CVE) updateYearFile(ctx context.Context, year int, cves []nvdapi.CVEItem) error { |
There was a problem hiding this comment.
🦩 🟠 Legacy feed year clamping logic duplicated verbatim between updateYearFile and updateVulnCheckYearFile
Extracted the duplicated "clamp year to 2002" logic into a new small helper legacyFeedYear(year int) int, used by both updateYearFile and updateVulnCheckYearFile, so future changes to the year floor only need to be made in one place. The finding also mentions broader duplication in the read/convert/merge/store pattern between the two functions, which was NOT deduplicated (doing so would be a larger structural refactor touching both functions' distinct merge semantics — VulnCheck merge preserves existing configurations and tracks mod/add counts differently than the plain NVD merge). This is a partial fix: it resolves the specific "year < 2002" duplication cited in the evidence but leaves the larger duplicated read/merge/store pattern as-is, which the finding also flags as a risk.
🤖 Prompt for AI agents
In server/vulnerabilities/nvd/sync/cve_syncer.go around line 204, review and complete this code-review fix: Legacy feed year clamping logic duplicated verbatim between updateYearFile and updateVulnCheckYearFile.
What the draft fix changed: Extracted the duplicated "clamp year to 2002" logic into a new small helper `legacyFeedYear(year int) int`, used by both `updateYearFile` and `updateVulnCheckYearFile`, so future changes to the year floor only need to be made in one place. The finding also mentions broader duplication in the read/convert/merge/store pattern between the two functions, which was NOT deduplicated (doing so would be a larger structural refactor touching both functions' distinct merge semantics — VulnCheck merge preserves existing configurations and tracks mod/add counts differently than the plain NVD merge). This is a partial fix: it resolves the specific "year < 2002" duplication cited in the evidence but leaves the larger duplicated read/merge/store pattern as-is, which the finding also flags as a risk.
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
| } | ||
|
|
||
| // writeLastModStartDateFile writes the lastModStartDate to a file in the local DB directory. | ||
| func (s *CVE) writeLastModStartDateFile(lastModStartDate string) error { |
There was a problem hiding this comment.
🦩 🟠 writeLastModStartDateFile propagates raw error from parseAndFormatForNVD without wrapping
In writeLastModStartDateFile, changed return err to return fmt.Errorf("writeLastModStartDateFile: %w", err) when parseAndFormatForNVD fails, wrapping the error with call-site context per the cited convention, using %w to preserve error chain unwrapping.
🤖 Prompt for AI agents
In server/vulnerabilities/nvd/sync/cve_syncer.go around line 333, review and complete this code-review fix: writeLastModStartDateFile propagates raw error from parseAndFormatForNVD without wrapping.
What the draft fix changed: In `writeLastModStartDateFile`, changed `return err` to `return fmt.Errorf("writeLastModStartDateFile: %w", err)` when `parseAndFormatForNVD` fails, wrapping the error with call-site context per the cited convention, using `%w` to preserve error chain unwrapping.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -168,6 +168,10 @@ func (cf cpeFile) Sync(ctx context.Context, src SourceConfig, localdir string) e | |||
|
|
|||
| func (cf cpeFile) needsUpdate(ctx context.Context, targetURL, localdir string) (bool, error) { | |||
There was a problem hiding this comment.
🦩 🟠 needsUpdate() only checks Etag file, not the actual presence of the data file itself
In needsUpdate() (cpe.go), added an os.Stat check on filepath.Join(localdir, cf.DataFile) at the top of the function; if the data file is missing, the function now immediately returns (true, nil) to force a re-sync, instead of relying solely on the etag file comparison. This directly addresses the finding that a matching etag with a missing/deleted data file previously caused needsUpdate to wrongly report no update needed.
🤖 Prompt for AI agents
In server/vulnerabilities/nvd/tools/providers/nvd/cpe.go around line 169, review and complete this code-review fix: needsUpdate() only checks Etag file, not the actual presence of the data file itself.
What the draft fix changed: In needsUpdate() (cpe.go), added an os.Stat check on filepath.Join(localdir, cf.DataFile) at the top of the function; if the data file is missing, the function now immediately returns (true, nil) to force a re-sync, instead of relying solely on the etag file comparison. This directly addresses the finding that a matching etag with a missing/deleted data file previously caused needsUpdate to wrongly report no update needed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
|
|
||
| func (cf cpeFile) needsUpdate(ctx context.Context, targetURL, localdir string) (bool, error) { | ||
| flog.V(1).Infof("checking etag for %q", targetURL) | ||
| if _, err := os.Stat(filepath.Join(localdir, cf.DataFile)); err != nil { | ||
| flog.V(1).Infof("data file %q does not exist in %q, needs sync", cf.DataFile, localdir) | ||
| return true, nil | ||
| } | ||
| req, err := httpNewRequestContext(ctx, "HEAD", targetURL) | ||
| if err != nil { | ||
| return false, err |
There was a problem hiding this comment.
🦩 🟠 cpe.go Sync uses non-atomic rename-swap that can leave data file missing on crash between renames
No rename/recovery logic was changed in Sync(); this finding is only partially mitigated as a side effect of fix #1 (needsUpdate now detects a missing data file and forces a re-download on the next run, so the crash window no longer causes a permanent stale state). This does NOT make the rename-swap atomic and does NOT add a startup check to recover from a stray .bak file — a complete fix would require detecting/restoring the .bak file explicitly (e.g., at the start of Sync, if dataFilename is missing but bakDataFilename exists, rename it back) or using a truly atomic replace mechanism. Risk: between the two renames, the file is still briefly missing on disk, and any concurrent reader in that instant sees no file; only the next Sync invocation self-heals now, via needsUpdate's new existence check.
🤖 Prompt for AI agents
In server/vulnerabilities/nvd/tools/providers/nvd/cpe.go around line 157, review and complete this code-review fix: cpe.go Sync uses non-atomic rename-swap that can leave data file missing on crash between renames.
What the draft fix changed: No rename/recovery logic was changed in Sync(); this finding is only partially mitigated as a side effect of fix #1 (needsUpdate now detects a missing data file and forces a re-download on the next run, so the crash window no longer causes a *permanent* stale state). This does NOT make the rename-swap atomic and does NOT add a startup check to recover from a stray .bak file — a complete fix would require detecting/restoring the .bak file explicitly (e.g., at the start of Sync, if dataFilename is missing but bakDataFilename exists, rename it back) or using a truly atomic replace mechanism. Risk: between the two renames, the file is still briefly missing on disk, and any concurrent reader in that instant sees no file; only the next Sync invocation self-heals now, via needsUpdate's new existence check.
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 != nil { | ||
| return "", "", err | ||
| } | ||
| defer dataFile.Close() | ||
| _, err = io.Copy(dataFile, resp.Body) | ||
| if err != nil { | ||
| return "", "", err |
There was a problem hiding this comment.
🦩 🔵 cpe.go download() ignores non-200 error body / does not clean up temp file on later errors within Sync
In download() (cpe.go), added defer dataFile.Close() immediately after the successful ioutil.TempFile call, ensuring the file descriptor is always closed on both the success path and the early-return error path when io.Copy fails, fixing the file descriptor leak. The partially-written temp file itself is still cleaned up by the caller's existing defer os.Remove(tempDataFilename) in Sync, which was already correct and untouched.
🤖 Prompt for AI agents
In server/vulnerabilities/nvd/tools/providers/nvd/cpe.go around line 201, review and complete this code-review fix: cpe.go download() ignores non-200 error body / does not clean up temp file on later errors within Sync.
What the draft fix changed: In download() (cpe.go), added `defer dataFile.Close()` immediately after the successful ioutil.TempFile call, ensuring the file descriptor is always closed on both the success path and the early-return error path when io.Copy fails, fixing the file descriptor leak. The partially-written temp file itself is still cleaned up by the caller's existing `defer os.Remove(tempDataFilename)` in Sync, which was already correct and untouched.
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 66 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.
tools/mdm/apple/loadtest/loadtest.go:204cmd/fleet/cron_test.go:363frontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/DeleteEntraClientIDModal/DeleteEntraClientIDModal.tsx:30frontend/pages/ApiOnlyUser/ApiOnlyUser.tsx:31frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsx:102frontend/pages/policies/ManagePoliciesPage/components/AutomationsModal/AutomationsModal.tsx:180server/datastore/mysql/migrations/tables/20251028140000_CreateTableOSVersionVulnerabilities.go:51server/goose/migrate_openframe_test.go:1server/mdm/nanomdm/storage/file/migrate.go:35server/vulnerabilities/nvd/tools/wfn/matcher.go:77tools/dibble/pkg/seed/profiles.go:122frontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/AddAbmModal/AddAbmModal.tsx:54server/service/apple_mdm_cmd_results.go:155server/datastore/mysql/migrations/tables/20260218175704_FMAActiveInstallers.go:26tools/fleet-mcp/auth.go:14frontend/pages/MfaPage/MfaPage.tsx:75server/datastore/mysql/migrations/tables/20250807140441_UpdateActivityTable.go:15server/datastore/mysql/migrations/tables/20260529091823_AddUpdateProfileSettingsTrackingTable.go:50server/datastore/mysql/migrations/tables/20260603120000_AddPollScheduleRelaxedToMDMWindowsEnrollments.go:37website/api/controllers/customers/get-stripe-checkout-session-url.js:46ee/server/service/embedded_scripts/linux_wipe.sh:32frontend/pages/hosts/details/cards/Activity/PastActivityFeed/PastActivityFeed.tsx:62server/activity/internal/types/activity.go:72server/datastore/mysql/migrations/tables/20231212094238_AddUniqueHashToSoftware.go:39server/datastore/mysql/migrations/tables/20250424153059_AddBatchScriptExecutionTables.go:27server/service/global_policies_test.go:162website/api/helpers/create-license-key.js:46server/datastore/mysql/migrations/tables/20260610172952_AddHasACMEPayloadToHostMDMAppleProfiles.go:25orbit/pkg/packaging/macos_rcodesign.go:75orbit/pkg/packaging/macos_rcodesign.go:17orbit/pkg/packaging/macos_rcodesign.go:67orbit/pkg/packaging/macos_rcodesign.go:75tools/snapshot/snapshot.go:163tools/snapshot/snapshot.go:252tools/snapshot/snapshot.go:148tools/snapshot/snapshot.go:285server/service/sessions.go:200server/service/sessions.go:195server/service/sessions.go:470server/service/sessions.go:144server/service/labels.go:210server/service/labels.go:341server/service/labels.go:115server/service/labels.go:211tools/android/android.go:220tools/android/android.go:200tools/android/android.go:75pkg/buildpkg/buildpkg.go:54pkg/buildpkg/buildpkg.go:64pkg/buildpkg/buildpkg.go:64tools/luks/luks/main.go:37tools/luks/luks/main.go:18tools/luks/luks/main.go:50server/service/client_live_query.go:137server/service/client_live_query.go:143server/service/client_live_query.go:79server/mdm/nanomdm/storage/allmulti/allmulti.go:80server/mdm/nanomdm/storage/allmulti/allmulti.go:51server/mdm/nanomdm/storage/allmulti/allmulti.go:108server/vulnerabilities/nvd/sync/cve_syncer.go:260server/vulnerabilities/nvd/sync/cve_syncer.go:204server/vulnerabilities/nvd/sync/cve_syncer.go:333server/vulnerabilities/nvd/tools/providers/nvd/cpe.go:169server/vulnerabilities/nvd/tools/providers/nvd/cpe.go:157server/vulnerabilities/nvd/tools/providers/nvd/cpe.go:201What 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:
3ba0fd37-c226-436b-ba81-b544a7d0a515Merging 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-86akbhhtv FleetMDM bulk review findings sweep (10 PRs)