mckaywrigley/chatbot-ui

delete_storage_object(bucket, object) and delete_storage_object_from_bucket(...) are created in the public schema as SECURITY DEFINER and perform an HTTP DELETE against Storage using a hardcoded service_role_key (line 54). Supabase exposes every public function through PostgREST (/rest/v1/rpc/...) and by default grants EXECUTE to anon and authenticated. Nothing in the function checks auth.uid() or ownership.

Anyone with the public anon key (it ships in the browser bundle) can POST /rest/v1/rpc/delete_storage_object {"bucket":"files","object":"<victim>/<path>"} and delete other users' files, message images, assistant/workspace/profile images, without even being logged in. The returned status/content also lets them probe object existence.

Revoke public execution and keep these helpers callable only from triggers: REVOKE EXECUTE ON FUNCTION public.delete_storage_object(text,text) FROM public, anon, authenticated; REVOKE EXECUTE ON FUNCTION public.delete_storage_object_from_bucket(text,text) FROM public, anon, authenticated; Optionally move them to a non-exposed schema (e.g. private) and update the trigger functions to call private.delete_storage_object_from_bucket. Do the same review for any other SECURITY DEFINER function in public.

Add a new Supabase migration that locks down the storage deletion helpers. In the new SQL file run: REVOKE EXECUTE ON FUNCTION public.delete_storage_object(text, text) FROM public, anon, authenticated; REVOKE EXECUTE ON FUNCTION public.delete_storage_object_from_bucket(text, text) FROM public, anon, authenticated; GRANT EXECUTE ON FUNCTION public.delete_storage_object(text,text) TO service_role; GRANT EXECUTE ON FUNCTION public.delete_storage_object_from_bucket(text,text) TO service_role; Reason: these are SECURITY DEFINER functions defined in supabase/migrations/20240108234540_setup.sql that call Storage with the service role key, and Supabase exposes public functions over PostgREST RPC to anon/authenticated by default, so any anonymous caller can delete any user's stored files. The trigger functions (delete_old_file, delete_old_profile_image, etc.) are themselves SECURITY DEFINER so they will still be able to call them.

The route never calls getServerProfile() or otherwise checks the session. It creates a service-role Supabase client (lines 20-23) and loads models by customModelId taken straight from the request body (line 28), then uses that model's api_key and base_url (lines 35-38) to make completions with whatever model/messages the caller sends.

Any unauthenticated caller who knows or guesses a models.id can run unlimited completions billed to another user's API key. It is also a server-side request to an attacker-controlled base_url. No rate limiting makes the abuse cheap.

Require auth and ownership: const profile = await getServerProfile(); then query models with the user's own client (RLS) or add .eq("user_id", profile.user_id) to the admin query and return 404/403 if not found. Prefer creating the Supabase client with the user's cookies (createServerClient + anon key) so RLS applies and the service-role key is not needed here at all.

In app/api/chat/custom/route.ts, require authentication and ownership before using a custom model. Import getServerProfile from @/lib/server/server-chat-helpers, call `const profile = await getServerProfile()` at the top of the try block, and change the models query to `.eq("id", customModelId).eq("user_id", profile.user_id).single()`. Better: replace the service-role createClient with a createServerClient using NEXT_PUBLIC_SUPABASE_ANON_KEY and the request cookies (same pattern as lib/server/server-chat-helpers.ts) so RLS enforces ownership. Return a 401 JSON response if getServerProfile throws 'User not found'. Reason: today this route is unauthenticated and reads any user's models.api_key/base_url with the service role key, letting anyone burn other users' API credits.

service_role_key is a literal inside delete_storage_object. The README (Hosted Quickstart step 2.4) instructs replacing it with the hosted project's real service role key and running `supabase db push`, which means the key is committed to the repo and stored in plaintext in the function definition.

Anyone with read access to the repo (contributors, leaked fork, CI logs) gets a key that bypasses all RLS and Storage policies. Rotating the key requires a new migration.

Do not embed the key. Options: (a) store it in Supabase Vault and read it with vault.decrypted_secrets inside the function; (b) use a Postgres GUC set outside version control (ALTER DATABASE postgres SET app.settings.service_role_key = '...'; current_setting('app.settings.service_role_key')); (c) drop the HTTP approach and delete storage objects from the app server with the env SUPABASE_SERVICE_ROLE_KEY when a row is deleted.

Refactor supabase/migrations/20240108234540_setup.sql so the service role key is not a literal in delete_storage_object. Create a new migration that redefines public.delete_storage_object to read the key via `current_setting('app.settings.service_role_key', true)` (and project_url via `current_setting('app.settings.project_url', true)`), and document in README that operators must run `ALTER DATABASE postgres SET app.settings.service_role_key = '<key>'` (or use Supabase Vault) instead of editing the migration. Remove the README instruction telling users to paste the production service role key into the migration file. Reason: committing a service role key to git gives full RLS-bypassing access to anyone who can read the repository.

fileIds come from the JSON body (line 9) and are passed unchanged to supabaseAdmin.rpc("match_file_items_openai"/"match_file_items_local") (lines 60-64, 75-79). The RPC is not SECURITY DEFINER, but because it is executed with the service-role client, RLS on file_items is bypassed. The route authenticates the caller (getServerProfile) but never checks that the files belong to them.

Any logged-in user who obtains another user's file UUIDs (e.g. from a shared assistant/collection response, logs, or a leaked link) can pull the full text chunks of that user's private documents. UUIDs are unguessable, which limits blind exploitation, but authorization should not depend on that.

Filter fileIds to those the user owns before calling the RPC, or call the RPC with a user-scoped client so RLS applies: const { data: owned } = await supabaseAdmin.from("files").select("id").in("id", uniqueFileIds).eq("user_id", profile.user_id); const allowedIds = owned.map(f => f.id). Also cap sourceCount (e.g. Math.min(sourceCount, 10)).

In app/api/retrieval/retrieve/route.ts, after `const profile = await getServerProfile()`, verify ownership of the requested files before running the vector search: query `supabaseAdmin.from("files").select("id").in("id", uniqueFileIds).eq("user_id", profile.user_id)` and replace uniqueFileIds with the returned ids (return an empty results array if none). Also clamp sourceCount to an integer between 1 and 10. Reason: the RPC is executed with the service-role client which bypasses RLS, so a user could currently retrieve chunks from any other user's files by supplying their file IDs.

Unlike the generic process route (which checks fileMetadata.user_id !== profile.user_id at line 46), the docx route takes fileId and text from the body and, using supabaseAdmin, upserts file_items rows (line 101) and updates files.tokens (lines 105-108) for that fileId with no check that the file belongs to the caller. text is also unbounded.

An authenticated user can inject arbitrary chunks into another user's file (poisoning that user's retrieval results / prompt-injecting their chats) and overwrite the file's token count. Unbounded text also allows very large embedding batches.

Load the file first and verify ownership exactly like process/route.ts: const { data: fileMetadata } = await supabaseAdmin.from("files").select("user_id").eq("id", fileId).single(); if (!fileMetadata || fileMetadata.user_id !== profile.user_id) return 403. Add a maximum text length (e.g. 2M chars) and validate fileExtension === "docx".

In app/api/retrieval/process/docx/route.ts, add an ownership check before any writes: after `const profile = await getServerProfile()`, fetch `supabaseAdmin.from("files").select("id, user_id").eq("id", fileId).single()` and return a 403/404 JSON response if the row is missing or `user_id !== profile.user_id` (mirror the check in app/api/retrieval/process/route.ts lines 30-48). Also reject `text` longer than a sane limit (e.g. 2,000,000 chars) with a 413. Reason: this route uses the service-role client and currently lets any authenticated user write file_items into any other user's file and overwrite its token count.

selectedTools (including schema and custom_headers) is read directly from the JSON body (lines 11-15, 31-34) rather than from the tools table, so the caller controls servers[0].url. The route then fetches schemaDetail.url + path with those headers (lines 153, 178). This route has no `runtime = "edge"`, so it runs on Node inside your hosting network. Responses are parsed with response.json() regardless of content type.

Any authenticated user can make your server issue GET/POST requests to internal addresses (cloud metadata endpoints, private services, localhost) and read the responses back through the model output. Non-JSON responses also crash the request with a 500.

Load tools by id from the DB with a user-scoped client instead of trusting the body (accept selectedToolIds). Validate the resolved server URL: must be https, hostname must not resolve to private/loopback/link-local ranges, and consider an allowlist. Wrap fetch in a timeout (AbortController) and check content-type before .json().

Harden app/api/chat/tools/route.ts against SSRF. 1) Change the request contract so the client sends `selectedToolIds: string[]` and the route loads the tools with a cookie-based Supabase client (RLS) via `.from("tools").select("*").in("id", selectedToolIds)`; update components/chat/chat-hooks/use-chat-handler.tsx (around line 289) to send ids. 2) Before each fetch, parse the target with `new URL(fullUrl)`, require protocol === "https:", and reject hostnames that are localhost, an IP literal in 10/8, 172.16/12, 192.168/16, 127/8, 169.254/16, or ::1/fc00::/fe80::. 3) Add an AbortController timeout (10s) to each fetch and only call response.json() when the content-type includes application/json, otherwise use response.text(). Reason: the route currently fetches attacker-controlled URLs with attacker-controlled headers from a Node runtime, enabling internal-network requests.

addApiKeysToProfile overrides every user's profile keys with server env keys when set (lines 60-64), so a single OPENAI_API_KEY etc. is used by all accounts. Signup is open with no email confirmation (app/[locale]/login/page.tsx lines 125-139; the whitelist is optional). None of /api/chat/*, /api/command, /api/retrieval/* or /api/assistants/openai apply rate limiting, per-user quotas, or model/input-size validation; chatSettings.model and messages are passed through verbatim.

Anyone can create accounts and run unlimited gpt-4/claude-opus requests or embed huge files on your bill. Even without env keys, the routes can be hammered to exhaust serverless quotas.

Add per-user and per-IP rate limits (e.g. Upstash Ratelimit or Vercel KV) to every route under app/api. Enforce a max request body size and message token budget server-side. If env keys are used, add a per-user daily token/cost budget table and check it before each call. Enable email confirmation or require the whitelist in production.

Add rate limiting and basic quotas to all API routes in app/api (chat/*, command, retrieval/*, assistants/openai). Create lib/server/rate-limit.ts using @upstash/ratelimit + @upstash/redis (sliding window, e.g. 30 requests/minute per profile.user_id and 60/min per IP from the x-forwarded-for header) and call it right after getServerProfile() in each route, returning a 429 JSON response when exceeded. Also validate that `chatSettings.model` is one of the models in lib/models/llm/llm-list.ts (or the user's own models/openrouter) and reject bodies over ~1MB. Reason: lib/server/server-chat-helpers.ts gives every signed-up user the server's env API keys and signup is open without email confirmation, so unmetered routes expose the owner to unbounded provider costs.

createFile/createDocXFile call uploadFile with file_id: createdFile.name (lines 120-124, 180-184), and uploadFile builds filePath = `${user_id}/${base64(file_id)}` with upsert: true (db/storage/files.ts lines 22-28). Two files with the same (sanitized) name for one user therefore map to the same storage object.

Uploading a second "notes.pdf" silently replaces the first file's bytes while the first DB record still points at the same path; deleting either record triggers delete_old_file and removes the object for both. Users lose documents without any error.

Use the DB id in the path: in db/files.ts pass file_id: createdFile.id and build filePath as `${user_id}/${createdFile.id}` (no base64 needed). Set upsert: false so accidental collisions fail loudly.

In db/files.ts, in both createFile (around line 120) and createDocXFile (around line 180), change the uploadFile payload to use `file_id: createdFile.id` instead of `createdFile.name`. In db/storage/files.ts change the path to `${payload.user_id}/${payload.file_id}` (drop the base64 encoding) and set `upsert: false`. Reason: today the storage key is derived from the file name, so two uploads with the same name overwrite each other's bytes and deleting one deletes the other's storage object.