1933 bugui settingspoller missing config options in forms #2409
No reviewers
Labels
No labels
1week
2weeks
Failed compliance check
IP cameras
NATS
Possible security concern
Review effort 1/5
Review effort 2/5
Review effort 3/5
Review effort 4/5
Review effort 5/5
UI
aardvark
accessibility
amd64
api
arm64
auth
back-end
bgp
blog
bug
build
checkers
ci-cd
cleanup
cnpg
codex
core
dependencies
device-management
documentation
duplicate
dusk
ebpf
enhancement
eta 1d
eta 1hr
eta 3d
eta 3hr
feature
fieldsurvey
github_actions
go
good first issue
help wanted
invalid
javascript
k8s
log-collector
mapper
mtr
needs-triage
netflow
network-sweep
observability
oracle
otel
plug-in
proton
python
question
reddit
redhat
research
rperf
rperf-checker
rust
sdk
security
serviceradar-agent
serviceradar-agent-gateway
serviceradar-web
serviceradar-web-ng
siem
snmp
sysmon
topology
ubiquiti
wasm
wontfix
zen-engine
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
carverauto/serviceradar!2409
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "refs/pull/2409/head"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Imported from GitHub pull request.
Original GitHub pull request: #1934
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1934
Original created: 2025-11-11T00:51:05Z
Original updated: 2025-11-11T02:08:43Z
Original head: carverauto/serviceradar:1933-bugui-settingspoller-missing-config-options-in-forms
Original base: main
Original merged: 2025-11-11T02:08:15Z 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:
Describe your changes
Issue ticket number and link
Code checklist before requesting a review
PR Type
Enhancement, Bug fix
Description
Add KV-backed checker config seeding to agent startup
Implement retry logic for KV manager initialization with configurable timeouts
Expand agent and poller config forms with comprehensive security and logging options
Fix remote watcher target deduplication and agent-scoped service scope normalization
Upgrade agent and poller KV permissions from reader to writer role
Diagram Walkthrough
File Walkthrough
10 files
Add checker config seeding from disk to KVAdd retry logic for KV manager initializationSupport explicit KV key paths for direct accessImplement KV manager retry with backoff and validationAdd explicit KV key prefix detection and derivationImplement watch stream resilience with exponential backoffAdd retry logic for core KV manager initializationRedesign agent form with security and logging sectionsRedesign poller form with agents, checks, and securityCreate reusable security configuration component2 files
Add test for explicit agent KV key loadingAdd tests for KV retry logic and environment validation1 files
Add gRPC status code dependencies3 files
Deduplicate and normalize remote watcher targetsFilter global services to allowed types onlyNormalize watcher scopes for agent-scoped services3 files
Upgrade agent KV role from reader to writerUpgrade agent KV role from reader to writerUpgrade agent KV role from reader to writer1 files
Fix error message wording in build scriptImported GitHub PR comment.
Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1934#issuecomment-3514512928
Original created: 2025-11-11T00:51:46Z
PR Compliance Guide 🔍
(Compliance updated until commit
github.com/carverauto/serviceradar@756ba115dc)Below is a summary of compliance checks for this PR:
Retry without jitter
Description: The retry loop for KV connection lacks a jitter component, which can cause thundering herd
and predictable backoff timing under load or outages.
kv_client.go [149-166]
Referred Code
Unvalidated config ingestion
Description: Seeding checker configs from disk into KV may ingest unvalidated JSON from local files,
risking malicious or malformed configuration if filesystem contents are untrusted.
main.go [108-114]
Referred Code
🎫 #1933
fields.
Codebase context is not defined
Follow the guide to enable codebase context checks.
Generic: Meaningful Naming and Self-Documenting Code
Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting
Status: Passed
Generic: Robust Error Handling and Edge Case Management
Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation
Status: Passed
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
Generic: Security-First Input Validation and Data Handling
Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities
Status: Passed
Generic: Comprehensive Audit Trails
Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.
Status:
Action Logging: The seeding of checker configurations into KV performs create operations but does not emit
structured audit logs including actor identity and outcome beyond generic info/warn
messages.
Referred Code
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:
Sensitive Paths: Logs include filesystem paths and KV keys for seeded configs which might reveal
environment structure and agent IDs; confirm this is acceptable and that no secrets are
ever logged.
Referred Code
Compliance status legend
🟢 - Fully Compliant🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label
Previous compliance checks
Compliance check up to commit 31e1361
Header injection risk
Description: The OTEL headers field accepts arbitrary JSON object input from the UI and writes it into
configuration without sanitization, which could enable header injection to external OTEL
endpoints if this data is propagated directly to requests.
PollerConfigForm.tsx [487-500]
Referred Code
Unvalidated config write
Description: Seeding checker configs reads arbitrary JSON files from disk and writes them into KV
without schema validation, which could allow malicious or malformed config content to be
propagated if the directory is writable by untrusted actors.
checker_seed.go [70-98]
Referred Code
🎫 #1933
set of relevant security and logging options.
appropriate fields.
telemetry/logging settings.
design expectations across browsers.
helpful.
in the deployed environment.
Codebase context is not defined
Follow the guide to enable codebase context checks.
Generic: Meaningful Naming and Self-Documenting Code
Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting
Status: Passed
Generic: Robust Error Handling and Edge Case Management
Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation
Status: Passed
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
Generic: Security-First Input Validation and Data Handling
Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities
Status: Passed
Generic: Comprehensive Audit Trails
Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.
Status:
Action logging: Seeding checker configs into KV performs create operations but does not produce structured
audit logs tied to an actor/user ID, making it unclear who initiated these critical
writes.
Referred Code
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:
Log sensitivity: Seeding logs include KV key paths and filesystem paths which may reveal internal
structure; verify no sensitive values from JSON payloads or secrets are logged at any
level.
Referred Code
Compliance check up to commit dea4a76
Information disclosure
Description: The function
resolveAgentIDreads a JSON config file path provided viaconfigPathandreturns errors containing file paths and parsing details, which may disclose sensitive
file system information to logs if not handled properly.
main.go [222-249]
Referred Code
Resource exhaustion
Description: The watch retry logic treats many errors as retryable and may loop indefinitely until
context timeout; if contexts are long-lived, this could enable resource exhaustion by an
attacker causing persistent reconnection attempts.
client.go [70-177]
Referred Code
🎫 #1933
pollers fully.
it uses.
supported browsers.
interactions.
Codebase context is not defined
Follow the guide to enable codebase context checks.
Generic: Meaningful Naming and Self-Documenting Code
Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting
Status: Passed
Generic: Robust Error Handling and Edge Case Management
Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation
Status: Passed
Generic: Comprehensive Audit Trails
Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.
Status:
Retry Logging: New KV connection retry logic logs attempts and outcomes but it is unclear whether all
critical KV-related actions (e.g., seeding configs, PutIfAbsent decisions) are
consistently logged with user/action context across the system.
Referred Code
Generic: Secure Error Handling
Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.
Status:
Error Exposure: User-facing context is not shown here, but added errors (e.g., resolveAgentID and seeding)
return detailed messages that might propagate; verify they are not exposed to end users in
UIs or APIs.
Referred Code
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:
Log Sensitivity: Seeding logs include KV keys and file paths which seem safe, but headers and config
content fields in UI forms could be sensitive if later logged; ensure no secrets (e.g.,
OTEL headers) are ever written to logs.
Referred Code
Generic: Security-First Input Validation and Data Handling
Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities
Status:
UI Validation: The new forms accept numerous free-text fields (endpoints, headers JSON, file paths) with
minimal validation beyond JSON parse, which may require stronger client/server-side
validation and sanitization.
Referred Code
Imported GitHub PR comment.
Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1934#issuecomment-3514515428
Original created: 2025-11-11T00:53:09Z
PR Code Suggestions ✨
Explore these optional code suggestions:
PR scope exceeds bug fix
The PR's scope extends beyond the stated UI bug fix, introducing major backend
architectural changes. These include agent-to-KV config seeding, KV connection
retry logic, and resilient configuration watching, which should be reviewed as a
new feature.
Examples:
cmd/agent/main.go [115-117]
pkg/config/kv_client.go [76-143]
Solution Walkthrough:
Before:
After:
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies that the PR's scope is much larger than the ticket's description, introducing significant backend architectural changes like KV connection retries and agent config seeding, which are critical to review as new features.
✅
Fix race condition in PutIfAbsentSuggestion Impact:
The commit changed PutIfAbsent to call m.client.Create(ctx, key, value, ttl) and handle kv.ErrKeyExists, removing the prior Get+Put sequence and returning success only when the create succeeds atomically.code diff:
To fix a race condition in
PutIfAbsent, replace the non-atomicGetfollowed byPutwith a single, atomic "create" or "put-if-absent" operation at the KV storeclient level.
pkg/config/kv_client.go [263-271]
[Suggestion processed]Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical check-then-act race condition in
PutIfAbsent, which could lead to data overwrites in a concurrent environment, violating the function's contract.✅
Handle database errors when collecting targetsSuggestion Impact:
The commit added error checks and warning logs for failures from ListPollers and ListAgentsWithPollers, instead of silently proceeding only when err == nil.code diff:
Add error handling and logging for the
s.dbService.ListPollersands.dbService.ListAgentsWithPollerscalls to prevent silent failures and improvedebuggability.
pkg/core/api/auth.go [318-342]
[Suggestion processed]Suggestion importance[1-10]: 7
__
Why: The suggestion correctly points out that database errors are ignored, and proposes adding logging to improve observability and aid debugging, which is a valuable improvement for robustness.