fix(ws server): support * in host and origin filtering
#781
Conversation
niklasad1
changed the title
* in host and origin filtering
| @@ -221,9 +221,9 @@ impl<T> From<AllowCors<T>> for Option<T> { | |||
| /// Returns correct CORS header (if any) given list of allowed origins and current origin. | |||
| pub(crate) fn get_cors_allow_origin( | |||
| origin: Option<&str>, | |||
| allowed: &Option<Vec<AllowOrigin>>, | |||
There was a problem hiding this comment.
Is the ordering of the params here considered a breaking change? Is there any reason for this other than cleaner order.
There was a problem hiding this comment.
We don't care about the breaking changes, the next will anyway be breaking.
The rationale is to avoid having "the type same" as two consecutive parameters such as fn foo(x: Option<&str>, y: Option<&str>, z: Option<Vec<B>>) because it's error prone and easy confuse the parameters.
| None => DomainsValidation::Disabled, | ||
| impl AllowHosts { | ||
| /// Verify a host. | ||
| pub fn verify(&self, value: &str) -> Result<(), Error> { |
There was a problem hiding this comment.
Ahh okay nice refactor. So were basically replacing DomainsValidation, and is_host_valid with this AllowHosts impl.
Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>
Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>
| let header = cors::get_cors_allow_headers(header_names, cors_headers, &self.allowed_headers, |name| name); | ||
|
|
||
| if let cors::AllowCors::Invalid = header { | ||
| Err(Error::HttpHeaderRejected("access-control-request-headers", "<too long to be logged>".into())) |
There was a problem hiding this comment.
Why "too long to be logged"?
There was a problem hiding this comment.
I think I was just lazy because the actual values were moved into the function above.
There was a problem hiding this comment.
Hrm yeah, I mulled over this a bit and I can see that it'd be a bit of a pain to do better!
| } | ||
|
|
||
| #[test] | ||
| fn should_accept_if_on_the_list() { | ||
| let valid = is_host_valid(Some("parity.io"), &AllowHosts::Only(vec!["parity.io".into()])); | ||
| assert!(valid); | ||
| assert!((AllowHosts::Only(vec!["parity.io".into()].into())).verify("parity.io").is_ok()); |
There was a problem hiding this comment.
if it's on the list without a port, like this, would .verify("parity.io:1234") work? In other words, is any port valid if no port is given in the allow list?
There was a problem hiding this comment.
no it will be denied unless the port is whitelisted or * is used but I will add a test to make it clear.
| to_result: F, | ||
| ) -> AllowCors<Vec<O>> { | ||
| // Check if the header fields which were sent in the request are allowed | ||
| if let AccessControlAllowHeaders::Only(only) = cors_allow_headers { | ||
| if let AllowHeaders::Only(only) = cors_allow_headers { | ||
| let are_all_allowed = headers.all(|header| { | ||
| let name = &Ascii::new(header.as_ref()); | ||
| only.iter().any(|h| Ascii::new(&*h) == name) || ALWAYS_ALLOWED_HEADERS.contains(name) |
There was a problem hiding this comment.
I'm struggling to understand ALWAYS_ALLOWED_HEADERS; I think that we allow any headers specified in AllowHeaders plus any in ALWAYS_ALLOWED_HEADERS.
I guess we have this list so that if the user doesn't allow any headers themselves via the access control stuff, standard requests will still work?
Some headers are also not completely black and white as to whether they are accepted or not. Eg with CORS, we'll always need to return "Content-Type" in the "Access-Control-Allowed-Headers" response, because otherwise I think the user couldn't set that to "application/json" without CORS disallowing it (see https://developer.mozilla.org/en-US/docs/Glossary/CORS-safelisted_request_header#additional_restrictions).
Also there is an Access-Control-Allow-Origin header in the list, which is what the server would respond with, so perhaps this doesn't need to be there?
Also a client can send an Access-Control-Request-Method header in a preflight request, and so perhaps that should be there?
There was a problem hiding this comment.
I'm struggling to understand ALWAYS_ALLOWED_HEADERS; I think that we allow any headers specified in AllowHeaders plus any in ALWAYS_ALLOWED_HEADERS.
yeah, you are correct.
the following check is performed on each header; allow_headers.contains(header) || ALLOWED_ALLOWED_HEADERS.contains(header)
I guess we have this list so that if the user doesn't allow any headers themselves via the access control stuff, standard requests will still work?
Yes, I have not written this myself so I don't know details but I guess so.
Also there is an Access-Control-Allow-Origin header in the list, which is what the server would respond with, so perhaps this doesn't need to be there?
Yes, that sounds wrong. Nice catch but nothing really I had in mind to touch in this PR :)
Also a client can send an Access-Control-Request-Method header in a preflight request, and so perhaps that should be there?
That is handled is separately which is passed separately into this function. I renamed it to cors_request_headers
There was a problem hiding this comment.
Ok, so your suggestion is to only whitelist Accept, Accept-Language, Content-Type, Content-Language and Access-Control-Request-Headers?
There was a problem hiding this comment.
the rest needs a CORS request I assume?
|
Love that the access control stuff is now consistent across HTTP and WS (as well as the *'s working!). Just a few questions/nits :) |
Fixes #780 by sharing the access control stuff between the servers.
TLDR: we didn't support
*when compared incominghostswith some concrete port number such aslocalhost:9993in WS-server.