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

Improves error messages when fetch fails due to TLS errors. #3116

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dom96
Copy link
Collaborator

@dom96 dom96 commented Nov 14, 2024

When running wrangler dev (or running workerd locally directly), it is possible for fetch to fail with an "internal error". This can likely happen for many reasons, but in my case it was because I was using WARP and had misconfigured TLS certs. The error I was running into was this one: https://github.com/capnproto/capnproto/blob/6e071e34d88a8fc489638535899cd9d02e55bf76/c%2B%2B/src/kj/compat/tls.c%2B%2B#L221.

I noticed that we already have some custom logic to catch certain exceptions raised from kj and report them back to the user in a more friendly manner. What I'm curious about is why we don't simply report all errors. I think that matching on strings here is fragile, so I went for a bit of a compromise and changed the code to report any error that occurred in kj. But perhaps that will expose far more than we want.

Test Plan

  • Added a KJ_FAIL_REQUIRE to tls.c++ that always fails
  • Ran workerd via LLVM_SYMBOLIZER=llvm-symbolizer-14 bazel run --config=release @workerd//src/workerd/server:workerd -- serve $PWD/deps/workerd/samples/pyodide/config.capnp --experimental
  • Modified pyodide sample to remove python_external_bundle compat flag and to fetch inside the worker instead
  • Re-ran workerd

@dom96 dom96 requested review from jasnell and mar-cf November 14, 2024 15:44
@dom96 dom96 requested review from a team as code owners November 14, 2024 15:44
@dom96
Copy link
Collaborator Author

dom96 commented Nov 15, 2024

@kentonv since you've had thoughts when this was modified previously in #2111 (comment). I wonder if you have any concerns about this change. Could we even maybe go further and expose all exception messages to the user?

@kentonv
Copy link
Member

kentonv commented Nov 19, 2024

Yeah this is a problem. KJ errors should absolutely should not be indiscriminately exposed to applications. At best, they are unlikely to be formatted appropriately; they usually contain a semicolon-delimited list of debug context which would look very strange in a JavaScript error. At worst, they may contain information which is inappropriate to reveal, like internal IDs or something. I apparently failed to chase @jasnell on this last time but this code needs to change back to hide the error.

What we usually do instead is instrument the lower-level systems with some way to provide an exception callback which can format exceptions more appropriately. E.g. the HTTP API has HttpClientErrorHandler. Maybe TLS needs one of these as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants