Skip to content

security: add bcrypt password hashing helpers#722

Open
saurabhhhcodes wants to merge 2 commits into
leonagoel:mainfrom
saurabhhhcodes:security/bcrypt-password-hashing-326
Open

security: add bcrypt password hashing helpers#722
saurabhhhcodes wants to merge 2 commits into
leonagoel:mainfrom
saurabhhhcodes:security/bcrypt-password-hashing-326

Conversation

@saurabhhhcodes

Copy link
Copy Markdown

What changed

  • Added backend.password_hashing with bcrypt hashing at cost factor 12.
  • Added safe verification helpers that fail closed for malformed hashes and legacy fast hashes such as MD5.
  • Added rehash detection for bcrypt hashes below the configured cost baseline.
  • Added focused tests for hash format, valid/invalid verification, MD5 rejection, weak-cost rejection, and rehash detection.
  • Documented the helper in SECURITY.md and added bcrypt to requirements.

Why

Issue #326 calls out weak password hashing with MD5. The current app delegates user email/password auth to Supabase Auth, so there is no live repository-managed MD5 password path to replace. This patch closes the local-password handling gap by providing a single approved helper for any backend/admin credential flow in this repo, while explicitly rejecting MD5-style hashes.

How to test

PYTHONPYCACHEPREFIX=/private/tmp/hybrid-recommender-326-pycache python3 -m py_compile backend/password_hashing.py tests/test_password_hashing.py
python3 -m ruff check backend/password_hashing.py tests/test_password_hashing.py
python3 -m pytest tests/test_password_hashing.py -q
git diff --check

Local result:

  • pytest tests/test_password_hashing.py -q -> 5 passed, 1 existing pytest config warning about unknown env option

Screenshots (if UI change)

Not applicable; backend security helper and tests only.

Checklist

  • I have read the CONTRIBUTING.md
  • My code follows PEP8 style (validated with ruff check for touched files)
  • I have tested my changes locally
  • I have added/updated tests where applicable
  • I can explain every line of code I've written
  • I have NOT used AI-generated code without understanding and attributing it

Related issue

Closes #326

AI assistance disclosure

  • I used AI assistance for implementation drafting and validation planning, and reviewed the final diff/tests before submission.

@github-actions

Copy link
Copy Markdown

🎉 Welcome to Hybrid Recommender, @saurabhhhcodes! This is your first contribution here!

Labels added: gssoc:approved | mentor:leonagoel | status:review-needed

PR Description Checklist:

YES - What changed section
YES - Why section
YES - How to test section
YES - Related issue linked

✅ PR description looks complete!

What happens next:

  1. @leonagoel will review your changes
  2. CI checks must pass
  3. Once approved, this PR will be auto-merged

⏱️ Please respond to review comments within 48 hours.

@github-actions

Copy link
Copy Markdown

🎉 Welcome to Hybrid Recommender, @saurabhhhcodes!

Thank you for your first pull request! Here's what happens next:

Step What Who
1 CI runs lint + smoke test 🤖 Automated
2 Code review 👤 @leonagoel
3 mentor:leonagoel label added 👤 Mentor
4 gssoc:approved label added 👤 Mentor
5 Auto-merge triggered 🤖 Automated
6 Points on leaderboard at 4 AM IST 🏆 GSSoC

⏱️ Please respond to any review comments within 48 hours.

📖 Resources:

Happy contributing! 🚀

@github-actions
github-actions Bot requested a review from leonagoel May 29, 2026 21:41
@saurabhhhcodes

Copy link
Copy Markdown
Author

Pushed a CI repair follow-up in 4ee1bf5 after the E2E workflow hit an import-time syntax error in backend/main.py.

What changed:

  • fixed the malformed /api/upload dependency signature
  • restored the cache setter's Redis setex try/except structure with in-memory fallback
  • moved cache metric globals to the top of _get_cached_response so Python can import the module

Local verification:

  • PYTHONPYCACHEPREFIX=/private/tmp/hybrid-recommender-326-pycache python3 -m py_compile backend/main.py backend/password_hashing.py tests/test_password_hashing.py
  • python3 -m pytest tests/test_password_hashing.py -q -> 5 passed
  • python3 -m ruff check backend/password_hashing.py tests/test_password_hashing.py -> passed
  • git diff --check -> passed

I also tried the E2E suite locally; it now reaches dependency import and stops because my local Python env is missing nltk, while the GitHub runner installs project requirements. The fresh Actions run is pending now.

@saurabhhhcodes

Copy link
Copy Markdown
Author

Follow-up pushed in 61020f6: the fresh E2E run got past the syntax/cache fixes, then failed because src/model/content_model.py imports sentence_transformers but the package was not listed in requirements.txt.

Fix:

  • added sentence-transformers>=2.2.2 to requirements.txt, matching the existing ContentRecommender implementation.

Local re-checks after the dependency-file change:

  • PYTHONPYCACHEPREFIX=/private/tmp/hybrid-recommender-326-pycache python3 -m py_compile backend/main.py backend/password_hashing.py tests/test_password_hashing.py
  • python3 -m pytest tests/test_password_hashing.py -q -> 5 passed
  • git diff --check -> passed

The next Actions run should verify the full E2E path with project requirements installed.

@saurabhhhcodes

Copy link
Copy Markdown
Author

Pushed another narrow E2E import fix in 01f7f1d.

The latest Actions run got past the SentenceTransformers dependency, then failed during app import because backend/main.py referenced CSRFMiddleware / CSRF token helpers without importing them from backend.csrf. This commit imports the existing CSRF middleware/helper symbols and adds the missing csrf_header_dep dependency used by mutating endpoints.

Local verification after the patch:

  • PYTHONPYCACHEPREFIX=/private/tmp/hybrid-recommender-326-pycache python3 -m py_compile backend/main.py backend/password_hashing.py tests/test_password_hashing.py
  • python3 -m pytest tests/test_password_hashing.py -q -> 5 passed
  • python3 -m ruff check backend/password_hashing.py tests/test_password_hashing.py -> passed
  • git diff --check -> passed

Fresh Actions are queued again now.

@saurabhhhcodes

Copy link
Copy Markdown
Author

Pushed ed9ea72 for the next E2E startup blocker: the app now reaches lifespan startup, where download_nltk_assets() uses logger before the module defines it.

Fix:

  • initialized logger = logging.getLogger(__name__) near the backend imports.

Local verification:

  • PYTHONPYCACHEPREFIX=/private/tmp/hybrid-recommender-326-pycache python3 -m py_compile backend/main.py backend/password_hashing.py tests/test_password_hashing.py
  • python3 -m pytest tests/test_password_hashing.py -q -> 5 passed
  • git diff --check -> passed

Fresh Actions are queued again.

@saurabhhhcodes

Copy link
Copy Markdown
Author

Final CI update: the fresh Actions run on ed9ea72 is fully green now.

Passing checks:

  • End-to-End API Test Suite
  • PEP8 Lint (flake8)
  • Pipeline Smoke Test

This confirms the bcrypt helper PR plus the import/startup repairs are clean in GitHub CI. Ready for maintainer review.

@leonagoel leonagoel left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @saurabhhhcodes! 👋

PR #722 has conflicts in 2 files that need resolving before we can merge:

  • backend/main.py (5 conflicts)
  • requirements.txt

Please fix locally:

git checkout security/bcrypt-password-hashing-326
git fetch origin
git merge origin/main

Resolve conflicts in both files, then:

git add backend/main.py requirements.txt
git commit -m "resolve merge conflicts with main"
git push

Thanks! 😊

@saurabhhhcodes

Copy link
Copy Markdown
Author

Resolved the merge conflicts with current main and pushed head 95655b1.

Resolution notes:

  • preserved this PR’s bcrypt password hashing helper and bcrypt>=4.1.0 dependency
  • kept current main’s bounded LRU response cache and newer cache/trending/categories updates
  • kept the CSRF dependency/import repair and backend logger initialization so the API imports cleanly
  • accepted current main’s PR-template casing and workflow/test updates

Validation run locally:

  • python3 -m py_compile backend/main.py backend/password_hashing.py tests/test_password_hashing.py
  • python3 -m pytest tests/test_password_hashing.py -q -> 5 passed
  • git diff --check
  • git diff --cached --check

I also attempted the E2E suite locally, but this machine does not have the repo dependencies installed (nltk missing before app import). GitHub Actions installs requirements.txt, so the fresh run should be the source of truth for the full E2E workflow.

@saurabhhhcodes

Copy link
Copy Markdown
Author

Small workflow follow-up pushed in 2bc033d after the refreshed run failed before producing logs.

Root cause: the E2E job’s Python setup step had both uses: actions/setup-python@v5 and a run: block on the same step after the main-merge conflict resolution. GitHub Actions treats that as an invalid workflow step.

Fix:

  • split dependency installation into its own Install dependencies step under the E2E job
  • kept the workflow commands unchanged

Validation:

  • local PyYAML parse of .github/workflows/ci.yml passed
  • git diff --check passed

Fresh CI should now be able to start normally.

@saurabhhhcodes

Copy link
Copy Markdown
Author

CI is green now after the rebase follow-up fixes. Latest run passed E2E API tests, pipeline smoke test, and flake8: https://github.com/leonagoel/hybrid-recommender/actions/runs/26693448969

@saurabhhhcodes

Copy link
Copy Markdown
Author

Hi @leonagoel, the requested conflict resolution is complete now. The PR is mergeable again and the latest CI run is green across E2E API tests, flake8, and pipeline smoke test. Could you please re-review when you get a chance?

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #758 ("test : added unit tests for causal_config validation errors") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #760 ("test : added unit tests for causal_model bounds") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #762 ("test : added unit tests for propensity_model smoothing bounds") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #764 ("test : added unit tests for compute_product_sentiment in nlp_engine") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #766 ("test : added unit tests for supplementary evaluation metrics") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #768 ("test : added unit tests for DatasetManager") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #770 ("test : added unit tests for db supabase client initialization") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #772 ("Feature: Implement Redis caching for database queries") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #773 ("Feature: Set up automated unit and integration test suite") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #774 ("Feature: Implement API rate-limiting middleware") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1040 ("fixed: Duplicate PR template files cause Git conflicts on case-insens…") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1527 ("fix(model): fix IndentationError in select_bandit_arm and unindented bandit arm weight assignment") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@saurabhhhcodes
saurabhhhcodes force-pushed the security/bcrypt-password-hashing-326 branch from 2263c9d to 35fbc64 Compare June 17, 2026 21:31
@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1633 ("fix: Duplicate results-processing loop removed from search_items(#1631)") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1640 ("fix: resolve hybrid_model merge conflicts and add explanation string") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1641 ("Fix/svd memory leak") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1642 ("Feature/evaluation dashboard") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1643 ("Feature/streamlit tfidf config") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1644 ("Fix/strict csrf origin validation") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1645 ("Feature/svd online updating") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1647 ("fix: add title query param endpoint to resolve HTTP 422 (#1075)") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1648 ("fix(federated): prevent regularization decay in aggregate_updates") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1650 ("fix: remove duplicate code block in hybrid_model.py causing Indentati…") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1653 ("fix: remove duplicate /api/recommend route") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1654 ("build: add faiss dependency for two-tower retrieval") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1655 ("fix: enforce stronger signup password validation") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1660 ("security: reject unissued csrf tokens") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1661 ("feat: add KNN-based user collaborative filtering (Issue #51)") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1665 ("fix: add missing faiss-cpu dependency to requirements.txt ") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1515 ("feat: harden realtime recommendation WebSocket flow") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1668 ("fix(ci): resolve syntax and indentation errors breaking upstream main") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1669 ("feat(rate-limit): implement Token Bucket algorithm for rate-limiting middleware for search API protection") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1675 ("fix: IDOR vulnerability on purchases endpoints (#294)") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1676 ("docs: add required Supabase initialization steps to Docker setup guide") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1677 ("fix: implement background cache cleanup to prevent OOM") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1678 ("feat: add recommendation feedback loop support") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

⚠️ Main branch was just updated!

PR #1679 ("fix: use gin_trgm_ops for GIN trigram index") was just merged into main.

Please rebase your branch to avoid conflicts:

git fetch origin
git rebase origin/main
git push --force-with-lease

Keeping your branch up to date prevents merge conflicts. 🚀

@saurabhhhcodes
saurabhhhcodes force-pushed the security/bcrypt-password-hashing-326 branch from 1924c65 to 44930e0 Compare July 2, 2026 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security: Weak password hashing using MD5 — migrate to bcrypt with cost factor 12

2 participants