carverauto/serviceradar
1
1
Fork 0
Code Issues 190 Pull requests 16 Projects Releases 173 Packages Wiki Activity Actions

cleaning up broken sql queries #2399

Merged
mfreeman451 merged 2 commits from refs/pull/2399/head into main 2025-11-03 05:56:16 +00:00
Conversation 2 Commits 2 Files changed 4 +292 -55
mfreeman451 commented 2025-11-03 05:30:48 +00:00 (Migrated from github.com)
Owner
Copy link

Imported from GitHub pull request.

Original GitHub pull request: #1918
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1918
Original created: 2025-11-03T05:30:48Z
Original updated: 2025-11-03T05:56:40Z
Original head: carverauto/serviceradar:bug/proton_max_query_size
Original base: main
Original merged: 2025-11-03T05:56:16Z 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

Description

  • Fix SQL query construction to handle large parameter sets

  • Replace parameterized queries with string interpolation for batch operations

  • Implement batching with 200-item limit to prevent query size overflow

  • Add deduplication and sorting utilities for device queries

Diagram Walkthrough

flowchart LR
  A["Large IP/Device ID Lists"] -->|"Deduplicate"| B["Deduplicated Lists"]
  B -->|"Chunk by 200"| C["Batch Chunks"]
  C -->|"Build WITH Clauses"| D["SQL WITH Statements"]
  D -->|"Execute Queries"| E["Collect Results"]
  E -->|"Sort by LastSeen"| F["Final Results"]

File Walkthrough
Relevant files
Bug fix
unified_devices.go
Implement batching and deduplication for device queries   

pkg/db/unified_devices.go

  • Add unifiedDeviceBatchLimit constant (200) to prevent query size
    overflow
  • Refactor GetUnifiedDevicesByIPsOrIDs to batch queries and deduplicate
    inputs
  • Replace inline SQL string concatenation with WITH clauses using VALUES
    syntax
  • Add helper functions: chunkStrings, dedupeStrings, buildWithClause,
    joinValueTuples, escapeLiteral
  • Extract batch query logic into new queryUnifiedDeviceBatch method
  • Sort results by LastSeen timestamp in descending order
 +134/-21
service_registry_queries.go
Convert service registry queries to string interpolation 

pkg/registry/service_registry_queries.go

  • Convert parameterized queries to string interpolation using
    fmt.Sprintf
  • Replace ? placeholders with escaped string literals in GetPoller,
    GetAgent, GetChecker
  • Update ListPollers to build filter conditions with inline string
    formatting
  • Update ListAgentsByPoller and ListCheckersByAgent to use string
    interpolation
  • Update IsKnownPoller cache query to use string interpolation
  • Add escapeLiteral and quoteStringSlice helper functions for SQL
    escaping
  • Remove parameterized args arrays and pass queries directly to Query
    methods
 +49/-34 
Imported from GitHub pull request. Original GitHub pull request: #1918 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1918 Original created: 2025-11-03T05:30:48Z Original updated: 2025-11-03T05:56:40Z Original head: carverauto/serviceradar:bug/proton_max_query_size Original base: main Original merged: 2025-11-03T05:56:16Z 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 ___ ### **Description** - Fix SQL query construction to handle large parameter sets - Replace parameterized queries with string interpolation for batch operations - Implement batching with 200-item limit to prevent query size overflow - Add deduplication and sorting utilities for device queries ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Large IP/Device ID Lists"] -->|"Deduplicate"| B["Deduplicated Lists"] B -->|"Chunk by 200"| C["Batch Chunks"] C -->|"Build WITH Clauses"| D["SQL WITH Statements"] D -->|"Execute Queries"| E["Collect Results"] E -->|"Sort by LastSeen"| F["Final Results"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>unified_devices.go</strong><dd><code>Implement batching and deduplication for device queries</code>&nbsp; &nbsp; </dd></summary> <hr> pkg/db/unified_devices.go <ul><li>Add <code>unifiedDeviceBatchLimit</code> constant (200) to prevent query size <br>overflow<br> <li> Refactor <code>GetUnifiedDevicesByIPsOrIDs</code> to batch queries and deduplicate <br>inputs<br> <li> Replace inline SQL string concatenation with <code>WITH</code> clauses using <code>VALUES</code> <br>syntax<br> <li> Add helper functions: <code>chunkStrings</code>, <code>dedupeStrings</code>, <code>buildWithClause</code>, <br><code>joinValueTuples</code>, <code>escapeLiteral</code><br> <li> Extract batch query logic into new <code>queryUnifiedDeviceBatch</code> method<br> <li> Sort results by <code>LastSeen</code> timestamp in descending order</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1918/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679">+134/-21</a></td> </tr> <tr> <td> <details> <summary><strong>service_registry_queries.go</strong><dd><code>Convert service registry queries to string interpolation</code>&nbsp; </dd></summary> <hr> pkg/registry/service_registry_queries.go <ul><li>Convert parameterized queries to string interpolation using <br><code>fmt.Sprintf</code><br> <li> Replace <code>?</code> placeholders with escaped string literals in <code>GetPoller</code>, <br><code>GetAgent</code>, <code>GetChecker</code><br> <li> Update <code>ListPollers</code> to build filter conditions with inline string <br>formatting<br> <li> Update <code>ListAgentsByPoller</code> and <code>ListCheckersByAgent</code> to use string <br>interpolation<br> <li> Update <code>IsKnownPoller</code> cache query to use string interpolation<br> <li> Add <code>escapeLiteral</code> and <code>quoteStringSlice</code> helper functions for SQL <br>escaping<br> <li> Remove parameterized args arrays and pass queries directly to <code>Query</code> <br>methods</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9">+49/-34</a>&nbsp; </td> </tr> </table></td></tr></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2025-11-03 05:31:24 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1918#issuecomment-3478960513
Original created: 2025-11-03T05:31:24Z

PR Compliance Guide 🔍

(Compliance updated until commit github.com/carverauto/serviceradar@5ea76c20c8)

Below is a summary of compliance checks for this PR:

Security Compliance
⚪
SQL injection risk

Description: SQL queries are constructed via string interpolation with user-controlled identifiers
(e.g., poller_id) instead of parameterized queries, risking SQL injection despite basic
single-quote escaping that may be insufficient for all SQL dialect edge cases.
service_registry_queries.go [19-29]

Referred Code 
query := fmt.Sprintf(`SELECT
	poller_id, component_id, status, registration_source,
	first_registered, first_seen, last_seen, metadata,
	spiffe_identity, created_by, agent_count, checker_count
FROM table(pollers)
WHERE poller_id = '%s'
ORDER BY _tp_time DESC
LIMIT 1`, escapeLiteral(pollerID))

row := r.db.Conn.QueryRow(ctx, query)
SQL injection risk

Description: Agent lookup builds SQL with interpolated agent_id using custom escaping rather than
parameters, exposing potential injection vectors if escaping is bypassed or incomplete.
service_registry_queries.go [75-86]

Referred Code 
func (r *ServiceRegistry) GetAgent(ctx context.Context, agentID string) (*RegisteredAgent, error) {
	query := fmt.Sprintf(`SELECT
		agent_id, poller_id, component_id, status, registration_source,
		first_registered, first_seen, last_seen, metadata,
		spiffe_identity, created_by, checker_count
	FROM table(agents)
	WHERE agent_id = '%s'
	ORDER BY _tp_time DESC
	LIMIT 1`, escapeLiteral(agentID))

	row := r.db.Conn.QueryRow(ctx, query)
SQL injection risk

Description: Checker lookup constructs SQL via fmt.Sprintf with checker_id embedded directly, relying
on ad-hoc escaping instead of prepared statements.
service_registry_queries.go [133-143]

Referred Code 
query := fmt.Sprintf(`SELECT
	checker_id, agent_id, poller_id, checker_kind, component_id,
	status, registration_source, first_registered, first_seen, last_seen,
	metadata, spiffe_identity, created_by
FROM table(checkers)
WHERE checker_id = '%s'
ORDER BY _tp_time DESC
LIMIT 1`, escapeLiteral(checkerID))

row := r.db.Conn.QueryRow(ctx, query)
SQL injection risk

Description: ListAgentsByPoller interpolates poller_id into the WHERE clause; although escaped, direct
string construction is vulnerable across dialect differences and may allow injection in
complex inputs.
service_registry_queries.go [288-298]

Referred Code 
query := fmt.Sprintf(`SELECT
	agent_id, poller_id, component_id, status, registration_source,
	first_registered, first_seen, last_seen, metadata,
	spiffe_identity, created_by, checker_count
FROM agents
FINAL
WHERE poller_id = '%s'
ORDER BY first_registered DESC`, escapeLiteral(pollerID))

rows, err := r.db.Conn.Query(ctx, query)
if err != nil {
SQL injection risk

Description: ListCheckersByAgent interpolates agent_id into SQL using custom escaping rather than
parameterization, creating an injection risk surface.
service_registry_queries.go [358-368]

Referred Code 
query := fmt.Sprintf(`SELECT
	checker_id, agent_id, poller_id, checker_kind, component_id,
	status, registration_source, first_registered, first_seen, last_seen,
	metadata, spiffe_identity, created_by
FROM checkers
FINAL
WHERE agent_id = '%s'
ORDER BY first_registered DESC`, escapeLiteral(agentID))

rows, err := r.db.Conn.Query(ctx, query)
if err != nil {
SQL injection risk

Description: Batch device query builds WITH and WHERE clauses via string concatenation of values
(device IDs and IPs) with only quote-doubling, which may be bypassed or unsafe in certain
SQL engines and prevents the DB from optimizing via parameters.
unified_devices.go [280-309]

Referred Code 
func (db *DB) queryUnifiedDeviceBatch(ctx context.Context, deviceIDs, ips []string) ([]*models.UnifiedDevice, error) {
	var conditions []string
	var withClauses []string

	if len(deviceIDs) > 0 {
		withClauses = append(withClauses, fmt.Sprintf(`device_candidates AS (
        SELECT device_id
        FROM VALUES('device_id string', %s)
    )`, joinValueTuples(deviceIDs)))
		conditions = append(conditions, "device_id IN (SELECT device_id FROM device_candidates)")
	}

	if len(ips) > 0 {
		withClauses = append(withClauses, fmt.Sprintf(`ip_candidates AS (
        SELECT ip
        FROM VALUES('ip string', %s)
    )`, joinValueTuples(ips)))
		conditions = append(conditions, "ip IN (SELECT ip FROM ip_candidates)")
	}

	query := fmt.Sprintf(`%sSELECT


 ... (clipped 9 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

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: 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

⚪
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing Auditing: The newly added query-building and batch retrieval functions perform database reads and
updates without adding any audit logging for critical actions, and the diff does not show
existing logging around these paths.

Referred Code 
query := fmt.Sprintf(`SELECT
	poller_id, component_id, status, registration_source,
	first_registered, first_seen, last_seen, metadata,
	spiffe_identity, created_by, agent_count, checker_count
FROM table(pollers)
WHERE poller_id = '%s'
ORDER BY _tp_time DESC
LIMIT 1`, escapeLiteral(pollerID))

row := r.db.Conn.QueryRow(ctx, query)

var (
	poller       RegisteredPoller
	metadataJSON string
	firstSeenPtr *time.Time
	lastSeenPtr  *time.Time
	statusStr    string
	sourceStr    string
)

err := row.Scan(


 ... (clipped 656 lines)
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Partial Errors: Batch querying and collection paths return underlying errors but do not log or annotate
which IDs/IPs failed, potentially hindering debugging for partial failures and edge cases.

Referred Code 
ips = dedupeStrings(ips)
deviceIDs = dedupeStrings(deviceIDs)

if len(ips) == 0 && len(deviceIDs) == 0 {
	return nil, nil
}

resultByID := make(map[string]*models.UnifiedDevice)

collect := func(batch []*models.UnifiedDevice) {
	for _, device := range batch {
		if device == nil {
			continue
		}
		resultByID[device.DeviceID] = device
	}
}

queryBatch := func(ids, address []string) error {
	if len(ids) == 0 && len(address) == 0 {
		return nil


 ... (clipped 73 lines)
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Unsafe SQL Build: New code replaces parameterized queries with string interpolation and manual escaping
(e.g., WHERE clauses using fmt.Sprintf), which reduces defense-in-depth and may still risk
SQL injection depending on the database engine and context.

Referred Code 
query := fmt.Sprintf(`SELECT
	poller_id, component_id, status, registration_source,
	first_registered, first_seen, last_seen, metadata,
	spiffe_identity, created_by, agent_count, checker_count
FROM table(pollers)
WHERE poller_id = '%s'
ORDER BY _tp_time DESC
LIMIT 1`, escapeLiteral(pollerID))

row := r.db.Conn.QueryRow(ctx, query)

var (
	poller       RegisteredPoller
	metadataJSON string
	firstSeenPtr *time.Time
	lastSeenPtr  *time.Time
	statusStr    string
	sourceStr    string
)

err := row.Scan(


 ... (clipped 327 lines)
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Previous compliance checks

Compliance check up to commit c828026
Security Compliance
🔴
SQL injection

Description: Unparameterized SQL query builds WHERE clause with interpolated poller ID using
fmt.Sprintf, allowing SQL injection via crafted pollerID despite basic quote-escaping.
service_registry_queries.go [19-29]

Referred Code 
query := fmt.Sprintf(`SELECT
	poller_id, component_id, status, registration_source,
	first_registered, first_seen, last_seen, metadata,
	spiffe_identity, created_by, agent_count, checker_count
FROM table(pollers)
WHERE poller_id = '%s'
ORDER BY _tp_time DESC
LIMIT 1`, escapeLiteral(pollerID))

row := r.db.Conn.QueryRow(ctx, query)
SQL injection

Description: Unparameterized SQL in GetAgent uses string interpolation of agentID into the query,
risking SQL injection even with manual quote escaping.
service_registry_queries.go [76-86]

Referred Code 
query := fmt.Sprintf(`SELECT
	agent_id, poller_id, component_id, status, registration_source,
	first_registered, first_seen, last_seen, metadata,
	spiffe_identity, created_by, checker_count
FROM table(agents)
WHERE agent_id = '%s'
ORDER BY _tp_time DESC
LIMIT 1`, escapeLiteral(agentID))

row := r.db.Conn.QueryRow(ctx, query)
SQL injection

Description: GetChecker constructs SQL by interpolating checkerID directly, enabling SQL injection.
service_registry_queries.go [133-143]

Referred Code 
query := fmt.Sprintf(`SELECT
	checker_id, agent_id, poller_id, checker_kind, component_id,
	status, registration_source, first_registered, first_seen, last_seen,
	metadata, spiffe_identity, created_by
FROM table(checkers)
WHERE checker_id = '%s'
ORDER BY _tp_time DESC
LIMIT 1`, escapeLiteral(checkerID))

row := r.db.Conn.QueryRow(ctx, query)
SQL injection

Description: ListAgentsByPoller interpolates pollerID into SQL without parameters, enabling SQL
injection.
service_registry_queries.go [288-298]

Referred Code 
query := fmt.Sprintf(`SELECT
	agent_id, poller_id, component_id, status, registration_source,
	first_registered, first_seen, last_seen, metadata,
	spiffe_identity, created_by, checker_count
FROM agents
FINAL
WHERE poller_id = '%s'
ORDER BY first_registered DESC`, escapeLiteral(pollerID))

rows, err := r.db.Conn.Query(ctx, query)
if err != nil {
SQL injection

Description: ListCheckersByAgent interpolates agentID into SQL query, creating SQL injection risk.
service_registry_queries.go [358-368]

Referred Code 
query := fmt.Sprintf(`SELECT
	checker_id, agent_id, poller_id, checker_kind, component_id,
	status, registration_source, first_registered, first_seen, last_seen,
	metadata, spiffe_identity, created_by
FROM checkers
FINAL
WHERE agent_id = '%s'
ORDER BY first_registered DESC`, escapeLiteral(agentID))

rows, err := r.db.Conn.Query(ctx, query)
if err != nil {
SQL injection

Description: IsKnownPoller builds SQL with interpolated pollerID; COUNT query is susceptible to SQL
injection.
service_registry_queries.go [557-564]

Referred Code 
// Query database
query := fmt.Sprintf(`SELECT COUNT(*) FROM pollers
		  FINAL
		  WHERE poller_id = '%s' AND status IN ('pending', 'active')`, escapeLiteral(pollerID))

var count int
row := r.db.Conn.QueryRow(ctx, query)
if err := row.Scan(&count); err != nil {
⚪
SQL injection

Description: Batch query builder composes SQL with VALUES lists from user-controlled IDs/IPs using
manual escaping, allowing SQL injection or syntax manipulation if escaping is
insufficient.
unified_devices.go [284-308]

Referred Code 
	if len(deviceIDs) > 0 {
		withClauses = append(withClauses, fmt.Sprintf(`device_candidates AS (
        SELECT device_id
        FROM VALUES('device_id string', %s)
    )`, joinValueTuples(deviceIDs)))
		conditions = append(conditions, "device_id IN (SELECT device_id FROM device_candidates)")
	}

	if len(ips) > 0 {
		withClauses = append(withClauses, fmt.Sprintf(`ip_candidates AS (
        SELECT ip
        FROM VALUES('ip string', %s)
    )`, joinValueTuples(ips)))
		conditions = append(conditions, "ip IN (SELECT ip FROM ip_candidates)")
	}

	query := fmt.Sprintf(`%sSELECT
        device_id, ip, poller_id, hostname, mac, discovery_sources,
        is_available, first_seen, last_seen, metadata, agent_id, device_type, 
        service_type, service_status, last_heartbeat, os_info, version_info
    FROM table(unified_devices)


 ... (clipped 4 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

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:
SQL injection risk: Parameterized queries were replaced with string interpolation across multiple methods,
introducing SQL injection risk despite basic quote escaping.

Referred Code 
query := fmt.Sprintf(`SELECT
	poller_id, component_id, status, registration_source,
	first_registered, first_seen, last_seen, metadata,
	spiffe_identity, created_by, agent_count, checker_count
FROM table(pollers)
WHERE poller_id = '%s'
ORDER BY _tp_time DESC
LIMIT 1`, escapeLiteral(pollerID))

row := r.db.Conn.QueryRow(ctx, query)

var (
	poller       RegisteredPoller
	metadataJSON string
	firstSeenPtr *time.Time
	lastSeenPtr  *time.Time
	statusStr    string
	sourceStr    string
)
⚪
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing auditing: New database read operations and batch queries are added without any explicit audit
logging of access to potentially sensitive device or registry data.

Referred Code 
query := fmt.Sprintf(`SELECT
	poller_id, component_id, status, registration_source,
	first_registered, first_seen, last_seen, metadata,
	spiffe_identity, created_by, agent_count, checker_count
FROM table(pollers)
WHERE poller_id = '%s'
ORDER BY _tp_time DESC
LIMIT 1`, escapeLiteral(pollerID))

row := r.db.Conn.QueryRow(ctx, query)

var (
	poller       RegisteredPoller
	metadataJSON string
	firstSeenPtr *time.Time
	lastSeenPtr  *time.Time
	statusStr    string
	sourceStr    string
)
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Edge cases partial: While batching and deduplication are added, database query errors are wrapped generically
and there is no handling for overly large input sets beyond chunking nor contextual
logging for failures per batch.

Referred Code 
	if len(ids) == 0 && len(address) == 0 {
		return nil
	}

	devices, err := db.queryUnifiedDeviceBatch(ctx, ids, address)
	if err != nil {
		return err
	}

	collect(devices)

	return nil
}

for _, chunk := range chunkStrings(deviceIDs, unifiedDeviceBatchLimit) {
	if err := queryBatch(chunk, nil); err != nil {
		return nil, err
	}
}

for _, chunk := range chunkStrings(ips, unifiedDeviceBatchLimit) {


 ... (clipped 53 lines)
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:
No logging shown: The new code paths add batching and queries but do not show structured or redacted logging
for failures or key actions, which may be acceptable if logging exists elsewhere.

Referred Code 
	}

	if err := rows.Err(); err != nil {
		return nil, fmt.Errorf("%w: %w", errIterRows, err)
	}

	return devices, nil
}
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1918#issuecomment-3478960513 Original created: 2025-11-03T05:31:24Z --- ## PR Compliance Guide 🔍 <!-- https://github.com/carverauto/serviceradar/commit/5ea76c20c84031885635e4ca25a39fa67d1e0474 --> #### (Compliance updated until commit https://github.com/carverauto/serviceradar/commit/5ea76c20c84031885635e4ca25a39fa67d1e0474) Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=6>⚪</td> <td><details><summary><strong>SQL injection risk </strong></summary><br> <b>Description:</b> SQL queries are constructed via string interpolation with user-controlled identifiers <br>(e.g., poller_id) instead of parameterized queries, risking SQL injection despite basic <br>single-quote escaping that may be insufficient for all SQL dialect edge cases.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R19-R29'>service_registry_queries.go [19-29]</a></strong><br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT poller_id, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by, agent_count, checker_count FROM table(pollers) WHERE poller_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(pollerID)) row := r.db.Conn.QueryRow(ctx, query) ``` </details></details></td></tr> <tr><td><details><summary><strong>SQL injection risk </strong></summary><br> <b>Description:</b> Agent lookup builds SQL with interpolated agent_id using custom escaping rather than <br>parameters, exposing potential injection vectors if escaping is bypassed or incomplete.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R75-R86'>service_registry_queries.go [75-86]</a></strong><br> <details open><summary>Referred Code</summary> ```go func (r *ServiceRegistry) GetAgent(ctx context.Context, agentID string) (*RegisteredAgent, error) { query := fmt.Sprintf(`SELECT agent_id, poller_id, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by, checker_count FROM table(agents) WHERE agent_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(agentID)) row := r.db.Conn.QueryRow(ctx, query) ``` </details></details></td></tr> <tr><td><details><summary><strong>SQL injection risk </strong></summary><br> <b>Description:</b> Checker lookup constructs SQL via fmt.Sprintf with checker_id embedded directly, relying <br>on ad-hoc escaping instead of prepared statements.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R133-R143'>service_registry_queries.go [133-143]</a></strong><br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT checker_id, agent_id, poller_id, checker_kind, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by FROM table(checkers) WHERE checker_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(checkerID)) row := r.db.Conn.QueryRow(ctx, query) ``` </details></details></td></tr> <tr><td><details><summary><strong>SQL injection risk </strong></summary><br> <b>Description:</b> ListAgentsByPoller interpolates poller_id into the WHERE clause; although escaped, direct <br>string construction is vulnerable across dialect differences and may allow injection in <br>complex inputs.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R288-R298'>service_registry_queries.go [288-298]</a></strong><br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT agent_id, poller_id, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by, checker_count FROM agents FINAL WHERE poller_id = '%s' ORDER BY first_registered DESC`, escapeLiteral(pollerID)) rows, err := r.db.Conn.Query(ctx, query) if err != nil { ``` </details></details></td></tr> <tr><td><details><summary><strong>SQL injection risk </strong></summary><br> <b>Description:</b> ListCheckersByAgent interpolates agent_id into SQL using custom escaping rather than <br>parameterization, creating an injection risk surface.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R358-R368'>service_registry_queries.go [358-368]</a></strong><br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT checker_id, agent_id, poller_id, checker_kind, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by FROM checkers FINAL WHERE agent_id = '%s' ORDER BY first_registered DESC`, escapeLiteral(agentID)) rows, err := r.db.Conn.Query(ctx, query) if err != nil { ``` </details></details></td></tr> <tr><td><details><summary><strong>SQL injection risk </strong></summary><br> <b>Description:</b> Batch device query builds WITH and WHERE clauses via string concatenation of values <br>(device IDs and IPs) with only quote-doubling, which may be bypassed or unsafe in certain <br>SQL engines and prevents the DB from optimizing via parameters.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R280-R309'>unified_devices.go [280-309]</a></strong><br> <details open><summary>Referred Code</summary> ```go func (db *DB) queryUnifiedDeviceBatch(ctx context.Context, deviceIDs, ips []string) ([]*models.UnifiedDevice, error) { var conditions []string var withClauses []string if len(deviceIDs) > 0 { withClauses = append(withClauses, fmt.Sprintf(`device_candidates AS ( SELECT device_id FROM VALUES('device_id string', %s) )`, joinValueTuples(deviceIDs))) conditions = append(conditions, "device_id IN (SELECT device_id FROM device_candidates)") } if len(ips) > 0 { withClauses = append(withClauses, fmt.Sprintf(`ip_candidates AS ( SELECT ip FROM VALUES('ip string', %s) )`, joinValueTuples(ips))) conditions = append(conditions, "ip IN (SELECT ip FROM ip_candidates)") } query := fmt.Sprintf(`%sSELECT ... (clipped 9 lines) ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </summary></strong> - [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true --> </details></td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></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'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=3>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> </details></td></tr> <tr><td rowspan=3>⚪</td> <td><details> <summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R19-R695'><strong>Missing Auditing</strong></a>: The newly added query-building and batch retrieval functions perform database reads and <br>updates without adding any audit logging for critical actions, and the diff does not show <br>existing logging around these paths.<br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT poller_id, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by, agent_count, checker_count FROM table(pollers) WHERE poller_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(pollerID)) row := r.db.Conn.QueryRow(ctx, query) var ( poller RegisteredPoller metadataJSON string firstSeenPtr *time.Time lastSeenPtr *time.Time statusStr string sourceStr string ) err := row.Scan( ... (clipped 656 lines) ``` </details></details></td></tr> <tr><td><details> <summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R219-R312'><strong>Partial Errors</strong></a>: Batch querying and collection paths return underlying errors but do not log or annotate <br>which IDs/IPs failed, potentially hindering debugging for partial failures and edge cases.<br> <details open><summary>Referred Code</summary> ```go ips = dedupeStrings(ips) deviceIDs = dedupeStrings(deviceIDs) if len(ips) == 0 && len(deviceIDs) == 0 { return nil, nil } resultByID := make(map[string]*models.UnifiedDevice) collect := func(batch []*models.UnifiedDevice) { for _, device := range batch { if device == nil { continue } resultByID[device.DeviceID] = device } } queryBatch := func(ids, address []string) error { if len(ids) == 0 && len(address) == 0 { return nil ... (clipped 73 lines) ``` </details></details></td></tr> <tr><td><details> <summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R19-R366'><strong>Unsafe SQL Build</strong></a>: New code replaces parameterized queries with string interpolation and manual escaping <br>(e.g., WHERE clauses using fmt.Sprintf), which reduces defense-in-depth and may still risk <br>SQL injection depending on the database engine and context.<br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT poller_id, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by, agent_count, checker_count FROM table(pollers) WHERE poller_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(pollerID)) row := r.db.Conn.QueryRow(ctx, query) var ( poller RegisteredPoller metadataJSON string firstSeenPtr *time.Time lastSeenPtr *time.Time statusStr string sourceStr string ) err := row.Scan( ... (clipped 327 lines) ``` </details></details></td></tr> <tr><td align="center" colspan="2"> <!-- placeholder --> <!-- /compliance --update_compliance=true --> </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details> ___ #### Previous compliance checks <details> <summary>Compliance check up to commit <a href='https://github.com/carverauto/serviceradar/commit/c828026bd56f69fa4637f89af394e26fbaf53603'>c828026</a></summary><br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=6>🔴</td> <td><details><summary><strong>SQL injection </strong></summary><br> <b>Description:</b> Unparameterized SQL query builds WHERE clause with interpolated poller ID using <br>fmt.Sprintf, allowing SQL injection via crafted pollerID despite basic quote-escaping.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R19-R29'>service_registry_queries.go [19-29]</a></strong><br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT poller_id, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by, agent_count, checker_count FROM table(pollers) WHERE poller_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(pollerID)) row := r.db.Conn.QueryRow(ctx, query) ``` </details></details></td></tr> <tr><td><details><summary><strong>SQL injection </strong></summary><br> <b>Description:</b> Unparameterized SQL in GetAgent uses string interpolation of agentID into the query, <br>risking SQL injection even with manual quote escaping.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R76-R86'>service_registry_queries.go [76-86]</a></strong><br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT agent_id, poller_id, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by, checker_count FROM table(agents) WHERE agent_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(agentID)) row := r.db.Conn.QueryRow(ctx, query) ``` </details></details></td></tr> <tr><td><details><summary><strong>SQL injection </strong></summary><br> <b>Description:</b> GetChecker constructs SQL by interpolating checkerID directly, enabling SQL injection. <strong><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R133-R143'>service_registry_queries.go [133-143]</a></strong><br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT checker_id, agent_id, poller_id, checker_kind, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by FROM table(checkers) WHERE checker_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(checkerID)) row := r.db.Conn.QueryRow(ctx, query) ``` </details></details></td></tr> <tr><td><details><summary><strong>SQL injection </strong></summary><br> <b>Description:</b> ListAgentsByPoller interpolates pollerID into SQL without parameters, enabling SQL <br>injection.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R288-R298'>service_registry_queries.go [288-298]</a></strong><br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT agent_id, poller_id, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by, checker_count FROM agents FINAL WHERE poller_id = '%s' ORDER BY first_registered DESC`, escapeLiteral(pollerID)) rows, err := r.db.Conn.Query(ctx, query) if err != nil { ``` </details></details></td></tr> <tr><td><details><summary><strong>SQL injection </strong></summary><br> <b>Description:</b> ListCheckersByAgent interpolates agentID into SQL query, creating SQL injection risk. <strong><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R358-R368'>service_registry_queries.go [358-368]</a></strong><br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT checker_id, agent_id, poller_id, checker_kind, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by FROM checkers FINAL WHERE agent_id = '%s' ORDER BY first_registered DESC`, escapeLiteral(agentID)) rows, err := r.db.Conn.Query(ctx, query) if err != nil { ``` </details></details></td></tr> <tr><td><details><summary><strong>SQL injection </strong></summary><br> <b>Description:</b> IsKnownPoller builds SQL with interpolated pollerID; COUNT query is susceptible to SQL <br>injection.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R557-R564'>service_registry_queries.go [557-564]</a></strong><br> <details open><summary>Referred Code</summary> ```go // Query database query := fmt.Sprintf(`SELECT COUNT(*) FROM pollers FINAL WHERE poller_id = '%s' AND status IN ('pending', 'active')`, escapeLiteral(pollerID)) var count int row := r.db.Conn.QueryRow(ctx, query) if err := row.Scan(&count); err != nil { ``` </details></details></td></tr> <tr><td rowspan=1>⚪</td> <td><details><summary><strong>SQL injection </strong></summary><br> <b>Description:</b> Batch query builder composes SQL with VALUES lists from user-controlled IDs/IPs using <br>manual escaping, allowing SQL injection or syntax manipulation if escaping is <br>insufficient.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R284-R308'>unified_devices.go [284-308]</a></strong><br> <details open><summary>Referred Code</summary> ```go if len(deviceIDs) > 0 { withClauses = append(withClauses, fmt.Sprintf(`device_candidates AS ( SELECT device_id FROM VALUES('device_id string', %s) )`, joinValueTuples(deviceIDs))) conditions = append(conditions, "device_id IN (SELECT device_id FROM device_candidates)") } if len(ips) > 0 { withClauses = append(withClauses, fmt.Sprintf(`ip_candidates AS ( SELECT ip FROM VALUES('ip string', %s) )`, joinValueTuples(ips))) conditions = append(conditions, "ip IN (SELECT ip FROM ip_candidates)") } query := fmt.Sprintf(`%sSELECT device_id, ip, poller_id, hostname, mac, discovery_sources, is_available, first_seen, last_seen, metadata, agent_id, device_type, service_type, service_status, last_heartbeat, os_info, version_info FROM table(unified_devices) ... (clipped 4 lines) ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </summary></strong> - [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true --> </details></td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></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'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=2>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> </details></td></tr> <tr><td rowspan=1>🔴</td> <td><details> <summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R19-R38'><strong>SQL injection risk</strong></a>: Parameterized queries were replaced with string interpolation across multiple methods, <br>introducing SQL injection risk despite basic quote escaping.<br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT poller_id, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by, agent_count, checker_count FROM table(pollers) WHERE poller_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(pollerID)) row := r.db.Conn.QueryRow(ctx, query) var ( poller RegisteredPoller metadataJSON string firstSeenPtr *time.Time lastSeenPtr *time.Time statusStr string sourceStr string ) ``` </details></details></td></tr> <tr><td rowspan=3>⚪</td> <td><details> <summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R19-R37'><strong>Missing auditing</strong></a>: New database read operations and batch queries are added without any explicit audit <br>logging of access to potentially sensitive device or registry data.<br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT poller_id, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by, agent_count, checker_count FROM table(pollers) WHERE poller_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(pollerID)) row := r.db.Conn.QueryRow(ctx, query) var ( poller RegisteredPoller metadataJSON string firstSeenPtr *time.Time lastSeenPtr *time.Time statusStr string sourceStr string ) ``` </details></details></td></tr> <tr><td><details> <summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R238-R311'><strong>Edge cases partial</strong></a>: While batching and deduplication are added, database query errors are wrapped generically <br>and there is no handling for overly large input sets beyond chunking nor contextual <br>logging for failures per batch.<br> <details open><summary>Referred Code</summary> ```go if len(ids) == 0 && len(address) == 0 { return nil } devices, err := db.queryUnifiedDeviceBatch(ctx, ids, address) if err != nil { return err } collect(devices) return nil } for _, chunk := range chunkStrings(deviceIDs, unifiedDeviceBatchLimit) { if err := queryBatch(chunk, nil); err != nil { return nil, err } } for _, chunk := range chunkStrings(ips, unifiedDeviceBatchLimit) { ... (clipped 53 lines) ``` </details></details></td></tr> <tr><td><details> <summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1918/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R324-R331'><strong>No logging shown</strong></a>: The new code paths add batching and queries but do not show structured or redacted logging <br>for failures or key actions, which may be acceptable if logging exists elsewhere.<br> <details open><summary>Referred Code</summary> ```go } if err := rows.Err(); err != nil { return nil, fmt.Errorf("%w: %w", errIterRows, err) } return devices, nil } ``` </details></details></td></tr> <tr><td align="center" colspan="2"> <!-- /compliance --update_compliance=true --> </td></tr></tbody></table> </details>
qodo-code-review[bot] commented 2025-11-03 05:32:40 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1918#issuecomment-3478963183
Original created: 2025-11-03T05:32:40Z

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Security
Revert to using parameterized queries

Revert to using secure parameterized queries instead of manual string formatting
with fmt.Sprintf and escapeLiteral in GetPoller and similar functions to prevent
SQL injection vulnerabilities.

pkg/registry/service_registry_queries.go [18-32]

 // GetPoller retrieves a poller by ID.
 func (r *ServiceRegistry) GetPoller(ctx context.Context, pollerID string) (*RegisteredPoller, error) {
-	query := fmt.Sprintf(`SELECT
+	query := `SELECT
 		poller_id, component_id, status, registration_source,
 		first_registered, first_seen, last_seen, metadata,
 		spiffe_identity, created_by, agent_count, checker_count
 	FROM table(pollers)
-	WHERE poller_id = '%s'
+	WHERE poller_id = ?
 	ORDER BY _tp_time DESC
-	LIMIT 1`, escapeLiteral(pollerID))
+	LIMIT 1`
 
-	row := r.db.Conn.QueryRow(ctx, query)
+	row := r.db.Conn.QueryRow(ctx, query, pollerID)
 
 	var (
 		poller       RegisteredPoller
 ...

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical SQL injection vulnerability introduced by replacing secure parameterized queries with manual string formatting, which is a major security regression.

High
Use parameterized queries for batch lookups

In queryUnifiedDeviceBatch, replace manual query construction and escaping with
secure parameterized queries for IN clauses to prevent SQL injection and
simplify the code.

pkg/db/unified_devices.go [280-311]

 func (db *DB) queryUnifiedDeviceBatch(ctx context.Context, deviceIDs, ips []string) ([]*models.UnifiedDevice, error) {
 	var conditions []string
-	var withClauses []string
+	var args []interface{}
 
 	if len(deviceIDs) > 0 {
-		withClauses = append(withClauses, fmt.Sprintf(`device_candidates AS (
-        SELECT device_id
-        FROM VALUES('device_id string', %s)
-    )`, joinValueTuples(deviceIDs)))
-		conditions = append(conditions, "device_id IN (SELECT device_id FROM device_candidates)")
+		conditions = append(conditions, "device_id IN (?)")
+		args = append(args, deviceIDs)
 	}
 
 	if len(ips) > 0 {
-		withClauses = append(withClauses, fmt.Sprintf(`ip_candidates AS (
-        SELECT ip
-        FROM VALUES('ip string', %s)
-    )`, joinValueTuples(ips)))
-		conditions = append(conditions, "ip IN (SELECT ip FROM ip_candidates)")
+		conditions = append(conditions, "ip IN (?)")
+		args = append(args, ips)
 	}
 
-	query := fmt.Sprintf(`%sSELECT
+	if len(conditions) == 0 {
+		return nil, nil
+	}
+
+	query := fmt.Sprintf(`SELECT
         device_id, ip, poller_id, hostname, mac, discovery_sources,
-        is_available, first_seen, last_seen, metadata, agent_id, device_type, 
+        is_available, first_seen, last_seen, metadata, agent_id, device_type,
         service_type, service_status, last_heartbeat, os_info, version_info
     FROM table(unified_devices)
     WHERE %s
-    ORDER BY _tp_time DESC`, buildWithClause(withClauses), strings.Join(conditions, " OR "))
+    ORDER BY _tp_time DESC`, strings.Join(conditions, " OR "))
 
-	rows, err := db.Conn.Query(ctx, query)
+	rows, err := db.Conn.Query(ctx, query, args...)
 	if err != nil {
 		return nil, fmt.Errorf("failed to batch query unified devices: %w", err)
 	}
 ...

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical SQL injection vulnerability introduced by using manual string formatting and escaping, and proposes a much safer and simpler implementation using parameterized queries.

High
  • Update
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1918#issuecomment-3478963183 Original created: 2025-11-03T05:32:40Z --- ## PR Code Suggestions ✨ <!-- c828026 --> Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=2>Security</td> <td> <details><summary>Revert to using parameterized queries</summary> ___ **Revert to using secure parameterized queries instead of manual string formatting <br>with <code>fmt.Sprintf</code> and <code>escapeLiteral</code> in <code>GetPoller</code> and similar functions to prevent <br>SQL injection vulnerabilities.** [pkg/registry/service_registry_queries.go [18-32]](https://github.com/carverauto/serviceradar/pull/1918/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9R18-R32) ```diff // GetPoller retrieves a poller by ID. func (r *ServiceRegistry) GetPoller(ctx context.Context, pollerID string) (*RegisteredPoller, error) { - query := fmt.Sprintf(`SELECT + query := `SELECT poller_id, component_id, status, registration_source, first_registered, first_seen, last_seen, metadata, spiffe_identity, created_by, agent_count, checker_count FROM table(pollers) - WHERE poller_id = '%s' + WHERE poller_id = ? ORDER BY _tp_time DESC - LIMIT 1`, escapeLiteral(pollerID)) + LIMIT 1` - row := r.db.Conn.QueryRow(ctx, query) + row := r.db.Conn.QueryRow(ctx, query, pollerID) var ( poller RegisteredPoller ... ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 10</summary> __ Why: The suggestion correctly identifies a critical SQL injection vulnerability introduced by replacing secure parameterized queries with manual string formatting, which is a major security regression. </details></details></td><td align=center>High </td></tr><tr><td> <details><summary>Use parameterized queries for batch lookups</summary> ___ **In <code>queryUnifiedDeviceBatch</code>, replace manual query construction and escaping with <br>secure parameterized queries for <code>IN</code> clauses to prevent SQL injection and <br>simplify the code.** [pkg/db/unified_devices.go [280-311]](https://github.com/carverauto/serviceradar/pull/1918/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R280-R311) ```diff func (db *DB) queryUnifiedDeviceBatch(ctx context.Context, deviceIDs, ips []string) ([]*models.UnifiedDevice, error) { var conditions []string - var withClauses []string + var args []interface{} if len(deviceIDs) > 0 { - withClauses = append(withClauses, fmt.Sprintf(`device_candidates AS ( - SELECT device_id - FROM VALUES('device_id string', %s) - )`, joinValueTuples(deviceIDs))) - conditions = append(conditions, "device_id IN (SELECT device_id FROM device_candidates)") + conditions = append(conditions, "device_id IN (?)") + args = append(args, deviceIDs) } if len(ips) > 0 { - withClauses = append(withClauses, fmt.Sprintf(`ip_candidates AS ( - SELECT ip - FROM VALUES('ip string', %s) - )`, joinValueTuples(ips))) - conditions = append(conditions, "ip IN (SELECT ip FROM ip_candidates)") + conditions = append(conditions, "ip IN (?)") + args = append(args, ips) } - query := fmt.Sprintf(`%sSELECT + if len(conditions) == 0 { + return nil, nil + } + + query := fmt.Sprintf(`SELECT device_id, ip, poller_id, hostname, mac, discovery_sources, - is_available, first_seen, last_seen, metadata, agent_id, device_type, + is_available, first_seen, last_seen, metadata, agent_id, device_type, service_type, service_status, last_heartbeat, os_info, version_info FROM table(unified_devices) WHERE %s - ORDER BY _tp_time DESC`, buildWithClause(withClauses), strings.Join(conditions, " OR ")) + ORDER BY _tp_time DESC`, strings.Join(conditions, " OR ")) - rows, err := db.Conn.Query(ctx, query) + rows, err := db.Conn.Query(ctx, query, args...) if err != nil { return nil, fmt.Errorf("failed to batch query unified devices: %w", err) } ... ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 10</summary> __ Why: The suggestion correctly identifies a critical SQL injection vulnerability introduced by using manual string formatting and escaping, and proposes a much safer and simpler implementation using parameterized queries. </details></details></td><td align=center>High </td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /improve_multi --more_suggestions=true --> </td><td></td></tr></tbody></table>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
1week

   
2weeks

   
Failed compliance check

   
IP cameras

   
NATS

NATS JetStream

  
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

Something isn't working

  
build

   
checkers

   
ci-cd

continuous integration-continuous deployments

  
cleanup

   
cnpg

cloud-native postgres

  
codex

   
core

core service

  
dependencies

Pull requests that update a dependency file

  
device-management

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
dusk

   
ebpf

   
enhancement

New feature or request

  
eta 1d

   
eta 1hr

   
eta 3d

   
eta 3hr

   
feature

   
fieldsurvey

   
github_actions

Pull requests that update GitHub Actions code

  
go

Pull requests that update Go code

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update Javascript code

  
k8s

   
log-collector

   
mapper

   
mtr

multi traceroute

  
needs-triage

   
netflow

   
network-sweep

   
observability

   
oracle

Oracle Linux related issues

  
otel

opentelemetry logs, traces, metrics

  
plug-in

   
proton

timeplus proton streaming database

  
python

   
question

Further information is requested

  
reddit

   
redhat

   
research

   
rperf

   
rperf-checker

   
rust

Pull requests that update rust code

  
sdk

   
security

   
serviceradar-agent

   
serviceradar-agent-gateway

   
serviceradar-web

   
serviceradar-web-ng

   
siem

   
snmp

   
sysmon

   
topology

   
ubiquiti

   
wasm

   
wontfix

This will not be worked on

  
zen-engine
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
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
carverauto/serviceradar!2399
No description provided.