actix-cors: Is a Error mapping based fix for CORS headers for middleware errors wanted?

[rustonaut]

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<dyn ResponseError> 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 Is it possible to respond Cors errors with a custom error type? #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

[JohnTitor]

I've investigated some other web frameworks' middlewares and I'm tend to that we should add CORS headers. For example, Starlette has added it even on errors (with the correct order of middlewares): https://starlette.dev/middleware/#corsmiddleware-global-enforcement

@robjtede Any thoughts on this?

[robjtede]

My frustrations with how middleware errors work are long-standing and CORS seems to be a common entrypoint to this problem.

Unfortunately, the guidance of "never return errors in middleware, unless absolutely necessary" has been able to keep us from needing a breaking release, because I now believe we need one in order to properly address this.

I had a go with PR (good lord... 3 years ago) actix/actix-web#2979 which is more of an escape hatch than good design.

In reality we would need to change the interface of middleware and their associated traits (perhaps adding new ones) to include more, and more-obvious, control flow semantics.

<musings>

The std ControlFlow enum is a source of inspiration in this way, in that it discourages us thinking of middleware errors as self-contained. A Break variant would be a clearer signal of what the library intends to do with the result. Though, I think Continue and Break as variants for middleware are too limiting, we'd want something like:

enum MiddlewareControlFlow {
  /// Forward request to next service.
  Continue(Request)

  /// Early return but propagate this response back down the service stack.
  Return(Response, Option<Box<Error>) // remembering response contains the request

  /// Skip all outer middleware layers and return this response directly.
  Fatal(Response, Box<Error>)
}

(On the boxed errors, I'm glad we require there to be some distinct type of error down that chain. Axum seems to get the opposite side of the complaints re. middleware in that such context is lost when the interface is effecively -> Result<Response, Response>. Howeverm we don't have a perfect setup either.)

</musings>

In summary, yes we should endeavor to add CORS headers to as many requests as possible, since it doesn't care about the specifics of error responses.