From e58d063baea2b24d79c256d91ecec83eb318d8a3 Mon Sep 17 00:00:00 2001 From: dsociative Date: Thu, 27 Aug 2026 16:21:31 +0300 Subject: [PATCH 1/3] fix(client): download newer-storage attachments through their signed storage url MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Observed live on a card with a `type` 11 attachment: the authenticated API path `/api/v1/cards/{card_uid}/files/{id}` does not redirect to storage — it answers 200 with the file's JSON metadata, whose `url` is a signed storage link valid for about fifteen seconds. `Files::download` saved that metadata as the file. It now recognises a JSON answer on the API origin, decodes the metadata and immediately fetches the signed url without credentials (the token still goes only to the API origin); any other answer is returned as the bytes it is, so a redirecting instance keeps working. `get_bytes` reports the content type for this. Fixtures are replaced by the live (sanitized) shapes: the card `files[]` entry and the metadata answer. Client, CLI and MCP tests drive the two-step protocol; the two client tests were written first and failed against the previous code. --- crates/kaiten-client/src/api/files.rs | 53 ++++++++++++--- crates/kaiten-client/src/client.rs | 12 +++- crates/kaiten-client/tests/files_test.rs | 65 +++++++++++++++++-- .../tests/fixtures/card_get_files_type11.json | 28 ++++---- .../tests/fixtures/file_metadata_type11.json | 13 ++++ crates/kaiten/src/mcp/mod.rs | 16 ++++- crates/kaiten/tests/card_file_test.rs | 17 +++-- .../tests/fixtures/card_with_files.json | 20 +++--- .../card_file_test__card_file_list.snap | 2 +- 9 files changed, 179 insertions(+), 47 deletions(-) create mode 100644 crates/kaiten-client/tests/fixtures/file_metadata_type11.json diff --git a/crates/kaiten-client/src/api/files.rs b/crates/kaiten-client/src/api/files.rs index 77afd47..d4efa82 100644 --- a/crates/kaiten-client/src/api/files.rs +++ b/crates/kaiten-client/src/api/files.rs @@ -8,9 +8,10 @@ use crate::models::{CardFile, FileRef}; /// /// SECURITY: Kaiten's classic storage serves uploaded files from a public /// (unguessable) URL without authentication — never attach secrets. The -/// newer storage serves files through an authenticated API path that -/// redirects to storage; [`Files::download`] sends the API token only to -/// the API origin, never to a file or storage host. +/// newer storage answers its authenticated API path with the file's +/// metadata, whose `url` is a signed storage link valid for seconds; +/// [`Files::download`] sends the API token only to the API origin and +/// fetches the signed link without credentials. pub struct Files<'a> { pub(crate) client: &'a KaitenClient, } @@ -24,10 +25,13 @@ impl Files<'_> { } /// Download an attachment's content. An absolute `url` (classic storage) - /// is fetched as is; a host-root-relative one (newer storage) is resolved - /// against the API origin. The token goes only to the API origin — never - /// to the public file host, nor to a storage host reached through a - /// redirect. The whole body is held in memory, as with [`Files::attach`]. + /// is fetched as is. A host-root-relative one (newer storage) is resolved + /// against the API origin, where the API answers with the file's + /// metadata rather than its bytes: the `url` in it is a signed storage + /// link that expires within seconds, fetched immediately and without + /// credentials. The token goes only to the API origin — never to the + /// public file host nor to storage. The whole body is held in memory, as + /// with [`Files::attach`]. pub async fn download(&self, file: &CardFile) -> Result> { let raw = file.url.as_deref().ok_or_else(|| { invalid_input(format!( @@ -37,7 +41,32 @@ impl Files<'_> { )) })?; let url = resolve_file_url(self.client.base_url(), raw)?; - self.client.get_bytes(&url).await + let (content_type, bytes) = self.client.get_bytes(&url).await?; + let is_metadata = url.origin() == self.client.base_url().origin() + && content_type + .as_deref() + .is_some_and(|ct| ct.starts_with("application/json")); + if !is_metadata { + return Ok(bytes); + } + let text = String::from_utf8(bytes).map_err(|e| { + invalid_input(format!( + "file metadata for `{}` is not UTF-8: {e}", + file.name + )) + })?; + let location: FileLocation = KaitenClient::decode(&text)?; + let signed = location.url.ok_or_else(|| { + invalid_input(format!( + "file metadata for `{}` ({}) has no download url", + file.name, + FileRef::from(file) + )) + })?; + let signed = url::Url::parse(&signed) + .map_err(|e| invalid_input(format!("storage url `{signed}` is not valid: {e}")))?; + let (_, bytes) = self.client.get_bytes(&signed).await?; + Ok(bytes) } /// [`Files::download`] straight into `path` (created or truncated; @@ -79,6 +108,14 @@ impl Files<'_> { } } +/// What the newer storage's API path returns for a file: its metadata, of +/// which only the signed storage `url` matters here. +#[derive(serde::Deserialize)] +struct FileLocation { + #[serde(default)] + url: Option, +} + /// Absolute URLs pass through; a path is resolved against the API **origin** /// (`/api/v1/...` lives next to, not under, the `/api/latest` base) and must /// stay there — WHATWG parsing would otherwise let `/\host/x` wander off to diff --git a/crates/kaiten-client/src/client.rs b/crates/kaiten-client/src/client.rs index c5ebf6d..9537eac 100644 --- a/crates/kaiten-client/src/client.rs +++ b/crates/kaiten-client/src/client.rs @@ -252,7 +252,8 @@ impl KaitenClient { } } - /// GET `url` and return the body bytes (attachment downloads). + /// GET `url` and return the content type and body bytes (attachment + /// downloads). /// /// The bearer token is sent only when `url` is on the API origin — never /// to the public file host, nor to a storage host reached through a @@ -261,7 +262,7 @@ impl KaitenClient { /// downgrade would keep it, which only Kaiten itself could trigger). /// Retry and tracing mirror `send_with_retry`; the body is binary and /// never traced; errors go through `download_error`. - pub(crate) async fn get_bytes(&self, url: &url::Url) -> Result> { + pub(crate) async fn get_bytes(&self, url: &url::Url) -> Result<(Option, Vec)> { let with_auth = url.origin() == self.base_url.origin(); let mut retries = 0u32; loop { @@ -300,7 +301,12 @@ impl KaitenClient { let text = resp.text().await?; return Err(download_error(status, text)); } - return Ok(resp.bytes().await?.to_vec()); + let content_type = resp + .headers() + .get(reqwest::header::CONTENT_TYPE) + .and_then(|v| v.to_str().ok()) + .map(str::to_owned); + return Ok((content_type, resp.bytes().await?.to_vec())); } } diff --git a/crates/kaiten-client/tests/files_test.rs b/crates/kaiten-client/tests/files_test.rs index 2664c27..de657eb 100644 --- a/crates/kaiten-client/tests/files_test.rs +++ b/crates/kaiten-client/tests/files_test.rs @@ -92,10 +92,12 @@ async fn attach_missing_local_file_is_io_error_without_any_request() { // --------------------------------------------------------------------------- const CARD_GET_FULL: &str = include_str!("fixtures/card_get_full.json"); -/// Shape B as reported in issue #12 from another Kaiten instance (`type` 11, -/// UUID `id`, string `size`, host-root-relative `url`); not yet reproduced on -/// the test account — replace with a live capture when one is available. +/// Shape B, captured live (sanitized) from a card on the newer storage +/// (`type` 11): UUID `id`, string `size`, host-root-relative `url`. const CARD_GET_FILES_TYPE11: &str = include_str!("fixtures/card_get_files_type11.json"); +/// What the newer storage's API path answers (captured live, sanitized): the +/// file's metadata with a short-lived signed storage `url` — not the bytes. +const FILE_METADATA_TYPE11: &str = include_str!("fixtures/file_metadata_type11.json"); /// Matches a request that carries NO `Authorization` header. struct NoAuthHeader; @@ -139,7 +141,7 @@ async fn list_takes_files_from_card_get_not_from_files_endpoint() { async fn list_parses_uuid_id_and_string_size_from_newer_storage() { let server = MockServer::start().await; Mock::given(method("GET")) - .and(path("/cards/12345678")) + .and(path("/cards/64533247")) .respond_with( ResponseTemplate::new(200).set_body_raw(CARD_GET_FILES_TYPE11, "application/json"), ) @@ -147,10 +149,11 @@ async fn list_parses_uuid_id_and_string_size_from_newer_storage() { .await; let client = KaitenClient::new(&server.uri(), "test-token").unwrap(); - let files = client.files().list(12_345_678).await.unwrap(); + let files = client.files().list(64_533_247).await.unwrap(); assert_eq!(files.len(), 1); assert_eq!(files[0].id, 0); - assert_eq!(files[0].size, Some(58_818)); + assert_eq!(files[0].size, Some(33_055)); + assert_eq!(files[0].file_type, Some(11)); assert!( files[0] .url @@ -160,7 +163,7 @@ async fn list_parses_uuid_id_and_string_size_from_newer_storage() { ); assert_eq!( kaiten_client::FileRef::from(&files[0]), - kaiten_client::FileRef::Uid("6a8e66af-0000-0000-0000-000000000000".into()) + kaiten_client::FileRef::Uid("08b5876d-0000-0000-0000-000000000000".into()) ); } @@ -332,3 +335,51 @@ async fn download_html_error_page_is_not_dumped_verbatim() { other => panic!("unexpected {other:?}"), } } + +/// Newer storage, as observed live: `GET /api/v1/cards/{card_uid}/files/{id}` +/// (bearer) answers 200 with JSON metadata whose `url` is a short-lived signed +/// storage link; the bytes are fetched from there, without credentials. +#[tokio::test] +async fn download_newer_storage_follows_the_signed_url_from_the_metadata() { + let api = MockServer::start().await; + let storage = MockServer::start().await; + let meta = FILE_METADATA_TYPE11.replace( + "https://storage.example/bucket", + &format!("{}/bucket", storage.uri()), + ); + Mock::given(method("GET")) + .and(path("/api/v1/cards/14dc3064-0000-0000-0000-000000000000/files/08b5876d-0000-0000-0000-000000000000")) + .and(header("Authorization", "Bearer test-token")) + .respond_with(ResponseTemplate::new(200).set_body_raw(meta, "application/json")) + .expect(1) + .mount(&api) + .await; + Mock::given(method("GET")) + .and(path("/bucket/08b5876d-0000-0000-0000-000000000000")) + .and(NoAuthHeader) + .respond_with(ResponseTemplate::new(200).set_body_raw("report", "text/html")) + .expect(1) + .mount(&storage) + .await; + + let client = KaitenClient::new(&format!("{}/api/latest", api.uri()), "test-token").unwrap(); + let card: kaiten_client::Card = serde_json::from_str(CARD_GET_FILES_TYPE11).unwrap(); + let bytes = client.files().download(&card.files[0]).await.unwrap(); + assert_eq!(bytes, b"report"); +} + +#[tokio::test] +async fn download_newer_storage_metadata_without_url_is_an_error_without_a_second_request() { + let api = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/api/v1/cards/14dc3064-0000-0000-0000-000000000000/files/08b5876d-0000-0000-0000-000000000000")) + .respond_with(ResponseTemplate::new(200).set_body_raw(r#"{"id": "x", "name": "r.html"}"#, "application/json")) + .expect(1) + .mount(&api) + .await; + + let client = KaitenClient::new(&format!("{}/api/latest", api.uri()), "test-token").unwrap(); + let card: kaiten_client::Card = serde_json::from_str(CARD_GET_FILES_TYPE11).unwrap(); + let err = client.files().download(&card.files[0]).await.unwrap_err(); + assert!(err.to_string().contains("download url"), "{err}"); +} diff --git a/crates/kaiten-client/tests/fixtures/card_get_files_type11.json b/crates/kaiten-client/tests/fixtures/card_get_files_type11.json index c73e380..6439a3f 100644 --- a/crates/kaiten-client/tests/fixtures/card_get_files_type11.json +++ b/crates/kaiten-client/tests/fixtures/card_get_files_type11.json @@ -1,24 +1,26 @@ { - "id": 12345678, - "uid": "0ca503b2-0000-0000-0000-000000000000", - "title": "report card", + "id": 64533247, + "uid": "14dc3064-0000-0000-0000-000000000000", + "title": "card with a newer-storage attachment", "board_id": 1826109, "files": [ { - "id": "6a8e66af-0000-0000-0000-000000000000", - "name": "report.xlsx", - "size": "58818", - "mime_type": "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", "author_uid": "51b4f5a0-0000-0000-0000-000000000000", - "card_uid": "0ca503b2-0000-0000-0000-000000000000", - "entity_type": "card", - "created": "2026-08-10T07:26:08.653Z", - "resizes": [], "card_cover": false, + "card_id": 64533247, + "card_uid": "14dc3064-0000-0000-0000-000000000000", + "company_uid": "cde617a8-0000-0000-0000-000000000000", + "created": "2026-05-08T15:56:33.382Z", "deleted": false, + "entity_type": "card", + "id": "08b5876d-0000-0000-0000-000000000000", + "mime_type": "text/html", + "name": "report.html", + "resizes": [], + "size": "33055", "type": 11, - "url": "/api/v1/cards/0ca503b2-0000-0000-0000-000000000000/files/6a8e66af-0000-0000-0000-000000000000", - "card_id": 12345678 + "updated": "2026-05-08T15:56:33.382Z", + "url": "/api/v1/cards/14dc3064-0000-0000-0000-000000000000/files/08b5876d-0000-0000-0000-000000000000" } ] } diff --git a/crates/kaiten-client/tests/fixtures/file_metadata_type11.json b/crates/kaiten-client/tests/fixtures/file_metadata_type11.json new file mode 100644 index 0000000..5cc81c8 --- /dev/null +++ b/crates/kaiten-client/tests/fixtures/file_metadata_type11.json @@ -0,0 +1,13 @@ +{ + "id": "08b5876d-0000-0000-0000-000000000000", + "name": "report.html", + "size": "33055", + "mime_type": "text/html", + "entity_type": "card", + "created": "2026-05-08T15:56:33.382Z", + "updated": "2026-05-08T15:56:33.382Z", + "card_uid": "14dc3064-0000-0000-0000-000000000000", + "author_uid": "51b4f5a0-0000-0000-0000-000000000000", + "card_cover": false, + "url": "https://storage.example/bucket/08b5876d-0000-0000-0000-000000000000?X-Amz-Signature=REDACTED" +} diff --git a/crates/kaiten/src/mcp/mod.rs b/crates/kaiten/src/mcp/mod.rs index c6c4024..9076a8e 100644 --- a/crates/kaiten/src/mcp/mod.rs +++ b/crates/kaiten/src/mcp/mod.rs @@ -2092,8 +2092,10 @@ mod tests { ); } + /// Newer storage: the API path (bearer) answers with metadata whose `url` + /// is a signed storage link; the bytes come from there without the token. #[tokio::test] - async fn download_file_by_uid_resolves_relative_url_on_api_host_with_bearer() { + async fn download_file_by_uid_follows_the_signed_url_from_the_metadata() { let server = MockServer::start().await; Mock::given(method("GET")) .and(path("/cards/5")) @@ -2109,6 +2111,18 @@ mod tests { "/api/v1/cards/cu/files/6a8e66af-0000-0000-0000-000000000000", )) .and(header("Authorization", "Bearer test-token")) + .respond_with(ResponseTemplate::new(200).set_body_raw( + format!( + r#"{{"id": "6a8e66af-0000-0000-0000-000000000000", "url": "{}/signed/blob"}}"#, + server.uri() + ), + "application/json", + )) + .expect(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/signed/blob")) .respond_with(ResponseTemplate::new(200).set_body_bytes(b"xls".to_vec())) .expect(1) .mount(&server) diff --git a/crates/kaiten/tests/card_file_test.rs b/crates/kaiten/tests/card_file_test.rs index 70bf8cb..7d44e0a 100644 --- a/crates/kaiten/tests/card_file_test.rs +++ b/crates/kaiten/tests/card_file_test.rs @@ -195,18 +195,25 @@ async fn card_file_get_saves_original_name_in_cwd() { ); } +/// Newer storage, as observed live: the API path (bearer) answers with the +/// file's metadata carrying a short-lived signed storage url; the bytes come +/// from there, without credentials. #[tokio::test(flavor = "multi_thread")] -async fn card_file_get_by_uid_uses_bearer_on_api_host_and_follows_redirect() { +async fn card_file_get_by_uid_fetches_the_signed_storage_url_from_the_metadata() { let api = MockServer::start().await; let storage = MockServer::start().await; mock_card(&api, &storage).await; Mock::given(method("GET")) .and(path(NEWER_PATH)) .and(header("Authorization", "Bearer test-token")) - .respond_with( - ResponseTemplate::new(302) - .insert_header("Location", format!("{}/blob.xlsx", storage.uri()).as_str()), - ) + .respond_with(ResponseTemplate::new(200).set_body_raw( + format!( + r#"{{"id": "{NEWER_UID}", "name": "report.xlsx", "size": "10", + "url": "{}/blob.xlsx?X-Amz-Signature=x"}}"#, + storage.uri() + ), + "application/json", + )) .expect(1) .mount(&api) .await; diff --git a/crates/kaiten/tests/fixtures/card_with_files.json b/crates/kaiten/tests/fixtures/card_with_files.json index 9fbc947..4dace72 100644 --- a/crates/kaiten/tests/fixtures/card_with_files.json +++ b/crates/kaiten/tests/fixtures/card_with_files.json @@ -30,20 +30,22 @@ "updated": "2026-07-16T19:00:00.000Z" }, { - "id": "6a8e66af-0000-0000-0000-000000000000", - "name": "report.xlsx", - "size": "58818", - "mime_type": "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", "author_uid": "51b4f5a0-0000-0000-0000-000000000000", + "card_cover": false, + "card_id": 67089469, "card_uid": "c78e313c-ab37-4456-9eb0-904681c4e309", + "company_uid": "cde617a8-0000-0000-0000-000000000000", + "created": "2026-05-08T15:56:33.382Z", + "deleted": false, "entity_type": "card", - "created": "2026-08-10T07:26:08.653Z", + "id": "6a8e66af-0000-0000-0000-000000000000", + "mime_type": "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + "name": "report.xlsx", "resizes": [], - "card_cover": false, - "deleted": false, + "size": "58818", "type": 11, - "url": "/api/v1/cards/c78e313c-ab37-4456-9eb0-904681c4e309/files/6a8e66af-0000-0000-0000-000000000000", - "card_id": 67089469 + "updated": "2026-05-08T15:56:33.382Z", + "url": "/api/v1/cards/c78e313c-ab37-4456-9eb0-904681c4e309/files/6a8e66af-0000-0000-0000-000000000000" } ] } diff --git a/crates/kaiten/tests/snapshots/card_file_test__card_file_list.snap b/crates/kaiten/tests/snapshots/card_file_test__card_file_list.snap index 927ae52..76a21a6 100644 --- a/crates/kaiten/tests/snapshots/card_file_test__card_file_list.snap +++ b/crates/kaiten/tests/snapshots/card_file_test__card_file_list.snap @@ -6,5 +6,5 @@ expression: "String::from_utf8(out).unwrap()" │ ID NAME SIZE MIME TYPE CREATED │ ╞══════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════╡ │ 61256602 probe-attach.txt 58 text/plain 2026-07-16 │ -│ 6a8e66af-0000-0000-0000-000000000000 report.xlsx 58818 application/vnd.openxmlformats-officedocument.spreadsheetml.sheet 2026-08-10 │ +│ 6a8e66af-0000-0000-0000-000000000000 report.xlsx 58818 application/vnd.openxmlformats-officedocument.spreadsheetml.sheet 2026-05-08 │ └──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ From f979e8f61849491761c467b8288869fa724f3639 Mon Sep 17 00:00:00 2001 From: dsociative Date: Thu, 27 Aug 2026 16:32:15 +0300 Subject: [PATCH 2/3] fix(client): treat an answer as file metadata only when the API itself served it (review) The metadata check keyed on the requested url's origin, but the content type belongs to the final response: on a redirecting instance a JSON attachment reached through a 302 would have been parsed as metadata and its `url` followed. `get_bytes` now reports the url that answered, and `Files::download` decodes metadata only when that url is the one asked for, on the API origin, with a JSON media type (compared case-insensitively, parameters ignored). A refused signed link is reported as such, naming the file. Tests: JSON attachment behind a redirect and a classic `.json` are returned verbatim, uppercase media type recognised, invalid metadata is a decode error, the MCP test fetches the signed url from a second server without the token. --- crates/kaiten-client/src/api/files.rs | 45 +++++-- crates/kaiten-client/src/client.rs | 21 ++- crates/kaiten-client/tests/files_test.rs | 155 +++++++++++++++++++++++ crates/kaiten/src/mcp/mod.rs | 15 ++- 4 files changed, 220 insertions(+), 16 deletions(-) diff --git a/crates/kaiten-client/src/api/files.rs b/crates/kaiten-client/src/api/files.rs index d4efa82..27421ad 100644 --- a/crates/kaiten-client/src/api/files.rs +++ b/crates/kaiten-client/src/api/files.rs @@ -41,15 +41,18 @@ impl Files<'_> { )) })?; let url = resolve_file_url(self.client.base_url(), raw)?; - let (content_type, bytes) = self.client.get_bytes(&url).await?; - let is_metadata = url.origin() == self.client.base_url().origin() - && content_type - .as_deref() - .is_some_and(|ct| ct.starts_with("application/json")); + let fetched = self.client.get_bytes(&url).await?; + // Metadata comes only from the API itself: the answer must be JSON and + // must have been served by the very url we asked for. A redirect means + // the API handed us over to storage, and whatever comes back — even a + // JSON attachment — is the file. + let is_metadata = fetched.url == url + && url.origin() == self.client.base_url().origin() + && is_json(fetched.content_type.as_deref()); if !is_metadata { - return Ok(bytes); + return Ok(fetched.bytes); } - let text = String::from_utf8(bytes).map_err(|e| { + let text = String::from_utf8(fetched.bytes).map_err(|e| { invalid_input(format!( "file metadata for `{}` is not UTF-8: {e}", file.name @@ -65,8 +68,24 @@ impl Files<'_> { })?; let signed = url::Url::parse(&signed) .map_err(|e| invalid_input(format!("storage url `{signed}` is not valid: {e}")))?; - let (_, bytes) = self.client.get_bytes(&signed).await?; - Ok(bytes) + // The signed link is valid for seconds; it is fetched right away, and + // the API-side 429 retries all happen before it is minted. + match self.client.get_bytes(&signed).await { + Ok(fetched) => Ok(fetched.bytes), + Err(KaitenError::Api { + status, + message, + body, + }) => Err(KaitenError::Api { + status, + message: format!( + "storage refused the signed link for `{}` (it is valid only for seconds): {message}", + file.name + ), + body, + }), + Err(other) => Err(other), + } } /// [`Files::download`] straight into `path` (created or truncated; @@ -112,10 +131,16 @@ impl Files<'_> { /// which only the signed storage `url` matters here. #[derive(serde::Deserialize)] struct FileLocation { - #[serde(default)] url: Option, } +/// Media types are case-insensitive; parameters (`; charset=…`) are ignored. +fn is_json(content_type: Option<&str>) -> bool { + content_type + .and_then(|ct| ct.split(';').next()) + .is_some_and(|media| media.trim().eq_ignore_ascii_case("application/json")) +} + /// Absolute URLs pass through; a path is resolved against the API **origin** /// (`/api/v1/...` lives next to, not under, the `/api/latest` base) and must /// stay there — WHATWG parsing would otherwise let `/\host/x` wander off to diff --git a/crates/kaiten-client/src/client.rs b/crates/kaiten-client/src/client.rs index 9537eac..ed525c5 100644 --- a/crates/kaiten-client/src/client.rs +++ b/crates/kaiten-client/src/client.rs @@ -252,8 +252,8 @@ impl KaitenClient { } } - /// GET `url` and return the content type and body bytes (attachment - /// downloads). + /// GET `url` and return where the answer finally came from (after + /// redirects), its content type and body bytes (attachment downloads). /// /// The bearer token is sent only when `url` is on the API origin — never /// to the public file host, nor to a storage host reached through a @@ -262,7 +262,7 @@ impl KaitenClient { /// downgrade would keep it, which only Kaiten itself could trigger). /// Retry and tracing mirror `send_with_retry`; the body is binary and /// never traced; errors go through `download_error`. - pub(crate) async fn get_bytes(&self, url: &url::Url) -> Result<(Option, Vec)> { + pub(crate) async fn get_bytes(&self, url: &url::Url) -> Result { let with_auth = url.origin() == self.base_url.origin(); let mut retries = 0u32; loop { @@ -306,7 +306,12 @@ impl KaitenClient { .get(reqwest::header::CONTENT_TYPE) .and_then(|v| v.to_str().ok()) .map(str::to_owned); - return Ok((content_type, resp.bytes().await?.to_vec())); + let final_url = resp.url().clone(); + return Ok(Fetched { + url: final_url, + content_type, + bytes: resp.bytes().await?.to_vec(), + }); } } @@ -342,6 +347,14 @@ impl KaitenClient { } } +/// Result of [`KaitenClient::get_bytes`]. +pub(crate) struct Fetched { + /// The URL that answered — differs from the requested one after a redirect. + pub(crate) url: url::Url, + pub(crate) content_type: Option, + pub(crate) bytes: Vec, +} + /// `X-RateLimit-Reset` as sent (missing/garbage → `None`). fn rate_limit_reset(resp: &reqwest::Response) -> Option { resp.headers() diff --git a/crates/kaiten-client/tests/files_test.rs b/crates/kaiten-client/tests/files_test.rs index de657eb..5960fea 100644 --- a/crates/kaiten-client/tests/files_test.rs +++ b/crates/kaiten-client/tests/files_test.rs @@ -381,5 +381,160 @@ async fn download_newer_storage_metadata_without_url_is_an_error_without_a_secon let client = KaitenClient::new(&format!("{}/api/latest", api.uri()), "test-token").unwrap(); let card: kaiten_client::Card = serde_json::from_str(CARD_GET_FILES_TYPE11).unwrap(); let err = client.files().download(&card.files[0]).await.unwrap_err(); + assert!( + matches!(&err, kaiten_client::KaitenError::Io(e) if e.kind() == std::io::ErrorKind::InvalidInput), + "{err:?}" + ); assert!(err.to_string().contains("download url"), "{err}"); } + +/// A redirecting instance hands the bytes over as-is: a JSON *attachment* +/// reached through a 302 must never be mistaken for storage metadata. +#[tokio::test] +async fn download_json_attachment_behind_a_redirect_is_returned_verbatim() { + let api = MockServer::start().await; + let storage = MockServer::start().await; + let attachment = format!(r#"{{"url": "{}/elsewhere"}}"#, storage.uri()); + Mock::given(method("GET")) + .and(path("/api/v1/cards/CU/files/FU")) + .and(header("Authorization", "Bearer test-token")) + .respond_with( + ResponseTemplate::new(302) + .insert_header("Location", format!("{}/data.json", storage.uri()).as_str()), + ) + .expect(1) + .mount(&api) + .await; + Mock::given(method("GET")) + .and(path("/data.json")) + .and(NoAuthHeader) + .respond_with( + ResponseTemplate::new(200).set_body_raw(attachment.clone(), "application/json"), + ) + .expect(1) + .mount(&storage) + .await; + Mock::given(method("GET")) + .and(path("/elsewhere")) + .respond_with(ResponseTemplate::new(200).set_body_bytes(b"wrong".to_vec())) + .expect(0) + .mount(&storage) + .await; + + let client = KaitenClient::new(&format!("{}/api/latest", api.uri()), "test-token").unwrap(); + let file = + card_file(r#"{"id": "FU", "name": "data.json", "url": "/api/v1/cards/CU/files/FU"}"#); + assert_eq!( + client.files().download(&file).await.unwrap(), + attachment.as_bytes() + ); +} + +#[tokio::test] +async fn download_classic_json_attachment_is_returned_verbatim() { + let storage = MockServer::start().await; + let attachment = r#"{"url": "https://example.invalid/not-followed"}"#; + Mock::given(method("GET")) + .and(path("/a.json")) + .respond_with(ResponseTemplate::new(200).set_body_raw(attachment, "application/json")) + .expect(1) + .mount(&storage) + .await; + + let client = KaitenClient::new("http://127.0.0.1:9", "test-token").unwrap(); + let file = card_file(&format!( + r#"{{"id": 1, "name": "a.json", "url": "{}/a.json"}}"#, + storage.uri() + )); + assert_eq!( + client.files().download(&file).await.unwrap(), + attachment.as_bytes() + ); +} + +#[tokio::test] +async fn download_newer_storage_recognises_the_media_type_case_insensitively() { + let api = MockServer::start().await; + let storage = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/api/v1/cards/CU/files/FU")) + .respond_with(ResponseTemplate::new(200).set_body_raw( + format!(r#"{{"id": "FU", "url": "{}/signed"}}"#, storage.uri()), + "Application/JSON; charset=UTF-8", + )) + .expect(1) + .mount(&api) + .await; + Mock::given(method("GET")) + .and(path("/signed")) + .and(NoAuthHeader) + .respond_with(ResponseTemplate::new(200).set_body_bytes(b"real".to_vec())) + .expect(1) + .mount(&storage) + .await; + + let client = KaitenClient::new(&format!("{}/api/latest", api.uri()), "test-token").unwrap(); + let file = card_file(r#"{"id": "FU", "name": "r", "url": "/api/v1/cards/CU/files/FU"}"#); + assert_eq!(client.files().download(&file).await.unwrap(), b"real"); +} + +#[tokio::test] +async fn download_newer_storage_invalid_json_metadata_is_a_decode_error() { + let api = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/api/v1/cards/CU/files/FU")) + .respond_with( + ResponseTemplate::new(200).set_body_raw("login", "application/json"), + ) + .mount(&api) + .await; + + let client = KaitenClient::new(&format!("{}/api/latest", api.uri()), "test-token").unwrap(); + let file = card_file(r#"{"id": "FU", "name": "r", "url": "/api/v1/cards/CU/files/FU"}"#); + let err = client.files().download(&file).await.unwrap_err(); + assert!( + matches!(err, kaiten_client::KaitenError::Decode { .. }), + "{err:?}" + ); +} + +/// The signed link lives for seconds; when storage refuses it the error must +/// say so and name the file, not just echo an S3 XML page. +#[tokio::test] +async fn download_newer_storage_refused_signed_link_names_the_file_and_storage() { + let api = MockServer::start().await; + let storage = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/api/v1/cards/CU/files/FU")) + .respond_with(ResponseTemplate::new(200).set_body_raw( + format!( + r#"{{"id": "FU", "name": "report.html", "url": "{}/signed"}}"#, + storage.uri() + ), + "application/json", + )) + .mount(&api) + .await; + Mock::given(method("GET")) + .and(path("/signed")) + .respond_with(ResponseTemplate::new(403).set_body_raw( + "AccessDeniedRequest has expired", + "application/xml", + )) + .mount(&storage) + .await; + + let client = KaitenClient::new(&format!("{}/api/latest", api.uri()), "test-token").unwrap(); + let file = + card_file(r#"{"id": "FU", "name": "report.html", "url": "/api/v1/cards/CU/files/FU"}"#); + let err = client.files().download(&file).await.unwrap_err(); + assert!( + matches!(err, kaiten_client::KaitenError::Api { status: 403, .. }), + "{err:?}" + ); + let text = err.to_string(); + assert!( + text.contains("storage refused") && text.contains("report.html"), + "{text}" + ); +} diff --git a/crates/kaiten/src/mcp/mod.rs b/crates/kaiten/src/mcp/mod.rs index 9076a8e..2124e39 100644 --- a/crates/kaiten/src/mcp/mod.rs +++ b/crates/kaiten/src/mcp/mod.rs @@ -2092,11 +2092,21 @@ mod tests { ); } + /// Matches a request that carries NO `Authorization` header. + struct NoAuthHeader; + + impl wiremock::Match for NoAuthHeader { + fn matches(&self, request: &wiremock::Request) -> bool { + !request.headers.contains_key("authorization") + } + } + /// Newer storage: the API path (bearer) answers with metadata whose `url` /// is a signed storage link; the bytes come from there without the token. #[tokio::test] async fn download_file_by_uid_follows_the_signed_url_from_the_metadata() { let server = MockServer::start().await; + let storage = MockServer::start().await; Mock::given(method("GET")) .and(path("/cards/5")) .respond_with(ResponseTemplate::new(200).set_body_string( @@ -2114,7 +2124,7 @@ mod tests { .respond_with(ResponseTemplate::new(200).set_body_raw( format!( r#"{{"id": "6a8e66af-0000-0000-0000-000000000000", "url": "{}/signed/blob"}}"#, - server.uri() + storage.uri() ), "application/json", )) @@ -2123,9 +2133,10 @@ mod tests { .await; Mock::given(method("GET")) .and(path("/signed/blob")) + .and(NoAuthHeader) .respond_with(ResponseTemplate::new(200).set_body_bytes(b"xls".to_vec())) .expect(1) - .mount(&server) + .mount(&storage) .await; let dir = tempfile::tempdir().unwrap(); From c46e7f22ab58e9e797274541b23b7a8836f7d13f Mon Sep 17 00:00:00 2001 From: dsociative Date: Thu, 27 Aug 2026 16:37:38 +0300 Subject: [PATCH 3/3] fix(client): ignore a fragment on the attachment url when telling metadata from the file (review) reqwest never sends a fragment, so the url that answers never carries one; comparing against the resolved url with its fragment would have read the metadata as the file. Also pins that a same-origin redirect to JSON is returned verbatim. --- crates/kaiten-client/src/api/files.rs | 8 +++ crates/kaiten-client/tests/files_test.rs | 66 ++++++++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/crates/kaiten-client/src/api/files.rs b/crates/kaiten-client/src/api/files.rs index 27421ad..9bc03a0 100644 --- a/crates/kaiten-client/src/api/files.rs +++ b/crates/kaiten-client/src/api/files.rs @@ -147,6 +147,14 @@ fn is_json(content_type: Option<&str>) -> bool { /// another host, so the invariant is enforced here and not left to the /// caller's token check. pub(crate) fn resolve_file_url(base_url: &url::Url, raw: &str) -> Result { + // A fragment is never sent, so the answering url never has one: drop it + // here so "the url that answered is the one we asked for" holds. + let mut resolved = resolve_file_url_raw(base_url, raw)?; + resolved.set_fragment(None); + Ok(resolved) +} + +fn resolve_file_url_raw(base_url: &url::Url, raw: &str) -> Result { match url::Url::parse(raw) { Ok(url) => Ok(url), Err(url::ParseError::RelativeUrlWithoutBase) => { diff --git a/crates/kaiten-client/tests/files_test.rs b/crates/kaiten-client/tests/files_test.rs index 5960fea..165dd8f 100644 --- a/crates/kaiten-client/tests/files_test.rs +++ b/crates/kaiten-client/tests/files_test.rs @@ -538,3 +538,69 @@ async fn download_newer_storage_refused_signed_link_names_the_file_and_storage() "{text}" ); } + +/// reqwest never sends a fragment, so the answering url has none: a `#…` on +/// the file url must not make the metadata look like the file itself. +#[tokio::test] +async fn download_newer_storage_ignores_a_fragment_on_the_file_url() { + let api = MockServer::start().await; + let storage = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/api/v1/cards/CU/files/FU")) + .respond_with(ResponseTemplate::new(200).set_body_raw( + format!(r#"{{"id": "FU", "url": "{}/signed"}}"#, storage.uri()), + "application/json", + )) + .expect(1) + .mount(&api) + .await; + Mock::given(method("GET")) + .and(path("/signed")) + .and(NoAuthHeader) + .respond_with(ResponseTemplate::new(200).set_body_bytes(b"real".to_vec())) + .expect(1) + .mount(&storage) + .await; + + let client = KaitenClient::new(&format!("{}/api/latest", api.uri()), "test-token").unwrap(); + let file = card_file(r#"{"id": "FU", "name": "r", "url": "/api/v1/cards/CU/files/FU#frag"}"#); + assert_eq!(client.files().download(&file).await.unwrap(), b"real"); +} + +/// A redirect to another path on the API origin serving JSON is still "the +/// API handed us the file", not metadata. +#[tokio::test] +async fn download_json_attachment_behind_a_same_origin_redirect_is_returned_verbatim() { + let api = MockServer::start().await; + let attachment = format!(r#"{{"url": "{}/elsewhere"}}"#, api.uri()); + Mock::given(method("GET")) + .and(path("/api/v1/cards/CU/files/FU")) + .respond_with(ResponseTemplate::new(302).insert_header( + "Location", + format!("{}/storage/FU.json", api.uri()).as_str(), + )) + .expect(1) + .mount(&api) + .await; + Mock::given(method("GET")) + .and(path("/storage/FU.json")) + .respond_with( + ResponseTemplate::new(200).set_body_raw(attachment.clone(), "application/json"), + ) + .expect(1) + .mount(&api) + .await; + Mock::given(method("GET")) + .and(path("/elsewhere")) + .respond_with(ResponseTemplate::new(200).set_body_bytes(b"wrong".to_vec())) + .expect(0) + .mount(&api) + .await; + + let client = KaitenClient::new(&format!("{}/api/latest", api.uri()), "test-token").unwrap(); + let file = card_file(r#"{"id": "FU", "name": "a.json", "url": "/api/v1/cards/CU/files/FU"}"#); + assert_eq!( + client.files().download(&file).await.unwrap(), + attachment.as_bytes() + ); +}