Add bcrypt hashing for password #8
Conversation
WalkthroughThe pull request introduces a modification to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Filament/Resources/AccountResource/Form/Components/Password.php (2)
16-16: Consider adding password update specific handlingThe current implementation might benefit from separating password creation from password update logic.
Consider implementing a separate method for password updates:
public static function makeForUpdate(): Forms\Components\Field { return static::make() ->dehydrated(fn ($state) => filled($state)) // Only process if password is being changed ->required(false) ->minLength(8) ->rules(['confirmed']); }
16-16: Consider configuring bcrypt cost factorWhile Laravel's default bcrypt cost of 10 is generally good, you might want to explicitly configure it based on your server capabilities.
You can customize the cost factor in your
.envfile:BCRYPT_ROUNDS=12This allows you to adjust the computational cost based on your server's performance characteristics while maintaining security.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Filament/Resources/AccountResource/Form/Components/Password.php(1 hunks)
🔇 Additional comments (1)
src/Filament/Resources/AccountResource/Form/Components/Password.php (1)
16-16: Verify the component usage across the codebase
Let's ensure this component is properly integrated with account creation and update flows.
✅ Verification successful
Based on the shell script results, I can now provide a final response:
Password handling is properly integrated across account flows
The password component is correctly integrated within the account management system:
- Password encryption is consistently handled using bcrypt across all entry points:
- Account creation (
CreateAccount.php) - Account editing (
EditAccount.php) - Password change action (
ChangePasswordAction.php) - Form component (
Password.php)
- Account creation (
- Password confirmation is properly implemented with matching components
- Password fields are only visible when login is enabled (
is_logincheck)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how this Password component is used in account-related forms
# Find account-related form configurations
rg -l "AccountResource" --type php
# Check for password handling in account controllers
rg -A 5 "password" --type php
Length of output: 29387
| @@ -13,6 +13,7 @@ public static function make(): Forms\Components\Field | |||
| ->confirmed() | |||
| ->visible(fn (Forms\Get $get) => $get('is_login')) | |||
| ->password() | |||
| ->dehydrateStateUsing(fn ($state) => bcrypt($state)) | |||
There was a problem hiding this comment.
Prevent potential double-hashing of passwords
The current implementation has several security concerns:
- It will hash an already hashed password if the form is resubmitted
- It doesn't handle null/empty password states
- It might double-hash existing passwords during updates
Apply this diff to fix these issues:
- ->dehydrateStateUsing(fn ($state) => bcrypt($state))
+ ->dehydrateStateUsing(function ($state) {
+ if (empty($state)) {
+ return $state;
+ }
+ // Only hash if the password is modified/new
+ if (strlen($state) < 60 || !password_get_info($state)['algo']) {
+ return bcrypt($state);
+ }
+ return $state;
+ })📝 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.
| ->dehydrateStateUsing(fn ($state) => bcrypt($state)) | |
| ->dehydrateStateUsing(function ($state) { | |
| if (empty($state)) { | |
| return $state; | |
| } | |
| // Only hash if the password is modified/new | |
| if (strlen($state) < 60 || !password_get_info($state)['algo']) { | |
| return bcrypt($state); | |
| } | |
| return $state; | |
| }) |
Summary by CodeRabbit