carverauto/serviceradar

Fork 0

fix: inject service user context in API key authentication #2556

Merged

mfreeman451 merged 5 commits from refs/pull/2556/head into main

2025-12-13 20:40:53 +00:00

mfreeman451 commented

2025-12-13 17:05:34 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2117
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2117
Original created: 2025-12-13T17:05:34Z
Original updated: 2025-12-13T20:40:58Z
Original head: carverauto/serviceradar:bug/icmp_metrics_stoppage
Original base: main
Original merged: 2025-12-13T20:40:53Z by @mfreeman451

User description

API key authentication was not setting a user in the request context, causing RouteProtectionMiddleware and RBAC checks to fail with HTTP 401. This broke the /api/devices/{id}/metrics endpoint when called with API key auth (e.g., from the web frontend), preventing ICMP metrics from being displayed in the UI.

The fix injects a service user with admin/operator/viewer roles when API key authentication succeeds, matching the behavior of edge-download authentication and bearer token authentication.

🤖 Generated with Claude Code

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

Inject service user context in API key authentication to fix RBAC failures
Allows /api/devices/{id}/metrics endpoint to work with API key auth
Restores ICMP metrics display in UI that was showing stale data
Matches behavior of edge-download and bearer token authentication

Diagram Walkthrough

flowchart LR
  A["API Key Request"] -->|X-API-Key header| B["handleAPIKeyAuth"]
  B -->|Key matches| C["Inject Service User<br/>admin/operator/viewer"]
  C -->|User in context| D["RBAC Middleware"]
  D -->|Authorization passes| E["Protected Endpoint<br/>/api/devices/{id}/metrics"]
  E -->|HTTP 200| F["ICMP Metrics Returned"]

File Walkthrough

Relevant files

Bug fix

server.go

Inject service user context in API key auth

pkg/core/api/server.go

Modified handleAPIKeyAuth function to inject a service user into
request context after successful API key validation
Service user has email api-key@system and roles admin, operator,
viewer
Uses context.WithValue to set user in context before calling next
handler
Enables RBAC middleware to authorize API key authenticated requests

+6/-1

Documentation

proposal.md `Change proposal for ICMP metrics display fix` openspec/changes/fix-icmp-metrics-display-stoppage/proposal.md Documents root cause: RBAC middleware rejects requests without user context from API key auth Details investigation findings showing ICMP data is collected but API returns HTTP 401 Specifies fix to inject service/system user into request context Notes secondary issue with KV watcher telemetry for future investigation	+22/-0
spec.md `API key authentication context propagation specification` openspec/changes/fix-icmp-metrics-display-stoppage/specs/api-authentication/spec.md Defines requirement for API key authentication context propagation Specifies that system must inject valid user context after successful API key validation Documents scenario for API key authenticated access to protected endpoints Includes scenario for fetching device ICMP metrics with API key authentication	+17/-0
tasks.md `Implementation and verification tasks for metrics fix` openspec/changes/fix-icmp-metrics-display-stoppage/tasks.md Lists investigation tasks confirming ICMP data collection and API endpoint HTTP 401 error Documents implementation tasks for fixing `handleAPIKeyAuth` and injecting user context Outlines verification steps for deployment and monitoring Identifies secondary issue with KV watcher telemetry for separate investigation	+25/-0

Imported from GitHub pull request. Original GitHub pull request: #2117 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2117 Original created: 2025-12-13T17:05:34Z Original updated: 2025-12-13T20:40:58Z Original head: carverauto/serviceradar:bug/icmp_metrics_stoppage Original base: main Original merged: 2025-12-13T20:40:53Z by @mfreeman451 --- ### **User description** API key authentication was not setting a user in the request context, causing RouteProtectionMiddleware and RBAC checks to fail with HTTP 401. This broke the /api/devices/{id}/metrics endpoint when called with API key auth (e.g., from the web frontend), preventing ICMP metrics from being displayed in the UI. The fix injects a service user with admin/operator/viewer roles when API key authentication succeeds, matching the behavior of edge-download authentication and bearer token authentication. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## 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** - Inject service user context in API key authentication to fix RBAC failures - Allows `/api/devices/{id}/metrics` endpoint to work with API key auth - Restores ICMP metrics display in UI that was showing stale data - Matches behavior of edge-download and bearer token authentication ___ ### Diagram Walkthrough ```mermaid flowchart LR A["API Key Request"] -->|X-API-Key header| B["handleAPIKeyAuth"] B -->|Key matches| C["Inject Service User admin/operator/viewer"] C -->|User in context| D["RBAC Middleware"] D -->|Authorization passes| E["Protected Endpoint /api/devices/{id}/metrics"] E -->|HTTP 200| F["ICMP Metrics Returned"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Bug fix</td><td><table> <tr> <td> <details> <summary>server.go<dd><code>Inject service user context in API key auth</code>                            </dd></summary> <hr> pkg/core/api/server.go <ul><li>Modified <code>handleAPIKeyAuth</code> function to inject a service user into request context after successful API key validation <li> Service user has email <code>api-key@system</code> and roles <code>admin</code>, <code>operator</code>, <code>viewer</code> <li> Uses <code>context.WithValue</code> to set user in context before calling next handler <li> Enables RBAC middleware to authorize API key authenticated requests</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2117/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021e">+6/-1</a>      </td> </tr> </table></td></tr><tr><td>Documentation</td><td><table> <tr> <td> <details> <summary>proposal.md<dd><code>Change proposal for ICMP metrics display fix</code>                          </dd></summary> <hr> openspec/changes/fix-icmp-metrics-display-stoppage/proposal.md <ul><li>Documents root cause: RBAC middleware rejects requests without user context from API key auth <li> Details investigation findings showing ICMP data is collected but API returns HTTP 401 <li> Specifies fix to inject service/system user into request context <li> Notes secondary issue with KV watcher telemetry for future investigation</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2117/files#diff-5242b9cc31250adc8aed233e6ee84c2a31c5893306d8fc046beee1094784038b">+22/-0</a>    </td> </tr> <tr> <td> <details> <summary>spec.md<dd><code>API key authentication context propagation specification</code>  </dd></summary> <hr> openspec/changes/fix-icmp-metrics-display-stoppage/specs/api-authentication/spec.md <ul><li>Defines requirement for API key authentication context propagation <li> Specifies that system must inject valid user context after successful API key validation <li> Documents scenario for API key authenticated access to protected endpoints <li> Includes scenario for fetching device ICMP metrics with API key authentication</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2117/files#diff-ae17ef7a8ce6b25b03058ddef3aed3c6a6dea5a702a01440fdbb3d73c8cdcfdb">+17/-0</a>    </td> </tr> <tr> <td> <details> <summary>tasks.md<dd><code>Implementation and verification tasks for metrics fix</code>        </dd></summary> <hr> openspec/changes/fix-icmp-metrics-display-stoppage/tasks.md <ul><li>Lists investigation tasks confirming ICMP data collection and API endpoint HTTP 401 error <li> Documents implementation tasks for fixing <code>handleAPIKeyAuth</code> and injecting user context <li> Outlines verification steps for deployment and monitoring <li> Identifies secondary issue with KV watcher telemetry for separate investigation</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2117/files#diff-9be9ebca1cf77891c44d95c7bde2e39431924b037ed5c0528503a5b19fe0fbb4">+25/-0</a>    </td> </tr> </table></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-13 17:06:08 +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/2117#issuecomment-3649626756
Original created: 2025-12-13T17:06:08Z

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Privilege escalation Description: Broad privilege escalation via API key: a successful API key match injects a service user with high-privilege roles (`admin`, `operator`, `viewer`) into the request context, granting full RBAC access to any holder of the single shared API key without scoping, rotation, or audit linkage to a real principal. server.go [545-550] Referred Code `// Inject a service user into context for RBAC middleware compatibility ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx))`
	Insecure context handling Description: Insecure context key usage risk: uses `context.WithValue(r.Context(), auth.UserKey,` `&models.User{...})` which can be overwritten or spoofed by other middleware if `auth.UserKey` is not an unexported unique type, potentially allowing user impersonation or confusion of auth state. server.go [546-550] Referred Code `ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx))`
	Weak API key auth Description: Static shared API key from environment with header comparison only (`X-API-Key` == `API_KEY`) lacks rate limiting, replay protection, or IP throttling and now unlocks admin access, increasing brute-force and credential stuffing impact if the key leaks. server.go [539-551] Referred Code `apiKey := os.Getenv("API_KEY") if apiKey == "" { return false } if r.Header.Get("X-API-Key") == apiKey { // Inject a service user into context for RBAC middleware compatibility ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx)) return true`
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 log: The new API key success path injects a service user and proceeds without logging the authentication event or user context, leaving no audit trail for this critical action. Referred Code `// Inject a service user into context for RBAC middleware compatibility ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx)) return true` 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: Context error handling: The code creates a context with user data but does not handle potential issues (e.g., nil keys/types or downstream failures) or add diagnostic logs for the invalid API key branch. Referred Code `// Inject a service user into context for RBAC middleware compatibility ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx)) return true` 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: Broad role grant: Injecting a service user with admin/operator/viewer roles for any valid API key may exceed least privilege; consider restricting roles or scoping access for API key usage. Referred Code `ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, })` 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/2117#issuecomment-3649626756 Original created: 2025-12-13T17:06:08Z --- _You are nearing your monthly Qodo Merge usage quota. For more information, please visit [here](https://qodo-merge-docs.qodo.ai/installation/qodo_merge/#cloud-users)._ ## 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=3>⚪</td> <td><details><summary>Privilege escalation </summary> Description: Broad privilege escalation via API key: a successful API key match injects a service user with high-privilege roles (<code>admin</code>, <code>operator</code>, <code>viewer</code>) into the request context, granting full RBAC access to any holder of the single shared API key without scoping, rotation, or audit linkage to a real principal. <a href='https://github.com/carverauto/serviceradar/pull/2117/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR545-R550'>server.go [545-550]</a> <details open><summary>Referred Code</summary> ```go // Inject a service user into context for RBAC middleware compatibility ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx)) ``` </details></details></td></tr> <tr><td><details><summary>Insecure context handling </summary> Description: Insecure context key usage risk: uses <code>context.WithValue(r.Context(), auth.UserKey, </code> <code>&models.User{...})</code> which can be overwritten or spoofed by other middleware if <code>auth.UserKey</code> is not an unexported unique type, potentially allowing user impersonation or confusion of auth state. <a href='https://github.com/carverauto/serviceradar/pull/2117/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR546-R550'>server.go [546-550]</a> <details open><summary>Referred Code</summary> ```go ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx)) ``` </details></details></td></tr> <tr><td><details><summary>Weak API key auth </summary> Description: Static shared API key from environment with header comparison only (<code>X-API-Key</code> == <code>API_KEY</code>) lacks rate limiting, replay protection, or IP throttling and now unlocks admin access, increasing brute-force and credential stuffing impact if the key leaks. <a href='https://github.com/carverauto/serviceradar/pull/2117/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR539-R551'>server.go [539-551]</a> <details open><summary>Referred Code</summary> ```go apiKey := os.Getenv("API_KEY") if apiKey == "" { return false } if r.Header.Get("X-API-Key") == apiKey { // Inject a service user into context for RBAC middleware compatibility ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx)) return true ``` </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/2117/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR545-R551'>Missing audit log</a>: The new API key success path injects a service user and proceeds without logging the authentication event or user context, leaving no audit trail for this critical action. <details open><summary>Referred Code</summary> ```go // Inject a service user into context for RBAC middleware compatibility ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx)) return true ``` </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/2117/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR545-R551'>Context error handling</a>: The code creates a context with user data but does not handle potential issues (e.g., nil keys/types or downstream failures) or add diagnostic logs for the invalid API key branch. <details open><summary>Referred Code</summary> ```go // Inject a service user into context for RBAC middleware compatibility ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx)) return true ``` </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/2117/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR546-R549'>Broad role grant</a>: Injecting a service user with admin/operator/viewer roles for any valid API key may exceed least privilege; consider restricting roles or scoping access for API key usage. <details open><summary>Referred Code</summary> ```go ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) ``` </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-13 17:07:07 +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/2117#issuecomment-3649627557
Original created: 2025-12-13T17:07:07Z

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Re-evaluate hardcoded admin permissions for API keys The current fix grants excessive admin permissions to any user authenticating with an API key, creating a security risk. A better approach would be to associate API keys with specific, limited roles instead of a static, all-powerful user. Examples: pkg/core/api/server.go [546-549] `ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, })` Solution Walkthrough: Before: `func handleAPIKeyAuth(w, r, next) bool { apiKey := os.Getenv("API_KEY") if r.Header.Get("X-API-Key") == apiKey { // Inject a hardcoded user with full admin rights ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx)) return true } // ... }` After: func handleAPIKeyAuth(w, r, next) bool { providedKey := r.Header.Get("X-API-Key") // Conceptual: Look up the API key and its associated roles from a service apiKeyDetails, err := s.apiKeyService.GetRolesForKey(providedKey) if err != nil { // Handle invalid key return false } // Inject user with specific, limited roles from the key ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: apiKeyDetails.OwnerEmail, Roles: apiKeyDetails.Roles, }) next.ServeHTTP(w, r.WithContext(ctx)) return true } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a significant security risk by hardcoding full admin privileges for the API key, which violates the principle of least privilege and has system-wide implications.	High
Security	Avoid hardcoding permissive admin roles Make the API key roles configurable via an environment variable and default to a least-privilege role like `viewer` instead of hardcoding full admin access. pkg/core/api/server.go [545-550] `// Inject a service user into context for RBAC middleware compatibility +apiKeyRoles := os.Getenv("API_KEY_ROLES") +if apiKeyRoles == "" { + apiKeyRoles = "viewer" // Default to least privilege +} + ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", - Roles: []string{"admin", "operator", "viewer"}, + Roles: strings.Split(apiKeyRoles, ","), }) next.ServeHTTP(w, r.WithContext(ctx))` Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a significant security risk by hardcoding admin-level permissions for the API key, and proposes a more secure, configurable approach that adheres to the principle of least privilege.	High
General	Cache API key on startup To improve performance, read the `API_KEY` from environment variables once at server startup and cache it in the `APIServer` struct, instead of reading it on every request. pkg/core/api/server.go [537-559] // handleAPIKeyAuth handles API key authentication func (s APIServer) handleAPIKeyAuth(w http.ResponseWriter, r http.Request, next http.Handler) bool { - apiKey := os.Getenv("API_KEY") - if apiKey == "" { + if s.apiKey == "" { return false } - if r.Header.Get("X-API-Key") == apiKey { + if r.Header.Get("X-API-Key") == s.apiKey { // Inject a service user into context for RBAC middleware compatibility ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx)) return true } // API key is configured but the provided key is invalid s.logger.Warn().Msg("API key authentication is enabled but no valid API key provided") writeError(w, "Invalid API key", http.StatusUnauthorized) return true // Handled (with rejection) } `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 5 __ Why: The suggestion proposes a valid performance optimization by caching the `API_KEY` on startup, which is a good practice, although the performance impact of `os.Getenv` is likely minor in this context.	Low
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2117#issuecomment-3649627557 Original created: 2025-12-13T17:07:07Z --- _You are nearing your monthly Qodo Merge usage quota. For more information, please visit [here](https://qodo-merge-docs.qodo.ai/installation/qodo_merge/#cloud-users)._ ## 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>High-level</td> <td> <details><summary>Re-evaluate hardcoded admin permissions for API keys</summary> ___ **The current fix grants excessive admin permissions to any user authenticating with an API key, creating a security risk. A better approach would be to associate API keys with specific, limited roles instead of a static, all-powerful user.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2117/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR546-R549">pkg/core/api/server.go [546-549]</a> </summary> ```go ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) ``` </details> ### Solution Walkthrough: #### Before: ```go func handleAPIKeyAuth(w, r, next) bool { apiKey := os.Getenv("API_KEY") if r.Header.Get("X-API-Key") == apiKey { // Inject a hardcoded user with full admin rights ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx)) return true } // ... } ``` #### After: ```go func handleAPIKeyAuth(w, r, next) bool { providedKey := r.Header.Get("X-API-Key") // Conceptual: Look up the API key and its associated roles from a service apiKeyDetails, err := s.apiKeyService.GetRolesForKey(providedKey) if err != nil { // Handle invalid key return false } // Inject user with specific, limited roles from the key ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: apiKeyDetails.OwnerEmail, Roles: apiKeyDetails.Roles, }) next.ServeHTTP(w, r.WithContext(ctx)) return true } ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies a significant security risk by hardcoding full admin privileges for the API key, which violates the principle of least privilege and has system-wide implications. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>Security</td> <td> <details><summary>Avoid hardcoding permissive admin roles</summary> ___ **Make the API key roles configurable via an environment variable and default to a least-privilege role like <code>viewer</code> instead of hardcoding full admin access.** [pkg/core/api/server.go [545-550]](https://github.com/carverauto/serviceradar/pull/2117/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR545-R550) ```diff // Inject a service user into context for RBAC middleware compatibility +apiKeyRoles := os.Getenv("API_KEY_ROLES") +if apiKeyRoles == "" { + apiKeyRoles = "viewer" // Default to least privilege +} + ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", - Roles: []string{"admin", "operator", "viewer"}, + Roles: strings.Split(apiKeyRoles, ","), }) next.ServeHTTP(w, r.WithContext(ctx)) ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies a significant security risk by hardcoding admin-level permissions for the API key, and proposes a more secure, configurable approach that adheres to the principle of least privilege. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Cache API key on startup</summary> ___ **To improve performance, read the <code>API_KEY</code> from environment variables once at server startup and cache it in the <code>APIServer</code> struct, instead of reading it on every request.** [pkg/core/api/server.go [537-559]](https://github.com/carverauto/serviceradar/pull/2117/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR537-R559) ```diff // handleAPIKeyAuth handles API key authentication func (s *APIServer) handleAPIKeyAuth(w http.ResponseWriter, r *http.Request, next http.Handler) bool { - apiKey := os.Getenv("API_KEY") - if apiKey == "" { + if s.apiKey == "" { return false } - if r.Header.Get("X-API-Key") == apiKey { + if r.Header.Get("X-API-Key") == s.apiKey { // Inject a service user into context for RBAC middleware compatibility ctx := context.WithValue(r.Context(), auth.UserKey, &models.User{ Email: "api-key@system", Roles: []string{"admin", "operator", "viewer"}, }) next.ServeHTTP(w, r.WithContext(ctx)) return true } // API key is configured but the provided key is invalid s.logger.Warn().Msg("API key authentication is enabled but no valid API key provided") writeError(w, "Invalid API key", http.StatusUnauthorized) return true // Handled (with rejection) } ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion proposes a valid performance optimization by caching the `API_KEY` on startup, which is a good practice, although the performance impact of `os.Getenv` is likely minor in this context. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>