Skip to content

feat(ethapi): add eth cors settings#1021

Merged
raulk merged 4 commits intoconsensus-shipyard:mainfrom
sanderpick:ethapi-cors
Jul 22, 2024
Merged

feat(ethapi): add eth cors settings#1021
raulk merged 4 commits intoconsensus-shipyard:mainfrom
sanderpick:ethapi-cors

Conversation

@sanderpick
Copy link
Copy Markdown
Collaborator

Second PR requested in a convo with @raulk.

This one adds eth CORS settings to control allowed origins, methods, and headers. It's modeled after the cometbft config, which includes the following settings under [rpc]:

# A list of origins a cross-domain request can be executed from
# Default value '[]' disables cors support
# Use '["*"]' to allow any origin
cors_allowed_origins = []

# A list of methods the client is allowed to use with cross-domain requests
cors_allowed_methods = ["HEAD", "GET", "POST", ]

# A list of non simple headers the client is allowed to use with cross-domain requests
cors_allowed_headers = ["Origin", "Accept", "Content-Type", "X-Requested-With", "X-Server-Time", ]

allowed_origins = []
# A list of methods the client is allowed to use with cross-domain requests
# Suggested methods if allowing origins: "GET", "OPTIONS", "HEAD", "POST"
allowed_methods = []
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left these empty by default because there is no way to override them to be empty using an env var (empty strings are ignored by the config list parser).

}

impl TryInto<std::net::SocketAddr> for SocketAddress {
impl TryInto<SocketAddr> for SocketAddress {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some cleanups... can revert if annoying.

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.

I tend to prefer using fully-qualified type/trait names in trait implementations when they're stdlib or 3rd party types, to signal clearly that they're external. Will take the liberty to revert this in your branch.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! My IDE shows a warning if a trait/struct has a top level import, but the fully qualified path is used in the same file. That's what happened in this case.

assert_eq!(settings.resolver.membership.static_subnets.len(), 2);
assert_eq!(
format!("{:?}", settings.eth.cors.allowed_origins),
"List([\"https://example.com\", \"https://www.example.org\"])"
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugly, but no other way of testing this... there isn't a public API on AllowedOrigin to list its members.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just alphabetizing the dependencies... can revert if annoying. Some crates have ordered dependencies and other don't. I thought may as well clean up the ones I'm touching.

{
let mut origins = Vec::new();
while let Some(v) = seq.next_element::<String>()? {
if v == "*" {
Copy link
Copy Markdown
Contributor

@cryptoAtwill cryptoAtwill Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick question, sometimes, there are situation such as wildcard subdomains, like *.examples.com, it seems it's not handled by the parsing here? Just curious will it work if it's only raw strings parsed here and the json rpc server can handle wildcard subdomain? Or actually we dont allow wildcard subdomains?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically speaking wildcard subdomains aren't a proper origin. Given a request, the server can respond with one of...

Access-Control-Allow-Origin: *
Access-Control-Allow-Origin: <origin>
Access-Control-Allow-Origin: null

ie, "*", a specific domain, or none.

We could enable wildcard subdomains with some additional parsing + a predicate:

use tower_http::cors::{CorsLayer, AllowOrigin};
use http::{request::Parts as RequestParts, HeaderValue};

let layer = CorsLayer::new().allow_origin(AllowOrigin::predicate(
    |origin: &HeaderValue, _request_parts: &RequestParts| {
        origin.as_bytes().ends_with(b".rust-lang.org")
    },
));

This way the server would respond with the full valid request origin. I don't have a strong opinion on how important it is to support this though.

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.

I guess my question is more like current RPC server library does not support AllowOrigin::from_str so that we have to parse the strings ourselves?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can parse a HeaderValue from string and then parse AllowOrigin from that, but that would only work if the parsing could handle String and Vec<String> as input:

  • "*" -> HeaderValue -> AllowOrigin
  • "http://example.com" -> HeaderValue -> AllowOrigin
  • ["http://example.com", "http://example.org"] -> Vec<HeaderValue> -> AllowOrigin
  • ["*"] -> not parse-able without custom handling ("*" is not allowed in a list of you want to use the Vec<HeaderValue> -> AllowOrigin parsing.
  • "*.example.org" -> not directly parse-able, need to use a predicate / closure

I originally took the String and Vec<String> as input approach, but it's not possible to provide a single String as input from the environment variables (they get parsed as Vec). Which means that without custom parsing, there would have been no way to use "*". The same applies to methods and headers. It's also less confusing to only allow a list in the config files (modeled after cometbft config).

@sanderpick
Copy link
Copy Markdown
Collaborator Author

sanderpick commented Jun 12, 2024

@cryptoAtwill do we want to allow CORS requests that require authentication? If so, we'll need some additional handling:

@raulk raulk changed the title ethapi: Add eth cors settings feat(ethapi): add eth cors settings Jul 22, 2024
}

impl TryInto<std::net::SocketAddr> for SocketAddress {
impl TryInto<SocketAddr> for SocketAddress {
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.

I tend to prefer using fully-qualified type/trait names in trait implementations when they're stdlib or 3rd party types, to signal clearly that they're external. Will take the liberty to revert this in your branch.

@raulk
Copy link
Copy Markdown
Contributor

raulk commented Jul 22, 2024

Thanks for the contribution, @sanderpick! Looking forward to many more 💪

@raulk raulk merged commit 05f5149 into consensus-shipyard:main Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants