fix(python): harden protocol validation#228
Open
EfeDurmaz16 wants to merge 68 commits into
Open
Conversation
Collaborator
Author
|
@greptile-apps please review |
Greptile SummaryThis PR hardens Python-side protocol validation across MPP and x402: RFC 3339 receipt timestamps are now semantically validated, the x402
Confidence Score: 4/5Safe to merge with one correctness gap in the settlement state machine that can strand a channel under a narrow but real task-cancellation scenario. The settlement path persists a signed transaction signature before broadcasting, then relies on transaction-failed to know when a reconcile attempt can be retired. A CancelledError raised by persist_intent after the store write but before broadcast_prepared_transaction sets intent_persisted=True while the channel carries a phantom settled_signature for a tx that was never sent. The reconcile path times out with transaction-not-found so retire_failed_intent is never triggered and the channel cannot be sealed without manual store intervention. python/src/solana_pay_kit/protocols/mpp/server/session_method.py around the _settle_channel exception handler and persist_intent closure at lines 1183-1222. Important Files Changed
Reviews (19): Last reviewed commit: "Merge remote-tracking branch 'origin/fix..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…o fix/python-security-hardening
…o fix/python-security-hardening
…o fix/python-security-hardening
…o fix/python-security-hardening
…o fix/python-security-hardening
…o fix/python-security-hardening
Collaborator
Author
|
@greptile-apps please review. |
…o fix/python-security-hardening
EfeDurmaz16
marked this pull request as ready for review
July 10, 2026 15:15
Collaborator
|
I'd like to understand better:
It's not the responsibility of the payer to pay for ATA recipients. The only exception is in |
Collaborator
Author
|
Agreed: session settlement no longer payer-funds recipient ATAs; that remains exclusive to |
…grades The canonical cross-protocol settlement key silently orphaned the solana-charge and x402-svm-exact markers written by not-yet-upgraded workers, so a signature settled by an old worker could settle again under the new key (and vice versa). Settlement claims now cover the canonical network-qualified key plus both legacy markers through the store's atomic put_if_absent, canonical first; the x402 rollback releases them in reverse order. Rollback tests now assert the canonical key instead of the obsolete legacy-only key, and the nominal ProductionReplayStore contract documents its operator-attestation semantics.
Direct SessionServer construction bypassed the new_session channel-store policy, so a MemoryChannelStore was accepted on mainnet. The policy now lives in one enforce_channel_store_policy helper shared by the factory and the constructor: localnet accepts anything, an attested ProductionChannelStore is required otherwise, the devnet opt-in stays devnet-only, and the opt-in env is forbidden on mainnet outright, mirroring resolve_replay_store. Top-up transaction signatures are now single-use: the atomic update_channel mutator that raises the deposit also records the signature on the channel state (additive, omit-empty wire field), so a confirmed top-up replayed with a higher newDeposit can no longer raise the deposit repeatedly. ProductionChannelStore documents that subclassing is an operator attestation the SDK cannot machine-verify, with tests pinning exactly that boundary, and the top-up verifier rejection tests now assert exact PaymentError codes.
…licy The framework integrations surface replay-store misconfiguration as the public ConfigurationError; asserting the leaked internal PaymentError pinned an implementation detail.
…ides Run the Python replay-store boot-policy probe only in uv-provisioned jobs (python.yml and harness.yml legs) with a guard test that refuses to skip when HARNESS_REQUIRE_PYTHON_BOOT_PROBE=1, instead of the toolchain-neutral Node job where a missing uv could self-skip it. The missing-ATA session leg is now blocking: a 200 response must never hide a settlement that delivered no funds. The harness process spawner now removes inherited environment variables when an override is undefined, and the no-opt-in boot probe explicitly unsets PAY_KIT_ALLOW_INMEMORY_REPLAY_STORE in the child so a parent CI environment cannot false-green the fail-closed assertion. Each probe now requires its SDK's precise missing-store rejection so the separate mainnet opt-in-forbidden branch cannot satisfy it.
Adversarial review: a direct SessionServer construction with the mainnet-beta alias plus the devnet opt-in env skipped the opt-in-forbidden-on-mainnet tripwire because nothing canonicalized the network on that seam. enforce_channel_store_policy now canonicalizes first; unknown strings still fall to the final fail-closed rejection. Also pins the trusted-mode exemption: a signatureless top-up is only possible when no verifier is configured and records no fence entry.
Adversarial review: dropping the reversed release order (canonical key deleted first) passed the whole suite while opening the stranding race the docstring warns about, and nothing asserted that a losing claim performs zero deletes through the full adapter path. Both are pinned, and the legacy-marker block tests now assert the signature_consumed code and that the old worker's marker survives untouched.
MppAdapter re-raises the store policy violation as the public ConfigurationError (a PayKitError, not a PaymentError), so the boot probe's except PaymentError was dead: the rejection escaped as a bare traceback that only matched the fail-closed signature by luck of the message landing in the stderr ring buffer. Catch the exact public type and correct the docstring to the real exception and message.
The generic Node job omits boot-policy.test.ts because that suite requires the TypeScript charge server to fail closed, which ships in the TypeScript subscription leaf, not this Python leaf. Document that the Go and TypeScript probes return to CI there, so a reader does not read the omission as lost coverage.
Collaborator
Author
|
@greptileai review |
EfeDurmaz16
added a commit
that referenced
this pull request
Jul 13, 2026
This leaf shipped the identical missing-ATA blocking change (remove continue-on-error from the session no-ATA leg, "This regression is blocking" comment) that #228 already delivers, and #228 owns python.yml. Revert python.yml to base so only #228 delivers it and the two leaves do not conflict at integration.
EfeDurmaz16
added a commit
that referenced
this pull request
Jul 13, 2026
The go/ts/python fail-closed probes required their SDK guard, but those remediations land in sibling leaves (#227/#238/#228), so in this harness leaf they hard-failed. Resolve each covered probe through the same sdkImplementsGuard git-grep mechanism the native php/ruby/lua probes already use: a probe is required only when its SDK source references the opt-in marker in-tree, else it asserts-skip with a loud PENDING note and auto-promotes to required at integration. Moves the REPO_ROOT const above the probe resolution to avoid a module-eval temporal-dead-zone error.
EfeDurmaz16
added a commit
that referenced
this pull request
Jul 14, 2026
# Conflicts: # .github/workflows/ci.yml # harness/test/boot-policy.test.ts
EfeDurmaz16
added a commit
that referenced
this pull request
Jul 14, 2026
Add PR #239's unique behavior on top of sibling #228: the state-aware open verifier that fetches the on-chain Channel account at the confirmed slot and binds deposit, splits, grace period and PDA seeds to on-chain reality, so a payload's claimed economics are never persisted as facts. - session_onchain: BoundChannel, fetch_and_bind_channel_account, new_open_state_tx_verifier (+ signature-only confirm/bind helpers, _session_distribution_hash, _expected_session_grace_period). The structural open path reuses #228's verify_open_tx via the recipients contract. - confirm_transaction_signature now returns the confirmed slot and rejects a present-but-malformed slot, so the account read is pinned with min_context_slot; a status that omits the slot still confirms. - session: SessionConfig.verify_open_state_tx / open_tx_submitter seams, VerifiedOpenFacts, and process_open fail-closed: off localnet a payment-channel open requires the state verifier and persists only account-derived facts. - session_method: new_session installs the state verifier off localnet; _handle_open propagates the challenge recentSlot for signature-only push. - session_store: ChannelState.salt / open_signature round-trip. - rpc: SolanaRpc.get_account_info min_context_slot + typed get_transaction. - tests: add test_session_state_binding.py (adapted to #228's ProductionChannelStore policy, which is kept as-is). #228's structural open, top-up fence, channel-store policy and replay code are kept unchanged; their tests continue to pass.
EfeDurmaz16
added a commit
that referenced
this pull request
Jul 14, 2026
… union Order the mpp-session leaf (#239) ahead of the python leaf (#228) so their overlapping edits to the shared ci.yml + boot-policy.test.ts interleave without a phantom 3-way conflict (both are ultimately reconciled by the harness meta leaf, which owns those paths and merges last). Add a pure-union assertion after the merges: every non-merge commit the rehearsal introduces over base must be contained in some leaf head, else it is a hand-authored patch on #240 and the script refuses to push. This makes 'green by construction' mechanically checked, not asserted.
EfeDurmaz16
force-pushed
the
fix/python-security-hardening
branch
from
July 15, 2026 10:41
5c3a383 to
1c91439
Compare
…into fix/python-security-hardening # Conflicts: # .github/workflows/ci.yml # harness/test/boot-policy.test.ts
…into fix/python-security-hardening
…leaf fixes The cascade root recorded python's calendar-impossible receipt timestamps (month/day overflow, feb30) as known divergences; this leaf hardens the parser so both cases now conform, and the inverted ledger demands their removal. Python protocol vectors: 206/206 green here.
EfeDurmaz16
changed the base branch from
split/pr216-ci-harness-mega
to
fix/mpp-subscription-hardening
July 15, 2026 11:17
…into fix/python-security-hardening
…into fix/python-security-hardening
…into fix/python-security-hardening
Comment on lines
+1183
to
+1222
| intent_signature: str | None = None | ||
| intent_persisted = False | ||
|
|
||
| async def persist_intent(prepared: PreparedTransaction) -> None: | ||
| nonlocal intent_persisted, intent_signature | ||
| intent_signature = prepared.signature | ||
|
|
||
| def persist(current: ChannelState | None) -> ChannelState: | ||
| if current is None: | ||
| raise ValueError(f"channel {channel_id} disappeared before settlement broadcast") | ||
| if current.sealed: | ||
| return current | ||
| if not current.settling: | ||
| raise ValueError(f"channel {channel_id} settle claim was released before intent persistence") | ||
| if current.settled_signature not in (None, prepared.signature): | ||
| raise ValueError(f"channel {channel_id} settlement signature changed before broadcast") | ||
| nxt = current.clone() | ||
| nxt.settled_signature = prepared.signature | ||
| return nxt | ||
|
|
||
| # Finish persistence despite cancellation before propagating it. | ||
| # From this point a send may have happened, so the claim must stay. | ||
| cancelled = await _finish_store_update(self._core.store().update_channel(channel_id, persist)) | ||
| intent_persisted = True | ||
| if cancelled: | ||
| raise asyncio.CancelledError | ||
|
|
||
| await self._core.store().update_channel(channel_id, seal) | ||
| settled_signature = signature | ||
| return settled_signature | ||
| try: | ||
| signature = await settle_and_seal_channel( | ||
| state, | ||
| merchant=self._signer.keypair, | ||
| rpc=self._rpc, | ||
| config=self._core.config, | ||
| on_prepared=persist_intent, | ||
| ) | ||
| except BaseException as exc: | ||
| if not intent_persisted: | ||
| await release_prebroadcast_claim() | ||
| elif isinstance(exc, PaymentError) and exc.code == "transaction-failed" and intent_signature is not None: | ||
| await retire_failed_intent(intent_signature) |
There was a problem hiding this comment.
persist_intent leaves channel permanently stuck
_finish_store_update guarantees the store write completes before returning, so intent_persisted = True and CancelledError is raised before broadcast_prepared_transaction is ever called. At that point we know the transaction was never sent. However, the except BaseException handler in _settle_channel treats CancelledError + intent_persisted=True identically to "broadcast started, outcome unknown," so neither release_prebroadcast_claim nor retire_failed_intent runs.
On every subsequent retry _settlement_status returns AWAITING_CONFIRMATION (because settled_signature is set) and the reconcile path calls confirm_transaction_signature(search_transaction_history=True) against a signature for a transaction that was never submitted. The poll times out and raises PaymentError(code="transaction-not-found"), which does not satisfy the exc.code == "transaction-failed" guard for retire_failed_intent. The channel is permanently stuck.
A minimal fix is to track whether the broadcast was ever initiated (e.g. a broadcast_started flag flipped at the top of broadcast_prepared_transaction) so the handler can distinguish CancelledError that arrived before the send from one that arrived during or after it, and call retire_failed_intent only in the former case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
ruff check src testspyright(0 errors)Stack
Targets #219 as requested. Cross-SDK harness checks may remain red until the sibling language and harness PRs are merged into #219.