Skip to content

Staging #230

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.

Sign up for GitHub

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

Merged
gkorland merged 8 commits into from
Sep 7, 2025
Merged

Staging #230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fdc5810
don't show logo
gkorland Sep 5, 2025
53b60db
fix email login
gkorland Sep 7, 2025
461060b
fix comments
gkorland Sep 7, 2025
5ab0d4e
fix comment
gkorland Sep 7, 2025
24c5ff3
fix comment
gkorland Sep 7, 2025
3549a12
fix name result
gkorland Sep 7, 2025
5182962
Merge pull request #229 from FalkorDB/email-login
gkorland Sep 7, 2025
a7ccd41
Merge branch 'main' into staging
gkorland Sep 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 58 additions & 44 deletions api/routes/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import logging
import os
import re
import time
import secrets

from pathlib import Path
Expand Down Expand Up @@ -161,6 +160,17 @@
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail="Internal server error"
)

def _is_request_secure(request: Request) -> bool:
"""Determine if the request is secure (HTTPS)."""

# Check X-Forwarded-Proto first (proxy-aware)
forwarded_proto = request.headers.get("x-forwarded-proto")
if forwarded_proto:
return forwarded_proto == "https"

# Fallback to request URL scheme
return request.url.scheme == "https"
Comment on lines +164 to +173
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Sep 7, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Make proxy HTTPS detection robust (multi-proxy, casing).

Handle comma-separated X-Forwarded-Proto and case-insensitivity to avoid misclassifying HTTPS and failing to set Secure on cookies.

-def _is_request_secure(request: Request) -> bool:
-    """Determine if the request is secure (HTTPS)."""
-    
-    # Check X-Forwarded-Proto first (proxy-aware)
-    forwarded_proto = request.headers.get("x-forwarded-proto")
-    if forwarded_proto:
-        return forwarded_proto == "https"
-    
-    # Fallback to request URL scheme
-    return request.url.scheme == "https"
+def _is_request_secure(request: Request) -> bool:
+    """Determine if the request is secure (HTTPS), proxy-aware."""
+    forwarded_proto = request.headers.get("x-forwarded-proto", "")
+    if forwarded_proto:
+        first = forwarded_proto.split(",")[0].strip().lower()
+        return first == "https"
+    return request.url.scheme.lower() == "https"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def _is_request_secure(request: Request) -> bool:
"""Determine if the request is secure (HTTPS)."""
# Check X-Forwarded-Proto first (proxy-aware)
forwarded_proto = request.headers.get("x-forwarded-proto")
if forwarded_proto:
return forwarded_proto == "https"
# Fallback to request URL scheme
return request.url.scheme == "https"
def _is_request_secure(request: Request) -> bool:
"""Determine if the request is secure (HTTPS), proxy-aware."""
forwarded_proto = request.headers.get("x-forwarded-proto", "")
if forwarded_proto:
first = forwarded_proto.split(",")[0].strip().lower()
return first == "https"
return request.url.scheme.lower() == "https"
🤖 Prompt for AI Agents 
In api/routes/auth.py around lines 164 to 173, the X-Forwarded-Proto check is
too strict: it doesn't handle comma-separated values from multiple proxies or
case differences, which can misclassify HTTPS requests; change the logic to
split the header on commas, strip whitespace, lower-case each value, and return
True if any of the entries equals "https" (fall back to request.url.scheme ==
"https" if header missing).


async def _authenticate_email_user(email: str, password: str):
"""Authenticate an email user."""
Expand Down Expand Up @@ -206,18 +216,18 @@
"""Handle email/password user registration."""
try:
# Check if email authentication is enabled
if os.getenv("EMAIL_AUTH_ENABLED", "").lower() not in ["true", "1", "yes", "on"]:
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail="GEmail authentication is not enabled"
)
if not _is_email_auth_enabled():
return JSONResponse(
{"success": False, "error": "Email authentication is not enabled"},
status_code=status.HTTP_403_FORBIDDEN
)

# Validate required fields
if not all([signup_data.firstName, signup_data.lastName,
signup_data.email, signup_data.password]):
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="All fields are required"
return JSONResponse(
{"success": False, "error": "All fields are required"},
status_code=status.HTTP_400_BAD_REQUEST
)

first_name = signup_data.firstName.strip()
Expand All @@ -227,16 +237,16 @@

# Validate email format
if not _validate_email(email):
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Invalid email format"
return JSONResponse(
{"success": False, "error": "Invalid email format"},
status_code=status.HTTP_400_BAD_REQUEST
)

# Validate password strength
if len(password) < 8:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Password must be at least 8 characters long"
return JSONResponse(
{"success": False, "error": "Password must be at least 8 characters long"},
status_code=status.HTTP_400_BAD_REQUEST
)

api_token = secrets.token_urlsafe(32)
Expand Down Expand Up @@ -265,23 +275,23 @@
key="api_token",
value=api_token,
httponly=True,
secure=True
secure=_is_request_secure(request)
)
return response

except Exception as e:
logging.error("Signup error: %s", e)
raise HTTPException(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail="Registration failed"
return JSONResponse(
{"success": False, "error": "Registration failed"},
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR
)
Comment on lines 282 to 287
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Sep 7, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Tighten exception handling and improve logging.

Avoid blind except Exception; re-raise HTTPException and use logging.exception for stack traces (matches Ruff hints).

-    except Exception as e:
-        logging.error("Signup error: %s", e)
+    except HTTPException:
+        raise
+    except Exception as e:
+        logging.exception("Signup error: %s", e)
         return JSONResponse(
             {"success": False, "error": "Registration failed"},
             status_code=status.HTTP_500_INTERNAL_SERVER_ERROR
         )
-    except Exception as e:
-        logging.error("Login error: %s", e)
+    except HTTPException:
+        raise
+    except Exception as e:
+        logging.exception("Login error: %s", e)
         return JSONResponse(
             {"success": False, "error": "Login failed"},
             status_code=status.HTTP_500_INTERNAL_SERVER_ERROR
         )

Also applies to: 364-369

🧰 Tools
🪛 Ruff (0.12.2)

282-282: Do not catch blind exception: Exception

(BLE001)

283-283: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

🤖 Prompt for AI Agents 
In api/routes/auth.py around lines 282-287 (and similarly 364-369), replace the
blind "except Exception" with explicit handling that re-raises HTTPException
instances and only catches unexpected exceptions; use logging.exception(...) to
record the full stack trace for unexpected errors and then return the
JSONResponse 500. Concretely: add an except HTTPException: raise to let FastAPI
handle known HTTP errors, and change the general catch to except Exception as e:
logging.exception("Signup error"); return the existing JSONResponse with status
500.


@auth_router.post("/login/email")
async def email_login(request: Request, login_data: EmailLoginRequest) -> JSONResponse:
"""Handle email/password user login."""
try:
# Check if email authentication is enabled
if os.getenv("EMAIL_AUTH_ENABLED", "").lower() not in ["true", "1", "yes", "on"]:
if not _is_email_auth_enabled():
return JSONResponse(
{"success": False, "error": "Email authentication is not enabled"},
status_code=status.HTTP_403_FORBIDDEN
Expand Down Expand Up @@ -309,48 +319,52 @@

if not success:
return JSONResponse(
{"success": False, "error": result},
{"success": False, "error": result},
status_code=status.HTTP_401_UNAUTHORIZED
)

# Set session data - result is a dict when success is True
if isinstance(result, dict):
user_node = result.get("user")
identity_node = result.get("identity")

# Access node properties correctly
user_props = (
user_node.properties
if user_node and hasattr(user_node, "properties")
else {}
)
identity_props = (
identity_node.properties
if identity_node and hasattr(identity_node, "properties")
else {}
)

request.session["user_info"] = {
"id": identity_props.get("provider_user_id", email),
"name": user_props.get("name", ""),
"email": user_props.get("email", email),
"picture": user_props.get("picture", ""),
"provider": "email",

user_data = {
'id': identity_props.get("provider_user_id", email),
'email': identity_props.get('email', email),
'name': identity_props.get('name', ''),
'picture': identity_props.get('picture', ''),
}
request.session["email_authenticated"] = True
request.session["token_validated_at"] = time.time()

return JSONResponse({"success": True, "message": "Login successful"})
else:
return JSONResponse(
{"success": False, "error": "Authentication failed"},
status_code=status.HTTP_401_UNAUTHORIZED
)
# Call the registered Google callback handler if it exists to store user data.
handler = getattr(request.app.state, "callback_handler", None)
if handler:
api_token = secrets.token_urlsafe(32) # ~43 chars, hard to guess

# Call the registered handler (await if async)
await handler('email', user_data, api_token)
response = JSONResponse({"success": True}, status_code=200)

response.set_cookie(
key="api_token",
value=api_token,

Check failure

Code scanning / CodeQL

Clear-text storage of sensitive information High

This expression stores
sensitive data (password)
Loading
as clear text.

Copilot Autofix

AI 7 months ago

The best way to fix this issue is to not store the raw API token directly in the cookie. Instead, employ one of the following robust mechanisms:

  • Store only a cryptographically signed version of the token that can be validated but not forged by the client.
  • Encrypt the token payload before placing it in the cookie.
  • Even better, store a random session identifier in the cookie and keep the actual token associated with that session ID server-side.

Given only the code provided and the likely stateless nature of the token, the simplest strong improvement is to sign the token using a secure algorithm such as HMAC with a server-side secret, or to encrypt it using a standard cryptographic library such as cryptography.fernet. This way, even if the token is leaked, it cannot be tampered with or easily deciphered by an attacker.

The fix will be to:

  • Encrypt the api_token before placing it in the cookie.
  • Decrypt it upon use (not shown in the provided code, so just encryption before storage for now).

Necessary changes:

  • Import Fernet from the cryptography.fernet library and initialize it with a server-side key (loaded from a secure location or generated on startup; for demonstration, set via an environment variable).
  • Encrypt api_token before storing it in the cookie.

All edits are to be performed in api/routes/auth.py as shown in the snippet.

Suggested changeset 2
api/routes/auth.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/api/routes/auth.py b/api/routes/auth.py
--- a/api/routes/auth.py
+++ b/api/routes/auth.py
@@ -11,6 +11,7 @@
 from pathlib import Path
 from urllib.parse import urljoin
 
+from cryptography.fernet import Fernet
 from authlib.integrations.starlette_client import OAuth
 
 from fastapi import APIRouter, Request, HTTPException, status
@@ -24,6 +25,14 @@
 from api.extensions import db
 
 
+# --- Sensitive cookie encryption setup ---
+_FERNET_KEY = os.environ.get("API_TOKEN_ENCRYPTION_KEY")
+if _FERNET_KEY is None:
+    # In production, this key should be set securely via environment variables and remain stable
+    _FERNET_KEY = Fernet.generate_key()
+    logging.warning("No API_TOKEN_ENCRYPTION_KEY set! Using generated key; tokens will not persist across restarts.")
+_fernet = Fernet(_FERNET_KEY)
+
 # Router
 auth_router = APIRouter(tags=["Authentication"])
 TEMPLATES_DIR = str((Path(__file__).resolve().parents[1] / "../app/templates").resolve())
@@ -349,9 +358,11 @@
                 await handler('email', user_data, api_token)
                 response = JSONResponse({"success": True}, status_code=200)
                 
+                # Encrypt the api_token before storing in the cookie
+                encrypted_token = _fernet.encrypt(api_token.encode()).decode()
                 response.set_cookie(
                     key="api_token",
-                    value=api_token,
+                    value=encrypted_token,
                     httponly=True,
                     secure=_is_request_secure(request)
                 )
EOF
@@ -11,6 +11,7 @@
from pathlib import Path
from urllib.parse import urljoin

from cryptography.fernet import Fernet
from authlib.integrations.starlette_client import OAuth

from fastapi import APIRouter, Request, HTTPException, status
@@ -24,6 +25,14 @@
from api.extensions import db


# --- Sensitive cookie encryption setup ---
_FERNET_KEY = os.environ.get("API_TOKEN_ENCRYPTION_KEY")
if _FERNET_KEY is None:
# In production, this key should be set securely via environment variables and remain stable
_FERNET_KEY = Fernet.generate_key()
logging.warning("No API_TOKEN_ENCRYPTION_KEY set! Using generated key; tokens will not persist across restarts.")
_fernet = Fernet(_FERNET_KEY)

# Router
auth_router = APIRouter(tags=["Authentication"])
TEMPLATES_DIR = str((Path(__file__).resolve().parents[1] / "../app/templates").resolve())
@@ -349,9 +358,11 @@
await handler('email', user_data, api_token)
response = JSONResponse({"success": True}, status_code=200)

# Encrypt the api_token before storing in the cookie
encrypted_token = _fernet.encrypt(api_token.encode()).decode()
response.set_cookie(
key="api_token",
value=api_token,
value=encrypted_token,
httponly=True,
secure=_is_request_secure(request)
)
Pipfile
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/Pipfile b/Pipfile
--- a/Pipfile
+++ b/Pipfile
@@ -1,4 +1,7 @@
 [[source]]
+[packages]
+
+cryptography = "^45.0.7"
 url = "https://pypi.org/simple"
 verify_ssl = true
 name = "pypi"
EOF
@@ -1,4 +1,7 @@
[[source]]
[packages]

cryptography = "^45.0.7"
url = "https://pypi.org/simple"
verify_ssl = true
name = "pypi"
This fix introduces these dependencies
Package Version Security advisories
cryptography (pypi) 45.0.7 None
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
Copy link
Copy Markdown
Contributor Author

@gkorland gkorland Sep 7, 2025

Choose a reason for hiding this comment

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

but we're using secure cookie, why do we need to encrypt it twice?

httponly=True,
secure=_is_request_secure(request)
)
return response

return JSONResponse(
{"success": False, "error": "Authentication failed"},
status_code=status.HTTP_401_UNAUTHORIZED
)
except Exception as e:
logging.error("Login error: %s", e)
return JSONResponse(
{"success": False, "error": "Login failed"},
{"success": False, "error": "Login failed"},
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR
)

Expand Down
8 changes: 8 additions & 0 deletions app/public/css/modals.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,14 @@
display: block;
}

.user-profile-initial {
display: flex;
align-items: center;
justify-content: center;
background-color: var(--falkor-quaternary);
color: var(--text-primary);
font-weight: bold;
}
.user-profile-info {
padding: 15px;
border-bottom: 1px solid var(--falkor-quaternary);
Expand Down
6 changes: 5 additions & 1 deletion app/templates/components/user_profile.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
{# User profile button and dropdown #}
<button id="user-profile-btn" class="user-profile-btn" title="{{ user_info.name }}"
aria-haspopup="true" aria-expanded="false" aria-controls="user-profile-dropdown">
<img src="{{ user_info.picture }}" alt="{{ user_info.name[0] | upper }}" class="user-profile-img">
{% if user_info.picture %}
<img src="{{ user_info.picture }}" alt="{{ user_info.name[0] | upper }}" class="user-profile-img">
{% else %}
<div class="user-profile-img user-profile-initial">{{ user_info.name[0] | upper }}</div>
{% endif %}
Comment on lines +4 to +8
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Sep 7, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Guard against empty/missing name; improve alt text.

Indexing user_info.name[0] will error if name is empty/None. Also set alt to the full name (or email) for accessibility.

-    {% if user_info.picture %}
-        <img src="{{ user_info.picture }}" alt="{{ user_info.name[0] | upper }}" class="user-profile-img">
-    {% else %}
-        <div class="user-profile-img user-profile-initial">{{ user_info.name[0] | upper }}</div>
-    {% endif %}
+    {% set display_char = (user_info.name or user_info.email or '?')[:1] | upper %}
+    {% set alt_text = (user_info.name or user_info.email or 'User') %}
+    {% if user_info.picture %}
+        <img src="{{ user_info.picture }}" alt="{{ alt_text }}" class="user-profile-img">
+    {% else %}
+        <div class="user-profile-img user-profile-initial">{{ display_char }}</div>
+    {% endif %}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{% if user_info.picture %}
<img src="{{ user_info.picture }}" alt="{{ user_info.name[0] | upper }}" class="user-profile-img">
{% else %}
<div class="user-profile-img user-profile-initial">{{ user_info.name[0] | upper }}</div>
{% endif %}
{% set display_char = (user_info.name or user_info.email or '?')[:1] | upper %}
{% set alt_text = (user_info.name or user_info.email or 'User') %}
{% if user_info.picture %}
<img src="{{ user_info.picture }}" alt="{{ alt_text }}" class="user-profile-img">
{% else %}
<div class="user-profile-img user-profile-initial">{{ display_char }}</div>
{% endif %}
🤖 Prompt for AI Agents 
In app/templates/components/user_profile.j2 around lines 4 to 8, the template
indexes user_info.name[0] which will raise if name is missing or empty and the
alt text uses only the first initial; update the template to first guard the
name (e.g., check that user_info.name is defined and non-empty) before accessing
[0], and for alt text use the full display name or fallback to user_info.email
or a localized default string; also ensure the initials fallback uses the first
character of that safe display name so no direct indexing of a possibly
None/empty value occurs.

</button>
<div id="user-profile-dropdown" class="user-profile-dropdown" role="menu" aria-labelledby="user-profile-btn">
<div class="user-profile-info">
Expand Down
12 changes: 2 additions & 10 deletions app/ts/modules/modals.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,16 +172,8 @@ async function handleEmailSignup(e: Event) {
const data = await response.json();

if (data.success) {
alert('Account created successfully! Please sign in.');
// Switch to login modal
const signupModal = document.getElementById('signup-modal') as HTMLElement | null;
const loginModal = document.getElementById('login-modal') as HTMLElement | null;
if (signupModal && loginModal) {
signupModal.style.display = 'none';
loginModal.style.display = 'flex';
}
// Clear form
form.reset();
// Reload page to update authentication state
window.location.reload();
} else {
alert(data.error || 'Signup failed. Please try again.');
}
Expand Down
2 changes: 1 addition & 1 deletion start.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ FALKORDB_HOST="${FALKORDB_HOST:-localhost}"
FALKORDB_PORT="${FALKORDB_PORT:-6379}"

# Start FalkorDB Redis server in background
redis-server --loadmodule /var/lib/falkordb/bin/falkordb.so &
redis-server --loadmodule /var/lib/falkordb/bin/falkordb.so | cat &
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Sep 7, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Pipeline hides Redis startup failures; remove pipe or enable pipefail.

set -e won’t catch failures from the first command in a pipeline. If redis-server fails, cat still exits 0 and the script continues.

-set -e
+set -e
+set -o pipefail
@@
-redis-server --loadmodule /var/lib/falkordb/bin/falkordb.so | cat &
+redis-server --loadmodule /var/lib/falkordb/bin/falkordb.so &

If you need to tee logs, prefer >> /var/log/redis.log 2>&1 & over a pipe.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
redis-server --loadmodule /var/lib/falkordb/bin/falkordb.so | cat &
set -e
set -o pipefail
redis-server --loadmodule /var/lib/falkordb/bin/falkordb.so &
🤖 Prompt for AI Agents 
In start.sh around line 10, the redis-server startup is piped into cat which
masks failures; remove the pipe or enable pipefail and explicitly check the
command status. Replace the piped invocation with a direct background start that
appends both stdout and stderr to /var/log/redis.log (so redis-server runs in
background and logs to that file), or add "set -o pipefail" near the top and
capture the pipeline exit status after starting to fail the script on
redis-server errors.


# Wait until FalkorDB is ready
echo "Waiting for FalkorDB to start on $FALKORDB_HOST:$FALKORDB_PORT..."
Expand Down