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.

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

Remove CORS logic #851

Merged
merged 49 commits into from
Aug 16, 2022
Merged

Remove CORS logic #851

merged 49 commits into from
Aug 16, 2022

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Aug 11, 2022

This PR removes the CORS logic from the HTTP server.

JSONRPSee exposes the ability to add custom tower service builders,
which are capable of handling many use-cases, including CORS layers.

The CORS example is adjusted to work with both the internal access control
and the upstream CORS layers from the tower_http crate.

Internal code is modified to support filtering for HOST and ORIGIN via
the access control, to keep parity with substrate.

Differences

JSONRPSee previously filtered the HOST, ORIGIN, and all the headers of a request.
This behavior causes the RPC server to respond with an error, even for OPTIONS
requests, if the request's headers are not whitelisted.

The header filtering is removed from the codebase, relying upon browsers to submit
a preflight request (OPTIONS).

Users that need to implement CORS can rely upon upstream code to achieve the same
behavior.

The RPC cors example was modified:

  • for internal CORS logic (removed)
let acl = AccessControlBuilder::new()
		.set_allowed_headers(vec!["Content-Type"])?
		.allow_all_origins()
		.allow_all_hosts()
		.build();
  • for upstream CORS logic
let acl = AccessControlBuilder::new().allow_all_hosts().build();
	let cors = CorsLayer::new()
		// Allow `POST` when accessing the resource
		.allow_methods([Method::POST])
		// Allow requests from any origin
		.allow_origin(Any)
		.allow_headers([hyper::header::CONTENT_TYPE]);
	let middleware = tower::ServiceBuilder::new().layer(cors);

Requests were submitted via CURL, with / without a custom Dummy_header:

curl -ik  127.0.0.1:49308  -X OPTIONS -H "Content-Type: application/json" -H "Dummy_header: tmp" -H "Origin: http://not.com" -d '{"jsonrpc": "2.0", "method":  "say_hello", "id": 1}'

curl -ik  127.0.0.1:49308  -X POST  -H "Content-Type: application/json" -H "Dummy_header: tmp" -H "Origin: http://not.com" -d '{"jsonrpc": "2.0", "method":  "say_hello", "id": 1}'
Method Old Behavior New Behavior
POST HTTP/1.1 403 Forbidden; content-type: text/plain; content-length: 120 ;date: Wed, 10 Aug 2022 12:49:03 GMT; Requested headers are not allowed for CORS. CORS headers would not be sent and any side-effects were cancelled as well. HTTP/1.1 200 OK; Content-type: application/json; charset=utf-8; access-control-allow-origin: *; vary: origin; vary: access-control-request-method; vary: access-control-request-headers; content-length: 49; date: Wed, 10 Aug 2022 12:40:31 GMT
OPTIONS HTTP/1.1 403 Forbidden; content-type: text/plain; content-length: 120; date: Wed, 10 Aug 2022 12:49:25 GMT; Requested headers are not allowed for CORS. CORS headers would not be sent and any side-effects were cancelled as well. HTTP/1.1 200 OK; access-control-allow-origin: *; vary: origin; vary: access-control-request-method; vary: access-control-request-headers; access-control-allow-methods: POST; access-control-allow-headers: content-type; content-length: 0; date: Wed, 10 Aug 2022 12:38:42 GMT

Closes #795, closes #842.

lexnv and others added 30 commits July 25, 2022 17:26
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
…r_v4

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@niklasad1
Copy link
Member

niklasad1 commented Aug 12, 2022

For simplicity and to follow along this PR I suggest that you keep:

let acl = AccessControlBuilder::new()
		.allow_all_origins()
		.allow_all_hosts()
		.build();

We still have origin and host filtering left in the HTTP server AFAIU so I think we should keep it to have some sort of uniform API between the ws and http server but as you showed in the example users the CORS stuff based on Access-Control-Allow-Origin which is quite confusing I misunderstood that when reading the PR.

Thus, it only applies for "CORS requests i.e. HTTP Option requests` only

@jsdw
Copy link
Collaborator

jsdw commented Aug 12, 2022

I'd recommend running a search for "cors" to see what's left; there appear to be some other bits and pieces around and I'm wondering whether we can remove those too?

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
…move_cors_v2

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Base automatically changed from poc_middleware_layer_v4 to master August 12, 2022 15:02
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
// RPC access control that allows all hosts and all origins.
// Note: the access control does not modify the response headers,
// it only acts as a filter.
// If you need the ORIGIN header to be mirrored back in the response,
Copy link
Member

Choose a reason for hiding this comment

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

this is a bit awkward i.e, if I enable origin filtering but forget to enable it in the CORS middleware the correct value is not sent back if one performs a preflight request....

but nothing really we can do about it... as we don't control the CORS middleware

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's OK isn't it? I mean, if we assume that enabling origin filtering in jsonrpsee has nothing to do with CORS any more and will just block requests from origins people don't want accessing it.

(which I know sortof overlaps the CORS functionality, but I guess we're only keeping it here because we want to maintain consistency with WS connections and they don't have any CORS stuff)

Copy link
Member

@niklasad1 niklasad1 Aug 15, 2022

Choose a reason for hiding this comment

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

yupp, you are correct those are kind of "redundant" now missed that.

niklasad1 and others added 2 commits August 15, 2022 14:48
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
lexnv and others added 5 commits August 15, 2022 16:04
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

nice looks clean 👍

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Looks great! Much clearer IMO now that we offer basic filtering and there's a separate CORS thing if you want proper CORS :)

@lexnv lexnv merged commit 1ebaf62 into master Aug 16, 2022
@lexnv lexnv deleted the remove_cors_v2 branch August 16, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove CORS support from Jsonrpsee and rely on tower middleware instead refactor HTTP header verification
3 participants