Skip to content

Conversation

@Mohima6
Copy link

@Mohima6 Mohima6 commented Aug 1, 2025

No description provided.

@Adez017
Copy link
Contributor

Adez017 commented Aug 2, 2025

Hi @Mohima6 , i think you had done pretty work but if you are contributing through GSSoC'25 . the ideal approach is first you need to create an issue and when the issue is assigned to you , then you can open an PR

@Mohima6
Copy link
Author

Mohima6 commented Aug 3, 2025

hello, @Adez017 ; thanks for letting me know.

@indra7777
Copy link
Owner

Security Review - Request Changes

Thanks for contributing the authentication API module @Mohima6! This is indeed an important backend component. However, I've identified several critical security issues that must be addressed before approval:

🚨 Critical Security Issues

  1. Hardcoded Secret Key (Line 10): SECRET_KEY = "your-secret-key" is a major vulnerability. This must be loaded from environment variables:

    import os
    SECRET_KEY = os.getenv("SECRET_KEY", "fallback-key-for-dev-only")
  2. Missing Rate Limiting: Login/signup endpoints are vulnerable to brute force attacks. Consider adding rate limiting with slowapi or similar.

  3. Weak Error Messages: Generic error messages can aid attackers. The login endpoint should return the same message for both "user not found" and "wrong password" scenarios.

📋 Additional Improvements Needed

  1. Missing Input Validation: No validation for username/password requirements (length, complexity, etc.)

  2. Missing Documentation: Add docstrings to all functions explaining parameters, return values, and usage.

  3. Missing Unit Tests: Authentication logic should have comprehensive test coverage.

  4. Database Transaction Safety: Consider adding proper error handling and rollback for database operations.

✅ What's Good

  • Proper use of bcrypt for password hashing
  • JWT implementation follows good practices
  • Clean separation of concerns
  • Proper FastAPI patterns

Recommendation: Please address the critical security issues (especially #1-3) before this can be approved for merge. The hardcoded secret key alone makes this unsuitable for any production environment.

Would you like to update the PR with these fixes?

@Mohima6
Copy link
Author

Mohima6 commented Sep 14, 2025

sure @indra7777 . thanks for letting me know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants