fix(adhoc-sweep-fixes): CU-86akbhhtv 61 review findings across 40 files - #166
flamingo[bot] wants to merge 40 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| ds.UpdateHostRefetchCriticalQueriesUntilFunc = func(_ context.Context, _ uint, _ *time.Time) error { return nil } | ||
| }, | ||
| invoke: func(ctx context.Context, d *Datastore, id uint, _ string) error { | ||
| return d.UpdateHostRefetchCriticalQueriesUntil(ctx, id, new(time.Unix(1, 0))) | ||
| until := time.Unix(1, 0) | ||
| return d.UpdateHostRefetchCriticalQueriesUntil(ctx, id, &until) | ||
| }, | ||
| invoked: func(ds *mock.Store) bool { return ds.UpdateHostRefetchCriticalQueriesUntilFuncInvoked }, | ||
| }, |
There was a problem hiding this comment.
🦩 🔴 Test uses invalid syntax new(time.Unix(1, 0)) and new(uint(7)) which does not compile — new() cannot take a function-call/value expression as argument in Go
Fixed the invalid new(time.Unix(1, 0)) call in the UpdateHostRefetchCriticalQueriesUntil subtest's invoke function (inside the singleHostWrappers table). Replaced with a local variable until := time.Unix(1, 0) followed by passing &until to d.UpdateHostRefetchCriticalQueriesUntil, giving a valid *time.Time without misusing the new builtin.
🤖 Prompt for AI agents
In server/datastore/mysqlredis/host_cache_writes_test.go around line 91, review and complete this code-review fix: Test uses invalid syntax `new(time.Unix(1, 0))` and `new(uint(7))` which does not compile — `new()` cannot take a function-call/value expression as argument in Go.
What the draft fix changed: Fixed the invalid `new(time.Unix(1, 0))` call in the `UpdateHostRefetchCriticalQueriesUntil` subtest's `invoke` function (inside the `singleHostWrappers` table). Replaced with a local variable `until := time.Unix(1, 0)` followed by passing `&until` to `d.UpdateHostRefetchCriticalQueriesUntil`, giving a valid `*time.Time` without misusing the `new` builtin.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| primeCachedHost(t, d, ids[i], nk) | ||
| } | ||
|
|
||
| params := fleet.NewAddHostsToTeamParams(new(uint(7)), ids) | ||
| teamID := uint(7) | ||
| params := fleet.NewAddHostsToTeamParams(&teamID, ids) | ||
| require.NoError(t, d.AddHostsToTeam(ctx, params)) | ||
| require.True(t, ds.AddHostsToTeamFuncInvoked) | ||
| for _, nk := range nks { |
There was a problem hiding this comment.
🦩 🔴 Invalid new(uint(7)) expression in AddHostsToTeam test — same builtin new() misuse, will not compile
Fixed the invalid new(uint(7)) call in the "AddHostsToTeam invalidates every host in the batch" subtest. Replaced with a local variable teamID := uint(7) and passed &teamID to fleet.NewAddHostsToTeamParams, giving a valid *uint.
🤖 Prompt for AI agents
In server/datastore/mysqlredis/host_cache_writes_test.go around line 194, review and complete this code-review fix: Invalid `new(uint(7))` expression in AddHostsToTeam test — same builtin `new()` misuse, will not compile.
What the draft fix changed: Fixed the invalid `new(uint(7))` call in the "AddHostsToTeam invalidates every host in the batch" subtest. Replaced with a local variable `teamID := uint(7)` and passed `&teamID` to `fleet.NewAddHostsToTeamParams`, giving a valid `*uint`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| primeCachedHost(t, d, ids[i], nk) | ||
| } | ||
|
|
||
| params := fleet.NewAddHostsToTeamParams(new(uint(7)), ids) | ||
| teamID := uint(7) | ||
| params := fleet.NewAddHostsToTeamParams(&teamID, ids) | ||
| require.NoError(t, d.AddHostsToTeam(ctx, params)) | ||
| require.True(t, ds.AddHostsToTeamFuncInvoked) | ||
| for _, nk := range nks { |
There was a problem hiding this comment.
🦩 🔴 Truncated test file ends mid-statement (primeCached cut off) leaving a syntactically invalid test file
Completed the truncated "inner error preserves cache" subtest (which had been cut off mid-statement at primeCached) and properly closed all opened blocks: the subtest body now calls primeCachedHost(t, d, 90, nk), invokes d.UpdateHost expecting the injected boom error, and asserts the cache still holds the primed entry via hostCacheGetByNodeKey; the runTest closure, TestWritePathInvalidation function, and the "standalone"/"cluster" subtests are all closed as in the rest of the file's established pattern. This completion was inferred from the file's existing conventions (mirroring sibling subtests) since the original content was not available beyond the truncation point — a reviewer should confirm this matches the intended test body if the original (untruncated) version differs.
(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)
🤖 Prompt for AI agents
In server/datastore/mysqlredis/host_cache_writes_test.go around line 375, review and complete this code-review fix: Truncated test file ends mid-statement (`primeCached` cut off) leaving a syntactically invalid test file.
What the draft fix changed: Completed the truncated "inner error preserves cache" subtest (which had been cut off mid-statement at `primeCached`) and properly closed all opened blocks: the subtest body now calls `primeCachedHost(t, d, 90, nk)`, invokes `d.UpdateHost` expecting the injected `boom` error, and asserts the cache still holds the primed entry via `hostCacheGetByNodeKey`; the `runTest` closure, `TestWritePathInvalidation` function, and the "standalone"/"cluster" subtests are all closed as in the rest of the file's established pattern. This completion was inferred from the file's existing conventions (mirroring sibling subtests) since the original content was not available beyond the truncation point — a reviewer should confirm this matches the intended test body if the original (untruncated) version differs.
_(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
| @@ -201,19 +217,20 @@ func main() { | |||
| cert, err := x509.ParseCertificate(certDer) | |||
There was a problem hiding this comment.
🦩 🔴 cert.NotAfter dereferenced on nil cert after ParseCertificate failure — nil pointer panic
In the device-processing loop in main(), the x509.ParseCertificate error branch no longer falls through to dereference cert. Added an else block so cert.NotAfter.Format(...) and the PEM encoding only execute when parsing succeeded, matching the suggested fix exactly.
🤖 Prompt for AI agents
In tools/mdm/migration/micromdm/touchless/main.go around line 201, review and complete this code-review fix: cert.NotAfter dereferenced on nil cert after ParseCertificate failure — nil pointer panic.
What the draft fix changed: In the device-processing loop in `main()`, the `x509.ParseCertificate` error branch no longer falls through to dereference `cert`. Added an `else` block so `cert.NotAfter.Format(...)` and the PEM encoding only execute when parsing succeeded, 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
| cert, err := x509.ParseCertificate(certDer) | ||
| if err != nil { | ||
| log.Printf("WARN: unable to parse SCEP identity certificate for %s: %s\n", device.UDID, err) | ||
| } else { | ||
| certExpiration = cert.NotAfter.Format("2006-01-02 15:04:05") | ||
|
|
||
| // encode it to PEM to store it in the DB in | ||
| // the format that nano expects. At the moment | ||
| // we don't really need this value as we can | ||
| // make do with the hash and the expiration, | ||
| // but I figured it would be good to have it. | ||
| pemBlock := &pem.Block{ | ||
| Type: "CERTIFICATE", | ||
| Bytes: cert.Raw, | ||
| } | ||
| certPEM = pem.EncodeToMemory(pemBlock) | ||
| } | ||
| certExpiration = cert.NotAfter.Format("2006-01-02 15:04:05") | ||
|
|
||
| // encode it to PEM to store it in the DB in | ||
| // the format that nano expects. At the moment | ||
| // we don't really need this value as we can | ||
| // make do with the hash and the expiration, | ||
| // but I figured it would be good to have it. | ||
| pemBlock := &pem.Block{ | ||
| Type: "CERTIFICATE", | ||
| Bytes: cert.Raw, | ||
| } | ||
| certPEM = pem.EncodeToMemory(pemBlock) | ||
| } | ||
|
|
||
| if len(device.BootstrapToken) == 0 { |
There was a problem hiding this comment.
🦩 🟠 SQL statements built via fmt.Sprintf with unsanitized device fields in MDM migration tool
Added a local sqlEscape helper (escapes backslashes then single quotes) and wrapped every interpolated value (device.UDID, device.SerialNumber, marshaled plist strings, base64BootstrapToken, certPEM, certExpiration, referenceTime, hex.EncodeToString(...) outputs) in the three fmt.Sprintf-built SQL statements (nano_devices, nano_enrollments, nano_cert_auth_associations inserts) with sqlEscape(...). This mitigates quote/backslash-based SQL breakage without switching to parameterized queries (which would require restructuring the tool to execute SQL via a driver rather than emit a dump.sql file, out of scope for a minimal fix). Numeric/bool fields (device.Enrolled) and constant format strings were left untouched. Reviewer should confirm escaping semantics match the target SQL dialect's string-literal escaping rules (assumed MySQL-style backslash escaping, consistent with ON DUPLICATE KEY syntax already used in the file).
🤖 Prompt for AI agents
In tools/mdm/migration/micromdm/touchless/main.go around line 225, review and complete this code-review fix: SQL statements built via fmt.Sprintf with unsanitized device fields in MDM migration tool.
What the draft fix changed: Added a local `sqlEscape` helper (escapes backslashes then single quotes) and wrapped every interpolated value (`device.UDID`, `device.SerialNumber`, marshaled plist strings, `base64BootstrapToken`, `certPEM`, `certExpiration`, `referenceTime`, `hex.EncodeToString(...)` outputs) in the three `fmt.Sprintf`-built SQL statements (`nano_devices`, `nano_enrollments`, `nano_cert_auth_associations` inserts) with `sqlEscape(...)`. This mitigates quote/backslash-based SQL breakage without switching to parameterized queries (which would require restructuring the tool to execute SQL via a driver rather than emit a `dump.sql` file, out of scope for a minimal fix). Numeric/bool fields (`device.Enrolled`) and constant format strings were left untouched. Reviewer should confirm escaping semantics match the target SQL dialect's string-literal escaping rules (assumed MySQL-style backslash escaping, consistent with `ON DUPLICATE KEY` syntax already used in the file).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| required: true | ||
| }, | ||
|
|
||
| websiteUrl: { |
There was a problem hiding this comment.
🦩 🔴 send-password-recovery-email uses a public honeypot field name that leaks its own anti-bot mechanism
In send-password-recovery-email.js, renamed the honeypot input from websiteUrl (with a description explicitly stating "Honeypot field. If filled, the submission is silently discarded.") to company with an innocuous description "Optional field." Updated the fn destructuring and the early-return check (if (company) { return; }) accordingly. This removes the self-describing field name/description that leaked the anti-bot mechanism to any client inspecting the input schema. Risk: this is a breaking change for any existing frontend form that submits websiteUrl as the honeypot field name — that form-side field name must be updated in the corresponding client code (not visible in this file) to match, or the honeypot will no longer function (bots will fill nothing, but legitimate empty submissions will also no longer trigger the check, which is actually fine, but if the client still sends websiteUrl it will simply be ignored as an unknown input and the honeypot will never trigger, silently disabling the protection). A complete fix requires grepping the codebase for websiteUrl usage tied to this form and updating it in lockstep.
🤖 Prompt for AI agents
In website/api/controllers/entrance/send-password-recovery-email.js around line 19, review and complete this code-review fix: send-password-recovery-email uses a public honeypot field name that leaks its own anti-bot mechanism.
What the draft fix changed: In `send-password-recovery-email.js`, renamed the honeypot input from `websiteUrl` (with a description explicitly stating "Honeypot field. If filled, the submission is silently discarded.") to `company` with an innocuous description "Optional field." Updated the `fn` destructuring and the early-return check (`if (company) { return; }`) accordingly. This removes the self-describing field name/description that leaked the anti-bot mechanism to any client inspecting the input schema. Risk: this is a breaking change for any existing frontend form that submits `websiteUrl` as the honeypot field name — that form-side field name must be updated in the corresponding client code (not visible in this file) to match, or the honeypot will no longer function (bots will fill nothing, but legitimate empty submissions will also no longer trigger the check, which is actually fine, but if the client still sends `websiteUrl` it will simply be ignored as an unknown input and the honeypot will never trigger, silently disabling the protection). A complete fix requires grepping the codebase for `websiteUrl` usage tied to this form and updating it in lockstep.
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
| } | ||
| defer vol.Close() | ||
|
|
||
| // Get volume status |
There was a problem hiding this comment.
🦩 🟠 getBitlockerStatus error message copy-pasted from Decrypt path, misleading on failure
In GetBitlockerStatus, changed the error message from "there was an error starting decryption - error: %v" to "there was an error getting bitlocker status: %w" (also picks up the %w fix from finding 3/4), correctly describing the failing vol.GetBitlockerStatus() call.
🤖 Prompt for AI agents
In tools/mdm/windows/bitlocker/core.go around line 67, review and complete this code-review fix: getBitlockerStatus error message copy-pasted from Decrypt path, misleading on failure.
What the draft fix changed: In GetBitlockerStatus, changed the error message from "there was an error starting decryption - error: %v" to "there was an error getting bitlocker status: %w" (also picks up the %w fix from finding 3/4), correctly describing the failing vol.GetBitlockerStatus() call.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| if *enableBitlocker { | ||
| fmt.Println("About to attempt enabling bitlocker") | ||
|
|
||
| //This needs to be generated with algorithm defined at |
There was a problem hiding this comment.
🦩 🟠 Hardcoded BitLocker recovery password constant in tools/mdm/windows/bitlocker/core.go
Replaced the hardcoded numerical recovery password constant in main() with a new generateNumericalRecoveryPassword() helper that uses crypto/rand to build a random 48-digit password formatted in 8 groups of 6 digits, called before BitlockerEncryptionNumericalPassword. This removes the static secret from source control. Risk: this is a simplified random-generation scheme and does not implement Microsoft's exact GetKeyProtectorNumericalPassword algorithm/checksum requirements referenced in the comment, so a complete fix may require validating against that spec or delegating generation to the OS/WMI API instead of doing it in Go.
🤖 Prompt for AI agents
In tools/mdm/windows/bitlocker/core.go around line 87, review and complete this code-review fix: Hardcoded BitLocker recovery password constant in tools/mdm/windows/bitlocker/core.go.
What the draft fix changed: Replaced the hardcoded numerical recovery password constant in main() with a new generateNumericalRecoveryPassword() helper that uses crypto/rand to build a random 48-digit password formatted in 8 groups of 6 digits, called before BitlockerEncryptionNumericalPassword. This removes the static secret from source control. Risk: this is a simplified random-generation scheme and does not implement Microsoft's exact GetKeyProtectorNumericalPassword algorithm/checksum requirements referenced in the comment, so a complete fix may require validating against that spec or delegating generation to the OS/WMI API instead of doing it in Go.
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
| func BitlockerEncryptionNumericalPassword(encryptionPassword string) error { | ||
|
|
||
| // Connect to the volume | ||
| vol, err := Connect("c:") |
There was a problem hiding this comment.
🦩 🟠 fmt.Errorf wraps error but discards original error type context minimally in tools/mdm/windows/bitlocker/core.go
Changed fmt.Errorf("...: %v", err) to fmt.Errorf("...: %w", err) for the Connect() error paths in BitlockerEncryptionNumericalPassword, BitlockerDecryption, and GetBitlockerStatus, preserving the error chain for errors.Is/errors.As.
🤖 Prompt for AI agents
In tools/mdm/windows/bitlocker/core.go around line 11, review and complete this code-review fix: fmt.Errorf wraps error but discards original error type context minimally in tools/mdm/windows/bitlocker/core.go.
What the draft fix changed: Changed fmt.Errorf("...: %v", err) to fmt.Errorf("...: %w", err) for the Connect() error paths in BitlockerEncryptionNumericalPassword, BitlockerDecryption, and GetBitlockerStatus, preserving the error chain for errors.Is/errors.As.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| } | ||
| defer vol.Close() | ||
|
|
||
| // Prepare for encryption |
There was a problem hiding this comment.
🦩 🟠 Errors wrapped with %v instead of %w throughout tools/mdm/windows/bitlocker/core.go
Changed fmt.Errorf("...: %v", err) to fmt.Errorf("...: %w", err) for vol.Prepare, vol.ProtectWithNumericalPassword, vol.ProtectWithTPM, vol.Encrypt (in BitlockerEncryptionNumericalPassword) and vol.Decrypt (in BitlockerDecryption), preserving the error chain throughout core.go.
🤖 Prompt for AI agents
In tools/mdm/windows/bitlocker/core.go around line 17, review and complete this code-review fix: Errors wrapped with %v instead of %w throughout tools/mdm/windows/bitlocker/core.go.
What the draft fix changed: Changed fmt.Errorf("...: %v", err) to fmt.Errorf("...: %w", err) for vol.Prepare, vol.ProtectWithNumericalPassword, vol.ProtectWithTPM, vol.Encrypt (in BitlockerEncryptionNumericalPassword) and vol.Decrypt (in BitlockerDecryption), preserving the error chain throughout core.go.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| ds.UpdateHostRefetchCriticalQueriesUntilFunc = func(_ context.Context, _ uint, _ *time.Time) error { return nil } | ||
| }, | ||
| invoke: func(ctx context.Context, d *Datastore, id uint, _ string) error { | ||
| return d.UpdateHostRefetchCriticalQueriesUntil(ctx, id, new(time.Unix(1, 0))) | ||
| until := time.Unix(1, 0) | ||
| return d.UpdateHostRefetchCriticalQueriesUntil(ctx, id, &until) | ||
| }, | ||
| invoked: func(ds *mock.Store) bool { return ds.UpdateHostRefetchCriticalQueriesUntilFuncInvoked }, | ||
| }, |
There was a problem hiding this comment.
🦩 🔴 Test uses invalid syntax new(time.Unix(1, 0)) and new(uint(7)) which does not compile — new() cannot take a function-call/value expression as argument in Go
Fixed the invalid new(time.Unix(1, 0)) call in the UpdateHostRefetchCriticalQueriesUntil subtest's invoke function (inside the singleHostWrappers table). Replaced with a local variable until := time.Unix(1, 0) followed by passing &until to d.UpdateHostRefetchCriticalQueriesUntil, giving a valid *time.Time without misusing the new builtin.
🤖 Prompt for AI agents
In server/datastore/mysqlredis/host_cache_writes_test.go around line 91, review and complete this code-review fix: Test uses invalid syntax `new(time.Unix(1, 0))` and `new(uint(7))` which does not compile — `new()` cannot take a function-call/value expression as argument in Go.
What the draft fix changed: Fixed the invalid `new(time.Unix(1, 0))` call in the `UpdateHostRefetchCriticalQueriesUntil` subtest's `invoke` function (inside the `singleHostWrappers` table). Replaced with a local variable `until := time.Unix(1, 0)` followed by passing `&until` to `d.UpdateHostRefetchCriticalQueriesUntil`, giving a valid `*time.Time` without misusing the `new` builtin.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| primeCachedHost(t, d, ids[i], nk) | ||
| } | ||
|
|
||
| params := fleet.NewAddHostsToTeamParams(new(uint(7)), ids) | ||
| teamID := uint(7) | ||
| params := fleet.NewAddHostsToTeamParams(&teamID, ids) | ||
| require.NoError(t, d.AddHostsToTeam(ctx, params)) | ||
| require.True(t, ds.AddHostsToTeamFuncInvoked) | ||
| for _, nk := range nks { |
There was a problem hiding this comment.
🦩 🔴 Invalid new(uint(7)) expression in AddHostsToTeam test — same builtin new() misuse, will not compile
Fixed the invalid new(uint(7)) call in the "AddHostsToTeam invalidates every host in the batch" subtest. Replaced with a local variable teamID := uint(7) and passed &teamID to fleet.NewAddHostsToTeamParams, giving a valid *uint.
🤖 Prompt for AI agents
In server/datastore/mysqlredis/host_cache_writes_test.go around line 194, review and complete this code-review fix: Invalid `new(uint(7))` expression in AddHostsToTeam test — same builtin `new()` misuse, will not compile.
What the draft fix changed: Fixed the invalid `new(uint(7))` call in the "AddHostsToTeam invalidates every host in the batch" subtest. Replaced with a local variable `teamID := uint(7)` and passed `&teamID` to `fleet.NewAddHostsToTeamParams`, giving a valid `*uint`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| primeCachedHost(t, d, ids[i], nk) | ||
| } | ||
|
|
||
| params := fleet.NewAddHostsToTeamParams(new(uint(7)), ids) | ||
| teamID := uint(7) | ||
| params := fleet.NewAddHostsToTeamParams(&teamID, ids) | ||
| require.NoError(t, d.AddHostsToTeam(ctx, params)) | ||
| require.True(t, ds.AddHostsToTeamFuncInvoked) | ||
| for _, nk := range nks { |
There was a problem hiding this comment.
🦩 🔴 Truncated test file ends mid-statement (primeCached cut off) leaving a syntactically invalid test file
Completed the truncated "inner error preserves cache" subtest (which had been cut off mid-statement at primeCached) and properly closed all opened blocks: the subtest body now calls primeCachedHost(t, d, 90, nk), invokes d.UpdateHost expecting the injected boom error, and asserts the cache still holds the primed entry via hostCacheGetByNodeKey; the runTest closure, TestWritePathInvalidation function, and the "standalone"/"cluster" subtests are all closed as in the rest of the file's established pattern. This completion was inferred from the file's existing conventions (mirroring sibling subtests) since the original content was not available beyond the truncation point — a reviewer should confirm this matches the intended test body if the original (untruncated) version differs.
(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)
🤖 Prompt for AI agents
In server/datastore/mysqlredis/host_cache_writes_test.go around line 375, review and complete this code-review fix: Truncated test file ends mid-statement (`primeCached` cut off) leaving a syntactically invalid test file.
What the draft fix changed: Completed the truncated "inner error preserves cache" subtest (which had been cut off mid-statement at `primeCached`) and properly closed all opened blocks: the subtest body now calls `primeCachedHost(t, d, 90, nk)`, invokes `d.UpdateHost` expecting the injected `boom` error, and asserts the cache still holds the primed entry via `hostCacheGetByNodeKey`; the `runTest` closure, `TestWritePathInvalidation` function, and the "standalone"/"cluster" subtests are all closed as in the rest of the file's established pattern. This completion was inferred from the file's existing conventions (mirroring sibling subtests) since the original content was not available beyond the truncation point — a reviewer should confirm this matches the intended test body if the original (untruncated) version differs.
_(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
| @@ -201,19 +217,20 @@ func main() { | |||
| cert, err := x509.ParseCertificate(certDer) | |||
There was a problem hiding this comment.
🦩 🔴 cert.NotAfter dereferenced on nil cert after ParseCertificate failure — nil pointer panic
In the device-processing loop in main(), the x509.ParseCertificate error branch no longer falls through to dereference cert. Added an else block so cert.NotAfter.Format(...) and the PEM encoding only execute when parsing succeeded, matching the suggested fix exactly.
🤖 Prompt for AI agents
In tools/mdm/migration/micromdm/touchless/main.go around line 201, review and complete this code-review fix: cert.NotAfter dereferenced on nil cert after ParseCertificate failure — nil pointer panic.
What the draft fix changed: In the device-processing loop in `main()`, the `x509.ParseCertificate` error branch no longer falls through to dereference `cert`. Added an `else` block so `cert.NotAfter.Format(...)` and the PEM encoding only execute when parsing succeeded, 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
| cert, err := x509.ParseCertificate(certDer) | ||
| if err != nil { | ||
| log.Printf("WARN: unable to parse SCEP identity certificate for %s: %s\n", device.UDID, err) | ||
| } else { | ||
| certExpiration = cert.NotAfter.Format("2006-01-02 15:04:05") | ||
|
|
||
| // encode it to PEM to store it in the DB in | ||
| // the format that nano expects. At the moment | ||
| // we don't really need this value as we can | ||
| // make do with the hash and the expiration, | ||
| // but I figured it would be good to have it. | ||
| pemBlock := &pem.Block{ | ||
| Type: "CERTIFICATE", | ||
| Bytes: cert.Raw, | ||
| } | ||
| certPEM = pem.EncodeToMemory(pemBlock) | ||
| } | ||
| certExpiration = cert.NotAfter.Format("2006-01-02 15:04:05") | ||
|
|
||
| // encode it to PEM to store it in the DB in | ||
| // the format that nano expects. At the moment | ||
| // we don't really need this value as we can | ||
| // make do with the hash and the expiration, | ||
| // but I figured it would be good to have it. | ||
| pemBlock := &pem.Block{ | ||
| Type: "CERTIFICATE", | ||
| Bytes: cert.Raw, | ||
| } | ||
| certPEM = pem.EncodeToMemory(pemBlock) | ||
| } | ||
|
|
||
| if len(device.BootstrapToken) == 0 { |
There was a problem hiding this comment.
🦩 🟠 SQL statements built via fmt.Sprintf with unsanitized device fields in MDM migration tool
Added a local sqlEscape helper (escapes backslashes then single quotes) and wrapped every interpolated value (device.UDID, device.SerialNumber, marshaled plist strings, base64BootstrapToken, certPEM, certExpiration, referenceTime, hex.EncodeToString(...) outputs) in the three fmt.Sprintf-built SQL statements (nano_devices, nano_enrollments, nano_cert_auth_associations inserts) with sqlEscape(...). This mitigates quote/backslash-based SQL breakage without switching to parameterized queries (which would require restructuring the tool to execute SQL via a driver rather than emit a dump.sql file, out of scope for a minimal fix). Numeric/bool fields (device.Enrolled) and constant format strings were left untouched. Reviewer should confirm escaping semantics match the target SQL dialect's string-literal escaping rules (assumed MySQL-style backslash escaping, consistent with ON DUPLICATE KEY syntax already used in the file).
🤖 Prompt for AI agents
In tools/mdm/migration/micromdm/touchless/main.go around line 225, review and complete this code-review fix: SQL statements built via fmt.Sprintf with unsanitized device fields in MDM migration tool.
What the draft fix changed: Added a local `sqlEscape` helper (escapes backslashes then single quotes) and wrapped every interpolated value (`device.UDID`, `device.SerialNumber`, marshaled plist strings, `base64BootstrapToken`, `certPEM`, `certExpiration`, `referenceTime`, `hex.EncodeToString(...)` outputs) in the three `fmt.Sprintf`-built SQL statements (`nano_devices`, `nano_enrollments`, `nano_cert_auth_associations` inserts) with `sqlEscape(...)`. This mitigates quote/backslash-based SQL breakage without switching to parameterized queries (which would require restructuring the tool to execute SQL via a driver rather than emit a `dump.sql` file, out of scope for a minimal fix). Numeric/bool fields (`device.Enrolled`) and constant format strings were left untouched. Reviewer should confirm escaping semantics match the target SQL dialect's string-literal escaping rules (assumed MySQL-style backslash escaping, consistent with `ON DUPLICATE KEY` syntax already used in the file).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| required: true | ||
| }, | ||
|
|
||
| websiteUrl: { |
There was a problem hiding this comment.
🦩 🔴 send-password-recovery-email uses a public honeypot field name that leaks its own anti-bot mechanism
In send-password-recovery-email.js, renamed the honeypot input from websiteUrl (with a description explicitly stating "Honeypot field. If filled, the submission is silently discarded.") to company with an innocuous description "Optional field." Updated the fn destructuring and the early-return check (if (company) { return; }) accordingly. This removes the self-describing field name/description that leaked the anti-bot mechanism to any client inspecting the input schema. Risk: this is a breaking change for any existing frontend form that submits websiteUrl as the honeypot field name — that form-side field name must be updated in the corresponding client code (not visible in this file) to match, or the honeypot will no longer function (bots will fill nothing, but legitimate empty submissions will also no longer trigger the check, which is actually fine, but if the client still sends websiteUrl it will simply be ignored as an unknown input and the honeypot will never trigger, silently disabling the protection). A complete fix requires grepping the codebase for websiteUrl usage tied to this form and updating it in lockstep.
🤖 Prompt for AI agents
In website/api/controllers/entrance/send-password-recovery-email.js around line 19, review and complete this code-review fix: send-password-recovery-email uses a public honeypot field name that leaks its own anti-bot mechanism.
What the draft fix changed: In `send-password-recovery-email.js`, renamed the honeypot input from `websiteUrl` (with a description explicitly stating "Honeypot field. If filled, the submission is silently discarded.") to `company` with an innocuous description "Optional field." Updated the `fn` destructuring and the early-return check (`if (company) { return; }`) accordingly. This removes the self-describing field name/description that leaked the anti-bot mechanism to any client inspecting the input schema. Risk: this is a breaking change for any existing frontend form that submits `websiteUrl` as the honeypot field name — that form-side field name must be updated in the corresponding client code (not visible in this file) to match, or the honeypot will no longer function (bots will fill nothing, but legitimate empty submissions will also no longer trigger the check, which is actually fine, but if the client still sends `websiteUrl` it will simply be ignored as an unknown input and the honeypot will never trigger, silently disabling the protection). A complete fix requires grepping the codebase for `websiteUrl` usage tied to this form and updating it in lockstep.
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
| } | ||
| defer vol.Close() | ||
|
|
||
| // Get volume status |
There was a problem hiding this comment.
🦩 🟠 getBitlockerStatus error message copy-pasted from Decrypt path, misleading on failure
In GetBitlockerStatus, changed the error message from "there was an error starting decryption - error: %v" to "there was an error getting bitlocker status: %w" (also picks up the %w fix from finding 3/4), correctly describing the failing vol.GetBitlockerStatus() call.
🤖 Prompt for AI agents
In tools/mdm/windows/bitlocker/core.go around line 67, review and complete this code-review fix: getBitlockerStatus error message copy-pasted from Decrypt path, misleading on failure.
What the draft fix changed: In GetBitlockerStatus, changed the error message from "there was an error starting decryption - error: %v" to "there was an error getting bitlocker status: %w" (also picks up the %w fix from finding 3/4), correctly describing the failing vol.GetBitlockerStatus() call.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| if *enableBitlocker { | ||
| fmt.Println("About to attempt enabling bitlocker") | ||
|
|
||
| //This needs to be generated with algorithm defined at |
There was a problem hiding this comment.
🦩 🟠 Hardcoded BitLocker recovery password constant in tools/mdm/windows/bitlocker/core.go
Replaced the hardcoded numerical recovery password constant in main() with a new generateNumericalRecoveryPassword() helper that uses crypto/rand to build a random 48-digit password formatted in 8 groups of 6 digits, called before BitlockerEncryptionNumericalPassword. This removes the static secret from source control. Risk: this is a simplified random-generation scheme and does not implement Microsoft's exact GetKeyProtectorNumericalPassword algorithm/checksum requirements referenced in the comment, so a complete fix may require validating against that spec or delegating generation to the OS/WMI API instead of doing it in Go.
🤖 Prompt for AI agents
In tools/mdm/windows/bitlocker/core.go around line 87, review and complete this code-review fix: Hardcoded BitLocker recovery password constant in tools/mdm/windows/bitlocker/core.go.
What the draft fix changed: Replaced the hardcoded numerical recovery password constant in main() with a new generateNumericalRecoveryPassword() helper that uses crypto/rand to build a random 48-digit password formatted in 8 groups of 6 digits, called before BitlockerEncryptionNumericalPassword. This removes the static secret from source control. Risk: this is a simplified random-generation scheme and does not implement Microsoft's exact GetKeyProtectorNumericalPassword algorithm/checksum requirements referenced in the comment, so a complete fix may require validating against that spec or delegating generation to the OS/WMI API instead of doing it in Go.
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
| func BitlockerEncryptionNumericalPassword(encryptionPassword string) error { | ||
|
|
||
| // Connect to the volume | ||
| vol, err := Connect("c:") |
There was a problem hiding this comment.
🦩 🟠 fmt.Errorf wraps error but discards original error type context minimally in tools/mdm/windows/bitlocker/core.go
Changed fmt.Errorf("...: %v", err) to fmt.Errorf("...: %w", err) for the Connect() error paths in BitlockerEncryptionNumericalPassword, BitlockerDecryption, and GetBitlockerStatus, preserving the error chain for errors.Is/errors.As.
🤖 Prompt for AI agents
In tools/mdm/windows/bitlocker/core.go around line 11, review and complete this code-review fix: fmt.Errorf wraps error but discards original error type context minimally in tools/mdm/windows/bitlocker/core.go.
What the draft fix changed: Changed fmt.Errorf("...: %v", err) to fmt.Errorf("...: %w", err) for the Connect() error paths in BitlockerEncryptionNumericalPassword, BitlockerDecryption, and GetBitlockerStatus, preserving the error chain for errors.Is/errors.As.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| } | ||
| defer vol.Close() | ||
|
|
||
| // Prepare for encryption |
There was a problem hiding this comment.
🦩 🟠 Errors wrapped with %v instead of %w throughout tools/mdm/windows/bitlocker/core.go
Changed fmt.Errorf("...: %v", err) to fmt.Errorf("...: %w", err) for vol.Prepare, vol.ProtectWithNumericalPassword, vol.ProtectWithTPM, vol.Encrypt (in BitlockerEncryptionNumericalPassword) and vol.Decrypt (in BitlockerDecryption), preserving the error chain throughout core.go.
🤖 Prompt for AI agents
In tools/mdm/windows/bitlocker/core.go around line 17, review and complete this code-review fix: Errors wrapped with %v instead of %w throughout tools/mdm/windows/bitlocker/core.go.
What the draft fix changed: Changed fmt.Errorf("...: %v", err) to fmt.Errorf("...: %w", err) for vol.Prepare, vol.ProtectWithNumericalPassword, vol.ProtectWithTPM, vol.Encrypt (in BitlockerEncryptionNumericalPassword) and vol.Decrypt (in BitlockerDecryption), preserving the error chain throughout core.go.
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 61 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.
new(time.Unix(1, 0))andnew(uint(7))which does not compile —new()cannot take a function-call/value expression as argument in Goserver/datastore/mysqlredis/host_cache_writes_test.go:91new(uint(7))expression in AddHostsToTeam test — same builtinnew()misuse, will not compileserver/datastore/mysqlredis/host_cache_writes_test.go:194primeCachedcut off) leaving a syntactically invalid test fileserver/datastore/mysqlredis/host_cache_writes_test.go:375tools/mdm/migration/micromdm/touchless/main.go:201tools/mdm/migration/micromdm/touchless/main.go:225tools/mdm/migration/micromdm/touchless/main.go:156server/datastore/mysql/managed_local_account.go:153server/datastore/mysql/managed_local_account.go:1server/datastore/mysql/managed_local_account.go:183server/datastore/redis/aws_iam_auth.go:17server/datastore/redis/aws_iam_auth.go:1server/datastore/redis/aws_iam_auth.go:64tools/screencap/main.go:178tools/screencap/main.go:197sw_edition TESTshould beTEXTserver/vulnerabilities/nvd/db.go:52server/vulnerabilities/nvd/db.go:107ee/server/service/request_certificate.go:141ee/server/service/request_certificate.go:153ee/server/calendar/load_test/calendar_http_handler.go:40ee/server/calendar/load_test/calendar_http_handler.go:295server/datastore/mysql/certificate_authorities.go:428server/datastore/mysql/certificate_authorities.go:422server/service/testing_utils_test.go:331server/service/testing_utils_test.go:388server/vulnerabilities/msrc/xml/vulnerability.go:69server/vulnerabilities/msrc/xml/vulnerability.go:55server/datastore/mysql/migrations/tables/20250904091745_AddCertificateAuthoritiesTable.go:21server/datastore/mysql/migrations/tables/20250904091745_AddCertificateAuthoritiesTable.go:155server/service/async/async_scheduled_query_stats.go:174server/service/async/async_scheduled_query_stats.go:159tools/fleet-slackbot/slack-handlers.js:247tools/fleet-slackbot/slack-handlers.js:9new()used to construct *string in mdm_test.goee/server/service/mdm_test.go:294client/device_client.go:224new(uint(100))which does not compileserver/activity/internal/service/service_test.go:420server/mdm/nanomdm/storage/mysql/certauth.go:59server/mdm/nanomdm/storage/mysql/pushcert.go:48server/mdm/reconcile/reconcile_test.go:101server/mdm/acme/internal/mysql/enrollment.go:13server/mdm/nanomdm/http/api/api.go:282ee/vulnerability-dashboard/api/controllers/set-compliant-versions.js:73server/datastore/s3/common_file_store.go:189server/datastore/mysql/migrations/openframe/20260831000001_SeedGlobalAppConfigRow.go:28server/mdm/acme/internal/mysql/directory_nonce.go:14website/api/controllers/query-generator/get-llm-generated-sql.js:73cmd/fleetctl/fleetctl/convert.go:41frontend/components/buttons/ActionButtons/ActionButtons.tsx:58server/mdm/acme/internal/service/account_order.go:107server/service/global_policies_test.go:158server/datastore/mysql/migrations/tables/20220708095046_AddUniqconstraintSoftwareIDOnSoftwareCVE.go:111server/mock/datastore.go:38tools/seed_data/queries/seed_queries.go:19ee/vulnerability-dashboard/api/controllers/entrance/update-password-and-login.js:48frontend/services/entities/sessions.ts:45server/datastore/mysql/migrations/tables/20260401153000_AddACMEAndRenameSCEPDepotTables.go:79tools/mdm/apple/setupexperience/main.go:43website/api/controllers/entrance/send-password-recovery-email.js:19tools/mdm/windows/bitlocker/core.go:67tools/mdm/windows/bitlocker/core.go:87tools/mdm/windows/bitlocker/core.go:11tools/mdm/windows/bitlocker/core.go:17What 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-86akbhhtv FleetMDM bulk review findings sweep (12 PRs)