Repository navigation
fix: enforce ownership on Knowledge by-id read routes (#1056, SECURITY) - #1183
Merged
Merged
Conversation
…ge (#1056) The knowledge listing routes honour BYPASS_ADMIN_ACCESS_CONTROL (PR #960), but GET /{id}, GET /{id}/files, GET /{id}/files/pending and GET /{id}/export short-circuited on user.role == 'admin' and never consulted it. On this shared chat instance every tenant OWNER is an Open WebUI instance admin, so a collection id alone was enough to read another tenant's collection, including document content via include_content=true and a full zip download via the export route. New owui-patches/apply_knowledge_authz_patch.py rewrites all four gates in the built backend to the listing routes' predicate: owner, AccessGrants read grant, or admin when BYPASS_ADMIN_ACCESS_CONTROL is set. The export route drops its bare get_admin_user dependency for get_verified_user. Every edit asserts its own effect (exact-count anchors plus ast.parse) and fails the build otherwise; the Dockerfile RUN line independently counts the four in-image markers and re-parses the file.
…#1056) scripts/test_owui_knowledge_authz.py runs the real patch against the vendored knowledge.py in CI (make test-scripts) and asserts the four gates, the export dependency swap, and that the unpatched source still carries the defect as a negative control. The compose comment no longer describes knowledge by-id reads as discovery-only-closed, and the Caddyfile comment now points at the backend patch instead of a tracked-separately TODO; both stay accurate to what actually enforces what.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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 |
This was referenced Aug 25, 2026
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 25, 2026
…1186) (#1190) Closes #1186. ## Defect class The vendored Open WebUI backend short-circuits on bare `user.role == 'admin'` without consulting the tenant visibility flags this fork already sets (`BYPASS_ADMIN_ACCESS_CONTROL`, false in compose; chats.py's own `ENABLE_ADMIN_CHAT_ACCESS`, also false). On a shared chat instance every tenant OWNER is an instance admin (issue #748 family), so role alone granted cross tenant read and write access. PR #960 gated the knowledge listings and PR #1183 fixed the knowledge by-id reads; this PR applies the same predicate everywhere else. ## HIGH slice (commit 1) `retrieval/utils.py filter_accessible_collections` returned every requested collection for any admin-role caller, so `POST /api/v1/retrieval/query/collection` with a known KB id returned another tenant's RAG document chunks on the documented RAG path (worse than #1056: content exposure through the shared choke point that also feeds /query/doc and web search collection allowlisting). New `deploy/docker/owui-patches/apply_retrieval_authz_patch.py` rewrites it to the #960 predicate: owner, AccessGrants read grant, or admin when BYPASS_ADMIN_ACCESS_CONTROL is set. ## Family sweep (commit 2): 74 sites patched, 10 routers | File | Sites | Verdict | |---|---|---| | knowledge.py | 12 | 9 write gates (/{id}/update, /access/update, /file/add, /file/update, /file/remove, DELETE /{id}/delete, /reset, /files/batch/add, `_verify_knowledge_write_access` covering dirs/create, dirs/update, dirs/delete, file/move) + 3 adjacent same-class gates found in sweep: per-file read check in file/add (:1409), delete_file object deletion bypass in file/remove (:1614), batch per-file read-check skip (:2024) | | files.py | 10 | 9 content/read/download/update one-line gates + the upload path knowledge-write gate (:189) | | evaluations.py | 4 | GET/update/delete feedback by id: admin branch fetched or mutated any user's feedback | | folders.py | 7 | GET by id read gate, update write gate, access-grants rewrite gate, is_admin for folder chat listing (cross-tenant chat reads), subfolder delete gate, root-shared-folder delete gate, import added | | calendar.py | 2 | single choke point `_check_calendar_access` covers every CRUD route; :405/:446 owner checks become unreachable cross-tenant once it is gated | | chats.py | 7 | delete by id admin branch, message edit/delete/event gates x3, export-stats gate, plus shared-chat access-grant read/update branches x2, gated on ENABLE_ADMIN_CHAT_ACCESS (its own upstream flag, matching siblings at :1056/:1174/:1592); gated admins fall through to ownership-scoped else branches, verified ownership-enforcing | | prompts.py | 11 | 6 write tails + GET-by-id head + 4 if-not heads | | notes.py | 6 | 5 deny-shaped gates + the write_access response indicator so the UI stops offering cross-tenant edit affordances | | tools.py | 10 | GET-by-id head + 9 write tails | | models.py | 5 | `_verify_knowledge_file_access` skip, toggle gate, update/delete tails x2, import overwrite compound gate | False positives verified and NOT patched: knowledge.py reindex (:331) stays admin-global by design; feature-permission gates of the shape `role != admin AND not has_permission(...)` (notes/tools/folders/chats community-sharing/calendar automations) guard features, not other tenants' resources; files.py :828 requires the file OWNER to be an admin rather than bypassing anything. Mechanism: backend Python edits are inert in the chat image unless applied through owui-patches, so nothing here touches vendored source directly; both scripts follow #1183 exactly: fail-loud exact-count anchors, ast.parse gate, idempotent re-run exits 0. The consolidated Dockerfile drift guard asserts the exact marker count in every patched file (#1056 markers included) so a vendor bump cannot silently un-fix any hole (same silent-drift class as #1162). The compose comment claiming Knowledge was fully closed is softened to match reality. Verification: 1. Both patches run twice against copies of the vendored tree, second run reports already applied. 2. `python3 scripts/test_owui_knowledge_authz.py`: 42/42 PASS including per-file negative controls that keep the test able to go red on vendor drift. 3. Image build green from the branch tree (`hive-open-webui:secfamily`, log /tmp/secfamily-build.log): patch RUN lines report "flag-gated 74 unflagged admin bypasses across 10 routers", the consolidated drift guard passed, direct inspection of the built image shows every expected marker count (retrieval 2; knowledge 12 plus four #1056 markers; files 10; evaluations 4; folders 7; calendar 2; chats 7; prompts 11; notes 6; tools 10; models 5) and ast.parse passes over retrieval/utils.py and every router.
sakibsadmanshajib
added a commit
that referenced
this pull request
Sep 2, 2026
#1358) (#1707) Closes #1358. ## The false claim, and where it broke `ProjectDetail.svelte:308` tells the user "No files yet. Files here are given to every conversation in this project." Nothing delivered them. The trace, end to end: | Step | State on `main` | |---|---| | File uploaded into a project | Lands in the Open WebUI knowledge collection that IS the project, embedded into its own vector collection. Works. | | Conversation created in the project | `createBoundChat` (`vendor/open-webui/src/lib/hive/projects/projects.ts:327`) writes `{ chat: { hiveProject: projectId } }` and nothing else. | | Conversation assembles a request | `sendMessageSocket` (`vendor/open-webui/src/lib/components/chat/Chat.svelte:2799`) builds `files` from `chatFiles` plus this turn's own attachments. It has never read `hiveProject`; before this change the key appeared in no file outside `src/lib/hive/projects/`. | | Model sees the documents | No. `files` reaches the request without a single project document in it. | So the break point is `Chat.svelte:2828`, the `files` assembly inside `sendMessageSocket`. The value the marker was supposed to carry had no reader. ## Why the obvious fix does not work The issue proposes wiring the project onto `chat.files` at `createBoundChat` time. That fails, and the reason is the interesting half of this defect. Immediately above the assembly, `sendMessageSocket` prunes: ```js const chatMessageFiles = _messages.filter((message) => message.files).flatMap((message) => message.files); // Filter chatFiles to only include files that are in the chatMessageFiles chatFiles = chatFiles.filter((item) => { const fileExists = chatMessageFiles.some((messageFile) => messageFile.id === item.id); return fileExists; }); ``` An attachment written onto the chat blob at creation is referenced by no message, so it is deleted on the first send, and then `saveChatHandler` persists `files: chatFiles` and deletes it from the stored chat as well. The wiring would exist and the value would still never arrive, which is the shape this repository keeps hitting. That is also why a manual attach works today: the plus menu puts the collection on the user message, so a message references it and the prune keeps it. ## The fix Resolve the binding at request assembly, after the prune, in the one function every entry point reaches. * `Chat.svelte` reads `hiveProject` off the chat blob at load (`hiveProjectId`), clears it when a new chat starts, and calls `withProjectFiles(files, hiveProjectId)` at the end of the `files` assembly in `sendMessageSocket`. `submitPrompt`, regeneration and continue all route through that one function, so no sibling entry point is left broken. * `withProjectFiles` in `projects.ts` appends `{ type: 'collection', id: projectId }` when the chat is bound and the project is not already on the turn, and returns the identical array otherwise. A reference, not a snapshot. The collection is resolved to its current file set on every request, so a file uploaded into the project tomorrow reaches a conversation created today, which is what the sentence on the project page actually claims. A list of file ids captured at bind time would not do that. ## The other half: the conversation the project page creates The project page's **New chat** used to create the conversation itself. That was wrong twice over, and a review caught the second one. It created a blob carrying the marker alone, so `loadChat` reached `convertMessagesToHistory(undefined)` and threw before the page rendered. And a conversation that already exists makes its first request carry a `chat_id`, so the backend's `is_new_chat` is false (`main.py:1096`) and the title and tag tasks are dropped for it permanently: every conversation in a project would have read "New Chat" forever, and the project's list would have been a column of identical rows. So the page now creates nothing. It navigates to `/?project=<id>`, `initNewChat` picks the binding off the query string the way it already picks up `models` and `youtube`, and the ordinary composer path supplies the blob, the models, the title and the tags. Two helper functions this branch had added, `createBoundChat` and `seedChatModels`, are deleted rather than kept in step with the code they were re-implementing. One measured correction to that shape, because the obvious place to write the binding does not work here. `initChatHandler`'s `createNewChat` never runs on the send path on this fork: a send produces no `POST /api/v1/chats/new` at all, and the stored blob carries no `params`, which that payload would have supplied. The chat row is created server side from the completion request. That is why the branch handling the returned id already persists chat level params "that the backend doesn't receive in the chat completion request", and the binding is written one line below it, for the same reason. The tradeoff, stated rather than buried: the conversation appears in the project once it has a first message rather than immediately. An abandoned pre created chat was a permanent empty row, so this is the better behaviour. ## Retrieval scope: unchanged, stated explicitly This touches document retrieval, so the scope question is answered directly rather than by omission. `{ type: 'collection', id }` is byte for byte the item `MessageInput/InputMenu/Knowledge.svelte:179` already produces when a user attaches the same project by hand. It lands in the existing `item.get('type') == 'collection'` branch of `get_sources_from_items` (`backend/open_webui/retrieval/utils.py:1468`), which resolves the collection only after checking `user.role == 'admin' or knowledge_base.user_id == user.id or AccessGrants.has_access(...)`, and whose collection names then pass through `filter_accessible_collections`, the choke point PR #1183 and the #1186 patch hardened. So: no new retrieval path, no new permission, no widening. A user who can open a project bound conversation is a user who could already attach that project from the composer and get the same passages. In particular this does not touch `/v1/rag/*`, so it neither helps nor worsens #1643, the tenant readable RAG store; that issue keeps its full scope. ## Known gap, named rather than left quiet Cowork turns never reach the injection. `submitHandler` returns after `submitCoworkRun`, so a run started inside a project receives none of its documents. That is issue #1312 and task 8 of the Projects unification spec, both of which already own it, and closing it here would mean a `project_id` on `POST /v1/agent/tasks` with an ownership check in Go. It is recorded in the code comment at the injection site so it cannot go quiet, along with the one cost this adds: a non empty `files` array makes the backend run `generate_queries`, so a project bound turn spends one extra task model completion even while the project is empty. Retrieval over an empty collection returns no sources and the injection is guarded, so no answer is degraded. ## Visual proof Posted on this pull request, four frames from a chat image built from this branch on a fresh instance, plus the log in `docs/proof/projects-attach-delivers-1358-2026-09-02/`. Re-captured after review with title generation left at its default, on. The first capture had disabled it, which made it structurally incapable of showing the defect above: a proof configured with the relevant feature off proves that feature's absence, not its correctness. The load bearing line is not the screenshot, it is the outgoing request body read off the wire in each case: ``` control, before the project existed: files=null in the project, same model: files=[{"type":"collection","id":"<the project>"}] ``` The document's only distinguishing content is a string that exists nowhere else. In the project the conversation renders "Retrieved 1 source", answers with it, and cites `rack-register.txt`. In the control the model searched, found nothing and said so. And the two things the review said this had to show, both from the same run: ``` generated conversation title: What is the rack asset tag of ... title is still the placeholder: false stored chat: hiveProject=f6aab089-94d1-4134-89d8-6533089c3e42 project page conversation rows: ["rack-register.txt Remove","What is the rack asset tag of the Hive demo box? Unlink"] ``` What the capture still cannot show is the denial path, because it runs with `WEBUI_AUTH=false` and therefore has one identity. That claim rests on the independent backend review, not on these frames. ## Tests `vendor/open-webui/src/lib/hive/projects/projects.test.ts`, seven new cases, written before the fix and observed red for the right reasons (`withProjectFiles` absent, and `Chat.svelte` containing no read of the marker and no call at the assembly site): * the bound chat case starts from the state the prune actually leaves, an empty `files` array, which is the case a naive blob attachment fails; * the turn's own attachments survive alongside the project; * an unbound chat gets the identical array back; * attaching the project by hand does not duplicate it; * the attached item carries `type` and `id` only, pinning reference semantics rather than a snapshot; * guards on `Chat.svelte`'s source for the URL pickup, the write of the binding once the backend returns the new id, the load off an existing blob, and both reset sites; * an ordering guard, added in review, so an edit that moves the attachment above the `chatFiles` prune fails rather than silently restoring the exact bug this description spends half its length on; * two guards on `ProjectDetail.svelte`, so the second chat creation path cannot come back. `make test-owui-frontend`: 380 passed, 25 files. The same sources run again in place during the chat image build (`npm run test:frontend`), so this is a build time gate, not only a CI one. ## Relationship to #1595 The Projects unification spec (`spec-2026-08-31-projects-unification`, Obsidian vault) moves Projects onto Hive's own `/v1/rag/*` store, and its task 6 fixes #1358 as a side effect of the new attachment control. Tasks 4, 5 and 7 of that spec, the proxy routes, the repointed `projects.ts` and the `hive_project` retrieval branch, are not built: nothing on `main` reads `rag_projects` from the chat surface, and no pull request is open against them. This change is deliberately scoped to the store Projects is bound to today. When task 5 repoints `projects.ts`, `withProjectFiles` is the one line that changes, from `type: 'collection'` to `type: 'hive_project'`, and the call site in `Chat.svelte` and every test above stay as they are. It does not block, duplicate or pre-empt that work; it stops the interface lying in the meantime. Spec acceptance criteria 1, 5 and 6 remain #1595's, and remain unmet: they are about the new store, the `ENABLE_RAG` default and cross member project isolation on `/v1/rag/*`, none of which this touches. ## Buglog entry ```json {"date":"2026-09-02","issue":1358,"title":"Projects promised files to every conversation and delivered none","error_message":"A conversation created inside a Project answered as if the project's uploaded documents did not exist, while ProjectDetail.svelte stated 'Files here are given to every conversation in this project'.","root_cause":"createBoundChat wrote only the hiveProject marker onto the chat blob and no code outside src/lib/hive/projects read it, so Chat.svelte's sendMessageSocket assembled the request files with no project document in them. The apparent fix, writing the attachment onto chat.files at bind time, also fails: sendMessageSocket prunes chatFiles to only those files some message in the branch references, so a chat level attachment is dropped on the first send and then written out of the persisted chat by the next save.","fix":"Resolve the binding at request assembly instead of persisting it. Chat.svelte reads hiveProject off the chat blob at load, clears it on a new chat, and calls withProjectFiles(files, hiveProjectId) after the prune and dedupe in sendMessageSocket, the one function submit, regeneration and continue all reach. withProjectFiles appends { type: 'collection', id } , the same item the plus menu produces, so retrieval scope and access control are unchanged.","tags":["owui-fork","projects","rag","claimed-state-no-reader","chat-svelte","frontend"]} ```
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SECURITY fix: Knowledge by-id routes read across tenants (residual half of #947)
Closes #1056.
Do not merge before the other parity-sec fixes; reviewers please prioritize.
Defect
PR #960 gated the knowledge LISTING routes (
GET /,GET /search) onBYPASS_ADMIN_ACCESS_CONTROL, which docker-compose.yml sets tofalse. Three by-id read routes in the chat backend never consulted it and short-circuited onuser.role == 'admin':GET /api/v1/knowledge/{id}GET /api/v1/knowledge/{id}/filesinclude_content=truereturned extracted document textGET /api/v1/knowledge/{id}/exportget_admin_user, no ownership check at all: full zip of any tenant's collection given only its idA fourth route,
GET /{id}/files/pending, carries the identical guard text and got the same fix in the same edit.On this shared chat instance every tenant OWNER is an Open WebUI instance admin (issue #748 family), so a collection id, which is not secret (shared links, support tickets, logs), was enough to read another tenant's knowledge.
Mechanism
The chat image builds only the frontend from
vendor/open-webui; the backend comes from the pinned upstream image, so an edit to vendored backend Python is inert at runtime unless applied throughowui-patches. This PR follows the established pattern (apply_tenant_role_patch.pyand siblings): newdeploy/docker/owui-patches/apply_knowledge_authz_patch.pyrewrites all four gates at image build time with exact-count anchors plus anast.parsecheck, failing the build loudly if upstream source shifts.Enforcement semantics
All four routes now use exactly the listing routes' predicate: access iff owner, or
AccessGrants.has_access(..., permission='read'), or admin whenBYPASS_ADMIN_ACCESS_CONTROLis set. With the compose valuefalse, instance admins stop reading across tenants; flipping the flag restores upstream behaviour for deployments that want it.Verification
grep -c '# hive (#1056)' -eq 4,ast.parse)./app/backend/open_webui/routers/knowledge.py, export route nowDepends(get_verified_user).scripts/test_owui_knowledge_authz.py(wired intomake test-scripts) passes locally, including its negative control that keeps the test able to go red.