actix-cors: Is a Error mapping based fix for CORS headers for middleware errors wanted?
- Ngôn ngữ chính
- Rust
- Star
- 889
- Fork
- 224
- Merge trung bình
- 9 giờ 37 phút
- Pull request đã merge (30 ngày)
- 20
Mô tả
Related/Fixes #19 but in a way not considered in the closed bug report.
If a fix is wanted and choices for the different variants below are done I can make a PR (fix a or b).
## Expected Behavior
All responses from a server should have appropriate CORS headers, no matter if error response or not.
## Current Behavior
If a middleware produces an error no CORS headers are set but the error is still converted to a response.
This obfuscates the actual error as a CORS problem and effectively hides the error message/body away from the client.
## Possible Solution
### Proposed
- wrap middleware error into a new error which also contains the needed header
- `impl ResponseError ` then forwards to the wrapped error but also adds the headers to any produced `HttpResponse`
- as `actix_web::error::Error` is just a `Box` this new error can be converted back into an `actix_web::error::Error`
- this also means the wrapping is only needed iff headers need to be added
- and that you could have a middleware `bool` flag to enable/disable this iff wanted
### Issues / Open Questions
- wrapping errors will affect anyone using `Error::as_error` (downcast)
- fix a: make it a crate-feature which needs to be enabled
- fix b: add a `bool` flag to the builder/middleware to dynamically enable it
- in both cases recommend using it in the documentation
- in both cases it would default to not being enabled until the next breaking change version
### Rejected Solutions
- convert error to response, then add headers
- not intended to be done by actix
- needs very hacky workaround to even be done, a `.clone()` on the request will compile but panic due to a `Rc::get_mut().unwrap()` (which IMHO, in combination with a transparent clone on request is a pretty bad/misguided mico optimization, but it's outside of the scope of this fix to change it)
- convince `actix_web` to have (partial?) build in CORS in the dispatch layer (reason: actix isn't just for web APIs and different scopes/might need different CORS setup)
- convince `actix_web` to add `set_header` or similar to `actix_web::Error` (reason: rather large changes, might need unsafe vtable magic to keep same performance)
- "just use `Ok(req.error_response(err))`" doesn't fix the issues. All responses, no matter what, need CORS headers (for CORS requests), this includes errors from middleware outside of our control and errors meant to be fatal middleware errors.
### Interactions
- affects how #472 could be implemented
## Steps to Reproduce (for bugs)
skipped this as the issue is well understood from previous reports
## Context
Missing CORS handler will obfuscate errors as CORS issues in browsers.
This can lead to all kind of friction like wrong bug reports, wrong error statistics (for client side statistics) and clients not realizing an error is on their side (e.g. idk. broken compression, malformed headers extracted in function-signature based extractors of from_fn middleware (which can't return a errornous response, only their value or a middleware error), etc.).
Also unexpected behavior (which missing CORS header are) and kinda non standard compliance are never desirable in general.
## Your Environment
- as known all supported environments are affected
- Actix Web Version at least since 2.0.0
- Actix Cors Version at least since 0.2.0
Hướng dẫn đóng góp
Đánh giá
Issue này chưa được đánh giá.