Skip to content

🔒 fix: close /query and /query_multiple authorization holes; add /v1/embeddings and /v1/rerank - #318

Open
danny-avila wants to merge 20 commits into
mainfrom
feat/search-stack-v1
Open

danny-avila wants to merge 20 commits into
mainfrom
feat/search-stack-v1

Conversation

@danny-avila

@danny-avila danny-avila commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Summary

Closes two authorization holes in the retrieval endpoints, then builds two new
service endpoints (POST /v1/embeddings, POST /v1/rerank) on top of the
fixed authorization model. Also removes the default database credentials that
ship in this repository.

The security fixes stand on their own and are the reason to review this first.


1. Security fixes

1a. POST /query authorized a whole result set from the first returned hit

query_embeddings_by_file_id ran the vector search filtered by file_id
alone, then inspected documents[0] — the top-ranked hit — and returned
either the entire result list or nothing:

documents = await vector_store.asimilarity_search_with_score_by_vector(
    embedding, k=body.k, filter={"file_id": {"$eq": body.file_id}}, ...
)
document, score = documents[0]
doc_user_id = document.metadata.get("user_id")
if doc_user_id is None or doc_user_id == user_authorized:
    authorized_documents = documents      # every hit, checked or not

Two independent defects:

  • Hits 2..k were never checked. When one file_id holds chunks from more
    than one owner, a caller whose own chunk ranks first receives every other
    owner's chunk in the same response. Chunks are stored with custom_id = file_id, so co-owned file_id values are a normal state, not a corrupted
    one.
  • doc_user_id is None returned everything. Any chunk written without
    user_id metadata was readable by every caller.

1b. entity_id was taken as the identity to compare against

The identity the check compared to was chosen from the request body:

user_authorized = body.entity_id if body.entity_id else request.state.user.get("id")

entity_id is caller-controlled. Sending {"file_id": "<victim's file>", "entity_id": "<victim's user id>"} made doc_user_id == user_authorized
true, and the victim's chunks came back verbatim. This required no token
beyond a valid one of the attacker's own — it is a full read of another user's
file content.

1c. POST /query_multiple had no authorization at all

query_embeddings_by_file_ids filtered on file_id only and returned the
result set unconditionally. Any authenticated caller who could name (or
enumerate) a file_id read its chunks. GET /ids returns every custom_id
in the store, so the ids were not a secret.

The fix

Authorization moved into the query predicate, before ranking:

scope = resolve_scope(request, body.entity_id)
query_filter = scope.predicate(file_clause(body.file_id))
documents = await vector_store.asimilarity_search_with_score_by_vector(
    embedding, k=body.k, filter=query_filter, ...
)
return _apply_distance_threshold(documents)
  • The predicate is (tenant, owner/entity, file_id). An out-of-scope chunk
    cannot rank, cannot be counted against k, and is never read out of the
    database — as opposed to being filtered out of a result set afterwards.
  • No result-derived authorization remains anywhere. Nothing downstream reads
    metadata["user_id"] to decide anything.
  • Owner scope comes from the verified token. entity_id widens it only when
    the token's entities claim names that entity; otherwise the request is
    403.
  • One builder (app/scope.py) produces the predicate for every path —
    /query, /query_multiple, and the rerank stored-vector lookup — so the
    three cannot drift apart.

1d. Authorize before egress

/v1/rerank accepts candidate text and sends it to an inference provider.
Text that reaches a provider has left the trust boundary whether or not the
caller ever sees a response, so filtering the response is not sufficient.

Every candidate id is probed against the store before anything is embedded.
The probe reads metadata only — no vectors, no document text. An id that
exists in the store but resolves to nothing inside the caller's scope makes
the whole request 403 with zero egress. Ids that match nothing (web-scrape
candidates, synthetic ids) are unaffected. If the probe cannot run, the
request fails closed with 503 rather than embedding text it could not check;
there is no store-without-a-probe fallback.

/v1/embeddings reads no store at all — it embeds only text the authenticated
caller supplied — and rejects on scope, quota and limits before calling the
backend.

1e. Signing-key separation

JWT_SECRET verifies tokens minted by the calling application. Before this
change the same secret was the only key, so a token minted for this service
was simultaneously a full session token for the caller's own API, and vice
versa — the confusion runs in both directions.

RAG_JWT_SECRET is now a dedicated key for this service, and startup refuses
to proceed if it equals JWT_SECRET. The /v1 endpoints accept only
RAG_JWT_SECRET-signed tokens, in every mode: a captured application session
token cannot reach them, whatever claims it carries.

1f. Raw exception text in 500 responses

The database-facing routes returned detail=str(e). SQLAlchemy and driver
errors can carry connection details, so they now return a fixed message and
log the exception server-side.

1g. The document plane and the inference plane are separate capabilities

GET /ids, GET /documents, GET /documents/{id}/context and
DELETE /documents read or delete stored chunks. None of them calls an
inference provider. But the strict-token vocabulary offered only rag:embed
and rag:rerank, and a strict token carrying no scope at all is refused — so a
caller that needed nothing but a delete had to ask for rag:embed, and the
resulting credential also carried the ability to spend embedding budget.
Least-privilege in name only.

Those four routes now require rag:documents, and the scopes do not substitute
for each other in either direction:

Scope Grants
rag:embed POST /v1/embeddings
rag:rerank POST /v1/rerank
rag:documents GET /ids, GET /documents, GET /documents/{id}/context, DELETE /documents

A rag:embed token reaches no document route; a rag:documents token is
refused by both /v1 endpoints. A leaked delete credential therefore cannot be
replayed against an inference provider, and a leaked embedding credential
cannot read or destroy stored content.

The refusal is a 403 raised before the store is touched, so it says nothing
about whether a file exists — the 404-not-403 rule for out-of-scope reads
is unchanged, as is the scope inside the DELETE predicate. Legacy
{"id": userId} tokens predate every scope and are grandfathered into this one
exactly as into the others. Deployments with no signing key configured have no
principal to check and are unchanged.

Test evidence

tests/test_query_authorization.py (19 tests) routes the endpoint through a
store that evaluates the metadata filter the route actually builds, so a
route that stops putting scope into the predicate fails rather than passing on
a post-hoc check. Checked out against the pre-fix document_routes.py, 10 of
the original 11 fail; against the fix, all pass.

tests/integration/test_query_authorization_pg.py (18 tests) writes through
the real pgvector path and lets PostgreSQL execute the predicate: foreign
files return nothing, each user sees only their own side of a co-owned
file_id, entity_id impersonation is 403, tenants do not see each other,
and uploaded chunks are stamped with the writer's owner and tenant.

tests/test_rerank_endpoint.py::TestAuthorizeBeforeEgress and
tests/integration/test_rerank_pg.py::TestRerankOverStoredVectors assert the
egress rule directly — the fake and real embedders record every call, and the
assertions are that the call list is empty on every rejection path.

tests/test_auth.py covers the key-separation matrix, including a session
token failing against /v1/embeddings both with and without full claims.

tests/test_document_authorization.py::TestThePlanesAreSeparable drives both
planes against one another: a rag:documents token reaches all four document
routes and is 403 on both /v1 endpoints, a rag:embed token embeds
successfully and is 403 on all four document routes, and legacy tokens still
reach both. Checked out against the pre-fix routes, the three tests asserting
that an inference scope reaches no document route fail; the rest pass either
way, which is what makes them the control.

Totals: 481 unit tests, 80 integration tests (pgvector via testcontainers),
all passing. The integration fixtures skip cleanly when no container runtime is
reachable.


2. Residual risk during the legacy-token transition

This is not closed by this PR and needs to be understood before deploying.

A legacy {"id": userId} token carries no entities claim. This service
therefore cannot distinguish a legitimate agent id from a victim's user id in
entity_id, and while RAG_AUTH_ACCEPT_LEGACY=true it takes the caller's
entity_id at face value — so 1b is mitigated but not eliminated for legacy
tokens
: the blast radius drops from "any file, any owner" to "the owner named
in entity_id, and only for chunks the caller also names by file_id", but a
caller can still reach another owner's chunks by naming them.

What the flag being true does buy immediately, for every caller including
legacy ones:

  • hits 2..k are scoped (1a is fully closed)
  • user_id is None no longer returns everything (1a is fully closed)
  • /query_multiple is scoped at all (1c is fully closed)

Flipping RAG_AUTH_ACCEPT_LEGACY=false eliminates the remainder, and requires
callers to mint the full claim set first. Startup logs a warning for as long as
the flag is true, and
test_legacy_entity_id_is_still_trusted_until_the_flag_flips pins both the
residual behaviour and the fact that flipping the flag ends it.


3. New endpoints

POST /v1/embeddings
{ "space": "chat-v1", "input_type": "query" | "document",
  "inputs": [{ "id": "...", "text": "..." }] }
-> { "space", "model", "dimensions", "normalized",
     "items": [{ "id", "content_hash", "embedding" }], "usage" }

POST /v1/rerank
{ "profile": "fast-v1", "query": "...",
  "candidates": [{ "id", "text", "base_score" }], "top_n": 25 }
-> { "profile", "model", "results": [{ "id", "index", "score" }] }

Enabled by RAG_SEARCH_API_ENABLED=true (default false). Scopes:
rag:embed and rag:rerank. Neither accepts rag:documents — see §1g.

Limits: 64 inputs and 256,000 aggregate characters per embeddings call; 50
candidates and top_n <= 25 per rerank call. Caller ids are preserved. Tie
ordering is deterministic — ties break on the candidate's position in the
request, so identical input always produces identical output.

content_hash is the SHA-256 of the NFKC-normalized, whitespace-collapsed text
that was actually embedded, which lets a caller detect that stored text has
drifted from its stored vector.

Vectors leave the service L2-normalized. This is load-bearing rather than
cosmetic: the provider used in testing returns unnormalized vectors (measured
norms ≈ 67–71), so a caller assuming unit vectors and using a dot product as
cosine similarity would be silently wrong.

The chat-v1 space is substitution-locked. If its backend fails, or returns a
different dimensionality than the space declares, the call is 503 — never a
quiet fall back to another model, another width, or the file-search space. A
model swap behind an OpenAI-compatible endpoint is invisible in the response
body, so the width check is the only thing that catches it.

Neither endpoint returns candidate text, and neither logs query or candidate
text — only lengths and non-reversible fingerprints. The
RequestValidationError handler no longer logs raw input values, and no longer
500s when a custom field validator puts a ValueError in ctx (it was not
JSON-serializable).

fast-v1 embed-blend

The query is embedded once through the retrieval cache, so a rerank that
follows a /query on the same query string pays no query inference at all.
Candidate vectors are read from the pgvector store wherever they already exist
— scoped to the caller — and only vectorless candidates are embedded. On the
file-search path that is zero candidate inference.

The score is reciprocal-rank fusion of cosine similarity with the caller's
base_score, never pure embedding order. Bi-encoder similarity alone regresses
identifier and exact-match queries, where the retrieval score is the better
signal; fusing the two arms keeps the semantic gain without discarding it.
RAG_RERANK_RRF_K (default 60) and the two arm weights are configurable. An
arm with no usable values is dropped rather than contributing a constant, so a
fully vectorless call degrades to the caller's own order instead of shuffling
it.

Candidate ids resolve against the stored row's uuid or the chunk digest in
its metadata. custom_id is the file id and is shared by every chunk of a
file, so it identifies no single vector; digest is the handle a caller
already holds from a /query response.

Measured cost of the in-process scoring, 50 candidates × 1024 dimensions:
p50 2.3 ms, p95 2.6 ms.

Rate limiting

Per tenant and per subject, with separate embedding and rerank budgets so a
rerank burst cannot starve the interactive query-embedding path. Counters are
process-local, which a multi-pod deployment should read as limit × pods; they
are sized as a safety valve, not a billing control.


4. Tenant scope

Retrieval is scoped by (tenant, owner/entity, file_id). Chunks now record the
writing caller's tenant_id alongside user_id.

Chunks written before tenant_id existed carry no value and normalize to the
base tenant __BASE__, so a single-tenant deployment reads them exactly as
before while a named tenant never absorbs untagged content. This is implemented
by teaching the pgvector filter builder MongoDB's $in: [null] semantic —
match null or absent — rather than dropping the null and silently narrowing
the filter, so both backends behave identically.

Writes are scoped too: uploading under an entity_id the token does not permit
is 403, so a caller cannot plant content in a knowledge base it cannot read.


5. Credentials

POSTGRES_DB, POSTGRES_USER and POSTGRES_PASSWORD no longer have working
defaults. A default that works is the problem, not the convenience: a database
brought up with credentials that are public in this repository is reachable by
anyone who can route to it.

  • app/config.py requires them when VECTOR_DB_TYPE=pgvector, and refuses to
    start otherwise (POSTGRES_PASSWORD may be empty only under
    POSTGRES_USE_UNIX_SOCKET=True, where peer authentication carries none).
  • docker-compose.yaml and db-compose.yaml use ${VAR:?}, so
    docker compose up fails loudly rather than starting a database with a
    known password.
  • .env.example ships REPLACE_ME_* placeholders and is the documented
    starting point.
  • tests/test_credentials.py scans every tracked file and fails if a default
    credential reappears anywhere.

There is no fallback path for the signing configuration either: with
RAG_SEARCH_API_ENABLED=true and no RAG_JWT_SECRET, startup raises. No key
is generated or defaulted, and secrets appear in no log line or error message.


Breaking changes

  1. POSTGRES_DB / POSTGRES_USER / POSTGRES_PASSWORD are required.
    Deployments relying on the previous defaults must set them. This is
    deliberate — see §5.
  2. Chunks with no user_id in metadata are no longer readable. They were
    readable by everyone (§1a); re-embed them if they are still needed.
  3. /query and /query_multiple return only the caller's own chunks. A
    caller that was relying on reading another owner's chunks through a shared
    file_id will see fewer results. That is the fix.
  4. atlas-mongo deployments must declare user_id and tenant_id as
    filter fields on the Atlas vector index, since the owner and tenant
    predicates are now part of the vector-search filter.
  5. The file-addressed document routes require rag:documents. Strict
    tokens minted for them before the scope existed carried rag:embed — the
    smallest set that satisfied the non-empty-scopes rule — and are now 403.
    Mint rag:documents for calls that read or delete stored chunks and keep
    rag:embed for the ones that actually embed. Legacy {"id": userId} tokens
    are unaffected while RAG_AUTH_ACCEPT_LEGACY is true.

Migration

RAG_AUTH_ACCEPT_LEGACY defaults to true, so existing {"id": userId}
tokens keep working and no caller breaks on deploy. Order of operations:

  1. Deploy this change with RAG_AUTH_ACCEPT_LEGACY=true and a
    RAG_JWT_SECRET distinct from JWT_SECRET.
  2. Update every caller that mints a token for this service to emit the full
    claim set — iss, aud, sub, exp, a tenant claim, scopes, and
    entities for any agent-scoped file access — signed with
    RAG_JWT_SECRET. In the LibreChat integration this is
    generateShortLivedToken plus its six call sites (file search, vector
    CRUD, context handlers, RAG delete, text parsing), which must move together
    because they share one helper.
  3. Set RAG_AUTH_ACCEPT_LEGACY=false. This closes the residual risk in §2 and
    is the point at which entity_id becomes verifiable.

Ordering for the rag:documents requirement

Step 2 above and this build are coupled, and the coupling is asymmetric:

  • Callers first, then this build — safe. A service build that does not yet
    enforce rag:documents never inspects the scope on those routes, so a token
    carrying it is simply accepted. The unknown scope is inert.
  • This build first, then callers — breaks. This build refuses a token that
    still carries only rag:embed, so every document read and delete returns
    403 until the callers catch up.

Roll the minting change out first, or deploy both together. The LibreChat side
is danny-avila/LibreChat#14692.

Verification

pytest                  # 335 unit tests
pytest -m integration   # 54 integration tests (pgvector via testcontainers)

Integration fixtures skip rather than error when no container runtime is
reachable. An optional live check against a real OpenAI-compatible endpoint
runs only when RAG_GATEWAY_TEST_BASEURL and RAG_GATEWAY_TEST_API_KEY are
set.

@danny-avila
danny-avila marked this pull request as ready for review August 7, 2026 11:13
/query authorized an entire result set from documents[0] — the first
*returned* hit — so any hit past the first was never checked, and
/query_multiple performed no authorization at all. Both routes also took the
caller-supplied entity_id as the identity to compare against, so naming a
victim's id authorized the victim's chunks.

Owner scope is now resolved from the verified token and pushed into the
vector-store filter before ranking; nothing downstream re-derives
authorization from what the search returned.

Adds a dedicated RAG signing key (RAG_JWT_SECRET, never JWT_SECRET) with
issuer/audience/subject/tenant/scope verification, a RAG_AUTH_ACCEPT_LEGACY
transition flag for the {id}-style tokens LibreChat mints today, and startup
validation that fails closed on broken signing configuration.

Raw query text is replaced by a non-reversible fingerprint in the two error
logs that carried it.
/v1/embeddings serves the substitution-locked chat-v1 space: 64 inputs and
256,000 aggregate characters per call, caller ids preserved, content_hash over
NFKC-normalized text, L2-normalized vectors, and a 503 — never a quiet model
or dimensionality switch — when the backend is unavailable or answers off-spec.

/v1/rerank implements fast-v1 as embed-blend v0: the query embeds once through
the retrieval cache, candidate vectors come from the pgvector store where they
exist (scoped to the owners the token permits), and only vectorless candidates
are embedded. Scores are RRF over cosine similarity and the caller's
base_score, never pure embedding order, with ties broken by request position.
No candidate text appears in responses or logs.

Both endpoints are budgeted per tenant and per subject, with separate embedding
and rerank allowances.

Also fixes the RequestValidationError handler, which 500'd whenever a custom
field validator put a ValueError in `ctx`, and stops it logging raw input
values.
… API

Adds pgvector-container suites that write through the real store and let
PostgreSQL execute the predicate: cross-user isolation on /query and
/query_multiple, entity impersonation refused, and the fast-v1 stored-vector
lookup scoped to the owners the token permits (a foreign candidate id resolves
to nothing and falls through to inference on the caller's own text).

The container fixture now skips rather than errors when no runtime is
reachable, and an optional live check exercises the real gateway client when
RAG_GATEWAY_TEST_BASEURL / RAG_GATEWAY_TEST_API_KEY are set.

README documents the two token generations, the key-separation rule, the
service endpoint contracts and limits, the embed-blend profile, the rate-limit
budgets, and the Atlas index note the owner predicate implies.
A legacy token carries no entity list, so a caller-supplied entity_id cannot
be distinguished from a victim's user id. Startup now warns while
RAG_AUTH_ACCEPT_LEGACY is true, a test pins the behaviour and proves flipping
the flag ends it, and the README states it alongside the note that chunks
written without user_id metadata are no longer visible.
@danny-avila

Copy link
Copy Markdown
Owner Author

@codex review

…ore egress

Scope is now one predicate from one builder. app/scope.py produces
(tenant, owner/entity, file_id) and every retrieval path consumes it, so no
route hand-writes a scope clause and none can drift. Chunks record the writing
caller's tenant_id and user_id; chunks written before tenant_id existed carry
no value and normalize to the base tenant, so single-tenant deployments read
them unchanged while a named tenant never absorbs untagged content. The
pgvector filter builder implements MongoDB's `$in: [null]` semantic (match null
*or absent*) rather than dropping the null and silently narrowing the filter.

Writes are scoped too: uploading under an entity_id the token does not permit
is refused, so a caller cannot plant content in a knowledge base it cannot read.

Authorize before egress on /v1/rerank. Candidate text is sent to an inference
provider, so it has left the trust boundary whether or not the caller sees a
response. Every candidate id is now probed against the store — metadata only,
no vectors and no document text — before anything is embedded. An id that
exists but resolves to nothing in the caller's scope makes the request 403 with
zero egress; an unrunnable probe fails closed with 503 rather than embedding
text it could not check. /v1/embeddings reads no store at all and rejects on
scope, quota and limits before calling the backend.

No hardcoded credentials. POSTGRES_DB/USER/PASSWORD lose their working
defaults and are required; the compose files use ${VAR:?}; .env.example ships
REPLACE_ME_* placeholders. The 500 handlers on the database-facing routes no
longer echo the raw exception, which can carry connection details.
A store without a candidate probe silently skipped the authorize-before-egress
check. There is no correct fallback there — without the probe there is no way
to tell a foreign candidate from a fresh one — so the request now fails closed
with 503 instead.
The per-test collection name was derived from id(embeddings). CPython reuses
addresses after collection, so two tests could land in the same collection and
seed the corpus twice, making row-count assertions flaky.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd23416286

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread README.md
Comment on lines +337 to +338
"path": "file_id",
"type": "filter"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Add scoped fields to the Atlas index definition

For Atlas deployments using this documented index—or existing indexes that contain only file_id—the new /query and /query_multiple predicates also pre-filter on user_id and tenant_id via ScopeFilter.scope_clauses(). Atlas vector-search pre-filters require each referenced path to be declared as a filter field, so these requests will fail instead of returning results until both fields are added to the index definition and migration instructions.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 953d282.

AtlasMongoVector.similarity_search_with_score_by_vector passes the route's filter straight through as pre_filter, and ScopeFilter.scope_clauses() now emits user_id and tenant_id alongside file_id. Following the documented definition produced an index that makes both endpoints fail.

Added both paths to the JSON definition, and a "Migrating an existing Atlas vector index" section with the edit-and-rebuild steps.

The migration also needs one thing beyond the index edit, which is worth flagging: chunks embedded before this release carry no tenant_id field at all. The pgvector side handles that through $in: [null] matching absent keys, but Atlas vector-search pre-filters match declared scalar values rather than missing fields, so those chunks would stay invisible after the rebuild. The migration therefore includes a one-time backfill:

db.getCollection("<COLLECTION_NAME>").updateMany(
  { tenant_id: { $exists: false } },
  { $set: { tenant_id: "__BASE__" } }
)

__BASE__ is what a caller with no tenant claim resolves to, so this restores exactly the visibility those chunks had before.

To keep the definition from drifting from the code again, tests/test_query_authorization.py::TestDocumentedAtlasIndex parses the JSON block out of README.md and asserts every path ScopeFilter.scope_clauses() and file_clause() reference is declared as a filter field. It fails against the old definition.

Comment thread app/services/ratelimit.py Outdated
Comment on lines +125 to +129
retry_after = self._hit(
(budget_name, "subject", subject),
budget.subject_limit,
budget.window_seconds,
now,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid charging subject-rejected requests to the tenant

When one subject exceeds its subject limit, the tenant _hit above has already incremented its counter before this subject check rejects the request. For example, with the default 120/600 limits, a single token can keep sending rejected requests until it consumes all 600 tenant slots and then deny service to every other subject in that tenant. Check both counters before committing either increment, or roll back the tenant increment when the subject arm rejects.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 849d4ea.

Took the "check both before committing either" option. _hit is split into a read-only _retry_after and a _commit, and check now inspects both arms before either counter moves:

retry_after = self._retry_after(tenant_key, budget.tenant_limit, window, now)
if retry_after is not None:
    return RateLimitDecision(False, "tenant", retry_after)
retry_after = self._retry_after(subject_key, budget.subject_limit, window, now)
if retry_after is not None:
    return RateLimitDecision(False, "subject", retry_after)
self._commit(tenant_key, window, now)
self._commit(subject_key, window, now)

Both commits happen inside the same lock acquisition, so the pair stays atomic. Tenant is still evaluated first, so a tenant-exhausted request still reports scope: "tenant". The symmetric case is fixed too — a tenant rejection no longer spends the subject's budget.

New tests in tests/test_ratelimit.py: a unit test where one subject sends 21 requests against a 4/1 budget and only spends one tenant slot, a symmetric one for tenant rejections, and an end-to-end test that a throttled subject does not lock its tenant out.

One existing test needed correcting rather than preserving: test_tenants_have_independent_budgets exhausted the tenant budget by sending three requests from a single subject against a 3/2 budget — which only worked because the third was rejected on the subject arm and charged to the tenant anyway. It now uses three distinct subjects, so the tenant budget is spent by requests that were actually served.

Comment thread app/models.py
Comment on lines +74 to +75
total_characters = sum(len(item.text) for item in inputs)
if total_characters > MAX_EMBEDDING_CHARS:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate the normalized embedding payload size

This limit is calculated before the route applies NFKC normalization, even though the normalized strings are what get sent to the backend. Compatibility characters can expand substantially—for example, 100,000 characters pass this check but normalize to 300,000 characters—so requests can exceed the advertised 256,000-character provider limit. Recheck the aggregate length after normalization and before calling space.embed_documents.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 0c99e1b.

Re-checked the aggregate after normalize_text and before space.embed(...), so the limit binds the text that is actually sent:

normalized_characters = sum(len(text) for text in texts)
if normalized_characters > MAX_EMBEDDING_CHARS:
    raise HTTPException(422, detail=f"inputs normalize to {normalized_characters} characters, ...")

The model validator stays as the cheap arm that rejects on the raw payload, with a docstring explaining why it cannot be the only one. Placed before the backend call, so an over-limit request still embeds nothing.

tests/test_embeddings_endpoint.py::TestNormalizedSizeLimit uses your example: a single input under the limit as written and 1.5x over it once normalized, the same spread across three inputs, an assertion that nothing reaches the backend, plus a case where expansion stays within the limit and succeeds (with usage.total_characters reporting the normalized 3x length) and a non-expanding request at the boundary. Three fail without the fix.

For completeness — the rerank path was already correct here. RerankRequest._validate_candidates measures the raw payload, but the route recomputes pending_characters from the normalized texts before calling the backend, so the same class of bypass does not exist there.

Comment thread app/auth.py
Comment on lines +350 to +354
except (
jwt.InvalidAudienceError,
jwt.InvalidIssuerError,
jwt.MissingRequiredClaimError,
AuthError,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep malformed strict tokens from gaining legacy privileges

With the default RAG_AUTH_ACCEPT_LEGACY=true, a RAG-signed token that fails strict validation because it is missing exp, tenant, or scopes falls through to the legacy decode. _principal_from_legacy also accepts sub-based tokens and marks them legacy, after which has_scope and permits_entity grant every scope and arbitrary entity access—even if the token explicitly contains only rag:embed and a restricted entity list. Restrict this fallback to the actual legacy id claim shape without strict authorization claims, or preserve any explicit scope/entity restrictions.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in be041cc.

Verified the escalation: a RAG-signed token carrying sub, iss, aud, tenant and scopes: ["rag:embed"] but no exp raises MissingRequiredClaimError, which the accept_legacy branch swallows; the legacy decode then accepts it via the sub fallback and marks it legacy=True, after which has_scope returns True for everything and permits_entity returns True for any id. Same for a missing tenant (AuthError) and empty scopes.

Fix takes both remedies you named. The grandfather in has_scope/permits_entity now applies only to the genuine pre-scopes {"id": userId} shape, and any token that states scopes, scope or entities keeps exactly what it stated:

legacy=_LEGACY_SUBJECT_CLAIM in claims and not declares_authorization,

So legacy acceptance covers claims a token omits, never claims it makes. A stated-but-empty scopes: [] or entities: [] is a restriction and grants nothing rather than falling back to "unspecified". A sub-shaped token with no authorization claims is no longer grandfathered either — it resolves with no scopes (403 on /v1, subject-only on /query). The fallback also logs a warning when it fires, since a malformed strict token is an anomaly worth surfacing.

Regression tests in tests/test_auth.py::TestMalformedStrictTokensGainNothing cover each malformation (no exp, no tenant, no audience, empty scope list, empty entity list, sub-shaped) at both the verify_token and endpoint level, and assert the genuine legacy shape is still grandfathered. All 7 fail without the fix.

Comment thread app/routes/search_routes.py Outdated
Comment on lines +312 to +314
type(exc).__name__,
exc,
traceback.format_exc(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Stop logging inference-provider exception contents

If the embedding provider includes rejected query or candidate text in its exception message, logging exc and the traceback writes that sensitive text verbatim despite this endpoint's no-raw-text guarantee. The embeddings error block has the same issue because SpaceBackendError embeds the original provider message. Log only a sanitized error category or correlation identifier rather than provider-controlled exception text and tracebacks.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 5ba9348.

Replaced exc and traceback.format_exc() with an error_category() helper that walks the __cause__ chain and joins the class names only:

logger.error(
    "Rerank embedding backend unavailable | candidates=%d pending=%d | %s",
    len(body.candidates), len(pending_texts), error_category(exc),
)

A failure logs e.g. SpaceBackendError <- RuntimeError, which still identifies what broke and where it came from without reproducing anything the provider or the caller wrote. The traceback is gone entirely — beyond the message it can carry frame reprs into the log.

Applied to the embeddings block as well, as you noted, and also to the two store-exception logs in _authorize_candidates and _stored_vectors. Those were logging exc too, and a SQLAlchemy DBAPIError embeds the statement parameters — which on that path are the caller-supplied candidate ids, arbitrary strings from the request body. Same rule, one helper.

tests/test_rerank_endpoint.py::TestFailuresAreLoggedWithoutProviderText covers a query-embedding failure that echoes the query, a candidate-embedding failure that echoes the batch, absence of any traceback, and a store failure that echoes the candidate ids; tests/test_embeddings_endpoint.py::test_a_provider_that_echoes_the_input_still_leaks_nothing covers the embeddings block and asserts the class chain is still present so the failure stays diagnosable. All fail without the fix.

Comment thread app/auth.py Outdated
Comment on lines +177 to +178
if not self.search_api_enabled:
return

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Validate configured RAG keys even when search is disabled

When RAG_SEARCH_API_ENABLED=false, this early return skips the HMAC minimum-length and issuer/audience validation even though middleware still accepts RAG_JWT_SECRET-signed strict tokens on /query, uploads, and the other non-/v1 routes. A deployment with a short RAG key and search disabled therefore starts successfully while protecting document APIs with a trivially weak signing secret. Apply signing-key validation whenever a verification key is configured, not only when the search router is enabled.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in be041cc.

RAG_SEARCH_API_ENABLED only decides whether the /v1 router is mounted; security_middleware verifies RAG-signed strict tokens on /query, /query_multiple and the upload routes regardless of the flag. So the early return meant a deployment could protect its document APIs with an 8-character secret and start cleanly.

Restructured so the flag gates only "a key is required", and everything that validates a key that is configured runs unconditionally:

if not self.verification_key:
    if not self.search_api_enabled:
        return
    raise RuntimeError("RAG_SEARCH_API_ENABLED=true requires ...")

# HMAC minimum length, issuer, audience — all below this point.

The issuer/audience messages changed from "must be set when the search API is enabled" to "must not be empty when a RAG signing key is configured", since that is now the actual condition. These matter outside the search API too: strict decode requires iss/aud and compares them against the configured values, so an empty one breaks strict tokens on /query as well.

The legacy-only deployment is unaffected — no RAG_JWT_SECRET and no search means there is no key to validate, and that path still returns early.

tests/test_auth.py::TestStartupValidation gains four cases: short HMAC secret, empty issuer and empty audience all refused with RAG_SEARCH_API_ENABLED=false, plus a positive case proving the legacy-only deployment still starts. The three negative ones fail without the fix.

self.EmbeddingStore.cmetadata["user_id"].astext,
self.EmbeddingStore.cmetadata["tenant_id"].astext,
)
.filter(self._candidate_id_clause(wanted))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restrict rerank lookups to the active PGVector collection

These new candidate queries scan the entire shared embedding table without filtering by the vector store's current collection_id; get_vectors_by_ids repeats the same omission. In databases hosting multiple PGVector collections, a digest from another collection can therefore be treated as existing or authorized, produce a false 403, or have its vector reused by this collection. Resolve the active collection and include its UUID in both the authorization probe and vector lookup predicates.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 4ce4992.

Both queries filtered on _candidate_id_clause alone against langchain_pg_embedding, which every collection in the database shares, so a row from a sibling collection could be counted as existing (false 403) or have its vector returned and scored.

Added _collection_clause, which resolves the store's collection in the open session and returns EmbeddingStore.collection_id == collection.uuid, and applied it as the first predicate in both probe_candidate_ids and get_vectors_by_ids. A collection that does not exist yet holds no rows, so both return empty instead of falling back to a table-wide scan; if collection resolution raises, _authorize_candidates still fails closed with a 503.

Worth noting the probe's semantics are unchanged where it matters: a sibling collection's row is now "not existing" rather than "existing but unauthorized", which is correct — this store never serves that row, so the only thing the old behaviour could produce was a 403 for a candidate the deployment never held. The authorize-before-egress guarantee is untouched for rows this store does serve.

tests/integration/test_rerank_pg.py::TestCollectionIsolation runs against a real second PGVector collection in the same container and covers: a sibling's row not reported as existing, its vector never reused, the same digest resolving independently in each collection, a sibling's row uuid resolving to nothing, and an empty collection authorizing nothing. All 5 fail without the fix.

One related change: test_row_uuid_resolves_to_the_stored_vector was selecting a row uuid by digest across the whole table, which only worked while a single collection existed. It now joins langchain_pg_collection and scopes to the store under test.

Comment on lines +711 to 714
"tenant_id": tenant_id,
"digest": generate_digest(doc.page_content),
**(doc.metadata or {}),
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Prevent loader metadata from overriding the writer tenant

The trusted tenant_id is inserted before doc.metadata is expanded, so loader-produced metadata with the same key silently replaces the verified token's tenant. Formats whose loaders preserve document metadata, including crafted document properties, can consequently stamp chunks into a different tenant and defeat the new write-side tenant boundary. Merge untrusted loader metadata first and assign file_id, user_id, tenant_id, and the digest afterward.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 086b050.

**(doc.metadata or {}) was last in the dict literal, so loader metadata won every collision — not just tenant_id but file_id, user_id and digest too. Loaders that preserve embedded document properties therefore let an uploaded file choose which tenant its chunks land in, which defeats the write-side boundary the read predicate relies on.

Fixed by merging the untrusted loader metadata first and assigning the resolved values afterwards:

metadata={
    **(doc.metadata or {}),
    "file_id": file_id,
    "user_id": user_id,
    "tenant_id": tenant_id,
    "digest": generate_digest(doc.page_content),
},

Harmless loader metadata (source, page, …) is still preserved — only the four resolved keys are protected.

tests/test_upload_isolation.py::TestLoaderMetadataCannotOverrideTheWriter parametrizes over each of the four keys individually, then asserts all four at once, plus that benign metadata survives and that documents with no metadata are unaffected. 6 of the 8 fail without the fix.

Comment thread app/routes/search_routes.py Outdated
)

try:
vectors = await _run(request, space.embed_documents, texts)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Honor the requested embedding input type

Every request is sent through embed_documents regardless of whether input_type is query or document, making the required field operationally meaningless. Embedding backends that use different query and passage encoders or task prefixes will return document-oriented vectors for query inputs, reducing or invalidating retrieval compatibility. Dispatch query inputs through the space's query embedding path, or remove the unsupported distinction from the API contract.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 5a3394b — took the "honor it" option rather than dropping it from the contract.

EmbeddingSpace now encodes by input type. space.embed(texts, input_type) applies the space's task prefix for that type and dispatches to the backend's batch query encoder when it exposes one:

def _encoder(self, input_type):
    client = self._client_or_raise()
    if input_type == INPUT_TYPE_QUERY:
        query_encoder = getattr(client, "embed_queries", None)
        if callable(query_encoder):
            return query_encoder
    return client.embed_documents

SpaceSpec gains query_prefix / document_prefix, wired to RAG_CHAT_EMBEDDING_QUERY_PREFIX / RAG_CHAT_EMBEDDING_DOCUMENT_PREFIX and empty by default.

The prefix rather than a per-text embed_query loop is deliberate. LangChain's Embeddings interface has no batch query method, so falling back to [client.embed_query(t) for t in texts] would cost up to 64 round trips per request while returning identical vectors on any symmetric backend — OpenAIEmbeddings.embed_query(t) is embed_documents([t])[0]. The prefix is what actually carries the distinction for the asymmetric models this matters for, including the configured qwen3-embedding-8b, and it stays one batched call.

Two consequences worth calling out: the prefixes are part of the space's locked definition, since changing one changes every vector the space produces and stored vectors have to be rebuilt — documented in the module docstring, README and .env.example. And content_hash is deliberately taken over the un-prefixed normalized text, so it stays a stable cache key for the same content across both input types.

Defaults are empty, so no existing deployment's vectors change.

tests/test_embeddings_endpoint.py::TestInputTypeIsHonoured covers the query encoder being used when present and skipped for documents, both prefixes reaching the backend, the two input types producing different vectors, the shared content_hash, and an unprefixed space being unaffected. Three fail without the fix.

@danny-avila
danny-avila force-pushed the feat/search-stack-v1 branch from fd23416 to decbf88 Compare August 7, 2026 11:48
With RAG_AUTH_ACCEPT_LEGACY=true, a RAG-signed token that failed strict
validation — no exp, no tenant, no scopes — fell through to the legacy
decode and came back marked legacy. Legacy principals are grandfathered
into every scope and into arbitrary entity access, so a token that
restricted itself to rag:embed and one entity gained full privileges by
being malformed.

The grandfather now applies only to the pre-scopes {"id": userId} shape.
A token that states its own scopes or entities keeps exactly those, so
legacy acceptance covers claims a token omits and never claims it makes.

Also validate a configured RAG signing key whenever it is set, not only
when RAG_SEARCH_API_ENABLED is true: the middleware honours RAG-signed
strict tokens on /query and the upload routes either way, so gating the
HMAC length and issuer/audience checks on that flag let a deployment
protect its document APIs with a trivially weak secret and still start.
The trusted file_id, user_id, tenant_id and digest were inserted before
doc.metadata was expanded, so loader-produced metadata carrying the same
keys silently replaced them. Loaders for several formats preserve
embedded document properties verbatim, which meant a crafted property
named tenant_id could stamp chunks into another tenant and defeat the
write-side tenant boundary the read predicate depends on.

Merge the untrusted loader metadata first and assign the resolved values
afterwards, so the request's verified identity always wins.
probe_candidate_ids and get_vectors_by_ids matched on candidate id alone
against langchain_pg_embedding, which every collection in the database
shares. Where one database hosts multiple PGVector collections, a digest
belonging to a sibling collection could be reported as existing — a false
403 for a candidate this deployment never served — or have its vector
handed back and scored.

Resolve the store's collection and put its uuid in both predicates. A
collection that does not exist yet holds no rows, so both lookups return
empty rather than falling back to a table-wide scan.
The tenant counter incremented before the subject arm ran, so a request
the subject arm went on to reject had already spent a tenant slot. With
the default 120/600 limits one token could burn all 600 tenant slots on
requests that were refused anyway and deny service to every other subject
in that tenant.

Inspect both arms before either counter moves, and commit both only once
the request is actually served. Tenant is still checked first, so a
tenant-exhausted request still reports scope "tenant" and leaves the
subject's own budget untouched.
Both endpoints promise that no query or candidate text is logged, and
that has to hold on the error paths. An inference gateway routinely
echoes the rejected input in its error message and SpaceBackendError
wraps that message verbatim, so logging the exception — or a traceback,
which carries frame reprs with it — wrote caller text into the log. A
database driver does the same with statement parameters, which here are
caller-supplied candidate ids.

Log the exception's class chain instead. It identifies the failure and
its cause without reproducing anything the caller or the provider wrote.
The 256,000-character aggregate was measured on the request as sent, but
NFKC normalization runs afterwards and is what reaches the backend.
Compatibility characters expand under it — 100,000 U+FB03 become 300,000
characters — so a request could pass the check and still exceed the
advertised provider limit.

Re-check the aggregate after normalization and before the backend is
called, so an over-limit request is refused with nothing embedded. The
model validator stays as the cheap arm that rejects on the raw payload.
input_type was required on every /v1/embeddings request and then ignored:
queries and documents both went through embed_documents. Backends with
distinct query and passage encoders, or with task prefixes, returned
document-oriented vectors for queries and degraded retrieval.

A space now encodes by input type. It applies the matching task prefix,
configurable per space and empty by default, and uses the backend's batch
query encoder where one exists. The prefixes join model, dimensions and
normalization in the space's locked definition, since changing one
changes every vector the space produces. content_hash stays over the
un-prefixed normalized text so it remains a stable cache key.
/query and /query_multiple pre-filter on user_id and tenant_id alongside
file_id, and Atlas rejects a $vectorSearch whose filter references a path
the index does not declare as a filter field. The documented definition
carried only file_id, so following it produced an index that made those
endpoints fail rather than return results.

Add both paths to the definition, and give migration steps for indexes
created before this release — including the tenant_id backfill, since
chunks embedded earlier carry no such field and Atlas pre-filters match
declared values rather than absent ones.

A test reads the definition out of the README and asserts it covers every
path ScopeFilter builds, so the two cannot drift apart silently.
@danny-avila

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 953d282e13

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/middleware.py
Comment on lines +64 to 66
principal = verify_token(
token, settings, allow_legacy_secret=not is_service_path
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep strict RAG tokens out of unscoped document routes

Accepting every RAG-signed strict token on non-/v1 paths lets restricted service credentials call legacy routes that do not enforce the new principal scope. In the inspected document router, GET /documents, GET /documents/{id}/context, and DELETE /documents fetch or delete solely by caller-supplied file IDs, without filtering by subject or tenant; consequently, a token carrying only rag:embed can read or delete another tenant's known file ID. Either apply ScopeFilter to all such routes or limit which principals this middleware admits there.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in ce5ae0c — took the ScopeFilter/resolve_scope option for all of them rather than restricting principals at the middleware.

Verified the reach first. security_middleware passes allow_legacy_secret=not is_service_path, so on /documents* the strict decode runs first and succeeds; a RAG-signed token with scopes: ["rag:embed"] lands as a full Principal and the four file-addressed routes never looked at it. They queried on the caller's ids alone: get_filtered_ids filtered custom_id.in_(ids), get_documents_by_ids the same, and the DELETE ran _delete_multiple on custom_id.in_(ids) with no owner, tenant or even collection predicate.

I folded GET /ids into the same fix. It listed every custom_id in the table, which is the enumeration half of the same attack — "given its ID" stops being a precondition when the service hands out the ids.

Each route now builds its predicate from resolve_scope(request, entity_id) and passes (ids, owners, tenants) to the store:

Route Before After
GET /ids every file id in the table the caller's own
GET /documents?ids= any id the caller's; anything else 404
GET /documents/{id}/context any id as above
DELETE /documents WHERE custom_id IN (...) scope in the DELETE predicate

Three things worth calling out.

The scope is inside the DELETE, not beside it. file_id is caller-supplied — anyone can POST /embed under a chosen id — so two owners can hold rows under one id, and a delete that filtered on the id alone would take both even with a scoped existence check in front of it. tests/integration/test_document_authorization_pg.py::TestDeletesAreScopedInSql::test_a_collided_file_id_deletes_only_the_callers_rows is that case against real pgvector. The same reasoning applied to the sync batched rollback (vector_store.delete(ids=[file_id])), which is now scoped to the writer.

The scope arguments are required, not optional. get_all_ids, get_filtered_ids, get_documents_by_ids and the new delete_scoped take (owners, tenants) positionally on both stores, so a future call site cannot skip scope by omission — it fails to call. Collection is in the predicate too, matching 4ce4992.

Out-of-scope reads as "not found", not "forbidden". Distinguishing the two would leave the routes as existence oracles over the whole deployment.

One behavioural consequence, documented in the README: files embedded under an entity_id are owned by that entity, so reading or deleting them now needs entity_id on these routes exactly as querying them already does. LibreChat's deleteVectors/deleteRagFile send a plain user token and no entity_id, so deleting an agent knowledge-base file returns 404 and leaves the chunks in place until the client passes the entity. That is a client-side follow-up rather than a reason to leave the delete unscoped — the alternative is a route where any authenticated caller destroys any file id.

Regression coverage: tests/test_document_authorization.py (24 tests) drives all four routes through a store that evaluates the predicate it is handed and answers unscoped when handed none, so the pre-fix code path is exercised rather than crashing on a signature — 16 of them fail without the fix. tests/integration/test_document_authorization_pg.py (15) proves the SQL against a real container, including collection isolation; 6 fail when the scope clause is removed. tests/services/test_atlas_mongo_vector.py covers the Atlas half; 5 fail without it.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on the second half of this — "or limit which principals this middleware admits there". Taking the ScopeFilter option closed the read/delete-by-file-id hole, but it left the credential itself over-privileged, and that is worth fixing on its own terms. Done in a293666.

The remaining problem was the scope vocabulary, not the predicate. GET /ids, GET /documents, GET /documents/{id}/context and DELETE /documents call no inference provider, but the only scopes on offer were rag:embed and rag:rerank, and _principal_from_strict refuses a token carrying none. So a caller that needed nothing but a delete had to ask for rag:embed purely to satisfy the non-empty-scopes rule — and the resulting credential could then be spent against /v1/embeddings. Least-privilege in name only: every document-plane token was also an inference token.

rag:documents closes that. The four file-addressed routes require it, and no scope substitutes for another in either direction:

Scope Grants
rag:embed POST /v1/embeddings
rag:rerank POST /v1/rerank
rag:documents GET /ids, GET /documents, GET /documents/{id}/context, DELETE /documents

A rag:embed token now reaches no document route, and a rag:documents token is refused by both /v1 endpoints. A stolen delete credential cannot be replayed against an inference provider; a stolen embedding credential cannot read or destroy stored content.

Three things worth calling out.

It is a separate dependency from require_scope, deliberately. That one gates the /v1 router and returns 503 when RAG_SEARCH_API_ENABLED is false, while these routes serve every deployment — reusing it would have made the document API unavailable wherever search is off. require_document_scope shares only the scope check itself, so the two cannot drift on what "missing scope" means.

The refusal is a 403 raised before the store is touched, so it is a statement about the token and never about whether a file exists. The 404-not-403 rule for out-of-scope reads is unchanged, as is the scope inside the DELETE predicate — a rag:documents token still reads 404 on another owner's file, and still deletes only its own rows under a co-owned file_id.

Nothing regresses for existing callers. Legacy {"id": userId} tokens predate every scope and are grandfathered into this one exactly as into the others, so they keep working while RAG_AUTH_ACCEPT_LEGACY=true — including tokens signed with the application secret, which these routes still accept. A deployment with no signing key configured has no principal to check and is unchanged; scope enforcement starts where tokens do.

tests/test_document_authorization.py::TestThePlanesAreSeparable drives both planes against each other rather than testing each alone: a rag:documents token reaches all four document routes and is 403 on both /v1 endpoints; a rag:embed token embeds successfully and is 403 on all four; a rag:rerank token likewise; and legacy tokens, RAG-signed and application-signed, still reach everything they did. The rest of the file was already exercising these routes with an rag:embed-only token, so it moved to a documents-scoped one with every assertion intact — the 404s, the co-owned-file_id delete, the tenant separation and the entity cases all still assert exactly what they did.

Negative control: checked out against the pre-change routes, the three tests asserting that an inference scope reaches no document route fail (the routes answer 200). The reverse-direction tests pass either way, which is the point of them — rag:documents must never become an inference grant as it is introduced, so those are a guard rather than a demonstration.

481 unit tests and 80 integration tests pass.

One deployment note, since this is a cross-repo contract: the coupling with the client side is asymmetric. A service build that does not yet enforce rag:documents never inspects the scope on those routes, so a client that starts minting it early is simply accepted — the unknown scope is inert. A build that does enforce it refuses a client still sending rag:embed. So the minting change ships first, or both together; never this side alone. The LibreChat half is danny-avila/LibreChat#14692, and both PR bodies state the order.

Comment thread app/models.py Outdated
Comment on lines +118 to +120
query: str = Field(min_length=1)
candidates: List[RerankCandidate] = Field(
min_length=1, max_length=MAX_RERANK_CANDIDATES

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Bound the rerank query before embedding it

A rerank request limits candidate text but places no upper bound on query, even though the route sends that entire value to get_cached_query_embedding. An authenticated caller can therefore submit an arbitrarily large query, consuming request memory and potentially exceeding the inference provider's input limit; unlike oversized candidates, this reaches the backend and is reported as a 503. Add a query or aggregate character limit before inference.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 35b6998.

Verified the asymmetry: _validate_candidates sizes sum(len(candidate.text or "")) against MAX_EMBEDDING_CHARS, and the route re-checks pending_characters after normalization — but query carried only min_length=1, and it is embedded on its own through get_cached_query_embedding, so neither check ever covered it. An oversized candidate set is a 422 that embeds nothing; an oversized query was a paid round trip that came back as a 503, which reads as a service fault for a request the service could have refused.

Bounded on the model, so it stays a 422 like every other limit:

# Bounded here rather than in the route: the query is embedded on its own, so
# the aggregate candidate budget below never covers it, and an unbounded one
# reaches the provider and comes back as a 503 instead of a 422.
query: str = Field(min_length=1, max_length=MAX_QUERY_CHARS)

MAX_QUERY_CHARS = 8_000 sits in app/constants.py with the rest of the contract limits, and the README's limits line now states it. 8,000 is generous for a retrieval query while leaving the per-input budget nowhere near the aggregate — worth noting the query is also the get_cached_query_embedding LRU key, so an unbounded one was retained memory as well as a retained request.

tests/test_rerank_endpoint.py::TestTheQueryIsBounded covers a query at the limit succeeding, one character over rejected, nothing reaching the backend when it is, an empty query still refused, and the case that shows the gap directly — one one-character candidate with a 255,999-character query, which passed the candidate aggregate comfortably. Three fail without the fix.

I left /query and /query_multiple alone deliberately: their query is a user message on the file-search path, and picking a bound for it is a product decision rather than a fix for this finding. Happy to add one if you'd rather it were uniform.

Comment thread app/routes/search_routes.py Outdated
Comment on lines +126 to +127
normalized_characters = sum(len(text) for text in texts)
if normalized_characters > MAX_EMBEDDING_CHARS:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include task prefixes in the embedding payload limit

When either task-prefix environment variable is configured, this follow-up check still measures only normalized caller text, while EmbeddingSpace.embed subsequently prepends the selected prefix to every input. A request at the 256,000-character boundary therefore passes but sends more than the stated provider limit—multiplied across as many as 64 inputs. Validate the final prefixed payload, or reserve prefix length in this check, before invoking the backend.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 35b6998 — reserved the prefix in the check rather than measuring after the fact, so the rejection still happens before anything is built for egress.

The two halves had drifted apart in the same release: 0c99e1b moved the size check onto the normalized text, and 5a3394b made EmbeddingSpace.embed prepend spec.prefix_for(input_type) to every input. Between them, the measured string and the sent string stopped being the same string.

The arithmetic now lives next to the code that applies the prefix, which is the only place that knows it exists:

def payload_characters(self, texts: List[str], input_type: str) -> int:
    prefix_length = len(self.spec.prefix_for(input_type))
    return sum(len(text) for text in texts) + prefix_length * len(texts)

and the route sizes the request with it. usage.total_characters still reports the caller's normalized text — the prefix is the service's, not something to bill the caller for.

tests/test_embeddings_endpoint.py::TestTaskPrefixesCountTowardTheLimit covers a request at the 256,000 boundary rejected once prefixed, nothing reaching the backend when it is, the document prefix on the document path, NFKC expansion and the prefix counted together, an unprefixed space still accepting the full payload, and the multiplication you flagged: 64 inputs of 3,901 characters is 249,664 as written and 256,064 with a 100-character prefix on each. Five fail without the fix.

Comment thread app/services/space.py Outdated
raise SpaceBackendError(
f"Embedding backend for space '{self.spec.name}' failed: {exc}"
) from exc
return self._finalize(vectors, len(texts))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Translate malformed embedding responses into backend errors

If an OpenAI-compatible backend returns a malformed payload such as None, a non-list vector, or correctly sized non-numeric components, _finalize raises TypeError or another built-in exception outside the guarded encoder call. The route catches only SpaceBackendError, so this violates the substitution-locked contract by producing an unhandled 500 instead of the documented sanitized 503. Wrap finalization failures as SpaceBackendError and reject non-finite or non-numeric components explicitly.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in cdda13b.

Reproduced each shape: None fails on len(vectors), [3.5] and [None] on len(vector), and [["x"] * 8] survives the width check and dies inside l2_normalize on component * component. All three raise TypeError from _finalize, which ran after the try — so the route's except SpaceBackendError never saw them and each became an unhandled 500 rather than the documented 503.

The framing that fixes it: reading the response is as much "the backend" as calling it. _finalize now runs inside the same guard, and every shape violation is raised as SpaceBackendError in its own right rather than relying on the outer wrap:

try:
    vectors = encoder(prepared)
    return self._finalize(vectors, len(texts))
except SpaceBackendError:
    raise
except Exception as exc:
    raise SpaceBackendError(...) from exc

Components are checked explicitly, per your note. Non-numeric ones failed deep inside the normalizer with a message about multiplication; non-finite ones did not fail at all, which was the worse case — inf/inf is NaN, NaN survives normalization untouched (norm == 0.0 is False for NaN), and the endpoint returned 200 with a vector that poisons every cosine score computed against it and is not even valid JSON. bool is refused too: it is a Real in Python, so [[True] * 8] used to normalize into a plausible-looking unit vector. numbers.Real rather than float keeps genuine backends working — integral components and numpy scalars are still accepted, and there is a test for that.

tests/test_embeddings_endpoint.py::TestMalformedBackendPayloadsStaySubstitutionLocked parametrizes eleven malformed payloads (null response, string response, object response, null vector, scalar vector, string vector, non-numeric components, boolean components, one null component, NaN, infinity) and asserts each is a 503 carrying the sanitized detail; plus space-level tests that finalization raises SpaceBackendError, that non-finite components are refused rather than normalized, that an unnormalized space checks its components too (nothing else would ever touch them there), that integral components still succeed, and that a malformed payload echoing the input leaks neither the text nor a traceback. 12 of the 16 fail without the fix.

Comment on lines +102 to +105
cursor = self._collection.find(
self._candidate_id_query(wanted),
{"_id": 1, "digest": 1, "user_id": 1, "tenant_id": 1},
).sort("_id", 1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Index digest lookups used by Atlas reranking

On Atlas, every rerank authorization probe—and then every stored-vector lookup—queries candidate IDs through an $or on _id and digest. The inspected deployment documentation creates only a standard file_id index; the Atlas vector-search index does not accelerate these ordinary find calls, and candidate IDs returned to callers are normally digests rather than Mongo _id values. Consequently, each rerank can scan the entire vector collection twice as it grows. Document and create an appropriate standard digest index, ideally including the scope fields needed by the second lookup.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in bed8be8.

The reasoning holds on both counts. _candidate_id_query is an $or over _id and digest; the Atlas vector-search index only serves $vectorSearch, so these are unindexed find calls against a file_id index that neither arm can use. And the ids really are digests in practice — /query returns digest in each hit's metadata while the Mongo _id is stripped in similarity_search_with_score_by_vector, so a caller reranking its own retrieval hands back digests every time. Two lookups per rerank (probe_candidate_ids, then get_vectors_by_ids), so two full collection scans.

Added { digest: 1, user_id: 1, tenant_id: 1 }. The trailing scope fields are exactly what the second lookup filters on, so with the projection it already uses it can be answered from the index rather than fetching rows it then discards; the _id arm of the $or uses the default _id_ index, so both arms are served.

One judgement call beyond what you asked for: the service now creates both standard indexes on startup (AtlasMongoVector.ensure_indexes, called from the lifespan for atlas-mongo) rather than only documenting them. createIndex is idempotent, and the existing file_id index has been a documented manual step for a while — a step that is easy to skip and invisible when skipped. A database user without index privileges logs a warning and starts; the lookups still return the right answers, they just scan.

README: the "Create a file_id Index" section is now "Standard indexes", stating both, why the vector index does not cover them, and what each one serves. The migration section gains step 5 for existing deployments.

tests/services/test_atlas_mongo_vector.py::TestTheIndexesTheLookupsNeed asserts both indexes are created, that they are named, and — to stop the docs drifting from the code the way the vector index definition did — that the set created at startup equals the set documented in README.md. Two fail without the digest index in either place.

GET /ids, GET /documents, GET /documents/{id}/context and DELETE /documents
addressed the store by caller-supplied file id alone, so any authenticated
caller could list the deployment's file ids and then read or delete another
owner's chunks by naming one. The strict tokens this release introduces reach
those routes too, which put the same reach in the hands of a service credential
holding nothing but rag:embed.

Every one of them now builds its predicate from resolve_scope, exactly as
/query does, and accepts the same optional entity_id. The scope is part of the
store query rather than a check beside it: a file id is chosen by whoever
uploads, so two owners can hold rows under one id and only the caller's may be
read or removed. A file outside the scope reads as "not found" rather than
"found but refused", so none of these routes is an existence oracle.

The scope arguments are required rather than optional on both stores, so a new
call site cannot skip them by omission. The sync batched rollback is scoped to
the writer for the same reason.
Two limits did not bind what leaves the process.

The rerank query had no bound at all. Candidate text is capped in aggregate,
but the query is embedded on its own, so that cap never covered it — an
authenticated caller could hand the gateway an arbitrarily large query and get
the provider's own refusal back as a 503, inference paid for. It now carries
MAX_QUERY_CHARS on the model, so it is a 422 like every other limit.

The embeddings payload check measured caller text only, while EmbeddingSpace
prepends the space's task prefix to every input. A request at the 256,000
boundary therefore passed and sent more, by the prefix length times up to 64
inputs. The route now sizes the request with space.payload_characters, which
keeps the prefix arithmetic next to the code that applies the prefix.
A backend that answered `null`, a scalar where a vector belongs, or a
correctly-sized vector of strings raised TypeError from inside _finalize, which
ran outside the guarded encoder call. The route catches SpaceBackendError and
nothing else, so those became unhandled 500s instead of the documented
sanitized 503 — the one failure mode chat-v1 is substitution-locked against.

Reading the response is as much "the backend" as calling it, so _finalize now
runs inside the same guard and every shape violation is raised as
SpaceBackendError in its own right. Components are checked explicitly:
non-numeric ones used to fail deep inside l2_normalize, and non-finite ones
never failed at all — inf/inf is NaN, which survives normalization and poisons
every cosine score computed against it. bool is refused too, since it is a Real
in Python and a payload of true/false is malformed rather than a vector of ones
and zeroes.
On Atlas the rerank authorization probe and the stored-vector lookup both
resolve candidate ids through an `$or` on `_id` and `digest`, and candidate ids
handed back to callers are normally digests. The Atlas vector-search index does
not accelerate ordinary `find` calls and the documented deployment created only
a `file_id` index, so every rerank scanned the whole vector collection twice —
a cost that grows with the corpus.

Adds a compound `{ digest: 1, user_id: 1, tenant_id: 1 }` index. The trailing
scope fields are what the second lookup filters on, so it can be answered from
the index instead of fetching rows it then discards. Both standard indexes are
created on startup, since `createIndex` is idempotent and a documented manual
step is a step deployments skip; a user without index privileges only warns.

README documents both, and the Atlas migration section gains the step for
existing deployments.
`GET /ids`, `GET /documents`, `GET /documents/{id}/context` and
`DELETE /documents` read or delete stored chunks and call no inference
provider to do it, but the strict-token vocabulary offered only `rag:embed`
and `rag:rerank` — and refuses a token carrying no scope at all. A caller that
needed nothing but a delete had to ask for `rag:embed`, so the credential also
carried the ability to spend embedding budget.

Those four routes now require `rag:documents`, and the inference scopes do not
substitute for it in either direction: a `rag:embed` token reaches no document
route, and a `rag:documents` token is refused by `/v1/embeddings` and
`/v1/rerank`. The refusal is a `403` raised before the store is touched, so it
says nothing about whether a file exists — the `404`-not-`403` rule for
out-of-scope reads is unchanged, as is the scope inside the `DELETE` predicate.

Legacy `{"id": userId}` tokens predate every scope and are grandfathered into
this one exactly as they are into the others, so they keep working while
`RAG_AUTH_ACCEPT_LEGACY` is true. Deployments with no signing key configured
have no principal to check and are unchanged.
@danny-avila

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a293666aa3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/auth.py
tenant=str(claims.get("tenant") or claims.get("tenant_id") or BASE_TENANT_ID),
scopes=_split_scopes(claims),
entities=_string_set(claims.get("entities")),
legacy=_LEGACY_SUBJECT_CLAIM in claims and not declares_authorization,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject mixed strict and legacy token shapes

With the default RAG_AUTH_ACCEPT_LEGACY=true, a RAG-signed token containing both id and normal strict claims but omitting scopes first fails _principal_from_strict, then is marked legacy here merely because id is present. Principal.has_scope and permits_entity consequently grant every scope and arbitrary entity access. Fresh evidence after the prior fix is that the current predicate checks only for id, rather than rejecting a mixed id/sub token as non-legacy; require the genuine legacy claim shape before enabling the grandfather.

Useful? React with 👍 / 👎.

Comment on lines +297 to +298
resolve_scope(request, entity_id)
return get_user_id(request, entity_id), writer_tenant(request)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Enforce a write capability on document ingestion routes

When RAG authentication is configured, a strict token carrying only rag:documents can still call /embed, /embed-upload, and /local/embed: middleware admits strict tokens on non-/v1 paths, while this shared helper checks only the entity and tenant. Those routes then invoke the vector-store embedding backend, so a credential minted only to read or delete documents can write content and spend provider capacity outside the /v1 rate limits. Add an explicit write/inference capability check to all ingestion routes.

Useful? React with 👍 / 👎.

Comment on lines +356 to +357
for position, index in enumerate(pending):
vectors[index] = candidate_vectors[position]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate rerank vectors before using the backend response

If the rerank embedding backend returns fewer vectors than requested, this indexing step raises IndexError after the guarded backend call has completed; malformed component types likewise fail later in cosine_similarity. These exceptions bypass the sanitized backend-error handler and produce an unhandled 500 instead of the endpoint's documented 503, while wrong-width vectors can silently lose the similarity arm. Validate the query vector and the complete candidate-vector batch before leaving the try block, as the embeddings endpoint does.

Useful? React with 👍 / 👎.

Comment on lines +103 to +109
return self._collection.distinct(
"file_id",
{
"$and": [
{"user_id": {"$in": allowed}},
{"tenant_id": {"$in": list(tenants)}},
]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Index the scope fields used by Atlas ID listings

On Atlas, every GET /ids call executes this distinct("file_id") query using only user_id and tenant_id predicates, but the startup indexes begin with file_id and digest; neither can seek to the caller's scoped rows. The endpoint therefore scans the collection or the full file_id index and fetches rows for all tenants as the corpus grows. Add a standard index beginning with the scope fields and ending in file_id so scoped listings remain proportional to the caller's data.

Useful? React with 👍 / 👎.

for index, candidate in enumerate(body.candidates):
if candidate.id in stored or not candidate.text:
continue
text = normalize_text(candidate.text)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve store preprocessing when embedding rerank candidates

When a candidate vector is absent—or a stored-vector lookup transiently fails—the route embeds this NFKC-normalized, whitespace-collapsed text, whereas document ingestion embeds the original chunk text (apart from format-specific cleaning). The same candidate therefore receives a different vector and potentially a different rank depending only on whether its stored vector was available; code and other whitespace-sensitive content are especially affected. Use the same preprocessing as document ingestion, or normalize stored chunks and rebuild their vectors so both paths share one embedding space.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant