feat: support bcryt password #383
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name=".env.example">
<violation number="1" location=".env.example:253">
P2: Duplicate `# SECURITY` section. There's already a SECURITY block at line 89-91 with `# OPEN_NOTEBOOK_PASSWORD=`. This creates duplicate documentation and an inconsistent state where `OPEN_NOTEBOOK_PASSWORD` appears twice (once commented, once uncommented). Consider removing the old section at lines 89-91 or updating it instead of adding a new one.</violation>
</file>
<file name="api/auth.py">
<violation number="1" location="api/auth.py:33">
P1: Plaintext password comparison using `==` is vulnerable to timing attacks. Use `secrets.compare_digest()` for constant-time string comparison to prevent attackers from inferring password characters based on response time differences.</violation>
</file>
<file name="docs/5-CONFIGURATION/security.md">
<violation number="1" location="docs/5-CONFIGURATION/security.md:82">
P1: Bcrypt hashes contain `$` characters that will be interpreted as shell variables. The example should use single quotes to prevent variable expansion: `OPEN_NOTEBOOK_PASSWORD='$2b$12$K1...'`</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
api/auth.py
Outdated
| return False | ||
|
|
||
| # Plaintext comparison | ||
| return provided == stored |
There was a problem hiding this comment.
P1: Plaintext password comparison using == is vulnerable to timing attacks. Use secrets.compare_digest() for constant-time string comparison to prevent attackers from inferring password characters based on response time differences.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/auth.py, line 33:
<comment>Plaintext password comparison using `==` is vulnerable to timing attacks. Use `secrets.compare_digest()` for constant-time string comparison to prevent attackers from inferring password characters based on response time differences.</comment>
<file context>
@@ -6,64 +6,88 @@
+ return False
+
+ # Plaintext comparison
+ return provided == stored
+
</file context>
✅ Addressed in 7491ee8
There was a problem hiding this comment.
@liyimeng can you address?
To be honest, compared to the naive security solution we provide today(allowing plain text password), worry about this kind of attack is unnecessary. If user does concern about security, they should at least use the encrypted password as this pr providing.
But I can provide an update anyway when I get access to my computers
docs/5-CONFIGURATION/security.md
Outdated
| Then set the environment variable to the printed hash: | ||
|
|
||
| ```bash | ||
| OPEN_NOTEBOOK_PASSWORD=$2b$12$K1... (paste the generated hash) |
There was a problem hiding this comment.
P1: Bcrypt hashes contain $ characters that will be interpreted as shell variables. The example should use single quotes to prevent variable expansion: OPEN_NOTEBOOK_PASSWORD='$2b$12$K1...'
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/5-CONFIGURATION/security.md, line 82:
<comment>Bcrypt hashes contain `$` characters that will be interpreted as shell variables. The example should use single quotes to prevent variable expansion: `OPEN_NOTEBOOK_PASSWORD='$2b$12$K1...'`</comment>
<file context>
@@ -63,6 +63,30 @@ OPEN_NOTEBOOK_PASSWORD=Notebook$Dev$2024$Strong!
+Then set the environment variable to the printed hash:
+
+```bash
+OPEN_NOTEBOOK_PASSWORD=$2b$12$K1... (paste the generated hash)
+```
+
</file context>
| OPEN_NOTEBOOK_PASSWORD=$2b$12$K1... (paste the generated hash) | |
| OPEN_NOTEBOOK_PASSWORD='$2b$12$K1...' # Use single quotes to prevent shell variable expansion |
✅ Addressed in f597bd2
.env.example
Outdated
| SURREAL_COMMANDS_MAX_TASKS=5 | ||
|
|
||
| # OPEN_NOTEBOOK_PASSWORD= | ||
| # SECURITY |
There was a problem hiding this comment.
P2: Duplicate # SECURITY section. There's already a SECURITY block at line 89-91 with # OPEN_NOTEBOOK_PASSWORD=. This creates duplicate documentation and an inconsistent state where OPEN_NOTEBOOK_PASSWORD appears twice (once commented, once uncommented). Consider removing the old section at lines 89-91 or updating it instead of adding a new one.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .env.example, line 253:
<comment>Duplicate `# SECURITY` section. There's already a SECURITY block at line 89-91 with `# OPEN_NOTEBOOK_PASSWORD=`. This creates duplicate documentation and an inconsistent state where `OPEN_NOTEBOOK_PASSWORD` appears twice (once commented, once uncommented). Consider removing the old section at lines 89-91 or updating it instead of adding a new one.</comment>
<file context>
@@ -250,7 +250,15 @@ SURREAL_COMMANDS_RETRY_WAIT_MAX=30
SURREAL_COMMANDS_MAX_TASKS=5
-# OPEN_NOTEBOOK_PASSWORD=
+# SECURITY
+# Set this to protect your Open Notebook instance with a password (for public hosting)
+# You may supply either a plaintext password or a bcrypt hash.
</file context>
✅ Addressed in 9b433a9
liyimeng
force-pushed
the
feature/support-bcrypt-password
branch
2 times, most recently
from
9b433a9 to
f597bd2
Compare
liyimeng
force-pushed
the
feature/support-bcrypt-password
branch
from
f597bd2 to
7491ee8
Compare
lfnovo
left a comment
lfnovo
left a comment
There was a problem hiding this comment.
Thanks for this security improvement! This is a valuable addition. I have a few suggestions before merging:
1. Remove Dead Code
The password_is_hash variable is defined but never used:
# In __init__
self.password_is_hash = bool(self.password and self.password.startswith("$2"))This can be removed since the verify_password() function handles the detection logic.
2. Add Error Logging
The bcrypt verification silently catches all exceptions:
try:
return bcrypt.checkpw(provided.encode("utf-8"), stored.encode("utf-8"))
except Exception:
# Any error in bcrypt verification should be treated as an invalid password
return FalseThis could hide configuration issues. Consider logging the error with loguru:
from loguru import logger
# In verify_password function
except Exception as e:
logger.error(f"bcrypt verification failed: {e}")
return False3. Clarify Documentation
In docs/5-CONFIGURATION/security.md:87, it says:
You must have
bcryptinstalled on the server for hashed-mode to work.
But bcrypt is now a required dependency in pyproject.toml, so this note is confusing. Consider rephrasing to:
The server includes bcrypt support by default for password hashing.
Great work on addressing the timing attack vulnerability with secrets.compare_digest() and cleaning up the duplicate password entries! This is a solid security improvement.
2 participants
Description
Right now, only simple plain text password is supported, this PR allow you to set an encrypted password, which is far more safer than plain text.
Related Issue
Fixes #
Type of Change
How Has This Been Tested?
uv run pytest)Test Details:
Design Alignment
Which design principles does this PR support? (See DESIGN_PRINCIPLES.md)
Explanation:
This PR allow you set OPEN_NOTEBOOK_PASSWORD into a bcrypted password, and the backend will smartly check if you are using plain text or encrypted password, and act accordingly.
Checklist
Code Quality
Testing
make rufforruff check . --fixmake lintoruv run python -m mypy .Documentation
/docs(if applicable)Database Changes
/migrations)Breaking Changes
Screenshots (if applicable)
Additional Context
Pre-Submission Verification
Before submitting, please verify:
Thank you for contributing to Open Notebook! 🎉