Merge dev to main - #323
Conversation
Extract retry_with_backoff() helper in components/ray_utils.py and wire it into MarkerPool, DoclingPool, and WhisperPool so transient worker failures (GPU OOM, broken ProcessPoolExecutor, etc.) retry per-task instead of failing the whole document. Retries happen at the pool layer so each attempt re-acquires a worker from the queue and re-runs health checks. Marker keeps per-chunk granularity — a failed chunk retries without re-running sibling chunks or post-processing. Also adds timeout coverage to DoclingPool and WhisperPool, which previously awaited actor.remote() directly with no timeout or ray.cancel on caller cancellation. Per-loader knobs (marker/docling default 3 retries; whisper default 1 since failures there are often deterministic like corrupt audio).
Marker retries
Introduces AUTH_MODE=oidc alongside the existing token flow. In the new
mode, humans authenticate via an external IdP (Authorization Code + PKCE)
and receive an opaque httpOnly session cookie; programmatic clients keep
using Bearer users.token.
Backend:
- New /auth/{login,callback,backchannel-logout,logout,me} routes
- AuthMiddleware: oidc_sessions cookie lookup, users.token Bearer fallback,
lazy access_token refresh with last_refresh_at short-circuit + SELECT
FOR UPDATE, UI routes 302 to /auth/login, API routes 401 JSON
- OIDC client based on Authlib (discovery, JWKS, token exchange, refresh,
userinfo, logout_token verification)
- Fernet-encrypted IdP tokens at rest; openrag_session cookie is opaque
(SHA-256 hashed in oidc_sessions)
- Back-channel logout: revokes by sid (spec-compliant)
- User matching: external_user_id=sub (fast path) → email (with backfill
of external_user_id=sub) → 403 (no auto-provisioning)
Chainlit:
- header_auth_callback reads openrag_session cookie in oidc mode
- password_auth_callback gated behind AUTH_MODE!=oidc
DB:
- Alembic migration f5b6c918f741: users.email (unique nullable) +
oidc_sessions table (encrypted tokens, sid index, composite user_sub)
Deps: authlib>=1.3, itsdangerous>=2.2, cryptography>=42, respx (dev)
Docs:
- docs/oidc.md: full guide (flow, config, Keycloak + LemonLDAP::NG,
programmatic access, back-channel logout, troubleshooting, security)
- CLAUDE.md: Authentication section extended
- README.md + .env.example
Backward compat:
- AUTH_MODE=token (default) behavior strictly unchanged
- Legacy 403 "Missing token" / "Invalid token" responses preserved
Pairs with linagora/openrag-admin-ui#18.
Per design review: drop the email-fallback matching path and its backfill/collision machinery. Matching is now purely by external_user_id == sub (admin MUST pre-provision with the right sub). The users.email column stays as optional metadata; it is no longer used for matching. Introduce an opt-in OIDC_CLAIM_MAPPING env var to let the IdP be the source of truth for display_name / email: OIDC_CLAIM_MAPPING=display_name:name,email:email OIDC_CLAIM_SOURCE=id_token # or 'userinfo' After a successful login, claims are read from the ID token (cheap) or /userinfo (complete) and the user row is updated. Writable fields are strictly whitelisted (display_name, email) — is_admin, external_user_id, file_quota, token can never be written via OIDC. The whitelist is enforced at three layers (startup parser, request-time parser, DB method) as defence in depth. Breaking changes inside the draft PR: - OIDC_EMAIL_SOURCE → OIDC_CLAIM_SOURCE (rename, same semantics) - OIDC_ALLOWED_EMAIL_DOMAINS: removed (no longer applicable) - get_user_by_email / set_user_external_id: removed (unused) Added: - update_user_fields DB method + Ray actor delegation - Three new callback tests (mapping from id_token, from userinfo, skipped when unset); five new update_user_fields tests Also in this commit: - Fix CI lint: ruff autofix (sorted imports, typing.Callable → abc, unused imports removed, dict() literal) - Fix CI tests: respx.MockTransport → MockRouter + httpx.MockTransport (respx >= 0.22 API) - Small doc updates: CLAUDE.md, docs/oidc.md, .env.example
- JsonWebToken() → JsonWebToken(["RS256"]) in both _sign_jwt helpers (Authlib >= 1.0 requires the allowed-algorithms list as first arg). - test_auth_router.py: respx.MockTransport → MockRouter + httpx.MockTransport (same fix as test_oidc_client.py last commit — respx >= 0.22 API). - test_oidc_lifecycle.py: drop unused `Request` import + sort imports.
When a user starts the OIDC flow from http://<backend>/ (no ?next= override), the callback lands them back on "/" after authentication and previously got a 404. Add a simple root handler that forwards to the indexer-ui (if it runs on a different host/port) or to /chainlit/, otherwise returns a minimal JSON status.
Correctness / security: - middleware: cookie-session lookup now gated behind AUTH_MODE=oidc so a stray openrag_session cookie cannot authenticate requests in token mode (preserves legacy Bearer-only contract) - /auth/callback: on token-exchange failure, return a generic error message to the client; full exception now logs via logger.exception() only — no IdP URLs / stack-adjacent internals leak via the HTTP response - docker-compose: remove the hardcoded `auth.example.com:host-gateway` extra_host (would shadow a real external domain for other users) + restore vllm-gpu / vllm-cpu service definitions that an unrelated local change had commented out - deps: reset_oidc_client() now best-effort closes the underlying httpx.AsyncClient to avoid "Unclosed client session" warnings in tests Tests — CI green: - test_auth_router.py: finish the respx.MockTransport → MockRouter migration (rename the oidc_transport fixture attr + all callsites; swap _mock_token_endpoint's param; drop the stray .router.get chain) - test_oidc_lifecycle.py: same migration (MockRouter + httpx.MockTransport), add ["RS256"] to the JsonWebToken factory (Authlib >= 1.0), tighten the post-logout /users/info assertion to strict 401 - test_middleware.py: docstring says "naive local time" not "naive UTC" Docs: - docs/oidc.md CSRF section: fix cookie name (openrag_oidc_state, not idp_state) and TTL (10 min, not 5) - CLAUDE.md Session Management: correct session-token shape description (secrets.token_urlsafe(32) → URL-safe ~43 chars, not 32-byte hex)
- JWKS kid was set with setdefault() which authlib's JsonWebKey.generate_key() had already populated with a random value, so signed JWTs advertised kid="test-key-1" while the JWKS served random-kid keys → authlib raised "Key not found" during verification. Use explicit []= assignment in all three test files (test_oidc_client.py, test_auth_router.py, test_oidc_lifecycle.py) to force the expected kid. Doc: - Add docs/sso-quickstart.md — step-by-step onboarding for non-experts: what to ask the SSO admin (client registration fields, redirect URIs, back-channel logout URL), how to verify the exact issuer string via curl | jq, how to generate the Fernet key, the .env block, and how to pre-provision users.
Previously defaulted to '/' which lands the user back on OpenRag's root
and immediately re-triggers OIDC login (silent re-auth if the IdP
session survived, or a loop if it didn't). Now optional:
- If set: forwarded to the IdP's end_session_endpoint as
post_logout_redirect_uri (when available) or returned directly.
- If unset: /auth/logout redirects to the IdP's end_session endpoint
WITHOUT a post_logout_redirect (IdP shows its own 'logged out' page),
or if the IdP doesn't advertise end_session, returns a plain 200
{"detail": "Logged out"} with the session cookie deleted.
Also drop the misleading 'PKCE S256 required' row from docs/sso-quickstart
— PKCE is RP-side, no IdP admin action needed.
…override
- Run ruff format on tests/api_tests/test_oidc_lifecycle.py to fix the
remaining CI lint failure (single-line function calls where the project
style allows them).
- Add docker-compose.override.yaml{,.yml} to .gitignore so developers can
keep local-only overrides (e.g. mapping an IdP hostname to host-gateway,
skipping heavy deps) without risking an accidental commit.
Lets operators mount indexer-ui under a subpath (e.g. `/indexerui/`) on the same vhost as the OpenRag backend, eliminating the cross-origin cookie problem when front and back were on different hosts. Changes: - docker-compose.yaml: forward INDEXERUI_BASE_PATH env as BASE_PATH build-arg to the indexer-ui Dockerfile. - .env.example: document INDEXERUI_BASE_PATH (commented, empty = root, rebuild required after change). - extern/indexer-ui: bump pointer to the merged main of openrag-admin-ui (c967017 — OIDC support). Pairs with linagora/openrag-admin-ui#19 (base-path support in the SvelteKit app). Merging that PR + a subsequent submodule pointer bump are required to actually deploy under a subpath. Without the PR merged, setting INDEXERUI_BASE_PATH has no effect (the build-arg is passed but the Dockerfile it reaches doesn't honor it yet).
Base.metadata.create_all() at app startup may already have created columns/tables/indexes from the SQLAlchemy models on existing deployments. Guard each ADD/CREATE op so re-running alembic upgrade on such a database no-ops instead of raising DuplicateColumn / DuplicateTable.
Move the duplicated table_exists / column_exists / index_exists / fk_exists checks (plus a new column_type_is) out of individual migration files and into a single schema_helpers module alongside env.py. env.py prepends the alembic directory to sys.path so versions can import it regardless of cwd. All call sites updated to the canonical (table, index) arg order.
…rd reference Postgres rejects 'JOIN workspaces w ON w.workspace_id = wf.workspace_id' inside the FROM clause of an UPDATE because the target table (wf) cannot be referenced from a from_item's JOIN ON. Move workspaces into the FROM list and put all join conditions in WHERE.
docs(oidc): clarify OIDC_REDIRECT_URI for proxy vs direct access
Add a §3 "Mount the Indexer UI under a Subpath" section to setup_indexerui.md covering the single-vhost use case (same-origin cookie for AUTH_MODE=oidc), the build-arg semantics and rebuild requirement, and an example nginx config for the /indexerui/ subpath.
feat(deploy): INDEXERUI_BASE_PATH build-arg + bump indexer-ui submodule
Fix/sql migration
Move hardcoded English chat profile descriptions into translation files so they respect CHAINLIT_DEFAULT_LANGUAGE setting.
…exer UI Rename CHAINLIT_DEFAULT_LANGUAGE to DEFAULT_LANGUAGE and pass it to the indexer-ui service so both UIs share one language setting. Bump the indexer-ui submodule to main with i18n support merged in.
Front/language translation
The POST /users/{user_id}/regenerate_token endpoint used to be
documented as "admin or self" but had no permission check at all —
any authenticated caller could rotate any other user's token. This
adds a `require_admin_or_self` dependency that enforces the
documented contract: admins may regenerate any user's token; non-admins
may only regenerate their own.
Also:
- Return 404 instead of crashing when the target user does not exist.
- Bump the indexer-ui submodule to pull in the companion UI branch
that surfaces this action as a button in the NavBar.
Feat/regenerate own api token
📝 WalkthroughWalkthroughThis PR introduces comprehensive OpenID Connect (OIDC) authentication support alongside existing token-based auth, including a new authentication middleware, OIDC client implementation, session management with encrypted tokens, an auth router handling login/callback/logout flows, database schema for OIDC sessions, internationalization support via language selection, and refined loader timeout/retry configurations. Multiple existing migrations are updated for idempotency compliance. Changes
Sequence DiagramssequenceDiagram
actor User
participant Browser
participant Backend as Backend (OpenRAG)
participant IdP as External IdP
participant VectorDB as VectorDB (Ray)
User->>Browser: Click "Sign In"
Browser->>Backend: GET /auth/login?next=/
Backend->>Backend: Generate state, nonce, PKCE verifier
Backend->>Browser: Set openrag_oidc_state cookie + Redirect to IdP
Browser->>IdP: GET /authorize (state, code_challenge, nonce, etc.)
User->>IdP: Enter credentials
IdP->>Browser: Redirect to /auth/callback?code=...&state=...
Browser->>Backend: GET /auth/callback?code=...&state=...
Backend->>Backend: Validate state cookie & CSRF
Backend->>IdP: POST /token (code, code_verifier)
IdP->>Backend: Return id_token, access_token, refresh_token
Backend->>Backend: Verify id_token (signature, issuer, aud, nonce)
Backend->>VectorDB: Get user by external_user_id (sub)
VectorDB->>Backend: Return user record
Backend->>VectorDB: Optionally fetch userinfo, apply claim_mapping
Backend->>VectorDB: Create OIDC session (encrypt tokens, store in DB)
Backend->>Browser: Set openrag_session cookie + Redirect to /
Browser->>Backend: GET / (with openrag_session cookie)
Backend->>Backend: AuthMiddleware reads session cookie
Backend->>VectorDB: Get OIDC session by token
VectorDB->>Backend: Return session (check expiry, revocation)
Backend->>Backend: If near expiry, call refresh (idempotent stampede guard)
Backend->>Browser: Serve content as authenticated user
sequenceDiagram
participant Middleware as AuthMiddleware
participant VectorDB as VectorDB (Ray)
participant Cache as Session Cache<br/>(per request)
participant IdP as External IdP
Middleware->>Middleware: Read request (Auth header / cookies)
alt AUTH_MODE = "token"
Middleware->>VectorDB: Lookup user by Bearer token
VectorDB->>Middleware: Return user or None
Middleware->>Middleware: Set request.state.user (or 403)
else AUTH_MODE = "oidc"
Middleware->>Cache: Extract openrag_session cookie
alt Cookie exists
Middleware->>VectorDB: Get OIDC session by plaintext token
VectorDB->>Middleware: Return session (check revoked/expired)
alt Session valid & near expiry
Middleware->>VectorDB: Decrypt refresh_token, call IdP refresh
IdP->>Middleware: Return new tokens
Middleware->>VectorDB: Update encrypted tokens + last_refresh_at
Middleware->>Middleware: Set request.state.oidc_session
else Session valid & fresh
Middleware->>Middleware: Set request.state.oidc_session
else Invalid/revoked
Middleware->>Middleware: Redirect to login (UI) or 401 (API)
end
else No cookie, try Bearer token fallback
Middleware->>VectorDB: Lookup user by long-lived token
alt Found
Middleware->>Middleware: Set request.state.user
else Not found
Middleware->>Middleware: Redirect (UI) or 401 (API)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
openrag/routers/openai.py (1)
328-333:⚠️ Potential issue | 🟠 MajorApply output-token validation to both direct LLM and RAG requests.
RAG requests bypass the
check_tokens_limit()validation that direct LLM requests enforce. Sincerequest.max_tokensflows through to the backend in both cases, the router should validate the output-token bound uniformly. Extract output-token validation into a standalone check and apply it before the direct-vs-RAG branch so that neither path can send an oversizedmax_tokensto the LLM backend.🛡️ Proposed direction
+def check_output_tokens_limit( + request: OpenAIChatCompletionRequest | OpenAICompletionRequest, + log, +): + max_tokens_allowed = get_max_model_tokens() + default_output_tokens = int(config.llm_context.max_output_tokens) + requested_tokens = request.max_tokens or default_output_tokens + if requested_tokens > max_tokens_allowed: + detail = ( + f"Requested output tokens ({requested_tokens}) exceed " + f"maximum allowed model tokens ({max_tokens_allowed})." + ) + log.info("Request exceeds output token limit", detail=detail) + raise HTTPException( + status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE, + detail=detail, + ) + @@ - if is_direct_llm_model(request): + check_output_tokens_limit(request, log) + if is_direct_llm_model(request): check_tokens_limit(request, log) partitions = None @@ - if is_direct_llm_model(request): + check_output_tokens_limit(request, log) + if is_direct_llm_model(request): check_tokens_limit(request, log) partitions = NoneAlso applies to: 423–428
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/routers/openai.py` around lines 328 - 333, The output-token check is only applied in the direct LLM branch (is_direct_llm_model) so RAG requests can bypass it; move or call check_tokens_limit(request, log) before the direct-vs-RAG branch so request.max_tokens is validated for both paths, i.e., invoke check_tokens_limit(request, log) immediately prior to selecting partitions/get_partition_name(...) and before any other branching that handles direct LLM vs RAG (also mirror this same change in the other similar branch later in the file that currently skips the check).pyproject.toml (1)
3-3:⚠️ Potential issue | 🟡 MinorBump the package version for the v1.1.9 release.
The PR is described as
v1.1.9, but the project metadata still publishes1.1.8. If this file is the source for package metadata, release artifacts will be mislabeled.Proposed fix
-version = "1.1.8" +version = "1.1.9"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 3, Update the package version in pyproject.toml from "1.1.8" to "1.1.9" so the project metadata matches the release; locate the version = "1.1.8" line (the version entry in pyproject.toml) and change it to version = "1.1.9" to align with the v1.1.9 release described in the PR.openrag/scripts/migrations/alembic/versions/e7f8a9b0c1d2_add_workspaces.py (1)
13-61:⚠️ Potential issue | 🟠 MajorRemove inline
index=Trueand create indexes explicitly withindex_existsguards.The migration defines indexes inline via
index=Trueinop.create_table()(lines 32, 50, 51), but does not guard their creation with explicitindex_exists()checks. If these tables already exist without the indexes, this migration will no-op and leaveworkspaces.workspace_id,workspace_files.workspace_id, andworkspace_files.file_idunindexed. Per the idempotency requirement, extract these indexes into explicitop.create_index()calls guarded byindex_exists()in bothupgrade()anddowngrade()methods, following the pattern used in other migrations like4add4d260575_initial_migration.pyandcd642e4502d8_create_users_memberships_tables.py.Proposed fix
- sa.Column("workspace_id", sa.String, unique=True, nullable=False, index=True), + sa.Column("workspace_id", sa.String, unique=True, nullable=False), sa.Column( "partition_name", sa.String, @@ -47,12 +47,24 @@ def upgrade() -> None: sa.Column( "workspace_id", sa.String, sa.ForeignKey("workspaces.workspace_id", ondelete="CASCADE"), nullable=False, - index=True, ), - sa.Column("file_id", sa.String, nullable=False, index=True), + sa.Column("file_id", sa.String, nullable=False), sa.UniqueConstraint("workspace_id", "file_id", name="uix_workspace_file"), ) + # Create indexes explicitly with idempotent guards + if table_exists("workspaces"): + if not index_exists("workspaces", "ix_workspaces_workspace_id"): + op.create_index("ix_workspaces_workspace_id", "workspaces", ["workspace_id"]) + if table_exists("workspace_files"): + if not index_exists("workspace_files", "ix_workspace_files_workspace_id"): + op.create_index("ix_workspace_files_workspace_id", "workspace_files", ["workspace_id"]) + if not index_exists("workspace_files", "ix_workspace_files_file_id"): + op.create_index("ix_workspace_files_file_id", "workspace_files", ["file_id"]) def downgrade() -> None: """Downgrade schema.""" + if table_exists("workspace_files"): + if index_exists("workspace_files", "ix_workspace_files_file_id"): + op.drop_index("ix_workspace_files_file_id", table_name="workspace_files") + if index_exists("workspace_files", "ix_workspace_files_workspace_id"): + op.drop_index("ix_workspace_files_workspace_id", table_name="workspace_files") if table_exists("workspace_files"): op.drop_table("workspace_files") + if table_exists("workspaces"): + if index_exists("workspaces", "ix_workspaces_workspace_id"): + op.drop_index("ix_workspaces_workspace_id", table_name="workspaces") if table_exists("workspaces"): op.drop_table("workspaces")Also import
index_existsfromschema_helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/scripts/migrations/alembic/versions/e7f8a9b0c1d2_add_workspaces.py` around lines 13 - 61, The migration currently uses inline index=True in op.create_table() for columns workspaces.workspace_id, workspace_files.workspace_id and workspace_files.file_id; remove those inline index=True flags in the op.create_table() calls inside upgrade() and instead add explicit op.create_index() calls for each index (e.g. names like ix_workspaces_workspace_id, ix_workspace_files_workspace_id, ix_workspace_files_file_id) wrapped with index_exists(...) guards (import index_exists from schema_helpers) so index creation is idempotent; likewise update downgrade() to drop those indexes with op.drop_index() only if index_exists(...) is false/true as appropriate (mirror pattern used in 4add4d260575_initial_migration.py and cd642e4502d8_create_users_memberships_tables.py) while keeping existing table_exists checks for table creation.openrag/config/models.py (1)
245-252:⚠️ Potential issue | 🟠 MajorAdd validation constraints for the new timeout/retry settings.
These values directly drive
asyncio.wait_for()andretry_with_backoff(). Invalid env overrides like negative retries or zero/negative timeouts can make loader tasks fail immediately or hit the skipped-loop path in the retry helper.Proposed fix
class LocalWhisperConfig(ConfigMixin): model: str = "base" whisper_n_workers: int = 3 whisper_num_gpus: float = 0.01 whisper_concurrency_per_worker: int = 2 - whisper_timeout: int = 1800 - whisper_max_task_retry: int = 1 - whisper_retry_base_delay: float = 2.0 + whisper_timeout: int = Field(default=1800, gt=0) + whisper_max_task_retry: int = Field(default=1, ge=0) + whisper_retry_base_delay: float = Field(default=2.0, ge=0) @@ - marker_max_task_retry: int = 3 - marker_retry_base_delay: float = 2.0 + marker_max_task_retry: int = Field(default=3, ge=0) + marker_retry_base_delay: float = Field(default=2.0, ge=0) @@ - docling_timeout: int = 3600 - docling_max_task_retry: int = 3 - docling_retry_base_delay: float = 2.0 + docling_timeout: int = Field(default=3600, gt=0) + docling_max_task_retry: int = Field(default=3, ge=0) + docling_retry_base_delay: float = Field(default=2.0, ge=0)Also applies to: 330-351
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/config/models.py` around lines 245 - 252, Add explicit validation to ensure timeout/retry/concurrency fields in LocalWhisperConfig (and the analogous config class around lines 330-351) cannot be set to invalid values: require whisper_timeout > 0, whisper_n_workers > 0, whisper_concurrency_per_worker > 0, whisper_max_task_retry >= 0, whisper_retry_base_delay >= 0.0, and whisper_num_gpus >= 0.0; implement these checks using the model's validators (per-field `@validator` or a root_validator) and raise ValueError with a clear message identifying the offending field (e.g., "whisper_timeout must be > 0") so invalid env overrides are rejected early.conf/config.yaml (1)
151-161:⚠️ Potential issue | 🟡 MinorUpdate the env-var comments for the new retry knobs.
This config file says overrides are documented in comments, but the new Whisper, Marker, and Docling timeout/retry fields are omitted from the
Env:lists. Add the new env names so operators can discover and tune them without reading code.Proposed documentation update
- # Env: WHISPER_MODEL, WHISPER_N_WORKERS, WHISPER_NUM_GPUS, WHISPER_CONCURRENCY_PER_WORKER + # Env: WHISPER_MODEL, WHISPER_N_WORKERS, WHISPER_NUM_GPUS, WHISPER_CONCURRENCY_PER_WORKER, + # WHISPER_TIMEOUT, WHISPER_MAX_TASK_RETRY, WHISPER_RETRY_BASE_DELAY @@ - # Env: MARKER_MAX_TASKS_PER_CHILD, MARKER_POOL_SIZE, MARKER_MAX_PROCESSES, - # MARKER_NUM_GPUS, MARKER_TIMEOUT, MARKER_PDFTEXT_WORKERS, MARKER_CHUNK_SIZE + # Env: MARKER_MAX_TASKS_PER_CHILD, MARKER_POOL_SIZE, MARKER_MAX_PROCESSES, + # MARKER_NUM_GPUS, MARKER_TIMEOUT, MARKER_PDFTEXT_WORKERS, MARKER_CHUNK_SIZE, + # MARKER_MAX_TASK_RETRY, MARKER_RETRY_BASE_DELAY @@ - # Env: DOCLING_NUM_GPUS, DOCLING_POOL_SIZE, DOCLING_MAX_TASKS_PER_WORKER + # Env: DOCLING_NUM_GPUS, DOCLING_POOL_SIZE, DOCLING_MAX_TASKS_PER_WORKER, + # DOCLING_TIMEOUT, DOCLING_MAX_TASK_RETRY, DOCLING_RETRY_BASE_DELAYAlso applies to: 185-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@conf/config.yaml` around lines 151 - 161, Update the Env: comment lists to include the new timeout and retry environment variable names so operators can discover them; for the local_whisper block add WHISPER_TIMEOUT, WHISPER_MAX_TASK_RETRY, and WHISPER_RETRY_BASE_DELAY to the existing WHISPER_* list (alongside WHISPER_MODEL, WHISPER_N_WORKERS, WHISPER_NUM_GPUS, WHISPER_CONCURRENCY_PER_WORKER), and make the analogous additions for the Marker and Docling sections by adding MARKER_TIMEOUT, MARKER_MAX_TASK_RETRY, MARKER_RETRY_BASE_DELAY and DOCLING_TIMEOUT, DOCLING_MAX_TASK_RETRY, DOCLING_RETRY_BASE_DELAY to their respective Env: comment lines (also apply the same change where the other block is documented around the later config region).
🟡 Minor comments (8)
openrag/scripts/migrations/alembic/versions/c224d4befe71_add_file_count_and_file_quota.py-34-35 (1)
34-35:⚠️ Potential issue | 🟡 MinorRename FK to match explicit constraint name or adjust migration check.
The migration checks for FK named
fk_files_created_by, but the model definescreated_byusingForeignKey("users.id", ondelete="SET NULL")without an explicitconstraint_name. SQLAlchemy auto-generates FK names (typicallyfiles_created_by_fk), sofk_exists()will not find the auto-generated FK and attempt to create a duplicate whenBase.metadata.create_all()has already created it.Add
constraint_name="fk_files_created_by"to the model'sForeignKey()definition, or modify the migration to check column existence instead of explicit FK name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/scripts/migrations/alembic/versions/c224d4befe71_add_file_count_and_file_quota.py` around lines 34 - 35, The migration is checking for a foreign key named "fk_files_created_by" but the model's ForeignKey on the created_by column was defined without an explicit constraint name so SQLAlchemy auto-generates a different name; either add constraint_name="fk_files_created_by" to the model's ForeignKey(...) on the File.created_by column so the DB uses the same name the migration expects, or change the migration's check (the fk_exists("files","fk_files_created_by") call and op.create_foreign_key("fk_files_created_by", ...)) to instead detect the created_by column/constraint generically (e.g., check for the column or existing FK by column pair) to avoid creating a duplicate constraint.CLAUDE.md-419-419 (1)
419-419:⚠️ Potential issue | 🟡 MinorTypo: "Programmtic" → "Programmatic".
Flagged by LanguageTool.
Proposed fix
-- Programmtic access: Bearer `users.token` accepted in both modes +- Programmatic access: Bearer `users.token` accepted in both modes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 419, The line containing "Programmtic access: Bearer `users.token` accepted in both modes" has a typo; change "Programmtic" to "Programmatic" so the sentence reads "Programmatic access: Bearer `users.token` accepted in both modes". Locate the exact string in CLAUDE.md and update it accordingly.openrag/scripts/migrations/alembic/versions/f1a2b3c4d5e6_add_workspace_files_file_id_fk.py-86-103 (1)
86-103:⚠️ Potential issue | 🟡 MinorDowngrade: unguarded rename + unguarded data-population UPDATE.
Two small parity gaps with
upgrade():
- Line 96
UPDATE workspace_files wf SET file_str = f.file_id FROM files f WHERE f.id = wf.file_idruns unconditionally. If the downgrade is re-invoked after a partial failure wherefile_strwas already populated, this re-runs harmlessly but acolumn_exists+ "not already populated" or a short-circuit symmetric to upgrade'scolumn_type_is("workspace_files", "file_id", sa.String)would be more consistent with the rest of the file.- Line 99
alter_column("workspace_files", "file_str", new_column_name="file_id", ...)is also unguarded. Same reasoning as the upgrade rename — re-running after a partial failure past this line will fail becausefile_strno longer exists. A top-of-function short-circuit (if column_type_is("workspace_files", "file_id", sa.String): return after ensuring FK/index absent) would make the downgrade fully rerunnable.As per coding guidelines: "Alembic migrations must be idempotent: guard every schema-mutating operation with inspector-based existence checks ... in both
upgrade()anddowngrade()" and "For type-conversion migrations, short-circuit in Alembic if the column is already the target type".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/scripts/migrations/alembic/versions/f1a2b3c4d5e6_add_workspace_files_file_id_fk.py` around lines 86 - 103, Add idempotency guards to downgrade(): at the top of downgrade() short-circuit when workspace_files.file_id is already a String (use column_type_is("workspace_files", "file_id", sa.String) and return after ensuring FK/index are removed), make the data-population UPDATE conditional (only run op.execute("UPDATE workspace_files ...") if column_exists("workspace_files", "file_str") and rows still have NULL/empty file_str or use an inspector check to skip if already populated), and guard the rename/alter by checking column_exists("workspace_files", "file_str") before calling op.alter_column(..., new_column_name="file_id", ...); keep using the existing helper functions (column_exists, column_type_is, index_exists, fk_exists, unique_constraint_exists) and the same constraint/index names (fk_workspace_files_file_id, ix_workspace_files_file_id, uix_workspace_file).docs/sso-quickstart.md-65-67 (1)
65-67:⚠️ Potential issue | 🟡 MinorMinor: illustrative Fernet key is not a valid Fernet key.
The example value
XFlT-ZfXkdqf0v-5Z8kVt9xhU6c7Z4z0ZY8Z4Z4Z4=is only 42 url-safe base64 chars, not 44, and won't decode to the required 32-byte key if a reader copy-pastes it to sanity-checkFernet(...)construction. Consider either replacing with a realFernet.generate_key()output or prefacing it as<example — generate your own>to avoid confusion. Also addresses the static-analysis false positive on a "Generic API Key".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/sso-quickstart.md` around lines 65 - 67, The example Fernet key in the docs ("XFlT-ZfXkdqf0v-5Z8kVt9xhU6c7Z4z0ZY8Z4Z4Z4=") is not a valid 44-char url-safe base64 Fernet key and will not decode to a 32-byte key; update the docs to either replace that string with an actual output from Fernet.generate_key() or prefix it with an explicit label like "<example — generate your own>" so readers don’t copy an invalid key; reference this example string and the Fernet.generate_key() call in the change so reviewers can verify the fix.docs/oidc.md-79-79 (1)
79-79:⚠️ Potential issue | 🟡 MinorKeep the state-cookie TTL consistent.
Line 79 says the OIDC state cookie has a 5-minute TTL, but Line 744 says 10 minutes. Please align this with the actual implementation so callback-expiry troubleshooting is reliable.
Also applies to: 741-745
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/oidc.md` at line 79, The docs conflict about the OIDC state cookie TTL — update the markdown to match the actual implementation in state_cookie.py: inspect the TTL constant or cookie max-age (e.g., STATE_COOKIE_TTL / COOKIE_MAX_AGE / any expiry value in state_cookie.py) and then set the TTL description at the locations around "state_cookie.py" (line ~79) and the block around lines 741–745 to that exact value so both references are consistent with the code.docs/oidc.md-198-207 (1)
198-207:⚠️ Potential issue | 🟡 MinorMake API-token examples match the generated token format.
The examples show shortened
or-...values. Since the implementation generatesor-plus 32 hex characters, use 32 placeholder hex characters after the prefix in docs so users can recognize valid tokens.📝 Proposed docs adjustment
- "token": "or-xxxxxxxxxxxxxxxxxxxxxxxx" + "token": "or-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" @@ -curl -H "Authorization: Bearer or-xxxxxxxxxxxxxxxxxxxxxxxx" \ +curl -H "Authorization: Bearer or-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \ @@ -API_TOKEN="or-xxxxxxxxxxxxxxxxxxxxxxxx" +API_TOKEN="or-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"Also applies to: 464-498
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/oidc.md` around lines 198 - 207, Update the API token examples so they match the actual generated format: replace the shortened "or-..." placeholders in the Response JSON examples (the "token" field) with "or-" followed by 32 hexadecimal characters (e.g., or-0123456789abcdef0123456789abcdef); apply the same change to all other example occurrences noted (the additional examples referenced around the later section). Ensure the "token" example strings use 32 hex chars after the "or-" prefix so they reflect valid token shape.docs/oidc.md-145-170 (1)
145-170:⚠️ Potential issue | 🟡 MinorAvoid publishing reusable-looking Fernet key examples.
The concrete key on Lines 147 and 169 is likely to trigger secret scanners and may be copied by users. Prefer a placeholder everywhere after showing the generation command.
📝 Proposed docs adjustment
Output example: -``` -XFlT-ZfXkdqf0v-5Z8kVt9xhU6c7Z4z0ZY8Z4Z4Z4= +```text +<generated-fernet-key>@@
-OIDC_TOKEN_ENCRYPTION_KEY=XFlT-ZfXkdqf0v-5Z8kVt9xhU6c7Z4z0ZY8Z4Z4Z4=
+OIDC_TOKEN_ENCRYPTION_KEY=</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/oidc.mdaround lines 145 - 170, The docs include a concrete
Fernet-looking key ("XFlT-ZfXkdqf0v-5Z8kVt9xhU6c7Z4z0ZY8Z4Z4Z4=") in both the
output example and the .env snippet; replace those literal keys with a
non-sensitive placeholder (e.g., ) so readers won’t copy a
reusable secret and secret scanners won’t be triggered, updating the output
example and the OIDC_TOKEN_ENCRYPTION_KEY line (referencing the
OIDC_TOKEN_ENCRYPTION_KEY env var and the concrete key string) accordingly while
keeping the key generation command shown earlier as the canonical way to produce
a real key.</details> </blockquote></details> <details> <summary>openrag/components/ray_utils.py-73-88 (1)</summary><blockquote> `73-88`: _⚠️ Potential issue_ | _🟡 Minor_ **Validate retry parameters before entering the loop.** A negative `max_retries` skips the loop and reaches `raise last_exc` with `None`; a negative `base_delay` also makes retry timing invalid. Since these values are config-driven, fail fast with a clear error and keep retry logs structured. <details> <summary>Proposed fix</summary> ```diff Backoff: base_delay * 2**attempt seconds. CancelledError is never retried. """ + if max_retries < 0: + raise ValueError("max_retries must be >= 0") + if base_delay < 0: + raise ValueError("base_delay must be >= 0") + + retry_logger = logger.bind(task_description=task_description, max_attempts=max_retries + 1) last_exc: Exception | None = None for attempt in range(max_retries + 1): try: return await attempt_fn(attempt) @@ last_exc = e if attempt >= max_retries: - logger.error(f"{task_description} failed after {attempt + 1} attempts: {e}") + retry_logger.bind(attempt=attempt + 1, error=str(e)).error("Task failed after retries") raise delay = base_delay * (2**attempt) - logger.warning( - f"{task_description} failed (attempt {attempt + 1}/{max_retries + 1}): {e}. Retrying in {delay:.1f}s..." - ) + retry_logger.bind(attempt=attempt + 1, delay=delay, error=str(e)).warning("Task failed; retrying") await asyncio.sleep(delay)As per coding guidelines,
**/*.py: Use Loguru with structured logging viaget_logger()fromutils.logger, and use.bind()for contextual fields likefile_idandpartition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/ray_utils.py` around lines 73 - 88, Validate retry parameters before entering the retry loop in the function using max_retries, base_delay, attempt_fn and task_description: raise a clear ValueError if max_retries is negative or base_delay is negative (fail fast rather than allowing last_exc to remain None), and ensure the retry loop only runs with valid params; replace raw logger usage with Loguru via get_logger() from utils.logger and use .bind() to attach contextual fields (e.g., file_id, partition) to the logger so all logger.error/logger.warning calls inside the loop use structured logging.
🧹 Nitpick comments (14)
openrag/scripts/migrations/alembic/versions/4add4d260575_initial_migration.py (1)
13-13: Use the package-qualified migration helper import.The bare
schema_helpersimport depends on Alembic/runtimesys.path; prefer theopenragabsolute import used by the rest of the package.Proposed fix
-from schema_helpers import index_exists, table_exists +from openrag.scripts.migrations.alembic.schema_helpers import index_exists, table_existsAs per coding guidelines, “Use absolute imports from the
openrag/directory (Python path root) instead of relative imports across packages”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/scripts/migrations/alembic/versions/4add4d260575_initial_migration.py` at line 13, Replace the bare module import with the package-qualified import: change "from schema_helpers import index_exists, table_exists" to "from openrag.schema_helpers import index_exists, table_exists" so the migration uses the absolute openrag package path (references: schema_helpers, index_exists, table_exists).openrag/scripts/migrations/alembic/versions/cd642e4502d8_create_users_memberships_tables.py (1)
13-13: Use the package-qualified migration helper import.The bare
schema_helpersimport depends on Alembic/runtimesys.path; prefer theopenragabsolute import.Proposed fix
-from schema_helpers import index_exists, table_exists +from openrag.scripts.migrations.alembic.schema_helpers import index_exists, table_existsAs per coding guidelines, “Use absolute imports from the
openrag/directory (Python path root) instead of relative imports across packages”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/scripts/migrations/alembic/versions/cd642e4502d8_create_users_memberships_tables.py` at line 13, Replace the bare module import for the migration helpers with an absolute package-qualified import: change the "from schema_helpers import index_exists, table_exists" import to use the openrag package path so the migration helpers are imported as the package-qualified module (referencing the index_exists and table_exists symbols) to avoid relying on alembic runtime sys.path.openrag/scripts/migrations/alembic/versions/c224d4befe71_add_file_count_and_file_quota.py (1)
13-13: Use the package-qualified migration helper import.The bare
schema_helpersimport depends on Alembic/runtimesys.path; prefer theopenragabsolute import.Proposed fix
-from schema_helpers import column_exists, fk_exists, index_exists +from openrag.scripts.migrations.alembic.schema_helpers import column_exists, fk_exists, index_existsAs per coding guidelines, “Use absolute imports from the
openrag/directory (Python path root) instead of relative imports across packages”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/scripts/migrations/alembic/versions/c224d4befe71_add_file_count_and_file_quota.py` at line 13, Replace the bare import "from schema_helpers import column_exists, fk_exists, index_exists" with the package-qualified import so it uses the project root (e.g. "from openrag.scripts.migrations.alembic.schema_helpers import column_exists, fk_exists, index_exists"); update the import line in the migration module (the one that references column_exists, fk_exists, index_exists) to use that fully-qualified module path.docs/content/docs/documentation/setup_indexerui.md (1)
48-97: Clear and accurate subpath-mounting guide.The rebuild-required callout and the explanation of the trailing slash on
proxy_passare exactly the right things to emphasize — these are the two most common footguns with SvelteKitbase+ nginx. No issues.One optional nitpick: the nginx example hardcodes the upstream container port
3000(the indexer-ui container's internal listen port), while the env docs referenceINDEXERUI_PORT(the host-side mapping). That is correct when nginx runs inside the compose network, but a quick one-liner noting "use the container's internal port3000when nginx is on the compose network, or the host-mapped${INDEXERUI_PORT}otherwise" would save some readers a detour.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/documentation/setup_indexerui.md` around lines 48 - 97, Add a brief clarifying sentence to the nginx example noting which port to use based on where nginx runs: explain that when nginx runs inside the Docker Compose network it should proxy to the indexer-ui container's internal listen port (3000) but when nginx runs outside/on the host it should use the host-mapped INDEXERUI_PORT; update the text near the proxy_pass example and reference INDEXERUI_PORT and the proxy_pass http://indexer-ui:3000/; so readers understand the difference without changing the existing rebuild/INDEXERUI_BASE_PATH guidance.tests/api_tests/OIDC_TEST_COVERAGE.md (1)
24-24: Consider automating AC17 (migration idempotency).Every other AC has a concrete test reference; AC17 is the only "Manual" entry. A small pytest fixture that spins up a fresh Postgres (or SQLite in-memory, if the migrations allow) and runs
alembic upgrade head && alembic downgrade -1 && alembic upgrade headwould close the gap and protect the hard-won idempotency work in the two migration files touched by this PR. Given theBase.metadata.create_all()interaction noted inCLAUDE.md, an automated regression is especially valuable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api_tests/OIDC_TEST_COVERAGE.md` at line 24, Add an automated pytest test for AC17 by creating a fixture (e.g., alembic_migration_runner) that provisions a fresh DB (Postgres test container or SQLite in-memory if compatible) and a test named test_ac17_migration_idempotency that runs the Alembic migration sequence `alembic upgrade head`, `alembic downgrade -1`, `alembic upgrade head` (either via subprocess calling `alembic` or using alembic.command API) and asserts no exceptions and that the DB schema is valid after the final upgrade; ensure the fixture cleans up the DB and that the test is discoverable by pytest..env.example (1)
79-114: OIDC env block is thorough and well-commented.The callouts about issuer trailing-slash exactness, callback URI byte-match, and the
OIDC_POST_LOGOUT_REDIRECT_URIloop hazard are exactly the kinds of footguns that burn hours in production — nice additions. The claim-mapping whitelist is clearly spelled out, matching the runtime enforcement.One small inconsistency:
OIDC_SCOPES=openid email profile offline_accessis shown uncommented on line 99, whereas all the other OIDC vars in this block are commented out (the section is gated onAUTH_MODE=oidc). Either comment it for visual consistency, or leave a note that it is safe to set unconditionally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 79 - 114, The OIDC env block mixes commented vars with an uncommented OIDC_SCOPES line; make this consistent by either commenting out OIDC_SCOPES or adding a short clarifying note that it’s safe to set unconditionally when AUTH_MODE is not oidc. Update the .env.example so the OIDC_SCOPES line (and any other OIDC_* vars) follow the same convention as AUTH_MODE and the rest of the block; reference OIDC_SCOPES and AUTH_MODE when making the change.openrag/scripts/migrations/alembic/versions/f1a2b3c4d5e6_add_workspace_files_file_id_fk.py (1)
35-83: Solid idempotent migration — one unguarded rename worth acknowledging.The
column_type_is(...) → returnshort-circuit at the top ofupgrade()is the right pattern and cleanly handles the common "Base.metadata.create_all() already built the final schema" case. The switch to an implicit-cross-joinFROM files f, workspaces w ... WHEREis necessary given the inline comment about Postgres UPDATE/FROM restrictions — good explanation.Note that
op.alter_column("workspace_files", "file_fk", new_column_name="file_id", nullable=False)on line 68 is the one op not guarded by an existence check. It is safe in practice because thecolumn_type_isearly return catches the post-rename state, but if a prior run ever crashed between line 68 and the FK/unique creation, re-running would work only because by that pointfile_idis already Integer and the early return fires. Worth a one-line comment making that invariant explicit so future edits don't accidentally break it.Minor:
DELETE FROM workspace_files WHERE file_id NOT IN (SELECT file_id FROM files)(line 39) returns unexpected results iffiles.file_idcontains any NULLs (SQL three-valued logic). Givenfiles.file_idis the old string PK this is almost certainly non-null, so more of a defensive-coding note than a real bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/scripts/migrations/alembic/versions/f1a2b3c4d5e6_add_workspace_files_file_id_fk.py` around lines 35 - 83, The op.alter_column("workspace_files", "file_fk", new_column_name="file_id", nullable=False) call is the only unguarded rename and relies on the early return from column_type_is(...); add a one-line comment immediately above that op.alter_column explaining the invariant (that column_type_is(...) short-circuits when file_id is already Integer so this rename is safe and only reached when file_fk exists), and optionally replace the initial DELETE ... NOT IN with a NULL-safe pattern (e.g., use NOT EXISTS) if you want to defensively handle files.file_id NULLs; reference op.alter_column and column_type_is when making the change.docker-compose.yaml (1)
23-23: Non-standard directory naming:i8nshould follow the conventioni18n.The
./i8n/directory exists in the repository and is correctly referenced in this bind mount. However,i8nis a non-standard abbreviation; the conventional numeronym for internationalization isi18n(18 letters betweeniandn). Renaming the directory toi18n/would improve consistency with industry conventions, tooling, and future contributor expectations.Proposed refactor (requires renaming the directory)
- - ./i8n:/app/openrag/.chainlit/translations + - ./i18n:/app/openrag/.chainlit/translations🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yaml` at line 23, Rename the non-standard mount source "./i8n" to the conventional numeronym "./i18n" and update the bind mount entry in docker-compose.yaml (the line mapping ./i8n:/app/openrag/.chainlit/translations) to use ./i18n:/app/openrag/.chainlit/translations; ensure you also rename the repository directory from i8n to i18n and update any other references to the old ./i8n path in the project (e.g., CI, docs, scripts) so the service continues to mount translations correctly.openrag/routers/test_auth_router.py (1)
304-309: Fixture calls__init__()on a live instance instead of rebinding.Re-invoking
__init__on_stub_vectordb_singletonworks only because_StubVectorDB.__init__has no side-effects beyond attribute assignment, and it leaves any references captured by the_RayMethodStubchildren from the previous test pointing at staleself.callslists momentarily during re-init. A cleaner reset is to construct a fresh instance and rebind the module global so all new stubs close over the new state atomically.♻️ Proposed refactor
`@pytest.fixture` def fresh_stub_vectordb(): global _stub_vectordb_singleton - # Re-create so tests see a clean state. - _stub_vectordb_singleton.__init__() + # Re-create so tests see a clean state. + _stub_vectordb_singleton = _StubVectorDB() + sys.modules["utils.dependencies"].get_vectordb = lambda: _stub_vectordb_singleton return _stub_vectordb_singleton🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/routers/test_auth_router.py` around lines 304 - 309, The fixture fresh_stub_vectordb should not call _stub_vectordb_singleton.__init__ on the live instance; instead construct a new _StubVectorDB() and rebind the module global _stub_vectordb_singleton so all _RayMethodStub children close over the fresh instance atomically—update the fixture to create a new _StubVectorDB(), assign it to _stub_vectordb_singleton, and return that new object (referencing the _StubVectorDB class and _stub_vectordb_singleton global in the fixture implementation).openrag/components/auth/deps.py (1)
71-83: Preferasyncio.get_running_loop()over the deprecated policy API.
asyncio.get_event_loop_policy().get_event_loop()is deprecated since Python 3.12 and emits aDeprecationWarningwhen there is no running loop; the underlyingget_event_loop()(implicit-loop creation) is slated for removal. Since this is a best-effort test hook wrapped intry/except, swap to the modern idiom so CI stays warning-clean and forward-compatible. Also noteloop.create_task()is not thread-safe when called from a non-loop thread —asyncio.run_coroutine_threadsafewould be the correct primitive if this ever runs from pytest's main thread while the loop runs elsewhere.♻️ Proposed refactor
- try: - import asyncio - - loop = asyncio.get_event_loop_policy().get_event_loop() - if loop.is_running(): - # Schedule close on the running loop without awaiting — caller - # doesn't need to be async. - loop.create_task(old.aclose()) - else: - loop.run_until_complete(old.aclose()) - except Exception: - # Closing is best-effort; never let a reset blow up the caller. - pass + try: + import asyncio + + try: + loop = asyncio.get_running_loop() + except RuntimeError: + # No running loop — run the coroutine to completion in a fresh one. + asyncio.run(old.aclose()) + else: + # Schedule close on the running loop without awaiting. + loop.create_task(old.aclose()) + except Exception: + # Closing is best-effort; never let a reset blow up the caller. + pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/auth/deps.py` around lines 71 - 83, The current close logic uses the deprecated asyncio.get_event_loop_policy().get_event_loop() and loop.create_task(), which can emit DeprecationWarning and is not thread-safe; replace that call with asyncio.get_running_loop() inside the try so we only proceed if a running loop exists, and when scheduling old.aclose() from a non-loop thread use asyncio.run_coroutine_threadsafe(old.aclose(), loop) instead of loop.create_task(); keep the fallback of loop.run_until_complete(old.aclose()) for the synchronous case and preserve the surrounding try/except to keep this best-effort test hook non-raising.openrag/api.py (1)
258-272:root_redirectloop prevention is a fragile substring check.
f":{APP_PORT}" not in INDEXERUI_URLis a stringly-typed heuristic that breaks in plausible deployments:
- INDEXERUI_URL behind a reverse proxy on standard ports (e.g.
https://ui.mycorp.comorhttp://ui.mycorp.com:80) with APP_PORT=8080 → redirects correctly, but a deployer who setsAPP_PORT=80would suddenly stop redirecting because:80is now a substring of:80anywhere.- Any INDEXERUI_URL that coincidentally contains the APP_PORT string in a path or query (
https://ui.mycorp.com/app:8080) would falsely suppress the redirect.- The hostname isn't compared at all, so
http://app-internal:8080vshttp://localhost:8080are treated identically.Parse the URL and compare
(scheme, host, port)against the request's own host to prevent genuine loops. Minor, but worth tightening while the flow is fresh.♻️ Proposed refactor sketch
-@app.get("/", include_in_schema=False) -def root_redirect(): +@app.get("/", include_in_schema=False) +def root_redirect(request: Request): ... - if INDEXERUI_URL and f":{os.getenv('APP_PORT', '8080')}" not in INDEXERUI_URL: - return RedirectResponse(url=INDEXERUI_URL, status_code=302) + if INDEXERUI_URL: + from urllib.parse import urlparse + + ui = urlparse(INDEXERUI_URL) + app_port = int(os.getenv("APP_PORT", "8080")) + ui_port = ui.port or (443 if ui.scheme == "https" else 80) + same_host = ui.hostname in (None, request.url.hostname, "localhost", "127.0.0.1") + if not (same_host and ui_port == app_port): + return RedirectResponse(url=INDEXERUI_URL, status_code=302) if WITH_CHAINLIT_UI: return RedirectResponse(url="/chainlit/", status_code=302) return JSONResponse({"status": "ok", "app": "openrag", "version": app.version})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/api.py` around lines 258 - 272, root_redirect uses a fragile substring check to avoid redirect loops; change it to parse INDEXERUI_URL and compare its scheme/hostname/port to the incoming request's scheme/hostname/port instead of checking f":{APP_PORT}" in the string: add a Request parameter to root_redirect, use urllib.parse.urlparse (or fastapi Request.url) to extract host and port from INDEXERUI_URL and from request.url, normalize default ports for http/https, and only call RedirectResponse(URL=INDEXERUI_URL) when the parsed (scheme, hostname, port) do not match the request's (scheme, hostname, port); keep the existing WITH_CHAINLIT_UI and JSONResponse behavior otherwise and still reference INDEXERUI_URL, WITH_CHAINLIT_UI, RedirectResponse, and JSONResponse by name.i8n/fr.json (1)
1-1: Directory namei8n/uses a non-standard spelling; rename toi18n/for consistency with convention.
i18nis the established abbreviation (i + 18 letters + n) used across the industry. The unconventional spellingi8nmay surprise contributors familiar with standard tooling that detectsi18n/paths. Renaming requires only updating the mount path indocker-compose.yaml(./i8n:/app/openrag/.chainlit/translations→./i18n:/app/openrag/.chainlit/translations). No code references the path directly, so the change is low-risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@i8n/fr.json` at line 1, Rename the non-standard directory "i8n" to "i18n" and update any references to it (particularly the docker-compose mount) so tooling and contributors use the conventional i18n path; specifically, rename the folder from i8n → i18n and change the docker-compose mount string "./i8n:/app/openrag/.chainlit/translations" to "./i18n:/app/openrag/.chainlit/translations", and scan for and update any CI, README, or deployment references that mention the old "i8n" name.openrag/scripts/migrations/alembic/versions/f5b6c918f741_add_oidc_auth.py (1)
39-45: Remove redundantindex=Truefromsession_token_hash.SQLAlchemy's
unique=Trueimplicitly creates a unique index. Addingindex=Trueon the same column creates a second, redundant non-unique index — unnecessary write amplification and wasted storage on a hot-path column (used for every session validation). Dropindex=Trueon lines 43–44 of the migration and line 120 of the model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/scripts/migrations/alembic/versions/f5b6c918f741_add_oidc_auth.py` around lines 39 - 45, The column definition for session_token_hash currently sets both unique=True and index=True, which creates a redundant non-unique index; remove index=True from the sa.Column(...) declaration in the migration (the Column named "session_token_hash") and also remove index=True from the corresponding model field named session_token_hash so only unique=True remains.openrag/components/indexer/vectordb/utils.py (1)
18-27: Use the repository’s absolute import style here.This new relative import crosses the package boundary style used by the repo; switch it to the Python-path-root import. As per coding guidelines,
**/*.py: Use absolute imports from theopenrag/directory (Python path root) instead of relative imports across packages.♻️ Proposed import adjustment
-from .models import ( +from components.indexer.vectordb.models import ( Base, File, OIDCSession,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/indexer/vectordb/utils.py` around lines 18 - 27, Replace the relative import in utils.py with the repository’s absolute import: import Base, File, OIDCSession, Partition, PartitionMembership, User, Workspace, WorkspaceFile from openrag.components.indexer.vectordb.models instead of using a relative import; update the single import statement that currently references ".models" to use the full python-path-root module path so the symbols Base, File, OIDCSession, Partition, PartitionMembership, User, Workspace, and WorkspaceFile are imported absolutely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fe0b5e8f-3d1b-4bf4-ae45-735168004248
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (54)
.env.example.gitignoreCLAUDE.mdREADME.mdconf/config.yamldocker-compose.yamldocs/content/docs/documentation/env_vars.mddocs/content/docs/documentation/setup_indexerui.mddocs/oidc.mddocs/sso-quickstart.mdextern/indexer-uii8n/en-US.jsoni8n/fr.jsonopenrag/api.pyopenrag/app_front.pyopenrag/components/auth/__init__.pyopenrag/components/auth/deps.pyopenrag/components/auth/middleware.pyopenrag/components/auth/oidc_client.pyopenrag/components/auth/refresh.pyopenrag/components/auth/session_tokens.pyopenrag/components/auth/state_cookie.pyopenrag/components/auth/test_middleware.pyopenrag/components/auth/test_oidc_client.pyopenrag/components/auth/test_session_tokens.pyopenrag/components/auth/test_state_cookie.pyopenrag/components/indexer/loaders/audio/local_whisper.pyopenrag/components/indexer/loaders/pdf_loaders/docling2.pyopenrag/components/indexer/loaders/pdf_loaders/marker.pyopenrag/components/indexer/vectordb/models.pyopenrag/components/indexer/vectordb/test_oidc_sessions.pyopenrag/components/indexer/vectordb/utils.pyopenrag/components/indexer/vectordb/vectordb.pyopenrag/components/ray_utils.pyopenrag/config/models.pyopenrag/models/user.pyopenrag/routers/auth.pyopenrag/routers/openai.pyopenrag/routers/test_auth_router.pyopenrag/routers/users.pyopenrag/routers/utils.pyopenrag/scripts/migrations/alembic/env.pyopenrag/scripts/migrations/alembic/schema_helpers.pyopenrag/scripts/migrations/alembic/versions/4add4d260575_initial_migration.pyopenrag/scripts/migrations/alembic/versions/a1b2c3d4e5f6_add_document_relationships.pyopenrag/scripts/migrations/alembic/versions/c224d4befe71_add_file_count_and_file_quota.pyopenrag/scripts/migrations/alembic/versions/cd642e4502d8_create_users_memberships_tables.pyopenrag/scripts/migrations/alembic/versions/e7f8a9b0c1d2_add_workspaces.pyopenrag/scripts/migrations/alembic/versions/f1a2b3c4d5e6_add_workspace_files_file_id_fk.pyopenrag/scripts/migrations/alembic/versions/f5b6c918f741_add_oidc_auth.pypyproject.tomlquick_start/docker-compose.yamltests/api_tests/OIDC_TEST_COVERAGE.mdtests/api_tests/test_oidc_lifecycle.py
| try: | ||
| async with httpx.AsyncClient(timeout=httpx.Timeout(30.0)) as client: | ||
| response = await client.get( | ||
| url=f"{INTERNAL_BASE_URL}/users/info", | ||
| headers=get_headers(session_token), | ||
| ) | ||
| response.raise_for_status() | ||
| data = response.json() | ||
| except httpx.HTTPStatusError as e: | ||
| logger.info("Session cookie rejected by /users/info", status=e.response.status_code) | ||
| return None | ||
| except Exception as e: | ||
| logger.exception("Chainlit header_auth_callback failure", error=str(e)) | ||
| return None | ||
|
|
||
| return cl.User( | ||
| identifier=data.get("display_name", "user"), | ||
| metadata={ | ||
| "role": "admin" if data.pop("is_admin", False) else "user", | ||
| "provider": "oidc", | ||
| "api_key": session_token, # opaque cookie value — used as Bearer for internal calls | ||
| "extra": data, |
There was a problem hiding this comment.
Do not treat the OIDC session cookie as an API token.
Line 155 sends the openrag_session value as Authorization: Bearer, and Line 171 stores it as api_key; downstream, file source URLs append ?token={api_key}, which can expose the httpOnly session token in browser history, logs, and Referer headers. Keep OIDC sessions in the Cookie header and avoid using them as URL tokens; use a separate auth-header helper or short-lived file access token instead. Based on learnings, when AUTH_MODE=oidc, use opaque session tokens in an httpOnly openrag_session cookie.
🛡️ Direction for the fix
- response = await client.get(
- url=f"{INTERNAL_BASE_URL}/users/info",
- headers=get_headers(session_token),
- )
+ response = await client.get(
+ url=f"{INTERNAL_BASE_URL}/users/info",
+ headers={
+ "accept": "application/json",
+ "Cookie": f"openrag_session={session_token}",
+ },
+ )
@@
- "api_key": session_token, # opaque cookie value — used as Bearer for internal calls
+ # Do not store the OIDC cookie value as api_key; keep auth mode separate
+ # and ensure source/file URLs do not include the session token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/app_front.py` around lines 151 - 172, The code is treating the OIDC
session cookie as an API token (used in get_headers(session_token) as Bearer and
stored as "api_key" in cl.User), which risks leaking the httpOnly session in
URLs; change header handling and user metadata so the opaque openrag_session
remains only in Cookie headers and never used as a URL or Bearer token.
Concretely: update get_headers usage and any header_auth_callback or auth helper
to stop setting Authorization: Bearer when AUTH_MODE== "oidc" and instead keep
the session cookie in Cookie header; remove storing session_token as "api_key"
in the cl.User metadata (replace with a separate short-lived file access token
key or null e.g., "file_token" generated by a new helper like
create_file_access_token(session) for file URLs); ensure downstream code that
appends ?token={api_key} uses the new short-lived file token instead of the OIDC
cookie. Ensure logging/error paths (e.g., header_auth_callback and the cl.User
construction) are updated accordingly.
| # Browser-facing paths — unauthenticated access in oidc mode → 302 /auth/login. | ||
| _UI_PATH_PREFIXES = ("/static",) | ||
|
|
||
|
|
||
| def is_ui_path(path: str) -> bool: | ||
| """True if this path is a browser-facing page (UI), not an API route. | ||
|
|
||
| Used only in ``AUTH_MODE=oidc`` to decide between a 302 redirect to | ||
| ``/auth/login`` (UI) and a 401 JSON response (API). When in doubt we | ||
| return ``False`` to avoid redirect loops on non-browser clients. | ||
| """ | ||
| if path == "/": | ||
| return True | ||
| if any(path.startswith(p) for p in _API_PREFIXES): | ||
| return False | ||
| if any(path.startswith(p) for p in _UI_PATH_PREFIXES): | ||
| return True | ||
| return False | ||
|
|
||
|
|
||
| def is_bypass_path(path: str) -> bool: | ||
| return path in _BYPASS_PATHS or path.startswith("/chainlit") |
There was a problem hiding this comment.
Don’t bypass authentication for /chainlit; classify it as UI instead.
Line 96 lets all /chainlit... requests through before session validation, so OIDC mode will not redirect unauthenticated Chainlit UI requests or bind request.state.user.
Proposed fix
-_UI_PATH_PREFIXES = ("/static",)
+_UI_PATH_PREFIXES = ("/static", "/chainlit")
@@
def is_bypass_path(path: str) -> bool:
- return path in _BYPASS_PATHS or path.startswith("/chainlit")
+ return path in _BYPASS_PATHSBased on learnings, In OIDC mode, return 302 redirects to /auth/login?next=... for UI paths (/, /chainlit, /static) without auth; return 401 JSON for API paths without auth.
📝 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.
| # Browser-facing paths — unauthenticated access in oidc mode → 302 /auth/login. | |
| _UI_PATH_PREFIXES = ("/static",) | |
| def is_ui_path(path: str) -> bool: | |
| """True if this path is a browser-facing page (UI), not an API route. | |
| Used only in ``AUTH_MODE=oidc`` to decide between a 302 redirect to | |
| ``/auth/login`` (UI) and a 401 JSON response (API). When in doubt we | |
| return ``False`` to avoid redirect loops on non-browser clients. | |
| """ | |
| if path == "/": | |
| return True | |
| if any(path.startswith(p) for p in _API_PREFIXES): | |
| return False | |
| if any(path.startswith(p) for p in _UI_PATH_PREFIXES): | |
| return True | |
| return False | |
| def is_bypass_path(path: str) -> bool: | |
| return path in _BYPASS_PATHS or path.startswith("/chainlit") | |
| # Browser-facing paths — unauthenticated access in oidc mode → 302 /auth/login. | |
| _UI_PATH_PREFIXES = ("/static", "/chainlit") | |
| def is_ui_path(path: str) -> bool: | |
| """True if this path is a browser-facing page (UI), not an API route. | |
| Used only in ``AUTH_MODE=oidc`` to decide between a 302 redirect to | |
| ``/auth/login`` (UI) and a 401 JSON response (API). When in doubt we | |
| return ``False`` to avoid redirect loops on non-browser clients. | |
| """ | |
| if path == "/": | |
| return True | |
| if any(path.startswith(p) for p in _API_PREFIXES): | |
| return False | |
| if any(path.startswith(p) for p in _UI_PATH_PREFIXES): | |
| return True | |
| return False | |
| def is_bypass_path(path: str) -> bool: | |
| return path in _BYPASS_PATHS |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/auth/middleware.py` around lines 75 - 96, The code
currently treats "/chainlit" as an authentication bypass in is_bypass_path,
causing Chainlit UI requests to skip session binding and OIDC redirects; remove
the path.startswith("/chainlit") check from is_bypass_path and instead classify
Chainlit as a UI path by adding "/chainlit" to _UI_PATH_PREFIXES (or updating
is_ui_path to treat paths starting with "/chainlit" as UI) so unauthenticated
browser requests to "/" and "/chainlit" get 302 → /auth/login?next=... while API
routes still receive 401 JSON responses; keep _BYPASS_PATHS handling for true
API bypasses unchanged.
| vectordb = self._get_vectordb() | ||
|
|
||
| # --- Dev mode: AUTH_MODE=token + AUTH_TOKEN unset → user 1 bypass. | ||
| if auth_mode == "token" and auth_token is None: | ||
| user = await vectordb.get_user.remote(1) | ||
| user_partitions = await vectordb.list_user_partitions.remote(1) | ||
| request.state.user = user | ||
| request.state.user_partitions = user_partitions | ||
| request.state.oidc_session = None | ||
| return await call_next(request) | ||
|
|
||
| # --- Bypass list (docs, health, /auth/* callbacks, chainlit). | ||
| path = request.url.path | ||
| if is_bypass_path(path): |
There was a problem hiding this comment.
Check bypass paths before touching VectorDB.
Line 117 runs _get_vectordb() for /health_check, /docs, /auth/callback, etc.; in token dev mode Lines 120-126 can even await user lookups before bypassing. A Ray/DB outage would break public health/auth endpoints.
Proposed fix
- vectordb = self._get_vectordb()
+ # --- Bypass list (docs, health, /auth/* callbacks).
+ path = request.url.path
+ if is_bypass_path(path):
+ return await call_next(request)
+
+ vectordb = self._get_vectordb()
# --- Dev mode: AUTH_MODE=token + AUTH_TOKEN unset → user 1 bypass.
if auth_mode == "token" and auth_token is None:
@@
- # --- Bypass list (docs, health, /auth/* callbacks, chainlit).
- path = request.url.path
- if is_bypass_path(path):
- return await call_next(request)
-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/auth/middleware.py` around lines 117 - 130, The code
currently calls self._get_vectordb() before checking is_bypass_path, which can
cause DB/Ray outages to break public endpoints; refactor the middleware to check
the request path via is_bypass_path(request.url.path) first and return await
call_next(request) for bypassed routes (docs, health, /auth/*, chainlit) without
calling _get_vectordb(); then only call self._get_vectordb() for non-bypass
flows and perform the dev-mode token branch (await vectordb.get_user.remote and
list_user_partitions.remote) after the bypass check so the dev bypass logic
remains but only runs when the route is not a bypass.
| cookie_token = request.cookies.get(SESSION_COOKIE_NAME) if auth_mode == "oidc" else None | ||
| if cookie_token: | ||
| session = await vectordb.get_oidc_session_by_token.remote(cookie_token) | ||
| if session is not None: | ||
| refreshed = await refresh_session_if_needed( | ||
| session=session, | ||
| enc_key=enc_key, | ||
| vectordb=vectordb, | ||
| ) | ||
| if refreshed is None: | ||
| # Refresh failed or session unusable → revoke and fall through. | ||
| try: | ||
| await vectordb.revoke_oidc_session_by_id.remote(session["id"]) | ||
| except Exception as e: | ||
| logger.bind(error=str(e)).warning("Failed to revoke invalid OIDC session") | ||
| session = None | ||
| else: | ||
| session = refreshed | ||
| user = await vectordb.get_user.remote(session["user_id"]) | ||
|
|
||
| # --- 2) Fallback: Bearer / ?token= (programmatic clients + internal | ||
| # callers like Chainlit's header_auth_callback which forwards | ||
| # the browser cookie value in the Authorization header). | ||
| if user is None: | ||
| token = None | ||
| if path.startswith("/static"): | ||
| token = getattr(request.state, "original_token", None) | ||
| else: | ||
| auth = request.headers.get("authorization", "") | ||
| if auth and auth.lower().startswith("bearer "): | ||
| token = auth.split(" ", 1)[1] | ||
|
|
||
| if token is not None: | ||
| # In oidc mode, a Bearer may carry an OIDC session token (not | ||
| # a ``users.token`` hash). Try the session lookup first with | ||
| # the same lazy-refresh semantics as the cookie branch above. | ||
| if auth_mode == "oidc": | ||
| session = await vectordb.get_oidc_session_by_token.remote(token) | ||
| if session is not None: | ||
| refreshed = await refresh_session_if_needed( | ||
| session=session, | ||
| enc_key=enc_key, | ||
| vectordb=vectordb, | ||
| ) | ||
| if refreshed is None: | ||
| try: | ||
| await vectordb.revoke_oidc_session_by_id.remote(session["id"]) | ||
| except Exception as e: | ||
| logger.bind(error=str(e)).warning("Failed to revoke invalid OIDC session (bearer path)") | ||
| session = None | ||
| else: | ||
| session = refreshed | ||
| user = await vectordb.get_user.remote(session["user_id"]) | ||
|
|
||
| if user is None: | ||
| # Either token mode, or oidc mode with no matching session | ||
| # — fall back to the long-lived ``users.token`` used by | ||
| # programmatic clients (CI, scripts, service agents). | ||
| user = await vectordb.get_user_by_token.remote(token) | ||
| if not user and auth_mode == "token": | ||
| # Legacy test contract: robot suite asserts 403 + "Invalid token". | ||
| return JSONResponse(status_code=403, content={"detail": "Invalid token"}) | ||
| elif auth_mode == "token": | ||
| # Token mode: no cookie + no bearer → legacy 403 "Missing token". | ||
| return JSONResponse(status_code=403, content={"detail": "Missing token"}) | ||
|
|
||
| # --- 3) Unauthenticated: redirect UI in oidc mode, else 401 JSON. | ||
| if user is None: | ||
| if auth_mode == "oidc" and is_ui_path(path): | ||
| next_path = path | ||
| if request.url.query: | ||
| next_path = f"{path}?{request.url.query}" | ||
| return RedirectResponse( | ||
| url=f"/auth/login?next={quote(next_path, safe='')}", | ||
| status_code=302, | ||
| ) | ||
| return JSONResponse(status_code=401, content={"detail": "Unauthenticated"}) | ||
|
|
||
| # --- Happy path: user resolved. | ||
| request.state.user = user | ||
| request.state.user_partitions = await vectordb.list_user_partitions.remote(user["id"]) |
There was a problem hiding this comment.
Wrap VectorDB actor calls with the Ray timeout helper.
All await vectordb.*.remote(...) calls here can hang the request indefinitely if the actor stalls. Route authentication is a hot path; use call_ray_actor_with_timeout() consistently.
As per coding guidelines, Call Ray actors with timeout and cancellation handling using call_ray_actor_with_timeout() from components.ray_utils, which handles timeout, cancellation, RayTaskError, and TaskCancelledError.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/auth/middleware.py` around lines 140 - 220, Replace direct
awaits of Ray actor calls with the Ray timeout helper: import
call_ray_actor_with_timeout from components.ray_utils and wrap every
vectordb.*.remote(...) invocation (get_oidc_session_by_token.remote,
revoke_oidc_session_by_id.remote, get_user.remote, get_user_by_token.remote,
list_user_partitions.remote) with call_ray_actor_with_timeout(..., timeout=...)
so the calls time out and propagate cancellations consistently; keep
refresh_session_if_needed usage as-is but ensure any internal vectordb.*.remote
calls it performs are also wrapped similarly, and preserve existing exception
handling (revocation logging and token-mode returns) when replacing the await
calls.
| token = None | ||
| if path.startswith("/static"): | ||
| token = getattr(request.state, "original_token", None) | ||
| else: | ||
| auth = request.headers.get("authorization", "") | ||
| if auth and auth.lower().startswith("bearer "): | ||
| token = auth.split(" ", 1)[1] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expectation: if no assignment to request.state.original_token exists before AuthMiddleware,
# /static?token=... currently cannot authenticate via the documented query token path.
rg -n -C3 '\boriginal_token\b|query_params\.get\(["'\'']token["'\'']\)'Repository: linagora/openrag
Length of output: 1190
🏁 Script executed:
# Check docstring for the middleware and surrounding context
cat -n openrag/components/auth/middleware.py | head -180 | tail -50Repository: linagora/openrag
Length of output: 2889
🏁 Script executed:
# Examine the full openrag/api.py to understand request lifecycle and middleware setup
head -250 openrag/api.py | tail -100Repository: linagora/openrag
Length of output: 3402
🏁 Script executed:
# Search for any documentation about /static token support
rg -n "static.*token|token.*static" -A 3 -B 3Repository: linagora/openrag
Length of output: 1245
Restore ?token= parsing for /static to match documented behavior.
The docstring (line 10) explicitly documents that both Authorization: Bearer ... and ?token= work for /static routes. However, the current code uses an if/else that prevents /static from checking the Authorization header—it only reads from request.state.original_token.
While TokenRedactingMiddleware (in openrag/api.py) does extract tokens from the query string and set original_token before this middleware runs, the middleware should not rely on undocumented upstream behavior. Additionally, the code doesn't explicitly attempt query_params.get("token"), making the token flow implicit.
The proposed fix ensures:
- Both Bearer headers and query tokens work for
/staticroutes, matching the documented contract - Direct access to query params (defensive against middleware changes)
- Authorization header fallback is available for all paths
Proposed fix
token = None
if path.startswith("/static"):
- token = getattr(request.state, "original_token", None)
- else:
+ token = request.query_params.get("token") or getattr(request.state, "original_token", None)
+ if token is None:
auth = request.headers.get("authorization", "")
if auth and auth.lower().startswith("bearer "):
token = auth.split(" ", 1)[1]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/auth/middleware.py` around lines 164 - 170, The middleware
currently only reads request.state.original_token for paths starting with
"/static", which prevents reading ?token= and Authorization headers as
documented; update the token extraction in the middleware (the block that
assigns token) to: 1) check query_params.get("token") on the request first for
"/static"; 2) if absent, check the Authorization header
(auth.lower().startswith("bearer ")) and extract the bearer token; 3) if still
absent, fallback to request.state.original_token; ensure non-/static paths keep
the existing Authorization-header behavior. Reference symbols: token variable,
request.state.original_token, authorization header handling, and
TokenRedactingMiddleware (which may set original_token).
| if _claim_source() == "userinfo": | ||
| try: | ||
| claims_for_mapping: dict[str, Any] = await client.fetch_userinfo(bundle.access_token) | ||
| except Exception as e: | ||
| logger.warning(f"OIDC userinfo fetch failed: {e}") | ||
| return _json_error( | ||
| status.HTTP_400_BAD_REQUEST, | ||
| "Failed to fetch userinfo from IdP.", | ||
| delete_state_cookie=True, | ||
| ) | ||
| else: | ||
| claims_for_mapping = bundle.claims |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenID Connect Core UserInfo Response sub claim must exactly match ID Token sub claim
💡 Result:
Yes, according to the OpenID Connect Core 1.0 specification, the 'sub' claim in the UserInfo Response MUST exactly match the 'sub' claim in the corresponding ID Token. This is a required validation step for Relying Parties (clients) to prevent token substitution attacks. The specification states in Section 5.3.2 (Successful UserInfo Response): "The UserInfo Response MUST include the sub (subject) Claim." And importantly: "Note: Due to the possibility of token substitution attacks (see Section 16.11), the UserInfo Response is not guaranteed to be about the End-User identified by the sub Claim of the ID Token. Therefore, the sub Claim in the UserInfo Response MUST be verified to match the sub Claim in the ID Token. If they do not match, the UserInfo Response MUST NOT be used."
Citations:
- 1: https://openid.net/specs/openid-connect-core-1_0.html
- 2: https://zenn.dev/maronn/articles/need-sub-in-id-token?locale=en
- 3: https://openid.net/specs/openid-connect-core-1_0-errata2.html
🏁 Script executed:
# First, find the exact location and get broader context around lines 312-323
head -n 350 openrag/routers/auth.py | tail -n 100 | cat -nRepository: linagora/openrag
Length of output: 4560
🏁 Script executed:
# Also search for any existing sub validation in the file
rg "sub.*userinfo\|userinfo.*sub" openrag/routers/auth.py -A 3 -B 3Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Look for where `sub` variable is used and check if validation exists
rg "claims_for_mapping.*sub|\.get\(.*sub\)" openrag/routers/auth.py -A 2 -B 2Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Get the full callback implementation to see the complete flow
sed -n '250,400p' openrag/routers/auth.py | cat -nRepository: linagora/openrag
Length of output: 6780
🏁 Script executed:
# Check if there's any validation of userinfo sub elsewhere in the file
rg "fetch_userinfo" openrag/routers/auth.py -A 20 | head -60Repository: linagora/openrag
Length of output: 914
🏁 Script executed:
# Look at the OIDCClient.fetch_userinfo implementation to understand what it returns
fd -name "*.py" -path "*/auth*" | xargs rg "def fetch_userinfo" -A 15Repository: linagora/openrag
Length of output: 1650
Verify UserInfo sub before applying mapped claims.
When OIDC_CLAIM_SOURCE=userinfo, the code fetches claims without validating that the UserInfo response belongs to the same subject. Per OpenID Connect Core specification (Section 5.3.2), the sub claim in the UserInfo response MUST exactly match the sub claim in the ID token; if they do not match, the UserInfo response must be rejected. An attacker who compromises or replays the access token could use it to fetch userinfo for a different user and apply those mapped claims to the wrong user record.
Proposed fix
if _claim_source() == "userinfo":
try:
claims_for_mapping: dict[str, Any] = await client.fetch_userinfo(bundle.access_token)
except Exception as e:
logger.warning(f"OIDC userinfo fetch failed: {e}")
return _json_error(
status.HTTP_400_BAD_REQUEST,
"Failed to fetch userinfo from IdP.",
delete_state_cookie=True,
)
+ if claims_for_mapping.get("sub") != sub:
+ logger.warning("OIDC userinfo subject mismatch")
+ return _json_error(
+ status.HTTP_400_BAD_REQUEST,
+ "OIDC userinfo subject mismatch.",
+ delete_state_cookie=True,
+ )
else:
claims_for_mapping = bundle.claims🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/routers/auth.py` around lines 312 - 323, When OIDC_CLAIM_SOURCE is
"userinfo", after calling client.fetch_userinfo(bundle.access_token) (see
_claim_source() and client.fetch_userinfo usage) validate that the "sub" in the
returned claims_for_mapping exactly matches the "sub" from the ID token
(bundle.claims["sub"]); if the "sub" is missing or does not match, treat it as a
failure and return the same error path (_json_error with 400 and "Failed to
fetch userinfo from IdP." and delete_state_cookie=True) instead of applying the
fetched claims; keep the existing exception handling for fetch failures.
| path="/", | ||
| ) | ||
|
|
||
| logger.info(f"OIDC login success — user_id={user['id']}, sid={sid!r}, next={next_url!r}") |
There was a problem hiding this comment.
Avoid logging user-controlled redirect URLs.
next_url can include query parameters from the original request. Logging it on successful login can leak tokens or PII; bind structured fields and omit or sanitize the redirect target.
Proposed fix
- logger.info(f"OIDC login success — user_id={user['id']}, sid={sid!r}, next={next_url!r}")
+ logger.bind(user_id=user["id"], sid=sid).info("OIDC login success")As per coding guidelines, Use Loguru with structured logging via get_logger() from utils.logger, and use .bind() for contextual fields like file_id and partition.
📝 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.
| logger.info(f"OIDC login success — user_id={user['id']}, sid={sid!r}, next={next_url!r}") | |
| logger.bind(user_id=user["id"], sid=sid).info("OIDC login success") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/routers/auth.py` at line 391, The current log line exposes a
user-controlled redirect URL (next_url); replace free-form string interpolation
with structured logging and do not log the raw next_url. Import and use
get_logger() from utils.logger, call get_logger().bind(user_id=user['id'],
sid=sid) and then .info with a fixed message like "OIDC login success" (omit
next_url) or, if you must record redirect intent, bind a sanitized field (e.g.,
redirect_domain or redirect_safe=true) instead of the full URL; ensure no raw
next_url appears in the log output.
| # Replace the OIDCClient's internal httpx client with one backed by respx. | ||
| # respx >= 0.22 removed the top-level MockTransport; use MockRouter + | ||
| # httpx.MockTransport(router.handler) instead. | ||
| router = respx.MockRouter(assert_all_called=False) | ||
| http = httpx.AsyncClient(transport=httpx.MockTransport(router.handler)) | ||
|
|
||
| # Force singleton creation using our mocked http client. | ||
| _auth_deps.reset_oidc_client() | ||
| _auth_deps._client = _auth_router_module.OIDCClient( | ||
| issuer=ISSUER, | ||
| client_id=CLIENT_ID, | ||
| client_secret=CLIENT_SECRET, | ||
| redirect_uri=REDIRECT_URI, | ||
| scopes=SCOPES, | ||
| http_client=http, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the pinned respx version and whether it exposes async_handler.
rg -nP '^\s*respx\s*[=<>!~]' pyproject.toml uv.lock requirements*.txt 2>/dev/null
fd -i 'pyproject.toml|uv.lock' -x rg -nP 'respx' {} \;Repository: linagora/openrag
Length of output: 772
🌐 Web query:
respx 0.23.1 async_handler MockRouter AsyncClient documentation
💡 Result:
In respx 0.23.1, MockRouter (provided by the pytest fixture respx_mock or returned by respx.mock) is a configured Router instance used for mocking HTTPX requests. It supports adding routes with methods like .get, .post, etc., and mocking responses with .mock(return_value=...) or .respond(...). To use with AsyncClient without patching global HTTPX (manual transport setup): python import httpx import respx router = respx.Router # or respx.mock instance, aka MockRouter router.post("https://example.org/").mock(return_value=httpx.Response(200)) async def test_client: transport = httpx.MockTransport(router.async_handler) async with httpx.AsyncClient(transport=transport) as client: response = await client.post("https://example.org/") assert response.status_code == 200 # Manually call if needed: router.assert_all_called The async_handler is an async callable on Router/MockRouter instances that handles HTTP Core async requests, compatible with httpx.MockTransport for AsyncClient. It processes routes, returns mocked httpx.Response, or raises assertions like AllMockedAssertionError if configured. Official documentation: https://lundberg.github.io/respx/ (User Guide and API Reference cover Router methods; source code confirms async_handler). PyPI confirms 0.23.1 released 2026-04-08, requires HTTPX >=0.25.0.
Citations:
- 1: https://pypi.org/project/respx/0.23.1/
- 2: https://lundberg.github.io/respx/
- 3: https://pypi.org/project/respx/
- 4: https://lundberg.github.io/respx/guide/
- 5: https://lundberg.github.io/respx/api/
- 6: https://github.com/lundberg/respx/blob/master/respx/mocks.py
🏁 Script executed:
# Check the actual test file to see if there are any test failures or skip markers
cat -n openrag/routers/test_auth_router.py | head -50Repository: linagora/openrag
Length of output: 2165
🏁 Script executed:
# Search for any other usages of respx.MockRouter in the codebase to see patterns
rg -n 'MockRouter|async_handler|router\.handler' --type pyRepository: linagora/openrag
Length of output: 1704
Use router.async_handler with httpx.AsyncClient, not router.handler.
respx 0.23.1 (pinned in uv.lock) explicitly requires router.async_handler when wrapping in httpx.MockTransport(...) for an AsyncClient. The official documentation shows:
transport = httpx.MockTransport(router.async_handler)
async with httpx.AsyncClient(transport=transport) as client:
...Using the sync .handler is incorrect and incompatible with the async client. The inline comment at line 323 incorrectly prescribes .handler and should be updated as well.
🧪 Proposed fix
- router = respx.MockRouter(assert_all_called=False)
- http = httpx.AsyncClient(transport=httpx.MockTransport(router.handler))
+ router = respx.MockRouter(assert_all_called=False)
+ http = httpx.AsyncClient(transport=httpx.MockTransport(router.async_handler))Also update the comment at line 323 to reference router.async_handler instead of router.handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/routers/test_auth_router.py` around lines 322 - 337, The test uses
respx.MockRouter with httpx.MockTransport but passes the sync router.handler to
httpx.AsyncClient; change to use router.async_handler when constructing
httpx.MockTransport for an AsyncClient and update the inline comment
accordingly. Specifically, replace usage of router.handler with
router.async_handler where httpx.MockTransport(...) is created (the block that
forces singleton creation via _auth_deps.reset_oidc_client() and
_auth_deps._client = OIDCClient(..., http_client=http)), and update the comment
that currently mentions `.handler` to reference `.async_handler`.
| environment: | ||
| - API_BASE_URL=${API_BASE_URL:-http://localhost:${APP_PORT:-8080}} | ||
| - INCLUDE_CREDENTIALS=${INCLUDE_CREDENTIALS:-false} | ||
| - DEFAULT_LANGUAGE=${DEFAULT_LANGUAGE:-} |
There was a problem hiding this comment.
Pass AUTH_MODE to the quick-start indexer UI too.
DEFAULT_LANGUAGE is forwarded, but AUTH_MODE is still missing here. Quick-start OIDC deployments can end up with the API in OIDC mode while the UI keeps token-mode defaults.
🔧 Proposed compose update
environment:
- API_BASE_URL=${API_BASE_URL:-http://localhost:${APP_PORT:-8080}}
+ - AUTH_MODE=${AUTH_MODE:-token}
- INCLUDE_CREDENTIALS=${INCLUDE_CREDENTIALS:-false}
- DEFAULT_LANGUAGE=${DEFAULT_LANGUAGE:-}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@quick_start/docker-compose.yaml` around lines 63 - 66, The quick-start
docker-compose environment block for the indexer UI is missing AUTH_MODE,
causing UI and API auth mismatches; update the environment list alongside
API_BASE_URL, INCLUDE_CREDENTIALS, and DEFAULT_LANGUAGE to also forward
AUTH_MODE (e.g., - AUTH_MODE=${AUTH_MODE:-token}) so the UI receives the same
authentication mode as the API and stays in sync with OIDC deployments.
| # ── Step 5: GET /users/info with session cookie → 200 alice profile ─────── | ||
| r3 = client.get("/users/info", cookies={"openrag_session": session_cookie}) | ||
| # The users router depends on task_state_manager for file counts; since we | ||
| # stub it as None the endpoint may 500 on full wiring — we accept 200 or | ||
| # verify the middleware resolved alice (status != 401/302). | ||
| assert r3.status_code not in (401, 302), f"Middleware should resolve alice, got {r3.status_code}: {r3.text}" |
There was a problem hiding this comment.
Assert the protected endpoint succeeds, not just that auth was attempted.
Line 397 currently allows 500, so this lifecycle test can pass even when /users/info is broken after OIDC login. Please make the stub complete enough for the endpoint and assert r3.status_code == 200 plus the expected Alice identity; otherwise this won’t catch session/user integration regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/api_tests/test_oidc_lifecycle.py` around lines 392 - 397, The test
currently only checks that the middleware attempted auth by asserting
r3.status_code not in (401, 302), which allows 500s; update the test to require
a successful protected-endpoint response by asserting r3.status_code == 200
after client.get("/users/info", cookies={"openrag_session": session_cookie}),
and assert the response contains the expected Alice identity (e.g.,
username/email in r3.json() or r3.text). To enable this, make the
task_state_manager stub used by the users router provide the minimal data the
endpoint expects (file counts or any attributes the users.info handler reads) so
the endpoint can complete rather than error; reference r3,
client.get("/users/info"), session_cookie and task_state_manager when locating
and updating the code.
4 participants
v1.1.9
Summary by CodeRabbit
New Features
Configuration
Documentation