Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(ws server): support * in host and origin filtering #781

Merged
merged 20 commits into from
Jun 13, 2022
Merged

fix(ws server): support * in host and origin filtering #781

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
a68905b
initial rewrite to re-use HTTP access control
niklasad1 May 24, 2022
98b397a
clean things up
niklasad1 May 25, 2022
b42ff53
Update core/src/error.rs
niklasad1 May 25, 2022
00820d3
Update core/src/error.rs
niklasad1 May 25, 2022
04a34f9
allow origin: add back removed Display impl
niklasad1 May 25, 2022
17b355b
Merge remote-tracking branch 'origin/na-fix-host-filtering' into na-f…
niklasad1 May 25, 2022
da017c2
Merge remote-tracking branch 'origin/master' into na-fix-host-filtering
niklasad1 May 25, 2022
ea3b92d
cleanup again
niklasad1 May 25, 2022
78fabfa
Update http-server/src/lib.rs
niklasad1 May 26, 2022
d0cd8e5
Update examples/examples/cors_server.rs
niklasad1 May 26, 2022
59f30c9
Update core/src/server/access_control/mod.rs
niklasad1 Jun 7, 2022
5df48c8
Update http-server/src/server.rs
niklasad1 Jun 7, 2022
dc1fb5c
fix bad comment
niklasad1 Jun 7, 2022
af9cecc
remove todo
niklasad1 Jun 7, 2022
094976b
fix grumbles
niklasad1 Jun 13, 2022
d5357c1
more grumbles
niklasad1 Jun 13, 2022
2af268f
rename and document a bit
niklasad1 Jun 13, 2022
8991d4b
remove `Access-Control-Allow-Origin` in whitelist
niklasad1 Jun 13, 2022
060badb
fix nit: pub(super)
niklasad1 Jun 13, 2022
c9cff4c
fix bad naming
niklasad1 Jun 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,25 @@ parking_lot = { version = "0.12", optional = true }
tokio = { version = "1.16", optional = true }
wasm-bindgen-futures = { version = "0.4.19", optional = true }
futures-timer = { version = "3", optional = true }
globset = { version = "0.4", optional = true }
lazy_static = { version = "1", optional = true }
unicase = { version = "2.6.0", optional = true }

[features]
default = []
http-helpers = ["hyper", "futures-util"]
server = [
"arrayvec",
"futures-util/alloc",
"globset",
"rustc-hash/std",
"tracing",
"parking_lot",
"rand",
"tokio/rt",
"tokio/sync",
"lazy_static",
"unicase",
]
client = ["futures-util/sink", "futures-channel/sink", "futures-channel/std"]
async-client = [
Expand Down
5 changes: 4 additions & 1 deletion core/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,12 @@ pub enum Error {
/// Attempted to stop server that is already stopped.
#[error("Attempted to stop server that is already stopped")]
AlreadyStopped,
/// List passed into `set_allowed_origins` was empty
/// List passed into access control based on HTTP header verification.
#[error("Must set at least one allowed value for the {0} header")]
EmptyAllowList(&'static str),
/// Access control verification of HTTP headers failed.
#[error("HTTP header: `{0}` value: `{1}` verification failed")]
HttpHeaderRejected(&'static str, String),
/// Failed to execute a method because a resource was already at capacity
#[error("Resource at capacity: {0}")]
ResourceAtCapacity(&'static str),
Expand Down
132 changes: 61 additions & 71 deletions http-server/src/access_control/cors.rs → core/src/server/access_control/cors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@
use std::collections::HashSet;
use std::{fmt, ops};

use crate::access_control::hosts::{Host, Port};
use crate::access_control::matcher::{Matcher, Pattern};
use jsonrpsee_core::Cow;
use crate::server::access_control::host::{Host, Port};
use crate::server::access_control::matcher::{Matcher, Pattern};
use crate::Cow;
use lazy_static::lazy_static;
use unicase::Ascii;

Expand Down Expand Up @@ -128,54 +128,54 @@ impl ops::Deref for Origin {

/// Origins allowed to access
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum AccessControlAllowOrigin {
/// Specific hostname
Value(Origin),
pub enum AllowOrigin {
/// Specific origin.
Origin(Origin),
/// null-origin (file:///, sandboxed iframe)
Null,
/// Any non-null origin
Any,
}

impl fmt::Display for AccessControlAllowOrigin {
impl fmt::Display for AllowOrigin {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(
f,
"{}",
match *self {
AccessControlAllowOrigin::Any => "*",
AccessControlAllowOrigin::Null => "null",
AccessControlAllowOrigin::Value(ref val) => val,
Self::Any => "*",
Self::Null => "null",
Self::Origin(ref val) => val,
}
)
}
}

impl<T: Into<String>> From<T> for AccessControlAllowOrigin {
fn from(s: T) -> AccessControlAllowOrigin {
impl<T: Into<String>> From<T> for AllowOrigin {
fn from(s: T) -> Self {
match s.into().as_str() {
"all" | "*" | "any" => AccessControlAllowOrigin::Any,
"null" => AccessControlAllowOrigin::Null,
origin => AccessControlAllowOrigin::Value(origin.into()),
"all" | "*" | "any" => Self::Any,
"null" => Self::Null,
origin => Self::Origin(origin.into()),
}
}
}

/// Headers allowed to access
#[derive(Debug, Clone, PartialEq)]
pub enum AccessControlAllowHeaders {
pub enum AllowHeaders {
/// Specific headers
Only(Vec<String>),
/// Any header
Any,
}

impl AccessControlAllowHeaders {
impl AllowHeaders {
/// Return an appropriate value for the CORS header "Access-Control-Allow-Headers".
pub fn to_cors_header_value(&self) -> Cow<'_, str> {
match self {
AccessControlAllowHeaders::Any => "*".into(),
AccessControlAllowHeaders::Only(headers) => headers.join(", ").into(),
AllowHeaders::Any => "*".into(),
AllowHeaders::Only(headers) => headers.join(", ").into(),
}
}
}
Expand Down Expand Up @@ -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(
niklasad1 marked this conversation as resolved.
Show resolved Hide resolved
origin: Option<&str>,
allowed: &Option<Vec<AllowOrigin>>,
Copy link
Member

Choose a reason for hiding this comment

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

Is the ordering of the params here considered a breaking change? Is there any reason for this other than cleaner order.

Copy link
Member Author

@niklasad1 niklasad1 Jun 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

host: Option<&str>,
allowed: &Option<Vec<AccessControlAllowOrigin>>,
) -> AllowCors<AccessControlAllowOrigin> {
) -> AllowCors<AllowOrigin> {
match origin {
None => AllowCors::NotRequired,
Some(ref origin) => {
Expand All @@ -239,38 +239,38 @@ pub(crate) fn get_cors_allow_origin(
}

match allowed.as_ref() {
None if *origin == "null" => AllowCors::Ok(AccessControlAllowOrigin::Null),
None => AllowCors::Ok(AccessControlAllowOrigin::Value(Origin::parse(origin))),
None if *origin == "null" => AllowCors::Ok(AllowOrigin::Null),
None => AllowCors::Ok(AllowOrigin::Origin(Origin::parse(origin))),
Some(allowed) if *origin == "null" => allowed
.iter()
.find(|cors| **cors == AccessControlAllowOrigin::Null)
.find(|cors| **cors == AllowOrigin::Null)
.cloned()
.map(AllowCors::Ok)
.unwrap_or(AllowCors::Invalid),
Some(allowed) => allowed
.iter()
.find(|cors| match **cors {
AccessControlAllowOrigin::Any => true,
AccessControlAllowOrigin::Value(ref val) if val.matches(origin) => true,
AllowOrigin::Any => true,
AllowOrigin::Origin(ref val) if val.matches(origin) => true,
_ => false,
})
.map(|_| AccessControlAllowOrigin::Value(Origin::parse(origin)))
.map(|_| AllowOrigin::Origin(Origin::parse(origin)))
.map(AllowCors::Ok)
.unwrap_or(AllowCors::Invalid),
}
}
}
}

/// Validates if the `AccessControlAllowedHeaders` in the request are allowed.
/// Validates if the `AllowHeaders` in the request are allowed.
pub(crate) fn get_cors_allow_headers<T: AsRef<str>, O, F: Fn(T) -> O>(
mut headers: impl Iterator<Item = T>,
requested_headers: impl Iterator<Item = T>,
cors_allow_headers: &AccessControlAllowHeaders,
cors_allow_headers: &AllowHeaders,
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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@niklasad1 niklasad1 Jun 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@niklasad1 niklasad1 Jun 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, so your suggestion is to only whitelist Accept, Accept-Language, Content-Type, Content-Language and Access-Control-Request-Headers?

Copy link
Member Author

Choose a reason for hiding this comment

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

the rest needs a CORS request I assume?

Expand All @@ -283,11 +283,11 @@ pub(crate) fn get_cors_allow_headers<T: AsRef<str>, O, F: Fn(T) -> O>(

// Check if `AccessControlRequestHeaders` contains fields which were allowed
let (filtered, headers) = match cors_allow_headers {
AccessControlAllowHeaders::Any => {
AllowHeaders::Any => {
let headers = requested_headers.map(to_result).collect();
(false, headers)
}
AccessControlAllowHeaders::Only(only) => {
AllowHeaders::Only(only) => {
let mut filtered = false;
let headers: Vec<_> = requested_headers
.filter(|header| {
Expand Down Expand Up @@ -337,7 +337,7 @@ mod tests {
use std::iter;

use super::*;
use crate::access_control::hosts::Host;
use crate::server::access_control::host::Host;

#[test]
fn should_parse_origin() {
Expand Down Expand Up @@ -365,8 +365,8 @@ mod tests {
let host = Some(&*host);

// when
let res1 = get_cors_allow_origin(origin1, host, &Some(vec![]));
let res2 = get_cors_allow_origin(origin2, host, &Some(vec![]));
let res1 = get_cors_allow_origin(origin1, &Some(vec![]), host);
let res2 = get_cors_allow_origin(origin2, &Some(vec![]), host);

// then
assert_eq!(res1, AllowCors::Invalid);
Expand All @@ -383,7 +383,7 @@ mod tests {
let host = Some(&*host);

// when
let res = get_cors_allow_origin(origin, host, &None);
let res = get_cors_allow_origin(origin, &None, host);

// then
assert_eq!(res, AllowCors::NotRequired);
Expand All @@ -396,7 +396,7 @@ mod tests {
let host = None;

// when
let res = get_cors_allow_origin(origin, host, &None);
let res = get_cors_allow_origin(origin, &None, host);

// then
assert_eq!(res, AllowCors::NotRequired);
Expand All @@ -409,7 +409,7 @@ mod tests {
let host = None;

// when
let res = get_cors_allow_origin(origin, host, &None);
let res = get_cors_allow_origin(origin, &None, host);

// then
assert_eq!(res, AllowCors::Ok("parity.io".into()));
Expand All @@ -422,11 +422,7 @@ mod tests {
let host = None;

// when
let res = get_cors_allow_origin(
origin,
host,
&Some(vec![AccessControlAllowOrigin::Value("http://ethereum.org".into())]),
);
let res = get_cors_allow_origin(origin, &Some(vec![AllowOrigin::Origin("http://ethereum.org".into())]), host);

// then
assert_eq!(res, AllowCors::NotRequired);
Expand All @@ -439,7 +435,7 @@ mod tests {
let host = None;

// when
let res = get_cors_allow_origin(origin, host, &Some(Vec::new()));
let res = get_cors_allow_origin(origin, &Some(Vec::new()), host);

// then
assert_eq!(res, AllowCors::NotRequired);
Expand All @@ -452,11 +448,7 @@ mod tests {
let host = None;

// when
let res = get_cors_allow_origin(
origin,
host,
&Some(vec![AccessControlAllowOrigin::Value("http://ethereum.org".into())]),
);
let res = get_cors_allow_origin(origin, &Some(vec![AllowOrigin::Origin("http://ethereum.org".into())]), host);

// then
assert_eq!(res, AllowCors::Invalid);
Expand All @@ -469,10 +461,10 @@ mod tests {
let host = None;

// when
let res = get_cors_allow_origin(origin, host, &Some(vec![AccessControlAllowOrigin::Any]));
let res = get_cors_allow_origin(origin, &Some(vec![AllowOrigin::Any]), host);

// then
assert_eq!(res, AllowCors::Ok(AccessControlAllowOrigin::Value("http://parity.io".into())));
assert_eq!(res, AllowCors::Ok(AllowOrigin::Origin("http://parity.io".into())));
}

#[test]
Expand All @@ -482,7 +474,7 @@ mod tests {
let host = None;

// when
let res = get_cors_allow_origin(origin, host, &Some(vec![AccessControlAllowOrigin::Null]));
let res = get_cors_allow_origin(origin, &Some(vec![AllowOrigin::Null]), host);

// then
assert_eq!(res, AllowCors::NotRequired);
Expand All @@ -495,10 +487,10 @@ mod tests {
let host = None;

// when
let res = get_cors_allow_origin(origin, host, &Some(vec![AccessControlAllowOrigin::Null]));
let res = get_cors_allow_origin(origin, &Some(vec![AllowOrigin::Null]), host);

// then
assert_eq!(res, AllowCors::Ok(AccessControlAllowOrigin::Null));
assert_eq!(res, AllowCors::Ok(AllowOrigin::Null));
}

#[test]
Expand All @@ -510,15 +502,15 @@ mod tests {
// when
let res = get_cors_allow_origin(
origin,
host,
&Some(vec![
AccessControlAllowOrigin::Value("http://ethereum.org".into()),
AccessControlAllowOrigin::Value("http://parity.io".into()),
AllowOrigin::Origin("http://ethereum.org".into()),
AllowOrigin::Origin("http://parity.io".into()),
]),
host,
);

// then
assert_eq!(res, AllowCors::Ok(AccessControlAllowOrigin::Value("http://parity.io".into())));
assert_eq!(res, AllowCors::Ok(AllowOrigin::Origin("http://parity.io".into())));
}

#[test]
Expand All @@ -528,26 +520,24 @@ mod tests {
let origin2 = Some("http://parity.iot");
let origin3 = Some("chrome-extension://test");
let host = None;
let allowed = Some(vec![
AccessControlAllowOrigin::Value("http://*.io".into()),
AccessControlAllowOrigin::Value("chrome-extension://*".into()),
]);
let allowed =
Some(vec![AllowOrigin::Origin("http://*.io".into()), AllowOrigin::Origin("chrome-extension://*".into())]);

// when
let res1 = get_cors_allow_origin(origin1, host, &allowed);
let res2 = get_cors_allow_origin(origin2, host, &allowed);
let res3 = get_cors_allow_origin(origin3, host, &allowed);
let res1 = get_cors_allow_origin(origin1, &allowed, host);
let res2 = get_cors_allow_origin(origin2, &allowed, host);
let res3 = get_cors_allow_origin(origin3, &allowed, host);

// then
assert_eq!(res1, AllowCors::Ok(AccessControlAllowOrigin::Value("http://parity.io".into())));
assert_eq!(res1, AllowCors::Ok(AllowOrigin::Origin("http://parity.io".into())));
assert_eq!(res2, AllowCors::Invalid);
assert_eq!(res3, AllowCors::Ok(AccessControlAllowOrigin::Value("chrome-extension://test".into())));
assert_eq!(res3, AllowCors::Ok(AllowOrigin::Origin("chrome-extension://test".into())));
}

#[test]
fn should_return_invalid_if_header_not_allowed() {
// given
let cors_allow_headers = AccessControlAllowHeaders::Only(vec!["x-allowed".to_owned()]);
let cors_allow_headers = AllowHeaders::Only(vec!["x-allowed".to_owned()]);
let headers = vec!["Access-Control-Request-Headers"];
let requested = vec!["x-not-allowed"];

Expand All @@ -562,7 +552,7 @@ mod tests {
fn should_return_valid_if_header_allowed() {
// given
let allowed = vec!["x-allowed".to_owned()];
let cors_allow_headers = AccessControlAllowHeaders::Only(allowed);
let cors_allow_headers = AllowHeaders::Only(allowed);
let headers = vec!["Access-Control-Request-Headers"];
let requested = vec!["x-allowed"];

Expand All @@ -578,7 +568,7 @@ mod tests {
fn should_return_no_allowed_headers_if_none_in_request() {
// given
let allowed = vec!["x-allowed".to_owned()];
let cors_allow_headers = AccessControlAllowHeaders::Only(allowed);
let cors_allow_headers = AllowHeaders::Only(allowed);
let headers: Vec<String> = vec![];

// when
Expand All @@ -591,7 +581,7 @@ mod tests {
#[test]
fn should_return_not_required_if_any_header_allowed() {
// given
let cors_allow_headers = AccessControlAllowHeaders::Any;
let cors_allow_headers = AllowHeaders::Any;
let headers: Vec<String> = vec![];

// when
Expand Down
Loading