🔒 Fix unsanitized exception data from OpenAI responses#86
Open
🔒 Fix unsanitized exception data from OpenAI responses#86
Conversation
Co-authored-by: sonra44 <215552628+sonra44@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideSanitizes OpenAI HTTP error response bodies before including them in OpenAIResponsesError messages and adds a regression test to ensure non-printable characters are stripped from logged error messages. Sequence diagram for sanitized OpenAI error handling in create_response_json_schemasequenceDiagram
participant Caller
participant OpenAIResponsesClient
participant OpenAIAPI
participant OpenAIResponsesError
Caller->>OpenAIResponsesClient: create_response_json_schema()
OpenAIResponsesClient->>OpenAIAPI: HTTP request for response schema
OpenAIAPI-->>OpenAIResponsesClient: HTTPError exc
Note over OpenAIResponsesClient: Error handling path
OpenAIResponsesClient->>OpenAIResponsesClient: exc.read().decode(utf_8) as raw_detail
OpenAIResponsesClient->>OpenAIResponsesClient: detail = sanitized(raw_detail)
OpenAIResponsesClient->>OpenAIResponsesError: raise with message including detail
OpenAIResponsesError-->>Caller: Exception with sanitized detail
Flow diagram for sanitizing OpenAI error response detailflowchart TD
A[Start error handling] --> B["Read exc.read() and decode utf_8 into raw_detail"]
B --> C[Iterate characters in raw_detail]
C --> D{Character c is printable?}
D -- Yes --> E[Append c to detail]
D -- No --> F[Append space to detail]
E --> G{More characters?}
F --> G
G -- Yes --> C
G -- No --> H[Join characters into final detail string]
H --> I[Use sanitized detail in OpenAIResponsesError message]
I --> J[End error handling]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughВнесены изменения в обработку ошибок HTTP в Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- To further harden log safety and avoid excessively large exception messages, consider truncating or bounding the sanitized
detailstring before including it in theOpenAIResponsesErrormessage (e.g., keeping only the first N characters and appending an ellipsis). - The test
assert "Error [31m message details here " in error_msgis quite brittle due to the exact spacing; it may be more robust to assert absence of control characters and presence of key substrings separately rather than matching a full formatted string.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- To further harden log safety and avoid excessively large exception messages, consider truncating or bounding the sanitized `detail` string before including it in the `OpenAIResponsesError` message (e.g., keeping only the first N characters and appending an ellipsis).
- The test `assert "Error [31m message details here " in error_msg` is quite brittle due to the exact spacing; it may be more robust to assert absence of control characters and presence of key substrings separately rather than matching a full formatted string.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qiki/services/q_core_agent/core/openai_responses_client.py`:
- Around line 115-119: The current try/except around reading the HTTPError body
swallows all exceptions and sets detail = "", losing context; change it to catch
specific decode/read errors (e.g., UnicodeDecodeError, ValueError, OSError) or
catch Exception as e and capture the error context into detail (e.g., include
e.__class__.__name__ and str(e) or a short fallback message) instead of blanking
it out, and either log the captured error via the same logger used in this
module or re-raise/convert to a domain error so callers can see the cause;
update the block that uses raw_detail, detail and exc (the HTTPError handling
code in openai_responses_client.py) to preserve and surface the underlying
exception information.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26f76ad6-bd1c-468f-a796-ba42aa699a24
📒 Files selected for processing (2)
src/qiki/services/q_core_agent/core/openai_responses_client.pysrc/qiki/services/q_core_agent/tests/test_openai_responses_client.py
Comment on lines
115
to
119
| try: | ||
| detail = exc.read().decode("utf-8") | ||
| raw_detail = exc.read().decode("utf-8") | ||
| detail = "".join(c if c.isprintable() else " " for c in raw_detail) | ||
| except Exception: # noqa: BLE001 | ||
| detail = "" |
There was a problem hiding this comment.
Не теряйте контекст при ошибке чтения тела HTTPError.
На Line 118 исключение перехватывается слишком широко и причина теряется (detail = ""). Это ухудшает диагностику инцидентов.
Предлагаемая правка
try:
- raw_detail = exc.read().decode("utf-8")
- detail = "".join(c if c.isprintable() else " " for c in raw_detail)
- except Exception: # noqa: BLE001
- detail = ""
+ raw_detail = exc.read().decode("utf-8", errors="replace")
+ except Exception as read_exc: # noqa: BLE001
+ raw_detail = f"<failed to read error body: {type(read_exc).__name__}>"
+ detail = "".join(c if c.isprintable() else " " for c in raw_detail)As per coding guidelines "Don't swallow exceptions silently; log context and re-raise or convert to a clear domain error".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/qiki/services/q_core_agent/core/openai_responses_client.py` around lines
115 - 119, The current try/except around reading the HTTPError body swallows all
exceptions and sets detail = "", losing context; change it to catch specific
decode/read errors (e.g., UnicodeDecodeError, ValueError, OSError) or catch
Exception as e and capture the error context into detail (e.g., include
e.__class__.__name__ and str(e) or a short fallback message) instead of blanking
it out, and either log the captured error via the same logger used in this
module or re-raise/convert to a domain error so callers can see the cause;
update the block that uses raw_detail, detail and exc (the HTTPError handling
code in openai_responses_client.py) to preserve and surface the underlying
exception information.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 What: The vulnerability fixed
Unsanitized error response bodies from OpenAI were directly included in the
OpenAIResponsesErrorexception message. This could lead to inclusion of control characters like newlines (\n,\r) or ANSI escape sequences (\x1b) in logs or terminal output.Though low impact because the data comes from OpenAI, injecting raw control characters into application logs or terminal output could lead to log injection (CRLF injection) or terminal sequence injection, allowing an attacker to manipulate logs, obscure malicious activity, or inject spoofed log entries.
🛡️ Solution: How the fix addresses the vulnerability
The
detailstring is now sanitized before being placed into the exception message. A generator expression"".join(c if c.isprintable() else " " for c in raw_detail)is used to safely replace all non-printable characters (including\n,\r,\t, and ANSI codes) with spaces. A unit test was also added to verify this behavior.PR created automatically by Jules for task 13610696534626654639 started by @sonra44
Summary by Sourcery
Sanitize OpenAI HTTP error details before including them in OpenAIResponsesError messages to prevent control characters from reaching logs or terminal output.
Bug Fixes:
Tests:
Summary by CodeRabbit
Улучшения
Исправления ошибок
Тесты