refactor(upload): apply review feedback from #145

Four small follow-ups from @dobby-coder's review:

1. Cheap pre-check before reading the chunk body. Mirrors the structural
   part of `classify_chunk_request` (normal-next OR replay-candidate)
   and rejects mismatches before consuming up to `chunk_size` bytes,
   restoring the prior security profile against UUIDs leaked to an
   attacker.

2. Drop `chunk_sha256` and `chunk_len` from `LastChunkRecord`. Body
   identity falls out of the rolling-token chain itself: recompute
   `compute_hash(prev_token, body)` and compare to `response_token`.
   Length divergence surfaces as a hash mismatch — one less invariant
   to keep in sync, smaller `FileState`, simpler Phase 2 schema. The
   length-specific test folds into the body-mismatch test.

3. Promote the duplicated "Cryptify Token header does not match"
   string to a `TOKEN_MISMATCH_MSG` const referenced by both
   `check_cryptify_token` (finalize path) and the chunk classifier so
   the wording can't drift silently between call sites.

4. One fewer `shasum.clone()` — the final responder takes the moved
   value while `state.cryptify_token` and `last_chunk.response_token`
   share a single clone.

`sha256_of` is removed (sole caller is gone). Tests gain a
`last_chunk_for(prev_token, offset, body)` helper that builds a
`LastChunkRecord` whose `response_token` is the actual rolling hash,
matching the production handler's invariant.

Refs #145 review.

Author: rubenhensen

src/main.rs

@@ -421,11 +421,14 @@ fn compute_hash(cryptify_token: &[u8], data: &[u8]) -> String {
     bytes_to_hex(&hash.finalize())
 }
 
+/// Wire-level error message for a `CryptifyToken` mismatch. Reused by both
+/// `check_cryptify_token` (the finalize path) and the chunk classifier so the
+/// message can't drift silently between call sites.
+const TOKEN_MISMATCH_MSG: &str = "Cryptify Token header does not match";
+
 fn check_cryptify_token(header: &str, expected: &str) -> Result<(), Error> {
     if header != expected {
-        return Err(Error::BadRequest(Some(
-            "Cryptify Token header does not match".to_owned(),
-        )));
+        return Err(Error::BadRequest(Some(TOKEN_MISMATCH_MSG.to_owned())));
     }
     Ok(())
 }
@@ -470,6 +473,25 @@ async fn upload_chunk(
         ))));
     }
 
+    // Cheap pre-check before reading the body, so a leaked UUID can't be
+    // used to force the server to buffer up to `chunk_size` bytes per
+    // request just to be rejected. Mirrors the structural part of
+    // `classify_chunk_request` — we only commit to reading the body when
+    // the request looks like either a normal next chunk or a candidate
+    // replay of the last committed chunk.
+    let is_normal_next = state.uploaded == start && headers.cryptify_token == state.cryptify_token;
+    let is_replay_candidate = state.last_chunk.as_ref().is_some_and(|last| {
+        last.prev_uploaded == start && headers.cryptify_token == last.prev_token
+    });
+    if !is_normal_next && !is_replay_candidate {
+        if state.uploaded != start {
+            return Err(Error::BadRequest(Some(
+                "Incorrect Content-Range header".to_owned(),
+            )));
+        }
+        return Err(Error::BadRequest(Some(TOKEN_MISMATCH_MSG.to_owned())));
+    }
+
     let body = data
         .open((end - start).bytes())
         .into_bytes()
@@ -482,7 +504,7 @@ async fn upload_chunk(
 
     // Three branches: normal next chunk, idempotent retry of the last
     // committed chunk, or rejection.
-    match classify_chunk_request(&state, &headers.cryptify_token, start, end, &body) {
+    match classify_chunk_request(&state, &headers.cryptify_token, start, &body) {
         ChunkClassification::NormalNext => {}
         ChunkClassification::ReplayLastChunk(token) => {
             drop(state);
@@ -540,16 +562,13 @@ async fn upload_chunk(
         .await
         .map_err(|_| Error::InternalServerError(Some("Could not write file".to_owned())))?;
 
-    let prev_token = headers.cryptify_token.clone();
-    let chunk_sha256 = sha256_of(&body);
+    let prev_token = headers.cryptify_token;
     let shasum = compute_hash(prev_token.as_bytes(), &body);
     state.cryptify_token = shasum.clone();
     state.uploaded += end - start;
     state.last_chunk = Some(LastChunkRecord {
         prev_token,
         prev_uploaded: start,
-        chunk_len: end - start,
-        chunk_sha256,
         response_token: shasum.clone(),
     });
 
@@ -571,17 +590,17 @@ enum ChunkClassification {
     /// returns this token to the client without re-writing or double-counting.
     ReplayLastChunk(String),
     /// Reject the request with this error — the standard 400 you'd get
-    /// before idempotent-retry support, plus stricter 400s when the request
-    /// looks like a retry but the body or length diverges (almost certainly
-    /// a client bug, never accept different bytes for the same offset).
+    /// before idempotent-retry support, plus a stricter 400 when the
+    /// request looks like a retry but the body bytes (or their length)
+    /// diverge from the cached chunk. Never accept different bytes for
+    /// the same offset.
     Reject(Error),
 }
 
 fn classify_chunk_request(
     state: &FileState,
     request_token: &str,
     start: u64,
-    end: u64,
     body: &[u8],
 ) -> ChunkClassification {
     if state.uploaded == start && request_token == state.cryptify_token {
@@ -590,17 +609,18 @@ fn classify_chunk_request(
 
     if let Some(last) = state.last_chunk.as_ref() {
         if request_token == last.prev_token && start == last.prev_uploaded {
-            if end - start != last.chunk_len {
-                return ChunkClassification::Reject(Error::BadRequest(Some(
-                    "Idempotent retry: chunk length differs from the original".to_owned(),
-                )));
-            }
-            if sha256_of(body) != last.chunk_sha256 {
-                return ChunkClassification::Reject(Error::BadRequest(Some(
-                    "Idempotent retry: body hash differs from the original chunk".to_owned(),
-                )));
+            // Recompute the rolling hash over the incoming body. Identity
+            // is implicit in the rolling-token construction itself: if the
+            // hash matches `response_token`, the body is byte-identical to
+            // the original chunk (modulo a SHA-256 collision, which would
+            // also break the rolling chain). Length divergence surfaces
+            // here too.
+            if compute_hash(last.prev_token.as_bytes(), body) == last.response_token {
+                return ChunkClassification::ReplayLastChunk(last.response_token.clone());
             }
-            return ChunkClassification::ReplayLastChunk(last.response_token.clone());
+            return ChunkClassification::Reject(Error::BadRequest(Some(
+                "Idempotent retry: body differs from the original chunk".to_owned(),
+            )));
         }
     }
 
@@ -610,17 +630,7 @@ fn classify_chunk_request(
         )));
     }
 
-    // Right offset but wrong token: the existing `check_cryptify_token`
-    // wording, so error messages don't change for non-retry callers.
-    ChunkClassification::Reject(Error::BadRequest(Some(
-        "Cryptify Token header does not match".to_owned(),
-    )))
-}
-
-fn sha256_of(data: &[u8]) -> [u8; 32] {
-    let mut hash = sha2::Sha256::new();
-    hash.update(data);
-    hash.finalize().into()
+    ChunkClassification::Reject(Error::BadRequest(Some(TOKEN_MISMATCH_MSG.to_owned())))
 }
 
 struct FinalizeHeaders {
@@ -1151,10 +1161,22 @@ mod tests {
         s
     }
 
+    /// Build a `LastChunkRecord` whose `response_token` correctly encodes
+    /// `prev_token + body`, the same construction the production handler
+    /// uses. Tests use this so the replay path's hash check passes on a
+    /// genuine retry and fails when the body is tampered with.
+    fn last_chunk_for(prev_token: &str, prev_uploaded: u64, body: &[u8]) -> LastChunkRecord {
+        LastChunkRecord {
+            prev_token: prev_token.to_owned(),
+            prev_uploaded,
+            response_token: compute_hash(prev_token.as_bytes(), body),
+        }
+    }
+
     #[test]
     fn classify_normal_next_chunk() {
         let state = empty_filestate(100, "tok-current");
-        match classify_chunk_request(&state, "tok-current", 100, 200, b"chunk") {
+        match classify_chunk_request(&state, "tok-current", 100, b"chunk") {
             ChunkClassification::NormalNext => {}
             _ => panic!("expected NormalNext"),
         }
@@ -1163,73 +1185,54 @@ mod tests {
     #[test]
     fn classify_replays_last_chunk_on_matching_retry() {
         let body = b"hello world";
-        let last = LastChunkRecord {
-            prev_token: "tok-prev".into(),
-            prev_uploaded: 100,
-            chunk_len: body.len() as u64,
-            chunk_sha256: sha256_of(body),
-            response_token: "tok-after".into(),
-        };
-        let state = filestate_with_last_chunk(100 + body.len() as u64, "tok-after", last);
-        match classify_chunk_request(&state, "tok-prev", 100, 100 + body.len() as u64, body) {
-            ChunkClassification::ReplayLastChunk(t) => assert_eq!(t, "tok-after"),
+        let last = last_chunk_for("tok-prev", 100, body);
+        let response_token = last.response_token.clone();
+        let state = filestate_with_last_chunk(100 + body.len() as u64, &response_token, last);
+        match classify_chunk_request(&state, "tok-prev", 100, body) {
+            ChunkClassification::ReplayLastChunk(t) => assert_eq!(t, response_token),
             _ => panic!("expected ReplayLastChunk"),
         }
     }
 
     #[test]
     fn classify_rejects_retry_with_different_body() {
         let body = b"original";
-        let last = LastChunkRecord {
-            prev_token: "tok-prev".into(),
-            prev_uploaded: 100,
-            chunk_len: body.len() as u64,
-            chunk_sha256: sha256_of(body),
-            response_token: "tok-after".into(),
-        };
-        let state = filestate_with_last_chunk(100 + body.len() as u64, "tok-after", last);
+        let last = last_chunk_for("tok-prev", 100, body);
+        let response_token = last.response_token.clone();
+        let state = filestate_with_last_chunk(100 + body.len() as u64, &response_token, last);
         let tampered = b"tampered";
-        let result = classify_chunk_request(
-            &state,
-            "tok-prev",
-            100,
-            100 + tampered.len() as u64,
-            tampered,
-        );
+        let result = classify_chunk_request(&state, "tok-prev", 100, tampered);
         match result {
             ChunkClassification::Reject(Error::BadRequest(Some(msg))) => {
-                assert!(msg.contains("body hash"), "got: {}", msg);
+                assert!(msg.contains("body differs"), "got: {}", msg);
             }
-            _ => panic!("expected BadRequest about body hash"),
+            _ => panic!("expected BadRequest about body differs"),
         }
     }
 
     #[test]
     fn classify_rejects_retry_with_different_length() {
+        // Same prev_token + start, but a shorter body. The recomputed
+        // rolling hash won't match, so the body-differs path catches this
+        // case too — we no longer need a length-specific record.
         let body = b"original";
-        let last = LastChunkRecord {
-            prev_token: "tok-prev".into(),
-            prev_uploaded: 100,
-            chunk_len: body.len() as u64,
-            chunk_sha256: sha256_of(body),
-            response_token: "tok-after".into(),
-        };
-        let state = filestate_with_last_chunk(100 + body.len() as u64, "tok-after", last);
-        // Same prev_token + start, but length differs from cached record.
-        let result = classify_chunk_request(&state, "tok-prev", 100, 100 + 5, b"short");
+        let last = last_chunk_for("tok-prev", 100, body);
+        let response_token = last.response_token.clone();
+        let state = filestate_with_last_chunk(100 + body.len() as u64, &response_token, last);
+        let result = classify_chunk_request(&state, "tok-prev", 100, b"short");
         match result {
             ChunkClassification::Reject(Error::BadRequest(Some(msg))) => {
-                assert!(msg.contains("chunk length"), "got: {}", msg);
+                assert!(msg.contains("body differs"), "got: {}", msg);
             }
-            _ => panic!("expected BadRequest about chunk length"),
+            _ => panic!("expected BadRequest about body differs"),
         }
     }
 
     #[test]
     fn classify_rejects_offset_mismatch_with_no_replay() {
         // No last_chunk recorded → offset mismatch is just the regular 400.
         let state = empty_filestate(100, "tok-current");
-        let result = classify_chunk_request(&state, "tok-current", 50, 60, b"abc");
+        let result = classify_chunk_request(&state, "tok-current", 50, b"abc");
         match result {
             ChunkClassification::Reject(Error::BadRequest(Some(msg))) => {
                 assert_eq!(msg, "Incorrect Content-Range header");
@@ -1241,10 +1244,10 @@ mod tests {
     #[test]
     fn classify_rejects_token_mismatch_at_correct_offset() {
         let state = empty_filestate(100, "tok-current");
-        let result = classify_chunk_request(&state, "tok-wrong", 100, 110, b"chunk");
+        let result = classify_chunk_request(&state, "tok-wrong", 100, b"chunk");
         match result {
             ChunkClassification::Reject(Error::BadRequest(Some(msg))) => {
-                assert_eq!(msg, "Cryptify Token header does not match");
+                assert_eq!(msg, TOKEN_MISMATCH_MSG);
             }
             _ => panic!("expected BadRequest about token mismatch"),
         }
@@ -1255,15 +1258,10 @@ mod tests {
         // Last chunk exists but the retry presents a *different* prev_token.
         // Falls through to the regular offset-mismatch rejection.
         let body = b"original";
-        let last = LastChunkRecord {
-            prev_token: "tok-prev".into(),
-            prev_uploaded: 100,
-            chunk_len: body.len() as u64,
-            chunk_sha256: sha256_of(body),
-            response_token: "tok-after".into(),
-        };
-        let state = filestate_with_last_chunk(100 + body.len() as u64, "tok-after", last);
-        let result = classify_chunk_request(&state, "tok-something-else", 100, 108, body);
+        let last = last_chunk_for("tok-prev", 100, body);
+        let response_token = last.response_token.clone();
+        let state = filestate_with_last_chunk(100 + body.len() as u64, &response_token, last);
+        let result = classify_chunk_request(&state, "tok-something-else", 100, body);
         match result {
             ChunkClassification::Reject(Error::BadRequest(Some(msg))) => {
                 assert_eq!(msg, "Incorrect Content-Range header");

src/store.rs

@@ -51,15 +51,21 @@ pub struct FileState {
     /// handler detect a duplicate retry (when the client never saw the
     /// previous response): if the request's `CryptifyToken` matches
     /// `prev_token` and `Content-Range.start` matches `prev_uploaded`, and
-    /// the body's SHA-256 matches `chunk_sha256`, the server replays
-    /// `response_token` instead of advancing the rolling-token chain or
-    /// double-writing the chunk. `None` until at least one chunk has been
-    /// successfully committed.
+    /// recomputing the rolling hash over the incoming body equals
+    /// `response_token`, the server replays `response_token` instead of
+    /// advancing the rolling-token chain or double-writing the chunk.
+    /// `None` until at least one chunk has been successfully committed.
     pub last_chunk: Option<LastChunkRecord>,
 }
 
 /// Replay record of the most recently committed chunk. See
 /// [`FileState::last_chunk`].
+///
+/// Body identity is checked by recomputing the rolling hash
+/// `sha256(prev_token || body)` and comparing against `response_token` —
+/// the same construction the rolling-token chain itself relies on, so no
+/// separate digest needs to be cached. Length differences also surface as
+/// a hash mismatch.
 #[derive(Clone, Debug)]
 pub struct LastChunkRecord {
     /// The `CryptifyToken` the client sent in the chunk PUT — i.e., the
@@ -69,11 +75,6 @@ pub struct LastChunkRecord {
     /// `state.uploaded` *before* this chunk was applied — equals the
     /// chunk's `Content-Range` start.
     pub prev_uploaded: u64,
-    /// The chunk body length in bytes.
-    pub chunk_len: u64,
-    /// SHA-256 of the chunk body. Verified on retry to ensure the client
-    /// is replaying the same bytes (not a colliding-offset different chunk).
-    pub chunk_sha256: [u8; 32],
     /// The token the server returned in response to the original PUT —
     /// i.e., the value of `state.cryptify_token` after this chunk was
     /// applied. Replayed verbatim on a detected retry.