refactor(download)!: validate owned parameters before routing

Merged
JL
24d ago
+370-29

DownloadParams now rejects malformed owned media metadata before routing or copying it. Both constructors return Result<DownloadParams, DownloadPreparationError>, fields are private, and existing Downloadable getters provide read access. Custom Downloadable implementations receive early hash/type validation before media-connection IQ or HTTP; valid optional hashes, static URLs and raw transport key lengths retain their existing contracts.

Corrective report R03. #1654 was squash-merged as f1080df166bbc2c88e762eda6eb5bb7f6efc2f05. This PR now targets main with head c42dd44c59acff7825eae7d09e940a08c71d91a2 and only four R03 commits. The five-file diff has identical stable patch-id 0c3e126625dc9b3a63aa5313d84e18760f470fd4 before and after reconciliation; merged R01 commits are not duplicated.

Latest-main compatibility check, separate from the fixed-SHA performance experiment: #1659 merged as ee89f5ad9cf0553825f201f0e264ad32c4b73966. A read-only merge-tree with c42dd44c reproduces b9e11eec099f5db8cdead9105e1224dd90a30324, the exact combined tree for which its owner reported 46 download and 12 media-connection tests passing. Main subsequently advanced through #1657 to e14ddca2a6ba8823e82473a3b6a438cd82010e3b; a read-only merge-tree also succeeds, producing e48742219f64617d7781fe298f91ea21bdd484a5. That newer combined tree was subsequently tested by this lane: all 103 selected download/media-connection tests, six native consumer tests, four compile-fail controls and the WASM sdk,js consumer check passed. The external squash merge d804acaec1439e54c43c129e2f90d45016506abe has exactly this tree. No integration commit, rebase or push was made: local/remote head remains c42dd44c so the bounded performance experiment's target is unchanged. CI and performance results below remain attributed to c42dd44c with validated base f1080df1.

Migration:

  • Replace DownloadParams struct literals with encrypted(...) or plaintext(...).
  • Propagate or handle the constructor Result. Encrypted parameters require a 32-byte key and two 32-byte hashes; plaintext parameters require a 32-byte hash.
  • Use Downloadable getters instead of field access. There are no mutation setters or compatibility aliases.
  • MusicArtwork requires encryption. NewsletterMusicArtwork and ProductCatalogImage require plaintext. Plaintext image/video remains supported.
  • DownloadPreparationError is available from whatsapp_rust::download. Low-level DownloadRequest and MediaDecryption contracts remain unchanged.
  • All seven existing E2E constructor calls now propagate errors with ?. No workflow, dependency or credential changes.

Current-head validation:

  • All CI jobs are terminal. Build & Test, MSRV, default and all-features Clippy/build/tests, every feature-matrix job, native/MSRV consumers, the full WASM job, Miri, rustdoc, generated artifacts, formatting and Cargo Deny passed.
  • Runtime E2E: 168 tests passed, 11 skipped. All 20 media tests passed, including the seven migrated constructor calls, buffered/streaming round trips, large-file transfer, retained-route download after disconnect and media-connection reuse. Job: https://github.com/oxidezap/whatsapp-rust/actions/runs/37347655647/job/111890213011
  • Current-head reviews: Codex completed without major issues, Greptile passed, and CodeRabbit's full review of f1080df1..c42dd44c finished with no actionable findings. No open review threads. CodeRabbit's formal approval on ade74fa4 remains attributed to the pre-reconciliation head; Cubic did not provide a review; its current neutral check says the history rewrite did not trigger an automatic review.
  • The informational semver check retains the same 24/3/7 major-check categories against published 0.7.0. Separately, this PR intentionally introduces the SDK source breaks documented in Migration; they are not claimed to be pre-existing.
  • Before reconciliation, 4,265 core/SDK tests passed with four existing ignored probes; six native consumer tests, four compile-fail controls and WASM consumer compilation passed. Current CI results above cover the reconciled head.
  • Tests cover rejection before IQ/HTTP, optional static-URL metadata, preserved raw transport keys, valid plaintext types, private fields, native Send controls, WASM Rc/RefCell implementations, writer truncation/rewind, crypto cleanup and host fallback.

Footprint:

  • Native Binary Size passed against the exact f1080df1 baseline: stripped binary +896 B, .text +896 B, allocated sections +4,096 B, dependency count unchanged. The measured merge 544316b81492c5cc75cb3a4cd2db613a9ea0d79a has parents f1080df1 and c42dd44c.
  • The first size attempt failed because the exact baseline artifact was not yet available. Main's baseline job then passed, and only the failed PR size job was rerun successfully. No older baseline or override was used.
  • Prior isolated WASM probe against R01, before reconciliation: +1,039 total bytes, +921 noncustom bytes, +584 code and +336 data bytes with identical toolchain/lock/features. This is not a new paired measurement against f1080df1.

Performance investigation:

  • CodSpeed Performance Analysis remains neutral with two Simulation regressions in bench_session_with_self[false/true], -8.22%/-8.21%, one root-key-update improvement, 839 unchanged results and 12 skipped.
  • For false, modeled CPU time is 1,209.023 ns -> 1,317.356 ns. Instruction time is identical at 31.244865751 ns; the increase comes from modeled memory/cache penalties, localized in spec_eq and as_ref. The exact comparison attributes an AMD EPYC 7763 -> Intel Xeon Platinum 8573C CPU/vendor/flags mismatch to these Simulation rows. The libm linkage warning belongs to other Memory rows, not these session Simulation results; the earlier libm attribution was incorrect.
  • libsignal source, its benchmark, Cargo.lock and the workflow are unchanged by this PR. These observations do not prove harmlessness or walltime equivalence.
  • The existing proto-signal job was repeated on the same head without changing code, flags or gates. It uploaded results at 18:18:21 UTC; the report updated at 18:18:36 UTC still reports the same two alerts. No acknowledgment or waiver was applied.
  • Comparison: https://app.codspeed.io/oxidezap/whatsapp-rust/runs/compare/6ac3de2b405e81fc16a75dea..6ac3de21405e81fc16a75de9
  • The bounded original-benchmark walltime diagnostic in #1655 is complete. Its owner reports fixed source checkouts f1080df1 and c42dd44c on matching AMD EPYC 7763 runners and eight byte-identical benchmark executables. CodSpeed associates these diagnostic uploads with workflow commit 4667b45; that metadata is not the measured source revision.
  • Independently checked diagnostic results: non-self is 27 ns for both sources; self is 26 ns and 24 ns in the two base repeats versus 25 ns at head. Both comparisons classify the two measured cases as unchanged and report no environment differences. Each skips 860 other cases.
  • Comparisons: https://app.codspeed.io/oxidezap/whatsapp-rust/runs/compare/6ac3ef84b0348977a25e4a6b..6ac3f04ca913657ad09b1be8 and https://app.codspeed.io/oxidezap/whatsapp-rust/runs/compare/6ac3ef5db0348977a25e4a68..6ac3f04ca913657ad09b1be8
  • This bounded evidence does not establish performance equivalence, rule out every regression, or measure the download path. The original Simulation alerts remain visible. Following the benchmark owner's recommendation and the coordinating user's authorization, they no longer independently block readiness. No CodSpeed acknowledgment, threshold change or production gate modification was made.
  • Readiness recheck: all 41 current-head checks remain terminal, with 36 success, 2 neutral, 2 skipped and the informational semver failure. Current-head reviews have no outstanding findings, all threads are resolved, and a fresh read-only merge-tree against main e14ddca2 reproduces e48742219f64617d7781fe298f91ea21bdd484a5 without conflicts. The subsequent integration validation below covers that exact combined tree. Head c42dd44c and the original CI/performance attribution remain unchanged.

Protocol evidence uses the verified WhatsApp Web 2.3000.1047483476 bundle set, whatspec 1f5167c4cebf263edaf001e06a2497f213c55793. The original key generator creates 32-byte keys, while the raw HKDF helper does not impose that length; this change enforces the stronger owned-container invariant without narrowing raw transport behavior.

Post-transition validation:

  • Tested tree e48742219f64617d7781fe298f91ea21bdd484a5 combines c42dd44c with main e14ddca2. GitHub subsequently confirmed an external merge at 19:06:29 UTC as d804acaec1439e54c43c129e2f90d45016506abe; its tree is byte-for-byte the tested tree. No merge was performed by this lane.
  • cargo nextest run --locked -p whatsapp-rust -p wacore --lib -E 'test(download::) | test(mediaconn::)': 103 passed, comprising 46 SDK download, 12 SDK media-connection, 39 core download and six core media-connection IQ tests.
  • Existing isolated api-consumer with native: six download tests and all four relevant compile-fail doctests passed. Native Send checks remain in the compiled module. WASM wasm32-unknown-unknown with sdk,js passed, including Rc/RefCell media/writer probes.
  • The WASM check emitted two existing unused-qualification warnings in request.rs and upload.rs, whose contents are unchanged between c42dd44c and e14ddca2. No warning suppression or source edit was made.
  • The ready-triggered Codex code review completed on c42dd44c at 19:08:27 UTC without new findings. A separate security-review attempt was blocked by account usage limits and is not counted as completed coverage. CodeRabbit still records the completed five-file f1080df1..c42dd44c review with no actionable findings; no new formal approval or distinct post-ready review is claimed. All inline threads are resolved. Greptile passed; Cubic supplied no completed review.
  • No published source/head change was needed. Fixed-head CI and performance evidence stays attributed to c42dd44c versus f1080df1, separately from these integration tests. No download-path performance measurement or new full combined-tree CI/E2E run is claimed.