fix: harden validation and error handling (audit #18-#34)#35
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 4 high |
🟢 Metrics 63 complexity · 6 duplication
Metric Results Complexity 63 Duplication 6
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
This PR addresses 16 audit findings (#18-#34) with consistent hardening across the workspace, including strict semver validation, fail-closed redaction logic, and GS1 URI constraints. While the changes are technically sound, the overall Codacy analysis is currently flagged as 'not up to standards' due to missing coverage requirements.
Key areas of concern include a significant complexity increase in crates/dpp-calc/src/co2e/calculator.rs (+15) and the introduction of non-deterministic system clock dependencies which may hinder future testing. The diff is exceptionally broad, and while no critical security flaws were introduced, the manual memory management in the plugin SDK requires an additional safety check to prevent potential invalid memory access on allocation failure.
About this PR
- This PR is exceptionally broad, addressing 16 distinct audit findings. While the consistency of the hardening logic is noted, the scale of the changes across the workspace makes verification complex. Future hardening passes should ideally be partitioned into smaller, more focused units.
Test suggestions
- Found recommended test scenario: Reject ruleset not yet in force in CO2e calculation
- Found recommended test scenario: Reject non-finite result overflow in CO2e calculation
- Found recommended test scenario: Return error for malformed nonce in keystore operations instead of panicking
- Found recommended test scenario: Reject duplicate primary key (AI 01) in Digital Link parsing
- Found recommended test scenario: Reject oversized AI values exceeding GS1 max lengths in Digital Link parsing
- Found recommended test scenario: Reject trailing unpaired AI segments in Digital Link parsing
- Found recommended test scenario: Preserve non-object payloads in JSON-LD framing instead of returning empty envelope
- Found recommended test scenario: Enforce strict semver and existence in version list during sector catalog registration
- Found recommended test scenario: Reject protected fields (status, signatures, etc) in passport patch operations
- Found recommended test scenario: Fail closed (return Null) on unknown sector data during redaction for non-confidential tiers
- Found recommended test scenario: Handle non-finite floats during JCS canonicalization for ruleset content hashing
- Found recommended test scenario: Safe handling of 32-bit ABI pointers and memory bounds on 64-bit hosts in plugin SDK
- Found recommended test scenario: Reject non-finite metrics during plugin result serialization via custom serializer
- Found recommended test scenario: Strict semver comparison for plugin schema version compatibility (no lexicographic fallback)
- Found recommended test scenario: Reject empty mandatory country codes in registry identifiers
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| const PROTECTED_PATCH_FIELDS: [&str; 12] = [ | ||
| "id", | ||
| "status", | ||
| "retentionLocked", | ||
| "retentionUntil", | ||
| "jwsSignature", | ||
| "publicJwsSignature", | ||
| "seal", | ||
| "version", | ||
| "publishedAt", | ||
| "createdAt", | ||
| "supersedesId", | ||
| "schemaVersion", | ||
| ]; |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: To ensure the PROTECTED_PATCH_FIELDS list stays in sync with the Passport struct in the future and prevents accidental exposure of immutable fields, consider adding a test that uses reflection or a macro to verify all metadata fields are included in this protected list.
| }; | ||
| // SAFETY: `layout` has non-zero size; a null return is handled by the host. | ||
| unsafe { mem_alloc(layout) as u32 } | ||
| let ptr = unsafe { mem_alloc(layout) } as usize; |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Manual memory allocation should be followed by an explicit null check. While the host environment manages the Wasm linear memory, a null return from the allocator in the guest will result in an immediate invalid memory access if used.
Consider refactoring the host_alloc function to explicitly check if mem_alloc returns a null pointer before performing the size check and casting to u32.
|
|
||
| // A signed, dated receipt must never be computed from a ruleset that is not | ||
| // legally in force today (crate-wide invariant; see repairability::calculate). | ||
| ruleset |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The calculation logic now depends on the system clock via Utc::now(), which increases function complexity and reduces testability. Consider passing the reference date as a parameter to calculate to allow deterministic testing of 'future' and 'expired' rulesets without clock manipulation.
…fier and facility to PROTECTED_PATCH_FIELDS
Summary
Hardening pass across
dpp-coreclosing the open audit findings (#18–#34). Every fix is defensive: malformed, adversarial, or out-of-bounds input that previously panicked, failed open, or silently mis-validated now fails closed with an explicit error. Each change ships with a regression test, and the fulljust checkgate is green.Fixes by crate
dpp-domain (#24) —
patch_fieldsno longer bypasses the passport state machine;redact()fails closed instead of leaking confidential fields when a field is unknown; schema validation and catalog registration reject malformed input; semver parsed viasemver::Version.dpp-crypto (#22) — keystore decrypt returns
Resulton a malformed/legacy on-disk nonce instead of panicking.dpp-rules (#21) — bundle verification returns an error on attacker-controlled non-finite JSON numbers instead of panicking; content hashing is now fallible.
dpp-digital-link (#23) — malformed/duplicate GS1 AI segments are rejected rather than silently mis-parsed; decoded values are length-capped.
dpp-registry (#27) — mandatory Article-13 identifier fields (operator name/country) can no longer be empty and still pass
validate().dpp-plugin-traits (#20) — the ABI result surfaces serialization errors instead of swallowing them; non-finite metrics become a hard serialization error; the version-range fallback no longer silently accepts non-semver strings.
dpp-plugin-sdk (#26) — raw ABI pointer math is guarded off
wasm32; an oversized allocation request returns an error instead of panicking the instance; addedoptional_range/optional_non_negativehelpers.dpp-calc (#19) — the CO2e ruleset gains an effective-date guard (
RulesetNotYetEffective), guards against silent overflow toInfinity, and corrects the contradictory expiry message.Sector plugins (#18, #28–#34) — furniture, aluminium, steel, electronics, detergent, tyre, textile, battery: each plugin now bounds-checks its metrics and validates its discriminating fields (
productionRoute,tyreClass, fibre-composition sum, repairability/CO2e bounds, biodegradable declaration, battery recycled-content scoping + chemistry-conflict) instead of falling through to the most permissive verdict.Testing
just checkgreen: fmt-check · clippy--workspace --all-targets -D warnings· debug-check · subjects-check · mod-rs-check ·cargo nextest --workspace· audit, plusRUSTDOCFLAGS="-D warnings" cargo doc.Note
Core issue #25 (dpp-evidence transfer/kid verification) is intentionally not closed here — its fix lives in the dpp-engine PR (
dpp-vaultverification module).Closes #18, #19, #20, #21, #22, #23, #24, #26, #27, #28, #29, #30, #31, #32, #33, #34