-
Notifications
You must be signed in to change notification settings - Fork 92
feat(security): add mandatory password change step to admin setup wizard #694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
c9c31a2
e4910f2
ef89ba6
c114425
517a088
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ | |
| QuickAccessTeam, | ||
| WelcomeConfigResponse, | ||
| ) | ||
| from app.schemas.user import UserCreate, UserInDB, UserUpdate | ||
| from app.schemas.user import ChangePasswordRequest, UserCreate, UserInDB, UserUpdate | ||
| from app.services.kind import kind_service | ||
| from app.services.user import user_service | ||
|
|
||
|
|
@@ -80,6 +80,26 @@ async def update_current_user_endpoint( | |
| raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) | ||
|
|
||
|
|
||
| @router.put("/me/password", response_model=UserInDB) | ||
| async def change_password( | ||
| request: ChangePasswordRequest, | ||
| db: Session = Depends(get_db), | ||
| current_user: User = Depends(security.get_current_user), | ||
| ): | ||
| """Change current user's password""" | ||
| if request.new_password != request.confirm_password: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail="Passwords do not match", | ||
| ) | ||
|
|
||
| # Hash and update the password | ||
| current_user.password_hash = security.get_password_hash(request.new_password) | ||
| db.commit() | ||
| db.refresh(current_user) | ||
| return current_user | ||
|
Comment on lines
+83
to
+110
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: Password change endpoint (authenticated user) — security best practices1) Require strong proof it’s really the user
2) Prevent abuse (brute force/DoS)
3) Make password selection resistant to guessing and breaches
4) Protect the endpoint itself
5) Secure lifecycle after the change
6) Avoid authorization mistakes
Sources
🏁 Script executed: # Find the ChangePasswordRequest model definition
rg "class ChangePasswordRequest" --type pyRepository: wecode-ai/Wegent Length of output: 127 🏁 Script executed: # Search for where the change_password endpoint is called or documented
rg "change_password|/me/password" --type py -B 2 -A 2Repository: wecode-ai/Wegent Length of output: 422 🏁 Script executed: # Check for rate limiting or security middleware
rg "rate_limit|RateLimiter|throttle" --type pyRepository: wecode-ai/Wegent Length of output: 8207 🏁 Script executed: # Search for session invalidation logic after password change
rg "session|Session|refresh_token|clear_cookies" backend/app/api/endpoints/users.py -B 2 -A 2Repository: wecode-ai/Wegent Length of output: 1480 🏁 Script executed: # Check for audit logging or security event tracking
rg "audit|log_event|security_log" --type pyRepository: wecode-ai/Wegent Length of output: 847 🏁 Script executed: # Look at the full users.py file to understand the context of the endpoint
wc -l backend/app/api/endpoints/users.pyRepository: wecode-ai/Wegent Length of output: 98 🏁 Script executed: # View the ChangePasswordRequest model to see what fields it requires
cat -n backend/app/schemas/user.py | grep -A 10 "class ChangePasswordRequest"Repository: wecode-ai/Wegent Length of output: 277 🏁 Script executed: # View the complete change_password endpoint implementation
sed -n '83,120p' backend/app/api/endpoints/users.pyRepository: wecode-ai/Wegent Length of output: 1397 🏁 Script executed: # Check if rate limiting is applied to the password change endpoint
sed -n '75,105p' backend/app/api/endpoints/users.pyRepository: wecode-ai/Wegent Length of output: 1028 🏁 Script executed: # Check for session/token invalidation after password change
rg "invalidate|revoke|token|session" backend/app/api/endpoints/users.py -B 2 -A 2Repository: wecode-ai/Wegent Length of output: 648 🏁 Script executed: # Look at security module to understand what get_current_user provides
fd "security" --type f backend/app | head -10Repository: wecode-ai/Wegent Length of output: 135 🏁 Script executed: # View the security module to understand authentication and token handling
cat backend/app/core/security.py | head -100Repository: wecode-ai/Wegent Length of output: 3592 🏁 Script executed: # Check if there's any token refresh or invalidation logic after password change
rg "password|token|refresh" backend/app/core/security.py -B 2 -A 2Repository: wecode-ai/Wegent Length of output: 10906 🏁 Script executed: # Look for any password change event handlers or callbacks
rg "change_password|password.*change" --type pyRepository: wecode-ai/Wegent Length of output: 1546 🏁 Script executed: # Check if rate limiting is defined anywhere for sensitive endpoints
cat backend/app/core/rate_limit.pyRepository: wecode-ai/Wegent Length of output: 3168 Add current password verification to password change endpoint. The endpoint currently allows password changes without verifying the current password, which violates OWASP ASVS security standards. This creates security risks:
Add an
If this endpoint is only for initial admin setup (where the default password is well-known), document this constraint and consider a separate, more restrictive flow for general user password changes. 🧰 Tools🪛 Ruff (0.15.4)[warning] 86-86: Do not perform function call (B008) [warning] 87-87: Do not perform function call (B008) 🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| @router.delete("/me/git-token/{git_domain:path}", response_model=UserInDB) | ||
| async def delete_git_token( | ||
| git_domain: str, | ||
|
|
@@ -312,6 +332,7 @@ async def get_welcome_config( | |
|
|
||
| # Check admin setup status for admin users | ||
| admin_setup_completed = None | ||
| admin_password_changed = None | ||
| if current_user.role == "admin": | ||
| setup_config = ( | ||
| db.query(SystemConfig) | ||
|
|
@@ -323,6 +344,17 @@ async def get_welcome_config( | |
| else: | ||
| admin_setup_completed = False | ||
|
|
||
| # Check if admin password has been changed from default | ||
| from app.core.yaml_init import DEFAULT_ADMIN_Password_HASH | ||
|
|
||
| admin_user = db.query(User).filter(User.user_name == "admin").first() | ||
| if admin_user: | ||
| admin_password_changed = ( | ||
| admin_user.password_hash != DEFAULT_ADMIN_Password_HASH | ||
| ) | ||
| else: | ||
| admin_password_changed = True | ||
|
|
||
| if not config: | ||
| # Return default configuration | ||
| return WelcomeConfigResponse( | ||
|
|
@@ -331,6 +363,7 @@ async def get_welcome_config( | |
| ], | ||
| tips=[ChatTipItem(**tip) for tip in DEFAULT_SLOGAN_TIPS_CONFIG["tips"]], | ||
| admin_setup_completed=admin_setup_completed, | ||
| admin_password_changed=admin_password_changed, | ||
| ) | ||
|
|
||
| config_value = config.config_value or {} | ||
|
|
@@ -344,6 +377,7 @@ async def get_welcome_config( | |
| for tip in config_value.get("tips", DEFAULT_SLOGAN_TIPS_CONFIG["tips"]) | ||
| ], | ||
| admin_setup_completed=admin_setup_completed, | ||
| admin_password_changed=admin_password_changed, | ||
| ) | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: wecode-ai/Wegent
Length of output: 355
🏁 Script executed:
Repository: wecode-ai/Wegent
Length of output: 3051
🏁 Script executed:
Repository: wecode-ai/Wegent
Length of output: 287
🏁 Script executed:
Repository: wecode-ai/Wegent
Length of output: 670
🏁 Script executed:
Repository: wecode-ai/Wegent
Length of output: 756
🏁 Script executed:
Repository: wecode-ai/Wegent
Length of output: 548
Security & code duplication: Exposing
admin_password_changedon unauthenticated endpoint with duplicated logic across endpoints.The
/healthendpoint is unauthenticated, exposingadmin_password_changedpublicly. This allows attackers to probe whether default credentials (admin/Wegent2025!) are still active. While this is an intentional design choice for the login page to auto-fill credentials (verified infrontend/src/features/login/components/LoginForm.tsx), it creates an information disclosure vector that should be explicitly documented as an approved security trade-off.Additionally, the admin password check logic is duplicated across at least three endpoints:
backend/app/api/endpoints/health.py(lines 38-47)backend/app/api/endpoints/users.py(lines 346-355)backend/app/api/endpoints/admin/system_config.py(lines 238-241)Extract this into a shared utility function to eliminate duplication and ensure consistent behavior:
Then replace duplicated logic in all three endpoints with calls to this utility.
🧰 Tools
🪛 Ruff (0.15.4)
[warning] 49-56: Consider moving this statement to an
elseblock(TRY300)
🤖 Prompt for AI Agents