Handle tasks asynchronously #62
Conversation
Threated
force-pushed
the
feat/async-tasks
branch
from
6ba75f9 to
05fe232
Compare
Threated
force-pushed
the
feat/async-tasks
branch
from
05fe232 to
dc4592e
Compare
|
Running flawlessly on the central dnpm servers |
| let response_inner = match result.status { | ||
| WorkStatus::Succeeded => { | ||
| result.body | ||
| }, | ||
| e => { | ||
| warn!("Reply had unexpected workresult code: {e:?}"); | ||
| warn!("Reply had unexpected workresult code: {e:?}: {:#?}", result.body); |
There was a problem hiding this comment.
I'm a bit concerned about his. It only triggers at an error status code, right? So we don't run into the risk of logging sensitive data by printing the body?
There was a problem hiding this comment.
Yes although I just realized we won't even get here as a few lines earlier we try to deserialize into a Something<HttpResponse> which won't work and error early so to improve the error messages I will change that to deserialize into serde_json::Values first and if the status is success then into the HttpResponse. What do you think?
src/msg.rs
Outdated
| f.debug_struct("HttpResponse") | ||
| .field("status", &self.status) | ||
| .field("headers", &self.headers) | ||
| .field("body", &String::from_utf8_lossy(&self.body).as_ref()) |
There was a problem hiding this comment.
Again, please check if we debug print the respones somewhere, where sensitive data might be in the body
There was a problem hiding this comment.
Will double check good catch
There was a problem hiding this comment.
Would logging headers still be fine?
We could also have it include the body if its build in debug mode and skip it in release builds
There was a problem hiding this comment.
As we are logging the response, there should be no e.g. Authorization header present. So I think that should be fine?
Not logging the bodies in release builds is certainly the secure path forward.
Review commit by commit.
I will test this branch on the DNPM server before merging