fix: 14 small triaged issues (OIDC/security hardening, loaders, logging, flaky test) - #491
Conversation
|
Warning Review limit reached
More reviews will be available in 3 minutes and 11 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR applies a batch of security hardening fixes and bug corrections: OIDC token validation gains ChangesSecurity Hardening and Bug Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
5233ca5 to
b911d68
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openrag/routers/indexer.py (1)
410-425:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap the copy actor call with the centralized timeout helper.
This endpoint still awaits
indexer.copy_file.remote(...)directly, so a stalled Ray call can hang request handling and bypass the standardized cancellation/error path.Suggested patch
- await indexer.copy_file.remote(file_id=source_file_id, metadata=metadata, partition=source_partition, user=user) + await call_ray_actor_with_timeout( + indexer.copy_file.remote( + file_id=source_file_id, + metadata=metadata, + partition=source_partition, + user=user, + ), + timeout=VECTORDB_TIMEOUT, + task_description=f"copy_file({source_partition}:{source_file_id}->{partition}:{file_id})", + )As per coding guidelines, “Use the centralized
call_ray_actor_with_timeout()utility fromopenrag/components/ray_utilsfor all Ray actor method calls, which handles timeout, cancellation, and error management.”🤖 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 `@openrag/routers/indexer.py` around lines 410 - 425, The direct await of indexer.copy_file.remote() bypasses the standardized timeout and error handling mechanisms. Replace the direct await call to indexer.copy_file.remote() with a call to the centralized call_ray_actor_with_timeout() utility function from openrag/components/ray_utils, passing the appropriate parameters (actor method, timeout value, and any required arguments) to ensure consistent timeout management, cancellation handling, and error management across all Ray actor calls.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 `@ansible/ansible.cfg`:
- Line 10: Remove the `-o UserKnownHostsFile=/dev/null` parameter from the
ssh_args configuration line in ansible.cfg. This parameter discards host keys
after each connection, preventing SSH from validating host key changes across
sessions and eliminating MITM detection. Keep the other ssh_args options like
ControlMaster, ControlPersist, and StrictHostKeyChecking intact, allowing SSH to
use its default known_hosts location for proper host key validation.
In `@ansible/playbooks/openrag.yml`:
- Around line 127-130: The shell task using curl piped to sh does not enable
pipefail, which means if curl fails to download the installation script, the
failure will be masked and the task will appear successful. Modify the shell
task by adding an executable parameter to use bash instead of the default POSIX
sh, and add set -o pipefail to the shell command itself (or use the args option
to set it) so that if curl fails, the entire pipeline fails and the task
properly reports the error. This ensures the system does not enter an
inconsistent state when the uv installation fails.
In `@tests/api_tests/test_indexer.py`:
- Around line 787-799: The final assertion in the polling loop uses strict
equality `assert cancelled_after == cancelled_before + 1` which is overly strict
for an asynchronous shared counter. The loop correctly waits for the counter to
increment by at least one using `>=`, but the final assertion should also use
`>=` instead of `==` to account for the possibility that another cancellation
increment might occur during the same polling window. Change the final assertion
to check that `cancelled_after >= cancelled_before + 1` to prevent test
flakiness.
---
Outside diff comments:
In `@openrag/routers/indexer.py`:
- Around line 410-425: The direct await of indexer.copy_file.remote() bypasses
the standardized timeout and error handling mechanisms. Replace the direct await
call to indexer.copy_file.remote() with a call to the centralized
call_ray_actor_with_timeout() utility function from
openrag/components/ray_utils, passing the appropriate parameters (actor method,
timeout value, and any required arguments) to ensure consistent timeout
management, cancellation handling, and error management across all Ray actor
calls.
🪄 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: Pro
Run ID: e3c12dcf-5822-4117-838b-6431f0c35795
📒 Files selected for processing (13)
ansible/ansible.cfgansible/playbooks/openrag.ymldocs/assets/compose_linux_gpu.yamlopenrag/components/auth/oidc_client.pyopenrag/components/indexer/loaders/CustomDocLoader.pyopenrag/components/indexer/utils/text_sanitizer.pyopenrag/components/indexer/vectordb/utils.pyopenrag/models/user.pyopenrag/routers/auth.pyopenrag/routers/indexer.pyopenrag/routers/openai.pyopenrag/routers/search.pytests/api_tests/test_indexer.py
b911d68 to
9a73200
Compare
|
CodeRabbit review addressed:
|
1 participant
What this does
Fixes a batch of small, self-contained issues found during the recent open-issue triage. Each change is under 20 lines. Grouped by area below.
Auth and OIDC
nextURL, so/\evil.comand header-injection payloads can't slip past the open-redirect guard. (OIDC next URL accepts backslash hosts and CRLF injection #360, [security] Open redirect via backslash in OIDCnextparameter #483)/userinfosubto match the verified ID-tokensubbefore applying claim mapping. (OIDC userinfo claims accepted without sub binding to ID token #386)azp == client_idon multi-audience ID and logout tokens. ([security] OIDC: missingazpcheck on multi-audience ID tokens #485)AUTH_TOKENis unset; keep the stored one. (Admin token rotates on every startup when AUTH_TOKEN is unset #361)revoke_oidc_sessions_by_user_id. (partial [security] OIDC sessions not revoked on user token regeneration / de-admin #486)Indexing and loaders
external_user_idto NULL so it can't collide on the unique index. (Empty string in external_user_id causes uniqueness constraint violation #121)API and deploy
source_file_idin the copy endpoint, and bind the Ray dashboard to localhost in the last compose file that still exposed it. (security follow-ups: unvalidated source_file_id in copy endpoint + Ray dashboard exposed in 3 compose files #477).envat 0600, pinned uv installer. ([security] Ansible deploy hardening: host key checking disabled, world-readable .env, unpinned installer #488)Tests
test_cancel_increments_total_cancelledby polling/queue/infoinstead of reading the counter right after the delete. (Flaky API test: test_cancel_increments_total_cancelled races the async counter update #426)Testing
uv run pytest openrag/→ 386 passed, 5 skippeduv run ruff check openrag/andruff format --check openrag/→ cleanFixes #360, fixes #483, fixes #386, fixes #485, fixes #361, fixes #376, fixes #121, fixes #477, fixes #487, fixes #363, fixes #481, fixes #488, fixes #426.