-
Notifications
You must be signed in to change notification settings - Fork 27
Fastapi #97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 (
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| ) + MESSAGE_DELIMITER | ||
|
|
||
| return Response(stream_with_context(generate()), content_type="application/json") | ||
| return StreamingResponse(generate(), media_type="application/json") |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Stack trace information
Stack trace information
Stack trace information
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, we need to ensure that any error messages returned from the loader (specifically, those from MySQLLoader.load and MySQLLoader.refresh_graph_schema) are not sent directly to the user if they may contain sensitive information. Instead, we should log the detailed error message on the server and return a generic error message to the user.
The main region to change is in api/routes/graphs.py, inside the generate() function, where the result of loader_class.refresh_graph_schema(graph_id, db_url) is handled. If refresh_success is False, we should log refresh_message and return a generic error message in the response.
No changes are needed in api/loaders/mysql_loader.py unless you want to further sanitize error messages at the source, but the main exposure is in the API response.
Required changes:
- In
api/routes/graphs.py, inside thegenerate()function, update the block that handles schema refresh failures to log the detailed error and return a generic message to the user. - Ensure logging is done using the existing
loggingmodule.
-
Copy modified lines R467-R469
| @@ -464,8 +464,9 @@ | ||
| } | ||
| ) + MESSAGE_DELIMITER | ||
| else: | ||
| failure_msg = (f"⚠️ Schema was modified but graph " | ||
| f"refresh failed: {refresh_message}") | ||
| # Log the detailed error message server-side | ||
| logging.error("Graph schema refresh failed: %s", str(refresh_message)) | ||
| failure_msg = ("⚠️ Schema was modified but graph refresh failed due to an internal error. Please contact support or try again later.") | ||
| yield json.dumps( | ||
| { | ||
| "type": "schema_refresh", |
| ) + MESSAGE_DELIMITER | ||
|
|
||
| return Response(stream_with_context(generate_confirmation()), content_type="application/json") | ||
| return StreamingResponse(generate_confirmation(), media_type="application/json") |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Stack trace information
Stack trace information
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, we need to ensure that any error messages returned from the loader (specifically, refresh_message from loader_class.refresh_graph_schema) are not sent directly to the user if the operation fails. Instead, we should log the detailed error message on the server and return a generic error message in the API response. This change should be made in api/routes/graphs.py, in the confirm_destructive_operation function, specifically in the generator function generate_confirmation, where the schema refresh failure message is yielded to the user. The fix involves replacing the use of refresh_message in the response with a generic message, while logging the actual error for debugging purposes.
-
Copy modified line R576 -
Copy modified line R580
| @@ -573,11 +573,11 @@ | ||
| } | ||
| ) + MESSAGE_DELIMITER | ||
| else: | ||
| logging.error("Schema was modified but graph refresh failed: %s", refresh_message) | ||
| yield json.dumps( | ||
| { | ||
| "type": "schema_refresh", | ||
| "message": (f"⚠️ Schema was modified but graph refresh failed: " | ||
| f"{refresh_message}"), | ||
| "message": "⚠️ Schema was modified but graph refresh failed due to an internal error.", | ||
| "refresh_status": "failed" | ||
| } | ||
| ) + MESSAGE_DELIMITER |
No description provided.