Skip to content

fix(dapi): classify malformed protobuf requests as invalid arguments - #5351

Open
shumkov wants to merge 1 commit into
v5.0-devfrom
shumkov/audit-v5-l10-protobuf-request-errors
Open

shumkov wants to merge 1 commit into
v5.0-devfrom
shumkov/audit-v5-l10-protobuf-request-errors

Conversation

@shumkov

@shumkov shumkov commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Basic explanation

What this does: Return INVALID_ARGUMENT when a caller supplies undecodable protobuf in an otherwise accepted gRPC message. Corrupt node responses still return INTERNAL.

Value: Callers receive a request error, and the existing SDK stops without retrying or banning a healthy node. No SDK policy change is needed.

Risks: The externally visible request status changes deliberately. Generated clients, message types and descriptors retain their existing output; deterministic transport and proof controls cover the direction boundary. No consensus or stored-state change.

Issue being fixed or feature implemented

The Sakura audit reports malformed protobuf as INTERNAL. This is gRPC's library convention; this PR deliberately applies the caller-error policy at the server request decoder. Reconstructed malformed inputs reproduce the reported status locally; the original private campaign scripts are unavailable.

What was done?

  • Add a private server request codec that maps only a typed Prost decode error to INVALID_ARGUMENT, retaining its description and the existing response encoder.
  • Generate native clients and servers with separate codec selections in the existing output layout. Client response decoding stays on ProstCodec, including DAPI's Drive client when server features are enabled.
  • Add offline transport fixtures for Core unary/streaming, DriveInternal, real DAPI forwarding and real Drive queries. The narrow offline construction seams are cfg(test) only.
  • Keep framing/compression errors, size caps, application statuses, unsupported-version behavior and proof verification unchanged. The lockfile adds required build/test dependency edges without upgrading packages.

In-place changes to shipped generations

This boundary runs before a request reaches a query or state-transition handler. Undecodable outer protobuf cannot affect block execution; valid requests, application state, replay inputs, proof format and verified roots are unchanged. Native Client and WASM generation are untouched, so no new consensus generation or protocol table is required. Older binaries retain the old status; this PR does not establish deployment or historical repair.

How Has This Been Tested?

Actual red→green on identical regression files before/after the decoder change: generated services 1 failed / 2 passed → 3 passed, real DAPI forwarding 2 failed / 2 passed → 4 passed, real Drive 1 failed / 1 passed → 2 passed. The passing baseline controls include real corrupt-response bytes, valid calls, original-query proofs and exact stored roots. Final test cleanup only removes an unused import, adds the Core feature gate and factors the stream type.

  • cargo test -p dapi-grpc --all-features --tests --locked -j 2: malformed tags/varints/lengths/UTF-8, no handler invocation, stream refusal/termination, unknown fields, framing/compression and cap controls.
  • cargo test -p rs-dapi --lib services::platform_service:: --locked -j 2: 74 passing tests on parent b49f382949, including all four regressions and neighboring error mappings. After safely fast-forwarding to ecfc96d44a, the focused services::platform_service::protobuf_request_errors selection passed all four again. Actual DAPI receives raw malformed caller messages and actual corrupt Drive response bytes. Executor retry/ban controls replay the exact served status through the real DapiClient; they are not a second SDK wire-transport test.
  • cargo test -p drive-abci --test protobuf_request_errors --test query_request_errors --locked -j 2: five passing tests, including present/absent canonical data, original-query proof/root verification and caller/node-failure controls.
  • On parent b49f382949: native server/client/transport/serde/mocks and wasm32 client generation checks; native DAPI/Drive/DAPI-client/Rust-SDK and wasm32 WASM-SDK consumer checks. Upstream advancement left these owned boundaries unchanged. The WASM-SDK check uses target-specific installed LLVM CC/AR through sccache; the initial Apple-clang attempt lacked a wasm32 target.
  • Generated output comparison: native server files differ only at codec selection; client/message/descriptor output remains identical. Native client 7/7 and wasm32 target 14/14 files are byte-identical to baseline.
  • Scoped rustfmt and Clippy checks with warnings denied. Core's final three tests, Drive's five tests and scoped Drive Clippy also passed on ecfc96d44a. CI provides the pinned cargo-machete tool, which is unavailable locally, and checks all targets on the clean PR head.

All transport tests use in-memory duplexes and deterministic fixtures, with no live network requests.

Breaking Changes

No schema or API-shape changes. Malformed caller protobuf intentionally changes from INTERNAL to INVALID_ARGUMENT; node-failure statuses retain their existing retry semantics.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed
  • If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Summary by CodeRabbit

  • Bug Fixes
    • Malformed protobuf requests now return an InvalidArgument error without being processed or changing Drive state.
    • Invalid caller requests no longer trigger retries or node bans. Certain Drive response errors continue to trigger node failover.
    • Valid requests, including those with unknown fields, continue to work as expected.
  • Tests
    • Added coverage for malformed requests, request size limits, response errors, streaming requests, and follow-up queries.

PR Hygiene · 1b34e74

  • Bots — coderabbitai ✓ · thepastaclaw ✓
  • Self-review — post /self-reviewed
  • Reviewer requests paused — your 5 review slots are occupied; this PR is excluded from reviewers' queues. Required approvals still count without a slot.
  • Build green
  • Approvals
    • files with no dedicated owner — you own it
    • rust-dapi (packages/rs-dapi/Cargo.toml, packages/rs-dapi/src/clients/drive_client.rs, packages/rs-dapi/src/clients/tenderdash_client.rs and 2 more) — QuantumExplorer or lklimek
    • rs-drive-abci — you own it

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

Map only typed Prost decode errors at generated server request boundaries. Keep generated client response decoding, framing/compression statuses, message caps and response encoding unchanged, including DAPI's server-feature Drive client. Valid application requests, stored roots and proofs retain their behavior; this transport correction does not affect consensus or replay.

Test would have caught this in CI: ✖ before fix, ✔ after.
Observed runtime RED→GREEN on identical raw-transport regression files: Core/DriveInternal 1 failed and 2 passed→3 passed, real DAPI forwarding 2 failed and 2 passed→4 passed, real Drive 1 failed and 1 passed→2 passed. Final tests and neighboring query-error controls pass on current upstream. Generated client/messages/descriptors remain identical; native and wasm SDK consumers compile.
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: dashpay/platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 11ee7b21-c836-4055-ad6f-7154ae7d8870
📥 Commits

Reviewing files that changed from the base of the PR and between ecfc96d and 1b34e74.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (12)
  • packages/dapi-grpc/Cargo.toml
  • packages/dapi-grpc/build.rs
  • packages/dapi-grpc/src/lib.rs
  • packages/dapi-grpc/src/request_codec.rs
  • packages/dapi-grpc/tests/protobuf_request_errors.rs
  • packages/rs-dapi/Cargo.toml
  • packages/rs-dapi/src/clients/drive_client.rs
  • packages/rs-dapi/src/clients/tenderdash_client.rs
  • packages/rs-dapi/src/services/platform_service/mod.rs
  • packages/rs-dapi/src/services/platform_service/protobuf_request_errors.rs
  • packages/rs-drive-abci/Cargo.toml
  • packages/rs-drive-abci/tests/protobuf_request_errors.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The server build now uses a custom Prost request decoder that maps protobuf decode failures to INVALID_ARGUMENT. New in-memory tests cover gRPC routes, DAPI forwarding and retry behavior, and Drive query responses for malformed and valid requests.

Changes

Protobuf Request Decode Errors

Layer / File(s) Summary
Server request codec and gRPC coverage
packages/dapi-grpc/Cargo.toml, packages/dapi-grpc/build.rs, packages/dapi-grpc/src/lib.rs, packages/dapi-grpc/src/request_codec.rs, packages/dapi-grpc/tests/protobuf_request_errors.rs
Server generation uses a custom Prost request decoder that maps protobuf decode failures to INVALID_ARGUMENT. In-memory tests cover malformed and valid Core and DriveInternal requests, as well as malformed frames, encodings, and oversized messages.
DAPI forwarding and retry tests
packages/rs-dapi/Cargo.toml, packages/rs-dapi/src/clients/*, packages/rs-dapi/src/services/platform_service/*
An in-memory test harness exercises the Platform service and a fake Drive endpoint. Tests cover malformed caller requests, valid forwarding, request-size limits, corrupt Drive responses, Drive gRPC faults, and retry behavior.
Drive query request tests
packages/rs-drive-abci/Cargo.toml, packages/rs-drive-abci/tests/protobuf_request_errors.rs
In-memory Drive query tests cover malformed protobuf payloads, unknown fields, version errors, stored contract results, proofs, and unchanged Drive roots.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant RawGrpcClient
  participant CoreGrpcService
  participant RequestDecoder
  participant CoreHandler
  RawGrpcClient->>CoreGrpcService: Send request payload
  CoreGrpcService->>RequestDecoder: Decode protobuf payload
  alt Payload is malformed
    RequestDecoder-->>CoreGrpcService: Return INVALID_ARGUMENT
    CoreGrpcService-->>RawGrpcClient: Return decode error
  else Payload is valid
    RequestDecoder-->>CoreGrpcService: Return decoded request
    CoreGrpcService->>CoreHandler: Invoke handler
  end
Loading

Suggested reviewers: quantumexplorer

Merge Risk: ⚪ Minimal · up to 1b34e

Malformed protobuf requests now return an invalid-argument error rather than an internal error, so callers can avoid retrying or banning healthy nodes. The rest of the change is tests, and no merge-blocking risk is evident.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 22.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 62 functions across 9 files. (3 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: mapping malformed protobuf requests to invalid arguments in DAPI.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 22.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 62 functions across 9 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@shumkov
shumkov marked this pull request as ready for review October 8, 2026 21:58
@thepastaclaw

thepastaclaw commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit 1b34e74) · triage: normal

@github-actions github-actions Bot added this to the v5.0.0 milestone Oct 8, 2026
@shumkov

shumkov commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@shumkov

shumkov commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

The docstring-coverage warning counts private test fixtures and trait implementations. This change adds no public API; the codec module and generation comments explain the request/response boundary, and the offline construction helpers already have documentation. Following the surrounding repository conventions, no boilerplate comments were added solely to raise that ratio. The advisory warning remains; it is not reported as passed.

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added the waiting-bots Waiting for the review bots to report on this head label Oct 8, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 1 + Phase 2

The server request codec maps Prost payload-decoding failures to INVALID_ARGUMENT while preserving the existing response encoder and client response decoding. Static inspection of the exact PR head confirms the transport boundary and regression fixtures support the stated goal; no actionable in-scope findings remain. No builds or tests were run in this lane, and the supplied CI snapshot shows Rust workspace tests skipped with other checks still queued, pending, or in progress.

Review provenance

Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: normal by gpt-6.1-sol (effort low) — The production change is a contained server-side protobuf error mapping with direction-specific code generation, not a large or intricate change to network deserialization despite the extensive regression tests.
  • Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — rust-quality (completed, effort high); agent phase1-reviewer
  • Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 81% left, 5h 80% left
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 8, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-self-review Waiting for the author to post /self-reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants