Allow customising access control allow headers #305

cmichi · 2018-09-03T11:09:50Z

This should implement #114.

I also discovered that @tomusdrw made a comment at openethereum/parity-ethereum#6616 which seems to refer to the same issue. So I think that issue would be solved through this PR as well. I implemented this PR in the way suggested in that issue (with Only and Any() enums).

A thing that I am a bit unsure about is the set of header fields which are always allowed (get_always_allowed_headers() in the PR). This is something we might need to discuss. The behavior of this PR is that even when AccessControlAllowHeaders::Only is used there are still some fields which are always allowed (Host etc.).

tomusdrw

Looks solid, couple of minor issues.

tomusdrw · 2018-09-08T08:27:07Z

http/src/handler.rs

@@ -33,6 +36,7 @@ impl<M: Metadata, S: Middleware<M>> ServerHandler<M, S> {
 		jsonrpc_handler: Rpc<M, S>,
 		cors_domains: CorsDomains,
 		cors_max_age: Option<u32>,
+		allowed_headers: AccessControlAllowHeaders,


Please refer to the type in a consistent way (either cors::AccessControlAllowHeaders or directly). TBH I prefer the former.

tomusdrw · 2018-09-08T08:28:48Z

http/src/handler.rs

-			]));
+
+			if let AllowHeaders::Ok(cors_allow_headers) = cors_allow_headers {
+				if cors_allow_headers.len() > 0 {


!....is_empty()?

tomusdrw · 2018-09-08T08:33:46Z

server-utils/src/cors.rs

+			// if they can be used.
+
+			let echo = AllowHeaders::Ok(
+				header::AccessControlAllowHeaders(requested.deref().clone())


.deref() most likely unneessary

I dereferenced here to extract the vec, if I leave it away the compiler complains about:

expected struct `std::vec::Vec`, found struct `cors::hyper::header::AccessControlRequestHeaders`

(*requested).clone() should work and doesn't require Deref to be imported. I think it's also more idiomatic.

tomusdrw · 2018-09-08T08:35:59Z

server-utils/src/cors.rs

+				},
+
+				AccessControlAllowHeaders::Only(only) => {
+					let mut allowed_headers = only.clone();


Why do we need to clone it with every request? Maybe just do two checks instead of extending and doing one?

let are_all_allowed = requested.iter().all(|header| only.contains(header) || always_allowed.contains(header));

Ah yeah, you're right, the cloning is not necessary. I replaced it with your suggestion.

tomusdrw · 2018-09-08T08:38:03Z

server-utils/src/cors.rs

+}
+
+/// Returns headers which are always allowed.
+pub fn get_always_allowed_headers() -> Vec<Ascii<String>> {


Can we make it a static array? You should be able to use either Ascii<&str> or Ascii<Cow<str>>

Good point, I went for Vec<Ascii<&'static str>>.

cmichi · 2018-09-10T19:13:00Z

@tomusdrw Thanks for the feedback! I addressed your comments and amended the last commit. Can you please take a look again?

Do you have any thoughts on the default set of headers which should be returned by get_always_allowed_headers()?

tomusdrw

Few more grumbles

tomusdrw · 2018-09-17T07:38:31Z

http/examples/http_meta.rs

 use jsonrpc_http_server::jsonrpc_core::*;
 use self::hyper::header;
+use self::unicase::Ascii;


IMHO we should use re-exported value - actually user doesn't really care that it's Ascii internally, can't we do like this:

AccessControlAllowHeaders::Only(vec![ "Authorization", ])

and convert inside the builder?

Ah yeah, I agree, the API should only expose what is necessary.

I added a commit which introduces a struct AccessControlAllowHeadersUnicase which is a HashSet<Ascii<&'static str>> and is used internally now. I added an Into trait which converts the exposed AccessControlAllowHeaders to this internal type, so it's still possible to use a Vector for setting everything up.

tomusdrw · 2018-09-17T07:41:20Z

server-utils/src/cors.rs

+		let are_all_allowed = request_headers.iter()
+			.all(|header| {
+				only.contains(&Ascii::new(header.name().to_owned())) ||
+				get_always_allowed_headers().contains(&Ascii::new(header.name()))


Why do we alloc always_allowed_headers on every iteration? Maybe just create it once and cache?

tomusdrw · 2018-09-17T07:42:35Z

server-utils/src/cors.rs

+	if let AccessControlAllowHeaders::Only(only) = cors_allow_headers {
+		let are_all_allowed = request_headers.iter()
+			.all(|header| {
+				only.contains(&Ascii::new(header.name().to_owned())) ||


Would also be good to avoid copying the header here, Can't we compare Ascii<String> with Ascii<Cow> or Ascii<&'a str>?

I changed the type of only to HashSet<Ascii<&'static str>>, so it's no longer necessary to copy the header.

tomusdrw · 2018-09-17T07:43:39Z

server-utils/src/cors.rs

+			// if they can be used.
+
+			let echo = AllowHeaders::Ok(
+				header::AccessControlAllowHeaders(requested.deref().clone())


(*requested).clone() should work and doesn't require Deref to be imported. I think it's also more idiomatic.

tomusdrw · 2018-09-17T07:44:19Z

server-utils/src/cors.rs

+					let are_all_allowed = requested.iter()
+						.all(|header| {
+							let name = &Ascii::new(header.as_ref());
+							only.contains(header) || get_always_allowed_headers().contains(name)


Same as above - create once, not in every iteration.

tomusdrw · 2018-09-17T07:44:50Z

server-utils/src/cors.rs

 	}
 }

+/// Returns headers which are always allowed.
+pub fn get_always_allowed_headers() -> Vec<Ascii<&'static str>> {


I think it would be better to do a HashSet here instead of doing linear search every time. The list of headers looks good.

tomusdrw · 2018-09-20T12:27:15Z

@cmichi could you rebase on latest master to fix the build?

This commit adds the capability to customise the header fields which are allowed. A CORS preflight request uses the `Access-Control-Request-Headers` header to inquire about header fields which are allowed to be used. The response is either `403 Forbidden` or `200 OK` with the header `Access-Control-Allow-Headers` and the list of fields which were supplied in the `Access-Control-Request-Headers`. This list is not necessarily a complete list of all allowed fields. Rather certain fields are always allowed and don't need to be listed. Also `Access-Control-Request-Headers` might not contain header fields which would in fact be allowed to send; these fields are currently not exposed -- only fields which were in `Access-Control-Request-Headers` are containted in the responding `Access-Control-Allow-Headers`. The API can be used with two enums: `cors_allow_headers(AccessControlAllowHeaders::Only(Vec))` `cors_allow_headers(AccessControlAllowHeaders::Any)` The default is `Any`, as to not break the existing behavior. If `Only` is chosen there will still be some default headers which are always allowed (`Content-Type`, etc.).

cmichi · 2018-09-21T11:38:50Z

Thanks for the comments!

I have added a number of commits labelled (…) to the PR to make it easier reviewing them. Before merging I would squash them into the main Support Access-Control-Request-Headers in http-server commit though.

…ontainment)

tomusdrw · 2018-09-24T09:50:15Z

@cmichi Perfect, thanks!

* Fix typo * Refactor CorsHeader to AllowOrigin * Support Access-Control-Request-Headers in http-server This commit adds the capability to customise the header fields which are allowed. A CORS preflight request uses the `Access-Control-Request-Headers` header to inquire about header fields which are allowed to be used. The response is either `403 Forbidden` or `200 OK` with the header `Access-Control-Allow-Headers` and the list of fields which were supplied in the `Access-Control-Request-Headers`. This list is not necessarily a complete list of all allowed fields. Rather certain fields are always allowed and don't need to be listed. Also `Access-Control-Request-Headers` might not contain header fields which would in fact be allowed to send; these fields are currently not exposed -- only fields which were in `Access-Control-Request-Headers` are containted in the responding `Access-Control-Allow-Headers`. The API can be used with two enums: `cors_allow_headers(AccessControlAllowHeaders::Only(Vec))` `cors_allow_headers(AccessControlAllowHeaders::Any)` The default is `Any`, as to not break the existing behavior. If `Only` is chosen there will still be some default headers which are always allowed (`Content-Type`, etc.). * (cache always allowed headers and use HashSet for them) * (get rid of Deref import) * (do not expose Ascii) * (convert Vector to HashSet for allowed headers set by user) * (change to non-owned str to prevent copying the header when testing containment)

cmichi force-pushed the allow-customising-access-control-allow-headers branch 2 times, most recently from 73691e4 to c9907db Compare September 7, 2018 08:13

cmichi added 2 commits September 7, 2018 08:13

Fix typo

b4d9750

Refactor CorsHeader to AllowOrigin

cd54068

cmichi force-pushed the allow-customising-access-control-allow-headers branch from c9907db to bb00c35 Compare September 7, 2018 08:13

tomusdrw reviewed Sep 8, 2018

View reviewed changes

cmichi force-pushed the allow-customising-access-control-allow-headers branch from bb00c35 to 7e25946 Compare September 10, 2018 19:06

cmichi force-pushed the allow-customising-access-control-allow-headers branch from 7e25946 to 56bd7fd Compare September 10, 2018 20:32

tomusdrw reviewed Sep 17, 2018

View reviewed changes

cmichi force-pushed the allow-customising-access-control-allow-headers branch 2 times, most recently from 6bb4348 to 7210d57 Compare September 20, 2018 07:27

cmichi added 4 commits September 21, 2018 10:00

(cache always allowed headers and use HashSet for them)

20b8f2d

(get rid of Deref import)

9959073

(do not expose Ascii)

96ff8eb

cmichi force-pushed the allow-customising-access-control-allow-headers branch from 7210d57 to 0ead1d8 Compare September 21, 2018 11:38

cmichi added 2 commits September 21, 2018 11:52

(convert Vector to HashSet for allowed headers set by user)

035deb0

(change to non-owned str to prevent copying the header when testing c…

573c43c

…ontainment)

cmichi force-pushed the allow-customising-access-control-allow-headers branch from 0ead1d8 to 573c43c Compare September 21, 2018 11:53

tomusdrw approved these changes Sep 24, 2018

View reviewed changes

tomusdrw merged commit 53f814e into paritytech:master Sep 24, 2018

cmichi deleted the allow-customising-access-control-allow-headers branch September 24, 2018 15:30

tomusdrw mentioned this pull request Sep 26, 2018

Update hyper to version 0.12. #303

Merged

cmichi mentioned this pull request Nov 21, 2018

Allow customisation of Access-Control-Allow-Headers #114

Closed

cmichi mentioned this pull request Jan 9, 2019

Parity rejects MetaMask's x-metamask-origin header, and consequently all MetaMask/Firefox requests, during CORS preflight openethereum/parity-ethereum#6616

Closed

tomusdrw mentioned this pull request Jan 28, 2019

CORS headers whitelist #209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow customising access control allow headers #305

Allow customising access control allow headers #305

cmichi commented Sep 3, 2018

tomusdrw left a comment

tomusdrw Sep 8, 2018

tomusdrw Sep 8, 2018

tomusdrw Sep 8, 2018

cmichi Sep 10, 2018

tomusdrw Sep 17, 2018

tomusdrw Sep 8, 2018

cmichi Sep 10, 2018

tomusdrw Sep 8, 2018

cmichi Sep 10, 2018

cmichi commented Sep 10, 2018

tomusdrw left a comment

tomusdrw Sep 17, 2018

cmichi Sep 21, 2018

tomusdrw Sep 17, 2018

tomusdrw Sep 17, 2018

cmichi Sep 21, 2018 •

edited

Loading

tomusdrw Sep 17, 2018

tomusdrw Sep 17, 2018

tomusdrw Sep 17, 2018

tomusdrw commented Sep 20, 2018

cmichi commented Sep 21, 2018

tomusdrw commented Sep 24, 2018

Allow customising access control allow headers #305

Allow customising access control allow headers #305

Conversation

cmichi commented Sep 3, 2018

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmichi commented Sep 10, 2018

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmichi Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw commented Sep 20, 2018

cmichi commented Sep 21, 2018

tomusdrw commented Sep 24, 2018

cmichi Sep 21, 2018 •

edited

Loading