fix(FLEETMDM-001): 16 review findings across 13 files - #126
fix(FLEETMDM-001): 16 review findings across 13 files#126flamingo[bot] wants to merge 12 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
There was a problem hiding this comment.
🦩 What this fix changed, finding by finding
16 finding(s) fixed in this draft — 15 explained inline on the diff.
Fixes without an inline anchor in this diff
🔴 9. deploymentAnnotations inline OPENFRAME comment lacks matching closing sentinel — charts/fleet/values.yaml:12
Replaced the inline deploymentAnnotations: {} # ... # OPENFRAME(helm): ... single-line comment with a proper bracketed sentinel pair: added # >>> OPENFRAME(helm): fork-added Deployment annotations — openframe/docs/helm-chart.md on the line above deploymentAnnotations: {}, restored the original trailing # Additional annotations to add to the Fleet Deployment comment on the deploymentAnnotations line itself, and added # <<< OPENFRAME(helm) on the line immediately after, matching the >>> OPENFRAME(...) / <<< OPENFRAME(...) convention used elsewhere in this file.
🤖 Prompt for AI agents
In charts/fleet/values.yaml around line 12, review and complete this code-review fix: deploymentAnnotations inline OPENFRAME comment lacks matching closing sentinel.
What the draft fix changed: Replaced the inline `deploymentAnnotations: {} # ... # OPENFRAME(helm): ...` single-line comment with a proper bracketed sentinel pair: added `# >>> OPENFRAME(helm): fork-added Deployment annotations — openframe/docs/helm-chart.md` on the line above `deploymentAnnotations: {}`, restored the original trailing `# Additional annotations to add to the Fleet Deployment` comment on the `deploymentAnnotations` line itself, and added `# <<< OPENFRAME(helm)` on the line immediately after, matching the `>>> OPENFRAME(...)` / `<<< OPENFRAME(...)` convention used elsewhere in this file.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| decoded.PostInstallScript = &decodedScript | ||
| } | ||
| } | ||
| // <<< OPENFRAME(waf-bypass-scripts) | ||
|
|
||
| return &decoded, nil | ||
| } |
There was a problem hiding this comment.
🦩 🔴 Base64 script-encoding bypass logic added to upstream software_installers.go without OPENFRAME sentinel comments
In updateSoftwareInstallerRequest.DecodeRequest, wrapped the existing isScriptsEncoded(r) / decodeBase64Script(...) block (covering install_script, uninstall_script, pre_install_query, post_install_script) with // >>> OPENFRAME(waf-bypass-scripts): base64-encode scripts to avoid WAF pattern blocks — openframe/docs/waf-bypass.md and // <<< OPENFRAME(waf-bypass-scripts) sentinel comment lines, exactly as suggested. No logic was altered, only sentinel comments added around the pre-existing code block.
🤖 Prompt for AI agents
In server/service/software_installers.go around line 236, review and complete this code-review fix: Base64 script-encoding bypass logic added to upstream software_installers.go without OPENFRAME sentinel comments.
What the draft fix changed: In `updateSoftwareInstallerRequest.DecodeRequest`, wrapped the existing `isScriptsEncoded(r)` / `decodeBase64Script(...)` block (covering install_script, uninstall_script, pre_install_query, post_install_script) with `// >>> OPENFRAME(waf-bypass-scripts): base64-encode scripts to avoid WAF pattern blocks — openframe/docs/waf-bypass.md` and `// <<< OPENFRAME(waf-bypass-scripts)` sentinel comment lines, exactly as suggested. No logic was altered, only sentinel comments added around the pre-existing code block.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| decoded.AutomaticInstall = parsed | ||
| } | ||
|
|
||
| // >>> OPENFRAME(waf-bypass-scripts): base64-encode scripts to avoid WAF pattern blocks — openframe/docs/waf-bypass.md | ||
| // Check if scripts are base64 encoded (to bypass WAF rules that block script patterns) | ||
| if isScriptsEncoded(r) { | ||
| var err error |
There was a problem hiding this comment.
🦩 🔴 Same base64-decoding fork addition unwrapped in uploadSoftwareInstallerRequest.DecodeRequest
In uploadSoftwareInstallerRequest.DecodeRequest, wrapped the second identical isScriptsEncoded(r) / decodeBase64Script(...) block (install_script, uninstall_script, pre_install_query, post_install_script) with the same // >>> OPENFRAME(waf-bypass-scripts): ... / // <<< OPENFRAME(waf-bypass-scripts) sentinel pair, using the identical slug so both occurrences of this fork-only WAF-bypass logic are tagged consistently for upstream sync tooling. No logic changed.
🤖 Prompt for AI agents
In server/service/software_installers.go around line 335, review and complete this code-review fix: Same base64-decoding fork addition unwrapped in uploadSoftwareInstallerRequest.DecodeRequest.
What the draft fix changed: In `uploadSoftwareInstallerRequest.DecodeRequest`, wrapped the second identical `isScriptsEncoded(r)` / `decodeBase64Script(...)` block (install_script, uninstall_script, pre_install_query, post_install_script) with the same `// >>> OPENFRAME(waf-bypass-scripts): ...` / `// <<< OPENFRAME(waf-bypass-scripts)` sentinel pair, using the identical slug so both occurrences of this fork-only WAF-bypass logic are tagged consistently for upstream sync tooling. No logic changed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| func (m *Store) Drop() error { return nil } | ||
| func (m *Store) MigrateTables(ctx context.Context) error { return nil } | ||
| func (m *Store) MigrateData(ctx context.Context) error { return nil } | ||
|
|
||
| // >>> OPENFRAME(mock-migrate-openframe): add MigrateOpenframe mock stub for OpenFrame migration hook — openframe/docs/datastore.md | ||
| func (m *Store) MigrateOpenframe(ctx context.Context) error { return nil } | ||
|
|
||
| // <<< OPENFRAME(mock-migrate-openframe) | ||
| func (m *Store) MigrationStatus(ctx context.Context) (*fleet.MigrationStatus, error) { | ||
| return &fleet.MigrationStatus{}, nil | ||
| } | ||
| func (m *Store) Name() string { return "mock" } | ||
|
|
There was a problem hiding this comment.
🦩 🔴 MigrateOpenframe addition to upstream server/mock/datastore.go lacks required OPENFRAME sentinel comments
In server/mock/datastore.go, wrapped the MigrateOpenframe method (near the MigrateTables/MigrateData/MigrationStatus group) with // >>> OPENFRAME(mock-migrate-openframe): ... and // <<< OPENFRAME(mock-migrate-openframe) sentinel comments per FLEETMDM-001, using the suggested slug and description so future upstream syncs can locate and preserve this fork-specific addition.
🤖 Prompt for AI agents
In server/mock/datastore.go around line 24, review and complete this code-review fix: MigrateOpenframe addition to upstream server/mock/datastore.go lacks required OPENFRAME sentinel comments.
What the draft fix changed: In `server/mock/datastore.go`, wrapped the `MigrateOpenframe` method (near the `MigrateTables`/`MigrateData`/`MigrationStatus` group) with `// >>> OPENFRAME(mock-migrate-openframe): ...` and `// <<< OPENFRAME(mock-migrate-openframe)` sentinel comments per FLEETMDM-001, using the suggested slug and description so future upstream syncs can locate and preserve this fork-specific addition.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| func (m *Store) Drop() error { return nil } | ||
| func (m *Store) MigrateTables(ctx context.Context) error { return nil } | ||
| func (m *Store) MigrateData(ctx context.Context) error { return nil } | ||
|
|
||
| // >>> OPENFRAME(mock-migrate-openframe): add MigrateOpenframe mock stub for OpenFrame migration hook — openframe/docs/datastore.md | ||
| func (m *Store) MigrateOpenframe(ctx context.Context) error { return nil } | ||
|
|
||
| // <<< OPENFRAME(mock-migrate-openframe) | ||
| func (m *Store) MigrationStatus(ctx context.Context) (*fleet.MigrationStatus, error) { | ||
| return &fleet.MigrationStatus{}, nil | ||
| } | ||
| func (m *Store) Name() string { return "mock" } | ||
|
|
There was a problem hiding this comment.
🦩 🔵 server/mock/datastore.go MigrateOpenframe method inconsistently indented/formatted, likely a fork-specific addition without sentinel wrapping
Same location (MigrateOpenframe in server/mock/datastore.go): the sentinel wrapping added for finding 1 also addresses this duplicate finding about missing sentinels/ad-hoc formatting. I did not further realign the method with the other one-liners (e.g. adjusting whitespace to match Drop/MigrateTables column alignment) since that would be a cosmetic reformat beyond the sentinel requirement; if strict visual alignment is also desired, that is a separate minimal follow-up.
🤖 Prompt for AI agents
In server/mock/datastore.go around line 24, review and complete this code-review fix: server/mock/datastore.go MigrateOpenframe method inconsistently indented/formatted, likely a fork-specific addition without sentinel wrapping.
What the draft fix changed: Same location (`MigrateOpenframe` in `server/mock/datastore.go`): the sentinel wrapping added for finding 1 also addresses this duplicate finding about missing sentinels/ad-hoc formatting. I did not further realign the method with the other one-liners (e.g. adjusting whitespace to match `Drop`/`MigrateTables` column alignment) since that would be a cosmetic reformat beyond the sentinel requirement; if strict visual alignment is also desired, that is a separate minimal follow-up.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| ) | ||
|
|
||
| // >>> OPENFRAME(msrc-display-version): match hosts missing DisplayVersion via OS name fallback — openframe/docs/msrc-matching.md | ||
| // displayVersionPattern matches Windows display version strings like "22H2", "23H2", "24H2". |
There was a problem hiding this comment.
🦩 🔴 MSRC display-version fallback logic added to upstream fleetdm product.go without OPENFRAME sentinel comments
Wrapped all fork-specific MSRC display-version fallback logic in server/vulnerabilities/msrc/parsed/product.go with // >>> OPENFRAME(msrc-display-version): ... — openframe/docs/msrc-matching.md / // <<< OPENFRAME(msrc-display-version) sentinel comment pairs, as required. This covers: the displayVersionPattern regex var declaration; the isServerCoreHost/installationTypeKnown/matchByDisplayVersion/matchByBuildNumber variable block and installation-type/isCore matching refinements inside GetMatchForOS; the display-version fallback lookup and build-number fallback assignment inside the GetMatchForOS loop; the final match-selection return logic at the end of GetMatchForOS; and the IsServerCore() method plus extractDisplayVersionFromName helper function. No logic was altered, only comments added, preserving existing behavior and formatting. Confidence is not higher because the referenced doc file openframe/docs/msrc-matching.md is not verified to exist — creating it is outside this file's scope, and a complete fix should confirm/create that documentation.
🤖 Prompt for AI agents
In server/vulnerabilities/msrc/parsed/product.go around line 14, review and complete this code-review fix: MSRC display-version fallback logic added to upstream fleetdm product.go without OPENFRAME sentinel comments.
What the draft fix changed: Wrapped all fork-specific MSRC display-version fallback logic in `server/vulnerabilities/msrc/parsed/product.go` with `// >>> OPENFRAME(msrc-display-version): ... — openframe/docs/msrc-matching.md` / `// <<< OPENFRAME(msrc-display-version)` sentinel comment pairs, as required. This covers: the `displayVersionPattern` regex var declaration; the `isServerCoreHost`/`installationTypeKnown`/`matchByDisplayVersion`/`matchByBuildNumber` variable block and installation-type/isCore matching refinements inside `GetMatchForOS`; the display-version fallback lookup and build-number fallback assignment inside the `GetMatchForOS` loop; the final match-selection return logic at the end of `GetMatchForOS`; and the `IsServerCore()` method plus `extractDisplayVersionFromName` helper function. No logic was altered, only comments added, preserving existing behavior and formatting. Confidence is not higher because the referenced doc file `openframe/docs/msrc-matching.md` is not verified to exist — creating it is outside this file's scope, and a complete fix should confirm/create that documentation.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 82 medium — react 👍/👎 to teach the reviewer
| } | ||
|
|
||
| func (t *Task) collectLabelQueryExecutions(ctx context.Context, ds fleet.Datastore, pool fleet.RedisPool, stats *collectorExecStats) error { | ||
| // >>> OPENFRAME(async-otel): add OTEL span for label collection task — openframe/docs/observability.md | ||
| // Create a root span for this async collection task if OTEL is enabled | ||
| if t.otelEnabled { | ||
| tracer := otel.Tracer("async") |
There was a problem hiding this comment.
🦩 🔴 async_label.go: OTEL span instrumentation added to upstream-shared file without OPENFRAME sentinel comments
In collectLabelQueryExecutions, wrapped the existing OTEL span creation block (the if t.otelEnabled { ... } block using otel.Tracer, trace.Span, and attribute.String) with the required // >>> OPENFRAME(async-otel): add OTEL span for label collection task — openframe/docs/observability.md and // <<< OPENFRAME(async-otel) sentinel comments, exactly as suggested in the finding, with no other code changes.
🤖 Prompt for AI agents
In server/service/async/async_label.go around line 98, review and complete this code-review fix: async_label.go: OTEL span instrumentation added to upstream-shared file without OPENFRAME sentinel comments.
What the draft fix changed: In `collectLabelQueryExecutions`, wrapped the existing OTEL span creation block (the `if t.otelEnabled { ... }` block using `otel.Tracer`, `trace.Span`, and `attribute.String`) with the required `// >>> OPENFRAME(async-otel): add OTEL span for label collection task — openframe/docs/observability.md` and `// <<< OPENFRAME(async-otel)` sentinel comments, exactly as suggested in the finding, with no other code changes.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| } | ||
|
|
||
| func (t *Task) collectPolicyQueryExecutions(ctx context.Context, ds fleet.Datastore, pool fleet.RedisPool, stats *collectorExecStats) error { | ||
| // Create a root span for this async collection task if OTEL is enabled | ||
| // >>> OPENFRAME(async-otel): add OTEL span for policy collection task — openframe/docs/observability.md | ||
| if t.otelEnabled { | ||
| tracer := otel.Tracer("async") | ||
| var span trace.Span |
There was a problem hiding this comment.
🦩 🔴 async_policy.go: OTEL span instrumentation added to upstream-shared file without OPENFRAME sentinel comments
In collectPolicyQueryExecutions, wrapped the existing if t.otelEnabled { ... } OTEL span block with // >>> OPENFRAME(async-otel): ... and // <<< OPENFRAME(async-otel) sentinel comments, matching the suggested fix exactly. No functional code was altered, only comments added around the fork-added block.
🤖 Prompt for AI agents
In server/service/async/async_policy.go around line 122, review and complete this code-review fix: async_policy.go: OTEL span instrumentation added to upstream-shared file without OPENFRAME sentinel comments.
What the draft fix changed: In `collectPolicyQueryExecutions`, wrapped the existing `if t.otelEnabled { ... }` OTEL span block with `// >>> OPENFRAME(async-otel): ...` and `// <<< OPENFRAME(async-otel)` sentinel comments, matching the suggested fix exactly. No functional code was altered, only comments added around the fork-added block.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -22,6 +22,7 @@ import ( | |||
| "github.com/go-kit/kit/endpoint" | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
🦩 🟠 extractCertSerialFromHeader and iDevice URL auth fallback lack OPENFRAME sentinel comments
Wrapped extractCertSerialFromHeader (including its doc comment) in server/service/endpoint_middleware.go with // >>> OPENFRAME(idevice-cert-auth): cert-serial extraction for iOS/iPadOS mTLS device auth — openframe/docs/idevice-cert-auth.md before the function and // <<< OPENFRAME(idevice-cert-auth) immediately after, per FLEETMDM-001 sentinel convention.
🤖 Prompt for AI agents
In server/service/endpoint_middleware.go around line 24, review and complete this code-review fix: extractCertSerialFromHeader and iDevice URL auth fallback lack OPENFRAME sentinel comments.
What the draft fix changed: Wrapped extractCertSerialFromHeader (including its doc comment) in server/service/endpoint_middleware.go with `// >>> OPENFRAME(idevice-cert-auth): cert-serial extraction for iOS/iPadOS mTLS device auth — openframe/docs/idevice-cert-auth.md` before the function and `// <<< OPENFRAME(idevice-cert-auth)` immediately after, per FLEETMDM-001 sentinel convention.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| @@ -76,6 +79,7 @@ func authenticatedDevice(svc fleet.Service, logger *slog.Logger, next endpoint.E | |||
| var debug bool | |||
There was a problem hiding this comment.
🦩 🟠 Cert/token/URL authentication branching in authenticatedDevice is unwrapped fork logic
Wrapped the cert/token/URL three-way authentication branch inside authenticatedDevice with // >>> OPENFRAME(idevice-cert-auth): cert/token/URL authentication branching for iOS/iPadOS device auth — openframe/docs/idevice-cert-auth.md and // <<< OPENFRAME(idevice-cert-auth) sentinels, matching the same slug used in finding 1 since both concern the same fork feature. The referenced doc file (openframe/docs/idevice-cert-auth.md) is assumed/synthesized and should be confirmed to exist or be created by the reviewer.
🤖 Prompt for AI agents
In server/service/endpoint_middleware.go around line 76, review and complete this code-review fix: Cert/token/URL authentication branching in authenticatedDevice is unwrapped fork logic.
What the draft fix changed: Wrapped the cert/token/URL three-way authentication branch inside authenticatedDevice with `// >>> OPENFRAME(idevice-cert-auth): cert/token/URL authentication branching for iOS/iPadOS device auth — openframe/docs/idevice-cert-auth.md` and `// <<< OPENFRAME(idevice-cert-auth)` sentinels, matching the same slug used in finding 1 since both concern the same fork feature. The referenced doc file (openframe/docs/idevice-cert-auth.md) is assumed/synthesized and should be confirmed to exist or be created by the reviewer.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
|
|
||
| // DecodeRequest implements the RequestDecoder interface to support base64-encoded | ||
| // script fields. This allows bypassing WAF rules that may block requests containing | ||
| // shell/PowerShell script patterns. When the X-Fleet-Scripts-Encoded header is set |
There was a problem hiding this comment.
🦩 🟠 New base64-script-decoding feature in shared upstream file lacks OPENFRAME sentinel comments
Wrapped the DecodeRequest method on addFleetMaintainedAppRequest (including the fleet_id/team_id aliasing logic and the base64 script-decoding block) with // >>> OPENFRAME(maintained-apps-decode-request): ... — openframe/docs/FLEETMDM-001.md and // <<< OPENFRAME(maintained-apps-decode-request) sentinel comments, matching the pattern used for verifyHostsToAssociate in labels_util.go. The doc reference points at FLEETMDM-001.md per the finding; if that file/slug doesn't exist yet in openframe/docs, it should be created or the slug adjusted to match project convention.
🤖 Prompt for AI agents
In server/service/maintained_apps.go around line 36, review and complete this code-review fix: New base64-script-decoding feature in shared upstream file lacks OPENFRAME sentinel comments.
What the draft fix changed: Wrapped the `DecodeRequest` method on `addFleetMaintainedAppRequest` (including the fleet_id/team_id aliasing logic and the base64 script-decoding block) with `// >>> OPENFRAME(maintained-apps-decode-request): ... — openframe/docs/FLEETMDM-001.md` and `// <<< OPENFRAME(maintained-apps-decode-request)` sentinel comments, matching the pattern used for `verifyHostsToAssociate` in labels_util.go. The doc reference points at FLEETMDM-001.md per the finding; if that file/slug doesn't exist yet in openframe/docs, it should be created or the slug adjusted to match project convention.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
Closes 16 review findings across 13 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
server/service/software_installers.go:236server/service/software_installers.go:335server/mock/datastore.go:24server/mock/datastore.go:24server/vulnerabilities/msrc/parsed/product.go:14frontend/utilities/auth_token/index.ts:9server/contexts/viewer/viewer.go:108server/vulnerabilities/msrc/parsed/security_bulletin.go:36charts/fleet/values.yaml:12server/datastore/mysqlredis/hosts.go:143server/mdm/nanomdm/storage/mysql/mysql.go:158server/service/async/async_label.go:98server/service/async/async_policy.go:122server/service/endpoint_middleware.go:24server/service/endpoint_middleware.go:76server/service/maintained_apps.go:36What 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.