solana icon indicating copy to clipboard operation
solana copied to clipboard

RPC does not validate shape and types of `RpcContextConfig`

Open steveluscher opened this issue 1 year ago • 1 comments

Problem

The RPC does not appear to validate or throw on invalid values for RpcContextConfig::commitment.

curl https://api.devnet.solana.com/ -X POST -H "Content-Type: application/json" -d '
  {
    "jsonrpc": "2.0",
    "id": 1,
    "method": "getBalance",
    "params": [
      "83astBRguLMdt2h5U1Tpdq5tjFoJ6noeGwaY3mDLVcri",
      {"commitment": "wheeeeeee"}
    ]
  }
'

It also isn't strict about what properties you supply. You can slightly misspell ‘commitment’ which results in your setting having no effect.

curl https://api.devnet.solana.com/ -X POST -H "Content-Type: application/json" -d '
  {
    "jsonrpc": "2.0",
    "id": 1,
    "method": "getBalance",
    "params": [
      "83astBRguLMdt2h5U1Tpdq5tjFoJ6noeGwaY3mDLVcri",
      {"committment": "processed"}
    ]
  }
'

Similarly, you can supply properties that don't exist.

curl https://api.devnet.solana.com/ -X POST -H "Content-Type: application/json" -d '
  {
    "jsonrpc": "2.0",
    "id": 1,
    "method": "getBalance",
    "params": [
      "83astBRguLMdt2h5U1Tpdq5tjFoJ6noeGwaY3mDLVcri",
      {"xommitment": "processed"}
    ]
  }
'

This happens in the wild.

  • https://github.com/tDev0809/solana-staking-cli/blob/a7ec96cb140ce067e3e22813e9234a6abbb3e288/server/user.js#L29

Proposed Solution

Throw when RPC inputs don't match the Rust types.

steveluscher avatar Dec 14 '23 02:12 steveluscher

Dupe of https://github.com/solana-labs/solana/issues/18867 I looked into fixing this back in the day, and originally thought it was too complex a fix, since the struct parsing happens upstream in the jsonrpc library. But now it looks like we might be able to get at least part of the way there by tagging our config structs with #[serde(deny_unknown_fields)]: https://serde.rs/container-attrs.html#deny_unknown_fields

This will prevent the field misspelling case, but not generally the misspelled value case, because of our use of flattening. (quoted from above link) "This attribute is not supported in combination with flatten, neither on the outer struct nor on the flattened field." Here's a rust playground that demonstrates: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a14f9aabc51935c833904eee8cbb8678

Then the question is whether such a change would be too breaking. I'd have to run through all our backward-compatibility Options to make sure.

CriesofCarrots avatar Dec 14 '23 03:12 CriesofCarrots