Skip to content

fix(core): preserve remote transaction errors - #764

Open
xternet wants to merge 1 commit into
solana-foundation:mainfrom
xternet:agent/preserve-remote-errors
Open

fix(core): preserve remote transaction errors#764
xternet wants to merge 1 commit into
solana-foundation:mainfrom
xternet:agent/preserve-remote-errors

Conversation

@xternet

@xternet xternet commented Aug 13, 2026

Copy link
Copy Markdown

Summary

Surfpool's remote getTransaction path previously treated every client error as a missing transaction. This conflated two different RPC outcomes:

  • a valid JSON-RPC null, meaning the transaction is unavailable; and
  • transport, HTTP, provider, or decoding failures, meaning the lookup did not complete reliably.

The Solana RPC client convenience method deserializes directly into a concrete transaction, so a valid null arrives through its error path too.

Change

  • Deserialize the raw getTransaction response as Option<EncodedConfirmedTransactionWithStatusMeta>.
  • Keep valid null mapped to GetTransactionResult::None.
  • Propagate genuine provider failures through the locker's existing SurfpoolResult boundary.
  • Preserve the published SurfnetRemoteClient::get_transaction return type with a compatibility wrapper; the new fallible method is crate-private.
  • Sanitize transport failures before putting them in client-visible JSON-RPC error data. Reqwest errors can include the full datasource URL, so callers receive the useful failure category plus scheme/host, never userinfo, path, or query credentials.

Regression coverage

  • A recorded null response remains None and verifies the exact outgoing GetTransaction request/config.
  • A provider failure reaches the locker caller instead of becoming None.
  • A real local HTTP 401 response is requested through a URL carrying credentials in userinfo, path, and query; the propagated error retains 401 Unauthorized and the sanitized host but omits every secret.

Validation

  • cargo test -p surfpool-core surfnet::remote::tests::a_ -- --test-threads=1 — 5 passed
  • cargo test -p surfpool-core --no-default-features surfnet::remote::tests::a_ -- --test-threads=1 — 5 passed
  • cargo +nightly fmt --all -- --check
  • cargo clippy -p surfpool-core --lib --no-default-features — completed with existing repository warnings only
  • git diff --check

This branch and #762 both add tests to surfnet/remote.rs; whichever lands second may need a trivial rebase, but the production changes are independent.

@xternet
xternet marked this pull request as ready for review August 13, 2026 19:54
@greptile-apps

greptile-apps Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR distinguishes valid remote getTransaction null responses from provider failures while retaining the existing public client method as a compatibility wrapper.

  • Adds a fallible raw getTransaction request that decodes nullable responses explicitly.
  • Propagates remote lookup failures through the locker and JSON-RPC boundary.
  • Adds provider-error sanitization and regression coverage for null, failure, and HTTP credential-redaction paths.

Confidence Score: 3/5

This PR should not merge until provider-controlled RPC error text is sanitized without exposing credential-bearing datasource path or query fragments.

The new fallible path correctly preserves provider failures, but it also forwards arbitrary RpcError text to client-visible error data after an exact full-URL replacement that does not remove reflected URL fragments.

Files Needing Attention: crates/core/src/surfnet/remote.rs

Security Review

The RpcError sanitization replaces only the exact complete datasource URL. A provider error reflecting a credential-bearing path or query fragment can therefore expose that secret through client-visible JSON-RPC error data. How this was verified: The reflected provider message flows from the RpcError branch into SurfpoolError.data and the public RPC response without another redaction step.

Important Files Changed

Filename Overview
crates/core/src/error.rs Adds a transaction-specific internal error constructor that includes the signature and sanitized remote failure text in JSON-RPC error data.
crates/core/src/surfnet/locker.rs Switches remote transaction fallback to the new fallible method so genuine provider failures reach callers.
crates/core/src/surfnet/remote.rs Separates null responses from transport/provider errors and adds sanitization, but provider-controlled RpcError fragments can still disclose path or query credentials.

Reviews (1): Last reviewed commit: "fix(core): preserve remote transaction e..." | Re-trigger Greptile

error.line(),
error.column()
),
ClientErrorKind::RpcError(error) => error.to_string().replace(datasource_url, &endpoint),

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.

P1 security RPC fragment redaction bypass

When a provider returns a JSON-RPC error containing only a credential-bearing request path or query fragment, the exact full-URL replacement leaves that fragment intact, causing datasource credentials to be disclosed in client-visible error data. How this was verified: The provider message flows from this RpcError branch into SurfpoolError.data and the public RPC response without another redaction step.

Knowledge Base Used: Surfnet Engine (SVM, Locker, Remote Fetch)

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.

1 participant