Conversation
|
Caution Review failedThe pull request is closed. WalkthroughCentralized 401 Unauthorized response handling by introducing UNAUTHORIZED_RESPONSE and applying it across tokens, database, and graph routes. Added Pydantic request models and input-sanitization and DB-type helper utilities in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Router as API Router
participant Auth as Auth Check
participant Graphs as Graphs Controller
participant Logger as Logger
Client->>Router: POST /graphs/... (payload)
Router->>Auth: Validate token/session
alt Unauthorized
Auth-->>Router: 401 UNAUTHORIZED_RESPONSE
Router-->>Client: 401 {"description": "Unauthorized - Please log in or provide a valid API token"}
else Authorized
Router->>Graphs: Parse & validate (Pydantic models)
Graphs->>Logger: sanitize_query / sanitize_log_input
Graphs->>Graphs: get_database_type_and_loader (if DB URL used)
Graphs-->>Client: 200 / 4xx (existing logic)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ 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 (3)
api/routes/tokens.py (1)
126-136: Risk: deleting by last 4 chars can remove multiple tokens.RIGHT(token.id, 4) match is ambiguous; if two tokens share the same suffix for a user, both will be deleted. Guard and delete by exact id once uniquely identified.
Apply this refactor to first resolve the suffix to a single token, then delete by full id; return 409 when ambiguous:
- # Delete the token - delete_query = """ - MATCH (user:Identity {email:$user_email, provider:'api'})-[:HAS_TOKEN]->(token:Token) - WHERE RIGHT(token.id, 4)=$token_id - DELETE token - RETURN COUNT(*) AS deleted_count - """ - - result = await organizations_graph.query(delete_query, { - "user_email": user_email, - "token_id": token_id - }) + # Resolve last-4 suffix to a single token id, then delete by exact id + match_query = """ + MATCH (user:Identity {email:$user_email, provider:'api'})-[:HAS_TOKEN]->(token:Token) + WHERE RIGHT(token.id, 4) = $token_suffix + RETURN token.id AS id + """ + match_res = await organizations_graph.query(match_query, { + "user_email": user_email, + "token_suffix": token_id, + }) + ids = [row[0] for row in (match_res.result_set or [])] + if not ids: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Token not found") + if len(ids) > 1: + raise HTTPException(status_code=status.HTTP_409_CONFLICT, + detail="Multiple tokens match this suffix; specify a unique token") + + delete_by_id = """ + MATCH (user:Identity {email:$user_email, provider:'api'})-[:HAS_TOKEN]->(token:Token {id:$token_full_id}) + DELETE token + RETURN 1 AS deleted_count + """ + result = await organizations_graph.query(delete_by_id, { + "user_email": user_email, + "token_full_id": ids[0], + })Optionally, document in the route that path parameter expects a last-4 suffix.
Also applies to: 142-147
api/routes/graphs.py (2)
34-63: Fix mutable default and tighten request models.
- ConfirmRequest.chat uses a mutable default list; switch to Field(default_factory=list).
- Optional: enforce a non-empty chat list at the schema level for ChatRequest to reduce runtime checks.
-from pydantic import BaseModel +from pydantic import BaseModel, Field @@ class ChatRequest(BaseModel): @@ - chat: list[str] + chat: list[str] # consider enforcing non-empty via validators/Field if desired @@ class ConfirmRequest(BaseModel): @@ - chat: list = [] + chat: list[str] = Field(default_factory=list)
395-399: Sanitize SQL text before logging.Even generated SQL can include user content; prevent log injection/newlines.
- logging.info("Generated SQL query: %s", answer_an['sql_query']) # nosemgrep + logging.info("Generated SQL query: %s", + sanitize_log_input(answer_an.get('sql_query', ''))) # nosemgrep
🧹 Nitpick comments (8)
api/routes/tokens.py (1)
14-14: Decouple shared response constants from route modules.To avoid inter-route dependencies (database/graphs importing tokens), move UNAUTHORIZED_RESPONSE to a small shared module (e.g., api/http/responses.py) and import from there.
api/routes/database.py (3)
20-21: Avoid duplicating MESSAGE_DELIMITER across modules.Consider a single source (e.g., api.config or api.constants) used by both database.py and graphs.py to prevent drift with the frontend delimiter.
86-86: Minor style nit: assign without creating a list.Slightly cleaner and clearer.
- success, result = [False, ""] + success, result = False, ""
88-106: Consider adding a timeout to schema loading.External DB connections can hang; wrap loader.load with a timeout configurable via Config to fail fast and surface an actionable error.
- async for progress in loader.load(request.state.user_id, url): + # e.g., Config.DB_CONNECT_TIMEOUT seconds + try: + async with asyncio.timeout(getattr(Config, "DB_CONNECT_TIMEOUT", 60)): + async for progress in loader.load(request.state.user_id, url): + success, result = progress + if success: + steps_counter += 1 + yield json.dumps( + {"type": "reasoning_step", "message": f"Step {steps_counter}: {result}"} + ) + MESSAGE_DELIMITER + else: + break + except TimeoutError: + yield json.dumps( + {"type": "error", "message": "Timeout while connecting/loading schema"} + ) + MESSAGE_DELIMITER + returnapi/routes/graphs.py (4)
92-101: Docstring vs implementation mismatch in sanitize_log_input.Docstring says “wrap in repr()” but code doesn’t. Either update the docstring or actually wrap.
- return value.replace('\n', ' ').replace('\r', ' ').replace('\t', ' ') + cleaned = value.replace('\n', ' ').replace('\r', ' ').replace('\t', ' ') + return repr(cleaned)
85-87: Defaulting unknown URLs to Postgres can misroute operations.If backward-compat isn’t required, prefer failing fast by returning (None, None) on unknown scheme so callers surface a clear error.
- # Default to PostgresLoader for backward compatibility - return 'postgresql', PostgresLoader + # Unknown scheme: fail fast + return None, None
27-28: Single source of truth for MESSAGE_DELIMITER.Same suggestion as database.py—lift this to a shared constant to keep API and frontend coordinated.
102-114: Optional: strengthen graph_id validation.If FalkorDB has name constraints, validate against a safe pattern (e.g., [A-Za-z0-9_-]{1,200}) to avoid problematic characters.
def _graph_name(request: Request, graph_id:str) -> str: - if not graph_id or not isinstance(graph_id, str): + if not graph_id or not isinstance(graph_id, str): raise HTTPException(status_code=400, detail="Invalid graph_id") - graph_id = graph_id.strip()[:200] + graph_id = graph_id.strip()[:200] + # e.g., enforce allowed chars if desired + # import re at top if adopting this + # if not re.fullmatch(r"[A-Za-z0-9_-]{1,200}", graph_id): + # raise HTTPException(status_code=400, detail="graph_id contains invalid characters")
📜 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)
api/routes/database.py(2 hunks)api/routes/graphs.py(8 hunks)api/routes/tokens.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/routes/tokens.pyapi/routes/database.pyapi/routes/graphs.py
🧬 Code graph analysis (1)
api/routes/graphs.py (1)
app/ts/modules/config.ts (1)
MESSAGE_DELIMITER(5-5)
⏰ 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). (5)
- GitHub Check: deploy
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (5)
api/routes/tokens.py (2)
14-14: Good centralization of 401 response metadata.Defining UNAUTHORIZED_RESPONSE once improves consistency and avoids drift.
29-30: Consistent reuse of UNAUTHORIZED_RESPONSE across endpoints.Nice cleanup; reduces duplication and keeps docs uniform.
Also applies to: 74-75, 114-115
api/routes/database.py (1)
15-16: Adopting shared UNAUTHORIZED_RESPONSE looks good.Keeps OpenAPI docs consistent with other routes.
Also applies to: 32-33
api/routes/graphs.py (2)
24-25: Good reuse of UNAUTHORIZED_RESPONSE.This aligns graph routes with the standardized 401 docs.
116-117: 401 docs standardized across graph endpoints.Nice consistency pass.
Also applies to: 138-139, 237-238, 273-274, 667-668, 831-832
1 participant
Summary by CodeRabbit