Add OTP volunteer auth, approval gating, and volunteer import - #1
Open
SpektorY wants to merge 1 commit into
Open
Add OTP volunteer auth, approval gating, and volunteer import#1SpektorY wants to merge 1 commit into
SpektorY wants to merge 1 commit into
Conversation
This introduces phone-based SMS OTP login for volunteers, enforces pending/approved access gates in public event flow, and adds admin approval plus bulk volunteer import to speed onboarding. Made-with: Cursor
📝 WalkthroughWalkthroughThis pull request introduces OTP-based volunteer authentication and volunteer approval workflows. It adds volunteer status management (PENDING/APPROVED), creates OTP signup endpoints, implements volunteer approval with SMS notifications, enables bulk volunteer import, and extends the event join flow to require authenticated approved volunteers. Frontend updates include volunteer login pages, approval management in the admin dashboard, and simplified event join flows leveraging token-based authentication. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Volunteer Client
participant Server as API Server
participant DB as Database
participant SMS as SMS Service (Twilio)
Client->>Server: POST /public/request-otp (phone)
Server->>DB: Create VolunteerOtp record
Server->>SMS: Send OTP code via SMS
SMS-->>Client: SMS with code
Server-->>Client: Success (request ID or message)
Note over Client: User receives SMS and enters code
Client->>Server: POST /public/verify-otp (phone, code)
Server->>DB: Validate VolunteerOtp (code + expiry)
alt OTP valid
Server->>DB: Check/Create Volunteer record
Server->>DB: Fetch volunteer status
alt Volunteer APPROVED
Server->>Server: Create access token (ROLE_VOLUNTEER)
Server-->>Client: VolunteerAuthResponse (token, volunteer details)
Client->>Client: Store volunteer_token in localStorage
else Volunteer PENDING
Server-->>Client: Error: Pending approval
end
else OTP invalid/expired
Server-->>Client: Error: Invalid or expired OTP
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (11)
backend/.env.example (1)
10-11: Align Twilio env key order with dotenv-linter.Line 10-11 currently triggers
UnorderedKey(TWILIO_SMS_FROMshould appear beforeTWILIO_WHATSAPP_FROM).♻️ Suggested cleanup
-TWILIO_WHATSAPP_FROM= -TWILIO_SMS_FROM= +TWILIO_SMS_FROM= +TWILIO_WHATSAPP_FROM=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/.env.example` around lines 10 - 11, The TWILIO env keys in .env.example are out of order causing dotenv-linter UnorderedKey; swap the lines so TWILIO_SMS_FROM appears before TWILIO_WHATSAPP_FROM (i.e., reorder the keys TWILIO_SMS_FROM and TWILIO_WHATSAPP_FROM) to match the linter's expected ordering.backend/app/schemas/volunteer.py (1)
32-32: Makestatusrequired inVolunteerResponseinstead of defaulting.Defaulting to
PENDINGcan silently misreport status if upstream mapping omits the field. Prefer requiring explicit value from the ORM/data source.♻️ Suggested change
- status: VolunteerStatus = VolunteerStatus.PENDING + status: VolunteerStatus🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/schemas/volunteer.py` at line 32, The VolunteerResponse model currently defaults status to VolunteerStatus.PENDING which can mask missing upstream data; remove the default so the field is required (change "status: VolunteerStatus = VolunteerStatus.PENDING" to "status: VolunteerStatus" in the VolunteerResponse model) and update any places that instantiate VolunteerResponse (ORM-to-schema mappers or constructors) to explicitly pass a status value from the data source to avoid silent misreports; reference: VolunteerResponse and VolunteerStatus.backend/alembic/versions/011_add_volunteer_status_and_otps.py (1)
36-44: Consider adding TTL-based cleanup for expired OTPs.The
volunteer_otpstable will accumulate expired records over time. Consider adding a periodic cleanup job or database-level TTL mechanism to purge records whereexpires_at < now().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/alembic/versions/011_add_volunteer_status_and_otps.py` around lines 36 - 44, The migration creates volunteer_otps with expires_at but has no cleanup; add a TTL cleanup by updating this migration (or a follow-up migration) to create a scheduled job or DB-level TTL that deletes rows where expires_at < now(), and ensure you create an index on expires_at for efficient deletion (in addition to the existing ix_volunteer_otps_phone). Reference the volunteer_otps table and the expires_at column and either emit raw SQL to register a scheduler task (pg_cron/pgagent) or create a DB function + trigger/cron entry in the migration so expired OTPs are purged automatically.frontend/src/features/admin/VolunteersPage.tsx (1)
104-109: Consider adding error feedback for the approve mutation.The
approveMutationsilently fails if the API call errors. Adding anonErrorhandler to display feedback would improve UX.♻️ Suggested improvement
const approveMutation = useMutation({ mutationFn: (id: number) => apiRequest(`/api/v1/volunteers/${id}/approve`, { method: "POST" }), onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["volunteers"] }) }, + onError: (e) => { + alert(e instanceof Error ? e.message : "שגיאה באישור המתנדב") + }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/admin/VolunteersPage.tsx` around lines 104 - 109, approveMutation currently has no onError handler so failures are silent; add an onError callback to the useMutation call that surfaces API errors to the user (for example call your existing toast/notification helper or set an error state) and include the error message returned by apiRequest; keep the existing onSuccess logic (queryClient.invalidateQueries({ queryKey: ["volunteers"] })) and ensure the onError references the same mutation (approveMutation) so the UI can show a failure message and optionally revert any optimistic UI changes.frontend/src/features/public/VolunteerLoginPage.tsx (1)
95-97: Consider clearing the OTP code when switching back to phone step.When the user clicks "החלף טלפון" to go back, the previously entered OTP code remains in state. Clearing it improves UX by starting fresh.
♻️ Suggested fix
- <Button variant="outline" className="w-full" onClick={() => setStep("phone")} disabled={loading}> + <Button variant="outline" className="w-full" onClick={() => { setCode(""); setStep("phone"); }} disabled={loading}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/public/VolunteerLoginPage.tsx` around lines 95 - 97, When switching back to the phone step via the "החלף טלפון" button, also clear the stored OTP state so the previous code doesn't remain; update the onClick handler that currently calls setStep("phone") to additionally reset the OTP state (call the OTP setter, e.g., setOtp("") or setVerificationCode("") as used in this component) and ensure the loading/validation state tied to the OTP is reset if applicable.backend/app/api/v1/endpoints/event_token.py (1)
93-94: Optional: Redundant volunteer query.The
get_event_volunteer_by_tokenhelper already fetches and validates the volunteer (lines 57-67), but this endpoint queries it again. Consider returning the volunteer from the helper to avoid the extra database round-trip.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/v1/endpoints/event_token.py` around lines 93 - 94, The endpoint is performing a redundant DB query for Volunteer after already fetching and validating it in get_event_volunteer_by_token; modify get_event_volunteer_by_token to return both the Event and Volunteer (or a tuple/structure containing the volunteer) and update the caller in get_event_volunteer_by_token usage site (where ev is used) to use the returned volunteer instead of running db.query(Volunteer).filter(Volunteer.id == ev.volunteer_id).first(); ensure function signature and all call sites (including get_event_volunteer_by_token and the endpoint that currently assigns event and volunteer) are updated accordingly so only one DB round-trip fetches the volunteer.backend/app/api/v1/endpoints/volunteers.py (3)
196-213: Anonymization preserves record by clearingdeleted_at.Setting
deleted_at = Noneon line 211 ensures the anonymized volunteer record remains visible and isn't filtered out by soft-delete queries. This appears intentional for audit/history purposes. Consider adding a brief comment explaining this design decision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/v1/endpoints/volunteers.py` around lines 196 - 213, The anonymize_volunteer function currently sets v.deleted_at = None to keep the anonymized volunteer record visible rather than soft-deleted; add a concise inline comment next to the v.deleted_at assignment explaining this design choice (e.g., that deleted_at is cleared so the anonymized record remains queryable for audit/history and not filtered out by soft-delete logic) so future readers understand the intent.
30-44: Parameterstatusshadows the importedstatusmodule.The parameter name
statuson line 33 shadowsfrom fastapi import ... statusimported on line 4. While this works because thestatusmodule is only used for HTTP status codes (e.g.,status.HTTP_400_BAD_REQUEST) elsewhere in the file and not within this function, it can cause confusion and potential bugs iflist_volunteersis modified later.♻️ Rename parameter to avoid shadowing
`@router.get`("", response_model=List[VolunteerResponse]) def list_volunteers( group_tag: Optional[str] = None, - status: Optional[VolunteerStatus] = None, + volunteer_status: Optional[VolunteerStatus] = None, include_deleted: bool = False, db: Session = Depends(get_db), current_user: AdminAuth = Depends(get_current_user), ) -> List[VolunteerResponse]: q = db.query(Volunteer) if not include_deleted: q = q.filter(Volunteer.deleted_at.is_(None)) if group_tag: q = q.filter(Volunteer.group_tag == group_tag) - if status: - q = q.filter(Volunteer.status == status) + if volunteer_status: + q = q.filter(Volunteer.status == volunteer_status) volunteers = q.order_by(Volunteer.last_name, Volunteer.first_name).all() return [VolunteerResponse.model_validate(v) for v in volunteers]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/v1/endpoints/volunteers.py` around lines 30 - 44, The parameter name status in function list_volunteers shadows the imported status module from FastAPI; rename the parameter (for example to volunteer_status or filter_status) and update its type hint (Optional[VolunteerStatus]) and every reference inside list_volunteers (the q.filter call) to use the new name so the imported status module remains accessible without conflict.
79-117: Bulk import loads entire file into memory.Line 87 reads the entire uploaded file into memory with
file.file.read(). For large CSV/XLSX files with thousands of volunteers, this could cause memory pressure. Consider streaming or chunked processing for production use if large imports are expected.Would you like me to propose a streaming approach using
openpyxlin read-only mode orcsv.readerwith the file object directly?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/v1/endpoints/volunteers.py` around lines 79 - 117, The handler import_volunteers currently calls file.file.read() which loads the entire upload into memory; change parsing to stream the uploaded file instead by modifying parse_volunteers_file to accept a file-like/stream (or a path) and iterate rows rather than taking a bytes blob, then call it with the UploadFile.file (or wrap it in a text IO wrapper for CSV) so rows are yielded one-by-one; for XLSX use openpyxl in read_only mode or for CSV use csv.reader on the file object, process and validate each row incrementally in import_volunteers (normalize_phone_for_storage, duplicate check, db.add) and optionally commit in batches to avoid memory/transaction bloat.backend/app/api/v1/endpoints/public.py (1)
194-206: Redundant empty phone check after normalization.Lines 197-198 check
if not phoneafter_normalize_phone(body.phone)on line 196. However,_normalize_phonealready raisesHTTPExceptionfor empty or invalid phones (vianormalize_phone_for_storagewhich raisesValueError("נא להזין טלפון")for empty input). This check is unreachable.♻️ Remove redundant check
`@router.post`("/auth/request-otp") def request_otp(body: OtpRequest, db: Session = Depends(get_db)) -> dict: phone = _normalize_phone(body.phone) - if not phone: - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="נא להזין טלפון") code = _generate_otp_code()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/v1/endpoints/public.py` around lines 194 - 206, The check for "if not phone" in request_otp is redundant because _normalize_phone(body.phone) already validates and raises the appropriate HTTPException (via normalize_phone_for_storage); remove the conditional block that raises HTTPException and its branch so request_otp simply assigns phone = _normalize_phone(body.phone) and proceeds with generating/saving the OTP and sending the SMS; keep the rest of request_otp (code generation, DB delete/add/commit, send_otp_sms) unchanged.backend/tests/test_event_volunteer_flow.py (1)
43-51: Consider using existing session or fixture for consistency.The
approve_volunteer_by_phonehelper creates its own session, which works due toStaticPoolbut deviates from typical test patterns where a shared fixture session is used. This is acceptable for test code but could be refactored to accept a session parameter for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_event_volunteer_flow.py` around lines 43 - 51, The helper approve_volunteer_by_phone currently creates its own TestingSessionLocal() which breaks consistency with tests that use a shared session fixture; change the function signature to accept a db/session parameter (e.g., db: Session) and use that session to query and commit, falling back to creating and closing a session only if none is provided; update callers in tests to pass the shared fixture session so Volunteer and VolunteerStatus operations use the same transaction context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/v1/endpoints/public.py`:
- Around line 63-67: The _generate_otp_code function uses random.randint
(Mersenne Twister) which is not cryptographically secure; replace it with the
secrets module by computing n as before and generating the integer via
secrets.randbelow(high - low + 1) + low (or an equivalent secrets-based method)
then return its string; also add the necessary import for the secrets module and
keep the existing logic that bounds n using settings.otp_code_length.
In `@backend/app/core/config.py`:
- Around line 56-57: Validate and constrain the OTP config values at load time:
ensure otp_code_length is clamped to 1..10 (to match VolunteerOtp.code
String(10) and prevent zero-length codes) and otp_expiry_minutes is at least 1
(positive). In the config initializer where otp_code_length and
otp_expiry_minutes are read/set, add validation logic that replaces out-of-range
values with safe defaults (e.g., 6 and 10) and/or raises a clear configuration
error; reference the otp_code_length and otp_expiry_minutes variables and the
VolunteerOtp.code width to locate where to add this check.
In `@backend/app/models/volunteer_otp.py`:
- Line 11: The volunteer OTP model currently stores OTPs in plaintext in the
Column named "code" (volunteer_otp.py); change to store a one-way hash instead:
update the model to increase the "code" column length to hold the hash, hash the
OTP on write using a secure algorithm (e.g., bcrypt or argon2) inside the
create/update path for functions that set OTPs, and modify the verification path
to hash the incoming OTP and compare with the stored hash (or use the hash
library's verify function). Add a migration to convert existing plaintext rows
(either re-hash after forcing expiry/reset or mark them expired and require
re-issue) and update any unit/integration tests that call the OTP
creation/verification functions to use the new hash-based flow. Ensure salts and
appropriate cost parameters are used and do not log OTPs or hashes.
In `@backend/app/services/excel_import.py`:
- Around line 329-331: The code incorrectly claims to support “.xls” but then
calls parse_volunteers_excel which uses openpyxl and will crash for old .xls
files; update the filename check so only formats openpyxl supports are accepted
(e.g. .xlsx, .xlsm, .xltx, .xltm) and treat .xls as unsupported (return the
existing error message) or alternately implement a separate handler (e.g.
parse_volunteers_xls using xlrd) and route .xls to that handler; modify the
condition around fn.endswith(...) and references to parse_volunteers_excel
accordingly so .xls is no longer passed to parse_volunteers_excel.
In `@backend/app/services/sms.py`:
- Around line 35-37: The debug print currently exposes sensitive data by logging
phone and full OTP/body; update the debug branch in sms.py so it does not print
the full message or raw phone: when settings.debug is true replace the
print(f"[DEV] SMS skip (missing Twilio config): to={phone} body={body}") with a
redacted message that masks the phone (e.g. show only last 2–4 digits) and does
not include the OTP/body (use a placeholder like "<redacted_body>" or the body
length), referencing the same variables (settings.debug, phone, body) so you
locate and change that exact statement; keep returning False.
- Around line 48-49: The current bare except block "except Exception: return
False" in backend/app/services/sms.py swallows all errors; change it to catch
the exception as a variable (e.g., "except Exception as e"), log full error and
stacktrace (using logger.exception(...) or logging.exception(...)) with
contextual details (provider name, recipient, payload) and optionally report to
your monitoring (Sentry) before returning False; this preserves behavior while
exposing the error for debugging. Ensure the code references the existing logger
used in the module or import logging if none exists.
In `@frontend/src/features/public/JoinEventPage.tsx`:
- Around line 107-112: The useEffect in JoinEventPage referencing joinMutation
in its dependency list risks re-running when the mutation object identity
changes; instead destructure the stable properties you care about (e.g. const {
isPending, data: joinData, error: joinError, mutate } = joinMutation) and use
those in the effect dependency array (useEffect(() => { if (meQuery.data?.status
!== "approved") return; if (!isPending && !joinData && !joinError) mutate(); },
[meQuery.data, meQuery.error, isPending, joinData, joinError]) ), or
alternatively use a ref/one-time flag to ensure mutate is called only once.
Reference joinMutation, isPending, data, error, and mutate when making the
change.
---
Nitpick comments:
In `@backend/.env.example`:
- Around line 10-11: The TWILIO env keys in .env.example are out of order
causing dotenv-linter UnorderedKey; swap the lines so TWILIO_SMS_FROM appears
before TWILIO_WHATSAPP_FROM (i.e., reorder the keys TWILIO_SMS_FROM and
TWILIO_WHATSAPP_FROM) to match the linter's expected ordering.
In `@backend/alembic/versions/011_add_volunteer_status_and_otps.py`:
- Around line 36-44: The migration creates volunteer_otps with expires_at but
has no cleanup; add a TTL cleanup by updating this migration (or a follow-up
migration) to create a scheduled job or DB-level TTL that deletes rows where
expires_at < now(), and ensure you create an index on expires_at for efficient
deletion (in addition to the existing ix_volunteer_otps_phone). Reference the
volunteer_otps table and the expires_at column and either emit raw SQL to
register a scheduler task (pg_cron/pgagent) or create a DB function +
trigger/cron entry in the migration so expired OTPs are purged automatically.
In `@backend/app/api/v1/endpoints/event_token.py`:
- Around line 93-94: The endpoint is performing a redundant DB query for
Volunteer after already fetching and validating it in
get_event_volunteer_by_token; modify get_event_volunteer_by_token to return both
the Event and Volunteer (or a tuple/structure containing the volunteer) and
update the caller in get_event_volunteer_by_token usage site (where ev is used)
to use the returned volunteer instead of running
db.query(Volunteer).filter(Volunteer.id == ev.volunteer_id).first(); ensure
function signature and all call sites (including get_event_volunteer_by_token
and the endpoint that currently assigns event and volunteer) are updated
accordingly so only one DB round-trip fetches the volunteer.
In `@backend/app/api/v1/endpoints/public.py`:
- Around line 194-206: The check for "if not phone" in request_otp is redundant
because _normalize_phone(body.phone) already validates and raises the
appropriate HTTPException (via normalize_phone_for_storage); remove the
conditional block that raises HTTPException and its branch so request_otp simply
assigns phone = _normalize_phone(body.phone) and proceeds with generating/saving
the OTP and sending the SMS; keep the rest of request_otp (code generation, DB
delete/add/commit, send_otp_sms) unchanged.
In `@backend/app/api/v1/endpoints/volunteers.py`:
- Around line 196-213: The anonymize_volunteer function currently sets
v.deleted_at = None to keep the anonymized volunteer record visible rather than
soft-deleted; add a concise inline comment next to the v.deleted_at assignment
explaining this design choice (e.g., that deleted_at is cleared so the
anonymized record remains queryable for audit/history and not filtered out by
soft-delete logic) so future readers understand the intent.
- Around line 30-44: The parameter name status in function list_volunteers
shadows the imported status module from FastAPI; rename the parameter (for
example to volunteer_status or filter_status) and update its type hint
(Optional[VolunteerStatus]) and every reference inside list_volunteers (the
q.filter call) to use the new name so the imported status module remains
accessible without conflict.
- Around line 79-117: The handler import_volunteers currently calls
file.file.read() which loads the entire upload into memory; change parsing to
stream the uploaded file instead by modifying parse_volunteers_file to accept a
file-like/stream (or a path) and iterate rows rather than taking a bytes blob,
then call it with the UploadFile.file (or wrap it in a text IO wrapper for CSV)
so rows are yielded one-by-one; for XLSX use openpyxl in read_only mode or for
CSV use csv.reader on the file object, process and validate each row
incrementally in import_volunteers (normalize_phone_for_storage, duplicate
check, db.add) and optionally commit in batches to avoid memory/transaction
bloat.
In `@backend/app/schemas/volunteer.py`:
- Line 32: The VolunteerResponse model currently defaults status to
VolunteerStatus.PENDING which can mask missing upstream data; remove the default
so the field is required (change "status: VolunteerStatus =
VolunteerStatus.PENDING" to "status: VolunteerStatus" in the VolunteerResponse
model) and update any places that instantiate VolunteerResponse (ORM-to-schema
mappers or constructors) to explicitly pass a status value from the data source
to avoid silent misreports; reference: VolunteerResponse and VolunteerStatus.
In `@backend/tests/test_event_volunteer_flow.py`:
- Around line 43-51: The helper approve_volunteer_by_phone currently creates its
own TestingSessionLocal() which breaks consistency with tests that use a shared
session fixture; change the function signature to accept a db/session parameter
(e.g., db: Session) and use that session to query and commit, falling back to
creating and closing a session only if none is provided; update callers in tests
to pass the shared fixture session so Volunteer and VolunteerStatus operations
use the same transaction context.
In `@frontend/src/features/admin/VolunteersPage.tsx`:
- Around line 104-109: approveMutation currently has no onError handler so
failures are silent; add an onError callback to the useMutation call that
surfaces API errors to the user (for example call your existing
toast/notification helper or set an error state) and include the error message
returned by apiRequest; keep the existing onSuccess logic
(queryClient.invalidateQueries({ queryKey: ["volunteers"] })) and ensure the
onError references the same mutation (approveMutation) so the UI can show a
failure message and optionally revert any optimistic UI changes.
In `@frontend/src/features/public/VolunteerLoginPage.tsx`:
- Around line 95-97: When switching back to the phone step via the "החלף טלפון"
button, also clear the stored OTP state so the previous code doesn't remain;
update the onClick handler that currently calls setStep("phone") to additionally
reset the OTP state (call the OTP setter, e.g., setOtp("") or
setVerificationCode("") as used in this component) and ensure the
loading/validation state tied to the OTP is reset if applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fcd5c161-a9c3-4592-a121-b81d5e626824
📒 Files selected for processing (21)
backend/.env.examplebackend/alembic/versions/011_add_volunteer_status_and_otps.pybackend/app/api/v1/endpoints/auth.pybackend/app/api/v1/endpoints/event_token.pybackend/app/api/v1/endpoints/public.pybackend/app/api/v1/endpoints/volunteers.pybackend/app/core/config.pybackend/app/core/security.pybackend/app/models/__init__.pybackend/app/models/volunteer.pybackend/app/models/volunteer_otp.pybackend/app/schemas/volunteer.pybackend/app/services/excel_import.pybackend/app/services/sms.pybackend/tests/test_event_volunteer_flow.pyfrontend/src/App.tsxfrontend/src/features/admin/ControlRoomPage.tsxfrontend/src/features/admin/VolunteersPage.tsxfrontend/src/features/public/JoinEventPage.tsxfrontend/src/features/public/VolunteerLoginPage.tsxfrontend/src/lib/api.ts
Comment on lines
+63
to
+67
| def _generate_otp_code() -> str: | ||
| n = max(4, min(8, settings.otp_code_length)) | ||
| low = 10 ** (n - 1) | ||
| high = (10**n) - 1 | ||
| return str(random.randint(low, high)) |
There was a problem hiding this comment.
Use cryptographically secure random for OTP codes.
random.randint() uses a Mersenne Twister PRNG which is predictable and not suitable for security-sensitive operations. OTP codes should be generated using the secrets module to prevent potential prediction attacks.
🔒 Proposed fix using secrets module
-import random
+import secrets
from datetime import datetime, timedelta, timezone
...
def _generate_otp_code() -> str:
n = max(4, min(8, settings.otp_code_length))
low = 10 ** (n - 1)
high = (10**n) - 1
- return str(random.randint(low, high))
+ return str(secrets.randbelow(high - low + 1) + low)🧰 Tools
🪛 Ruff (0.15.6)
[error] 67-67: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/v1/endpoints/public.py` around lines 63 - 67, The
_generate_otp_code function uses random.randint (Mersenne Twister) which is not
cryptographically secure; replace it with the secrets module by computing n as
before and generating the integer via secrets.randbelow(high - low + 1) + low
(or an equivalent secrets-based method) then return its string; also add the
necessary import for the secrets module and keep the existing logic that bounds
n using settings.otp_code_length.
Comment on lines
+56
to
+57
| otp_code_length: int = 6 | ||
| otp_expiry_minutes: int = 10 |
There was a problem hiding this comment.
Bound OTP settings to safe ranges.
Line 56-57 accept any integer from env. Misconfigurations (e.g., otp_code_length=0 or >10, otp_expiry_minutes<=0) can break auth or violate the VolunteerOtp.code width (String(10)).
🛡️ Suggested hardening
-from pydantic import computed_field, Field
+from pydantic import computed_field, Field
@@
- otp_code_length: int = 6
- otp_expiry_minutes: int = 10
+ otp_code_length: int = Field(default=6, ge=4, le=10)
+ otp_expiry_minutes: int = Field(default=10, ge=1, le=60)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/core/config.py` around lines 56 - 57, Validate and constrain the
OTP config values at load time: ensure otp_code_length is clamped to 1..10 (to
match VolunteerOtp.code String(10) and prevent zero-length codes) and
otp_expiry_minutes is at least 1 (positive). In the config initializer where
otp_code_length and otp_expiry_minutes are read/set, add validation logic that
replaces out-of-range values with safe defaults (e.g., 6 and 10) and/or raises a
clear configuration error; reference the otp_code_length and otp_expiry_minutes
variables and the VolunteerOtp.code width to locate where to add this check.
|
|
||
| id = Column(Integer, primary_key=True, index=True) | ||
| phone = Column(String(20), nullable=False, index=True) | ||
| code = Column(String(10), nullable=False) |
There was a problem hiding this comment.
Avoid storing OTP codes in plaintext.
Line 11 stores active OTPs as raw values, which weakens breach impact containment. Store only a one-way hash and compare hashed input during verification.
I can draft a minimal migration + verification-path patch (hash on write, hash compare on verify, keep expiry logic unchanged) if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/models/volunteer_otp.py` at line 11, The volunteer OTP model
currently stores OTPs in plaintext in the Column named "code"
(volunteer_otp.py); change to store a one-way hash instead: update the model to
increase the "code" column length to hold the hash, hash the OTP on write using
a secure algorithm (e.g., bcrypt or argon2) inside the create/update path for
functions that set OTPs, and modify the verification path to hash the incoming
OTP and compare with the stored hash (or use the hash library's verify
function). Add a migration to convert existing plaintext rows (either re-hash
after forcing expiry/reset or mark them expired and require re-issue) and update
any unit/integration tests that call the OTP creation/verification functions to
use the new hash-based flow. Ensure salts and appropriate cost parameters are
used and do not log OTPs or hashes.
Comment on lines
+329
to
+331
| if fn.endswith(".xlsx") or fn.endswith(".xls"): | ||
| return parse_volunteers_excel(content) | ||
| return [], ["סוג קובץ לא נתמך. השתמש ב־CSV או Excel (xlsx)."] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to the official openpyxl documentation, does load_workbooksupport legacy.xlsfiles, or only XML-based formats such as.xlsx/.xlsm/.xltx/.xltm?
💡 Result:
According to the official openpyxl documentation, load_workbook does not support legacy .xls files. It only supports XML-based Office Open XML formats such as .xlsx, .xlsm, .xltx, and .xltm. The source code in openpyxl.reader.excel explicitly defines SUPPORTED_FORMATS = ('.xlsx', '.xlsm', '.xltx', '.xltm') and includes validation in _validate_archive that raises an InvalidFileException for .xls files, with the message: "openpyxl does not support the old .xls file format, please use xlrd to read this file, or convert it to the more recent .xlsx file format." The main documentation page confirms: "openpyxl is a Python library to read/write Excel 2010 xlsx/xlsm/xltx/xltm files."
Citations:
- 1: https://openpyxl.readthedocs.io/en/stable/_modules/openpyxl/reader/excel.html?highlight=load_workbook
- 2: https://openpyxl.readthedocs.io/
- 3: https://openpyxl.readthedocs.io/en/latest/api/openpyxl.reader.excel.html
- 4: https://openpyxl.readthedocs.io/en/stable/_modules/openpyxl/reader/excel.html
Unsupported .xls format will cause runtime crashes.
Line 329 advertises .xls support but routes it to openpyxl.load_workbook(), which explicitly rejects .xls files. According to openpyxl documentation, load_workbook only supports XML-based formats (.xlsx, .xlsm, .xltx, .xltm) and raises InvalidFileException with the message: "openpyxl does not support the old .xls file format." Any user uploading a .xls file will encounter a runtime failure despite the API claiming support.
✅ Suggested fix
- if fn.endswith(".xlsx") or fn.endswith(".xls"):
+ if fn.endswith(".xlsx"):
return parse_volunteers_excel(content)
- return [], ["סוג קובץ לא נתמך. השתמש ב־CSV או Excel (xlsx)."]
+ return [], ["סוג קובץ לא נתמך. השתמש ב־CSV או Excel (xlsx)."]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if fn.endswith(".xlsx") or fn.endswith(".xls"): | |
| return parse_volunteers_excel(content) | |
| return [], ["סוג קובץ לא נתמך. השתמש ב־CSV או Excel (xlsx)."] | |
| if fn.endswith(".xlsx"): | |
| return parse_volunteers_excel(content) | |
| return [], ["סוג קובץ לא נתמך. השתמש ב־CSV או Excel (xlsx)."] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/excel_import.py` around lines 329 - 331, The code
incorrectly claims to support “.xls” but then calls parse_volunteers_excel which
uses openpyxl and will crash for old .xls files; update the filename check so
only formats openpyxl supports are accepted (e.g. .xlsx, .xlsm, .xltx, .xltm)
and treat .xls as unsupported (return the existing error message) or alternately
implement a separate handler (e.g. parse_volunteers_xls using xlrd) and route
.xls to that handler; modify the condition around fn.endswith(...) and
references to parse_volunteers_excel accordingly so .xls is no longer passed to
parse_volunteers_excel.
Comment on lines
+35
to
+37
| if settings.debug: | ||
| print(f"[DEV] SMS skip (missing Twilio config): to={phone} body={body}") | ||
| return False |
There was a problem hiding this comment.
Do not log full OTP/body and phone together, even in debug mode.
Line 36 logs sensitive auth data (to + OTP body). Redact message content and mask phone to reduce accidental exposure.
🔐 Suggested redaction
- if settings.debug:
- print(f"[DEV] SMS skip (missing Twilio config): to={phone} body={body}")
+ if settings.debug:
+ masked = f"***{(phone or '')[-4:]}"
+ print(f"[DEV] SMS skip (missing Twilio config): to={masked}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/sms.py` around lines 35 - 37, The debug print currently
exposes sensitive data by logging phone and full OTP/body; update the debug
branch in sms.py so it does not print the full message or raw phone: when
settings.debug is true replace the print(f"[DEV] SMS skip (missing Twilio
config): to={phone} body={body}") with a redacted message that masks the phone
(e.g. show only last 2–4 digits) and does not include the OTP/body (use a
placeholder like "<redacted_body>" or the body length), referencing the same
variables (settings.debug, phone, body) so you locate and change that exact
statement; keep returning False.
Comment on lines
+48
to
+49
| except Exception: | ||
| return False |
There was a problem hiding this comment.
Avoid silent failure on SMS provider errors.
Line 48-49 swallows all exceptions and drops error context, which makes OTP delivery failures hard to detect and debug.
🛠️ Suggested reliability improvement
+import logging
+
+logger = logging.getLogger(__name__)
@@
- except Exception:
+ except Exception as exc:
+ logger.exception("SMS send failed")
return False🧰 Tools
🪛 Ruff (0.15.6)
[warning] 48-48: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/sms.py` around lines 48 - 49, The current bare except
block "except Exception: return False" in backend/app/services/sms.py swallows
all errors; change it to catch the exception as a variable (e.g., "except
Exception as e"), log full error and stacktrace (using logger.exception(...) or
logging.exception(...)) with contextual details (provider name, recipient,
payload) and optionally report to your monitoring (Sentry) before returning
False; this preserves behavior while exposing the error for debugging. Ensure
the code references the existing logger used in the module or import logging if
none exists.
Comment on lines
107
to
+112
| useEffect(() => { | ||
| if (step === "details" && phoneValue) { | ||
| setDetailsValue("phone", phoneValue) | ||
| if (meQuery.data?.status !== "approved") return | ||
| if (!joinMutation.isPending && !joinMutation.data && !joinMutation.error) { | ||
| joinMutation.mutate() | ||
| } | ||
| }, [step, phoneValue, setDetailsValue]) | ||
|
|
||
| function onPhoneSubmit(data: PhoneForm) { | ||
| setPhoneValue(data.phone) | ||
| joinMutation.mutate({ phone: data.phone }) | ||
| } | ||
|
|
||
| function onDetailsSubmit(data: DetailsForm) { | ||
| joinMutation.mutate({ | ||
| phone: data.phone, | ||
| first_name: data.first_name, | ||
| last_name: data.last_name, | ||
| area: data.area, | ||
| group_tag: data.group_tag, | ||
| }) | ||
| } | ||
| }, [joinMutation, meQuery.data, meQuery.error]) |
There was a problem hiding this comment.
Potential infinite loop risk in useEffect dependency.
Including joinMutation in the dependency array can cause issues because the mutation object reference may change, potentially triggering unnecessary re-evaluations. The current guards (!joinMutation.isPending && !joinMutation.data && !joinMutation.error) should prevent actual infinite calls, but the pattern is fragile.
🛠️ Safer alternative using a ref or state flag
+ const joinAttemptedRef = useRef(false)
+
useEffect(() => {
if (meQuery.data?.status !== "approved") return
- if (!joinMutation.isPending && !joinMutation.data && !joinMutation.error) {
+ if (!joinAttemptedRef.current) {
+ joinAttemptedRef.current = true
joinMutation.mutate()
}
- }, [joinMutation, meQuery.data, meQuery.error])
+ }, [meQuery.data])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (step === "details" && phoneValue) { | |
| setDetailsValue("phone", phoneValue) | |
| if (meQuery.data?.status !== "approved") return | |
| if (!joinMutation.isPending && !joinMutation.data && !joinMutation.error) { | |
| joinMutation.mutate() | |
| } | |
| }, [step, phoneValue, setDetailsValue]) | |
| function onPhoneSubmit(data: PhoneForm) { | |
| setPhoneValue(data.phone) | |
| joinMutation.mutate({ phone: data.phone }) | |
| } | |
| function onDetailsSubmit(data: DetailsForm) { | |
| joinMutation.mutate({ | |
| phone: data.phone, | |
| first_name: data.first_name, | |
| last_name: data.last_name, | |
| area: data.area, | |
| group_tag: data.group_tag, | |
| }) | |
| } | |
| }, [joinMutation, meQuery.data, meQuery.error]) | |
| const joinAttemptedRef = useRef(false) | |
| useEffect(() => { | |
| if (meQuery.data?.status !== "approved") return | |
| if (!joinAttemptedRef.current) { | |
| joinAttemptedRef.current = true | |
| joinMutation.mutate() | |
| } | |
| }, [meQuery.data]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/public/JoinEventPage.tsx` around lines 107 - 112, The
useEffect in JoinEventPage referencing joinMutation in its dependency list risks
re-running when the mutation object identity changes; instead destructure the
stable properties you care about (e.g. const { isPending, data: joinData, error:
joinError, mutate } = joinMutation) and use those in the effect dependency array
(useEffect(() => { if (meQuery.data?.status !== "approved") return; if
(!isPending && !joinData && !joinError) mutate(); }, [meQuery.data,
meQuery.error, isPending, joinData, joinError]) ), or alternatively use a
ref/one-time flag to ensure mutate is called only once. Reference joinMutation,
isPending, data, error, and mutate when making the change.
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
1 participant
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
pending/approved) and enforce access gating in join and event-token flows; admins can approve volunteers and send approval SMS.Test plan
cd backend && pytest -qMade with Cursor
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests