max / synckit
- Co-Authored-By
- Claude Opus 5 (1M context) <noreply@anthropic.com>
- Claude-Session
- https://claude.ai/code/session_013vDpLixQiknHhfHiGxFWo7
3 files changed,
+137 insertions,
-7 deletions
| @@ -33,7 +33,13 @@ | |||
| 33 | 33 | /// Upper bound on a single decrypted blob held in memory, guarding against a | |
| 34 | 34 | /// hostile server returning an absurdly large body that would OOM the client. | |
| 35 | 35 | /// Generous (4 GiB) so legitimate large media still flow. | |
| 36 | - | const MAX_BLOB_BYTES: usize = 4 * 1024 * 1024 * 1024; | |
| 36 | + | /// | |
| 37 | + | /// This is the default a client is built with, not the value the guards read. | |
| 38 | + | /// They read [`SyncKitClient::max_blob_bytes`], which is this constant unless a | |
| 39 | + | /// test lowered it: a guard that fires only at 4 GiB cannot be exercised at its | |
| 40 | + | /// own boundary without allocating four gibibytes, and a boundary nothing | |
| 41 | + | /// observes is a boundary that can be off by one in either direction. | |
| 42 | + | pub(crate) const MAX_BLOB_BYTES: usize = 4 * 1024 * 1024 * 1024; | |
| 37 | 43 | ||
| 38 | 44 | /// How old a recorded multipart session may be before a resume is not worth | |
| 39 | 45 | /// attempting. | |
| @@ -169,9 +175,10 @@ | |||
| 169 | 175 | /// the only path the server accepts above its one-shot PUT ceiling. | |
| 170 | 176 | #[instrument(skip(self, presigned_url, data))] | |
| 171 | 177 | pub async fn blob_upload(&self, hash: &str, presigned_url: &str, data: Vec<u8>) -> Result<()> { | |
| 172 | - | if data.len() > MAX_BLOB_BYTES { | |
| 178 | + | let cap = self.max_blob_bytes(); | |
| 179 | + | if data.len() > cap { | |
| 173 | 180 | return Err(SyncKitError::InvalidArgument(format!( | |
| 174 | - | "blob is {} bytes, over the {MAX_BLOB_BYTES}-byte in-memory cap; use blob_upload_streaming for a file this size", | |
| 181 | + | "blob is {} bytes, over the {cap}-byte in-memory cap; use blob_upload_streaming for a file this size", | |
| 175 | 182 | data.len() | |
| 176 | 183 | ))); | |
| 177 | 184 | } | |
| @@ -255,9 +262,10 @@ | |||
| 255 | 262 | })?; | |
| 256 | 263 | let plaintext_len = usize::try_from(meta.len()) | |
| 257 | 264 | .map_err(|_| SyncKitError::InvalidArgument("blob file is larger than usize".into()))?; | |
| 258 | - | if plaintext_len > MAX_BLOB_BYTES { | |
| 265 | + | let cap = self.max_blob_bytes(); | |
| 266 | + | if plaintext_len > cap { | |
| 259 | 267 | return Err(SyncKitError::InvalidArgument(format!( | |
| 260 | - | "blob is {plaintext_len} bytes, over the {MAX_BLOB_BYTES}-byte client cap" | |
| 268 | + | "blob is {plaintext_len} bytes, over the {cap}-byte client cap" | |
| 261 | 269 | ))); | |
| 262 | 270 | } | |
| 263 | 271 | // The session is sized in ciphertext, which is knowable from the | |
| @@ -618,6 +626,13 @@ | |||
| 618 | 626 | ||
| 619 | 627 | // Every part but the last is exactly `part_size`; the remainder is | |
| 620 | 628 | // the final part, so it is never sent from inside this loop. | |
| 629 | + | // | |
| 630 | + | // `<` and `<=` on the part index are equivalent, provably, so no | |
| 631 | + | // test distinguishes them: `staged.len() >= part_size` can hold at | |
| 632 | + | // `next_part == part_count` only when the ciphertext is an exact | |
| 633 | + | // multiple of the part size, and the extra iteration would then | |
| 634 | + | // send byte-identical bytes to the same URL before the function | |
| 635 | + | // returned down the resume branch regardless. | |
| 621 | 636 | while staged.len() >= part_size && next_part < start.part_count { | |
| 622 | 637 | let body = staged.split_to(part_size).freeze(); | |
| 623 | 638 | let etag = self | |
| @@ -893,13 +908,14 @@ | |||
| 893 | 908 | let mut next_chunk: u32 = 0; | |
| 894 | 909 | let mut plaintext: Vec<u8> = Vec::new(); | |
| 895 | 910 | let mut total_in: usize = 0; | |
| 911 | + | let cap = self.max_blob_bytes(); | |
| 896 | 912 | ||
| 897 | 913 | while let Some(part) = stream.next().await { | |
| 898 | 914 | let part = part.map_err(SyncKitError::Http)?; | |
| 899 | 915 | total_in = total_in.saturating_add(part.len()); | |
| 900 | - | if total_in > MAX_BLOB_BYTES { | |
| 916 | + | if total_in > cap { | |
| 901 | 917 | return Err(SyncKitError::Internal(format!( | |
| 902 | - | "blob exceeds {MAX_BLOB_BYTES}-byte limit", | |
| 918 | + | "blob exceeds {cap}-byte limit", | |
| 903 | 919 | ))); | |
| 904 | 920 | } | |
| 905 | 921 | buf.extend_from_slice(&part); | |
| @@ -922,6 +938,10 @@ | |||
| 922 | 938 | header = Some(h); | |
| 923 | 939 | } | |
| 924 | 940 | // Peel and decrypt every complete chunk currently buffered. | |
| 941 | + | // | |
| 942 | + | // `<` and `<=` are equivalent here too: one chunk past the count | |
| 943 | + | // breaks on the next line against an empty buffer. The bound is | |
| 944 | + | // written as the count because that is what the count means. | |
| 925 | 945 | let h = header.unwrap(); | |
| 926 | 946 | while next_chunk < h.chunk_count { | |
| 927 | 947 | let len = h.sealed_chunk_len(next_chunk); |
| @@ -100,6 +100,7 @@ | |||
| 100 | 100 | use parking_lot::RwLock; | |
| 101 | 101 | use reqwest::Client; | |
| 102 | 102 | use std::sync::Arc; | |
| 103 | + | use std::sync::atomic::{AtomicUsize, Ordering}; | |
| 103 | 104 | use std::time::Duration; | |
| 104 | 105 | #[cfg(test)] | |
| 105 | 106 | use uuid::Uuid; | |
| @@ -428,6 +429,18 @@ | |||
| 428 | 429 | /// which is the behaviour this client always had. See | |
| 429 | 430 | /// [`resume`](crate::client::resume). | |
| 430 | 431 | resume_store: RwLock<Option<Arc<dyn resume::BlobResumeStore>>>, | |
| 432 | + | /// The ceiling on a blob this client will hold whole in memory, on either | |
| 433 | + | /// the upload or the download side. | |
| 434 | + | /// | |
| 435 | + | /// [`blob::MAX_BLOB_BYTES`] (4 GiB) unless a test lowered it. It is a field | |
| 436 | + | /// rather than the constant read directly because the guards fire only at | |
| 437 | + | /// 4 GiB: `>`, `>=` and `==` agree at every size a test can reach, so the | |
| 438 | + | /// boundary is unobservable without either allocating four gibibytes or | |
| 439 | + | /// moving the boundary. Moving it is the cheaper truth. | |
| 440 | + | /// | |
| 441 | + | /// Relaxed ordering: nothing else is published through this, and a cap read | |
| 442 | + | /// one transfer late is still a cap. | |
| 443 | + | max_blob_bytes: AtomicUsize, | |
| 431 | 444 | /// The storage version sealed into every pushed change, so a peer can refuse | |
| 432 | 445 | /// a shared changelog it does not fully understand. | |
| 433 | 446 | /// | |
| @@ -498,6 +511,7 @@ | |||
| 498 | 511 | pending_key: RwLock::new(None), | |
| 499 | 512 | gck_cache: RwLock::new(std::collections::HashMap::new()), | |
| 500 | 513 | resume_store: RwLock::new(None), | |
| 514 | + | max_blob_bytes: AtomicUsize::new(blob::MAX_BLOB_BYTES), | |
| 501 | 515 | storage_version: RwLock::new(None), | |
| 502 | 516 | } | |
| 503 | 517 | } | |
| @@ -521,6 +535,7 @@ | |||
| 521 | 535 | pending_key: RwLock::new(None), | |
| 522 | 536 | gck_cache: RwLock::new(std::collections::HashMap::new()), | |
| 523 | 537 | resume_store: RwLock::new(None), | |
| 538 | + | max_blob_bytes: AtomicUsize::new(blob::MAX_BLOB_BYTES), | |
| 524 | 539 | storage_version: RwLock::new(None), | |
| 525 | 540 | } | |
| 526 | 541 | } | |
| @@ -592,6 +607,24 @@ | |||
| 592 | 607 | *self.master_key.write() = Some(crypto::ZeroizeOnDrop(key)); | |
| 593 | 608 | } | |
| 594 | 609 | ||
| 610 | + | /// The in-memory blob ceiling this client is enforcing. | |
| 611 | + | pub(crate) fn max_blob_bytes(&self) -> usize { | |
| 612 | + | self.max_blob_bytes.load(Ordering::Relaxed) | |
| 613 | + | } | |
| 614 | + | ||
| 615 | + | /// Lower (or raise) the in-memory blob ceiling. | |
| 616 | + | /// | |
| 617 | + | /// Test-only. The guards that read it are one character from being wrong in | |
| 618 | + | /// a way that refuses legitimate media or admits an unbounded allocation, | |
| 619 | + | /// and at the shipped 4 GiB no test can put a value on either side of the | |
| 620 | + | /// boundary. A cap of a few dozen bytes puts both sides within reach of a | |
| 621 | + | /// fixture, exercising the real guard rather than a copy of it. | |
| 622 | + | #[doc(hidden)] | |
| 623 | + | #[cfg(any(test, feature = "testing"))] | |
| 624 | + | pub fn set_max_blob_bytes(&self, cap: usize) { | |
| 625 | + | self.max_blob_bytes.store(cap, Ordering::Relaxed); | |
| 626 | + | } | |
| 627 | + | ||
| 595 | 628 | // ── Internal helpers ── | |
| 596 | 629 | ||
| 597 | 630 | /// Extract the bearer token from the current session. |
| @@ -507,3 +507,80 @@ | |||
| 507 | 507 | "a trailing byte is not part of any chunk and must be refused" | |
| 508 | 508 | ); | |
| 509 | 509 | } | |
| 510 | + | ||
| 511 | + | // ── The in-memory size cap, at its own boundary ── | |
| 512 | + | // | |
| 513 | + | // Both guards are `>` against a 4 GiB ceiling, and `>`, `>=` and `==` agree at | |
| 514 | + | // every size below it. Nothing in the suite could tell them apart without | |
| 515 | + | // holding four gibibytes in memory, so the cap is lowered instead and the real | |
| 516 | + | // guard is driven from both sides of wherever it now sits. Off-by-one in one | |
| 517 | + | // direction refuses legitimate media; in the other it admits the unbounded | |
| 518 | + | // allocation the cap exists to prevent. | |
| 519 | + | ||
| 520 | + | #[tokio::test] | |
| 521 | + | async fn the_in_memory_upload_cap_admits_a_blob_of_exactly_the_cap_and_refuses_one_byte_more() { | |
| 522 | + | let kit = MockKit::start().await; | |
| 523 | + | let (client, _key) = kit.keyed(); | |
| 524 | + | client.set_max_blob_bytes(64); | |
| 525 | + | ||
| 526 | + | let upload_path = "/s3/cap-upload"; | |
| 527 | + | kit.put(upload_path).empty().await; | |
| 528 | + | let hash = "c".repeat(64); | |
| 529 | + | ||
| 530 | + | client | |
| 531 | + | .blob_upload(&hash, &kit.url(upload_path), vec![7u8; 64]) | |
| 532 | + | .await | |
| 533 | + | .expect("a blob of exactly the cap is under it and must be sent"); | |
| 534 | + | assert_eq!(kit.hits(upload_path).await, 1); | |
| 535 | + | ||
| 536 | + | let err = client | |
| 537 | + | .blob_upload(&hash, &kit.url(upload_path), vec![7u8; 65]) | |
| 538 | + | .await | |
| 539 | + | .unwrap_err(); | |
| 540 | + | match err { | |
| 541 | + | SyncKitError::InvalidArgument(m) => { | |
| 542 | + | assert!(m.contains("in-memory cap"), "wrong rejection: {m}"); | |
| 543 | + | } | |
| 544 | + | other => panic!("one byte over the cap must be refused, got {other:?}"), | |
| 545 | + | } | |
| 546 | + | assert_eq!( | |
| 547 | + | kit.hits(upload_path).await, | |
| 548 | + | 1, | |
| 549 | + | "the refused blob must not have reached the wire" | |
| 550 | + | ); | |
| 551 | + | } | |
| 552 | + | ||
| 553 | + | #[tokio::test] | |
| 554 | + | async fn the_download_cap_admits_a_body_of_exactly_the_cap_and_refuses_it_one_byte_lower() { | |
| 555 | + | // The same body twice, with the cap moved by one byte, so the only thing | |
| 556 | + | // the two runs can be telling apart is where the boundary sits. | |
| 557 | + | let kit = MockKit::start().await; | |
| 558 | + | let (client, key) = kit.keyed(); | |
| 559 | + | ||
| 560 | + | let plaintext = b"a body served against a lowered ceiling"; | |
| 561 | + | let hash = hex::encode(sha2::Sha256::digest(plaintext)); | |
| 562 | + | let encrypted = synckit_client::crypto::encrypt_bytes(plaintext, &key).unwrap(); | |
| 563 | + | let len = encrypted.len(); | |
| 564 | + | ||
| 565 | + | let path = "/s3/cap-download"; | |
| 566 | + | kit.get(path).bytes(encrypted).await; | |
| 567 | + | ||
| 568 | + | client.set_max_blob_bytes(len); | |
| 569 | + | let got = client | |
| 570 | + | .blob_download(&hash, &kit.url(path)) | |
| 571 | + | .await | |
| 572 | + | .expect("a body of exactly the cap is under it and must decrypt"); | |
| 573 | + | assert_eq!(got, plaintext); | |
| 574 | + | ||
| 575 | + | client.set_max_blob_bytes(len - 1); | |
| 576 | + | let err = client | |
| 577 | + | .blob_download(&hash, &kit.url(path)) | |
| 578 | + | .await | |
| 579 | + | .unwrap_err(); | |
| 580 | + | match err { | |
| 581 | + | SyncKitError::Internal(m) => { | |
| 582 | + | assert!(m.contains("exceeds"), "wrong rejection: {m}"); | |
| 583 | + | } | |
| 584 | + | other => panic!("a body over the cap must be refused, got {other:?}"), | |
| 585 | + | } | |
| 586 | + | } |