Repository navigation
fix: correct defects found while retiring SonarCloud - #1711
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
498cd30 to
bbf33a7
Compare
|
CORS advertised credentialed access to any origin and did not decorate rate-limit rejections; consensus events were gated on a comparison that never matched; readiness jitter was identical across replicas.
bbf33a7 to
d9ca846
Compare
1 participant
Five verified defects, harvested from a SonarCloud triage sweep before that project is retired (see #1710). Independent fixes, reviewable separately in the diff.
Context on the sweep: 198 findings triaged, 11 real (~1 in 18). Notably, all 70
VULNERABILITY-type findings yielded zero real defects. Two of the five below were found while checking a Sonar finding rather than being flagged by it.1. CORS advertised credentialed access to any origin
fastapi_server.pyhadallow_origins=["*"]together withallow_credentials=True. Starlette responds by echoing the caller's Origin rather than a literal*, so any website could make credentialed cross-origin requests to hosted Studio.Verified that credentials are not needed: authentication is the
X-API-Keyheader (rate_limit_middleware.py:64), there is noset_cookieanywhere in the RPC backend, and the frontend does not usewithCredentials. A custom header is already permitted byallow_headers=["*"]. Setallow_credentials=False; origins are unchanged, since this is deliberately a public RPC.Sonar did not flag this. It was found while investigating the middleware-order issue below.
2. Rate-limit rejections carried no CORS headers
add_middlewareinserts at index 0, so the last middleware added is outermost. CORS was added first and RateLimit second, making RateLimit outermost — its 429 short-circuit returned without passing back out through CORS, so browsers saw an opaque network/CORS failure instead of a readable 429. The inline comment asserted the opposite of what the code did.RateLimit is now added first (inner), CORS last (outermost), and the comment states the actual relationship.
3. Consensus events were gated on a comparison that never matched
redis_worker_handler.pytestedlog_event.scope.value in ["TRANSACTION", "CONSENSUS"], but the enum's values are"Transaction"and"Consensus". The same class compared them correctly elsewhere. The test was therefore always false, and publication was gated solely ontransaction_hash— scope-only consensus events were silently never published.Present in three places (the sweep identified two;
worker_handler.pyhad it as well). All now compare againstEventScopemembers rather than string literals, so a value rename cannot silently reintroduce it.4. Readiness jitter was identical across replicas
health.pyseeded jitter withrandom.Random(os.getpid()). The stated intent is jitter stable within a process but different between processes, so replicas don't flip readiness in lockstep — but containerised replicas commonly share a PID, so every pod computed the same jitter and would withdraw readiness simultaneously, which is the behaviour the jitter exists to prevent. Now unseeded; per-process stability still comes from the module-level assignment.5. Two smaller ones
transactions_processor.py:if byte_content or len(byte_content) >= 0:—len(x) >= 0is always true, making the guard a no-op and the following return unreachable. Removed the tautology without changing behaviour. The original intent is ambiguous; this is deliberately not a behavioural change.backend/validators/__init__.py:__all__exportedwith_lock, which is defined nowhere, soimport *would raise. Latent (no star-import exists today). Confirmed no definition or caller, then removed the entry.Verification
Backend unit tests pass (56 in the touched files). The new tests were checked against the unfixed code first — 6 of them fail without these changes, so they pin the defects rather than merely passing.
Items 1 and 2 affect a live hosted service; item 2's behaviour is browser-observable and worth a smoke check after deploy.