security: address remaining GHSA advisory findings (C3, H2–H8, M2–M13, N3–N13, deps) - #478
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (36)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (31)
📝 WalkthroughWalkthroughThis PR is a broad security hardening release (v1.1.11) spanning infrastructure and application layers: containers run as non-root user 10001, all images are pinned to 1.1.11, exposed ports bind to localhost only, hardcoded credentials are replaced with required env vars, a shared SSRF guard utility is introduced, document loaders gain parser-bomb caps, a tiered rate-limiting middleware is added, OIDC auth gains clock-skew tolerance and jti replay detection, and a new ChangesOpenRAG 1.1.11 Security Hardening
Sequence Diagram(s)sequenceDiagram
participant Client
participant RateLimitMiddleware
participant AuthMiddleware
participant FastAPIRouter
participant RateLimiter as MovingWindowRateLimiter
Client->>RateLimitMiddleware: POST /v1/chat/completions
RateLimitMiddleware->>RateLimitMiddleware: _limit_for("/v1/...") → RATE_LIMIT_CHAT
RateLimitMiddleware->>RateLimitMiddleware: _identity(request) → user_id or IP
RateLimitMiddleware->>RateLimiter: hit(limit, identity)
alt budget available
RateLimitMiddleware->>AuthMiddleware: call_next(request)
AuthMiddleware->>FastAPIRouter: authenticated request
FastAPIRouter-->>Client: 200 response
else budget exceeded
RateLimitMiddleware-->>Client: 429 Too Many Requests + Retry-After
end
sequenceDiagram
participant IdP as Identity Provider
participant BackchannelEndpoint as POST /auth/backchannel-logout
participant OIDCClient
participant JtiCache as jti_seen_cache
participant SessionStore
IdP->>BackchannelEndpoint: logout_token JWT
BackchannelEndpoint->>OIDCClient: verify_logout_token(token)
OIDCClient->>OIDCClient: require exp, validate exp±60s leeway, require jti, validate nbf
OIDCClient-->>BackchannelEndpoint: LogoutTokenClaims(jti, exp, sub, sid)
BackchannelEndpoint->>JtiCache: prune expired entries
BackchannelEndpoint->>JtiCache: is_jti_seen(jti)?
alt replay detected
BackchannelEndpoint-->>IdP: 400 invalid_request
else first occurrence
BackchannelEndpoint->>JtiCache: record jti → exp
BackchannelEndpoint->>SessionStore: revoke session
BackchannelEndpoint-->>IdP: 200 OK
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/assets/compose_ollama_cpu.yaml (1)
9-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRay dashboard exposed without localhost binding.
Line 10 exposes port 8265 to all interfaces (
8265:8265), but the maindocker-compose.yamlcorrectly binds to localhost only (127.0.0.1:${RAY_DASHBOARD_PORT:-8265}:8265). The Ray dashboard/Jobs API is unauthenticated (CVE-2023-48022) and allows arbitrary code execution, so this port should be bound to localhost here as well for consistency.Proposed fix
ports: - 8090:8080 - - 8265:8265 # Disable when in cluster mode + - 127.0.0.1:8265:8265 # Disable when in cluster mode; unauthenticated (CVE-2023-48022)🤖 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 `@docs/assets/compose_ollama_cpu.yaml` around lines 9 - 10, In the compose_ollama_cpu.yaml file, the Ray dashboard port 8265 is exposed to all interfaces without localhost binding, creating a security vulnerability since the Ray dashboard is unauthenticated and allows arbitrary code execution. Change the port mapping on line 10 from `- 8265:8265` to bind to localhost only using the format `- 127.0.0.1:${RAY_DASHBOARD_PORT:-8265}:8265` to match the secure configuration used in the main docker-compose.yaml file.openrag/app_front.py (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix formatting to pass CI.
The pipeline reports that
ruff format --checkfailed for this file. Runuv run ruff format openrag/to fix.🤖 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/app_front.py` at line 1, The file openrag/app_front.py does not conform to the ruff formatting standards as indicated by the failed CI check. Run the command `uv run ruff format openrag/` to automatically fix all formatting issues in the openrag directory, which will apply the required formatting to app_front.py and ensure the CI check passes.Source: Pipeline failures
openrag/components/indexer/loaders/pdf_loaders/marker.py (1)
283-285:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSingle-chunk fast path bypasses
max_pdf_pages.When
len(chunks) == 1, the call usespage_range=None, which processes all pages and can ignore the computed cap.Suggested fix
if len(chunks) == 1: page_range, label = chunks[0] - return await self._process_chunk(file_path, page_range=None, label=label) + return await self._process_chunk(file_path, page_range=page_range, label=label)🤖 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/components/indexer/loaders/pdf_loaders/marker.py` around lines 283 - 285, The single-chunk fast path in the conditional block where len(chunks) == 1 is extracting the page_range from chunks[0] but then passing page_range=None to the _process_chunk method call, which bypasses the max_pdf_pages constraint. Instead of passing page_range=None, pass the extracted page_range variable that was retrieved from chunks[0] to ensure the computed page limit is respected.
🧹 Nitpick comments (2)
pyproject.toml (1)
58-58: Remove unusedslowapidependency; depend directly onlimitsinstead.The rate limiting middleware imports directly from the
limitslibrary (from limits import parse,from limits.aio.storage import MemoryStorage, etc.) but declaresslowapias a dependency. Sinceslowapiis only a FastAPI/Starlette ASGI integration wrapper aroundlimitsand is not used anywhere in the codebase, it's cleaner to declare the actual library you depend on.♻️ Proposed fix
- "slowapi>=0.1.9", + "limits>=3.0",🤖 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 `@pyproject.toml` at line 58, The project declares slowapi as a dependency in pyproject.toml at line 58, but the codebase only imports directly from the limits library (via imports like from limits import parse and from limits.aio.storage import MemoryStorage). Since slowapi is merely a FastAPI/Starlette wrapper around limits and is not actually used in the code, remove the slowapi>=0.1.9 dependency line from pyproject.toml and instead add limits as a direct dependency to accurately reflect what the project actually depends on.openrag/components/indexer/loaders/base.py (1)
38-41: ⚡ Quick winConsider streaming to enforce the size cap before full download.
The size check on line 122 happens after
resp.contenthas already downloaded the entire response body. A malicious server could still force the client to buffer a large payload before rejection.For stronger protection, consider using
resp.aiter_bytes()with an accumulator that raises once the cap is exceeded:♻️ Suggested streaming approach
- data = resp.content - if len(data) > self.MAX_REMOTE_IMAGE_BYTES: - logger.warning("Remote image exceeds size cap", url=url, size=len(data)) - return None + chunks = [] + total = 0 + async for chunk in resp.aiter_bytes(): + total += len(chunk) + if total > self.MAX_REMOTE_IMAGE_BYTES: + logger.warning("Remote image exceeds size cap", url=url, size=total) + return None + chunks.append(chunk) + data = b"".join(chunks)That said, the current implementation still provides meaningful protection against unbounded memory growth, so this is a minor improvement.
Also applies to: 94-130
🤖 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/components/indexer/loaders/base.py` around lines 38 - 41, The size check for remote images currently happens after the entire response body has already been downloaded via resp.content, which allows a malicious server to force buffering of a large payload before rejection. To fix this, refactor the image fetching logic to use streaming with resp.aiter_bytes() and an accumulator that tracks bytes downloaded and raises an exception immediately once the MAX_REMOTE_IMAGE_BYTES limit is exceeded, ensuring the cap is enforced during the download rather than after the full response is buffered.
🤖 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 `@entrypoint.sh`:
- Line 19: Unquoted environment variable expansions in shell commands can break
when variables contain whitespace or special characters. In entrypoint.sh at
line 9, quote the $ENV_ARG variable expansion in the command uv run $ENV_ARG
api.py by wrapping it as "$ENV_ARG". Similarly at line 19, quote all unquoted
variable expansions in the uvicorn command: wrap $ENV_ARG as "$ENV_ARG", wrap
$RELOAD_ARG as "$RELOAD_ARG", wrap ${APP_iPORT:-8080} as "${APP_iPORT:-8080}",
and wrap ${API_NUM_WORKERS:-1} as "${API_NUM_WORKERS:-1}" to prevent argv
splitting when these variables are expanded.
In `@openrag/components/indexer/loaders/docx.py`:
- Around line 145-150: The code at images[pos - 1] = img is vulnerable to
invalid indexing because pos comes from untrusted filename-derived data and
could be zero or negative. Add validation to ensure each pos value from the
by_order dictionary is positive (greater than 0) before using it to index into
the images array. Check this condition within the for loop that iterates through
by_order.items() and skip or handle invalid positions appropriately to prevent
index errors that would abort DOCX processing.
In `@openrag/components/indexer/loaders/eml_loader.py`:
- Around line 73-78: Move the attachment cap check (comparing
len(email_data["attachment"]) against self.max_attachments) to occur before the
part.get_payload(decode=True) call, not after. Currently the payload is fully
decoded before checking if the attachment count has reached
self.max_attachments, which means the decoding cost is still incurred for
attachments that will be discarded. Reorganize the condition structure so that
if the max_attachments cap has already been reached, the continue statement
skips the payload decoding entirely.
In `@openrag/components/rate_limit.py`:
- Around line 66-75: The _identity method uses getattr(user, "id", None) to
extract the user ID, but request.state.user is set as a dictionary by
AuthMiddleware, not an object with attributes. Using getattr on a dict will
always return None, causing all authenticated users to fall back to IP-based
rate limiting. Replace the attribute access with dictionary access by using
user.get("id") instead of getattr(user, "id", None) to properly retrieve the
user ID from the dict.
---
Outside diff comments:
In `@docs/assets/compose_ollama_cpu.yaml`:
- Around line 9-10: In the compose_ollama_cpu.yaml file, the Ray dashboard port
8265 is exposed to all interfaces without localhost binding, creating a security
vulnerability since the Ray dashboard is unauthenticated and allows arbitrary
code execution. Change the port mapping on line 10 from `- 8265:8265` to bind to
localhost only using the format `- 127.0.0.1:${RAY_DASHBOARD_PORT:-8265}:8265`
to match the secure configuration used in the main docker-compose.yaml file.
In `@openrag/app_front.py`:
- Line 1: The file openrag/app_front.py does not conform to the ruff formatting
standards as indicated by the failed CI check. Run the command `uv run ruff
format openrag/` to automatically fix all formatting issues in the openrag
directory, which will apply the required formatting to app_front.py and ensure
the CI check passes.
In `@openrag/components/indexer/loaders/pdf_loaders/marker.py`:
- Around line 283-285: The single-chunk fast path in the conditional block where
len(chunks) == 1 is extracting the page_range from chunks[0] but then passing
page_range=None to the _process_chunk method call, which bypasses the
max_pdf_pages constraint. Instead of passing page_range=None, pass the extracted
page_range variable that was retrieved from chunks[0] to ensure the computed
page limit is respected.
---
Nitpick comments:
In `@openrag/components/indexer/loaders/base.py`:
- Around line 38-41: The size check for remote images currently happens after
the entire response body has already been downloaded via resp.content, which
allows a malicious server to force buffering of a large payload before
rejection. To fix this, refactor the image fetching logic to use streaming with
resp.aiter_bytes() and an accumulator that tracks bytes downloaded and raises an
exception immediately once the MAX_REMOTE_IMAGE_BYTES limit is exceeded,
ensuring the cap is enforced during the download rather than after the full
response is buffered.
In `@pyproject.toml`:
- Line 58: The project declares slowapi as a dependency in pyproject.toml at
line 58, but the codebase only imports directly from the limits library (via
imports like from limits import parse and from limits.aio.storage import
MemoryStorage). Since slowapi is merely a FastAPI/Starlette wrapper around
limits and is not actually used in the code, remove the slowapi>=0.1.9
dependency line from pyproject.toml and instead add limits as a direct
dependency to accurately reflect what the project actually depends on.
🪄 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: 33d899ba-41b6-4ace-ae3a-21b2a6e613af
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
.env.exampleDockerfileDockerfile.raycharts/openrag-stack/templates/networkpolicy.yamlcharts/openrag-stack/values.yamlcluster.yamlconf/config.yamldocker-compose.yamldocs/assets/compose_ollama_cpu.yamlentrypoint.shopenrag/api.pyopenrag/app_front.pyopenrag/components/auth/oidc_client.pyopenrag/components/auth/test_oidc_client.pyopenrag/components/indexer/loaders/base.pyopenrag/components/indexer/loaders/docx.pyopenrag/components/indexer/loaders/eml_loader.pyopenrag/components/indexer/loaders/image.pyopenrag/components/indexer/loaders/pdf_loaders/marker.pyopenrag/components/indexer/loaders/pptx_loader.pyopenrag/components/indexer/utils/test_text_sanitizer.pyopenrag/components/indexer/utils/text_sanitizer.pyopenrag/components/indexer/vectordb/vectordb.pyopenrag/components/pipeline.pyopenrag/components/rate_limit.pyopenrag/components/test_rate_limit.pyopenrag/components/utils.pyopenrag/components/websearch/content_fetcher.pyopenrag/components/websearch/service.pyopenrag/components/websearch/test_content_fetcher.pyopenrag/models/openai.pyopenrag/routers/auth.pyopenrag/routers/indexer.pyopenrag/routers/openai.pyopenrag/routers/partition.pyopenrag/routers/test_auth_router.pyopenrag/utils/ssrf.pyopenrag_metrics/docker-compose.yamlpyproject.tomlquick_start/docker-compose.yamlquick_start/vdb/milvus.yamltests/api_tests/api_run/docker-compose.yamlvdb/milvus.yaml
💤 Files with no reviewable changes (1)
- openrag/components/pipeline.py
EnjoyBacon7
force-pushed
the
security/advisory-hardening
branch
2 times, most recently
from
June 15, 2026 12:57
43732b0 to
a7b44f3
Compare
Collaborator
Author
|
Thanks for the review — all four findings addressed and folded into their originating commits:
Also fixed the failing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@entrypoint.sh`:
- Around line 15-19: When UVICORN_RELOAD is set to "true", the script enables
reload mode on line 17 but then line 19 still forwards the --workers argument
which may be greater than 1. Since uvicorn cannot use reload mode with multiple
workers, modify the logic to enforce single-worker mode when reload is enabled.
If RELOAD_ARG is set to "--reload", override the API_NUM_WORKERS setting to
force --workers 1 regardless of the environment variable value, while still
respecting the API_NUM_WORKERS value when reload is not enabled.
🪄 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: 1135e875-caa7-478c-a8aa-d52c3a3fb82a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
.env.exampleDockerfileDockerfile.raycharts/openrag-stack/templates/networkpolicy.yamlcharts/openrag-stack/values.yamlcluster.yamlconf/config.yamldocker-compose.yamldocs/assets/compose_ollama_cpu.yamlentrypoint.shopenrag/api.pyopenrag/app_front.pyopenrag/components/auth/oidc_client.pyopenrag/components/auth/test_oidc_client.pyopenrag/components/indexer/loaders/base.pyopenrag/components/indexer/loaders/docx.pyopenrag/components/indexer/loaders/eml_loader.pyopenrag/components/indexer/loaders/image.pyopenrag/components/indexer/loaders/pdf_loaders/marker.pyopenrag/components/indexer/loaders/pptx_loader.pyopenrag/components/indexer/utils/test_text_sanitizer.pyopenrag/components/indexer/utils/text_sanitizer.pyopenrag/components/indexer/vectordb/vectordb.pyopenrag/components/pipeline.pyopenrag/components/rate_limit.pyopenrag/components/test_rate_limit.pyopenrag/components/utils.pyopenrag/components/websearch/content_fetcher.pyopenrag/components/websearch/service.pyopenrag/components/websearch/test_content_fetcher.pyopenrag/models/openai.pyopenrag/routers/auth.pyopenrag/routers/indexer.pyopenrag/routers/openai.pyopenrag/routers/partition.pyopenrag/routers/test_auth_router.pyopenrag/utils/ssrf.pyopenrag_metrics/docker-compose.yamlpyproject.tomlquick_start/docker-compose.yamlquick_start/vdb/milvus.yamltests/api_tests/api_run/docker-compose.yamlvdb/milvus.yaml
💤 Files with no reviewable changes (1)
- openrag/components/pipeline.py
✅ Files skipped from review due to trivial changes (1)
- openrag/components/websearch/service.py
🚧 Files skipped from review as they are similar to previous changes (37)
- pyproject.toml
- openrag/routers/openai.py
- vdb/milvus.yaml
- openrag/components/websearch/content_fetcher.py
- quick_start/vdb/milvus.yaml
- openrag/components/auth/test_oidc_client.py
- openrag/components/indexer/loaders/image.py
- openrag/routers/test_auth_router.py
- openrag/components/indexer/utils/text_sanitizer.py
- quick_start/docker-compose.yaml
- openrag_metrics/docker-compose.yaml
- Dockerfile.ray
- cluster.yaml
- openrag/routers/partition.py
- openrag/components/indexer/vectordb/vectordb.py
- openrag/models/openai.py
- openrag/components/indexer/loaders/docx.py
- openrag/components/auth/oidc_client.py
- openrag/app_front.py
- conf/config.yaml
- openrag/components/indexer/loaders/pptx_loader.py
- openrag/components/indexer/loaders/pdf_loaders/marker.py
- docs/assets/compose_ollama_cpu.yaml
- .env.example
- openrag/components/indexer/loaders/eml_loader.py
- openrag/components/indexer/utils/test_text_sanitizer.py
- openrag/components/utils.py
- docker-compose.yaml
- openrag/routers/indexer.py
- openrag/routers/auth.py
- Dockerfile
- charts/openrag-stack/values.yaml
- openrag/components/test_rate_limit.py
- openrag/api.py
- openrag/components/indexer/loaders/base.py
- openrag/components/rate_limit.py
- openrag/utils/ssrf.py
The Ray dashboard and co-hosted Jobs API are unauthenticated (CVE-2023-48022 "ShadowRay"), so exposing them on a routable interface is an unauthenticated-RCE vector. Bind to 127.0.0.1 by default everywhere it is configured, overridable via RAY_DASHBOARD_HOST: - openrag/api.py: ray.init reads RAY_DASHBOARD_HOST (default 127.0.0.1) - quick_start/docker-compose.yaml: publish 127.0.0.1:8265 only - cluster.yaml: --dashboard-host ${RAY_DASHBOARD_HOST:-127.0.0.1} The KubeRay pod (charts/.../raycluster.yaml) keeps 0.0.0.0 because the dashboard Service requires in-pod reachability; it is instead isolated via the NetworkPolicy added separately (N12).
A document containing `` previously made the server/VLM fetch an attacker-chosen URL while captioning, exposing internal services (cloud metadata, RFC1918 hosts, etc.). - Extract the websearch SSRF guard into utils/ssrf.py (literal pre-check + resolve-then-validate httpx request hook) and reuse it in both places. - loaders/base.py now fetches remote image URLs in-process with the guard, refuses redirects, enforces an image/* content-type and a 20 MB size cap, and passes only a data URI to the VLM (the VLM never sees the URL). - Default loader.image_captioning_url to false.
…ault (H3) Hardcoded minioadmin:minioadmin in the Milvus stacks meant any foothold on the internal network yielded full object-store access. Require operators to supply MINIO_ACCESS_KEY / MINIO_SECRET_KEY (no default — compose fails fast if unset) in both vdb/milvus.yaml and quick_start/vdb/milvus.yaml, and pass the same credentials to the Milvus container (MINIO_ACCESS_KEY_ID / MINIO_SECRET_ACCESS_KEY) so it no longer relies on the minioadmin default. Document the new required vars in .env.example.
Both images ran as uid 0, so any RCE in the stack (e.g. via the Ray dashboard) executed as root. Add a dedicated uid/gid 10001 "app" user to Dockerfile and Dockerfile.ray, own /app and the writable data/logs/model_weights dirs, and drop to that user with USER 10001:10001. HOME is set to /app before the uv python install so the pinned interpreter and caches land under the user-owned tree (no runtime re-download). NOTE: the image build should be smoke-tested (build + ingest a document) before release; bind-mounted host volumes may need matching ownership.
Previously an unset CHAINLIT_AUTH_SECRET silently fell back to the public constant "default_secret_for_openrag_ui", letting anyone forge Chainlit UI session cookies. Refuse to start unless the secret is set; the insecure built-in default is only used with the explicit dev opt-in ALLOW_NO_AUTH=true (the same flag that gates the no-auth backend bypass), with a loud warning. Document CHAINLIT_AUTH_SECRET in .env.example.
Retrieved document chunks (and web results) were concatenated into the system prompt verbatim, so a poisoned document could embed "[Source N]" blocks, a "[Sources: 1, 2]" citation tag, or the "----------" separator to forge displayed citations or fake source boundaries / inject instructions. Add neutralize_prompt_control_tokens() to the text sanitizer and apply it to every chunk in format_context and to web title/body in format_web_context. It defangs the bracketed [Source.../[Sources... markers, the unbracketed line-terminal "Sources: N" form the answer parser also accepts, and long hyphen runs — so those tokens can only originate from our own formatter.
Shipped defaults (root_password, POSTGRES_PASSWORD=root, AUTH_TOKEN=OpenRAG,
minioadmin) are usable credentials on any exposed deployment.
- docker-compose.yaml / quick_start: require ${POSTGRES_PASSWORD:?} (fail fast)
- conf/config.yaml: drop the "root_password" default; password must come from
the POSTGRES_PASSWORD env var
- docs/assets/compose_ollama_cpu.yaml: require AUTH_TOKEN, POSTGRES_PASSWORD
and MinIO credentials via env instead of the weak literals
- document POSTGRES_PASSWORD in .env.example
The Helm chart's Postgres password is addressed in the N7 commit (moved to a
Secret) alongside the rest of the chart hardening.
No endpoint had any rate limiting, leaving auth and inference surfaces open to brute-force and resource-exhaustion abuse. Add RateLimitMiddleware (backed by the `limits` moving-window strategy) keyed on the authenticated user id, falling back to the client IP for unauthenticated/bypass paths. Tiered limits: /auth/* (20/min), /v1/* (60/min), everything else (300/min); all configurable via RATE_LIMIT_* env vars and disableable with RATE_LIMIT_ENABLED=false. Registered before AuthMiddleware so it runs after user state is populated. Disabled in the API-test compose (the suite issues request bursts); the limiter is unit-tested in components/test_rate_limit.py.
- /indexer file upload: a failed save returned str(e) (filesystem paths,
internals) to the caller. Log the detail server-side and return a generic
"Failed to save uploaded file." message instead.
- GET /indexer/task/{id}/error: the raw traceback (paths, internals) was
returned to any task owner. Gate the traceback behind is_admin; non-admin
owners get a generic failure indicator.
Small malicious documents could exhaust CPU/memory during ingestion. Add configurable caps (conf/config.yaml loader.*): - EML: cap attachments processed per email (max_attachments) and stop nested .eml-in-.eml recursion past max_eml_depth (the .eml loader recurses into itself), threading depth through sub-loader kwargs. - DOCX: cap embedded media entries iterated (max_archive_entries) and per-entry decompressed size; replace the `[None] * max_order` allocation (max_order came from the attacker-controlled media filename → memory bomb) with a size-guarded reconstruction. - PPTX: cap slides processed and images decoded into memory. - PDF (marker): cap pages processed per file (max_pdf_pages).
…out (M9) OIDC back-channel logout tokens are short-lived single-use tokens, but the verifier defaulted a missing exp to "now + 1" (so an absent exp never expired) and ignored jti entirely. - verify_logout_token now requires exp (and rejects expired tokens) and requires jti, both per the OIDC back-channel logout spec; exp is surfaced on LogoutTokenClaims. - The /auth/backchannel-logout handler records consumed jti -> exp in a pruned in-process cache and rejects replays (defence-in-depth; logout is idempotent so this complements rather than replaces the DB revocation). Updated/extended oidc_client tests (default token now carries exp; added missing-exp, expired, and missing-jti cases).
…M10) DEBUG was the default level and user query text was logged (including at WARNING level, which survives an INFO default) and persisted to a long-lived JSON sink — leaking potentially sensitive request content. - conf/config.yaml + .env.example: default log level INFO. - pipeline.py: drop the query string from the temporal-filter-dropped warning. - websearch/service.py: drop the query string from the zero-results warning. Remaining query logging is at DEBUG and is now off by default.
- The input token-limit check (check_tokens_limit) only ran for direct-LLM requests, so RAG-mode requests could submit unbounded message/prompt sizes. Run it in both modes for /chat/completions and /completions (RAG context is added server-side and separately capped). - best_of and n on the legacy completion request were unbounded ints; each multiplies generation cost. Constrain both to 1..8 (422 on violation).
EnjoyBacon7
force-pushed
the
security/advisory-hardening
branch
from
June 15, 2026 13:13
a7b44f3 to
72e0157
Compare
Collaborator
Author
|
Follow-up: addressed the remaining CodeRabbit findings from the review body (outside-diff + nitpicks), folded into the originating commits:
Lint, unit suite (303 passed), and imports all pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
entrypoint.sh (1)
15-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForce single-worker mode when reload is enabled.
Line 19 always forwards
--workers "${API_NUM_WORKERS:-1}". WithUVICORN_RELOAD=trueand workers > 1, uvicorn startup fails.Proposed fix
RELOAD_ARGS=() + WORKERS="${API_NUM_WORKERS:-1}" if [[ "${UVICORN_RELOAD}" == "true" ]]; then RELOAD_ARGS+=("--reload") + WORKERS="1" fi - uv run --no-dev "${ENV_ARGS[@]}" uvicorn api:app --host 0.0.0.0 --port "${APP_iPORT:-8080}" "${RELOAD_ARGS[@]}" --workers "${API_NUM_WORKERS:-1}" + uv run --no-dev "${ENV_ARGS[@]}" uvicorn api:app --host 0.0.0.0 --port "${APP_iPORT:-8080}" "${RELOAD_ARGS[@]}" --workers "${WORKERS}"🤖 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 `@entrypoint.sh` around lines 15 - 19, The uvicorn command on line 19 always sets the workers count to the API_NUM_WORKERS value, but uvicorn cannot use reload mode with multiple workers. Similar to how RELOAD_ARGS is conditionally built in lines 16-18, create a WORKERS_ARGS variable that is set to 1 when UVICORN_RELOAD is true, otherwise use the API_NUM_WORKERS environment variable (defaulting to 1). Then replace the inline --workers argument in the uvicorn command with the WORKERS_ARGS variable to ensure single-worker mode when reload is enabled.
🤖 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 `@openrag/routers/partition.py`:
- Around line 238-253: There is a race condition in the partition creation logic
where the partition limit check (based on counting owned partitions in the
snapshot `request.state.user_partitions`) happens separately from the actual
creation call to `vectordb.create_partition.remote()`. Concurrent requests can
all observe the same owned count below the limit and bypass the check. Fix this
by consolidating the limit enforcement and partition creation into a single
atomic operation: either move the partition limit check (the condition checking
if owned >= max_per_user) into the vectordb create path as part of a database
transaction, or create a single actor method in vectordb that performs both the
limit verification and the partition creation atomically, replacing the separate
check-then-create calls with a single atomic call.
---
Duplicate comments:
In `@entrypoint.sh`:
- Around line 15-19: The uvicorn command on line 19 always sets the workers
count to the API_NUM_WORKERS value, but uvicorn cannot use reload mode with
multiple workers. Similar to how RELOAD_ARGS is conditionally built in lines
16-18, create a WORKERS_ARGS variable that is set to 1 when UVICORN_RELOAD is
true, otherwise use the API_NUM_WORKERS environment variable (defaulting to 1).
Then replace the inline --workers argument in the uvicorn command with the
WORKERS_ARGS variable to ensure single-worker mode when reload is enabled.
🪄 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: f2a0cad5-50b7-4cb5-92e8-7bb3c828193d
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
.env.exampleDockerfileDockerfile.raycharts/openrag-stack/templates/networkpolicy.yamlcharts/openrag-stack/values.yamlcluster.yamlconf/config.yamldocker-compose.yamldocs/assets/compose_ollama_cpu.yamlentrypoint.shopenrag/api.pyopenrag/app_front.pyopenrag/components/auth/oidc_client.pyopenrag/components/auth/test_oidc_client.pyopenrag/components/indexer/loaders/base.pyopenrag/components/indexer/loaders/docx.pyopenrag/components/indexer/loaders/eml_loader.pyopenrag/components/indexer/loaders/image.pyopenrag/components/indexer/loaders/pdf_loaders/marker.pyopenrag/components/indexer/loaders/pptx_loader.pyopenrag/components/indexer/utils/test_text_sanitizer.pyopenrag/components/indexer/utils/text_sanitizer.pyopenrag/components/indexer/vectordb/vectordb.pyopenrag/components/pipeline.pyopenrag/components/rate_limit.pyopenrag/components/test_rate_limit.pyopenrag/components/utils.pyopenrag/components/websearch/content_fetcher.pyopenrag/components/websearch/service.pyopenrag/components/websearch/test_content_fetcher.pyopenrag/models/openai.pyopenrag/routers/auth.pyopenrag/routers/indexer.pyopenrag/routers/openai.pyopenrag/routers/partition.pyopenrag/routers/test_auth_router.pyopenrag/utils/ssrf.pyopenrag_metrics/docker-compose.yamlpyproject.tomlquick_start/docker-compose.yamlquick_start/vdb/milvus.yamltests/api_tests/api_run/docker-compose.yamlvdb/milvus.yaml
💤 Files with no reviewable changes (1)
- openrag/components/pipeline.py
🚧 Files skipped from review as they are similar to previous changes (31)
- openrag/api.py
- cluster.yaml
- tests/api_tests/api_run/docker-compose.yaml
- quick_start/docker-compose.yaml
- vdb/milvus.yaml
- openrag/routers/openai.py
- docker-compose.yaml
- openrag/components/utils.py
- openrag/routers/test_auth_router.py
- openrag/components/indexer/loaders/eml_loader.py
- openrag/routers/auth.py
- .env.example
- openrag/components/indexer/loaders/docx.py
- openrag/components/indexer/utils/test_text_sanitizer.py
- openrag/routers/indexer.py
- openrag/components/indexer/utils/text_sanitizer.py
- Dockerfile
- openrag/components/indexer/vectordb/vectordb.py
- openrag/components/websearch/content_fetcher.py
- openrag/models/openai.py
- Dockerfile.ray
- openrag/components/auth/test_oidc_client.py
- openrag/app_front.py
- openrag/components/indexer/loaders/base.py
- openrag_metrics/docker-compose.yaml
- conf/config.yaml
- openrag/components/rate_limit.py
- openrag/components/auth/oidc_client.py
- openrag/components/indexer/loaders/pptx_loader.py
- openrag/utils/ssrf.py
- charts/openrag-stack/values.yaml
create_partition had no authorization or quota, so any authenticated user could create unbounded partitions and exhaust storage/metadata. Enforce a per-user owned-partition cap (MAX_PARTITIONS_PER_USER, default 100, -1 to disable); admins bypass. Owned count is derived from the user's existing partition memberships already on request.state, so no extra DB round-trip.
GET /auth/logout is state-changing (revokes the session) and cookie-authed, so a forged request (e.g. <img src=".../auth/logout">) could force-logout a user. It must stay a GET to support the OIDC RP-initiated logout redirect, so add a Fetch-Metadata guard that rejects cross-site non-navigation requests (the silent CSRF vector) while allowing genuine top-level navigations and same-origin calls.
… (N6) get_surrounding_chunks queried neighbouring chunks by section_id alone. Since section_id is only unique within a partition, a prev/next id could resolve to another tenant's chunk, leaking it into results. Pair each section_id with its source doc's partition and add `and partition == '<partition>'` to the query; drop refs that have no partition rather than querying unscoped.
…, M2) The Postgres password was rendered into the world-readable ConfigMap (env.config) and defaulted to root_password. Move POSTGRES_PASSWORD to env.secrets (rendered into the Opaque Secret, which the deployment already mounts via secretRef) and replace the default with an obvious placeholder that must be overridden at install time.
…k8s)
The chart shipped no NetworkPolicy, so any pod in the cluster could reach the
unauthenticated Ray dashboard (8265), GCS (6379), Ray client (10001), Postgres
and Milvus. Add a default-deny-ingress NetworkPolicy (podSelector: {}) that
allows only intra-namespace traffic plus the configurable public HTTP ports
(8080, 3000). Gated by networkPolicy.enabled (default true). This is the
in-cluster isolation referenced by the C3 fix for the KubeRay dashboard.
The Milvus services disabled syscall filtering entirely (security_opt: seccomp:unconfined), widening container-escape surface. Remove the override so the default seccomp profile applies in vdb/milvus.yaml and quick_start.
The production docker-compose bind-mounted ./openrag over the image and the entrypoint always ran uvicorn with --reload (a dev feature, also incompatible with multiple workers). Both let host-side changes override the running code. - Comment out the ./openrag dev bind-mount (uncomment for local dev). - Gate --reload behind UVICORN_RELOAD=true (default off).
The metrics compose exposed Prometheus and exporters on 0.0.0.0 and enabled Prometheus' unauthenticated lifecycle endpoints. - Prometheus: bind 127.0.0.1:9090 and drop --web.enable-lifecycle (unauthenticated /-/reload and /-/quit). - node-exporter (host pid + rootfs) and nvidia-gpu-exporter: bind to 127.0.0.1. Prometheus scrapes them over the compose network by service name, so localhost binding doesn't affect scraping.
Source file links embedded the credential as ?token=<api_key>, leaking it to browser history, proxy logs and Referer headers. In OIDC mode the browser already sends the openrag_session cookie on same-origin file fetches (the auth middleware checks the cookie first for /static), so the token query param is redundant — drop it there. Token mode keeps it (no cookie exists); the backend already redacts ?token= from its own access logs.
…cation (crypto) ID-token (and logout-token) exp was checked with zero tolerance and nbf was not validated at all. Add a 60s clock-skew leeway to the exp checks and honour nbf (reject tokens whose not-before is in the future beyond the leeway). Adjusted the expired-logout-token test to exceed the leeway.
Tracking :latest makes deploys non-reproducible and silently pulls unreviewed images. Pin the OpenRAG-owned images to the release version and the metrics stack to specific stable versions: - docker-compose.yaml, quick_start: openrag & indexer-ui -> :1.1.11 - charts: openrag, openrag-ray, indexer-ui -> 1.1.11 - metrics: prometheus v2.54.1, grafana 11.2.2, node-exporter v1.8.2 Operator-supplied model-serving images (vLLM engines, infinity reranker) carry a comment to pin to a specific release/digest before production; digests are the ideal target but can't be resolved offline here.
Starlette 0.46.2 is vulnerable to the multipart upload DoS CVE-2025-54121 (fixed in 0.47.2). Constrain starlette>=0.47.2,<0.48 and the coordinated fastapi>=0.116.1,<0.117 (0.115.x pinned starlette<0.47). To satisfy the new Starlette line the resolver also bumps chainlit 2.6.2 -> 2.11.1 (it capped starlette<0.47); the chainlit-based UI should be smoke-tested before release. Pillow could NOT be bumped to >=11.3.0 (CVE-2025-48379): marker-pdf (all versions in range) caps pillow<11.0.0, so the upgrade is blocked upstream until marker-pdf relaxes that pin. All 301 component unit tests pass; app_front and routers import cleanly under the new versions.
…/cairosvg) Untrusted SVGs are rasterized via cairosvg before captioning. cairosvg's default unsafe=False already routes resource loading through safe_fetch and sets forbid_external/forbid_entities (blocking SSRF and XXE), but the call relied on that implicit default. Pass unsafe=False explicitly to document and lock in the guarantee. spire-doc (closed-source binary parsing untrusted .doc) is noted in the advisory for sandboxing/replacement; that is a larger change left as a follow-up — no safe drop-in is available here.
verify_logout_token now requires exp per the OIDC spec (M9), so the router test's logout-token fixture must include it. Full unit suite green (384 passed).
The rate limiter imports from the limits library directly; slowapi (a thin FastAPI/Starlette wrapper around limits) was declared but never used. Declare limits directly and drop slowapi.
EnjoyBacon7
force-pushed
the
security/advisory-hardening
branch
from
June 15, 2026 13:25
72e0157 to
4d8bca0
Compare
Collaborator
Author
|
Addressed the two findings from the latest review:
Folded into the M13 and N8 commits. Lint, unit suite, and the vectordb tests pass. |
Ahmath-Gadji
added
breaking-change
This was referenced Jun 17, 2026
ThibautChoppy
pushed a commit
to ThibautChoppy/openrag
that referenced
this pull request
Jun 19, 2026
The uvicorn deployment path fed API_NUM_WORKERS into `uvicorn --workers N`, but the app calls ray.init() at import time, so each extra worker starts its own isolated Ray cluster with duplicate named actors (Indexer, Vectordb, TaskStateManager), fragmenting task state and vector-DB access. The flag was silently ignored until v1.1.12 because the entrypoint always passed --reload (which forces a single uvicorn worker); gating --reload behind UVICORN_RELOAD=true (PR linagora#478, N8) unmasked it. - entrypoint.sh: always run a single uvicorn worker; warn if API_NUM_WORKERS is set to a non-1 value, pointing operators to Ray Serve. - charts: drop the dead API_NUM_WORKERS: "8" (the chart runs Ray Serve, which takes the api.py branch and never reads it). - .env.example / docs: remove the knob and document Ray Serve (ENABLE_RAY_SERVE + RAY_SERVE_NUM_REPLICAS) as the HTTP scaling path. Closes linagora#500
This was referenced Jul 15, 2026
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
2 participants
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.
Summary
Addresses the remaining open findings across the four draft security advisories (Critical → Low) plus the dependency items. The previously-merged PRs (#466–#476) covered C1, C2, H1, H5, H6, M1, N5 and the websearch SSRF; this PR knocks out the rest, one commit per finding.
Findings addressed
Critical / High
127.0.0.1by default inapi.py, quick_start andcluster.yaml(k8s isolated via the new NetworkPolicy).image_captioning_url: false. Guard extracted toutils/ssrf.pyand shared with the websearch fetcher.MINIO_ACCESS_KEY/MINIO_SECRET_KEY), nominioadmindefault; Milvus gets matching creds.uid 10001).CHAINLIT_AUTH_SECRETis unset (no public-constant fallback; dev opt-in viaALLOW_NO_AUTH).[Source N]/[Sources: …]/ separator control tokens in retrieved chunks + web results (prompt-injection / citation forgery).Medium — M2/M3 weak default DB password & AUTH_TOKEN; M6 path-tiered rate limiting (
slowapi/limits); M7 stop leaking stack traces / FS paths; M8 parser-bomb caps (eml/docx/pptx/pdf, incl. a[None]*max_ordermemory bomb); M9 back-channel logoutexp/jti+ replay guard; M10 default logs to INFO + drop user queries; M12 enforce token limit in RAG mode + boundn/best_of; M13 per-user partition cap.Low / crypto — N3 logout CSRF (Fetch-Metadata guard); N6 surrounding-chunk partition scoping; N7 Helm DB password → Secret; N8 no dev bind-mount/
--reloadin prod; N9 Prometheus/exporter exposure + drop--web.enable-lifecycle; N10 pin image tags; N11 dropseccomp:unconfined; N12 default-deny NetworkPolicy; N13 no token in UI file URLs under OIDC; JWT clock-skew leeway +nbf.Dependencies — Starlette → 0.47.3 / FastAPI → 0.116.2 (CVE-2025-54121; transitively bumps Chainlit 2.6.2 → 2.11.1). cairosvg SVG render pinned
unsafe=False(explicit SSRF/XXE guard).Testing
test_full_oidc_lifecycle) is pre-existing — reproduces identically on the pre-merge commit (users.pycallstask_state_manager.remote()while the test stubs itNoneunderraise_server_exceptions=True).n=100/best_of=50→ 422; M6 → 429 +Retry-After; M13 non-admin 3rd partition → 403, admin bypass.uid 10001. Helm: chart renders; NetworkPolicy valid,POSTGRES_PASSWORDin Secret not ConfigMap.Caveats for reviewers
marker-pdfcapspillow<11.0.0— blocked upstream.CHANGE_MEplaceholder that must be overridden at install.spire-docsandboxing/replacement (untrusted.doc) is noted as a follow-up — no safe drop-in.Summary by CodeRabbit
Release Notes
Security Enhancements
Retry-AfterReliability & Safety
Configuration
1.1.11, enabled default-deny NetworkPolicy, and bound dashboards/metrics to localhost