carverauto/serviceradar

Fork 0

initial #2546

Merged

mfreeman451 merged 6 commits from refs/pull/2546/head into main

2025-12-11 22:07:43 +00:00

mfreeman451 commented

2025-12-11 10:31:33 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2107
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2107
Original created: 2025-12-11T10:31:33Z
Original updated: 2025-12-11T22:07:52Z
Original head: carverauto/serviceradar:bug/edge_onboarding_missing_sysmon_template
Original base: main
Original merged: 2025-12-11T22:07:43Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Bug fix, Enhancement

Description

Add ListKeys RPC to KV service for prefix-based key discovery
Implement checker template seeding into KV at deployment time
Add API endpoint to list available checker templates for UI discovery
Update web UI to show checker templates as dropdown selector
Create default templates for all supported checker types (sysmon, snmp, rperf, dusk, sysmon-osx)

Diagram Walkthrough

flowchart LR
  A["Proto: Add ListKeys RPC"] --> B["KV Service: Implement ListKeys"]
  B --> C["Edge Onboarding: List Templates"]
  C --> D["API: GET /checker-templates"]
  D --> E["Web UI: Template Dropdown"]
  F["Seed Job: Bootstrap Templates"] --> B
  G["Default Templates"] --> F

File Walkthrough

Relevant files

Enhancement

12 files

kv.proto `Add ListKeys RPC and message types`	+13/-0
interfaces.go `Add ListKeys method to KVStore interface`	+4/-0
nats.go `Implement ListKeys with prefix filtering`	+34/-0
server.go `Add ListKeys RPC handler implementation`	+12/-0
interfaces.go `Add ListKeys to KVClient interface`	+1/-0
edge_onboarding.go `Add CheckerTemplate model type`	+6/-0
edge_onboarding.go `Implement ListCheckerTemplates method`	+38/-0
types.go `Add ListCheckerTemplates to service interface`	+1/-0
edge_onboarding.go `Add handler for GET /checker-templates endpoint`	+20/-0
server.go `Register /checker-templates route`	+1/-0
page.tsx `Replace text input with template dropdown selector`	+93/-24
seed-checker-templates.sh `Create template seeding script for docker-compose`	+78/-0

Code generation

2 files

kv.pb.go `Regenerate protobuf with ListKeys support`	+190/-91
kv_grpc.pb.go `Regenerate gRPC with ListKeys handler`	+42/-2

Tests

4 files

mock_datasvc.go `Add ListKeys mock for testing`	+15/-0
mock_sync.go `Add ListKeys mock for sync testing`	+20/-0
edge_onboarding_test.go `Add comprehensive template listing tests`	+254/-0
edge_onboarding_test.go `Add stub implementation for new interface method`	+4/-0

Configuration changes

14 files

docker-compose.yml `Add checker-templates-seed service`	+23/-0
sysmon.json `Add sysmon checker default template`	+24/-0
sysmon-osx.json `Add macOS sysmon checker default template`	+11/-0
snmp.json `Add SNMP checker default template`	+19/-0
rperf.json `Add rperf checker default template`	+15/-0
dusk.json `Add dusk checker default template`	+15/-0
sysmon.json `Create sysmon template with variable substitution`	+24/-0
sysmon-osx.json `Create macOS sysmon template with variables`	+11/-0
snmp.json `Update SNMP template with variable placeholders`	+17/-45
rperf.json `Update rperf template with SPIFFE security`	+6/-28
dusk.json `Create dusk checker default template`	+15/-0
values.yaml `Add checkerTemplates.enabled configuration flag`	+3/-0
checker-templates-config.yaml `Create Helm ConfigMap with default templates`	+97/-0
checker-templates-kv-bootstrap-job.yaml `Create Helm Job for KV template seeding`	+78/-0

Documentation

4 files

checker-template-registration.md `Update documentation for template seeding approach`	+129/-191
proposal.md `Add change proposal documentation`	+37/-0
tasks.md `Add implementation task checklist`	+37/-0
spec.md `Add specification requirements for templates`	+41/-0

Imported from GitHub pull request. Original GitHub pull request: #2107 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2107 Original created: 2025-12-11T10:31:33Z Original updated: 2025-12-11T22:07:52Z Original head: carverauto/serviceradar:bug/edge_onboarding_missing_sysmon_template Original base: main Original merged: 2025-12-11T22:07:43Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix, Enhancement ___ ### **Description** - Add `ListKeys` RPC to KV service for prefix-based key discovery - Implement checker template seeding into KV at deployment time - Add API endpoint to list available checker templates for UI discovery - Update web UI to show checker templates as dropdown selector - Create default templates for all supported checker types (sysmon, snmp, rperf, dusk, sysmon-osx) ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Proto: Add ListKeys RPC"] --> B["KV Service: Implement ListKeys"] B --> C["Edge Onboarding: List Templates"] C --> D["API: GET /checker-templates"] D --> E["Web UI: Template Dropdown"] F["Seed Job: Bootstrap Templates"] --> B G["Default Templates"] --> F ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Enhancement</td><td><details><summary>12 files</summary><table> <tr> <td>kv.proto<dd><code>Add ListKeys RPC and message types</code>                                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-beb3969f93aa78befd0e06133c639384ab4b2b120a68f656d33f6ae3fc5352a0">+13/-0</a>    </td> </tr> <tr> <td>interfaces.go<dd><code>Add ListKeys method to KVStore interface</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-910534aed053891acf39a5dec4cf05d1e3c04ef08bd8fcedeac139d1747faace">+4/-0</a>      </td> </tr> <tr> <td>nats.go<dd><code>Implement ListKeys with prefix filtering</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-5158b29e580a201f5f461d47939ece7caf94f27d43a5e0d465d79ea6b41ec4b4">+34/-0</a>    </td> </tr> <tr> <td>server.go<dd><code>Add ListKeys RPC handler implementation</code>                                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-ca4e0dbf349806d8de2c79e953a7c2bb55bce593f8818613dc92c55ca7411195">+12/-0</a>    </td> </tr> <tr> <td>interfaces.go<dd><code>Add ListKeys to KVClient interface</code>                                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-b29a7503f8613d3c47d857f4c2071aa9f2d2e17a1ab64dfb974ec4a3992e8f25">+1/-0</a>      </td> </tr> <tr> <td>edge_onboarding.go<dd><code>Add CheckerTemplate model type</code>                                                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-0c66da94363b25ecb9e4766c8f12cde387c1874aeb9099447c609c4e867b4fe0">+6/-0</a>      </td> </tr> <tr> <td>edge_onboarding.go<dd><code>Implement ListCheckerTemplates method</code>                                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-85874e3c4bdcc9110db09909f10648d44cdee554b26c987f910502321eb20b5c">+38/-0</a>    </td> </tr> <tr> <td>types.go<dd><code>Add ListCheckerTemplates to service interface</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-39f024121630282f9e3eeee5b77e5a63a87950b9eae4f479277a289796b42d56">+1/-0</a>      </td> </tr> <tr> <td>edge_onboarding.go<dd><code>Add handler for GET /checker-templates endpoint</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-c494568149a0b526ca5349c1d78d5e5ee7ec4144ecfd324c44f25fef584292bc">+20/-0</a>    </td> </tr> <tr> <td>server.go<dd><code>Register /checker-templates route</code>                                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021e">+1/-0</a>      </td> </tr> <tr> <td>page.tsx<dd><code>Replace text input with template dropdown selector</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-d627c4ba6a102a46a28024061371276f01053b63f20c75986aef2631b967bafc">+93/-24</a>  </td> </tr> <tr> <td>seed-checker-templates.sh<dd><code>Create template seeding script for docker-compose</code>                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-bbb097073bf656804940727b073e52cc8ed5ae652c52cf61ac8378afd0c7f2ee">+78/-0</a>    </td> </tr> </table></details></td></tr><tr><td>Code generation</td><td><details><summary>2 files</summary><table> <tr> <td>kv.pb.go<dd><code>Regenerate protobuf with ListKeys support</code>                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-0b9bbdd1cf0f65de86dd5ee0c824e5d35326bb4b8edcf1c97b61864493f4477f">+190/-91</a></td> </tr> <tr> <td>kv_grpc.pb.go<dd><code>Regenerate gRPC with ListKeys handler</code>                                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-6daa12c34cd941284969fc2b1098fff1f7a96a17c57d99c7ba9267d494ebddd5">+42/-2</a>    </td> </tr> </table></details></td></tr><tr><td>Tests</td><td><details><summary>4 files</summary><table> <tr> <td>mock_datasvc.go<dd><code>Add ListKeys mock for testing</code>                                                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-6d7e0689768ae0c41885bb6177204dde23a5601983bef06105c61637ca64080a">+15/-0</a>    </td> </tr> <tr> <td>mock_sync.go<dd><code>Add ListKeys mock for sync testing</code>                                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-e71d4ddbb89163e41caca6350f74ca3208339195d364e411f507de81922ae028">+20/-0</a>    </td> </tr> <tr> <td>edge_onboarding_test.go<dd><code>Add comprehensive template listing tests</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-83ba269452783436d693c0f17ab42c959efdef44c7cf6ec27301dd1b0e7d5744">+254/-0</a>  </td> </tr> <tr> <td>edge_onboarding_test.go<dd><code>Add stub implementation for new interface method</code>                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-ff98b520d3df871ff219eb56ab1a2ac2d55afb266b87393bd63360635ba02880">+4/-0</a>      </td> </tr> </table></details></td></tr><tr><td>Configuration changes</td><td><details><summary>14 files</summary><table> <tr> <td>docker-compose.yml<dd><code>Add checker-templates-seed service</code>                                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3">+23/-0</a>    </td> </tr> <tr> <td>sysmon.json<dd><code>Add sysmon checker default template</code>                                            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-5a8fa31bb733b90481555a50abe3523e719f4ad36e3649ae57c726e835994ff7">+24/-0</a>    </td> </tr> <tr> <td>sysmon-osx.json<dd><code>Add macOS sysmon checker default template</code>                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-43563fa504b7e6dfdc0310c5d3e07654c2ac5135cc6c0c33cdc6e8a47722555d">+11/-0</a>    </td> </tr> <tr> <td>snmp.json<dd><code>Add SNMP checker default template</code>                                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-cc91167633eb85378116d9b83f86856939623028f0898bacbd157fc078740952">+19/-0</a>    </td> </tr> <tr> <td>rperf.json<dd><code>Add rperf checker default template</code>                                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-45b7d617c365a463fa975030f1475b374aeaa181eb11431e56c4ffaff5c275cb">+15/-0</a>    </td> </tr> <tr> <td>dusk.json<dd><code>Add dusk checker default template</code>                                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-dd501de2452f9290baa8d4caac18e06a5957c5681ad0454066ceed272c077b33">+15/-0</a>    </td> </tr> <tr> <td>sysmon.json<dd><code>Create sysmon template with variable substitution</code>                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-2083e4f3e5751d8da3423dd633f0516e6a88dc00e0e84e7b5bd78d70b7197e45">+24/-0</a>    </td> </tr> <tr> <td>sysmon-osx.json<dd><code>Create macOS sysmon template with variables</code>                            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-add2f98d54f50680b2a8b8f75431ebd406ce85f07b7b24b9018661ea831cea1f">+11/-0</a>    </td> </tr> <tr> <td>snmp.json<dd><code>Update SNMP template with variable placeholders</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-8bc0492d7a06f6e1bb053046ccb6d3b3025f7d3e6a626bf6724df364febfeb05">+17/-45</a>  </td> </tr> <tr> <td>rperf.json<dd><code>Update rperf template with SPIFFE security</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-ad0e2fee17bd37f44f66a48c3afe3deff686b20a61e03752bfbedc280af4e655">+6/-28</a>    </td> </tr> <tr> <td>dusk.json<dd><code>Create dusk checker default template</code>                                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-d548a3d6f0d9c6151efb352427b747cb7ce86e436d5dd7519c531cdf00747ea3">+15/-0</a>    </td> </tr> <tr> <td>values.yaml<dd><code>Add checkerTemplates.enabled configuration flag</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-d4449c7cb70362554b274f81eae5a4b81a8e81df494282e383d1b7ea3871c452">+3/-0</a>      </td> </tr> <tr> <td>checker-templates-config.yaml<dd><code>Create Helm ConfigMap with default templates</code>                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-ea32be24a0a24ae0eee66f3a7af317c7d44dcd0953410ec3f2fefc226e883495">+97/-0</a>    </td> </tr> <tr> <td>checker-templates-kv-bootstrap-job.yaml<dd><code>Create Helm Job for KV template seeding</code>                                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-f84a118b256172692343bb29a43932b07f59804e7648aa88164853c05479d570">+78/-0</a>    </td> </tr> </table></details></td></tr><tr><td>Documentation</td><td><details><summary>4 files</summary><table> <tr> <td>checker-template-registration.md<dd><code>Update documentation for template seeding approach</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-592a3d8a2648917d208e668b887122e9cdb33acb7af4ed99c6a506d5d9ad7c02">+129/-191</a></td> </tr> <tr> <td>proposal.md<dd><code>Add change proposal documentation</code>                                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-31a1ba838a1bfcbb5a63932945cc997f286342f71e0bf604337e3de92afbc641">+37/-0</a>    </td> </tr> <tr> <td>tasks.md<dd><code>Add implementation task checklist</code>                                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-d0490fd7ef43a5b3e9e76392a51dea4af16941aa2dbb452463830813a5a9503c">+37/-0</a>    </td> </tr> <tr> <td>spec.md<dd><code>Add specification requirements for templates</code>                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2107/files#diff-7a38ed8c4deeb1199af13dcc361e22ee9ba229c8b2b663c26d5334ff0af7f824">+41/-0</a>    </td> </tr> </table></details></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-11 10:32:38 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2107#issuecomment-3641282076
Original created: 2025-12-11T10:32:38Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Supply chain risk Description: The seeding script overwrites KV templates unconditionally and logs failures but continues overall process, which could enable unauthorized or accidental template tampering if the container is compromised or if credentials/env are misconfigured; consider integrity checks (e.g., signed templates), strict failure on unexpected changes, and least-privilege credentials. seed-checker-templates.sh [49-56] Referred Code `key="templates/checkers/${checker_kind}.json" # Always overwrite templates (they're factory defaults, safe to update) log "Seeding checker template: ${key} from ${source_file}" if ! nats_cmd kv put "${KV_BUCKET}" "${key}" <"${source_file}"; then log "Failed to seed ${key}" return 1 fi`
	Information exposure via logs Description: Errors from stopping the NATS KeyLister are only printed via log.Printf without context controls or escalation, potentially leaking internal details and masking cleanup failures; prefer structured logging with controlled verbosity and ensure safe error handling to avoid information exposure in logs. nats.go [468-474] Referred Code `for key := range lister.Keys() { keys = append(keys, key) } if err := lister.Stop(); err != nil { log.Printf("warning: failed to stop key lister: %v", err) }`
	Error detail leakage Description: The API handler returns a generic 500 with "Failed to list checker templates" while logging the underlying error; ensure logs do not include sensitive backend details from KV errors to prevent leaking infrastructure information (e.g., prefixes, connection details). edge_onboarding.go [733-744] Referred Code `templates, err := s.edgeOnboarding.ListCheckerTemplates(r.Context()) if err != nil { s.logger.Error().Err(err).Msg("Failed to list checker templates") writeError(w, "Failed to list checker templates", http.StatusInternalServerError) return } w.Header().Set("Content-Type", "application/json") if err := json.NewEncoder(w).Encode(templates); err != nil { s.logger.Error().Err(err).Msg("Failed to encode checker templates response") } }`
	Client-side info leakage Description: The frontend fetches '/api/admin/checker-templates' and on non-OK responses only logs warnings to console without user feedback; while not a direct vuln, it may encourage users to open devtools where sensitive error details could appear; ensure server and client avoid emitting sensitive info in console logs. page.tsx [574-597] Referred Code let cancelled = false; const fetchTemplates = async () => { setTemplatesLoading(true); try { const response = await fetch('/api/admin/checker-templates', { headers: buildHeaders(), cache: 'no-store', }); if (!response.ok) { console.warn('Failed to load checker templates'); return; } const data: CheckerTemplate[] = await response.json(); if (!cancelled) { setCheckerTemplates(data ?? []); } } catch (err) { console.error('Failed to load checker templates', err); } finally { if (!cancelled) { setTemplatesLoading(false); ... (clipped 3 lines)
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: The new endpoint handler returns data and logs only on errors without explicit audit logging of the access (who requested template listing and outcome), which may be required for critical actions visibility. Referred Code // handleListCheckerTemplates returns the list of available checker templates. func (s APIServer) handleListCheckerTemplates(w http.ResponseWriter, r http.Request) { if s.edgeOnboarding == nil { writeError(w, "Edge onboarding service is disabled", http.StatusServiceUnavailable) return } templates, err := s.edgeOnboarding.ListCheckerTemplates(r.Context()) if err != nil { s.logger.Error().Err(err).Msg("Failed to list checker templates") writeError(w, "Failed to list checker templates", http.StatusInternalServerError) return } w.Header().Set("Content-Type", "application/json") if err := json.NewEncoder(w).Encode(templates); err != nil { s.logger.Error().Err(err).Msg("Failed to encode checker templates response") } } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Weak error context: The ListKeys method wraps errors with a generic message and logs a warning on Stop but does not include domain/prefix context in logs which may hinder debugging for edge cases or partial failures. Referred Code // ListKeys returns all keys matching the given prefix filter. // If prefix is empty, all keys are returned. func (n *NATSStore) ListKeys(ctx context.Context, prefix string) ([]string, error) { domain, realPrefix := n.extractDomain(prefix) kv, err := n.getKVForDomain(ctx, domain) if err != nil { return nil, err } var keys []string var lister jetstream.KeyLister if realPrefix == "" { // List all keys lister, err = kv.ListKeys(ctx) } else { // List keys with prefix filter using wildcard pattern lister, err = kv.ListKeysFiltered(ctx, realPrefix+">") } if err != nil { return nil, fmt.Errorf("failed to list keys with prefix %s: %w", prefix, err) ... (clipped 12 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Unvalidated input: The new API handler proxies template listing without explicit authorization/role checks or rate limiting visible in this diff; if upstream middleware does not enforce auth, this could expose template metadata. Referred Code // handleListCheckerTemplates returns the list of available checker templates. func (s APIServer) handleListCheckerTemplates(w http.ResponseWriter, r http.Request) { if s.edgeOnboarding == nil { writeError(w, "Edge onboarding service is disabled", http.StatusServiceUnavailable) return } templates, err := s.edgeOnboarding.ListCheckerTemplates(r.Context()) if err != nil { s.logger.Error().Err(err).Msg("Failed to list checker templates") writeError(w, "Failed to list checker templates", http.StatusInternalServerError) return } w.Header().Set("Content-Type", "application/json") if err := json.NewEncoder(w).Encode(templates); err != nil { s.logger.Error().Err(err).Msg("Failed to encode checker templates response") } } Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2107#issuecomment-3641282076 Original created: 2025-12-11T10:32:38Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR: <table><tbody><tr><td colspan='2'>Security Compliance</td></tr> <tr><td rowspan=4>⚪</td> <td><details><summary>Supply chain risk </summary> Description: The seeding script overwrites KV templates unconditionally and logs failures but continues overall process, which could enable unauthorized or accidental template tampering if the container is compromised or if credentials/env are misconfigured; consider integrity checks (e.g., signed templates), strict failure on unexpected changes, and least-privilege credentials. <a href='https://github.com/carverauto/serviceradar/pull/2107/files#diff-bbb097073bf656804940727b073e52cc8ed5ae652c52cf61ac8378afd0c7f2eeR49-R56'>seed-checker-templates.sh [49-56]</a> <details open><summary>Referred Code</summary> ```shell key="templates/checkers/${checker_kind}.json" # Always overwrite templates (they're factory defaults, safe to update) log "Seeding checker template: ${key} from ${source_file}" if ! nats_cmd kv put "${KV_BUCKET}" "${key}" <"${source_file}"; then log "Failed to seed ${key}" return 1 fi ``` </details></details></td></tr> <tr><td><details><summary>Information exposure via logs </summary> Description: Errors from stopping the NATS KeyLister are only printed via log.Printf without context controls or escalation, potentially leaking internal details and masking cleanup failures; prefer structured logging with controlled verbosity and ensure safe error handling to avoid information exposure in logs. <a href='https://github.com/carverauto/serviceradar/pull/2107/files#diff-5158b29e580a201f5f461d47939ece7caf94f27d43a5e0d465d79ea6b41ec4b4R468-R474'>nats.go [468-474]</a> <details open><summary>Referred Code</summary> ```go for key := range lister.Keys() { keys = append(keys, key) } if err := lister.Stop(); err != nil { log.Printf("warning: failed to stop key lister: %v", err) } ``` </details></details></td></tr> <tr><td><details><summary>Error detail leakage </summary> Description: The API handler returns a generic 500 with "Failed to list checker templates" while logging the underlying error; ensure logs do not include sensitive backend details from KV errors to prevent leaking infrastructure information (e.g., prefixes, connection details). <a href='https://github.com/carverauto/serviceradar/pull/2107/files#diff-c494568149a0b526ca5349c1d78d5e5ee7ec4144ecfd324c44f25fef584292bcR733-R744'>edge_onboarding.go [733-744]</a> <details open><summary>Referred Code</summary> ```go templates, err := s.edgeOnboarding.ListCheckerTemplates(r.Context()) if err != nil { s.logger.Error().Err(err).Msg("Failed to list checker templates") writeError(w, "Failed to list checker templates", http.StatusInternalServerError) return } w.Header().Set("Content-Type", "application/json") if err := json.NewEncoder(w).Encode(templates); err != nil { s.logger.Error().Err(err).Msg("Failed to encode checker templates response") } } ``` </details></details></td></tr> <tr><td><details><summary>Client-side info leakage </summary> Description: The frontend fetches '/api/admin/checker-templates' and on non-OK responses only logs warnings to console without user feedback; while not a direct vuln, it may encourage users to open devtools where sensitive error details could appear; ensure server and client avoid emitting sensitive info in console logs. <a href='https://github.com/carverauto/serviceradar/pull/2107/files#diff-d627c4ba6a102a46a28024061371276f01053b63f20c75986aef2631b967bafcR574-R597'>page.tsx [574-597]</a> <details open><summary>Referred Code</summary> ```tsx let cancelled = false; const fetchTemplates = async () => { setTemplatesLoading(true); try { const response = await fetch('/api/admin/checker-templates', { headers: buildHeaders(), cache: 'no-store', }); if (!response.ok) { console.warn('Failed to load checker templates'); return; } const data: CheckerTemplate[] = await response.json(); if (!cancelled) { setCheckerTemplates(data ?? []); } } catch (err) { console.error('Failed to load checker templates', err); } finally { if (!cancelled) { setTemplatesLoading(false); ... (clipped 3 lines) ``` </details></details></td></tr> <tr><td colspan='2'>Ticket Compliance</td></tr> <tr><td>⚪</td><td><details><summary>🎫 No ticket provided </summary> - [ ] Create ticket/issue  </details></td></tr> <tr><td colspan='2'>Codebase Duplication Compliance</td></tr> <tr><td>⚪</td><td><details><summary>Codebase context is not defined </summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'>Custom Compliance</td></tr> <tr><td rowspan=3>🟢</td><td> <details><summary>Generic: Meaningful Naming and Self-Documenting Code</summary> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code self-documenting **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Logging Practices</summary> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=3>⚪</td> <td><details> <summary>Generic: Comprehensive Audit Trails</summary> **Objective:** To create a detailed and reliable record of critical system actions for security analysis and compliance. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2107/files#diff-c494568149a0b526ca5349c1d78d5e5ee7ec4144ecfd324c44f25fef584292bcR726-R744'>Missing audit logs</a>: The new endpoint handler returns data and logs only on errors without explicit audit logging of the access (who requested template listing and outcome), which may be required for critical actions visibility. <details open><summary>Referred Code</summary> ```go // handleListCheckerTemplates returns the list of available checker templates. func (s *APIServer) handleListCheckerTemplates(w http.ResponseWriter, r *http.Request) { if s.edgeOnboarding == nil { writeError(w, "Edge onboarding service is disabled", http.StatusServiceUnavailable) return } templates, err := s.edgeOnboarding.ListCheckerTemplates(r.Context()) if err != nil { s.logger.Error().Err(err).Msg("Failed to list checker templates") writeError(w, "Failed to list checker templates", http.StatusInternalServerError) return } w.Header().Set("Content-Type", "application/json") if err := json.NewEncoder(w).Encode(templates); err != nil { s.logger.Error().Err(err).Msg("Failed to encode checker templates response") } } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2107/files#diff-5158b29e580a201f5f461d47939ece7caf94f27d43a5e0d465d79ea6b41ec4b4R445-R477'>Weak error context</a>: The ListKeys method wraps errors with a generic message and logs a warning on Stop but does not include domain/prefix context in logs which may hinder debugging for edge cases or partial failures. <details open><summary>Referred Code</summary> ```go // ListKeys returns all keys matching the given prefix filter. // If prefix is empty, all keys are returned. func (n *NATSStore) ListKeys(ctx context.Context, prefix string) ([]string, error) { domain, realPrefix := n.extractDomain(prefix) kv, err := n.getKVForDomain(ctx, domain) if err != nil { return nil, err } var keys []string var lister jetstream.KeyLister if realPrefix == "" { // List all keys lister, err = kv.ListKeys(ctx) } else { // List keys with prefix filter using wildcard pattern lister, err = kv.ListKeysFiltered(ctx, realPrefix+">") } if err != nil { return nil, fmt.Errorf("failed to list keys with prefix %s: %w", prefix, err) ... (clipped 12 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Security-First Input Validation and Data Handling</summary> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2107/files#diff-c494568149a0b526ca5349c1d78d5e5ee7ec4144ecfd324c44f25fef584292bcR726-R744'>Unvalidated input</a>: The new API handler proxies template listing without explicit authorization/role checks or rate limiting visible in this diff; if upstream middleware does not enforce auth, this could expose template metadata. <details open><summary>Referred Code</summary> ```go // handleListCheckerTemplates returns the list of available checker templates. func (s *APIServer) handleListCheckerTemplates(w http.ResponseWriter, r *http.Request) { if s.edgeOnboarding == nil { writeError(w, "Edge onboarding service is disabled", http.StatusServiceUnavailable) return } templates, err := s.edgeOnboarding.ListCheckerTemplates(r.Context()) if err != nil { s.logger.Error().Err(err).Msg("Failed to list checker templates") writeError(w, "Failed to list checker templates", http.StatusInternalServerError) return } w.Header().Set("Content-Type", "application/json") if err := json.NewEncoder(w).Encode(templates); err != nil { s.logger.Error().Err(err).Msg("Failed to encode checker templates response") } } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant 🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label </details>

qodo-code-review[bot] commented

2025-12-11 10:34:16 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2107#issuecomment-3641288189
Original created: 2025-12-11T10:34:16Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Ensure Helm job fails on error Remove the error-suppressing `\|\| echo "Warning: ..."` from the `checker-templates-kv-bootstrap-job.yaml` script to ensure the Kubernetes Job fails correctly if a template cannot be seeded. helm/serviceradar/templates/checker-templates-kv-bootstrap-job.yaml [59] `-seed_template "${kind}" \|\| echo "Warning: failed to seed ${kind}" +seed_template "${kind}"` Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly points out that error codes are being suppressed, which would cause the Helm bootstrap job to succeed silently even on failure. The fix is critical for ensuring deployment reliability and observability.	Medium
General	Improve error handling in seeding script Modify the `seed-checker-templates.sh` script to explicitly handle errors during template seeding, log the specific failure, and exit immediately to prevent partial success. docker/compose/seed-checker-templates.sh [71-73] `-if seed_template "${checker_kind}" "${template_file}"; then - seeded=$((seeded + 1)) +if ! seed_template "${checker_kind}" "${template_file}"; then + log "Error seeding template ${checker_kind}, exiting." + exit 1 fi +seeded=$((seeded + 1))` Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the script does not exit on a seeding failure and proposes a fix that makes the script fail-fast, which improves the robustness of the bootstrap process.	Low
General	Simplify and improve template listing logic Simplify the `ListCheckerTemplates` function by removing the redundant `strings.HasPrefix` check, as prefix filtering is already handled by the `kvClient.ListKeys` call. pkg/core/edge_onboarding.go [1825-1861] // ListCheckerTemplates returns all available checker templates from KV. func (s edgeOnboardingService) ListCheckerTemplates(ctx context.Context) ([]models.CheckerTemplate, error) { if s.kvClient == nil { return nil, fmt.Errorf("KV client not available") } const templatePrefix = "templates/checkers/" resp, err := s.kvClient.ListKeys(ctx, &proto.ListKeysRequest{ Prefix: templatePrefix, }) if err != nil { return nil, fmt.Errorf("failed to list checker templates: %w", err) } templates := make([]models.CheckerTemplate, 0, len(resp.GetKeys())) for _, key := range resp.GetKeys() { - // Extract checker kind from key (e.g., "templates/checkers/sysmon.json" -> "sysmon") - if !strings.HasPrefix(key, templatePrefix) \|\| !strings.HasSuffix(key, ".json") { + if !strings.HasSuffix(key, ".json") { continue } - kind := strings.TrimPrefix(key, templatePrefix) - kind = strings.TrimSuffix(kind, ".json") + // Extract checker kind from key (e.g., "templates/checkers/sysmon.json" -> "sysmon") + kind := strings.TrimSuffix(strings.TrimPrefix(key, templatePrefix), ".json") if kind == "" { continue } templates = append(templates, models.CheckerTemplate{ Kind: kind, TemplateKey: key, }) } return templates, nil } Apply / Chat* Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies a redundant `HasPrefix` check, as the `kvClient.ListKeys` call already filters by prefix. Removing it simplifies the code, though the performance impact is likely minimal.	Low
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2107#issuecomment-3641288189 Original created: 2025-12-11T10:34:16Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td>Category</td><td align=left>Suggestion                                                                                                                                    </td><td align=center>Impact</td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Ensure Helm job fails on error</summary> ___ **Remove the error-suppressing <code>|| echo "Warning: ..."</code> from the <code>checker-templates-kv-bootstrap-job.yaml</code> script to ensure the Kubernetes Job fails correctly if a template cannot be seeded.** [helm/serviceradar/templates/checker-templates-kv-bootstrap-job.yaml [59]](https://github.com/carverauto/serviceradar/pull/2107/files#diff-f84a118b256172692343bb29a43932b07f59804e7648aa88164853c05479d570R59-R59) ```diff -seed_template "${kind}" || echo "Warning: failed to seed ${kind}" +seed_template "${kind}" ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly points out that error codes are being suppressed, which would cause the Helm bootstrap job to succeed silently even on failure. The fix is critical for ensuring deployment reliability and observability. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=2>General</td> <td> <details><summary>Improve error handling in seeding script</summary> ___ **Modify the <code>seed-checker-templates.sh</code> script to explicitly handle errors during template seeding, log the specific failure, and exit immediately to prevent partial success.** [docker/compose/seed-checker-templates.sh [71-73]](https://github.com/carverauto/serviceradar/pull/2107/files#diff-bbb097073bf656804940727b073e52cc8ed5ae652c52cf61ac8378afd0c7f2eeR71-R73) ```diff -if seed_template "${checker_kind}" "${template_file}"; then - seeded=$((seeded + 1)) +if ! seed_template "${checker_kind}" "${template_file}"; then + log "Error seeding template ${checker_kind}, exiting." + exit 1 fi +seeded=$((seeded + 1)) ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies that the script does not exit on a seeding failure and proposes a fix that makes the script fail-fast, which improves the robustness of the bootstrap process. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>Simplify and improve template listing logic</summary> ___ **Simplify the <code>ListCheckerTemplates</code> function by removing the redundant <code>strings.HasPrefix</code> check, as prefix filtering is already handled by the <code>kvClient.ListKeys</code> call.** [pkg/core/edge_onboarding.go [1825-1861]](https://github.com/carverauto/serviceradar/pull/2107/files#diff-85874e3c4bdcc9110db09909f10648d44cdee554b26c987f910502321eb20b5cR1825-R1861) ```diff // ListCheckerTemplates returns all available checker templates from KV. func (s *edgeOnboardingService) ListCheckerTemplates(ctx context.Context) ([]models.CheckerTemplate, error) { if s.kvClient == nil { return nil, fmt.Errorf("KV client not available") } const templatePrefix = "templates/checkers/" resp, err := s.kvClient.ListKeys(ctx, &proto.ListKeysRequest{ Prefix: templatePrefix, }) if err != nil { return nil, fmt.Errorf("failed to list checker templates: %w", err) } templates := make([]models.CheckerTemplate, 0, len(resp.GetKeys())) for _, key := range resp.GetKeys() { - // Extract checker kind from key (e.g., "templates/checkers/sysmon.json" -> "sysmon") - if !strings.HasPrefix(key, templatePrefix) || !strings.HasSuffix(key, ".json") { + if !strings.HasSuffix(key, ".json") { continue } - kind := strings.TrimPrefix(key, templatePrefix) - kind = strings.TrimSuffix(kind, ".json") + // Extract checker kind from key (e.g., "templates/checkers/sysmon.json" -> "sysmon") + kind := strings.TrimSuffix(strings.TrimPrefix(key, templatePrefix), ".json") if kind == "" { continue } templates = append(templates, models.CheckerTemplate{ Kind: kind, TemplateKey: key, }) } return templates, nil } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 4</summary> __ Why: The suggestion correctly identifies a redundant `HasPrefix` check, as the `kvClient.ListKeys` call already filters by prefix. Removing it simplifies the code, though the performance impact is likely minimal. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>

mfreeman451 commented

2025-12-11 10:40:02 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2107#discussion_r2610079536
Original created: 2025-12-11T10:40:02Z
Original path: pkg/models/edge_onboarding.go
Original line: 163

bad name

Imported GitHub PR review comment. Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2107#discussion_r2610079536 Original created: 2025-12-11T10:40:02Z Original path: pkg/models/edge_onboarding.go Original line: 163 --- bad name

mfreeman451 commented

2025-12-11 10:40:45 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2107#discussion_r2610081788
Original created: 2025-12-11T10:40:45Z
Original path: web/src/app/admin/edge-packages/page.tsx
Original line: 105

bad name?

Imported GitHub PR review comment. Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2107#discussion_r2610081788 Original created: 2025-12-11T10:40:45Z Original path: web/src/app/admin/edge-packages/page.tsx Original line: 105 --- bad name?