Skip to content

Fix dependency security and Sonar quality findings#45

Open
eliteprox wants to merge 3 commits into
mainfrom
cursor/fix-security-quality-d829
Open

Fix dependency security and Sonar quality findings#45
eliteprox wants to merge 3 commits into
mainfrom
cursor/fix-security-quality-d829

Conversation

@eliteprox

@eliteprox eliteprox commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • upgrade direct Go dependencies to current releases, including patched chi and pgx versions
  • remove chi's deprecated RealIP middleware so untrusted forwarding headers cannot spoof the logged client address
  • refactor all seven open Sonar code-quality findings
  • add coverage for registry capabilities that intentionally omit offerings
  • retain the intentional self-signed orchestrator TLS behavior and its security-tool suppression

Verification

  • go test -race -count=1 ./...
  • go vet ./...
  • go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.12.2 run --timeout=5m
  • go run golang.org/x/vuln/cmd/govulncheck@latest ./...
  • CGO_ENABLED=0 go build -o /tmp/discoveryd ./cmd/discoveryd
  • gofmt and whitespace checks
Open in Web Open in Cursor 

Summary by CodeRabbit

  • Bug Fixes
    • Improved registry manifest parsing when capabilities do not include offerings.
    • Improved orchestrator discovery result handling and error reporting.
    • Preserved reliable URI and Ethereum address mapping during resolution.
  • Improvements
    • Updated core networking, database, caching, and security components.
    • Simplified request processing and internal data handling for improved maintainability.

Co-authored-by: John | Elite Encoder <john@eliteencoder.net>
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR refactors registry parsing, orchestrator discovery probing, and resolver merging; removes RealIP middleware; groups Go parameter declarations; and updates direct and indirect module dependencies.

Changes

Discovery service refactors

Layer / File(s) Summary
Registry manifest parsing
internal/sources/registry_parser.go, internal/sources/registry_parser_test.go
V3 manifest parsing is split into node, capability, and offering helpers, with a test for capabilities that omit offerings.
Orchestrator discovery probing
internal/sources/orch_discovery.go
Probe results, concurrency handling, claim recording, and probe statistics are centralized in dedicated helpers.
Resolver mapping and merge helpers
internal/resolver/resolve_helpers.go
ETH/URI mapping, alternate source lookup, and member field merging are extracted into focused helpers and a shared merge context.
Runtime and module cleanup
internal/httpapi/server.go, internal/query/query.go, go.mod
RealIP middleware is removed, parameter declarations are grouped, and direct and indirect dependency versions are updated.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • pymthouse/discovery-service#35: Adds orchestrator /discovery probing and live-runner app claim materialization related to the probing refactor here.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main changes: dependency security updates and Sonar-driven code quality fixes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cursor/fix-security-quality-d829

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

cursoragent and others added 2 commits July 14, 2026 22:25
Co-authored-by: John | Elite Encoder <john@eliteencoder.net>
Co-authored-by: John | Elite Encoder <john@eliteencoder.net>
@sonarqubecloud

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
internal/sources/orch_discovery.go (1)

247-248: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Respect context cancellation when acquiring the semaphore.

If the system is under high load and the context is canceled (e.g., due to a client disconnect or an outer timeout), goroutines waiting to acquire the semaphore will still block until a slot frees up before checking the context in the subsequent HTTP request. Adding a select with ctx.Done() prevents goroutines from needlessly piling up.

♻️ Proposed refactor
-	sem <- struct{}{}
+	select {
+	case sem <- struct{}{}:
+	case <-ctx.Done():
+		results.record(nil, uri, ctx.Err())
+		return
+	}
 	defer func() { <-sem }()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/sources/orch_discovery.go` around lines 247 - 248, Update the
semaphore acquisition in the surrounding discovery operation to select between
sending on sem and receiving from ctx.Done(), returning or propagating
cancellation when the context is canceled. Keep the deferred semaphore release
only for goroutines that successfully acquire the slot, and leave the subsequent
HTTP request flow unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/sources/orch_discovery.go`:
- Around line 247-248: Update the semaphore acquisition in the surrounding
discovery operation to select between sending on sem and receiving from
ctx.Done(), returning or propagating cancellation when the context is canceled.
Keep the deferred semaphore release only for goroutines that successfully
acquire the slot, and leave the subsequent HTTP request flow unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3d50d3b1-09ce-4015-ab8d-095d8cdba93c

📥 Commits

Reviewing files that changed from the base of the PR and between 8e8e9af and c2c239c.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (7)
  • go.mod
  • internal/httpapi/server.go
  • internal/query/query.go
  • internal/resolver/resolve_helpers.go
  • internal/sources/orch_discovery.go
  • internal/sources/registry_parser.go
  • internal/sources/registry_parser_test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants