Skip to content

feat: add rpc filter for early rejection of configured method-param t… - #64

Open
notwedtm wants to merge 1 commit into
mainfrom
feat/param-filters
Open

feat: add rpc filter for early rejection of configured method-param t…#64
notwedtm wants to merge 1 commit into
mainfrom
feat/param-filters

Conversation

@notwedtm

Copy link
Copy Markdown
Contributor

This pull request introduces an exact method and parameter filtering mechanism for the superbank-rpc server, allowing requests to be rejected before reaching handlers or consuming resources. The feature is configured via a shared YAML file and is integrated throughout the codebase, including configuration, server startup, request handling, and tests.

The most important changes are:

Parameter Filtering Feature:

  • Added a new RpcParameterFilterSet (in crates/superbank-rpc/src/request_filter.rs) that loads method/parameter filters from a YAML config file, provides efficient matching, and rejects requests that match any filter entry. Includes comprehensive tests for filter logic and config parsing.
  • Updated RpcConfig to accept a --config CLI argument or SUPERBANK_CONFIG environment variable, specifying the shared YAML file for filters. Added parsing tests. [1] [2]
  • On server startup, the filter set is loaded and stored in AppState, with logging of the number of loaded filters. [1] [2] [3] [4] [5]

Request Handling Logic:

  • Integrated the filter into single and batch JSON-RPC request handlers. Requests matching a filter are rejected with HTTP 405 and a JSON-RPC error code/message, with batch handling ensuring only matched calls are rejected and the batch status is set accordingly. [1] [2] [3] [4]
  • Added metric observation for filtered requests to track their occurrence.

Documentation:

  • Updated README.md and crates/superbank-rpc/README.md to document the new filter configuration, YAML syntax, matching semantics, and error behavior. [1] [2]

Testing and Test Utilities:

  • Updated test helpers to support parameter filters in test AppState construction, ensuring coverage of the new filtering logic. [1] [2] [3] [4] [5] [6] [7]

With these changes, superbank-rpc can efficiently reject unwanted method/parameter combinations at the entry point, improving security and resource utilization.…uples

@Mctursh Mctursh 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.

Went through this pretty carefully. The engine itself is great, matching's exact and order-independent, batch keeps its indexes straight, the hot path's basically free when nothing's configured, and the shared YAML plays nice with the ingestor. No notes there.

What kept catching me is the rejection path rolling its own HTTP + JSON-RPC error shape when the server already has conventions for the same cases, so most of these are really "reuse what's there?" questions:

  • -32601 is what the server already returns for unknown methods (tests assert on it), so a blocked call looks identical by code to a method that doesn't exist. The commitment=processed path already does the right thing for "method's fine, param isn't", -32602 + data. Filter could match that.

  • The 405 is always on, even with emit_http_errors off, which cuts against the "HTTP status behavior" section that says client errors stay 200. And in a batch, one filtered item flips the whole batch to 405 while the other results are sitting right there in the body.

  • One real bug: an empty --config/SUPERBANK_CONFIG crash-loops on boot (clap hands you Some(""), not None), where the other path flags guard the empty case.

Minor: a method-only entry only matches params: [], so [getHealth] won't catch a no-arg getHealth that omits params. Documented, just easy to trip on.

Nothing blocking really, mostly the reuse questions plus that config fix. Nice feature to have.

const JSON_RPC_INTERNAL_ERROR_CODE: i64 = -32603;
const JSON_RPC_REQUEST_TIMEOUT_CODE: i64 = -32000;
const JSON_RPC_REQUEST_TIMEOUT_MESSAGE: &str = "Request timeout";
const JSON_RPC_METHOD_NOT_ALLOWED_CODE: i32 = -32601;

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.

This reuses -32601, which is JSON-RPC's "Method not found" and the same code the server already returns for genuinely-unknown methods (the fallback arm in dispatch, and tests/mod.rs asserts code == -32601). So a client keying on the code can't tell a blocked (method, params) from a method that doesn't exist. Only the message text and the HTTP status differ.

The repo already handles this exact shape (method is valid, a specific param value is rejected by server policy): the commitment=processed rejection in signatures.rs and blocks.rs returns -32602 "Only confirmed or finalized commitments are supported" with data: {requestedCommitment}. Could the filter match that, -32602 + data describing the matched method/params, or take a dedicated code in the -32000..-32099 server range if it should read as "server-imposed"?

JSON_RPC_METHOD_NOT_ALLOWED_MESSAGE,
None,
);
*response.status_mut() = StatusCode::METHOD_NOT_ALLOWED;

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.

This sets 405 unconditionally. Everywhere else the server treats HTTP error statuses as opt-in via emit_http_errors (default false means everything is HTTP 200; only four server-side codes ever promote, to 503). The README's "HTTP status behavior" section says it directly: "Client, malformed-request, and data-condition errors remain HTTP 200 OK." A filtered request is a client error by that taxonomy but returns 405 regardless of the flag, so that section and the filter section now disagree.

Should the 405 be gated behind emit_http_errors (200 by default, like every other client error) so the "HTTP errors are opt-in" contract holds? If the 405 is intentional for ops visibility, maybe a one-line exception in the HTTP-status section so the two don't read as contradictory.

Comment on lines +1216 to +1218
let mut response = Json(Value::Array(response_values)).into_response();
if parameter_filter_matched {
*response.status_mut() = StatusCode::METHOD_NOT_ALLOWED;

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.

One filtered item sets the whole batch's HTTP status to 405, while response_values still carries every other item's successful result. A client that checks resp.ok before parsing (the exact client the opt-in 503 mechanism exists for) treats the succeeded siblings as failed and may drop or retry the whole batch. JSON-RPC models a batch as N independent responses, so one administratively blocked item escalating the transport status for all of them is a sharp edge, and unlike the 503 batch-promotion it isn't gated by emit_http_errors.

Related: if emit_http_errors is on and a batch has both a filtered item and a 503-eligible error, promote_http_status_for_json_rpc_errors overwrites this 405 with 503 unconditionally, so the documented "returns 405" doesn't hold in that combo.

pub async fn run_server(args: RpcConfig) -> RpcResult<()> {
info!("Starting Solana RPC server on {}:{}", args.host, args.port);
let rpc_parameter_filters =
RpcParameterFilterSet::load(args.config.as_deref()).map_err(RpcError::Config)?;

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.

config is Option<PathBuf> passed straight to load() with no empty guard, and clap turns SUPERBANK_CONFIG="" (or --config "") into Some("") (checked on 4.6.0). So an empty placeholder reads "" and fails boot instead of no-op'ing as "unset." The other optional path flags guard this (disk_cache_path, dragonsmouth_endpoint use .map(str::trim).filter(|v| !v.is_empty())). Same idea here:

RpcParameterFilterSet::load(
    args.config.as_deref().filter(|p| !p.as_os_str().is_empty()),
)

Comment on lines +78 to +84
if method.trim().is_empty() {
return Err(Self::entry_error(path, index, "method must not be empty"));
}

let candidates = filters
.by_method_and_arity
.entry(method)

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.

Small one: the loader validates method.trim().is_empty() but then stores the untrimmed method as the key. A quoted entry with a stray space (["getThing ", ...]) passes validation, counts in len(), and logs as loaded, but never matches, since matches compares the raw request method. Narrow case (unquoted YAML strips the space), but it's a silent no-op that slips past the loader's otherwise fail-loud validation. Trimming before storing, or rejecting a non-trimmed method like the empty case, keeps it fail-loud.

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.

2 participants