fix: drop passlib + security hardening — fixes #15#16
Conversation
passlib 1.7.4 (abandoned since 2020) is incompatible with bcrypt >=5.0. Its internal detect_wrap_bug() self-test sends a 255-byte password to bcrypt.hashpw(), which bcrypt 5.0+ strictly rejects with ValueError. This crashes on the first call regardless of actual password length. Replace passlib with direct bcrypt.hashpw()/checkpw(). The $2b$ hash format is identical, so existing password hashes remain valid. Also adds 15 auth tests (hashing, truncation, sessions).
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR addresses a signup bug by switching password hashing to bcrypt with byte truncation, adds security headers middleware, validates worker URLs to mitigate SSRF, tightens registration validation, updates timezone awareness, supports fiat currency conversion, escapes HTML output, and includes comprehensive auth tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
- Worker status page: escape all dynamic HTML (XSS fix) - Session cookie: auto-detect secure flag via CASHPILOT_SECURE_COOKIE env - Security headers middleware: X-Content-Type-Options, X-Frame-Options, Referrer-Policy, Permissions-Policy - SSRF protection: validate worker URLs before proxying (reject loopback, link-local, localhost) - Username validation: 3-32 alphanumeric chars on registration - Password minimum raised from 6 to 8 characters - exchange_rates.to_usd(): add fiat currency conversion via Frankfurter rates - Replace datetime.utcnow() with datetime.now(datetime.UTC) (deprecated) - Remove inline imports (re, datetime) — moved to top-level - Replace __import__() with standard from-import - Chart.js: pin v4.4.8 with SRI integrity hash - DRY: extract _get_verified_worker_url() shared helper for proxy functions
GeiserX
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/main.py (1)
207-211:⚠️ Potential issue | 🔴 CriticalTimezone-aware vs naive datetime comparison will raise TypeError.
cutoffis timezone-aware (datetime.now(datetime.UTC)), butlast_heartbeatfrom the database is naive (SQLite'sdatetime('now')has no timezone). Comparing them at line 211 will raiseTypeError: can't compare offset-naive and offset-aware datetimes.Proposed fix
- cutoff = datetime.now(datetime.UTC) - timedelta(seconds=STALE_WORKER_SECONDS) + cutoff = datetime.utcnow() - timedelta(seconds=STALE_WORKER_SECONDS)Or make the parsed datetime timezone-aware:
last = datetime.fromisoformat(w["last_heartbeat"]) + last = last.replace(tzinfo=datetime.UTC) if last < cutoff:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/main.py` around lines 207 - 211, The comparison is failing because cutoff is timezone-aware (cutoff = datetime.now(datetime.UTC)) while last parsed from w["last_heartbeat"] is naive; update the parsing in the workers loop (where last = datetime.fromisoformat(w["last_heartbeat"])) to produce a timezone-aware datetime (e.g., attach datetime.UTC via replace(tzinfo=...) or parse with an aware parser) so that last and cutoff use the same timezone awareness before comparing, or alternatively make cutoff naive by using naive now() — adjust only the branch around cutoff/last in the worker status check to keep both datetimes consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/main.py`:
- Around line 207-211: The comparison is failing because cutoff is
timezone-aware (cutoff = datetime.now(datetime.UTC)) while last parsed from
w["last_heartbeat"] is naive; update the parsing in the workers loop (where last
= datetime.fromisoformat(w["last_heartbeat"])) to produce a timezone-aware
datetime (e.g., attach datetime.UTC via replace(tzinfo=...) or parse with an
aware parser) so that last and cutoff use the same timezone awareness before
comparing, or alternatively make cutoff naive by using naive now() — adjust only
the branch around cutoff/last in the worker status check to keep both datetimes
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83ccc6ea-4644-4f88-985c-deb3f7076d96
📒 Files selected for processing (8)
app/auth.pyapp/database.pyapp/exchange_rates.pyapp/main.pyapp/templates/base.htmlapp/worker_api.pyrequirements.txttests/test_auth.py
CodeRabbit found that datetime.fromisoformat() returns naive datetimes from SQLite, but cutoff uses datetime.now(UTC). Attach tzinfo to the parsed heartbeat timestamp to avoid TypeError on comparison.
1 participant
Summary
Fixes #15 —
ValueError: password cannot be longer than 72 byteson any signup/login.Root Cause
passlib 1.7.4 (abandoned since 2020) runs an internal self-test (
detect_wrap_bug) that sends a 255-byte test password tobcrypt.hashpw(). bcrypt >=5.0.0 strictly rejects passwords >72 bytes. This crashes on the first call regardless of actual password length — every new deployment is broken.Fix: Drop passlib, use bcrypt directly
Replace
passlib.hash.bcryptwith directbcrypt.hashpw()/checkpw(). The$2b$hash format is identical — existing password hashes remain valid, no migration needed.Security Hardening (from automated review)
html.escape)secureflag viaCASHPILOT_SECURE_COOKIE/CASHPILOT_BASE_URLX-Content-Type-Options,X-Frame-Options,Referrer-Policy,Permissions-Policya-z,0-9,_,-) on registrationCode Quality
exchange_rates.to_usd()now handles fiat currencies via Frankfurter rates (previously silently dropped non-USD fiat earnings)datetime.utcnow()withdatetime.now(datetime.UTC)across database.py and main.pyimport re,from datetime import ...to top-level__import__hack: Replace with standardfrom app.collectors import make_collectors_get_verified_worker_url()shared helper for 3 proxy functionsFiles Changed
app/auth.py— Drop passlib, direct bcrypt, secure cookie flagapp/main.py— Security headers, SSRF validation, username validation, password min, imports cleanupapp/worker_api.py— HTML escape all dynamic valuesapp/exchange_rates.py— Add fiat currency conversionapp/database.py— Replace deprecated datetime.utcnow()app/templates/base.html— Chart.js SRIrequirements.txt— Remove passlib, keep bcrypt>=4.0tests/test_auth.py(new) — 15 tests: password hashing, truncation, sessionsTest plan
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes