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.

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

[Feature Request] Better error messages when tunneling #2252

Open
maxmcd opened this issue Apr 19, 2024 · 3 comments · May be fixed by #2372
Open

[Feature Request] Better error messages when tunneling #2252

maxmcd opened this issue Apr 19, 2024 · 3 comments · May be fixed by #2372
Labels
C-refactor Category: refactor. This would improve the clarity of internal code. E-easy Effort: Easy! Start here :D E-pr-welcome The feature is welcome to be added, instruction should be found in the issue.

Comments

@maxmcd
Copy link

maxmcd commented Apr 19, 2024

When our end users request through a HTTP(S)_PROXY and there is an error connecting the error message simply returns "unsuccessful tunnel". It would be great to have the option of passing through a more detailed error message. ie: reporting that there was an error dialing the host, or explaining that the request was blocked.

Any interest in this being extended? I'd be happy to work on a PR if there is interest. Would be great to hear thoughts on how exactly to surface this. Maybe by supporting more http status, or even passing through the body if it fits in the header buf.

@seanmonstar
Copy link
Owner

Better error messages are always appreciated ❤️

@maxmcd
Copy link
Author

maxmcd commented Apr 19, 2024

Nice! It seems like I could extend to support more status messages pretty easily (rough code):

else if recvd.starts_with(b"HTTP/1.1 407") {
   return Err("proxy authentication required".into());
} else if recvd.starts_with(b"HTTP/1.1 502") {
   return Err("unsuccessful tunnel: Bad Gateway".into());
// and so on...

Additionally, if there is any body included in what we have read, we could add that to the error as well (rough code as well, would likely combine the two to support both stating the status and including the body):

let trailer = b"\r\n\r\n";
if let Some(index) = recvd.windows(trailer.len()).position(|w| w == trailer) {
    // TODO: ensure we have read body bytes beyond this trailer
    let message = String::from_utf8_lossy(&recvd[index + trailer.len()..]);
    return Err(format!("unsuccessful tunnel: {}", message).into());
}

Thoughts on these approaches? Option one seems v straightforward. Option 2 seeks a little tricky and I am unsure of the experience of getting a partial body in an error message being a good thing.

Maybe worth noting that we want users to know when the proxy is a DNS error or an error dialing, etc, so surfacing the body on some level has appeal.

@seanmonstar
Copy link
Owner

The first one is a definite improvement. Happy to merge that.

Like you said about the second, knowing whether the body is useful is tricky. I might hold off there.

Thanks for taking the initiative!

@seanmonstar seanmonstar added E-easy Effort: Easy! Start here :D E-pr-welcome The feature is welcome to be added, instruction should be found in the issue. C-refactor Category: refactor. This would improve the clarity of internal code. labels Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-refactor Category: refactor. This would improve the clarity of internal code. E-easy Effort: Easy! Start here :D E-pr-welcome The feature is welcome to be added, instruction should be found in the issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants