Cross-origin protection #375

maciejhirsz · 2021-06-10T20:12:11Z

No filters for Host, Substrate doesn't seem to care, it's more of interest to the likes of nginx reverse proxy for setting up virtual hosts. IIRC it was always confusing in parity-ethereum?

maciejhirsz · 2021-06-10T20:53:23Z

ws-server/src/server.rs

+	/// ```
+	///
+	/// By default allows any `Origin`.
+	pub fn set_allowed_origins<Origin, List>(mut self, list: List) -> Self


Maybe instead of accepting a list, we should have a method that will push onto an internal vec, and construct Arc<String> on build?

I'd like to avoid a situation where an empty list is set, which then rejects all connections.

I'm ok with the current function myself (I'd just add a note in the comment that setting an empty list will deny all origins). Perhaps if we added an allow_all_origins function to toggle to AllowAny it would become a little clearer by contrast? This would also allow one to programmatically undo the effect of calling set_allowed_origins, which doesn't feel super necessary, but for the sake of completeness it feels nice to be able to toggle between the full range of possible values.

(To elaborate a little; if we had an add_allowed_origins instead that pushed to the list, it would feel slightly odd to me that you could add an origin to go from allowing everything (nothing added yet) to only allowing what's been added. Whereas, "set_allowed_origins" feels clear to me and doesn't have that slightly odd transition in my mind)

My two cents: having a single set_ method is enough. We do not expect the list to change once the server is started, and even if it is technically possible to dynamically add/remove allowed origins, I think it's fine to be opinionated here.

Wrt to the semantics of setting the allowed origins to the empty set, I think we should pick one of these:

empty list => everyone can connect, i.e. switch to Cors::AllowAny

empty list => return error

I'd be more in favour of 2: I wouldn't be super keen on an empty list (perhaps by mistake) loosening restrictions and allowing all origins, so getting an error back feels more helpful.

jsdw · 2021-06-14T16:06:22Z

ws-server/src/server.rs

+		if let (Cors::AllowList(list), Some(origin)) = (self, origin) {
+			if !list.iter().any(|o| o.as_bytes() == origin) {
+				let error = format!("Origin denied: {}", String::from_utf8_lossy(origin));
+				log::warn!("{}", error);
+				return Err(Error::Request(error));
+			}
+		}


Do we want/need to handle "*" patterns as well in the allowed origins?

yeah, I think so

There are 3 possible states here:

There is no Origin header, which equates to the request being done either on the domain (no cross-origin shenanigans), or the request isn't coming from a browser at all (in which case the client can spoof the header to whatever it wants). We always allow these.

The header has a protocol://hostname value, if so we check it against the list (unless we allow all origins).

The header has a null value, which can be explicitly allowed by the list, but is generally advised against.

There is no * origin.

I think you explained this in a previous comment which makes sense to me now :)

My initial thinking was that CORS responses can contain "*" in allowed origins, so perhaps we need to handle something like this.

I did a little reading about WebSockets (since I didn't really know anything about the WebSocket upgrade protocol) and, indeed, there is no such thing as CORS really when talking about WS connections by the sounds of it, and so the origin checking here can essentially take whatever form we like.

Given that, I have no problem with not handling * in an allowed origin; it could potentially be a future enhancement (so that you can say configure this to allow eg any connections from "https://*.mydomain.com") but I guess there's no need for that sort of thing in the first cut!

ws-server/src/server.rs

maciejhirsz · 2021-06-15T11:05:54Z

Updated the comment on set_allowed_origins, and made it return an error if the list is empty. It might still be a good idea to include allow_all_origins() method on the builder that restores the default, I can imagine that during the setup on runtime someone might want to set their default allowed origins, and then override them to allow all with a CLI setting.

ws-server/src/server.rs

jsdw

Looking good to me; thanks for explaining the Origins stuff!

A couple of nits, but happy to approve :)

niklasad1 · 2021-06-15T16:17:29Z

No filters for Host, Substrate doesn't seem to care, it's more of interest to the likes of nginx reverse proxy for setting up virtual hosts. IIRC it was always confusing in parity-ethereum?

hmm, I don't really follow are you referring to this in substrate?

when I did this the request got denied for bad host in the host header how did that happen without host filtering?

maciejhirsz · 2021-06-15T16:33:15Z

No filters for Host, Substrate doesn't seem to care, it's more of interest to the likes of nginx reverse proxy for setting up virtual hosts. IIRC it was always confusing in parity-ethereum?

hmm, I don't really follow are you referring to this in substrate?

when I did this the request got denied for bad host in the host header how did that happen without host filtering?

Ha, curious. I didn't see it in the CLI when I checked it, but you might be right. I'll double check to make sure.

maciejhirsz · 2021-06-18T12:14:33Z

Will do a follow-up PR with Host filtering.

* master: (21 commits) New proc macro (#387) Streaming RpcParams parsing (#401) Set allowed Host header values (#399) Synchronization-less async connections in ws-server (#388) [ws server]: terminate already established connection(s) when the server is stopped (#396) feat: customizable JSON-RPC error codes via new enum variant on `CallErrror` (#394) [ci]: test each individual crate's manifest (#392) Add a way to stop servers (#386) [jsonrpsee types]: unify a couple of types + more tests (#389) Update roadmap link in readme (#390) Cross-origin protection (#375) Method aliases + RpcModule: Clone (#383) Use criterion's async bencher (#385) Async/subscription benches (#372) send text (#374) Fix link to ws server in README.md (#373) Concat -> simple push (#370) Add missing `rt` feature (#369) Release prep for v0.2 (#368) chore(scripts): publish script (#354) ...

maciejhirsz added 3 commits June 10, 2021 21:47

Initial implementation

576d9e0

Comments

c7173d1

Send a 403 on denied origin

1db6167

maciejhirsz requested review from jsdw and niklasad1 June 10, 2021 20:12

maciejhirsz changed the title ~~Cross-origin protection~~ [WIP] Cross-origin protection Jun 10, 2021

Noodling around with set_allowed_origins

aadd9b9

maciejhirsz commented Jun 10, 2021

View reviewed changes

jsdw reviewed Jun 14, 2021

View reviewed changes

ws-server/src/server.rs Outdated Show resolved Hide resolved

Error on empty list

a046a9a

maciejhirsz added 2 commits June 15, 2021 12:12

Soketto 0.6

81f7b83

fmt

7239586

maciejhirsz changed the title ~~[WIP] Cross-origin protection~~ Cross-origin protection Jun 15, 2021

Add Builder::allow_all_origins, clarify doc comments

bdeeab9

maciejhirsz mentioned this pull request Jun 15, 2021

Add CORS support #381

Closed

2 tasks

jsdw reviewed Jun 15, 2021

View reviewed changes

ws-server/src/server.rs Outdated Show resolved Hide resolved

jsdw reviewed Jun 15, 2021

View reviewed changes

ws-server/src/server.rs Outdated Show resolved Hide resolved

jsdw approved these changes Jun 15, 2021

View reviewed changes

maciejhirsz mentioned this pull request Jun 16, 2021

Update soketto requirement from 0.5 to 0.6 #384

Closed

Rename Cors -> AllowedOrigins, nits, no panic

b82edf7

niklasad1 approved these changes Jun 16, 2021

View reviewed changes

Merge branch 'master' of github.com:paritytech/jsonrpsee into mh-cors

ebf226f

maciejhirsz merged commit 26b0613 into master Jun 18, 2021

maciejhirsz deleted the mh-cors branch June 18, 2021 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross-origin protection #375

Cross-origin protection #375

maciejhirsz commented Jun 10, 2021

maciejhirsz Jun 10, 2021

jsdw Jun 14, 2021 •

edited

Loading

jsdw Jun 14, 2021

dvdplm Jun 15, 2021

jsdw Jun 15, 2021

jsdw Jun 14, 2021

niklasad1 Jun 14, 2021

dvdplm Jun 15, 2021

maciejhirsz Jun 15, 2021 •

edited

Loading

jsdw Jun 15, 2021 •

edited

Loading

maciejhirsz commented Jun 15, 2021

jsdw left a comment

niklasad1 commented Jun 15, 2021 •

edited

Loading

maciejhirsz commented Jun 15, 2021

maciejhirsz commented Jun 18, 2021

Cross-origin protection #375

Cross-origin protection #375

Conversation

maciejhirsz commented Jun 10, 2021

maciejhirsz Jun 10, 2021

Choose a reason for hiding this comment

jsdw Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

jsdw Jun 14, 2021

Choose a reason for hiding this comment

dvdplm Jun 15, 2021

Choose a reason for hiding this comment

jsdw Jun 15, 2021

Choose a reason for hiding this comment

jsdw Jun 14, 2021

Choose a reason for hiding this comment

niklasad1 Jun 14, 2021

Choose a reason for hiding this comment

dvdplm Jun 15, 2021

Choose a reason for hiding this comment

maciejhirsz Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

jsdw Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

maciejhirsz commented Jun 15, 2021

jsdw left a comment

Choose a reason for hiding this comment

niklasad1 commented Jun 15, 2021 • edited Loading

maciejhirsz commented Jun 15, 2021

maciejhirsz commented Jun 18, 2021

jsdw Jun 14, 2021 •

edited

Loading

maciejhirsz Jun 15, 2021 •

edited

Loading

jsdw Jun 15, 2021 •

edited

Loading

niklasad1 commented Jun 15, 2021 •

edited

Loading