bug: make CORS default less restrictive. #348
Conversation
jrconlin
requested review from
pjenvey and
a team
and removed request for
a team
| actix_web::http::Method::GET, | ||
| actix_web::http::Method::POST, | ||
| actix_web::http::Method::PUT, | ||
| ]) |
There was a problem hiding this comment.
Do you want to expose the Location response header with .expose_headers(), also?
There was a problem hiding this comment.
Not seeing a Location header, but yeah, there are a few X-Forward headers we should probably not disclose.
sigh
Really didn't want to list out all the headers either.
There was a problem hiding this comment.
Hmm... Looking at allow_any_headers, the call will echo the list of headers requested in Access-Control-Request-Headers into Access-Control-Allow-Headers, so it's not returning all the headers that are sent, only the ones that are requested to be sent, and even then, it's only returning the names, not the values.
There was a problem hiding this comment.
Ohh, gotcha—I thought you might be returning a Location header in 201s for POST requests, sorry! Thanks for double-checking, @jrconlin! 😊
There was a problem hiding this comment.
We do send a Location header (routers send them via RouterResponse::success) as well as TTL. The python also sent www-authenticate alongside errors but the rust doesn't.
It's not clear to me that these need exposing, but I guess the Test Page might want access to them in theory which would require exposing them (or expose_any_header), I think?
There was a problem hiding this comment.
The problem is that the browser is very strict about what sort of information gets disclosed to the DOM level Javascript. An unexpected or disallowed value will prevent the DOM javascript from getting the response. This results in the Test Page breaking.
So, in this case, the test page doesn't really care or want the response (aside from getting the status) but is blocked from even sending the inbound request due to the strict header limitations.
The default actix-cors is even more restrictive than it was before. We need to loosen it up again, so this time, be more explicit about what we're allowing.
(I wonder if we should just use
.permissive()since we handle most of the actual security internally?)Fixes: Sync-3608