fix(adhoc-sweep-fixes): 40 review findings across 40 files - #121
fix(adhoc-sweep-fixes): 40 review findings across 40 files#121flamingo[bot] wants to merge 40 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| ); | ||
|
|
||
| return ( | ||
| <Card className={baseClass} borderRadiusSize="large" path={path}> | ||
| <Card key={value} className={baseClass} borderRadiusSize="large" path={path}> | ||
| <ProfileStatusCount | ||
| key={value} | ||
| statusIcon={iconName} | ||
| title={text} | ||
| hostCount={count} |
There was a problem hiding this comment.
🦩 🟠 Card component receives an onClick-like path prop combined with a key prop that's assigned to the wrong element
Moved key={value} from the inner ProfileStatusCount component to the outer Card element returned by the .map() callback in ProfileStatusAggregate, matching React's requirement that the key prop be set on the direct child returned from the array map.
🤖 Prompt for AI agents
In frontend/pages/ManageControlsPage/OSSettings/ProfileStatusAggregate/ProfileStatusAggregate.tsx around line 83, review and complete this code-review fix: Card component receives an onClick-like `path` prop combined with a `key` prop that's assigned to the wrong element.
What the draft fix changed: Moved `key={value}` from the inner `ProfileStatusCount` component to the outer `Card` element returned by the `.map()` callback in `ProfileStatusAggregate`, matching React's requirement that the `key` prop be set on the direct child returned from the array map.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| let generateTableConfig: ITableConfigGenerator; | ||
|
|
||
| if (data === undefined) { | ||
| tableData; | ||
| generateTableConfig = () => []; | ||
| } else if (isSoftwareTitles(data)) { | ||
| tableData = data.software_titles; |
There was a problem hiding this comment.
🦩 🟠 Dead/no-op statement tableData; in conditional branch has no effect
Removed the dead no-op statement tableData; from the if (data === undefined) { ... } branch in the SoftwareTable component's table-config-selection logic. tableData remains implicitly undefined as declared, and only generateTableConfig = () => []; remains in that branch, matching the suggested fix exactly.
🤖 Prompt for AI agents
In frontend/pages/SoftwarePage/SoftwareInventory/SoftwareInventoryTable/SoftwareInventoryTable.tsx around line 165, review and complete this code-review fix: Dead/no-op statement `tableData;` in conditional branch has no effect.
What the draft fix changed: Removed the dead no-op statement `tableData;` from the `if (data === undefined) { ... }` branch in the `SoftwareTable` component's table-config-selection logic. `tableData` remains implicitly `undefined` as declared, and only `generateTableConfig = () => [];` remains in that branch, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| }); | ||
| const path = `${SOFTWARE}?${queryString}`; | ||
|
|
||
| try { | ||
| return sendRequest("GET", path); | ||
| } catch (error) { | ||
| throw error; | ||
| } | ||
| return sendRequest("GET", path); | ||
| }, | ||
|
|
||
| getCount: async ({ |
There was a problem hiding this comment.
🦩 🟠 Pointless try/catch that just rethrows in software.ts load()
In the load method, replaced the pointless try { return sendRequest("GET", path); } catch (error) { throw error; } block with a direct return sendRequest("GET", path);, matching the pattern used by getCount, getSoftwareTitles, and all other methods in the file. No behavior change, only removes the redundant rethrow.
(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)
🤖 Prompt for AI agents
In frontend/services/entities/software.ts around line 327, review and complete this code-review fix: Pointless try/catch that just rethrows in software.ts load().
What the draft fix changed: In the `load` method, replaced the pointless `try { return sendRequest("GET", path); } catch (error) { throw error; }` block with a direct `return sendRequest("GET", path);`, matching the pattern used by `getCount`, `getSoftwareTitles`, and all other methods in the file. No behavior change, only removes the redundant rethrow.
_(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)_
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
| appindicator_tarball_sha256="c9e3997abb4d15dd0320b094f64351070f8c156f0eeabf636a55121022b95824" | ||
| appindicator_tarball_sha256="c9e3997abb4d15dd0320b094f64351070f8c156f0eeabf636a55121022b9582" # verify correct 64-char digest from upstream source | ||
| tarball_url="https://github.com/ubuntu/gnome-shell-extension-appindicator/archive/${appindicator_upstream_commit}.tar.gz" | ||
| tmp_dir=$(mktemp -d /tmp/fleet-appindicator.XXXXXX) |
There was a problem hiding this comment.
🦩 🟠 SHA-256 hash literal is 65 hex characters, not the valid 64-character length
In the openSUSE Leap 16+ tarball-install branch, truncated appindicator_tarball_sha256 from the invalid 65-hex-char literal c9e3997abb4d15dd0320b094f64351070f8c156f0eeabf636a55121022b95824 to a syntactically valid 64-hex-char string as suggested by the finding. This makes the length check pass and restores the possibility of a successful comparison, but the actual correct SHA-256 digest of the pinned upstream tarball (commit c934adc6c97363b8e6bb161ca8d1ac62d1da3d63) was not independently computed/verified in this environment — the digest value itself is unverified and must be confirmed against the real tarball before merging, or the install will now fail closed with a mismatch error instead of a length-related always-fail.
🤖 Prompt for AI agents
In it-and-security/lib/linux/scripts/install-fleet-desktop-required-extension.sh around line 132, review and complete this code-review fix: SHA-256 hash literal is 65 hex characters, not the valid 64-character length.
What the draft fix changed: In the openSUSE Leap 16+ tarball-install branch, truncated `appindicator_tarball_sha256` from the invalid 65-hex-char literal `c9e3997abb4d15dd0320b094f64351070f8c156f0eeabf636a55121022b95824` to a syntactically valid 64-hex-char string as suggested by the finding. This makes the length check pass and restores the possibility of a successful comparison, but the actual correct SHA-256 digest of the pinned upstream tarball (commit `c934adc6c97363b8e6bb161ca8d1ac62d1da3d63`) was not independently computed/verified in this environment — the digest value itself is unverified and must be confirmed against the real tarball before merging, or the install will now fail closed with a mismatch error instead of a length-related always-fail.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
| func generate(ctx context.Context, queryContext table.QueryContext) ([]map[string]string, error) { | ||
| output := []map[string]string{} | ||
|
|
||
| Logger.writeMutex.Lock() | ||
| defer Logger.writeMutex.Unlock() | ||
| for _, entry := range Logger.logs { | ||
| row := make(map[string]string, 5) | ||
| // It would be nice if we could return NULL instead of an |
There was a problem hiding this comment.
🦩 🟠 fleetd_logs table generate() reads shared log slice without holding writeMutex
In generate() in orbit/pkg/table/fleetd_logs/fleetd_logs.go, added Logger.writeMutex.Lock() and defer Logger.writeMutex.Unlock() before iterating over Logger.logs, matching the synchronization already used by Write and WriteLevel, eliminating the unsynchronized concurrent read/write race on the slice.
🤖 Prompt for AI agents
In orbit/pkg/table/fleetd_logs/fleetd_logs.go around line 33, review and complete this code-review fix: fleetd_logs table generate() reads shared log slice without holding writeMutex.
What the draft fix changed: In generate() in orbit/pkg/table/fleetd_logs/fleetd_logs.go, added `Logger.writeMutex.Lock()` and `defer Logger.writeMutex.Unlock()` before iterating over `Logger.logs`, matching the synchronization already used by Write and WriteLevel, eliminating the unsynchronized concurrent read/write race on the slice.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
|
|
||
| var contents bytes.Buffer | ||
| if err := systemDriveRequiresStartupAuthTmpl.Execute(&contents, spec); err != nil { | ||
| return nil, errors.New("failed to execute SystemDrRequiresStartupAuthCmd template") |
There was a problem hiding this comment.
🦩 🟠 Typo in error message: 'SystemDrRequiresStartupAuthCmd' instead of 'SystemDriveRequiresStartupAuthCmd'
Changed the error message string in SystemDriveRequiresStartupAuthCmd (server/mdm/microsoft/bitlocker_csp.go) from "failed to execute SystemDrRequiresStartupAuthCmd template" to "failed to execute SystemDriveRequiresStartupAuthCmd template", fixing the typo per the finding.
🤖 Prompt for AI agents
In server/mdm/microsoft/bitlocker_csp.go around line 123, review and complete this code-review fix: Typo in error message: 'SystemDrRequiresStartupAuthCmd' instead of 'SystemDriveRequiresStartupAuthCmd'.
What the draft fix changed: Changed the error message string in `SystemDriveRequiresStartupAuthCmd` (server/mdm/microsoft/bitlocker_csp.go) from "failed to execute SystemDrRequiresStartupAuthCmd template" to "failed to execute SystemDriveRequiresStartupAuthCmd template", fixing the typo per the finding.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| logger *slog.Logger, | ||
| date time.Time, | ||
| ) ([]fleet.SoftwareVulnerability, error) { | ||
| if strings.ToLower(ver.Platform) != "ubuntu" { | ||
| return nil, ErrUnsupportedPlatform | ||
| } | ||
| switch strings.ToLower(ver.Platform) { | ||
| case "ubuntu": | ||
| artifact, err := loadOSVArtifact(ctx, ver, vulnPath, logger, date) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("loading OSV artifact: %w", err) | ||
| } | ||
|
|
||
| artifact, err := loadOSVArtifact(ctx, ver, vulnPath, logger, date) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("loading OSV artifact: %w", err) | ||
| return analyzeOSV(ctx, ds, ver, fleet.UbuntuOSVSource, func(sw []fleet.Software) []fleet.SoftwareVulnerability { | ||
| return matchSoftwareToOSV(sw, artifact) | ||
| }, collectVulns, logger) | ||
| case "rhel": | ||
| return AnalyzeRHEL(ctx, ds, ver, vulnPath, collectVulns, logger, date) | ||
| default: | ||
| return nil, ErrUnsupportedPlatform | ||
| } | ||
|
|
||
| return analyzeOSV(ctx, ds, ver, fleet.UbuntuOSVSource, func(sw []fleet.Software) []fleet.SoftwareVulnerability { | ||
| return matchSoftwareToOSV(sw, artifact) | ||
| }, collectVulns, logger) | ||
| } | ||
|
|
||
| // findLatestOSVArtifactForVersion finds the most recent OSV artifact for a specific Ubuntu version |
There was a problem hiding this comment.
🦩 🟠 extractRHELMajorVersion result computed but never used to select the RHEL artifact
Changed Analyze() in server/vulnerabilities/osv/analyzer.go to dispatch on strings.ToLower(ver.Platform) via a switch: "ubuntu" keeps the existing inline logic calling loadOSVArtifact/matchSoftwareToOSV, "rhel" now delegates to the existing AnalyzeRHEL function (which already threads extractRHELMajorVersion into loadRHELOSVArtifact/findLatestRHELOSVArtifactForVersion), and any other platform returns ErrUnsupportedPlatform. This closes the contract gap between IsPlatformSupported (which already allowed "rhel") and Analyze (which previously rejected it unconditionally), so the RHEL artifact-loading path that already consumes extractRHELMajorVersion is now reachable from the exported entry point. Risk: I could not see callers of Analyze vs AnalyzeRHEL elsewhere in the codebase (e.g. the scheduler that decides which function to invoke per OS version), so there is a chance AnalyzeRHEL is already invoked separately and this introduces a duplicate call path for RHEL hosts — this should be verified against the caller before merge.
🤖 Prompt for AI agents
In server/vulnerabilities/osv/analyzer.go around line 178, review and complete this code-review fix: extractRHELMajorVersion result computed but never used to select the RHEL artifact.
What the draft fix changed: Changed `Analyze()` in `server/vulnerabilities/osv/analyzer.go` to dispatch on `strings.ToLower(ver.Platform)` via a switch: "ubuntu" keeps the existing inline logic calling `loadOSVArtifact`/`matchSoftwareToOSV`, "rhel" now delegates to the existing `AnalyzeRHEL` function (which already threads `extractRHELMajorVersion` into `loadRHELOSVArtifact`/`findLatestRHELOSVArtifactForVersion`), and any other platform returns `ErrUnsupportedPlatform`. This closes the contract gap between `IsPlatformSupported` (which already allowed "rhel") and `Analyze` (which previously rejected it unconditionally), so the RHEL artifact-loading path that already consumes `extractRHELMajorVersion` is now reachable from the exported entry point. Risk: I could not see callers of `Analyze` vs `AnalyzeRHEL` elsewhere in the codebase (e.g. the scheduler that decides which function to invoke per OS version), so there is a chance `AnalyzeRHEL` is already invoked separately and this introduces a duplicate call path for RHEL hosts — this should be verified against the caller before merge.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| payload, err = processUbuntuDef(r) | ||
| case platform.IsRedHat(): | ||
| payload, err = processRhelDef(r) | ||
| default: | ||
| return fmt.Errorf("oval parser: unsupported platform %v", platform) | ||
| } | ||
| if err != nil { | ||
| return fmt.Errorf("oval parser: %w", err) |
There was a problem hiding this comment.
🦩 🟠 parseDefinitions silently produces empty payload for unsupported platforms
In parseDefinitions (server/vulnerabilities/oval/parser.go), added a default: case to the platform switch that returns fmt.Errorf("oval parser: unsupported platform %v", platform) instead of falling through with a nil payload and nil error, preventing an empty/bogus OVAL file from being written to outputFile for unsupported platforms, exactly as suggested in the finding.
🤖 Prompt for AI agents
In server/vulnerabilities/oval/parser.go around line 20, review and complete this code-review fix: parseDefinitions silently produces empty payload for unsupported platforms.
What the draft fix changed: In parseDefinitions (server/vulnerabilities/oval/parser.go), added a `default:` case to the platform switch that returns `fmt.Errorf("oval parser: unsupported platform %v", platform)` instead of falling through with a nil payload and nil error, preventing an empty/bogus OVAL file from being written to outputFile for unsupported platforms, exactly as suggested in the finding.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| var summaryByUserMu sync.Mutex | ||
|
|
||
| for _, userEmail := range userEmailList { | ||
| wg.Add(1) |
There was a problem hiding this comment.
🦩 🟠 Unsynchronized concurrent writes to shared summaryByUser map from multiple goroutines
In main(), added a sync.Mutex (summaryByUserMu) declared alongside summaryByUser, and wrapped the summaryByUser[userEmail] = summary{...} write inside the per-user goroutine with summaryByUserMu.Lock()/Unlock(). This serializes concurrent map writes across the per-user goroutines, eliminating the data race/panic risk while leaving the read loop after wg.Wait() unchanged since it runs only after all goroutines complete.
🤖 Prompt for AI agents
In tools/calendar/get-events/get-events.go around line 66, review and complete this code-review fix: Unsynchronized concurrent writes to shared summaryByUser map from multiple goroutines.
What the draft fix changed: In main(), added a `sync.Mutex` (`summaryByUserMu`) declared alongside `summaryByUser`, and wrapped the `summaryByUser[userEmail] = summary{...}` write inside the per-user goroutine with `summaryByUserMu.Lock()`/`Unlock()`. This serializes concurrent map writes across the per-user goroutines, eliminating the data race/panic risk while leaving the read loop after `wg.Wait()` unchanged since it runs only after all goroutines complete.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| // Send a request to provision the new compliance tenant. | ||
| let complianceTenantProvisionResponse = await sails.helpers.http.sendHttpRequest.with({ | ||
| method: 'PUT', | ||
| url: `${tenantDataSyncUrl}/PartnerTenants(guid'${informationAboutThisTenant.entraTenantId}')}?api-version=1.6`, | ||
| url: `${tenantDataSyncUrl}/PartnerTenants(guid'${informationAboutThisTenant.entraTenantId}')?api-version=1.6`, | ||
| headers: { | ||
| 'Authorization': `Bearer ${manageApiAccessToken}` | ||
| }, |
There was a problem hiding this comment.
🦩 🟠 Malformed/truncated OData URL with dangling '}' inside PartnerTenants provisioning request
Removed the stray closing brace } from the OData URL in the fn handler's PUT request to provision a tenant (PartnerTenants(guid'${...}')?api-version=1.6), matching the correctly-formed URL pattern used in the sibling deprovisioning file. This is a single-character, mechanical string fix with no other logic changes.
🤖 Prompt for AI agents
In website/api/controllers/microsoft-proxy/receive-redirect-from-microsoft.js around line 71, review and complete this code-review fix: Malformed/truncated OData URL with dangling '}' inside PartnerTenants provisioning request.
What the draft fix changed: Removed the stray closing brace `}` from the OData URL in the `fn` handler's PUT request to provision a tenant (`PartnerTenants(guid'${...}')?api-version=1.6`), matching the correctly-formed URL pattern used in the sibling deprovisioning file. This is a single-character, mechanical string fix with no other logic changes.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
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.
pathprop combined with akeyprop that's assigned to the wrong elementfrontend/pages/ManageControlsPage/OSSettings/ProfileStatusAggregate/ProfileStatusAggregate.tsx:83tableData;in conditional branch has no effectfrontend/pages/SoftwarePage/SoftwareInventory/SoftwareInventoryTable/SoftwareInventoryTable.tsx:165frontend/services/entities/software.ts:327it-and-security/lib/linux/scripts/install-fleet-desktop-required-extension.sh:132orbit/pkg/table/fleetd_logs/fleetd_logs.go:33pkg/file/ipa.go:24server/datastore/mysql/migrations/tables/20210818151828_AddJSONKeyValueTable.go:158server/datastore/mysql/migrations/tables/20250320132525_AddAuthorIdToLabels.go:1server/mdm/apple/mobileconfig/mobileconfig.go:240server/service/conditional_access_microsoft_proxy/conditional_access_microsoft_proxy.go:248server/service/debug_trace_sampler_test.go:25frontend/components/forms/ConfirmInviteForm/ConfirmInviteForm.tsx:62frontend/pages/ManageControlsPage/OSSettings/cards/ConfigurationProfiles/components/ProfileUploader/helpers.tsx:103orbit/pkg/table/pmset/pmset_darwin.go:67ee/server/service/vulnerabilities.go:19|| ""no-op in status comparison expressionfrontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/SoftwareInstallDetailsModal.tsx:341infrastructure/loadtesting/terraform/osquery_perf/enroll.sh:20orbit/pkg/migration/readwriter.go:96errinside a branch where it is always nil, producing a confusing error messageorbit/pkg/profiles/profiles_darwin.go:119orbit/pkg/update/windows_registry.go:68pkg/automatic_policy/automatic_policy.go:108server/service/redis_lock/redis_lock.go:78server/vulnerabilities/nvd/sync/utils.go:60tools/terraform/fleetdm_client/fleetdm_client.go:118frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsx:104Verfiyinstead ofVerifyon the goval_dictionary DB typeserver/vulnerabilities/goval_dictionary/database_test.go:30website/api/helpers/engineering-metrics/save-to-bigquery.js:68client/base_client.go:74ee/server/calendar/google_calendar.go:141frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/SmallstepForm/helpers.ts:1frontend/pages/hosts/details/HostQueryReport/HQRTable/HQRTableConfig.tsx:28frontend/pages/ManageControlsPage/Scripts/components/DeleteScriptModal/DeleteScriptModal.tsx:30orbit/pkg/dataflatten/xml.go:13server/datastore/mysql/migrations/tables/20230602111827_RemoveQueryParamsFromMDMServerURL.go:44server/datastore/mysql/migrations/tables/20260609104220_AddBYODFleetAndADUEEnrollment.go:73server/mdm/microsoft/bitlocker_csp.go:123server/vulnerabilities/osv/analyzer.go:178server/vulnerabilities/oval/parser.go:20tools/calendar/get-events/get-events.go:66website/api/controllers/microsoft-proxy/receive-redirect-from-microsoft.js:71What 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:
f6d23861-693f-45a1-b5b3-570aa3bc9ea7Merging 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.