Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't panic, handle errors. #57

Merged
merged 6 commits into from
Nov 11, 2022
Merged

Don't panic, handle errors. #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9fa83a4
Give each request their own handler
DanGould Nov 9, 2022
7b0a3ed
Return HttpError instead of unwrap()
DanGould Nov 9, 2022
380d760
Format LndErrors
DanGould Nov 9, 2022
e4da0bd
Stop LndClient from Panicking
DanGould Nov 9, 2022
5628a96
Stop scheduler panics
DanGould Nov 9, 2022
325acd5
Squander unwraps everywhere
DanGould Nov 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ In other words, your grandmother will be able to somewhat privately open a bunch
* To work with an *empty* LND wallet you need to use LND 0.14.2
* Funds in LND or other wallet are not used, so it's not true PayJoin.
* Invalid request can kill the whole server
* `.unwraps()`s EVERYWHERE!

## License

Expand Down
200 changes: 137 additions & 63 deletions src/http.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use std::net::SocketAddr;
use std::path::Path;

use bip78::receiver::*;
use hyper::service::{make_service_fn, service_fn};
use hyper::{Body, Method, Request, Response, Server, StatusCode};
use qrcode_generator::QrCodeEcc;

use crate::scheduler::{self, ScheduledPayJoin, Scheduler};
use crate::scheduler::{self, ScheduledPayJoin, Scheduler, SchedulerError};

#[cfg(not(feature = "test_paths"))]
const STATIC_DIR: &str = "/usr/share/nolooking/static";
Expand Down Expand Up @@ -45,77 +46,150 @@ async fn handle_web_req(
req: Request<Body>,
endpoint: url::Url,
) -> Result<Response<Body>, hyper::Error> {
use std::path::Path;
let result = match (req.method(), req.uri().path()) {
(&Method::GET, "/pj") => handle_index().await,
DanGould marked this conversation as resolved.
Show resolved Hide resolved
(&Method::GET, path) if path.starts_with("/pj/static/") => handle_static(path).await,
(&Method::POST, "/pj") => handle_pj(scheduler, req).await,
(&Method::POST, "/pj/schedule") => handle_pj_schedule(scheduler, endpoint, req).await,
_ => handle_404().await,
};

match result {
Ok(resp) => Ok(resp),
Err(err) => err.into_response(),
}
}

match (req.method(), req.uri().path()) {
(&Method::GET, "/pj") => {
let index =
std::fs::read(Path::new(STATIC_DIR).join("index.html")).expect("can't open index");
Ok(Response::new(Body::from(index)))
}
async fn handle_404() -> Result<Response<Body>, HttpError> {
let mut not_found = Response::default();
*not_found.status_mut() = StatusCode::NOT_FOUND;
Ok(not_found)
}

(&Method::GET, path) if path.starts_with("/pj/static/") => {
let directory_traversal_vulnerable_path = &path[("/pj/static/".len())..];
let file =
std::fs::read(Path::new(STATIC_DIR).join(directory_traversal_vulnerable_path))
.expect("can't open static file");
Ok(Response::builder()
.status(200)
.header("Cache-Control", "max-age=604800")
.body(Body::from(file))
.expect("valid response"))
}
async fn handle_index() -> Result<Response<Body>, HttpError> {
let index = std::fs::read(Path::new(STATIC_DIR).join("index.html")).expect("can't open index");
Ok(Response::new(Body::from(index)))
}

(&Method::POST, "/pj") => {
dbg!(req.uri().query());

let headers = Headers(req.headers().to_owned());
let query = {
let uri = req.uri();
if let Some(query) = uri.query() {
Some(&query.to_owned());
}
None
};
let body = req.into_body();
let bytes = hyper::body::to_bytes(body).await?;
dbg!(&bytes); // this is correct by my accounts
let reader = &*bytes;
let original_request = UncheckedProposal::from_request(reader, query, headers).unwrap();

let proposal_psbt = scheduler.propose_payjoin(original_request).await.unwrap();

Ok(Response::new(Body::from(proposal_psbt)))
}
async fn handle_static(path: &str) -> Result<Response<Body>, HttpError> {
let directory_traversal_vulnerable_path = &path[("/pj/static/".len())..];
match std::fs::read(Path::new(STATIC_DIR).join(directory_traversal_vulnerable_path)) {
Ok(file) => Response::builder()
.status(200)
.header("Cache-Control", "max-age=604800")
.body(Body::from(file))
.map_err(HttpError::Http),
Err(_) => handle_404().await,
}
}

(&Method::POST, "/pj/schedule") => {
let bytes = hyper::body::to_bytes(req.into_body()).await?;
// deserialize x-www-form-urlencoded data with non-strict encoded "channel[arrayindex]"
let conf = serde_qs::Config::new(2, false);
let request: ScheduledPayJoin =
conf.deserialize_bytes(&bytes).expect("invalid request");

let address = scheduler.schedule_payjoin(&request).await.unwrap();
let total_amount = request.total_amount();
let uri = scheduler::format_bip21(address.clone(), total_amount, endpoint);
let mut response = Response::new(Body::from(uri.clone()));
create_qr_code(&uri, &address.to_string());
response
.headers_mut()
.insert(hyper::header::CONTENT_TYPE, "text/plain".parse().unwrap());
Ok(response)
}
async fn handle_pj(scheduler: Scheduler, req: Request<Body>) -> Result<Response<Body>, HttpError> {
dbg!(req.uri().query());

// Return the 404 Not Found for other routes.
_ => {
let mut not_found = Response::default();
*not_found.status_mut() = StatusCode::NOT_FOUND;
Ok(not_found)
let headers = Headers(req.headers().to_owned());
let query = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a cleaner way of getting around not having Copy..

let uri = req.uri();
if let Some(query) = uri.query() {
Some(&query.to_owned());
}
}
None
};
let body = req.into_body();
let bytes = hyper::body::to_bytes(body).await?;
let reader = &*bytes;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually want to dbg! these bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has let us check the original PSBT in the past. Of little use here. We could introduce the log crate to show debug/warn/errors logging soon

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it for this pr

let original_request = UncheckedProposal::from_request(reader, query, headers)?;

let proposal_psbt = scheduler.propose_payjoin(original_request).await?;

Ok(Response::new(Body::from(proposal_psbt)))
}

async fn handle_pj_schedule(
scheduler: Scheduler,
endpoint: url::Url,
req: Request<Body>,
) -> Result<Response<Body>, HttpError> {
let bytes = hyper::body::to_bytes(req.into_body()).await?;
// deserialize x-www-form-urlencoded data with non-strict encoded "channel[arrayindex]"
let conf = serde_qs::Config::new(2, false);
let request: ScheduledPayJoin = conf.deserialize_bytes(&bytes)?;

let address = scheduler.schedule_payjoin(&request).await?;
let total_amount = request.total_amount();
let uri = scheduler::format_bip21(address.clone(), total_amount, endpoint);
let mut response = Response::new(Body::from(uri.clone()));
create_qr_code(&uri, &address.to_string());
response.headers_mut().insert(hyper::header::CONTENT_TYPE, "text/plain".parse()?);
Ok(response)
}

pub(crate) struct Headers(hyper::HeaderMap);
impl bip78::receiver::Headers for Headers {
fn get_header(&self, key: &str) -> Option<&str> { self.0.get(key)?.to_str().ok() }
}

#[derive(Debug)]
pub enum HttpError {
Bip78Request(bip78::receiver::RequestError),
Hyper(hyper::Error),
Http(hyper::http::Error),
InvalidHeaderValue(hyper::header::InvalidHeaderValue),
Scheduler(SchedulerError),
Serde(serde_qs::Error),
}

impl HttpError {
/// Transforms an [HttpError] into a HTTP response
pub fn into_response(self) -> Result<Response<Body>, hyper::Error> {
if let Self::Hyper(err) = self {
return Err(err);
}

let resp = Response::new(Body::from(self.to_string()));
let (mut parts, body) = resp.into_parts();

// TODO respond with well known errors as defined in BIP-0078
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this also give us more informative errors on most senders?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference to start would be UI would say

"not-enough-money | The receiver added some inputs but could not bump the fee of the payjoin proposal."

or

"original-psbt-rejected | The receiver rejected the original PSBT. "

otherwise the spec says to put errors into logs only. This becomes more useful when we're using our own sender because we can show can decide to propagate new well-known errors into our UIs.

https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#receivers-well-known-errors

// https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#receivers-well-known-errors
parts.status = match self {
Self::Bip78Request(_)
| Self::Hyper(_)
| Self::Http(_)
| Self::InvalidHeaderValue(_)
| Self::Scheduler(_)
| Self::Serde(_) => StatusCode::BAD_REQUEST,
};

// TODO: Avoid writing error directly to HTTP response (bad security if public facing)
// instead respond with the same format as well known errors
Ok(Response::from_parts(parts, body))
}
}

impl std::fmt::Display for HttpError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
// TODO: Have proper error printing
write!(f, "api: {:?}", self)
}
}

impl std::error::Error for HttpError {}

impl From<bip78::receiver::RequestError> for HttpError {
fn from(e: bip78::receiver::RequestError) -> Self { Self::Bip78Request(e) }
}

impl From<hyper::Error> for HttpError {
fn from(e: hyper::Error) -> Self { Self::Hyper(e) }
}

impl From<hyper::header::InvalidHeaderValue> for HttpError {
fn from(e: hyper::header::InvalidHeaderValue) -> Self { Self::InvalidHeaderValue(e) }
}

impl From<SchedulerError> for HttpError {
fn from(e: SchedulerError) -> Self { Self::Scheduler(e) }
}

impl From<serde_qs::Error> for HttpError {
fn from(e: serde_qs::Error) -> Self { Self::Serde(e) }
}
116 changes: 59 additions & 57 deletions src/lnd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,56 +13,6 @@ use tonic_lnd::rpc::{

use crate::scheduler::ChannelId;

#[derive(Debug)]
pub enum LndError {
Generic(tonic_lnd::Error),
ConnectError(tonic_lnd::ConnectError),
ParseBitcoinAddressFailed(bitcoin::util::address::Error),
VersionRequestFailed(tonic_lnd::Error),
ParseVersionFailed { version: String, error: std::num::ParseIntError },
LNDTooOld(String),
}

impl fmt::Display for LndError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
LndError::Generic(error) => error.fmt(f),
LndError::ConnectError(error) => error.fmt(f),
LndError::ParseBitcoinAddressFailed(err) => err.fmt(f),
LndError::VersionRequestFailed(_) => write!(f, "failed to get LND version"),
LndError::ParseVersionFailed { version, error: _ } => {
write!(f, "Unparsable LND version '{}'", version)
}
LndError::LNDTooOld(version) => write!(
f,
"LND version {} is too old - it would cause GUARANTEED LOSS of sats!",
version
),
}
}
}

impl std::error::Error for LndError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
match self {
LndError::Generic(error) => Some(error),
LndError::ConnectError(error) => Some(error),
LndError::ParseBitcoinAddressFailed(error) => Some(error),
LndError::VersionRequestFailed(error) => Some(error),
LndError::ParseVersionFailed { version: _, error } => Some(error),
LndError::LNDTooOld(_) => None,
}
}
}

impl From<tonic_lnd::Error> for LndError {
fn from(value: tonic_lnd::Error) -> Self { LndError::Generic(value) }
}

impl From<tonic_lnd::ConnectError> for LndError {
fn from(value: tonic_lnd::ConnectError) -> Self { LndError::ConnectError(value) }
}

#[derive(Clone)]
pub struct LndClient(Arc<AsyncMutex<tonic_lnd::Client>>);

Expand Down Expand Up @@ -144,8 +94,6 @@ impl LndClient {
}

/// Requests to open a channel with remote node, returning the psbt of the funding transaction.
///
/// TODO: This should not panic, have proper error handling.
pub async fn open_channel(
&self,
req: OpenChannelRequest,
Expand All @@ -160,9 +108,8 @@ impl LndClient {
use tonic_lnd::rpc::open_status_update::Update;
match update {
Update::PsbtFund(ready) => {
// TODO: Do not panic here
let psbt =
PartiallySignedTransaction::consensus_decode(&mut &*ready.psbt).unwrap();
let psbt = PartiallySignedTransaction::consensus_decode(&mut &*ready.psbt)
.map_err(LndError::Decode)?;
eprintln!(
"PSBT received from LND for pending chan id {:?}: {:#?}",
pending_chan_id, psbt
Expand All @@ -171,8 +118,7 @@ impl LndClient {

return Ok(Some(psbt));
}
// TODO: do not panic
x => panic!("Unexpected update {:?}", x),
x => return Err(LndError::UnexpectedUpdate(x)),
}
}
Ok(None)
Expand Down Expand Up @@ -212,3 +158,59 @@ impl LndClient {
Ok(())
}
}

#[derive(Debug)]
pub enum LndError {
Generic(tonic_lnd::Error),
ConnectError(tonic_lnd::ConnectError),
Decode(bitcoin::consensus::encode::Error),
ParseBitcoinAddressFailed(bitcoin::util::address::Error),
VersionRequestFailed(tonic_lnd::Error),
UnexpectedUpdate(tonic_lnd::rpc::open_status_update::Update),
ParseVersionFailed { version: String, error: std::num::ParseIntError },
LNDTooOld(String),
}

impl fmt::Display for LndError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
LndError::Generic(e) => e.fmt(f),
LndError::ConnectError(e) => e.fmt(f),
LndError::Decode(e) => e.fmt(f),
LndError::ParseBitcoinAddressFailed(e) => e.fmt(f),
LndError::VersionRequestFailed(_) => write!(f, "failed to get LND version"),
LndError::UnexpectedUpdate(e) => write!(f, "Unexpected channel update {:?}", e),
LndError::ParseVersionFailed { version, error: _ } => {
write!(f, "Unparsable LND version '{}'", version)
}
LndError::LNDTooOld(version) => write!(
f,
"LND version {} is too old - it would cause GUARANTEED LOSS of sats!",
version
),
}
}
}

impl std::error::Error for LndError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
match self {
LndError::Generic(e) => Some(e),
LndError::ConnectError(e) => Some(e),
LndError::Decode(e) => Some(e),
LndError::ParseBitcoinAddressFailed(e) => Some(e),
LndError::VersionRequestFailed(e) => Some(e),
Self::UnexpectedUpdate(_) => None,
LndError::ParseVersionFailed { version: _, error } => Some(error),
LndError::LNDTooOld(_) => None,
}
}
}

impl From<tonic_lnd::Error> for LndError {
fn from(value: tonic_lnd::Error) -> Self { LndError::Generic(value) }
}

impl From<tonic_lnd::ConnectError> for LndError {
fn from(value: tonic_lnd::ConnectError) -> Self { LndError::ConnectError(value) }
}
Loading