fix(FLEETMDM-002): 19 review findings across 14 files - #125
fix(FLEETMDM-002): 19 review findings across 14 files#125flamingo[bot] wants to merge 14 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| savedAssets, err := ds.GetAllMDMConfigAssetsByName(context.Background(), expectedAssets, nil) | ||
| savedAssets, err := ds.GetAllMDMConfigAssetsByName(ctx, expectedAssets, nil) | ||
| if err != nil { | ||
| // allow not found errors as it means we're generating the assets for the first time. |
There was a problem hiding this comment.
🦩 🔴 ee/server package uses fmt.Errorf instead of ctxerr for error creation/wrapping
In initAssets, replaced fmt.Errorf("loading existing host identity assets from the database: %w", err) with ctxerr.Wrap(ctx, err, "loading existing host identity assets from the database"). This required changing the function signature to accept a context.Context parameter (replacing the internal context.Background() call) since ctxerr.Wrap needs a ctx; this is a signature change to the function which may require updating callers outside this file (not visible here), so the change is not fully verified as complete across the codebase.
🤖 Prompt for AI agents
In ee/server/service/hostidentity/config.go around line 20, review and complete this code-review fix: ee/server package uses fmt.Errorf instead of ctxerr for error creation/wrapping.
What the draft fix changed: In `initAssets`, replaced `fmt.Errorf("loading existing host identity assets from the database: %w", err)` with `ctxerr.Wrap(ctx, err, "loading existing host identity assets from the database")`. This required changing the function signature to accept a `context.Context` parameter (replacing the internal `context.Background()` call) since ctxerr.Wrap needs a ctx; this is a signature change to the function which may require updating callers outside this file (not visible here), so the change is not fully verified as complete across the codebase.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| ) | ||
| scepCert, scepKey, err := depot.NewCACertKey(caCert) | ||
| if err != nil { | ||
| return fmt.Errorf("generating host identity SCEP cert and key: %w", err) | ||
| return ctxerr.Wrap(ctx, err, "generating host identity SCEP cert and key") | ||
| } | ||
|
|
||
| // Store our config assets encrypted |
There was a problem hiding this comment.
🦩 🔴 ee/server package uses fmt.Errorf instead of ctxerr for SCEP cert generation error
In initAssets, replaced fmt.Errorf("generating host identity SCEP cert and key: %w", err) with ctxerr.Wrap(ctx, err, "generating host identity SCEP cert and key"), using the same ctx parameter introduced by the signature change described in note 1.
🤖 Prompt for AI agents
In ee/server/service/hostidentity/config.go around line 31, review and complete this code-review fix: ee/server package uses fmt.Errorf instead of ctxerr for SCEP cert generation error.
What the draft fix changed: In `initAssets`, replaced `fmt.Errorf("generating host identity SCEP cert and key: %w", err)` with `ctxerr.Wrap(ctx, err, "generating host identity SCEP cert and key")`, using the same `ctx` parameter introduced by the signature change described in note 1.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
| ) | ||
| scepCert, scepKey, err := depot.NewCACertKey(caCert) | ||
| if err != nil { | ||
| return fmt.Errorf("generating host identity SCEP cert and key: %w", err) | ||
| return ctxerr.Wrap(ctx, err, "generating host identity SCEP cert and key") | ||
| } | ||
|
|
||
| // Store our config assets encrypted |
There was a problem hiding this comment.
🦩 🔴 ee/server package uses fmt.Errorf instead of ctxerr for asset insert error
In initAssets, replaced fmt.Errorf("inserting host identity SCEP assets: %w", err) with ctxerr.Wrap(ctx, err, "inserting host identity SCEP assets"), and changed the ds.InsertMDMConfigAssets(context.Background(), ...) call to use the passed-in ctx instead of context.Background(), consistent with the finding's suggested fix and removing the now-unused fmt import.
🤖 Prompt for AI agents
In ee/server/service/hostidentity/config.go around line 44, review and complete this code-review fix: ee/server package uses fmt.Errorf instead of ctxerr for asset insert error.
What the draft fix changed: In `initAssets`, replaced `fmt.Errorf("inserting host identity SCEP assets: %w", err)` with `ctxerr.Wrap(ctx, err, "inserting host identity SCEP assets")`, and changed the `ds.InsertMDMConfigAssets(context.Background(), ...)` call to use the passed-in `ctx` instead of `context.Background()`, consistent with the finding's suggested fix and removing the now-unused `fmt` import.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
| // >>> OPENFRAME(msrc-empty-bulletin-guard): reject corrupted empty MSRC feeds instead of remediating all vulns — openframe/docs/msrc-guard.md | ||
| // Refuse to proceed if the loaded bulletin contains no vulnerability data — an empty | ||
| // bulletin would cause every existing MSRC OS vulnerability for this OS to be marked as | ||
| // remediated. This usually indicates the bulletin file was corrupted during download. |
There was a problem hiding this comment.
🦩 🔴 errors.New used instead of ctxerr.New in server-layer MSRC analyzer
In Analyze, replaced errors.New(...) with ctxerr.New(ctx, ...) for the empty-bulletin guard error, and removed the now-unused "errors" import from the import block, since ctx is already available in scope.
🤖 Prompt for AI agents
In server/vulnerabilities/msrc/analyzer.go around line 39, review and complete this code-review fix: errors.New used instead of ctxerr.New in server-layer MSRC analyzer.
What the draft fix changed: In `Analyze`, replaced `errors.New(...)` with `ctxerr.New(ctx, ...)` for the empty-bulletin guard error, and removed the now-unused `"errors"` import from the import block, since `ctx` is already available in scope.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -34,12 +33,14 @@ func Analyze( | |||
| return nil, err | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
🦩 🔴 New guard clause in upstream fleetdm/fleet file lacks OPENFRAME sentinel comments
Wrapped the empty-bulletin guard clause in Analyze with // >>> OPENFRAME(msrc-empty-bulletin-guard): ... — openframe/docs/msrc-guard.md and // <<< OPENFRAME(msrc-empty-bulletin-guard) sentinel comments, per the suggested fix format. The referenced doc file openframe/docs/msrc-guard.md is assumed but not verified to exist; a complete fix may require creating that documentation file.
🤖 Prompt for AI agents
In server/vulnerabilities/msrc/analyzer.go around line 36, review and complete this code-review fix: New guard clause in upstream fleetdm/fleet file lacks OPENFRAME sentinel comments.
What the draft fix changed: Wrapped the empty-bulletin guard clause in `Analyze` with `// >>> OPENFRAME(msrc-empty-bulletin-guard): ... — openframe/docs/msrc-guard.md` and `// <<< OPENFRAME(msrc-empty-bulletin-guard)` sentinel comments, per the suggested fix format. The referenced doc file `openframe/docs/msrc-guard.md` is assumed but not verified to exist; a complete fix may require creating that documentation file.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| if crt.Subject.CommonName == "" || len(crt.Subject.CommonName) > maxCommonNameLength { | ||
| return errors.New("common name empty or too long") | ||
| return ctxerr.New(context.Background(), "common name empty or too long") | ||
| } |
There was a problem hiding this comment.
🦩 🟠 Put() in depot.go returns errors.New/fmt.Errorf instead of ctxerr in server-layer code
In Put() (ee/server/service/hostidentity/depot/depot.go), replaced all errors.New(...) calls (common name check, serial number check, ECDSA public key check) and fmt.Errorf(...) calls (public key raw creation, existing certificate check) with ctxerr.New(context.Background(), ...) / ctxerr.Wrap(context.Background(), err, ...) respectively, routing these error paths through ctxerr as required. The pre-existing ctxerr.Errorf rate-limit call and the low-level DB errors returned directly from the sqlx transaction closure (which are wrapped/handled by callers per existing pattern) were left unchanged since the findings only cited the specific fmt.Errorf/errors.New lines.
🤖 Prompt for AI agents
In ee/server/service/hostidentity/depot/depot.go around line 90, review and complete this code-review fix: Put() in depot.go returns errors.New/fmt.Errorf instead of ctxerr in server-layer code.
What the draft fix changed: In Put() (ee/server/service/hostidentity/depot/depot.go), replaced all `errors.New(...)` calls (common name check, serial number check, ECDSA public key check) and `fmt.Errorf(...)` calls (public key raw creation, existing certificate check) with `ctxerr.New(context.Background(), ...)` / `ctxerr.Wrap(context.Background(), err, ...)` respectively, routing these error paths through ctxerr as required. The pre-existing `ctxerr.Errorf` rate-limit call and the low-level DB errors returned directly from the sqlx transaction closure (which are wrapped/handled by callers per existing pattern) were left unchanged since the findings only cited the specific fmt.Errorf/errors.New lines.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| WHERE host_uuid = :host_uuid AND certificate_template_id = :certificate_template_id` | ||
| result, err := sqlx.NamedExecContext(ctx, ds.writer(ctx), updateStmt, update) | ||
| if err != nil { | ||
| return err | ||
| return ctxerr.Wrap(ctx, err, "update host certificate template status") | ||
| } | ||
|
|
||
| rowsAffected, err := result.RowsAffected() | ||
| if err != nil { | ||
| return err | ||
| return ctxerr.Wrap(ctx, err, "get rows affected for host certificate template status update") | ||
| } | ||
|
|
||
| // If no records were updated, then insert a new status. |
There was a problem hiding this comment.
🦩 🟠 fmt.Errorf used instead of ctxerr in UpsertCertificateStatus
In UpsertCertificateStatus (server/datastore/mysql/host_certificate_templates.go), replaced the two bare return err statements following sqlx.NamedExecContext and result.RowsAffected() with return ctxerr.Wrap(ctx, err, "update host certificate template status") and return ctxerr.Wrap(ctx, err, "get rows affected for host certificate template status update") respectively, so all error returns in the function now go through ctxerr as required, matching the style of the pre-existing ctxerr.Wrap calls elsewhere in the same function.
(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/mysql/host_certificate_templates.go around line 300, review and complete this code-review fix: fmt.Errorf used instead of ctxerr in UpsertCertificateStatus.
What the draft fix changed: In `UpsertCertificateStatus` (server/datastore/mysql/host_certificate_templates.go), replaced the two bare `return err` statements following `sqlx.NamedExecContext` and `result.RowsAffected()` with `return ctxerr.Wrap(ctx, err, "update host certificate template status")` and `return ctxerr.Wrap(ctx, err, "get rows affected for host certificate template status update")` respectively, so all error returns in the function now go through ctxerr as required, matching the style of the pre-existing `ctxerr.Wrap` calls elsewhere in the same function.
_(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
| return ctxerr.New(ctx, "invalid job args") | ||
| } | ||
|
|
||
| var hosts []fleet.HostVulnerabilitySummary |
There was a problem hiding this comment.
🦩 🟠 runVuln returns a plain errors.New instead of ctxerr.New for a server-layer validation error
In runVuln (server/worker/zendesk.go), replaced return errors.New("invalid job args") with return ctxerr.New(ctx, "invalid job args"), and removed the now-unused errors import from the import block, since it was only used at that call site.
🤖 Prompt for AI agents
In server/worker/zendesk.go around line 273, review and complete this code-review fix: runVuln returns a plain errors.New instead of ctxerr.New for a server-layer validation error.
What the draft fix changed: In `runVuln` (server/worker/zendesk.go), replaced `return errors.New("invalid job args")` with `return ctxerr.New(ctx, "invalid job args")`, and removed the now-unused `errors` import from the import block, since it was only used at that call site.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -93,11 +94,11 @@ func (ds *Datastore) GetMDMSCEPCertBySerial(ctx context.Context, serialNumber ui | |||
| // The hash is calculated from cert.Raw (DER-encoded bytes), not the PEM string | |||
There was a problem hiding this comment.
🦩 🟠 fmt.Errorf used instead of ctxerr in host_identity_scep.go server package
In GetMDMSCEPCertBySerial (server/datastore/mysql/host_identity_scep.go), replaced errors.New("failed to decode PEM certificate") with ctxerr.New(ctx, "failed to decode PEM certificate") and fmt.Errorf("failed to parse certificate: %w", err) with ctxerr.Wrap(ctx, err, "parse certificate"). Added import for github.com/fleetdm/fleet/v4/server/contexts/ctxerr. The fmt and errors imports remain in use elsewhere in the file (fmt.Sprintf, errors.Is), so no import removal was needed.
🤖 Prompt for AI agents
In server/datastore/mysql/host_identity_scep.go around line 93, review and complete this code-review fix: fmt.Errorf used instead of ctxerr in host_identity_scep.go server package.
What the draft fix changed: In `GetMDMSCEPCertBySerial` (server/datastore/mysql/host_identity_scep.go), replaced `errors.New("failed to decode PEM certificate")` with `ctxerr.New(ctx, "failed to decode PEM certificate")` and `fmt.Errorf("failed to parse certificate: %w", err)` with `ctxerr.Wrap(ctx, err, "parse certificate")`. Added import for `github.com/fleetdm/fleet/v4/server/contexts/ctxerr`. The `fmt` and `errors` imports remain in use elsewhere in the file (fmt.Sprintf, errors.Is), so no import removal was needed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
|
|
||
| users, acts, err := action(ctx, opts.UUID) | ||
| if err != nil { | ||
| return err | ||
| return ctxerr.Wrap(ctx, err, "execute uuid action") | ||
| } | ||
| return t.createActivities(ctx, users, acts) | ||
| } |
There was a problem hiding this comment.
🦩 🟠 lifecycle.go: raw error returned without ctxerr wrapping in doWithUUIDValidation
In doWithUUIDValidation (server/mdm/lifecycle/lifecycle.go), changed return err to return ctxerr.Wrap(ctx, err, "execute uuid action") for the error returned by action(ctx, opts.UUID), matching the ctxerr-wrapping convention used elsewhere in the file. This is a mechanical, low-risk one-line change exactly matching the suggested fix.
🤖 Prompt for AI agents
In server/mdm/lifecycle/lifecycle.go around line 137, review and complete this code-review fix: lifecycle.go: raw error returned without ctxerr wrapping in doWithUUIDValidation.
What the draft fix changed: In `doWithUUIDValidation` (server/mdm/lifecycle/lifecycle.go), changed `return err` to `return ctxerr.Wrap(ctx, err, "execute uuid action")` for the error returned by `action(ctx, opts.UUID)`, matching the ctxerr-wrapping convention used elsewhere in the file. This is a mechanical, low-risk one-line change exactly matching the suggested fix.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
Closes 19 review findings across 14 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
ee/server/service/hostidentity/config.go:20ee/server/service/hostidentity/config.go:31ee/server/service/hostidentity/config.go:44server/vulnerabilities/msrc/analyzer.go:39server/vulnerabilities/msrc/analyzer.go:36ee/server/service/apple_mdm.go:58server/service/apple_mdm_batched.go:36server/vulnerabilities/macoffice/analyzer.go:171server/datastore/mysql/android_enterprises.go:65ee/server/service/calendar.go:27server/datastore/failing/common_store.go:17server/mdm/acme/internal/mysql/challenge.go:58server/mdm/acme/internal/mysql/challenge.go:58ee/server/service/hostidentity/depot/depot.go:56ee/server/service/hostidentity/depot/depot.go:90server/datastore/mysql/host_certificate_templates.go:300server/worker/zendesk.go:273server/datastore/mysql/host_identity_scep.go:93server/mdm/lifecycle/lifecycle.go:137What 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.