Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReimplemented left-toolbar behavior by moving inline template script into a new TypeScript module and wiring it from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as app/ts/app.ts
participant Module as app/ts/modules/left_toolbar.ts
participant DOM as Document
participant MQ as MediaQueryList (min-width:768px)
participant Win as window
App->>Module: initLeftToolbar()
Module->>DOM: querySelector #left-toolbar, #burger-toggle-btn
alt elements missing
Module-->>App: no-op
else
Module->>MQ: evaluate (min-width:768px)
MQ-->>Module: initial match result
Module->>DOM: setOpen(initial)
Module->>MQ: subscribe to change events
Module->>DOM: bind pointerdown & click handlers
Module->>Win: set __leftToolbar.{open,close,toggle}
Module-->>App: initialized
end
sequenceDiagram
autonumber
participant Client as Browser
participant Routes as api/routes/auth.py
participant Handlers as app.state.callback_handler
participant DB as Organizations graph (neo4j)
participant Auth as api/auth/user_management.py
Client->>Routes: OAuth provider callback
Routes->>Handlers: persist user_data (id,email,name,picture)
Handlers->>Auth: ensure_user_in_organizations(..., api_token)
Auth->>DB: merge Identity & Token (expires_at=24h) & link HAS_TOKEN
Auth-->>Routes: success
Routes->>Client: set cookie api_token (HttpOnly, Secure) + redirect "/"
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
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.OpenSSF Scorecard
Scanned Files
|
![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
🧹 Nitpick comments (6)
app/ts/modules/left_toolbar.ts (6)
29-42: Guard for missing window.matchMedia and register listeners conditionally.On older/embedded browsers,
window.matchMediamay be undefined. Also, event listener registration should be skipped whenmqis null. Suggest the following refactor.- const mq = window.matchMedia('(min-width: 768px)'); - - try { - setOpen(mq.matches); - } catch (e) { - setOpen(true); - } - - // Support both modern and legacy addListener APIs - if (typeof (mq as MediaQueryList).addEventListener === 'function') { - (mq as MediaQueryList).addEventListener('change', (ev: MediaQueryListEvent) => setOpen(ev.matches)); - } else if (typeof (mq as any).addListener === 'function') { - (mq as any).addListener((ev: MediaQueryListEvent) => setOpen(ev.matches)); - } + const mq: MediaQueryList | null = typeof window.matchMedia === 'function' + ? window.matchMedia('(min-width: 768px)') + : null; + + if (mq) { + setOpen(mq.matches); + // Support both modern and legacy listener APIs + if (typeof mq.addEventListener === 'function') { + mq.addEventListener('change', (ev: MediaQueryListEvent) => setOpen(ev.matches)); + } else if (typeof (mq as any).addListener === 'function') { + (mq as any).addListener((ev: MediaQueryListEvent) => setOpen(ev.matches)); + } + } else { + // Fallback default + setOpen(true); + }
44-63: Prefer a single click handler; avoid pointerdown to preserve focus/drag cancel and simplify logic.Toggling on
pointerdowncan prevent focus, activate on canceled drags, and requires the ignore flag. A singleclickhandler is simpler and more accessible.- let ignoreNextClick = false; - function handleToggleEvent() { const isCollapsed = nav!.classList.contains('collapsed'); setOpen(isCollapsed); } - - btn.addEventListener('pointerdown', function (e: PointerEvent) { - e.preventDefault(); - handleToggleEvent(); - ignoreNextClick = true; - }); - - btn.addEventListener('click', function (_e: MouseEvent) { - if (ignoreNextClick) { - ignoreNextClick = false; - return; - } - handleToggleEvent(); - }); + btn.addEventListener('click', function (e: MouseEvent) { + e.preventDefault(); + handleToggleEvent(); + });If
#burger-toggle-btnis not a real , add a key handler for Space/Enter; otherwise native behavior is sufficient.
10-27: A11y: hide collapsed nav from tab order and screen readers (inert).When collapsed, the nav remains focusable by tabbing unless removed from the accessibility tree. Suggest toggling
inert.if (open) { nav!.classList.remove('collapsed'); + nav!.removeAttribute('inert'); document.body.classList.add('left-toolbar-open'); @@ } else { nav!.classList.add('collapsed'); + nav!.setAttribute('inert', ''); document.body.classList.remove('left-toolbar-open');
6-8: A11y: associate the toggle with its controlled region.Set
aria-controls="left-toolbar"on the toggle button once during init.- if (!nav || !btn) return; + if (!nav || !btn) return; + // Link toggle to controlled nav for assistive tech + btn!.setAttribute('aria-controls', 'left-toolbar');
5-8: Make initialization idempotent to avoid duplicate listeners on re-entry.Hot reloads or multiple calls could register listeners twice. Add a module-scoped guard.
-export function initLeftToolbar(): void { +let __leftToolbarInitialized = false; +export function initLeftToolbar(): void { + if (__leftToolbarInitialized) return; const nav = document.getElementById('left-toolbar') as HTMLElement | null; const btn = document.getElementById('burger-toggle-btn') as HTMLElement | null; - if (!nav || !btn) return; + if (!nav || !btn) return; + __leftToolbarInitialized = true;
66-70: Don’t clobber an existing global and freeze the API object.Avoid overwriting if something else already attached to
window.__leftToolbar. Freezing helps keep the API surface immutable.- (window as any).__leftToolbar = { - open: () => setOpen(true), - close: () => setOpen(false), - toggle: () => setOpen(!nav.classList.contains('collapsed')), - }; + const api = Object.freeze({ + open: () => setOpen(true), + close: () => setOpen(false), + toggle: () => setOpen(!nav!.classList.contains('collapsed')), + }); + (window as any).__leftToolbar = (window as any).__leftToolbar ?? api;Optional: add a TS global type to avoid
any. Createapp/ts/types/global.d.ts:export {}; declare global { interface Window { __leftToolbar?: { open: () => void; close: () => void; toggle: () => void; }; } }
📜 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 (3)
app/templates/components/left_toolbar.j2(0 hunks)app/ts/app.ts(2 hunks)app/ts/modules/left_toolbar.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/templates/components/left_toolbar.j2
🧰 Additional context used
📓 Path-based instructions (1)
app/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the TypeScript frontend sources in
app/and build them before production runs
Files:
app/ts/app.tsapp/ts/modules/left_toolbar.ts
🧬 Code graph analysis (1)
app/ts/app.ts (1)
app/ts/modules/left_toolbar.ts (1)
initLeftToolbar(5-71)
⏰ 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). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (4)
app/ts/modules/left_toolbar.ts (2)
5-8: Good extraction and defensive DOM lookup.Early return if required nodes are missing keeps the module resilient and SSR-safe since DOM access happens only inside the init function.
73-74: Explicit non-auto-init is a good call.Keeps startup order clear and bundling deterministic. The import-and-init in app.ts aligns with this.
app/ts/app.ts (2)
22-22: Import wiring looks correct.Relative path resolves within app/ts/modules; no public surface changes here.
106-107: No duplicate toolbar bindings or legacy listeners detectedVerified that:
initLeftToolbar()is only invoked once, in app/ts/app.ts (line 107).- There are no other call sites for
initLeftToolbar()elsewhere in the codebase.- The
#burger-toggle-btnelement is only referenced in:
- app/ts/modules/left_toolbar.ts
- app/templates/components/left_toolbar.j2
- app/public/css/buttons.css
- No remnants of the old global API (
__leftToolbar) exist outside the new module.All checks pass—no duplicate listeners or legacy bindings remain.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/templates/base.j2 (1)
37-37: Micro perf: consider lazy-loading the footer logo.Footer assets are typically below-the-fold. Adding
loading="lazy"can defer the request with no UX downside.Apply this diff:
- <img src="{{ url_for('static', path='icons/logo.svg') }}" alt="FalkorDB Logo"/> + <img src="{{ url_for('static', path='icons/logo.svg') }}" alt="FalkorDB Logo" loading="lazy"/>app/templates/components/toolbar.j2 (1)
4-12: Accessibility polish: add an explicit label and hide decorative SVGs from AT.
- Provide an accessible name via
aria-labelon the link (title isn’t consistently announced).- Mark the two SVGs as decorative with
aria-hidden="true".- If the star count updates dynamically, consider
aria-live="polite"on the span.Proposed diffs:
-<a href="https://github.com/FalkorDB/QueryWeaver" target="_blank" rel="noopener" class="github-link" id="github-link-btn" title="View QueryWeaver on GitHub"> +<a href="https://github.com/FalkorDB/QueryWeaver" target="_blank" rel="noopener" class="github-link" id="github-link-btn" title="View QueryWeaver on GitHub" aria-label="View QueryWeaver on GitHub">- <svg width="16" height="16" viewBox="0 0 24 24" fill="currentColor" xmlns="http://www.w3.org/2000/svg"> + <svg width="16" height="16" viewBox="0 0 24 24" fill="currentColor" aria-hidden="true" xmlns="http://www.w3.org/2000/svg">- <svg width="12" height="12" viewBox="0 0 24 24" fill="currentColor" xmlns="http://www.w3.org/2000/svg"> + <svg width="12" height="12" viewBox="0 0 24 24" fill="currentColor" aria-hidden="true" xmlns="http://www.w3.org/2000/svg">- <span id="github-stars">-</span> + <span id="github-stars" aria-live="polite">-</span>
📜 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 (5)
app/public/css/base.css(1 hunks)app/public/css/buttons.css(1 hunks)app/templates/base.j2(1 hunks)app/templates/components/login_modal.j2(1 hunks)app/templates/components/toolbar.j2(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- app/public/css/base.css
- app/templates/components/login_modal.j2
- app/public/css/buttons.css
🧰 Additional context used
📓 Path-based instructions (1)
app/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the TypeScript frontend sources in
app/and build them before production runs
Files:
app/templates/components/toolbar.j2app/templates/base.j2
⏰ 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)
app/templates/base.j2 (1)
37-37: Inline style removal from footer logo is a solid move.Good separation of concerns by shifting presentation to CSS. Alt text is present and the external link already uses rel="noopener". Looks good.
| <footer id="powered-by-falkordb"> | ||
| Powered by <a href="https://falkordb.com" target="_blank" rel="noopener"> | ||
| <img src="{{ url_for('static', path='icons/logo.svg') }}" alt="FalkorDB Logo" style="height:1.5em;vertical-align:middle;" /> | ||
| <img src="{{ url_for('static', path='icons/logo.svg') }}" alt="FalkorDB Logo"/> | ||
| </a> | ||
| </footer> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm the new CSS rule is actually loaded (main.css vs. base.css).
The template links only to /static/css/main.css. Since the PR adds the rule to base.css, please verify that main.css bundles/imports the footer image rule (e.g., #powered-by-falkordb img { height: 1.5em; vertical-align: middle; }) to avoid a visual regression.
Run this script to check that the rule is present in your built or source CSS:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching CSS sources for #powered-by-falkordb img rule…"
rg -n -C1 '#powered-by-falkordb\s*img' --glob 'app/**/css/**' || echo "Not found in source CSS."
echo
echo "If a built main.css exists, searching it as well…"
fd -tf main.css | while read -r css; do
echo "→ Checking $css"
rg -n -C2 '#powered-by-falkordb\s*img' "$css" || echo "Rule not found in $css"
doneLength of output: 731
Include the footer CSS rule in the served stylesheet
The CSS rule
#powered-by-falkordb img {
height: 1.5em;
vertical-align: middle;
}is defined in app/public/css/base.css (lines 66–67) but is not present in app/public/css/main.css—the only stylesheet linked by app/templates/base.j2. As a result, the footer logo sizing won’t be applied and you’ll see a visual regression. Please address this by:
- Either adding a
<link href="{{ url_for('static', path='css/base.css') }}" rel="stylesheet">inapp/templates/base.j2alongside the existingmain.csslink - Or moving the above rule into your
main.cssand updating your build pipeline so that it’s bundled there
After making the change, verify that app/public/css/main.css contains the #powered-by-falkordb img rule.
🤖 Prompt for AI Agents
In app/templates/base.j2 around lines 35 to 39 the footer image sizing rule
(#powered-by-falkordb img { height: 1.5em; vertical-align: middle; }) is coming
from app/public/css/base.css but base.j2 only links main.css, so the rule isn’t
applied; fix by either adding a stylesheet link for base.css next to the
existing main.css link in app/templates/base.j2, or move the rule into your
source stylesheet(s) that are bundled into app/public/css/main.css and update
the build pipeline so main.css contains the rule; after making the change,
rebuild if necessary and verify app/public/css/main.css includes the
#powered-by-falkordb img rule.
| <svg width="12" height="12" viewBox="0 0 24 24" fill="currentColor" xmlns="http://www.w3.org/2000/svg"> | ||
| <path d="M12 2l3.09 6.26L22 9.27l-5 4.87 1.18 6.88L12 17.77l-6.18 3.25L7 14.14 2 9.27l6.91-1.01L12 2z"/> | ||
| </svg> | ||
| <span id="github-stars" style="margin-left: 2px; font-size: 12px;">-</span> | ||
| <span id="github-stars">-</span> | ||
| </a> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Removing inline styles from the star icon and count is good; ensure CSS replaces the spacing/typography.
Spacing for the 12×12 star and the #github-stars text now relies on CSS. Please confirm the stylesheet adds the equivalent margins and font-size so the layout doesn’t compress.
Run this to verify the CSS defines spacing for .github-link svg and #github-stars:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Looking for toolbar CSS rules…"
rg -n -C2 '(#github-stars|\.github-link)' --glob 'app/**/css/**'
echo
echo "Specifically checking for margin/font-size near #github-stars…"
rg -n -C3 '#github-stars' --glob 'app/**/css/**' | sed -n '1,160p'Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching in app directory for CSS/SCSS/LESS rules for .github-link and #github-stars…"
rg -n -C2 '(\.github-link|#github-stars)' --glob 'app/**/*.{css,scss,sass,less}'
echo
echo "Searching in static directory for CSS/SCSS/LESS rules for .github-link and #github-stars…"
rg -n -C2 '(\.github-link|#github-stars)' --glob 'static/**/*.{css,scss,sass,less}'
echo
echo "Searching repository-wide for any stylesheet definitions…"
rg -n -C2 '(\.github-link|#github-stars)' --glob '*.{css,scss,sass,less}'Length of output: 285
Missing CSS for the GitHub star icon and count
Ripgrep across all CSS/SCSS/SASS/LESS files did not find any rules targeting .github-link, its child svg, or #github-stars. Without inline styles, the star and count will collapse together. Please add explicit spacing and typography rules to the toolbar stylesheet:
• Confirm or create a CSS file for toolbar components (e.g. app/static/css/toolbar.css).
• Add rules such as:
.github-link {
display: inline-flex;
align-items: center;
}
.github-link svg {
width: 12px;
height: 12px;
margin-right: 4px; /* adjust for desired gap */
fill: currentColor;
}
#github-stars {
font-size: 0.875rem; /* ensure consistent typography */
margin-left: 4px; /* match the SVG gap */
}• Ensure this stylesheet is imported into the base template so the toolbar’s layout remains intact.
• After adding, re-run your ripgrep commands to verify the selectors are present.
🤖 Prompt for AI Agents
In app/templates/components/toolbar.j2 around lines 8 to 12, the GitHub star
icon and count lack CSS rules so they collapse together; create or update the
toolbar stylesheet (e.g. app/static/css/toolbar.css) to add rules for
.github-link (display:inline-flex; align-items:center), .github-link svg
(explicit width/height, margin-right and fill:currentColor) and #github-stars
(font-size and margin-left) and then ensure that stylesheet is included in the
base template so the toolbar loads the CSS; after adding, run your ripgrep check
to confirm the selectors are present.
Bumps [eslint](https://github.com/eslint/eslint) from 9.33.0 to 9.34.0. - [Release notes](https://github.com/eslint/eslint/releases) - [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md) - [Commits](eslint/eslint@v9.33.0...v9.34.0) --- updated-dependencies: - dependency-name: eslint dependency-version: 9.34.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Move to login with token and not session cookie
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| redirect = RedirectResponse(url="/", status_code=302) | ||
| redirect.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 8 months ago
To address the issue, we should avoid storing the clear-text api_token value in the cookie. Instead, we should store an encrypted form of api_token if it must be in a cookie, or better yet, store only a session identifier that maps to the token on the server side. Since we are limited to only using the shown code, the best solution is to encrypt the api_token before storing it in the cookie.
To implement this, we can use the cryptography library (a well-known external library) to encrypt the token using a server-side key. We should:
- Import the relevant cryptography methods (
Fernet, etc.). - Initialize the key (for demonstration, from an environment variable or constant—should be secure in real implementation).
- When setting the cookie, encrypt the
api_token, then store the encrypted value as the cookie. - When reading the token elsewhere, remember to decrypt it (not shown here since we only edit the provided code).
The changes to make:
- Add imports for the cryptography library.
- Define a Fernet key (from env or constant).
- Encrypt
api_tokenbefore setting it as a cookie. - Ensure only the encrypted token value is placed in the cookie.
| @@ -5,6 +5,7 @@ | ||
| import secrets | ||
| from pathlib import Path | ||
| from urllib.parse import urljoin | ||
| from cryptography.fernet import Fernet | ||
|
|
||
| from fastapi import APIRouter, Request, HTTPException, status | ||
| from fastapi.responses import RedirectResponse, HTMLResponse | ||
| @@ -36,7 +37,6 @@ | ||
|
|
||
| templates.env.globals["google_tag_manager_id"] = os.getenv("GOOGLE_TAG_MANAGER_ID") | ||
|
|
||
| # ---- Helpers ---- | ||
| def _get_provider_client(request: Request, provider: str): | ||
| """Get an OAuth provider client from app.state.oauth""" | ||
| oauth = getattr(request.app.state, "oauth", None) | ||
| @@ -167,9 +167,11 @@ | ||
| await handler('google', user_data, api_token) | ||
|
|
||
| redirect = RedirectResponse(url="/", status_code=302) | ||
| # Encrypt the api_token before setting as cookie | ||
| encrypted_token = fernet.encrypt(api_token.encode()).decode() | ||
| redirect.set_cookie( | ||
| key="api_token", | ||
| value=api_token, | ||
| value=encrypted_token, | ||
| httponly=True, | ||
| secure=True | ||
| ) |
| @@ -1,4 +1,7 @@ | ||
| [[source]] | ||
| [packages] | ||
|
|
||
| cryptography = "^45.0.6" | ||
| url = "https://pypi.org/simple" | ||
| verify_ssl = true | ||
| name = "pypi" |
| Package | Version | Security advisories |
| cryptography (pypi) | 45.0.6 | None |
| redirect = RedirectResponse(url="/", status_code=302) | ||
| redirect.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 8 months ago
To fix this problem, the authentication token (api_token) should not be stored directly as clear text in the user's cookie. Instead, implement one of two approaches:
- Preferably, store a random identifier in the cookie (e.g., a session ID) that maps to the token in a server-side session store (Redis, database, etc.), where the sensitive token remains server-side.
- Alternatively, encrypt the token before storing it client-side. Use strong encryption (such as Fernet symmetric encryption from the
cryptographypackage) with a key managed securely at the server. Upon receiving the cookie, the server can decrypt it and validate the token.
The best solution here, given the limited code shown, is to encrypt the token using Fernet before placing it in the cookie and decrypt it server-side as needed. This avoids clear-text exposure and does not require architectural changes. Update the /login/github/callback handler in api/routes/auth.py to:
- Import
Fernetand load a secret key. - Encrypt
api_tokenbefore setting it as the cookie value.
You will need to:
- Add an import for
Fernetfromcryptography.fernet. - Ensure your application has a Fernet key stored securely (perhaps as an environment variable).
- Encrypt
api_tokenbefore putting it in the cookie.
| @@ -5,6 +5,7 @@ | ||
| import secrets | ||
| from pathlib import Path | ||
| from urllib.parse import urljoin | ||
| from cryptography.fernet import Fernet | ||
|
|
||
| from fastapi import APIRouter, Request, HTTPException, status | ||
| from fastapi.responses import RedirectResponse, HTMLResponse | ||
| @@ -249,10 +250,17 @@ | ||
| # call the registered handler (await if async) | ||
| await handler('github', user_data, api_token) | ||
|
|
||
| # Fernet key can be loaded from environment, e.g., FERNET_KEY env var | ||
| fernet_key = os.getenv("FERNET_KEY") | ||
| if not fernet_key: | ||
| raise HTTPException(status_code=500, detail="Encryption key not configured") | ||
| fernet = Fernet(fernet_key.encode()) | ||
| encrypted_token = fernet.encrypt(api_token.encode()).decode() | ||
|
|
||
| redirect = RedirectResponse(url="/", status_code=302) | ||
| redirect.set_cookie( | ||
| key="api_token", | ||
| value=api_token, | ||
| value=encrypted_token, | ||
| httponly=True, | ||
| secure=True | ||
| ) |
| @@ -1,4 +1,7 @@ | ||
| [[source]] | ||
| [packages] | ||
|
|
||
| cryptography = "^45.0.6" | ||
| url = "https://pypi.org/simple" | ||
| verify_ssl = true | ||
| name = "pypi" |
| Package | Version | Security advisories |
| cryptography (pypi) | 45.0.6 | None |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/routes/graphs.py (1)
356-366: Handle errors from the find() task to avoid stream crashes.If
find()raises, the generator currently fails mid-stream. Catch and return a structured error message instead.- # Query is on-topic, wait for find results - result = await find_task + # Query is on-topic, wait for find results + try: + result = await find_task + except Exception as e: + logging.error("Error during graph analysis: %s", str(e), exc_info=True) + yield json.dumps({ + "type": "error", + "message": "Error analyzing graph" + }) + MESSAGE_DELIMITER + overall_elapsed = time.perf_counter() - overall_start + logging.info( + "Query processing failed during analysis - Total time: %.2f seconds", + overall_elapsed + ) + returnapi/app_factory.py (1)
83-97: Don’t globally catch all Exceptions for OAuth; scope to HTTPException and move the check.Catching all exceptions risks masking unrelated server errors. Limit to HTTPException and only redirect on OAuth/token-related messages. Also remove the TODO and silence the unused-argument warning.
- @app.exception_handler(Exception) - async def handle_oauth_error(request: Request, exc: Exception): + @app.exception_handler(HTTPException) + async def handle_oauth_error(request: Request, exc: HTTPException): # pylint: disable=unused-argument """Handle OAuth-related errors gracefully""" - # Check if it's an OAuth-related error - # TODO check this scenario - if "token" in str(exc).lower() or "oauth" in str(exc).lower(): - logging.warning("OAuth error occurred: %s", exc) - return RedirectResponse(url="/", status_code=302) - - # If it's an HTTPException, re-raise so FastAPI handles it properly - if isinstance(exc, HTTPException): - raise exc - - # For other errors, let them bubble up - raise exc + # Redirect only for OAuth/token-related HTTP errors + if "token" in str(exc).lower() or "oauth" in str(exc).lower(): + logging.warning("OAuth-related error occurred: %s", exc) + return RedirectResponse(url="/", status_code=302) + # Let FastAPI handle all other HTTP errors + raise exc
🧹 Nitpick comments (24)
api/loaders/base_loader.py (1)
10-16: Makeloadtruly abstract and raise NotImplementedError.Right now the base returns a tuple; marking it abstract enforces implementation and avoids accidental calls.
Apply this diff:
- @staticmethod - async def load(_graph_id: str, _data) -> Tuple[bool, str]: + @abstractmethod + @staticmethod + async def load(_graph_id: str, _data) -> Tuple[bool, str]: @@ - return False, "Not implemented" + raise NotImplementedError("Subclasses must implement load()")And add the missing import at the top (outside the selected lines):
from abc import ABC, abstractmethodapi/routes/graphs.py (4)
69-81: Tighten style: avoid else/elif after early returns.This satisfies pylint’s no-else-return and keeps the flow linear.
- if db_url_lower.startswith('postgresql://') or db_url_lower.startswith('postgres://'): - return 'postgresql', PostgresLoader - elif db_url_lower.startswith('mysql://'): - return 'mysql', MySQLLoader - else: - # Default to PostgresLoader for backward compatibility - return 'postgresql', PostgresLoader + if db_url_lower.startswith('postgresql://') or db_url_lower.startswith('postgres://'): + return 'postgresql', PostgresLoader + if db_url_lower.startswith('mysql://'): + return 'mysql', MySQLLoader + # Default to PostgresLoader for backward compatibility + return 'postgresql', PostgresLoader
277-283: Unify graph_id sanitation and length limit.You trim and limit to 100 here but 200 in get_graph_data. Recommend reusing the same sanitizer and constant everywhere to avoid edge-case mismatches.
- graph_id = graph_id.strip()[:100] # Limit length and strip whitespace + graph_id = sanitize_graph_id_component(graph_id) @@ - graph_id = f"{request.state.user_id}_{graph_id}" + graph_id = f"{request.state.user_id}_{graph_id}"
48-57: Avoid mutable default for Pydantic model field.
chat: list = []can be error-prone. UseField(default_factory=list)for clarity and to satisfy linters.Example change (outside selected lines):
from pydantic import BaseModel, Field class ConfirmRequest(BaseModel): sql_query: str confirmation: str = "" chat: list = Field(default_factory=list)
713-719: Broadened exception handler: include stack traces.Keep the broad catch if needed, but ensure full trace is logged.
- except Exception as e: - logging.error("Error in manual schema refresh: %s", e) + except Exception as e: + logging.error("Error in manual schema refresh: %s", e, exc_info=True)api/auth/user_management.py (7)
20-27: Docstring mismatch.The function fetches by
api_token, not by email. Update the docstring to avoid confusion.
40-46: Implement TODO: delete expired/invalid tokens on read.Cleaning up expired tokens reduces clutter and prevents reuse.
- # TODO delete invalid token from DB - if token_valid: + if token_valid: return { "email": result.result_set[0][0], "name": result.result_set[0][1], "picture": result.result_set[0][2] } + # Token is invalid/expired: clean it up + try: + await delete_user_token(api_token) + except Exception as cleanup_err: # best-effort + logging.warning("Failed to delete expired token: %s", cleanup_err)
107-146: Replace magic number TTL with parameterized constant; fixes line-length and improves clarity.Hardcoded
86400000is opaque. Use a constant and pass as a parameter.- MERGE (token:Token {id: $api_token}) - ON CREATE SET - token.created_at = timestamp(), - token.expires_at = timestamp() + 86400000 // 24h expiry + MERGE (token:Token {id: $api_token}) + ON CREATE SET + token.created_at = timestamp(), + token.expires_at = timestamp() + $token_ttl MERGE (identity)-[:HAS_TOKEN]->(token) @@ - "last_name": last_name, - "api_token": api_token + "last_name": last_name, + "api_token": api_token, + "token_ttl": API_TOKEN_TTL_MSAdd this constant near the imports (outside selected lines):
API_TOKEN_TTL_MS = 24 * 60 * 60 * 1000 # 24hAlso applies to: 148-157
49-51: Prefer logging.exception to capture stack traces.Gives more context in ops.
- logging.error("Error fetching user info: %s", e) + logging.exception("Error fetching user info")
70-72: Catching broad Exception.If db client exposes a specific exception type, prefer catching that; otherwise keep the broad catch but include
exc_info=True.- except Exception as e: - logging.error("Error deleting user token: %s", e) + except Exception as e: + logging.error("Error deleting user token: %s", e, exc_info=True)
260-266: Stale comment.This retry no longer clears session; update the comment to reflect current behavior.
- # Second attempt after clearing session to force re-validation + # Second attempt to re-validate before rejecting
248-251: Include stack traces for unexpected auth errors.Improves diagnosability.
- logging.error("Unexpected error in validate_user: %s", e) + logging.error("Unexpected error in validate_user: %s", e, exc_info=True)api/auth/oauth_handlers.py (1)
22-24: Return type and docstring clarity for handle_callbackAdd an explicit return type and tighten the docstring to make the contract obvious to callers and linters.
- async def handle_callback(provider: str, user_info: Dict[str, Any], api_token: str): - """Handle Provider OAuth callback processing""" + async def handle_callback(provider: str, user_info: Dict[str, Any], api_token: str) -> bool: + """ + Persist OAuth identity and session token. + Returns: + bool: True on successful persistence, False otherwise. + """api/routes/auth.py (11)
107-109: Add missing docstring to satisfy PylintSmall docstring prevents the “missing-function-docstring” warning.
@auth_router.get("/login", response_class=RedirectResponse) async def login_page(_: Request) -> RedirectResponse: + """Redirect to the default login provider (Google).""" return RedirectResponse(url="/login/google", status_code=status.HTTP_302_FOUND)
151-151: Add Google userinfo fallback when token lacks 'userinfo'Some IdP configs won’t hydrate token["userinfo"]. Fallback to parsing the ID token to avoid brittle failures.
- user_info = token.get("userinfo") + user_info = token.get("userinfo") + if not user_info: + # Fallback: derive userinfo from ID token when 'userinfo' claim not present + user_info = await google.parse_id_token(request, token)
179-183: Propagate original exception context when re-raisingUse “raise ... from e” to satisfy Ruff B904 and keep cause chains.
- raise HTTPException(status_code=400, detail=f"Authentication failed: {str(e)}") + raise HTTPException(status_code=400, detail=f"Authentication failed: {e}") from e
186-190: Add docstring for legacy Google callbackMinor lint fix and self-documenting intent.
async def google_callback_compat(request: Request) -> RedirectResponse: + """Back-compat handler: forward legacy /callback to /authorized preserving query string.""" qs = f"?{request.url.query}" if request.url.query else "" redirect = f"/login/google/authorized{qs}" return RedirectResponse(url=redirect, status_code=status.HTTP_307_TEMPORARY_REDIRECT)
193-196: Add docstring to GitHub login routeKeeps parity with Google route and satisfies lint.
@auth_router.get("/login/github", name="github.login", response_class=RedirectResponse) async def login_github(request: Request) -> RedirectResponse: + """Initiate GitHub OAuth login flow.""" github = _get_provider_client(request, "github")
262-262: Nit: Fix “Github” capitalization in errorUse “GitHub”.
- raise HTTPException(status_code=400, detail="Failed to get user info from Github") + raise HTTPException(status_code=400, detail="Failed to get user info from GitHub")
265-266: Propagate original exception context when re-raising (GitHub)Same as Google flow; makes debugging easier and satisfies Ruff B904.
- raise HTTPException(status_code=400, detail=f"Authentication failed: {str(e)}") + raise HTTPException(status_code=400, detail=f"Authentication failed: {e}") from e
275-286: Logout: align cookie deletion with attributes (optional)Not strictly required, but specifying path and samesite during deletion can help certain browsers match attributes during removal.
- if api_token: - resp.delete_cookie("api_token") + if api_token: + resp.delete_cookie("api_token", path="/", samesite="lax") await delete_user_token(api_token)
209-214: Add missing docstring to github_authorizedDocumenting the flow and satisfying Pylint.
@auth_router.get("/login/github/authorized", response_class=RedirectResponse) async def github_authorized(request: Request) -> RedirectResponse: + """Handle GitHub OAuth callback and user authorization.""" try:
269-273: Add docstring for legacy GitHub callbackParallels the Google compat route.
async def github_callback_compat(request: Request) -> RedirectResponse: + """Back-compat handler: forward legacy /callback to /authorized preserving query string.""" qs = f"?{request.url.query}" if request.url.query else "" redirect = f"/login/github/authorized{qs}" return RedirectResponse(url=redirect, status_code=status.HTTP_307_TEMPORARY_REDIRECT)
165-175: Hash tokens before persisting and store only the hash in DBCodeQL flagged “Clear-text storage of sensitive information.” Today the raw api_token is stored both in the cookie and as a DB node id. To reduce blast radius if the DB is leaked, store a one-way hash (e.g., SHA-256) in the DB and keep the raw token only in the cookie. On validation, hash the presented cookie and compare to the DB.
High-level steps:
- Compute token_hash = base64url(sha256(api_token)).
- Pass token_hash to ensure_user_in_organizations and persist that instead of the raw token.
- Update validate_user/_get_user_info to hash incoming cookie before lookup.
Happy to draft this change across oauth_handlers.py, user_management.py, and validation helpers if you want it in this PR.
Also applies to: 254-258
📜 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 (9)
api/app_factory.py(3 hunks)api/auth/__init__.py(1 hunks)api/auth/oauth_handlers.py(2 hunks)api/auth/user_management.py(6 hunks)api/loaders/base_loader.py(1 hunks)api/loaders/mysql_loader.py(1 hunks)api/loaders/postgres_loader.py(1 hunks)api/routes/auth.py(7 hunks)api/routes/graphs.py(6 hunks)
✅ Files skipped from review due to trivial changes (2)
- api/loaders/mysql_loader.py
- api/loaders/postgres_loader.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to pylint standards across all Python files (repository uses
make lint)
Files:
api/auth/oauth_handlers.pyapi/app_factory.pyapi/auth/__init__.pyapi/loaders/base_loader.pyapi/auth/user_management.pyapi/routes/auth.pyapi/routes/graphs.py
api/app_factory.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain the application factory and OAuth setup in
api/app_factory.py(core configuration in the top of the file)
Files:
api/app_factory.py
🧠 Learnings (1)
📚 Learning: 2025-08-24T17:15:21.337Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-24T17:15:21.337Z
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/app_factory.py
🧬 Code graph analysis (5)
api/auth/oauth_handlers.py (1)
api/auth/user_management.py (1)
ensure_user_in_organizations(74-190)
api/auth/__init__.py (2)
api/auth/user_management.py (2)
validate_user(228-250)token_required(252-292)api/auth/oauth_handlers.py (1)
setup_oauth_handlers(16-50)
api/loaders/base_loader.py (5)
api/loaders/mysql_loader.py (1)
load(128-170)api/loaders/postgres_loader.py (1)
load(67-108)api/loaders/csv_loader.py (1)
load(17-233)api/loaders/json_loader.py (1)
load(27-71)api/loaders/odata_loader.py (1)
load(17-28)
api/routes/auth.py (1)
api/auth/user_management.py (2)
delete_user_token(54-71)validate_user(228-250)
api/routes/graphs.py (5)
api/loaders/json_loader.py (2)
JSONLoader(23-71)load(27-71)api/loaders/mysql_loader.py (1)
load(128-170)api/loaders/postgres_loader.py (1)
load(67-108)api/loaders/csv_loader.py (1)
load(17-233)api/loaders/odata_loader.py (2)
load(17-28)ODataLoader(11-135)
🪛 GitHub Actions: Pylint
api/auth/oauth_handlers.py
[warning] 45-45: Pylint: Catching too general exception (broad-exception-caught).
api/app_factory.py
[warning] 86-86: Pylint: TODO check this scenario (fixme).
[warning] 23-23: Pylint: Too few public methods (too-few-public-methods).
[warning] 83-83: Pylint: Unused argument 'request' (unused-argument).
api/auth/user_management.py
[warning] 74-74: Pylint: Line too long (137/100) (line-too-long).
[warning] 40-40: Pylint: TODO delete invalid token from DB (fixme).
[warning] 70-70: Pylint: Catching too general exception (broad-exception-caught).
api/routes/auth.py
[warning] 80-80: Pylint: Line too long (127/100) (line-too-long).
[warning] 107-107: Pylint: Missing function or method docstring (missing-function-docstring).
[warning] 182-182: Pylint: Catching too general exception (broad-exception-caught).
[warning] 186-186: Pylint: Missing function or method docstring (missing-function-docstring).
[warning] 193-193: Pylint: Missing function or method docstring (missing-function-docstring).
[warning] 209-209: Pylint: Missing function or method docstring (missing-function-docstring).
[warning] 265-265: Pylint: Catching too general exception (broad-exception-caught).
[warning] 269-269: Pylint: Missing function or method docstring (missing-function-docstring).
api/routes/graphs.py
[warning] 74-74: Pylint: Unnecessary 'elif' after 'return' (no-else-return).
[warning] 108-108: Pylint: Too many local variables (too-many-locals).
[warning] 713-713: Pylint: Catching too general exception (broad-exception-caught).
🪛 Ruff (0.12.2)
api/routes/auth.py
182-182: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
265-265: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
api/routes/graphs.py
238-238: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 GitHub Check: CodeQL
api/routes/auth.py
[failure] 172-172: Clear-text storage of sensitive information
This expression stores sensitive data (password) as clear text.
[failure] 255-255: 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). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (6)
api/loaders/base_loader.py (1)
11-16: Async conversion looks good; contract preserved.Keeping the same return shape while moving to
asyncis compatible with existing async subclasses.api/routes/graphs.py (1)
119-121: Good: user-scoped graph IDs.Namespacing with
request.state.user_idprevents cross-user collisions.api/app_factory.py (1)
54-62: Centralizing SECRET_KEY is good.Using a single source of truth for the session secret reduces config drift.
api/auth/__init__.py (1)
10-21: API surface update looks consistent.Export now matches the token-based flow (
validate_userreplaces the old helper). No further issues.api/auth/oauth_handlers.py (1)
50-50: LGTM on centralized handler registrationMoving to a single provider-agnostic handler on app.state keeps routes thin and consistent.
api/routes/auth.py (1)
54-59: LGTM on explicit coercion of unauthenticated stateDefensively normalizing (is_authenticated=False, user_info=None) simplifies template logic.
| _, _ = await ensure_user_in_organizations( | ||
| user_id, | ||
| email, | ||
| name, | ||
| "google", | ||
| provider, | ||
| api_token, | ||
| user_info.get("picture"), | ||
| ) | ||
|
|
||
| return True | ||
| except Exception as exc: # capture exception for logging |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use ensure_user_in_organizations result to decide success
Right now True is returned even if persistence fails. Wire the returned tuple to control flow so routes can react correctly.
- _, _ = await ensure_user_in_organizations(
+ created, info = await ensure_user_in_organizations(
user_id,
email,
name,
provider,
api_token,
user_info.get("picture"),
)
-
- return True
+ if not info:
+ logging.error("Failed persisting identity for provider=%s email=%s", provider, email)
+ return False
+ return True📝 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.
| _, _ = await ensure_user_in_organizations( | |
| user_id, | |
| email, | |
| name, | |
| "google", | |
| provider, | |
| api_token, | |
| user_info.get("picture"), | |
| ) | |
| return True | |
| except Exception as exc: # capture exception for logging | |
| created, info = await ensure_user_in_organizations( | |
| user_id, | |
| email, | |
| name, | |
| provider, | |
| api_token, | |
| user_info.get("picture"), | |
| ) | |
| if not info: | |
| logging.error( | |
| "Failed persisting identity for provider=%s email=%s", | |
| provider, | |
| email, | |
| ) | |
| return False | |
| return True | |
| except Exception as exc: # capture exception for logging |
🧰 Tools
🪛 GitHub Actions: Pylint
[warning] 45-45: Pylint: Catching too general exception (broad-exception-caught).
🤖 Prompt for AI Agents
In api/auth/oauth_handlers.py around lines 35 to 45, the code always returns
True even if ensure_user_in_organizations fails; capture and inspect the tuple
it returns and use that to decide the return value: assign the returned tuple to
meaningful variables (e.g., success, payload = await
ensure_user_in_organizations(...)), then if success is falsy return False (or
propagate the error) and only return True when success is truthy; keep the
existing exception logging behavior but ensure the normal flow uses the
function's result to determine success so callers can react correctly.
| logging.error("Error handling %s OAuth callback: %s", provider, exc) | ||
| return False |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Narrow exception handling and log stack traces
Addresses Pylint “broad-exception-caught” and improves debuggability by logging tracebacks. Keep a final broad catch if you want maximal safety.
- except Exception as exc: # capture exception for logging
- logging.error("Error handling %s OAuth callback: %s", provider, exc)
- return False
+ except (KeyError, ValueError) as exc:
+ logging.error("Invalid OAuth payload for %s: %s", provider, exc)
+ return False
+ except Exception as exc: # pylint: disable=broad-except
+ logging.exception("Error handling %s OAuth callback", provider)
+ return FalseOptionally, also import and catch Authlib’s OAuthError for even better specificity:
🏁 Script executed:
#!/bin/bash
# Verify if Authlib's OAuthError is available in this project version.
python - <<'PY'
try:
from authlib.oauth2.rfc6749.errors import OAuthError
print("OAuthError available")
except Exception as e:
print("OAuthError not available:", e)
PYLength of output: 62
🏁 Script executed:
#!/bin/bash
# Search for authlib in dependency files and codebase
echo "Listing potential dependency files:"
find . -maxdepth 2 -type f \( -name "requirements*.txt" -o -name "Pipfile" -o -name "setup.py" -o -name "pyproject.toml" \) -print
echo
echo "Checking for 'authlib' in requirements files:"
grep -R "authlib" -n requirements*.txt 2>/dev/null || echo " → No authlib entry in requirements*.txt"
echo
echo "Checking for 'authlib' in Pipfile:"
grep -R "authlib" -n Pipfile 2>/dev/null || echo " → No authlib entry in Pipfile"
echo
echo "Checking for 'authlib' in setup.py and pyproject.toml:"
grep -R "authlib" -n setup.py pyproject.toml 2>/dev/null || echo " → No authlib entry in setup.py or pyproject.toml"
echo
echo "Searching codebase for any authlib imports or references:"
rg -n "authlib" . || echo " → No authlib references found in code"
echo
echo "Searching codebase for OAuthError usage:"
rg -n "OAuthError" . || echo " → No OAuthError references found in code"Length of output: 1509
Narrow exception handling and log full tracebacks
To address the Pylint “broad-exception-caught” warning and improve debuggability, catch only the errors you expect and log full tracebacks for anything unexpected. Since Authlib is declared in your Pipfile (authlib ~=1.4.0) and imported elsewhere, you can also catch its OAuthError.
• Add at the top of api/auth/oauth_handlers.py (adjust the import path if your Authlib version differs):
from authlib.integrations.base_client.errors import OAuthError• Replace the broad except Exception block with:
- except Exception as exc: # capture exception for logging
- logging.error("Error handling %s OAuth callback: %s", provider, exc)
- return False
+ except (KeyError, ValueError) as exc:
+ logging.error("Invalid OAuth payload for %s: %s", provider, exc)
+ return False
+ except OAuthError as exc:
+ logging.error("OAuth error during %s callback: %s", provider, exc)
+ return False
+ except Exception as exc: # pylint: disable=broad-except
+ logging.exception("Unexpected error handling %s OAuth callback", provider)
+ return False– The first two blocks handle common payload or client errors cleanly.
– The final block still guards against anything else, but now emits a full traceback to aid debugging.
🤖 Prompt for AI Agents
In api/auth/oauth_handlers.py around lines 46-47, replace the broad except
Exception block with narrow handlers: add an import for OAuthError from
authlib.integrations.base_client.errors at the top, then catch known errors
explicitly (e.g., except OAuthError: and except ValueError: or other expected
payload/client errors) and log concise error messages for those, and keep a
final generic except Exception that uses logging.exception(...) to record the
full traceback before returning False.
| api_token = request.cookies.get("api_token") | ||
| if not api_token: | ||
| api_token = request.query_params.get("api_token") | ||
|
|
||
| if api_token: | ||
| db_info = await _get_user_info(api_token) | ||
|
|
||
| if db_info: | ||
| return db_info, True | ||
|
|
||
| return None, False |
There was a problem hiding this comment.
Do not accept API tokens via query parameters; prefer Authorization: Bearer.
Tokens in URLs leak via logs, referrers, and proxies. Support the standard Authorization header and deprecate the query-param fallback.
Apply this diff:
- # token might be in the URL if not in the cookie for API access
- api_token = request.cookies.get("api_token")
- if not api_token:
- api_token = request.query_params.get("api_token")
+ # Prefer cookie, then Authorization: Bearer, then (deprecated) query param
+ api_token = request.cookies.get("api_token")
+ if not api_token:
+ auth_header = request.headers.get("Authorization")
+ if auth_header and auth_header.lower().startswith("bearer "):
+ api_token = auth_header[7:].strip()
+ if not api_token:
+ # Deprecated: remove once clients migrate
+ api_token = request.query_params.get("api_token")If you must keep the query-param path for now, consider feature-gating it behind an env flag and adding rate limits.
📝 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.
| api_token = request.cookies.get("api_token") | |
| if not api_token: | |
| api_token = request.query_params.get("api_token") | |
| if api_token: | |
| db_info = await _get_user_info(api_token) | |
| if db_info: | |
| return db_info, True | |
| return None, False | |
| # Prefer cookie, then Authorization: Bearer, then (deprecated) query param | |
| api_token = request.cookies.get("api_token") | |
| if not api_token: | |
| auth_header = request.headers.get("Authorization") | |
| if auth_header and auth_header.lower().startswith("bearer "): | |
| api_token = auth_header[7:].strip() | |
| if not api_token: | |
| # Deprecated: remove once clients migrate | |
| api_token = request.query_params.get("api_token") | |
| if api_token: | |
| db_info = await _get_user_info(api_token) | |
| if db_info: | |
| return db_info, True | |
| return None, False |
🤖 Prompt for AI Agents
In api/auth/user_management.py around lines 236 to 246, the current code accepts
api_token from query parameters which leaks secrets; change it to read the token
from the Authorization: Bearer header only (request.headers.get("Authorization")
-> extract bearer token) and call _get_user_info with that token; if you must
temporarily keep the query-param path, gate it behind an environment flag (e.g.,
ALLOW_QUERY_API_TOKEN) and ensure any fallback is rate-limited and clearly
logged as deprecated, otherwise remove the query-param branch entirely.
| # we're using the email as BASE64 encoded | ||
| request.state.user_id = base64.b64encode(user_info.get("email").encode()).decode() | ||
| if not request.state.user_id: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use URL-safe Base64 without padding for user_id.
Avoids '=' padding and '+'/'/' characters in graph IDs.
- request.state.user_id = base64.b64encode(user_info.get("email").encode()).decode()
+ request.state.user_id = (
+ base64.urlsafe_b64encode(user_info.get("email").encode())
+ .decode()
+ .rstrip("=")
+ )📝 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.
| # we're using the email as BASE64 encoded | |
| request.state.user_id = base64.b64encode(user_info.get("email").encode()).decode() | |
| if not request.state.user_id: | |
| # we're using the email as BASE64 encoded | |
| request.state.user_id = ( | |
| base64.urlsafe_b64encode(user_info.get("email").encode()) | |
| .decode() | |
| .rstrip("=") | |
| ) | |
| if not request.state.user_id: |
🤖 Prompt for AI Agents
In api/auth/user_management.py around lines 273 to 275, replace the standard
Base64 encoding with URL-safe Base64 and remove padding: encode the user's email
to bytes, use base64.urlsafe_b64encode(...).decode() and strip trailing '='
characters (e.g. rstrip('=')) so the resulting request.state.user_id contains no
'+' '/' or '='; keep the existing falsy check afterwards unchanged.
| """Handle the home page, rendering the landing page for unauthenticated users and the chat page for authenticated users.""" | ||
| user_info, is_authenticated_flag = await validate_user(request) | ||
|
|
||
| if not is_authenticated_flag: | ||
| _clear_auth_session(request.session) | ||
|
|
||
| if not is_authenticated_flag: | ||
| if is_authenticated_flag or user_info: | ||
| return templates.TemplateResponse( | ||
| "landing.j2", | ||
| "chat.j2", | ||
| { | ||
| "request": request, | ||
| "is_authenticated": False, | ||
| "user_info": None | ||
| "is_authenticated": True, | ||
| "user_info": user_info | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix long docstring and avoid granting chat on partial auth
- Break the docstring to satisfy line length linting.
- Require both is_authenticated_flag and user_info before rendering chat to avoid showing the chat to a user with a stale/partial state.
- """Handle the home page, rendering the landing page for unauthenticated users and the chat page for authenticated users."""
+ """
+ Render landing page for unauthenticated users; render chat for authenticated users.
+ """
user_info, is_authenticated_flag = await validate_user(request)
- if is_authenticated_flag or user_info:
+ if is_authenticated_flag and user_info:
return templates.TemplateResponse(
"chat.j2",
{
"request": request,
- "is_authenticated": True,
+ "is_authenticated": True,
"user_info": user_info
}
)📝 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.
| """Handle the home page, rendering the landing page for unauthenticated users and the chat page for authenticated users.""" | |
| user_info, is_authenticated_flag = await validate_user(request) | |
| if not is_authenticated_flag: | |
| _clear_auth_session(request.session) | |
| if not is_authenticated_flag: | |
| if is_authenticated_flag or user_info: | |
| return templates.TemplateResponse( | |
| "landing.j2", | |
| "chat.j2", | |
| { | |
| "request": request, | |
| "is_authenticated": False, | |
| "user_info": None | |
| "is_authenticated": True, | |
| "user_info": user_info | |
| } | |
| """ | |
| Render landing page for unauthenticated users; render chat for authenticated users. | |
| """ | |
| user_info, is_authenticated_flag = await validate_user(request) | |
| if is_authenticated_flag and user_info: | |
| return templates.TemplateResponse( | |
| "chat.j2", | |
| { | |
| "request": request, | |
| "is_authenticated": True, | |
| "user_info": user_info | |
| } | |
| ) |
🧰 Tools
🪛 GitHub Actions: Pylint
[warning] 80-80: Pylint: Line too long (127/100) (line-too-long).
🤖 Prompt for AI Agents
In api/routes/auth.py around lines 80 to 90, the module docstring is too long
and the current auth check uses "if is_authenticated_flag or user_info" which
can grant chat access on partial/stale state; split the long docstring into
multiple shorter lines to satisfy line-length linting, and change the condition
to require both values (use "if is_authenticated_flag and user_info") before
rendering chat.j2 so the chat is shown only when the user is fully authenticated
and user_info is present.
| 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('google', user_data, api_token) | ||
|
|
||
| # Call the registered Google callback handler if it exists to store user data. | ||
| handler = getattr(request.app.state, "google_callback_handler", None) | ||
| if handler: | ||
| # call the registered handler (await if async) | ||
| await handler(request, token, user_info) | ||
| redirect = RedirectResponse(url="/", status_code=302) | ||
| redirect.set_cookie( | ||
| key="api_token", | ||
| value=api_token, | ||
| httponly=True, | ||
| secure=True | ||
| ) | ||
|
|
||
| return RedirectResponse(url="/", status_code=status.HTTP_302_FOUND) | ||
| return redirect | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handler presence check and hardened cookie settings (samesite, max_age, HTTPS gating)
- Raise a 500 if callback_handler isn’t configured to avoid silent misbehavior.
- Align cookie lifetime (24h) with DB expiry.
- Add SameSite and set Secure dynamically so local HTTP dev still works.
- 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('google', user_data, api_token)
-
- redirect = RedirectResponse(url="/", status_code=302)
- redirect.set_cookie(
- key="api_token",
- value=api_token,
- httponly=True,
- secure=True
- )
-
- return redirect
+ handler = getattr(request.app.state, "callback_handler", None)
+ if not handler:
+ raise HTTPException(status_code=500, detail="OAuth callback handler not configured")
+
+ api_token = secrets.token_urlsafe(32) # ~43 chars, hard to guess
+
+ # persist identity/session
+ await handler('google', user_data, api_token)
+
+ redirect = RedirectResponse(url="/", status_code=302)
+ redirect.set_cookie(
+ key="api_token",
+ value=api_token,
+ httponly=True,
+ secure=(request.url.scheme == "https"),
+ samesite="lax",
+ max_age=86400,
+ )
+
+ return redirect📝 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.
| 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('google', user_data, api_token) | |
| # Call the registered Google callback handler if it exists to store user data. | |
| handler = getattr(request.app.state, "google_callback_handler", None) | |
| if handler: | |
| # call the registered handler (await if async) | |
| await handler(request, token, user_info) | |
| redirect = RedirectResponse(url="/", status_code=302) | |
| redirect.set_cookie( | |
| key="api_token", | |
| value=api_token, | |
| httponly=True, | |
| secure=True | |
| ) | |
| return RedirectResponse(url="/", status_code=status.HTTP_302_FOUND) | |
| return redirect | |
| handler = getattr(request.app.state, "callback_handler", None) | |
| if not handler: | |
| raise HTTPException(status_code=500, detail="OAuth callback handler not configured") | |
| api_token = secrets.token_urlsafe(32) # ~43 chars, hard to guess | |
| # persist identity/session | |
| await handler('google', user_data, api_token) | |
| redirect = RedirectResponse(url="/", status_code=302) | |
| redirect.set_cookie( | |
| key="api_token", | |
| value=api_token, | |
| httponly=True, | |
| secure=(request.url.scheme == "https"), | |
| samesite="lax", | |
| max_age=86400, | |
| ) | |
| return redirect |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 172-172: Clear-text storage of sensitive information
This expression stores sensitive data (password) as clear text.
🤖 Prompt for AI Agents
In api/routes/auth.py around lines 162-178, the code silently skips if
callback_handler is missing and sets a cookie without SameSite or max_age and
always Secure=true; change this to raise a 500 (HTTPException) when
callback_handler is not configured, keep generating the api_token and awaiting
the handler, then when building the RedirectResponse set_cookie include
max_age=86400 (24h) and same_site="Lax" and set secure=request.url.scheme ==
"https" so local HTTP dev works; ensure HTTPException is imported and return the
redirect after setting the hardened cookie.
| user_data = { | ||
| 'id': user_info.get('id'), | ||
| 'email': user_info.get('email'), | ||
| 'name': user_info.get('name'), | ||
| 'picture': user_info.get('avatar_url'), | ||
| } |
There was a problem hiding this comment.
Fix GitHub email handling (uses wrong variable)
You fetch primary email above but don’t use it when building user_data. Use the resolved email to avoid failing downstream validation.
- user_data = {
- 'id': user_info.get('id'),
- 'email': user_info.get('email'),
- 'name': user_info.get('name'),
- 'picture': user_info.get('avatar_url'),
- }
+ user_data = {
+ 'id': user_info.get('id'),
+ 'email': email,
+ 'name': user_info.get('name'),
+ 'picture': user_info.get('avatar_url'),
+ }Optional: for GitHub, name can be null; consider falling back to login if needed.
📝 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.
| user_data = { | |
| 'id': user_info.get('id'), | |
| 'email': user_info.get('email'), | |
| 'name': user_info.get('name'), | |
| 'picture': user_info.get('avatar_url'), | |
| } | |
| user_data = { | |
| 'id': user_info.get('id'), | |
| 'email': email, | |
| 'name': user_info.get('name'), | |
| 'picture': user_info.get('avatar_url'), | |
| } |
🤖 Prompt for AI Agents
In api/routes/auth.py around lines 236 to 241 the user_data dict is populated
using user_info.get('email') instead of the resolved primary email you fetched
earlier; replace the email field to use the resolved primary email variable
(e.g., primary_email or the variable name used when fetching GitHub emails) so
downstream validation receives the correct address, and optionally set name to
user_info.get('name') or fallback to user_info.get('login') when name is null.
| # Call the registered GitHub 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 GitHub callback handler if it exists to store user data. | ||
| handler = getattr(request.app.state, "github_callback_handler", None) | ||
| if handler: | ||
| # call the registered handler (await if async) | ||
| await handler(request, token, user_info) | ||
| # call the registered handler (await if async) | ||
| await handler('github', user_data, api_token) | ||
|
|
||
| return RedirectResponse(url="/", status_code=status.HTTP_302_FOUND) | ||
| redirect = RedirectResponse(url="/", status_code=302) | ||
| redirect.set_cookie( | ||
| key="api_token", | ||
| value=api_token, | ||
| httponly=True, | ||
| secure=True | ||
| ) | ||
|
|
||
| except AuthlibBaseError as e: | ||
| logging.error("GitHub OAuth error: %s", e) | ||
| _clear_auth_session(request.session) | ||
| return RedirectResponse(url="/", status_code=status.HTTP_302_FOUND) | ||
| return redirect | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror handler presence check and cookie hardening for GitHub
Same concerns and fixes as the Google flow.
- # Call the registered GitHub 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('github', user_data, api_token)
-
- redirect = RedirectResponse(url="/", status_code=302)
- redirect.set_cookie(
- key="api_token",
- value=api_token,
- httponly=True,
- secure=True
- )
-
- return redirect
+ handler = getattr(request.app.state, "callback_handler", None)
+ if not handler:
+ raise HTTPException(status_code=500, detail="OAuth callback handler not configured")
+
+ api_token = secrets.token_urlsafe(32) # ~43 chars, hard to guess
+
+ # persist identity/session
+ await handler('github', user_data, api_token)
+
+ redirect = RedirectResponse(url="/", status_code=302)
+ redirect.set_cookie(
+ key="api_token",
+ value=api_token,
+ httponly=True,
+ secure=(request.url.scheme == "https"),
+ samesite="lax",
+ max_age=86400,
+ )
+
+ return redirect📝 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.
| # Call the registered GitHub 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 GitHub callback handler if it exists to store user data. | |
| handler = getattr(request.app.state, "github_callback_handler", None) | |
| if handler: | |
| # call the registered handler (await if async) | |
| await handler(request, token, user_info) | |
| # call the registered handler (await if async) | |
| await handler('github', user_data, api_token) | |
| return RedirectResponse(url="/", status_code=status.HTTP_302_FOUND) | |
| redirect = RedirectResponse(url="/", status_code=302) | |
| redirect.set_cookie( | |
| key="api_token", | |
| value=api_token, | |
| httponly=True, | |
| secure=True | |
| ) | |
| except AuthlibBaseError as e: | |
| logging.error("GitHub OAuth error: %s", e) | |
| _clear_auth_session(request.session) | |
| return RedirectResponse(url="/", status_code=status.HTTP_302_FOUND) | |
| return redirect | |
| handler = getattr(request.app.state, "callback_handler", None) | |
| if not handler: | |
| raise HTTPException(status_code=500, detail="OAuth callback handler not configured") | |
| api_token = secrets.token_urlsafe(32) # ~43 chars, hard to guess | |
| # persist identity/session | |
| await handler('github', user_data, api_token) | |
| redirect = RedirectResponse(url="/", status_code=302) | |
| redirect.set_cookie( | |
| key="api_token", | |
| value=api_token, | |
| httponly=True, | |
| secure=(request.url.scheme == "https"), | |
| samesite="lax", | |
| max_age=86400, | |
| ) | |
| return redirect |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 255-255: Clear-text storage of sensitive information
This expression stores sensitive data (password) as clear text.
| graph_id = f"{request.state.user_id}_{data.database}" | ||
| success, result = await JSONLoader.load(graph_id, data.dict()) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize graph_id components from client-controlled fields (JSON database name and filenames).
These strings can contain path separators or control chars. Normalize to a safe charset to avoid malformed graph names and surprising behavior. Use a single helper for consistency across endpoints.
Apply this diff to use a sanitizer (helper added below):
- graph_id = f"{request.state.user_id}_{data.database}"
+ graph_id = f"{request.state.user_id}_{sanitize_graph_id_component(data.database)}"
@@
- graph_id = f"{request.state.user_id}_{data.get('database', '')}"
+ graph_id = f"{request.state.user_id}_{sanitize_graph_id_component(data.get('database', ''))}"
@@
- graph_id = f"{request.state.user_id}_{filename.replace('.xml', '')}"
+ graph_id = f"{request.state.user_id}_{sanitize_graph_id_component(filename).replace('.xml', '')}"
@@
- graph_id = f"{request.state.user_id}_{filename.replace('.csv', '')}"
+ graph_id = f"{request.state.user_id}_{sanitize_graph_id_component(filename).replace('.csv', '')}"And adjust the JSON decode except to preserve cause (ruff B904):
- except json.JSONDecodeError:
- raise HTTPException(status_code=400, detail="Invalid JSON file")
+ except json.JSONDecodeError as e:
+ raise HTTPException(status_code=400, detail="Invalid JSON file") from eAdd this helper near the other sanitizer functions (outside selected lines):
import re
SAFE_GRAPH_RE = re.compile(r'[^A-Za-z0-9._-]+')
MAX_GRAPH_ID_LEN = 100
def sanitize_graph_id_component(value: str) -> str:
if not isinstance(value, str):
value = str(value or "")
value = value.strip()
value = SAFE_GRAPH_RE.sub("_", value)
return value[:MAX_GRAPH_ID_LEN] or "unnamed"Also applies to: 235-238, 243-245, 249-251
…ging/eslint-9.34.0 Bump eslint from 9.33.0 to 9.34.0 in /app
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/package.json (1)
14-16: ESLint 9.34.0 Upgrade Verified – Optional Improvements Recommended
- Node.js version is v24.3.0 locally, which exceeds ESLint 9’s minimum requirement (>= 18.18.0). Confirm your CI and editor integrations likewise run Node >= 18.18.0 to avoid unexpected failures.
- No
"engines"field found in app/package.json. To prevent installs on unsupported Node versions, consider adding:{ // …existing fields… + "engines": { + "node": ">=18.18.0" + }, "devDependencies": { "eslint": "^9.34.0", "@typescript-eslint/parser": "^8.40.0", "@typescript-eslint/eslint-plugin": "^8.40.0" } }- @typescript-eslint/parser & eslint-plugin v8.40.0 declare
eslintpeerDependency of^8.0.0 || ^9.0.0, so they are compatible with ESLint 9. Ensure your lockfile resolves these cleanly (e.g. via a drynpm install --package-lock-only && npm ls --depth=0check).- No flat-config file (
eslint.config.js) detected in app/. If you intend to use the new flat config, add aneslint.config.js; otherwise explicitly opt back into the legacy.eslintrc.*format by settingESLINT_USE_FLAT_CONFIG=falseor keeping your existing configuration file in place.- Optional speedup: leverage ESLint’s multithreading and caching in your lint script. For example, in app/package.json:
"scripts": { - "lint": "eslint \"src/**/*.{ts,tsx}\"" + "lint": "eslint --ext .ts,.tsx --concurrency=auto --cache \"src/\"" }
📜 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 ignored due to path filters (1)
app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
app/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the TypeScript frontend sources in
app/and build them before production runs
Files:
app/package.json
⏰ 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
…4.60.0 Bump openai from 4.55.4 to 4.60.0
2 participants
Summary by CodeRabbit
New Features
Accessibility
Bug Fixes
Refactor
Style