Conversation
| except Exception as e: | ||
| logger.error("Exa search failed", extra={"query": search.query, "error": str(e)}, exc_info=True) | ||
| raise HTTPException(status_code=500, detail="Internal server error") | ||
| return {"error": f"Exa search failed: {e!s}"} |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, the fix is to avoid returning the raw exception object or its message to the client. Instead, log the detailed error (including stack trace) on the server side, and return a generic, non-sensitive error message to the client. This keeps observability for developers while preventing information disclosure.
For this specific function in server/app/domains/mcp/api/proxy_controller.py, the best change with minimal impact is:
- Keep the broad
except Exception as e:block but:- Log the exception (with stack trace) using the existing
logurulogger. - Return a generic error object to the client that does not interpolate
eat all.
- Log the exception (with stack trace) using the existing
- We already import
loggerfromloguruat line 17, so no new imports are needed. - The change is localized to the
exceptblock at lines 80–81.
Concretely:
- Replace the current
exceptblock body so that it callslogger.exception("Exa search failed")(which logs the stack trace) and returns something like{"error": "Exa search failed"}or a slightly more user-friendly but still generic message. - Do not include
e, its string form, or any traceback in the returned JSON.
| @@ -78,7 +78,8 @@ | ||
| return results | ||
|
|
||
| except Exception as e: | ||
| return {"error": f"Exa search failed: {e!s}"} | ||
| logger.exception("Exa search failed") | ||
| return {"error": "Exa search failed"} | ||
|
|
||
|
|
||
| @router.get("/google") |
| </body> | ||
| </html> | ||
| """ | ||
| return HTMLResponse(content=html_content) |
Check warning
Code scanning / CodeQL
Reflected server-side cross-site scripting Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, the safest pattern is: (1) strictly validate user input, (2) encode it for its specific output context (URL, HTML, JavaScript), and (3) minimize the amount of user-controlled data that reaches executable contexts like <script>. The existing code already validates and URL-encodes values, then uses json.dumps to put the URL into a JavaScript string. To satisfy CodeQL and make the safety guarantees clearer, we should tighten validation and make it explicit that only a restricted, URL-safe character set is permitted, and that nothing resembling HTML/JS metacharacters can ever reach the script block.
The single best change here—without altering functionality—is to (a) narrow the allowed character set in _validate_oauth_param to a small URL/token-safe set and remove spaces and / from the non-underscore whitelist used for code, state, and app, and (b) ensure redirect_url is always treated as a plain string safely embedded via json.dumps. This makes it cryptographically obvious to the analyzer that no <, >, ", ', `, (, ), or ; can appear from user input, so the JavaScript context is safe. Concretely:
- Update
_OAUTH_SAFE_CHARSto exclude characters that are not needed in these parameters and that could complicate analysis. - Adjust
_validate_oauth_param’s per-character check so that all characters must be in_OAUTH_SAFE_CHARS, instead of allowing whitespace and additional punctuation like/,+,=. - Keep the existing
quote(..., safe='')andjson.dumps(redirect_url)so the effective behavior (building aneigent://callback/oauth?...URL and redirecting to it in JS) remains unchanged for valid OAuth codes and states, but any value containing previously-allowed punctuation is now rejected with HTTP 400 instead of being reflected.
All required changes are limited to server/app/domains/oauth/api/oauth_controller.py, modifying the constants and validation logic shown.
| @@ -28,7 +28,9 @@ | ||
|
|
||
| _OAUTH_CODE_MAX_LEN = 2048 | ||
| _OAUTH_STATE_MAX_LEN = 512 | ||
| _OAUTH_SAFE_CHARS = set("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_.") | ||
| # Only allow a strict, URL-safe subset of characters for OAuth parameters to | ||
| # ensure they cannot introduce HTML/JS metacharacters into responses. | ||
| _OAUTH_SAFE_CHARS = set("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_.~") | ||
|
|
||
|
|
||
| def _validate_oauth_param(value: Optional[str], name: str, max_len: int) -> str: | ||
| @@ -38,11 +40,11 @@ | ||
| s = str(value).strip() | ||
| if len(s) > max_len: | ||
| raise HTTPException(status_code=400, detail=f"Invalid {name}: too long") | ||
| if s and not all(c in _OAUTH_SAFE_CHARS or c in " /+=" for c in s): | ||
| # Reject any character that is not in the strict safe set. | ||
| if s and not all(c in _OAUTH_SAFE_CHARS for c in s): | ||
| raise HTTPException(status_code=400, detail=f"Invalid {name}: invalid characters") | ||
| return s | ||
|
|
||
|
|
||
| @router.get("/{app}/login", name="OAuth Login Redirect") | ||
| def oauth_login(app: str, request: Request, state: Optional[str] = None): | ||
| callback_url = str(request.url_for("OAuth Callback", app=app)) |
| try: | ||
| result = TriggerCrudService.create(data, auth.id, db_session) | ||
| _raise_on_error(result) | ||
| return result["trigger_out"] |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
General approach: Avoid returning raw exception messages or stack traces to the client. Instead, log detailed errors on the server and return a generic, user-safe message (or a controlled validation message) in the HTTP response. In this codebase, the problematic path is that TriggerCrudService.create returns {"success": False, "error": str(e), "status_code": 400} and _raise_on_error forwards error verbatim as HTTPException.detail. We should (a) stop putting the raw exception string into the service result, and (b) ensure _raise_on_error uses a generic message for unexpected errors while still allowing safe, pre-defined messages.
Best concrete fix without changing functionality:
-
In
TriggerCrudService.create(service layer), when catchingValueErrorfrom_validate_trigger_config, do not exposestr(e)directly. Instead:- Log the exception server-side with
logger.exception(...)so developers still see details. - Return a safe, generic or controlled error message such as
"Invalid trigger configuration"with status code 400.
This keeps the same HTTP status code and overall control flow (still
success: Falseand 400) but removes the stack-trace/exception-message leakage. - Log the exception server-side with
-
In
_raise_on_errorintrigger_controller.py, keep the general behavior but now it will only ever see sanitizederrorvalues from the service. If later more service methods are added that include an unsanitizederror, having the pattern already fixed increateencourages them to follow the same approach.
Changes needed:
server/app/domains/trigger/service/trigger_crud_service.py:- In the
createmethod’sexcept ValueError as e:block, replace{"success": False, "error": str(e), "status_code": 400}with:- A
logger.exception("...")call to record the stack trace. - A dict using a safe message like
"Invalid trigger configuration"(or similar generic wording).
- A
- In the
server/app/domains/trigger/api/trigger_controller.py:- No logic change strictly required for the fix, but nothing here needs modification as long as the service now returns safe
errorvalues.
- No logic change strictly required for the fix, but nothing here needs modification as long as the service now returns safe
This preserves external API semantics (400 vs 500 codes, validation still reported as a client error) while eliminating the flow of exception details to clients.
| @@ -233,7 +233,15 @@ | ||
| try: | ||
| TriggerCrudService._validate_trigger_config(data.trigger_type, data.config) | ||
| except ValueError as e: | ||
| return {"success": False, "error": str(e), "status_code": 400} | ||
| logger.exception( | ||
| "Trigger configuration validation failed", | ||
| extra={"user_id": user_id, "project_id": getattr(data, "project_id", None), "error": str(e)}, | ||
| ) | ||
| return { | ||
| "success": False, | ||
| "error": "Invalid trigger configuration", | ||
| "status_code": 400, | ||
| } | ||
|
|
||
| # 4. Determine initial status | ||
| initial_status = TriggerCrudService._determine_initial_status(data, user_id, s) |
| try: | ||
| result = TriggerCrudService.update(trigger_id, data, auth.id, db_session) | ||
| _raise_on_error(result) | ||
| return result["trigger_out"] |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, we should avoid propagating raw exception messages (and certainly never any stack trace) directly to clients. Instead, we should log detailed errors server-side and return sanitized, high-level error messages to the client. For validation errors that result in 400 responses, we can still provide clear messages, but they should be controlled strings we define, not arbitrary str(e) from a lower-level function.
The best targeted fix, without changing existing functionality too much, is:
- In
TriggerCrudService.update, replace the direct use ofstr(e)in the error dictionary with a generic, user-safe message such as"Invalid trigger configuration". We keep the400status code so behaviour from the client’s perspective remains a validation error, but the actual text is now controlled. - Log the original exception internally at the service level so developers can still diagnose issues without relying on leaking details to the client. Since
loguru.loggeris already imported at the top oftrigger_crud_service.py, we can use it directly.
Concretely:
- Edit
server/app/domains/trigger/service/trigger_crud_service.pyaround lines 283–288 to add alogger.error(...)call in theexcept ValueError as eblock, and change the returnederrorstring to a safe, generic message. - No changes are required in
trigger_controller.py, because_raise_on_errorwill now only ever see sanitizederrorstrings from this path.
| @@ -285,7 +285,20 @@ | ||
| try: | ||
| TriggerCrudService._validate_trigger_config(trigger.trigger_type, update_data["config"]) | ||
| except ValueError as e: | ||
| return {"success": False, "error": str(e), "status_code": 400} | ||
| logger.error( | ||
| "Trigger config validation failed", | ||
| extra={ | ||
| "user_id": user_id, | ||
| "trigger_id": trigger_id, | ||
| "error": str(e), | ||
| }, | ||
| exc_info=True, | ||
| ) | ||
| return { | ||
| "success": False, | ||
| "error": "Invalid trigger configuration", | ||
| "status_code": 400, | ||
| } | ||
|
|
||
| for key, value in update_data.items(): | ||
| setattr(trigger, key, value) |
a78c265 to
9a94e2b
Compare
Related Issue
eigent-ai/eigent_server#80
Description
Adapt to the v1 domain-driven architecture introduced in eigent-ai/eigent_server#80.
Server
controller/component/servicelayout into domain-driven modules:chat,config,mcp,model_provider,oauth,trigger,userapi/ schema/ service/three-layer conventioncomponent/→core/shared/packagecomponent/auth.py,component/permission.py,component/stack_auth.py,component/time_friendly.pyFrontend
/api/...to/api/v1/...endpoints to match the new server routingTesting Evidence (REQUIRED)
What is the purpose of this pull request?
Contribution Guidelines Acknowledgement