fix: codebase audit — bugs, dead code, hardening, test gaps#85
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 5 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (42)
📝 WalkthroughWalkthroughSecurity-focused release hardening authentication (per-user password-epoch invalidation), database (connection pooling, config masking), collectors (strict field validation), worker URLs (SSRF protection with DNS rebinding defense), and API key exposure (lazy reveal). Includes router refactoring, password-change endpoints, and comprehensive test coverage. ChangesSecurity hardening and API changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ 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 |
Owner
Author
|
@coderabbitai review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
==========================================
+ Coverage 90.99% 91.54% +0.54%
==========================================
Files 26 30 +4
Lines 2811 3063 +252
==========================================
+ Hits 2558 2804 +246
- Misses 253 259 +6
🚀 New features to boost your workflow:
|
✅ Action performedReview finished.
|
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 33907540 | Triggered | Username Password | d6f1af5 | tests/test_main_routes.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
GeiserX
force-pushed
the
fix/codebase-audit-safe-wave
branch
2 times, most recently
from
3381d8a to
eb38c1a
Compare
…e, DB pooling, fixes
Consolidated wave from a whole-codebase audit + follow-up research. Full
suite 950 passing, ruff clean. Every behavior change is guarded by a
regression test verified to fail against the pre-fix code.
Security:
- SSRF guard on worker URLs: cloud-metadata IPs (IPv4 169.254.169.254 +
IPv6 fd00:ec2::254) always blocked; IPv6 loopback/link-local + IPv4-mapped
(::ffff:) bypasses closed; DNS-rebinding guard resolves hostnames and
re-validates the resolved IP before every outbound request. Default policy
"permissive" keeps LAN (RFC1918) + Tailscale (CGNAT 100.64.0.0/10) workers
working with zero config; opt-in "strict" mode via CASHPILOT_WORKER_URL_POLICY
+ CASHPILOT_WORKER_ALLOWED_HOSTS (CIDRs + *.suffix); CASHPILOT_WORKER_ALLOW_METADATA
escape hatch.
- Write-only secrets: GET /api/config returns a {key: is_set} map (no decrypted
values); /api/env-info drops secret values (CASHPILOT_SECRET_KEY never leaves
the server); fleet key revealed only via owner-only POST /api/fleet/api-key/reveal
(audit-logged, uid only).
- Change-password + session invalidation: self-service POST /api/users/me/password
(re-mints caller cookie) + owner reset POST /api/users/{id}/password; a change
bumps a per-user epoch (warmed at startup) so older tokens for that user are
rejected.
Performance:
- SQLite connection sharing (one connection per event loop via borrowed-handle
proxy) — ~3x faster dashboard loads, far less write contention. Per-loop
keying preserves test isolation.
Audit fixes (bugs / dead code / consistency):
- Collectors: missing balance field surfaces an error instead of persisting a
fake 0.0 (11 collectors); BaseCollector is a real ABC; proxyrack returns a
consistent Response.
- metrics: read the real last_heartbeat column (was last_seen) so dead-worker
alerting fires.
- Scheduler: error/missed-job listener + max_instances/coalesce/misfire_grace_time.
- _run_collection publishes a synthetic alert on crash (no green bell during an
outage). Worker-proxy errors no longer leak internal URLs into HTTP detail.
- auth: narrowed except (BadSignature, Exception) to BadData.
- database: created_at indexes for retention purge; single upsert_earnings.
- _schema.yml: documented status:dropped, cashout.method:auto, collector.type:auto,
docker.privileged; removed dead referral.bonus; dropped services hidden from catalog.
- exchange_rates: rates_stale() + non-200 logging. Deleted dead orchestrator
deploy_service/reset_docker_status, main _require_auth, JS worker-cache +
goToCollectorSettings.
Refactor:
- Partial router split: users/auth/pages → app/routers/* with shared deps in
app/deps.py, calling shared symbols through app.main so patch seams hold and
no auth guard is lost. Fleet/services/earnings/deploy/config stay in main.py
for a dedicated follow-up PR.
Docs: CHANGELOG, SECURITY.md SSRF section, fleet.md env vars, getting-started.
Adds ~150 regression tests (881 -> 950).
GeiserX
force-pushed
the
fix/codebase-audit-safe-wave
branch
from
eb38c1a to
d6f1af5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Whole-codebase multi-agent audit (9 auditors: architecture, security, complexity, dead code, error handling, test health, consistency, performance) followed by an independent verification pass. This PR lands the safe, high-value findings; four large/risky items are deferred to dedicated follow-up PRs (listed below).
904 tests passing (was 887 — +17 regression tests), ruff + format clean. Every fix was verified to fail against the pre-fix code.
Correctness / silent-failure bugs
float(data.get("balance", 0))→ a fake0.0persisted as a real reading, corrupting deltas and producing false spikes when an upstream API shape changed. Now a missing field surfaces an error (visible in the alert bell); legitimate zeros are preserved. 11 collectors + anyone non-numeric guard._refresh_gaugesreadw.get("last_seen"), a column that never existed (it'slast_heartbeat), so the staleness gauge was never set. Fixed.EVENT_JOB_ERROR | EVENT_JOB_MISSEDlistener +max_instances/coalesce/misfire_grace_timeso a crashing job is logged, not silently stopped._run_collectionnow publishes a synthetic alert on crash so the bell can't show green during a total collection outage.except (BadSignature, Exception)→except BadDataand moved the epoch check out of thetry, so genuine bugs surface instead of masquerading as silent logouts (legit bad/expired tokens still return 401).Security hardening
detail(topology + SSRF-oracle leak); real detail logged server-side only.Performance
database.py: addedcreated_atindexes for the daily retention purge (health_eventsgrows ~4.6M rows/yr → was a full-scan write-lock stall); collapsedupsert_earningsto a singleON CONFLICT DO UPDATE.Consistency / correctness
BaseCollectoris now a realabc.ABC(@abstractmethod collect); proxyrack_fetch_balancereturns only aResponse._schema.yml: documentedstatus: dropped,cashout.method: auto,collector.type: auto,docker.privileged; removed deadreferral.bonus.droppedservices are now hidden from the available catalog.exchange_rates: exposedrates_stale()+ log non-200 rate fetches.Dead code
orchestrator.deploy_service(also had a latent port-parse crash) andreset_docker_status;docker_available()now re-probes instead of cachingFalseforever. Deleted unused_require_auth. Removed abandoned JS worker-cache + unwiredgoToCollectorSettings;alert()→toast()..gitignorenow covers tooling/coverage artifacts.Tests
+17 regression tests, each verified to fail against the pre-fix code: per-collector missing-balance (incl. legitimate-zero cases), heartbeat gauge populated from
last_heartbeat(with a negative case proving the oldlast_seenpayload leaves it unset),rates_stale()states.Deferred to follow-up PRs (intentionally NOT in this sweep)
/api/configsecret-readback + change-password/session-invalidation — changes the Settings UX contract + adds a feature; warrants its own careful PR.database.pyconnection-per-query) — 36-function refactor.main.pyrouter split (1848-line god module) — large structural refactor.Audit headline scores
Architecture 6 · Security 6.5 · Complexity 5 · Dead Code 8.5 · Error Handling 6.5 · Test Health 6 · Consistency 7 · Performance 5
Test plan
Summary by CodeRabbit
Release Notes
New Features
Security
Improvements