Add CORS headers to error responses #546
Conversation
src/utils.rs
Outdated
| res.set_body(err.to_string()); | ||
| } | ||
| res | ||
| } |
There was a problem hiding this comment.
This is the implementation part I am not sure about. I have the feeling that having this as a utility method is not the best solution, so I am definitely open to other ideas of how to streamline the error responses here?
|
Not sure if it’s ideal that this middleware would “swallow” all errors. What if you have another middleware that logs errors or otherwise does stuff to errors? That’s why I suggested instead being able to add headers directly to the error itself. |
@davidpdrsn Thanks for bringing up that valid concern 👍! IMHO, you'd just have to add the middlewares in the order they should do stuff to errors. I think, headers shouldn't be the concern of a |
|
Hm alright. Maybe it would be nice to also update the docs to make it clear that it’s important to add this middleware last, otherwise your other middlewares won’t see errors. |
src/utils.rs
Outdated
| // errors default to 500 by default, so sending the error | ||
| // body is opt-in at the call site. | ||
| if !res.status().is_server_error() { | ||
| res.set_body(err.to_string()); |
There was a problem hiding this comment.
I don't think all apps would want the same error response here. I'm not sure what the right solution is, but perhaps we need a way of adding an Option<tide::Error> to a Response or something like that.
There was a problem hiding this comment.
I agree, however, I've only extracted the current error handling into that utility method, to make it re-usable. So this PR is not changing the current error handling. As far as I remember from Discord, the current approach for custom error handling would be a custom middleware that intercepts errors and converts them into a response (basically similar to this one #525 (comment)).
Related to all of the following discussions, it would be useful for http_types's errors to optionally carry data similar to a response, particulaly Headers and/or a Body. This allows for better handling of situations where things like middleware want to act only partialy on an error. Refs: http-rs/tide#546 Refs: http-rs/tide#532 Refs: http-rs/tide#452
Related to all of the following discussions, it would be useful for http_types's errors to optionally carry data similar to a response, particularly Headers and/or a Body. This allows for better handling of situations where things like middleware want to act only partially on an error. Refs: http-rs/tide#546 Refs: http-rs/tide#532 Refs: http-rs/tide#452
|
Btw I opened http-rs/http-types#169 which should hopefully make this kind of code nicer and more robust. |
This allows for robust creation of Response-s directly from Error-s, with error capture for future reference, and retreival via `error() -> Option<&Error>`. Refs: http-rs#169 Refs: http-rs/tide#546 Refs: http-rs/tide#532 Refs: http-rs/tide#452
This allows for robust creation of Response-s directly from Error-s, with error capture for future reference, and retrieval via `error() -> Option<&Error>`. Refs: http-rs#169 Refs: http-rs/tide#546 Refs: http-rs/tide#532 Refs: http-rs/tide#452
This allows for robust creation of Response-s directly from Error-s, with error capture for future reference, and retrieval via `error() -> Option<&Error>`. Refs: http-rs#169 Refs: http-rs/tide#546 Refs: http-rs/tide#532 Refs: http-rs/tide#452
This allows for robust creation of Response-s directly from Error-s, with error capture for future reference, and retrieval via `error() -> Option<&Error>`. Refs: http-rs#169 Refs: http-rs/tide#546 Refs: http-rs/tide#532 Refs: http-rs/tide#452
This allows for robust creation of Response-s directly from Error-s, with error capture for future reference, and retrieval via `error() -> Option<&Error>`. Refs: http-rs#169 Refs: http-rs/tide#546 Refs: http-rs/tide#532 Refs: http-rs/tide#452
This allows for robust creation of Response-s directly from Error-s, with error capture for future reference, and retrieval via `error() -> Option<&Error>`. PR-URL: http-rs#174 Refs: http-rs#169 Refs: http-rs/tide#546 Refs: http-rs/tide#532 Refs: http-rs/tide#452
This allows for robust creation of Response-s directly from Error-s, with error capture for future reference, and retrieval via `error() -> Option<&Error>`. PR-URL: http-rs#174 Refs: http-rs#169 Refs: http-rs/tide#546 Refs: http-rs/tide#532 Refs: http-rs/tide#452
This allows for robust creation of Response-s directly from Error-s, with error capture for future reference, and retrieval via `error() -> Option<&Error>`. PR-URL: http-rs#174 Refs: http-rs#169 Refs: http-rs/tide#546 Refs: http-rs/tide#532 Refs: http-rs/tide#452
|
It looks like this got fixed with recent changes like http-rs/http-types#174 |
Browsers are currently unable to read responses the originate from
tide::Errors when theCORSmiddleware is used. This PR makes sure to add CORS headers to error responses as well.I guess we want to implement parts of the PR differently (I'll highlight the line I am talking about via a code comment), but I still wanted to open a PR to start the discussion and maybe at least provide a unit test another implementation could reuse.
Refs #525