OLS-3072 Fix SAR security findings - #3034
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds request-size validation, replaces exposed exception text with fixed error causes, and associates pending tool approvals with authenticated user IDs. Tests and the OpenAPI schema reflect the new validation and response behavior. ChangesRequest safety and approval integrity
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AuthenticatedDependency
participant submit_tool_approval_decision
participant set_approval_decision
participant InMemoryPendingApprovalStore
AuthenticatedDependency->>submit_tool_approval_decision: provide user_id
submit_tool_approval_decision->>set_approval_decision: approval_id, user_id, approved
set_approval_decision->>InMemoryPendingApprovalStore: set_decision(approval_id, user_id, approved)
InMemoryPendingApprovalStore-->>set_approval_decision: ApprovalSetResult
set_approval_decision-->>submit_tool_approval_decision: decision result
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ols/app/endpoints/ols.py (1)
401-416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve exception context when raising HTTP errors.
Ruff B904 flags both raises in this handler. Append
from pg_errorandfrom quota_exceed_error(or intentionally usefrom None) so exception chaining is explicit; the fixed response details remain sanitized.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ols/app/endpoints/ols.py` around lines 401 - 416, Update both HTTPException raises in the handler’s pg_error and quota_exceed_error exception blocks to explicitly chain the original exception with from pg_error and from quota_exceed_error, respectively, while preserving the existing sanitized response details.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ols/app/models/models.py`:
- Line 41: Update LLMRequest query reassignment and redact_attachments() so
redacted query and Attachment fields are revalidated against their model limits
before processing. Ensure oversized redacted values produce the existing
client-error/4xx response path rather than an uncaught exception reaching the
generic 500 handler.
In `@ols/src/tools/tools.py`:
- Around line 415-417: Update the approval creation flow around
register_pending_approval to avoid registering requests with an empty user_id:
require a valid audit_ctx.user_id before creating an approvable request, or
route the audit_ctx-absent case through an explicitly non-interactive path.
Ensure every registered request has an owner matching the authenticated user
used by the approval endpoint.
---
Nitpick comments:
In `@ols/app/endpoints/ols.py`:
- Around line 401-416: Update both HTTPException raises in the handler’s
pg_error and quota_exceed_error exception blocks to explicitly chain the
original exception with from pg_error and from quota_exceed_error, respectively,
while preserving the existing sanitized response details.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0b4ce378-4e4d-45fa-b62c-cb9bcd1de047
📒 Files selected for processing (8)
ols/app/endpoints/mcp_apps.pyols/app/endpoints/ols.pyols/app/endpoints/tool_approvals.pyols/app/models/models.pyols/src/llms/providers/provider.pyols/src/tools/approval.pyols/src/tools/tools.pyols/utils/errors_parsing.py
💤 Files with no reviewable changes (1)
- ols/src/llms/providers/provider.py
| attachment_type: str | ||
| content_type: str | ||
| content: str | ||
| content: str = Field(max_length=100_000) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File list ==\n'
git ls-files ols/app/models/models.py ols/app/endpoints/ols.py
printf '\n== Outline: models.py ==\n'
ast-grep outline ols/app/models/models.py --view expanded || true
printf '\n== Outline: ols.py ==\n'
ast-grep outline ols/app/endpoints/ols.py --view expanded || true
printf '\n== Relevant lines from models.py ==\n'
sed -n '1,220p' ols/app/models/models.py | nl -ba | sed -n '1,220p'
printf '\n== Relevant lines from ols.py around redaction ==\n'
sed -n '620,760p' ols/app/endpoints/ols.py | nl -ba | sed -n '1,220p'Repository: openshift/lightspeed-service
Length of output: 8700
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== models.py: Attachment, LLMRequest, CacheEntry configs ==\n'
sed -n '1,130p' ols/app/models/models.py | cat -n
printf '\n== ols.py: redaction and response path ==\n'
sed -n '660,740p' ols/app/endpoints/ols.py | cat -nRepository: openshift/lightspeed-service
Length of output: 9414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '295,380p' ols/app/endpoints/ols.py | cat -n
printf '\n==\n'
sed -n '508,620p' ols/app/endpoints/ols.py | cat -nRepository: openshift/lightspeed-service
Length of output: 9279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "query_redactor|class .*Redactor|def redact\(" ols -g '!**/__pycache__/**'Repository: openshift/lightspeed-service
Length of output: 1035
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' ols/utils/redactor.py | cat -nRepository: openshift/lightspeed-service
Length of output: 2234
Re-check redacted values before applying the model limits. LLMRequest.query is reassigned after construction without assignment validation, and redact_attachments() rebuilds each Attachment directly. If redaction expands either field, the query can slip past the 32_000 bound and oversized attachments currently fall into the generic 500 path. Return a 4xx when redaction exceeds the limit instead of letting it bubble up.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ols/app/models/models.py` at line 41, Update LLMRequest query reassignment
and redact_attachments() so redacted query and Attachment fields are revalidated
against their model limits before processing. Ensure oversized redacted values
produce the existing client-error/4xx response path rather than an uncaught
exception reaching the generic 500 handler.
| approval_id = str(uuid4()) | ||
| register_pending_approval(approval_id=approval_id) | ||
| user_id = audit_ctx.user_id if audit_ctx else "" | ||
| register_pending_approval(approval_id=approval_id, user_id=user_id) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not create approvable requests with an empty owner.
When audit_ctx is absent, this stores user_id="". approval.py Lines 95-96 rejects every authenticated user whose ID differs, while the endpoint passes the authenticated ID, so these requests can never be approved and will time out. Require a user ID before registering or make the no-audit path explicitly non-interactive.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ols/src/tools/tools.py` around lines 415 - 417, Update the approval creation
flow around register_pending_approval to avoid registering requests with an
empty user_id: require a valid audit_ctx.user_id before creating an approvable
request, or route the audit_ctx-absent case through an explicitly
non-interactive path. Ensure every registered request has an owner matching the
authenticated user used by the approval endpoint.
|
/retest |
|
@xrajesh PR needs to fix test failures first |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/utils/test_errors_parsing.py (1)
144-152: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the complete sanitized HTTP response.
The changed assertions verify only
cause. The existingerror_messagechecks usestartswithandin, so raw details such as"Connection refused"or"Timed out"could be appended without failing the tests.Assert the exact fixed response for both HTTPX cases.
Proposed fix
- assert error_message.startswith(errors_parsing._NETWORK_PREFIX) - assert errors_parsing.NETWORK_ERROR_MSG in error_message + assert error_message == ( + f"{errors_parsing._NETWORK_PREFIX} " + f"{errors_parsing.NETWORK_ERROR_MSG}" + )Apply the same assertion to both HTTPX tests.
Also applies to: 155-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/utils/test_errors_parsing.py` around lines 144 - 152, Update both HTTPX error tests, test_parse_generic_llm_error_on_httpx_connect_error and the adjacent HTTPX case, to assert the complete expected sanitized error_message rather than using startswith or substring checks. Keep the existing status_code and cause assertions unchanged, and ensure the exact fixed response excludes raw details such as “Connection refused” or “Timed out.”
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/app/models/test_models.py`:
- Around line 173-174: Add the return annotation -> None to the three new test
methods, including test_llm_request_query_max_length and the methods at the
other indicated locations, while preserving their existing parameters and test
behavior.
- Around line 173-196: Add return annotations of -> None to all three new test
methods in tests/unit/app/models/test_models.py lines 173-196, including
test_llm_request_query_max_length, test_attachment_content_max_length, and
test_llm_request_attachments_max_count; also annotate
test_set_approval_decision_user_mismatch in tests/unit/tools/test_approval.py
lines 146-149. No other changes are needed.
---
Nitpick comments:
In `@tests/unit/utils/test_errors_parsing.py`:
- Around line 144-152: Update both HTTPX error tests,
test_parse_generic_llm_error_on_httpx_connect_error and the adjacent HTTPX case,
to assert the complete expected sanitized error_message rather than using
startswith or substring checks. Keep the existing status_code and cause
assertions unchanged, and ensure the exact fixed response excludes raw details
such as “Connection refused” or “Timed out.”
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a9f31ce-c282-4385-804e-9ee545c690fb
📒 Files selected for processing (5)
ols/src/tools/tools.pytests/unit/app/endpoints/test_tool_approvals.pytests/unit/app/models/test_models.pytests/unit/tools/test_approval.pytests/unit/utils/test_errors_parsing.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ols/src/tools/tools.py
| @staticmethod | ||
| def test_llm_request_query_max_length(): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add -> None to the new test methods.
The three new test methods have unannotated signatures. Add return annotations before running the repository's strict MyPy checks.
Proposed fix
- def test_llm_request_query_max_length():
+ def test_llm_request_query_max_length() -> None:
- def test_attachment_content_max_length():
+ def test_attachment_content_max_length() -> None:
- def test_llm_request_attachments_max_count():
+ def test_llm_request_attachments_max_count() -> None:As per coding guidelines, Python code must use type hints for all function signatures.
Also applies to: 180-181, 195-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/app/models/test_models.py` around lines 173 - 174, Add the return
annotation -> None to the three new test methods, including
test_llm_request_query_max_length and the methods at the other indicated
locations, while preserving their existing parameters and test behavior.
Source: Coding guidelines
| @staticmethod | ||
| def test_llm_request_query_max_length(): | ||
| """Test query field rejects input exceeding 32000 characters.""" | ||
| LLMRequest(query="x" * 32_000) | ||
| with pytest.raises(ValidationError, match="String should have at most 32000"): | ||
| LLMRequest(query="x" * 32_001) | ||
|
|
||
| @staticmethod | ||
| def test_attachment_content_max_length(): | ||
| """Test attachment content rejects input exceeding 100000 characters.""" | ||
| Attachment( | ||
| attachment_type="log", | ||
| content_type="text/plain", | ||
| content="x" * 100_000, | ||
| ) | ||
| with pytest.raises(ValidationError, match="String should have at most 100000"): | ||
| Attachment( | ||
| attachment_type="log", | ||
| content_type="text/plain", | ||
| content="x" * 100_001, | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def test_llm_request_attachments_max_count(): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add return annotations to the new test functions.
Both files add test functions without -> None.
tests/unit/app/models/test_models.py#L173-L196: add-> Noneto all three new test methods.tests/unit/tools/test_approval.py#L146-L149: add-> Nonetotest_set_approval_decision_user_mismatch.
As per coding guidelines, Python code must use type hints for all function signatures.
📍 Affects 2 files
tests/unit/app/models/test_models.py#L173-L196(this comment)tests/unit/tools/test_approval.py#L146-L149
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/app/models/test_models.py` around lines 173 - 196, Add return
annotations of -> None to all three new test methods in
tests/unit/app/models/test_models.py lines 173-196, including
test_llm_request_query_max_length, test_attachment_content_max_length, and
test_llm_request_attachments_max_count; also annotate
test_set_approval_decision_user_mismatch in tests/unit/tools/test_approval.py
lines 146-149. No other changes are needed.
Source: Coding guidelines
Remove check_hostname = False that was incorrectly disabling hostname verification when custom certificate stores were used without a TLS security profile. ssl.create_default_context() already enables hostname verification by default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace str(error) in HTTP response bodies with generic messages. Internal error details are already logged server-side for debugging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Limit query input to 32,000 characters at the API boundary to prevent oversized payloads from reaching the LLM pipeline. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Limit attachment content to 100,000 characters and the attachments list to 10 items to bound request payload size. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Store user_id with each pending approval and verify that the user submitting the decision matches the approval owner. This prevents users from approving or rejecting other users' tool execution requests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update existing tests to match new function signatures (user_id parameter in approval module, generic error messages in error parsing). Add new tests for max_length validation, user_id ownership mismatch, and attachment count limits. Add warning log when tool approval is requested without audit context. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update unit and integration tests to match generic error messages: - store_conversation_history tests expect "Conversation storage failed" - quota exceed test expects "Quota limit reached" - streaming error test expects "An unexpected error occurred" - summarizer error integration test expects generic cause - tool approval integration test passes user_id through - large attachment test reduced to stay under 100K content limit - regenerate OpenAPI schema with new field constraints Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Revert provider.py to match main (TLS fix commit is now a no-op after PR openshift#3044 removed the custom certificate store path) - Update e2e test_too_long_question to expect 422 (Pydantic validation) instead of 413, matching the max_length=32_000 change on query field Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
19c740c to
c2dd7d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/e2e/test_query_endpoint.py (1)
417-422: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAssert that the unredacted query is absent from logs.
The new checks only require a redacted log entry. They pass if the service logs both the original query and the transformed query. Add
assert query not in container_log_lowerafter retrieving the container log.
tests/e2e/test_query_endpoint.py#L417-L422: Assert thatqueryis absent fromcontainer_log_lower.tests/e2e/test_streaming_query_endpoint.py#L399-L404: Assert thatqueryis absent fromcontainer_log_lower.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/test_query_endpoint.py` around lines 417 - 422, Update the log assertions in tests/e2e/test_query_endpoint.py (lines 417-422) and tests/e2e/test_streaming_query_endpoint.py (lines 399-404) to assert that query is absent from container_log_lower after retrieving the container log, while preserving the existing redacted-query checks.ols/app/endpoints/ols.py (1)
338-352: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the added implementation comments.
The calls to
redact_query,retrieve_attachments,redact_attachments, and
append_attachments_to_querydescribe these operations. Remove the narrative comments.As per coding guidelines, “Avoid comments unless explicitly requested; make code
self-documenting.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ols/app/endpoints/ols.py` around lines 338 - 352, Remove the narrative implementation comments surrounding redact_query, retrieve_attachments, redact_attachments, and append_attachments_to_query, while preserving the existing calls and behavior.Source: Coding guidelines
🧹 Nitpick comments (1)
tests/e2e/test_query_endpoint.py (1)
182-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the new explanatory comments.
The setup and assertions already describe the test behavior.
tests/e2e/test_query_endpoint.py#L182-L182: Remove the maximum-length comment.tests/e2e/test_streaming_query_endpoint.py#L164-L164: Remove the maximum-length comment.tests/e2e/test_query_endpoint.py#L417-L418: Remove the redaction-pipeline comments.tests/e2e/test_streaming_query_endpoint.py#L399-L400: Remove the redaction-pipeline comments.As per coding guidelines, “Avoid comments unless explicitly requested; make code self-documenting.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/test_query_endpoint.py` at line 182, Remove the unnecessary explanatory comments from tests/e2e/test_query_endpoint.py lines 182 and 417-418, and tests/e2e/test_streaming_query_endpoint.py lines 164 and 399-400; leave the surrounding test setup and assertions unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ols/app/endpoints/ols.py`:
- Around line 387-401: Update the HTTPException raises in the PostgreSQL error
handler and quota-exceeded handler to explicitly chain them from their caught
exceptions, using pg_error for the database branch and quota_exceed_error for
the quota branch.
---
Outside diff comments:
In `@ols/app/endpoints/ols.py`:
- Around line 338-352: Remove the narrative implementation comments surrounding
redact_query, retrieve_attachments, redact_attachments, and
append_attachments_to_query, while preserving the existing calls and behavior.
In `@tests/e2e/test_query_endpoint.py`:
- Around line 417-422: Update the log assertions in
tests/e2e/test_query_endpoint.py (lines 417-422) and
tests/e2e/test_streaming_query_endpoint.py (lines 399-404) to assert that query
is absent from container_log_lower after retrieving the container log, while
preserving the existing redacted-query checks.
---
Nitpick comments:
In `@tests/e2e/test_query_endpoint.py`:
- Line 182: Remove the unnecessary explanatory comments from
tests/e2e/test_query_endpoint.py lines 182 and 417-418, and
tests/e2e/test_streaming_query_endpoint.py lines 164 and 399-400; leave the
surrounding test setup and assertions unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 96219116-dc65-44cc-9ef3-e6ba9b982220
📒 Files selected for processing (4)
ols/app/endpoints/ols.pyols/src/tools/tools.pytests/e2e/test_query_endpoint.pytests/e2e/test_streaming_query_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ols/src/tools/tools.py
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail={ | ||
| "response": message, | ||
| "cause": str(pg_error), | ||
| "cause": "Database connection error", | ||
| }, | ||
| ) | ||
| except Exception as quota_exceed_error: | ||
| message = "The quota has been exceeded" | ||
| logger.error(message) | ||
| logger.error("%s: %s", message, quota_exceed_error) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail={ | ||
| "response": message, | ||
| "cause": str(quota_exceed_error), | ||
| "cause": "Quota limit reached", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Chain the replacement HTTPException from the caught exception.
Ruff B904 reports the PostgreSQL branch. Use from pg_error and
from quota_exceed_error on the respective raise HTTPException(...) statements.
Proposed fix
- raise HTTPException(
+ raise HTTPException(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail={
"response": message,
"cause": "Database connection error",
},
- )
+ ) from pg_error
...
- raise HTTPException(
+ raise HTTPException(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail={
"response": message,
"cause": "Quota limit reached",
},
- )
+ ) from quota_exceed_error📝 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.
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail={ | |
| "response": message, | |
| "cause": str(pg_error), | |
| "cause": "Database connection error", | |
| }, | |
| ) | |
| except Exception as quota_exceed_error: | |
| message = "The quota has been exceeded" | |
| logger.error(message) | |
| logger.error("%s: %s", message, quota_exceed_error) | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail={ | |
| "response": message, | |
| "cause": str(quota_exceed_error), | |
| "cause": "Quota limit reached", | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail={ | |
| "response": message, | |
| "cause": "Database connection error", | |
| }, | |
| ) from pg_error | |
| except Exception as quota_exceed_error: | |
| message = "The quota has been exceeded" | |
| logger.error("%s: %s", message, quota_exceed_error) | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail={ | |
| "response": message, | |
| "cause": "Quota limit reached", | |
| }, | |
| ) from quota_exceed_error |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 387-393: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 394-394: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ols/app/endpoints/ols.py` around lines 387 - 401, Update the HTTPException
raises in the PostgreSQL error handler and quota-exceeded handler to explicitly
chain them from their caught exceptions, using pg_error for the database branch
and quota_exceed_error for the quota branch.
Sources: Coding guidelines, Linters/SAST tools
|
/test e2e-ols-cluster |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ols/app/endpoints/ols.py (1)
129-135: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winMake audit content capture opt-in.
request_startedomitsqueryand records onlyattachment_countwhen capture is disabled. However,AuditContext.capture_contentdefaults toTrue, andprocess_requestdoes not pass a configuration value. Addcapture_contenttoAuditConfigwith a default ofFalse, then pass it toAuditContext.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ols/app/endpoints/ols.py` around lines 129 - 135, Make audit content capture opt-in by adding a False-default capture_content setting to AuditConfig and passing that configured value when constructing AuditContext in process_request; ensure request_started uses the resulting context flag.
🧹 Nitpick comments (1)
ols/app/endpoints/ols.py (1)
338-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove comments that restate immediate control flow.
The calls already express the operation. The added comments violate the repository rule to avoid comments unless requested.
ols/app/endpoints/ols.py#L338-L352: remove the query-redaction and attachment-retrieval comments.ols/app/endpoints/ols.py#L702-L703: remove the in-loop implementation comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ols/app/endpoints/ols.py` around lines 338 - 352, Remove the redundant query-redaction and attachment-retrieval comments around redact_query and retrieve_attachments in ols/app/endpoints/ols.py lines 338-352, and remove the in-loop implementation comment at lines 702-703; leave the surrounding logic unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ols/app/endpoints/ols.py`:
- Around line 129-135: Make audit content capture opt-in by adding a
False-default capture_content setting to AuditConfig and passing that configured
value when constructing AuditContext in process_request; ensure request_started
uses the resulting context flag.
---
Nitpick comments:
In `@ols/app/endpoints/ols.py`:
- Around line 338-352: Remove the redundant query-redaction and
attachment-retrieval comments around redact_query and retrieve_attachments in
ols/app/endpoints/ols.py lines 338-352, and remove the in-loop implementation
comment at lines 702-703; leave the surrounding logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d1c15b9f-2fa3-4baa-8455-3599e03c9179
📒 Files selected for processing (4)
ols/app/endpoints/ols.pyols/src/tools/tools.pytests/e2e/test_query_endpoint.pytests/e2e/test_streaming_query_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/test_query_endpoint.py
- tests/e2e/test_streaming_query_endpoint.py
- ols/src/tools/tools.py
blublinsky
left a comment
blublinsky
left a comment
There was a problem hiding this comment.
Non-blocking suggestion (observability): InMemoryPendingApprovalStore.set_decision silently returns NOT_FOUND on user-id mismatch without any logging. Other edge cases in the same module (get_approval_decision) do log timeouts, missing state, and unexpected errors — so this omission is inconsistent.
For security audit trail purposes, a warning log would let operators detect potential IDOR probing without leaking anything to the caller:
if pending.user_id != user_id:
logger.warning(
"Approval %s: ownership mismatch (requester=%s, owner=%s)",
approval_id, user_id, pending.user_id,
)
return ApprovalSetResult.NOT_FOUNDNot blocking — the ownership check itself is correct and the security boundary is enforced. This could be a follow-up.
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xrajesh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@xrajesh: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
Fixes five security findings from the SAR (Security Architecture Review) under epic OLS-3072:
check_hostname = Falsethat disabled TLS hostname verification when using custom certificate stores without a TLS security profilestr(error)) in HTTP response bodies with generic messages acrossols.py,mcp_apps.py, anderrors_parsing.pymax_length=32000to thequeryfield inLLMRequestto bound input size at the API boundarymax_length=100000toAttachment.contentandmax_length=10to theattachmentslist to limit payload sizeuser_idownership toPendingApprovaland verify that the user submitting a tool approval decision matches the approval ownerTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Validation
Documentation