polish medium audit items: bearer case, require_role fallback, bcrypt limit, column type, passkey config#16
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR implements several behavioral improvements across authentication and configuration: Bearer token scheme matching is now case-insensitive, role-based authorization returns a clean 403 when roles are missing instead of erroring, bcrypt password hashing validates the 72-byte UTF-8 limit, SQLModel's hashed_password field is explicitly mapped to Text column type, and FullAuthConfig validates passkey settings at instantiation time. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Summary
Five small fixes from the medium-severity audit list, batched because each is a few lines.
BearerBackendaccepts any case of theBearerscheme. RFC 7235 says auth schemes are case-insensitive; we were rejectingbearer abc/BEARER abc. Now handled viaauth[:7].lower() == "bearer ".require_roletolerates aUserSchemawith norolesfield. DefaultUserSchemaships withoutroles— apps that don't use RBAC would get anAttributeError(500) fromrequire_roleinstead of a clean403. Usesgetattr(user, "roles", []) or []now.hash_password(..., algorithm="bcrypt")with a password over 72 UTF-8 bytes now raisesInvalidPasswordErrorinstead of silently truncating. Prevents a subtle lockout if someone ever migrates a bcrypt-hashed DB to argon2id (argon2 doesn't truncate). Length check happens beforeimport bcryptso it still validates cleanly in environments without the bcrypt extra installed.UserBase.hashed_passwordis explicitText. Matches the SQLAlchemy adapter. Argon2id output is ~97 chars, so the implicitVARCHAR(255)default on MySQL/MSSQL was fine — but now the intent is obvious from the model, and any future hash-length creep is safe.FullAuthConfigvalidates passkey settings at construction time. WhenPASSKEY_ENABLED=True, the config now rejects emptyPASSKEY_RP_ID,PASSKEY_RP_IDwith a scheme or path, emptyPASSKEY_ORIGINS, origins without a scheme, andPASSKEY_CHALLENGE_BACKEND="redis"withoutREDIS_URL. All of these used to surface as a 500 on the first passkey request; now they fail fast at app startup.Test plan
uv run ruff format --check .cleanuv run ruff check .cleanuv run pytest tests/— 188 passing, 1 skipped (bcrypt install-dependent test). 12 new tests intests/test_polish.pycover every fix.PASSKEY_RP_ID(e.g.https://example.com) — confirm the app refuses to start instead of crashing on the first register/begin request.Authorization: bearer <token>— confirm it authenticates.Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation