Skip to content

feat(parser_app): verify gateway-signed payment markers - #448

Open
pepe-anchor wants to merge 12 commits into
mainfrom
pepefigueira/prs-581-04a-payment-verify
Open

feat(parser_app): verify gateway-signed payment markers#448
pepe-anchor wants to merge 12 commits into
mainfrom
pepefigueira/prs-581-04a-payment-verify

Conversation

@pepe-anchor

@pepe-anchor pepe-anchor commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Why

parser_app is the enclave half of the x402 trust pair. Something outside this repo takes the payment and signs a VerifiedPaymentMarker; the enclave's job is to refuse to parse unless that marker was signed by the pinned gateway key and is bound to this exact request. Without the second half, a marker paid for one parse can be replayed against a different transaction.

Nothing here turns verification on. Every call site passes PaymentPolicy::Disabled, so behaviour is unchanged by this PR. What it does is put the enforcement point in place and settle two things that are free now and expensive later:

The wire number. ParseRequest field 4 shipped on main as include_intermediate_output (#414), while payment_marker was still claiming 4 on the unmerged #304. It takes 5 here. Renumbering an undeployed field costs nothing. Renumbering a deployed one costs a coordinated rollout.

The signed preimage. request_hash is what binds a marker to a request. Getting the set of bound fields right before anything signs against it is free. Changing it afterwards invalidates every marker in flight.

Provenance

The verifier is ported from the unmerged #304 onto current main, not rebased. #304 is stacked on #295, which is closed, so its base is a dead branch and it sits 132 commits behind main. Splitting it into reviewable pieces was the only way forward. Original implementation by @prasanna-anchorage on #304; this PR supersedes the payment_verify and payment_marker portions of it.

Tracked as PRS-581. Based on main, independent of the pivot PRs.

What

  • ParseRequest.payment_marker takes field 5, leaving include_intermediate_output = 4 untouched.
  • host_primitives::payment_marker: the VerifiedPaymentMarker / SignedVerifiedPaymentMarker types, VPM_VERSION, and request_hash. Borsh-encoded, because the encoding has to match wire-for-wire between whatever signs a marker and the enclave that verifies it.
  • parser_app::payment_verify: PaymentPolicy (Disabled or Required) plus the signature and request-binding check, run as the first thing parse() does.
  • parse() gains a &PaymentPolicy parameter. Callers updated: parser_app::service, parser_grpc_server::main (both pass &PaymentPolicy::Disabled), and parser_gateway::main picks up the new struct-literal field.

PaymentPolicy::from_env from #304 was dropped on purpose. This repo cannot set env vars in tests (edition 2024 plus forbid(unsafe) bans std::env::set_var), and the real binaries take config from CLI args. from_hex is the only constructor.

What request_hash covers, and what it doesn't

The preimage is:

SHA256( int32_LE(chain)
      || u32_LE(len(unsigned_payload))  || unsigned_payload
      || u32_LE(len(chain_metadata))    || chain_metadata
      || u8(include_intermediate_output) )

chain_metadata is the borsh encoding of the inner ChainMetadata, not the Option wrapper, so there is no discriminant byte.

What the binding gives you: a marker cannot be moved to a different request. All four inputs change what the enclave actually attests, so covering fewer of them would let a marker paid for one parse authorise another just by varying metadata.

What it does not give you: it does not make a marker single-use. The enclave holds no state and does not track redemption, so preventing the same payment from being spent twice belongs to whatever signs the markers, not here. Worth stating explicitly so nobody reads request_hash as a replay defence it isn't.

The length prefixes matter on their own. Without them the preimage is a bare concatenation, so ("AB", []) and ("A", [b'B']) hash identically and a marker bought for one request verifies against the other.

Prerequisite before PaymentPolicy::Required

The signer is not in this repo. The only non-test caller of request_hash here is the verifier, so the preimage is a contract with an external component, and that component has to move to the identical 4-field length-prefixed form before policy can flip to Required. Otherwise every real marker fails the binding check and paid parses break.

That is a rollout prerequisite, not a blocker for this PR, and it is exactly why the preimage moves now: nothing verifies markers today, so the change is free. Same argument as the field-5 renumbering above.

Hardening pass (92ed4c48)

The pre-push review loop found three real defects in the verification path this PR introduces. Fixed in a follow-up commit rather than folded in, so the original feature commit stays reviewable on its own.

request_hash under-bound the request. It covered only (chain, unsigned_payload). Now all four fields, length-prefixed, per the layout above.

signing_digest() had an unsound fallback. It swallowed borsh failures with unwrap_or_default(), so the digest became sha256(empty) on that path. One signature over that fixed fallback would verify against any marker that also failed to serialise. It returns Result now and the caller propagates.

The pinned key never matched a 0x-prefixed config value. pinned_hex_lower came from the raw config string while the signer emits unprefixed hex, so a 0x-prefixed GATEWAY_SIGNING_PUBKEY_HEX rejected every request. It is derived from the decoded key now, and decoding goes through visualsign::encodings::decode_hex per the repo's hex-handling rule (which also closes a 0X gap, since qos_hex::decode strips lowercase only).

Also drops the subtle dependency (both sides of that compare are public keys, not secrets), dedups the metadata-encoding and test-setup helpers, and corrects docs that claimed parser_app verifies x_payment_hash. It does not, and no proto field carries those bytes yet.

Rebase note (2026-08-25)

The NEAR series landed on main while this sat open, which made the field-5 argument concrete rather than hypothetical. NearMetadata near = 3 and the NEAR codegen attributes now merge alongside payment_marker = 5, with include_intermediate_output = 4 untouched.

The only conflict was src/generated/src/generated/descriptor.bin, which is generated. Resolved by re-running make -C src generated against the merged proto, not by hand-picking a side. Main's NEAR tests also added five new ParseRequest literals in integration/tests/parser.rs that predate field 5, so the branch stopped compiling; all five now pass payment_marker: vec![], matching the six sites the feature commit already updated. Folded into the same commit, since a commit that adds a field has to leave its call sites compiling.

Test evidence

make -C src lint       -> exit 0, 0 warnings
cargo fmt -- --check   -> clean
make -C src generated  -> no diff (codegen is deterministic)
make -C src test       -> 39 suites, 1617 passed, 0 failed

New regression tests:

  • replay with a different chain_metadata
  • replay with a toggled include_intermediate_output
  • a forged marker claiming the pinned key but signed by another keypair, covered at both the payment_verify layer and the public parse() entry point
  • a preimage boundary-collision test

That last one was added because a review pass showed the length-prefix fix was not pinned by anything: reverting the fix left all 12 existing tests green. Verified by mutation, reverting the length prefixes now fails request_hash_length_prefixes_defeat_field_boundary_collisions and only that test.

Rollback

Revert the commits and re-run make -C src generated. The proto field is additive and unused by any deployed caller, and PaymentPolicy is Disabled everywhere, so nothing on the wire or in behaviour depends on it yet.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds gateway-signed payment marker verification infrastructure while preserving existing behavior with payment enforcement disabled.

Changes:

  • Adds Borsh payment marker types, request hashing, and P-256 verification.
  • Adds ParseRequest.payment_marker as protobuf field 5.
  • Updates parser callers, generated code, dependencies, and tests.

Reviewed changes

Copilot reviewed 12 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
proto/parser/parser.proto Defines payment marker field 5.
src/generated/src/generated/parser.rs Regenerates ParseRequest.
src/host_primitives/src/payment_marker.rs Adds marker wire types and hashing.
src/host_primitives/src/lib.rs Exports payment marker module.
src/host_primitives/Cargo.toml Adds marker dependencies.
src/parser/app/src/payment_verify.rs Implements payment policy and verification.
src/parser/app/src/routes/parse.rs Verifies markers before parsing.
src/parser/app/src/service.rs Passes the disabled policy.
src/parser/app/src/lib.rs Exports verification module.
src/parser/app/Cargo.toml Adds verification dependencies.
src/parser/grpc-server/src/main.rs Passes the disabled policy.
src/parser/gateway/src/main.rs Initializes the new request field.
src/integration/tests/parser.rs Updates request fixtures.
src/Cargo.lock Records dependency changes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/host_primitives/src/payment_marker.rs Outdated
Comment thread src/parser/app/src/payment_verify.rs Outdated
Comment thread src/host_primitives/src/payment_marker.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 15 changed files in this pull request and generated 2 comments.

Comment thread src/host_primitives/src/payment_marker.rs Outdated
Comment thread src/host_primitives/src/payment_marker.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

proto/parser/parser.proto:59

  • The documented wire type is incorrect: the verifier deserializes SignedVerifiedPaymentMarker, not the inner VerifiedPaymentMarker. An external gateway following this proto comment would omit the signature wrapper and every marker would fail decoding. Name the wrapper explicitly here; regenerated Rust documentation will then match the actual contract.
  // Borsh-encoded VerifiedPaymentMarker signed by the gateway. parser_app

src/parser/app/src/routes/parse.rs:469

  • These function-local imports deviate from this test module's established top-level import layout (routes/parse.rs:163-173; see also payment_verify.rs:190-193). Move both imports to the top of the test module so test dependencies remain centralized.
        use host_primitives::payment_marker::{
            SignedVerifiedPaymentMarker, VPM_VERSION, VerifiedPaymentMarker, request_hash,
        };
        use qos_p256::sign::P256SignPair;

src/parser/app/src/payment_verify.rs:76

  • The policy tests only construct unprefixed keys via qos_hex::encode, so the newly supported 0x/0X input path is not protected against regression. Add a from_hex test using a prefixed generated public key and verify that it accepts a valid marker.
        let trimmed = hex_value.trim();
        let bytes = decode_hex(trimmed).map_err(|e| {

@pepe-anchor
pepe-anchor marked this pull request as ready for review August 25, 2026 17:47
serde::Serialize,
serde::Deserialize,
)]
pub struct VerifiedPaymentMarker {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we group these together in x402 crate or something so that we know that this is

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Your comment cuts off at "so that we know that this is", answering what I think you meant.

Skipping the crate for now. Once the settlement fields moved behind PaymentDetails the envelope is scheme-agnostic on purpose, so x402 is the wrong name for it, and there's not much to gather anyway: one file, one consumer (parser_app), and #449's x402 code shares nothing with it.

Happy to split it out the moment there's a second consumer, a Rust signer or a golden-vector harness for the Go side. If you meant a payment module rather than a crate, I'll do that here.

Comment thread src/host_primitives/src/payment_marker.rs
Comment thread src/parser/app/src/payment_verify.rs
pepe-anchor added a commit that referenced this pull request Aug 26, 2026
…st exhaustively

Two review suggestions on #448.

The settlement fields (txid, payer, pay_to, amount, mint, x_payment_hash,
network) move behind a PaymentDetails enum with a single X402Direct variant.
verify() never reads any of them, so this is a schema change with no logic
change, and a second payment scheme becomes a sibling variant instead of a
breaking restructure of a flat struct. Doing it now is free: PaymentPolicy is
Disabled everywhere, so no marker in existence depends on the current layout.

It is still a wire change. Borsh prefixes an enum with a 1-byte LE variant
index, so borsh(vpm) and therefore signing_digest() both shift, and the
gateway-side signer has to write that byte. x402_direct_is_borsh_variant_zero
pins the index so appending a variant cannot silently re-map it.

verify() now destructures ParseRequest instead of dot-accessing it. It is a
plain generated struct with no non_exhaustive, so a new proto field fails the
build (E0027, verified with a canary field) until someone decides whether it
belongs in request_hash. Field 4 was already missed once while this work sat on
an unmerged branch; this turns review diligence into a compile error. It only
guards the verifier, so the comment says the signer has to move too.

Co-Authored-By: Claude <noreply@anthropic.com>
@pepe-anchor
pepe-anchor requested review from prasanna-anchorage and a balanced review from Copilot August 26, 2026 09:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 15 changed files in this pull request and generated 2 comments.

Suppressed comments (6)

Previously missed (4) — in code that hasn't changed since the last review.

src/parser/app/src/payment_verify.rs:34

  • This says the policy is loaded from GATEWAY_SIGNING_PUBKEY_HEX, but this PR deliberately removed env-coupled loading and from_hex accepts configuration chosen by the embedding binary. Keeping the old env-specific contract can mislead future callers wiring Required.
/// Whether `parser_app` requires (and verifies) a `VerifiedPaymentMarker`
/// on every parse call. Loaded once at startup from
/// `GATEWAY_SIGNING_PUBKEY_HEX`.

src/parser/app/src/routes/parse.rs:473

  • Test imports are kept at the test module scope in this file (see lines 163-169), rather than inside individual test functions. Move this import there or qualify these two calls so the new test follows that convention.
        use qos_p256::sign::P256SignPair;

src/host_primitives/src/payment_marker.rs:1

  • The repository keeps source text ASCII-only for terminal compatibility; replace this em dash with an ASCII hyphen.

This issue also appears on line 8 of the same file.

//! VerifiedPaymentMarker — the signed proof the gateway hands to parser_app

src/parser/app/src/payment_verify.rs:385

  • The repository keeps source text ASCII-only for terminal compatibility; replace this em dash with an ASCII hyphen.
        // Flip the last byte (inside the signature region — the signature

src/host_primitives/src/payment_marker.rs:229

  • The “stable” assertion invokes request_hash twice, so it does not pin the externally consumed preimage contract; changing field order, endianness, or framing would leave all these sensitivity tests green while breaking every issued marker. Add a fixed cross-implementation vector covering non-empty payload and metadata plus the flag.
    fn request_hash_is_stable_and_sensitive_to_every_bound_field() {
        let h1 = request_hash(1, "0xdeadbeef", &[], false);
        let h2 = request_hash(1, "0xdeadbeef", &[], false);

src/host_primitives/src/payment_marker.rs:8

  • The repository keeps source text ASCII-only for terminal compatibility; replace this em dash with an ASCII hyphen.
//! bytes — no schema drift.

Comment thread src/host_primitives/src/payment_marker.rs
Comment thread src/host_primitives/src/payment_marker.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

proto/parser/parser.proto:63

  • This comment implies payment_marker is always verified, but every runtime caller currently supplies PaymentPolicy::Disabled, which returns without reading the field. Qualify verification as conditional on PaymentPolicy::Required so API consumers do not mistake marker presence for validation.
  // Borsh-encoded VerifiedPaymentMarker signed by the gateway. parser_app
  // verifies this before processing. Empty for the open v1 routes and for

src/parser/app/src/payment_verify.rs:34

  • This says the policy is loaded from GATEWAY_SIGNING_PUBKEY_HEX, but this module deliberately has no environment-backed constructor and all current callers pass Disabled. Describe Required as being constructed by caller-owned startup configuration instead, consistent with from_hex and the module documentation.
/// Whether `parser_app` requires (and verifies) a `VerifiedPaymentMarker`
/// on every parse call. Loaded once at startup from
/// `GATEWAY_SIGNING_PUBKEY_HEX`.

pepe-anchor and others added 4 commits August 26, 2026 16:17
Enclave-side half of the x402 trust pair: parser_app checks that a
VerifiedPaymentMarker was signed by the pinned gateway key and is bound
to this exact request, so a paid parse cannot be replayed against a
different transaction.

payment_marker takes proto field 5. Field 4 shipped as
include_intermediate_output (#414) while this work sat on an unmerged
branch; renumbering here is free, renumbering after deploy is not.

Policy defaults to Disabled everywhere in this PR, so no caller changes
behavior yet.

Co-Authored-By: Claude <noreply@anthropic.com>
Three real defects in the verification path, all surfaced by the pre-push
review loop.

request_hash only covered (chain, unsigned_payload). chain_metadata and
include_intermediate_output both change what the enclave attests, so a
marker paid for one parse would verify against a different one just by
varying metadata. The hash now covers all four fields, and every
variable-length field is length-prefixed so shifting a byte across the
payload/metadata boundary cannot produce the same preimage.

signing_digest() swallowed borsh failures via unwrap_or_default(), making
the digest sha256(empty) on that path. One signature over that fixed
fallback would then verify against any marker that also failed to
serialize. It returns Result now, and the caller propagates.

pinned_hex_lower came from the raw config string, so a 0x-prefixed
GATEWAY_SIGNING_PUBKEY_HEX would never match the gateway's unprefixed
qos_hex::encode output and every request would be rejected. It is derived
from the decoded key now, so operator formatting does not matter.

Also drops the subtle dependency (both sides of that compare are public
keys, not secrets), dedups the metadata-encoding and test-setup helpers,
and corrects docs that claimed parser_app checks x_payment_hash. It does
not, and no proto field carries the X-PAYMENT bytes yet.

The request_hash preimage is a contract shared with the x402 gateway,
which lives outside this repo. Nothing verifies markers today because
PaymentPolicy is Disabled at every call site, so changing the preimage
costs nothing right now. It stops being free once policy flips to
Required, which is why it moves here.

Co-Authored-By: Claude <noreply@anthropic.com>
Generated by /finish P3 iteration 0.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated by /finish P3 iteration 1.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pepe-anchor and others added 2 commits August 26, 2026 16:17
…st exhaustively

Two review suggestions on #448.

The settlement fields (txid, payer, pay_to, amount, mint, x_payment_hash,
network) move behind a PaymentDetails enum with a single X402Direct variant.
verify() never reads any of them, so this is a schema change with no logic
change, and a second payment scheme becomes a sibling variant instead of a
breaking restructure of a flat struct. Doing it now is free: PaymentPolicy is
Disabled everywhere, so no marker in existence depends on the current layout.

It is still a wire change. Borsh prefixes an enum with a 1-byte LE variant
index, so borsh(vpm) and therefore signing_digest() both shift, and the
gateway-side signer has to write that byte. x402_direct_is_borsh_variant_zero
pins the index so appending a variant cannot silently re-map it.

verify() now destructures ParseRequest instead of dot-accessing it. It is a
plain generated struct with no non_exhaustive, so a new proto field fails the
build (E0027, verified with a canary field) until someone decides whether it
belongs in request_hash. Field 4 was already missed once while this work sat on
an unmerged branch; this turns review diligence into a compile error. It only
guards the verifier, so the comment says the signer has to move too.

Co-Authored-By: Claude <noreply@anthropic.com>
Generated by /finish P3 iteration 0.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pepe-anchor
pepe-anchor force-pushed the pepefigueira/prs-581-04a-payment-verify branch from 5cec551 to bcb8110 Compare August 26, 2026 16:21
@pepe-anchor
pepe-anchor marked this pull request as draft August 26, 2026 16:49
Comment thread src/parser/app/src/payment_verify.rs Outdated
Comment thread src/host_primitives/src/payment_marker.rs
@pepe-anchor
pepe-anchor marked this pull request as ready for review August 27, 2026 10:07
pepe-anchor and others added 3 commits August 27, 2026 10:07
…equest_hash

Three things Shahan flagged on #448, all cheap to close now and awkward
to close after an external signer exists.

The gateway pubkey cross-check compared hex strings with
eq_ignore_ascii_case, which handles case but not a 0x prefix. The field
doc told the signer it MUST equal the pinned GATEWAY_SIGNING_PUBKEY_HEX,
and from_hex accepts that value with a prefix, so a signer following the
doc literally produced PinnedKeyMismatch on every request with a valid
P256 signature underneath. Now both sides decode and the bytes are
compared, so the prefix and case rules match on both ends. That leaves
pinned_hex_lower for log and Debug output only.

Decode and UnsupportedVersion no longer map to FailedPrecondition. The
gateway turns that into HTTP 402, so a truncated marker or a schema skew
told the caller to pay again for a request that can never succeed however
many times it retries. Both are caller or deployment bugs, so they map to
InvalidArgument. The match is exhaustive by variant, so a new variant
forces the 402-or-not decision at compile time.

request_hash had no golden vector. Both existing tests are differential,
so reordering the four writes, switching to_le_bytes to to_be_bytes, or
dropping the chain write entirely left them green while every marker from
the gateway signer stopped verifying. The new literal was produced by
hand-assembling the preimage and hashing it outside this crate, so the
test passing is independent evidence that the preimage matches its doc
rather than a capture of whatever the code emits.

Also documents why settled_at_ms has no max-age check. request_hash binds
a marker to one exact request and parse is read-only, so a replay only
re-runs the identical parse. That is repeat service on an already-paid
request, not a bypass, and it doesn't justify making enclave verification
depend on signer/enclave clock skew.

Co-Authored-By: Claude <noreply@anthropic.com>
…ire type

The field comment said "Borsh-encoded VerifiedPaymentMarker", but
parser_app deserializes the field as SignedVerifiedPaymentMarker, the
wrapper carrying the marker plus the gateway's signature.

That comment is the only wire-format spec an out-of-repo signer sees. A
team implementing to it literally would emit borsh(VerifiedPaymentMarker)
with no signature envelope, and every marker they produced would fail
try_from_slice on the enclave side. Total integration failure for the one
thing this PR exists to enable, from a doc typo.

Says the wrapper is what goes on the wire, and says explicitly that the
bare inner struct is the wrong thing to send. Regenerated so the mirrored
Rust doc carries it too, since that is what a Rust consumer reads.

Co-Authored-By: Claude <noreply@anthropic.com>
…t, pin the envelope

Review follow-ups on the verification path.

A gateway_pubkey_hex that is not valid hex was returning PinnedKeyMismatch,
which maps to FailedPrecondition and becomes HTTP 402. That tells the caller
to pay again for a marker whose key field is corrupt, which no amount of
paying can fix. It takes the Decode path (InvalidArgument) now, alongside the
other malformed-marker cases. The underlying hex error is deliberately not
echoed into the message: it embeds the offending character, and the field is
attacker-controlled and unauthenticated at that point, since the signature
has not been checked yet.

SignedVerifiedPaymentMarker had no byte-level test, only VerifiedPaymentMarker
did. The envelope is equally part of the contract with the external signer, so
a field swap, or a signer writing vpm_bytes || sig[64] with no length prefix
on the signature, passed every round-trip test while producing bytes the
enclave cannot deserialize. The new test hand-assembles the expected layout
rather than capturing it from borsh::to_vec, so it does not move when the
implementation does.

Also drops serde derives from VerifiedPaymentMarker and PaymentDetails. They
were dead (these types cross the wire as borsh, and nothing in the workspace
serializes them) and asymmetric, since SignedVerifiedPaymentMarker never had
them. Shares chain_metadata_bytes and the VPM test helpers with routes::parse
instead of keeping two copies, and aligns the thiserror pin with the rest of
the workspace.

Co-Authored-By: Claude <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 15 changed files in this pull request and generated 3 comments.

Suppressed comments (3)

src/host_primitives/src/payment_marker.rs:10

  • This trust-model statement describes verification as current behavior, while payment_verify.rs:10 and all production call sites explicitly disable it. Qualify the statement with PaymentPolicy::Required so the security guarantee is not overstated.
//! Trust model: parser_app verifies the gateway's P256 signature against a

src/parser/app/src/payment_verify.rs:536

  • request_hash has four bound inputs, including include_intermediate_output, so describing metadata as one-third of the preimage is stale and omits a security-relevant field.
        // `chain_metadata_bytes` (borsh(ChainMetadata)) is one-third of
        // `request_hash`'s preimage, alongside `unsigned_payload` and
        // `chain`, both of which already have hand-encoded pinned tests in

src/host_primitives/src/payment_marker.rs:8

  • Repository guidance requires ASCII-only source for terminal compatibility. Replace this em dash with ASCII punctuation.
//! bytes — no schema drift.

Comment thread proto/parser/parser.proto Outdated
Comment thread src/parser/app/src/payment_verify.rs Outdated
Comment thread src/host_primitives/src/payment_marker.rs Outdated
Generated by /finish P3 iteration 1.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 15 changed files in this pull request and generated 3 comments.

Comment thread src/parser/app/src/payment_verify.rs
Comment thread src/parser/app/src/payment_verify.rs
Comment thread src/host_primitives/src/payment_marker.rs
…_hash layout

Copilot round 2 on PR #448. Adds a VPM-specific size limit before Borsh
decoding: try_from_slice ran on the full attacker-controlled
payment_marker with only the 25 MiB whole-request cap behind it, letting
an unsigned oversized marker force a decode plus a second full
serialize+hash before the signature check rejected it. A real marker is
well under 1 KiB, so 8 KiB leaves ample headroom while cutting that cost
by three orders of magnitude.

Also adds hand-encoded Borsh vectors for the Solana and Near
ChainMetadata variants, companions to the existing Ethereum one: that
test only pinned Ethereum's discriminant, so swapping Solana/Near in the
proto oneof (or reordering their fields) would silently change the
request_hash preimage without failing anything.

Co-Authored-By: Claude <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/parser/app/src/payment_verify.rs:239

  • The request hash is computed before the marker's signature is authenticated. In Required mode, an unauthenticated caller can supply the public pinned key, a self-computed matching hash, and a random signature, causing metadata serialization plus allocation and hashing of a preimage approaching the 25 MiB request limit before rejection. Verify the bounded VPM signature first, then compute and compare the full request hash only for an authenticated marker.
    let chain_metadata_bytes = chain_metadata_bytes(chain_metadata.as_ref())
        .map_err(|e| PaymentVerifyError::Internal(format!("chain_metadata borsh encode: {e:?}")))?;
    let expected = request_hash(
        *chain,
        unsigned_payload,
        &chain_metadata_bytes,
        *include_intermediate_output,
    );

Comment thread proto/parser/parser.proto Outdated
The field comment is the only wire spec an out-of-repo signer sees, and it
still said the marker relies solely on the 25 MiB gRPC cap. 61b55c9 added
an 8 KiB length check before Borsh decoding, so a signer reading this
comment would have planned against the wrong limit.

Also drops a leftover double hyphen in the same block.

Codegen regenerated so the generated output carries the same text.

Co-Authored-By: Claude <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/parser/app/src/payment_verify.rs:239

  • Authenticate the marker before hashing request-controlled data. As ordered now, anyone can send an 8 KiB syntactically valid marker with a bad signature plus a near-limit payload/metadata and force chain_metadata_bytes and request_hash to allocate/copy/hash up to the full request before rejection. Move the pinned-key and P256 signature checks ahead of this request-binding calculation so forged markers take only bounded work.
    let chain_metadata_bytes = chain_metadata_bytes(chain_metadata.as_ref())
        .map_err(|e| PaymentVerifyError::Internal(format!("chain_metadata borsh encode: {e:?}")))?;
    let expected = request_hash(
        *chain,
        unsigned_payload,
        &chain_metadata_bytes,
        *include_intermediate_output,
    );

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants