fix: use bcrypt +hash instead of storing plain password (issue #19)#21
fix: use bcrypt +hash instead of storing plain password (issue #19)#21Mykal-Steele wants to merge 3 commits intohowwohmm:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded bcrypt-based admin password hash support via Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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)
main.py (2)
496-497: Minor: Username comparison is not constant-time.The
req.username != ADMIN_USERcomparison could leak timing information. While low-risk for a single admin user, consider usinghmac.compare_digest()for consistency with the token comparison approach.🔒 Proposed fix for constant-time username comparison
- if req.username != ADMIN_USER or not _verify_admin_password(req.password): + if not hmac.compare_digest(req.username, ADMIN_USER) or not _verify_admin_password(req.password):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 496 - 497, The username equality check should be done in constant time to avoid timing leaks: replace the plain comparison req.username != ADMIN_USER with a constant-time comparison using hmac.compare_digest and keep the existing _verify_admin_password(req.password) call; if either compare_digest(req.username, ADMIN_USER) is False or _verify_admin_password returns False, raise the same HTTPException(401, "Invalid credentials") to preserve behavior (update the conditional that currently references req.username and ADMIN_USER).
32-32: Optional:ADMIN_PASSWORDimport is unused.
ADMIN_PASSWORDis imported but no longer used in any active code paths. Consider removing it to avoid confusion, since_verify_admin_passwordnow exclusively usesADMIN_PASSWORD_HASH.♻️ Remove unused import
-from config import CRON_SECRET, ADMIN_SECRET, ADMIN_USER, ADMIN_PASSWORD, ADMIN_PASSWORD_HASH, SENTRY_DSN, CORS_ORIGINS +from config import CRON_SECRET, ADMIN_SECRET, ADMIN_USER, ADMIN_PASSWORD_HASH, SENTRY_DSN, CORS_ORIGINS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` at line 32, The import list in main.py still includes ADMIN_PASSWORD though it is unused; remove ADMIN_PASSWORD from the from config import ... statement so only the actually used symbols (e.g., ADMIN_PASSWORD_HASH, ADMIN_SECRET, ADMIN_USER, CRON_SECRET, SENTRY_DSN, CORS_ORIGINS) are imported; verify that _verify_admin_password references ADMIN_PASSWORD_HASH and that no other code relies on ADMIN_PASSWORD before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config.py`:
- Around line 21-23: The current config exposes ADMIN_PASSWORD and
ADMIN_PASSWORD_HASH but when ADMIN_PASSWORD_HASH is empty
_verify_admin_password() will always return False causing admins with only
ADMIN_PASSWORD to lose access; update startup logic to detect when
ADMIN_PASSWORD_HASH is empty and ADMIN_PASSWORD is set and either (a) log a
clear warning at startup (include both ENV names and advise setting
ADMIN_PASSWORD_HASH) using the existing logger, or (b) implement a temporary
fallback in _verify_admin_password() that accepts the plaintext ADMIN_PASSWORD
when ADMIN_PASSWORD_HASH is empty (and add a deprecation warning log), ensuring
you reference ADMIN_PASSWORD, ADMIN_PASSWORD_HASH and _verify_admin_password to
locate the code to change.
---
Nitpick comments:
In `@main.py`:
- Around line 496-497: The username equality check should be done in constant
time to avoid timing leaks: replace the plain comparison req.username !=
ADMIN_USER with a constant-time comparison using hmac.compare_digest and keep
the existing _verify_admin_password(req.password) call; if either
compare_digest(req.username, ADMIN_USER) is False or _verify_admin_password
returns False, raise the same HTTPException(401, "Invalid credentials") to
preserve behavior (update the conditional that currently references req.username
and ADMIN_USER).
- Line 32: The import list in main.py still includes ADMIN_PASSWORD though it is
unused; remove ADMIN_PASSWORD from the from config import ... statement so only
the actually used symbols (e.g., ADMIN_PASSWORD_HASH, ADMIN_SECRET, ADMIN_USER,
CRON_SECRET, SENTRY_DSN, CORS_ORIGINS) are imported; verify that
_verify_admin_password references ADMIN_PASSWORD_HASH and that no other code
relies on ADMIN_PASSWORD before committing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8fec0504-fef9-4a8a-b3e9-f25185f06b7c
📒 Files selected for processing (3)
config.pymain.pyrequirements.txt
…moving to ADMIN_PASSWORD_HASH)
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="main.py">
<violation number="1" location="main.py:469">
P1: Admin auth silently falls back to plaintext `ADMIN_PASSWORD` when bcrypt hash is unset, undermining hashed-only credential enforcement.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if ADMIN_PASSWORD: | ||
| return hmac.compare_digest(candidate, ADMIN_PASSWORD) | ||
|
|
||
| return False |
There was a problem hiding this comment.
P1: Admin auth silently falls back to plaintext ADMIN_PASSWORD when bcrypt hash is unset, undermining hashed-only credential enforcement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At main.py, line 469:
<comment>Admin auth silently falls back to plaintext `ADMIN_PASSWORD` when bcrypt hash is unset, undermining hashed-only credential enforcement.</comment>
<file context>
@@ -459,13 +459,17 @@ def _verify_admin_password(candidate: str) -> bool:
- except (ValueError, TypeError):
- return False
+ # fallback if ADMIN_PASSWORD_HASH is not set
+ if ADMIN_PASSWORD:
+ return hmac.compare_digest(candidate, ADMIN_PASSWORD)
+
</file context>
| if ADMIN_PASSWORD: | |
| return hmac.compare_digest(candidate, ADMIN_PASSWORD) | |
| return False | |
| return False |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Around line 468-470: The current fallback that compares candidate to
ADMIN_PASSWORD using hmac.compare_digest leaves the plaintext legacy secret
active; remove this unconditional fallback or gate it behind an explicit
migration flag (e.g., LEGACY_PASSWORD_MIGRATION_ENABLED) so the legacy
ADMIN_PASSWORD is only accepted when that flag is true; update the
authentication logic where ADMIN_PASSWORD_HASH and ADMIN_PASSWORD are used
(referencing ADMIN_PASSWORD_HASH, ADMIN_PASSWORD, and hmac.compare_digest) to
require the migration flag before falling back, and add a clear log/error when
the hash is missing but migration is disabled.
- Around line 451-452: The current _check_admin() treats ADMIN_SECRET as a
universal admin bearer and the bookmarklet now sends that secret; instead create
a separate ingest-only secret (e.g., INGEST_SECRET) and change the bookmarklet
flow to send INGEST_SECRET instead of ADMIN_SECRET, then modify _check_admin()
(or wrap it with a new function used by ingest endpoints) to only accept
INGEST_SECRET for ingest routes and reserve ADMIN_SECRET for full admin actions;
update any places that call _check_admin() from ingest handlers to perform the
ingest-only check, and ensure /api/admin/* handlers that require full admin
still validate against ADMIN_SECRET.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if ADMIN_SECRET and hmac.compare_digest(token, ADMIN_SECRET): | ||
| return |
There was a problem hiding this comment.
ADMIN_SECRET now has full-admin blast radius in the bookmarklet flow.
_check_admin() accepts ADMIN_SECRET as a universal admin bearer, and this PR now sends that same secret through a client-side bookmarklet flow. If the bookmarklet token leaks, every /api/admin/* endpoint is exposed, not just transcript ingest. Please split this into a dedicated ingest-only secret or stop treating ADMIN_SECRET as a global admin bypass.
Also applies to: 858-866
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main.py` around lines 451 - 452, The current _check_admin() treats
ADMIN_SECRET as a universal admin bearer and the bookmarklet now sends that
secret; instead create a separate ingest-only secret (e.g., INGEST_SECRET) and
change the bookmarklet flow to send INGEST_SECRET instead of ADMIN_SECRET, then
modify _check_admin() (or wrap it with a new function used by ingest endpoints)
to only accept INGEST_SECRET for ingest routes and reserve ADMIN_SECRET for full
admin actions; update any places that call _check_admin() from ingest handlers
to perform the ingest-only check, and ensure /api/admin/* handlers that require
full admin still validate against ADMIN_SECRET.
| # fallback if ADMIN_PASSWORD_HASH is not set | ||
| if ADMIN_PASSWORD: | ||
| return hmac.compare_digest(candidate, ADMIN_PASSWORD) |
There was a problem hiding this comment.
The plaintext fallback keeps the old auth path alive.
If ADMIN_PASSWORD_HASH is missing, login still authenticates against ADMIN_PASSWORD, so the plaintext secret can remain in active use indefinitely. That also conflicts with the config.py note that the legacy password is no longer part of the active auth flow. This should be gated behind an explicit temporary migration flag at most, or removed before release.
Also applies to: 500-500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main.py` around lines 468 - 470, The current fallback that compares candidate
to ADMIN_PASSWORD using hmac.compare_digest leaves the plaintext legacy secret
active; remove this unconditional fallback or gate it behind an explicit
migration flag (e.g., LEGACY_PASSWORD_MIGRATION_ENABLED) so the legacy
ADMIN_PASSWORD is only accepted when that flag is true; update the
authentication logic where ADMIN_PASSWORD_HASH and ADMIN_PASSWORD are used
(referencing ADMIN_PASSWORD_HASH, ADMIN_PASSWORD, and hmac.compare_digest) to
require the migration flag before falling back, and add a clear log/error when
the hash is missing but migration is disabled.
1 participant
Summary
ADMIN_PASSWORDcomparison with bcrypt (ADMIN_PASSWORD_HASH) in/api/admin/loginADMIN_PASSWORD_HASHfor login/api/admin/ingest-by-youtube-idto requireADMIN_SECRETtoken (bookmarklet flow)ADMIN_PASSWORDenv var only as migration commentbcryptdependency to requirements.txtWhat changed
bcryptand_verify_admin_password()tokenagainstADMIN_SECRET(503 when unset)ADMIN_PASSWORD_HASHbcryptWhy
Issue #19: admin password is stored/compared as plaintext; fix with secure hashing.
Validation
python -m py_compile main.pypassesNotes
.env:ADMIN_PASSWORD_HASH= generated bcrypt hashADMIN_SECRET= strong random tokenADMIN_PASSWORDfrom production values once migrated.Summary by cubic
Replaced plaintext admin auth with
bcrypt-based login via ADMIN_PASSWORD_HASH and moved the ingest bookmarklet toADMIN_SECRETtoken auth, using constant-time comparisons for secrets. Login temporarily falls back to ADMIN_PASSWORD if the hash is unset; ingest returns 503 when ADMIN_SECRET is missing.Migration
bcrypthash of the admin password.token(notpassword) to/api/admin/ingest-by-youtube-id.Dependencies
bcrypttorequirements.txt.Written for commit 5a32ea1. Summary will update on new commits.
Summary by CodeRabbit
Security
Chores