Conversation
load env from os.env and not directly from .env
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughModule-level OAuth import added and Jinja2 template loading switched to a custom Environment with file-system bytecode cache (TEMPLATES_CACHE_DIR = "/tmp/jinja_cache"); OAuth Config now loads from environment ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App as FastAPI App
participant Auth as auth.routes
participant OAuthProv as OAuth Provider
participant FS as FileSystem (/tmp/jinja_cache)
Client->>App: HTTP /login or /logout
App->>Auth: route handler
Auth->>FS: ensure TEMPLATES_CACHE_DIR exists (on import)
Auth->>Auth: create Jinja2 Environment with bytecode cache
Auth->>OAuthProv: use OAuth (Config(environ=os.environ)) to build auth client
Auth->>OAuthProv: redirect / exchange tokens
OAuthProv-->>Auth: auth response
Auth-->>Client: render template (via Environment, possibly using cached bytecode)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/routes/auth.py (1)
253-258: Google token revoke request uses query params; must be form-encoded bodyGoogle’s revoke endpoint expects
application/x-www-form-urlencodedwith the token in the request body, not in the query string. Usingparams=can cause silent failures. Add a short timeout as well.Apply this diff:
- resp = await client.post( - "https://oauth2.googleapis.com/revoke", - params={"token": token}, - headers={"content-type": "application/x-www-form-urlencoded"}, - ) + resp = await client.post( + "https://oauth2.googleapis.com/revoke", + data={"token": token}, + headers={"content-type": "application/x-www-form-urlencoded"}, + timeout=5.0, + )
🧹 Nitpick comments (4)
api/routes/auth.py (4)
149-153: Broaden exception handling to cover HTTP/network errors during userinfo exchangeThe current handlers only catch
AuthlibBaseError. Network/HTTP parsing errors from the underlying client can surface ashttpx.HTTPErrororValueError(JSON decode). Recommend handling them similarly to avoid a 500 and ensure session cleanup.Apply this diff:
- except AuthlibBaseError as e: + except (AuthlibBaseError, httpx.HTTPError, ValueError) as e: logging.error("Google OAuth error: %s", e) _clear_auth_session(request.session) return RedirectResponse(url="/", status_code=status.HTTP_302_FOUND)- except AuthlibBaseError as e: + except (AuthlibBaseError, httpx.HTTPError, ValueError) as e: logging.error("GitHub OAuth error: %s", e) _clear_auth_session(request.session) return RedirectResponse(url="/", status_code=status.HTTP_302_FOUND)Also applies to: 223-227
276-278: Use the existing helper to clear auth keysReuse
_clear_auth_sessionfor consistency and to avoid drifting key lists between call sites.Apply this diff:
- # ---- Clear session auth keys ---- - for key in ["user_info", "google_token", "github_token", "token_validated_at"]: - request.session.pop(key, None) + # ---- Clear session auth keys ---- + _clear_auth_session(request.session)
21-23: Minor: simplify templates path computationCurrent path uses
parents[1] / "../app/templates". A more direct construct avoids..segments and is easier to read.Apply this diff:
-TEMPLATES_DIR = str((Path(__file__).resolve().parents[1] / "../app/templates").resolve()) +TEMPLATES_DIR = str((Path(__file__).resolve().parents[2] / "app" / "templates").resolve())
293-314: Registration blocks are fine; consider small hardening knobsBoth providers look correctly registered with appropriate scopes. Optional niceties:
- Google: add
prompt="consent"oraccess_type="offline"inclient_kwargsif you rely on refresh tokens in future.- GitHub: if you only need primary email, consider
read:user user:emailminimal scopes and prefer primary+verified email when selecting.I can stage these as guarded by env flags (e.g.,
OAUTH_GOOGLE_PROMPT,OAUTH_GITHUB_SCOPES) to keep staging flexible. Interested?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
api/routes/auth.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to pylint standards across all Python files (repository uses
make lint)
Files:
api/routes/auth.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-24T17:15:21.325Z
Learning: Applies to api/app_factory.py : Maintain the application factory and OAuth setup in `api/app_factory.py` (core configuration in the top of the file)
📚 Learning: 2025-08-24T17:15:21.325Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-24T17:15:21.325Z
Learning: Applies to api/app_factory.py : Maintain the application factory and OAuth setup in `api/app_factory.py` (core configuration in the top of the file)
Applied to files:
api/routes/auth.py
🧬 Code graph analysis (1)
api/routes/auth.py (1)
api/config.py (1)
Config(47-137)
🔇 Additional comments (4)
api/routes/auth.py (4)
14-16: Module-level OAuth import is appropriate and preferableMoving
OAuthto a module-level import reduces cold-path latency and avoids repeated imports withininit_auth. No issues spotted.
285-287: Switch to Config(environ=os.environ) is container-friendly and correctConstructing
Configfrom the process environment is a good move for staging/production. Passing that intoOAuth(config)remains valid.
60-67: Callback URL builder looks robust; handles base override and slashes correctlyGood defensive handling of
OAUTH_BASE_URLvs.request.base_urland trailing slash normalization. Consider adding unit tests to lock this behavior (with and without override, and with localhost vs 127.0.0.1).If helpful, I can draft pytest cases that parametrize base URL permutations to ensure stable joins. Want me to add them?
281-317: OAuth initialization hook invocation confirmedI’ve verified that
init_auth(app)is indeed called in api/app_factory.py at line 79, ensuring the OAuth setup runs at startup andrequest.app.state.oauthis populated before handling requests. No further changes are needed here.
add tempalte caching
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
api/routes/auth.py (4)
15-15: Import select_autoescape to support safe Jinja2 configuration.-from jinja2 import Environment, FileSystemLoader, FileSystemBytecodeCache +from jinja2 import Environment, FileSystemLoader, FileSystemBytecodeCache, select_autoescape
24-26: Anchor the Jinja bytecode cache directory to the repo/app root instead of CWD.Relative "./.jinja_cache" depends on process working directory. Use an absolute path anchored via file to avoid surprises in prod/uvicorn reloads.
-TEMPLATES_CACHE_DIR = "./.jinja_cache" -os.makedirs(TEMPLATES_CACHE_DIR, exist_ok=True) # ✅ ensures the folder exists +TEMPLATES_CACHE_DIR = str((Path(__file__).resolve().parents[2] / ".jinja_cache")) +os.makedirs(TEMPLATES_CACHE_DIR, exist_ok=True)
300-301: Preserve local.envdev ergonomics while preferring environment variables.Switching from
Config(".env")toConfig(environ=os.environ)can break local dev for teams relying on a.env. Prefer a fallback to.envif present.- config = Config(environ=os.environ) + env_file = Path(".env") + config = Config(str(env_file)) if env_file.exists() else Config(environ=os.environ)To verify, ensure init_auth still works when only a
.envis present (no exported env vars).
267-273: Send revoke token in the request body to match the declared content type.You set
content-type: application/x-www-form-urlencodedbut send the token viaparams. Some providers require the token in the body; align header and payload to reduce flakiness.- resp = await client.post( - "https://oauth2.googleapis.com/revoke", - params={"token": token}, - headers={"content-type": "application/x-www-form-urlencoded"}, - ) + resp = await client.post( + "https://oauth2.googleapis.com/revoke", + data={"token": token}, + headers={"content-type": "application/x-www-form-urlencoded"}, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.gitignore(1 hunks)api/routes/auth.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to pylint standards across all Python files (repository uses
make lint)
Files:
api/routes/auth.py
🧠 Learnings (1)
📚 Learning: 2025-08-24T17:15:21.325Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-24T17:15:21.325Z
Learning: Applies to api/app_factory.py : Maintain the application factory and OAuth setup in `api/app_factory.py` (core configuration in the top of the file)
Applied to files:
api/routes/auth.py
🧬 Code graph analysis (1)
api/routes/auth.py (1)
api/auth/user_management.py (1)
validate_and_cache_user(162-278)
🪛 GitHub Check: CodeQL
api/routes/auth.py
[warning] 28-35: Jinja2 templating with autoescape=False
Using jinja2 templates with autoescape=False can potentially allow XSS attacks.
⏰ 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). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (2)
api/routes/auth.py (2)
14-14: LGTM: Moving OAuth import to module scope is fine.This avoids per-call imports and keeps init_auth lean. No issues noted.
296-331: All checks passed: Authentication hook and session middleware are correctly configured
- Verified that
init_auth(app)is invoked in api/app_factory.py at line 79, wiring the OAuth setup into the app.- Confirmed that
SessionMiddlewareis imported (line 11) and added (lines 60–65) withsecret_keysourced fromFASTAPI_SECRET_KEY(or auto-generated if unset) before any routes are mounted.No further action required.
add tempalte caching
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/routes/auth.py (1)
24-37: Autoescape enabled + Jinja bytecode cache: good security/perf improvements.Enabling
select_autoescapecloses the CodeQL warning and mitigates XSS risks; the bytecode cache reduces template compilation cost.
🧹 Nitpick comments (2)
api/routes/auth.py (2)
24-37: Harden cache location, portability, and autoescape defaults.
- Make the cache dir configurable and portable (Windows/containers) and restrict permissions.
- Consider
default_for_string=Trueso futurefrom_stringrendering stays auto-escaped.- Make
auto_reloadconfigurable to avoid perf hit in prod.Apply this diff:
-TEMPLATES_CACHE_DIR = "/tmp/jinja_cache" -os.makedirs(TEMPLATES_CACHE_DIR, exist_ok=True) # ✅ ensures the folder exists +TEMPLATES_CACHE_DIR = os.getenv( + "JINJA_CACHE_DIR", + os.path.join(tempfile.gettempdir(), "jinja_cache") +) +os.makedirs(TEMPLATES_CACHE_DIR, mode=0o700, exist_ok=True) @@ - auto_reload=True, - autoescape=select_autoescape(['html', 'xml', 'j2']) + auto_reload=os.getenv("JINJA_AUTO_RELOAD", "").lower() in ("1", "true", "yes"), + autoescape=select_autoescape( + enabled_extensions=("html", "htm", "xml", "j2", "jinja2"), + default_for_string=True + )Add this import outside the selected range:
import tempfile
300-303: Dev-friendly .env fallback & init_auth wiring confirmed
- Verified that
init_auth(app)is already called inapi/app_factory.py(line 79), so no changes needed there.- To improve local development parity, update
api/routes/auth.pyto fall back to a.envfile when present:--- a/api/routes/auth.py +++ b/api/routes/auth.py @@ init_auth(app): - config = Config(environ=os.environ) + from pathlib import Path + + dotenv_path = Path(".env") + config = ( + Config(str(dotenv_path)) + if dotenv_path.exists() + else Config(environ=os.environ) + )
- (Optional) For consistency, consider reading OAuth secrets via
config("OAUTH_CLIENT_ID")instead ofos.getenv("OAUTH_CLIENT_ID").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
api/routes/auth.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to pylint standards across all Python files (repository uses
make lint)
Files:
api/routes/auth.py
🧠 Learnings (1)
📚 Learning: 2025-08-24T17:15:21.325Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-24T17:15:21.325Z
Learning: Applies to api/app_factory.py : Maintain the application factory and OAuth setup in `api/app_factory.py` (core configuration in the top of the file)
Applied to files:
api/routes/auth.py
🧬 Code graph analysis (1)
api/routes/auth.py (1)
api/auth/user_management.py (1)
validate_and_cache_user(162-278)
⏰ 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). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (1)
api/routes/auth.py (1)
14-15: Authlib dependency confirmed; module‐level OAuth import is safeVerified that
authlibis declared under[packages]in your Pipfile (~=1.4.0) and locked to==1.4.1. With the dependency guaranteed at runtime, keeping thefrom authlib.integrations.starlette_client import OAuthat module scope is acceptable—no import-time failures will occur in supported environments.
- Bump litellm ~=1.82.0 → ~=1.83.0 (fixes CVE: auth bypass via OIDC cache key collision, privilege escalation via unrestricted proxy config) - Bump fastmcp >=2.13.1 → >=3.2.0 (fixes CVE: SSRF & path traversal in OpenAPI provider) - Add aiohttp>=3.13.4 constraint (fixes multiple HTTP-level CVEs: duplicate Host headers, header injection, multipart bypass, memory DoS, CRLF injection, cookie leaks, SSRF on Windows) - Add npm override for lodash-es to 4.18.1 (fixes prototype pollution via array path bypass, code injection via template imports) - Update fastmcp import path from deprecated fastmcp.server.openapi to fastmcp.server.providers.openapi Closes #125, #124, #123, #122, #121, #120, #119, #118, #117, #116, #115, #114, #113, #112, #111, #110, #109, #108, #107, #106, #105, #104, #103, #102, #101, #100, #99, #98, #97, #96 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2 participants
Summary by CodeRabbit
Refactor
Chores