implement captcha on auth pages#384
Conversation
…d sign in link to the password reset page; implement shared `CaptchaSession` struct; reorder some struct members ASC
WalkthroughThis pull request integrates CAPTCHA validation into the user authentication and account recovery flows. A new CaptchaSession helper struct generates and stores CAPTCHA data server-side, which is then embedded into page and form structures for signin, signup, and reset endpoints. CAPTCHA validation occurs before password verification in each flow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Handler
participant CaptchaSession
participant Storage
participant AuthCheck
User->>Handler: GET /signin (or /reset, /signup)
Handler->>CaptchaSession: create(site_config)
CaptchaSession->>Storage: store captcha string
CaptchaSession-->>Handler: {id, image}
Handler-->>User: PageSignin {captcha_id, captcha_image, ...}
User->>Handler: POST form {captcha_id, captcha_value, ...}
Handler->>Storage: retrieve captcha for captcha_id
alt Captcha Valid
Handler->>AuthCheck: proceed with password/auth checks
AuthCheck-->>Handler: auth result
Handler-->>User: success or auth error
else Captcha Invalid
Handler-->>User: captcha validation error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/controller/user.rs (1)
739-745: Consider case-insensitive CAPTCHA validation.The current validation (line 743) is case-sensitive, which may frustrate users who type the CAPTCHA in the wrong case. Consider normalizing both sides to lowercase for better UX:
- if captcha_char != input.captcha_value { + if captcha_char.to_lowercase() != input.captcha_value.to_lowercase() { return Err(AppError::CaptchaError); }Apply this same change to
signin_post(line 932) andsignup_post(line 1035).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/controller/user.rstemplates/reset.htmltemplates/signin.html
🧰 Additional context used
🧬 Code graph analysis (1)
src/controller/user.rs (3)
src/controller/admin.rs (1)
default(363-384)src/controller/fmt.rs (1)
clean_html(68-83)src/controller/db_utils.rs (2)
is_valid_name(374-389)generate_nanoid_ttl(412-416)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cargo test, clippy and doc (windows-latest, x86_64-pc-windows-gnu)
- GitHub Check: Cargo test, clippy and doc (windows-latest, x86_64-pc-windows-msvc)
🔇 Additional comments (11)
templates/signin.html (2)
3-7: LGTM! CSP properly configured for CAPTCHA.The Content Security Policy correctly allows
data:URIs for images, which is necessary for displaying base64-encoded CAPTCHA images while maintaining a secure CSP.
31-41: LGTM! CAPTCHA UI elements properly integrated.The CAPTCHA implementation includes all necessary elements: image display, user input field, and hidden ID field for validation.
src/controller/user.rs (6)
690-720: LGTM! CAPTCHA properly integrated into reset flow.The reset page correctly generates and embeds CAPTCHA data before rendering, following the same pattern as other auth pages.
897-924: LGTM! Signin flow properly implements CAPTCHA.The signin page correctly creates a CAPTCHA session and includes a read-only check to prevent signins when the site is in read-only mode.
928-934: LGTM! CAPTCHA validation prevents brute-force attacks.Good use of
take()to ensure one-time CAPTCHA usage, and validation occurs before the expensive password check. The same case-sensitivity improvement mentioned earlier applies here as well.
987-1005: LGTM! Signup flow properly implements CAPTCHA.The signup page follows the same pattern as signin and reset, creating a CAPTCHA session before rendering.
1031-1037: LGTM! Signup validation properly implements CAPTCHA.The signup flow correctly validates CAPTCHA before proceeding with account creation. The same case-insensitivity improvement mentioned earlier applies here as well.
26-29: Web search unavailable — unable to verify captcha crate best practices.The original comment requests verification of the captcha library configuration against official best practices, but the web search tool is currently unavailable. While the code shows reasonable filter usage (Noise, Wave, Cow) with sensible parameters and a functional implementation, verification against the library's official recommendations cannot be completed.
templates/reset.html (3)
3-7: LGTM! CSP properly configured.The Content Security Policy correctly allows
data:URIs for CAPTCHA images while maintaining security.
65-83: LGTM! CAPTCHA UI elements properly integrated.The CAPTCHA implementation follows the same pattern as the signin page, with proper form field structure and hidden ID field.
96-103: LGTM! Nice UX improvement.Adding a sign-in link on the password reset page helps users who may have navigated there by mistake.
| /// Captcha session members | ||
| struct CaptchaSession { | ||
| id: String, | ||
| image: String, | ||
| } | ||
|
|
||
| impl CaptchaSession { | ||
| /// Generate new captcha | ||
| fn create(site_config: &SiteConfig) -> Result<Self, AppError> { | ||
| let captcha_difficulty = match site_config.captcha_difficulty.as_str() { | ||
| "Easy" => Difficulty::Easy, | ||
| "Medium" => Difficulty::Medium, | ||
| "Hard" => Difficulty::Hard, | ||
| _ => return Err(AppError::NotFound), | ||
| }; | ||
|
|
||
| let captcha = match site_config.captcha_name.as_str() { | ||
| "Amelia" => captcha::by_name(captcha_difficulty, CaptchaName::Amelia), | ||
| "Lucy" => captcha::by_name(captcha_difficulty, CaptchaName::Lucy), | ||
| "Mila" => captcha::by_name(captcha_difficulty, CaptchaName::Mila), | ||
| "Digits" => captcha_digits(), | ||
| _ => return Err(AppError::NotFound), | ||
| }; | ||
|
|
||
| let id = generate_nanoid_ttl(60); | ||
|
|
||
| DB.open_partition("captcha", Default::default())? | ||
| .insert(&id, &*captcha.chars_as_string())?; | ||
|
|
||
| Ok(Self { | ||
| id, | ||
| image: captcha.as_base64().unwrap(), | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider increasing the CAPTCHA TTL.
The 60-second TTL (line 1229) may be too short for users who need to read the form, solve the CAPTCHA, or switch between password managers. Consider increasing to 300 seconds (5 minutes) or making it configurable via SiteConfig.
🔎 Suggested improvement
- let id = generate_nanoid_ttl(60);
+ let id = generate_nanoid_ttl(300); // 5 minutesOr better yet, add a captcha_ttl field to SiteConfig for configurability.
📝 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.
| /// Captcha session members | |
| struct CaptchaSession { | |
| id: String, | |
| image: String, | |
| } | |
| impl CaptchaSession { | |
| /// Generate new captcha | |
| fn create(site_config: &SiteConfig) -> Result<Self, AppError> { | |
| let captcha_difficulty = match site_config.captcha_difficulty.as_str() { | |
| "Easy" => Difficulty::Easy, | |
| "Medium" => Difficulty::Medium, | |
| "Hard" => Difficulty::Hard, | |
| _ => return Err(AppError::NotFound), | |
| }; | |
| let captcha = match site_config.captcha_name.as_str() { | |
| "Amelia" => captcha::by_name(captcha_difficulty, CaptchaName::Amelia), | |
| "Lucy" => captcha::by_name(captcha_difficulty, CaptchaName::Lucy), | |
| "Mila" => captcha::by_name(captcha_difficulty, CaptchaName::Mila), | |
| "Digits" => captcha_digits(), | |
| _ => return Err(AppError::NotFound), | |
| }; | |
| let id = generate_nanoid_ttl(60); | |
| DB.open_partition("captcha", Default::default())? | |
| .insert(&id, &*captcha.chars_as_string())?; | |
| Ok(Self { | |
| id, | |
| image: captcha.as_base64().unwrap(), | |
| }) | |
| } | |
| } | |
| /// Captcha session members | |
| struct CaptchaSession { | |
| id: String, | |
| image: String, | |
| } | |
| impl CaptchaSession { | |
| /// Generate new captcha | |
| fn create(site_config: &SiteConfig) -> Result<Self, AppError> { | |
| let captcha_difficulty = match site_config.captcha_difficulty.as_str() { | |
| "Easy" => Difficulty::Easy, | |
| "Medium" => Difficulty::Medium, | |
| "Hard" => Difficulty::Hard, | |
| _ => return Err(AppError::NotFound), | |
| }; | |
| let captcha = match site_config.captcha_name.as_str() { | |
| "Amelia" => captcha::by_name(captcha_difficulty, CaptchaName::Amelia), | |
| "Lucy" => captcha::by_name(captcha_difficulty, CaptchaName::Lucy), | |
| "Mila" => captcha::by_name(captcha_difficulty, CaptchaName::Mila), | |
| "Digits" => captcha_digits(), | |
| _ => return Err(AppError::NotFound), | |
| }; | |
| let id = generate_nanoid_ttl(300); // 5 minutes | |
| DB.open_partition("captcha", Default::default())? | |
| .insert(&id, &*captcha.chars_as_string())?; | |
| Ok(Self { | |
| id, | |
| image: captcha.as_base64().unwrap(), | |
| }) | |
| } | |
| } |
|
Just for info: I'm using this branch on my instance and can confirm it's working, at least on login step.
|
|
Maybe better to increase delay freedit/src/controller/user.rs Line 912 in 5f86b3f |
#381
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.