Lodestar REST API should accept percent-encoded URIs

[TobiWo]

Describe the bug

Note: This report assumes the Lodestar beacon api can be reached on http://localhost:5054

Lodestar does not accept URL percent-encoding for params send via a get request.
For example, sending a request via curl:

This works:

curl --location 'http://localhost:5054/eth/v1/beacon/states/head/validators?id=111,222'

This does not work:

curl --location 'http://localhost:5054/eth/v1/beacon/states/head/validators?id=111%2C222'

Expected behavior

Lodestar should accept percent-encoded URIs like so:

curl --location 'http://localhost:5054/eth/v1/beacon/states/head/validators?id=111%2C222'

Percent-encoding is a widely accepted standard and should also work with Lodestar. Also I tested this with most other clients (Lighthouse, Teku and Nimbus) and all three accept percent-encoding. Therefore Lodestar would be in line with these other clients.

Steps to Reproduce

Curl lodestar API endpoint with percent-encoded URI:

curl --location 'http://localhost:5054/eth/v1/beacon/states/head/validators?id=111%2C222'

Desktop (please complete the following information):

• OS: Ubuntu
• Version: 22.04
• Branch: LTS

[nflaig]

Thanks @TobiWo for reporting!

This is the behavior of a downstream library we are using called qs.
I looked into this and it seems like this is intended behavior, as defined in RFC 3986 sub-delims, , is considered a delimiter, whereas URL encoding it changes the behavior and it should just be handled as normal text. This is the purpose of applying URL encoding in the first place, to handle reserved characters as plain text.

quote from the author of qs

The arrayFormat comma feature treats unencoded commas differently from encoded ones. Encoded commas are not special delimiters, they're just text - the , character. Unencoded commas are special delimiters.

There is also a related issue for this ljharb/qs#311, the library initially treated URL encoded commas as reserved character which was considered a bug and the library added a test for this "parses comma delimited array while having percent-encoded comma treated as normal text".

Considering all of this, I would say this is rather bug in the other clients. %2C should be treated as normal text (,) and not a delimiter/reserved character.

See #5280 for examples on what query array formats Lodestar supports currently (tl;dr id=3&id=4&id=5 and id=3,4,5)

[TobiWo]

@nflaig That makes a lot of sense. Thanks for clarifying.
I just found this because in the Python request library (and also with Postman), commas get percent-encoded by default. This might be the case because they probably don't use reserved characters like defined in the RFC definition. Interesting.

The question is, how to proceed here? Should I/we open issues in all other client repositories 😉 ? Probably this is too much? In general it would be nice that all clients handle REST requests the same way ofc. Maybe you can bring this up in an ACDC call? Don't know if this is too much as well because there are probably much more important stuff to discuss in these calls.

[nflaig]

Python request library (and also with Postman), commas get percent-encoded by default

Yeah definitely looks like it, the libraries treat , as normal text in which case it is correct to URL encode it. Just looking at few libraries, they seem to support and use different formats...and there seems to be general confusion of why URL encoding exists in the first place.

In general it would be nice that all clients handle REST requests the same way ofc.

It looks like array queries like id=3&id=4&id=5 should work with all clients, this is also the format the Beacon API spec uses by default (e.g. see getStateValidators), we also just started supporting id=3,4,5 because it was required by rocketpool (#5268).

Maybe you can bring this up in an ACDC call vs open issues in all other client repositories

This is rather an implementation detail so I would say an issue on the respective client repository is the better approach.
Right now, I think handling a URL encoded comma (%2C) as reserved character might not be an issue because there are no values that contain an actual comma, although they might also incorrectly handle other reserved characters like /, &, etc. if URL encoded, those should not have any special meaning, it's just text...

Would still be interesting to see what comes out of that, I would assume what happens in the other clients is that they apply URI decoding before parsing the query which is definitely incorrect and should be fixed. Might even be a breaking change if they fix this bug as downstream tooling might rely on it at this point...

[philknows]

Maybe you can bring this up in an ACDC call vs open issues in all other client repositories

@nflaig @TobiWo I would probably bring this up to the ETH R&D Discord (maybe Consensus Layer > APIs channel or General > consensus-dev) for better coordination and general consensus on how to move forward with this on all CL teams. If it requires a beacon spec inclusion/amendment, then we would push a PR to https://github.com/ethereum/beacon-APIs/

[TobiWo]

Hi @philknows, I was just reading through @nflaig answer and thought that opening an issue in every repository is a bit repetitive. Your suggested approach sounds much better. I will go forward with that idea.
Also thanks @nflaig for the detailed answer and the medium article. Interesting how confusing those tiny code pieces can be 🙂 .

[nflaig]

Sounds good to me, thanks for driving this @TobiWo 👍

[nflaig]

@TobiWo did you get a response from other client teams on this? I can also just bring it up in discord if you'd like, looking at some interop issues right now so it is good time to also look at this

[philknows]

Closing this issue, thanks for clearing this up @nflaig