Conversation
Fix email login and cookie store with token
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| response.set_cookie( | ||
| key="api_token", | ||
| value=api_token, |
Check failure
Code scanning / CodeQL
Clear-text storage of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 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_tokenbefore 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
Fernetfrom thecryptography.fernetlibrary 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_tokenbefore storing it in the cookie.
All edits are to be performed in api/routes/auth.py as shown in the snippet.
| @@ -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) | ||
| ) |
| @@ -1,4 +1,7 @@ | ||
| [[source]] | ||
| [packages] | ||
|
|
||
| cryptography = "^45.0.7" | ||
| url = "https://pypi.org/simple" | ||
| verify_ssl = true | ||
| name = "pypi" |
| Package | Version | Security advisories |
| cryptography (pypi) | 45.0.7 | None |
There was a problem hiding this comment.
but we're using secure cookie, why do we need to encrypt it twice?
WalkthroughRefactors auth flows in api/routes/auth.py to use proxy-aware HTTPS detection for cookie security, replace exceptions with JSON responses, and route post-auth via a callback handler issuing api_token cookies. Adds avatar-initial fallback in the user profile template with supporting CSS. Changes signup modal success behavior to full page reload. Adjusts start.sh Redis launch piping. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant A as Auth Route
participant H as Callback Handler
participant S as Cookie
rect rgb(232, 244, 253)
note over C,A: Email signup/login
C->>A: POST /auth/signup|login (email, password)
A->>A: Validate inputs, check _is_email_auth_enabled()
alt valid credentials
A->>H: persist user_data + request token
H-->>A: api_token
A->>A: _is_request_secure(request)
A->>S: Set cookie api_token (secure by scheme/proxy)
A-->>C: { "success": true }
else invalid / disabled
A-->>C: JSON error (400/401/403/500)
end
end
rect rgb(240, 232, 252)
note over C,A: OAuth callback (Google/GitHub)
C->>A: GET /auth/callback?code=...
A->>A: Exchange code, fetch user_info
alt user_info present
A->>H: persist user_data + provider + request token
H-->>A: api_token
A->>A: _is_request_secure(request)
A->>S: Set cookie api_token (secure by scheme/proxy)
A-->>C: Redirect to home
else missing user_info
A-->>C: 400 JSON/exception
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api/routes/auth.py (3)
133-146: Align identity MERGE with login query; ensure provider is set.
_authenticate_email_usermatchesIdentity {provider: 'email', email: $email}, but_set_mail_hashMERGEs withoutprovider, risking a node that login won't see.- MERGE (i:Identity { - provider_user_id: $email, - email: $email - }) - SET i.password_hash = $password_hash + MERGE (i:Identity { provider: 'email', email: $email }) + SET i.provider_user_id = coalesce(i.provider_user_id, $email), + i.password_hash = $password_hash RETURN iOptional: rename
_set_mail_hashto_set_password_hashfor clarity and update the call site.Also applies to: 148-156
257-279: Signup logic can issue a token for an existing account without verifying password.If the identity already exists (
new_identityis false), the flow still returns success and sets anapi_token. That can create inconsistent state (and a potential auth bypass if the handler stores the token) without proving email ownership.- else: - logging.info("User already exists: %s", _sanitize_for_log(email)) - - logging.info("User registration successful: %s", _sanitize_for_log(email)) - - response = JSONResponse({ - "success": True, - }, status_code=201) + else: + logging.info("User already exists: %s", _sanitize_for_log(email)) + return JSONResponse( + {"success": False, "error": "Email is already registered"}, + status_code=status.HTTP_409_CONFLICT + ) + + logging.info("User registration successful: %s", _sanitize_for_log(email)) + + response = JSONResponse({"success": True}, status_code=201)Follow-up: if you intend “idempotent signup,” require proof of possession (e.g., password check or email verification) before minting a new token.
271-279: Cookie settings: unifysecurehandling and consider explicit SameSite.
- Use
_is_request_secure(request)for OAuth callbacks too.- Being explicit with
samesite="lax"avoids framework-default surprises.response.set_cookie( key="api_token", value=api_token, httponly=True, - secure=_is_request_secure(request) + secure=_is_request_secure(request), + samesite="lax", ) @@ redirect.set_cookie( key="api_token", value=api_token, httponly=True, - secure=True + secure=_is_request_secure(request), + samesite="lax", ) @@ redirect.set_cookie( key="api_token", value=api_token, httponly=True, - secure=True + secure=_is_request_secure(request), + samesite="lax", )Also applies to: 350-358, 488-493, 592-597
♻️ Duplicate comments (1)
api/routes/auth.py (1)
354-354: Re: “clear-text storage of sensitive information” finding.Passwords are PBKDF2-hashed with salt before storage; no clear-text password is persisted. Consider documenting the hashing scheme where CodeQL flagged this to reduce future false positives.
If a separate location stores raw passwords, please point it out; otherwise we can mark the alert as a false positive or tune the query.
🧹 Nitpick comments (2)
app/ts/modules/modals.ts (1)
175-176: Reload-on-signup OK; consider small hardening.
- Optional: use
location.assign('/')to avoid potential cache quirks vsreload().- Optional: make
fetchexplicit about cookies to future-proof:credentials: 'same-origin'.- const response = await fetch('/signup/email', { + const response = await fetch('/signup/email', { method: 'POST', headers: { 'Content-Type': 'application/json', }, + credentials: 'same-origin', body: JSON.stringify({app/public/css/modals.css (1)
268-275: Avatar-initial style: ensure circular shape without relying on other classes.If
.user-profile-imgdoesn't always set radius/size in all contexts, the initial may render as a square..user-profile-initial { display: flex; align-items: center; justify-content: center; background-color: var(--falkor-quaternary); color: var(--text-primary); font-weight: bold; + border-radius: 50%; + text-transform: uppercase; /* defensive; template also uppercases */ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/routes/auth.py(5 hunks)app/public/css/modals.css(1 hunks)app/templates/components/user_profile.j2(1 hunks)app/ts/modules/modals.ts(1 hunks)start.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Frontend source changes in app/**/*.{ts,tsx} must be compiled before production runs
Files:
app/ts/modules/modals.ts
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/routes/auth.py
🪛 Ruff (0.12.2)
api/routes/auth.py
280-280: Consider moving this statement to an else block
(TRY300)
282-282: Do not catch blind exception: Exception
(BLE001)
283-283: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
364-364: Do not catch blind exception: Exception
(BLE001)
365-365: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🪛 GitHub Check: CodeQL
api/routes/auth.py
[failure] 354-354: Clear-text storage of sensitive information
This expression stores sensitive data (password) as clear text.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (1)
app/ts/modules/modals.ts (1)
41-52: Ensure TypeScript build/type-check passes before merging
- Run
npm run buildornpm run typecheckand confirm there are no compiler errors.- Applies to app/ts/modules/modals.ts (lines 41–52, 93–101, 158–166).
| 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" |
There was a problem hiding this comment.
🛠️ 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.
| 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).
| 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 | ||
| ) |
There was a problem hiding this comment.
🛠️ 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.
| {% 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 %} |
There was a problem hiding this comment.
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.
| {% 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.
|
|
||
| # 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 & |
There was a problem hiding this comment.
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.
| 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.
1 participant
Summary by CodeRabbit