directory should not leak operational errors to client

[nothingmuch]

when failing to connect to redis, this is returned as a bad request error, when it should be a 500

see DanGould/pull/8 for failure mode

[nothingmuch]

in some cases i also saw a reset by peer instead of the error leaking, which is even worse as that borgs the error

[DanGould]

Is this closed by #488, #456, ?

[nothingmuch]

No, but those PR get us closer to fixing this.

The BadRequest variant of HandlerError fortunately does not send the internal anyhow::Error in the body, so this is not a serious/urgent problem:

rust-payjoin/payjoin-directory/src/lib.rs

Lines 302 to 305 in 7d8116e

	HandlerError::BadRequest(e) => {
	error!("Bad request: {}", e);
	*res.status_mut() = StatusCode::BAD_REQUEST
	}

The problem is only that some of these instances should use the InternalServerError variant instead. lib.rs has 7 instances of the expression HandlerError::BadRequest(e.into()). I think the least error prone way way to address this is to go just blanket replace them all with e.into(), i.e. just remove the explicit tagging, and provide HandlerErrror with bunch of From impls for the necessary types. Dispatching based on what error it is on a type basis, not where it's generated as it's currently done, with the ? operator and explicit tagging of BadRequest vs. InternalServerError not being really distinguishable from a type checking point of view, since their single parameter is of the same type, that seems very easy to get wrong.

I'm not sure which of these prompted me to open this issue, can't recall, but here's one example to pick on:

rust-payjoin/payjoin-directory/src/lib.rs

Lines 410 to 413 in 7d8116e

	match pool.push_default(id, req.into()).await {
	Ok(_) => Ok(none_response),
	Err(e) => Err(HandlerError::BadRequest(e.into())),
	}

afaict push_result can only result in operational errors, at this point the subdirectory id and the payload have already been validated. if the connection from the directory process to redis fails, for example, the client will get a bad request error even if it's request is prefectly valid.

There's already a From`` instance for hyper::http::Error. Another one for RedisResultthat is alwaysInternalServerError` seems obvious, not sure what else is missing but probably it's not much.

I think this issue is also a bit broader, for example:

rust-payjoin/payjoin-directory/src/lib.rs

Lines 240 to 241 in 7d8116e

	let full_body =
	body.collect().await.map_err(|e| HandlerError::InternalServerError(e.into()))?.to_bytes();

It's not clear to me that this is always an internal server error, we might want to add gateway related errors if the failure is somehow semantically different. The laborious part of this issue is going through all instantiations of HandlerError with an explicit variant, working out what errors could end up getting wrapped like that, by following the nested ? and error conversions, and then thinking about who's fault that would be, kinda tedious.

[nothingmuch]

this compiles, but i didn't really think through the From impls just, gotta draw the rest of the fucking owl as it were,. e -> HandlerError { e.into() } is probably also me being a rust n00b

-git a/payjoin-directory/src/lib.rs b/payjoin-directory/src/lib.rs
index 3f921b5..98dac2b 100644
--- a/payjoin-directory/src/lib.rs
+++ b/payjoin-directory/src/lib.rs
@@ -207,16 +207,13 @@ async fn handle_ohttp_gateway(
     ohttp: Arc<Mutex<ohttp::Server>>,
 ) -> Result<Response<BoxBody<Bytes, hyper::Error>>, HandlerError> {
     // decapsulate
-    let ohttp_body =
-        body.collect().await.map_err(|e| HandlerError::BadRequest(e.into()))?.to_bytes();
+    let ohttp_body = body.collect().await.map_err(|e| -> HandlerError { e.into() })?.to_bytes();
     let ohttp_locked = ohttp.lock().await;
-    let (bhttp_req, res_ctx) = ohttp_locked
-        .decapsulate(&ohttp_body)
-        .map_err(|e| HandlerError::OhttpKeyRejection(e.into()))?;
+    let (bhttp_req, res_ctx) =
+        ohttp_locked.decapsulate(&ohttp_body).map_err(|e| -> HandlerError { e.into() })?;
     drop(ohttp_locked);
     let mut cursor = std::io::Cursor::new(bhttp_req);
-    let req =
-        bhttp::Message::read_bhttp(&mut cursor).map_err(|e| HandlerError::BadRequest(e.into()))?;
+    let req = bhttp::Message::read_bhttp(&mut cursor).map_err(|e| -> HandlerError { e.into() })?;
     let uri = Uri::builder()
         .scheme(req.control().scheme().unwrap_or_default())
         .authority(req.control().authority().unwrap_or_default())
@@ -237,17 +234,14 @@ async fn handle_ohttp_gateway(
     for (name, value) in parts.headers.iter() {
         bhttp_res.put_header(name.as_str(), value.to_str().unwrap_or_default());
     }
-    let full_body =
-        body.collect().await.map_err(|e| HandlerError::InternalServerError(e.into()))?.to_bytes();
+    let full_body = body.collect().await.map_err(|e| -> HandlerError { e.into() })?.to_bytes();
     bhttp_res.write_content(&full_body);
     let mut bhttp_bytes = Vec::new();
     bhttp_res
         .write_bhttp(bhttp::Mode::KnownLength, &mut bhttp_bytes)
-        .map_err(|e| HandlerError::InternalServerError(e.into()))?;
+        .map_err(|e| -> HandlerError { e.into() })?;
     bhttp_bytes.resize(BHTTP_REQ_BYTES, 0);
-    let ohttp_res = res_ctx
-        .encapsulate(&bhttp_bytes)
-        .map_err(|e| HandlerError::InternalServerError(e.into()))?;
+    let ohttp_res = res_ctx.encapsulate(&bhttp_bytes).map_err(|e| -> HandlerError { e.into() })?;
     assert!(ohttp_res.len() == ENCAPSULATED_MESSAGE_BYTES, "Unexpected OHTTP response size");
     Ok(Response::new(full(ohttp_res)))
 }
@@ -309,10 +303,26 @@ impl HandlerError {
     }
 }
 
+impl From<bhttp::Error> for HandlerError {
+    fn from(e: bhttp::Error) -> Self { HandlerError::InternalServerError(e.into()) }
+}
+
+impl From<hyper::Error> for HandlerError {
+    fn from(e: hyper::Error) -> Self { HandlerError::InternalServerError(e.into()) }
+}
+
 impl From<hyper::http::Error> for HandlerError {
     fn from(e: hyper::http::Error) -> Self { HandlerError::InternalServerError(e.into()) }
 }
 
+impl From<db::Error> for HandlerError {
+    fn from(e: db::Error) -> Self { HandlerError::InternalServerError(e.into()) }
+}
+
+impl From<ohttp::Error> for HandlerError {
+    fn from(e: ohttp::Error) -> Self { HandlerError::OhttpKeyRejection(e.into()) }
+}
+
 fn handle_peek(
     result: db::Result<Vec<u8>>,
     timeout_response: Response<BoxBody<Bytes, hyper::Error>>,
@@ -354,9 +364,7 @@ async fn post_fallback_v1(
 
     let v2_compat_body = format!("{}\n{}", body_str, query);
     let id = check_id_length(id)?;
-    pool.push_default(id, v2_compat_body.into())
-        .await
-        .map_err(|e| HandlerError::BadRequest(e.into()))?;
+    pool.push_default(id, v2_compat_body.into()).await.map_err(|e| -> HandlerError { e.into() })?;
     handle_peek(pool.peek_v1(id).await, none_response)
 }
 
@@ -369,15 +377,14 @@ async fn put_payjoin_v1(
     let ok_response = Response::builder().status(StatusCode::OK).body(empty())?;
 
     let id = check_id_length(id)?;
-    let req =
-        body.collect().await.map_err(|e| HandlerError::InternalServerError(e.into()))?.to_bytes();
+    let req = body.collect().await.map_err(|e| -> HandlerError { e.into() })?.to_bytes();
     if req.len() > V1_MAX_BUFFER_SIZE {
         return Err(HandlerError::PayloadTooLarge);
     }
 
     match pool.push_v1(id, req.into()).await {
         Ok(_) => Ok(ok_response),
-        Err(e) => Err(HandlerError::BadRequest(e.into())),
+        Err(e) => Err(e.into()),
     }
 }
 
@@ -401,15 +408,14 @@ async fn post_subdir(
 
     let id = check_id_length(id)?;
 
-    let req =
-        body.collect().await.map_err(|e| HandlerError::InternalServerError(e.into()))?.to_bytes();
+    let req = body.collect().await.map_err(|e| -> HandlerError { e.into() })?.to_bytes();
     if req.len() > V1_MAX_BUFFER_SIZE {
         return Err(HandlerError::PayloadTooLarge);
     }
 
     match pool.push_default(id, req.into()).await {
         Ok(_) => Ok(none_response),
-        Err(e) => Err(HandlerError::BadRequest(e.into())),
+        Err(e) => Err(e.into()),
     }
 }
 
@@ -432,12 +438,8 @@ fn not_found() -> Response<BoxBody<Bytes, hyper::Error>> {
 async fn get_ohttp_keys(
     ohttp: &Arc<Mutex<ohttp::Server>>,
 ) -> Result<Response<BoxBody<Bytes, hyper::Error>>, HandlerError> {
-    let ohttp_keys = ohttp
-        .lock()
-        .await
-        .config()
-        .encode()
-        .map_err(|e| HandlerError::InternalServerError(e.into()))?;
+    let ohttp_keys =
+        ohttp.lock().await.config().encode().map_err(|e| -> HandlerError { e.into() })?;
     let mut res = Response::new(full(ohttp_keys));
     res.headers_mut().insert(CONTENT_TYPE, HeaderValue::from_static("application/ohttp-keys"));
     Ok(res)

[DanGould]

e -> HandlerError { e.into() } is probably also me being a rust n00b

You're looking for .map_err(HandlerError::from)?