Skip to content

fix: address MEDIUM review findings (light witness resilience, proxy CORS, threshold warning) - #1382

Draft
lklimek wants to merge 3 commits into
v1.6-devfrom
fix/review-medium-findings
Draft

fix: address MEDIUM review findings (light witness resilience, proxy CORS, threshold warning)#1382
lklimek wants to merge 3 commits into
v1.6-devfrom
fix/review-medium-findings

Conversation

@lklimek

@lklimek lklimek commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Why this PR exists

  • Problem: A code review of the v1.5.4 → v1.6.0 diff surfaced three MEDIUM-severity issues in the light client and consensus state machine that degrade resilience and safety posture.
  • What breaks without it: (1) a single faulty or partitioned witness can abort the entire light client (ErrConflictingWitnessHeader), breaking liveness for honest users; (2) the light proxy accepts WebSocket connections from any origin, enabling cross-site subscription to consensus streams; (3) running below the 2/3+1 voting-power threshold proceeds silently with no operator warning that safety is no longer guaranteed.
  • Blocking relationship: none — standalone fix branch based on v1.6-dev.

Issue being fixed or feature implemented

Addresses three review findings:

  • CODE-001 (resilience): tolerate one dissenting witness instead of failing the client.
  • SEC-001 (security): enforce WebSocket origin checking on the light proxy by default.
  • SEC-002 (safety): warn explicitly when voting_power_threshold drops below 2/3+1.

What was done?

  • light/client.go: single conflicting witness (with ≥1 corroborating) is removed from the trusted set; ≥2 conflicting witnesses, or a conflict with zero corroboration, still fail closed.
  • light/proxy/proxy.go: replaced the permissive func(*http.Request) bool { return true } origin check with rpcserver.OriginChecker over a new AllowedOrigins field (zero-trust default: empty list rejects all). cmd/tenderdash/commands/light.go wires conf.RPC.CORSAllowedOrigins into the proxy.
  • internal/consensus/state_data.go: logs an explicit OVERRIDE ENABLED - NOT SAFE FOR PRODUCTION error when validators.BelowStrictThreshold() is true.
  • CHANGELOG.md + docs/nodes/configuration.md: document the behavior changes.

How Has This Been Tested?

  • gofmt clean on all changed files.
  • go vet ./light/ ./light/proxy/ ./internal/consensus/ clean (BLS native deps built via make build-bls).
  • go test ./light/ passes (13.4s), including updated TestCompareFirstHeaderWithWitness* cases: single conflicting witness → removed (no error), two conflicting → fail, single conflicting with no corroboration → fail.
  • go build ./light/proxy/ ./cmd/tenderdash/commands/ succeeds.

Breaking Changes

None in the Go API. Behavioral: the light proxy now rejects WebSocket connections from unconfigured origins by default (previously accepted all). Operators relying on the open behavior must set rpc.cors-allowed-origins.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

🤖 Co-authored by Claudius the Magnificent AI Agent

lklimek and others added 3 commits July 8, 2026 11:56
…he client

Previously any single witness returning a first/new header that disagreed with
the primary would abort the light client with ErrConflictingWitnessHeader,
letting one faulty or partitioned witness break liveness for the whole client.

Now a single conflicting witness (with at least one corroborating witness) is
removed from the trusted set, while two or more conflicting witnesses, or a
conflict with no corroboration, still fail closed. This prevents one bad witness
from taking down the client while preserving safety against genuine forks.

Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…trust)

The light proxy previously accepted WebSocket connections from any origin
(func(*http.Request) bool { return true }), allowing cross-site clients to
subscribe to consensus/block streams.

The proxy now uses rpcserver.OriginChecker against the configured
rpc.cors-allowed-origins list, defaulting to an empty list (reject all) for a
zero-trust posture. Operators opt in by setting cors-allowed-origins in config.

Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…2/3+1

When validators.BelowStrictThreshold() is true the node can no longer
guarantee safety, yet previously continued silently. The state machine now logs
an explicit "OVERRIDE ENABLED - NOT SAFE FOR PRODUCTION" error so operators are
aware they are running an unsafe override.

Also documents the behavior change and that cors-allowed-origins governs the
light proxy.

Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 11f1c9f4-6a06-4604-ae91-6488126f7128

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/review-medium-findings

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.

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.

1 participant