mytheclipse/crates/mytheclipse-storage/AUDIT-FINDINGS.md

Audit Findings — mytheclipse-storage

Scope: crates/mytheclipse-storage/src/ (lib.rs, traits.rs, local.rs, s3.rs, gcs.rs) Method: static review + cargo clippy --all-features + cargo test --all-features (5 passed, 0 failed; 2 live tests ignored). Severity legend: CRITICAL · HIGH · MEDIUM · LOW

---

1. Path traversal — local.rs (mostly PASS, one gap)

LocalFileStorage::resolve (local.rs:25-33) rejects Component::ParentDir (..) and Component::Prefix (Windows drive letters). This correctly blocks foo/../../etc/passwd, .., and absolute C:\... paths. ✅

Gap — MEDIUM local.rs:26: Leading/trailing slashes and NUL bytes are not sanitized. path.trim_start_matches('/') turns /etc/passwd into etc/passwd (sandboxed — safe), but a NUL byte (foo\0bar) is passed straight to tokio::fs::File::open/create. On Unix this is benign, but it is passed unmodified to resolve. Not exploited, but inconsistent with the documented "rejected path" posture. Also .. inside a single filename like foo/./../bar is decomposed by components() and caught — good.

No actual traversal found. The existing path_traversal_is_rejected test (local.rs:156-164) covers ../escape.txt only; add coverage for %2e%2e encoded input and NUL.

2. File handle leaks — local.rs (NO OS-handle leak; durability + cleanup gaps)

There is no raw file-descriptor leak: every tokio::fs::File is a Rust local that is dropped (and closed) when its function returns — including the copy-error path in put (local.rs:57-63), where the ? drops file. ✅

However two related gaps exist:

• MEDIUM — local.rs:60-62 (no flush/sync): put calls tokio::io::copy(&mut data, &mut file) then returns Ok(written) with no file.flush().await and no sync_all. The data sits in the kernel page cache; on process/crash before close it may be lost, and the returned written count reflects bytes handed to copy, not bytes durably on disk. For an object store promising "written", this is a durability gap.

• MEDIUM — local.rs:57-63 (partial file on failure): File::create truncates the destination, then if copy fails partway the function returns Err(StorageError::Io) but a partially-written, truncated file remains at full. The caller gets an error and a corrupt object on disk. Suggested: write to a temp path (e.g. full.with_extension("tmp").with_added_extension("partial")) and rename only on success; on error, remove_file.

• LOW — local.rs:38-48 (get stream lifetime): get returns ObjectStream wrapping the open File. The handle stays open until the consumer drains/drops the stream. If a caller abandons the stream, the handle is held until the ObjectStream is dropped (correct), but the trait offers no explicit "release" — callers must scope-drop the stream. This is inherent to streaming APIs, but the doc on StorageDriver::get should warn the caller they own the handle lifetime.

3. Multipart upload correctness — s3.rs (PASS with validation gap)

S3Storage::put (s3.rs:100-183) and upload_parts (s3.rs:238-291) implement the standard read-first-chunk → 1-part PutObject vs. multipart pattern correctly:

• Single-part upload when filled < MULTIPART_CHUNK_SIZE (s3.rs:117). ✅
• First chunk becomes part 1 of the multipart (s3.rs:144-151). ✅
• Loop reads full 8 MiB chunks and uploads each; breaks when a read returns empty. ✅
• On part-upload error, abort_multipart_upload is invoked (s3.rs:171-181) — proper cleanup. ✅
• CompletedPart is built with e_tag + part_number. ✅
• complete_multipart_upload is sent with the accumulated parts (s3.rs:154-169). ✅

Gap — MEDIUM — s3.rs:248 / s3.rs:269 (part-number overflow, no client cap): part_number is i32, starting at 1 and incremented per part. S3 allows max 10,000 parts. With MULTIPART_CHUNK_SIZE = 8 MiB (s3.rs:21), the silent failure point is ~80 GiB. At part 10001 the AWS SDK returns a server error; the code maps it to StorageError::Io after a part has already been uploaded, triggering abort_multipart_upload (good — cleanup runs). But the failure should be detected client-side with a clear StorageError before attempting an illegal part number, and the constant 8 MiB (s3.rs:21) exceeds S3's documented minimum of 5 MiB — the comment says "5 MiB" but the value is 8 * 1024 * 1024. The comment is wrong.

• s3.rs:21 comment mismatch: // S3's minimum multipart part size (5 MiB) but MULTIPART_CHUNK_SIZE = 8 MiB. Fix the comment. ✅-ish but misleading docs.

• LOW — s3.rs:265: resp.e_tag().unwrap_or_default() — if S3 omits the ETag (rare), an empty string is sent in CompletedPart. Some backends reject empty ETags. Use unwrap_or("") explicitly with a TODO, or skip. Minor.

4. Error-handling gaps — all backends

4.1 Fragile NotFound detection (s3.rs:295-297, gcs.rs:66, gcs.rs:125)

is_not_found (s3.rs:295):

fn is_not_found<E: std::fmt::Debug>(e: &E) -> bool {
    format!("{e:?}").contains("NotFound") || format!("{e:?}").contains("404")
}

• 404 substring match will false-positive on any error whose Debug string mentions "404" (e.g. a 4048-port URL, a 4040-style code in a message). HIGH risk for incorrect NotFound mapping on S3.
• Relies on Debug, not typed error matching. The aws-sdk-s3 v1 Error enum exposes typed NoSuchKey/NotFound via .kind() — prefer e.kind() == Some(ErrorCode::NoSuchKey) or the Display-based NotFound variant.

GCS (gcs.rs:66, gcs.rs:125) uses e.to_string().contains("404") — same 404 false-positive problem; GCS errors surface 404 in Display for Status but a non-NotFound 404-ish message would be mis-mapped to NotFound. MEDIUM.

Recommendation: match on typed error variants / HTTP status codes, not substring.

4.2 Inconsistent delete semantics (MEDIUM)

• local.rs:66-75: deleting a missing object → StorageError::NotFound. ✅ (documented in test)
• s3.rs:185-194: delete_object on a missing key returns 204 No Content from S3 (idempotent) → Ok(()). ✅ consistent with S3 semantics.
• gcs.rs:96-105: delete_object on missing → GCS returns 404, mapped to StorageError::Io (via map_err), not NotFound. Inconsistent: GcsStorage::delete of a missing object errors with Io, while LocalFileStorage::delete errors with NotFound. Callers handling "ignore missing" must special-case. MEDIUM.

4.3 StorageError has no Backend-level detail, only Io(String) (LOW)

traits.rs:17 — Io(String) collapses all transport errors into a string. No structured access to the underlying std::io::Error (e.g. ErrorKind::UnexpectedEof). Limits retry logic. Acceptable for v1, but map_sdk_err (s3.rs:23-25) re-formats via {e:?} (Debug) — callers see a Debug dump, not a clean message. Consider Display. LOW.

4.4 stat size coercion (LOW — s3.rs:227)

resp.content_length().unwrap_or(0).max(0) as u64 — content_length() returns Option<i64>; .max(0) clamps negatives. Fine, but the .unwrap_or(0) for a missing object is unreachable here because a 404 is mapped to NotFound first. OK.

5. Feature / API consistency gaps (MEDIUM)

Operation	local (default)	s3	gcs
put streaming	✅ streaming	✅ multipart streaming	❌ buffers whole file in Vec<u8> (gcs.rs:76-79) — contradicts crate promise "never holds the whole object in memory"
get streaming	✅ file handle	✅ into_async_read	✅ but actually downloads full object into Vec<u8> then re-boxes (gcs.rs:54-72) — not true streaming
Multipart / resumable	n/a	✅ multipart	❌ none (simple upload only)
not_impl! for disabled features	n/a	the trait is always compiled; backends are feature-gated at module level	✅

gcs.rs:75-93 put: data.read_to_end(&mut buf) materializes the entire upload in memory. The module doc (gcs.rs:4-8) admits this. For a storage abstraction whose USP is "huge objects never buffered in memory", GCS is the weak backend. Recommend implementing resumable upload (google-cloud-storage supports upload_types::Resumable) to match S3.

Feature consistency note: because StorageDriver is in traits.rs (always compiled) and tokio is declared with only io-util (Cargo.toml), consumers using s3/gcs without the tokio full feature for their own runtime could hit compile friction. Minor — Cargo.toml dev-dependencies pulls tokio = { features = ["full"] }. LOW.

6. Build / clippy

cargo clippy -p mytheclipse-storage --all-features → 0 warnings. ✅ cargo test -p mytheclipse-storage --all-features → 5 passed, 2 ignored (live-only). ✅

---

Summary table

#	Area	Severity	Location
1	No real path traversal (control rejected)	—	local.rs:25-33 ✅
2	No OS file-handle leak (Rust Drop closes files)	—	local.rs:57-63 ✅
3	put missing flush/sync (durability)	MEDIUM	local.rs:60-62
4	Failed put leaves partial/truncated file	MEDIUM	local.rs:57-63
5	get stream handle lifetime not documented	LOW	local.rs:38-48, traits.rs:56-58
6	Multipart part-number not capped at 10000	MEDIUM	s3.rs:248,269
7	Comment says 5 MiB min, value is 8 MiB	LOW	s3.rs:21
8	Fragile 404/NotFound substring matching (S3)	HIGH	s3.rs:295-297
9	Fragile 404 substring matching (GCS)	MEDIUM	gcs.rs:66,125
10	Inconsistent delete on missing object (GCS returns Io, not NotFound)	MEDIUM	gcs.rs:96-105 vs local.rs:66-75
11	GCS put/get buffer whole object (no resumable/multipart)	MEDIUM	gcs.rs:54-93
12	map_sdk_err uses Debug format for messages	LOW	s3.rs:23-25
13	CompletedPart ETag unwrap_or_default() can be empty	LOW	s3.rs:265
14	StorageError::Io(String) loses structured io::Error	LOW	traits.rs:17-18

Recommended remediation (priority)

1. (HIGH) s3.rs:295 / gcs.rs:66,125 — replace format!("{e:?}").contains("404") with typed error-variant matching (S3 Error::kind() / ErrorCode::NoSuchKey; GCS Error::status.code() == 404).
2. (MEDIUM) s3.rs — validate part_number <= 10000 client-side and return a clear StorageError before exceeding; fix the 5 MiB → 8 MiB comment.
3. (MEDIUM) local.rs — write to a temp file and rename on success; clean up on error (remove_file).
4. (MEDIUM) local.rs — put should file.flush().await (and optionally sync_all) before returning Ok.
5. (MEDIUM) gcs.rs — implement resumable uploads to restore the streaming contract; map GCS 404 on delete to NotFound for cross-backend consistency.
6. (LOW) s3.rs:265 — handle missing ETag explicitly; traits.rs — consider StorageError::Io carrying the raw io::Error or an ErrorKind for retry heuristics.