diff --git a/CLAUDE.md b/CLAUDE.md index ce18eed..58f0224 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -195,9 +195,9 @@ For a per-user connector whose credential is produced by **pairing** (`auth.type ## Multimodal attachments -Uploads (`POST /api/{source}/uploads`) are saved per-user under `data/uploads/{userid}/{session_id}/` (older rows may still reference the pre-namespacing `data/uploads/{session_id}/` layout — both stay readable), streamed to disk with a 256 MiB cap, with the sniffed magic-byte MIME preferred over the client claim; `/data/*` is served behind the same session-cookie gate as `/api`. Attachment metadata travels as structured JSON in `chat_history.metadata` — never as persisted text. +Uploads go through **one centralized seam** — `ChatHub::save_upload` (behind `ChatHubApi::save_upload`, backed by `skald_core::uploads::save_to_home`) — so every surface persists identically and no two callers can drift on placement (the class of bug where the agent was handed a path it couldn't reach). The seam writes into the **caller's container home** under `uploads/{session_id}/` (agent path `uploads/{session}/{name}`, the `UPLOADS_SUBDIR` const in `core-api/user_fs.rs`), collision-dedupes the name, and prefers the sniffed magic-byte MIME over the client claim. The **web** handler (`POST /api/{source}/uploads`) buffers each field with a 256 MiB cap then calls the seam; the **Telegram** plugin downloads bytes then calls the same seam via `handle.chat_hub().save_upload("telegram", …)`. Because the file lands in the home (bind-mounted at `/root`), it is reachable by the fs-tools, `execute_cmd`, and the file viewer (`GET /api/file`, per-user via `resolve_view_path`) — there is **no** `/data` static route anymore (removed: it was `require_auth`-only, not ownership-scoped, and also exposed internal server state under `data/`). Attachment metadata travels as structured JSON in `chat_history.metadata` — never as persisted text. -At context-build time (`MessageBuilder`), attachments of the **current turn** (the user/agent rows following the last completed assistant reply, including across in-flight tool rounds) are partitioned by `session/handler/media.rs`: when the resolved model's `LlmEntry.capabilities` include the modality (`vision` → `image_url` parts, `video` → `video_url` parts), the file is inlined as a base64 data-URL content part — but only if it canonicalizes under `data/uploads/`, its sniffed MIME is in the allowlist, and it fits the budgets (4 files / 10 MiB image / 32 MiB video / 48 MiB total per turn). Everything else — older turns, other kinds, any failed check — keeps the textual `[SYSTEM INFO]` path block, so a non-vision model produces a byte-identical payload to before. `OpenAiClient` forwards parts verbatim; `AnthropicClient` translates `image_url` data URLs to `image` blocks (video unsupported; Anthropic models get `vision` by editing the model row's capabilities — no catalog refresh writes them). On LLM fallback mid-round, messages are rebuilt with the replacement model's capabilities. +At context-build time (`MessageBuilder`), attachments of the **current turn** (the user/agent rows following the last completed assistant reply, including across in-flight tool rounds) are partitioned by `session/handler/media.rs`: when the resolved model's `LlmEntry.capabilities` include the modality (`vision` → `image_url` parts, `video` → `video_url` parts), the file is inlined as a base64 data-URL content part — but only if it resolves (through the caller's `UserFs`, via `resolve_host_path`) under the home's `uploads/` dir, its sniffed MIME is in the allowlist, and it fits the budgets (4 files / 10 MiB image / 32 MiB video / 48 MiB total per turn). Everything else — older turns, other kinds, any failed check — keeps the textual `[SYSTEM INFO]` path block, so a non-vision model produces a byte-identical payload to before. `OpenAiClient` forwards parts verbatim; `AnthropicClient` translates `image_url` data URLs to `image` blocks (video unsupported; Anthropic models get `vision` by editing the model row's capabilities — no catalog refresh writes them). On LLM fallback mid-round, messages are rebuilt with the replacement model's capabilities. ## Token streaming & reasoning display diff --git a/crates/core-api/src/chat_hub.rs b/crates/core-api/src/chat_hub.rs index a9f1849..280bc94 100644 --- a/crates/core-api/src/chat_hub.rs +++ b/crates/core-api/src/chat_hub.rs @@ -5,7 +5,7 @@ use tokio::sync::broadcast; use crate::events::GlobalEvent; use crate::interface_tool::InterfaceTool; -use crate::message_meta::MessageMetadata; +use crate::message_meta::{Attachment, MessageMetadata}; // ── SendMessageOptions ──────────────────────────────────────────────────────── @@ -59,6 +59,20 @@ pub trait ChatHubApi: Send + Sync { opts: SendMessageOptions, ) -> anyhow::Result<()>; + /// Persist an uploaded file for `source_id` into the owner's + /// `~/uploads/{session}/` and return its [`Attachment`] (home-relative agent + /// path). Channel adapters (e.g. the Telegram plugin) call this instead of + /// writing files themselves, so the core owns *where* uploads land and every + /// surface produces a path the agent can actually reach. The recognized + /// magic-byte MIME wins over the caller-claimed `client_mime`. + async fn save_upload( + &self, + source_id: &str, + file_name: &str, + client_mime: Option, + bytes: &[u8], + ) -> anyhow::Result; + /// Create a new session for the source, discarding the previous one. async fn clear(&self, source_id: &str) -> anyhow::Result; diff --git a/crates/core-api/src/message_meta.rs b/crates/core-api/src/message_meta.rs index e14c25f..cde40b1 100644 --- a/crates/core-api/src/message_meta.rs +++ b/crates/core-api/src/message_meta.rs @@ -12,9 +12,10 @@ use serde::{Deserialize, Serialize}; -/// One file attached by the user to a message. `path` is relative to the project -/// root (e.g. `data/uploads/123/file.pdf`) so it is both servable under `/data/…` -/// and resolvable by the filesystem tools. +/// One file attached by the user to a message. `path` is a home-relative agent +/// path (e.g. `uploads/123/file.pdf`) — the caller's container home is its root, +/// so the fs-tools, `execute_cmd`, the file viewer (`/api/file`) and the media +/// inliner all resolve it to the same physical file. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct Attachment { pub path: String, diff --git a/crates/core-api/src/user_fs.rs b/crates/core-api/src/user_fs.rs index d8e8cd4..6cb57dc 100644 --- a/crates/core-api/src/user_fs.rs +++ b/crates/core-api/src/user_fs.rs @@ -22,6 +22,12 @@ use std::path::{Component, Path, PathBuf}; use std::sync::{Arc, RwLock}; +/// The subdirectory of a user's home where chat uploads are saved +/// (`{home}/uploads/{session_id}/…`, reachable by the agent as `uploads/…`). +/// Shared by the upload handler (write path) and the media inliner (containment +/// root) so the two anchors can never drift. +pub const UPLOADS_SUBDIR: &str = "uploads"; + /// One shared folder mounted into a user's container. #[derive(Debug, Clone)] pub struct SharedMount { diff --git a/crates/plugin-telegram-bot/src/attachments.rs b/crates/plugin-telegram-bot/src/attachments.rs index da67595..19269a5 100644 --- a/crates/plugin-telegram-bot/src/attachments.rs +++ b/crates/plugin-telegram-bot/src/attachments.rs @@ -1,7 +1,6 @@ use std::path::Path; use anyhow::Result; -use core_api::message_meta::Attachment; use teloxide::net::Download; use teloxide::prelude::*; @@ -10,9 +9,9 @@ use teloxide::prelude::*; /// # Extending /// Add a new variant here, then handle it in: /// 1. `handlers::classify_message` — detect the message type and build the variant -/// 2. `TelegramAttachment::download_and_save` — fetch bytes and persist to disk, -/// returning an [`Attachment`] -/// (return `Ok(None)` if no file is involved) +/// 2. `TelegramAttachment::download` — fetch the bytes (return `Ok(None)` if no +/// file is involved); the caller persists them +/// via the shared `ChatHubApi::save_upload` seam /// 3. `TelegramAttachment::system_info_message` — describe a file-less variant /// (Location) for the LLM pub(crate) enum TelegramAttachment { @@ -36,20 +35,16 @@ pub(crate) enum TelegramAttachment { } impl TelegramAttachment { - /// Downloads the attachment from Telegram, writes it to `base_dir//`, - /// and returns the saved [`Attachment`] (shared with the web/mobile path so the - /// copilot UI renders it identically). Returns `None` for attachment types that - /// carry no binary content (e.g. Location). - /// - /// The returned `path` is made relative to the process working directory (the - /// project root) when possible, so it is both servable under `/data/…` and - /// resolvable by the filesystem tools — matching web uploads. - pub(crate) async fn download_and_save( + /// Downloads the attachment's bytes from Telegram, returning + /// `(file_name, mimetype, bytes)`. Persistence is **not** done here: the caller + /// hands the bytes to the shared upload seam (`ChatHubApi::save_upload`), which + /// saves them into the user's home under `uploads/{session}/…` and produces the + /// [`Attachment`] — the same path every surface uses, so the agent can reach it. + /// Returns `None` for attachment types that carry no binary content (e.g. Location). + pub(crate) async fn download( &self, - bot: &Bot, - base_dir: &Path, - chat_id: i64, - ) -> Result> { + bot: &Bot, + ) -> Result, Vec)>> { let (file_id, file_name, mimetype): (&str, String, Option) = match self { Self::Document { file_id, file_name, mime_type, .. } => (file_id, file_name.clone(), mime_type.clone()), @@ -58,28 +53,11 @@ impl TelegramAttachment { Self::Location { .. } => return Ok(None), }; - let dir = base_dir.join(chat_id.to_string()); - tokio::fs::create_dir_all(&dir).await?; - let tg_file = bot.get_file(teloxide::types::FileId(file_id.to_string())).await?; let mut bytes = Vec::new(); bot.download_file(&tg_file.path, &mut bytes).await?; - let path = dir.join(&file_name); - tokio::fs::write(&path, &bytes).await?; - - // Prefer a project-root-relative path so `/data/…` serving works. - let rel = std::env::current_dir() - .ok() - .and_then(|cwd| path.strip_prefix(&cwd).ok().map(Path::to_path_buf)) - .unwrap_or_else(|| path.clone()); - - Ok(Some(Attachment { - path: rel.to_string_lossy().to_string(), - name: file_name, - mimetype, - filesize: Some(bytes.len() as u64), - })) + Ok(Some((file_name, mimetype, bytes))) } /// Builds the `[TELEGRAM SYSTEM INFO]` message injected into the conversation history. diff --git a/crates/plugin-telegram-bot/src/handlers.rs b/crates/plugin-telegram-bot/src/handlers.rs index 6942c25..8f992d1 100644 --- a/crates/plugin-telegram-bot/src/handlers.rs +++ b/crates/plugin-telegram-bot/src/handlers.rs @@ -512,17 +512,32 @@ async fn handle_attachment( bot.send_chat_action(chat_id, ChatAction::UploadDocument).await.ok(); - let saved = match attachment.download_and_save(&bot, &shared.uploads_dir, chat_id.0).await { - Ok(s) => s, + let downloaded = match attachment.download(&bot).await { + Ok(d) => d, Err(e) => { - error!(error = %e, "telegram: failed to save attachment"); - bot.send_message(chat_id, "⚠️ Could not save the attachment.").await.ok(); + error!(error = %e, "telegram: failed to download attachment"); + bot.send_message(chat_id, "⚠️ Could not download the attachment.").await.ok(); return; } }; - match saved { - Some(att) => { + match downloaded { + Some((file_name, mimetype, bytes)) => { + // Persist through the shared upload seam so the file lands in the user's + // home (`uploads/{session}/…`) with an agent-reachable path — identical + // to a web upload. + let att = match handle + .chat_hub() + .save_upload("telegram", &file_name, mimetype, &bytes) + .await + { + Ok(a) => a, + Err(e) => { + error!(error = %e, "telegram: failed to save attachment"); + bot.send_message(chat_id, "⚠️ Could not save the attachment.").await.ok(); + return; + } + }; info!(chat_id = chat_id.0, path = %att.path, "telegram: attachment saved, forwarding to LLM"); let caption = match &attachment { TelegramAttachment::Document { caption, .. } => caption.clone(), diff --git a/crates/plugin-telegram-bot/src/lib.rs b/crates/plugin-telegram-bot/src/lib.rs index 5820fe8..bc3adb1 100644 --- a/crates/plugin-telegram-bot/src/lib.rs +++ b/crates/plugin-telegram-bot/src/lib.rs @@ -26,7 +26,6 @@ /// `ApprovalApi`. use std::collections::{HashMap, HashSet}; -use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; @@ -106,7 +105,6 @@ pub(crate) struct TgShared { pub(crate) transcribe: Arc, pub(crate) tts: Arc, pub(crate) location: Arc, - pub(crate) uploads_dir: PathBuf, // ── Pairing / bindings (config-table-backed, cached in memory) ── pub(crate) bindings: RwLock, @@ -272,11 +270,6 @@ impl Plugin for TelegramPlugin { anyhow::bail!("telegram: token is empty — set it via the plugins API"); } - let uploads_dir = std::env::current_dir() - .unwrap_or_default() - .join("uploads") - .join("telegram"); - // Load bindings from the config table (or default if absent). let telegram_config = auth::load_config(&*ctx.config).await .unwrap_or_default(); @@ -293,7 +286,6 @@ impl Plugin for TelegramPlugin { transcribe: Arc::clone(&ctx.transcribe), tts: Arc::clone(&ctx.tts_provider), location: Arc::clone(&ctx.location), - uploads_dir, bindings: RwLock::new(telegram_config), pending_approvals: Mutex::new(HashMap::new()), pending_questions: Mutex::new(HashMap::new()), diff --git a/crates/skald-core/src/chat_hub/mod.rs b/crates/skald-core/src/chat_hub/mod.rs index ce9e54b..a5daf7d 100644 --- a/crates/skald-core/src/chat_hub/mod.rs +++ b/crates/skald-core/src/chat_hub/mod.rs @@ -4,6 +4,7 @@ use std::sync::{Arc, OnceLock, Weak}; use std::time::Duration; use async_trait::async_trait; +use core_api::message_meta::Attachment; use sqlx::SqlitePool; use tokio::sync::{Mutex, broadcast, mpsc}; use tokio_util::sync::CancellationToken; @@ -234,6 +235,32 @@ impl ChatHub { self.session_mgr.get_or_create_handler(session_id).await } + /// Persist an uploaded file for `source_id` into the owner's home + /// (`~/uploads/{session}/`) and return its [`Attachment`]. The single entry + /// point every surface (web handler, channel plugins) routes through, so + /// uploads can't drift on placement or on the recorded agent path — see + /// [`crate::uploads::save_to_home`]. Resolves the source's active session (so + /// the upload shares the directory the following message references). + pub async fn save_upload( + &self, + source_id: &str, + file_name: &str, + client_mime: Option, + bytes: &[u8], + ) -> anyhow::Result { + let handler = self.session_handler(source_id).await?; + let fs = handler.user_fs(); + let att = crate::uploads::save_to_home( + &fs, + handler.session_id, + file_name, + client_mime, + bytes, + ) + .await?; + Ok(att) + } + /// Returns the handler for a specific `session_id`, creating one lazily if needed. /// Used to resolve a pending tool against the session that actually owns it, /// independent of any source's "active" session. @@ -781,6 +808,16 @@ impl ChatHubApi for ChatHub { self.send_message(source_id, prompt, opts).await } + async fn save_upload( + &self, + source_id: &str, + file_name: &str, + client_mime: Option, + bytes: &[u8], + ) -> anyhow::Result { + self.save_upload(source_id, file_name, client_mime, bytes).await + } + async fn clear(&self, source_id: &str) -> anyhow::Result { self.clear(source_id).await } diff --git a/crates/skald-core/src/lib.rs b/crates/skald-core/src/lib.rs index 6c20a0f..044e754 100644 --- a/crates/skald-core/src/lib.rs +++ b/crates/skald-core/src/lib.rs @@ -48,4 +48,5 @@ pub mod tool_discovery; pub mod tools; pub mod transcribe; pub mod tts; +pub mod uploads; pub mod users; diff --git a/crates/skald-core/src/session/handler/media.rs b/crates/skald-core/src/session/handler/media.rs index 7894614..77480fd 100644 --- a/crates/skald-core/src/session/handler/media.rs +++ b/crates/skald-core/src/session/handler/media.rs @@ -10,8 +10,9 @@ //! Promotion is deliberately strict: an attachment is inlined only when ALL of //! these hold — //! - the model has the modality's capability; -//! - the file lives under `data/uploads/`, canonicalized (attachments saved -//! anywhere else, e.g. by the Telegram plugin, stay textual); +//! - the file lives under the caller's `~/uploads/` (where the upload handler +//! saves it), resolved through their per-user filesystem — attachments stored +//! anywhere else stay textual; //! - the sniffed magic bytes match an allowed MIME — the client-supplied //! `mimetype` is never trusted; //! - the per-file and per-turn byte/count budgets are not exhausted. @@ -26,7 +27,7 @@ use tracing::debug; use core_api::message_meta::Attachment; use core_api::tool::MediaRef; -use core_api::user_fs::UserFs; +use core_api::user_fs::{UserFs, UPLOADS_SUBDIR}; /// Max media parts inlined per turn. const MAX_MEDIA_PER_TURN: usize = 4; @@ -108,22 +109,21 @@ pub struct MediaPartition { } /// Splits a message's attachments into inline media parts and leftovers. -/// Files are resolved against the process working directory. -pub async fn partition(attachments: &[Attachment], capabilities: &[String]) -> MediaPartition { - let base = std::env::current_dir().unwrap_or_default(); - partition_under(attachments, capabilities, &base).await -} - -/// [`partition`] with an explicit base directory (tests). -pub async fn partition_under( +/// +/// Each attachment path is resolved through the caller's per-user [`UserFs`] — +/// the same resolver the fs-tools use, fail-closed on traversal / workspace +/// escape — and inlined only when it lands under their `~/uploads/` directory, +/// where the upload handler saves them. Attachments stored anywhere else (a +/// path outside the home, or another surface's directory) stay textual. +pub async fn partition( attachments: &[Attachment], capabilities: &[String], - base: &Path, + fs: &UserFs, ) -> MediaPartition { let capable = MODALITIES .iter() .any(|m| capabilities.iter().any(|c| c == m.capability)); - let root = std::fs::canonicalize(base.join("data").join("uploads")).ok(); + let root = std::fs::canonicalize(fs.home_host.join(UPLOADS_SUBDIR)).ok(); if !capable || root.is_none() { return MediaPartition { parts: Vec::new(), rest: attachments.to_vec() }; } @@ -138,7 +138,7 @@ pub async fn partition_under( rest.push(a.clone()); continue; } - match try_inline(a, capabilities, base, &root, total).await { + match try_inline(a, capabilities, fs, &root, total).await { Some((part, bytes)) => { total += bytes; parts.push(part); @@ -150,16 +150,17 @@ pub async fn partition_under( } /// Promotes one uploaded attachment to a content part, or `None` when any check -/// fails (logged at debug level; the caller keeps it on the textual path). -/// Containment is against the uploads `root`; the rest is [`promote`]. +/// fails (logged at debug level; the caller keeps it on the textual path). The +/// agent path is resolved through the per-user filesystem (fail-closed) and then +/// re-checked to land under the uploads `root`; the rest is [`promote`]. async fn try_inline( a: &Attachment, capabilities: &[String], - base: &Path, + fs: &UserFs, root: &Path, used_total: u64, ) -> Option<(Value, u64)> { - let abs = tokio::fs::canonicalize(base.join(&a.path)).await.ok()?; + let abs = crate::tools::fs::resolve_host_path(fs, &a.path).ok()?; if !abs.starts_with(root) { debug!(path = %a.path, "media not inlined: outside the uploads root"); return None; @@ -205,7 +206,7 @@ async fn promote( } /// Inline media a tool produced (e.g. `read_file` on an image) as content parts, -/// for the current turn only. Mirrors [`partition_under`] but contains against the +/// for the current turn only. Mirrors [`partition`] but contains against the /// caller's **workspace roots** (home + shared + projects + docs) rather than the /// uploads dir — the tool already resolved + contained the path, so this is a /// fail-closed re-check against a symlink swap since the read (§6). Same per-file, @@ -395,11 +396,13 @@ mod tests { #[tokio::test] async fn partition_inlines_png_for_vision_model() { let tmp = std::env::temp_dir().join(format!("skald-media-{}", uuid::Uuid::new_v4())); - let dir = tmp.join("data/uploads/u/1"); + let home = tmp.join("homes/u1"); + let dir = home.join("uploads/1"); tokio::fs::create_dir_all(&dir).await.unwrap(); tokio::fs::write(dir.join("a.png"), png_bytes()).await.unwrap(); + let fs = fs_home(&home); - let p = partition_under(&[att("data/uploads/u/1/a.png")], &caps(&["vision"]), &tmp).await; + let p = partition(&[att("uploads/1/a.png")], &caps(&["vision"]), &fs).await; assert!(p.rest.is_empty()); assert_eq!(p.parts.len(), 1); let url = p.parts[0]["image_url"]["url"].as_str().unwrap(); @@ -411,27 +414,30 @@ mod tests { #[tokio::test] async fn partition_gates_on_capability_and_containment() { let tmp = std::env::temp_dir().join(format!("skald-media-{}", uuid::Uuid::new_v4())); - let dir = tmp.join("data/uploads/u/1"); + let home = tmp.join("homes/u1"); + let dir = home.join("uploads/1"); tokio::fs::create_dir_all(&dir).await.unwrap(); tokio::fs::write(dir.join("a.png"), png_bytes()).await.unwrap(); - tokio::fs::write(tmp.join("secret.png"), png_bytes()).await.unwrap(); + // A real image inside the home but OUTSIDE the uploads dir. + tokio::fs::write(home.join("secret.png"), png_bytes()).await.unwrap(); + let fs = fs_home(&home); // No capability → everything stays textual. - let p = partition_under(&[att("data/uploads/u/1/a.png")], &caps(&[]), &tmp).await; + let p = partition(&[att("uploads/1/a.png")], &caps(&[]), &fs).await; assert_eq!(p.rest.len(), 1); assert!(p.parts.is_empty()); // vision capability does not unlock video parts. - let p = partition_under(&[att("data/uploads/u/1/a.png")], &caps(&["video"]), &tmp).await; + let p = partition(&[att("uploads/1/a.png")], &caps(&["video"]), &fs).await; assert_eq!(p.rest.len(), 1); - // A real image outside the uploads root is never read inline. - let p = partition_under(&[att("secret.png")], &caps(&["vision"]), &tmp).await; + // A real image in the home but outside the uploads dir is never inlined. + let p = partition(&[att("secret.png")], &caps(&["vision"]), &fs).await; assert_eq!(p.rest.len(), 1); assert!(p.parts.is_empty()); - // Traversal out of the root is rejected. - let p = partition_under(&[att("data/uploads/../../secret.png")], &caps(&["vision"]), &tmp).await; + // Traversal out of the workspace is rejected fail-closed. + let p = partition(&[att("uploads/../../secret.png")], &caps(&["vision"]), &fs).await; assert_eq!(p.rest.len(), 1); assert!(p.parts.is_empty()); @@ -441,15 +447,16 @@ mod tests { #[tokio::test] async fn partition_enforces_count_budget() { let tmp = std::env::temp_dir().join(format!("skald-media-{}", uuid::Uuid::new_v4())); - let dir = tmp.join("data/uploads/u/1"); + let home = tmp.join("homes/u1"); + let dir = home.join("uploads/1"); tokio::fs::create_dir_all(&dir).await.unwrap(); let mut atts = Vec::new(); for i in 0..(MAX_MEDIA_PER_TURN + 2) { - let rel = format!("data/uploads/u/1/{i}.png"); tokio::fs::write(dir.join(format!("{i}.png")), png_bytes()).await.unwrap(); - atts.push(att(&rel)); + atts.push(att(&format!("uploads/1/{i}.png"))); } - let p = partition_under(&atts, &caps(&["vision"]), &tmp).await; + let fs = fs_home(&home); + let p = partition(&atts, &caps(&["vision"]), &fs).await; assert_eq!(p.parts.len(), MAX_MEDIA_PER_TURN); assert_eq!(p.rest.len(), 2); @@ -465,12 +472,14 @@ mod tests { #[tokio::test] async fn partition_inlines_pdf_as_file_part_for_document_model() { let tmp = std::env::temp_dir().join(format!("skald-media-{}", uuid::Uuid::new_v4())); - let dir = tmp.join("data/uploads/u/1"); + let home = tmp.join("homes/u1"); + let dir = home.join("uploads/1"); tokio::fs::create_dir_all(&dir).await.unwrap(); tokio::fs::write(dir.join("a.pdf"), pdf_bytes()).await.unwrap(); + let fs = fs_home(&home); // A document-capable model inlines the PDF as the OpenAI `file` part shape. - let p = partition_under(&[att("data/uploads/u/1/a.pdf")], &caps(&["document"]), &tmp).await; + let p = partition(&[att("uploads/1/a.pdf")], &caps(&["document"]), &fs).await; assert!(p.rest.is_empty()); assert_eq!(p.parts.len(), 1); assert_eq!(p.parts[0]["type"], "file"); @@ -479,7 +488,7 @@ mod tests { assert!(fd.starts_with("data:application/pdf;base64,"), "{fd}"); // vision alone does not unlock PDFs. - let p = partition_under(&[att("data/uploads/u/1/a.pdf")], &caps(&["vision"]), &tmp).await; + let p = partition(&[att("uploads/1/a.pdf")], &caps(&["vision"]), &fs).await; assert_eq!(p.rest.len(), 1); assert!(p.parts.is_empty()); diff --git a/crates/skald-core/src/session/handler/message_builder.rs b/crates/skald-core/src/session/handler/message_builder.rs index 0667c49..1952a28 100644 --- a/crates/skald-core/src/session/handler/message_builder.rs +++ b/crates/skald-core/src/session/handler/message_builder.rs @@ -258,8 +258,14 @@ impl MessageBuilder { // textual path block, generated on the fly and never // persisted as content. let (text, media) = match &entry.metadata { - Some(meta) if !meta.attachments.is_empty() && idx >= media_turn_start => { - let partition = super::media::partition(&meta.attachments, capabilities).await; + Some(meta) + if !meta.attachments.is_empty() + && idx >= media_turn_start + && self.fs.is_some() => + { + let fs = self.fs.as_deref().expect("guarded by is_some()"); + let partition = + super::media::partition(&meta.attachments, capabilities, fs).await; ( format!( "{}{}", diff --git a/crates/skald-core/src/session/handler/mod.rs b/crates/skald-core/src/session/handler/mod.rs index 829c274..59b2e18 100644 --- a/crates/skald-core/src/session/handler/mod.rs +++ b/crates/skald-core/src/session/handler/mod.rs @@ -20,7 +20,7 @@ use crate::config::DatetimeConfig; use crate::db::{chat_history, chat_sessions_stack}; use crate::events::ServerEvent; use core_api::message_meta::MessageMetadata; -use core_api::user_fs::SharedFs; +use core_api::user_fs::{SharedFs, UserFs}; use crate::llm::LlmManager; use crate::mcp::McpProvider; use crate::image_generate::ImageGeneratorManager; @@ -414,6 +414,13 @@ impl ChatSessionHandler { } } + /// The caller's current filesystem snapshot (home + shared folders + projects + /// + docs). Cheap — clones an `Arc`. Used by upload persistence to place files + /// in the owner's home. + pub fn user_fs(&self) -> Arc { + self.fs.load() + } + /// Override the session used for scratchpad reads/writes. /// Called by the cron runner for async tasks so they share the parent's scratchpad. pub fn set_scratchpad_session_id(&self, id: i64) { diff --git a/crates/skald-core/src/uploads.rs b/crates/skald-core/src/uploads.rs new file mode 100644 index 0000000..910b9c2 --- /dev/null +++ b/crates/skald-core/src/uploads.rs @@ -0,0 +1,154 @@ +//! Centralized upload persistence. +//! +//! The single place any surface — the web `POST /uploads` handler, a channel +//! plugin like Telegram, or a future one — turns received file bytes into a +//! saved [`Attachment`]. Keeping placement + naming + metadata here means no two +//! callers can drift on *where* an upload lands or *what* path is recorded (the +//! bug this fixes: uploads that the agent then couldn't reach). +//! +//! Files are written into the user's private container home under +//! `uploads/{session_id}/…`. That single agent path is resolved identically by +//! every consumer — the fs-tools, `execute_cmd` (the home is bind-mounted at +//! `/root`), the file viewer (`/api/file`) and the media inliner — through the +//! same per-user [`UserFs`]. + +use std::path::{Path, PathBuf}; + +use core_api::message_meta::Attachment; +use core_api::user_fs::{UserFs, UPLOADS_SUBDIR}; + +use crate::session::handler::media::sniff_mime; + +/// Persist `bytes` as a file named `file_name` into the user's +/// `~/uploads/{session_id}/`, returning the resulting [`Attachment`] whose `path` +/// is the home-relative agent path. The recognized magic-byte MIME wins over the +/// caller-claimed `client_mime`. +/// +/// The caller owns byte transport (streaming a multipart body, downloading from +/// an API) and any size cap; this owns placement, collision-safe naming, MIME +/// sniffing and the metadata shape. +pub async fn save_to_home( + fs: &UserFs, + session_id: i64, + file_name: &str, + client_mime: Option, + bytes: &[u8], +) -> std::io::Result { + let dir_host = fs + .home_host + .join(UPLOADS_SUBDIR) + .join(session_id.to_string()); + tokio::fs::create_dir_all(&dir_host).await?; + + let (abs_path, final_name) = unique_target(&dir_host, &sanitize_filename(file_name)); + tokio::fs::write(&abs_path, bytes).await?; + + // The sniffed type wins over the client claim when we recognize the bytes. + let mimetype = sniff_mime(&bytes[..bytes.len().min(16)]) + .map(str::to_string) + .or(client_mime); + + Ok(Attachment { + path: format!("{UPLOADS_SUBDIR}/{session_id}/{final_name}"), + name: final_name, + mimetype, + filesize: Some(bytes.len() as u64), + }) +} + +/// Reduces an arbitrary client filename to a safe basename: directory components +/// are dropped and an empty/`.`/`..` result falls back to `"file"`. +fn sanitize_filename(raw: &str) -> String { + let base = Path::new(raw) + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or("") + .trim(); + if base.is_empty() || base == "." || base == ".." { + "file".to_string() + } else { + base.to_string() + } +} + +/// Returns a non-colliding `(absolute_path, final_name)` inside `dir`. If `name` +/// already exists, inserts `_1`, `_2`, … before the extension. +fn unique_target(dir: &Path, name: &str) -> (PathBuf, String) { + let candidate = dir.join(name); + if !candidate.exists() { + return (candidate, name.to_string()); + } + let path = Path::new(name); + let stem = path.file_stem().and_then(|s| s.to_str()).unwrap_or(name); + let ext = path.extension().and_then(|s| s.to_str()); + for n in 1.. { + let next = match ext { + Some(ext) => format!("{stem}_{n}.{ext}"), + None => format!("{stem}_{n}"), + }; + let candidate = dir.join(&next); + if !candidate.exists() { + return (candidate, next); + } + } + unreachable!("unique_target loop always returns") +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A throwaway [`UserFs`] whose private home is `home`. + fn fs_home(home: &Path) -> UserFs { + UserFs::new( + "u1", + home.to_path_buf(), + "skald-u1", + PathBuf::from("/root"), + vec![], + vec![], + None, + ) + } + + fn pdf_bytes() -> Vec { + let mut v = b"%PDF-1.7\n".to_vec(); + v.extend_from_slice(&[0x00; 32]); + v + } + + #[tokio::test] + async fn saves_into_home_uploads_with_agent_path_and_sniffs_mime() { + let tmp = std::env::temp_dir().join(format!("skald-upload-{}", uuid::Uuid::new_v4())); + let home = tmp.join("homes/u1"); + tokio::fs::create_dir_all(&home).await.unwrap(); + let fs = fs_home(&home); + + // A wrong client MIME is overridden by the sniffed PDF signature. + let att = save_to_home(&fs, 7, "cv.pdf", Some("application/octet-stream".into()), &pdf_bytes()) + .await + .unwrap(); + + assert_eq!(att.path, "uploads/7/cv.pdf"); + assert_eq!(att.name, "cv.pdf"); + assert_eq!(att.mimetype.as_deref(), Some("application/pdf")); + assert_eq!(att.filesize, Some(pdf_bytes().len() as u64)); + // Physically lands under the home's uploads dir (reachable by the agent). + assert!(home.join("uploads/7/cv.pdf").exists()); + + // A second upload of the same name never overwrites — it is de-duped. + let att2 = save_to_home(&fs, 7, "cv.pdf", None, &pdf_bytes()).await.unwrap(); + assert_eq!(att2.path, "uploads/7/cv_1.pdf"); + assert!(home.join("uploads/7/cv_1.pdf").exists()); + + let _ = tokio::fs::remove_dir_all(&tmp).await; + } + + #[test] + fn sanitize_strips_directory_components() { + assert_eq!(sanitize_filename("../../etc/passwd"), "passwd"); + assert_eq!(sanitize_filename("a/b/c.txt"), "c.txt"); + assert_eq!(sanitize_filename(".."), "file"); + assert_eq!(sanitize_filename(""), "file"); + } +} diff --git a/src/frontend/api/uploads.rs b/src/frontend/api/uploads.rs index fced831..f75a42d 100644 --- a/src/frontend/api/uploads.rs +++ b/src/frontend/api/uploads.rs @@ -1,36 +1,36 @@ -use std::path::{Path as StdPath, PathBuf}; use std::sync::Arc; use axum::{ Json, Extension, extract::{Multipart, Path, State}, }; -use tokio::io::AsyncWriteExt; use core_api::message_meta::Attachment; -use skald_core::session::handler::media::sniff_mime; use skald_core::skald::Skald; -use skald_core::tools::fs as fs_tools; use super::{ApiError, guard::AuthUser, require_context}; use super::sessions::SourcePath; -/// Max bytes accepted for a single uploaded file; anything larger is cut off -/// mid-stream, the partial file removed, and the request answered 413. +/// Max bytes accepted for a single uploaded file; anything larger is refused 413. const MAX_UPLOAD_BYTES: u64 = 256 * 1024 * 1024; /// `POST /api/{source}/uploads` /// -/// Accepts a `multipart/form-data` body with one or more file fields and saves -/// each under `data/uploads/{user_id}/{session_id}/` (per-user namespaced, so -/// colliding session ids across users never share a directory). Bytes are -/// streamed straight to disk (`field.chunk()` → file), never buffered whole in -/// RAM — the route disables the default body-size limit (see router) and -/// enforces [`MAX_UPLOAD_BYTES`] itself. When the magic bytes are recognized, -/// the sniffed MIME wins over the client-supplied `Content-Type`. +/// Accepts a `multipart/form-data` body with one or more file fields and persists +/// each through the shared upload seam ([`skald_core::chat_hub::ChatHub::save_upload`]), +/// which saves into the caller's container home under `uploads/{session_id}/` — +/// so a single agent path (`uploads/{session_id}/…`) is reachable by the fs-tools, +/// by `execute_cmd` (the home is bind-mounted at `/root`), and by the file viewer +/// alike. The web handler and every channel plugin go through that one seam, so no +/// two surfaces can drift on *where* an upload lands. /// -/// Returns the saved [`Attachment`]s (project-root-relative path, name, MIME, -/// size) so the client can show chips and echo them back when sending the message. +/// Each field is read with the [`MAX_UPLOAD_BYTES`] cap enforced during accumulation +/// (an over-cap field is refused before anything is written); the route disables the +/// default body-size limit (see router). The seam sniffs the magic bytes and prefers +/// them over the client-supplied `Content-Type`. +/// +/// Returns the saved [`Attachment`]s (home-relative agent path, name, MIME, size) so +/// the client can show chips and echo them back when sending the message. pub async fn upload( State(skald): State>, Extension(auth): Extension, @@ -38,13 +38,6 @@ pub async fn upload( mut multipart: Multipart, ) -> Result>, ApiError> { let ctx = require_context(&skald, &auth.user_id).await?; - // Resolve (creating if needed) the source's session so uploads land in the - // directory the message will reference. - let session_id = ctx.chat_hub.session_handler(&p.source).await?.session_id; - - let dir_rel = format!("data/uploads/{}/{session_id}", auth.user_id); - let dir_abs = fs_tools::resolve(&dir_rel)?; - tokio::fs::create_dir_all(&dir_abs).await?; let mut saved: Vec = Vec::new(); @@ -53,93 +46,26 @@ pub async fn upload( { // Only fields carrying a filename are file uploads; skip plain text fields. let Some(orig_name) = field.file_name().map(str::to_string) else { continue }; - let mimetype = field.content_type().map(str::to_string); + let client_mime = field.content_type().map(str::to_string); - let base_name = sanitize_filename(&orig_name); - let (abs_path, final_name) = unique_target(&dir_abs, &base_name); - - let mut file = tokio::fs::File::create(&abs_path).await - .map_err(|e| ApiError::from(anyhow::anyhow!("cannot create {}: {e}", abs_path.display())))?; - - let mut size: u64 = 0; - let mut too_large = false; + // Buffer the field, enforcing the size cap as we read so an over-limit + // upload is refused before any bytes are handed to the store. + let mut bytes: Vec = Vec::new(); while let Some(chunk) = field.chunk().await .map_err(|e| ApiError::bad_request(format!("upload read error: {e}")))? { - size += chunk.len() as u64; - if size > MAX_UPLOAD_BYTES { - too_large = true; - break; + if bytes.len() as u64 + chunk.len() as u64 > MAX_UPLOAD_BYTES { + return Err(ApiError::payload_too_large(format!( + "'{orig_name}' exceeds the {} MiB upload limit", + MAX_UPLOAD_BYTES / 1024 / 1024 + ))); } - file.write_all(&chunk).await?; - } - file.flush().await?; - drop(file); - - if too_large { - let _ = tokio::fs::remove_file(&abs_path).await; - return Err(ApiError::payload_too_large(format!( - "'{final_name}' exceeds the {} MiB upload limit", - MAX_UPLOAD_BYTES / 1024 / 1024 - ))); + bytes.extend_from_slice(&chunk); } - // The sniffed type wins over the client claim when we recognize the bytes. - let mimetype = sniff_head(&abs_path).await.map(String::from).or(mimetype); - - saved.push(Attachment { - path: format!("{dir_rel}/{final_name}"), - name: final_name, - mimetype, - filesize: Some(size), - }); + let att = ctx.chat_hub.save_upload(&p.source, &orig_name, client_mime, &bytes).await?; + saved.push(att); } Ok(Json(saved)) } - -/// Reads the first bytes of a saved upload and sniffs its real media type. -async fn sniff_head(path: &StdPath) -> Option<&'static str> { - let mut file = tokio::fs::File::open(path).await.ok()?; - let mut head = [0u8; 16]; - let n = tokio::io::AsyncReadExt::read(&mut file, &mut head).await.ok()?; - sniff_mime(&head[..n]) -} - -/// Reduces an arbitrary client filename to a safe basename: directory components -/// are dropped and an empty/`.`/`..` result falls back to `"file"`. -fn sanitize_filename(raw: &str) -> String { - let base = StdPath::new(raw) - .file_name() - .and_then(|n| n.to_str()) - .unwrap_or("") - .trim(); - if base.is_empty() || base == "." || base == ".." { - "file".to_string() - } else { - base.to_string() - } -} - -/// Returns a non-colliding `(absolute_path, final_name)` inside `dir`. If `name` -/// already exists, inserts `_1`, `_2`, … before the extension. -fn unique_target(dir: &StdPath, name: &str) -> (PathBuf, String) { - let candidate = dir.join(name); - if !candidate.exists() { - return (candidate, name.to_string()); - } - let path = StdPath::new(name); - let stem = path.file_stem().and_then(|s| s.to_str()).unwrap_or(name); - let ext = path.extension().and_then(|s| s.to_str()); - for n in 1.. { - let next = match ext { - Some(ext) => format!("{stem}_{n}.{ext}"), - None => format!("{stem}_{n}"), - }; - let candidate = dir.join(&next); - if !candidate.exists() { - return (candidate, next); - } - } - unreachable!("unique_target loop always returns") -} diff --git a/src/frontend/server.rs b/src/frontend/server.rs index 920e74a..2bf0af0 100644 --- a/src/frontend/server.rs +++ b/src/frontend/server.rs @@ -1,4 +1,3 @@ -use std::path::Path; use std::sync::Arc; use anyhow::{Context, Result}; @@ -78,7 +77,6 @@ impl WebServer { Arc::clone(&skald), api::guard::require_auth, )); - let skald_for_data = Arc::clone(&skald); // Resolve the app state first so the resulting `Router<()>` can host the // stateless plugin routers via `nest`. @@ -108,29 +106,21 @@ impl WebServer { )); router = router.nest(&format!("/api/plugin/{id}"), gated); } - // Serve the data/ directory under /data/ (accessible via URL), behind the - // same session-cookie gate as /api — uploads are private user content. - let data_dir = Path::new(static_dir).parent().unwrap_or(Path::new(".")).join("data"); - // Static responses (SPA assets + /data) get `Cache-Control: no-cache`: - // the browser may store them but MUST revalidate before use, so after a - // self-rewrite/restart the client never serves a stale asset (no heuristic + // User files are never served as static content: chat uploads live in the + // caller's container home and are fetched, per-user and access-checked, + // through `/api/file`. (The former `/data` static mount was removed — it + // was gated by `require_auth` only, not ownership, so it also exposed + // internal server state under `data/`.) + // + // Static responses (the SPA assets) get `Cache-Control: no-cache`: the + // browser may store them but MUST revalidate before use, so after a + // rebuild/restart the client never serves a stale asset (no heuristic // caching). Revalidation yields cheap 304s (the body is already on disk). // `/api` is deliberately left without this header (dynamic, not cached). let static_assets = || ServiceBuilder::new().layer(SetResponseHeaderLayer::overriding( header::CACHE_CONTROL, HeaderValue::from_static("no-cache"), )); - let data_service = ServiceBuilder::new() - .layer(axum::middleware::from_fn_with_state( - skald_for_data, - api::guard::require_auth, - )) - .layer(SetResponseHeaderLayer::overriding( - header::CACHE_CONTROL, - HeaderValue::from_static("no-cache"), - )) - .service(ServeDir::new(&data_dir)); - router = router.nest_service("/data", data_service); router = router.fallback_service(static_assets().service(ServeDir::new(static_dir))); // Negotiated gzip/brotli compression (Accept-Encoding). Matters most for // the mobile WebView, whose HTTP traffic is reverse-proxied byte-for-byte